| Message ID | 20250408122933.121056-4-frank.li@vivo.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [1/4] btrfs: use BTRFS_PATH_AUTO_FREE in insert_balance_item() | expand |
On Tue, Apr 8, 2025 at 1:19 PM Yangtao Li <frank.li@vivo.com> 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@vivo.com> > --- > 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 > >
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@vivo.com> 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@vivo.com> > > --- > > 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.
On Wed, Apr 9, 2025 at 12:04 AM David Sterba <dsterba@suse.cz> wrote: > > 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@vivo.com> 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@vivo.com> > > > --- > > > 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. That's just not possible in 2 cases out of 3 (btrfs_cancel_balance() being the exception where it's possible), since we already have an error return value to return to user space. The btrfs_handle_fs_error() call is exaggerated and doesn't accomplish anything as the balance item was already persisted in a transaction committed previously.
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); return ret; }
Handle errors by adding explicit btrfs_abort_transaction and btrfs_end_transaction calls. Signed-off-by: Yangtao Li <frank.li@vivo.com> --- fs/btrfs/volumes.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-)