Re: [PATCH] pagecache lock ordering

[Andrew Morton]

Hugh Dickins wrote:
> 
> On Wed, 9 Jan 2002, Andrew Morton wrote:
> > Hugh Dickins wrote:
> > >
> > > There's two places, do_buffer_fdatasync
> >
> > generic_buffer_fdatasync() and hence do_buffer_fdatasync()
> > are completely unused.  It may be simpler to just trash
> > them.
> 
> Oh, nice observation.  writeout_one_page could be trashed at
> the same time (but not waitfor_one_page, still in use elsewhere).

I'm struggling to see a use for generic_buffer_fdatasync().  Maybe
for a filesystem which doesn't implement ->writepage()?  Dunno.

The only places where waitfor_one_page() is used are in the
directory-in-pagecache filesystems, for synchronous directories.
And the usage there seems completely incorrect.

They call ->commit_write(), which leaves the buffers dirty
and unlocked, then they call waitfor_one_page(), which does nothing
at all because the buffers are unlocked.  I/O was never started,
we didn't sync the blocks.

I feel I'm missing somethng here, because I checked that code
a few months back.  hmmm..

> But what's the chance that an out-of-tree filesystem might
> be using generic_buffer_fdatasync, which is still prototyped
> and exported?  Remove from 2.5 but leave in 2.4?  I'd like to
> see them go completely - revenge for being misled by them -
> but Marcelo may decide otherwise.

This code is untested, and there is no way for us to test it.
And now we think it may be deadlocky.  It has to go.

But the withdrawal of an exported API from 2.4.x is a policy
decision for Marcelo to make.

> ...
> 
> > I get the feeling that a lot of this would be cleaned up
> > if presence on an LRU contributed to page->count.  It
> > seems strange, kludgy and probably racy that this is not
> > the case.
> 
> That makes a lot of sense: but I feel much safer in agreeing
> with you than in making the corresponding changes!
> 

Heh.  I'll do the death-to-generic_buffer_fdatasync patch, and
I'll check whether we need to convert the dir-in-pagecache
filesytems over to writepage/wait_on_page.

-