Re: [patch 1/2] mm: pagecache allocation gfp fixes

[Nick Piggin <npiggin@suse.de>]

On Thu, Nov 27, 2008 at 06:14:18PM +0000, Hugh Dickins wrote:
> On Thu, 27 Nov 2008, Nick Piggin wrote:
> > On Thu, Nov 27, 2008 at 11:52:40AM +0200, Pekka Enberg wrote:
> > > > -               err = add_to_page_cache_lru(page, mapping, index, gfp_mask);
> > > > +               err = add_to_page_cache_lru(page, mapping, index,
> > > > +                       (gfp_mask & (__GFP_FS|__GFP_IO|__GFP_WAIT|__GFP_HIGH)));
> > > 
> > > Can we use GFP_RECLAIM_MASK here? I mean, surely we need to pass
> > > __GFP_NOFAIL, for example, down to radix_tree_preload() et al?
> 
> I certainly agree with Pekka's suggestion to use GFP_RECLAIM_MASK.
> 
> > 
> > Updated patch.
> 
> I'm not sure about it.  I came here before 2.6.25, not yet got around
> to submitting, I went in the opposite direction.  What drove me was an
> irritation at the growing number of & ~__GFP_HIGHMEMs (after adding a
> couple myself in shmem.c).  At the least, I think we ought to change
> those to & GFP_RECLAIM_MASKs (it seems we don't have one, but can
> imagine a block driver that wants GFP_DMA or GFP_DMA32 for pagecache:
> there's no reason to alloc its kernel-internal data structures for DMA).
> 
> My patch went the opposite direction to yours, in that I was pushing
> down the GFP_RECLAIM_MASKing into lib/radix-tree.c and mm/memcontrol.c
> (but that now doesn't kmalloc for itself, so no longer needs the mask).
> 
> I'm not sure which way is the right way: you can say I'm inconsistent
> not to push it down into slab/sleb/slib/slob/slub itself, but I've a
> notion that someone might object to any extra intrns in their hotpaths.
> 
> My design principle, I think, was to minimize the line length of
> the maximum number of source lines: you may believe in other
> design principles of higher value.

I think logically it doesn't belong in those files. The problem comes
purely from the pagecache layer because we're using gfp masks to ask
for two different (and not quite compatible things).

We're using it to specify the pagecache page's memory type, and also
the allocation context for both the pagecache page, and any other
supporting allocations required.

I think it's much more logical to go into the pagecache layer.

 
> Updating it quickly to 2.6.28-rc6, built but untested, here's mine.
> I'm not saying it's the right approach and yours wrong, just please
> consider it before deciding on which way to go.
> 
> I've left in the hunk from fs/buffer.c in case you can decipher it,
> I think that's what held me up from submitting: I've never worked
> out since whether that change is a profound insight into reality
> here, or just a blind attempt to reduce the line length.

I don't see why you drop the mapping_gfp_mask from there... if we ever
change the buffer layer to support HIGHMEM pages, we'd want to set the
inode's mapping_gfp_mask to GFP_HIGHUSER rather than GFP_USER, wouldn't
we?