| 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 |
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".
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 --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;
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(+)