[PATCH 00/20] Error handling fixes

[PATCH 00/20] Error handling fixes

[David Sterba]

Get rid of some easy BUG_ONs, there are a few groups fixing the same
pattern. At the end there are three patches that move transaction abort
to the right place. I have tested it on my setups only, not the CI, but
given the type of fix we'd never hit any of them without special
instrumentation.

David Sterba (20):
  btrfs: handle directory and dentry mismatch in btrfs_may_delete()
  btrfs: handle invalid range and start in merge_extent_mapping()
  btrfs: handle block group lookup error when it's being removed
  btrfs: handle root deletion lookup error in btrfs_del_root()
  btrfs: handle invalid root reference found in btrfs_find_root()
  btrfs: handle invalid root reference found in btrfs_init_root_free_objectid()
  btrfs: handle chunk tree lookup error in btrfs_relocate_sys_chunks()
  btrfs: handle invalid extent item reference found in check_committed_ref()
  btrfs: export: handle invalid inode or root reference in btrfs_get_parent()
  btrfs: delayed-inode: drop pointless BUG_ON in __btrfs_remove_delayed_item()
  btrfs: change BUG_ON to assertion when checking for delayed_node root
  btrfs: defrag: change BUG_ON to assertion in btrfs_defrag_leaves()
  btrfs: change BUG_ON to assertion in btrfs_read_roots()
  btrfs: change BUG_ON to assertion when verifying lockdep class setup
  btrfs: change BUG_ON to assertion when verifying root in btrfs_alloc_reserved_file_extent()
  btrfs: change BUG_ON to assertion in reset_balance_state()
  btrfs: unify handling of return values of btrfs_insert_empty_items()
  btrfs: move transaction abort to the error site in btrfs_delete_free_space_tree()
  btrfs: move transaction abort to the error site in btrfs_create_free_space_tree()
  btrfs: move transaction abort to the error site btrfs_rebuild_free_space_tree()

 fs/btrfs/block-group.c     |  4 ++-
 fs/btrfs/ctree.c           |  4 +++
 fs/btrfs/defrag.c          |  2 +-
 fs/btrfs/delayed-inode.c   |  4 +--
 fs/btrfs/disk-io.c         | 11 ++++++--
 fs/btrfs/export.c          |  9 +++++-
 fs/btrfs/extent-tree.c     | 11 ++++++--
 fs/btrfs/extent_map.c      |  9 +++---
 fs/btrfs/file-item.c       |  3 --
 fs/btrfs/free-space-tree.c | 56 ++++++++++++++++++++++----------------
 fs/btrfs/ioctl.c           |  4 ++-
 fs/btrfs/locking.c         |  2 +-
 fs/btrfs/root-tree.c       | 16 +++++++++--
 fs/btrfs/uuid-tree.c       |  2 +-
 fs/btrfs/volumes.c         | 14 ++++++++--
 15 files changed, 102 insertions(+), 49 deletions(-)

-- 
2.42.1

[PATCH 01/20] btrfs: handle directory and dentry mismatch in btrfs_may_delete()

[David Sterba]

The helper btrfs_may_delete() is a copy of generic fs/namei.c:may_delete()
to verify various conditions before deletion. There's a BUG_ON added
before linux.git started, we can turn it to a proper error handling
at least in our local helper. A mistmatch between directory and the
deleted dentry is clearly invalid.

