| Message ID | 20241004105333.15266-1-jth@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] btrfs: don't BUG_ON() NOCOW ordered-extents with checksum list | expand |
在 2024/10/4 20:23, Johannes Thumshirn 写道: > From: Johannes Thumshirn <johannes.thumshirn@wdc.com> > > Currently we BUG_ON() in btrfs_finish_one_ordered() if we finishing an > ordered-extent that is flagged as NOCOW, but it's checsum list is non-empty. > > This is clearly a logic error which we can recover from by aborting the > transaction. > > For developer builds which enable CONFIG_BTRFS_ASSERT, also ASSERT() that the > list is empty. > > Suggested-by: Filipe Manana <fdmanana@suse.com> > Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com> Reviewed-by: Qu Wenruo <wqu@suse.com> Thanks, Qu > --- > Changes to v1: > * Fixup if () and ASSERT() (Qu) > * Fix spelling of 'Currently' > --- > fs/btrfs/inode.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c > index 103ec917ca9d..e57b73943ab8 100644 > --- a/fs/btrfs/inode.c > +++ b/fs/btrfs/inode.c > @@ -3088,7 +3088,10 @@ int btrfs_finish_one_ordered(struct btrfs_ordered_extent *ordered_extent) > > if (test_bit(BTRFS_ORDERED_NOCOW, &ordered_extent->flags)) { > /* Logic error */ > - BUG_ON(!list_empty(&ordered_extent->list)); > + if (!list_empty(&ordered_extent->list)) { > + ASSERT(list_empty(&ordered_extent->list)); > + btrfs_abort_transaction(trans, -EINVAL); > + } > > btrfs_inode_safe_disk_i_size_write(inode, 0); > ret = btrfs_update_inode_fallback(trans, inode);
On Fri, Oct 4, 2024 at 11:53 AM Johannes Thumshirn <jth@kernel.org> wrote: > > From: Johannes Thumshirn <johannes.thumshirn@wdc.com> > > Currently we BUG_ON() in btrfs_finish_one_ordered() if we finishing an > ordered-extent that is flagged as NOCOW, but it's checsum list is non-empty. > > This is clearly a logic error which we can recover from by aborting the > transaction. > > For developer builds which enable CONFIG_BTRFS_ASSERT, also ASSERT() that the > list is empty. > > Suggested-by: Filipe Manana <fdmanana@suse.com> > Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com> > --- > Changes to v1: > * Fixup if () and ASSERT() (Qu) > * Fix spelling of 'Currently' > --- > fs/btrfs/inode.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c > index 103ec917ca9d..e57b73943ab8 100644 > --- a/fs/btrfs/inode.c > +++ b/fs/btrfs/inode.c > @@ -3088,7 +3088,10 @@ int btrfs_finish_one_ordered(struct btrfs_ordered_extent *ordered_extent) > > if (test_bit(BTRFS_ORDERED_NOCOW, &ordered_extent->flags)) { > /* Logic error */ > - BUG_ON(!list_empty(&ordered_extent->list)); > + if (!list_empty(&ordered_extent->list)) { > + ASSERT(list_empty(&ordered_extent->list)); I find this confusing and not so easy to grasp immediately. It's the same as older places where we have: if (unexpected condition) { ASSERT(0); (...) } I find it more natural and less confusing to just do: ASSERT(list_empty(&ordered_extent->list)); if (unlikely(!list_empty(&ordered_extent->list))) { ret = -EINVAL; btrfs_abort_transaction(trans, ret); goto out; } > + btrfs_abort_transaction(trans, -EINVAL); > + } This also misses setting 'ret' to the error and the goto into the label 'out', as I've placed in the example above. Thanks. > > btrfs_inode_safe_disk_i_size_write(inode, 0); > ret = btrfs_update_inode_fallback(trans, inode); > -- > 2.43.0 > >
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c index 103ec917ca9d..e57b73943ab8 100644 --- a/fs/btrfs/inode.c +++ b/fs/btrfs/inode.c @@ -3088,7 +3088,10 @@ int btrfs_finish_one_ordered(struct btrfs_ordered_extent *ordered_extent) if (test_bit(BTRFS_ORDERED_NOCOW, &ordered_extent->flags)) { /* Logic error */ - BUG_ON(!list_empty(&ordered_extent->list)); + if (!list_empty(&ordered_extent->list)) { + ASSERT(list_empty(&ordered_extent->list)); + btrfs_abort_transaction(trans, -EINVAL); + } btrfs_inode_safe_disk_i_size_write(inode, 0); ret = btrfs_update_inode_fallback(trans, inode);