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

[David Sterba]

On Tue, Apr 08, 2025 at 03:53:04PM +0100, Filipe Manana wrote:
> 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.

The del_balance_item() case seems to be unique, there's only one caller
reset_balance_state() that calls btrfs_handle_fs_error() in case of an
error. This is almost the same as a transaction abort, but
del_balance_item() may be called from various contexts.

The error handling may be improved here, e.g. some callers may care
about the actual error of del_balance_item/reset_balance_state, but
rather a hard transaction abort it could be better to handle it more
gracefully for operations that are restartable, like return an EAGAIN.