diff mbox series

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

Message ID 3f41d6e2a62eacc4a31966dfaa8a3e0b8f64766b.1682798736.git.josef@toxicpanda.com (mailing list archive)
State New, archived
Headers show
Series btrfs: various cleanups to make ctree.c sync easier | expand

Commit Message

Josef Bacik April 29, 2023, 8:07 p.m. UTC 
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(-)

Comments

Johannes Thumshirn May 2, 2023, 11:27 a.m. UTC | #1 
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>
David Sterba May 9, 2023, 9:01 p.m. UTC | #2 
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.
diff mbox series

Patch

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,