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