| Message ID | 0f71fe47579f137a626d9c9f4e68419bad9dd4c7.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: > When adding the extent buffer leak detection I started getting failures > on some of the fuzz tests. This is because we don't clean up dirty > buffers for aborted transactions, we just leave them dirty and thus we > leak them. Fix this up by making btrfs_commit_transaction() on an > aborted transaction properly cleanup the dirty buffers that exist in the > system. > > Signed-off-by: Josef Bacik <josef@toxicpanda.com> Shouldn't we call the new clean_dirty_buffers() inside btrfs_abort_transaction()? Thanks, Qu > --- > kernel-shared/transaction.c | 45 +++++++++++++++++++++---------------- > 1 file changed, 26 insertions(+), 19 deletions(-) > > diff --git a/kernel-shared/transaction.c b/kernel-shared/transaction.c > index fcd8e6e0..01f08f0f 100644 > --- a/kernel-shared/transaction.c > +++ b/kernel-shared/transaction.c > @@ -132,6 +132,25 @@ int commit_tree_roots(struct btrfs_trans_handle *trans, > return 0; > } > > +static void clean_dirty_buffers(struct btrfs_trans_handle *trans) > +{ > + struct btrfs_fs_info *fs_info = trans->fs_info; > + struct extent_io_tree *tree = &fs_info->dirty_buffers; > + struct extent_buffer *eb; > + u64 start, end; > + > + while (find_first_extent_bit(tree, 0, &start, &end, EXTENT_DIRTY, > + NULL) == 0) { > + while (start <= end) { > + eb = find_first_extent_buffer(fs_info, start); > + BUG_ON(!eb || eb->start != start); > + start += eb->len; > + btrfs_clear_buffer_dirty(trans, eb); > + free_extent_buffer(eb); > + } > + } > +} > + > int __commit_transaction(struct btrfs_trans_handle *trans, > struct btrfs_root *root) > { > @@ -174,23 +193,7 @@ cleanup: > * Mark all remaining dirty ebs clean, as they have no chance to be written > * back anymore. > */ > - while (1) { > - int find_ret; > - > - find_ret = find_first_extent_bit(tree, 0, &start, &end, > - EXTENT_DIRTY, NULL); > - > - if (find_ret) > - break; > - > - while (start <= end) { > - eb = find_first_extent_buffer(fs_info, start); > - BUG_ON(!eb || eb->start != start); > - start += eb->len; > - btrfs_clear_buffer_dirty(trans, eb); > - free_extent_buffer(eb); > - } > - } > + clean_dirty_buffers(trans); > return ret; > } > > @@ -202,8 +205,11 @@ int btrfs_commit_transaction(struct btrfs_trans_handle *trans, > struct btrfs_fs_info *fs_info = root->fs_info; > struct btrfs_space_info *sinfo; > > - if (trans->fs_info->transaction_aborted) > - return -EROFS; > + if (trans->fs_info->transaction_aborted) { > + ret = -EROFS; > + goto error; > + } > + > /* > * Flush all accumulated delayed refs so that root-tree updates are > * consistent > @@ -277,6 +283,7 @@ commit_tree: > return ret; > error: > btrfs_abort_transaction(trans, ret); > + clean_dirty_buffers(trans); > btrfs_destroy_delayed_refs(trans); > free(trans); > return ret;
diff --git a/kernel-shared/transaction.c b/kernel-shared/transaction.c index fcd8e6e0..01f08f0f 100644 --- a/kernel-shared/transaction.c +++ b/kernel-shared/transaction.c @@ -132,6 +132,25 @@ int commit_tree_roots(struct btrfs_trans_handle *trans, return 0; } +static void clean_dirty_buffers(struct btrfs_trans_handle *trans) +{ + struct btrfs_fs_info *fs_info = trans->fs_info; + struct extent_io_tree *tree = &fs_info->dirty_buffers; + struct extent_buffer *eb; + u64 start, end; + + while (find_first_extent_bit(tree, 0, &start, &end, EXTENT_DIRTY, + NULL) == 0) { + while (start <= end) { + eb = find_first_extent_buffer(fs_info, start); + BUG_ON(!eb || eb->start != start); + start += eb->len; + btrfs_clear_buffer_dirty(trans, eb); + free_extent_buffer(eb); + } + } +} + int __commit_transaction(struct btrfs_trans_handle *trans, struct btrfs_root *root) { @@ -174,23 +193,7 @@ cleanup: * Mark all remaining dirty ebs clean, as they have no chance to be written * back anymore. */ - while (1) { - int find_ret; - - find_ret = find_first_extent_bit(tree, 0, &start, &end, - EXTENT_DIRTY, NULL); - - if (find_ret) - break; - - while (start <= end) { - eb = find_first_extent_buffer(fs_info, start); - BUG_ON(!eb || eb->start != start); - start += eb->len; - btrfs_clear_buffer_dirty(trans, eb); - free_extent_buffer(eb); - } - } + clean_dirty_buffers(trans); return ret; } @@ -202,8 +205,11 @@ int btrfs_commit_transaction(struct btrfs_trans_handle *trans, struct btrfs_fs_info *fs_info = root->fs_info; struct btrfs_space_info *sinfo; - if (trans->fs_info->transaction_aborted) - return -EROFS; + if (trans->fs_info->transaction_aborted) { + ret = -EROFS; + goto error; + } + /* * Flush all accumulated delayed refs so that root-tree updates are * consistent @@ -277,6 +283,7 @@ commit_tree: return ret; error: btrfs_abort_transaction(trans, ret); + clean_dirty_buffers(trans); btrfs_destroy_delayed_refs(trans); free(trans); return ret;
When adding the extent buffer leak detection I started getting failures on some of the fuzz tests. This is because we don't clean up dirty buffers for aborted transactions, we just leave them dirty and thus we leak them. Fix this up by making btrfs_commit_transaction() on an aborted transaction properly cleanup the dirty buffers that exist in the system. Signed-off-by: Josef Bacik <josef@toxicpanda.com> --- kernel-shared/transaction.c | 45 +++++++++++++++++++++---------------- 1 file changed, 26 insertions(+), 19 deletions(-)