diff mbox series

[v3] btrfs-progs: add verbose option to btrfs device scan

Message ID 1569989512-5594-1-git-send-email-anand.jain@oracle.com (mailing list archive)
State New, archived
Headers show
Series [v3] btrfs-progs: add verbose option to btrfs device scan | expand

Commit Message

Anand Jain Oct. 2, 2019, 4:11 a.m. UTC 
To help debug device scan issues, add verbose option to btrfs device scan.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
Tested-by: Graham Cobb <g.btrfs@cobb.uk.net>
---
v3: Add --verbose long option and update documentation. Thanks Graham.A
    Add Tested-by.
v2: Use bool instead of int as a btrfs_scan_device() argument.

 Documentation/btrfs-device.asciidoc | 2 ++
 cmds/device.c                       | 9 +++++++--
 cmds/filesystem.c                   | 2 +-
 common/device-scan.c                | 4 +++-
 common/device-scan.h                | 3 ++-
 common/utils.c                      | 2 +-
 disk-io.c                           | 2 +-
 7 files changed, 17 insertions(+), 7 deletions(-)

Comments

David Sterba Oct. 7, 2019, 5:41 p.m. UTC | #1 
On Wed, Oct 02, 2019 at 12:11:52PM +0800, Anand Jain wrote:
> To help debug device scan issues, add verbose option to btrfs device scan.

The common options like --verbose are going to be added into the global
command so I'd rather avoid adding them to new subcommands as this would
become unnecessary compatibility issue.

There's an pattern to follow, the output formats (--format). So add a
definition for global verbosity options, add new GETOPT_VAL global enum
values that do not clash with existing options, add relevant
HELPINFO_INSERT_ text string and use it in commands where needed.
Anand Jain Oct. 8, 2019, 2:53 a.m. UTC | #2 
On 10/8/19 1:41 AM, David Sterba wrote:
> On Wed, Oct 02, 2019 at 12:11:52PM +0800, Anand Jain wrote:
>> To help debug device scan issues, add verbose option to btrfs device scan.
> 
> The common options like --verbose are going to be added into the global
> command so I'd rather avoid adding them to new subcommands as this would
> become unnecessary compatibility issue.

> There's an pattern to follow, the output formats (--format). So add a
> definition for global verbosity options, add new GETOPT_VAL global enum
> values that do not clash with existing options, add relevant
> HELPINFO_INSERT_ text string and use it in commands where needed.
> 

  IMO a debug option should rather be at the top level command.
  If verbose is the top level it would emit a lot of unwanted messages.
  Here is how a user is using --verbose option in dev scan.

 
https://lore.kernel.org/linux-btrfs/2daf15de-d1e7-b56a-be51-a6a3062ad28a@oracle.com/T/#t

------------
useful to get the list of devices it finds.
------------

  OR I didn't get the whole idea here. Looks like you are suggesting
  something like

   btrfs --verbose device scan
   btrfs --verbose subvolume list <mnt>
   ::

  How does the user will know if a subcommand will have any verbose
  or not?

  How would you not emit unwanted messages and keep the output clutter
  free.


Thanks, Anand
David Sterba Oct. 14, 2019, 3:24 p.m. UTC | #3 
On Tue, Oct 08, 2019 at 10:53:37AM +0800, Anand Jain wrote:
> On 10/8/19 1:41 AM, David Sterba wrote:
> > On Wed, Oct 02, 2019 at 12:11:52PM +0800, Anand Jain wrote:
> >> To help debug device scan issues, add verbose option to btrfs device scan.
> > 
> > The common options like --verbose are going to be added into the global
> > command so I'd rather avoid adding them to new subcommands as this would
> > become unnecessary compatibility issue.
> 
> > There's an pattern to follow, the output formats (--format). So add a
> > definition for global verbosity options, add new GETOPT_VAL global enum
> > values that do not clash with existing options, add relevant
> > HELPINFO_INSERT_ text string and use it in commands where needed.
> > 
> 
>   IMO a debug option should rather be at the top level command.
>   If verbose is the top level it would emit a lot of unwanted messages.
>   Here is how a user is using --verbose option in dev scan.
> 
>  
> https://lore.kernel.org/linux-btrfs/2daf15de-d1e7-b56a-be51-a6a3062ad28a@oracle.com/T/#t
> 
> ------------
> useful to get the list of devices it finds.
> ------------
> 
>   OR I didn't get the whole idea here. Looks like you are suggesting
>   something like
> 
>    btrfs --verbose device scan
>    btrfs --verbose subvolume list <mnt>

