[PATCH 00/14] More error handling and BUG_ON cleanups

[PATCH 00/14] More error handling and BUG_ON cleanups

[David Sterba]

Continuation of https://lore.kernel.org/linux-btrfs/cover.1706130791.git.dsterba@suse.com/
BUG_ON converted to error handling, removed or moved.

David Sterba (14):
  btrfs: push errors up from add_async_extent()
  btrfs: update comment and drop assertion in extent item lookup in
    find_parent_nodes()
  btrfs: handle invalid extent item reference found in
    extent_from_logical()
  btrfs: handle invalid extent item reference found in
    find_first_extent_item()
  btrfs: handle invalid root reference found in may_destroy_subvol()
  btrfs: send: handle unexpected data in header buffer in begin_cmd()
  btrfs: send: handle unexpected inode in header process_recorded_refs()
  btrfs: send: handle path ref underflow in header iterate_inode_ref()
  btrfs: change BUG_ON to assertion in tree_move_down()
  btrfs: change BUG_ONs to assertions in btrfs_qgroup_trace_subtree()
  btrfs: delete pointless BUG_ON check on quota root in
    btrfs_qgroup_account_extent()
  btrfs: delete pointless BUG_ONs on extent item size
  btrfs: handle unexpected parent block offset in
    btrfs_alloc_tree_block()
  btrfs: delete BUG_ON in btrfs_init_locked_inode()

 fs/btrfs/backref.c     | 20 +++++++++++++++-----
 fs/btrfs/extent-tree.c | 12 ++++++++++--
 fs/btrfs/inode.c       | 23 ++++++++++++++++-------
 fs/btrfs/qgroup.c      |  6 ++----
 fs/btrfs/scrub.c       |  9 ++++++++-
 fs/btrfs/send.c        | 27 +++++++++++++++++++++++----
 6 files changed, 74 insertions(+), 23 deletions(-)

-- 
2.42.1

[PATCH 01/14] btrfs: push errors up from add_async_extent()

[David Sterba]

