Re: [PATCH 2/3] btrfs-progs: properly cleanup aborted transactions in check

[Qu Wenruo <quwenruo.btrfs@gmx.com>]

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;