Yes this is what I mean.

>    ::
> 
>   How does the user will know if a subcommand will have any verbose
>   or not?

The point is that the global option will work for all subcommands, so
the user does not have to know which support that or not. For
compatiblity reasons, what works now will continue to work. This means
that the verbosity option will be duplicated for some commands.

>   How would you not emit unwanted messages and keep the output clutter
>   free.

What unwanted messages? Though the verbosity option will be set as
global option, it will be up to the command itself what to print.

  $ btrfs -v device scan

would be equivalent to

  $ btrfs device scan -v
Anand Jain Oct. 15, 2019, 3:29 a.m. UTC | #4 
On 10/14/19 11:24 PM, David Sterba wrote:
> On Tue, Oct 08, 2019 at 10:53:37AM +0800, Anand Jain wrote:
>> On 10/8/19 1:41 AM, David Sterba wrote:
>>> On Wed, Oct 02, 2019 at 12:11:52PM +0800, Anand Jain wrote:
>>>> To help debug device scan issues, add verbose option to btrfs device scan.
>>>
>>> The common options like --verbose are going to be added into the global
>>> command so I'd rather avoid adding them to new subcommands as this would
>>> become unnecessary compatibility issue.
>>
>>> There's an pattern to follow, the output formats (--format). So add a
>>> definition for global verbosity options, add new GETOPT_VAL global enum
>>> values that do not clash with existing options, add relevant
>>> HELPINFO_INSERT_ text string and use it in commands where needed.
>>>
>>
>>    IMO a debug option should rather be at the top level command.
>>    If verbose is the top level it would emit a lot of unwanted messages.
>>    Here is how a user is using --verbose option in dev scan.
>>
>>   
>> https://lore.kernel.org/linux-btrfs/2daf15de-d1e7-b56a-be51-a6a3062ad28a@oracle.com/T/#t
>>
>> ------------
>> useful to get the list of devices it finds.
>> ------------
>>
>>    OR I didn't get the whole idea here. Looks like you are suggesting
>>    something like
>>
>>     btrfs --verbose device scan
>>     btrfs --verbose subvolume list <mnt>
> 
> Yes this is what I mean.
> 
>>     ::
>>
>>    How does the user will know if a subcommand will have any verbose
>>    or not?
> 
> The point is that the global option will work for all subcommands, so
> the user does not have to know which support that or not. For
> compatiblity reasons, what works now will continue to work. This means
> that the verbosity option will be duplicated for some commands.
> 
>>    How would you not emit unwanted messages and keep the output clutter
>>    free.
> 
> What unwanted messages? Though the verbosity option will be set as
> global option, it will be up to the command itself what to print.
> 
>    $ btrfs -v device scan
> 
> would be equivalent to
> 
>    $ btrfs device scan -v
> 

  I was thinking there might be some common code between the
  sub-commands in btrfs-progs now or in future, and if the printf()
  due to verbose is required in one sub-command and the same printf()
  due to verbose is not required in another sub-command (which I
  called unwanted message) then we won't have any choice to not
  to print those unwanted printf().
  But as this is just an anticipatory only, so probably we could try
  global verbose and see how it fares. I will try.

Thanks, Anand
David Sterba Oct. 21, 2019, 1:43 p.m. UTC | #5 
On Tue, Oct 15, 2019 at 11:29:34AM +0800, Anand Jain wrote:
>   I was thinking there might be some common code between the
>   sub-commands in btrfs-progs now or in future, and if the printf()
>   due to verbose is required in one sub-command and the same printf()
>   due to verbose is not required in another sub-command (which I
>   called unwanted message) then we won't have any choice to not
>   to print those unwanted printf().
>   But as this is just an anticipatory only, so probably we could try
>   global verbose and see how it fares. I will try.

