| Message ID | 20190403115919.17049-1-wqu@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] btrfs: tree-checker: Make write time tree checker verify the content of leaves | expand |
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) >
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.
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
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) >>
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.
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)
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(-)