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

[Klara Modin <klarasmodin@gmail.com>]

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