[PATCH 0/2] btrfs: cleanup on the extra_gfp parameters

[PATCH 0/2] btrfs: cleanup on the extra_gfp parameters

[Qu Wenruo]

The @extra_gfp parameters for page/folio array allocation is only for
extent buffer allocation to pass __GFP_NOFAIL.

Meanwhile all the callers have to pay for the extra parameter.

This patchset removes the parameter, and introduce an internal version
of btrfs_alloc_page_array() to handle the __GFP_NOFAIL case.
Furthermore, for that internal version, use a bool to indicate wheter
it's a NOFAIL allocation or not.

Qu Wenruo (2):
  btrfs: remove the extra_gfp parameter from btrfs_alloc_folio_array()
  btrfs: remove the extra_gfp parameter from btrfs_alloc_page_array()

 fs/btrfs/compression.c |  2 +-
 fs/btrfs/extent_io.c   | 51 +++++++++++++++++++++---------------------
 fs/btrfs/extent_io.h   |  6 ++---
 fs/btrfs/inode.c       |  2 +-
 fs/btrfs/raid56.c      |  6 ++---
 fs/btrfs/scrub.c       |  2 +-
 6 files changed, 34 insertions(+), 35 deletions(-)

-- 
2.45.2

[PATCH 1/2] btrfs: remove the extra_gfp parameter from btrfs_alloc_folio_array()

[Qu Wenruo]

The function btrfs_alloc_folio_array() is only utilized in
btrfs_submit_compressed_read() and no other location, and the only
caller is not utilizing the @extra_gfp parameter.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/compression.c | 2 +-
 fs/btrfs/extent_io.c   | 8 +++-----
 fs/btrfs/extent_io.h   | 3 +--
 3 files changed, 5 insertions(+), 8 deletions(-)

diff --git a/fs/btrfs/compression.c b/fs/btrfs/compression.c
index 85eb2cadbbf6..a149f3659b15 100644
--- a/fs/btrfs/compression.c
+++ b/fs/btrfs/compression.c
@@ -609,7 +609,7 @@ void btrfs_submit_compressed_read(struct btrfs_bio *bbio)
 		goto out_free_bio;
 	}
 
-	ret2 = btrfs_alloc_folio_array(cb->nr_folios, cb->compressed_folios, 0);
+	ret2 = btrfs_alloc_folio_array(cb->nr_folios, cb->compressed_folios);
 	if (ret2) {
 		ret = BLK_STS_RESOURCE;
 		goto out_free_compressed_pages;
diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index c7a9284e45e1..dc416bad9ad8 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -667,24 +667,22 @@ static void end_bbio_data_read(struct btrfs_bio *bbio)
 }
 
 /*
- * Populate every free slot in a provided array with folios.
+ * Populate every free slot in a provided array with folios using GFP_NOFS.
  *
  * @nr_folios:   number of folios to allocate
  * @folio_array: the array to fill with folios; any existing non-NULL entries in
  *		 the array will be skipped
- * @extra_gfp:	 the extra GFP flags for the allocation
  *
  * Return: 0        if all folios were able to be allocated;
  *         -ENOMEM  otherwise, the partially allocated folios would be freed and
  *                  the array slots zeroed
  */
