Re: [patch] mm: close page_mkwrite races

[Nick Piggin <npiggin@suse.de>]

On Wed, Apr 01, 2009 at 04:02:41PM -0700, Andrew Morton wrote:
> On Tue, 31 Mar 2009 12:55:16 -0700 (PDT)
> Sage Weil <sage@newdream.net> wrote:
> 
> > On Mon, 30 Mar 2009, Nick Piggin wrote:
> > > [Fixed linux-mm address. Please reply here]
> > > 
> > > Hi,
> > > 
> > > I'd like opinions on this patch (applies on top of the previous
> > > page_mkwrite fixes in -mm). I was not going to ask to merge it
> > > immediately however it appears that fsblock is not the only one who
> > > needs it...
> > > --
> > > 
> > > I want to have the page be protected by page lock between page_mkwrite
> > > notification to the filesystem, and the actual setting of the page
> > > dirty. Do this by allowing the filesystem to return a locked page from
> > > page_mkwrite, and have the page fault code keep it held until after it
> > > calls set_page_dirty.
> > > 
> > > I need this in fsblock because I am working to ensure filesystem metadata
> > > can be correctly allocated and refcounted. This means that page cleaning
> > > should not require memory allocation.
> 
> wot?  "page cleaning" involves writeout.  How can we avoid doing
> allocations there?

Oh, guaranteed allocations like from mempools are fine of course.
But yeah the changelog is slightly more fsblock oriented than it
probably needs to be... I didn't really edit it after taking out
of my series.


> > > Without this patch, then for example we could get a concurrent writeout
> > > after the page_mkwrite (which allocates page metadata required to clean
> > > it), but before the set_page_dirty. The writeout will clean the page and
> > > notice that the metadata is now unused and may be deallocated (because
> > > it appears clean as set_page_dirty hasn't been called yet). So at this
> > > point the page may be dirtied via the pte without enough metadata to be
> > > able to write it back.
> > > 
> > > Sage needs this race closed for ceph, and Trond maybe for NFS.
> > 
> > I ran a few tests and this fixes the problem for me (although fyi the 
> > patch didn't apply cleanly on top of your previously posted page_mkwrite 
> > prototype change patch, due to some differences in block_page_mkwrite).
> 
> What is "the problem"?  Can we get "the problem"'s description included
> in the changelog?
> 
> The patch is fairly ugly, somewhat costly and makes things (even) more
> complex.    Sigh.

I guess it is another step toward being able to pull page_mkwrite into
->fault which will avoid one lock/unlock cycle and lots of stuff in
the main fault path.
 
Also fixes bug of course.