| Message ID | 20190715144241.1077-1-anand.jain@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs-progs: add verbose option to btrfs device scan | expand |
On 15.07.19 г. 17:42 ч., Anand Jain wrote: > To help debug device scan issues, add verbose option to btrfs device scan. > > Signed-off-by: Anand Jain <anand.jain@oracle.com> I fail to see what this patch helps for. We get the path in case of errors, in case of success what good could the path be ? > --- > cmds/device.c | 8 ++++++-- > cmds/filesystem.c | 2 +- > common/device-scan.c | 4 +++- > common/device-scan.h | 2 +- > common/utils.c | 2 +- > disk-io.c | 2 +- > 6 files changed, 13 insertions(+), 7 deletions(-) > > diff --git a/cmds/device.c b/cmds/device.c > index 24158308a41b..2fa13e61f806 100644 > --- a/cmds/device.c > +++ b/cmds/device.c > @@ -313,6 +313,7 @@ static int cmd_device_scan(const struct cmd_struct *cmd, int argc, char **argv) > int all = 0; > int ret = 0; > int forget = 0; > + int verbose = 0; nit: make it a bool. > > optind = 0; > while (1) { > @@ -323,7 +324,7 @@ static int cmd_device_scan(const struct cmd_struct *cmd, int argc, char **argv) > { NULL, 0, NULL, 0} > }; > > - c = getopt_long(argc, argv, "du", long_options, NULL); > + c = getopt_long(argc, argv, "duv", long_options, NULL); > if (c < 0) > break; > switch (c) { > @@ -333,6 +334,9 @@ static int cmd_device_scan(const struct cmd_struct *cmd, int argc, char **argv) > case 'u': > forget = 1; > break; > + case 'v': > + verbose = 1; > + break; > default: > usage_unknown_option(cmd, argv); > } > @@ -354,7 +358,7 @@ static int cmd_device_scan(const struct cmd_struct *cmd, int argc, char **argv) > } > } else { > printf("Scanning for Btrfs filesystems\n"); > - ret = btrfs_scan_devices(); > + ret = btrfs_scan_devices(verbose); > error_on(ret, "error %d while scanning", ret); > ret = btrfs_register_all_devices(); > error_on(ret, > diff --git a/cmds/filesystem.c b/cmds/filesystem.c > index 4f22089abeaa..37b23af36847 100644 > --- a/cmds/filesystem.c > +++ b/cmds/filesystem.c > @@ -746,7 +746,7 @@ devs_only: > else > ret = 1; > } else { > - ret = btrfs_scan_devices(); > + ret = btrfs_scan_devices(0); > } > > if (ret) { > diff --git a/common/device-scan.c b/common/device-scan.c > index 2c5ae225f710..bea201b351f0 100644 > --- a/common/device-scan.c > +++ b/common/device-scan.c > @@ -351,7 +351,7 @@ void free_seen_fsid(struct seen_fsid *seen_fsid_hash[]) > } > } > > -int btrfs_scan_devices(void) > +int btrfs_scan_devices(int verbose) > { > int fd = -1; > int ret; > @@ -380,6 +380,8 @@ int btrfs_scan_devices(void) > continue; > /* if we are here its definitely a btrfs disk*/ > strncpy_null(path, blkid_dev_devname(dev)); > + if (verbose) > + printf("blkid: btrfs device: %s\n", path); > > fd = open(path, O_RDONLY); > if (fd < 0) { > diff --git a/common/device-scan.h b/common/device-scan.h > index eda2bae5c6c4..8017a27511b9 100644 > --- a/common/device-scan.h > +++ b/common/device-scan.h > @@ -29,7 +29,7 @@ struct seen_fsid { > int fd; > }; > > -int btrfs_scan_devices(void); > +int btrfs_scan_devices(int verbose); > int btrfs_register_one_device(const char *fname); > int btrfs_register_all_devices(void); > int btrfs_add_to_fsid(struct btrfs_trans_handle *trans, > diff --git a/common/utils.c b/common/utils.c > index ad938409a94f..36ce89a025f1 100644 > --- a/common/utils.c > +++ b/common/utils.c > @@ -277,7 +277,7 @@ int check_mounted_where(int fd, const char *file, char *where, int size, > > /* scan other devices */ > if (is_btrfs && total_devs > 1) { > - ret = btrfs_scan_devices(); > + ret = btrfs_scan_devices(0); > if (ret) > return ret; > } > diff --git a/disk-io.c b/disk-io.c > index be44eead5cef..4f52a29700ab 100644 > --- a/disk-io.c > +++ b/disk-io.c > @@ -1085,7 +1085,7 @@ int btrfs_scan_fs_devices(int fd, const char *path, > } > > if (!skip_devices && total_devs != 1) { > - ret = btrfs_scan_devices(); > + ret = btrfs_scan_devices(0); > if (ret) > return ret; > } >
> On 15 Jul 2019, at 11:09 PM, Nikolay Borisov <nborisov@suse.com> wrote: > > > > On 15.07.19 г. 17:42 ч., Anand Jain wrote: >> To help debug device scan issues, add verbose option to btrfs device scan. >> >> Signed-off-by: Anand Jain <anand.jain@oracle.com> > > I fail to see what this patch helps for. > To know what are the devices path scanned. > We get the path in case of errors, No. We get the devices path when we use -v option and the cli [1] can be success / fail. [1] 'btrfs device scan -v' > in case of success what good could the path be ? > >> --- >> cmds/device.c | 8 ++++++-- >> cmds/filesystem.c | 2 +- >> common/device-scan.c | 4 +++- >> common/device-scan.h | 2 +- >> common/utils.c | 2 +- >> disk-io.c | 2 +- >> 6 files changed, 13 insertions(+), 7 deletions(-) >> >> diff --git a/cmds/device.c b/cmds/device.c >> index 24158308a41b..2fa13e61f806 100644 >> --- a/cmds/device.c >> +++ b/cmds/device.c >> @@ -313,6 +313,7 @@ static int cmd_device_scan(const struct cmd_struct *cmd, int argc, char **argv) >> int all = 0; >> int ret = 0; >> int forget = 0; >> + int verbose = 0; > > nit: make it a bool. yep. Will do. Thanks, Anand
On 2019/7/15 下午11:09, Nikolay Borisov wrote: > > > On 15.07.19 г. 17:42 ч., Anand Jain wrote: >> To help debug device scan issues, add verbose option to btrfs device scan. >> >> Signed-off-by: Anand Jain <anand.jain@oracle.com> > > I fail to see what this patch helps for. We get the path in case of > errors, in case of success what good could the path be ? AFAIK it would provide an easy way to debug blkid related bug. E.g. scan only works on some devices and misses some devices. So it makes sense to me, although "debug" would be more suitable in this case. Thanks, Qu > > >> --- >> cmds/device.c | 8 ++++++-- >> cmds/filesystem.c | 2 +- >> common/device-scan.c | 4 +++- >> common/device-scan.h | 2 +- >> common/utils.c | 2 +- >> disk-io.c | 2 +- >> 6 files changed, 13 insertions(+), 7 deletions(-) >> >> diff --git a/cmds/device.c b/cmds/device.c >> index 24158308a41b..2fa13e61f806 100644 >> --- a/cmds/device.c >> +++ b/cmds/device.c >> @@ -313,6 +313,7 @@ static int cmd_device_scan(const struct cmd_struct *cmd, int argc, char **argv) >> int all = 0; >> int ret = 0; >> int forget = 0; >> + int verbose = 0; > > nit: make it a bool. > >> >> optind = 0; >> while (1) { >> @@ -323,7 +324,7 @@ static int cmd_device_scan(const struct cmd_struct *cmd, int argc, char **argv) >> { NULL, 0, NULL, 0} >> }; >> >> - c = getopt_long(argc, argv, "du", long_options, NULL); >> + c = getopt_long(argc, argv, "duv", long_options, NULL); >> if (c < 0) >> break; >> switch (c) { >> @@ -333,6 +334,9 @@ static int cmd_device_scan(const struct cmd_struct *cmd, int argc, char **argv) >> case 'u': >> forget = 1; >> break; >> + case 'v': >> + verbose = 1; >> + break; >> default: >> usage_unknown_option(cmd, argv); >> } >> @@ -354,7 +358,7 @@ static int cmd_device_scan(const struct cmd_struct *cmd, int argc, char **argv) >> } >> } else { >> printf("Scanning for Btrfs filesystems\n"); >> - ret = btrfs_scan_devices(); >> + ret = btrfs_scan_devices(verbose); >> error_on(ret, "error %d while scanning", ret); >> ret = btrfs_register_all_devices(); >> error_on(ret, >> diff --git a/cmds/filesystem.c b/cmds/filesystem.c >> index 4f22089abeaa..37b23af36847 100644 >> --- a/cmds/filesystem.c >> +++ b/cmds/filesystem.c >> @@ -746,7 +746,7 @@ devs_only: >> else >> ret = 1; >> } else { >> - ret = btrfs_scan_devices(); >> + ret = btrfs_scan_devices(0); >> } >> >> if (ret) { >> diff --git a/common/device-scan.c b/common/device-scan.c >> index 2c5ae225f710..bea201b351f0 100644 >> --- a/common/device-scan.c >> +++ b/common/device-scan.c >> @@ -351,7 +351,7 @@ void free_seen_fsid(struct seen_fsid *seen_fsid_hash[]) >> } >> } >> >> -int btrfs_scan_devices(void) >> +int btrfs_scan_devices(int verbose) >> { >> int fd = -1; >> int ret; >> @@ -380,6 +380,8 @@ int btrfs_scan_devices(void) >> continue; >> /* if we are here its definitely a btrfs disk*/ >> strncpy_null(path, blkid_dev_devname(dev)); >> + if (verbose) >> + printf("blkid: btrfs device: %s\n", path); >> >> fd = open(path, O_RDONLY); >> if (fd < 0) { >> diff --git a/common/device-scan.h b/common/device-scan.h >> index eda2bae5c6c4..8017a27511b9 100644 >> --- a/common/device-scan.h >> +++ b/common/device-scan.h >> @@ -29,7 +29,7 @@ struct seen_fsid { >> int fd; >> }; >> >> -int btrfs_scan_devices(void); >> +int btrfs_scan_devices(int verbose); >> int btrfs_register_one_device(const char *fname); >> int btrfs_register_all_devices(void); >> int btrfs_add_to_fsid(struct btrfs_trans_handle *trans, >> diff --git a/common/utils.c b/common/utils.c >> index ad938409a94f..36ce89a025f1 100644 >> --- a/common/utils.c >> +++ b/common/utils.c >> @@ -277,7 +277,7 @@ int check_mounted_where(int fd, const char *file, char *where, int size, >> >> /* scan other devices */ >> if (is_btrfs && total_devs > 1) { >> - ret = btrfs_scan_devices(); >> + ret = btrfs_scan_devices(0); >> if (ret) >> return ret; >> } >> diff --git a/disk-io.c b/disk-io.c >> index be44eead5cef..4f52a29700ab 100644 >> --- a/disk-io.c >> +++ b/disk-io.c >> @@ -1085,7 +1085,7 @@ int btrfs_scan_fs_devices(int fd, const char *path, >> } >> >> if (!skip_devices && total_devs != 1) { >> - ret = btrfs_scan_devices(); >> + ret = btrfs_scan_devices(0); >> if (ret) >> return ret; >> } >>
> On 16 Jul 2019, at 9:26 AM, Qu Wenruo <quwenruo.btrfs@gmx.com> wrote: > > > > On 2019/7/15 下午11:09, Nikolay Borisov wrote: >> >> >> On 15.07.19 г. 17:42 ч., Anand Jain wrote: >>> To help debug device scan issues, add verbose option to btrfs device scan. >>> >>> Signed-off-by: Anand Jain <anand.jain@oracle.com> >> >> I fail to see what this patch helps for. We get the path in case of >> errors, in case of success what good could the path be ? > > AFAIK it would provide an easy way to debug blkid related bug. > > E.g. scan only works on some devices and misses some devices. > > So it makes sense to me, although "debug" would be more suitable in this > case. As the objective is to show the devices scanned with its device path, so IMO verbose would be suitable? Its a small change, can be modified at the time of integration. Will send V2 with a nit fix to use bool instead of int. Thanks, Anand
On 16.07.19 г. 4:26 ч., Qu Wenruo wrote: > > > On 2019/7/15 下午11:09, Nikolay Borisov wrote: >> >> >> On 15.07.19 г. 17:42 ч., Anand Jain wrote: >>> To help debug device scan issues, add verbose option to btrfs device scan. >>> >>> Signed-off-by: Anand Jain <anand.jain@oracle.com> >> >> I fail to see what this patch helps for. We get the path in case of >> errors, in case of success what good could the path be ? > > AFAIK it would provide an easy way to debug blkid related bug. > > E.g. scan only works on some devices and misses some devices. In this case (and I already debugged one such case) it's invaluable to use LIBBLKID_DEBUG environment variable, the debug string added in this patch won't help in this particular case. <snip>
> On 16 Jul 2019, at 2:46 PM, Nikolay Borisov <nborisov@suse.com> wrote: > > > > On 16.07.19 г. 4:26 ч., Qu Wenruo wrote: >> >> >> On 2019/7/15 下午11:09, Nikolay Borisov wrote: >>> >>> >>> On 15.07.19 г. 17:42 ч., Anand Jain wrote: >>>> To help debug device scan issues, add verbose option to btrfs device scan. >>>> >>>> Signed-off-by: Anand Jain <anand.jain@oracle.com> >>> >>> I fail to see what this patch helps for. We get the path in case of >>> errors, in case of success what good could the path be ? >> >> AFAIK it would provide an easy way to debug blkid related bug. >> >> E.g. scan only works on some devices and misses some devices. > > In this case (and I already debugged one such case) it's invaluable to > use LIBBLKID_DEBUG environment variable, the debug string added in this > patch won't help in this particular case. > > <snip> Export LIBBLKID_DEBUG=all is good for debugging libblkid itself, btrfs dev scan -v provides a confirmation on which devices were scanned and their paths. Thanks, Anand
diff --git a/cmds/device.c b/cmds/device.c index 24158308a41b..2fa13e61f806 100644 --- a/cmds/device.c +++ b/cmds/device.c @@ -313,6 +313,7 @@ static int cmd_device_scan(const struct cmd_struct *cmd, int argc, char **argv) int all = 0; int ret = 0; int forget = 0; + int verbose = 0; optind = 0; while (1) { @@ -323,7 +324,7 @@ static int cmd_device_scan(const struct cmd_struct *cmd, int argc, char **argv) { NULL, 0, NULL, 0} }; - c = getopt_long(argc, argv, "du", long_options, NULL); + c = getopt_long(argc, argv, "duv", long_options, NULL); if (c < 0) break; switch (c) { @@ -333,6 +334,9 @@ static int cmd_device_scan(const struct cmd_struct *cmd, int argc, char **argv) case 'u': forget = 1; break; + case 'v': + verbose = 1; + break; default: usage_unknown_option(cmd, argv); } @@ -354,7 +358,7 @@ static int cmd_device_scan(const struct cmd_struct *cmd, int argc, char **argv) } } else { printf("Scanning for Btrfs filesystems\n"); - ret = btrfs_scan_devices(); + ret = btrfs_scan_devices(verbose); error_on(ret, "error %d while scanning", ret); ret = btrfs_register_all_devices(); error_on(ret, diff --git a/cmds/filesystem.c b/cmds/filesystem.c index 4f22089abeaa..37b23af36847 100644 --- a/cmds/filesystem.c +++ b/cmds/filesystem.c @@ -746,7 +746,7 @@ devs_only: else ret = 1; } else { - ret = btrfs_scan_devices(); + ret = btrfs_scan_devices(0); } if (ret) { diff --git a/common/device-scan.c b/common/device-scan.c index 2c5ae225f710..bea201b351f0 100644 --- a/common/device-scan.c +++ b/common/device-scan.c @@ -351,7 +351,7 @@ void free_seen_fsid(struct seen_fsid *seen_fsid_hash[]) } } -int btrfs_scan_devices(void) +int btrfs_scan_devices(int verbose) { int fd = -1; int ret; @@ -380,6 +380,8 @@ int btrfs_scan_devices(void) continue; /* if we are here its definitely a btrfs disk*/ strncpy_null(path, blkid_dev_devname(dev)); + if (verbose) + printf("blkid: btrfs device: %s\n", path); fd = open(path, O_RDONLY); if (fd < 0) { diff --git a/common/device-scan.h b/common/device-scan.h index eda2bae5c6c4..8017a27511b9 100644 --- a/common/device-scan.h +++ b/common/device-scan.h @@ -29,7 +29,7 @@ struct seen_fsid { int fd; }; -int btrfs_scan_devices(void); +int btrfs_scan_devices(int verbose); int btrfs_register_one_device(const char *fname); int btrfs_register_all_devices(void); int btrfs_add_to_fsid(struct btrfs_trans_handle *trans, diff --git a/common/utils.c b/common/utils.c index ad938409a94f..36ce89a025f1 100644 --- a/common/utils.c +++ b/common/utils.c @@ -277,7 +277,7 @@ int check_mounted_where(int fd, const char *file, char *where, int size, /* scan other devices */ if (is_btrfs && total_devs > 1) { - ret = btrfs_scan_devices(); + ret = btrfs_scan_devices(0); if (ret) return ret; } diff --git a/disk-io.c b/disk-io.c index be44eead5cef..4f52a29700ab 100644 --- a/disk-io.c +++ b/disk-io.c @@ -1085,7 +1085,7 @@ int btrfs_scan_fs_devices(int fd, const char *path, } if (!skip_devices && total_devs != 1) { - ret = btrfs_scan_devices(); + ret = btrfs_scan_devices(0); if (ret) return ret; }
To help debug device scan issues, add verbose option to btrfs device scan. Signed-off-by: Anand Jain <anand.jain@oracle.com> --- cmds/device.c | 8 ++++++-- cmds/filesystem.c | 2 +- common/device-scan.c | 4 +++- common/device-scan.h | 2 +- common/utils.c | 2 +- disk-io.c | 2 +- 6 files changed, 13 insertions(+), 7 deletions(-)