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

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

[Qu Wenruo]

[CHANGELOG]
v2:
- Instead of a one line wrapper, just rename @extra_gfp to @nofail

The @extra_gfp parameters for page/folio array allocation is only for
extent buffer allocation to pass __GFP_NOFAIL.
Furthermore there is no usage of btrfs_alloc_folio_array() for the extra
gfp flags.

This patchset removes the parameter for btrfs_alloc_folio_array() first,
then rename @extra_gfp to @nofail for btrfs_alloc_page_array().

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

 fs/btrfs/compression.c |  2 +-
 fs/btrfs/extent_io.c   | 28 +++++++++++++---------------
 fs/btrfs/extent_io.h   |  5 ++---
 fs/btrfs/inode.c       |  2 +-
 fs/btrfs/raid56.c      |  6 +++---
 fs/btrfs/scrub.c       |  2 +-
 6 files changed, 21 insertions(+), 24 deletions(-)

-- 
2.45.2

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

Reviewed-by: Filipe Manana <fdmanana@suse.com>
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

[PATCH v2 2/2] btrfs: rename the extra_gfp parameter of btrfs_alloc_page_array()

[Qu Wenruo]

There is only one caller utilizing the @extra_gfp parameter,
alloc_eb_folio_array().
And in that case the extra_gfp is only assigned to __GFP_NOFAIL.

This patch would rename the @extra_gfp parameter to @nofail to indicate
that.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/extent_io.c | 20 ++++++++++----------
 fs/btrfs/extent_io.h |  2 +-
 fs/btrfs/inode.c     |  2 +-
 fs/btrfs/raid56.c    |  6 +++---
 fs/btrfs/scrub.c     |  2 +-
 5 files changed, 16 insertions(+), 16 deletions(-)

diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index dc416bad9ad8..d3ce07ab9692 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -696,21 +696,21 @@ int btrfs_alloc_folio_array(unsigned int nr_folios, struct folio **folio_array)
 }
 
 /*
- * Populate every free slot in a provided array with pages.
+ * 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
- * @extra_gfp:	the extra GFP flags for the allocation.
+ *		the array will be skipped
+ * @nofail:	whether using __GFP_NOFAIL flag
  *
  * 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)
+			   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;) {
@@ -734,13 +734,13 @@ int btrfs_alloc_page_array(unsigned int nr_pages, struct page **page_array,
  *
  * 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 +2722,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 +2755,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 +3121,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..e0cf9a367373 100644
--- a/fs/btrfs/extent_io.h
+++ b/fs/btrfs/extent_io.h
@@ -364,7 +364,7 @@ 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);
+			   bool nofail);
 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..0a11d309ee89 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, false);
 	if (ret) {
 		ret = -ENOMEM;
 		goto out;
diff --git a/fs/btrfs/raid56.c b/fs/btrfs/raid56.c
index 3858c00936e8..39bec672df0c 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, false);
 	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, false);
 	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, false);
 	if (ret < 0)
 		return ret;
 
diff --git a/fs/btrfs/scrub.c b/fs/btrfs/scrub.c
index 4677a4f55b6a..14a8d7100018 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, false);
 	if (ret < 0)
 		goto error;
 
-- 
2.45.2

Re: [PATCH v2 2/2] btrfs: rename the extra_gfp parameter of btrfs_alloc_page_array()

[Filipe Manana]

On Fri, Jun 28, 2024 at 5:22 AM Qu Wenruo <wqu@suse.com> wrote:
>
> There is only one caller utilizing the @extra_gfp parameter,
> alloc_eb_folio_array().
> And in that case the extra_gfp is only assigned to __GFP_NOFAIL.
>
> This patch would rename the @extra_gfp parameter to @nofail to indicate
> that.

"would rename" -> renames

Anyway, it's good now:

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

Thanks.

>
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>  fs/btrfs/extent_io.c | 20 ++++++++++----------
>  fs/btrfs/extent_io.h |  2 +-
>  fs/btrfs/inode.c     |  2 +-
>  fs/btrfs/raid56.c    |  6 +++---
>  fs/btrfs/scrub.c     |  2 +-
>  5 files changed, 16 insertions(+), 16 deletions(-)
>
> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> index dc416bad9ad8..d3ce07ab9692 100644
> --- a/fs/btrfs/extent_io.c
> +++ b/fs/btrfs/extent_io.c
> @@ -696,21 +696,21 @@ int btrfs_alloc_folio_array(unsigned int nr_folios, struct folio **folio_array)
>  }
>
>  /*
> - * Populate every free slot in a provided array with pages.
> + * 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
> - * @extra_gfp: the extra GFP flags for the allocation.
> + *             the array will be skipped
> + * @nofail:    whether using __GFP_NOFAIL flag
>   *
>   * 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)
> +                          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;) {
> @@ -734,13 +734,13 @@ int btrfs_alloc_page_array(unsigned int nr_pages, struct page **page_array,
>   *
>   * 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 +2722,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 +2755,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 +3121,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..e0cf9a367373 100644
> --- a/fs/btrfs/extent_io.h
> +++ b/fs/btrfs/extent_io.h
> @@ -364,7 +364,7 @@ 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);
> +                          bool nofail);
>  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..0a11d309ee89 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, false);
>         if (ret) {
>                 ret = -ENOMEM;
>                 goto out;
> diff --git a/fs/btrfs/raid56.c b/fs/btrfs/raid56.c
> index 3858c00936e8..39bec672df0c 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, false);
>         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, false);
>         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, false);
>         if (ret < 0)
>                 return ret;
>
> diff --git a/fs/btrfs/scrub.c b/fs/btrfs/scrub.c
> index 4677a4f55b6a..14a8d7100018 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, false);
>         if (ret < 0)
>                 goto error;
>
> --
> 2.45.2
>
>

Re: [PATCH v2 2/2] btrfs: rename the extra_gfp parameter of btrfs_alloc_page_array()

[David Sterba]

On Fri, Jun 28, 2024 at 10:33:19AM +0100, Filipe Manana wrote:
> On Fri, Jun 28, 2024 at 5:22 AM Qu Wenruo <wqu@suse.com> wrote:
> >
> > There is only one caller utilizing the @extra_gfp parameter,
> > alloc_eb_folio_array().
> > And in that case the extra_gfp is only assigned to __GFP_NOFAIL.
> >
> > This patch would rename the @extra_gfp parameter to @nofail to indicate
> > that.
> 
> "would rename" -> renames

Ideally also leave out the whole 'This patch ...', I've been fixing that
when I notice it but I've just found more in now immutable patches so
it's obviously not perfect.