[1/2] btrfs: tree-checker: Remove comprehensive root owner check
diff mbox series

Message ID 20190404034708.3399-2-wqu@suse.com
State New
Headers show
Series
  • Fixup and optimization for write time tree checker
Related show

Commit Message

Qu Wenruo April 4, 2019, 3:47 a.m. UTC
Commit 1ba98d086fe3 ("Btrfs: detect corruption when non-root leaf has
zero item") introduced comprehensive root owner checker.

However it's pretty expensive tree search to locate the owner root,
especially when it get reused by mandatory read and write time
tree-checker.

This patch will remove that check, and completely rely on owner based
empty leaf check, which is much faster and still works fine for most
case.

And since we skip the old root owner check, now write time tree check
can be merged with btrfs_check_leaf_full().

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/tree-checker.c | 24 ------------------------
 1 file changed, 24 deletions(-)

Comments

Nikolay Borisov April 4, 2019, 6:23 a.m. UTC | #1
On 4.04.19 г. 6:47 ч., Qu Wenruo wrote:
> Commit 1ba98d086fe3 ("Btrfs: detect corruption when non-root leaf has
> zero item") introduced comprehensive root owner checker.
> 
> However it's pretty expensive tree search to locate the owner root,
> especially when it get reused by mandatory read and write time
> tree-checker.
> 
> This patch will remove that check, and completely rely on owner based
> empty leaf check, which is much faster and still works fine for most
> case.
> 
> And since we skip the old root owner check, now write time tree check
> can be merged with btrfs_check_leaf_full().
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>

This patch was sent yesterday, now it's sent again with no material
changes whatsoever? What's the point?

> ---
>  fs/btrfs/tree-checker.c | 24 ------------------------
>  1 file changed, 24 deletions(-)
> 
> diff --git a/fs/btrfs/tree-checker.c b/fs/btrfs/tree-checker.c
> index d2c3c1f8870d..e325d4df5c23 100644
> --- a/fs/btrfs/tree-checker.c
> +++ b/fs/btrfs/tree-checker.c
> @@ -838,7 +838,6 @@ static int check_leaf(struct extent_buffer *leaf, bool check_item_data)
>  	 */
>  	if (nritems == 0 && !btrfs_header_flag(leaf, BTRFS_HEADER_FLAG_RELOC)) {
>  		u64 owner = btrfs_header_owner(leaf);
> -		struct btrfs_root *check_root;
>  
>  		/* These trees must never be empty */
>  		if (owner == BTRFS_ROOT_TREE_OBJECTID ||
> @@ -852,29 +851,6 @@ static int check_leaf(struct extent_buffer *leaf, bool check_item_data)
>  				    owner);
>  			return -EUCLEAN;
>  		}
> -		key.objectid = owner;
> -		key.type = BTRFS_ROOT_ITEM_KEY;
> -		key.offset = (u64)-1;
> -
> -		check_root = btrfs_get_fs_root(fs_info, &key, false);
> -		/*
> -		 * The only reason we also check NULL here is that during
> -		 * open_ctree() some roots has not yet been set up.
> -		 */
> -		if (!IS_ERR_OR_NULL(check_root)) {
> -			struct extent_buffer *eb;
> -
> -			eb = btrfs_root_node(check_root);
> -			/* if leaf is the root, then it's fine */
> -			if (leaf != eb) {
> -				generic_err(leaf, 0,
> -		"invalid nritems, have %u should not be 0 for non-root leaf",
> -					nritems);
> -				free_extent_buffer(eb);
> -				return -EUCLEAN;
> -			}
> -			free_extent_buffer(eb);
> -		}
>  		return 0;
>  	}
>  
>
Qu Wenruo April 4, 2019, 6:33 a.m. UTC | #2
On 2019/4/4 下午2:23, Nikolay Borisov wrote:
>
>
> On 4.04.19 г. 6:47 ч., Qu Wenruo wrote:
>> Commit 1ba98d086fe3 ("Btrfs: detect corruption when non-root leaf has
>> zero item") introduced comprehensive root owner checker.
>>
>> However it's pretty expensive tree search to locate the owner root,
>> especially when it get reused by mandatory read and write time
>> tree-checker.
>>
>> This patch will remove that check, and completely rely on owner based
>> empty leaf check, which is much faster and still works fine for most
>> case.
>>
>> And since we skip the old root owner check, now write time tree check
>> can be merged with btrfs_check_leaf_full().
>>
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>
> This patch was sent yesterday, now it's sent again with no material
> changes whatsoever? What's the point?

This is the version to be applied on misc-next.

Thanks,
Qu

>
>> ---
>>  fs/btrfs/tree-checker.c | 24 ------------------------
>>  1 file changed, 24 deletions(-)
>>
>> diff --git a/fs/btrfs/tree-checker.c b/fs/btrfs/tree-checker.c
>> index d2c3c1f8870d..e325d4df5c23 100644
>> --- a/fs/btrfs/tree-checker.c
>> +++ b/fs/btrfs/tree-checker.c
>> @@ -838,7 +838,6 @@ static int check_leaf(struct extent_buffer *leaf, bool check_item_data)
>>  	 */
>>  	if (nritems == 0 && !btrfs_header_flag(leaf, BTRFS_HEADER_FLAG_RELOC)) {
>>  		u64 owner = btrfs_header_owner(leaf);
>> -		struct btrfs_root *check_root;
>>
>>  		/* These trees must never be empty */
>>  		if (owner == BTRFS_ROOT_TREE_OBJECTID ||
>> @@ -852,29 +851,6 @@ static int check_leaf(struct extent_buffer *leaf, bool check_item_data)
>>  				    owner);
>>  			return -EUCLEAN;
>>  		}
>> -		key.objectid = owner;
>> -		key.type = BTRFS_ROOT_ITEM_KEY;
>> -		key.offset = (u64)-1;
>> -
>> -		check_root = btrfs_get_fs_root(fs_info, &key, false);
>> -		/*
>> -		 * The only reason we also check NULL here is that during
>> -		 * open_ctree() some roots has not yet been set up.
>> -		 */
>> -		if (!IS_ERR_OR_NULL(check_root)) {
>> -			struct extent_buffer *eb;
>> -
>> -			eb = btrfs_root_node(check_root);
>> -			/* if leaf is the root, then it's fine */
>> -			if (leaf != eb) {
>> -				generic_err(leaf, 0,
>> -		"invalid nritems, have %u should not be 0 for non-root leaf",
>> -					nritems);
>> -				free_extent_buffer(eb);
>> -				return -EUCLEAN;
>> -			}
>> -			free_extent_buffer(eb);
>> -		}
>>  		return 0;
>>  	}
>>
>>
David Sterba April 4, 2019, 3:24 p.m. UTC | #3
On Thu, Apr 04, 2019 at 09:23:11AM +0300, Nikolay Borisov wrote:
> 
> 
> On 4.04.19 г. 6:47 ч., Qu Wenruo wrote:
> > Commit 1ba98d086fe3 ("Btrfs: detect corruption when non-root leaf has
> > zero item") introduced comprehensive root owner checker.
> > 
> > However it's pretty expensive tree search to locate the owner root,
> > especially when it get reused by mandatory read and write time
> > tree-checker.
> > 
> > This patch will remove that check, and completely rely on owner based
> > empty leaf check, which is much faster and still works fine for most
> > case.
> > 
> > And since we skip the old root owner check, now write time tree check
> > can be merged with btrfs_check_leaf_full().
> > 
> > Signed-off-by: Qu Wenruo <wqu@suse.com>
> 
> This patch was sent yesterday, now it's sent again with no material
> changes whatsoever? What's the point?

The core change is the same, but there are removed hunks compared to v1.
We've discussed that yesterday and Qu did it right.

Patch
diff mbox series

diff --git a/fs/btrfs/tree-checker.c b/fs/btrfs/tree-checker.c
index d2c3c1f8870d..e325d4df5c23 100644
--- a/fs/btrfs/tree-checker.c
+++ b/fs/btrfs/tree-checker.c
@@ -838,7 +838,6 @@  static int check_leaf(struct extent_buffer *leaf, bool check_item_data)
 	 */
 	if (nritems == 0 && !btrfs_header_flag(leaf, BTRFS_HEADER_FLAG_RELOC)) {
 		u64 owner = btrfs_header_owner(leaf);
-		struct btrfs_root *check_root;
 
 		/* These trees must never be empty */
 		if (owner == BTRFS_ROOT_TREE_OBJECTID ||
@@ -852,29 +851,6 @@  static int check_leaf(struct extent_buffer *leaf, bool check_item_data)
 				    owner);
 			return -EUCLEAN;
 		}
-		key.objectid = owner;
-		key.type = BTRFS_ROOT_ITEM_KEY;
-		key.offset = (u64)-1;
-
-		check_root = btrfs_get_fs_root(fs_info, &key, false);
-		/*
-		 * The only reason we also check NULL here is that during
-		 * open_ctree() some roots has not yet been set up.
-		 */
-		if (!IS_ERR_OR_NULL(check_root)) {
-			struct extent_buffer *eb;
-
-			eb = btrfs_root_node(check_root);
-			/* if leaf is the root, then it's fine */
-			if (leaf != eb) {
-				generic_err(leaf, 0,
-		"invalid nritems, have %u should not be 0 for non-root leaf",
-					nritems);
-				free_extent_buffer(eb);
-				return -EUCLEAN;
-			}
-			free_extent_buffer(eb);
-		}
 		return 0;
 	}