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

Message ID 20190403115919.17049-1-wqu@suse.com
State New
Headers show
Series
  • [1/2] btrfs: tree-checker: Make write time tree checker verify the content of leaves
Related show

Commit Message

Qu Wenruo April 3, 2019, 11:59 a.m. UTC
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(-)

Comments

Nikolay Borisov April 3, 2019, 2:12 p.m. UTC | #1
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)
>
David Sterba April 3, 2019, 2:13 p.m. UTC | #2
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.
Qu Wenruo April 3, 2019, 2:15 p.m. UTC | #3
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
Qu Wenruo April 3, 2019, 2:20 p.m. UTC | #4
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)
>>
David Sterba April 3, 2019, 2:25 p.m. UTC | #5
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.

Patch
diff mbox series

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)