[v5,RESEND] btrfs-progs: dump-tree: add noscan option
diff mbox series

Message ID 20190404072957.2847-1-anand.jain@oracle.com
State New
Headers show
Series
  • [v5,RESEND] btrfs-progs: dump-tree: add noscan option
Related show

Commit Message

Anand Jain April 4, 2019, 7:29 a.m. UTC
From: Anand Jain <Anand.Jain@oracle.com>

The cli 'btrfs inspect dump-tree <dev>' will scan for the partner devices
if any by default.

So as of now you can not inspect each mirrored device independently.

This patch adds noscan option, which when used won't scan the system for
the partner devices, instead it just uses the devices provided in the
argument.

For example:
  btrfs inspect dump-tree --noscan <dev> [<dev>..]

This helps to debug degraded raid1 and raid10.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
v4->v5: nit: use %m to print error string.
	changelog update.
v3->v4: change the patch title.
	collapse scan_args() to its only parent cmd_inspect_dump_tree()
	(it was bit confusing).
	update the change log.
	update usage.
	update man page.
v2->v3: make it scalable for more than two disks in noscan mode
v1->v2: rename --degraded to --noscan

 Documentation/btrfs-inspect-internal.asciidoc |  5 ++-
 cmds-inspect-dump-tree.c                      | 53 +++++++++++++++++++++------
 2 files changed, 45 insertions(+), 13 deletions(-)

Comments

Qu Wenruo April 11, 2019, 4:53 a.m. UTC | #1
On 2019/4/4 下午3:29, Anand Jain wrote:
> From: Anand Jain <Anand.Jain@oracle.com>
> 
> The cli 'btrfs inspect dump-tree <dev>' will scan for the partner devices
> if any by default.
> 
> So as of now you can not inspect each mirrored device independently.
> 
> This patch adds noscan option, which when used won't scan the system for
> the partner devices, instead it just uses the devices provided in the
> argument.
> 
> For example:
>   btrfs inspect dump-tree --noscan <dev> [<dev>..]

So you can specify multiple devices, just like kernel "device=" mount
option.

Then I don't think --noscan is a good naming, while I don't have any
good alternative, as the --degraded is no better.

My idea is "(--device=<dev1>,<dev2>)|(<dev>)", but that looks more awful.

Despite of that, the patch looks pretty good.

> 
> This helps to debug degraded raid1 and raid10.
> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
> v4->v5: nit: use %m to print error string.
> 	changelog update.
> v3->v4: change the patch title.
> 	collapse scan_args() to its only parent cmd_inspect_dump_tree()
> 	(it was bit confusing).

I don't this is the correct direction.

You're introducing "--device=" btrfs-progs equivalent, there is no way
it will only be utilized by dump-tree.

Btrfs-check, btrfs-restore can all take advantage of this feature.

You don't need to resend, later code can export them when needed.
But doing it at the first place really helps.

Thanks,
Qu

