diff mbox series

btrfs: handle existing eb in the radix tree properly

Message ID 93ba6929e6ce070bd27bd80220bff7112793a3ca.1702658189.git.josef@toxicpanda.com (mailing list archive)
State New, archived
Headers show
Series btrfs: handle existing eb in the radix tree properly | expand

Commit Message

Josef Bacik Dec. 15, 2023, 4:36 p.m. UTC
This fix can be folded into "btrfs: refactor alloc_extent_buffer() to
allocate-then-attach method".

My previous fix simply fixed the panic, this fixes the memory leak that
I observed after fixing the panic.

When we have an existing extent buffer in the radix tree we'll goto out
to clean everything up, but we have a

if (ret < 0)
	return ERR_PTR(ret);

Even though we have the existing extent buffer.  We've looked this thing
up so have a reference on it so we leak that, but we're also returning
an error when we shouldn't be.  Fix this up by setting ret to 0 if we
get an error back from the radix tree insert.  With these two fixups I
can now get through btrfs/187 on subpage without anything blowing up.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 fs/btrfs/extent_io.c | 1 +
 1 file changed, 1 insertion(+)

Comments

David Sterba Dec. 15, 2023, 7:50 p.m. UTC | #1
On Fri, Dec 15, 2023 at 11:36:59AM -0500, Josef Bacik wrote:
> This fix can be folded into "btrfs: refactor alloc_extent_buffer() to
> allocate-then-attach method".
> 
> My previous fix simply fixed the panic, this fixes the memory leak that
> I observed after fixing the panic.
> 
> When we have an existing extent buffer in the radix tree we'll goto out
> to clean everything up, but we have a
> 
> if (ret < 0)
> 	return ERR_PTR(ret);
> 
> Even though we have the existing extent buffer.  We've looked this thing
> up so have a reference on it so we leak that, but we're also returning
> an error when we shouldn't be.  Fix this up by setting ret to 0 if we
> get an error back from the radix tree insert.  With these two fixups I
> can now get through btrfs/187 on subpage without anything blowing up.
> 
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>

Folded to the patch, thanks. I did not update the changelog as the fix
is a pattern that we use elsewhere "reset error code when retrying".
Qu Wenruo Dec. 15, 2023, 8:44 p.m. UTC | #2
On 2023/12/16 03:06, Josef Bacik wrote:
> This fix can be folded into "btrfs: refactor alloc_extent_buffer() to
> allocate-then-attach method".
>
> My previous fix simply fixed the panic, this fixes the memory leak that
> I observed after fixing the panic.
>
> When we have an existing extent buffer in the radix tree we'll goto out
> to clean everything up, but we have a
>
> if (ret < 0)
> 	return ERR_PTR(ret);

Stupid me...

I shouldn't put this refactor in a already bug-prone behavior change patch.

>
> Even though we have the existing extent buffer.  We've looked this thing
> up so have a reference on it so we leak that, but we're also returning
> an error when we shouldn't be.  Fix this up by setting ret to 0 if we
> get an error back from the radix tree insert.  With these two fixups I
> can now get through btrfs/187 on subpage without anything blowing up.
>
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>

Reviewed-by: Qu Wenruo <wqu@suse.com>

Thanks for pinning it down and the fix,
Qu

> ---
>   fs/btrfs/extent_io.c | 1 +
>   1 file changed, 1 insertion(+)
>
> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> index b42603098b6b..375fbec298bc 100644
> --- a/fs/btrfs/extent_io.c
> +++ b/fs/btrfs/extent_io.c
> @@ -3731,6 +3731,7 @@ struct extent_buffer *alloc_extent_buffer(struct btrfs_fs_info *fs_info,
>   	spin_unlock(&fs_info->buffer_lock);
>   	radix_tree_preload_end();
>   	if (ret == -EEXIST) {
> +		ret = 0;
>   		existing_eb = find_extent_buffer(fs_info, start);
>   		if (existing_eb)
>   			goto out;
diff mbox series

Patch

diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index b42603098b6b..375fbec298bc 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -3731,6 +3731,7 @@  struct extent_buffer *alloc_extent_buffer(struct btrfs_fs_info *fs_info,
 	spin_unlock(&fs_info->buffer_lock);
 	radix_tree_preload_end();
 	if (ret == -EEXIST) {
+		ret = 0;
 		existing_eb = find_extent_buffer(fs_info, start);
 		if (existing_eb)
 			goto out;