I see, but it would be better to have a concrete example where it's
problematic so we can figure out ways how to filter unwanted messages.
Anand Jain Oct. 21, 2019, 11:48 p.m. UTC | #6 
On 10/21/19 9:43 PM, David Sterba wrote:
> On Tue, Oct 15, 2019 at 11:29:34AM +0800, Anand Jain wrote:
>>    I was thinking there might be some common code between the
>>    sub-commands in btrfs-progs now or in future, and if the printf()
>>    due to verbose is required in one sub-command and the same printf()
>>    due to verbose is not required in another sub-command (which I
>>    called unwanted message) then we won't have any choice to not
>>    to print those unwanted printf().
>>    But as this is just an anticipatory only, so probably we could try
>>    global verbose and see how it fares. I will try.
> 
> I see, but it would be better to have a concrete example where it's
> problematic so we can figure out ways how to filter unwanted messages.
> 

I solved with an argument to btrfs_scan_devcies() [1], by adding
%verbose argument to btrfs_scan_devices() to make sure
only btrfs dev scan would print the verbose and not the btrfs fi show.
If btrfs fi show prints the verbose it shall break few test-cases
in fstests.

[1]
https://patchwork.kernel.org/patch/11201791/
https://patchwork.kernel.org/patch/11201793/
diff mbox series

Patch

diff --git a/Documentation/btrfs-device.asciidoc b/Documentation/btrfs-device.asciidoc
index 70ce6c5a97f8..a251be9e0fa2 100644
--- a/Documentation/btrfs-device.asciidoc
+++ b/Documentation/btrfs-device.asciidoc
@@ -120,6 +120,8 @@  available.
 -u|--forget::::
 Unregister a given device or all stale devices if no path is given, the device
 must be unmounted otherwise it's an error.
+-v|--verbose::::
+Be verbose and list the devices scanned.
 
 *stats* [options] <path>|<device>::
 Read and print the device IO error statistics for all devices of the given
diff --git a/cmds/device.c b/cmds/device.c
index 24158308a41b..655b97d83cb7 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;
+	bool verbose = false;
 
 	optind = 0;
 	while (1) {
@@ -320,10 +321,11 @@  static int cmd_device_scan(const struct cmd_struct *cmd, int argc, char **argv)
 		static const struct option long_options[] = {
 			{ "all-devices", no_argument, NULL, 'd'},
 			{ "forget", no_argument, NULL, 'u'},
+			{ "verbose", no_argument, NULL, 'v'},
 			{ 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 +335,9 @@  static int cmd_device_scan(const struct cmd_struct *cmd, int argc, char **argv)
 		case 'u':
 			forget = 1;
 			break;
+		case 'v':
+			verbose = true;
+			break;
 		default:
 			usage_unknown_option(cmd, argv);
 		}
@@ -354,7 +359,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..02d47a43a792 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(false);
 	}
 
 	if (ret) {
diff --git a/common/device-scan.c b/common/device-scan.c
index 48dbd9e19715..a500edf0f7d7 100644
--- a/common/device-scan.c
+++ b/common/device-scan.c
@@ -360,7 +360,7 @@  void free_seen_fsid(struct seen_fsid *seen_fsid_hash[])
 	}
 }
 
-int btrfs_scan_devices(void)
+int btrfs_scan_devices(bool verbose)
 {
 	int fd = -1;
 	int ret;
@@ -389,6 +389,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..3e473c48d1af 100644
--- a/common/device-scan.h
+++ b/common/device-scan.h
@@ -1,6 +1,7 @@ 
 #ifndef __DEVICE_SCAN_H__
 #define __DEVICE_SCAN_H__
 
+#include <stdbool.h>
 #include "kerncompat.h"
 #include "ioctl.h"
 
@@ -29,7 +30,7 @@  struct seen_fsid {
 	int fd;
 };
 
-int btrfs_scan_devices(void);
+int btrfs_scan_devices(bool 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 6617b3ef38b1..9027de596f5d 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(false);
 		if (ret)
 			return ret;
 	}
diff --git a/disk-io.c b/disk-io.c
index fa679133e171..58861ccfb4ec 100644
--- a/disk-io.c
+++ b/disk-io.c
@@ -1096,7 +1096,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(false);
 		if (ret)
 			return ret;
 	}