> 	update the change log.
> 	update usage.
> 	update man page.
> v2->v3: make it scalable for more than two disks in noscan mode
> v1->v2: rename --degraded to --noscan
> 
>  Documentation/btrfs-inspect-internal.asciidoc |  5 ++-
>  cmds-inspect-dump-tree.c                      | 53 +++++++++++++++++++++------
>  2 files changed, 45 insertions(+), 13 deletions(-)
> 
> diff --git a/Documentation/btrfs-inspect-internal.asciidoc b/Documentation/btrfs-inspect-internal.asciidoc
> index 381497d284b8..f9d7f1c58f00 100644
> --- a/Documentation/btrfs-inspect-internal.asciidoc
> +++ b/Documentation/btrfs-inspect-internal.asciidoc
> @@ -61,7 +61,7 @@ specify which mirror to print, valid values are 0, 1 and 2 and the superblock
>  must be present on the device with a valid signature, can be used together with
>  '--force'
>  
> -*dump-tree* [options] <device>::
> +*dump-tree* [options] <device> [device...]::
>  (replaces the standalone tool *btrfs-debug-tree*)
>  +
>  Dump tree structures from a given device in textual form, expand keys to human
> @@ -95,6 +95,9 @@ intermixed in the output
>  --bfs::::
>  use breadth-first search to print trees. the nodes are printed before all
>  leaves
> +--noscan::::
> +do not scan the system for other partner device(s), only use the device(s)
> +provided in the argument
>  -t <tree_id>::::
>  print only the tree with the specified ID, where the ID can be numerical or
>  common name in a flexible human readable form
> diff --git a/cmds-inspect-dump-tree.c b/cmds-inspect-dump-tree.c
> index ad5345b4f1db..37d9b29fa824 100644
> --- a/cmds-inspect-dump-tree.c
> +++ b/cmds-inspect-dump-tree.c
> @@ -21,6 +21,7 @@
>  #include <unistd.h>
>  #include <uuid/uuid.h>
>  #include <getopt.h>
> +#include <fcntl.h>
>  
>  #include "kerncompat.h"
>  #include "radix-tree.h"
> @@ -185,7 +186,7 @@ static u64 treeid_from_string(const char *str, const char **end)
>  }
>  
>  const char * const cmd_inspect_dump_tree_usage[] = {
> -	"btrfs inspect-internal dump-tree [options] device",
> +	"btrfs inspect-internal dump-tree [options] <device> [<device> ..]",
>  	"Dump tree structures from a given device",
>  	"Dump tree structures from a given device in textual form, expand keys to human",
>  	"readable equivalents where possible.",
> @@ -200,6 +201,7 @@ const char * const cmd_inspect_dump_tree_usage[] = {
>  	"-b|--block <block_num> print info from the specified block only",
>  	"-t|--tree <tree_id>    print only tree with the given id (string or number)",
>  	"--follow               use with -b, to show all children tree blocks of <block_num>",
> +	"--noscan               do not scan for the partner device(s)",
>  	NULL
>  };
>  
> @@ -214,7 +216,7 @@ int cmd_inspect_dump_tree(int argc, char **argv)
>  	struct btrfs_disk_key disk_key;
>  	struct btrfs_key found_key;
>  	char uuidbuf[BTRFS_UUID_UNPARSED_SIZE];
> -	int ret;
> +	int ret = 0;
>  	int slot;
>  	int extent_only = 0;
>  	int device_only = 0;
> @@ -222,6 +224,7 @@ int cmd_inspect_dump_tree(int argc, char **argv)
>  	int roots_only = 0;
>  	int root_backups = 0;
>  	int traverse = BTRFS_PRINT_TREE_DEFAULT;
> +	int dev_optind;
>  	unsigned open_ctree_flags;
>  	u64 block_only = 0;
>  	struct btrfs_root *tree_root_scan;
> @@ -239,8 +242,8 @@ int cmd_inspect_dump_tree(int argc, char **argv)
>  	optind = 0;
>  	while (1) {
>  		int c;
> -		enum { GETOPT_VAL_FOLLOW = 256, GETOPT_VAL_DFS,
> -		       GETOPT_VAL_BFS };
> +		enum { GETOPT_VAL_FOLLOW = 256, GETOPT_VAL_DFS, GETOPT_VAL_BFS,
> +		       GETOPT_VAL_NOSCAN};
>  		static const struct option long_options[] = {
>  			{ "extents", no_argument, NULL, 'e'},
>  			{ "device", no_argument, NULL, 'd'},
> @@ -252,6 +255,7 @@ int cmd_inspect_dump_tree(int argc, char **argv)
>  			{ "follow", no_argument, NULL, GETOPT_VAL_FOLLOW },
>  			{ "bfs", no_argument, NULL, GETOPT_VAL_BFS },
>  			{ "dfs", no_argument, NULL, GETOPT_VAL_DFS },
> +			{ "noscan", no_argument, NULL, GETOPT_VAL_NOSCAN },
>  			{ NULL, 0, NULL, 0 }
>  		};
>  
> @@ -313,24 +317,49 @@ int cmd_inspect_dump_tree(int argc, char **argv)
>  		case GETOPT_VAL_BFS:
>  			traverse = BTRFS_PRINT_TREE_BFS;
>  			break;
> +		case GETOPT_VAL_NOSCAN:
> +			open_ctree_flags |= OPEN_CTREE_NO_DEVICES;
> +			break;
>  		default:
>  			usage(cmd_inspect_dump_tree_usage);
>  		}
>  	}
>  
> -	if (check_argc_exact(argc - optind, 1))
> +	if (check_argc_min(argc - optind, 1))
>  		usage(cmd_inspect_dump_tree_usage);
>  
> -	ret = check_arg_type(argv[optind]);
> -	if (ret != BTRFS_ARG_BLKDEV && ret != BTRFS_ARG_REG) {
> +	dev_optind = optind;
> +	while (dev_optind < argc) {
> +		int fd;
> +		struct btrfs_fs_devices *fs_devices;
> +		u64 num_devices;
> +
> +		ret = check_arg_type(argv[optind]);
> +		if (ret != BTRFS_ARG_BLKDEV && ret != BTRFS_ARG_REG) {
> +			if (ret < 0) {
> +				errno = -ret;
> +				error("invalid argument %s: %m", argv[dev_optind]);
> +			} else {
> +				error("not a block device or regular file: %s",
> +				       argv[dev_optind]);
> +			}
> +		}
> +		fd = open(argv[dev_optind], O_RDONLY);
> +		if (fd < 0) {
> +			error("cannot open %s: %m", argv[dev_optind]);
> +			return -EINVAL;
> +		}
> +		ret = btrfs_scan_one_device(fd, argv[dev_optind], &fs_devices,
> +					    &num_devices,
> +					    BTRFS_SUPER_INFO_OFFSET,
> +					    SBREAD_DEFAULT);
> +		close(fd);
>  		if (ret < 0) {
>  			errno = -ret;
> -			error("invalid argument %s: %m", argv[optind]);
> -		} else {
> -			error("not a block device or regular file: %s",
> -			      argv[optind]);
> +			error("device scan %s: %m", argv[dev_optind]);
> +			return ret;
>  		}
> -		goto out;
> +		dev_optind++;
>  	}
>  
>  	printf("%s\n", PACKAGE_STRING);
>
Nikolay Borisov April 11, 2019, 6:52 a.m. UTC | #2
On 11.04.19 г. 7:53 ч., Qu Wenruo wrote:
> 
> 
> On 2019/4/4 下午3:29, Anand Jain wrote:
>> From: Anand Jain <Anand.Jain@oracle.com>
>>
>> The cli 'btrfs inspect dump-tree <dev>' will scan for the partner devices
>> if any by default.
>>
>> So as of now you can not inspect each mirrored device independently.
>>
>> This patch adds noscan option, which when used won't scan the system for
>> the partner devices, instead it just uses the devices provided in the
>> argument.
>>
>> For example:
>>   btrfs inspect dump-tree --noscan <dev> [<dev>..]
> 
> So you can specify multiple devices, just like kernel "device=" mount
> option.
> 
> Then I don't think --noscan is a good naming, while I don't have any
> good alternative, as the --degraded is no better.

How about skipscan ?

> 
> My idea is "(--device=<dev1>,<dev2>)|(<dev>)", but that looks more awful.
> 
> Despite of that, the patch looks pretty good.
> 
>>
>> This helps to debug degraded raid1 and raid10.
>>
>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>> ---
>> v4->v5: nit: use %m to print error string.
>> 	changelog update.
>> v3->v4: change the patch title.
>> 	collapse scan_args() to its only parent cmd_inspect_dump_tree()
>> 	(it was bit confusing).
> 
> I don't this is the correct direction.
> 
> You're introducing "--device=" btrfs-progs equivalent, there is no way
> it will only be utilized by dump-tree.
> 
> Btrfs-check, btrfs-restore can all take advantage of this feature.
> 
> You don't need to resend, later code can export them when needed.
> But doing it at the first place really helps.
> 
> Thanks,
> Qu
> 
>> 	update the change log.
>> 	update usage.
>> 	update man page.
>> v2->v3: make it scalable for more than two disks in noscan mode
>> v1->v2: rename --degraded to --noscan
>>
>>  Documentation/btrfs-inspect-internal.asciidoc |  5 ++-
>>  cmds-inspect-dump-tree.c                      | 53 +++++++++++++++++++++------
>>  2 files changed, 45 insertions(+), 13 deletions(-)
>>
>> diff --git a/Documentation/btrfs-inspect-internal.asciidoc b/Documentation/btrfs-inspect-internal.asciidoc
>> index 381497d284b8..f9d7f1c58f00 100644
>> --- a/Documentation/btrfs-inspect-internal.asciidoc
>> +++ b/Documentation/btrfs-inspect-internal.asciidoc
>> @@ -61,7 +61,7 @@ specify which mirror to print, valid values are 0, 1 and 2 and the superblock
>>  must be present on the device with a valid signature, can be used together with
>>  '--force'
>>  
>> -*dump-tree* [options] <device>::
>> +*dump-tree* [options] <device> [device...]::
>>  (replaces the standalone tool *btrfs-debug-tree*)
>>  +
>>  Dump tree structures from a given device in textual form, expand keys to human
>> @@ -95,6 +95,9 @@ intermixed in the output
>>  --bfs::::
>>  use breadth-first search to print trees. the nodes are printed before all
>>  leaves
>> +--noscan::::
>> +do not scan the system for other partner device(s), only use the device(s)
>> +provided in the argument
>>  -t <tree_id>::::
>>  print only the tree with the specified ID, where the ID can be numerical or
>>  common name in a flexible human readable form
>> diff --git a/cmds-inspect-dump-tree.c b/cmds-inspect-dump-tree.c
>> index ad5345b4f1db..37d9b29fa824 100644
>> --- a/cmds-inspect-dump-tree.c
>> +++ b/cmds-inspect-dump-tree.c
>> @@ -21,6 +21,7 @@
>>  #include <unistd.h>
>>  #include <uuid/uuid.h>
>>  #include <getopt.h>
>> +#include <fcntl.h>
>>  
>>  #include "kerncompat.h"
>>  #include "radix-tree.h"
>> @@ -185,7 +186,7 @@ static u64 treeid_from_string(const char *str, const char **end)
>>  }
>>  
>>  const char * const cmd_inspect_dump_tree_usage[] = {
>> -	"btrfs inspect-internal dump-tree [options] device",
>> +	"btrfs inspect-internal dump-tree [options] <device> [<device> ..]",
>>  	"Dump tree structures from a given device",
>>  	"Dump tree structures from a given device in textual form, expand keys to human",
>>  	"readable equivalents where possible.",
>> @@ -200,6 +201,7 @@ const char * const cmd_inspect_dump_tree_usage[] = {
>>  	"-b|--block <block_num> print info from the specified block only",
>>  	"-t|--tree <tree_id>    print only tree with the given id (string or number)",
>>  	"--follow               use with -b, to show all children tree blocks of <block_num>",
>> +	"--noscan               do not scan for the partner device(s)",
>>  	NULL
>>  };
>>  
>> @@ -214,7 +216,7 @@ int cmd_inspect_dump_tree(int argc, char **argv)
>>  	struct btrfs_disk_key disk_key;
>>  	struct btrfs_key found_key;
>>  	char uuidbuf[BTRFS_UUID_UNPARSED_SIZE];
>> -	int ret;
>> +	int ret = 0;
>>  	int slot;
>>  	int extent_only = 0;
>>  	int device_only = 0;
>> @@ -222,6 +224,7 @@ int cmd_inspect_dump_tree(int argc, char **argv)
>>  	int roots_only = 0;
>>  	int root_backups = 0;
>>  	int traverse = BTRFS_PRINT_TREE_DEFAULT;
>> +	int dev_optind;
>>  	unsigned open_ctree_flags;
>>  	u64 block_only = 0;
>>  	struct btrfs_root *tree_root_scan;
>> @@ -239,8 +242,8 @@ int cmd_inspect_dump_tree(int argc, char **argv)
>>  	optind = 0;
>>  	while (1) {
>>  		int c;
>> -		enum { GETOPT_VAL_FOLLOW = 256, GETOPT_VAL_DFS,
>> -		       GETOPT_VAL_BFS };
>> +		enum { GETOPT_VAL_FOLLOW = 256, GETOPT_VAL_DFS, GETOPT_VAL_BFS,
>> +		       GETOPT_VAL_NOSCAN};
>>  		static const struct option long_options[] = {
>>  			{ "extents", no_argument, NULL, 'e'},
>>  			{ "device", no_argument, NULL, 'd'},
>> @@ -252,6 +255,7 @@ int cmd_inspect_dump_tree(int argc, char **argv)
>>  			{ "follow", no_argument, NULL, GETOPT_VAL_FOLLOW },
>>  			{ "bfs", no_argument, NULL, GETOPT_VAL_BFS },
>>  			{ "dfs", no_argument, NULL, GETOPT_VAL_DFS },
>> +			{ "noscan", no_argument, NULL, GETOPT_VAL_NOSCAN },
>>  			{ NULL, 0, NULL, 0 }
>>  		};
>>  
>> @@ -313,24 +317,49 @@ int cmd_inspect_dump_tree(int argc, char **argv)
>>  		case GETOPT_VAL_BFS:
>>  			traverse = BTRFS_PRINT_TREE_BFS;
>>  			break;
>> +		case GETOPT_VAL_NOSCAN:
>> +			open_ctree_flags |= OPEN_CTREE_NO_DEVICES;
>> +			break;
>>  		default:
>>  			usage(cmd_inspect_dump_tree_usage);
>>  		}
>>  	}
>>  
>> -	if (check_argc_exact(argc - optind, 1))
>> +	if (check_argc_min(argc - optind, 1))
>>  		usage(cmd_inspect_dump_tree_usage);
>>  
>> -	ret = check_arg_type(argv[optind]);
>> -	if (ret != BTRFS_ARG_BLKDEV && ret != BTRFS_ARG_REG) {
>> +	dev_optind = optind;
>> +	while (dev_optind < argc) {
>> +		int fd;
>> +		struct btrfs_fs_devices *fs_devices;
>> +		u64 num_devices;
>> +
>> +		ret = check_arg_type(argv[optind]);
>> +		if (ret != BTRFS_ARG_BLKDEV && ret != BTRFS_ARG_REG) {
>> +			if (ret < 0) {
>> +				errno = -ret;
>> +				error("invalid argument %s: %m", argv[dev_optind]);
>> +			} else {
>> +				error("not a block device or regular file: %s",
>> +				       argv[dev_optind]);
>> +			}
>> +		}
>> +		fd = open(argv[dev_optind], O_RDONLY);
>> +		if (fd < 0) {
>> +			error("cannot open %s: %m", argv[dev_optind]);
>> +			return -EINVAL;
>> +		}
>> +		ret = btrfs_scan_one_device(fd, argv[dev_optind], &fs_devices,
>> +					    &num_devices,
>> +					    BTRFS_SUPER_INFO_OFFSET,
>> +					    SBREAD_DEFAULT);
>> +		close(fd);
>>  		if (ret < 0) {
>>  			errno = -ret;
>> -			error("invalid argument %s: %m", argv[optind]);
>> -		} else {
>> -			error("not a block device or regular file: %s",
>> -			      argv[optind]);
>> +			error("device scan %s: %m", argv[dev_optind]);
>> +			return ret;
>>  		}
>> -		goto out;
>> +		dev_optind++;
>>  	}
>>  
>>  	printf("%s\n", PACKAGE_STRING);
>>
>
Qu Wenruo April 11, 2019, 7:28 a.m. UTC | #3
On 2019/4/11 下午2:52, Nikolay Borisov wrote:
>
>
> On 11.04.19 г. 7:53 ч., Qu Wenruo wrote:
>>
>>
>> On 2019/4/4 下午3:29, Anand Jain wrote:
>>> From: Anand Jain <Anand.Jain@oracle.com>
>>>
>>> The cli 'btrfs inspect dump-tree <dev>' will scan for the partner devices
>>> if any by default.
>>>
>>> So as of now you can not inspect each mirrored device independently.
>>>
>>> This patch adds noscan option, which when used won't scan the system for
>>> the partner devices, instead it just uses the devices provided in the
>>> argument.
>>>
>>> For example:
>>>   btrfs inspect dump-tree --noscan <dev> [<dev>..]
>>
>> So you can specify multiple devices, just like kernel "device=" mount
>> option.
>>
>> Then I don't think --noscan is a good naming, while I don't have any
>> good alternative, as the --degraded is no better.
>
> How about skipscan ?

I still prefer "--device=", no surprise, anyone who used "device=" mount
option knows what it is.

The only problem is, when only using one device, it will be super stupid:
  dump-tree --device /dev/sda /dev/sda

If this doesn't sound good, then --noscan is good enough AFAIK.

Thanks,
Qu

>
>>
>> My idea is "(--device=<dev1>,<dev2>)|(<dev>)", but that looks more awful.
>>
>> Despite of that, the patch looks pretty good.
>>
>>>
>>> This helps to debug degraded raid1 and raid10.
>>>
>>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>>> ---
>>> v4->v5: nit: use %m to print error string.
>>> 	changelog update.
>>> v3->v4: change the patch title.
>>> 	collapse scan_args() to its only parent cmd_inspect_dump_tree()
>>> 	(it was bit confusing).
>>
>> I don't this is the correct direction.
>>
>> You're introducing "--device=" btrfs-progs equivalent, there is no way
>> it will only be utilized by dump-tree.
>>
>> Btrfs-check, btrfs-restore can all take advantage of this feature.
>>
>> You don't need to resend, later code can export them when needed.
>> But doing it at the first place really helps.
>>
>> Thanks,
>> Qu
>>
>>> 	update the change log.
>>> 	update usage.
>>> 	update man page.
>>> v2->v3: make it scalable for more than two disks in noscan mode
>>> v1->v2: rename --degraded to --noscan
>>>
>>>  Documentation/btrfs-inspect-internal.asciidoc |  5 ++-
>>>  cmds-inspect-dump-tree.c                      | 53 +++++++++++++++++++++------
>>>  2 files changed, 45 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/Documentation/btrfs-inspect-internal.asciidoc b/Documentation/btrfs-inspect-internal.asciidoc
>>> index 381497d284b8..f9d7f1c58f00 100644
>>> --- a/Documentation/btrfs-inspect-internal.asciidoc
>>> +++ b/Documentation/btrfs-inspect-internal.asciidoc
>>> @@ -61,7 +61,7 @@ specify which mirror to print, valid values are 0, 1 and 2 and the superblock
>>>  must be present on the device with a valid signature, can be used together with
>>>  '--force'
>>>
>>> -*dump-tree* [options] <device>::
>>> +*dump-tree* [options] <device> [device...]::
>>>  (replaces the standalone tool *btrfs-debug-tree*)
>>>  +
>>>  Dump tree structures from a given device in textual form, expand keys to human
>>> @@ -95,6 +95,9 @@ intermixed in the output
>>>  --bfs::::
>>>  use breadth-first search to print trees. the nodes are printed before all
>>>  leaves
>>> +--noscan::::
>>> +do not scan the system for other partner device(s), only use the device(s)
>>> +provided in the argument
>>>  -t <tree_id>::::
>>>  print only the tree with the specified ID, where the ID can be numerical or
>>>  common name in a flexible human readable form
>>> diff --git a/cmds-inspect-dump-tree.c b/cmds-inspect-dump-tree.c
>>> index ad5345b4f1db..37d9b29fa824 100644
>>> --- a/cmds-inspect-dump-tree.c
>>> +++ b/cmds-inspect-dump-tree.c
>>> @@ -21,6 +21,7 @@
>>>  #include <unistd.h>
>>>  #include <uuid/uuid.h>
>>>  #include <getopt.h>
>>> +#include <fcntl.h>
>>>
>>>  #include "kerncompat.h"
>>>  #include "radix-tree.h"
>>> @@ -185,7 +186,7 @@ static u64 treeid_from_string(const char *str, const char **end)
>>>  }
>>>
>>>  const char * const cmd_inspect_dump_tree_usage[] = {
>>> -	"btrfs inspect-internal dump-tree [options] device",
>>> +	"btrfs inspect-internal dump-tree [options] <device> [<device> ..]",
>>>  	"Dump tree structures from a given device",
>>>  	"Dump tree structures from a given device in textual form, expand keys to human",
>>>  	"readable equivalents where possible.",
>>> @@ -200,6 +201,7 @@ const char * const cmd_inspect_dump_tree_usage[] = {
>>>  	"-b|--block <block_num> print info from the specified block only",
>>>  	"-t|--tree <tree_id>    print only tree with the given id (string or number)",
>>>  	"--follow               use with -b, to show all children tree blocks of <block_num>",
>>> +	"--noscan               do not scan for the partner device(s)",
>>>  	NULL
>>>  };
>>>
>>> @@ -214,7 +216,7 @@ int cmd_inspect_dump_tree(int argc, char **argv)
>>>  	struct btrfs_disk_key disk_key;
>>>  	struct btrfs_key found_key;
>>>  	char uuidbuf[BTRFS_UUID_UNPARSED_SIZE];
>>> -	int ret;
>>> +	int ret = 0;
>>>  	int slot;
>>>  	int extent_only = 0;
>>>  	int device_only = 0;
>>> @@ -222,6 +224,7 @@ int cmd_inspect_dump_tree(int argc, char **argv)
>>>  	int roots_only = 0;
>>>  	int root_backups = 0;
>>>  	int traverse = BTRFS_PRINT_TREE_DEFAULT;
>>> +	int dev_optind;
>>>  	unsigned open_ctree_flags;
>>>  	u64 block_only = 0;
>>>  	struct btrfs_root *tree_root_scan;
>>> @@ -239,8 +242,8 @@ int cmd_inspect_dump_tree(int argc, char **argv)
>>>  	optind = 0;
>>>  	while (1) {
>>>  		int c;
>>> -		enum { GETOPT_VAL_FOLLOW = 256, GETOPT_VAL_DFS,
>>> -		       GETOPT_VAL_BFS };
>>> +		enum { GETOPT_VAL_FOLLOW = 256, GETOPT_VAL_DFS, GETOPT_VAL_BFS,
>>> +		       GETOPT_VAL_NOSCAN};
>>>  		static const struct option long_options[] = {
>>>  			{ "extents", no_argument, NULL, 'e'},
>>>  			{ "device", no_argument, NULL, 'd'},
>>> @@ -252,6 +255,7 @@ int cmd_inspect_dump_tree(int argc, char **argv)
>>>  			{ "follow", no_argument, NULL, GETOPT_VAL_FOLLOW },
>>>  			{ "bfs", no_argument, NULL, GETOPT_VAL_BFS },
>>>  			{ "dfs", no_argument, NULL, GETOPT_VAL_DFS },
>>> +			{ "noscan", no_argument, NULL, GETOPT_VAL_NOSCAN },
>>>  			{ NULL, 0, NULL, 0 }
>>>  		};
>>>
>>> @@ -313,24 +317,49 @@ int cmd_inspect_dump_tree(int argc, char **argv)
>>>  		case GETOPT_VAL_BFS:
>>>  			traverse = BTRFS_PRINT_TREE_BFS;
>>>  			break;
>>> +		case GETOPT_VAL_NOSCAN:
>>> +			open_ctree_flags |= OPEN_CTREE_NO_DEVICES;
>>> +			break;
>>>  		default:
>>>  			usage(cmd_inspect_dump_tree_usage);
>>>  		}
>>>  	}
>>>
>>> -	if (check_argc_exact(argc - optind, 1))
>>> +	if (check_argc_min(argc - optind, 1))
>>>  		usage(cmd_inspect_dump_tree_usage);
>>>
>>> -	ret = check_arg_type(argv[optind]);
>>> -	if (ret != BTRFS_ARG_BLKDEV && ret != BTRFS_ARG_REG) {
>>> +	dev_optind = optind;
>>> +	while (dev_optind < argc) {
>>> +		int fd;
>>> +		struct btrfs_fs_devices *fs_devices;
>>> +		u64 num_devices;
>>> +
>>> +		ret = check_arg_type(argv[optind]);
>>> +		if (ret != BTRFS_ARG_BLKDEV && ret != BTRFS_ARG_REG) {
>>> +			if (ret < 0) {
>>> +				errno = -ret;
>>> +				error("invalid argument %s: %m", argv[dev_optind]);
>>> +			} else {
>>> +				error("not a block device or regular file: %s",
>>> +				       argv[dev_optind]);
>>> +			}
>>> +		}
>>> +		fd = open(argv[dev_optind], O_RDONLY);
>>> +		if (fd < 0) {
>>> +			error("cannot open %s: %m", argv[dev_optind]);
>>> +			return -EINVAL;
>>> +		}
>>> +		ret = btrfs_scan_one_device(fd, argv[dev_optind], &fs_devices,
>>> +					    &num_devices,
>>> +					    BTRFS_SUPER_INFO_OFFSET,
>>> +					    SBREAD_DEFAULT);
>>> +		close(fd);
>>>  		if (ret < 0) {
>>>  			errno = -ret;
>>> -			error("invalid argument %s: %m", argv[optind]);
>>> -		} else {
>>> -			error("not a block device or regular file: %s",
>>> -			      argv[optind]);
>>> +			error("device scan %s: %m", argv[dev_optind]);
>>> +			return ret;
>>>  		}
>>> -		goto out;
>>> +		dev_optind++;
>>>  	}
>>>
>>>  	printf("%s\n", PACKAGE_STRING);
>>>
>>
Anand Jain April 12, 2019, 9:04 a.m. UTC | #4
On 11/4/19 3:28 PM, Qu Wenruo wrote:
> 
> 
> On 2019/4/11 下午2:52, Nikolay Borisov wrote:
>>
>>
>> On 11.04.19 г. 7:53 ч., Qu Wenruo wrote:
>>>
>>>
>>> On 2019/4/4 下午3:29, Anand Jain wrote:
>>>> From: Anand Jain <Anand.Jain@oracle.com>
>>>>
>>>> The cli 'btrfs inspect dump-tree <dev>' will scan for the partner devices
>>>> if any by default.
>>>>
>>>> So as of now you can not inspect each mirrored device independently.
>>>>
>>>> This patch adds noscan option, which when used won't scan the system for
>>>> the partner devices, instead it just uses the devices provided in the
>>>> argument.
>>>>
>>>> For example:
>>>>    btrfs inspect dump-tree --noscan <dev> [<dev>..]
>>>
>>> So you can specify multiple devices, just like kernel "device=" mount
>>> option.
>>>
>>> Then I don't think --noscan is a good naming, while I don't have any
>>> good alternative, as the --degraded is no better.
>>
>> How about skipscan ?
> 
> I still prefer "--device=", no surprise, anyone who used "device=" mount
> option knows what it is.
> 
> The only problem is, when only using one device, it will be super stupid:
>    dump-tree --device /dev/sda /dev/sda
> 
> If this doesn't sound good, then --noscan is good enough AFAIK.

  It was difficult to pick a name for this. IMO --degraded is not a bad
  either, matches with the kernel mount -o degraded. Thanks for your
  thoughts.

Thanks, Anand


> Thanks,
> Qu
> 
>>
>>>
>>> My idea is "(--device=<dev1>,<dev2>)|(<dev>)", but that looks more awful.
>>>
>>> Despite of that, the patch looks pretty good.
>>>
>>>>
>>>> This helps to debug degraded raid1 and raid10.
>>>>
>>>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>>>> ---
>>>> v4->v5: nit: use %m to print error string.
>>>> 	changelog update.
>>>> v3->v4: change the patch title.
>>>> 	collapse scan_args() to its only parent cmd_inspect_dump_tree()
>>>> 	(it was bit confusing).
>>>
>>> I don't this is the correct direction.
>>>
>>> You're introducing "--device=" btrfs-progs equivalent, there is no way
>>> it will only be utilized by dump-tree.
>>>
>>> Btrfs-check, btrfs-restore can all take advantage of this feature.
>>>
>>> You don't need to resend, later code can export them when needed.
>>> But doing it at the first place really helps.
>>>
>>> Thanks,
>>> Qu
>>>
>>>> 	update the change log.
>>>> 	update usage.
>>>> 	update man page.
>>>> v2->v3: make it scalable for more than two disks in noscan mode
>>>> v1->v2: rename --degraded to --noscan
>>>>
>>>>   Documentation/btrfs-inspect-internal.asciidoc |  5 ++-
>>>>   cmds-inspect-dump-tree.c                      | 53 +++++++++++++++++++++------
>>>>   2 files changed, 45 insertions(+), 13 deletions(-)
>>>>
>>>> diff --git a/Documentation/btrfs-inspect-internal.asciidoc b/Documentation/btrfs-inspect-internal.asciidoc
>>>> index 381497d284b8..f9d7f1c58f00 100644
>>>> --- a/Documentation/btrfs-inspect-internal.asciidoc
>>>> +++ b/Documentation/btrfs-inspect-internal.asciidoc
>>>> @@ -61,7 +61,7 @@ specify which mirror to print, valid values are 0, 1 and 2 and the superblock
>>>>   must be present on the device with a valid signature, can be used together with
>>>>   '--force'
>>>>
>>>> -*dump-tree* [options] <device>::
>>>> +*dump-tree* [options] <device> [device...]::
>>>>   (replaces the standalone tool *btrfs-debug-tree*)
>>>>   +
>>>>   Dump tree structures from a given device in textual form, expand keys to human
>>>> @@ -95,6 +95,9 @@ intermixed in the output
>>>>   --bfs::::
>>>>   use breadth-first search to print trees. the nodes are printed before all
>>>>   leaves
>>>> +--noscan::::
>>>> +do not scan the system for other partner device(s), only use the device(s)
>>>> +provided in the argument
>>>>   -t <tree_id>::::
>>>>   print only the tree with the specified ID, where the ID can be numerical or
>>>>   common name in a flexible human readable form
>>>> diff --git a/cmds-inspect-dump-tree.c b/cmds-inspect-dump-tree.c
>>>> index ad5345b4f1db..37d9b29fa824 100644
>>>> --- a/cmds-inspect-dump-tree.c
>>>> +++ b/cmds-inspect-dump-tree.c
>>>> @@ -21,6 +21,7 @@
>>>>   #include <unistd.h>
>>>>   #include <uuid/uuid.h>
>>>>   #include <getopt.h>
>>>> +#include <fcntl.h>
>>>>
>>>>   #include "kerncompat.h"
>>>>   #include "radix-tree.h"
>>>> @@ -185,7 +186,7 @@ static u64 treeid_from_string(const char *str, const char **end)
>>>>   }
>>>>
>>>>   const char * const cmd_inspect_dump_tree_usage[] = {
>>>> -	"btrfs inspect-internal dump-tree [options] device",
>>>> +	"btrfs inspect-internal dump-tree [options] <device> [<device> ..]",
>>>>   	"Dump tree structures from a given device",
>>>>   	"Dump tree structures from a given device in textual form, expand keys to human",
>>>>   	"readable equivalents where possible.",
>>>> @@ -200,6 +201,7 @@ const char * const cmd_inspect_dump_tree_usage[] = {
>>>>   	"-b|--block <block_num> print info from the specified block only",
>>>>   	"-t|--tree <tree_id>    print only tree with the given id (string or number)",
>>>>   	"--follow               use with -b, to show all children tree blocks of <block_num>",
>>>> +	"--noscan               do not scan for the partner device(s)",
>>>>   	NULL
>>>>   };
>>>>
>>>> @@ -214,7 +216,7 @@ int cmd_inspect_dump_tree(int argc, char **argv)
>>>>   	struct btrfs_disk_key disk_key;
>>>>   	struct btrfs_key found_key;
>>>>   	char uuidbuf[BTRFS_UUID_UNPARSED_SIZE];
>>>> -	int ret;
>>>> +	int ret = 0;
>>>>   	int slot;
>>>>   	int extent_only = 0;
>>>>   	int device_only = 0;
>>>> @@ -222,6 +224,7 @@ int cmd_inspect_dump_tree(int argc, char **argv)
>>>>   	int roots_only = 0;
>>>>   	int root_backups = 0;
>>>>   	int traverse = BTRFS_PRINT_TREE_DEFAULT;
>>>> +	int dev_optind;
>>>>   	unsigned open_ctree_flags;
>>>>   	u64 block_only = 0;
>>>>   	struct btrfs_root *tree_root_scan;
>>>> @@ -239,8 +242,8 @@ int cmd_inspect_dump_tree(int argc, char **argv)
>>>>   	optind = 0;
>>>>   	while (1) {
>>>>   		int c;
>>>> -		enum { GETOPT_VAL_FOLLOW = 256, GETOPT_VAL_DFS,
>>>> -		       GETOPT_VAL_BFS };
>>>> +		enum { GETOPT_VAL_FOLLOW = 256, GETOPT_VAL_DFS, GETOPT_VAL_BFS,
>>>> +		       GETOPT_VAL_NOSCAN};
>>>>   		static const struct option long_options[] = {
>>>>   			{ "extents", no_argument, NULL, 'e'},
>>>>   			{ "device", no_argument, NULL, 'd'},
>>>> @@ -252,6 +255,7 @@ int cmd_inspect_dump_tree(int argc, char **argv)
>>>>   			{ "follow", no_argument, NULL, GETOPT_VAL_FOLLOW },
>>>>   			{ "bfs", no_argument, NULL, GETOPT_VAL_BFS },
>>>>   			{ "dfs", no_argument, NULL, GETOPT_VAL_DFS },
>>>> +			{ "noscan", no_argument, NULL, GETOPT_VAL_NOSCAN },
>>>>   			{ NULL, 0, NULL, 0 }
>>>>   		};
>>>>
>>>> @@ -313,24 +317,49 @@ int cmd_inspect_dump_tree(int argc, char **argv)
>>>>   		case GETOPT_VAL_BFS:
>>>>   			traverse = BTRFS_PRINT_TREE_BFS;
>>>>   			break;
>>>> +		case GETOPT_VAL_NOSCAN:
>>>> +			open_ctree_flags |= OPEN_CTREE_NO_DEVICES;
>>>> +			break;
>>>>   		default:
>>>>   			usage(cmd_inspect_dump_tree_usage);
>>>>   		}
>>>>   	}
>>>>
>>>> -	if (check_argc_exact(argc - optind, 1))
>>>> +	if (check_argc_min(argc - optind, 1))
>>>>   		usage(cmd_inspect_dump_tree_usage);
>>>>
>>>> -	ret = check_arg_type(argv[optind]);
>>>> -	if (ret != BTRFS_ARG_BLKDEV && ret != BTRFS_ARG_REG) {
>>>> +	dev_optind = optind;
>>>> +	while (dev_optind < argc) {
>>>> +		int fd;
>>>> +		struct btrfs_fs_devices *fs_devices;
>>>> +		u64 num_devices;
>>>> +
>>>> +		ret = check_arg_type(argv[optind]);
>>>> +		if (ret != BTRFS_ARG_BLKDEV && ret != BTRFS_ARG_REG) {
>>>> +			if (ret < 0) {
>>>> +				errno = -ret;
>>>> +				error("invalid argument %s: %m", argv[dev_optind]);
>>>> +			} else {
>>>> +				error("not a block device or regular file: %s",
>>>> +				       argv[dev_optind]);
>>>> +			}
>>>> +		}
>>>> +		fd = open(argv[dev_optind], O_RDONLY);
>>>> +		if (fd < 0) {
>>>> +			error("cannot open %s: %m", argv[dev_optind]);
>>>> +			return -EINVAL;
>>>> +		}
>>>> +		ret = btrfs_scan_one_device(fd, argv[dev_optind], &fs_devices,
>>>> +					    &num_devices,
>>>> +					    BTRFS_SUPER_INFO_OFFSET,
>>>> +					    SBREAD_DEFAULT);
>>>> +		close(fd);
>>>>   		if (ret < 0) {
>>>>   			errno = -ret;
>>>> -			error("invalid argument %s: %m", argv[optind]);
>>>> -		} else {
>>>> -			error("not a block device or regular file: %s",
>>>> -			      argv[optind]);
>>>> +			error("device scan %s: %m", argv[dev_optind]);
>>>> +			return ret;
>>>>   		}
>>>> -		goto out;
>>>> +		dev_optind++;
>>>>   	}
>>>>
>>>>   	printf("%s\n", PACKAGE_STRING);
>>>>
>>>
Anand Jain May 16, 2019, 2:10 a.m. UTC | #5
On 11/4/19 3:28 PM, Qu Wenruo wrote:
> 
> 
> On 2019/4/11 下午2:52, Nikolay Borisov wrote:
>>
>>
>> On 11.04.19 г. 7:53 ч., Qu Wenruo wrote:
>>>
>>>
>>> On 2019/4/4 下午3:29, Anand Jain wrote:
>>>> From: Anand Jain <Anand.Jain@oracle.com>
>>>>
>>>> The cli 'btrfs inspect dump-tree <dev>' will scan for the partner devices
>>>> if any by default.
>>>>
>>>> So as of now you can not inspect each mirrored device independently.
>>>>
>>>> This patch adds noscan option, which when used won't scan the system for
>>>> the partner devices, instead it just uses the devices provided in the
>>>> argument.
>>>>
>>>> For example:
>>>>    btrfs inspect dump-tree --noscan <dev> [<dev>..]
>>>
>>> So you can specify multiple devices, just like kernel "device=" mount
>>> option.
>>>
>>> Then I don't think --noscan is a good naming, while I don't have any
>>> good alternative, as the --degraded is no better.
>>
>> How about skipscan ?
> 
> I still prefer "--device=", no surprise, anyone who used "device=" mount
> option knows what it is.

  --device is already taken.

usage: btrfs inspect-internal dump-tree [options] <device> [<device> ..]
::
     -d|--device            print only device info: tree root, chunk and 
device trees


> The only problem is, when only using one device, it will be super stupid:
>    dump-tree --device /dev/sda /dev/sda
> 
> If this doesn't sound good, then --noscan is good enough AFAIK.

   IMO --noscan OR --degraded is good.

Thanks, Anand

Patch
diff mbox series

diff --git a/Documentation/btrfs-inspect-internal.asciidoc b/Documentation/btrfs-inspect-internal.asciidoc
index 381497d284b8..f9d7f1c58f00 100644
--- a/Documentation/btrfs-inspect-internal.asciidoc
+++ b/Documentation/btrfs-inspect-internal.asciidoc
@@ -61,7 +61,7 @@  specify which mirror to print, valid values are 0, 1 and 2 and the superblock
 must be present on the device with a valid signature, can be used together with
 '--force'
 
-*dump-tree* [options] <device>::
+*dump-tree* [options] <device> [device...]::
 (replaces the standalone tool *btrfs-debug-tree*)
 +
 Dump tree structures from a given device in textual form, expand keys to human
@@ -95,6 +95,9 @@  intermixed in the output
 --bfs::::
 use breadth-first search to print trees. the nodes are printed before all
 leaves
+--noscan::::
+do not scan the system for other partner device(s), only use the device(s)
+provided in the argument
 -t <tree_id>::::
 print only the tree with the specified ID, where the ID can be numerical or
 common name in a flexible human readable form
diff --git a/cmds-inspect-dump-tree.c b/cmds-inspect-dump-tree.c
index ad5345b4f1db..37d9b29fa824 100644
--- a/cmds-inspect-dump-tree.c
+++ b/cmds-inspect-dump-tree.c
@@ -21,6 +21,7 @@ 
 #include <unistd.h>
 #include <uuid/uuid.h>
 #include <getopt.h>
+#include <fcntl.h>
 
 #include "kerncompat.h"
 #include "radix-tree.h"
@@ -185,7 +186,7 @@  static u64 treeid_from_string(const char *str, const char **end)
 }
 
 const char * const cmd_inspect_dump_tree_usage[] = {
-	"btrfs inspect-internal dump-tree [options] device",
+	"btrfs inspect-internal dump-tree [options] <device> [<device> ..]",
 	"Dump tree structures from a given device",
 	"Dump tree structures from a given device in textual form, expand keys to human",
 	"readable equivalents where possible.",
@@ -200,6 +201,7 @@  const char * const cmd_inspect_dump_tree_usage[] = {
 	"-b|--block <block_num> print info from the specified block only",
 	"-t|--tree <tree_id>    print only tree with the given id (string or number)",
 	"--follow               use with -b, to show all children tree blocks of <block_num>",
+	"--noscan               do not scan for the partner device(s)",
 	NULL
 };
 
@@ -214,7 +216,7 @@  int cmd_inspect_dump_tree(int argc, char **argv)
 	struct btrfs_disk_key disk_key;
 	struct btrfs_key found_key;
 	char uuidbuf[BTRFS_UUID_UNPARSED_SIZE];
-	int ret;
+	int ret = 0;
 	int slot;
 	int extent_only = 0;
 	int device_only = 0;
@@ -222,6 +224,7 @@  int cmd_inspect_dump_tree(int argc, char **argv)
 	int roots_only = 0;
 	int root_backups = 0;
 	int traverse = BTRFS_PRINT_TREE_DEFAULT;
+	int dev_optind;
 	unsigned open_ctree_flags;
 	u64 block_only = 0;
 	struct btrfs_root *tree_root_scan;
@@ -239,8 +242,8 @@  int cmd_inspect_dump_tree(int argc, char **argv)
 	optind = 0;
 	while (1) {
 		int c;
-		enum { GETOPT_VAL_FOLLOW = 256, GETOPT_VAL_DFS,
-		       GETOPT_VAL_BFS };
+		enum { GETOPT_VAL_FOLLOW = 256, GETOPT_VAL_DFS, GETOPT_VAL_BFS,
+		       GETOPT_VAL_NOSCAN};
 		static const struct option long_options[] = {
 			{ "extents", no_argument, NULL, 'e'},
 			{ "device", no_argument, NULL, 'd'},
@@ -252,6 +255,7 @@  int cmd_inspect_dump_tree(int argc, char **argv)
 			{ "follow", no_argument, NULL, GETOPT_VAL_FOLLOW },
 			{ "bfs", no_argument, NULL, GETOPT_VAL_BFS },
 			{ "dfs", no_argument, NULL, GETOPT_VAL_DFS },
+			{ "noscan", no_argument, NULL, GETOPT_VAL_NOSCAN },
 			{ NULL, 0, NULL, 0 }
 		};
 
@@ -313,24 +317,49 @@  int cmd_inspect_dump_tree(int argc, char **argv)
 		case GETOPT_VAL_BFS:
 			traverse = BTRFS_PRINT_TREE_BFS;
 			break;
+		case GETOPT_VAL_NOSCAN:
+			open_ctree_flags |= OPEN_CTREE_NO_DEVICES;
+			break;
 		default:
 			usage(cmd_inspect_dump_tree_usage);
 		}
 	}
 
