| Message ID | 9321364229bba40851c35fa63ba37c90ee1edf0a.1507292872.git.dsterba@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 10/06/2017 06:29 AM, David Sterba wrote: > The use of sector_t is not necessry, it's just for a warning. Switch to > u64 and rename the variable. The messages are adjusted as well. > > Signed-off-by: David Sterba <dsterba@suse.com> > --- > fs/btrfs/scrub.c | 14 +++++++------- > 1 file changed, 7 insertions(+), 7 deletions(-) > > diff --git a/fs/btrfs/scrub.c b/fs/btrfs/scrub.c > index e3f6c49e5c4d..34ea2e7048c2 100644 > --- a/fs/btrfs/scrub.c > +++ b/fs/btrfs/scrub.c > @@ -231,7 +231,7 @@ struct scrub_warning { > struct btrfs_path *path; > u64 extent_item_size; > const char *errstr; > - sector_t sector; > + u64 physical; > u64 logical; > struct btrfs_device *dev; > }; Just a thought, 'physical' alone seems a bit ambiguous, so a comment to add context would help clarify. > @@ -797,10 +797,10 @@ static int scrub_print_warning_inode(u64 inum, u64 offset, u64 root, > */ > for (i = 0; i < ipath->fspath->elem_cnt; ++i) > btrfs_warn_in_rcu(fs_info, > - "%s at logical %llu on dev %s, sector %llu, root %llu, inode %llu, offset %llu, length %llu, links %u (path: %s)", > +"%s at logical %llu on dev %s, physical %llu, root %llu, inode %llu, offset %llu, length %llu, links %u (path: %s)", > swarn->errstr, swarn->logical, > rcu_str_deref(swarn->dev->name), > - (unsigned long long)swarn->sector, > + swarn->physical, > root, inum, offset, > min(isize - offset, (u64)PAGE_SIZE), nlink, > (char *)(unsigned long)ipath->fspath->val[i]); > @@ -813,7 +813,7 @@ static int scrub_print_warning_inode(u64 inum, u64 offset, u64 root, > "%s at logical %llu on dev %s, sector %llu, root %llu, inode %llu, offset %llu: path resolving failed with ret=%d", This still says 'sector' here but was changed for the other print strings. > swarn->errstr, swarn->logical, > rcu_str_deref(swarn->dev->name), > - (unsigned long long)swarn->sector, > + swarn->physical, > root, inum, offset, ret); > > free_ipath(ipath); > @@ -845,7 +845,7 @@ static void scrub_print_warning(const char *errstr, struct scrub_block *sblock) > if (!path) > return; > > - swarn.sector = (sblock->pagev[0]->physical) >> 9; > + swarn.physical = sblock->pagev[0]->physical; Dropping the '>> 9', so this is a semantic change in addition to the rename? Thx, Ed -- 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, Oct 06, 2017 at 09:22:42AM -0600, Edmund Nadolski wrote: > On 10/06/2017 06:29 AM, David Sterba wrote: > > The use of sector_t is not necessry, it's just for a warning. Switch to > > u64 and rename the variable. The messages are adjusted as well. > > > > Signed-off-by: David Sterba <dsterba@suse.com> > > --- > > fs/btrfs/scrub.c | 14 +++++++------- > > 1 file changed, 7 insertions(+), 7 deletions(-) > > > > diff --git a/fs/btrfs/scrub.c b/fs/btrfs/scrub.c > > index e3f6c49e5c4d..34ea2e7048c2 100644 > > --- a/fs/btrfs/scrub.c > > +++ b/fs/btrfs/scrub.c > > @@ -231,7 +231,7 @@ struct scrub_warning { > > struct btrfs_path *path; > > u64 extent_item_size; > > const char *errstr; > > - sector_t sector; > > + u64 physical; > > u64 logical; > > struct btrfs_device *dev; > > }; > > Just a thought, 'physical' alone seems a bit ambiguous, so > a comment to add context would help clarify. The other thing that came to my mind was 'offset', but this would be confusing as well, as there is already the 'logical' member. The naming physical/logical is used in other structures, like btrfs_bio_stripe or scrub_page so I think this is kind of following the convention. > > > > @@ -797,10 +797,10 @@ static int scrub_print_warning_inode(u64 inum, u64 offset, u64 root, > > */ > > for (i = 0; i < ipath->fspath->elem_cnt; ++i) > > btrfs_warn_in_rcu(fs_info, > > - "%s at logical %llu on dev %s, sector %llu, root %llu, inode %llu, offset %llu, length %llu, links %u (path: %s)", > > +"%s at logical %llu on dev %s, physical %llu, root %llu, inode %llu, offset %llu, length %llu, links %u (path: %s)", > > swarn->errstr, swarn->logical, > > rcu_str_deref(swarn->dev->name), > > - (unsigned long long)swarn->sector, > > + swarn->physical, > > root, inum, offset, > > min(isize - offset, (u64)PAGE_SIZE), nlink, > > (char *)(unsigned long)ipath->fspath->val[i]); > > @@ -813,7 +813,7 @@ static int scrub_print_warning_inode(u64 inum, u64 offset, u64 root, > > "%s at logical %llu on dev %s, sector %llu, root %llu, inode %llu, offset %llu: path resolving failed with ret=%d", > > This still says 'sector' here but was changed for the other print strings. Right, I'll fix it. > > swarn->errstr, swarn->logical, > > rcu_str_deref(swarn->dev->name), > > - (unsigned long long)swarn->sector, > > + swarn->physical, > > root, inum, offset, ret); > > > > free_ipath(ipath); > > @@ -845,7 +845,7 @@ static void scrub_print_warning(const char *errstr, struct scrub_block *sblock) > > if (!path) > > return; > > > > - swarn.sector = (sblock->pagev[0]->physical) >> 9; > > + swarn.physical = sblock->pagev[0]->physical; > > Dropping the '>> 9', so this is a semantic change in addition to the rename? Yes, sector_t is traditionally in 512b units but we want "1b" where possible. I'll update the changelog so it's clear. -- 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, Oct 06, 2017 at 02:29:56PM +0200, David Sterba wrote: > The use of sector_t is not necessry, it's just for a warning. Switch to > u64 and rename the variable. The messages are adjusted as well. > > Signed-off-by: David Sterba <dsterba@suse.com> > --- > fs/btrfs/scrub.c | 14 +++++++------- > 1 file changed, 7 insertions(+), 7 deletions(-) > > diff --git a/fs/btrfs/scrub.c b/fs/btrfs/scrub.c > index e3f6c49e5c4d..34ea2e7048c2 100644 > --- a/fs/btrfs/scrub.c > +++ b/fs/btrfs/scrub.c > @@ -231,7 +231,7 @@ struct scrub_warning { > struct btrfs_path *path; > u64 extent_item_size; > const char *errstr; > - sector_t sector; > + u64 physical; > u64 logical; > struct btrfs_device *dev; > }; > @@ -797,10 +797,10 @@ static int scrub_print_warning_inode(u64 inum, u64 offset, u64 root, > */ > for (i = 0; i < ipath->fspath->elem_cnt; ++i) > btrfs_warn_in_rcu(fs_info, > - "%s at logical %llu on dev %s, sector %llu, root %llu, inode %llu, offset %llu, length %llu, links %u (path: %s)", > +"%s at logical %llu on dev %s, physical %llu, root %llu, inode %llu, offset %llu, length %llu, links %u (path: %s)", > swarn->errstr, swarn->logical, > rcu_str_deref(swarn->dev->name), > - (unsigned long long)swarn->sector, > + swarn->physical, > root, inum, offset, > min(isize - offset, (u64)PAGE_SIZE), nlink, > (char *)(unsigned long)ipath->fspath->val[i]); > @@ -813,7 +813,7 @@ static int scrub_print_warning_inode(u64 inum, u64 offset, u64 root, > "%s at logical %llu on dev %s, sector %llu, root %llu, inode %llu, offset %llu: path resolving failed with ret=%d", > swarn->errstr, swarn->logical, > rcu_str_deref(swarn->dev->name), > - (unsigned long long)swarn->sector, > + swarn->physical, > root, inum, offset, ret); > > free_ipath(ipath); > @@ -845,7 +845,7 @@ static void scrub_print_warning(const char *errstr, struct scrub_block *sblock) > if (!path) > return; > > - swarn.sector = (sblock->pagev[0]->physical) >> 9; > + swarn.physical = sblock->pagev[0]->physical; > swarn.logical = sblock->pagev[0]->logical; > swarn.errstr = errstr; > swarn.dev = NULL; > @@ -868,10 +868,10 @@ static void scrub_print_warning(const char *errstr, struct scrub_block *sblock) > item_size, &ref_root, > &ref_level); > btrfs_warn_in_rcu(fs_info, > - "%s at logical %llu on dev %s, sector %llu: metadata %s (level %d) in tree %llu", > +"%s at logical %llu on dev %s, physical %llu: metadata %s (level %d) in tree %llu", > errstr, swarn.logical, > rcu_str_deref(dev->name), > - (unsigned long long)swarn.sector, > + swarn.physical, > ref_level ? "node" : "leaf", > ret < 0 ? -1 : ref_level, > ret < 0 ? -1 : ref_root); I've got used to ambiguous physical/logical, so not a problem for me. Looks good to me. Reviewed-by: Liu Bo <bo.li.liu@oracle.com> -liubo -- 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/scrub.c b/fs/btrfs/scrub.c index e3f6c49e5c4d..34ea2e7048c2 100644 --- a/fs/btrfs/scrub.c +++ b/fs/btrfs/scrub.c @@ -231,7 +231,7 @@ struct scrub_warning { struct btrfs_path *path; u64 extent_item_size; const char *errstr; - sector_t sector; + u64 physical; u64 logical; struct btrfs_device *dev; }; @@ -797,10 +797,10 @@ static int scrub_print_warning_inode(u64 inum, u64 offset, u64 root, */ for (i = 0; i < ipath->fspath->elem_cnt; ++i) btrfs_warn_in_rcu(fs_info, - "%s at logical %llu on dev %s, sector %llu, root %llu, inode %llu, offset %llu, length %llu, links %u (path: %s)", +"%s at logical %llu on dev %s, physical %llu, root %llu, inode %llu, offset %llu, length %llu, links %u (path: %s)", swarn->errstr, swarn->logical, rcu_str_deref(swarn->dev->name), - (unsigned long long)swarn->sector, + swarn->physical, root, inum, offset, min(isize - offset, (u64)PAGE_SIZE), nlink, (char *)(unsigned long)ipath->fspath->val[i]); @@ -813,7 +813,7 @@ static int scrub_print_warning_inode(u64 inum, u64 offset, u64 root, "%s at logical %llu on dev %s, sector %llu, root %llu, inode %llu, offset %llu: path resolving failed with ret=%d", swarn->errstr, swarn->logical, rcu_str_deref(swarn->dev->name), - (unsigned long long)swarn->sector, + swarn->physical, root, inum, offset, ret); free_ipath(ipath); @@ -845,7 +845,7 @@ static void scrub_print_warning(const char *errstr, struct scrub_block *sblock) if (!path) return; - swarn.sector = (sblock->pagev[0]->physical) >> 9; + swarn.physical = sblock->pagev[0]->physical; swarn.logical = sblock->pagev[0]->logical; swarn.errstr = errstr; swarn.dev = NULL; @@ -868,10 +868,10 @@ static void scrub_print_warning(const char *errstr, struct scrub_block *sblock) item_size, &ref_root, &ref_level); btrfs_warn_in_rcu(fs_info, - "%s at logical %llu on dev %s, sector %llu: metadata %s (level %d) in tree %llu", +"%s at logical %llu on dev %s, physical %llu: metadata %s (level %d) in tree %llu", errstr, swarn.logical, rcu_str_deref(dev->name), - (unsigned long long)swarn.sector, + swarn.physical, ref_level ? "node" : "leaf", ret < 0 ? -1 : ref_level, ret < 0 ? -1 : ref_root);
The use of sector_t is not necessry, it's just for a warning. Switch to u64 and rename the variable. The messages are adjusted as well. Signed-off-by: David Sterba <dsterba@suse.com> --- fs/btrfs/scrub.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-)