Re: [patch 4/6] mm: merge populate and nopage into fault (fixes nonlinear)

[Nick Piggin]

On Tue, Mar 06, 2007 at 10:51:01PM -0800, Andrew Morton wrote:
> On Wed, 21 Feb 2007 05:50:17 +0100 (CET) Nick Piggin <npiggin@xxxxxxx> wrote:
> 
> > Nonlinear mappings are (AFAIKS) simply a virtual memory concept that
> > encodes the virtual address -> file offset differently from linear
> > mappings.
> > 
> > I can't see why the filesystem/pagecache code should need to know anything
> > about it, except for the fact that the ->nopage handler didn't quite pass
> > down enough information (ie. pgoff). But it is more logical to pass pgoff
> > rather than have the ->nopage function calculate it itself anyway. And
> > having the nopage handler install the pte itself is sort of nasty.
> > 
> > This patch introduces a new fault handler that replaces ->nopage and
> > ->populate and (later) ->nopfn. Most of the old mechanism is still in place
> > so there is a lot of duplication and nice cleanups that can be removed if
> > everyone switches over.
> > 
> > The rationale for doing this in the first place is that nonlinear mappings
> > are subject to the pagefault vs invalidate/truncate race too, and it seemed
> > stupid to duplicate the synchronisation logic rather than just consolidate
> > the two.
> > 
> 
> It's awkward to layer a largely do-nothing patch like this on top of a
> significant functional change.  Makes it harder to isolate the source of
> regressions, harder to revert the do-something patch.
> 
> > After this patch, MAP_NONBLOCK no longer sets up ptes for pages present in
> > pagecache. Seems like a fringe functionality anyway.
> 
> Does Ingo agree?

I cc'ed him when first posting it. He didn't disagree.

> > NOPAGE_REFAULT is removed. This should be implemented with ->fault, and
> > no users have hit mainline yet.
> 
> Did benh agree with that?

Yes.

> The patch unchangeloggedly adds a basic new structure to core mm
> (fault_data).  Would be nice to document its fields, especially `flags'.

OK. This is actually something that I would like more people to review.
Do we need any different fields? Should it be passed as arguments instead
of a structure?

> Please add less pointless blank lines.
> 
> 
> How well has this been tested?  The ocfs2 changes?  gfs2?  We should at
> least give those guys a heads-up.

Yes we should. Not all those filesystem changes have been tested.

> Does anybody really pass a NULL `type' arg into filemap_nopage()?

Dunno, it's exported. I remove that completely in a subsequent patch
anyway.

> This patch seems to churn things around an awful lot for minimal benefit.

Well it fixes the whole design of the nonlinear fault path.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/