-	if (check_argc_exact(argc - optind, 1))
+	if (check_argc_min(argc - optind, 1))
 		usage(cmd_inspect_dump_tree_usage);
 
-	ret = check_arg_type(argv[optind]);
-	if (ret != BTRFS_ARG_BLKDEV && ret != BTRFS_ARG_REG) {
+	dev_optind = optind;
+	while (dev_optind < argc) {
+		int fd;
+		struct btrfs_fs_devices *fs_devices;
+		u64 num_devices;
+
+		ret = check_arg_type(argv[optind]);
+		if (ret != BTRFS_ARG_BLKDEV && ret != BTRFS_ARG_REG) {
+			if (ret < 0) {
+				errno = -ret;
+				error("invalid argument %s: %m", argv[dev_optind]);
+			} else {
+				error("not a block device or regular file: %s",
+				       argv[dev_optind]);
+			}
+		}
+		fd = open(argv[dev_optind], O_RDONLY);
+		if (fd < 0) {
+			error("cannot open %s: %m", argv[dev_optind]);
+			return -EINVAL;
+		}
+		ret = btrfs_scan_one_device(fd, argv[dev_optind], &fs_devices,
+					    &num_devices,
+					    BTRFS_SUPER_INFO_OFFSET,
+					    SBREAD_DEFAULT);
+		close(fd);
 		if (ret < 0) {
 			errno = -ret;
-			error("invalid argument %s: %m", argv[optind]);
-		} else {
-			error("not a block device or regular file: %s",
-			      argv[optind]);
+			error("device scan %s: %m", argv[dev_optind]);
+			return ret;
 		}
-		goto out;
+		dev_optind++;
 	}
 
 	printf("%s\n", PACKAGE_STRING);