| Message ID | 91e588216500d2aaa7e119e5ac4be927c71bf066.1686164817.git.fdmanana@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: some fixes and updates around handling errors for tree mod log operations | expand |
On 2023/6/8 03:24, fdmanana@kernel.org wrote: > From: Filipe Manana <fdmanana@suse.com> > > At balance_level() we are calling btrfs_handle_fs_error() when the middle > child only has 1 item and the left child is missing, however we can simply > use btrfs_abort_transaction(), which achieves the same purposes: to turn > the fs to error state, abort the current transaction and turn the fs to > RO mode. Besides that, btrfs_abort_transaction() also prints a stack trace > which makes it more useful. > > Also, as this is an highly unexpected case and it's about a b+tree > inconsistency, change the error code from -EROFS to -EUCLEAN and tag the > if branch as 'unlikely'. > > Signed-off-by: Filipe Manana <fdmanana@suse.com> > --- > fs/btrfs/ctree.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c > index 4dcdcf25c3fe..e2943047b01d 100644 > --- a/fs/btrfs/ctree.c > +++ b/fs/btrfs/ctree.c > @@ -1164,9 +1164,9 @@ static noinline int balance_level(struct btrfs_trans_handle *trans, > * otherwise we would have pulled some pointers from the > * right > */ > - if (!left) { > - ret = -EROFS; > - btrfs_handle_fs_error(fs_info, ret, NULL); > + if (unlikely(!left)) { > + ret = -EUCLEAN; I'd prefer to have an message every time we return -EUCLEAN. Otherwise looks good to me. Thanks, Qu > + btrfs_abort_transaction(trans, ret); > goto out; > } > wret = balance_node_right(trans, mid, left);
On Thu, Jun 8, 2023 at 9:58 AM Qu Wenruo <quwenruo.btrfs@gmx.com> wrote: > > > > On 2023/6/8 03:24, fdmanana@kernel.org wrote: > > From: Filipe Manana <fdmanana@suse.com> > > > > At balance_level() we are calling btrfs_handle_fs_error() when the middle > > child only has 1 item and the left child is missing, however we can simply > > use btrfs_abort_transaction(), which achieves the same purposes: to turn > > the fs to error state, abort the current transaction and turn the fs to > > RO mode. Besides that, btrfs_abort_transaction() also prints a stack trace > > which makes it more useful. > > > > Also, as this is an highly unexpected case and it's about a b+tree > > inconsistency, change the error code from -EROFS to -EUCLEAN and tag the > > if branch as 'unlikely'. > > > > Signed-off-by: Filipe Manana <fdmanana@suse.com> > > --- > > fs/btrfs/ctree.c | 6 +++--- > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c > > index 4dcdcf25c3fe..e2943047b01d 100644 > > --- a/fs/btrfs/ctree.c > > +++ b/fs/btrfs/ctree.c > > @@ -1164,9 +1164,9 @@ static noinline int balance_level(struct btrfs_trans_handle *trans, > > * otherwise we would have pulled some pointers from the > > * right > > */ > > - if (!left) { > > - ret = -EROFS; > > - btrfs_handle_fs_error(fs_info, ret, NULL); > > + if (unlikely(!left)) { > > + ret = -EUCLEAN; > > I'd prefer to have an message every time we return -EUCLEAN. Sure... I didn't see a strong need for that because the transaction abort's stack trace will be obvious. But I can add it in v2. Thanks. > > Otherwise looks good to me. > > Thanks, > Qu > > + btrfs_abort_transaction(trans, ret); > > goto out; > > } > > wret = balance_node_right(trans, mid, left);
On Thu, Jun 08, 2023 at 10:47:49AM +0100, Filipe Manana wrote: > On Thu, Jun 8, 2023 at 9:58 AM Qu Wenruo <quwenruo.btrfs@gmx.com> wrote: > > > > > > > > On 2023/6/8 03:24, fdmanana@kernel.org wrote: > > > From: Filipe Manana <fdmanana@suse.com> > > > > > > At balance_level() we are calling btrfs_handle_fs_error() when the middle > > > child only has 1 item and the left child is missing, however we can simply > > > use btrfs_abort_transaction(), which achieves the same purposes: to turn > > > the fs to error state, abort the current transaction and turn the fs to > > > RO mode. Besides that, btrfs_abort_transaction() also prints a stack trace > > > which makes it more useful. > > > > > > Also, as this is an highly unexpected case and it's about a b+tree > > > inconsistency, change the error code from -EROFS to -EUCLEAN and tag the > > > if branch as 'unlikely'. > > > > > > Signed-off-by: Filipe Manana <fdmanana@suse.com> > > > --- > > > fs/btrfs/ctree.c | 6 +++--- > > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > > > diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c > > > index 4dcdcf25c3fe..e2943047b01d 100644 > > > --- a/fs/btrfs/ctree.c > > > +++ b/fs/btrfs/ctree.c > > > @@ -1164,9 +1164,9 @@ static noinline int balance_level(struct btrfs_trans_handle *trans, > > > * otherwise we would have pulled some pointers from the > > > * right > > > */ > > > - if (!left) { > > > - ret = -EROFS; > > > - btrfs_handle_fs_error(fs_info, ret, NULL); > > > + if (unlikely(!left)) { > > > + ret = -EUCLEAN; > > > > I'd prefer to have an message every time we return -EUCLEAN. > > Sure... I didn't see a strong need for that because the transaction > abort's stack > trace will be obvious. But I can add it in v2. The error messages are a nice to have description of what happened, it would be logged and may help to recognize the problem without reading the stack trace or source. Also the message itself can print the values for the failed conditions so this can give some clue as well.
diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c index 4dcdcf25c3fe..e2943047b01d 100644 --- a/fs/btrfs/ctree.c +++ b/fs/btrfs/ctree.c @@ -1164,9 +1164,9 @@ static noinline int balance_level(struct btrfs_trans_handle *trans, * otherwise we would have pulled some pointers from the * right */ - if (!left) { - ret = -EROFS; - btrfs_handle_fs_error(fs_info, ret, NULL); + if (unlikely(!left)) { + ret = -EUCLEAN; + btrfs_abort_transaction(trans, ret); goto out; } wret = balance_node_right(trans, mid, left);