[PATCH 1/2] etrfs: fix up misleading GFP_NOFS usage in btrfs_releasepage

[PATCH 1/2] etrfs: fix up misleading GFP_NOFS usage in btrfs_releasepage

[Michal Hocko]

From: Michal Hocko <mhocko@suse.com>

b335b0034e25 ("Btrfs: Avoid using __GFP_HIGHMEM with slab allocator")
has reduced the allocation mask in btrfs_releasepage to GFP_NOFS just
to prevent from giving an unappropriate gfp mask to the slab allocator
deeper down the callchain (in alloc_extent_state). This is wrong for
two reasons a) GFP_NOFS might be just too restrictive for the calling
context b) it is better to tweak the gfp mask down when it needs that.

So just remove the mask tweaking from btrfs_releasepage and move it
down to alloc_extent_state where it is needed.

Signed-off-by: Michal Hocko <mhocko@suse.com>
---
 fs/btrfs/extent_io.c | 5 +++++
 fs/btrfs/inode.c     | 2 +-
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index b38150eec6b4..f6ae94a4acad 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -226,6 +226,11 @@ 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);
 	if (!state)
 		return state;
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index baa40d34d2c9..d118d4659c28 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -8994,7 +8994,7 @@ static int btrfs_releasepage(struct page *page, gfp_t gfp_flags)
 {
 	if (PageWriteback(page) || PageDirty(page))
 		return 0;
-	return __btrfs_releasepage(page, gfp_flags & GFP_NOFS);
+	return __btrfs_releasepage(page, gfp_flags);
 }
 
 static void btrfs_invalidatepage(struct page *page, unsigned int offset,
-- 
2.11.0

[PATCH 2/2] btrfs: drop gfp mask tweaking in try_release_extent_state

[Michal Hocko]

From: Michal Hocko <mhocko@suse.com>

try_release_extent_state reduces the gfp mask to GFP_NOFS if it is
compatible. This is true for GFP_KERNEL as well. There is no real
reason to do that though. There is no new lock taken down the
the only consumer of the gfp mask which is
try_release_extent_state
  clear_extent_bit
    __clear_extent_bit
      alloc_extent_state

So this seems just unnecessary and confusing.

Signed-off-by: Michal Hocko <mhocko@suse.com>
---
 fs/btrfs/extent_io.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index f6ae94a4acad..8158930c8d4a 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -4326,8 +4326,6 @@ static int try_release_extent_state(struct extent_map_tree *map,
 			   EXTENT_IOBITS, 0, NULL))
 		ret = 0;
 	else {
-		if ((mask & GFP_NOFS) == GFP_NOFS)
-			mask = GFP_NOFS;
 		/*
 		 * at this point we can safely clear everything except the
 		 * locked bit and the nodatasum bit
-- 
2.11.0

Re: [PATCH 1/2] etrfs: fix up misleading GFP_NOFS usage in btrfs_releasepage

[David Sterba]

On Mon, Jan 09, 2017 at 03:39:02PM +0100, Michal Hocko wrote:
> From: Michal Hocko <mhocko@suse.com>
> 
> b335b0034e25 ("Btrfs: Avoid using __GFP_HIGHMEM with slab allocator")
> has reduced the allocation mask in btrfs_releasepage to GFP_NOFS just
> to prevent from giving an unappropriate gfp mask to the slab allocator
> deeper down the callchain (in alloc_extent_state). This is wrong for
> two reasons a) GFP_NOFS might be just too restrictive for the calling
> context b) it is better to tweak the gfp mask down when it needs that.
> 
> So just remove the mask tweaking from btrfs_releasepage and move it
> down to alloc_extent_state where it is needed.
> 
> Signed-off-by: Michal Hocko <mhocko@suse.com>
> ---
>  fs/btrfs/extent_io.c | 5 +++++
>  fs/btrfs/inode.c     | 2 +-
>  2 files changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> index b38150eec6b4..f6ae94a4acad 100644
> --- a/fs/btrfs/extent_io.c
> +++ b/fs/btrfs/extent_io.c
> @@ -226,6 +226,11 @@ 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);

Is this future proof enough? As it's enumerating some gfp flags, what if
more are necessary in the future? I'm interested about some synthetic
gfp flags that would not require knowledge about what is or is not
acceptable for slab allocator.

But otherwise looks ok to me, I'm going to merge the patch. Thanks.

Re: [PATCH 2/2] btrfs: drop gfp mask tweaking in try_release_extent_state

[David Sterba]

On Mon, Jan 09, 2017 at 03:39:03PM +0100, Michal Hocko wrote:
> From: Michal Hocko <mhocko@suse.com>
> 
> try_release_extent_state reduces the gfp mask to GFP_NOFS if it is
> compatible. This is true for GFP_KERNEL as well. There is no real
> reason to do that though. There is no new lock taken down the
> the only consumer of the gfp mask which is
> try_release_extent_state
>   clear_extent_bit
>     __clear_extent_bit
>       alloc_extent_state
> 
> So this seems just unnecessary and confusing.
> 
> Signed-off-by: Michal Hocko <mhocko@suse.com>

Reviewed-by: David Sterba <dsterba@suse.com>

Re: [PATCH 1/2] etrfs: fix up misleading GFP_NOFS usage in btrfs_releasepage

[Michal Hocko]

On Wed 11-01-17 14:55:50, David Sterba wrote:
> On Mon, Jan 09, 2017 at 03:39:02PM +0100, Michal Hocko wrote:
> > From: Michal Hocko <mhocko@suse.com>
> > 
> > b335b0034e25 ("Btrfs: Avoid using __GFP_HIGHMEM with slab allocator")
> > has reduced the allocation mask in btrfs_releasepage to GFP_NOFS just
> > to prevent from giving an unappropriate gfp mask to the slab allocator
> > deeper down the callchain (in alloc_extent_state). This is wrong for
> > two reasons a) GFP_NOFS might be just too restrictive for the calling
> > context b) it is better to tweak the gfp mask down when it needs that.
> > 
> > So just remove the mask tweaking from btrfs_releasepage and move it
> > down to alloc_extent_state where it is needed.
> > 
> > Signed-off-by: Michal Hocko <mhocko@suse.com>
> > ---
> >  fs/btrfs/extent_io.c | 5 +++++
> >  fs/btrfs/inode.c     | 2 +-
> >  2 files changed, 6 insertions(+), 1 deletion(-)
> > 
> > diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> > index b38150eec6b4..f6ae94a4acad 100644
> > --- a/fs/btrfs/extent_io.c
> > +++ b/fs/btrfs/extent_io.c
> > @@ -226,6 +226,11 @@ 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);
> 
> Is this future proof enough? As it's enumerating some gfp flags, what if
> more are necessary in the future? I'm interested about some synthetic
> gfp flags that would not require knowledge about what is or is not
> acceptable for slab allocator.

Well, I agree, that something like slab_restrict_gfp_mask(gfp_t gfp_mask)
would be much better. And in fact that sounds like a nice future
cleanup. I haven't checked how many users would find it useful yet but I
am putting that on my todo list.

> But otherwise looks ok to me, I'm going to merge the patch. Thanks.

Thanks!

-- 
Michal Hocko
SUSE Labs

Re: [PATCH 1/2] etrfs: fix up misleading GFP_NOFS usage in btrfs_releasepage

[Michal Hocko]

On Wed 11-01-17 14:55:50, David Sterba wrote:
[...]
> But otherwise looks ok to me, I'm going to merge the patch. Thanks.

I have only now noticed typo in the subject. s@etrfs:@btrfs:@

-- 
Michal Hocko
SUSE Labs