Re: Ognyan's libpager changes

[Ognyan Kulev]

Neal H. Walfield wrote:
> As a start to getting back into Hurd hacking mode, I have begun to
> review your ext2fs patch for large file system support.  At this
> point, I have only reviewed the libpager changes in any detail.  I
> want to work with you to develop a well thought out interface and
> implementation that not only does the job but is elegant and
> represents something with which I feel comfortable in advocating
> acceptance into the mainline.

Great!  Please use version 20040421 from 
http://debian.fmi.uni-sofia.bg/~ogi/hurd/ext3fs/ .  It's not uploaded in 
Savannah and contains some major changes in the core function that 
remaps pages.

> As such, I advise that we remove this extra flexibility and
> have pager_notify_pageout (which I prefer to call pager_notify_evict
> as data can be paged out without being evicted in the case of flushing
> dirty pages) called on a _per pager_ basis and indicate whether or not
> it should be called in the struct pager and set by pager_create.  If
> it is useful, we may also have a pair of get and set methods to toggle
> it, however, the implementation of turning this feature on after a
> pager has been executing for some time seems problematic: all pages
> need to be marked as precious; I can't think of an easy way to do this
> off hand.

Yes, in ext2fs, we need either all or none pages to be marked as 
precious.  I can't predict if there will be future pager users that will 
want more fine-grained marking, so I've just taken this by-page 
approach.  In conclusion, I think it's OK this option to be pager-wide.

> Second,  Ognyan suggests that there is a race.

I wouldn't find it if I didn't experience it :-)  BTW In complete hurd 
build, this optimization turns 1-3 times, usually 1 time.

> My sense is that this problem can be easily avoided by not sending a
> pager_notify_pageout in data-return.c if PM_PAGEINWAIT is set thereby
> eliminating all of the hubbub with PM_FORCEREAD and the inefficiencies
> which is introduces.  Or perhaps, I am missing some subtility.

This sounds reasonable.  I need to think a bit more about it though.

> Finally, when a pager_notify_evict is called on a page, the page is
> potentially changed.  Hence any state associated with the page must
> also be changed.  That is, its pagemap entry needs to be cleared
> otherwise, a page which is marked PM_EIO is shows for the wrong page.

Right, I didn't catch this.  The error will be lost, but libpager can do 
nothing about it.

So what name should our pager-wide flag has?  Following the current 
patch and the pager_notify_pageout -> pager_notify_evict rename, I will 
use notify_on_evict if noone has better proposal.

I'm close to releasing ext3fs 0.1, and after that I'll make these 
changes and future ones that will eventually come up.

Regards,
ogi