Re: [PATCH 4/4] btrfs: Fix transaction abort during failure in del_balance_item()

[Filipe Manana]

On Tue, Apr 8, 2025 at 1:19 PM Yangtao Li <frank.li@xxxxxxxx> wrote:
>
> Handle errors by adding explicit btrfs_abort_transaction
> and btrfs_end_transaction calls.

Again, like in the previous patch, why?
This provides no reason at all why we should abort.
And the same comment below.

>
> Signed-off-by: Yangtao Li <frank.li@xxxxxxxx>
> ---
>  fs/btrfs/volumes.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index 347c475028e0..23739d18d833 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -3777,7 +3777,7 @@ static int del_balance_item(struct btrfs_fs_info *fs_info)
>         struct btrfs_trans_handle *trans;
>         BTRFS_PATH_AUTO_FREE(path);
>         struct btrfs_key key;
> -       int ret, err;
> +       int ret;
>
>         path = btrfs_alloc_path();
>         if (!path)
> @@ -3800,10 +3800,13 @@ static int del_balance_item(struct btrfs_fs_info *fs_info)
>         }
>
>         ret = btrfs_del_item(trans, root, path);
> +       if (ret)
> +               goto out;
> +
> +       return btrfs_commit_transaction(trans);
>  out:
> -       err = btrfs_commit_transaction(trans);
> -       if (err && !ret)
> -               ret = err;
> +       btrfs_abort_transaction(trans, ret);
> +       btrfs_end_transaction(trans);

A transaction abort will turn the fs into RO mode, and it's meant to
be used when we can't proceed with changes to the fs after we did
partial changes, to avoid leaving things in an inconsistent state.
Here we don't abort because we haven't done any changes before using
the transaction handle, so an abort is pointless and will turn the fs
into RO mode unnecessarily.

Thanks.

>         return ret;
>  }
>
> --
> 2.39.0
>
>