| Message ID | 1b08b3f46e29dbb6c88a6f7cf038c2487386bdb0.1719462554.git.wqu@suse.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | btrfs: cleanup on the extra_gfp parameters | expand |
On Thu, Jun 27, 2024 at 5:33 AM Qu Wenruo <wqu@suse.com> wrote: > > There is only one caller utilizing the @extra_gfp parameter, > alloc_eb_folio_array(). All the other callers do not really bother the > @extra_gfp at all. > > This patch removes the parameter from the public function, meanwhile > implement an internal version with @nofail parameter just for > alloc_eb_folio_array() to utilize. > > Signed-off-by: Qu Wenruo <wqu@suse.com> > --- > fs/btrfs/extent_io.c | 43 +++++++++++++++++++++++-------------------- > fs/btrfs/extent_io.h | 3 +-- > fs/btrfs/inode.c | 2 +- > fs/btrfs/raid56.c | 6 +++--- > fs/btrfs/scrub.c | 2 +- > 5 files changed, 29 insertions(+), 27 deletions(-) > > diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c > index dc416bad9ad8..64f8e7b4d553 100644 > --- a/fs/btrfs/extent_io.c > +++ b/fs/btrfs/extent_io.c > @@ -695,22 +695,10 @@ int btrfs_alloc_folio_array(unsigned int nr_folios, struct folio **folio_array) > return -ENOMEM; > } > > -/* > - * Populate every free slot in a provided array with pages. > - * > - * @nr_pages: number of pages to allocate > - * @page_array: the array to fill with pages; any existing non-null entries in > - * the array will be skipped > - * @extra_gfp: the extra GFP flags for the allocation. > - * > - * Return: 0 if all pages were able to be allocated; > - * -ENOMEM otherwise, the partially allocated pages would be freed and > - * the array slots zeroed > - */ > -int btrfs_alloc_page_array(unsigned int nr_pages, struct page **page_array, > - gfp_t extra_gfp) > +static int __btrfs_alloc_page_array(unsigned int nr_pages, > + struct page **page_array, bool nofail) Please don't use functions prefixed with __, we have abandoned that practice, it's against our preferred coding style. Also instead of adding a wrapper function, why not just change the extra_gfp parameter of btrfs_alloc_page() to the "bool nofail" thing? You mentioned in the cover letter "callers have to pay for the extra parameter", but really are you worried about performance? On x86_64 the argument is passed in a register, which is super efficient, almost certainly better than the overhead of an extra function call. Thanks. > { > - const gfp_t gfp = GFP_NOFS | extra_gfp; > + const gfp_t gfp = nofail ? (GFP_NOFS | __GFP_NOFAIL) : GFP_NOFS; > unsigned int allocated; > > for (allocated = 0; allocated < nr_pages;) { > @@ -728,19 +716,34 @@ int btrfs_alloc_page_array(unsigned int nr_pages, struct page **page_array, > } > return 0; > } > +/* > + * Populate every free slot in a provided array with pages, using GFP_NOFS. > + * > + * @nr_pages: number of pages to allocate > + * @page_array: the array to fill with pages; any existing non-null entries in > + * the array will be skipped > + * > + * Return: 0 if all pages were able to be allocated; > + * -ENOMEM otherwise, the partially allocated pages would be freed and > + * the array slots zeroed > + */ > +int btrfs_alloc_page_array(unsigned int nr_pages, struct page **page_array) > +{ > + return __btrfs_alloc_page_array(nr_pages, page_array, false); > +} > > /* > * Populate needed folios for the extent buffer. > * > * For now, the folios populated are always in order 0 (aka, single page). > */ > -static int alloc_eb_folio_array(struct extent_buffer *eb, gfp_t extra_gfp) > +static int alloc_eb_folio_array(struct extent_buffer *eb, bool nofail) > { > struct page *page_array[INLINE_EXTENT_BUFFER_PAGES] = { 0 }; > int num_pages = num_extent_pages(eb); > int ret; > > - ret = btrfs_alloc_page_array(num_pages, page_array, extra_gfp); > + ret = __btrfs_alloc_page_array(num_pages, page_array, nofail); > if (ret < 0) > return ret; > > @@ -2722,7 +2725,7 @@ struct extent_buffer *btrfs_clone_extent_buffer(const struct extent_buffer *src) > */ > set_bit(EXTENT_BUFFER_UNMAPPED, &new->bflags); > > - ret = alloc_eb_folio_array(new, 0); > + ret = alloc_eb_folio_array(new, false); > if (ret) { > btrfs_release_extent_buffer(new); > return NULL; > @@ -2755,7 +2758,7 @@ struct extent_buffer *__alloc_dummy_extent_buffer(struct btrfs_fs_info *fs_info, > if (!eb) > return NULL; > > - ret = alloc_eb_folio_array(eb, 0); > + ret = alloc_eb_folio_array(eb, false); > if (ret) > goto err; > > @@ -3121,7 +3124,7 @@ struct extent_buffer *alloc_extent_buffer(struct btrfs_fs_info *fs_info, > > reallocate: > /* Allocate all pages first. */ > - ret = alloc_eb_folio_array(eb, __GFP_NOFAIL); > + ret = alloc_eb_folio_array(eb, true); > if (ret < 0) { > btrfs_free_subpage(prealloc); > goto out; > diff --git a/fs/btrfs/extent_io.h b/fs/btrfs/extent_io.h > index 8364dcb1ace3..0da5f1947a2b 100644 > --- a/fs/btrfs/extent_io.h > +++ b/fs/btrfs/extent_io.h > @@ -363,8 +363,7 @@ int extent_invalidate_folio(struct extent_io_tree *tree, > void btrfs_clear_buffer_dirty(struct btrfs_trans_handle *trans, > struct extent_buffer *buf); > > -int btrfs_alloc_page_array(unsigned int nr_pages, struct page **page_array, > - gfp_t extra_gfp); > +int btrfs_alloc_page_array(unsigned int nr_pages, struct page **page_array); > int btrfs_alloc_folio_array(unsigned int nr_folios, struct folio **folio_array); > > #ifdef CONFIG_BTRFS_FS_RUN_SANITY_TESTS > diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c > index 92ef9b01cf5e..6dfcd27b88ac 100644 > --- a/fs/btrfs/inode.c > +++ b/fs/btrfs/inode.c > @@ -9128,7 +9128,7 @@ static ssize_t btrfs_encoded_read_regular(struct kiocb *iocb, > pages = kcalloc(nr_pages, sizeof(struct page *), GFP_NOFS); > if (!pages) > return -ENOMEM; > - ret = btrfs_alloc_page_array(nr_pages, pages, 0); > + ret = btrfs_alloc_page_array(nr_pages, pages); > if (ret) { > ret = -ENOMEM; > goto out; > diff --git a/fs/btrfs/raid56.c b/fs/btrfs/raid56.c > index 3858c00936e8..294bf7349f96 100644 > --- a/fs/btrfs/raid56.c > +++ b/fs/btrfs/raid56.c > @@ -1051,7 +1051,7 @@ static int alloc_rbio_pages(struct btrfs_raid_bio *rbio) > { > int ret; > > - ret = btrfs_alloc_page_array(rbio->nr_pages, rbio->stripe_pages, 0); > + ret = btrfs_alloc_page_array(rbio->nr_pages, rbio->stripe_pages); > if (ret < 0) > return ret; > /* Mapping all sectors */ > @@ -1066,7 +1066,7 @@ static int alloc_rbio_parity_pages(struct btrfs_raid_bio *rbio) > int ret; > > ret = btrfs_alloc_page_array(rbio->nr_pages - data_pages, > - rbio->stripe_pages + data_pages, 0); > + rbio->stripe_pages + data_pages); > if (ret < 0) > return ret; > > @@ -1640,7 +1640,7 @@ static int alloc_rbio_data_pages(struct btrfs_raid_bio *rbio) > const int data_pages = rbio->nr_data * rbio->stripe_npages; > int ret; > > - ret = btrfs_alloc_page_array(data_pages, rbio->stripe_pages, 0); > + ret = btrfs_alloc_page_array(data_pages, rbio->stripe_pages); > if (ret < 0) > return ret; > > diff --git a/fs/btrfs/scrub.c b/fs/btrfs/scrub.c > index 4677a4f55b6a..2d819b027321 100644 > --- a/fs/btrfs/scrub.c > +++ b/fs/btrfs/scrub.c > @@ -261,7 +261,7 @@ static int init_scrub_stripe(struct btrfs_fs_info *fs_info, > atomic_set(&stripe->pending_io, 0); > spin_lock_init(&stripe->write_error_lock); > > - ret = btrfs_alloc_page_array(SCRUB_STRIPE_PAGES, stripe->pages, 0); > + ret = btrfs_alloc_page_array(SCRUB_STRIPE_PAGES, stripe->pages); > if (ret < 0) > goto error; > > -- > 2.45.2 > >
在 2024/6/27 18:20, Filipe Manana 写道: > On Thu, Jun 27, 2024 at 5:33 AM Qu Wenruo <wqu@suse.com> wrote: >> >> There is only one caller utilizing the @extra_gfp parameter, >> alloc_eb_folio_array(). All the other callers do not really bother the >> @extra_gfp at all. >> >> This patch removes the parameter from the public function, meanwhile >> implement an internal version with @nofail parameter just for >> alloc_eb_folio_array() to utilize. >> >> Signed-off-by: Qu Wenruo <wqu@suse.com> >> --- >> fs/btrfs/extent_io.c | 43 +++++++++++++++++++++++-------------------- >> fs/btrfs/extent_io.h | 3 +-- >> fs/btrfs/inode.c | 2 +- >> fs/btrfs/raid56.c | 6 +++--- >> fs/btrfs/scrub.c | 2 +- >> 5 files changed, 29 insertions(+), 27 deletions(-) >> >> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c >> index dc416bad9ad8..64f8e7b4d553 100644 >> --- a/fs/btrfs/extent_io.c >> +++ b/fs/btrfs/extent_io.c >> @@ -695,22 +695,10 @@ int btrfs_alloc_folio_array(unsigned int nr_folios, struct folio **folio_array) >> return -ENOMEM; >> } >> >> -/* >> - * Populate every free slot in a provided array with pages. >> - * >> - * @nr_pages: number of pages to allocate >> - * @page_array: the array to fill with pages; any existing non-null entries in >> - * the array will be skipped >> - * @extra_gfp: the extra GFP flags for the allocation. >> - * >> - * Return: 0 if all pages were able to be allocated; >> - * -ENOMEM otherwise, the partially allocated pages would be freed and >> - * the array slots zeroed >> - */ >> -int btrfs_alloc_page_array(unsigned int nr_pages, struct page **page_array, >> - gfp_t extra_gfp) >> +static int __btrfs_alloc_page_array(unsigned int nr_pages, >> + struct page **page_array, bool nofail) > > Please don't use functions prefixed with __, we have abandoned that > practice, it's against our preferred coding style. > > Also instead of adding a wrapper function, why not just change the > extra_gfp parameter of btrfs_alloc_page() to the "bool nofail" thing? > > You mentioned in the cover letter "callers have to pay for the extra > parameter", but really are you worried about performance? > On x86_64 the argument is passed in a register, which is super > efficient, almost certainly better than the overhead of an extra > function call. It's not about performance, but the burden on us reviewing/writing code using that function. As everytime we need to call that function, we have to check if we need to use any special value for the extra parameter. The basic idea is, to keep the most common call to be simple (aka, less parameters), and only for those special call sites to use the more complex one. This is the only time I miss function overloading in C++. Thanks, Qu > > Thanks. > >> { >> - const gfp_t gfp = GFP_NOFS | extra_gfp; >> + const gfp_t gfp = nofail ? (GFP_NOFS | __GFP_NOFAIL) : GFP_NOFS; >> unsigned int allocated; >> >> for (allocated = 0; allocated < nr_pages;) { >> @@ -728,19 +716,34 @@ int btrfs_alloc_page_array(unsigned int nr_pages, struct page **page_array, >> } >> return 0; >> } >> +/* >> + * Populate every free slot in a provided array with pages, using GFP_NOFS. >> + * >> + * @nr_pages: number of pages to allocate >> + * @page_array: the array to fill with pages; any existing non-null entries in >> + * the array will be skipped >> + * >> + * Return: 0 if all pages were able to be allocated; >> + * -ENOMEM otherwise, the partially allocated pages would be freed and >> + * the array slots zeroed >> + */ >> +int btrfs_alloc_page_array(unsigned int nr_pages, struct page **page_array) >> +{ >> + return __btrfs_alloc_page_array(nr_pages, page_array, false); >> +} >> >> /* >> * Populate needed folios for the extent buffer. >> * >> * For now, the folios populated are always in order 0 (aka, single page). >> */ >> -static int alloc_eb_folio_array(struct extent_buffer *eb, gfp_t extra_gfp) >> +static int alloc_eb_folio_array(struct extent_buffer *eb, bool nofail) >> { >> struct page *page_array[INLINE_EXTENT_BUFFER_PAGES] = { 0 }; >> int num_pages = num_extent_pages(eb); >> int ret; >> >> - ret = btrfs_alloc_page_array(num_pages, page_array, extra_gfp); >> + ret = __btrfs_alloc_page_array(num_pages, page_array, nofail); >> if (ret < 0) >> return ret; >> >> @@ -2722,7 +2725,7 @@ struct extent_buffer *btrfs_clone_extent_buffer(const struct extent_buffer *src) >> */ >> set_bit(EXTENT_BUFFER_UNMAPPED, &new->bflags); >> >> - ret = alloc_eb_folio_array(new, 0); >> + ret = alloc_eb_folio_array(new, false); >> if (ret) { >> btrfs_release_extent_buffer(new); >> return NULL; >> @@ -2755,7 +2758,7 @@ struct extent_buffer *__alloc_dummy_extent_buffer(struct btrfs_fs_info *fs_info, >> if (!eb) >> return NULL; >> >> - ret = alloc_eb_folio_array(eb, 0); >> + ret = alloc_eb_folio_array(eb, false); >> if (ret) >> goto err; >> >> @@ -3121,7 +3124,7 @@ struct extent_buffer *alloc_extent_buffer(struct btrfs_fs_info *fs_info, >> >> reallocate: >> /* Allocate all pages first. */ >> - ret = alloc_eb_folio_array(eb, __GFP_NOFAIL); >> + ret = alloc_eb_folio_array(eb, true); >> if (ret < 0) { >> btrfs_free_subpage(prealloc); >> goto out; >> diff --git a/fs/btrfs/extent_io.h b/fs/btrfs/extent_io.h >> index 8364dcb1ace3..0da5f1947a2b 100644 >> --- a/fs/btrfs/extent_io.h >> +++ b/fs/btrfs/extent_io.h >> @@ -363,8 +363,7 @@ int extent_invalidate_folio(struct extent_io_tree *tree, >> void btrfs_clear_buffer_dirty(struct btrfs_trans_handle *trans, >> struct extent_buffer *buf); >> >> -int btrfs_alloc_page_array(unsigned int nr_pages, struct page **page_array, >> - gfp_t extra_gfp); >> +int btrfs_alloc_page_array(unsigned int nr_pages, struct page **page_array); >> int btrfs_alloc_folio_array(unsigned int nr_folios, struct folio **folio_array); >> >> #ifdef CONFIG_BTRFS_FS_RUN_SANITY_TESTS >> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c >> index 92ef9b01cf5e..6dfcd27b88ac 100644 >> --- a/fs/btrfs/inode.c >> +++ b/fs/btrfs/inode.c >> @@ -9128,7 +9128,7 @@ static ssize_t btrfs_encoded_read_regular(struct kiocb *iocb, >> pages = kcalloc(nr_pages, sizeof(struct page *), GFP_NOFS); >> if (!pages) >> return -ENOMEM; >> - ret = btrfs_alloc_page_array(nr_pages, pages, 0); >> + ret = btrfs_alloc_page_array(nr_pages, pages); >> if (ret) { >> ret = -ENOMEM; >> goto out; >> diff --git a/fs/btrfs/raid56.c b/fs/btrfs/raid56.c >> index 3858c00936e8..294bf7349f96 100644 >> --- a/fs/btrfs/raid56.c >> +++ b/fs/btrfs/raid56.c >> @@ -1051,7 +1051,7 @@ static int alloc_rbio_pages(struct btrfs_raid_bio *rbio) >> { >> int ret; >> >> - ret = btrfs_alloc_page_array(rbio->nr_pages, rbio->stripe_pages, 0); >> + ret = btrfs_alloc_page_array(rbio->nr_pages, rbio->stripe_pages); >> if (ret < 0) >> return ret; >> /* Mapping all sectors */ >> @@ -1066,7 +1066,7 @@ static int alloc_rbio_parity_pages(struct btrfs_raid_bio *rbio) >> int ret; >> >> ret = btrfs_alloc_page_array(rbio->nr_pages - data_pages, >> - rbio->stripe_pages + data_pages, 0); >> + rbio->stripe_pages + data_pages); >> if (ret < 0) >> return ret; >> >> @@ -1640,7 +1640,7 @@ static int alloc_rbio_data_pages(struct btrfs_raid_bio *rbio) >> const int data_pages = rbio->nr_data * rbio->stripe_npages; >> int ret; >> >> - ret = btrfs_alloc_page_array(data_pages, rbio->stripe_pages, 0); >> + ret = btrfs_alloc_page_array(data_pages, rbio->stripe_pages); >> if (ret < 0) >> return ret; >> >> diff --git a/fs/btrfs/scrub.c b/fs/btrfs/scrub.c >> index 4677a4f55b6a..2d819b027321 100644 >> --- a/fs/btrfs/scrub.c >> +++ b/fs/btrfs/scrub.c >> @@ -261,7 +261,7 @@ static int init_scrub_stripe(struct btrfs_fs_info *fs_info, >> atomic_set(&stripe->pending_io, 0); >> spin_lock_init(&stripe->write_error_lock); >> >> - ret = btrfs_alloc_page_array(SCRUB_STRIPE_PAGES, stripe->pages, 0); >> + ret = btrfs_alloc_page_array(SCRUB_STRIPE_PAGES, stripe->pages); >> if (ret < 0) >> goto error; >> >> -- >> 2.45.2 >> >> >
On Thu, Jun 27, 2024 at 11:15 AM Qu Wenruo <quwenruo.btrfs@gmx.com> wrote: > > > > 在 2024/6/27 18:20, Filipe Manana 写道: > > On Thu, Jun 27, 2024 at 5:33 AM Qu Wenruo <wqu@suse.com> wrote: > >> > >> There is only one caller utilizing the @extra_gfp parameter, > >> alloc_eb_folio_array(). All the other callers do not really bother the > >> @extra_gfp at all. > >> > >> This patch removes the parameter from the public function, meanwhile > >> implement an internal version with @nofail parameter just for > >> alloc_eb_folio_array() to utilize. > >> > >> Signed-off-by: Qu Wenruo <wqu@suse.com> > >> --- > >> fs/btrfs/extent_io.c | 43 +++++++++++++++++++++++-------------------- > >> fs/btrfs/extent_io.h | 3 +-- > >> fs/btrfs/inode.c | 2 +- > >> fs/btrfs/raid56.c | 6 +++--- > >> fs/btrfs/scrub.c | 2 +- > >> 5 files changed, 29 insertions(+), 27 deletions(-) > >> > >> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c > >> index dc416bad9ad8..64f8e7b4d553 100644 > >> --- a/fs/btrfs/extent_io.c > >> +++ b/fs/btrfs/extent_io.c > >> @@ -695,22 +695,10 @@ int btrfs_alloc_folio_array(unsigned int nr_folios, struct folio **folio_array) > >> return -ENOMEM; > >> } > >> > >> -/* > >> - * Populate every free slot in a provided array with pages. > >> - * > >> - * @nr_pages: number of pages to allocate > >> - * @page_array: the array to fill with pages; any existing non-null entries in > >> - * the array will be skipped > >> - * @extra_gfp: the extra GFP flags for the allocation. > >> - * > >> - * Return: 0 if all pages were able to be allocated; > >> - * -ENOMEM otherwise, the partially allocated pages would be freed and > >> - * the array slots zeroed > >> - */ > >> -int btrfs_alloc_page_array(unsigned int nr_pages, struct page **page_array, > >> - gfp_t extra_gfp) > >> +static int __btrfs_alloc_page_array(unsigned int nr_pages, > >> + struct page **page_array, bool nofail) > > > > Please don't use functions prefixed with __, we have abandoned that > > practice, it's against our preferred coding style. > > > > Also instead of adding a wrapper function, why not just change the > > extra_gfp parameter of btrfs_alloc_page() to the "bool nofail" thing? > > > > You mentioned in the cover letter "callers have to pay for the extra > > parameter", but really are you worried about performance? > > On x86_64 the argument is passed in a register, which is super > > efficient, almost certainly better than the overhead of an extra > > function call. > > It's not about performance, but the burden on us reviewing/writing code > using that function. > As everytime we need to call that function, we have to check if we need > to use any special value for the extra parameter. Well, that's the case for pretty much every other function call everywhere, we have to figure out what parameter values to pass. If we start adding such wrappers around every case, we end up with plenty of these one line functions. And sooner or later someone will send a cleanup patch to remove them and make all callers pass the extra argument (we have a history of such cleanup patches). > > The basic idea is, to keep the most common call to be simple (aka, less > parameters), and only for those special call sites to use the more > complex one. > > This is the only time I miss function overloading in C++. > > Thanks, > Qu > > > > > Thanks. > > > >> { > >> - const gfp_t gfp = GFP_NOFS | extra_gfp; > >> + const gfp_t gfp = nofail ? (GFP_NOFS | __GFP_NOFAIL) : GFP_NOFS; > >> unsigned int allocated; > >> > >> for (allocated = 0; allocated < nr_pages;) { > >> @@ -728,19 +716,34 @@ int btrfs_alloc_page_array(unsigned int nr_pages, struct page **page_array, > >> } > >> return 0; > >> } > >> +/* > >> + * Populate every free slot in a provided array with pages, using GFP_NOFS. > >> + * > >> + * @nr_pages: number of pages to allocate > >> + * @page_array: the array to fill with pages; any existing non-null entries in > >> + * the array will be skipped > >> + * > >> + * Return: 0 if all pages were able to be allocated; > >> + * -ENOMEM otherwise, the partially allocated pages would be freed and > >> + * the array slots zeroed > >> + */ > >> +int btrfs_alloc_page_array(unsigned int nr_pages, struct page **page_array) > >> +{ > >> + return __btrfs_alloc_page_array(nr_pages, page_array, false); > >> +} > >> > >> /* > >> * Populate needed folios for the extent buffer. > >> * > >> * For now, the folios populated are always in order 0 (aka, single page). > >> */ > >> -static int alloc_eb_folio_array(struct extent_buffer *eb, gfp_t extra_gfp) > >> +static int alloc_eb_folio_array(struct extent_buffer *eb, bool nofail) > >> { > >> struct page *page_array[INLINE_EXTENT_BUFFER_PAGES] = { 0 }; > >> int num_pages = num_extent_pages(eb); > >> int ret; > >> > >> - ret = btrfs_alloc_page_array(num_pages, page_array, extra_gfp); > >> + ret = __btrfs_alloc_page_array(num_pages, page_array, nofail); > >> if (ret < 0) > >> return ret; > >> > >> @@ -2722,7 +2725,7 @@ struct extent_buffer *btrfs_clone_extent_buffer(const struct extent_buffer *src) > >> */ > >> set_bit(EXTENT_BUFFER_UNMAPPED, &new->bflags); > >> > >> - ret = alloc_eb_folio_array(new, 0); > >> + ret = alloc_eb_folio_array(new, false); > >> if (ret) { > >> btrfs_release_extent_buffer(new); > >> return NULL; > >> @@ -2755,7 +2758,7 @@ struct extent_buffer *__alloc_dummy_extent_buffer(struct btrfs_fs_info *fs_info, > >> if (!eb) > >> return NULL; > >> > >> - ret = alloc_eb_folio_array(eb, 0); > >> + ret = alloc_eb_folio_array(eb, false); > >> if (ret) > >> goto err; > >> > >> @@ -3121,7 +3124,7 @@ struct extent_buffer *alloc_extent_buffer(struct btrfs_fs_info *fs_info, > >> > >> reallocate: > >> /* Allocate all pages first. */ > >> - ret = alloc_eb_folio_array(eb, __GFP_NOFAIL); > >> + ret = alloc_eb_folio_array(eb, true); > >> if (ret < 0) { > >> btrfs_free_subpage(prealloc); > >> goto out; > >> diff --git a/fs/btrfs/extent_io.h b/fs/btrfs/extent_io.h > >> index 8364dcb1ace3..0da5f1947a2b 100644 > >> --- a/fs/btrfs/extent_io.h > >> +++ b/fs/btrfs/extent_io.h > >> @@ -363,8 +363,7 @@ int extent_invalidate_folio(struct extent_io_tree *tree, > >> void btrfs_clear_buffer_dirty(struct btrfs_trans_handle *trans, > >> struct extent_buffer *buf); > >> > >> -int btrfs_alloc_page_array(unsigned int nr_pages, struct page **page_array, > >> - gfp_t extra_gfp); > >> +int btrfs_alloc_page_array(unsigned int nr_pages, struct page **page_array); > >> int btrfs_alloc_folio_array(unsigned int nr_folios, struct folio **folio_array); > >> > >> #ifdef CONFIG_BTRFS_FS_RUN_SANITY_TESTS > >> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c > >> index 92ef9b01cf5e..6dfcd27b88ac 100644 > >> --- a/fs/btrfs/inode.c > >> +++ b/fs/btrfs/inode.c > >> @@ -9128,7 +9128,7 @@ static ssize_t btrfs_encoded_read_regular(struct kiocb *iocb, > >> pages = kcalloc(nr_pages, sizeof(struct page *), GFP_NOFS); > >> if (!pages) > >> return -ENOMEM; > >> - ret = btrfs_alloc_page_array(nr_pages, pages, 0); > >> + ret = btrfs_alloc_page_array(nr_pages, pages); > >> if (ret) { > >> ret = -ENOMEM; > >> goto out; > >> diff --git a/fs/btrfs/raid56.c b/fs/btrfs/raid56.c > >> index 3858c00936e8..294bf7349f96 100644 > >> --- a/fs/btrfs/raid56.c > >> +++ b/fs/btrfs/raid56.c > >> @@ -1051,7 +1051,7 @@ static int alloc_rbio_pages(struct btrfs_raid_bio *rbio) > >> { > >> int ret; > >> > >> - ret = btrfs_alloc_page_array(rbio->nr_pages, rbio->stripe_pages, 0); > >> + ret = btrfs_alloc_page_array(rbio->nr_pages, rbio->stripe_pages); > >> if (ret < 0) > >> return ret; > >> /* Mapping all sectors */ > >> @@ -1066,7 +1066,7 @@ static int alloc_rbio_parity_pages(struct btrfs_raid_bio *rbio) > >> int ret; > >> > >> ret = btrfs_alloc_page_array(rbio->nr_pages - data_pages, > >> - rbio->stripe_pages + data_pages, 0); > >> + rbio->stripe_pages + data_pages); > >> if (ret < 0) > >> return ret; > >> > >> @@ -1640,7 +1640,7 @@ static int alloc_rbio_data_pages(struct btrfs_raid_bio *rbio) > >> const int data_pages = rbio->nr_data * rbio->stripe_npages; > >> int ret; > >> > >> - ret = btrfs_alloc_page_array(data_pages, rbio->stripe_pages, 0); > >> + ret = btrfs_alloc_page_array(data_pages, rbio->stripe_pages); > >> if (ret < 0) > >> return ret; > >> > >> diff --git a/fs/btrfs/scrub.c b/fs/btrfs/scrub.c > >> index 4677a4f55b6a..2d819b027321 100644 > >> --- a/fs/btrfs/scrub.c > >> +++ b/fs/btrfs/scrub.c > >> @@ -261,7 +261,7 @@ static int init_scrub_stripe(struct btrfs_fs_info *fs_info, > >> atomic_set(&stripe->pending_io, 0); > >> spin_lock_init(&stripe->write_error_lock); > >> > >> - ret = btrfs_alloc_page_array(SCRUB_STRIPE_PAGES, stripe->pages, 0); > >> + ret = btrfs_alloc_page_array(SCRUB_STRIPE_PAGES, stripe->pages); > >> if (ret < 0) > >> goto error; > >> > >> -- > >> 2.45.2 > >> > >> > >
在 2024/6/27 20:08, Filipe Manana 写道: > On Thu, Jun 27, 2024 at 11:15 AM Qu Wenruo <quwenruo.btrfs@gmx.com> wrote: >> >> >> >> 在 2024/6/27 18:20, Filipe Manana 写道: >>> On Thu, Jun 27, 2024 at 5:33 AM Qu Wenruo <wqu@suse.com> wrote: >>>> >>>> There is only one caller utilizing the @extra_gfp parameter, >>>> alloc_eb_folio_array(). All the other callers do not really bother the >>>> @extra_gfp at all. >>>> >>>> This patch removes the parameter from the public function, meanwhile >>>> implement an internal version with @nofail parameter just for >>>> alloc_eb_folio_array() to utilize. >>>> >>>> Signed-off-by: Qu Wenruo <wqu@suse.com> >>>> --- >>>> fs/btrfs/extent_io.c | 43 +++++++++++++++++++++++-------------------- >>>> fs/btrfs/extent_io.h | 3 +-- >>>> fs/btrfs/inode.c | 2 +- >>>> fs/btrfs/raid56.c | 6 +++--- >>>> fs/btrfs/scrub.c | 2 +- >>>> 5 files changed, 29 insertions(+), 27 deletions(-) >>>> >>>> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c >>>> index dc416bad9ad8..64f8e7b4d553 100644 >>>> --- a/fs/btrfs/extent_io.c >>>> +++ b/fs/btrfs/extent_io.c >>>> @@ -695,22 +695,10 @@ int btrfs_alloc_folio_array(unsigned int nr_folios, struct folio **folio_array) >>>> return -ENOMEM; >>>> } >>>> >>>> -/* >>>> - * Populate every free slot in a provided array with pages. >>>> - * >>>> - * @nr_pages: number of pages to allocate >>>> - * @page_array: the array to fill with pages; any existing non-null entries in >>>> - * the array will be skipped >>>> - * @extra_gfp: the extra GFP flags for the allocation. >>>> - * >>>> - * Return: 0 if all pages were able to be allocated; >>>> - * -ENOMEM otherwise, the partially allocated pages would be freed and >>>> - * the array slots zeroed >>>> - */ >>>> -int btrfs_alloc_page_array(unsigned int nr_pages, struct page **page_array, >>>> - gfp_t extra_gfp) >>>> +static int __btrfs_alloc_page_array(unsigned int nr_pages, >>>> + struct page **page_array, bool nofail) >>> >>> Please don't use functions prefixed with __, we have abandoned that >>> practice, it's against our preferred coding style. >>> >>> Also instead of adding a wrapper function, why not just change the >>> extra_gfp parameter of btrfs_alloc_page() to the "bool nofail" thing? >>> >>> You mentioned in the cover letter "callers have to pay for the extra >>> parameter", but really are you worried about performance? >>> On x86_64 the argument is passed in a register, which is super >>> efficient, almost certainly better than the overhead of an extra >>> function call. >> >> It's not about performance, but the burden on us reviewing/writing code >> using that function. >> As everytime we need to call that function, we have to check if we need >> to use any special value for the extra parameter. > > Well, that's the case for pretty much every other function call > everywhere, we have to figure out what parameter values to pass. > > If we start adding such wrappers around every case, we end up with > plenty of these one line functions. > And sooner or later someone will send a cleanup patch to remove them > and make all callers pass the extra argument (we have a history of > such cleanup patches). OK, then I would just go rename the @extra_gfp parameter to @nofail as suggested. Thanks, Qu > >> >> The basic idea is, to keep the most common call to be simple (aka, less >> parameters), and only for those special call sites to use the more >> complex one. >> >> This is the only time I miss function overloading in C++. >> >> Thanks, >> Qu >> >>> >>> Thanks. >>> >>>> { >>>> - const gfp_t gfp = GFP_NOFS | extra_gfp; >>>> + const gfp_t gfp = nofail ? (GFP_NOFS | __GFP_NOFAIL) : GFP_NOFS; >>>> unsigned int allocated; >>>> >>>> for (allocated = 0; allocated < nr_pages;) { >>>> @@ -728,19 +716,34 @@ int btrfs_alloc_page_array(unsigned int nr_pages, struct page **page_array, >>>> } >>>> return 0; >>>> } >>>> +/* >>>> + * Populate every free slot in a provided array with pages, using GFP_NOFS. >>>> + * >>>> + * @nr_pages: number of pages to allocate >>>> + * @page_array: the array to fill with pages; any existing non-null entries in >>>> + * the array will be skipped >>>> + * >>>> + * Return: 0 if all pages were able to be allocated; >>>> + * -ENOMEM otherwise, the partially allocated pages would be freed and >>>> + * the array slots zeroed >>>> + */ >>>> +int btrfs_alloc_page_array(unsigned int nr_pages, struct page **page_array) >>>> +{ >>>> + return __btrfs_alloc_page_array(nr_pages, page_array, false); >>>> +} >>>> >>>> /* >>>> * Populate needed folios for the extent buffer. >>>> * >>>> * For now, the folios populated are always in order 0 (aka, single page). >>>> */ >>>> -static int alloc_eb_folio_array(struct extent_buffer *eb, gfp_t extra_gfp) >>>> +static int alloc_eb_folio_array(struct extent_buffer *eb, bool nofail) >>>> { >>>> struct page *page_array[INLINE_EXTENT_BUFFER_PAGES] = { 0 }; >>>> int num_pages = num_extent_pages(eb); >>>> int ret; >>>> >>>> - ret = btrfs_alloc_page_array(num_pages, page_array, extra_gfp); >>>> + ret = __btrfs_alloc_page_array(num_pages, page_array, nofail); >>>> if (ret < 0) >>>> return ret; >>>> >>>> @@ -2722,7 +2725,7 @@ struct extent_buffer *btrfs_clone_extent_buffer(const struct extent_buffer *src) >>>> */ >>>> set_bit(EXTENT_BUFFER_UNMAPPED, &new->bflags); >>>> >>>> - ret = alloc_eb_folio_array(new, 0); >>>> + ret = alloc_eb_folio_array(new, false); >>>> if (ret) { >>>> btrfs_release_extent_buffer(new); >>>> return NULL; >>>> @@ -2755,7 +2758,7 @@ struct extent_buffer *__alloc_dummy_extent_buffer(struct btrfs_fs_info *fs_info, >>>> if (!eb) >>>> return NULL; >>>> >>>> - ret = alloc_eb_folio_array(eb, 0); >>>> + ret = alloc_eb_folio_array(eb, false); >>>> if (ret) >>>> goto err; >>>> >>>> @@ -3121,7 +3124,7 @@ struct extent_buffer *alloc_extent_buffer(struct btrfs_fs_info *fs_info, >>>> >>>> reallocate: >>>> /* Allocate all pages first. */ >>>> - ret = alloc_eb_folio_array(eb, __GFP_NOFAIL); >>>> + ret = alloc_eb_folio_array(eb, true); >>>> if (ret < 0) { >>>> btrfs_free_subpage(prealloc); >>>> goto out; >>>> diff --git a/fs/btrfs/extent_io.h b/fs/btrfs/extent_io.h >>>> index 8364dcb1ace3..0da5f1947a2b 100644 >>>> --- a/fs/btrfs/extent_io.h >>>> +++ b/fs/btrfs/extent_io.h >>>> @@ -363,8 +363,7 @@ int extent_invalidate_folio(struct extent_io_tree *tree, >>>> void btrfs_clear_buffer_dirty(struct btrfs_trans_handle *trans, >>>> struct extent_buffer *buf); >>>> >>>> -int btrfs_alloc_page_array(unsigned int nr_pages, struct page **page_array, >>>> - gfp_t extra_gfp); >>>> +int btrfs_alloc_page_array(unsigned int nr_pages, struct page **page_array); >>>> int btrfs_alloc_folio_array(unsigned int nr_folios, struct folio **folio_array); >>>> >>>> #ifdef CONFIG_BTRFS_FS_RUN_SANITY_TESTS >>>> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c >>>> index 92ef9b01cf5e..6dfcd27b88ac 100644 >>>> --- a/fs/btrfs/inode.c >>>> +++ b/fs/btrfs/inode.c >>>> @@ -9128,7 +9128,7 @@ static ssize_t btrfs_encoded_read_regular(struct kiocb *iocb, >>>> pages = kcalloc(nr_pages, sizeof(struct page *), GFP_NOFS); >>>> if (!pages) >>>> return -ENOMEM; >>>> - ret = btrfs_alloc_page_array(nr_pages, pages, 0); >>>> + ret = btrfs_alloc_page_array(nr_pages, pages); >>>> if (ret) { >>>> ret = -ENOMEM; >>>> goto out; >>>> diff --git a/fs/btrfs/raid56.c b/fs/btrfs/raid56.c >>>> index 3858c00936e8..294bf7349f96 100644 >>>> --- a/fs/btrfs/raid56.c >>>> +++ b/fs/btrfs/raid56.c >>>> @@ -1051,7 +1051,7 @@ static int alloc_rbio_pages(struct btrfs_raid_bio *rbio) >>>> { >>>> int ret; >>>> >>>> - ret = btrfs_alloc_page_array(rbio->nr_pages, rbio->stripe_pages, 0); >>>> + ret = btrfs_alloc_page_array(rbio->nr_pages, rbio->stripe_pages); >>>> if (ret < 0) >>>> return ret; >>>> /* Mapping all sectors */ >>>> @@ -1066,7 +1066,7 @@ static int alloc_rbio_parity_pages(struct btrfs_raid_bio *rbio) >>>> int ret; >>>> >>>> ret = btrfs_alloc_page_array(rbio->nr_pages - data_pages, >>>> - rbio->stripe_pages + data_pages, 0); >>>> + rbio->stripe_pages + data_pages); >>>> if (ret < 0) >>>> return ret; >>>> >>>> @@ -1640,7 +1640,7 @@ static int alloc_rbio_data_pages(struct btrfs_raid_bio *rbio) >>>> const int data_pages = rbio->nr_data * rbio->stripe_npages; >>>> int ret; >>>> >>>> - ret = btrfs_alloc_page_array(data_pages, rbio->stripe_pages, 0); >>>> + ret = btrfs_alloc_page_array(data_pages, rbio->stripe_pages); >>>> if (ret < 0) >>>> return ret; >>>> >>>> diff --git a/fs/btrfs/scrub.c b/fs/btrfs/scrub.c >>>> index 4677a4f55b6a..2d819b027321 100644 >>>> --- a/fs/btrfs/scrub.c >>>> +++ b/fs/btrfs/scrub.c >>>> @@ -261,7 +261,7 @@ static int init_scrub_stripe(struct btrfs_fs_info *fs_info, >>>> atomic_set(&stripe->pending_io, 0); >>>> spin_lock_init(&stripe->write_error_lock); >>>> >>>> - ret = btrfs_alloc_page_array(SCRUB_STRIPE_PAGES, stripe->pages, 0); >>>> + ret = btrfs_alloc_page_array(SCRUB_STRIPE_PAGES, stripe->pages); >>>> if (ret < 0) >>>> goto error; >>>> >>>> -- >>>> 2.45.2 >>>> >>>> >>>
diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c index dc416bad9ad8..64f8e7b4d553 100644 --- a/fs/btrfs/extent_io.c +++ b/fs/btrfs/extent_io.c @@ -695,22 +695,10 @@ int btrfs_alloc_folio_array(unsigned int nr_folios, struct folio **folio_array) return -ENOMEM; } -/* - * Populate every free slot in a provided array with pages. - * - * @nr_pages: number of pages to allocate - * @page_array: the array to fill with pages; any existing non-null entries in - * the array will be skipped - * @extra_gfp: the extra GFP flags for the allocation. - * - * Return: 0 if all pages were able to be allocated; - * -ENOMEM otherwise, the partially allocated pages would be freed and - * the array slots zeroed - */ -int btrfs_alloc_page_array(unsigned int nr_pages, struct page **page_array, - gfp_t extra_gfp) +static int __btrfs_alloc_page_array(unsigned int nr_pages, + struct page **page_array, bool nofail) { - const gfp_t gfp = GFP_NOFS | extra_gfp; + const gfp_t gfp = nofail ? (GFP_NOFS | __GFP_NOFAIL) : GFP_NOFS; unsigned int allocated; for (allocated = 0; allocated < nr_pages;) { @@ -728,19 +716,34 @@ int btrfs_alloc_page_array(unsigned int nr_pages, struct page **page_array, } return 0; } +/* + * Populate every free slot in a provided array with pages, using GFP_NOFS. + * + * @nr_pages: number of pages to allocate + * @page_array: the array to fill with pages; any existing non-null entries in + * the array will be skipped + * + * Return: 0 if all pages were able to be allocated; + * -ENOMEM otherwise, the partially allocated pages would be freed and + * the array slots zeroed + */ +int btrfs_alloc_page_array(unsigned int nr_pages, struct page **page_array) +{ + return __btrfs_alloc_page_array(nr_pages, page_array, false); +} /* * Populate needed folios for the extent buffer. * * For now, the folios populated are always in order 0 (aka, single page). */ -static int alloc_eb_folio_array(struct extent_buffer *eb, gfp_t extra_gfp) +static int alloc_eb_folio_array(struct extent_buffer *eb, bool nofail) { struct page *page_array[INLINE_EXTENT_BUFFER_PAGES] = { 0 }; int num_pages = num_extent_pages(eb); int ret; - ret = btrfs_alloc_page_array(num_pages, page_array, extra_gfp); + ret = __btrfs_alloc_page_array(num_pages, page_array, nofail); if (ret < 0) return ret; @@ -2722,7 +2725,7 @@ struct extent_buffer *btrfs_clone_extent_buffer(const struct extent_buffer *src) */ set_bit(EXTENT_BUFFER_UNMAPPED, &new->bflags); - ret = alloc_eb_folio_array(new, 0); + ret = alloc_eb_folio_array(new, false); if (ret) { btrfs_release_extent_buffer(new); return NULL; @@ -2755,7 +2758,7 @@ struct extent_buffer *__alloc_dummy_extent_buffer(struct btrfs_fs_info *fs_info, if (!eb) return NULL; - ret = alloc_eb_folio_array(eb, 0); + ret = alloc_eb_folio_array(eb, false); if (ret) goto err; @@ -3121,7 +3124,7 @@ struct extent_buffer *alloc_extent_buffer(struct btrfs_fs_info *fs_info, reallocate: /* Allocate all pages first. */ - ret = alloc_eb_folio_array(eb, __GFP_NOFAIL); + ret = alloc_eb_folio_array(eb, true); if (ret < 0) { btrfs_free_subpage(prealloc); goto out; diff --git a/fs/btrfs/extent_io.h b/fs/btrfs/extent_io.h index 8364dcb1ace3..0da5f1947a2b 100644 --- a/fs/btrfs/extent_io.h +++ b/fs/btrfs/extent_io.h @@ -363,8 +363,7 @@ int extent_invalidate_folio(struct extent_io_tree *tree, void btrfs_clear_buffer_dirty(struct btrfs_trans_handle *trans, struct extent_buffer *buf); -int btrfs_alloc_page_array(unsigned int nr_pages, struct page **page_array, - gfp_t extra_gfp); +int btrfs_alloc_page_array(unsigned int nr_pages, struct page **page_array); int btrfs_alloc_folio_array(unsigned int nr_folios, struct folio **folio_array); #ifdef CONFIG_BTRFS_FS_RUN_SANITY_TESTS diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c index 92ef9b01cf5e..6dfcd27b88ac 100644 --- a/fs/btrfs/inode.c +++ b/fs/btrfs/inode.c @@ -9128,7 +9128,7 @@ static ssize_t btrfs_encoded_read_regular(struct kiocb *iocb, pages = kcalloc(nr_pages, sizeof(struct page *), GFP_NOFS); if (!pages) return -ENOMEM; - ret = btrfs_alloc_page_array(nr_pages, pages, 0); + ret = btrfs_alloc_page_array(nr_pages, pages); if (ret) { ret = -ENOMEM; goto out; diff --git a/fs/btrfs/raid56.c b/fs/btrfs/raid56.c index 3858c00936e8..294bf7349f96 100644 --- a/fs/btrfs/raid56.c +++ b/fs/btrfs/raid56.c @@ -1051,7 +1051,7 @@ static int alloc_rbio_pages(struct btrfs_raid_bio *rbio) { int ret; - ret = btrfs_alloc_page_array(rbio->nr_pages, rbio->stripe_pages, 0); + ret = btrfs_alloc_page_array(rbio->nr_pages, rbio->stripe_pages); if (ret < 0) return ret; /* Mapping all sectors */ @@ -1066,7 +1066,7 @@ static int alloc_rbio_parity_pages(struct btrfs_raid_bio *rbio) int ret; ret = btrfs_alloc_page_array(rbio->nr_pages - data_pages, - rbio->stripe_pages + data_pages, 0); + rbio->stripe_pages + data_pages); if (ret < 0) return ret; @@ -1640,7 +1640,7 @@ static int alloc_rbio_data_pages(struct btrfs_raid_bio *rbio) const int data_pages = rbio->nr_data * rbio->stripe_npages; int ret; - ret = btrfs_alloc_page_array(data_pages, rbio->stripe_pages, 0); + ret = btrfs_alloc_page_array(data_pages, rbio->stripe_pages); if (ret < 0) return ret; diff --git a/fs/btrfs/scrub.c b/fs/btrfs/scrub.c index 4677a4f55b6a..2d819b027321 100644 --- a/fs/btrfs/scrub.c +++ b/fs/btrfs/scrub.c @@ -261,7 +261,7 @@ static int init_scrub_stripe(struct btrfs_fs_info *fs_info, atomic_set(&stripe->pending_io, 0); spin_lock_init(&stripe->write_error_lock); - ret = btrfs_alloc_page_array(SCRUB_STRIPE_PAGES, stripe->pages, 0); + ret = btrfs_alloc_page_array(SCRUB_STRIPE_PAGES, stripe->pages); if (ret < 0) goto error;
There is only one caller utilizing the @extra_gfp parameter, alloc_eb_folio_array(). All the other callers do not really bother the @extra_gfp at all. This patch removes the parameter from the public function, meanwhile implement an internal version with @nofail parameter just for alloc_eb_folio_array() to utilize. Signed-off-by: Qu Wenruo <wqu@suse.com> --- fs/btrfs/extent_io.c | 43 +++++++++++++++++++++++-------------------- fs/btrfs/extent_io.h | 3 +-- fs/btrfs/inode.c | 2 +- fs/btrfs/raid56.c | 6 +++--- fs/btrfs/scrub.c | 2 +- 5 files changed, 29 insertions(+), 27 deletions(-)