Subject: Re: poolifying fileassoc
To: None <blymn@baesystems.com.au>
From: YAMAMOTO Takashi <yamt@mwd.biglobe.ne.jp>
List: tech-kern
Date: 04/08/2007 19:23:53
> }
>
> /*
> - * uvm_aio_aiodone: do iodone processing for async i/os.
> - * this should be called in thread context, not interrupt context.
> + *
> + * uvm_io_done: Set page flag status when an i/o (synch or asynch) has
> + * completed or an async i/o is started. This function handles the
> + * following situations:
> + *
> + * - Synchronous i/o has completed
> + * - Asynchronous i/o has been initiated
> + * - Asynchronous i/o had completed
> + * - i/o is associated with swap
the last one seems different from others.
> + *
> + * All these cases what needs to be done to the page structures is very
> + * similar with some small variations depending on the calling situation.
> + *
> + * Function arguments are:
> + *
> + * pgs array of pages to handle
> + * npages number of pages in request
> + * flags type of completion, see uvm_pager.h
> + * overwrite TRUE if overwriting pages
> + * ridx The io may have been rounded by prepending pages to
> + * the pgs array, this gives the index into the pgs array
> + * of the first page prior to rounding (set to 0 if no
> + * rounding done)
> + * orignpages original transaction size before rounding. If no
> + * rounding was done then set to npages.
these description seems inappropriate to me.
i'd suggest to rename ridx and orignpages to more meaningful names
like syncidx and syncnpages.
> +
> + if (pg == NULL) /* no pages to do... */
> + return error;
is this necessary?
> if (swap) {
> - if (pg->uobject != NULL) {
> + if (pg->uobject != NULL)
> slock = &pg->uobject->vmobjlock;
> - } else {
> + else
> slock = &pg->uanon->an_lock;
> - }
> simple_lock(slock);
> uvm_lock_pageq();
> }
please make this kind of style changes a separate commit.
> @@ -369,7 +404,9 @@
> if (error) {
> int slot;
> if (!write) {
> - pg->flags |= PG_RELEASED;
> + if ((flags == UVMPAGER_ASYNCIO_DONE) ||
> + (pg->flags & PG_FAKE))
> + pg->flags |= PG_RELEASED;
why you need to check UVMPAGER_ASYNCIO_DONE here?
> +/* Flags to uvm_io_done */
> +#define UVMPAGER_SYNCIO_DONE 0x01 /* Synch i/o has completed */
> +#define UVMPAGER_ASYNCIO_START 0x02 /* Asynch i/o started */
> +#define UVMPAGER_ASYNCIO_DONE 0x03 /* Asynch i/o has completed */
> +
can you explain "Asynch i/o started"?
to me, it seems to be used only when async getpages found all pages cached.
ie. no i/o is started.
i feel these flags should just go away.
btw, async and sync seem more popular than asynch and synch in nearby code.
> -startover:
> + startover:
?
> + if (mbp == NULL) {
> + /*
> + * fake up a io buf to use with uvm_io_done iff the pages
> + * were already resident, just fill in the bits used.
> + */
> + mbp = getiobuf();
> + mbp->b_flags = B_READ| (async ? B_ASYNC : 0);
> + mbp->b_error = 0;
> + if (error) {
> + mbp->b_flags |= B_ERROR;
> + mbp->b_error = error;
> }
> + mbp->b_vp = vp;
i'm unhappy with this allocation.
IMO it's cleaner to pass error and read/write as separate arguments.
vp is available from pg->uobject.
i'd like to suggest the following prototype.
int
uvm_iodone(struct vm_page **pgs, int npages,
int syncidx, int syncnpages, int flags, int error);
flags: UVM_IODONE_READ, UVM_IODONE_WRITE, UVM_IODONE_OVERWRITE.
YAMAMOTO Takashi