Re: [External] Re: [PATCH 2/3] include/linux/gfp.h: use unsigned int in gfp_zone

[Matthew Wilcox]

On Mon, May 07, 2018 at 11:25:01PM +0200, David Sterba wrote:
> On Mon, May 07, 2018 at 11:44:10AM -0700, Matthew Wilcox wrote:
> > But something like btrfs should almost certainly be using ~GFP_ZONEMASK.
> 
> Agreed, the direct use of __GFP_DMA32 was added in 3ba7ab220e8918176c6f
> to substitute GFP_NOFS, so the allocation flags are less restrictive but
> still acceptable for allocation from slab.
> 
> The requirement from btrfs is to avoid highmem, the 'must be acceptable
> for slab' requirement is more MM internal and should have been hidden
> under some opaque flag mask. There was no strong need for that at the
> time.

The GFP flags encode a multiple of different requirements.  There's
"What can the allocator do to free memory" and "what area of memory
can the allocation come from".  btrfs doesn't actually want to
allocate memory from ZONE_MOVABLE or ZONE_DMA either.  It's probably never
been called with those particular flags set, but in the spirit of
future-proofing btrfs, perhaps a patch like this is in order?

---- >8 ----

Subject: btrfs: Allocate extents from ZONE_NORMAL
From: Matthew Wilcox <mawilcox@xxxxxxxxxxxxx>

If anyone ever passes a GFP_DMA or GFP_MOVABLE allocation flag to
allocate_extent_state, it will try to allocate memory from the wrong zone.
We just want to allocate memory from ZONE_NORMAL, so use GFP_RECLAIM_MASK
to get what we want.

Signed-off-by: Matthew Wilcox <mawilcox@xxxxxxxxxxxxx>

diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index e99b329002cf..4e4a67b7b29d 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -216,12 +216,7 @@ static struct extent_state *alloc_extent_state(gfp_t mask)
 {
 	struct extent_state *state;
 
-	/*
-	 * The given mask might be not appropriate for the slab allocator,
-	 * drop the unsupported bits
-	 */
-	mask &= ~(__GFP_DMA32|__GFP_HIGHMEM);
-	state = kmem_cache_alloc(extent_state_cache, mask);
+	state = kmem_cache_alloc(extent_state_cache, mask & GFP_RECLAIM_MASK);
 	if (!state)
 		return state;
 	state->state = 0;