| Message ID | d24c0b846b150fa9e5638fc90258bf2728f88351.1693945163.git.josef@toxicpanda.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs-progs: add eb leak detection and fixes | expand |
On 2023/9/6 04:21, Josef Bacik wrote: > There are several places that we call btrfs_abort_transaction() in a > failure case, but never call btrfs_commit_transaction(). This leaks the > trans handle and the associated extent buffers and such. Fix all these > sites by making sure we call btrfs_commit_transaction() after we call > btrfs_abort_transaction() to make sure all the appropriate cleanup is > done. This gets rid of the leaked extent buffer errors. > > Signed-off-by: Josef Bacik <josef@toxicpanda.com> Reviewed-by: Qu Wenruo <wqu@suse.com> Although I'd say wouldn't it be better to make btrfs_abort_transaction() more standalone? It's pretty instinctive to think btrfs_abort_transaction() should handle everything. Thanks, Qu > --- > check/main.c | 10 ++++++++-- > 1 file changed, 8 insertions(+), 2 deletions(-) > > diff --git a/check/main.c b/check/main.c > index c99092a2..1d5f570a 100644 > --- a/check/main.c > +++ b/check/main.c > @@ -3114,6 +3114,7 @@ static int check_inode_recs(struct btrfs_root *root, > ret = btrfs_make_root_dir(trans, root, root_dirid); > if (ret < 0) { > btrfs_abort_transaction(trans, ret); > + btrfs_commit_transaction(trans, root); > return ret; > } > > @@ -8011,8 +8012,10 @@ static int repair_extent_item_generation(struct extent_record *rec) > rec->generation = new_gen; > out: > btrfs_release_path(&path); > - if (ret < 0) > + if (ret < 0) { > btrfs_abort_transaction(trans, ret); > + btrfs_commit_transaction(trans, extent_root); > + } > return ret; > } > > @@ -8223,8 +8226,11 @@ repair_abort: > } > > ret = btrfs_fix_block_accounting(trans); > - if (ret) > + if (ret) { > + btrfs_abort_transaction(trans, ret); > + btrfs_commit_transaction(trans, root); > goto repair_abort; > + } > ret = btrfs_commit_transaction(trans, root); > if (ret) > goto repair_abort;
On Wed, Sep 06, 2023 at 06:55:45AM +0800, Qu Wenruo wrote: > > > On 2023/9/6 04:21, Josef Bacik wrote: > > There are several places that we call btrfs_abort_transaction() in a > > failure case, but never call btrfs_commit_transaction(). This leaks the > > trans handle and the associated extent buffers and such. Fix all these > > sites by making sure we call btrfs_commit_transaction() after we call > > btrfs_abort_transaction() to make sure all the appropriate cleanup is > > done. This gets rid of the leaked extent buffer errors. > > > > Signed-off-by: Josef Bacik <josef@toxicpanda.com> > > Reviewed-by: Qu Wenruo <wqu@suse.com> > > Although I'd say wouldn't it be better to make btrfs_abort_transaction() > more standalone? > > It's pretty instinctive to think btrfs_abort_transaction() should handle > everything. > It doesn't handle everything in the kernel, we call abort but we still have to call btrfs_end_transaction() to clean up the trans handle. Thanks, Josef
diff --git a/check/main.c b/check/main.c index c99092a2..1d5f570a 100644 --- a/check/main.c +++ b/check/main.c @@ -3114,6 +3114,7 @@ static int check_inode_recs(struct btrfs_root *root, ret = btrfs_make_root_dir(trans, root, root_dirid); if (ret < 0) { btrfs_abort_transaction(trans, ret); + btrfs_commit_transaction(trans, root); return ret; } @@ -8011,8 +8012,10 @@ static int repair_extent_item_generation(struct extent_record *rec) rec->generation = new_gen; out: btrfs_release_path(&path); - if (ret < 0) + if (ret < 0) { btrfs_abort_transaction(trans, ret); + btrfs_commit_transaction(trans, extent_root); + } return ret; } @@ -8223,8 +8226,11 @@ repair_abort: } ret = btrfs_fix_block_accounting(trans); - if (ret) + if (ret) { + btrfs_abort_transaction(trans, ret); + btrfs_commit_transaction(trans, root); goto repair_abort; + } ret = btrfs_commit_transaction(trans, root); if (ret) goto repair_abort;
There are several places that we call btrfs_abort_transaction() in a failure case, but never call btrfs_commit_transaction(). This leaks the trans handle and the associated extent buffers and such. Fix all these sites by making sure we call btrfs_commit_transaction() after we call btrfs_abort_transaction() to make sure all the appropriate cleanup is done. This gets rid of the leaked extent buffer errors. Signed-off-by: Josef Bacik <josef@toxicpanda.com> --- check/main.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)