[PATCH 00/12] btrfs: various cleanups to make ctree.c sync easier

[PATCH 00/12] btrfs: various cleanups to make ctree.c sync easier

[Josef Bacik]

Hello,

These are various cleanups I needed to make syncing some of the kernel files
into btrfs-progs go smoothly.  They're cosmetic and organizational and shouldn't
have any functional changes.  Thanks,

Josef

Josef Bacik (12):
  btrfs: move btrfs_check_trunc_cache_free_space into block-rsv.c
  btrfs: remove level argument from btrfs_set_block_flags
  btrfs: simplify btrfs_check_leaf_* helpers into a single helper
  btrfs: add btrfs_tree_block_status definitions to tree-checker.h
  btrfs: use btrfs_tree_block_status for leaf item errors
  btrfs: extend btrfs_leaf_check to return btrfs_tree_block_status
  btrfs: add __btrfs_check_node helper
  btrfs: move btrfs_verify_level_key into tree-checker.c
  btrfs: move split_flags/combine_flags helpers to inode-item.h
  btrfs: add __KERNEL__ check for btrfs_no_printk
  btrfs: add a btrfs_csum_type_size helper
  btrfs: rename del_ptr -> btrfs_del_ptr and export it

 fs/btrfs/block-rsv.c        |  19 +++++
 fs/btrfs/block-rsv.h        |   2 +
 fs/btrfs/btrfs_inode.h      |  16 ----
 fs/btrfs/ctree.c            |  29 ++++---
 fs/btrfs/ctree.h            |   3 +
 fs/btrfs/disk-io.c          |  70 ++--------------
 fs/btrfs/disk-io.h          |   2 -
 fs/btrfs/extent-tree.c      |   7 +-
 fs/btrfs/extent-tree.h      |   2 +-
 fs/btrfs/free-space-cache.c |  19 -----
 fs/btrfs/free-space-cache.h |   2 -
 fs/btrfs/inode-item.h       |  16 ++++
 fs/btrfs/messages.h         |   7 ++
 fs/btrfs/tree-checker.c     | 157 ++++++++++++++++++++++++++----------
 fs/btrfs/tree-checker.h     |  29 ++++---
 15 files changed, 207 insertions(+), 173 deletions(-)

-- 
2.40.0

[PATCH 01/12] btrfs: move btrfs_check_trunc_cache_free_space into block-rsv.c

[Josef Bacik]

This is completely related to block rsv's, move it out of the free space
cache code and into block-rsv.c.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 fs/btrfs/block-rsv.c        | 19 +++++++++++++++++++
 fs/btrfs/block-rsv.h        |  2 ++
 fs/btrfs/free-space-cache.c | 19 -------------------
 fs/btrfs/free-space-cache.h |  2 --
 4 files changed, 21 insertions(+), 21 deletions(-)

diff --git a/fs/btrfs/block-rsv.c b/fs/btrfs/block-rsv.c
index 3ab707e26fa2..156ddb557004 100644
--- a/fs/btrfs/block-rsv.c
+++ b/fs/btrfs/block-rsv.c
@@ -540,3 +540,22 @@ struct btrfs_block_rsv *btrfs_use_block_rsv(struct btrfs_trans_handle *trans,
 
 	return ERR_PTR(ret);
 }
+
+int btrfs_check_trunc_cache_free_space(struct btrfs_fs_info *fs_info,
+				       struct btrfs_block_rsv *rsv)
+{
+	u64 needed_bytes;
+	int ret;
+
+	/* 1 for slack space, 1 for updating the inode */
+	needed_bytes = btrfs_calc_insert_metadata_size(fs_info, 1) +
+		btrfs_calc_metadata_size(fs_info, 1);
+
+	spin_lock(&rsv->lock);
+	if (rsv->reserved < needed_bytes)
+		ret = -ENOSPC;
+	else
+		ret = 0;
+	spin_unlock(&rsv->lock);
+	return ret;
+}
diff --git a/fs/btrfs/block-rsv.h b/fs/btrfs/block-rsv.h
index 6dc781709aca..b0bd12b8652f 100644
--- a/fs/btrfs/block-rsv.h
+++ b/fs/btrfs/block-rsv.h
@@ -82,6 +82,8 @@ void btrfs_release_global_block_rsv(struct btrfs_fs_info *fs_info);
 struct btrfs_block_rsv *btrfs_use_block_rsv(struct btrfs_trans_handle *trans,
 					    struct btrfs_root *root,
 					    u32 blocksize);
+int btrfs_check_trunc_cache_free_space(struct btrfs_fs_info *fs_info,
+				       struct btrfs_block_rsv *rsv);
 static inline void btrfs_unuse_block_rsv(struct btrfs_fs_info *fs_info,
 					 struct btrfs_block_rsv *block_rsv,
 					 u32 blocksize)
diff --git a/fs/btrfs/free-space-cache.c b/fs/btrfs/free-space-cache.c
index d84cef89cdff..5cef89162193 100644
--- a/fs/btrfs/free-space-cache.c
+++ b/fs/btrfs/free-space-cache.c
@@ -292,25 +292,6 @@ int btrfs_remove_free_space_inode(struct btrfs_trans_handle *trans,
 	return ret;
 }
 
-int btrfs_check_trunc_cache_free_space(struct btrfs_fs_info *fs_info,
-				       struct btrfs_block_rsv *rsv)
-{
-	u64 needed_bytes;
-	int ret;
-
-	/* 1 for slack space, 1 for updating the inode */
-	needed_bytes = btrfs_calc_insert_metadata_size(fs_info, 1) +
-		btrfs_calc_metadata_size(fs_info, 1);
-
-	spin_lock(&rsv->lock);
-	if (rsv->reserved < needed_bytes)
-		ret = -ENOSPC;
-	else
-		ret = 0;
-	spin_unlock(&rsv->lock);
-	return ret;
-}
-
 int btrfs_truncate_free_space_cache(struct btrfs_trans_handle *trans,
 				    struct btrfs_block_group *block_group,
 				    struct inode *vfs_inode)
diff --git a/fs/btrfs/free-space-cache.h b/fs/btrfs/free-space-cache.h
index a855e0483e03..33b4da3271b1 100644
--- a/fs/btrfs/free-space-cache.h
+++ b/fs/btrfs/free-space-cache.h
@@ -101,8 +101,6 @@ int btrfs_remove_free_space_inode(struct btrfs_trans_handle *trans,
 				  struct inode *inode,
 				  struct btrfs_block_group *block_group);
 
-int btrfs_check_trunc_cache_free_space(struct btrfs_fs_info *fs_info,
-				       struct btrfs_block_rsv *rsv);
 int btrfs_truncate_free_space_cache(struct btrfs_trans_handle *trans,
 				    struct btrfs_block_group *block_group,
 				    struct inode *inode);
-- 
2.40.0

Re: [PATCH 01/12] btrfs: move btrfs_check_trunc_cache_free_space into block-rsv.c

[Johannes Thumshirn]

Looks good,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

[PATCH 02/12] btrfs: remove level argument from btrfs_set_block_flags

[Josef Bacik]

We just pass in btrfs_header_level(eb) for the level, and we're passing
in the eb already, so simply get the level from the eb inside of
btrfs_set_block_flags.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 fs/btrfs/ctree.c       | 5 +----
 fs/btrfs/extent-tree.c | 7 +++----
 fs/btrfs/extent-tree.h | 2 +-
 3 files changed, 5 insertions(+), 9 deletions(-)

diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
index 2ff2961b1183..7071f90c23e3 100644
--- a/fs/btrfs/ctree.c
+++ b/fs/btrfs/ctree.c
@@ -464,10 +464,7 @@ static noinline int update_ref_for_cow(struct btrfs_trans_handle *trans,
 				return ret;
 		}
 		if (new_flags != 0) {
-			int level = btrfs_header_level(buf);
-
-			ret = btrfs_set_disk_extent_flags(trans, buf,
-							  new_flags, level);
+			ret = btrfs_set_disk_extent_flags(trans, buf, new_flags);
 			if (ret)
 				return ret;
 		}
diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index 5cd289de4e92..df8181ccb57b 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -2155,10 +2155,10 @@ int btrfs_run_delayed_refs(struct btrfs_trans_handle *trans,
 }
 
 int btrfs_set_disk_extent_flags(struct btrfs_trans_handle *trans,
-				struct extent_buffer *eb, u64 flags,
-				int level)
+				struct extent_buffer *eb, u64 flags)
 {
 	struct btrfs_delayed_extent_op *extent_op;
+	int level = btrfs_header_level(eb);
 	int ret;
 
 	extent_op = btrfs_alloc_delayed_extent_op();
@@ -5102,8 +5102,7 @@ static noinline int walk_down_proc(struct btrfs_trans_handle *trans,
 		BUG_ON(ret); /* -ENOMEM */
 		ret = btrfs_dec_ref(trans, root, eb, 0);
 		BUG_ON(ret); /* -ENOMEM */
-		ret = btrfs_set_disk_extent_flags(trans, eb, flag,
-						  btrfs_header_level(eb));
+		ret = btrfs_set_disk_extent_flags(trans, eb, flag);
 		BUG_ON(ret); /* -ENOMEM */
 		wc->flags[level] |= flag;
 	}
diff --git a/fs/btrfs/extent-tree.h b/fs/btrfs/extent-tree.h
index 0c958fc1b3b8..429d5c570061 100644
--- a/fs/btrfs/extent-tree.h
+++ b/fs/btrfs/extent-tree.h
@@ -141,7 +141,7 @@ int btrfs_inc_ref(struct btrfs_trans_handle *trans, struct btrfs_root *root,
 int btrfs_dec_ref(struct btrfs_trans_handle *trans, struct btrfs_root *root,
 		  struct extent_buffer *buf, int full_backref);
 int btrfs_set_disk_extent_flags(struct btrfs_trans_handle *trans,
-				struct extent_buffer *eb, u64 flags, int level);
+				struct extent_buffer *eb, u64 flags);
 int btrfs_free_extent(struct btrfs_trans_handle *trans, struct btrfs_ref *ref);
 
 int btrfs_free_reserved_extent(struct btrfs_fs_info *fs_info,
-- 
2.40.0

Re: [PATCH 02/12] btrfs: remove level argument from btrfs_set_block_flags

[Johannes Thumshirn]

Looks good,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

[PATCH 03/12] btrfs: simplify btrfs_check_leaf_* helpers into a single helper

[Josef Bacik]

We have two helpers for checking leaves, because we have an extra check
for debugging in btrfs_mark_buffer_dirty(), and at that stage we may
have item data that isn't consistent yet.  However we can handle this
case internally in the helper, if BTRFS_HEADER_FLAG_WRITTEN is set we
know the buffer should be internally consistent, otherwise we need to
skip checking the item data.

Simplify this helper down a single helper and handle the item data
checking logic internally to the helper.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 fs/btrfs/disk-io.c      | 12 +++++-------
 fs/btrfs/tree-checker.c | 19 +++++++------------
 fs/btrfs/tree-checker.h | 13 +------------
 3 files changed, 13 insertions(+), 31 deletions(-)

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 59ea049fe7ee..aea1ee834a80 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -325,7 +325,7 @@ static int csum_one_extent_buffer(struct extent_buffer *eb)
 	if (btrfs_header_level(eb))
 		ret = btrfs_check_node(eb);
 	else
-		ret = btrfs_check_leaf_full(eb);
+		ret = btrfs_check_leaf(eb);
 
 	if (ret < 0)
 		goto error;
@@ -582,7 +582,7 @@ static int validate_extent_buffer(struct extent_buffer *eb,
 	 * that we don't try and read the other copies of this block, just
 	 * return -EIO.
 	 */
-	if (found_level == 0 && btrfs_check_leaf_full(eb)) {
+	if (found_level == 0 && btrfs_check_leaf(eb)) {
 		set_bit(EXTENT_BUFFER_CORRUPT, &eb->bflags);
 		ret = -EIO;
 	}
@@ -4687,12 +4687,10 @@ void btrfs_mark_buffer_dirty(struct extent_buffer *buf)
 					 fs_info->dirty_metadata_batch);
 #ifdef CONFIG_BTRFS_FS_CHECK_INTEGRITY
 	/*
-	 * Since btrfs_mark_buffer_dirty() can be called with item pointer set
-	 * but item data not updated.
-	 * So here we should only check item pointers, not item data.
+	 * btrfs_check_leaf() won't check item data if we don't have WRITTEN
+	 * set, so this will only validate the basic structure of the items.
 	 */
-	if (btrfs_header_level(buf) == 0 &&
-	    btrfs_check_leaf_relaxed(buf)) {
+	if (btrfs_header_level(buf) == 0 && btrfs_check_leaf(buf)) {
 		btrfs_print_leaf(buf);
 		ASSERT(0);
 	}
diff --git a/fs/btrfs/tree-checker.c b/fs/btrfs/tree-checker.c
index e2b54793bf0c..f153ddc60ba1 100644
--- a/fs/btrfs/tree-checker.c
+++ b/fs/btrfs/tree-checker.c
@@ -1674,7 +1674,7 @@ static int check_leaf_item(struct extent_buffer *leaf,
 	return ret;
 }
 
-static int check_leaf(struct extent_buffer *leaf, bool check_item_data)
+int btrfs_check_leaf(struct extent_buffer *leaf)
 {
 	struct btrfs_fs_info *fs_info = leaf->fs_info;
 	/* No valid key type is 0, so all key should be larger than this key */
@@ -1682,6 +1682,7 @@ static int check_leaf(struct extent_buffer *leaf, bool check_item_data)
 	struct btrfs_key key;
 	u32 nritems = btrfs_header_nritems(leaf);
 	int slot;
+	bool check_item_data = btrfs_header_flag(leaf, BTRFS_HEADER_FLAG_WRITTEN);
 
 	if (unlikely(btrfs_header_level(leaf) != 0)) {
 		generic_err(leaf, 0,
@@ -1807,6 +1808,10 @@ static int check_leaf(struct extent_buffer *leaf, bool check_item_data)
 			return -EUCLEAN;
 		}
 
+		/*
+		 * We only want to do this if WRITTEN is set, otherwise the leaf
+		 * may be in some intermediate state and won't appear valid.
+		 */
 		if (check_item_data) {
 			/*
 			 * Check if the item size and content meet other
@@ -1824,17 +1829,7 @@ static int check_leaf(struct extent_buffer *leaf, bool check_item_data)
 
 	return 0;
 }
-
-int btrfs_check_leaf_full(struct extent_buffer *leaf)
-{
-	return check_leaf(leaf, true);
-}
-ALLOW_ERROR_INJECTION(btrfs_check_leaf_full, ERRNO);
-
-int btrfs_check_leaf_relaxed(struct extent_buffer *leaf)
-{
-	return check_leaf(leaf, false);
-}
+ALLOW_ERROR_INJECTION(btrfs_check_leaf, ERRNO);
 
 int btrfs_check_node(struct extent_buffer *node)
 {
diff --git a/fs/btrfs/tree-checker.h b/fs/btrfs/tree-checker.h
index bfb5efa4e01f..48321e8d91bb 100644
--- a/fs/btrfs/tree-checker.h
+++ b/fs/btrfs/tree-checker.h
@@ -40,18 +40,7 @@ struct btrfs_tree_parent_check {
 	u8 level;
 };
 
-/*
- * Comprehensive leaf checker.
- * Will check not only the item pointers, but also every possible member
- * in item data.
- */
-int btrfs_check_leaf_full(struct extent_buffer *leaf);
-
-/*
- * Less strict leaf checker.
- * Will only check item pointers, not reading item data.
- */
-int btrfs_check_leaf_relaxed(struct extent_buffer *leaf);
+int btrfs_check_leaf(struct extent_buffer *leaf);
 int btrfs_check_node(struct extent_buffer *node);
 
 int btrfs_check_chunk_valid(struct extent_buffer *leaf,
-- 
2.40.0

Re: [PATCH 03/12] btrfs: simplify btrfs_check_leaf_* helpers into a single helper

[Johannes Thumshirn]

On 29.04.23 22:07, Josef Bacik wrote:
> -static int check_leaf(struct extent_buffer *leaf, bool check_item_data)
> +int btrfs_check_leaf(struct extent_buffer *leaf)
>  {
>  	struct btrfs_fs_info *fs_info = leaf->fs_info;
>  	/* No valid key type is 0, so all key should be larger than this key */
> @@ -1682,6 +1682,7 @@ static int check_leaf(struct extent_buffer *leaf, bool check_item_data)
>  	struct btrfs_key key;
>  	u32 nritems = btrfs_header_nritems(leaf);
>  	int slot;
> +	bool check_item_data = btrfs_header_flag(leaf, BTRFS_HEADER_FLAG_WRITTEN);
>  
>  	if (unlikely(btrfs_header_level(leaf) != 0)) {
>  		generic_err(leaf, 0,
> @@ -1807,6 +1808,10 @@ static int check_leaf(struct extent_buffer *leaf, bool check_item_data)
>  			return -EUCLEAN;
>  		}
>  
> +		/*
> +		 * We only want to do this if WRITTEN is set, otherwise the leaf
> +		 * may be in some intermediate state and won't appear valid.
> +		 */
>  		if (check_item_data) {


Nit: I'd even go as far as:

diff --git a/fs/btrfs/tree-checker.c b/fs/btrfs/tree-checker.c
index f153ddc60ba1..2eff4e2f2c47 100644
--- a/fs/btrfs/tree-checker.c
+++ b/fs/btrfs/tree-checker.c
@@ -1682,7 +1682,6 @@ int btrfs_check_leaf(struct extent_buffer *leaf)
        struct btrfs_key key;
        u32 nritems = btrfs_header_nritems(leaf);
        int slot;
-       bool check_item_data = btrfs_header_flag(leaf, BTRFS_HEADER_FLAG_WRITTEN);
 
        if (unlikely(btrfs_header_level(leaf) != 0)) {
                generic_err(leaf, 0,
@@ -1812,7 +1811,7 @@ int btrfs_check_leaf(struct extent_buffer *leaf)
                 * We only want to do this if WRITTEN is set, otherwise the leaf
                 * may be in some intermediate state and won't appear valid.
                 */
-               if (check_item_data) {
+               if (btrfs_header_flag(leaf, BTRFS_HEADER_FLAG_WRITTEN)) {
                        /*
                         * Check if the item size and content meet other
                         * criteria

Otherwise,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

Re: [PATCH 03/12] btrfs: simplify btrfs_check_leaf_* helpers into a single helper

[David Sterba]

On Tue, May 02, 2023 at 11:27:35AM +0000, Johannes Thumshirn wrote:
> On 29.04.23 22:07, Josef Bacik wrote:
> > -static int check_leaf(struct extent_buffer *leaf, bool check_item_data)
> > +int btrfs_check_leaf(struct extent_buffer *leaf)
> >  {
> >  	struct btrfs_fs_info *fs_info = leaf->fs_info;
> >  	/* No valid key type is 0, so all key should be larger than this key */
> > @@ -1682,6 +1682,7 @@ static int check_leaf(struct extent_buffer *leaf, bool check_item_data)
> >  	struct btrfs_key key;
> >  	u32 nritems = btrfs_header_nritems(leaf);
> >  	int slot;
> > +	bool check_item_data = btrfs_header_flag(leaf, BTRFS_HEADER_FLAG_WRITTEN);
> >  
> >  	if (unlikely(btrfs_header_level(leaf) != 0)) {
> >  		generic_err(leaf, 0,
> > @@ -1807,6 +1808,10 @@ static int check_leaf(struct extent_buffer *leaf, bool check_item_data)
> >  			return -EUCLEAN;
> >  		}
> >  
> > +		/*
> > +		 * We only want to do this if WRITTEN is set, otherwise the leaf
> > +		 * may be in some intermediate state and won't appear valid.
> > +		 */
> >  		if (check_item_data) {
> 
> 
> Nit: I'd even go as far as:
> 
> diff --git a/fs/btrfs/tree-checker.c b/fs/btrfs/tree-checker.c
> index f153ddc60ba1..2eff4e2f2c47 100644
> --- a/fs/btrfs/tree-checker.c
> +++ b/fs/btrfs/tree-checker.c
> @@ -1682,7 +1682,6 @@ int btrfs_check_leaf(struct extent_buffer *leaf)
>         struct btrfs_key key;
>         u32 nritems = btrfs_header_nritems(leaf);
>         int slot;
> -       bool check_item_data = btrfs_header_flag(leaf, BTRFS_HEADER_FLAG_WRITTEN);
>  
>         if (unlikely(btrfs_header_level(leaf) != 0)) {
>                 generic_err(leaf, 0,
> @@ -1812,7 +1811,7 @@ int btrfs_check_leaf(struct extent_buffer *leaf)
>                  * We only want to do this if WRITTEN is set, otherwise the leaf
>                  * may be in some intermediate state and won't appear valid.
>                  */
> -               if (check_item_data) {
> +               if (btrfs_header_flag(leaf, BTRFS_HEADER_FLAG_WRITTEN)) {
>                         /*
>                          * Check if the item size and content meet other
>                          * criteria

Updated in the commit.

[PATCH 04/12] btrfs: add btrfs_tree_block_status definitions to tree-checker.h

[Josef Bacik]

We use this in btrfs-progs to determine if we can fix different types of
corruptions.  We don't care about this in the kernel, however it would
be good to share this code between the kernel and btrfs-progs, so add
the status definitions so we can start converting the tree-checker code
over to using these status flags instead of blanket returning -EUCLEAN.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 fs/btrfs/tree-checker.h | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/fs/btrfs/tree-checker.h b/fs/btrfs/tree-checker.h
index 48321e8d91bb..5e06d9ad2862 100644
--- a/fs/btrfs/tree-checker.h
+++ b/fs/btrfs/tree-checker.h
@@ -40,6 +40,18 @@ struct btrfs_tree_parent_check {
 	u8 level;
 };
 
+enum btrfs_tree_block_status {
+	BTRFS_TREE_BLOCK_CLEAN,
+	BTRFS_TREE_BLOCK_INVALID_NRITEMS,
+	BTRFS_TREE_BLOCK_INVALID_PARENT_KEY,
+	BTRFS_TREE_BLOCK_BAD_KEY_ORDER,
+	BTRFS_TREE_BLOCK_INVALID_LEVEL,
+	BTRFS_TREE_BLOCK_INVALID_FREE_SPACE,
+	BTRFS_TREE_BLOCK_INVALID_OFFSETS,
+	BTRFS_TREE_BLOCK_INVALID_BLOCKPTR,
+	BTRFS_TREE_BLOCK_INVALID_ITEM,
+};
+
 int btrfs_check_leaf(struct extent_buffer *leaf);
 int btrfs_check_node(struct extent_buffer *node);
 
-- 
2.40.0

Re: [PATCH 04/12] btrfs: add btrfs_tree_block_status definitions to tree-checker.h

[Johannes Thumshirn]

Looks good,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

[PATCH 05/12] btrfs: use btrfs_tree_block_status for leaf item errors

[Josef Bacik]

We have a variety of item specific errors that can occur.  For now
simply put these under the umbrella of BTRFS_TREE_BLOCK_INVALID_ITEM,
this can be fleshed out as we need in the future.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 fs/btrfs/tree-checker.c | 19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)

diff --git a/fs/btrfs/tree-checker.c b/fs/btrfs/tree-checker.c
index f153ddc60ba1..bfc1f65726f6 100644
--- a/fs/btrfs/tree-checker.c
+++ b/fs/btrfs/tree-checker.c
@@ -1620,9 +1620,10 @@ static int check_inode_ref(struct extent_buffer *leaf,
 /*
  * Common point to switch the item-specific validation.
  */
-static int check_leaf_item(struct extent_buffer *leaf,
-			   struct btrfs_key *key, int slot,
-			   struct btrfs_key *prev_key)
+static enum btrfs_tree_block_status check_leaf_item(struct extent_buffer *leaf,
+						    struct btrfs_key *key,
+						    int slot,
+						    struct btrfs_key *prev_key)
 {
 	int ret = 0;
 	struct btrfs_chunk *chunk;
@@ -1671,7 +1672,10 @@ static int check_leaf_item(struct extent_buffer *leaf,
 		ret = check_extent_data_ref(leaf, key, slot);
 		break;
 	}
-	return ret;
+
+	if (ret)
+		return BTRFS_TREE_BLOCK_INVALID_ITEM;
+	return BTRFS_TREE_BLOCK_CLEAN;
 }
 
 int btrfs_check_leaf(struct extent_buffer *leaf)
@@ -1752,7 +1756,6 @@ int btrfs_check_leaf(struct extent_buffer *leaf)
 	for (slot = 0; slot < nritems; slot++) {
 		u32 item_end_expected;
 		u64 item_data_end;
-		int ret;
 
 		btrfs_item_key_to_cpu(leaf, &key, slot);
 
@@ -1813,13 +1816,15 @@ int btrfs_check_leaf(struct extent_buffer *leaf)
 		 * may be in some intermediate state and won't appear valid.
 		 */
 		if (check_item_data) {
+			enum btrfs_tree_block_status ret;
+
 			/*
 			 * Check if the item size and content meet other
 			 * criteria
 			 */
 			ret = check_leaf_item(leaf, &key, slot, &prev_key);
-			if (unlikely(ret < 0))
-				return ret;
+			if (unlikely(ret != BTRFS_TREE_BLOCK_CLEAN))
+				return -EUCLEAN;
 		}
 
 		prev_key.objectid = key.objectid;
-- 
2.40.0

Re: [PATCH 05/12] btrfs: use btrfs_tree_block_status for leaf item errors

[Johannes Thumshirn]

Looks good,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

[PATCH 06/12] btrfs: extend btrfs_leaf_check to return btrfs_tree_block_status

[Josef Bacik]

Instead of blanket returning -EUCLEAN for all the failures in
btrfs_check_leaf, use btrfs_tree_block_status and return the appropriate
status for each failure.  Rename the helper to __btrfs_check_leaf and
then make a wrapper of btrfs_check_leaf that will return -EUCLEAN to
non-clean error codes.  This will allow us to have the
__btrfs_check_leaf variant in btrfs-progs while keeping the behavior in
the kernel consistent.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 fs/btrfs/tree-checker.c | 36 +++++++++++++++++++++++-------------
 fs/btrfs/tree-checker.h |  7 +++++++
 2 files changed, 30 insertions(+), 13 deletions(-)

diff --git a/fs/btrfs/tree-checker.c b/fs/btrfs/tree-checker.c
index bfc1f65726f6..2c330e9d123a 100644
--- a/fs/btrfs/tree-checker.c
+++ b/fs/btrfs/tree-checker.c
@@ -1678,7 +1678,7 @@ static enum btrfs_tree_block_status check_leaf_item(struct extent_buffer *leaf,
 	return BTRFS_TREE_BLOCK_CLEAN;
 }
 
-int btrfs_check_leaf(struct extent_buffer *leaf)
+enum btrfs_tree_block_status __btrfs_check_leaf(struct extent_buffer *leaf)
 {
 	struct btrfs_fs_info *fs_info = leaf->fs_info;
 	/* No valid key type is 0, so all key should be larger than this key */
@@ -1692,7 +1692,7 @@ int btrfs_check_leaf(struct extent_buffer *leaf)
 		generic_err(leaf, 0,
 			"invalid level for leaf, have %d expect 0",
 			btrfs_header_level(leaf));
-		return -EUCLEAN;
+		return BTRFS_TREE_BLOCK_INVALID_LEVEL;
 	}
 
 	/*
@@ -1715,32 +1715,32 @@ int btrfs_check_leaf(struct extent_buffer *leaf)
 			generic_err(leaf, 0,
 			"invalid root, root %llu must never be empty",
 				    owner);
-			return -EUCLEAN;
+			return BTRFS_TREE_BLOCK_INVALID_NRITEMS;
 		}
 
 		/* Unknown tree */
 		if (unlikely(owner == 0)) {
 			generic_err(leaf, 0,
 				"invalid owner, root 0 is not defined");
-			return -EUCLEAN;
+			return BTRFS_TREE_BLOCK_INVALID_OWNER;
 		}
 
 		/* EXTENT_TREE_V2 can have empty extent trees. */
 		if (btrfs_fs_incompat(fs_info, EXTENT_TREE_V2))
-			return 0;
+			return BTRFS_TREE_BLOCK_CLEAN;
 
 		if (unlikely(owner == BTRFS_EXTENT_TREE_OBJECTID)) {
 			generic_err(leaf, 0,
 			"invalid root, root %llu must never be empty",
 				    owner);
-			return -EUCLEAN;
+			return BTRFS_TREE_BLOCK_INVALID_NRITEMS;
 		}
 
-		return 0;
+		return BTRFS_TREE_BLOCK_CLEAN;
 	}
 
 	if (unlikely(nritems == 0))
-		return 0;
+		return BTRFS_TREE_BLOCK_CLEAN;
 
 	/*
 	 * Check the following things to make sure this is a good leaf, and
@@ -1766,7 +1766,7 @@ int btrfs_check_leaf(struct extent_buffer *leaf)
 				prev_key.objectid, prev_key.type,
 				prev_key.offset, key.objectid, key.type,
 				key.offset);
-			return -EUCLEAN;
+			return BTRFS_TREE_BLOCK_BAD_KEY_ORDER;
 		}
 
 		item_data_end = (u64)btrfs_item_offset(leaf, slot) +
@@ -1785,7 +1785,7 @@ int btrfs_check_leaf(struct extent_buffer *leaf)
 			generic_err(leaf, slot,
 				"unexpected item end, have %llu expect %u",
 				item_data_end, item_end_expected);
-			return -EUCLEAN;
+			return BTRFS_TREE_BLOCK_INVALID_OFFSETS;
 		}
 
 		/*
@@ -1797,7 +1797,7 @@ int btrfs_check_leaf(struct extent_buffer *leaf)
 			generic_err(leaf, slot,
 			"slot end outside of leaf, have %llu expect range [0, %u]",
 				item_data_end, BTRFS_LEAF_DATA_SIZE(fs_info));
-			return -EUCLEAN;
+			return BTRFS_TREE_BLOCK_INVALID_OFFSETS;
 		}
 
 		/* Also check if the item pointer overlaps with btrfs item. */
@@ -1808,7 +1808,7 @@ int btrfs_check_leaf(struct extent_buffer *leaf)
 				btrfs_item_nr_offset(leaf, slot) +
 				sizeof(struct btrfs_item),
 				btrfs_item_ptr_offset(leaf, slot));
-			return -EUCLEAN;
+			return BTRFS_TREE_BLOCK_INVALID_OFFSETS;
 		}
 
 		/*
@@ -1824,7 +1824,7 @@ int btrfs_check_leaf(struct extent_buffer *leaf)
 			 */
 			ret = check_leaf_item(leaf, &key, slot, &prev_key);
 			if (unlikely(ret != BTRFS_TREE_BLOCK_CLEAN))
-				return -EUCLEAN;
+				return ret;
 		}
 
 		prev_key.objectid = key.objectid;
@@ -1832,6 +1832,16 @@ int btrfs_check_leaf(struct extent_buffer *leaf)
 		prev_key.offset = key.offset;
 	}
 
+	return BTRFS_TREE_BLOCK_CLEAN;
+}
+
+int btrfs_check_leaf(struct extent_buffer *leaf)
+{
+	enum btrfs_tree_block_status ret;
+
+	ret = __btrfs_check_leaf(leaf);
+	if (unlikely(ret != BTRFS_TREE_BLOCK_CLEAN))
+		return -EUCLEAN;
 	return 0;
 }
 ALLOW_ERROR_INJECTION(btrfs_check_leaf, ERRNO);
diff --git a/fs/btrfs/tree-checker.h b/fs/btrfs/tree-checker.h
index 5e06d9ad2862..3b8de6d36141 100644
--- a/fs/btrfs/tree-checker.h
+++ b/fs/btrfs/tree-checker.h
@@ -50,8 +50,15 @@ enum btrfs_tree_block_status {
 	BTRFS_TREE_BLOCK_INVALID_OFFSETS,
 	BTRFS_TREE_BLOCK_INVALID_BLOCKPTR,
 	BTRFS_TREE_BLOCK_INVALID_ITEM,
+	BTRFS_TREE_BLOCK_INVALID_OWNER,
 };
 
+/*
+ * Exported simply for btrfs-progs which wants to have the
+ * btrfs_tree_block_status return codes.
+ */
+enum btrfs_tree_block_status __btrfs_check_leaf(struct extent_buffer *leaf);
+
 int btrfs_check_leaf(struct extent_buffer *leaf);
 int btrfs_check_node(struct extent_buffer *node);
 
-- 
2.40.0

Re: [PATCH 06/12] btrfs: extend btrfs_leaf_check to return btrfs_tree_block_status

[Johannes Thumshirn]

On 29.04.23 22:08, Josef Bacik wrote:
> diff --git a/fs/btrfs/tree-checker.h b/fs/btrfs/tree-checker.h
> index 5e06d9ad2862..3b8de6d36141 100644
> --- a/fs/btrfs/tree-checker.h
> +++ b/fs/btrfs/tree-checker.h
> @@ -50,8 +50,15 @@ enum btrfs_tree_block_status {
>  	BTRFS_TREE_BLOCK_INVALID_OFFSETS,
>  	BTRFS_TREE_BLOCK_INVALID_BLOCKPTR,
>  	BTRFS_TREE_BLOCK_INVALID_ITEM,
> +	BTRFS_TREE_BLOCK_INVALID_OWNER,
>  };

Why didn't you add 'BTRFS_TREE_BLOCK_INVALID_OWNER' in patch 4?

Re: [PATCH 06/12] btrfs: extend btrfs_leaf_check to return btrfs_tree_block_status

[David Sterba]

On Tue, May 02, 2023 at 12:03:54PM +0000, Johannes Thumshirn wrote:
> On 29.04.23 22:08, Josef Bacik wrote:
> > diff --git a/fs/btrfs/tree-checker.h b/fs/btrfs/tree-checker.h
> > index 5e06d9ad2862..3b8de6d36141 100644
> > --- a/fs/btrfs/tree-checker.h
> > +++ b/fs/btrfs/tree-checker.h
> > @@ -50,8 +50,15 @@ enum btrfs_tree_block_status {
> >  	BTRFS_TREE_BLOCK_INVALID_OFFSETS,
> >  	BTRFS_TREE_BLOCK_INVALID_BLOCKPTR,
> >  	BTRFS_TREE_BLOCK_INVALID_ITEM,
> > +	BTRFS_TREE_BLOCK_INVALID_OWNER,
> >  };
> 
> Why didn't you add 'BTRFS_TREE_BLOCK_INVALID_OWNER' in patch 4?

Moved to the patch adding all the other enums.

[PATCH 07/12] btrfs: add __btrfs_check_node helper

[Josef Bacik]

This helper returns a btrfs_tree_block_status for the various errors,
and then btrfs_check_node() will return -EUCLEAN if it gets anything
other than BTRFS_TREE_BLOCK_CLEAN which will be used by the kernel.  In
the future btrfs-progs will use this helper instead.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 fs/btrfs/tree-checker.c | 29 +++++++++++++++++------------
 fs/btrfs/tree-checker.h |  1 +
 2 files changed, 18 insertions(+), 12 deletions(-)

diff --git a/fs/btrfs/tree-checker.c b/fs/btrfs/tree-checker.c
index 2c330e9d123a..eb9ba48d92aa 100644
--- a/fs/btrfs/tree-checker.c
+++ b/fs/btrfs/tree-checker.c
@@ -1846,7 +1846,7 @@ int btrfs_check_leaf(struct extent_buffer *leaf)
 }
 ALLOW_ERROR_INJECTION(btrfs_check_leaf, ERRNO);
 
-int btrfs_check_node(struct extent_buffer *node)
+enum btrfs_tree_block_status __btrfs_check_node(struct extent_buffer *node)
 {
 	struct btrfs_fs_info *fs_info = node->fs_info;
 	unsigned long nr = btrfs_header_nritems(node);
@@ -1854,13 +1854,12 @@ int btrfs_check_node(struct extent_buffer *node)
 	int slot;
 	int level = btrfs_header_level(node);
 	u64 bytenr;
-	int ret = 0;
 
 	if (unlikely(level <= 0 || level >= BTRFS_MAX_LEVEL)) {
 		generic_err(node, 0,
 			"invalid level for node, have %d expect [1, %d]",
 			level, BTRFS_MAX_LEVEL - 1);
-		return -EUCLEAN;
+		return BTRFS_TREE_BLOCK_INVALID_LEVEL;
 	}
 	if (unlikely(nr == 0 || nr > BTRFS_NODEPTRS_PER_BLOCK(fs_info))) {
 		btrfs_crit(fs_info,
@@ -1868,7 +1867,7 @@ int btrfs_check_node(struct extent_buffer *node)
 			   btrfs_header_owner(node), node->start,
 			   nr == 0 ? "small" : "large", nr,
 			   BTRFS_NODEPTRS_PER_BLOCK(fs_info));
-		return -EUCLEAN;
+		return BTRFS_TREE_BLOCK_INVALID_NRITEMS;
 	}
 
 	for (slot = 0; slot < nr - 1; slot++) {
@@ -1879,15 +1878,13 @@ int btrfs_check_node(struct extent_buffer *node)
 		if (unlikely(!bytenr)) {
 			generic_err(node, slot,
 				"invalid NULL node pointer");
-			ret = -EUCLEAN;
-			goto out;
+			return BTRFS_TREE_BLOCK_INVALID_BLOCKPTR;
 		}
 		if (unlikely(!IS_ALIGNED(bytenr, fs_info->sectorsize))) {
 			generic_err(node, slot,
 			"unaligned pointer, have %llu should be aligned to %u",
 				bytenr, fs_info->sectorsize);
-			ret = -EUCLEAN;
-			goto out;
+			return BTRFS_TREE_BLOCK_INVALID_BLOCKPTR;
 		}
 
 		if (unlikely(btrfs_comp_cpu_keys(&key, &next_key) >= 0)) {
@@ -1896,12 +1893,20 @@ int btrfs_check_node(struct extent_buffer *node)
 				key.objectid, key.type, key.offset,
 				next_key.objectid, next_key.type,
 				next_key.offset);
-			ret = -EUCLEAN;
-			goto out;
+			return BTRFS_TREE_BLOCK_BAD_KEY_ORDER;
 		}
 	}
-out:
-	return ret;
+	return BTRFS_TREE_BLOCK_CLEAN;
+}
+
+int btrfs_check_node(struct extent_buffer *node)
+{
+	enum btrfs_tree_block_status ret;
+
+	ret = __btrfs_check_node(node);
+	if (unlikely(ret != BTRFS_TREE_BLOCK_CLEAN))
+		return -EUCLEAN;
+	return 0;
 }
 ALLOW_ERROR_INJECTION(btrfs_check_node, ERRNO);
 
diff --git a/fs/btrfs/tree-checker.h b/fs/btrfs/tree-checker.h
index 3b8de6d36141..c0861ce1429b 100644
--- a/fs/btrfs/tree-checker.h
+++ b/fs/btrfs/tree-checker.h
@@ -58,6 +58,7 @@ enum btrfs_tree_block_status {
  * btrfs_tree_block_status return codes.
  */
 enum btrfs_tree_block_status __btrfs_check_leaf(struct extent_buffer *leaf);
+enum btrfs_tree_block_status __btrfs_check_node(struct extent_buffer *node);
 
 int btrfs_check_leaf(struct extent_buffer *leaf);
 int btrfs_check_node(struct extent_buffer *node);
-- 
2.40.0

Re: [PATCH 07/12] btrfs: add __btrfs_check_node helper

[Johannes Thumshirn]

On 29.04.23 22:08, Josef Bacik wrote:
> diff --git a/fs/btrfs/tree-checker.h b/fs/btrfs/tree-checker.h
> index 3b8de6d36141..c0861ce1429b 100644
> --- a/fs/btrfs/tree-checker.h
> +++ b/fs/btrfs/tree-checker.h
> @@ -58,6 +58,7 @@ enum btrfs_tree_block_status {
>   * btrfs_tree_block_status return codes.
>   */
>  enum btrfs_tree_block_status __btrfs_check_leaf(struct extent_buffer *leaf);
> +enum btrfs_tree_block_status __btrfs_check_node(struct extent_buffer *node);

Sorry for only  noticing now, but shouldn't we do something like

#ifndef KERNEL
enum btrfs_tree_block_status __btrfs_check_leaf(struct extent_buffer *leaf);
enum btrfs_tree_block_status __btrfs_check_node(struct extent_buffer *node);
#endif

and in fs/btrfs/tree-checker.c (or fs.h):

#ifdef KERNEL
#define EXPORT_FOR_PROGS static
#else
#define EXPORT_FOR_PROGS
#endif

Just like we did with EXPORT_FOR_TESTS?

Re: [PATCH 07/12] btrfs: add __btrfs_check_node helper

[David Sterba]

On Tue, May 02, 2023 at 12:10:20PM +0000, Johannes Thumshirn wrote:
> On 29.04.23 22:08, Josef Bacik wrote:
> > diff --git a/fs/btrfs/tree-checker.h b/fs/btrfs/tree-checker.h
> > index 3b8de6d36141..c0861ce1429b 100644
> > --- a/fs/btrfs/tree-checker.h
> > +++ b/fs/btrfs/tree-checker.h
> > @@ -58,6 +58,7 @@ enum btrfs_tree_block_status {
> >   * btrfs_tree_block_status return codes.
> >   */
> >  enum btrfs_tree_block_status __btrfs_check_leaf(struct extent_buffer *leaf);
> > +enum btrfs_tree_block_status __btrfs_check_node(struct extent_buffer *node);
> 
> Sorry for only  noticing now, but shouldn't we do something like
> 
> #ifndef KERNEL
> enum btrfs_tree_block_status __btrfs_check_leaf(struct extent_buffer *leaf);
> enum btrfs_tree_block_status __btrfs_check_node(struct extent_buffer *node);
> #endif
> 
> and in fs/btrfs/tree-checker.c (or fs.h):
> 
> #ifdef KERNEL
> #define EXPORT_FOR_PROGS static
> #else
> #define EXPORT_FOR_PROGS
> #endif
> 
> Just like we did with EXPORT_FOR_TESTS?

That's a good idea. Could we do it at the end of the series though? It's
not the cleanest way but the patches are otherwise simple and we won't
need to backport them so having the change split like that should not
cause trouble. I can also edit the patches in place if we insist.

With a separate patch I'd like to see how the conditional definitions
for kernel and userspace should be done first.

[PATCH 08/12] btrfs: move btrfs_verify_level_key into tree-checker.c

[Josef Bacik]

This is more a buffer validation helper, move it into the tree-checker
files where it makes more sense.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 fs/btrfs/disk-io.c      | 58 -----------------------------------------
 fs/btrfs/disk-io.h      |  2 --
 fs/btrfs/tree-checker.c | 58 +++++++++++++++++++++++++++++++++++++++++
 fs/btrfs/tree-checker.h |  2 ++
 4 files changed, 60 insertions(+), 60 deletions(-)

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index aea1ee834a80..cd0e8f394420 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -180,64 +180,6 @@ int btrfs_check_super_csum(struct btrfs_fs_info *fs_info,
 	return 0;
 }
 
-int btrfs_verify_level_key(struct extent_buffer *eb, int level,
-			   struct btrfs_key *first_key, u64 parent_transid)
-{
-	struct btrfs_fs_info *fs_info = eb->fs_info;
-	int found_level;
-	struct btrfs_key found_key;
-	int ret;
-
-	found_level = btrfs_header_level(eb);
-	if (found_level != level) {
-		WARN(IS_ENABLED(CONFIG_BTRFS_DEBUG),
-		     KERN_ERR "BTRFS: tree level check failed\n");
-		btrfs_err(fs_info,
-"tree level mismatch detected, bytenr=%llu level expected=%u has=%u",
-			  eb->start, level, found_level);
-		return -EIO;
-	}
-
-	if (!first_key)
-		return 0;
-
-	/*
-	 * For live tree block (new tree blocks in current transaction),
-	 * we need proper lock context to avoid race, which is impossible here.
-	 * So we only checks tree blocks which is read from disk, whose
-	 * generation <= fs_info->last_trans_committed.
-	 */
-	if (btrfs_header_generation(eb) > fs_info->last_trans_committed)
-		return 0;
-
-	/* We have @first_key, so this @eb must have at least one item */
-	if (btrfs_header_nritems(eb) == 0) {
-		btrfs_err(fs_info,
-		"invalid tree nritems, bytenr=%llu nritems=0 expect >0",
-			  eb->start);
-		WARN_ON(IS_ENABLED(CONFIG_BTRFS_DEBUG));
-		return -EUCLEAN;
-	}
-
-	if (found_level)
-		btrfs_node_key_to_cpu(eb, &found_key, 0);
-	else
-		btrfs_item_key_to_cpu(eb, &found_key, 0);
-	ret = btrfs_comp_cpu_keys(first_key, &found_key);
-
-	if (ret) {
-		WARN(IS_ENABLED(CONFIG_BTRFS_DEBUG),
-		     KERN_ERR "BTRFS: tree first key check failed\n");
-		btrfs_err(fs_info,
-"tree first key mismatch detected, bytenr=%llu parent_transid=%llu key expected=(%llu,%u,%llu) has=(%llu,%u,%llu)",
-			  eb->start, parent_transid, first_key->objectid,
-			  first_key->type, first_key->offset,
-			  found_key.objectid, found_key.type,
-			  found_key.offset);
-	}
-	return ret;
-}
-
 static int btrfs_repair_eb_io_failure(const struct extent_buffer *eb,
 				      int mirror_num)
 {
diff --git a/fs/btrfs/disk-io.h b/fs/btrfs/disk-io.h
index 4d5772330110..a26ab3cbb449 100644
--- a/fs/btrfs/disk-io.h
+++ b/fs/btrfs/disk-io.h
@@ -31,8 +31,6 @@ struct btrfs_tree_parent_check;
 
 void btrfs_check_leaked_roots(struct btrfs_fs_info *fs_info);
 void btrfs_init_fs_info(struct btrfs_fs_info *fs_info);
-int btrfs_verify_level_key(struct extent_buffer *eb, int level,
-			   struct btrfs_key *first_key, u64 parent_transid);
 struct extent_buffer *read_tree_block(struct btrfs_fs_info *fs_info, u64 bytenr,
 				      struct btrfs_tree_parent_check *check);
 struct extent_buffer *btrfs_find_create_tree_block(
diff --git a/fs/btrfs/tree-checker.c b/fs/btrfs/tree-checker.c
index eb9ba48d92aa..a1038156d57d 100644
--- a/fs/btrfs/tree-checker.c
+++ b/fs/btrfs/tree-checker.c
@@ -1964,3 +1964,61 @@ int btrfs_check_eb_owner(const struct extent_buffer *eb, u64 root_owner)
 	}
 	return 0;
 }
+
+int btrfs_verify_level_key(struct extent_buffer *eb, int level,
+			   struct btrfs_key *first_key, u64 parent_transid)
+{
+	struct btrfs_fs_info *fs_info = eb->fs_info;
+	int found_level;
+	struct btrfs_key found_key;
+	int ret;
+
+	found_level = btrfs_header_level(eb);
+	if (found_level != level) {
+		WARN(IS_ENABLED(CONFIG_BTRFS_DEBUG),
+		     KERN_ERR "BTRFS: tree level check failed\n");
+		btrfs_err(fs_info,
+"tree level mismatch detected, bytenr=%llu level expected=%u has=%u",
+			  eb->start, level, found_level);
+		return -EIO;
+	}
+
+	if (!first_key)
+		return 0;
+
+	/*
+	 * For live tree block (new tree blocks in current transaction),
+	 * we need proper lock context to avoid race, which is impossible here.
+	 * So we only checks tree blocks which is read from disk, whose
+	 * generation <= fs_info->last_trans_committed.
+	 */
+	if (btrfs_header_generation(eb) > fs_info->last_trans_committed)
+		return 0;
+
+	/* We have @first_key, so this @eb must have at least one item */
+	if (btrfs_header_nritems(eb) == 0) {
+		btrfs_err(fs_info,
+		"invalid tree nritems, bytenr=%llu nritems=0 expect >0",
+			  eb->start);
+		WARN_ON(IS_ENABLED(CONFIG_BTRFS_DEBUG));
+		return -EUCLEAN;
+	}
+
+	if (found_level)
+		btrfs_node_key_to_cpu(eb, &found_key, 0);
+	else
+		btrfs_item_key_to_cpu(eb, &found_key, 0);
+	ret = btrfs_comp_cpu_keys(first_key, &found_key);
+
+	if (ret) {
+		WARN(IS_ENABLED(CONFIG_BTRFS_DEBUG),
+		     KERN_ERR "BTRFS: tree first key check failed\n");
+		btrfs_err(fs_info,
+"tree first key mismatch detected, bytenr=%llu parent_transid=%llu key expected=(%llu,%u,%llu) has=(%llu,%u,%llu)",
+			  eb->start, parent_transid, first_key->objectid,
+			  first_key->type, first_key->offset,
+			  found_key.objectid, found_key.type,
+			  found_key.offset);
+	}
+	return ret;
+}
diff --git a/fs/btrfs/tree-checker.h b/fs/btrfs/tree-checker.h
index c0861ce1429b..3c2a02a72f64 100644
--- a/fs/btrfs/tree-checker.h
+++ b/fs/btrfs/tree-checker.h
@@ -66,5 +66,7 @@ int btrfs_check_node(struct extent_buffer *node);
 int btrfs_check_chunk_valid(struct extent_buffer *leaf,
 			    struct btrfs_chunk *chunk, u64 logical);
 int btrfs_check_eb_owner(const struct extent_buffer *eb, u64 root_owner);
+int btrfs_verify_level_key(struct extent_buffer *eb, int level,
+			   struct btrfs_key *first_key, u64 parent_transid);
 
 #endif
-- 
2.40.0

Re: [PATCH 08/12] btrfs: move btrfs_verify_level_key into tree-checker.c

[Johannes Thumshirn]

Looks good,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

[PATCH 09/12] btrfs: move split_flags/combine_flags helpers to inode-item.h

[Josef Bacik]

These are more related to the inode item flags on disk than the
in-memory btrfs_inode, move the helpers to inode-item.h.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 fs/btrfs/btrfs_inode.h  | 16 ----------------
 fs/btrfs/inode-item.h   | 16 ++++++++++++++++
 fs/btrfs/tree-checker.c |  2 +-
 3 files changed, 17 insertions(+), 17 deletions(-)

diff --git a/fs/btrfs/btrfs_inode.h b/fs/btrfs/btrfs_inode.h
index ec2ae4406c16..9e8038273cdc 100644
--- a/fs/btrfs/btrfs_inode.h
+++ b/fs/btrfs/btrfs_inode.h
@@ -407,22 +407,6 @@ static inline bool btrfs_inode_can_compress(const struct btrfs_inode *inode)
 	return true;
 }
 
-/*
- * btrfs_inode_item stores flags in a u64, btrfs_inode stores them in two
- * separate u32s. These two functions convert between the two representations.
- */
-static inline u64 btrfs_inode_combine_flags(u32 flags, u32 ro_flags)
-{
-	return (flags | ((u64)ro_flags << 32));
-}
-
-static inline void btrfs_inode_split_flags(u64 inode_item_flags,
-					   u32 *flags, u32 *ro_flags)
-{
-	*flags = (u32)inode_item_flags;
-	*ro_flags = (u32)(inode_item_flags >> 32);
-}
-
 /* Array of bytes with variable length, hexadecimal format 0x1234 */
 #define CSUM_FMT				"0x%*phN"
 #define CSUM_FMT_VALUE(size, bytes)		size, bytes
diff --git a/fs/btrfs/inode-item.h b/fs/btrfs/inode-item.h
index b80aeb715701..ede43b6c6559 100644
--- a/fs/btrfs/inode-item.h
+++ b/fs/btrfs/inode-item.h
@@ -60,6 +60,22 @@ struct btrfs_truncate_control {
 	bool clear_extent_range;
 };
 
+/*
+ * btrfs_inode_item stores flags in a u64, btrfs_inode stores them in two
+ * separate u32s. These two functions convert between the two representations.
+ */
+static inline u64 btrfs_inode_combine_flags(u32 flags, u32 ro_flags)
+{
+	return (flags | ((u64)ro_flags << 32));
+}
+
+static inline void btrfs_inode_split_flags(u64 inode_item_flags,
+					   u32 *flags, u32 *ro_flags)
+{
+	*flags = (u32)inode_item_flags;
+	*ro_flags = (u32)(inode_item_flags >> 32);
+}
+
 int btrfs_truncate_inode_items(struct btrfs_trans_handle *trans,
 			       struct btrfs_root *root,
 			       struct btrfs_truncate_control *control);
diff --git a/fs/btrfs/tree-checker.c b/fs/btrfs/tree-checker.c
index a1038156d57d..d38f54efd443 100644
--- a/fs/btrfs/tree-checker.c
+++ b/fs/btrfs/tree-checker.c
@@ -25,10 +25,10 @@
 #include "compression.h"
 #include "volumes.h"
 #include "misc.h"
-#include "btrfs_inode.h"
 #include "fs.h"
 #include "accessors.h"
 #include "file-item.h"
+#include "inode-item.h"
 
 /*
  * Error message should follow the following format:
-- 
2.40.0

Re: [PATCH 09/12] btrfs: move split_flags/combine_flags helpers to inode-item.h

[Johannes Thumshirn]

Looks good,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

[PATCH 10/12] btrfs: add __KERNEL__ check for btrfs_no_printk

[Josef Bacik]

We want to override this in btrfs-progs, so wrap this in the __KERNEL__
check so we can easily sync this to btrfs-progs and have our local
version of btrfs_no_printk do the work.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 fs/btrfs/messages.h | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/fs/btrfs/messages.h b/fs/btrfs/messages.h
index ac2d1982ba3d..99143bbf78a5 100644
--- a/fs/btrfs/messages.h
+++ b/fs/btrfs/messages.h
@@ -7,11 +7,18 @@
 
 struct btrfs_fs_info;
 
+/*
+ * We want to be able to override this in btrfs-progs.
+ */
+#ifdef __KERNEL__
+
 static inline __printf(2, 3) __cold
 void btrfs_no_printk(const struct btrfs_fs_info *fs_info, const char *fmt, ...)
 {
 }
 
+#endif
+
 #ifdef CONFIG_PRINTK
 
 #define btrfs_printk(fs_info, fmt, args...)				\
-- 
2.40.0

Re: [PATCH 10/12] btrfs: add __KERNEL__ check for btrfs_no_printk

[Johannes Thumshirn]

Looks good,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

[PATCH 11/12] btrfs: add a btrfs_csum_type_size helper

[Josef Bacik]

This is needed in btrfs-progs for the tools that convert the checksum
types for file systems and a few other things.  We don't have it in the
kernel as we just want to get the size for the super blocks type.
However I don't want to have to manually add this every time we sync
ctree.c into btrfs-progs, so add the helper in the kernel with a note so
it doesn't get removed by a later cleanup.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 fs/btrfs/ctree.c | 8 +++++++-
 fs/btrfs/ctree.h | 1 +
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
index 7071f90c23e3..c95c62baef3e 100644
--- a/fs/btrfs/ctree.c
+++ b/fs/btrfs/ctree.c
@@ -150,13 +150,19 @@ static inline void copy_leaf_items(const struct extent_buffer *dst,
 			      nr_items * sizeof(struct btrfs_item));
 }
 
+/* This exists for btrfs-progs usages. */
+u16 btrfs_csum_type_size(u16 type)
+{
+	return btrfs_csums[type].size;
+}
+
 int btrfs_super_csum_size(const struct btrfs_super_block *s)
 {
 	u16 t = btrfs_super_csum_type(s);
 	/*
 	 * csum type is validated at mount time
 	 */
-	return btrfs_csums[t].size;
+	return btrfs_csum_type_size(t);
 }
 
 const char *btrfs_super_csum_name(u16 csum_type)
diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index 4c1986cd5bed..221e230787e3 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -702,6 +702,7 @@ static inline bool btrfs_is_data_reloc_root(const struct btrfs_root *root)
 	return root->root_key.objectid == BTRFS_DATA_RELOC_TREE_OBJECTID;
 }
 
+u16 btrfs_csum_type_size(u16 type);
 int btrfs_super_csum_size(const struct btrfs_super_block *s);
 const char *btrfs_super_csum_name(u16 csum_type);
 const char *btrfs_super_csum_driver(u16 csum_type);
-- 
2.40.0

Re: [PATCH 11/12] btrfs: add a btrfs_csum_type_size helper

[Johannes Thumshirn]

On 29.04.23 22:08, Josef Bacik wrote:
> This is needed in btrfs-progs for the tools that convert the checksum
> types for file systems and a few other things.  We don't have it in the
> kernel as we just want to get the size for the super blocks type.
> However I don't want to have to manually add this every time we sync
> ctree.c into btrfs-progs, so add the helper in the kernel with a note so
> it doesn't get removed by a later cleanup.
> 
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> ---
>  fs/btrfs/ctree.c | 8 +++++++-
>  fs/btrfs/ctree.h | 1 +
>  2 files changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
> index 7071f90c23e3..c95c62baef3e 100644
> --- a/fs/btrfs/ctree.c
> +++ b/fs/btrfs/ctree.c
> @@ -150,13 +150,19 @@ static inline void copy_leaf_items(const struct extent_buffer *dst,
>  			      nr_items * sizeof(struct btrfs_item));
>  }
>  
> +/* This exists for btrfs-progs usages. */
> +u16 btrfs_csum_type_size(u16 type)
> +{
> +	return btrfs_csums[type].size;
> +}
> +
>  int btrfs_super_csum_size(const struct btrfs_super_block *s)
>  {
>  	u16 t = btrfs_super_csum_type(s);
>  	/*
>  	 * csum type is validated at mount time
>  	 */
> -	return btrfs_csums[t].size;
> +	return btrfs_csum_type_size(t);
>  }
>  
>  const char *btrfs_super_csum_name(u16 csum_type)
> diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
> index 4c1986cd5bed..221e230787e3 100644
> --- a/fs/btrfs/ctree.h
> +++ b/fs/btrfs/ctree.h
> @@ -702,6 +702,7 @@ static inline bool btrfs_is_data_reloc_root(const struct btrfs_root *root)
>  	return root->root_key.objectid == BTRFS_DATA_RELOC_TREE_OBJECTID;
>  }
>  
> +u16 btrfs_csum_type_size(u16 type);
>  int btrfs_super_csum_size(const struct btrfs_super_block *s);
>  const char *btrfs_super_csum_name(u16 csum_type);
>  const char *btrfs_super_csum_driver(u16 csum_type);


Same comment here, can we do an EXPORT_FOR_PROGS please.

[PATCH 12/12] btrfs: rename del_ptr -> btrfs_del_ptr and export it

[Josef Bacik]

This exists internal to ctree.c, however btrfs check needs to use it for
some of its operations.  I'd rather not duplicate that code inside of
btrfs check as this is low level and I want to keep this code in one
place, so rename the function to btrfs_del_ptr and export it so that it
can be used inside of btrfs-progs safely.  Add a comment to make sure
this doesn't get removed by a future cleanup.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 fs/btrfs/ctree.c | 16 ++++++++--------
 fs/btrfs/ctree.h |  2 ++
 2 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
index c95c62baef3e..198773503cfd 100644
--- a/fs/btrfs/ctree.c
+++ b/fs/btrfs/ctree.c
@@ -37,8 +37,6 @@ static int push_node_left(struct btrfs_trans_handle *trans,
 static int balance_node_right(struct btrfs_trans_handle *trans,
 			      struct extent_buffer *dst_buf,
 			      struct extent_buffer *src_buf);
-static void del_ptr(struct btrfs_root *root, struct btrfs_path *path,
-		    int level, int slot);
 
 static const struct btrfs_csums {
 	u16		size;
@@ -1122,7 +1120,7 @@ static noinline int balance_level(struct btrfs_trans_handle *trans,
 		if (btrfs_header_nritems(right) == 0) {
 			btrfs_clear_buffer_dirty(trans, right);
 			btrfs_tree_unlock(right);
-			del_ptr(root, path, level + 1, pslot + 1);
+			btrfs_del_ptr(root, path, level + 1, pslot + 1);
 			root_sub_used(root, right->len);
 			btrfs_free_tree_block(trans, btrfs_root_id(root), right,
 					      0, 1);
@@ -1168,7 +1166,7 @@ static noinline int balance_level(struct btrfs_trans_handle *trans,
 	if (btrfs_header_nritems(mid) == 0) {
 		btrfs_clear_buffer_dirty(trans, mid);
 		btrfs_tree_unlock(mid);
-		del_ptr(root, path, level + 1, pslot);
+		btrfs_del_ptr(root, path, level + 1, pslot);
 		root_sub_used(root, mid->len);
 		btrfs_free_tree_block(trans, btrfs_root_id(root), mid, 0, 1);
 		free_extent_buffer_stale(mid);
@@ -4276,9 +4274,11 @@ int btrfs_duplicate_item(struct btrfs_trans_handle *trans,
  *
  * the tree should have been previously balanced so the deletion does not
  * empty a node.
+ *
+ * This is exported for use inside btrfs-progs, don't un-export it.
  */
-static void del_ptr(struct btrfs_root *root, struct btrfs_path *path,
-		    int level, int slot)
+void btrfs_del_ptr(struct btrfs_root *root, struct btrfs_path *path, int level,
+		   int slot)
 {
 	struct extent_buffer *parent = path->nodes[level];
 	u32 nritems;
@@ -4333,7 +4333,7 @@ static noinline void btrfs_del_leaf(struct btrfs_trans_handle *trans,
 				    struct extent_buffer *leaf)
 {
 	WARN_ON(btrfs_header_generation(leaf) != trans->transid);
-	del_ptr(root, path, 1, path->slots[1]);
+	btrfs_del_ptr(root, path, 1, path->slots[1]);
 
 	/*
 	 * btrfs_free_extent is expensive, we want to make sure we
@@ -4419,7 +4419,7 @@ int btrfs_del_items(struct btrfs_trans_handle *trans, struct btrfs_root *root,
 
 			/* push_leaf_left fixes the path.
 			 * make sure the path still points to our leaf
-			 * for possible call to del_ptr below
+			 * for possible call to btrfs_del_ptr below
 			 */
 			slot = path->slots[1];
 			atomic_inc(&leaf->refs);
diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index 221e230787e3..9dc9315c2bfa 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -541,6 +541,8 @@ int btrfs_copy_root(struct btrfs_trans_handle *trans,
 		      struct extent_buffer **cow_ret, u64 new_root_objectid);
 int btrfs_block_can_be_shared(struct btrfs_root *root,
 			      struct extent_buffer *buf);
+void btrfs_del_ptr(struct btrfs_root *root, struct btrfs_path *path, int level,
+		   int slot);
 void btrfs_extend_item(struct btrfs_path *path, u32 data_size);
 void btrfs_truncate_item(struct btrfs_path *path, u32 new_size, int from_end);
 int btrfs_split_item(struct btrfs_trans_handle *trans,
-- 
2.40.0

Re: [PATCH 12/12] btrfs: rename del_ptr -> btrfs_del_ptr and export it

[Johannes Thumshirn]

On 29.04.23 22:08, Josef Bacik wrote:
> This exists internal to ctree.c, however btrfs check needs to use it for
> some of its operations.  I'd rather not duplicate that code inside of
> btrfs check as this is low level and I want to keep this code in one
> place, so rename the function to btrfs_del_ptr and export it so that it
> can be used inside of btrfs-progs safely.  Add a comment to make sure
> this doesn't get removed by a future cleanup.
> 
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> ---
>  fs/btrfs/ctree.c | 16 ++++++++--------
>  fs/btrfs/ctree.h |  2 ++
>  2 files changed, 10 insertions(+), 8 deletions(-)
> 
> diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
> index c95c62baef3e..198773503cfd 100644
> --- a/fs/btrfs/ctree.c
> +++ b/fs/btrfs/ctree.c
> @@ -37,8 +37,6 @@ static int push_node_left(struct btrfs_trans_handle *trans,
>  static int balance_node_right(struct btrfs_trans_handle *trans,
>  			      struct extent_buffer *dst_buf,
>  			      struct extent_buffer *src_buf);
> -static void del_ptr(struct btrfs_root *root, struct btrfs_path *path,
> -		    int level, int slot);
>  
>  static const struct btrfs_csums {
>  	u16		size;
> @@ -1122,7 +1120,7 @@ static noinline int balance_level(struct btrfs_trans_handle *trans,
>  		if (btrfs_header_nritems(right) == 0) {
>  			btrfs_clear_buffer_dirty(trans, right);
>  			btrfs_tree_unlock(right);
> -			del_ptr(root, path, level + 1, pslot + 1);
> +			btrfs_del_ptr(root, path, level + 1, pslot + 1);
>  			root_sub_used(root, right->len);
>  			btrfs_free_tree_block(trans, btrfs_root_id(root), right,
>  					      0, 1);
> @@ -1168,7 +1166,7 @@ static noinline int balance_level(struct btrfs_trans_handle *trans,
>  	if (btrfs_header_nritems(mid) == 0) {
>  		btrfs_clear_buffer_dirty(trans, mid);
>  		btrfs_tree_unlock(mid);
> -		del_ptr(root, path, level + 1, pslot);
> +		btrfs_del_ptr(root, path, level + 1, pslot);
>  		root_sub_used(root, mid->len);
>  		btrfs_free_tree_block(trans, btrfs_root_id(root), mid, 0, 1);
>  		free_extent_buffer_stale(mid);
> @@ -4276,9 +4274,11 @@ int btrfs_duplicate_item(struct btrfs_trans_handle *trans,
>   *
>   * the tree should have been previously balanced so the deletion does not
>   * empty a node.
> + *
> + * This is exported for use inside btrfs-progs, don't un-export it.
>   */
> -static void del_ptr(struct btrfs_root *root, struct btrfs_path *path,
> -		    int level, int slot)
> +void btrfs_del_ptr(struct btrfs_root *root, struct btrfs_path *path, int level,
> +		   int slot)
>  {
>  	struct extent_buffer *parent = path->nodes[level];
>  	u32 nritems;
> @@ -4333,7 +4333,7 @@ static noinline void btrfs_del_leaf(struct btrfs_trans_handle *trans,
>  				    struct extent_buffer *leaf)
>  {
>  	WARN_ON(btrfs_header_generation(leaf) != trans->transid);
> -	del_ptr(root, path, 1, path->slots[1]);
> +	btrfs_del_ptr(root, path, 1, path->slots[1]);
>  
>  	/*
>  	 * btrfs_free_extent is expensive, we want to make sure we
> @@ -4419,7 +4419,7 @@ int btrfs_del_items(struct btrfs_trans_handle *trans, struct btrfs_root *root,
>  
>  			/* push_leaf_left fixes the path.
>  			 * make sure the path still points to our leaf
> -			 * for possible call to del_ptr below
> +			 * for possible call to btrfs_del_ptr below
>  			 */
>  			slot = path->slots[1];
>  			atomic_inc(&leaf->refs);
> diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
> index 221e230787e3..9dc9315c2bfa 100644
> --- a/fs/btrfs/ctree.h
> +++ b/fs/btrfs/ctree.h
> @@ -541,6 +541,8 @@ int btrfs_copy_root(struct btrfs_trans_handle *trans,
>  		      struct extent_buffer **cow_ret, u64 new_root_objectid);
>  int btrfs_block_can_be_shared(struct btrfs_root *root,
>  			      struct extent_buffer *buf);
> +void btrfs_del_ptr(struct btrfs_root *root, struct btrfs_path *path, int level,
> +		   int slot);
>  void btrfs_extend_item(struct btrfs_path *path, u32 data_size);
>  void btrfs_truncate_item(struct btrfs_path *path, u32 new_size, int from_end);
>  int btrfs_split_item(struct btrfs_trans_handle *trans,

And here again.

Thanks,
	Johannes

Re: [PATCH 00/12] btrfs: various cleanups to make ctree.c sync easier

[David Sterba]

On Sat, Apr 29, 2023 at 04:07:09PM -0400, Josef Bacik wrote:
> Hello,
> 
> These are various cleanups I needed to make syncing some of the kernel files
> into btrfs-progs go smoothly.  They're cosmetic and organizational and shouldn't
> have any functional changes.  Thanks,
> 
> Josef
> 
> Josef Bacik (12):
>   btrfs: move btrfs_check_trunc_cache_free_space into block-rsv.c
>   btrfs: remove level argument from btrfs_set_block_flags
>   btrfs: simplify btrfs_check_leaf_* helpers into a single helper
>   btrfs: add btrfs_tree_block_status definitions to tree-checker.h
>   btrfs: use btrfs_tree_block_status for leaf item errors
>   btrfs: extend btrfs_leaf_check to return btrfs_tree_block_status
>   btrfs: add __btrfs_check_node helper
>   btrfs: move btrfs_verify_level_key into tree-checker.c
>   btrfs: move split_flags/combine_flags helpers to inode-item.h
>   btrfs: add __KERNEL__ check for btrfs_no_printk
>   btrfs: add a btrfs_csum_type_size helper
>   btrfs: rename del_ptr -> btrfs_del_ptr and export it

Added to misc-next, thanks. There are still some comments regarding the
ifdefs and export macros, we can deal with that after merge or I can
update the patches as needed.