etrfs: fix up misleading GFP_NOFS usage in btrfs_releasepage
PATCH 1/2 etrfs: fix up misleading GFP_NOFS usage in btrfs_releasepage
From: Michal Hocko <[email protected]>
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.
--- fs/btrfs/extent_io.c | 5 +++++ fs/btrfs/inode.c | 2 +- 2 files changed, 6 insertions(+), 1 deletion(-)
fs/btrfs/extent_io.c +5 -0
@@ -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;
fs/btrfs/inode.c +1 -1
@@ -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,
Signature · 1 line
2.11.0
PATCH 2/2 btrfs: drop gfp mask tweaking in try_release_extent_state
From: Michal Hocko <[email protected]>
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.
--- fs/btrfs/extent_io.c | 2 -- 1 file changed, 2 deletions(-)
fs/btrfs/extent_io.c +0 -2
@@ -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
Signature · 1 line
2.11.0
Re: [PATCH 2/2] btrfs: drop gfp mask tweaking in try_release_extent_state
Quoted 11 lines · Michal Hocko
From: Michal Hocko <[email protected]>
try_release_extent_statereduces the gfp mask toGFP_NOFSif it is compatible. This is true forGFP_KERNELas 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 istry_release_extent_state
clear_extent_bit
__clear_extent_bit
alloc_extent_state
Quoted 3 lines
So this seems just unnecessary and confusing.
Signed-off-by Michal Hocko <[email protected]>
Quoted 23 lines · Michal Hocko
From: Michal Hocko <[email protected]>
b335b0034e25("Btrfs: Avoid using__GFP_HIGHMEMwith slab allocator") has reduced the allocation mask inbtrfs_releasepagetoGFP_NOFSjust to prevent from giving an unappropriate gfp mask to the slab allocator deeper down the callchain (inalloc_extent_state). This is wrong for two reasons a)GFP_NOFSmight 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_releasepageand move it down toalloc_extent_statewhere it is needed.Signed-off-by Michal Hocko <[email protected]>--- fs/btrfs/extent_io.c | 5 +++++ fs/btrfs/inode.c | 2 +- 2 files changed, 6 insertions(+), 1 deletion(-)fs/btrfs/extent_io.c +0
-0
fs/btrfs/extent_io.c static struct extent_state *alloc_extent_state(gfp_t mask) · line 233
Quoted 8 lines
fs/btrfs/extent_io.c +5
-0@@ -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.
Quoted 27 lines · David Sterba
Quoted 26 lines · Michal Hocko
From: Michal Hocko <[email protected]>
b335b0034e25("Btrfs: Avoid using__GFP_HIGHMEMwith slab allocator") has reduced the allocation mask inbtrfs_releasepagetoGFP_NOFSjust to prevent from giving an unappropriate gfp mask to the slab allocator deeper down the callchain (inalloc_extent_state). This is wrong for two reasons a)GFP_NOFSmight 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_releasepageand move it down toalloc_extent_statewhere it is needed.Signed-off-by Michal Hocko <[email protected]>--- fs/btrfs/extent_io.c | 5 +++++ fs/btrfs/inode.c | 2 +- 2 files changed, 6 insertions(+), 1 deletion(-)fs/btrfs/extent_io.c +0
-0@@ -226,6 +226,11 @@ static struct extent_state *alloc_extent_state(gfp_t mask) {struct extent_state *state;
Quoted 10 lines
fs/btrfs/extent_io.c static struct extent_state *alloc_extent_state(gfp_t mask) · line 233
Quoted 5 lines
fs/btrfs/extent_io.c +5
-0+/* +* 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.
Quoted 1 line
But otherwise looks ok to me, I'm going to merge the patch. Thanks.
Thanks!
Signature · 2 lines
Michal Hocko SUSE Labs
On Wed 11-01-17 14:55:50, David Sterba wrote:
[...]
Quoted 1 line
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:@
Signature · 2 lines
Michal Hocko SUSE Labs