On Sun, Apr 08, 2007 at 07:23:53PM +0900, YAMAMOTO Takashi wrote:
> 
> the last one seems different from others.
> 

It is sort of a special case of asynch i/o completed, I can just
remove it from the comment.

> 
> these description seems inappropriate to me.
> i'd suggest to rename ridx and orignpages to more meaningful names
> like syncidx and syncnpages.
>

No problems - ridx and orignpages came from the original variable
names in genfs_getpages()
 
> 
> is this necessary?
> 

Probably not - it was a sanity check.  I will remove it and probably
the preceding loop.  Since I should just use pgs[sycnidx]

> 
> please make this kind of style changes a separate commit.
> 

Oops - that is my automatic whitespace fixer.  Sorry.

> 
> why you need to check UVMPAGER_ASYNCIO_DONE here?
> 

Because in the original code uvm_aio_iodone() would unconditionally
release the page but in the genfs_getpages() the pages are only
released if they have PG_FAKE set.

> > +/* 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.
> 

No, from what I can see, if the flags to genfs_getpages() does not
have PGO_SYNCIO set then genfs_getpages() will start an asynch i/o
with a callback of uvm_biodone() when the i/o is completed.
uvm_biodone() then schedules puts an entry on a work queue for
uvm_aio_aiodone() to be called.  So, from what I can see we need to
know if PGO_SYNCIO was set to ensure we do not do operations on pages
that are not brought in yet and also need to know when we are being
called to complete an asynch operation so we can unbusy the pages and
other actions that could not be done before the pages are present.

> 
> btw, async and sync seem more popular than asynch and synch in nearby code.
> 

will fix :)

> > -startover:
> > + startover:
> 
> ?

emacs auto-indent :(

> 
> i'd like to suggest the following prototype.
>

Looks fine to me, I will sort this out.

Thanks.
 
-- 
Brett Lymn