| Message ID | 20190117101528.8675-5-wqu@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: Enhancement to tree block validation | expand |
On 17/01/2019 11:15, Qu Wenruo wrote: > Just add one extra line to show when the corruption is detected. > Currently only read time detection is possible. > > Reviewed-by: Nikolay Borisov <nborisov@suse.com> > Signed-off-by: Qu Wenruo <wqu@suse.com> > --- > fs/btrfs/disk-io.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c > index 659bab9de03b..bc2379cb2091 100644 > --- a/fs/btrfs/disk-io.c > +++ b/fs/btrfs/disk-io.c > @@ -652,11 +652,14 @@ static int btree_readpage_end_io_hook(struct btrfs_io_bio *io_bio, > */ > if (found_level == 0 && btrfs_check_leaf_full(fs_info, eb)) { > set_bit(EXTENT_BUFFER_CORRUPT, &eb->bflags); > + btrfs_err(fs_info, "read time tree block corruption detected"); > ret = -EIO; > } > > - if (found_level > 0 && btrfs_check_node(fs_info, eb)) > + if (found_level > 0 && btrfs_check_node(fs_info, eb)) { > + btrfs_err(fs_info, "read time tree block corruption detected"); > ret = -EIO; > + } It's still the same error message for two different checks (check_node vs. check_leaf_full). Granted you can still refer to the messages of btrfs_check_node() and btrfs_check_leaf_full() but where's the point in having the new message? Byte, Johannes
On 2019/1/17 下午9:36, Johannes Thumshirn wrote: > On 17/01/2019 11:15, Qu Wenruo wrote: >> Just add one extra line to show when the corruption is detected. >> Currently only read time detection is possible. >> >> Reviewed-by: Nikolay Borisov <nborisov@suse.com> >> Signed-off-by: Qu Wenruo <wqu@suse.com> >> --- >> fs/btrfs/disk-io.c | 5 ++++- >> 1 file changed, 4 insertions(+), 1 deletion(-) >> >> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c >> index 659bab9de03b..bc2379cb2091 100644 >> --- a/fs/btrfs/disk-io.c >> +++ b/fs/btrfs/disk-io.c >> @@ -652,11 +652,14 @@ static int btree_readpage_end_io_hook(struct btrfs_io_bio *io_bio, >> */ >> if (found_level == 0 && btrfs_check_leaf_full(fs_info, eb)) { >> set_bit(EXTENT_BUFFER_CORRUPT, &eb->bflags); >> + btrfs_err(fs_info, "read time tree block corruption detected"); >> ret = -EIO; >> } >> >> - if (found_level > 0 && btrfs_check_node(fs_info, eb)) >> + if (found_level > 0 && btrfs_check_node(fs_info, eb)) { >> + btrfs_err(fs_info, "read time tree block corruption detected"); >> ret = -EIO; >> + } > > It's still the same error message for two different checks (check_node > vs. check_leaf_full). > > Granted you can still refer to the messages of btrfs_check_node() and > btrfs_check_leaf_full() but where's the point in having the new message? To show when the error happens. This patch only handles the read time error, thus we have two same error message. But the next patch will introduce new write time validation check, thus we need to distinguish then by the extra message. Thanks, Qu > > Byte, > Johannes >
On 17/01/2019 14:42, Qu Wenruo wrote: > > To show when the error happens. > > This patch only handles the read time error, thus we have two same error > message. > > But the next patch will introduce new write time validation check, thus > we need to distinguish then by the extra message. OK, so if we really want the same message printed for all read time errors, how about this: if (found_level == 0 && btrfs_check_leaf_full(fs_info, eb)) { set_bit(EXTENT_BUFFER_CORRUPT, &eb->bflags); ret = -EIO; } if (found_level > 0 && btrfs_check_node(fs_info, eb)) ret = -EIO; + if (ret == -EIO) + btrfs_err(fs_info, + "read time tree block corruption detected"); + else if (!ret) - if (!ret) set_extent_buffer_uptodate(eb); So we really only have one message printed and not duplicating the prints. Byte, Johannes
On 2019/1/17 下午10:08, Johannes Thumshirn wrote: > On 17/01/2019 14:42, Qu Wenruo wrote: >> >> To show when the error happens. >> >> This patch only handles the read time error, thus we have two same error >> message. >> >> But the next patch will introduce new write time validation check, thus >> we need to distinguish then by the extra message. > > OK, so if we really want the same message printed for all read time > errors, how about this: > > if (found_level == 0 && btrfs_check_leaf_full(fs_info, eb)) { > set_bit(EXTENT_BUFFER_CORRUPT, &eb->bflags); > ret = -EIO; > } > > if (found_level > 0 && btrfs_check_node(fs_info, eb)) > ret = -EIO; > > + if (ret == -EIO) > + btrfs_err(fs_info, > + "read time tree block corruption detected"); > + else if (!ret) > - if (!ret) > set_extent_buffer_uptodate(eb); > > > So we really only have one message printed and not duplicating the prints. Works for me, I'll use this one in next version. Thanks, Qu > > Byte, > Johannes >
diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c index 659bab9de03b..bc2379cb2091 100644 --- a/fs/btrfs/disk-io.c +++ b/fs/btrfs/disk-io.c @@ -652,11 +652,14 @@ static int btree_readpage_end_io_hook(struct btrfs_io_bio *io_bio, */ if (found_level == 0 && btrfs_check_leaf_full(fs_info, eb)) { set_bit(EXTENT_BUFFER_CORRUPT, &eb->bflags); + btrfs_err(fs_info, "read time tree block corruption detected"); ret = -EIO; } - if (found_level > 0 && btrfs_check_node(fs_info, eb)) + if (found_level > 0 && btrfs_check_node(fs_info, eb)) { + btrfs_err(fs_info, "read time tree block corruption detected"); ret = -EIO; + } if (!ret) set_extent_buffer_uptodate(eb);