-int btrfs_alloc_folio_array(unsigned int nr_folios, struct folio **folio_array,
-			    gfp_t extra_gfp)
+int btrfs_alloc_folio_array(unsigned int nr_folios, struct folio **folio_array)
 {
 	for (int i = 0; i < nr_folios; i++) {
 		if (folio_array[i])
 			continue;
-		folio_array[i] = folio_alloc(GFP_NOFS | extra_gfp, 0);
+		folio_array[i] = folio_alloc(GFP_NOFS, 0);
 		if (!folio_array[i])
 			goto error;
 	}
diff --git a/fs/btrfs/extent_io.h b/fs/btrfs/extent_io.h
index 8b33cfea6b75..8364dcb1ace3 100644
--- a/fs/btrfs/extent_io.h
+++ b/fs/btrfs/extent_io.h
@@ -365,8 +365,7 @@ void btrfs_clear_buffer_dirty(struct btrfs_trans_handle *trans,
 
 int btrfs_alloc_page_array(unsigned int nr_pages, struct page **page_array,
 			   gfp_t extra_gfp);
-int btrfs_alloc_folio_array(unsigned int nr_folios, struct folio **folio_array,
-			    gfp_t extra_gfp);
+int btrfs_alloc_folio_array(unsigned int nr_folios, struct folio **folio_array);
 
 #ifdef CONFIG_BTRFS_FS_RUN_SANITY_TESTS
 bool find_lock_delalloc_range(struct inode *inode,
-- 
2.45.2

Re: [PATCH 1/2] btrfs: remove the extra_gfp parameter from btrfs_alloc_folio_array()

[Filipe Manana]

On Thu, Jun 27, 2024 at 5:32 AM Qu Wenruo <wqu@suse.com> wrote:
>
> The function btrfs_alloc_folio_array() is only utilized in
> btrfs_submit_compressed_read() and no other location, and the only
> caller is not utilizing the @extra_gfp parameter.
>
> Signed-off-by: Qu Wenruo <wqu@suse.com>

Reviewed-by: Filipe Manana <fdmanana@suse.com>

Looks good, thanks.

> ---
>  fs/btrfs/compression.c | 2 +-
>  fs/btrfs/extent_io.c   | 8 +++-----
>  fs/btrfs/extent_io.h   | 3 +--
>  3 files changed, 5 insertions(+), 8 deletions(-)
>
> diff --git a/fs/btrfs/compression.c b/fs/btrfs/compression.c
> index 85eb2cadbbf6..a149f3659b15 100644
> --- a/fs/btrfs/compression.c
> +++ b/fs/btrfs/compression.c
> @@ -609,7 +609,7 @@ void btrfs_submit_compressed_read(struct btrfs_bio *bbio)
>                 goto out_free_bio;
>         }
>
> -       ret2 = btrfs_alloc_folio_array(cb->nr_folios, cb->compressed_folios, 0);
> +       ret2 = btrfs_alloc_folio_array(cb->nr_folios, cb->compressed_folios);
>         if (ret2) {
>                 ret = BLK_STS_RESOURCE;
>                 goto out_free_compressed_pages;
> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> index c7a9284e45e1..dc416bad9ad8 100644
> --- a/fs/btrfs/extent_io.c
> +++ b/fs/btrfs/extent_io.c
> @@ -667,24 +667,22 @@ static void end_bbio_data_read(struct btrfs_bio *bbio)
>  }
>
>  /*
> - * Populate every free slot in a provided array with folios.
> + * Populate every free slot in a provided array with folios using GFP_NOFS.
>   *
>   * @nr_folios:   number of folios to allocate
>   * @folio_array: the array to fill with folios; any existing non-NULL entries in
>   *              the array will be skipped
> - * @extra_gfp:  the extra GFP flags for the allocation
>   *
>   * Return: 0        if all folios were able to be allocated;
>   *         -ENOMEM  otherwise, the partially allocated folios would be freed and
>   *                  the array slots zeroed
>   */
> -int btrfs_alloc_folio_array(unsigned int nr_folios, struct folio **folio_array,
> -                           gfp_t extra_gfp)
> +int btrfs_alloc_folio_array(unsigned int nr_folios, struct folio **folio_array)
>  {
>         for (int i = 0; i < nr_folios; i++) {
>                 if (folio_array[i])
>                         continue;
> -               folio_array[i] = folio_alloc(GFP_NOFS | extra_gfp, 0);
> +               folio_array[i] = folio_alloc(GFP_NOFS, 0);
>                 if (!folio_array[i])
>                         goto error;
>         }
> diff --git a/fs/btrfs/extent_io.h b/fs/btrfs/extent_io.h
> index 8b33cfea6b75..8364dcb1ace3 100644
> --- a/fs/btrfs/extent_io.h
> +++ b/fs/btrfs/extent_io.h
> @@ -365,8 +365,7 @@ void btrfs_clear_buffer_dirty(struct btrfs_trans_handle *trans,
>
>  int btrfs_alloc_page_array(unsigned int nr_pages, struct page **page_array,
>                            gfp_t extra_gfp);
> -int btrfs_alloc_folio_array(unsigned int nr_folios, struct folio **folio_array,
> -                           gfp_t extra_gfp);
> +int btrfs_alloc_folio_array(unsigned int nr_folios, struct folio **folio_array);
>
>  #ifdef CONFIG_BTRFS_FS_RUN_SANITY_TESTS
>  bool find_lock_delalloc_range(struct inode *inode,
> --
> 2.45.2
>
>

[PATCH 2/2] btrfs: remove the extra_gfp parameter from btrfs_alloc_page_array()

[Qu Wenruo]

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)
 {
-	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

Re: [PATCH 2/2] btrfs: remove the extra_gfp parameter from btrfs_alloc_page_array()

[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.

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
>
>

Re: [PATCH 2/2] btrfs: remove the extra_gfp parameter from btrfs_alloc_page_array()

[Qu Wenruo]

在 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
>>
>>
>

Re: [PATCH 2/2] btrfs: remove the extra_gfp parameter from btrfs_alloc_page_array()

[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).

>
> 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
> >>
> >>
> >

Re: [PATCH 2/2] btrfs: remove the extra_gfp parameter from btrfs_alloc_page_array()

[Qu Wenruo]

在 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
>>>>
>>>>
>>>