| Message ID | a7914cc7ab662f8bf4e0ab5622e81622acbc186e.1686164819.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 update_ref_for_cow() we are calling btrfs_handle_fs_error() if we find > that the extent buffer has an unexpected ref count of zero, 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 as well. Besides that, btrfs_abort_transaction() also prints a > stack trace which makes it more useful. > > Also, as this is a very unexpected situation, indicating a serious > corruption/inconsistency, tag the if branch as 'unlikely' and set the > error code to -EUCLEAN instead of -EROFS. > > 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 e2943047b01d..2971e7d70d04 100644 > --- a/fs/btrfs/ctree.c > +++ b/fs/btrfs/ctree.c > @@ -421,9 +421,9 @@ static noinline int update_ref_for_cow(struct btrfs_trans_handle *trans, > &refs, &flags); > if (ret) > return ret; > - if (refs == 0) { > - ret = -EROFS; > - btrfs_handle_fs_error(fs_info, ret, NULL); > + if (unlikely(refs == 0)) { > + ret = -EUCLEAN; The same as previous patch, just one extra error message explaining the reason for EUCLEAN would be better. Thanks, Qu > + btrfs_abort_transaction(trans, ret); > return ret; > } > } else {
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 update_ref_for_cow() we are calling btrfs_handle_fs_error() if we find > > that the extent buffer has an unexpected ref count of zero, 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 as well. Besides that, btrfs_abort_transaction() also prints a > > stack trace which makes it more useful. > > > > Also, as this is a very unexpected situation, indicating a serious > > corruption/inconsistency, tag the if branch as 'unlikely' and set the > > error code to -EUCLEAN instead of -EROFS. > > > > 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 e2943047b01d..2971e7d70d04 100644 > > --- a/fs/btrfs/ctree.c > > +++ b/fs/btrfs/ctree.c > > @@ -421,9 +421,9 @@ static noinline int update_ref_for_cow(struct btrfs_trans_handle *trans, > > &refs, &flags); > > if (ret) > > return ret; > > - if (refs == 0) { > > - ret = -EROFS; > > - btrfs_handle_fs_error(fs_info, ret, NULL); > > + if (unlikely(refs == 0)) { > > + ret = -EUCLEAN; > > The same as previous patch, just one extra error message explaining the > reason for EUCLEAN would be better. 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. > > Thanks, > Qu > > + btrfs_abort_transaction(trans, ret); > > return ret; > > } > > } else {
diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c index e2943047b01d..2971e7d70d04 100644 --- a/fs/btrfs/ctree.c +++ b/fs/btrfs/ctree.c @@ -421,9 +421,9 @@ static noinline int update_ref_for_cow(struct btrfs_trans_handle *trans, &refs, &flags); if (ret) return ret; - if (refs == 0) { - ret = -EROFS; - btrfs_handle_fs_error(fs_info, ret, NULL); + if (unlikely(refs == 0)) { + ret = -EUCLEAN; + btrfs_abort_transaction(trans, ret); return ret; } } else {