Re: [PATCH v2 2/2] btrfs: replace BUG() and BUG_ON() with error handling in extent-tree.c

[Qu Wenruo <wqu@suse.com>]

在 2026/2/28 19:36, Adarsh Das 写道:
> v2:
> - use ASSERT() instead of btrfs_err() + -EUCLEAN
> - append ASSERTs in btrfs_add_delayed_data_ref() and btrfs_add_delayed_tree_ref() to validate action at insertion time instead of runtime
> - fold coding style fixes into this patch

The same problem as patch 1.

> 
> Signed-off-by: Adarsh Das <adarshdas950@gmail.com>
> ---
>   fs/btrfs/delayed-ref.c |  8 ++++--
>   fs/btrfs/extent-tree.c | 62 ++++++++++++++++++++----------------------
>   2 files changed, 36 insertions(+), 34 deletions(-)
> 
> diff --git a/fs/btrfs/delayed-ref.c b/fs/btrfs/delayed-ref.c
> index 3766ff29fbbb..d308c70228af 100644
> --- a/fs/btrfs/delayed-ref.c
> +++ b/fs/btrfs/delayed-ref.c
> @@ -1113,7 +1113,9 @@ int btrfs_add_delayed_tree_ref(struct btrfs_trans_handle *trans,
>   			       struct btrfs_ref *generic_ref,
>   			       struct btrfs_delayed_extent_op *extent_op)
>   {
> -	ASSERT(generic_ref->type == BTRFS_REF_METADATA && generic_ref->action);
> +	ASSERT(generic_ref->type == BTRFS_REF_METADATA &&
> +	       (generic_ref->action == BTRFS_ADD_DELAYED_REF ||
> +					generic_ref->action == BTRFS_DROP_DELAYED_REF));
>   	return add_delayed_ref(trans, generic_ref, extent_op, 0);
>   }
>   
> @@ -1124,7 +1126,9 @@ int btrfs_add_delayed_data_ref(struct btrfs_trans_handle *trans,
>   			       struct btrfs_ref *generic_ref,
>   			       u64 reserved)
>   {
> -	ASSERT(generic_ref->type == BTRFS_REF_DATA && generic_ref->action);
> +	ASSERT(generic_ref->type == BTRFS_REF_DATA &&
> +	       (generic_ref->action == BTRFS_ADD_DELAYED_REF ||
> +	        generic_ref->action == BTRFS_DROP_DELAYED_REF));
>   	return add_delayed_ref(trans, generic_ref, NULL, reserved);
>   }
>   
> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
> index 03cf9f242c70..98bdf51774c4 100644
> --- a/fs/btrfs/extent-tree.c
> +++ b/fs/btrfs/extent-tree.c
> @@ -604,7 +604,7 @@ static noinline int remove_extent_data_ref(struct btrfs_trans_handle *trans,
>   		return -EUCLEAN;
>   	}
>   
> -	BUG_ON(num_refs < refs_to_drop);
> +	ASSERT(num_refs >= refs_to_drop);
>   	num_refs -= refs_to_drop;
>   
>   	if (num_refs == 0) {
> @@ -863,7 +863,7 @@ 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);
> +		ASSERT(ptr <= end);
>   	}
>   
>   	if (owner >= BTRFS_FIRST_FREE_OBJECTID)
> @@ -1237,7 +1237,7 @@ static int remove_extent_backref(struct btrfs_trans_handle *trans,
>   {
>   	int ret = 0;
>   
> -	BUG_ON(!is_data && refs_to_drop != 1);
> +	ASSERT(is_data || refs_to_drop == 1);
>   	if (iref)
>   		ret = update_inline_extent_backref(trans, path, iref,
>   						   -refs_to_drop, NULL);
> @@ -1451,10 +1451,9 @@ int btrfs_inc_extent_ref(struct btrfs_trans_handle *trans,
>   	struct btrfs_fs_info *fs_info = trans->fs_info;
>   	int ret;
>   
> -	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);
> +	ASSERT(generic_ref->type != BTRFS_REF_NOT_SET && generic_ref->action);
> +	ASSERT(generic_ref->type != BTRFS_REF_METADATA ||
> +	       generic_ref->ref_root != BTRFS_TREE_LOG_OBJECTID);
>   
>   	if (generic_ref->type == BTRFS_REF_METADATA)
>   		ret = btrfs_add_delayed_tree_ref(trans, generic_ref, NULL);
> @@ -1621,8 +1620,6 @@ static int run_delayed_data_ref(struct btrfs_trans_handle *trans,
>   		ret = __btrfs_inc_extent_ref(trans, node, extent_op);
>   	} else if (node->action == BTRFS_DROP_DELAYED_REF) {
>   		ret = __btrfs_free_extent(trans, href, node, extent_op);
> -	} else {
> -		BUG();
>   	}
>   	return ret;
>   }
> @@ -1639,7 +1636,7 @@ 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));
> +		ASSERT(flags & BTRFS_EXTENT_FLAG_TREE_BLOCK);
>   		bi = (struct btrfs_tree_block_info *)(ei + 1);
>   		btrfs_set_tree_block_key(leaf, bi, &extent_op->key);
>   	}
> @@ -1774,8 +1771,6 @@ static int run_delayed_tree_ref(struct btrfs_trans_handle *trans,
>   			ret = drop_remap_tree_ref(trans, node);
>   		else
>   			ret = __btrfs_free_extent(trans, href, node, extent_op);
> -	} else {
> -		BUG();
>   	}
>   	return ret;
>   }
> @@ -2088,7 +2083,7 @@ static noinline int __btrfs_run_delayed_refs(struct btrfs_trans_handle *trans,
>   			 * head
>   			 */
>   			ret = cleanup_ref_head(trans, locked_ref, &bytes_processed);
> -			if (ret > 0 ) {
> +			if (ret > 0) {
>   				/* We dropped our lock, we need to loop. */
>   				ret = 0;
>   				continue;
> @@ -2645,7 +2640,7 @@ 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 */
> +	ASSERT(cache);
>   
>   	pin_down_extent(trans, cache, bytenr, num_bytes, true);
>   
> @@ -4119,20 +4114,25 @@ static int do_allocation(struct btrfs_block_group *block_group,
>   			 struct find_free_extent_ctl *ffe_ctl,
>   			 struct btrfs_block_group **bg_ret)
>   {
> +	ASSERT(ffe_ctl->policy == BTRFS_EXTENT_ALLOC_CLUSTERED ||
> +	       ffe_ctl->policy == BTRFS_EXTENT_ALLOC_ZONED);
>   	switch (ffe_ctl->policy) {
>   	case BTRFS_EXTENT_ALLOC_CLUSTERED:
>   		return do_allocation_clustered(block_group, ffe_ctl, bg_ret);
>   	case BTRFS_EXTENT_ALLOC_ZONED:
>   		return do_allocation_zoned(block_group, ffe_ctl, bg_ret);
> -	default:
> -		BUG();
>   	}
> +	return -EUCLEAN;
>   }
>   
>   static void release_block_group(struct btrfs_block_group *block_group,
>   				struct find_free_extent_ctl *ffe_ctl,
>   				bool delalloc)
>   {
> +	ASSERT(btrfs_bg_flags_to_raid_index(block_group->flags) ==
> +	       ffe_ctl->index);
> +	ASSERT(ffe_ctl->policy == BTRFS_EXTENT_ALLOC_CLUSTERED ||
> +	       ffe_ctl->policy == BTRFS_EXTENT_ALLOC_ZONED);
>   	switch (ffe_ctl->policy) {
>   	case BTRFS_EXTENT_ALLOC_CLUSTERED:
>   		ffe_ctl->retry_uncached = false;
> @@ -4140,12 +4140,8 @@ static void release_block_group(struct btrfs_block_group *block_group,
>   	case BTRFS_EXTENT_ALLOC_ZONED:
>   		/* Nothing to do */
>   		break;
> -	default:
> -		BUG();
>   	}
>   
> -	BUG_ON(btrfs_bg_flags_to_raid_index(block_group->flags) !=
> -	       ffe_ctl->index);
>   	btrfs_release_block_group(block_group, delalloc);
>   }
>   
> @@ -4164,6 +4160,8 @@ static void found_extent_clustered(struct find_free_extent_ctl *ffe_ctl,
>   static void found_extent(struct find_free_extent_ctl *ffe_ctl,
>   			 struct btrfs_key *ins)
>   {
> +	ASSERT(ffe_ctl->policy == BTRFS_EXTENT_ALLOC_CLUSTERED ||
> +	       ffe_ctl->policy == BTRFS_EXTENT_ALLOC_ZONED);
>   	switch (ffe_ctl->policy) {
>   	case BTRFS_EXTENT_ALLOC_CLUSTERED:
>   		found_extent_clustered(ffe_ctl, ins);
> @@ -4171,8 +4169,6 @@ static void found_extent(struct find_free_extent_ctl *ffe_ctl,
>   	case BTRFS_EXTENT_ALLOC_ZONED:
>   		/* Nothing to do */
>   		break;
> -	default:
> -		BUG();
>   	}
>   }
>   
> @@ -4232,14 +4228,15 @@ static int can_allocate_chunk_zoned(struct btrfs_fs_info *fs_info,
>   static int can_allocate_chunk(struct btrfs_fs_info *fs_info,
>   			      struct find_free_extent_ctl *ffe_ctl)
>   {
> +	ASSERT(ffe_ctl->policy == BTRFS_EXTENT_ALLOC_CLUSTERED ||
> +	       ffe_ctl->policy == BTRFS_EXTENT_ALLOC_ZONED);
>   	switch (ffe_ctl->policy) {
>   	case BTRFS_EXTENT_ALLOC_CLUSTERED:
>   		return 0;
>   	case BTRFS_EXTENT_ALLOC_ZONED:
>   		return can_allocate_chunk_zoned(fs_info, ffe_ctl);
> -	default:
> -		BUG();
>   	}
> +	return -EUCLEAN;
>   }
>   
>   /*
> @@ -4310,8 +4307,7 @@ static int find_free_extent_update_loop(struct btrfs_fs_info *fs_info,
>   			if (ret == -ENOSPC) {
>   				ret = 0;
>   				ffe_ctl->loop++;
> -			}
> -			else if (ret < 0)
> +			} else if (ret < 0)
>   				btrfs_abort_transaction(trans, ret);
>   			else
>   				ret = 0;
> @@ -4441,15 +4437,16 @@ static int prepare_allocation(struct btrfs_fs_info *fs_info,
>   			      struct btrfs_space_info *space_info,
>   			      struct btrfs_key *ins)
>   {
> +	ASSERT(ffe_ctl->policy == BTRFS_EXTENT_ALLOC_CLUSTERED ||
> +	       ffe_ctl->policy == BTRFS_EXTENT_ALLOC_ZONED);
>   	switch (ffe_ctl->policy) {
>   	case BTRFS_EXTENT_ALLOC_CLUSTERED:
>   		return prepare_allocation_clustered(fs_info, ffe_ctl,
>   						    space_info, ins);
>   	case BTRFS_EXTENT_ALLOC_ZONED:
>   		return prepare_allocation_zoned(fs_info, ffe_ctl, space_info);
> -	default:
> -		BUG();
>   	}
> +	return -EUCLEAN;
>   }
>   
>   /*
> @@ -5260,6 +5257,8 @@ struct extent_buffer *btrfs_alloc_tree_block(struct btrfs_trans_handle *trans,
>   	bool skinny_metadata = btrfs_fs_incompat(fs_info, SKINNY_METADATA);
>   	u64 owning_root;
>   
> +	ASSERT(parent <= 0);
> +
>   #ifdef CONFIG_BTRFS_FS_RUN_SANITY_TESTS
>   	if (btrfs_is_testing(fs_info)) {
>   		buf = btrfs_init_new_buffer(trans, root, root->alloc_bytenr,
> @@ -5292,8 +5291,7 @@ 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);
> +	}
>   
>   	if (root_objectid != BTRFS_TREE_LOG_OBJECTID) {
>   		struct btrfs_delayed_extent_op *extent_op;
> @@ -5633,7 +5631,7 @@ static int check_ref_exists(struct btrfs_trans_handle *trans,
>   		 * If we get 0 then we found our reference, return 1, else
>   		 * return the error if it's not -ENOENT;
>   		 */
> -		return (ret < 0 ) ? ret : 1;
> +		return (ret < 0) ? ret : 1;
>   	}
>   
>   	/*
> @@ -6437,7 +6435,7 @@ 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);
> +	ASSERT(btrfs_root_id(root) == BTRFS_TREE_RELOC_OBJECTID);
>   
>   	path = btrfs_alloc_path();
>   	if (!path)