[PATCH] fs: drop GFP_NOFAIL mode from alloc_page_buffers

[PATCH] fs: drop GFP_NOFAIL mode from alloc_page_buffers

[Michal Hocko]

From: Michal Hocko <mhocko@suse.com>

There is only one called of alloc_page_buffers and it doesn't require
__GFP_NOFAIL so drop this allocation mode.

Signed-off-by: Michal Hocko <mhocko@suse.com>
---
 drivers/md/md-bitmap.c      | 2 +-
 fs/buffer.c                 | 5 +----
 include/linux/buffer_head.h | 3 +--
 3 files changed, 3 insertions(+), 7 deletions(-)

while looking at GFP_NOFAIL users I have encountered this left over.

diff --git a/drivers/md/md-bitmap.c b/drivers/md/md-bitmap.c
index 08232d8dc815..db5330d97348 100644
--- a/drivers/md/md-bitmap.c
+++ b/drivers/md/md-bitmap.c
@@ -360,7 +360,7 @@ static int read_file_page(struct file *file, unsigned long index,
 	pr_debug("read bitmap file (%dB @ %llu)\n", (int)PAGE_SIZE,
 		 (unsigned long long)index << PAGE_SHIFT);
 
-	bh = alloc_page_buffers(page, blocksize, false);
+	bh = alloc_page_buffers(page, blocksize);
 	if (!bh) {
 		ret = -ENOMEM;
 		goto out;
diff --git a/fs/buffer.c b/fs/buffer.c
index e55ad471c530..f1381686d325 100644
--- a/fs/buffer.c
+++ b/fs/buffer.c
@@ -958,12 +958,9 @@ struct buffer_head *folio_alloc_buffers(struct folio *folio, unsigned long size,
 }
 EXPORT_SYMBOL_GPL(folio_alloc_buffers);
 
-struct buffer_head *alloc_page_buffers(struct page *page, unsigned long size,
-				       bool retry)
+struct buffer_head *alloc_page_buffers(struct page *page, unsigned long size)
 {
 	gfp_t gfp = GFP_NOFS | __GFP_ACCOUNT;
-	if (retry)
-		gfp |= __GFP_NOFAIL;
 
 	return folio_alloc_buffers(page_folio(page), size, gfp);
 }
diff --git a/include/linux/buffer_head.h b/include/linux/buffer_head.h
index 14acf1bbe0ce..7e903457967a 100644
--- a/include/linux/buffer_head.h
+++ b/include/linux/buffer_head.h
@@ -199,8 +199,7 @@ void folio_set_bh(struct buffer_head *bh, struct folio *folio,
 		  unsigned long offset);
 struct buffer_head *folio_alloc_buffers(struct folio *folio, unsigned long size,
 					gfp_t gfp);
-struct buffer_head *alloc_page_buffers(struct page *page, unsigned long size,
-		bool retry);
+struct buffer_head *alloc_page_buffers(struct page *page, unsigned long size);
 struct buffer_head *create_empty_buffers(struct folio *folio,
 		unsigned long blocksize, unsigned long b_state);
 void end_buffer_read_sync(struct buffer_head *bh, int uptodate);
-- 
2.46.0

Re: [PATCH] fs: drop GFP_NOFAIL mode from alloc_page_buffers

[Song Liu]

On Thu, Aug 29, 2024 at 6:06 AM Michal Hocko <mhocko@kernel.org> wrote:
>
> From: Michal Hocko <mhocko@suse.com>
>
> There is only one called of alloc_page_buffers and it doesn't require
> __GFP_NOFAIL so drop this allocation mode.
>
> Signed-off-by: Michal Hocko <mhocko@suse.com>

Thanks for the cleanup!

I guess we will take this via the fs tree. So

Acked-by: Song Liu <song@kernel.org>

Re: [PATCH] fs: drop GFP_NOFAIL mode from alloc_page_buffers

[Jan Kara]

On Thu 29-08-24 15:06:40, Michal Hocko wrote:
> From: Michal Hocko <mhocko@suse.com>
> 
> There is only one called of alloc_page_buffers and it doesn't require
> __GFP_NOFAIL so drop this allocation mode.
> 
> Signed-off-by: Michal Hocko <mhocko@suse.com>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

Although even better fix would be to convert the last remaining caller of
alloc_page_buffers() to folio_alloc_buffers()... But that may be more
difficult.

								Honza


> ---
>  drivers/md/md-bitmap.c      | 2 +-
>  fs/buffer.c                 | 5 +----
>  include/linux/buffer_head.h | 3 +--
>  3 files changed, 3 insertions(+), 7 deletions(-)
> 
> while looking at GFP_NOFAIL users I have encountered this left over.
> 
> diff --git a/drivers/md/md-bitmap.c b/drivers/md/md-bitmap.c
> index 08232d8dc815..db5330d97348 100644
> --- a/drivers/md/md-bitmap.c
> +++ b/drivers/md/md-bitmap.c
> @@ -360,7 +360,7 @@ static int read_file_page(struct file *file, unsigned long index,
>  	pr_debug("read bitmap file (%dB @ %llu)\n", (int)PAGE_SIZE,
>  		 (unsigned long long)index << PAGE_SHIFT);
>  
> -	bh = alloc_page_buffers(page, blocksize, false);
> +	bh = alloc_page_buffers(page, blocksize);
>  	if (!bh) {
>  		ret = -ENOMEM;
>  		goto out;
> diff --git a/fs/buffer.c b/fs/buffer.c
> index e55ad471c530..f1381686d325 100644
> --- a/fs/buffer.c
> +++ b/fs/buffer.c
> @@ -958,12 +958,9 @@ struct buffer_head *folio_alloc_buffers(struct folio *folio, unsigned long size,
>  }
>  EXPORT_SYMBOL_GPL(folio_alloc_buffers);
>  
> -struct buffer_head *alloc_page_buffers(struct page *page, unsigned long size,
> -				       bool retry)
> +struct buffer_head *alloc_page_buffers(struct page *page, unsigned long size)
>  {
>  	gfp_t gfp = GFP_NOFS | __GFP_ACCOUNT;
> -	if (retry)
> -		gfp |= __GFP_NOFAIL;
>  
>  	return folio_alloc_buffers(page_folio(page), size, gfp);
>  }
> diff --git a/include/linux/buffer_head.h b/include/linux/buffer_head.h
> index 14acf1bbe0ce..7e903457967a 100644
> --- a/include/linux/buffer_head.h
> +++ b/include/linux/buffer_head.h
> @@ -199,8 +199,7 @@ void folio_set_bh(struct buffer_head *bh, struct folio *folio,
>  		  unsigned long offset);
>  struct buffer_head *folio_alloc_buffers(struct folio *folio, unsigned long size,
>  					gfp_t gfp);
> -struct buffer_head *alloc_page_buffers(struct page *page, unsigned long size,
> -		bool retry);
> +struct buffer_head *alloc_page_buffers(struct page *page, unsigned long size);
>  struct buffer_head *create_empty_buffers(struct folio *folio,
>  		unsigned long blocksize, unsigned long b_state);
>  void end_buffer_read_sync(struct buffer_head *bh, int uptodate);
> -- 
> 2.46.0
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH] fs: drop GFP_NOFAIL mode from alloc_page_buffers

[Hannes Reinecke]

On 8/29/24 21:17, Jan Kara wrote:
> On Thu 29-08-24 15:06:40, Michal Hocko wrote:
>> From: Michal Hocko <mhocko@suse.com>
>>
>> There is only one called of alloc_page_buffers and it doesn't require
>> __GFP_NOFAIL so drop this allocation mode.
>>
>> Signed-off-by: Michal Hocko <mhocko@suse.com>
> 
> Looks good. Feel free to add:
> 
> Reviewed-by: Jan Kara <jack@suse.cz>
> 
> Although even better fix would be to convert the last remaining caller of
> alloc_page_buffers() to folio_alloc_buffers()... But that may be more
> difficult.
> 
Already done by Pankajs large-block patchset, currently staged in vfs.git.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                  Kernel Storage Architect
hare@suse.de                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich

Re: [PATCH] fs: drop GFP_NOFAIL mode from alloc_page_buffers

[Michal Hocko]

On Fri 30-08-24 08:11:00, Hannes Reinecke wrote:
> On 8/29/24 21:17, Jan Kara wrote:
> > On Thu 29-08-24 15:06:40, Michal Hocko wrote:
> > > From: Michal Hocko <mhocko@suse.com>
> > > 
> > > There is only one called of alloc_page_buffers and it doesn't require
> > > __GFP_NOFAIL so drop this allocation mode.
> > > 
> > > Signed-off-by: Michal Hocko <mhocko@suse.com>
> > 
> > Looks good. Feel free to add:
> > 
> > Reviewed-by: Jan Kara <jack@suse.cz>
> > 
> > Although even better fix would be to convert the last remaining caller of
> > alloc_page_buffers() to folio_alloc_buffers()... But that may be more
> > difficult.
> > 
> Already done by Pankajs large-block patchset, currently staged in vfs.git.

Which branch should I be looking at?
-- 
Michal Hocko
SUSE Labs

Re: [PATCH] fs: drop GFP_NOFAIL mode from alloc_page_buffers

[Christian Brauner]

On Fri, Aug 30, 2024 at 11:38:24AM GMT, Michal Hocko wrote:
> On Fri 30-08-24 08:11:00, Hannes Reinecke wrote:
> > On 8/29/24 21:17, Jan Kara wrote:
> > > On Thu 29-08-24 15:06:40, Michal Hocko wrote:
> > > > From: Michal Hocko <mhocko@suse.com>
> > > > 
> > > > There is only one called of alloc_page_buffers and it doesn't require
> > > > __GFP_NOFAIL so drop this allocation mode.
> > > > 
> > > > Signed-off-by: Michal Hocko <mhocko@suse.com>
> > > 
> > > Looks good. Feel free to add:
> > > 
> > > Reviewed-by: Jan Kara <jack@suse.cz>
> > > 
> > > Although even better fix would be to convert the last remaining caller of
> > > alloc_page_buffers() to folio_alloc_buffers()... But that may be more
> > > difficult.
> > > 
> > Already done by Pankajs large-block patchset, currently staged in vfs.git.
> 
> Which branch should I be looking at?

Hi Michal, Hannes should be referring to:
https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git/log/?h=vfs.blocksize

Re: [PATCH] fs: drop GFP_NOFAIL mode from alloc_page_buffers

[Michal Hocko]

On Fri 30-08-24 13:01:29, Christian Brauner wrote:
> On Fri, Aug 30, 2024 at 11:38:24AM GMT, Michal Hocko wrote:
> > On Fri 30-08-24 08:11:00, Hannes Reinecke wrote:
> > > On 8/29/24 21:17, Jan Kara wrote:
> > > > On Thu 29-08-24 15:06:40, Michal Hocko wrote:
> > > > > From: Michal Hocko <mhocko@suse.com>
> > > > > 
> > > > > There is only one called of alloc_page_buffers and it doesn't require
> > > > > __GFP_NOFAIL so drop this allocation mode.
> > > > > 
> > > > > Signed-off-by: Michal Hocko <mhocko@suse.com>
> > > > 
> > > > Looks good. Feel free to add:
> > > > 
> > > > Reviewed-by: Jan Kara <jack@suse.cz>
> > > > 
> > > > Although even better fix would be to convert the last remaining caller of
> > > > alloc_page_buffers() to folio_alloc_buffers()... But that may be more
> > > > difficult.
> > > > 
> > > Already done by Pankajs large-block patchset, currently staged in vfs.git.
> > 
> > Which branch should I be looking at?
> 
> Hi Michal, Hannes should be referring to:
> https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git/log/?h=vfs.blocksize

OK, but that branch seems to still have alloc_page_buffers user.
Maybe I am just misunderstanding what am I supposed to do here.
Anyway, I won't have much time to spend refactoring this so if there are
more changes required then I will likely not get to that. Sorry.
-- 
Michal Hocko
SUSE Labs

Re: [PATCH] fs: drop GFP_NOFAIL mode from alloc_page_buffers

[Christian Brauner]

On Thu, 29 Aug 2024 15:06:40 +0200, Michal Hocko wrote:
> There is only one called of alloc_page_buffers and it doesn't require
> __GFP_NOFAIL so drop this allocation mode.
> 
> 

Applied to the vfs.misc branch of the vfs/vfs.git tree.
Patches in the vfs.misc branch should appear in linux-next soon.

Please report any outstanding bugs that were missed during review in a
new review to the original patch series allowing us to drop it.

It's encouraged to provide Acked-bys and Reviewed-bys even though the
patch has now been applied. If possible patch trailers will be updated.

Note that commit hashes shown below are subject to change due to rebase,
trailer updates or similar. If in doubt, please check the listed branch.

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git
branch: vfs.misc

[1/1] fs: drop GFP_NOFAIL mode from alloc_page_buffers
      https://git.kernel.org/vfs/vfs/c/bf72320f8348