[PATCH REBASED 0/1] btrfs-progs: dump-tree: allow -b multiple times

[PATCH REBASED 0/1] btrfs-progs: dump-tree: allow -b multiple times

[Qu Wenruo]

Although just one patch, it needs the extent buffer cleanup code as
basis, so please fetch it from my github repo:
https://github.com/adam900710/btrfs-progs/tree/dump_tree_multi_blocks

It's based on David's devel branch, with HEAD as:
commit ffc7e4f730bd5ec49ba14ae3a13576311d57d4a6 (david/devel)
Author: Axel Burri <axel@tty0.ch>
Date:   Tue May 15 17:22:14 2018 +0200

    btrfs-progs: fix regression preventing send -p with subvolumes mounted on "/"


This patch allow -b to be specified multiple times, and add extra basic
check for them.
For later enhancement (Issue: #113) it needs extra work to handle
special roots.


Qu Wenruo (1):
  btrfs-progs: inspect-dump-tree: Allow '-b|--block' to be specified
    multiple times

 Documentation/btrfs-inspect-internal.asciidoc |   2 +-
 cmds-inspect-dump-tree.c                      | 109 +++++++++++++++---
 2 files changed, 91 insertions(+), 20 deletions(-)

-- 
2.17.1

[PATCH REBASED 1/1] btrfs-progs: inspect-dump-tree: Allow '-b|--block' to be specified multiple times

[Qu Wenruo]

Reuse extent-cache facility to record multiple bytenr so '-b|--block'
can be specified multiple times.

Despite that, add a sector size alignment check before we try to print a
tree block.
(Please note that, nodesize alignment check is not suitable here as meta
chunk start bytenr could be unaligned to nodesize)

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 Documentation/btrfs-inspect-internal.asciidoc |   2 +-
 cmds-inspect-dump-tree.c                      | 109 +++++++++++++++---
 2 files changed, 91 insertions(+), 20 deletions(-)

diff --git a/Documentation/btrfs-inspect-internal.asciidoc b/Documentation/btrfs-inspect-internal.asciidoc
index e2db64660b9a..ba8529f57660 100644
--- a/Documentation/btrfs-inspect-internal.asciidoc
+++ b/Documentation/btrfs-inspect-internal.asciidoc
@@ -86,7 +86,7 @@ the respective tree root block offset
 -u|--uuid::::
 print only the uuid tree information, empty output if the tree does not exist
 -b <block_num>::::
-print info of the specified block only
+print info of the specified block only, can be specified multiple times.
 --follow::::
 use with '-b', print all children tree blocks of '<block_num>'
 -t <tree_id>::::
diff --git a/cmds-inspect-dump-tree.c b/cmds-inspect-dump-tree.c
index 92a2a45b267e..abb9a2bcc7cb 100644
--- a/cmds-inspect-dump-tree.c
+++ b/cmds-inspect-dump-tree.c
@@ -198,11 +198,92 @@ const char * const cmd_inspect_dump_tree_usage[] = {
 	"-R|--backups           same as --roots plus print backup root info",
 	"-u|--uuid              print only the uuid tree",
 	"-b|--block <block_num> print info from the specified block only",
+	"                       can be specified multile times",
 	"-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>",
 	NULL
 };
 
+/*
+ * Helper function to record all tree block bytenr so we don't need to put
+ * all code into deep indent.
+ *
+ * Return >0 if we hit a duplicated bytenr (already recorded)
+ * Return 0 if nothing went wrong
+ * Return <0 if error happens (ENOMEM)
+ *
+ * For != 0 return value, all warning/error will be outputted by this function.
+ */
+static int dump_add_tree_block(struct cache_tree *tree, u64 bytenr)
+{
+	int ret;
+
+	/*
+	 * We don't really care about the size and we don't have
+	 * nodesize before we open the fs, so just use 1 as size here.
+	 */
+	ret = add_cache_extent(tree, bytenr, 1);
+	if (ret == -EEXIST) {
+		warning("tree block bytenr %llu is duplicated", bytenr);
+		return 1;
+	}
+	if (ret < 0) {
+		error("failed to record tree block bytenr %llu: %d(%s)",
+			bytenr, ret, strerror(-ret));
+		return ret;
+	}
+	return ret;
+}
+
+/*
+ * Print all tree blocks recorded.
+ * All tree block bytenr record will also be freed in this function.
+ *
+ * Return 0 if nothing wrong happened for *each* tree blocks
+ * Return <0 if anything wrong happened, and return value will be the last
+ * error.
+ */
+static int dump_print_tree_blocks(struct btrfs_fs_info *fs_info,
+				  struct cache_tree *tree, bool follow)
+{
+	struct cache_extent *ce;
+	struct extent_buffer *eb;
+	u64 bytenr;
+	int ret = 0;
+
+	ce = first_cache_extent(tree);
+	while (ce) {
+		bytenr = ce->start;
+
+		/*
+		 * Please note that here we can't check it against nodesize,
+		 * as it's possible a chunk is just aligned to sectorsize but
+		 * not aligned to nodesize.
+		 */
+		if (!IS_ALIGNED(bytenr, fs_info->sectorsize)) {
+			error(
+		"tree block bytenr %llu is not aligned to sectorsize %u",
+			      bytenr, fs_info->sectorsize);
+			ret = -EINVAL;
+			goto next;
+		}
+
+		eb = read_tree_block(fs_info, bytenr, 0);
+		if (!extent_buffer_uptodate(eb)) {
+			error("failed to read tree block %llu", bytenr);
+			ret = -EIO;
+			goto next;
+		}
+		btrfs_print_tree(eb, follow);
+		free_extent_buffer(eb);
+next:
+		remove_cache_extent(tree, ce);
+		free(ce);
+		ce = first_cache_extent(tree);
+	}
+	return ret;
+}
+
 int cmd_inspect_dump_tree(int argc, char **argv)
 {
 	struct btrfs_root *root;
@@ -213,6 +294,7 @@ int cmd_inspect_dump_tree(int argc, char **argv)
 	struct extent_buffer *leaf;
 	struct btrfs_disk_key disk_key;
 	struct btrfs_key found_key;
+	struct cache_tree block_root;	/* for multiple --block parameters */
 	char uuidbuf[BTRFS_UUID_UNPARSED_SIZE];
 	int ret;
 	int slot;
@@ -222,7 +304,7 @@ int cmd_inspect_dump_tree(int argc, char **argv)
 	int roots_only = 0;
 	int root_backups = 0;
 	unsigned open_ctree_flags;
-	u64 block_only = 0;
+	u64 block_bytenr;
 	struct btrfs_root *tree_root_scan;
 	u64 tree_id = 0;
 	bool follow = false;
@@ -235,6 +317,7 @@ int cmd_inspect_dump_tree(int argc, char **argv)
 	 * tree blocks as possible.
 	 */
 	open_ctree_flags = OPEN_CTREE_PARTIAL | OPEN_CTREE_NO_BLOCK_GROUPS;
+	cache_tree_init(&block_root);
 	while (1) {
 		int c;
 		enum { GETOPT_VAL_FOLLOW = 256 };
@@ -276,7 +359,10 @@ int cmd_inspect_dump_tree(int argc, char **argv)
 			 * other than chunk root
 			 */
 			open_ctree_flags |= __OPEN_CTREE_RETURN_CHUNK_ROOT;
-			block_only = arg_strtou64(optarg);
+			block_bytenr = arg_strtou64(optarg);
+			ret = dump_add_tree_block(&block_root, block_bytenr);
+			if (ret < 0)
+				goto out;
 			break;
 		case 't': {
 			const char *end = NULL;
@@ -324,24 +410,9 @@ int cmd_inspect_dump_tree(int argc, char **argv)
 		goto out;
 	}
 
-	if (block_only) {
+	if (!cache_tree_empty(&block_root)) {
 		root = info->chunk_root;
-		leaf = read_tree_block(info, block_only, 0);
-		if (extent_buffer_uptodate(leaf) &&
-		    btrfs_header_level(leaf) != 0) {
-			free_extent_buffer(leaf);
-			leaf = NULL;
-		}
-
-		if (!leaf)
-			leaf = read_tree_block(info, block_only, 0);
-		if (!extent_buffer_uptodate(leaf)) {
-			error("failed to read %llu",
-				(unsigned long long)block_only);
-			goto close_root;
-		}
-		btrfs_print_tree(leaf, follow);
-		free_extent_buffer(leaf);
+		ret = dump_print_tree_blocks(info, &block_root, follow);
 		goto close_root;
 	}
 
-- 
2.17.1

Re: [PATCH REBASED 1/1] btrfs-progs: inspect-dump-tree: Allow '-b|--block' to be specified multiple times

[Nikolay Borisov]

On  1.06.2018 08:15, Qu Wenruo wrote:
> Reuse extent-cache facility to record multiple bytenr so '-b|--block'
> can be specified multiple times.
> 
> Despite that, add a sector size alignment check before we try to print a
> tree block.
> (Please note that, nodesize alignment check is not suitable here as meta
> chunk start bytenr could be unaligned to nodesize)
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>

Just one minor misspell but otherwise LGTM:

Reviewed-by: Nikolay Borisov <nborisov@suse.com>

> ---
>  Documentation/btrfs-inspect-internal.asciidoc |   2 +-
>  cmds-inspect-dump-tree.c                      | 109 +++++++++++++++---
>  2 files changed, 91 insertions(+), 20 deletions(-)
> 
> diff --git a/Documentation/btrfs-inspect-internal.asciidoc b/Documentation/btrfs-inspect-internal.asciidoc
> index e2db64660b9a..ba8529f57660 100644
> --- a/Documentation/btrfs-inspect-internal.asciidoc
> +++ b/Documentation/btrfs-inspect-internal.asciidoc
> @@ -86,7 +86,7 @@ the respective tree root block offset
>  -u|--uuid::::
>  print only the uuid tree information, empty output if the tree does not exist
>  -b <block_num>::::
> -print info of the specified block only
> +print info of the specified block only, can be specified multiple times.
>  --follow::::
>  use with '-b', print all children tree blocks of '<block_num>'
>  -t <tree_id>::::
> diff --git a/cmds-inspect-dump-tree.c b/cmds-inspect-dump-tree.c
> index 92a2a45b267e..abb9a2bcc7cb 100644
> --- a/cmds-inspect-dump-tree.c
> +++ b/cmds-inspect-dump-tree.c
> @@ -198,11 +198,92 @@ const char * const cmd_inspect_dump_tree_usage[] = {
>  	"-R|--backups           same as --roots plus print backup root info",
>  	"-u|--uuid              print only the uuid tree",
>  	"-b|--block <block_num> print info from the specified block only",
> +	"                       can be specified multile times",
nit: s/multile/multiple

[PATCH v1.1] btrfs-progs: inspect-dump-tree: Allow '-b|--block' to be specified multiple times

[Qu Wenruo]

Reuse extent-cache facility to record multiple bytenr so '-b|--block'
can be specified multiple times.

Despite that, add a sector size alignment check before we try to print a
tree block.
(Please note that, nodesize alignment check is not suitable here as meta
chunk start bytenr could be unaligned to nodesize)

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 Documentation/btrfs-inspect-internal.asciidoc |   2 +-
 cmds-inspect-dump-tree.c                      | 109 +++++++++++++++---
 2 files changed, 91 insertions(+), 20 deletions(-)

diff --git a/Documentation/btrfs-inspect-internal.asciidoc b/Documentation/btrfs-inspect-internal.asciidoc
index e2db64660b9a..ba8529f57660 100644
--- a/Documentation/btrfs-inspect-internal.asciidoc
+++ b/Documentation/btrfs-inspect-internal.asciidoc
@@ -86,7 +86,7 @@ the respective tree root block offset
 -u|--uuid::::
 print only the uuid tree information, empty output if the tree does not exist
 -b <block_num>::::
-print info of the specified block only
+print info of the specified block only, can be specified multiple times.
 --follow::::
 use with '-b', print all children tree blocks of '<block_num>'
 -t <tree_id>::::
diff --git a/cmds-inspect-dump-tree.c b/cmds-inspect-dump-tree.c
index 92a2a45b267e..d67406141394 100644
--- a/cmds-inspect-dump-tree.c
+++ b/cmds-inspect-dump-tree.c
@@ -198,11 +198,92 @@ const char * const cmd_inspect_dump_tree_usage[] = {
 	"-R|--backups           same as --roots plus print backup root info",
 	"-u|--uuid              print only the uuid tree",
 	"-b|--block <block_num> print info from the specified block only",
+	"                       can be specified multiple times",
 	"-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>",
 	NULL
 };
 
+/*
+ * Helper function to record all tree block bytenr so we don't need to put
+ * all code into deep indent.
+ *
+ * Return >0 if we hit a duplicated bytenr (already recorded)
+ * Return 0 if nothing went wrong
+ * Return <0 if error happens (ENOMEM)
+ *
+ * For != 0 return value, all warning/error will be outputted by this function.
+ */
+static int dump_add_tree_block(struct cache_tree *tree, u64 bytenr)
+{
+	int ret;
+
+	/*
+	 * We don't really care about the size and we don't have
+	 * nodesize before we open the fs, so just use 1 as size here.
+	 */
+	ret = add_cache_extent(tree, bytenr, 1);
+	if (ret == -EEXIST) {
+		warning("tree block bytenr %llu is duplicated", bytenr);
+		return 1;
+	}
+	if (ret < 0) {
+		error("failed to record tree block bytenr %llu: %d(%s)",
+			bytenr, ret, strerror(-ret));
+		return ret;
+	}
+	return ret;
+}
+
+/*
+ * Print all tree blocks recorded.
+ * All tree block bytenr record will also be freed in this function.
+ *
+ * Return 0 if nothing wrong happened for *each* tree blocks
+ * Return <0 if anything wrong happened, and return value will be the last
+ * error.
+ */
+static int dump_print_tree_blocks(struct btrfs_fs_info *fs_info,
+				  struct cache_tree *tree, bool follow)
+{
+	struct cache_extent *ce;
+	struct extent_buffer *eb;
+	u64 bytenr;
+	int ret = 0;
+
+	ce = first_cache_extent(tree);
+	while (ce) {
+		bytenr = ce->start;
+
+		/*
+		 * Please note that here we can't check it against nodesize,
+		 * as it's possible a chunk is just aligned to sectorsize but
+		 * not aligned to nodesize.
+		 */
+		if (!IS_ALIGNED(bytenr, fs_info->sectorsize)) {
+			error(
+		"tree block bytenr %llu is not aligned to sectorsize %u",
+			      bytenr, fs_info->sectorsize);
+			ret = -EINVAL;
+			goto next;
+		}
+
+		eb = read_tree_block(fs_info, bytenr, 0);
+		if (!extent_buffer_uptodate(eb)) {
+			error("failed to read tree block %llu", bytenr);
+			ret = -EIO;
+			goto next;
+		}
+		btrfs_print_tree(eb, follow);
+		free_extent_buffer(eb);
+next:
+		remove_cache_extent(tree, ce);
+		free(ce);
+		ce = first_cache_extent(tree);
+	}
+	return ret;
+}
+
 int cmd_inspect_dump_tree(int argc, char **argv)
 {
 	struct btrfs_root *root;
@@ -213,6 +294,7 @@ int cmd_inspect_dump_tree(int argc, char **argv)
 	struct extent_buffer *leaf;
 	struct btrfs_disk_key disk_key;
 	struct btrfs_key found_key;
+	struct cache_tree block_root;	/* for multiple --block parameters */
 	char uuidbuf[BTRFS_UUID_UNPARSED_SIZE];
 	int ret;
 	int slot;
@@ -222,7 +304,7 @@ int cmd_inspect_dump_tree(int argc, char **argv)
 	int roots_only = 0;
 	int root_backups = 0;
 	unsigned open_ctree_flags;
-	u64 block_only = 0;
+	u64 block_bytenr;
 	struct btrfs_root *tree_root_scan;
 	u64 tree_id = 0;
 	bool follow = false;
@@ -235,6 +317,7 @@ int cmd_inspect_dump_tree(int argc, char **argv)
 	 * tree blocks as possible.
 	 */
 	open_ctree_flags = OPEN_CTREE_PARTIAL | OPEN_CTREE_NO_BLOCK_GROUPS;
+	cache_tree_init(&block_root);
 	while (1) {
 		int c;
 		enum { GETOPT_VAL_FOLLOW = 256 };
@@ -276,7 +359,10 @@ int cmd_inspect_dump_tree(int argc, char **argv)
 			 * other than chunk root
 			 */
 			open_ctree_flags |= __OPEN_CTREE_RETURN_CHUNK_ROOT;
-			block_only = arg_strtou64(optarg);
+			block_bytenr = arg_strtou64(optarg);
+			ret = dump_add_tree_block(&block_root, block_bytenr);
+			if (ret < 0)
+				goto out;
 			break;
 		case 't': {
 			const char *end = NULL;
@@ -324,24 +410,9 @@ int cmd_inspect_dump_tree(int argc, char **argv)
 		goto out;
 	}
 
-	if (block_only) {
+	if (!cache_tree_empty(&block_root)) {
 		root = info->chunk_root;
-		leaf = read_tree_block(info, block_only, 0);
-		if (extent_buffer_uptodate(leaf) &&
-		    btrfs_header_level(leaf) != 0) {
-			free_extent_buffer(leaf);
-			leaf = NULL;
-		}
-
-		if (!leaf)
-			leaf = read_tree_block(info, block_only, 0);
-		if (!extent_buffer_uptodate(leaf)) {
-			error("failed to read %llu",
-				(unsigned long long)block_only);
-			goto close_root;
-		}
-		btrfs_print_tree(leaf, follow);
-		free_extent_buffer(leaf);
+		ret = dump_print_tree_blocks(info, &block_root, follow);
 		goto close_root;
 	}
 
-- 
2.17.1

Re: [PATCH v1.1] btrfs-progs: inspect-dump-tree: Allow '-b|--block' to be specified multiple times

[David Sterba]

On Fri, Jun 01, 2018 at 04:27:46PM +0800, Qu Wenruo wrote:
> Reuse extent-cache facility to record multiple bytenr so '-b|--block'
> can be specified multiple times.
> 
> Despite that, add a sector size alignment check before we try to print a
> tree block.
> (Please note that, nodesize alignment check is not suitable here as meta
> chunk start bytenr could be unaligned to nodesize)
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>

I'm going to add this patch to devel, but there's a crash when there's
not valid block on the offset, eg. 'btrfs inspect dump-tree -b 8192
/dev/'

$ ./btrfs inspect dump-tree -b 1024 -b 2048 -b 4096 -b 8192 zimg
btrfs-progs v4.20.2 
ERROR: tree block bytenr 1024 is not aligned to sectorsize 4096
ERROR: tree block bytenr 2048 is not aligned to sectorsize 4096
Couldn't map the block 4096
Invalid mapping for 4096-20480, got 13631488-22020096
Couldn't map the block 4096
bad tree block 4096, bytenr mismatch, want=4096, have=0
ERROR: failed to read tree block 4096
extent_io.c:665: free_extent_buffer_internal: BUG_ON `eb->refs < 0`
triggered, value 1
./btrfs[0x426e57]
./btrfs(free_extent_buffer+0xe)[0x427701]
./btrfs(alloc_extent_buffer+0x3f)[0x427872]
./btrfs(btrfs_find_create_tree_block+0xf)[0x415b3c]
./btrfs(read_tree_block+0x5c)[0x4171b5]
./btrfs(cmd_inspect_dump_tree+0x587)[0x46fb75]
./btrfs(handle_command_group+0x44)[0x40df89]
./btrfs(cmd_inspect+0x15)[0x44b569]
./btrfs(main+0x8b)[0x40e032]
/lib64/libc.so.6(__libc_start_main+0xeb)[0x7f2001a54b7b]
./btrfs(_start+0x2a)[0x40dd1a]
Aborted (core dumped)

I think the fix will be simple so I'll fold it in, otherwise the
multiple block offsets work and using the cache is ok.