Re: [PATCH] btrfs: handle subpage releasepage properly with xarray

[Qu Wenruo <quwenruo.btrfs@gmx.com>]

在 2025/5/8 03:19, Josef Bacik 写道:
> Qu reported a EB leak with the new xarray stuff.  I messed up and when I
> was converting to the easy xarray helpers I used xa_load() instead of
> xa_find(), so if we failed to find the extent buffer at the given index
> we'd just break. I meant to use the xa_find() logic which would mean
> that it would find the index or later.
> 
> But instead of hand doing the iteration, rework this function further to
> use the xa_for_each() helper to get all the eb's in the range of the
> folio.  With this we now properly drop all the extent buffers for the
> folio instead of stopping if we fail to find an eb at a given index.
> 
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>

It's indeed solving all the eb leaks so far.

I'll let the subpage fstests run for longer but I think it's safe now.

Thanks,
Qu

> ---
>   fs/btrfs/extent_io.c | 35 ++++++++++-------------------------
>   1 file changed, 10 insertions(+), 25 deletions(-)
> 
> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> index f56c99eb17dc..b44396846c04 100644
> --- a/fs/btrfs/extent_io.c
> +++ b/fs/btrfs/extent_io.c
> @@ -4246,31 +4246,14 @@ void memmove_extent_buffer(const struct extent_buffer *dst,
>   static int try_release_subpage_extent_buffer(struct folio *folio)
>   {
>   	struct btrfs_fs_info *fs_info = folio_to_fs_info(folio);
> -	u64 cur = folio_pos(folio);
> -	const u64 end = cur + PAGE_SIZE;
> +	struct extent_buffer *eb;
> +	unsigned long start = folio_pos(folio) >> fs_info->sectorsize_bits;
> +	unsigned long index = start;
> +	unsigned long end = index + (PAGE_SIZE >> fs_info->sectorsize_bits) - 1;
>   	int ret;
>   
> -	while (cur < end) {
> -		unsigned long index = cur >> fs_info->sectorsize_bits;
> -		struct extent_buffer *eb = NULL;
> -
> -		/*
> -		 * Unlike try_release_extent_buffer() which uses folio private
> -		 * to grab buffer, for subpage case we rely on xarray, thus we
> -		 * need to ensure xarray tree consistency.
> -		 *
> -		 * We also want an atomic snapshot of the xarray tree, thus go
> -		 * with spinlock rather than RCU.
> -		 */
> -		xa_lock_irq(&fs_info->buffer_tree);
> -		eb = xa_load(&fs_info->buffer_tree, index);
> -		if (!eb) {
> -			/* No more eb in the page range after or at cur */
> -			xa_unlock_irq(&fs_info->buffer_tree);
> -			break;
> -		}
> -		cur = eb->start + eb->len;
> -
> +	xa_lock_irq(&fs_info->buffer_tree);
> +	xa_for_each_range(&fs_info->buffer_tree, index, eb, start, end) {
>   		/*
>   		 * The same as try_release_extent_buffer(), to ensure the eb
>   		 * won't disappear out from under us.
> @@ -4278,8 +4261,7 @@ static int try_release_subpage_extent_buffer(struct folio *folio)
>   		spin_lock(&eb->refs_lock);
>   		if (atomic_read(&eb->refs) != 1 || extent_buffer_under_io(eb)) {
>   			spin_unlock(&eb->refs_lock);
> -			xa_unlock_irq(&fs_info->buffer_tree);
> -			break;
> +			continue;
>   		}
>   		xa_unlock_irq(&fs_info->buffer_tree);
>   
> @@ -4299,7 +4281,10 @@ static int try_release_subpage_extent_buffer(struct folio *folio)
>   		 * release_extent_buffer() will release the refs_lock.
>   		 */
>   		release_extent_buffer(eb);
> +		xa_lock_irq(&fs_info->buffer_tree);
>   	}
> +	xa_unlock_irq(&fs_info->buffer_tree);
> +
>   	/*
>   	 * Finally to check if we have cleared folio private, as if we have
>   	 * released all ebs in the page, the folio private should be cleared now.