The memory allocation error in add_async_extent() is not handled
properly, return an error and push the BUG_ON to the caller. Handling it
there is not trivial so at least make it visible.

Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/inode.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index adf11936a47e..311d252addaf 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -738,7 +738,8 @@ static noinline int add_async_extent(struct async_chunk *cow,
 	struct async_extent *async_extent;
 
 	async_extent = kmalloc(sizeof(*async_extent), GFP_NOFS);
-	BUG_ON(!async_extent); /* -ENOMEM */
+	if (!async_extent)
+		return -ENOMEM;
 	async_extent->start = start;
 	async_extent->ram_size = ram_size;
 	async_extent->compressed_size = compressed_size;
@@ -1025,8 +1026,9 @@ static void compress_file_range(struct btrfs_work *work)
 	 * The async work queues will take care of doing actual allocation on
 	 * disk for these compressed pages, and will submit the bios.
 	 */
-	add_async_extent(async_chunk, start, total_in, total_compressed, pages,
-			 nr_pages, compress_type);
+	ret = add_async_extent(async_chunk, start, total_in, total_compressed, pages,
+			       nr_pages, compress_type);
+	BUG_ON(ret);
 	if (start + total_in < end) {
 		start += total_in;
 		cond_resched();
@@ -1038,8 +1040,9 @@ static void compress_file_range(struct btrfs_work *work)
 	if (!btrfs_test_opt(fs_info, FORCE_COMPRESS) && !inode->prop_compress)
 		inode->flags |= BTRFS_INODE_NOCOMPRESS;
 cleanup_and_bail_uncompressed:
-	add_async_extent(async_chunk, start, end - start + 1, 0, NULL, 0,
-			 BTRFS_COMPRESS_NONE);
+	ret = add_async_extent(async_chunk, start, end - start + 1, 0, NULL, 0,
+			       BTRFS_COMPRESS_NONE);
+	BUG_ON(ret);
 free_pages:
 	if (pages) {
 		for (i = 0; i < nr_pages; i++) {
-- 
2.42.1

[PATCH 02/14] btrfs: update comment and drop assertion in extent item lookup in find_parent_nodes()

[David Sterba]

Same comment was added to this type of error, unify that and drop the
assertion as we'd find out quickly that something is wrong after
returning -EUCLEAN.

Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/backref.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/fs/btrfs/backref.c b/fs/btrfs/backref.c
index beed7e459dab..0fa27ed802f6 100644
--- a/fs/btrfs/backref.c
+++ b/fs/btrfs/backref.c
@@ -1435,8 +1435,10 @@ static int find_parent_nodes(struct btrfs_backref_walk_ctx *ctx,
 	if (ret < 0)
 		goto out;
 	if (ret == 0) {
-		/* This shouldn't happen, indicates a bug or fs corruption. */
-		ASSERT(ret != 0);
+		/*
+		 * Key with offset -1 found, there would have to exist an extent
+		 * item with such offset, but this is out of the valid range.
+		 */
 		ret = -EUCLEAN;
 		goto out;
 	}
-- 
2.42.1

[PATCH 03/14] btrfs: handle invalid extent item reference found in extent_from_logical()

[David Sterba]

The extent_from_logical() helper looks up an extent item by a key,
allowing to do an inexact search when key->offset is -1.  It's never
expected to find such item, as it would break the allowed range of a
extent item offset.

The same error is already handled in btrfs_backref_iter_start() so add a
comment for consistency.

Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/backref.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/fs/btrfs/backref.c b/fs/btrfs/backref.c
index 0fa27ed802f6..6ba743ddfe21 100644
--- a/fs/btrfs/backref.c
+++ b/fs/btrfs/backref.c
@@ -2227,6 +2227,13 @@ int extent_from_logical(struct btrfs_fs_info *fs_info, u64 logical,
 	ret = btrfs_search_slot(NULL, extent_root, &key, path, 0, 0);
 	if (ret < 0)
 		return ret;
+	if (ret == 0) {
+		/*
+		 * Key with offset -1 found, there would have to exist an extent
+		 * item with such offset, but this is out of the valid range.
+		 */
+		return -EUCLEAN;
+	}
 
 	ret = btrfs_previous_extent_item(extent_root, path, 0);
 	if (ret) {
@@ -2870,6 +2877,10 @@ int btrfs_backref_iter_start(struct btrfs_backref_iter *iter, u64 bytenr)
 	if (ret < 0)
 		return ret;
 	if (ret == 0) {
+		/*
+		 * Key with offset -1 found, there would have to exist an extent
+		 * item with such offset, but this is out of the valid range.
+		 */
 		ret = -EUCLEAN;
 		goto release;
 	}
-- 
2.42.1

[PATCH 04/14] btrfs: handle invalid extent item reference found in find_first_extent_item()

[David Sterba]

The find_first_extent_item() helper looks up an extent item by a key,
allowing to do an inexact search when key->offset is -1.  It's never
expected to find such item, as it would break the allowed range of a
extent item offset.

Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/scrub.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/fs/btrfs/scrub.c b/fs/btrfs/scrub.c
index 0123d2728923..c4bd0e60db59 100644
--- a/fs/btrfs/scrub.c
+++ b/fs/btrfs/scrub.c
@@ -1390,8 +1390,15 @@ static int find_first_extent_item(struct btrfs_root *extent_root,
 	ret = btrfs_search_slot(NULL, extent_root, &key, path, 0, 0);
 	if (ret < 0)
 		return ret;
+	if (ret == 0) {
+		/*
+		 * Key with offset -1 found, there would have to exist an extent
+		 * item with such offset, but this is out of the valid range.
+		 */
+		btrfs_release_path(path);
+		return -EUCLEAN;
+	}
 
-	ASSERT(ret > 0);
 	/*
 	 * Here we intentionally pass 0 as @min_objectid, as there could be
 	 * an extent item starting before @search_start.
-- 
2.42.1

[PATCH 05/14] btrfs: handle invalid root reference found in may_destroy_subvol()

[David Sterba]

The may_destroy_subvol() looks up a root by a key, allowing to do an
inexact search when key->offset is -1.  It's never expected to find such
item, as it would break the allowed range of a root id.

Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/inode.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 311d252addaf..0b36dfb6754b 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -4389,7 +4389,14 @@ static noinline int may_destroy_subvol(struct btrfs_root *root)
 	ret = btrfs_search_slot(NULL, fs_info->tree_root, &key, path, 0, 0);
 	if (ret < 0)
 		goto out;
-	BUG_ON(ret == 0);
+	if (ret == 0) {
+		/*
+		 * Key with offset -1 found, there would have to exist a root
+		 * with such id, but this is out of valid range.
+		 */
+		ret = -EUCLEAN;
+		goto out;
+	}
 
 	ret = 0;
 	if (path->slots[0] > 0) {
-- 
2.42.1

[PATCH 06/14] btrfs: send: handle unexpected data in header buffer in begin_cmd()

[David Sterba]

Change BUG_ON to a proper error handling in the unlikely case of seeing
data when the command is started. This is supposed to be reset when the
command is finished (send_cmd, send_encoded_extent).

Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/send.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/fs/btrfs/send.c b/fs/btrfs/send.c
index 14ea30850739..1bff7b3008ac 100644
--- a/fs/btrfs/send.c
+++ b/fs/btrfs/send.c
@@ -776,7 +776,12 @@ static int begin_cmd(struct send_ctx *sctx, int cmd)
 	if (WARN_ON(!sctx->send_buf))
 		return -EINVAL;
 
-	BUG_ON(sctx->send_size);
+	if (unlikely(sctx->send_size != 0)) {
+		btrfs_err(sctx->send_root->fs_info,
+			  "send: command header buffer not empty cmd %d offset %llu",
+			  cmd, sctx->send_off);
+		return -EINVAL;
+	}
 
 	sctx->send_size += sizeof(*hdr);
 	hdr = (struct btrfs_cmd_header *)sctx->send_buf;
-- 
2.42.1

[PATCH 07/14] btrfs: send: handle unexpected inode in header process_recorded_refs()

[David Sterba]

Change BUG_ON to proper error handling when an unexpected inode number
is encountered. As the comment says this should never happen.

Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/send.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/fs/btrfs/send.c b/fs/btrfs/send.c
index 1bff7b3008ac..778c2da1c9dd 100644
--- a/fs/btrfs/send.c
+++ b/fs/btrfs/send.c
@@ -4186,7 +4186,13 @@ static int process_recorded_refs(struct send_ctx *sctx, int *pending_move)
 	 * This should never happen as the root dir always has the same ref
 	 * which is always '..'
 	 */
-	BUG_ON(sctx->cur_ino <= BTRFS_FIRST_FREE_OBJECTID);
+	if (unlikely(sctx->cur_ino <= BTRFS_FIRST_FREE_OBJECTID)) {
+		btrfs_err(fs_info,
+			  "send: unexpected inode %llu in process_recorded_refs()",
+			  sctx->cur_ino);
+		ret = -EINVAL;
+		goto out;
+	}
 
 	valid_path = fs_path_alloc();
 	if (!valid_path) {
-- 
2.42.1

[PATCH 08/14] btrfs: send: handle path ref underflow in header iterate_inode_ref()

[David Sterba]

Change BUG_ON to proper error handling if building the path buffer
fails. The pointers are not printed so we don't accidentally leak kernel
addresses.

Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/send.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/fs/btrfs/send.c b/fs/btrfs/send.c
index 778c2da1c9dd..7a601de7fa7c 100644
--- a/fs/btrfs/send.c
+++ b/fs/btrfs/send.c
@@ -1074,7 +1074,15 @@ static int iterate_inode_ref(struct btrfs_root *root, struct btrfs_path *path,
 					ret = PTR_ERR(start);
 					goto out;
 				}
-				BUG_ON(start < p->buf);
+				if (unlikely(start < p->buf)) {
+					btrfs_err(root->fs_info,
+			"send: path ref buffer underflow for key (%llu %u %llu)",
+						  found_key->objectid,
+						  found_key->type,
+						  found_key->offset);
+					ret = -EINVAL;
+					goto out;
+				}
 			}
 			p->start = start;
 		} else {
-- 
2.42.1

[PATCH 09/14] btrfs: change BUG_ON to assertion in tree_move_down()

[David Sterba]

There's only one caller of tree_move_down() that does not pass level 0
so the assertion is better suited here.

Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/send.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/btrfs/send.c b/fs/btrfs/send.c
index 7a601de7fa7c..e96d511f9dd9 100644
--- a/fs/btrfs/send.c
+++ b/fs/btrfs/send.c
@@ -7438,8 +7438,8 @@ static int tree_move_down(struct btrfs_path *path, int *level, u64 reada_min_gen
 	u64 reada_done = 0;
 
 	lockdep_assert_held_read(&parent->fs_info->commit_root_sem);
+	ASSERT(*level != 0);
 
-	BUG_ON(*level == 0);
 	eb = btrfs_read_node_slot(parent, slot);
 	if (IS_ERR(eb))
 		return PTR_ERR(eb);
-- 
2.42.1

[PATCH 10/14] btrfs: change BUG_ONs to assertions in btrfs_qgroup_trace_subtree()

[David Sterba]

The only caller do_walk_down() of btrfs_qgroup_trace_subtree() validates
the value of level and uses it several times before it's passed as an
argument. Same for root_eb that's called 'next' in the caller.

Change both BUG_ONs to assertions as this is to assure proper interface
use rather than real errors.

Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/qgroup.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
index 5470e1cdf10c..cfe366110a69 100644
--- a/fs/btrfs/qgroup.c
+++ b/fs/btrfs/qgroup.c
@@ -2505,8 +2505,8 @@ int btrfs_qgroup_trace_subtree(struct btrfs_trans_handle *trans,
 	struct extent_buffer *eb = root_eb;
 	struct btrfs_path *path = NULL;
 
-	BUG_ON(root_level < 0 || root_level >= BTRFS_MAX_LEVEL);
-	BUG_ON(root_eb == NULL);
+	ASSERT(0 <= root_level && root_level < BTRFS_MAX_LEVEL);
+	ASSERT(root_eb != NULL);
 
 	if (!btrfs_qgroup_full_accounting(fs_info))
 		return 0;
-- 
2.42.1

[PATCH 11/14] btrfs: delete pointless BUG_ON check on quota root in btrfs_qgroup_account_extent()

[David Sterba]

The BUG_ON is deep in the qgroup code where we can expect that it
exists. A NULL pointer would cause a crash.

It was added long ago in 550d7a2ed5db35 ("btrfs: qgroup: Add new qgroup
calculation function btrfs_qgroup_account_extents()."). It maybe made
sense back then as the quota enable/disable state machine was not that
robust as it is nowadays, so we can just delete it.

Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/qgroup.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
index cfe366110a69..044331228bd0 100644
--- a/fs/btrfs/qgroup.c
+++ b/fs/btrfs/qgroup.c
@@ -2861,8 +2861,6 @@ int btrfs_qgroup_account_extent(struct btrfs_trans_handle *trans, u64 bytenr,
 	if (nr_old_roots == 0 && nr_new_roots == 0)
 		goto out_free;
 
-	BUG_ON(!fs_info->quota_root);
-
 	trace_btrfs_qgroup_account_extent(fs_info, trans->transid, bytenr,
 					num_bytes, nr_old_roots, nr_new_roots);
 
-- 
2.42.1

[PATCH 12/14] btrfs: delete pointless BUG_ONs on extent item size

[David Sterba]

Checking extent item size in add_inline_refs() is redundant, we do that
already in tree-checker after reading the extent buffer and it won't
change under normal circumstances.  It was added long ago in
8da6d5815c592b ("Btrfs: added btrfs_find_all_roots()") and does not seem
to have a clear purpose.

Similar case in extent_from_logical(), added in a542ad1bafc7df ("btrfs:
added helper functions to iterate backrefs").

Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/backref.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/fs/btrfs/backref.c b/fs/btrfs/backref.c
index 6ba743ddfe21..fe05e2f55bf7 100644
--- a/fs/btrfs/backref.c
+++ b/fs/btrfs/backref.c
@@ -1036,8 +1036,6 @@ static int add_inline_refs(struct btrfs_backref_walk_ctx *ctx,
 	slot = path->slots[0];
 
 	item_size = btrfs_item_size(leaf, slot);
-	BUG_ON(item_size < sizeof(*ei));
-
 	ei = btrfs_item_ptr(leaf, slot, struct btrfs_extent_item);
 
 	if (ctx->check_extent_item) {
@@ -2256,7 +2254,6 @@ int extent_from_logical(struct btrfs_fs_info *fs_info, u64 logical,
 
 	eb = path->nodes[0];
 	item_size = btrfs_item_size(eb, path->slots[0]);
-	BUG_ON(item_size < sizeof(*ei));
 
 	ei = btrfs_item_ptr(eb, path->slots[0], struct btrfs_extent_item);
 	flags = btrfs_extent_flags(eb, ei);
-- 
2.42.1

[PATCH 13/14] btrfs: handle unexpected parent block offset in btrfs_alloc_tree_block()

[David Sterba]

Change a BUG_ON to a proper error handling, here it checks that a root
other than reloc tree does not see a non-zero offset. This is set by
btrfs_force_cow_block() and is a special case so the check makes sure
it's not accidentally set by other callers.

Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/extent-tree.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index 0d72d0f7cefc..3708f886d21a 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -5187,8 +5187,16 @@ 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 (unlikely(parent > 0)) {
+			/*
+			 * Other roots than reloc tree don't expect start
+			 * offset of a parent block.
+			 */
+			ret = -EUCLEAN;
+			goto out_free_reserved;
+		}
+	}
 
 	if (root_objectid != BTRFS_TREE_LOG_OBJECTID) {
 		extent_op = btrfs_alloc_delayed_extent_op();
-- 
2.42.1

[PATCH 14/14] btrfs: delete BUG_ON in btrfs_init_locked_inode()

[David Sterba]

The purpose of the BUG_ON is not clear. The helper btrfs_grab_root()
could return a NULL in case args->root would be a NULL or if there are
zero references. Then we check if the root pointer stored in the inode
still exists.

The whole call chain is for iget:

btrfs_iget
  btrfs_iget_path
    btrfs_iget_locked
      iget5_locked
	btrfs_init_locked_inode

which is called from many contexts where we the root pointer is used and
we can safely assume has enough references.

Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/inode.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 0b36dfb6754b..459ec9ba06e0 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -5548,7 +5548,6 @@ static int btrfs_init_locked_inode(struct inode *inode, void *p)
 	BTRFS_I(inode)->location.type = BTRFS_INODE_ITEM_KEY;
 	BTRFS_I(inode)->location.offset = 0;
 	BTRFS_I(inode)->root = btrfs_grab_root(args->root);
-	BUG_ON(args->root && !BTRFS_I(inode)->root);
 
 	if (args->root && args->root == args->root->fs_info->tree_root &&
 	    args->ino != BTRFS_BTREE_INODE_OBJECTID)
-- 
2.42.1

Re: [PATCH 00/14] More error handling and BUG_ON cleanups

[Qu Wenruo]

On 2024/2/8 19:29, David Sterba wrote:
> Continuation of https://lore.kernel.org/linux-btrfs/cover.1706130791.git.dsterba@suse.com/
> BUG_ON converted to error handling, removed or moved.
>
> David Sterba (14):
>    btrfs: push errors up from add_async_extent()
>    btrfs: update comment and drop assertion in extent item lookup in
>      find_parent_nodes()
>    btrfs: handle invalid extent item reference found in
>      extent_from_logical()

The above ones look good to me.

>    btrfs: handle invalid extent item reference found in
>      find_first_extent_item()

For bad extent item offsets, I'm totally fine with the current -EUCLEAN
even it doesn't have any error message.
As tree-checker has already verified the extent items, thus it's way
much harder to get runtime corruption to create a -1 key.offset.


>    btrfs: handle invalid root reference found in may_destroy_subvol()

But for this, I strongly recommend a new tree-checker entry for
ROOT_REF, before returning -EUCLEAN without an error message.

>    btrfs: send: handle unexpected data in header buffer in begin_cmd()
>    btrfs: send: handle unexpected inode in header process_recorded_refs()
>    btrfs: send: handle path ref underflow in header iterate_inode_ref()
>    btrfs: change BUG_ON to assertion in tree_move_down()
>    btrfs: change BUG_ONs to assertions in btrfs_qgroup_trace_subtree()
>    btrfs: delete pointless BUG_ON check on quota root in
>      btrfs_qgroup_account_extent()
>    btrfs: delete pointless BUG_ONs on extent item size

The above ones look good to me.

>    btrfs: handle unexpected parent block offset in
>      btrfs_alloc_tree_block()

For this one, I'd prefer one error message or at least something noisy
enough (aka, ASSERT()), just to make life a little easier when we
screwed up in our development environment.

Thanks,
Qu
>    btrfs: delete BUG_ON in btrfs_init_locked_inode()
>
>   fs/btrfs/backref.c     | 20 +++++++++++++++-----
>   fs/btrfs/extent-tree.c | 12 ++++++++++--
>   fs/btrfs/inode.c       | 23 ++++++++++++++++-------
>   fs/btrfs/qgroup.c      |  6 ++----
>   fs/btrfs/scrub.c       |  9 ++++++++-
>   fs/btrfs/send.c        | 27 +++++++++++++++++++++++----
>   6 files changed, 74 insertions(+), 23 deletions(-)
>

Re: [PATCH 00/14] More error handling and BUG_ON cleanups

[David Sterba]

On Thu, Feb 08, 2024 at 08:20:55PM +1030, Qu Wenruo wrote:
> >    btrfs: handle invalid extent item reference found in
> >      find_first_extent_item()
> 
> For bad extent item offsets, I'm totally fine with the current -EUCLEAN
> even it doesn't have any error message.
> As tree-checker has already verified the extent items, thus it's way
> much harder to get runtime corruption to create a -1 key.offset.

Yeah, this is the same as for the previous patchset, tree-checker
catches things early and the new error handling makes sure any
improbable error will not propagate further.

Regarding the messages, I'll add them in the futre, currently I'm
analyzing all cases we might need and prototyping the error/warning
message macros. I've been going over BUG_ONs only but we have too many
random WARN_ONs too, this should be better converted to warnings or
assertions with clear purpose and use case.

> >    btrfs: handle invalid root reference found in may_destroy_subvol()
> 
> But for this, I strongly recommend a new tree-checker entry for
> ROOT_REF, before returning -EUCLEAN without an error message.

Right, we don't seem to have a case for BTRFS_ROOT_REF_KEY in
tree-checker, at least the allowed value range could be there.

> >    btrfs: send: handle unexpected data in header buffer in begin_cmd()
> >    btrfs: send: handle unexpected inode in header process_recorded_refs()
> >    btrfs: send: handle path ref underflow in header iterate_inode_ref()
> >    btrfs: change BUG_ON to assertion in tree_move_down()
> >    btrfs: change BUG_ONs to assertions in btrfs_qgroup_trace_subtree()
> >    btrfs: delete pointless BUG_ON check on quota root in
> >      btrfs_qgroup_account_extent()
> >    btrfs: delete pointless BUG_ONs on extent item size
> 
> The above ones look good to me.
> 
> >    btrfs: handle unexpected parent block offset in
> >      btrfs_alloc_tree_block()
> 
> For this one, I'd prefer one error message or at least something noisy
> enough (aka, ASSERT()), just to make life a little easier when we
> screwed up in our development environment.

Well, there are too many BUG_ONs that somebody just sprinkled over the
code, it still documents the invariants but in a very random way.
Getting rid of them is much harder as it requires reasoning in a much
broader context. I looked at all callers, this looks like a proper API
usage check rather than a random error that would need a message.

Each call site with EUCLEAN can be revisited if we really want to get
noticed about that or not, but I'm not doing that right now unless I'm
convince we do from the context.

Re: [PATCH 00/14] More error handling and BUG_ON cleanups

[David Sterba]

On Thu, Feb 08, 2024 at 08:20:55PM +1030, Qu Wenruo wrote:
> On 2024/2/8 19:29, David Sterba wrote:
> > Continuation of https://lore.kernel.org/linux-btrfs/cover.1706130791.git.dsterba@suse.com/
> >    btrfs: handle unexpected parent block offset in
> >      btrfs_alloc_tree_block()
> 
> For this one, I'd prefer one error message or at least something noisy
> enough (aka, ASSERT()), just to make life a little easier when we
> screwed up in our development environment.

I'll drop this patch for now and will add it to another series dealing
with the error handling and adding the tree-checker and/or verbose
messages.