| Message ID | 20241004092337.21486-1-jth@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: don't BUG_ON() NOCOW ordered-extents with checksum list | expand |
在 2024/10/4 18:53, Johannes Thumshirn 写道: > From: Johannes Thumshirn <johannes.thumshirn@wdc.com> > > Curretnly 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> > --- > 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..19ba101dc09c 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)); Will the ASSERT() really get triggered? We just checked the same list_empty() one line before. I guess you mean ASSERT(!list_empty()) instead? Otherwise changing it to ASSERT() and btrfs_abort_transaction() looks good to me. Thanks, Qu > + btrfs_abort_transaction(trans, -EINVAL); > + } > > btrfs_inode_safe_disk_i_size_write(inode, 0); > ret = btrfs_update_inode_fallback(trans, inode);
On 04.10.24 11:31, Qu Wenruo wrote: > > > 在 2024/10/4 18:53, Johannes Thumshirn 写道: >> From: Johannes Thumshirn <johannes.thumshirn@wdc.com> >> >> Curretnly 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> >> --- >> 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..19ba101dc09c 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)); > > Will the ASSERT() really get triggered? We just checked the same > list_empty() one line before. > > I guess you mean ASSERT(!list_empty()) instead? > > Otherwise changing it to ASSERT() and btrfs_abort_transaction() looks > good to me. Of cause you're right! Seems like I need more coffee.
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c index 103ec917ca9d..19ba101dc09c 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);