This won't be probably ever hit due to the way how the parameters are
set from the caller btrfs_ioctl_snap_destroy(), using a VFS helper
lookup_one().

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

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 3d476decde52..845fdc5f2a99 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -910,7 +910,9 @@ static int btrfs_may_delete(struct mnt_idmap *idmap,
 	if (d_really_is_negative(victim))
 		return -ENOENT;
 
-	BUG_ON(d_inode(victim->d_parent) != dir);
+	/* The @victim is not inside @dir. */
+	if (d_inode(victim->d_parent) != dir)
+		return -EINVAL;
 	audit_inode_child(dir, victim, AUDIT_TYPE_CHILD_DELETE);
 
 	error = inode_permission(idmap, dir, MAY_WRITE | MAY_EXEC);
-- 
2.42.1

[PATCH 02/20] btrfs: handle invalid range and start in merge_extent_mapping()

[David Sterba]

Turn a BUG_ON to a properly handled error and update the error message
in the caller.  It is expected that @em_in and @start passed to
btrfs_add_extent_mapping() overlap. Besides tests, the only caller
btrfs_get_extent() makes sure this is true.

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

diff --git a/fs/btrfs/extent_map.c b/fs/btrfs/extent_map.c
index f170e7122e74..ac5e366d57b2 100644
--- a/fs/btrfs/extent_map.c
+++ b/fs/btrfs/extent_map.c
@@ -539,7 +539,8 @@ static noinline int merge_extent_mapping(struct extent_map_tree *em_tree,
 	u64 end;
 	u64 start_diff;
 
-	BUG_ON(map_start < em->start || map_start >= extent_map_end(em));
+	if (map_start < em->start || map_start >= extent_map_end(em))
+		return -EINVAL;
 
 	if (existing->start > map_start) {
 		next = existing;
@@ -634,9 +635,9 @@ int btrfs_add_extent_mapping(struct btrfs_fs_info *fs_info,
 				free_extent_map(em);
 				*em_in = NULL;
 				WARN_ONCE(ret,
-"unexpected error %d: merge existing(start %llu len %llu) with em(start %llu len %llu)\n",
-					  ret, existing->start, existing->len,
-					  orig_start, orig_len);
+"extent map merge error existing [%llu, %llu) with em [%llu, %llu) start %llu\n",
+					  existing->start, existing->len,
+					  orig_start, orig_len, start);
 			}
 			free_extent_map(existing);
 		}
-- 
2.42.1

[PATCH 03/20] btrfs: handle block group lookup error when it's being removed

[David Sterba]

The unlikely case of lookup error in btrfs_remove_block_group() can be
handled properly, in its caller this would lead to a transaction abort.
We can't do anything else, a block group must have been loaded first.

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

diff --git a/fs/btrfs/block-group.c b/fs/btrfs/block-group.c
index 1905d76772a9..16a2b8609989 100644
--- a/fs/btrfs/block-group.c
+++ b/fs/btrfs/block-group.c
@@ -1063,7 +1063,9 @@ int btrfs_remove_block_group(struct btrfs_trans_handle *trans,
 	bool remove_rsv = false;
 
 	block_group = btrfs_lookup_block_group(fs_info, map->start);
-	BUG_ON(!block_group);
+	if (!block_group)
+		return -ENOENT;
+
 	BUG_ON(!block_group->ro);
 
 	trace_btrfs_remove_block_group(block_group);
-- 
2.42.1

[PATCH 04/20] btrfs: handle root deletion lookup error in btrfs_del_root()

[David Sterba]

We're deleting a root and looking it up by key does not succeed, this
is an inconsistent state and we can't do anything. All callers handle
errors and abort a transaction.

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

diff --git a/fs/btrfs/root-tree.c b/fs/btrfs/root-tree.c
index 603ad1459368..ba7e2181ff4e 100644
--- a/fs/btrfs/root-tree.c
+++ b/fs/btrfs/root-tree.c
@@ -323,8 +323,11 @@ int btrfs_del_root(struct btrfs_trans_handle *trans,
 	ret = btrfs_search_slot(trans, root, key, path, -1, 1);
 	if (ret < 0)
 		goto out;
-
-	BUG_ON(ret != 0);
+	if (ret != 0) {
+		/* The root must exist but we did not find it by the key. */
+		ret = -EUCLEAN;
+		goto out;
+	}
 
 	ret = btrfs_del_item(trans, root, path);
 out:
-- 
2.42.1

[PATCH 05/20] btrfs: handle invalid root reference found in btrfs_find_root()

[David Sterba]

The btrfs_find_root() 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 allowed the range of a root id.

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

diff --git a/fs/btrfs/root-tree.c b/fs/btrfs/root-tree.c
index ba7e2181ff4e..326cd0d03287 100644
--- a/fs/btrfs/root-tree.c
+++ b/fs/btrfs/root-tree.c
@@ -82,7 +82,14 @@ int btrfs_find_root(struct btrfs_root *root, const struct btrfs_key *search_key,
 		if (ret > 0)
 			goto out;
 	} else {
-		BUG_ON(ret == 0);		/* Logical error */
+		/*
+		 * Key with offset -1 found, there would have to exist a root
+		 * with such id, but this is out of the valid range.
+		 */
+		if (ret == 0) {
+			ret = -EUCLEAN;
+			goto out;
+		}
 		if (path->slots[0] == 0)
 			goto out;
 		path->slots[0]--;
-- 
2.42.1

[PATCH 06/20] btrfs: handle invalid root reference found in btrfs_init_root_free_objectid()

[David Sterba]

The btrfs_init_root_free_objectid() 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/disk-io.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 274a3e1faeab..3f7291a48a4d 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -4928,7 +4928,14 @@ int btrfs_init_root_free_objectid(struct btrfs_root *root)
 	ret = btrfs_search_slot(NULL, root, &search_key, path, 0, 0);
 	if (ret < 0)
 		goto error;
-	BUG_ON(ret == 0); /* Corruption */
+	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 error;
+	}
 	if (path->slots[0] > 0) {
 		slot = path->slots[0] - 1;
 		l = path->nodes[0];
-- 
2.42.1

[PATCH 07/20] btrfs: handle chunk tree lookup error in btrfs_relocate_sys_chunks()

[David Sterba]

The unhandled case in btrfs_relocate_sys_chunks() loop is a corruption,
as it could be caused only by two impossible conditions:

- at first the search key is set up to look for a chunk tree item, with
  offset -1, this is an inexact search and the key->offset will contain
  the correct offset upon a successful search, a valid chunk tree item
  cannot have an offset -1

- after first successful search, the found_key corresponds to a chunk
  item, the offset is decremented by 1 before the next loop, it's
  impossible to find a chunk item there due to alignment and size
  constraints

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

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index d67785be2c77..6aae92e4b424 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -3393,7 +3393,17 @@ static int btrfs_relocate_sys_chunks(struct btrfs_fs_info *fs_info)
 			mutex_unlock(&fs_info->reclaim_bgs_lock);
 			goto error;
 		}
-		BUG_ON(ret == 0); /* Corruption */
+		if (ret == 0) {
+			/*
+			 * On the first search we would find chunk tree with
+			 * offset -1, which is not possible. On subsequent
+			 * loops this would find an existing item on an invalid
+			 * offset (one less than the previous one, wrong
+			 * alignment and size).
+			 */
+			ret = -EUCLEAN;
+			goto error;
+		}
 
 		ret = btrfs_previous_item(chunk_root, path, key.objectid,
 					  key.type);
-- 
2.42.1

[PATCH 08/20] btrfs: handle invalid extent item reference found in check_committed_ref()

[David Sterba]

The check_committed_ref() 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/extent-tree.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index 4283e3025ab0..ba47f5996c84 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -2399,7 +2399,14 @@ static noinline int check_committed_ref(struct btrfs_root *root,
 	ret = btrfs_search_slot(NULL, extent_root, &key, path, 0, 0);
 	if (ret < 0)
 		goto out;
-	BUG_ON(ret == 0); /* Corruption */
+	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 out;
+	}
 
 	ret = -ENOENT;
 	if (path->slots[0] == 0)
-- 
2.42.1

[PATCH 09/20] btrfs: export: handle invalid inode or root reference in btrfs_get_parent()

[David Sterba]

The get_parent handler looks up a parent of a given dentry, this can be
either a subvolume or a directory. The search is set up with offset -1
but it's never expected to find such item, as it would break allowed
range of inode number or a root id. This means it's a corruption (ext4
also returns this error code).

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

diff --git a/fs/btrfs/export.c b/fs/btrfs/export.c
index 744a02b7fd67..203e5964c9b0 100644
--- a/fs/btrfs/export.c
+++ b/fs/btrfs/export.c
@@ -174,8 +174,15 @@ struct dentry *btrfs_get_parent(struct dentry *child)
 	ret = btrfs_search_slot(NULL, root, &key, path, 0, 0);
 	if (ret < 0)
 		goto fail;
+	if (ret == 0) {
+		/*
+		 * Key with offset of -1 found, there would have to exist an
+		 * inode with such number or a root with such id.
+		 */
+		ret = -EUCLEAN;
+		goto fail;
+	}
 
-	BUG_ON(ret == 0); /* Key with offset of -1 found */
 	if (path->slots[0] == 0) {
 		ret = -ENOENT;
 		goto fail;
-- 
2.42.1

[PATCH 10/20] btrfs: delayed-inode: drop pointless BUG_ON in __btrfs_remove_delayed_item()

[David Sterba]

There's a BUG_ON checking for a valid pointer of fs_info::delayed_root
but it is valid since init_mount_fs_info() and has the same lifetime as
fs_info.

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

diff --git a/fs/btrfs/delayed-inode.c b/fs/btrfs/delayed-inode.c
index 08102883f560..0b1701f1b8c9 100644
--- a/fs/btrfs/delayed-inode.c
+++ b/fs/btrfs/delayed-inode.c
@@ -430,8 +430,6 @@ static void __btrfs_remove_delayed_item(struct btrfs_delayed_item *delayed_item)
 
 	delayed_root = delayed_node->root->fs_info->delayed_root;
 
-	BUG_ON(!delayed_root);
-
 	if (delayed_item->type == BTRFS_DELAYED_INSERTION_ITEM)
 		root = &delayed_node->ins_root;
 	else
-- 
2.42.1

[PATCH 11/20] btrfs: change BUG_ON to assertion when checking for delayed_node root

[David Sterba]

The pointer to root is initialized in btrfs_init_delayed_node(), no need
to check for it again. Change the BUG_ON to assertion.

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

diff --git a/fs/btrfs/delayed-inode.c b/fs/btrfs/delayed-inode.c
index 0b1701f1b8c9..efe435403b77 100644
--- a/fs/btrfs/delayed-inode.c
+++ b/fs/btrfs/delayed-inode.c
@@ -978,7 +978,7 @@ static void btrfs_release_delayed_inode(struct btrfs_delayed_node *delayed_node)
 
 	if (delayed_node &&
 	    test_bit(BTRFS_DELAYED_NODE_INODE_DIRTY, &delayed_node->flags)) {
-		BUG_ON(!delayed_node->root);
+		ASSERT(delayed_node->root);
 		clear_bit(BTRFS_DELAYED_NODE_INODE_DIRTY, &delayed_node->flags);
 		delayed_node->count--;
 
-- 
2.42.1

[PATCH 12/20] btrfs: defrag: change BUG_ON to assertion in btrfs_defrag_leaves()

[David Sterba]

The BUG_ON verifies a condition that should be guaranteed by the correct
use of the path search (with keep_locks and lowest_level set), an
assertion is the suitable check.

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

diff --git a/fs/btrfs/defrag.c b/fs/btrfs/defrag.c
index dd1b5a060366..77695811f596 100644
--- a/fs/btrfs/defrag.c
+++ b/fs/btrfs/defrag.c
@@ -521,7 +521,7 @@ static int btrfs_defrag_leaves(struct btrfs_trans_handle *trans,
 	 * keep_locks set and lowest_level is 1, regardless of the value of
 	 * path->slots[1].
 	 */
-	BUG_ON(path->locks[1] == 0);
+	ASSERT(path->locks[1] != 0);
 	ret = btrfs_realloc_node(trans, root,
 				 path->nodes[1], 0,
 				 &last_ret,
-- 
2.42.1

[PATCH 13/20] btrfs: change BUG_ON to assertion in btrfs_read_roots()

[David Sterba]

There's one caller of btrfs_read_roots() and that already uses the
tree_root pointer, it's pointless to BUG_ON on it. As it's an assumption
of the initialization helpers make it an assert instead.

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

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 3f7291a48a4d..1477748626ef 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -2240,7 +2240,7 @@ static int btrfs_read_roots(struct btrfs_fs_info *fs_info)
 	struct btrfs_key location;
 	int ret;
 
-	BUG_ON(!fs_info->tree_root);
+	ASSERT(fs_info->tree_root);
 
 	ret = load_global_roots(tree_root);
 	if (ret)
-- 
2.42.1

[PATCH 14/20] btrfs: change BUG_ON to assertion when verifying lockdep class setup

[David Sterba]

The BUG_ON in btrfs_set_buffer_lockdep_class() is a sanity check of the
level which is verified in callers, e.g. when initializing an extent
buffer or reading from an eb header. Change it to an assertion as this
would not happen unless things are really bad and would fail elsewhere
too.

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

diff --git a/fs/btrfs/locking.c b/fs/btrfs/locking.c
index 74d8e2003f58..c1dad121099e 100644
--- a/fs/btrfs/locking.c
+++ b/fs/btrfs/locking.c
@@ -85,7 +85,7 @@ void btrfs_set_buffer_lockdep_class(u64 objectid, struct extent_buffer *eb, int
 {
 	struct btrfs_lockdep_keyset *ks;
 
-	BUG_ON(level >= ARRAY_SIZE(ks->keys));
+	ASSERT(level < ARRAY_SIZE(ks->keys));
 
 	/* Find the matching keyset, id 0 is the default entry */
 	for (ks = btrfs_lockdep_keysets; ks->id; ks++)
-- 
2.42.1

[PATCH 15/20] btrfs: change BUG_ON to assertion when verifying root in btrfs_alloc_reserved_file_extent()

[David Sterba]

The file extents are normally reserved in subvolume roots but could be
also in the data reloc tree. Change the BUG_ON to assertions as this
verifies the usage assumptions.

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

diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index ba47f5996c84..2a04c2083759 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -4964,7 +4964,7 @@ int btrfs_alloc_reserved_file_extent(struct btrfs_trans_handle *trans,
 	u64 root_objectid = root->root_key.objectid;
 	u64 owning_root = root_objectid;
 
-	BUG_ON(root_objectid == BTRFS_TREE_LOG_OBJECTID);
+	ASSERT(root_objectid != BTRFS_TREE_LOG_OBJECTID);
 
 	if (btrfs_is_data_reloc_root(root) && is_fstree(root->relocation_src_root))
 		owning_root = root->relocation_src_root;
-- 
2.42.1

[PATCH 16/20] btrfs: change BUG_ON to assertion in reset_balance_state()

[David Sterba]

The balance state machine is complex so it's good to verify the
assumptions in helpers, however reset_balance_state() is used
at the end of balance and fs_info::balance_ctl is properly set up before
and protected by the exclusive op ownership in btrfs_balance().

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

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 6aae92e4b424..59fc6ab18fef 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -3634,7 +3634,7 @@ static void reset_balance_state(struct btrfs_fs_info *fs_info)
 	struct btrfs_balance_control *bctl = fs_info->balance_ctl;
 	int ret;
 
-	BUG_ON(!fs_info->balance_ctl);
+	ASSERT(fs_info->balance_ctl);
 
 	spin_lock(&fs_info->balance_lock);
 	fs_info->balance_ctl = NULL;
-- 
2.42.1

[PATCH 17/20] btrfs: unify handling of return values of btrfs_insert_empty_items()

[David Sterba]

The error values returned by btrfs_insert_empty_items() are following
the common patter of 0/-errno, but some callers check for a value > 0,
which can't happen. Document that and update calls to not expect
positive values.

Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/ctree.c     | 4 ++++
 fs/btrfs/file-item.c | 3 ---
 fs/btrfs/uuid-tree.c | 2 +-
 3 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
index 33145da449cc..c878ca466b7c 100644
--- a/fs/btrfs/ctree.c
+++ b/fs/btrfs/ctree.c
@@ -4280,6 +4280,10 @@ void btrfs_setup_item_for_insert(struct btrfs_trans_handle *trans,
 /*
  * Given a key and some data, insert items into the tree.
  * This does all the path init required, making room in the tree if needed.
+ *
+ * Returns: 0        on success
+ *          -EEXIST  if the first key already exists
+ *          < 0      on other errors
  */
 int btrfs_insert_empty_items(struct btrfs_trans_handle *trans,
 			    struct btrfs_root *root,
diff --git a/fs/btrfs/file-item.c b/fs/btrfs/file-item.c
index 81ac1d474bf1..8f573c8b5a7a 100644
--- a/fs/btrfs/file-item.c
+++ b/fs/btrfs/file-item.c
@@ -179,7 +179,6 @@ int btrfs_insert_hole_extent(struct btrfs_trans_handle *trans,
 				      sizeof(*item));
 	if (ret < 0)
 		goto out;
-	BUG_ON(ret); /* Can't happen */
 	leaf = path->nodes[0];
 	item = btrfs_item_ptr(leaf, path->slots[0],
 			      struct btrfs_file_extent_item);
@@ -1229,8 +1228,6 @@ int btrfs_csum_file_blocks(struct btrfs_trans_handle *trans,
 				      ins_size);
 	if (ret < 0)
 		goto out;
-	if (WARN_ON(ret != 0))
-		goto out;
 	leaf = path->nodes[0];
 csum:
 	item = btrfs_item_ptr(leaf, path->slots[0], struct btrfs_csum_item);
diff --git a/fs/btrfs/uuid-tree.c b/fs/btrfs/uuid-tree.c
index 5be74f9e47eb..d08511695e94 100644
--- a/fs/btrfs/uuid-tree.c
+++ b/fs/btrfs/uuid-tree.c
@@ -114,7 +114,7 @@ int btrfs_uuid_tree_add(struct btrfs_trans_handle *trans, u8 *uuid, u8 type,
 
 	ret = btrfs_insert_empty_item(trans, uuid_root, path, &key,
 				      sizeof(subid_le));
-	if (ret >= 0) {
+	if (ret == 0) {
 		/* Add an item for the type for the first time */
 		eb = path->nodes[0];
 		slot = path->slots[0];
-- 
2.42.1

[PATCH 18/20] btrfs: move transaction abort to the error site in btrfs_delete_free_space_tree()

[David Sterba]

The recommended pattern for transaction abort after error is to place it
right after the error is handled. That way it's easier to locate where
it failed and help debugging.

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

diff --git a/fs/btrfs/free-space-tree.c b/fs/btrfs/free-space-tree.c
index 7b598b070700..888185265f4b 100644
--- a/fs/btrfs/free-space-tree.c
+++ b/fs/btrfs/free-space-tree.c
@@ -1273,12 +1273,18 @@ int btrfs_delete_free_space_tree(struct btrfs_fs_info *fs_info)
 	btrfs_clear_fs_compat_ro(fs_info, FREE_SPACE_TREE_VALID);
 
 	ret = clear_free_space_tree(trans, free_space_root);
-	if (ret)
-		goto abort;
+	if (ret) {
+		btrfs_abort_transaction(trans, ret);
+		btrfs_end_transaction(trans);
+		return ret;
+	}
 
 	ret = btrfs_del_root(trans, &free_space_root->root_key);
-	if (ret)
-		goto abort;
+	if (ret) {
+		btrfs_abort_transaction(trans, ret);
+		btrfs_end_transaction(trans);
+		return ret;
+	}
 
 	btrfs_global_root_delete(free_space_root);
 
@@ -1295,11 +1301,6 @@ int btrfs_delete_free_space_tree(struct btrfs_fs_info *fs_info)
 	btrfs_put_root(free_space_root);
 
 	return btrfs_commit_transaction(trans);
-
-abort:
-	btrfs_abort_transaction(trans, ret);
-	btrfs_end_transaction(trans);
-	return ret;
 }
 
 int btrfs_rebuild_free_space_tree(struct btrfs_fs_info *fs_info)
-- 
2.42.1

[PATCH 19/20] btrfs: move transaction abort to the error site in btrfs_create_free_space_tree()

[David Sterba]

The recommended pattern for transaction abort after error is to place it
right after the error is handled. That way it's easier to locate where
it failed and help debugging.

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

diff --git a/fs/btrfs/free-space-tree.c b/fs/btrfs/free-space-tree.c
index 888185265f4b..bdc2341c43e4 100644
--- a/fs/btrfs/free-space-tree.c
+++ b/fs/btrfs/free-space-tree.c
@@ -1176,12 +1176,16 @@ int btrfs_create_free_space_tree(struct btrfs_fs_info *fs_info)
 					    BTRFS_FREE_SPACE_TREE_OBJECTID);
 	if (IS_ERR(free_space_root)) {
 		ret = PTR_ERR(free_space_root);
-		goto abort;
+		btrfs_abort_transaction(trans, ret);
+		btrfs_end_transaction(trans);
+		goto out_clear;
 	}
 	ret = btrfs_global_root_insert(free_space_root);
 	if (ret) {
 		btrfs_put_root(free_space_root);
-		goto abort;
+		btrfs_abort_transaction(trans, ret);
+		btrfs_end_transaction(trans);
+		goto out_clear;
 	}
 
 	node = rb_first_cached(&fs_info->block_group_cache_tree);
@@ -1189,8 +1193,11 @@ int btrfs_create_free_space_tree(struct btrfs_fs_info *fs_info)
 		block_group = rb_entry(node, struct btrfs_block_group,
 				       cache_node);
 		ret = populate_free_space_tree(trans, block_group);
-		if (ret)
-			goto abort;
+		if (ret) {
+			btrfs_abort_transaction(trans, ret);
+			btrfs_end_transaction(trans);
+			goto out_clear;
+		}
 		node = rb_next(node);
 	}
 
@@ -1206,11 +1213,9 @@ int btrfs_create_free_space_tree(struct btrfs_fs_info *fs_info)
 	clear_bit(BTRFS_FS_FREE_SPACE_TREE_UNTRUSTED, &fs_info->flags);
 	return ret;
 
-abort:
+out_clear:
 	clear_bit(BTRFS_FS_CREATING_FREE_SPACE_TREE, &fs_info->flags);
 	clear_bit(BTRFS_FS_FREE_SPACE_TREE_UNTRUSTED, &fs_info->flags);
-	btrfs_abort_transaction(trans, ret);
-	btrfs_end_transaction(trans);
 	return ret;
 }
 
-- 
2.42.1

[PATCH 20/20] btrfs: move transaction abort to the error site btrfs_rebuild_free_space_tree()

[David Sterba]

The recommended pattern for transaction abort after error is to place it
right after the error is handled. That way it's easier to locate where
it failed and help debugging.

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

diff --git a/fs/btrfs/free-space-tree.c b/fs/btrfs/free-space-tree.c
index bdc2341c43e4..90f2938bd743 100644
--- a/fs/btrfs/free-space-tree.c
+++ b/fs/btrfs/free-space-tree.c
@@ -1328,8 +1328,11 @@ int btrfs_rebuild_free_space_tree(struct btrfs_fs_info *fs_info)
 	set_bit(BTRFS_FS_FREE_SPACE_TREE_UNTRUSTED, &fs_info->flags);
 
 	ret = clear_free_space_tree(trans, free_space_root);
-	if (ret)
-		goto abort;
+	if (ret) {
+		btrfs_abort_transaction(trans, ret);
+		btrfs_end_transaction(trans);
+		return ret;
+	}
 
 	node = rb_first_cached(&fs_info->block_group_cache_tree);
 	while (node) {
@@ -1338,8 +1341,11 @@ int btrfs_rebuild_free_space_tree(struct btrfs_fs_info *fs_info)
 		block_group = rb_entry(node, struct btrfs_block_group,
 				       cache_node);
 		ret = populate_free_space_tree(trans, block_group);
-		if (ret)
-			goto abort;
+		if (ret) {
+			btrfs_abort_transaction(trans, ret);
+			btrfs_end_transaction(trans);
+			return ret;
+		}
 		node = rb_next(node);
 	}
 
@@ -1350,10 +1356,6 @@ int btrfs_rebuild_free_space_tree(struct btrfs_fs_info *fs_info)
 	ret = btrfs_commit_transaction(trans);
 	clear_bit(BTRFS_FS_FREE_SPACE_TREE_UNTRUSTED, &fs_info->flags);
 	return ret;
-abort:
-	btrfs_abort_transaction(trans, ret);
-	btrfs_end_transaction(trans);
-	return ret;
 }
 
 static int __add_block_group_free_space(struct btrfs_trans_handle *trans,
-- 
2.42.1

Re: [PATCH 00/20] Error handling fixes

[Josef Bacik]

On Wed, Jan 24, 2024 at 10:17:35PM +0100, David Sterba wrote:
> Get rid of some easy BUG_ONs, there are a few groups fixing the same
> pattern. At the end there are three patches that move transaction abort
> to the right place. I have tested it on my setups only, not the CI, but
> given the type of fix we'd never hit any of them without special
> instrumentation.
> 

Reviewed-by: Josef Bacik <josef@toxicpanda.com>

Thanks,

Josef

Re: [PATCH 02/20] btrfs: handle invalid range and start in merge_extent_mapping()

[Qu Wenruo]

On 2024/1/25 07:47, David Sterba wrote:
> Turn a BUG_ON to a properly handled error and update the error message
> in the caller.  It is expected that @em_in and @start passed to
> btrfs_add_extent_mapping() overlap. Besides tests, the only caller
> btrfs_get_extent() makes sure this is true.
>
> Signed-off-by: David Sterba <dsterba@suse.com>
> ---
>   fs/btrfs/extent_map.c | 9 +++++----
>   1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/fs/btrfs/extent_map.c b/fs/btrfs/extent_map.c
> index f170e7122e74..ac5e366d57b2 100644
> --- a/fs/btrfs/extent_map.c
> +++ b/fs/btrfs/extent_map.c
> @@ -539,7 +539,8 @@ static noinline int merge_extent_mapping(struct extent_map_tree *em_tree,
>   	u64 end;
>   	u64 start_diff;
>
> -	BUG_ON(map_start < em->start || map_start >= extent_map_end(em));
> +	if (map_start < em->start || map_start >= extent_map_end(em))
> +		return -EINVAL;

Shouldn't we go -EUCLEAN?

This is not something we really expect to hit, as it normally means
something wrong with the extent map.

Thanks,
Qu

>
>   	if (existing->start > map_start) {
>   		next = existing;
> @@ -634,9 +635,9 @@ int btrfs_add_extent_mapping(struct btrfs_fs_info *fs_info,
>   				free_extent_map(em);
>   				*em_in = NULL;
>   				WARN_ONCE(ret,
> -"unexpected error %d: merge existing(start %llu len %llu) with em(start %llu len %llu)\n",
> -					  ret, existing->start, existing->len,
> -					  orig_start, orig_len);
> +"extent map merge error existing [%llu, %llu) with em [%llu, %llu) start %llu\n",
> +					  existing->start, existing->len,
> +					  orig_start, orig_len, start);
>   			}
>   			free_extent_map(existing);
>   		}

Re: [PATCH 03/20] btrfs: handle block group lookup error when it's being removed

[Qu Wenruo]

On 2024/1/25 07:47, David Sterba wrote:
> The unlikely case of lookup error in btrfs_remove_block_group() can be
> handled properly, in its caller this would lead to a transaction abort.
> We can't do anything else, a block group must have been loaded first.
>
> Signed-off-by: David Sterba <dsterba@suse.com>
> ---
>   fs/btrfs/block-group.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/fs/btrfs/block-group.c b/fs/btrfs/block-group.c
> index 1905d76772a9..16a2b8609989 100644
> --- a/fs/btrfs/block-group.c
> +++ b/fs/btrfs/block-group.c
> @@ -1063,7 +1063,9 @@ int btrfs_remove_block_group(struct btrfs_trans_handle *trans,
>   	bool remove_rsv = false;
>
>   	block_group = btrfs_lookup_block_group(fs_info, map->start);
> -	BUG_ON(!block_group);
> +	if (!block_group)
> +		return -ENOENT;

This -ENOENT return value is fine, as the only caller would call
btrfs_abort_transaction() to be noisy enough.

And talking about btrfs_abort_transaction(), I think we can call it
early to make debug a little easierly.

> +
>   	BUG_ON(!block_group->ro);

But shouldn't we also handle the RO case?

Thanks,
Qu

>
>   	trace_btrfs_remove_block_group(block_group);

Re: [PATCH 04/20] btrfs: handle root deletion lookup error in btrfs_del_root()

[Qu Wenruo]

On 2024/1/25 07:48, David Sterba wrote:
> We're deleting a root and looking it up by key does not succeed, this
> is an inconsistent state and we can't do anything. All callers handle
> errors and abort a transaction.
>
> Signed-off-by: David Sterba <dsterba@suse.com>
> ---
>   fs/btrfs/root-tree.c | 7 +++++--
>   1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/fs/btrfs/root-tree.c b/fs/btrfs/root-tree.c
> index 603ad1459368..ba7e2181ff4e 100644
> --- a/fs/btrfs/root-tree.c
> +++ b/fs/btrfs/root-tree.c
> @@ -323,8 +323,11 @@ int btrfs_del_root(struct btrfs_trans_handle *trans,
>   	ret = btrfs_search_slot(trans, root, key, path, -1, 1);
>   	if (ret < 0)
>   		goto out;
> -
> -	BUG_ON(ret != 0);
> +	if (ret != 0) {
> +		/* The root must exist but we did not find it by the key. */
> +		ret = -EUCLEAN;

IIRC every EUCLEAN needs a message (at least that's the rule inside
tree-checker).

And the only two callers are also aborting the transaction, thus I
believe we can just abort here.

Thanks,
Qu

> +		goto out;
> +	}
>
>   	ret = btrfs_del_item(trans, root, path);
>   out:

Re: [PATCH 05/20] btrfs: handle invalid root reference found in btrfs_find_root()

[Qu Wenruo]

On 2024/1/25 07:48, David Sterba wrote:
> The btrfs_find_root() 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 allowed the range of a root id.
>
> Signed-off-by: David Sterba <dsterba@suse.com>
> ---
>   fs/btrfs/root-tree.c | 9 ++++++++-
>   1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/fs/btrfs/root-tree.c b/fs/btrfs/root-tree.c
> index ba7e2181ff4e..326cd0d03287 100644
> --- a/fs/btrfs/root-tree.c
> +++ b/fs/btrfs/root-tree.c
> @@ -82,7 +82,14 @@ int btrfs_find_root(struct btrfs_root *root, const struct btrfs_key *search_key,
>   		if (ret > 0)
>   			goto out;
>   	} else {
> -		BUG_ON(ret == 0);		/* Logical error */
> +		/*
> +		 * Key with offset -1 found, there would have to exist a root
> +		 * with such id, but this is out of the valid range.
> +		 */
> +		if (ret == 0) {
> +			ret = -EUCLEAN;
> +			goto out;
> +		}

Considering all root items would already be checked by tree-checker,
I'd prefer to add an extra "key.offset == (u64)-1" check, and convert
this to ASSERT(ret == 0), so that we won't need to bother the impossible
case (nor its error messages).

Thanks,
Qu

>   		if (path->slots[0] == 0)
>   			goto out;
>   		path->slots[0]--;

Re: [PATCH 06/20] btrfs: handle invalid root reference found in btrfs_init_root_free_objectid()

[Qu Wenruo]

On 2024/1/25 07:48, David Sterba wrote:
> The btrfs_init_root_free_objectid() 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/disk-io.c | 9 ++++++++-
>   1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> index 274a3e1faeab..3f7291a48a4d 100644
> --- a/fs/btrfs/disk-io.c
> +++ b/fs/btrfs/disk-io.c
> @@ -4928,7 +4928,14 @@ int btrfs_init_root_free_objectid(struct btrfs_root *root)
>   	ret = btrfs_search_slot(NULL, root, &search_key, path, 0, 0);
>   	if (ret < 0)
>   		goto error;
> -	BUG_ON(ret == 0); /* Corruption */
> +	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;

Better with an error message, since we don't have a trans to abort and
one of the two callers are not aborting the transaction, making it
harder to debug.

Thanks,
Qu

> +		goto error;
> +	}
>   	if (path->slots[0] > 0) {
>   		slot = path->slots[0] - 1;
>   		l = path->nodes[0];

Re: [PATCH 07/20] btrfs: handle chunk tree lookup error in btrfs_relocate_sys_chunks()

[Qu Wenruo]

On 2024/1/25 07:48, David Sterba wrote:
> The unhandled case in btrfs_relocate_sys_chunks() loop is a corruption,
> as it could be caused only by two impossible conditions:
>
> - at first the search key is set up to look for a chunk tree item, with
>    offset -1, this is an inexact search and the key->offset will contain
>    the correct offset upon a successful search, a valid chunk tree item
>    cannot have an offset -1
>
> - after first successful search, the found_key corresponds to a chunk
>    item, the offset is decremented by 1 before the next loop, it's
>    impossible to find a chunk item there due to alignment and size
>    constraints
>
> Signed-off-by: David Sterba <dsterba@suse.com>
> ---
>   fs/btrfs/volumes.c | 12 +++++++++++-
>   1 file changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index d67785be2c77..6aae92e4b424 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -3393,7 +3393,17 @@ static int btrfs_relocate_sys_chunks(struct btrfs_fs_info *fs_info)
>   			mutex_unlock(&fs_info->reclaim_bgs_lock);
>   			goto error;
>   		}
> -		BUG_ON(ret == 0); /* Corruption */
> +		if (ret == 0) {
> +			/*
> +			 * On the first search we would find chunk tree with
> +			 * offset -1, which is not possible. On subsequent
> +			 * loops this would find an existing item on an invalid
> +			 * offset (one less than the previous one, wrong
> +			 * alignment and size).
> +			 */
> +			ret = -EUCLEAN;
> +			goto error;

This can be handled by adding new root item offset key check in
tree-checker, then converting to an ASSERT().

Thanks,
Qu
> +		}
>
>   		ret = btrfs_previous_item(chunk_root, path, key.objectid,
>   					  key.type);

Re: [PATCH 08/20] btrfs: handle invalid extent item reference found in check_committed_ref()

[Qu Wenruo]

On 2024/1/25 07:48, David Sterba wrote:
> The check_committed_ref() 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/extent-tree.c | 9 ++++++++-
>   1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
> index 4283e3025ab0..ba47f5996c84 100644
> --- a/fs/btrfs/extent-tree.c
> +++ b/fs/btrfs/extent-tree.c
> @@ -2399,7 +2399,14 @@ static noinline int check_committed_ref(struct btrfs_root *root,
>   	ret = btrfs_search_slot(NULL, extent_root, &key, path, 0, 0);
>   	if (ret < 0)
>   		goto out;
> -	BUG_ON(ret == 0); /* Corruption */
> +	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 out;
> +	}

It looks like we also need an key offset check for extent item.

Thanks,
Qu
>
>   	ret = -ENOENT;
>   	if (path->slots[0] == 0)

Re: [PATCH 00/20] Error handling fixes

[Qu Wenruo]

On 2024/1/25 07:47, David Sterba wrote:
> Get rid of some easy BUG_ONs, there are a few groups fixing the same
> pattern. At the end there are three patches that move transaction abort
> to the right place. I have tested it on my setups only, not the CI, but
> given the type of fix we'd never hit any of them without special
> instrumentation.

Sorry, I bombarded the mailing list again...

So there is the summary of my comments here:

>
> David Sterba (20):
>    btrfs: handle directory and dentry mismatch in btrfs_may_delete()
>    btrfs: handle invalid range and start in merge_extent_mapping()
>    btrfs: handle block group lookup error when it's being removed

For those error handling patches, I believe for all the BUG_ON() cases,
we should return -EUCLEAN, with an error message.

If possible we should also abort the transaction early (which can be
noisy enough to be caught by test cases, and give better call trace)

>    btrfs: handle root deletion lookup error in btrfs_del_root()
>    btrfs: handle invalid root reference found in btrfs_find_root()
>    btrfs: handle invalid root reference found in btrfs_init_root_free_objectid()

For those key.offset == -1 search case, tree-checker may be a better
solution, as it provides more detailed info for us to debug.

But you may only want such offset == -1 check for certain key types.
As I'm not sure if something key types (e.g. DIR_ITEM, whose key offset
is a hash).

Then those call sites can convert to ASSERT() for key hit case.

>    btrfs: handle chunk tree lookup error in btrfs_relocate_sys_chunks()
>    btrfs: handle invalid extent item reference found in check_committed_ref()
>    btrfs: export: handle invalid inode or root reference in btrfs_get_parent()
>    btrfs: delayed-inode: drop pointless BUG_ON in __btrfs_remove_delayed_item()
>    btrfs: change BUG_ON to assertion when checking for delayed_node root


>    btrfs: defrag: change BUG_ON to assertion in btrfs_defrag_leaves()
>    btrfs: change BUG_ON to assertion in btrfs_read_roots()
>    btrfs: change BUG_ON to assertion when verifying lockdep class setup
>    btrfs: change BUG_ON to assertion when verifying root in btrfs_alloc_reserved_file_extent()
>    btrfs: change BUG_ON to assertion in reset_balance_state()

I'm totally fine with BUG_ON() to ASSERT() changes.

>    btrfs: unify handling of return values of btrfs_insert_empty_items()
>    btrfs: move transaction abort to the error site in btrfs_delete_free_space_tree()
>    btrfs: move transaction abort to the error site in btrfs_create_free_space_tree()
>    btrfs: move transaction abort to the error site btrfs_rebuild_free_space_tree()

And earlier transaction abort is also pretty good to me.

Thanks,
Qu

>
>   fs/btrfs/block-group.c     |  4 ++-
>   fs/btrfs/ctree.c           |  4 +++
>   fs/btrfs/defrag.c          |  2 +-
>   fs/btrfs/delayed-inode.c   |  4 +--
>   fs/btrfs/disk-io.c         | 11 ++++++--
>   fs/btrfs/export.c          |  9 +++++-
>   fs/btrfs/extent-tree.c     | 11 ++++++--
>   fs/btrfs/extent_map.c      |  9 +++---
>   fs/btrfs/file-item.c       |  3 --
>   fs/btrfs/free-space-tree.c | 56 ++++++++++++++++++++++----------------
>   fs/btrfs/ioctl.c           |  4 ++-
>   fs/btrfs/locking.c         |  2 +-
>   fs/btrfs/root-tree.c       | 16 +++++++++--
>   fs/btrfs/uuid-tree.c       |  2 +-
>   fs/btrfs/volumes.c         | 14 ++++++++--
>   15 files changed, 102 insertions(+), 49 deletions(-)
>

Re: [PATCH 02/20] btrfs: handle invalid range and start in merge_extent_mapping()

[David Sterba]

On Thu, Jan 25, 2024 at 02:23:03PM +1030, Qu Wenruo wrote:
> 
> 
> On 2024/1/25 07:47, David Sterba wrote:
> > Turn a BUG_ON to a properly handled error and update the error message
> > in the caller.  It is expected that @em_in and @start passed to
> > btrfs_add_extent_mapping() overlap. Besides tests, the only caller
> > btrfs_get_extent() makes sure this is true.
> >
> > Signed-off-by: David Sterba <dsterba@suse.com>
> > ---
> >   fs/btrfs/extent_map.c | 9 +++++----
> >   1 file changed, 5 insertions(+), 4 deletions(-)
> >
> > diff --git a/fs/btrfs/extent_map.c b/fs/btrfs/extent_map.c
> > index f170e7122e74..ac5e366d57b2 100644
> > --- a/fs/btrfs/extent_map.c
> > +++ b/fs/btrfs/extent_map.c
> > @@ -539,7 +539,8 @@ static noinline int merge_extent_mapping(struct extent_map_tree *em_tree,
> >   	u64 end;
> >   	u64 start_diff;
> >
> > -	BUG_ON(map_start < em->start || map_start >= extent_map_end(em));
> > +	if (map_start < em->start || map_start >= extent_map_end(em))
> > +		return -EINVAL;
> 
> Shouldn't we go -EUCLEAN?
> 
> This is not something we really expect to hit, as it normally means
> something wrong with the extent map.

The logic I used here is that it's a simple helper and it verifies the
arguments. If they're invalid then it's EINVAL. EUCLEAN needs some
interpretation like in the other patches unexpectedly finding an item,
it's up to the caller.

Re: [PATCH 03/20] btrfs: handle block group lookup error when it's being removed

[David Sterba]

On Thu, Jan 25, 2024 at 02:28:02PM +1030, Qu Wenruo wrote:
> 
> 
> On 2024/1/25 07:47, David Sterba wrote:
> > The unlikely case of lookup error in btrfs_remove_block_group() can be
> > handled properly, in its caller this would lead to a transaction abort.
> > We can't do anything else, a block group must have been loaded first.
> >
> > Signed-off-by: David Sterba <dsterba@suse.com>
> > ---
> >   fs/btrfs/block-group.c | 4 +++-
> >   1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/fs/btrfs/block-group.c b/fs/btrfs/block-group.c
> > index 1905d76772a9..16a2b8609989 100644
> > --- a/fs/btrfs/block-group.c
> > +++ b/fs/btrfs/block-group.c
> > @@ -1063,7 +1063,9 @@ int btrfs_remove_block_group(struct btrfs_trans_handle *trans,
> >   	bool remove_rsv = false;
> >
> >   	block_group = btrfs_lookup_block_group(fs_info, map->start);
> > -	BUG_ON(!block_group);
> > +	if (!block_group)
> > +		return -ENOENT;
> 
> This -ENOENT return value is fine, as the only caller would call
> btrfs_abort_transaction() to be noisy enough.
> 
> And talking about btrfs_abort_transaction(), I think we can call it
> early to make debug a little easierly.

There are several patterns, one is that transaction abort is called by
the function that started it. It's not consistent but as a hint abort
can be used anywhere if things go so bad that it's impossible to roll
back, eg. in a middle of a big loop setting up block groups and such.

> > +
> >   	BUG_ON(!block_group->ro);
> 
> But shouldn't we also handle the RO case?

Of course but it's fixing a different problem with tracking of read-only
status of a bg and I will not mix that to that patch.

Re: [PATCH 04/20] btrfs: handle root deletion lookup error in btrfs_del_root()

[David Sterba]

On Thu, Jan 25, 2024 at 02:31:01PM +1030, Qu Wenruo wrote:
> 
> 
> On 2024/1/25 07:48, David Sterba wrote:
> > We're deleting a root and looking it up by key does not succeed, this
> > is an inconsistent state and we can't do anything. All callers handle
> > errors and abort a transaction.
> >
> > Signed-off-by: David Sterba <dsterba@suse.com>
> > ---
> >   fs/btrfs/root-tree.c | 7 +++++--
> >   1 file changed, 5 insertions(+), 2 deletions(-)
> >
> > diff --git a/fs/btrfs/root-tree.c b/fs/btrfs/root-tree.c
> > index 603ad1459368..ba7e2181ff4e 100644
> > --- a/fs/btrfs/root-tree.c
> > +++ b/fs/btrfs/root-tree.c
> > @@ -323,8 +323,11 @@ int btrfs_del_root(struct btrfs_trans_handle *trans,
> >   	ret = btrfs_search_slot(trans, root, key, path, -1, 1);
> >   	if (ret < 0)
> >   		goto out;
> > -
> > -	BUG_ON(ret != 0);
> > +	if (ret != 0) {
> > +		/* The root must exist but we did not find it by the key. */
> > +		ret = -EUCLEAN;
> 
> IIRC every EUCLEAN needs a message (at least that's the rule inside
> tree-checker).

In tree-checker yes, there are many reasons why reading/writing a block
may fail. There the data are read for the first time and we could expect
failures, so it's the first line of defence.

Many other EUCLEAN errors are returned without any messages from deep
call chains and are practically never to be seen, this is the last
resort defence.

> And the only two callers are also aborting the transaction, thus I
> believe we can just abort here.

As said in the other reply, it's up to the caller.

Re: [PATCH 05/20] btrfs: handle invalid root reference found in btrfs_find_root()

[David Sterba]

On Thu, Jan 25, 2024 at 02:33:53PM +1030, Qu Wenruo wrote:
> 
> 
> On 2024/1/25 07:48, David Sterba wrote:
> > The btrfs_find_root() 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 allowed the range of a root id.
> >
> > Signed-off-by: David Sterba <dsterba@suse.com>
> > ---
> >   fs/btrfs/root-tree.c | 9 ++++++++-
> >   1 file changed, 8 insertions(+), 1 deletion(-)
> >
> > diff --git a/fs/btrfs/root-tree.c b/fs/btrfs/root-tree.c
> > index ba7e2181ff4e..326cd0d03287 100644
> > --- a/fs/btrfs/root-tree.c
> > +++ b/fs/btrfs/root-tree.c
> > @@ -82,7 +82,14 @@ int btrfs_find_root(struct btrfs_root *root, const struct btrfs_key *search_key,
> >   		if (ret > 0)
> >   			goto out;
> >   	} else {
> > -		BUG_ON(ret == 0);		/* Logical error */
> > +		/*
> > +		 * Key with offset -1 found, there would have to exist a root
> > +		 * with such id, but this is out of the valid range.
> > +		 */
> > +		if (ret == 0) {
> > +			ret = -EUCLEAN;
> > +			goto out;
> > +		}
> 
> Considering all root items would already be checked by tree-checker,
> I'd prefer to add an extra "key.offset == (u64)-1" check, and convert
> this to ASSERT(ret == 0), so that we won't need to bother the impossible
> case (nor its error messages).

I did not think about tree-checker in this context and it actually does
verify offsets of the item keys so it's already prevented, assuming such
corrupted issue would come from the outside.

Assertions are not very popular in release builds and distros turn them
off. A real error handling prevents propagating an error further to the
code, so this is for catching bugs that could happen after tree-checker
lets it pass with valid data but something sets wrong value to offset.

The reasoning I'm using is to have full coverage of the error values as
real handling with worst case throwing an EUCLEAN. Assertions are not
popular in release builds and distros turn them off so it's effectively
removing error handling and allowing silent errors.

Re: [PATCH 08/20] btrfs: handle invalid extent item reference found in check_committed_ref()

[David Sterba]

On Thu, Jan 25, 2024 at 02:40:12PM +1030, Qu Wenruo wrote:
> 
> 
> On 2024/1/25 07:48, David Sterba wrote:
> > The check_committed_ref() 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/extent-tree.c | 9 ++++++++-
> >   1 file changed, 8 insertions(+), 1 deletion(-)
> >
> > diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
> > index 4283e3025ab0..ba47f5996c84 100644
> > --- a/fs/btrfs/extent-tree.c
> > +++ b/fs/btrfs/extent-tree.c
> > @@ -2399,7 +2399,14 @@ static noinline int check_committed_ref(struct btrfs_root *root,
> >   	ret = btrfs_search_slot(NULL, extent_root, &key, path, 0, 0);
> >   	if (ret < 0)
> >   		goto out;
> > -	BUG_ON(ret == 0); /* Corruption */
> > +	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 out;
> > +	}
> 
> It looks like we also need an key offset check for extent item.

We already have it (though I did not remeber that first),
check_extent_item() checks alignment or exact values. Why I don't want
that to be an assert is in my previous reply.

Re: [PATCH 00/20] Error handling fixes

[David Sterba]

On Thu, Jan 25, 2024 at 02:51:34PM +1030, Qu Wenruo wrote:
> 
> 
> On 2024/1/25 07:47, David Sterba wrote:
> > Get rid of some easy BUG_ONs, there are a few groups fixing the same
> > pattern. At the end there are three patches that move transaction abort
> > to the right place. I have tested it on my setups only, not the CI, but
> > given the type of fix we'd never hit any of them without special
> > instrumentation.
> 
> Sorry, I bombarded the mailing list again...
> 
> So there is the summary of my comments here:

I've replied to the patches and readin this mail I don't see any
unanswered questions or points. The main problem with the error handling
is that it's not done consistently everywhere, I'm going to spend more
time on that and the discussion we had helped me to clear some points so
I'll drop it to the documentation for further reference. Also it's not
set in stone so we may refine it as needed. Thanks.

Re: [PATCH 03/20] btrfs: handle block group lookup error when it's being removed

[Qu Wenruo]

On 2024/1/26 02:42, David Sterba wrote:
> On Thu, Jan 25, 2024 at 02:28:02PM +1030, Qu Wenruo wrote:
>>
>>
>> On 2024/1/25 07:47, David Sterba wrote:
>>> The unlikely case of lookup error in btrfs_remove_block_group() can be
>>> handled properly, in its caller this would lead to a transaction abort.
>>> We can't do anything else, a block group must have been loaded first.
>>>
>>> Signed-off-by: David Sterba <dsterba@suse.com>
>>> ---
>>>    fs/btrfs/block-group.c | 4 +++-
>>>    1 file changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/fs/btrfs/block-group.c b/fs/btrfs/block-group.c
>>> index 1905d76772a9..16a2b8609989 100644
>>> --- a/fs/btrfs/block-group.c
>>> +++ b/fs/btrfs/block-group.c
>>> @@ -1063,7 +1063,9 @@ int btrfs_remove_block_group(struct btrfs_trans_handle *trans,
>>>    	bool remove_rsv = false;
>>>
>>>    	block_group = btrfs_lookup_block_group(fs_info, map->start);
>>> -	BUG_ON(!block_group);
>>> +	if (!block_group)
>>> +		return -ENOENT;
>>
>> This -ENOENT return value is fine, as the only caller would call
>> btrfs_abort_transaction() to be noisy enough.
>>
>> And talking about btrfs_abort_transaction(), I think we can call it
>> early to make debug a little easierly.
>
> There are several patterns, one is that transaction abort is called by
> the function that started it. It's not consistent but as a hint abort
> can be used anywhere if things go so bad that it's impossible to roll
> back, eg. in a middle of a big loop setting up block groups and such.

In this case, I don't think we can do anything to really rollback, thus
aborting early would help debug and provide a better call trace.

Thanks,
Qu

>
>>> +
>>>    	BUG_ON(!block_group->ro);
>>
>> But shouldn't we also handle the RO case?
>
> Of course but it's fixing a different problem with tracking of read-only
> status of a bg and I will not mix that to that patch.
>

Re: [PATCH 05/20] btrfs: handle invalid root reference found in btrfs_find_root()

[Qu Wenruo]

On 2024/1/26 02:58, David Sterba wrote:
> On Thu, Jan 25, 2024 at 02:33:53PM +1030, Qu Wenruo wrote:
>>
>>
>> On 2024/1/25 07:48, David Sterba wrote:
>>> The btrfs_find_root() 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 allowed the range of a root id.
>>>
>>> Signed-off-by: David Sterba <dsterba@suse.com>
>>> ---
>>>    fs/btrfs/root-tree.c | 9 ++++++++-
>>>    1 file changed, 8 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/fs/btrfs/root-tree.c b/fs/btrfs/root-tree.c
>>> index ba7e2181ff4e..326cd0d03287 100644
>>> --- a/fs/btrfs/root-tree.c
>>> +++ b/fs/btrfs/root-tree.c
>>> @@ -82,7 +82,14 @@ int btrfs_find_root(struct btrfs_root *root, const struct btrfs_key *search_key,
>>>    		if (ret > 0)
>>>    			goto out;
>>>    	} else {
>>> -		BUG_ON(ret == 0);		/* Logical error */
>>> +		/*
>>> +		 * Key with offset -1 found, there would have to exist a root
>>> +		 * with such id, but this is out of the valid range.
>>> +		 */
>>> +		if (ret == 0) {
>>> +			ret = -EUCLEAN;
>>> +			goto out;
>>> +		}
>>
>> Considering all root items would already be checked by tree-checker,
>> I'd prefer to add an extra "key.offset == (u64)-1" check, and convert
>> this to ASSERT(ret == 0), so that we won't need to bother the impossible
>> case (nor its error messages).
>
> I did not think about tree-checker in this context and it actually does
> verify offsets of the item keys so it's already prevented, assuming such
> corrupted issue would come from the outside.

If the root item is fine, but it's some runtime memory bitflip, then I'd
say if hitting an ASSERT() is really the last time we need to consider.

Furthermore, we have further safenet at metadata write time, which would
definitely lead to a transaction abort.

>
> Assertions are not very popular in release builds and distros turn them
> off. A real error handling prevents propagating an error further to the
> code, so this is for catching bugs that could happen after tree-checker
> lets it pass with valid data but something sets wrong value to offset.
>
> The reasoning I'm using is to have full coverage of the error values as
> real handling with worst case throwing an EUCLEAN. Assertions are not
> popular in release builds and distros turn them off so it's effectively
> removing error handling and allowing silent errors.
>
Yep, I know ASSERT() is not popular in release builds.

But in this case, if tree-checker didn't catch such corruption, but some
runtime memory biflip (well, no single bitflip can result to -1) or
memory corruption created such -1 key offset, and ASSERT() is ignoring it.

That would still be fine, as our final write time tree-checker would
catch and abort current transaction.

So yes, I want to use ASSERT() intentionally here, because we're still
fine even it's not properly caught.

And again back to the old discussion on EUCLEAN, I really want it to be
noisy enough immediately, not waiting for the caller layers up the call
chain to do their error handling.

Thanks,
Qu

Re: [PATCH 05/20] btrfs: handle invalid root reference found in btrfs_find_root()

[Anand Jain]

On 1/25/24 05:18, David Sterba wrote:
> The btrfs_find_root() 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 allowed the range of a root id.
> 
> Signed-off-by: David Sterba <dsterba@suse.com>
> ---
>   fs/btrfs/root-tree.c | 9 ++++++++-
>   1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/btrfs/root-tree.c b/fs/btrfs/root-tree.c
> index ba7e2181ff4e..326cd0d03287 100644
> --- a/fs/btrfs/root-tree.c
> +++ b/fs/btrfs/root-tree.c
> @@ -82,7 +82,14 @@ int btrfs_find_root(struct btrfs_root *root, const struct btrfs_key *search_key,
>   		if (ret > 0)
>   			goto out;
>   	} else {
> -		BUG_ON(ret == 0);		/* Logical error */
> +		/*
> +		 * Key with offset -1 found, there would have to exist a root
> +		 * with such id, but this is out of the valid range.
> +		 */
> +		if (ret == 0) {
> +			ret = -EUCLEAN;
> +			goto out;
> +		}
>   		if (path->slots[0] == 0)
>   			goto out;
>   		path->slots[0]--;


While here, why not also add an error message, especially for calls
from btrfs_read_roots() when the IGNOREBADROOTS is set, we ignore
the error and continue without the abort(). Including an error
message will provide more information about the bad root.

btrfs_read_roots()
::
  btrfs_read_tree_root() | btrfs_get_fs_root_commit_root() | 
load_global_roots_objectid()
  read_tree_root_path()
   btrfs_find_root()


Thanks, Anand

Re: [PATCH 00/20] Error handling fixes

[Anand Jain]

On 1/25/24 05:17, David Sterba wrote:
> Get rid of some easy BUG_ONs, there are a few groups fixing the same
> pattern. At the end there are three patches that move transaction abort
> to the right place. I have tested it on my setups only, not the CI, but
> given the type of fix we'd never hit any of them without special
> instrumentation.
> 

What is our guideline for the error message location in the function
stack, leaf function or at the root function? IMO, it should be
in the leaf functions(), so that in the event of a serious error,
we have substantial logs to pinpoint the issue. Here, despite -EUCLEAN
we have no errors logged, in some of the patches. Why not also add
error message where it wont' endup with abort(). Otherwise, debugging
becomes too difficult when looking at the errors due to corruption.

Thanks, Anand


> David Sterba (20):
>    btrfs: handle directory and dentry mismatch in btrfs_may_delete()
>    btrfs: handle invalid range and start in merge_extent_mapping()
>    btrfs: handle block group lookup error when it's being removed
>    btrfs: handle root deletion lookup error in btrfs_del_root()
>    btrfs: handle invalid root reference found in btrfs_find_root()
>    btrfs: handle invalid root reference found in btrfs_init_root_free_objectid()
>    btrfs: handle chunk tree lookup error in btrfs_relocate_sys_chunks()
>    btrfs: handle invalid extent item reference found in check_committed_ref()
>    btrfs: export: handle invalid inode or root reference in btrfs_get_parent()
>    btrfs: delayed-inode: drop pointless BUG_ON in __btrfs_remove_delayed_item()
>    btrfs: change BUG_ON to assertion when checking for delayed_node root
>    btrfs: defrag: change BUG_ON to assertion in btrfs_defrag_leaves()
>    btrfs: change BUG_ON to assertion in btrfs_read_roots()
>    btrfs: change BUG_ON to assertion when verifying lockdep class setup
>    btrfs: change BUG_ON to assertion when verifying root in btrfs_alloc_reserved_file_extent()
>    btrfs: change BUG_ON to assertion in reset_balance_state()
>    btrfs: unify handling of return values of btrfs_insert_empty_items()
>    btrfs: move transaction abort to the error site in btrfs_delete_free_space_tree()
>    btrfs: move transaction abort to the error site in btrfs_create_free_space_tree()
>    btrfs: move transaction abort to the error site btrfs_rebuild_free_space_tree()
> 
>   fs/btrfs/block-group.c     |  4 ++-
>   fs/btrfs/ctree.c           |  4 +++
>   fs/btrfs/defrag.c          |  2 +-
>   fs/btrfs/delayed-inode.c   |  4 +--
>   fs/btrfs/disk-io.c         | 11 ++++++--
>   fs/btrfs/export.c          |  9 +++++-
>   fs/btrfs/extent-tree.c     | 11 ++++++--
>   fs/btrfs/extent_map.c      |  9 +++---
>   fs/btrfs/file-item.c       |  3 --
>   fs/btrfs/free-space-tree.c | 56 ++++++++++++++++++++++----------------
>   fs/btrfs/ioctl.c           |  4 ++-
>   fs/btrfs/locking.c         |  2 +-
>   fs/btrfs/root-tree.c       | 16 +++++++++--
>   fs/btrfs/uuid-tree.c       |  2 +-
>   fs/btrfs/volumes.c         | 14 ++++++++--
>   15 files changed, 102 insertions(+), 49 deletions(-)
> 

Re: [PATCH 05/20] btrfs: handle invalid root reference found in btrfs_find_root()

[Josef Bacik]

On Fri, Jan 26, 2024 at 09:44:52AM +1030, Qu Wenruo wrote:
> 
> 
> On 2024/1/26 02:58, David Sterba wrote:
> > On Thu, Jan 25, 2024 at 02:33:53PM +1030, Qu Wenruo wrote:
> > > 
> > > 
> > > On 2024/1/25 07:48, David Sterba wrote:
> > > > The btrfs_find_root() 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 allowed the range of a root id.
> > > > 
> > > > Signed-off-by: David Sterba <dsterba@suse.com>
> > > > ---
> > > >    fs/btrfs/root-tree.c | 9 ++++++++-
> > > >    1 file changed, 8 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/fs/btrfs/root-tree.c b/fs/btrfs/root-tree.c
> > > > index ba7e2181ff4e..326cd0d03287 100644
> > > > --- a/fs/btrfs/root-tree.c
> > > > +++ b/fs/btrfs/root-tree.c
> > > > @@ -82,7 +82,14 @@ int btrfs_find_root(struct btrfs_root *root, const struct btrfs_key *search_key,
> > > >    		if (ret > 0)
> > > >    			goto out;
> > > >    	} else {
> > > > -		BUG_ON(ret == 0);		/* Logical error */
> > > > +		/*
> > > > +		 * Key with offset -1 found, there would have to exist a root
> > > > +		 * with such id, but this is out of the valid range.
> > > > +		 */
> > > > +		if (ret == 0) {
> > > > +			ret = -EUCLEAN;
> > > > +			goto out;
> > > > +		}
> > > 
> > > Considering all root items would already be checked by tree-checker,
> > > I'd prefer to add an extra "key.offset == (u64)-1" check, and convert
> > > this to ASSERT(ret == 0), so that we won't need to bother the impossible
> > > case (nor its error messages).
> > 
> > I did not think about tree-checker in this context and it actually does
> > verify offsets of the item keys so it's already prevented, assuming such
> > corrupted issue would come from the outside.
> 
> If the root item is fine, but it's some runtime memory bitflip, then I'd
> say if hitting an ASSERT() is really the last time we need to consider.
> 
> Furthermore, we have further safenet at metadata write time, which would
> definitely lead to a transaction abort.
> 
> > 
> > Assertions are not very popular in release builds and distros turn them
> > off. A real error handling prevents propagating an error further to the
> > code, so this is for catching bugs that could happen after tree-checker
> > lets it pass with valid data but something sets wrong value to offset.
> > 
> > The reasoning I'm using is to have full coverage of the error values as
> > real handling with worst case throwing an EUCLEAN. Assertions are not
> > popular in release builds and distros turn them off so it's effectively
> > removing error handling and allowing silent errors.
> > 
> Yep, I know ASSERT() is not popular in release builds.
> 
> But in this case, if tree-checker didn't catch such corruption, but some
> runtime memory biflip (well, no single bitflip can result to -1) or
> memory corruption created such -1 key offset, and ASSERT() is ignoring it.
> 
> That would still be fine, as our final write time tree-checker would
> catch and abort current transaction.
> 
> So yes, I want to use ASSERT() intentionally here, because we're still
> fine even it's not properly caught.
> 
> And again back to the old discussion on EUCLEAN, I really want it to be
> noisy enough immediately, not waiting for the caller layers up the call
> chain to do their error handling.

We can have both.

This patch series is removing BUG_ON()'s, and this is the correct change for
that.

If we need followups to harden the tree checker that's valuable work too, but
that's a followup and doesn't mean this series needs changing.

With CONFIG_BTRFS_DEBUG on we're going to get a WARN_ON when this thing trips
and we'll see it in fstests.  Thanks,

Josef

Re: [PATCH 03/20] btrfs: handle block group lookup error when it's being removed

[David Sterba]

On Fri, Jan 26, 2024 at 09:39:14AM +1030, Qu Wenruo wrote:
> 
> 
> On 2024/1/26 02:42, David Sterba wrote:
> > On Thu, Jan 25, 2024 at 02:28:02PM +1030, Qu Wenruo wrote:
> >>
> >>
> >> On 2024/1/25 07:47, David Sterba wrote:
> >>> The unlikely case of lookup error in btrfs_remove_block_group() can be
> >>> handled properly, in its caller this would lead to a transaction abort.
> >>> We can't do anything else, a block group must have been loaded first.
> >>>
> >>> Signed-off-by: David Sterba <dsterba@suse.com>
> >>> ---
> >>>    fs/btrfs/block-group.c | 4 +++-
> >>>    1 file changed, 3 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/fs/btrfs/block-group.c b/fs/btrfs/block-group.c
> >>> index 1905d76772a9..16a2b8609989 100644
> >>> --- a/fs/btrfs/block-group.c
> >>> +++ b/fs/btrfs/block-group.c
> >>> @@ -1063,7 +1063,9 @@ int btrfs_remove_block_group(struct btrfs_trans_handle *trans,
> >>>    	bool remove_rsv = false;
> >>>
> >>>    	block_group = btrfs_lookup_block_group(fs_info, map->start);
> >>> -	BUG_ON(!block_group);
> >>> +	if (!block_group)
> >>> +		return -ENOENT;
> >>
> >> This -ENOENT return value is fine, as the only caller would call
> >> btrfs_abort_transaction() to be noisy enough.
> >>
> >> And talking about btrfs_abort_transaction(), I think we can call it
> >> early to make debug a little easierly.
> >
> > There are several patterns, one is that transaction abort is called by
> > the function that started it. It's not consistent but as a hint abort
> > can be used anywhere if things go so bad that it's impossible to roll
> > back, eg. in a middle of a big loop setting up block groups and such.
> 
> In this case, I don't think we can do anything to really rollback, thus
> aborting early would help debug and provide a better call trace.

Yeah, although looking again what btrfs_remove_block_group() does, there
are more error checks and none of them does transaction abort.

If we want to do make it the recommended way, then it's "once there's a
transaction handle, any error that's not recoverable must do transaction
abort". The decision is in the 'recoverable'. That would also require
audit everything once we settle on the way it's supposed to be handled.
I could add it here but then it's going to be an outlier, I'd rather do
another pass and keep things simpler for now.

Re: [PATCH 05/20] btrfs: handle invalid root reference found in btrfs_find_root()

[David Sterba]

On Fri, Jan 26, 2024 at 09:19:27AM -0500, Josef Bacik wrote:
> On Fri, Jan 26, 2024 at 09:44:52AM +1030, Qu Wenruo wrote:
> > 
> > 
> > On 2024/1/26 02:58, David Sterba wrote:
> > > On Thu, Jan 25, 2024 at 02:33:53PM +1030, Qu Wenruo wrote:
> > > > 
> > > > 
> > > > On 2024/1/25 07:48, David Sterba wrote:
> > > > > The btrfs_find_root() 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 allowed the range of a root id.
> > > > > 
> > > > > Signed-off-by: David Sterba <dsterba@suse.com>
> > > > > ---
> > > > >    fs/btrfs/root-tree.c | 9 ++++++++-
> > > > >    1 file changed, 8 insertions(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/fs/btrfs/root-tree.c b/fs/btrfs/root-tree.c
> > > > > index ba7e2181ff4e..326cd0d03287 100644
> > > > > --- a/fs/btrfs/root-tree.c
> > > > > +++ b/fs/btrfs/root-tree.c
> > > > > @@ -82,7 +82,14 @@ int btrfs_find_root(struct btrfs_root *root, const struct btrfs_key *search_key,
> > > > >    		if (ret > 0)
> > > > >    			goto out;
> > > > >    	} else {
> > > > > -		BUG_ON(ret == 0);		/* Logical error */
> > > > > +		/*
> > > > > +		 * Key with offset -1 found, there would have to exist a root
> > > > > +		 * with such id, but this is out of the valid range.
> > > > > +		 */
> > > > > +		if (ret == 0) {
> > > > > +			ret = -EUCLEAN;
> > > > > +			goto out;
> > > > > +		}
> > > > 
> > > > Considering all root items would already be checked by tree-checker,
> > > > I'd prefer to add an extra "key.offset == (u64)-1" check, and convert
> > > > this to ASSERT(ret == 0), so that we won't need to bother the impossible
> > > > case (nor its error messages).
> > > 
> > > I did not think about tree-checker in this context and it actually does
> > > verify offsets of the item keys so it's already prevented, assuming such
> > > corrupted issue would come from the outside.
> > 
> > If the root item is fine, but it's some runtime memory bitflip, then I'd
> > say if hitting an ASSERT() is really the last time we need to consider.
> > 
> > Furthermore, we have further safenet at metadata write time, which would
> > definitely lead to a transaction abort.
> > 
> > > 
> > > Assertions are not very popular in release builds and distros turn them
> > > off. A real error handling prevents propagating an error further to the
> > > code, so this is for catching bugs that could happen after tree-checker
> > > lets it pass with valid data but something sets wrong value to offset.
> > > 
> > > The reasoning I'm using is to have full coverage of the error values as
> > > real handling with worst case throwing an EUCLEAN. Assertions are not
> > > popular in release builds and distros turn them off so it's effectively
> > > removing error handling and allowing silent errors.
> > > 
> > Yep, I know ASSERT() is not popular in release builds.
> > 
> > But in this case, if tree-checker didn't catch such corruption, but some
> > runtime memory biflip (well, no single bitflip can result to -1) or
> > memory corruption created such -1 key offset, and ASSERT() is ignoring it.
> > 
> > That would still be fine, as our final write time tree-checker would
> > catch and abort current transaction.
> > 
> > So yes, I want to use ASSERT() intentionally here, because we're still
> > fine even it's not properly caught.
> > 
> > And again back to the old discussion on EUCLEAN, I really want it to be
> > noisy enough immediately, not waiting for the caller layers up the call
> > chain to do their error handling.
> 
> We can have both.
> 
> This patch series is removing BUG_ON()'s, and this is the correct change for
> that.
> 
> If we need followups to harden the tree checker that's valuable work too, but
> that's a followup and doesn't mean this series needs changing.
> 
> With CONFIG_BTRFS_DEBUG on we're going to get a WARN_ON when this thing trips
> and we'll see it in fstests.  Thanks,

I'd propose something different than an ASSERT for handling the EUCLEAN
cases. To make it print a WARN_ON under debug and with a message (all
builds).

The first step is to handle all the errors so I'd like not to mix it
with series. There are about 255 EUCLEAN cases (and possibly more
missing), that's a lot so we need to have a common way to handle them
instead of randomly adding ASSERT(0), duplicating an error condition
in the assert or doing thinsg like WARN(IS_ENABLED(CONFIG_BTRFS_DEBUG).

Re: [PATCH 05/20] btrfs: handle invalid root reference found in btrfs_find_root()

[David Sterba]

On Fri, Jan 26, 2024 at 08:06:06PM +0800, Anand Jain wrote:
> On 1/25/24 05:18, David Sterba wrote:
> > The btrfs_find_root() 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 allowed the range of a root id.
> > 
> > Signed-off-by: David Sterba <dsterba@suse.com>
> > ---
> >   fs/btrfs/root-tree.c | 9 ++++++++-
> >   1 file changed, 8 insertions(+), 1 deletion(-)
> > 
> > diff --git a/fs/btrfs/root-tree.c b/fs/btrfs/root-tree.c
> > index ba7e2181ff4e..326cd0d03287 100644
> > --- a/fs/btrfs/root-tree.c
> > +++ b/fs/btrfs/root-tree.c
> > @@ -82,7 +82,14 @@ int btrfs_find_root(struct btrfs_root *root, const struct btrfs_key *search_key,
> >   		if (ret > 0)
> >   			goto out;
> >   	} else {
> > -		BUG_ON(ret == 0);		/* Logical error */
> > +		/*
> > +		 * Key with offset -1 found, there would have to exist a root
> > +		 * with such id, but this is out of the valid range.
> > +		 */
> > +		if (ret == 0) {
> > +			ret = -EUCLEAN;
> > +			goto out;
> > +		}
> >   		if (path->slots[0] == 0)
> >   			goto out;
> >   		path->slots[0]--;
> 
> 
> While here, why not also add an error message, especially for calls
> from btrfs_read_roots() when the IGNOREBADROOTS is set, we ignore
> the error and continue without the abort(). Including an error
> message will provide more information about the bad root.

Yeah, after some thoughs i agree that the messages would be good, self
documenting instead of the comments. I'll do that in another series as
we first need some common way of handling it and wrappers.

Re: [PATCH 00/20] Error handling fixes

[David Sterba]

On Fri, Jan 26, 2024 at 08:28:12PM +0800, Anand Jain wrote:
> On 1/25/24 05:17, David Sterba wrote:
> > Get rid of some easy BUG_ONs, there are a few groups fixing the same
> > pattern. At the end there are three patches that move transaction abort
> > to the right place. I have tested it on my setups only, not the CI, but
> > given the type of fix we'd never hit any of them without special
> > instrumentation.
> > 
> 
> What is our guideline for the error message location in the function
> stack, leaf function or at the root function? IMO, it should be
> in the leaf functions(), so that in the event of a serious error,
> we have substantial logs to pinpoint the issue. Here, despite -EUCLEAN
> we have no errors logged, in some of the patches. Why not also add
> error message where it wont' endup with abort(). Otherwise, debugging
> becomes too difficult when looking at the errors due to corruption.


I'll try to sum up suggestions from the whole thread, the proposed end
result of the error handling should follow this:

- critical errors should be followed by a message at the place where
  they happen, typically next to an EUCLEAN
  - an exception could be something that is called frequently and the
    messages would flood the logs

- transaction abort should be at the place where an error is detected,
  assuming that it's clear from the context that it's unrecoverable and
  no rollback is possible (ie. free structures, undo changes and only
  return an error)
  - this also needs inspection for special cases and exceptions

- tree-checker verifies data at the time of reading from disk, we can
  make some assumptions about structure consistency

- error handling should cover all possible returned values, as an
  example search slot can return < 0, 0 or 1, although some of them
  might not be possible on a consistent filesystem, all should be
  handled and return EUCLEAN + message for the "impossible" ones

There are already examples that can be followed and copied elsewhere,
however we still lack some helpers for the EUCLEAN and messages
pattern.

Re: [PATCH 00/20] Error handling fixes

[Anand Jain]

On 1/30/24 00:13, David Sterba wrote:
> On Fri, Jan 26, 2024 at 08:28:12PM +0800, Anand Jain wrote:
>> On 1/25/24 05:17, David Sterba wrote:
>>> Get rid of some easy BUG_ONs, there are a few groups fixing the same
>>> pattern. At the end there are three patches that move transaction abort
>>> to the right place. I have tested it on my setups only, not the CI, but
>>> given the type of fix we'd never hit any of them without special
>>> instrumentation.
>>>
>>
>> What is our guideline for the error message location in the function
>> stack, leaf function or at the root function? IMO, it should be
>> in the leaf functions(), so that in the event of a serious error,
>> we have substantial logs to pinpoint the issue. Here, despite -EUCLEAN
>> we have no errors logged, in some of the patches. Why not also add
>> error message where it wont' endup with abort(). Otherwise, debugging
>> becomes too difficult when looking at the errors due to corruption.
> 
> 
> I'll try to sum up suggestions from the whole thread, the proposed end
> result of the error handling should follow this:
> 
> - critical errors should be followed by a message at the place where
>    they happen, typically next to an EUCLEAN
>    - an exception could be something that is called frequently and the
>      messages would flood the logs
> 
> - transaction abort should be at the place where an error is detected,
>    assuming that it's clear from the context that it's unrecoverable and
>    no rollback is possible (ie. free structures, undo changes and only
>    return an error)
>    - this also needs inspection for special cases and exceptions
> 
> - tree-checker verifies data at the time of reading from disk, we can
>    make some assumptions about structure consistency
> 
> - error handling should cover all possible returned values, as an
>    example search slot can return < 0, 0 or 1, although some of them
>    might not be possible on a consistent filesystem, all should be
>    handled and return EUCLEAN + message for the "impossible" ones
> 
> There are already examples that can be followed and copied elsewhere,
> however we still lack some helpers for the EUCLEAN and messages
> pattern.

  Some of error log might need a rate-limited messages.

  Thanks for the holistic error handling in the next series.
  For this series:

Reviewed-by: Anand Jain <anand.jain@oracle.com>