| Message ID | 20240829130640.1397970-1-mhocko@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | fs: drop GFP_NOFAIL mode from alloc_page_buffers | expand |
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>
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 >
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
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?
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
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.
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
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);