Re: [PATCH v2] btrfs-progs: inspect-internal: add option '-k u64,u8,u64' of dump-tree
On Mon, Jul 16, 2018 at 05:13:25PM +0800, Su Yue wrote:
> For big filesystems, there are many items in trees(extent tree
> specially).
> For example, to dump one extent, we usually dump extent tree then pipe
> result to grep. The time-consuming part is that dump tree traverses
> items. And it eats cpu and memory too.
>
> This patch introduces an option '-k u64,u8,u64' for users(developers more
> likely) to appoint a specific key to search and dump, then the path
> from root node down the leaf will be dumped.
> The search of the key costs most time of this way.
>
> Signed-off-by: Su Yue
> ---
> Change log:
> v2:
> Rename btrfs_search_spec_key() to btrfs_search_print_path().
> Rename is_key_spec to is_spec_key_set.
> Move btrfs_search_print_path() after open_ctree(), call it only once.
> Remove duplicate code and replaced with btrfs_print_tree().
> Modify the usage about '-k'.
> All are suggested by Qu Wenruo.
>
> cmds-inspect-dump-tree.c | 41 -
> print-tree.c | 77
> print-tree.h | 2 ++
> 3 files changed, 119 insertions(+), 1 deletion(-)
>
> diff --git a/cmds-inspect-dump-tree.c b/cmds-inspect-dump-tree.c
> index c8acd55a0c3a..80df826af0fd 100644
> --- a/cmds-inspect-dump-tree.c
> +++ b/cmds-inspect-dump-tree.c
> @@ -184,6 +184,21 @@ static u64 treeid_from_string(const char *str, const
> char **end)
> return id;
> }
>
> +static int parse_key(struct btrfs_key *key)
> +{
> +
Extra newline.
> + int ret = sscanf(optarg, "%llu,%hhu,%llu", >objectid,
> + >type, >offset);
Please don't do complex initializations in the declaration block.
And use of the global variable 'optarg' is not good, pass the scanned
string as a parameter.
> + if (ret != 3) {
> + error("error parsing key '%s'\n", optarg);
no "\n" at the end of the string in error(...)
> + ret = -EINVAL;
> + } else {
> + ret = 0;
> + }
> +
> + return ret;
> +}
> +
> const char * const cmd_inspect_dump_tree_usage[] = {
> "btrfs inspect-internal dump-tree [options] device",
> "Dump tree structures from a given device",
> @@ -199,6 +214,7 @@ const char * const cmd_inspect_dump_tree_usage[] = {
> "-u|--uuid print only the uuid tree",
> "-b|--block print info from the specified block only",
> "-t|--tree print only tree with the given id (string or
> number)",
> + "-k|--key search the specific key then print path, must
> with -t(conflicts with others)",
As there are more conflicting options, it would be better to group them
and just put a note about that. The message as you write it does not
provide clear usage instructions.
> "--follow use with -b, to show all children tree blocks
> of ",
> NULL
> };
> @@ -224,8 +240,10 @@ int cmd_inspect_dump_tree(int argc, char **argv)
> unsigned open_ctree_flags;
> u64 block_only = 0;
> struct btrfs_root *tree_root_scan;
> + struct btrfs_key spec_key = { 0 };
> u64 tree_id = 0;
> bool follow = false;
> + bool is_spec_key_set = false;
>
> /*
>* For debug-tree, we care nothing about extent tree (it's just backref
> @@ -248,10 +266,11 @@ int cmd_inspect_dump_tree(int argc, char **argv)
> { "block", required_argument, NULL, 'b'},
> { "tree", required_argument, NULL, 't'},
> { "follow", no_argument, NULL, GETOPT_VAL_FOLLOW },
> + { "key", required_argument, NULL, 'k'},
> { NULL, 0, NULL, 0 }
> };
>
> - c = getopt_long(argc, argv, "deb:rRut:", long_options, NULL);
> + c = getopt_long(argc, argv, "deb:rRut:k:", long_options, NULL);
> if (c < 0)
> break;
> switch (c) {
> @@ -300,6 +319,12 @@ int cmd_inspect_dump_tree(int argc, char **argv)
> }
> break;
> }
> + case 'k':
> + ret = parse_key(_key);
> + if (ret)
> + exit(1);
> + is_spec_key_set = true;
What if the key is already set?
> + break;
> case GETOPT_VAL_FOLLOW:
> follow = true;
> break;
> @@ -308,6 +333,9 @@ int cmd_inspect_dump_tree(int argc, char **argv)
> }
> }
>
> + if (!tree_id && is_spec_key_set)
> + usage(cmd_inspect_dump_tree_usage);
> +
> if (check_argc_exact(argc - optind, 1))
> usage(cmd_inspect_dump_tree_usage);
>
> @@ -325,6 +353,17 @@ int cmd_inspect_dump_tree(int argc, char **argv)
> goto out;
> }
>
> + if (is_spec_key_set) {
> + root = info->tree_root;
> + if (IS_ERR(root) || !root) {
Re: [PATCH v2] btrfs-progs: inspect-internal: add option '-k u64,u8,u64' of dump-tree
On 2018年07月16日 17:13, Su Yue wrote:
> For big filesystems, there are many items in trees(extent tree
> specially).
> For example, to dump one extent, we usually dump extent tree then pipe
> result to grep. The time-consuming part is that dump tree traverses
> items. And it eats cpu and memory too.
>
> This patch introduces an option '-k u64,u8,u64' for users(developers more
> likely) to appoint a specific key to search and dump, then the path
> from root node down the leaf will be dumped.
> The search of the key costs most time of this way.
>
> Signed-off-by: Su Yue
Looks pretty good, just small nitpicks.
Basically you could add my reviewed-by tag:
Reviewed-by: Qu Wenruo
> ---
> Change log:
> v2:
> Rename btrfs_search_spec_key() to btrfs_search_print_path().
> Rename is_key_spec to is_spec_key_set.
> Move btrfs_search_print_path() after open_ctree(), call it only once.
> Remove duplicate code and replaced with btrfs_print_tree().
> Modify the usage about '-k'.
> All are suggested by Qu Wenruo.
>
> cmds-inspect-dump-tree.c | 41 -
> print-tree.c | 77
> print-tree.h | 2 ++
> 3 files changed, 119 insertions(+), 1 deletion(-)
>
> diff --git a/cmds-inspect-dump-tree.c b/cmds-inspect-dump-tree.c
> index c8acd55a0c3a..80df826af0fd 100644
> --- a/cmds-inspect-dump-tree.c
> +++ b/cmds-inspect-dump-tree.c
> @@ -184,6 +184,21 @@ static u64 treeid_from_string(const char *str, const
> char **end)
> return id;
> }
>
> +static int parse_key(struct btrfs_key *key)
> +{
> +
> + int ret = sscanf(optarg, "%llu,%hhu,%llu", >objectid,
> + >type, >offset);
> + if (ret != 3) {
> + error("error parsing key '%s'\n", optarg);
> + ret = -EINVAL;
> + } else {
> + ret = 0;
> + }
> +
> + return ret;
> +}
> +
> const char * const cmd_inspect_dump_tree_usage[] = {
> "btrfs inspect-internal dump-tree [options] device",
> "Dump tree structures from a given device",
> @@ -199,6 +214,7 @@ const char * const cmd_inspect_dump_tree_usage[] = {
> "-u|--uuid print only the uuid tree",
> "-b|--block print info from the specified block only",
> "-t|--tree print only tree with the given id (string or
> number)",
> + "-k|--key search the specific key then print path, must
> with -t(conflicts with others)",
What are the "others"?
At least for me, other includes -r and -b.
> "--follow use with -b, to show all children tree blocks
> of ",
> NULL
> };
> @@ -224,8 +240,10 @@ int cmd_inspect_dump_tree(int argc, char **argv)
> unsigned open_ctree_flags;
> u64 block_only = 0;
> struct btrfs_root *tree_root_scan;
> + struct btrfs_key spec_key = { 0 };
> u64 tree_id = 0;
> bool follow = false;
> + bool is_spec_key_set = false;
>
> /*
>* For debug-tree, we care nothing about extent tree (it's just backref
> @@ -248,10 +266,11 @@ int cmd_inspect_dump_tree(int argc, char **argv)
> { "block", required_argument, NULL, 'b'},
> { "tree", required_argument, NULL, 't'},
> { "follow", no_argument, NULL, GETOPT_VAL_FOLLOW },
> + { "key", required_argument, NULL, 'k'},
> { NULL, 0, NULL, 0 }
> };
>
> - c = getopt_long(argc, argv, "deb:rRut:", long_options, NULL);
> + c = getopt_long(argc, argv, "deb:rRut:k:", long_options, NULL);
> if (c < 0)
> break;
> switch (c) {
> @@ -300,6 +319,12 @@ int cmd_inspect_dump_tree(int argc, char **argv)
> }
> break;
> }
> + case 'k':
> + ret = parse_key(_key);
> + if (ret)
> + exit(1);
> + is_spec_key_set = true;
> + break;
> case GETOPT_VAL_FOLLOW:
> follow = true;
> break;
> @@ -308,6 +333,9 @@ int cmd_inspect_dump_tree(int argc, char **argv)
> }
> }
>
> + if (!tree_id && is_spec_key_set)
> + usage(cmd_inspect_dump_tree_usage);
> +
And options conflict should also be done here.
Thanks,
Qu
> if (check_argc_exact(argc - optind, 1))
> usage(cmd_inspect_dump_tree_usage);
>
> @@ -325,6 +353,17 @@ int cmd_inspect_dump_tree(int argc, char **argv)
> goto out;
> }
>
> + if (is_spec_key_set) {
> + root = info->tree_root;
> + if (IS_ERR(root) || !root) {
> + ret = root ? PTR_ERR(root) : -1;
> + error("unable to open %s", argv[optind]);
> + goto out;
> + }
> +
> +
[PATCH v2] btrfs-progs: inspect-internal: add option '-k u64,u8,u64' of dump-tree
For big filesystems, there are many items in trees(extent tree
specially).
For example, to dump one extent, we usually dump extent tree then pipe
result to grep. The time-consuming part is that dump tree traverses
items. And it eats cpu and memory too.
This patch introduces an option '-k u64,u8,u64' for users(developers more
likely) to appoint a specific key to search and dump, then the path
from root node down the leaf will be dumped.
The search of the key costs most time of this way.
Signed-off-by: Su Yue
---
Change log:
v2:
Rename btrfs_search_spec_key() to btrfs_search_print_path().
Rename is_key_spec to is_spec_key_set.
Move btrfs_search_print_path() after open_ctree(), call it only once.
Remove duplicate code and replaced with btrfs_print_tree().
Modify the usage about '-k'.
All are suggested by Qu Wenruo.
cmds-inspect-dump-tree.c | 41 -
print-tree.c | 77
print-tree.h | 2 ++
3 files changed, 119 insertions(+), 1 deletion(-)
diff --git a/cmds-inspect-dump-tree.c b/cmds-inspect-dump-tree.c
index c8acd55a0c3a..80df826af0fd 100644
--- a/cmds-inspect-dump-tree.c
+++ b/cmds-inspect-dump-tree.c
@@ -184,6 +184,21 @@ static u64 treeid_from_string(const char *str, const char
**end)
return id;
}
+static int parse_key(struct btrfs_key *key)
+{
+
+ int ret = sscanf(optarg, "%llu,%hhu,%llu", >objectid,
+>type, >offset);
+ if (ret != 3) {
+ error("error parsing key '%s'\n", optarg);
+ ret = -EINVAL;
+ } else {
+ ret = 0;
+ }
+
+ return ret;
+}
+
const char * const cmd_inspect_dump_tree_usage[] = {
"btrfs inspect-internal dump-tree [options] device",
"Dump tree structures from a given device",
@@ -199,6 +214,7 @@ const char * const cmd_inspect_dump_tree_usage[] = {
"-u|--uuid print only the uuid tree",
"-b|--block print info from the specified block only",
"-t|--tree print only tree with the given id (string or
number)",
+ "-k|--key search the specific key then print path, must
with -t(conflicts with others)",
"--follow use with -b, to show all children tree blocks
of ",
NULL
};
@@ -224,8 +240,10 @@ int cmd_inspect_dump_tree(int argc, char **argv)
unsigned open_ctree_flags;
u64 block_only = 0;
struct btrfs_root *tree_root_scan;
+ struct btrfs_key spec_key = { 0 };
u64 tree_id = 0;
bool follow = false;
+ bool is_spec_key_set = false;
/*
* For debug-tree, we care nothing about extent tree (it's just backref
@@ -248,10 +266,11 @@ int cmd_inspect_dump_tree(int argc, char **argv)
{ "block", required_argument, NULL, 'b'},
{ "tree", required_argument, NULL, 't'},
{ "follow", no_argument, NULL, GETOPT_VAL_FOLLOW },
+ { "key", required_argument, NULL, 'k'},
{ NULL, 0, NULL, 0 }
};
- c = getopt_long(argc, argv, "deb:rRut:", long_options, NULL);
+ c = getopt_long(argc, argv, "deb:rRut:k:", long_options, NULL);
if (c < 0)
break;
switch (c) {
@@ -300,6 +319,12 @@ int cmd_inspect_dump_tree(int argc, char **argv)
}
break;
}
+ case 'k':
+ ret = parse_key(_key);
+ if (ret)
+ exit(1);
+ is_spec_key_set = true;
+ break;
case GETOPT_VAL_FOLLOW:
follow = true;
break;
@@ -308,6 +333,9 @@ int cmd_inspect_dump_tree(int argc, char **argv)
}
}
+ if (!tree_id && is_spec_key_set)
+ usage(cmd_inspect_dump_tree_usage);
+
if (check_argc_exact(argc - optind, 1))
usage(cmd_inspect_dump_tree_usage);
@@ -325,6 +353,17 @@ int cmd_inspect_dump_tree(int argc, char **argv)
goto out;
}
+ if (is_spec_key_set) {
+ root = info->tree_root;
+ if (IS_ERR(root) || !root) {
+ ret = root ? PTR_ERR(root) : -1;
+ error("unable to open %s", argv[optind]);
+ goto out;
+ }
+
+ btrfs_search_print_path(info, tree_id, _key);
+ goto close_root;
+ }
if (block_only) {
root = info->chunk_root;
leaf = read_tree_block(info, block_only, 0);
diff --git a/print-tree.c b/print-tree.c
index a09ecfbb28f0..3868e42d5d29 100644
--- a/print-tree.c
+++ b/print-tree.c
@@ -1459,3 +1459,80 @@ void btrfs_print_tree(struct extent_buffer *eb, int
