btrfs: put all allocated extent buffer folios in failure case

[PATCH v2] btrfs: put all allocated extent buffer folios in failure case

Posted by Daniel Vacek 9 months, 2 weeks ago

When attaching a folio fails, for example if another one is already mapped,
we need to put all newly allocated folios. And as a consequence we do not
need to flag the eb UNMAPPED anymore.

Signed-off-by: Daniel Vacek <neelx@suse.com>
---
 fs/btrfs/extent_io.c | 32 ++++++++++++++------------------
 1 file changed, 14 insertions(+), 18 deletions(-)

diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index 197f5e51c4744..7023dd527d3e7 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -3385,30 +3385,26 @@ struct extent_buffer *alloc_extent_buffer(struct btrfs_fs_info *fs_info,
 	 * we'll lookup the folio for that index, and grab that EB.  We do not
 	 * want that to grab this eb, as we're getting ready to free it.  So we
 	 * have to detach it first and then unlock it.
-	 *
-	 * We have to drop our reference and NULL it out here because in the
-	 * subpage case detaching does a btrfs_folio_dec_eb_refs() for our eb.
-	 * Below when we call btrfs_release_extent_buffer() we will call
-	 * detach_extent_buffer_folio() on our remaining pages in the !subpage
-	 * case.  If we left eb->folios[i] populated in the subpage case we'd
-	 * double put our reference and be super sad.
 	 */
-	for (int i = 0; i < attached; i++) {
-		ASSERT(eb->folios[i]);
-		detach_extent_buffer_folio(eb, eb->folios[i]);
-		folio_unlock(eb->folios[i]);
-		folio_put(eb->folios[i]);
+	for (int i = 0; i < num_extent_pages(eb); i++) {
+		struct folio *folio = eb->folios[i];
+
+		if (i < attached) {
+			ASSERT(folio);
+			detach_extent_buffer_folio(eb, folio);
+			folio_unlock(folio);
+		} else if (!folio)
+			continue;
+
+		ASSERT(!folio_test_private(folio));
+		folio_put(folio);
 		eb->folios[i] = NULL;
 	}
-	/*
-	 * Now all pages of that extent buffer is unmapped, set UNMAPPED flag,
-	 * so it can be cleaned up without utilizing folio->mapping.
-	 */
-	set_bit(EXTENT_BUFFER_UNMAPPED, &eb->bflags);
-
 	btrfs_release_extent_buffer(eb);
+
 	if (ret < 0)
 		return ERR_PTR(ret);
+
 	ASSERT(existing_eb);
 	return existing_eb;
 }
-- 
2.47.2

Re: [PATCH v2] btrfs: put all allocated extent buffer folios in failure case

Posted by Klara Modin 9 months, 2 weeks ago

Hi,

On 2025-04-24 17:08, Daniel Vacek wrote:
> When attaching a folio fails, for example if another one is already mapped,
> we need to put all newly allocated folios. And as a consequence we do not
> need to flag the eb UNMAPPED anymore.
> 
> Signed-off-by: Daniel Vacek <neelx@suse.com>

This version did not trigger an oops for me.

Thanks,
Tested-by: Klara Modin <klarasmodin@gmail.com>

> ---
>   fs/btrfs/extent_io.c | 32 ++++++++++++++------------------
>   1 file changed, 14 insertions(+), 18 deletions(-)
> 
> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> index 197f5e51c4744..7023dd527d3e7 100644
> --- a/fs/btrfs/extent_io.c
> +++ b/fs/btrfs/extent_io.c
> @@ -3385,30 +3385,26 @@ struct extent_buffer *alloc_extent_buffer(struct btrfs_fs_info *fs_info,
>   	 * we'll lookup the folio for that index, and grab that EB.  We do not
>   	 * want that to grab this eb, as we're getting ready to free it.  So we
>   	 * have to detach it first and then unlock it.
> -	 *
> -	 * We have to drop our reference and NULL it out here because in the
> -	 * subpage case detaching does a btrfs_folio_dec_eb_refs() for our eb.
> -	 * Below when we call btrfs_release_extent_buffer() we will call
> -	 * detach_extent_buffer_folio() on our remaining pages in the !subpage
> -	 * case.  If we left eb->folios[i] populated in the subpage case we'd
> -	 * double put our reference and be super sad.
>   	 */
> -	for (int i = 0; i < attached; i++) {
> -		ASSERT(eb->folios[i]);
> -		detach_extent_buffer_folio(eb, eb->folios[i]);
> -		folio_unlock(eb->folios[i]);
> -		folio_put(eb->folios[i]);
> +	for (int i = 0; i < num_extent_pages(eb); i++) {
> +		struct folio *folio = eb->folios[i];
> +
> +		if (i < attached) {
> +			ASSERT(folio);
> +			detach_extent_buffer_folio(eb, folio);
> +			folio_unlock(folio);
> +		} else if (!folio)
> +			continue;
> +
> +		ASSERT(!folio_test_private(folio));
> +		folio_put(folio);
>   		eb->folios[i] = NULL;
>   	}
> -	/*
> -	 * Now all pages of that extent buffer is unmapped, set UNMAPPED flag,
> -	 * so it can be cleaned up without utilizing folio->mapping.
> -	 */
> -	set_bit(EXTENT_BUFFER_UNMAPPED, &eb->bflags);
> -
>   	btrfs_release_extent_buffer(eb);
> +
>   	if (ret < 0)
>   		return ERR_PTR(ret);
> +
>   	ASSERT(existing_eb);
>   	return existing_eb;
>   }

Re: [PATCH v2] btrfs: put all allocated extent buffer folios in failure case

Posted by Boris Burkov 9 months, 2 weeks ago

On Thu, Apr 24, 2025 at 05:08:08PM +0200, Daniel Vacek wrote:
> When attaching a folio fails, for example if another one is already mapped,
> we need to put all newly allocated folios. And as a consequence we do not
> need to flag the eb UNMAPPED anymore.
> 
> Signed-off-by: Daniel Vacek <neelx@suse.com>

Reviewed-by: Boris Burkov <boris@bur.io>

> ---
>  fs/btrfs/extent_io.c | 32 ++++++++++++++------------------
>  1 file changed, 14 insertions(+), 18 deletions(-)
> 
> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> index 197f5e51c4744..7023dd527d3e7 100644
> --- a/fs/btrfs/extent_io.c
> +++ b/fs/btrfs/extent_io.c
> @@ -3385,30 +3385,26 @@ struct extent_buffer *alloc_extent_buffer(struct btrfs_fs_info *fs_info,
>  	 * we'll lookup the folio for that index, and grab that EB.  We do not
>  	 * want that to grab this eb, as we're getting ready to free it.  So we
>  	 * have to detach it first and then unlock it.
> -	 *
> -	 * We have to drop our reference and NULL it out here because in the
> -	 * subpage case detaching does a btrfs_folio_dec_eb_refs() for our eb.
> -	 * Below when we call btrfs_release_extent_buffer() we will call
> -	 * detach_extent_buffer_folio() on our remaining pages in the !subpage
> -	 * case.  If we left eb->folios[i] populated in the subpage case we'd
> -	 * double put our reference and be super sad.
>  	 */
> -	for (int i = 0; i < attached; i++) {
> -		ASSERT(eb->folios[i]);
> -		detach_extent_buffer_folio(eb, eb->folios[i]);
> -		folio_unlock(eb->folios[i]);
> -		folio_put(eb->folios[i]);
> +	for (int i = 0; i < num_extent_pages(eb); i++) {
> +		struct folio *folio = eb->folios[i];
> +
> +		if (i < attached) {
> +			ASSERT(folio);
> +			detach_extent_buffer_folio(eb, folio);
> +			folio_unlock(folio);
> +		} else if (!folio)
> +			continue;
> +
> +		ASSERT(!folio_test_private(folio));
> +		folio_put(folio);
>  		eb->folios[i] = NULL;
>  	}
> -	/*
> -	 * Now all pages of that extent buffer is unmapped, set UNMAPPED flag,
> -	 * so it can be cleaned up without utilizing folio->mapping.
> -	 */
> -	set_bit(EXTENT_BUFFER_UNMAPPED, &eb->bflags);
> -
>  	btrfs_release_extent_buffer(eb);
> +
>  	if (ret < 0)
>  		return ERR_PTR(ret);
> +
>  	ASSERT(existing_eb);
>  	return existing_eb;
>  }
> -- 
> 2.47.2
>