[PATCH 1/2] btrfs: tree-checker: Make write time tree checker verify the content of leaves

[PATCH 1/2] btrfs: tree-checker: Make write time tree checker verify the content of leaves

[Qu Wenruo]

Commit "btrfs: Do mandatory tree block check before submitting bio" is
designed to check leaves contents, just as read time check.

However due to the confusing parameter list, "false" is passed to where
it is supposed to be "true".

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

diff --git a/fs/btrfs/tree-checker.c b/fs/btrfs/tree-checker.c
index 4318e3e67443..204fe53c90aa 100644
--- a/fs/btrfs/tree-checker.c
+++ b/fs/btrfs/tree-checker.c
@@ -999,7 +999,7 @@ int btrfs_check_leaf_relaxed(struct btrfs_fs_info *fs_info,
 int btrfs_check_leaf_write(struct btrfs_fs_info *fs_info,
 			   struct extent_buffer *leaf)
 {
-	return check_leaf(leaf, false, false);
+	return check_leaf(leaf, true, false);
 }
 
 int btrfs_check_node(struct btrfs_fs_info *fs_info, struct extent_buffer *node)
-- 
2.21.0

[PATCH 2/2] btrfs: tree-checker: Remove comprehensive root owner check

[Qu Wenruo]

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/disk-io.c      |  2 +-
 fs/btrfs/tree-checker.c | 51 +++--------------------------------------
 fs/btrfs/tree-checker.h |  2 --
 3 files changed, 4 insertions(+), 51 deletions(-)

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index c3b89584dad6..fddcf7d7c30c 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -539,7 +539,7 @@ static int csum_dirty_buffer(struct btrfs_fs_info *fs_info, struct page *page)
 	if (btrfs_header_level(eb))
 		err = btrfs_check_node(fs_info, eb);
 	else
-		err = btrfs_check_leaf_write(fs_info, eb);
+		err = btrfs_check_leaf_full(fs_info, eb);
 
 	if (err < 0) {
 		btrfs_err(fs_info,
diff --git a/fs/btrfs/tree-checker.c b/fs/btrfs/tree-checker.c
index 204fe53c90aa..e325d4df5c23 100644
--- a/fs/btrfs/tree-checker.c
+++ b/fs/btrfs/tree-checker.c
@@ -812,8 +812,7 @@ static int check_leaf_item(struct extent_buffer *leaf,
 	return ret;
 }
 
-static int check_leaf(struct extent_buffer *leaf, bool check_item_data,
-		      bool check_empty_leaf)
+static int check_leaf(struct extent_buffer *leaf, bool check_item_data)
 {
 	struct btrfs_fs_info *fs_info = leaf->fs_info;
 	/* No valid key type is 0, so all key should be larger than this key */
@@ -839,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 ||
@@ -853,38 +851,6 @@ static int check_leaf(struct extent_buffer *leaf, bool check_item_data,
 				    owner);
 			return -EUCLEAN;
 		}
-
-		/*
-		 * Don't check empty leaves at write time for trees where we
-		 * can't use @owner to indicate the right owner. This happens
-		 * for reloc trees or for a new commit root.
-		 */
-		if (!check_empty_leaf)
-			return 0;
-
-		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;
 	}
 
@@ -982,24 +948,13 @@ static int check_leaf(struct extent_buffer *leaf, bool check_item_data,
 int btrfs_check_leaf_full(struct btrfs_fs_info *fs_info,
 			  struct extent_buffer *leaf)
 {
-	return check_leaf(leaf, true, true);
+	return check_leaf(leaf, true);
 }
 
 int btrfs_check_leaf_relaxed(struct btrfs_fs_info *fs_info,
 			     struct extent_buffer *leaf)
 {
-	return check_leaf(leaf, false, true);
-}
-
-/*
- * Write time specific leaf checker.
- * Don't check if the empty leaf belongs to a tree root. Mostly for balance
- * and new tree created in current transaction.
- */
-int btrfs_check_leaf_write(struct btrfs_fs_info *fs_info,
-			   struct extent_buffer *leaf)
-{
-	return check_leaf(leaf, true, false);
+	return check_leaf(leaf, false);
 }
 
 int btrfs_check_node(struct btrfs_fs_info *fs_info, struct extent_buffer *node)
diff --git a/fs/btrfs/tree-checker.h b/fs/btrfs/tree-checker.h
index 281eb7136bc1..4df45e8a6659 100644
--- a/fs/btrfs/tree-checker.h
+++ b/fs/btrfs/tree-checker.h
@@ -23,8 +23,6 @@ int btrfs_check_leaf_full(struct btrfs_fs_info *fs_info,
  */
 int btrfs_check_leaf_relaxed(struct btrfs_fs_info *fs_info,
 			     struct extent_buffer *leaf);
-int btrfs_check_leaf_write(struct btrfs_fs_info *fs_info,
-			   struct extent_buffer *leaf);
 int btrfs_check_node(struct btrfs_fs_info *fs_info, struct extent_buffer *node);
 
 int btrfs_check_chunk_valid(struct btrfs_fs_info *fs_info,
-- 
2.21.0

Re: [PATCH 1/2] btrfs: tree-checker: Make write time tree checker verify the content of leaves

[Nikolay Borisov]

On 3.04.19 г. 14:59 ч., Qu Wenruo wrote:
> Commit "btrfs: Do mandatory tree block check before submitting bio" is
> designed to check leaves contents, just as read time check.
> 
> However due to the confusing parameter list, "false" is passed to where
> it is supposed to be "true".
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>

The subject of the patch could be a bit more precise, by passing true
you are doing further validation of the body of individual items in the
leaves. Alternatively you are sanity checking the keys and the
boundaries of item body. So how about:

"Perform item body validation during write"


Reviewed-by: Nikolay Borisov <nborisov@suse.com>

> ---
>  fs/btrfs/tree-checker.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/btrfs/tree-checker.c b/fs/btrfs/tree-checker.c
> index 4318e3e67443..204fe53c90aa 100644
> --- a/fs/btrfs/tree-checker.c
> +++ b/fs/btrfs/tree-checker.c
> @@ -999,7 +999,7 @@ int btrfs_check_leaf_relaxed(struct btrfs_fs_info *fs_info,
>  int btrfs_check_leaf_write(struct btrfs_fs_info *fs_info,
>  			   struct extent_buffer *leaf)
>  {
> -	return check_leaf(leaf, false, false);
> +	return check_leaf(leaf, true, false);
>  }
>  
>  int btrfs_check_node(struct btrfs_fs_info *fs_info, struct extent_buffer *node)
> 

Re: [PATCH 1/2] btrfs: tree-checker: Make write time tree checker verify the content of leaves

[David Sterba]

On Wed, Apr 03, 2019 at 07:59:18PM +0800, Qu Wenruo wrote:
> Commit "btrfs: Do mandatory tree block check before submitting bio" is
> designed to check leaves contents, just as read time check.
> 
> However due to the confusing parameter list, "false" is passed to where
> it is supposed to be "true".

And this seems to invalidate all testing since false->true will do
additional expensive checks. I thought I'd fold the fix to the original
patch but in this case it'd be the wrong thing to do due to the visible
effects of the change.

Re: [PATCH 1/2] btrfs: tree-checker: Make write time tree checker verify the content of leaves

[Qu Wenruo]

[-- Attachment #1.1: Type: text/plain, Size: 989 bytes --]



On 2019/4/3 下午10:13, David Sterba wrote:
> On Wed, Apr 03, 2019 at 07:59:18PM +0800, Qu Wenruo wrote:
>> Commit "btrfs: Do mandatory tree block check before submitting bio" is
>> designed to check leaves contents, just as read time check.
>>
>> However due to the confusing parameter list, "false" is passed to where
>> it is supposed to be "true".
> 
> And this seems to invalidate all testing since false->true will do
> additional expensive checks.

That's the case.

However with the next patch, btrfs_check_leaf_full() is less expensive.
Just 3~5 times expensive as csum_tree_block(), reduced from 20 times.

My full fstests run is coming to the end and so far no regression.

> I thought I'd fold the fix to the original
> patch but in this case it'd be the wrong thing to do due to the visible
> effects of the change.

If that's the case, I'd add new Fixes: tag, but that can only happen
after write time patch get merged upstream.

Thanks,
Qu


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/2] btrfs: tree-checker: Make write time tree checker verify the content of leaves

[Qu Wenruo]

On 2019/4/3 下午10:12, Nikolay Borisov wrote:
> 
> 
> On 3.04.19 г. 14:59 ч., Qu Wenruo wrote:
>> Commit "btrfs: Do mandatory tree block check before submitting bio" is
>> designed to check leaves contents, just as read time check.
>>
>> However due to the confusing parameter list, "false" is passed to where
>> it is supposed to be "true".
>>
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
> 
> The subject of the patch could be a bit more precise, by passing true
> you are doing further validation of the body of individual items in the
> leaves. Alternatively you are sanity checking the keys and the
> boundaries of item body. So how about:
> 
> "Perform item body validation during write"

That's much better, I'd take that tile if I need to resend.

Thanks,
Qu

> 
> 
> Reviewed-by: Nikolay Borisov <nborisov@suse.com>
> 
>> ---
>>  fs/btrfs/tree-checker.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/fs/btrfs/tree-checker.c b/fs/btrfs/tree-checker.c
>> index 4318e3e67443..204fe53c90aa 100644
>> --- a/fs/btrfs/tree-checker.c
>> +++ b/fs/btrfs/tree-checker.c
>> @@ -999,7 +999,7 @@ int btrfs_check_leaf_relaxed(struct btrfs_fs_info *fs_info,
>>  int btrfs_check_leaf_write(struct btrfs_fs_info *fs_info,
>>  			   struct extent_buffer *leaf)
>>  {
>> -	return check_leaf(leaf, false, false);
>> +	return check_leaf(leaf, true, false);
>>  }
>>  
>>  int btrfs_check_node(struct btrfs_fs_info *fs_info, struct extent_buffer *node)
>>

Re: [PATCH 2/2] btrfs: tree-checker: Remove comprehensive root owner check

[David Sterba]

On Wed, Apr 03, 2019 at 07:59:19PM +0800, Qu Wenruo wrote:
> Commit 1ba98d086fe3 ("Btrfs: detect corruption when non-root leaf has
> zero item") introduced comprehensive root owner checker.

That's 4.7, so we might want to do stable backport to 4.9+.
> 
> 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().

And for that reason the change should be minimal, this patch depends on
current development queue so it's not suitable for the backport.

I'm thinking what to do here, I want to avoid patches that effectively
revert changes that are still in the development branch and haven't been
released.

This means to rework "btrfs: Do mandatory tree block check before
submitting bio", though it's deep in the branch, the scope of changes is
only 2 files so this should not cause any conflicts.

Re: [PATCH 2/2] btrfs: tree-checker: Remove comprehensive root owner check

[Nikolay Borisov]

On 3.04.19 г. 14:59 ч., 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>

Reviewed-by: Nikolay Borisov <nborisov@suse.com>

Re: [PATCH 1/2] btrfs: tree-checker: Make write time tree checker verify the content of leaves

[David Sterba]

On Wed, Apr 03, 2019 at 10:15:27PM +0800, Qu Wenruo wrote:
> 
> 
> On 2019/4/3 下午10:13, David Sterba wrote:
> > On Wed, Apr 03, 2019 at 07:59:18PM +0800, Qu Wenruo wrote:
> >> Commit "btrfs: Do mandatory tree block check before submitting bio" is
> >> designed to check leaves contents, just as read time check.
> >>
> >> However due to the confusing parameter list, "false" is passed to where
> >> it is supposed to be "true".
> > 
> > And this seems to invalidate all testing since false->true will do
> > additional expensive checks.
> 
> That's the case.
> 
> However with the next patch, btrfs_check_leaf_full() is less expensive.
> Just 3~5 times expensive as csum_tree_block(), reduced from 20 times.
> 
> My full fstests run is coming to the end and so far no regression.
> 
> > I thought I'd fold the fix to the original
> > patch but in this case it'd be the wrong thing to do due to the visible
> > effects of the change.
> 
> If that's the case, I'd add new Fixes: tag, but that can only happen
> after write time patch get merged upstream.

Fixes: to development patches should raise an alert that something's
wrong.

Re: [PATCH 2/2] btrfs: tree-checker: Remove comprehensive root owner check

[Qu Wenruo]

[-- Attachment #1.1: Type: text/plain, Size: 1884 bytes --]



On 2019/4/3 下午10:21, David Sterba wrote:
> On Wed, Apr 03, 2019 at 07:59:19PM +0800, Qu Wenruo wrote:
>> Commit 1ba98d086fe3 ("Btrfs: detect corruption when non-root leaf has
>> zero item") introduced comprehensive root owner checker.
> 
> That's 4.7, so we might want to do stable backport to 4.9+.

It's just a performance optimization.

I'm not sure if we should backport such things even if we can solve the
conflicts.

>>
>> 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().
> 
> And for that reason the change should be minimal, this patch depends on
> current development queue so it's not suitable for the backport.
> 
> I'm thinking what to do here, I want to avoid patches that effectively
> revert changes that are still in the development branch and haven't been
> released.

OK, I'll resend write time tree checker patchset.

> 
> This means to rework "btrfs: Do mandatory tree block check before
> submitting bio", though it's deep in the branch, the scope of changes is
> only 2 files so this should not cause any conflicts.

BTW, I'm a little interesting in the workflow on your side.

In this case, what's the correct way to handle them?
Just remove those two related patches from misc-next branch and re-apply
the newer version?

Or remove all those write time tree checker and later enhancement
patches and reapply them all?

Either way, I hope to experience the same workflow so I could reduce
your workload.

Thanks,
Qu


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 2/2] btrfs: tree-checker: Remove comprehensive root owner check

[David Sterba]

On Wed, Apr 03, 2019 at 10:26:05PM +0800, Qu Wenruo wrote:
> On 2019/4/3 下午10:21, David Sterba wrote:
> > On Wed, Apr 03, 2019 at 07:59:19PM +0800, Qu Wenruo wrote:
> >> Commit 1ba98d086fe3 ("Btrfs: detect corruption when non-root leaf has
> >> zero item") introduced comprehensive root owner checker.
> > 
> > That's 4.7, so we might want to do stable backport to 4.9+.
> 
> It's just a performance optimization.
> 
> I'm not sure if we should backport such things even if we can solve the
> conflicts.

That's for the consideration, if the performance improvement is
significant, but due to other changes in the codebases it might not be
safe.

> >> 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().
> > 
> > And for that reason the change should be minimal, this patch depends on
> > current development queue so it's not suitable for the backport.
> > 
> > I'm thinking what to do here, I want to avoid patches that effectively
> > revert changes that are still in the development branch and haven't been
> > released.
> 
> OK, I'll resend write time tree checker patchset.

Wait, it's just one patch that needs update, "btrfs: Do mandatory tree
block check before submitting bio".

If the above patch is dropped, then the expensive checks in check_leaf
can be removed as you do in this patch, without the updates to
parameters.

> > This means to rework "btrfs: Do mandatory tree block check before
> > submitting bio", though it's deep in the branch, the scope of changes is
> > only 2 files so this should not cause any conflicts.
> 
> BTW, I'm a little interesting in the workflow on your side.
> 
> In this case, what's the correct way to handle them?
> Just remove those two related patches from misc-next branch and re-apply
> the newer version?

If you mean these two

  btrfs: disk-io: Show the timing of corrupted tree block explicitly
  btrfs: Do mandatory tree block check before submitting bio

we don't need to remove both, just the second one.

> Or remove all those write time tree checker and later enhancement
> patches and reapply them all?

No, why removing them? That's independent enhancement of the tree
checker. What needs to be removed is the call at write time.

> Either way, I hope to experience the same workflow so I could reduce
> your workload.

This is an infrequent case when a patch deep in the branch needs update
so the solution depends on the impact on the patches above it. Here it's
only minor conflict with a cleanup patch removing redundant fs_info from
check_leaf.

So that's trivial and we don't need to update the patches in place.
Further updates can be applied on top of misc-next (with the patch
removed).

The ideal workflow is:

1) patches are developed, tested by the developer
2) posted to mailinglist
3) after review added to a topic branch in for-next for wider testing
   - patchset revisions are easy to update as it's in a topic branch
4) after some time the patches get merged to misc-next and no updates are
   expected, beyond minor updates like coding style or changelog
5) patch goes to release branch and is effectively frozen, all fixes go
   as separate patches

So if you want to reduce my workload, spend more time on 1-3 so we
don't have to be creative when fixing problems we discover at 4-5.

I know this cannot be followed 100% times, but so far we've been able to
resolve all the situations and the number of times is overall low. So
it's bearable.