| Message ID | 1526612375-95875-1-git-send-email-bo.liu@linux.alibaba.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 18.05.2018 05:59, Liu Bo wrote: > As verify_level_key() is checked after verify_parent_transid(), i.e. > > if (verify_parent_transid()) > ret = -EIO; > else if (verify_level_key()) > ret = -EUCLEAN; > > if parent_transid is 0, verify_parent_transid() skips verifying > parent_transid and considers eb as valid, and if verify_level_key() > reports something wrong, we're not going to know if it's caused by > corrupted metadata or non-checkecd eb (e.g. stale eb). It's really unclear (from the changelog) how the stale eb ties with parent_transid. You should have explained the relationship between checking parenttransid and stale/non-stale ebs > > @parent_transid is able to tell whether the eb's generation has been > verified by the caller. It's really unclear why parent_transid is able to tell whether the generation is verified by the caller. > > Signed-off-by: Liu Bo <bo.liu@linux.alibaba.com> > --- > v2: - more explicit commit log. > - adjust the position shown in error msg. > > fs/btrfs/disk-io.c | 13 +++++++------ > 1 file changed, 7 insertions(+), 6 deletions(-) > > diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c > index 60caa68c3618..ad865176a3ca 100644 > --- a/fs/btrfs/disk-io.c > +++ b/fs/btrfs/disk-io.c > @@ -416,7 +416,7 @@ static int btrfs_check_super_csum(struct btrfs_fs_info *fs_info, > > static int verify_level_key(struct btrfs_fs_info *fs_info, > struct extent_buffer *eb, int level, > - struct btrfs_key *first_key) > + struct btrfs_key *first_key, u64 parent_transid) > { > int found_level; > struct btrfs_key found_key; > @@ -454,10 +454,11 @@ static int verify_level_key(struct btrfs_fs_info *fs_info, > if (ret) { > WARN_ON(1); > btrfs_err(fs_info, > -"tree first key mismatch detected, bytenr=%llu key expected=(%llu, %u, %llu) has=(%llu, %u, %llu)", > - eb->start, first_key->objectid, first_key->type, > - first_key->offset, found_key.objectid, > - found_key.type, found_key.offset); > +"tree first key mismatch detected, bytenr=%llu parent_transid=%llu key expected=(%llu, %u, %llu) has=(%llu, %u, %llu)", > + eb->start, parent_transid, first_key->objectid, > + first_key->type, first_key->offset, > + found_key.objectid, found_key.type, > + found_key.offset); > } > #endif > return ret; > @@ -493,7 +494,7 @@ static int btree_read_extent_buffer_pages(struct btrfs_fs_info *fs_info, > parent_transid, 0)) > ret = -EIO; > else if (verify_level_key(fs_info, eb, level, > - first_key)) > + first_key, parent_transid)) > ret = -EUCLEAN; > else > break; > -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2018年05月18日 10:59, Liu Bo wrote: > As verify_level_key() is checked after verify_parent_transid(), i.e. > > if (verify_parent_transid()) > ret = -EIO; > else if (verify_level_key()) > ret = -EUCLEAN; > > if parent_transid is 0, verify_parent_transid() skips verifying > parent_transid and considers eb as valid, and if verify_level_key() > reports something wrong, we're not going to know if it's caused by > corrupted metadata or non-checkecd eb (e.g. stale eb). > > @parent_transid is able to tell whether the eb's generation has been > verified by the caller. > > Signed-off-by: Liu Bo <bo.liu@linux.alibaba.com> Reviewed-by: Qu Wenruo <wqu@suse.com> Thanks, Qu > --- > v2: - more explicit commit log. > - adjust the position shown in error msg. > > fs/btrfs/disk-io.c | 13 +++++++------ > 1 file changed, 7 insertions(+), 6 deletions(-) > > diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c > index 60caa68c3618..ad865176a3ca 100644 > --- a/fs/btrfs/disk-io.c > +++ b/fs/btrfs/disk-io.c > @@ -416,7 +416,7 @@ static int btrfs_check_super_csum(struct btrfs_fs_info *fs_info, > > static int verify_level_key(struct btrfs_fs_info *fs_info, > struct extent_buffer *eb, int level, > - struct btrfs_key *first_key) > + struct btrfs_key *first_key, u64 parent_transid) > { > int found_level; > struct btrfs_key found_key; > @@ -454,10 +454,11 @@ static int verify_level_key(struct btrfs_fs_info *fs_info, > if (ret) { > WARN_ON(1); > btrfs_err(fs_info, > -"tree first key mismatch detected, bytenr=%llu key expected=(%llu, %u, %llu) has=(%llu, %u, %llu)", > - eb->start, first_key->objectid, first_key->type, > - first_key->offset, found_key.objectid, > - found_key.type, found_key.offset); > +"tree first key mismatch detected, bytenr=%llu parent_transid=%llu key expected=(%llu, %u, %llu) has=(%llu, %u, %llu)", > + eb->start, parent_transid, first_key->objectid, > + first_key->type, first_key->offset, > + found_key.objectid, found_key.type, > + found_key.offset); > } > #endif > return ret; > @@ -493,7 +494,7 @@ static int btree_read_extent_buffer_pages(struct btrfs_fs_info *fs_info, > parent_transid, 0)) > ret = -EIO; > else if (verify_level_key(fs_info, eb, level, > - first_key)) > + first_key, parent_transid)) > ret = -EUCLEAN; > else > break; >
On 2018年05月18日 13:02, Nikolay Borisov wrote: > > > On 18.05.2018 05:59, Liu Bo wrote: >> As verify_level_key() is checked after verify_parent_transid(), i.e. >> >> if (verify_parent_transid()) >> ret = -EIO; >> else if (verify_level_key()) >> ret = -EUCLEAN; >> >> if parent_transid is 0, verify_parent_transid() skips verifying >> parent_transid and considers eb as valid, and if verify_level_key() >> reports something wrong, we're not going to know if it's caused by >> corrupted metadata or non-checkecd eb (e.g. stale eb). > > It's really unclear (from the changelog) how the stale eb ties with > parent_transid. You should have explained the relationship between > checking parenttransid and stale/non-stale ebs It's in fact related to the fixing patch: Btrfs: fix the corruption by reading stale btree blocks But whatever, extra mention won't hurt. > >> >> @parent_transid is able to tell whether the eb's generation has been >> verified by the caller. > > It's really unclear why parent_transid is able to tell whether the > generation is verified by the caller. At least this looks pretty straightforward to me. @parent_transid = 0 means no generation check, and @parent_transid != 0 means already checked. So at least for this part I didn't get any confusion. Thanks, Qu > >> >> Signed-off-by: Liu Bo <bo.liu@linux.alibaba.com> >> --- >> v2: - more explicit commit log. >> - adjust the position shown in error msg. >> >> fs/btrfs/disk-io.c | 13 +++++++------ >> 1 file changed, 7 insertions(+), 6 deletions(-) >> >> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c >> index 60caa68c3618..ad865176a3ca 100644 >> --- a/fs/btrfs/disk-io.c >> +++ b/fs/btrfs/disk-io.c >> @@ -416,7 +416,7 @@ static int btrfs_check_super_csum(struct btrfs_fs_info *fs_info, >> >> static int verify_level_key(struct btrfs_fs_info *fs_info, >> struct extent_buffer *eb, int level, >> - struct btrfs_key *first_key) >> + struct btrfs_key *first_key, u64 parent_transid) >> { >> int found_level; >> struct btrfs_key found_key; >> @@ -454,10 +454,11 @@ static int verify_level_key(struct btrfs_fs_info *fs_info, >> if (ret) { >> WARN_ON(1); >> btrfs_err(fs_info, >> -"tree first key mismatch detected, bytenr=%llu key expected=(%llu, %u, %llu) has=(%llu, %u, %llu)", >> - eb->start, first_key->objectid, first_key->type, >> - first_key->offset, found_key.objectid, >> - found_key.type, found_key.offset); >> +"tree first key mismatch detected, bytenr=%llu parent_transid=%llu key expected=(%llu, %u, %llu) has=(%llu, %u, %llu)", >> + eb->start, parent_transid, first_key->objectid, >> + first_key->type, first_key->offset, >> + found_key.objectid, found_key.type, >> + found_key.offset); >> } >> #endif >> return ret; >> @@ -493,7 +494,7 @@ static int btree_read_extent_buffer_pages(struct btrfs_fs_info *fs_info, >> parent_transid, 0)) >> ret = -EIO; >> else if (verify_level_key(fs_info, eb, level, >> - first_key)) >> + first_key, parent_transid)) >> ret = -EUCLEAN; >> else >> break; >> > -- > To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, May 18, 2018 at 01:10:59PM +0800, Qu Wenruo wrote: > > > On 2018年05月18日 13:02, Nikolay Borisov wrote: > > > > > > On 18.05.2018 05:59, Liu Bo wrote: > >> As verify_level_key() is checked after verify_parent_transid(), i.e. > >> > >> if (verify_parent_transid()) > >> ret = -EIO; > >> else if (verify_level_key()) > >> ret = -EUCLEAN; > >> > >> if parent_transid is 0, verify_parent_transid() skips verifying > >> parent_transid and considers eb as valid, and if verify_level_key() > >> reports something wrong, we're not going to know if it's caused by > >> corrupted metadata or non-checkecd eb (e.g. stale eb). > > > > It's really unclear (from the changelog) how the stale eb ties with > > parent_transid. You should have explained the relationship between > > checking parenttransid and stale/non-stale ebs > > It's in fact related to the fixing patch: > Btrfs: fix the corruption by reading stale btree blocks > > But whatever, extra mention won't hurt. I've added reference to the patch as it has more details so we don't have to repeat it in the changelog. Patch added to misc-next, thanks. -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c index 60caa68c3618..ad865176a3ca 100644 --- a/fs/btrfs/disk-io.c +++ b/fs/btrfs/disk-io.c @@ -416,7 +416,7 @@ static int btrfs_check_super_csum(struct btrfs_fs_info *fs_info, static int verify_level_key(struct btrfs_fs_info *fs_info, struct extent_buffer *eb, int level, - struct btrfs_key *first_key) + struct btrfs_key *first_key, u64 parent_transid) { int found_level; struct btrfs_key found_key; @@ -454,10 +454,11 @@ static int verify_level_key(struct btrfs_fs_info *fs_info, if (ret) { WARN_ON(1); btrfs_err(fs_info, -"tree first key mismatch detected, bytenr=%llu key expected=(%llu, %u, %llu) has=(%llu, %u, %llu)", - eb->start, first_key->objectid, first_key->type, - first_key->offset, found_key.objectid, - found_key.type, found_key.offset); +"tree first key mismatch detected, bytenr=%llu parent_transid=%llu key expected=(%llu, %u, %llu) has=(%llu, %u, %llu)", + eb->start, parent_transid, first_key->objectid, + first_key->type, first_key->offset, + found_key.objectid, found_key.type, + found_key.offset); } #endif return ret; @@ -493,7 +494,7 @@ static int btree_read_extent_buffer_pages(struct btrfs_fs_info *fs_info, parent_transid, 0)) ret = -EIO; else if (verify_level_key(fs_info, eb, level, - first_key)) + first_key, parent_transid)) ret = -EUCLEAN; else break;
As verify_level_key() is checked after verify_parent_transid(), i.e. if (verify_parent_transid()) ret = -EIO; else if (verify_level_key()) ret = -EUCLEAN; if parent_transid is 0, verify_parent_transid() skips verifying parent_transid and considers eb as valid, and if verify_level_key() reports something wrong, we're not going to know if it's caused by corrupted metadata or non-checkecd eb (e.g. stale eb). @parent_transid is able to tell whether the eb's generation has been verified by the caller. Signed-off-by: Liu Bo <bo.liu@linux.alibaba.com> --- v2: - more explicit commit log. - adjust the position shown in error msg. fs/btrfs/disk-io.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-)