Re: [PATCH 3/4] btrfs: replace BUG() and BUG_ON() with error handling in extent-tree.c
From: Qu Wenruo
Date: Fri Feb 27 2026 - 15:43:34 EST
在 2026/2/28 05:01, Adarsh Das 写道:
Replace BUG() and BUG_ON() calls with proper error handling instead
of crashing the kernel. Use btrfs_err() and return -EUCLEAN where
fs_info is available, and WARN_ON() with an error return where it
is not or where the condition is a programming bug rather than
on-disk corruption.
Signed-off-by: Adarsh Das <adarshdas950@xxxxxxxxx>
---
fs/btrfs/extent-tree.c | 82 ++++++++++++++++++++++++++++++++----------
1 file changed, 63 insertions(+), 19 deletions(-)
diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index 03cf9f242c70..9748a4c5bc2d 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -604,7 +604,13 @@ static noinline int remove_extent_data_ref(struct btrfs_trans_handle *trans,
return -EUCLEAN;
}
- BUG_ON(num_refs < refs_to_drop);
+ if (unlikely(num_refs < refs_to_drop)) {
+ btrfs_err(trans->fs_info,
+ "dropping more refs than available, have %u want %u",
+ num_refs, refs_to_drop);
+ btrfs_abort_transaction(trans, -EUCLEAN);
+ return -EUCLEAN;
+ }
num_refs -= refs_to_drop;
if (num_refs == 0) {
@@ -863,7 +869,13 @@ int lookup_inline_extent_backref(struct btrfs_trans_handle *trans,
if (flags & BTRFS_EXTENT_FLAG_TREE_BLOCK && !skinny_metadata) {
ptr += sizeof(struct btrfs_tree_block_info);
- BUG_ON(ptr > end);
+ if (unlikely(ptr > end)) {
+ btrfs_err(
+ fs_info,
Please put @fs_info in the same line of "btrfs_err(", there is definitely enough space for it.
+ "extent item overflow at slot %d, ptr %lu end %lu",
+ path->slots[0], ptr, end);
+ return -EUCLEAN;
+ }
And I don't think we even need to have this handling.
If the size is not correct, it should be caught by tree-checker first, and that will provide better debug output normally.
Normally it can be replaced by an ASSERT().
}
if (owner >= BTRFS_FIRST_FREE_OBJECTID)
@@ -1237,7 +1249,12 @@ static int remove_extent_backref(struct btrfs_trans_handle *trans,
{
int ret = 0;
- BUG_ON(!is_data && refs_to_drop != 1);
+ if (unlikely(!is_data && refs_to_drop != 1)) {
+ btrfs_err(trans->fs_info,
+ "invalid refs_to_drop %d for tree block, must be 1",
+ refs_to_drop);
+ return -EUCLEAN;
+ }
This is more like a runtime sanity checks, and we do not want it to be just ignored (although it will also flips the fs RO and noisy enough for most cases).
When this happens it's definitely some logical not correct.
I'd prefer it to be changed to ASSERT().
if (iref)
ret = update_inline_extent_backref(trans, path, iref,
-refs_to_drop, NULL);
@@ -1453,8 +1470,9 @@ int btrfs_inc_extent_ref(struct btrfs_trans_handle *trans,
ASSERT(generic_ref->type != BTRFS_REF_NOT_SET &&
generic_ref->action);
- BUG_ON(generic_ref->type == BTRFS_REF_METADATA &&
- generic_ref->ref_root == BTRFS_TREE_LOG_OBJECTID);
+ if (WARN_ON(generic_ref->type == BTRFS_REF_METADATA &&
+ generic_ref->ref_root == BTRFS_TREE_LOG_OBJECTID))
+ return -EINVAL;
Again it's a wrong logic, ASSERT() would be enough, especially the previous check is also an ASSERT().
if (generic_ref->type == BTRFS_REF_METADATA)
ret = btrfs_add_delayed_tree_ref(trans, generic_ref, NULL);
@@ -1622,7 +1640,9 @@ static int run_delayed_data_ref(struct btrfs_trans_handle *trans,
} else if (node->action == BTRFS_DROP_DELAYED_REF) {
ret = __btrfs_free_extent(trans, href, node, extent_op);
} else {
- BUG();
+ btrfs_err(trans->fs_info, "unexpected delayed ref action %d",
+ node->action);
+ return -EUCLEAN;
There is already an ASSERT() in add_delayed_data_ref(), you can enhance that check and replace the BUG() with ASSERT().
}
return ret;
}
@@ -1639,7 +1659,8 @@ static void __run_delayed_extent_op(struct btrfs_delayed_extent_op *extent_op,
if (extent_op->update_key) {
struct btrfs_tree_block_info *bi;
- BUG_ON(!(flags & BTRFS_EXTENT_FLAG_TREE_BLOCK));
+ if (WARN_ON(!(flags & BTRFS_EXTENT_FLAG_TREE_BLOCK)))
+ return;
You can grab the fs_info from @leaf.
And I don't like just pure warning in this case.
You can enhance __run_delayed_extent_op() to return an error code.
Furthermore you can fix the code style problem here, no need for a dedicated patch just to fix those minor new line problems.
As it's already mentioned in our guideline:
https://btrfs.readthedocs.io/en/latest/dev/Developer-s-FAQ.html#how-not-to-start
>> If you care so much about the whitespace/style issues, just fix them while doing a useful change as mentioned above that happens to touch the same code.
bi = (struct btrfs_tree_block_info *)(ei + 1);
btrfs_set_tree_block_key(leaf, bi, &extent_op->key);
}
@@ -1775,7 +1796,9 @@ static int run_delayed_tree_ref(struct btrfs_trans_handle *trans,
else
ret = __btrfs_free_extent(trans, href, node, extent_op);
} else {
- BUG();
+ btrfs_err(trans->fs_info, "unexpected delayed ref action %d",
+ node->action);
+ return -EUCLEAN;
The same, do more validation at insert time, the earlier detection the more usefulness it provides.
}
return ret;
}
@@ -2645,7 +2668,11 @@ int btrfs_pin_extent(struct btrfs_trans_handle *trans, u64 bytenr, u64 num_bytes
struct btrfs_block_group *cache;
cache = btrfs_lookup_block_group(trans->fs_info, bytenr);
- BUG_ON(!cache); /* Logic error */
+ if (unlikely(!cache)) {
+ btrfs_err(trans->fs_info,
+ "unable to find block group for bytenr %llu", bytenr);
+ return -EUCLEAN;
+ }
pin_down_extent(trans, cache, bytenr, num_bytes, true);
@@ -4125,7 +4152,9 @@ static int do_allocation(struct btrfs_block_group *block_group,
case BTRFS_EXTENT_ALLOC_ZONED:
return do_allocation_zoned(block_group, ffe_ctl, bg_ret);
default:
- BUG();
+ btrfs_err(block_group->fs_info, "invalid allocation policy %d",
+ ffe_ctl->policy);
+ return -EUCLEAN;
ASSERT() is more suitable here, the policy is not from on-disk data but purely from runtime code.
Unknown policy means some code is passing wrong numbers.
ASSERT() is more suitable.
This error message vs ASSERT() applies to all the remaining changes.
Thanks,
Qu
}
}
@@ -4141,11 +4170,20 @@ static void release_block_group(struct btrfs_block_group *block_group,
/* Nothing to do */
break;
default:
- BUG();
+ btrfs_err(block_group->fs_info, "invalid allocation policy %d",
+ ffe_ctl->policy);
+ goto release;
+ }
+
+ if (unlikely(btrfs_bg_flags_to_raid_index(block_group->flags) !=
+ ffe_ctl->index)) {
+ btrfs_err(block_group->fs_info,
+ "mismatched raid index, block group flags %llu index %d",
+ block_group->flags, ffe_ctl->index);
+ goto release;
}
- BUG_ON(btrfs_bg_flags_to_raid_index(block_group->flags) !=
- ffe_ctl->index);
+release:
btrfs_release_block_group(block_group, delalloc);
}
@@ -4172,7 +4210,8 @@ static void found_extent(struct find_free_extent_ctl *ffe_ctl,
/* Nothing to do */
break;
default:
- BUG();
+ WARN_ONCE(1, "invalid allocation policy %d", ffe_ctl->policy);
+ return;
}
}
@@ -4238,7 +4277,9 @@ static int can_allocate_chunk(struct btrfs_fs_info *fs_info,
case BTRFS_EXTENT_ALLOC_ZONED:
return can_allocate_chunk_zoned(fs_info, ffe_ctl);
default:
- BUG();
+ btrfs_err(fs_info, "invalid allocation policy %d",
+ ffe_ctl->policy);
+ return -EUCLEAN;
}
}
@@ -4448,7 +4489,9 @@ static int prepare_allocation(struct btrfs_fs_info *fs_info,
case BTRFS_EXTENT_ALLOC_ZONED:
return prepare_allocation_zoned(fs_info, ffe_ctl, space_info);
default:
- BUG();
+ btrfs_err(fs_info, "invalid allocation policy %d",
+ ffe_ctl->policy);
+ return -EUCLEAN;
}
}
@@ -5292,8 +5335,8 @@ struct extent_buffer *btrfs_alloc_tree_block(struct btrfs_trans_handle *trans,
parent = ins.objectid;
flags |= BTRFS_BLOCK_FLAG_FULL_BACKREF;
owning_root = reloc_src_root;
- } else
- BUG_ON(parent > 0);
+ } else if (WARN_ON(parent > 0))
+ return ERR_PTR(-EINVAL);
if (root_objectid != BTRFS_TREE_LOG_OBJECTID) {
struct btrfs_delayed_extent_op *extent_op;
@@ -6437,7 +6480,8 @@ int btrfs_drop_subtree(struct btrfs_trans_handle *trans,
int parent_level;
int ret = 0;
- BUG_ON(btrfs_root_id(root) != BTRFS_TREE_RELOC_OBJECTID);
+ if (WARN_ON(btrfs_root_id(root) != BTRFS_TREE_RELOC_OBJECTID))
+ return -EINVAL;
path = btrfs_alloc_path();
if (!path)