[PATCH] btrfs: handle subpage releasepage properly with xarray

[PATCH] btrfs: handle subpage releasepage properly with xarray

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

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

[Qu Wenruo]

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