btrfs-progs: Add 2 new subcommands to inspect-internal

btrfs-progs: Add 2 new subcommands to inspect-internal

[Divya Indi]

These patches aim to add 2 new subcommands that:
-> provide information about block groups
-> help to decide whether balance can reduce the no. of data block groups and if
 it can, provides the block group object id for "-dvrange"

[PATCH 1/3] btrfs-progs: Generic functions to retrieve chunks and their bg info
[PATCH 2/3] btrfs-progs: Add a command to show bg info
[PATCH 3/3] btrfs-progs: Add command to check if balance op is req

Now, you can run:
-------------------------------------------------------------------------------
$ btrfs inspect-internal balance_check /mnt/btrfs
               Start                 Len                Used
            12582912             8.00MiB           128.00KiB
           136708096           208.00MiB            74.00MiB

Total data bgs: 2
Total free space: 141.88MiB
For min used bg 12582912 used = 128.00KiB free = 7.88MiB
run 'btrfs balance start -dvrange=12582912..12582913 <mountpoint>'

$ btrfs balance start -dvrange=12582912..12582913 /mnt/btrfs
Done, had to relocate 1 out of 4 chunks

$ btrfs inspect-internal balance_check /mnt/btrfs
               Start                 Len                Used
           136708096           208.00MiB            74.12MiB
Data block groups in fs = 1, no need to do balance.

$ btrfs inspect-internal bg_analysis /mnt/btrfs

                Type               Start                 Len                Used
              SYSTEM            20971520             8.00MiB            16.00KiB
            METADATA            29360128           102.38MiB           192.00KiB
                DATA           136708096           208.00MiB            74.12MiB
-------------------------------------------------------------------------------

Thanks to the suggestion by Hans van Kranenburg, and the python-btrfs module,
an efficient alternative to retrieving block groups could be used:
Now, it first looks at the chunk tree, and for every chunk listed it looks up an
exact match in the extent tree.

[PATCH 1/3] btrfs-progs: Generic functions to retrieve chunks and their bg info

[Divya Indi]

An efficient alternative to retrieving block groups:
get_chunks(): Walk the chunk tree to retrieve the chunks.
get_bg_info(): For each retrieved chunk, lookup an exact match of block
group in the extent tree.

Signed-off-by: Divya Indi <divya.indi@oracle.com>
Reviewed-by: Ashish Samant <ashish.samant@oracle.com>
Reviewed-by: Liu Bo <bo.li.liu@oracle.com>
---
 cmds-inspect.c |   66 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 66 insertions(+), 0 deletions(-)

diff --git a/cmds-inspect.c b/cmds-inspect.c
index 4b7cea0..f435ea9 100644
--- a/cmds-inspect.c
+++ b/cmds-inspect.c
@@ -81,6 +81,72 @@ out:
 	return !!ret;
 }
 
+static void bg_flags_to_str(u64 flags, char *ret)
+{
+	int empty = 1;
+
+	if (flags & BTRFS_BLOCK_GROUP_DATA) {
+		empty = 0;
+		strcpy(ret, "DATA");
+	}
+	if (flags & BTRFS_BLOCK_GROUP_METADATA) {
+		if (!empty)
+			strcat(ret, "|");
+		strcat(ret, "METADATA");
+	}
+	if (flags & BTRFS_BLOCK_GROUP_SYSTEM) {
+		if (!empty)
+			strcat(ret, "|");
+		strcat(ret, "SYSTEM");
+	}
+}
+
+/* Walking through the chunk tree to retrieve chunks. */
+
+static int get_chunks(int fd, struct btrfs_ioctl_search_args *chunk_args)
+{
+	struct btrfs_ioctl_search_key *sk;
+	int ret;
+	int e;
+
+	sk = &chunk_args->key;
+
+	sk->tree_id = BTRFS_CHUNK_TREE_OBJECTID;
+	sk->min_objectid = sk->max_objectid = BTRFS_FIRST_CHUNK_TREE_OBJECTID;
+	sk->max_type = sk->min_type = BTRFS_CHUNK_ITEM_KEY;
+	sk->nr_items = 4096;
+
+	ret = ioctl(fd, BTRFS_IOC_TREE_SEARCH, chunk_args);
+	e = errno;
+	if (ret < 0) {
+		fprintf(stderr, "ret %d error '%s'\n", ret,
+				strerror(e));
+	}
+	return ret;
+}
+
+/* Given the objectid, find the block group item in the extent tree */
+static int get_bg_info(int fd, struct btrfs_ioctl_search_args *bg_args,
+		       u64 objectid, unsigned long length)
+{
+	struct btrfs_ioctl_search_key *bg_sk;
+	int ret;
+	int e;
+
+	bg_sk = &bg_args->key;
+
+	bg_sk->min_objectid = bg_sk->max_objectid = objectid;
+	bg_sk->nr_items = 1;
+	bg_sk->min_offset = bg_sk->max_offset = length;
+
+	ret = ioctl(fd, BTRFS_IOC_TREE_SEARCH, bg_args);
+	e = errno;
+	if (ret < 0) {
+		fprintf(stderr, "ret %d error '%s'\n", ret,
+				strerror(e));
+	}
+	return ret;
+}
 static const char * const cmd_inspect_inode_resolve_usage[] = {
 	"btrfs inspect-internal inode-resolve [-v] <inode> <path>",
 	"Get file system paths for the given inode",
-- 
1.7.1

[PATCH 2/3] btrfs-progs: Add a command to show bg info

[Divya Indi]

Add a new subcommand to btrfs inspect-internal

btrfs inspect-internal bg_analysis <path>
Gives information about all the block groups.

Signed-off-by: Divya Indi <divya.indi@oracle.com>
Reviewed-by: Ashish Samant <ashish.samant@oracle.com>
Reviewed-by: Liu Bo <bo.li.liu@oracle.com>
---
 cmds-inspect.c |  114 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 114 insertions(+), 0 deletions(-)

diff --git a/cmds-inspect.c b/cmds-inspect.c
index f435ea9..0e2f15a 100644
--- a/cmds-inspect.c
+++ b/cmds-inspect.c
@@ -147,6 +147,118 @@ static int get_bg_info(int fd, struct btrfs_ioctl_search_args *bg_args,
 	}
 	return ret;
 }
+
+static const char * const cmd_inspect_bg_analysis_usage[] = {
+	"btrfs inspect-internal bg_analysis <path> ",
+	"Get information about all block groups",
+	"",
+	"",
+	NULL
+};
+
+static int cmd_inspect_bg_analysis(int argc, char **argv)
+{
+	struct btrfs_ioctl_search_args args;
+	struct btrfs_ioctl_search_args bg_args;
+	struct btrfs_ioctl_search_header *header;
+	struct btrfs_ioctl_search_header *bg_header;
+	struct btrfs_ioctl_search_key *sk;
+	struct btrfs_ioctl_search_key *bg_sk;
+	struct btrfs_block_group_item *bg;
+	struct btrfs_chunk *chunk;
+	unsigned long off = 0;
+	unsigned long bg_off = 0;
+	DIR *dirstream = NULL;
+	int fd;
+	int i;
+	int ret = 0;
+	u64 used;
+	u64 flags;
+	char bg_type[32] = {0};
+
+	if (check_argc_exact(argc, 2))
+		usage(cmd_inspect_bg_analysis_usage);
+
+	fd = btrfs_open_dir(argv[optind], &dirstream, 1);
+	if (fd < 0)
+		return 1;
+
+	memset(&args, 0, sizeof(args));
+	sk = &args.key;
+	sk->min_offset = sk->min_transid = 0;
+	sk->max_offset = sk->max_transid = (u64)-1;
+	printf("%20s%20s%20s%20s\n", "Type", "Start", "Len", "Used");
+	while (1) {
+
+		/* Walk through the chunk tree and retrieve all the chunks */
+		ret = get_chunks(fd, &args);
+		if (ret < 0)
+			goto out;
+
+		/*
+		 * it should not happen.
+		 */
+		if (sk->nr_items == 0)
+			break;
+
+		off = 0;
+		memset(&bg_args, 0, sizeof(bg_args));
+		bg_sk = &bg_args.key;
+
+		bg_sk->tree_id = BTRFS_EXTENT_TREE_OBJECTID;
+		bg_sk->min_type = BTRFS_BLOCK_GROUP_ITEM_KEY;
+		bg_sk->max_type = BTRFS_BLOCK_GROUP_ITEM_KEY;
+		bg_sk->min_transid =  0;
+		bg_sk->max_transid = (u64)-1;
+
+		for (i = 0; i < sk->nr_items; i++) {
+			header = (struct btrfs_ioctl_search_header *)(args.buf
+								      + off);
+			off += sizeof(*header);
+			if (header->type == BTRFS_CHUNK_ITEM_KEY) {
+				chunk = (struct btrfs_chunk *)
+					(args.buf + off);
+
+				/* For every chunk lookup an exact match(bg) in
+				 * the extent tree and read its used values */
+				ret = get_bg_info(fd, &bg_args, header->offset,
+						  chunk->length);
+				if (ret < 0)
+					goto out;
+
+				/*
+				 * it should not happen.
+				 */
+				if (bg_sk->nr_items == 0)
+					continue;
+
+				bg_off = 0;
+				bg_header = (struct btrfs_ioctl_search_header *)
+					    (bg_args.buf + bg_off);
+				bg_off += sizeof(*bg_header);
+				bg = (struct btrfs_block_group_item *)
+				     (bg_args.buf + bg_off);
+
+				used = btrfs_block_group_used(bg);
+				memset(&bg_type, 0, 32);
+				flags = btrfs_block_group_flags(bg);
+				bg_flags_to_str(flags, bg_type);
+				printf("%20s%20llu%20s%20s\n",
+					bg_type,
+					bg_header->objectid,
+					pretty_size(bg_header->offset),
+					pretty_size(used));
+			}
+			off += header->len;
+			sk->min_offset = header->offset + header->len;
+		}
+		sk->nr_items = 4096;
+	}
+out:
+	close_file_or_dir(fd, dirstream);
+	return ret;
+}
+
 static const char * const cmd_inspect_inode_resolve_usage[] = {
 	"btrfs inspect-internal inode-resolve [-v] <inode> <path>",
 	"Get file system paths for the given inode",
@@ -702,6 +814,8 @@ const struct cmd_group inspect_cmd_group = {
 			0 },
 		{ "min-dev-size", cmd_inspect_min_dev_size,
 			cmd_inspect_min_dev_size_usage, NULL, 0 },
+		{ "bg_analysis", cmd_inspect_bg_analysis,
+			cmd_inspect_bg_analysis_usage, NULL, 0 },
 		{ "dump-tree", cmd_inspect_dump_tree,
 				cmd_inspect_dump_tree_usage, NULL, 0 },
 		{ "dump-super", cmd_inspect_dump_super,
-- 
1.7.1

[PATCH 3/3] btrfs-progs: Add command to check if balance op is req

[Divya Indi]

Add new subcommand to btrfs inspect-internal

btrfs inspect-internal balance_check <path>
Checks whether 'btrfs balance' can help creating more space (Only
considers data block groups).

Signed-off-by: Divya Indi <divya.indi@oracle.com>
Reviewed-by: Ashish Samant <ashish.samant@oracle.com>
Reviewed-by: Liu Bo <bo.li.liu@oracle.com>
---
 cmds-inspect.c |  147 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 147 insertions(+), 0 deletions(-)

diff --git a/cmds-inspect.c b/cmds-inspect.c
index 0e2f15a..5baaa49 100644
--- a/cmds-inspect.c
+++ b/cmds-inspect.c
@@ -267,6 +267,151 @@ static const char * const cmd_inspect_inode_resolve_usage[] = {
 	NULL
 };
 
+static const char * const cmd_inspect_balance_check_usage[] = {
+	"btrfs inspect-internal balance_check <path>",
+	"To check whether 'btrfs balance' can help creating more space",
+	"",
+	"",
+	NULL
+};
+
+static int cmd_inspect_balance_check(int argc, char **argv)
+{
+	struct btrfs_ioctl_search_args args;
+	struct btrfs_ioctl_search_args bg_args;
+	struct btrfs_ioctl_search_key *sk;
+	struct btrfs_ioctl_search_key *bg_sk;
+	struct btrfs_ioctl_search_header *header;
+	struct btrfs_ioctl_search_header *bg_header;
+	struct btrfs_block_group_item *bg;
+	struct btrfs_chunk *chunk;
+	unsigned long off = 0;
+	unsigned long bg_off = 0;
+	DIR *dirstream = NULL;
+	int fd;
+	int i;
+	u64 total_free = 0;
+	u64 min_used = (u64)-1;
+	u64 free_of_min_used = 0;
+	u64 bg_of_min_used = 0;
+	u64 flags;
+	u64 used;
+	int ret = 0;
+	int nr_data_bgs = 0;
+
+	if (check_argc_exact(argc, 2))
+		usage(cmd_inspect_balance_check_usage);
+
+	fd = btrfs_open_dir(argv[optind], &dirstream, 1);
+	if (fd < 0)
+		return 1;
+
+	memset(&args, 0, sizeof(args));
+	sk = &args.key;
+	sk->min_offset = sk->min_transid = 0;
+	sk->max_offset = sk->max_transid = (u64)-1;
+
+	printf("%20s%20s%20s\n", "Start", "Len", "Used");
+	while (1) {
+		ret = get_chunks(fd, &args);
+		if (ret < 0)
+			goto out;
+
+		/*
+		 * it should not happen.
+		 */
+		if (sk->nr_items == 0)
+			break;
+
+		off = 0;
+		memset(&bg_args, 0, sizeof(bg_args));
+		bg_sk = &bg_args.key;
+
+		/* For every chunk, look up 1 exact match for block group in
+		 * the extent tree. */
+		bg_sk->tree_id = BTRFS_EXTENT_TREE_OBJECTID;
+		bg_sk->min_type = BTRFS_BLOCK_GROUP_ITEM_KEY;
+		bg_sk->max_type = BTRFS_BLOCK_GROUP_ITEM_KEY;
+		bg_sk->min_transid =  0;
+		bg_sk->max_transid = (u64)-1;
+
+		for (i = 0; i < sk->nr_items; i++) {
+			header = (struct btrfs_ioctl_search_header *)(args.buf
+								      + off);
+			off += sizeof(*header);
+			if (header->type == BTRFS_CHUNK_ITEM_KEY) {
+				chunk = (struct btrfs_chunk *)
+					(args.buf + off);
+				ret = get_bg_info(fd, &bg_args, header->offset,
+						  chunk->length);
+				if (ret < 0)
+					goto out;
+
+				/*
+				 * it should not happen.
+				 */
+				if (bg_sk->nr_items == 0)
+					continue;
+
+				bg_off = 0;
+				bg_header = (struct btrfs_ioctl_search_header *)
+					    (bg_args.buf + bg_off);
+				bg_off += sizeof(*bg_header);
+				bg = (struct btrfs_block_group_item *)
+				     (bg_args.buf + bg_off);
+
+				flags = btrfs_block_group_flags(bg);
+				if (flags & BTRFS_BLOCK_GROUP_DATA) {
+					used = btrfs_block_group_used(bg);
+					nr_data_bgs++;
+					printf("%20llu%20s%20s\n",
+						bg_header->objectid,
+						pretty_size(bg_header->offset),
+						pretty_size(used));
+					total_free += bg_header->offset - used;
+					if (min_used >= used) {
+						min_used = used;
+						free_of_min_used =
+							bg_header->offset - used;
+						bg_of_min_used =
+							bg_header->objectid;
+					}
+				}
+			}
+
+			off += header->len;
+			sk->min_offset = header->offset + header->len;
+		}
+		sk->nr_items = 4096;
+
+	}
+
+	if (nr_data_bgs <= 1) {
+		printf("Data block groups in fs = %d, no need to do balance.\n",
+				nr_data_bgs);
+		ret = 1;
+		goto out;
+	}
+
+	printf("Total data bgs: %d\nTotal free space: %s\n"
+	       "For min used bg %llu used = %s free = %s\n",
+		nr_data_bgs, pretty_size(total_free), bg_of_min_used,
+		pretty_size(min_used), pretty_size(free_of_min_used));
+
+	if (total_free - free_of_min_used > min_used) {
+		printf("run 'btrfs balance start -dvrange=%llu..%llu <mountpoint>'\n",
+				bg_of_min_used, bg_of_min_used + 1);
+		ret = 0;
+	} else {
+		printf("Please don't balance data block groups, no free space\n");
+		ret = 1;
+	}
+
+out:
+	close_file_or_dir(fd, dirstream);
+	return ret;
+}
+
 static int cmd_inspect_inode_resolve(int argc, char **argv)
 {
 	int fd;
@@ -816,6 +961,8 @@ const struct cmd_group inspect_cmd_group = {
 			cmd_inspect_min_dev_size_usage, NULL, 0 },
 		{ "bg_analysis", cmd_inspect_bg_analysis,
 			cmd_inspect_bg_analysis_usage, NULL, 0 },
+		{ "balance_check", cmd_inspect_balance_check,
+			cmd_inspect_balance_check_usage, NULL, 0 },
 		{ "dump-tree", cmd_inspect_dump_tree,
 				cmd_inspect_dump_tree_usage, NULL, 0 },
 		{ "dump-super", cmd_inspect_dump_super,
-- 
1.7.1

Re: [PATCH 1/3] btrfs-progs: Generic functions to retrieve chunks and their bg info

[Qu Wenruo]

At 10/18/2016 08:35 AM, Divya Indi wrote:
> An efficient alternative to retrieving block groups:
> get_chunks(): Walk the chunk tree to retrieve the chunks.
> get_bg_info(): For each retrieved chunk, lookup an exact match of block
> group in the extent tree.
>
> Signed-off-by: Divya Indi <divya.indi@oracle.com>
> Reviewed-by: Ashish Samant <ashish.samant@oracle.com>
> Reviewed-by: Liu Bo <bo.li.liu@oracle.com>
> ---
>  cmds-inspect.c |   66 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 66 insertions(+), 0 deletions(-)
>
> diff --git a/cmds-inspect.c b/cmds-inspect.c
> index 4b7cea0..f435ea9 100644
> --- a/cmds-inspect.c
> +++ b/cmds-inspect.c
> @@ -81,6 +81,72 @@ out:
>  	return !!ret;
>  }
>
> +static void bg_flags_to_str(u64 flags, char *ret)
> +{
> +	int empty = 1;
> +
> +	if (flags & BTRFS_BLOCK_GROUP_DATA) {
> +		empty = 0;
> +		strcpy(ret, "DATA");
> +	}
> +	if (flags & BTRFS_BLOCK_GROUP_METADATA) {
> +		if (!empty)
> +			strcat(ret, "|");
> +		strcat(ret, "METADATA");
> +	}
> +	if (flags & BTRFS_BLOCK_GROUP_SYSTEM) {
> +		if (!empty)
> +			strcat(ret, "|");
> +		strcat(ret, "SYSTEM");
> +	}
> +}

Check print-tree.c, it has the same function.
Just export it.

And it's stronger than your version, which can also output profiles.

> +
> +/* Walking through the chunk tree to retrieve chunks. */
> +
> +static int get_chunks(int fd, struct btrfs_ioctl_search_args *chunk_args)
> +{
> +	struct btrfs_ioctl_search_key *sk;
> +	int ret;
> +	int e;
> +
> +	sk = &chunk_args->key;
> +
> +	sk->tree_id = BTRFS_CHUNK_TREE_OBJECTID;
> +	sk->min_objectid = sk->max_objectid = BTRFS_FIRST_CHUNK_TREE_OBJECTID;
> +	sk->max_type = sk->min_type = BTRFS_CHUNK_ITEM_KEY;
> +	sk->nr_items = 4096;
> +
> +	ret = ioctl(fd, BTRFS_IOC_TREE_SEARCH, chunk_args);
> +	e = errno;
> +	if (ret < 0) {
> +		fprintf(stderr, "ret %d error '%s'\n", ret,
> +				strerror(e));

Use error() function.

> +	}
> +	return ret;
> +}
> +
> +/* Given the objectid, find the block group item in the extent tree */
> +static int get_bg_info(int fd, struct btrfs_ioctl_search_args *bg_args,
> +		       u64 objectid, unsigned long length)
> +{
> +	struct btrfs_ioctl_search_key *bg_sk;
> +	int ret;
> +	int e;
> +
> +	bg_sk = &bg_args->key;
> +
> +	bg_sk->min_objectid = bg_sk->max_objectid = objectid;
> +	bg_sk->nr_items = 1;
> +	bg_sk->min_offset = bg_sk->max_offset = length;
> +
> +	ret = ioctl(fd, BTRFS_IOC_TREE_SEARCH, bg_args);
> +	e = errno;
> +	if (ret < 0) {
> +		fprintf(stderr, "ret %d error '%s'\n", ret,
> +				strerror(e));

Same problem here.

> +	}
> +	return ret;
> +}
>  static const char * const cmd_inspect_inode_resolve_usage[] = {
>  	"btrfs inspect-internal inode-resolve [-v] <inode> <path>",
>  	"Get file system paths for the given inode",
>

Re: [PATCH 2/3] btrfs-progs: Add a command to show bg info

[Qu Wenruo]

At 10/18/2016 08:35 AM, Divya Indi wrote:
> Add a new subcommand to btrfs inspect-internal
>
> btrfs inspect-internal bg_analysis <path>
> Gives information about all the block groups.
>
> Signed-off-by: Divya Indi <divya.indi@oracle.com>
> Reviewed-by: Ashish Samant <ashish.samant@oracle.com>
> Reviewed-by: Liu Bo <bo.li.liu@oracle.com>
> ---
>  cmds-inspect.c |  114 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 114 insertions(+), 0 deletions(-)
>
> diff --git a/cmds-inspect.c b/cmds-inspect.c
> index f435ea9..0e2f15a 100644
> --- a/cmds-inspect.c
> +++ b/cmds-inspect.c
> @@ -147,6 +147,118 @@ static int get_bg_info(int fd, struct btrfs_ioctl_search_args *bg_args,
>  	}
>  	return ret;
>  }
> +
> +static const char * const cmd_inspect_bg_analysis_usage[] = {
> +	"btrfs inspect-internal bg_analysis <path> ",
> +	"Get information about all block groups",

What about get bg info for given range, and not all?

I think it's just several new parameters, as your design is capable to 
handle that.

> +	"",
> +	"",
> +	NULL
> +};
> +
> +static int cmd_inspect_bg_analysis(int argc, char **argv)
> +{
> +	struct btrfs_ioctl_search_args args;
> +	struct btrfs_ioctl_search_args bg_args;
> +	struct btrfs_ioctl_search_header *header;
> +	struct btrfs_ioctl_search_header *bg_header;
> +	struct btrfs_ioctl_search_key *sk;
> +	struct btrfs_ioctl_search_key *bg_sk;
> +	struct btrfs_block_group_item *bg;
> +	struct btrfs_chunk *chunk;
> +	unsigned long off = 0;
> +	unsigned long bg_off = 0;
> +	DIR *dirstream = NULL;
> +	int fd;
> +	int i;
> +	int ret = 0;
> +	u64 used;
> +	u64 flags;
> +	char bg_type[32] = {0};
> +
> +	if (check_argc_exact(argc, 2))
> +		usage(cmd_inspect_bg_analysis_usage);
> +
> +	fd = btrfs_open_dir(argv[optind], &dirstream, 1);
> +	if (fd < 0)
> +		return 1;
> +
> +	memset(&args, 0, sizeof(args));
> +	sk = &args.key;
> +	sk->min_offset = sk->min_transid = 0;
> +	sk->max_offset = sk->max_transid = (u64)-1;
> +	printf("%20s%20s%20s%20s\n", "Type", "Start", "Len", "Used");
> +	while (1) {
> +
> +		/* Walk through the chunk tree and retrieve all the chunks */
> +		ret = get_chunks(fd, &args);
> +		if (ret < 0)
> +			goto out;
> +
> +		/*
> +		 * it should not happen.
> +		 */
> +		if (sk->nr_items == 0)
> +			break;
> +
> +		off = 0;
> +		memset(&bg_args, 0, sizeof(bg_args));
> +		bg_sk = &bg_args.key;
> +
> +		bg_sk->tree_id = BTRFS_EXTENT_TREE_OBJECTID;
> +		bg_sk->min_type = BTRFS_BLOCK_GROUP_ITEM_KEY;
> +		bg_sk->max_type = BTRFS_BLOCK_GROUP_ITEM_KEY;
> +		bg_sk->min_transid =  0;
> +		bg_sk->max_transid = (u64)-1;
> +
> +		for (i = 0; i < sk->nr_items; i++) {
> +			header = (struct btrfs_ioctl_search_header *)(args.buf
> +								      + off);
> +			off += sizeof(*header);
> +			if (header->type == BTRFS_CHUNK_ITEM_KEY) {
> +				chunk = (struct btrfs_chunk *)
> +					(args.buf + off);
> +
> +				/* For every chunk lookup an exact match(bg) in
> +				 * the extent tree and read its used values */
> +				ret = get_bg_info(fd, &bg_args, header->offset,
> +						  chunk->length);
> +				if (ret < 0)
> +					goto out;
> +
> +				/*
> +				 * it should not happen.
> +				 */
> +				if (bg_sk->nr_items == 0)
> +					continue;
> +
> +				bg_off = 0;
> +				bg_header = (struct btrfs_ioctl_search_header *)
> +					    (bg_args.buf + bg_off);
> +				bg_off += sizeof(*bg_header);
> +				bg = (struct btrfs_block_group_item *)
> +				     (bg_args.buf + bg_off);
> +
> +				used = btrfs_block_group_used(bg);
> +				memset(&bg_type, 0, 32);
> +				flags = btrfs_block_group_flags(bg);
> +				bg_flags_to_str(flags, bg_type);
> +				printf("%20s%20llu%20s%20s\n",
> +					bg_type,
> +					bg_header->objectid,
> +					pretty_size(bg_header->offset),
> +					pretty_size(used));

BTW, it would be quite handy if we can specify units.

> +			}
> +			off += header->len;
> +			sk->min_offset = header->offset + header->len;
> +		}
> +		sk->nr_items = 4096;
> +	}
> +out:
> +	close_file_or_dir(fd, dirstream);
> +	return ret;
> +}
> +
>  static const char * const cmd_inspect_inode_resolve_usage[] = {
>  	"btrfs inspect-internal inode-resolve [-v] <inode> <path>",
>  	"Get file system paths for the given inode",
> @@ -702,6 +814,8 @@ const struct cmd_group inspect_cmd_group = {
>  			0 },
>  		{ "min-dev-size", cmd_inspect_min_dev_size,
>  			cmd_inspect_min_dev_size_usage, NULL, 0 },
> +		{ "bg_analysis", cmd_inspect_bg_analysis,
> +			cmd_inspect_bg_analysis_usage, NULL, 0 },

Just naming preference, IMHO show-block-groups or dump-block-groups 
seems better for me.

Other part seems good.
Feel free to add my reviewed by tag.

Thanks,
Qu
>  		{ "dump-tree", cmd_inspect_dump_tree,
>  				cmd_inspect_dump_tree_usage, NULL, 0 },
>  		{ "dump-super", cmd_inspect_dump_super,
>

Re: [PATCH 3/3] btrfs-progs: Add command to check if balance op is req

[Qu Wenruo]

At 10/18/2016 08:35 AM, Divya Indi wrote:
> Add new subcommand to btrfs inspect-internal
>
> btrfs inspect-internal balance_check <path>
> Checks whether 'btrfs balance' can help creating more space (Only
> considers data block groups).

I didn't think it's good to add a new subcommand just for that.

Why not output such relocation sugguestion for you previous bg-analyze 
subcommand?
(It's better to make it a parameter to trigger such output)

Thanks,
Qu
>
> Signed-off-by: Divya Indi <divya.indi@oracle.com>
> Reviewed-by: Ashish Samant <ashish.samant@oracle.com>
> Reviewed-by: Liu Bo <bo.li.liu@oracle.com>
> ---
>  cmds-inspect.c |  147 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 147 insertions(+), 0 deletions(-)
>
> diff --git a/cmds-inspect.c b/cmds-inspect.c
> index 0e2f15a..5baaa49 100644
> --- a/cmds-inspect.c
> +++ b/cmds-inspect.c
> @@ -267,6 +267,151 @@ static const char * const cmd_inspect_inode_resolve_usage[] = {
>  	NULL
>  };
>
> +static const char * const cmd_inspect_balance_check_usage[] = {
> +	"btrfs inspect-internal balance_check <path>",
> +	"To check whether 'btrfs balance' can help creating more space",
> +	"",
> +	"",
> +	NULL
> +};
> +
> +static int cmd_inspect_balance_check(int argc, char **argv)
> +{
> +	struct btrfs_ioctl_search_args args;
> +	struct btrfs_ioctl_search_args bg_args;
> +	struct btrfs_ioctl_search_key *sk;
> +	struct btrfs_ioctl_search_key *bg_sk;
> +	struct btrfs_ioctl_search_header *header;
> +	struct btrfs_ioctl_search_header *bg_header;
> +	struct btrfs_block_group_item *bg;
> +	struct btrfs_chunk *chunk;
> +	unsigned long off = 0;
> +	unsigned long bg_off = 0;
> +	DIR *dirstream = NULL;
> +	int fd;
> +	int i;
> +	u64 total_free = 0;
> +	u64 min_used = (u64)-1;
> +	u64 free_of_min_used = 0;
> +	u64 bg_of_min_used = 0;
> +	u64 flags;
> +	u64 used;
> +	int ret = 0;
> +	int nr_data_bgs = 0;
> +
> +	if (check_argc_exact(argc, 2))
> +		usage(cmd_inspect_balance_check_usage);
> +
> +	fd = btrfs_open_dir(argv[optind], &dirstream, 1);
> +	if (fd < 0)
> +		return 1;
> +
> +	memset(&args, 0, sizeof(args));
> +	sk = &args.key;
> +	sk->min_offset = sk->min_transid = 0;
> +	sk->max_offset = sk->max_transid = (u64)-1;
> +
> +	printf("%20s%20s%20s\n", "Start", "Len", "Used");
> +	while (1) {
> +		ret = get_chunks(fd, &args);
> +		if (ret < 0)
> +			goto out;
> +
> +		/*
> +		 * it should not happen.
> +		 */
> +		if (sk->nr_items == 0)
> +			break;
> +
> +		off = 0;
> +		memset(&bg_args, 0, sizeof(bg_args));
> +		bg_sk = &bg_args.key;
> +
> +		/* For every chunk, look up 1 exact match for block group in
> +		 * the extent tree. */
> +		bg_sk->tree_id = BTRFS_EXTENT_TREE_OBJECTID;
> +		bg_sk->min_type = BTRFS_BLOCK_GROUP_ITEM_KEY;
> +		bg_sk->max_type = BTRFS_BLOCK_GROUP_ITEM_KEY;
> +		bg_sk->min_transid =  0;
> +		bg_sk->max_transid = (u64)-1;
> +
> +		for (i = 0; i < sk->nr_items; i++) {
> +			header = (struct btrfs_ioctl_search_header *)(args.buf
> +								      + off);
> +			off += sizeof(*header);
> +			if (header->type == BTRFS_CHUNK_ITEM_KEY) {
> +				chunk = (struct btrfs_chunk *)
> +					(args.buf + off);
> +				ret = get_bg_info(fd, &bg_args, header->offset,
> +						  chunk->length);
> +				if (ret < 0)
> +					goto out;
> +
> +				/*
> +				 * it should not happen.
> +				 */
> +				if (bg_sk->nr_items == 0)
> +					continue;
> +
> +				bg_off = 0;
> +				bg_header = (struct btrfs_ioctl_search_header *)
> +					    (bg_args.buf + bg_off);
> +				bg_off += sizeof(*bg_header);
> +				bg = (struct btrfs_block_group_item *)
> +				     (bg_args.buf + bg_off);
> +
> +				flags = btrfs_block_group_flags(bg);
> +				if (flags & BTRFS_BLOCK_GROUP_DATA) {
> +					used = btrfs_block_group_used(bg);
> +					nr_data_bgs++;
> +					printf("%20llu%20s%20s\n",
> +						bg_header->objectid,
> +						pretty_size(bg_header->offset),
> +						pretty_size(used));
> +					total_free += bg_header->offset - used;
> +					if (min_used >= used) {
> +						min_used = used;
> +						free_of_min_used =
> +							bg_header->offset - used;
> +						bg_of_min_used =
> +							bg_header->objectid;
> +					}
> +				}
> +			}
> +
> +			off += header->len;
> +			sk->min_offset = header->offset + header->len;
> +		}
> +		sk->nr_items = 4096;
> +
> +	}
> +
> +	if (nr_data_bgs <= 1) {
> +		printf("Data block groups in fs = %d, no need to do balance.\n",
> +				nr_data_bgs);
> +		ret = 1;
> +		goto out;
> +	}
> +
> +	printf("Total data bgs: %d\nTotal free space: %s\n"
> +	       "For min used bg %llu used = %s free = %s\n",
> +		nr_data_bgs, pretty_size(total_free), bg_of_min_used,
> +		pretty_size(min_used), pretty_size(free_of_min_used));
> +
> +	if (total_free - free_of_min_used > min_used) {
> +		printf("run 'btrfs balance start -dvrange=%llu..%llu <mountpoint>'\n",
> +				bg_of_min_used, bg_of_min_used + 1);
> +		ret = 0;
> +	} else {
> +		printf("Please don't balance data block groups, no free space\n");
> +		ret = 1;
> +	}
> +
> +out:
> +	close_file_or_dir(fd, dirstream);
> +	return ret;
> +}
> +
>  static int cmd_inspect_inode_resolve(int argc, char **argv)
>  {
>  	int fd;
> @@ -816,6 +961,8 @@ const struct cmd_group inspect_cmd_group = {
>  			cmd_inspect_min_dev_size_usage, NULL, 0 },
>  		{ "bg_analysis", cmd_inspect_bg_analysis,
>  			cmd_inspect_bg_analysis_usage, NULL, 0 },
> +		{ "balance_check", cmd_inspect_balance_check,
> +			cmd_inspect_balance_check_usage, NULL, 0 },
>  		{ "dump-tree", cmd_inspect_dump_tree,
>  				cmd_inspect_dump_tree_usage, NULL, 0 },
>  		{ "dump-super", cmd_inspect_dump_super,
>

Re: [PATCH 2/3] btrfs-progs: Add a command to show bg info

[Roman Mamedov]

On Tue, 18 Oct 2016 09:39:32 +0800
Qu Wenruo <quwenruo@cn.fujitsu.com> wrote:

> >  static const char * const cmd_inspect_inode_resolve_usage[] = {
> >  	"btrfs inspect-internal inode-resolve [-v] <inode> <path>",
> >  	"Get file system paths for the given inode",
> > @@ -702,6 +814,8 @@ const struct cmd_group inspect_cmd_group = {
> >  			0 },
> >  		{ "min-dev-size", cmd_inspect_min_dev_size,
> >  			cmd_inspect_min_dev_size_usage, NULL, 0 },
> > +		{ "bg_analysis", cmd_inspect_bg_analysis,
> > +			cmd_inspect_bg_analysis_usage, NULL, 0 },
> 
> Just naming preference, IMHO show-block-groups or dump-block-groups 
> seems better for me.

And in any case please don't mix separation by "-" and "_" in the same command
string. In btrfs tool the convention is to separate words in subcommand names
using "-".

-- 
With respect,
Roman

Re: [PATCH 3/3] btrfs-progs: Add command to check if balance op is req

[Divya Indi]

On 10/17/2016 06:42 PM, Qu Wenruo wrote:
>
>
> At 10/18/2016 08:35 AM, Divya Indi wrote:
>> Add new subcommand to btrfs inspect-internal
>>
>> btrfs inspect-internal balance_check <path>
>> Checks whether 'btrfs balance' can help creating more space (Only
>> considers data block groups).
>
> I didn't think it's good to add a new subcommand just for that.
>
> Why not output such relocation sugguestion for you previous bg-analyze 
> subcommand?
> (It's better to make it a parameter to trigger such output)
>
> Thanks,
> Qu
Or maybe as an option to btrfs balance start?
Eg: btrfs balance start --check-only <path>
>>
>> Signed-off-by: Divya Indi <divya.indi@oracle.com>
>> Reviewed-by: Ashish Samant <ashish.samant@oracle.com>
>> Reviewed-by: Liu Bo <bo.li.liu@oracle.com>
>> ---
>>  cmds-inspect.c |  147 
>> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  1 files changed, 147 insertions(+), 0 deletions(-)
>>
>> diff --git a/cmds-inspect.c b/cmds-inspect.c
>> index 0e2f15a..5baaa49 100644
>> --- a/cmds-inspect.c
>> +++ b/cmds-inspect.c
>> @@ -267,6 +267,151 @@ static const char * const 
>> cmd_inspect_inode_resolve_usage[] = {
>>      NULL
>>  };
>>
>> +static const char * const cmd_inspect_balance_check_usage[] = {
>> +    "btrfs inspect-internal balance_check <path>",
>> +    "To check whether 'btrfs balance' can help creating more space",
>> +    "",
>> +    "",
>> +    NULL
>> +};
>> +
>> +static int cmd_inspect_balance_check(int argc, char **argv)
>> +{
>> +    struct btrfs_ioctl_search_args args;
>> +    struct btrfs_ioctl_search_args bg_args;
>> +    struct btrfs_ioctl_search_key *sk;
>> +    struct btrfs_ioctl_search_key *bg_sk;
>> +    struct btrfs_ioctl_search_header *header;
>> +    struct btrfs_ioctl_search_header *bg_header;
>> +    struct btrfs_block_group_item *bg;
>> +    struct btrfs_chunk *chunk;
>> +    unsigned long off = 0;
>> +    unsigned long bg_off = 0;
>> +    DIR *dirstream = NULL;
>> +    int fd;
>> +    int i;
>> +    u64 total_free = 0;
>> +    u64 min_used = (u64)-1;
>> +    u64 free_of_min_used = 0;
>> +    u64 bg_of_min_used = 0;
>> +    u64 flags;
>> +    u64 used;
>> +    int ret = 0;
>> +    int nr_data_bgs = 0;
>> +
>> +    if (check_argc_exact(argc, 2))
>> +        usage(cmd_inspect_balance_check_usage);
>> +
>> +    fd = btrfs_open_dir(argv[optind], &dirstream, 1);
>> +    if (fd < 0)
>> +        return 1;
>> +
>> +    memset(&args, 0, sizeof(args));
>> +    sk = &args.key;
>> +    sk->min_offset = sk->min_transid = 0;
>> +    sk->max_offset = sk->max_transid = (u64)-1;
>> +
>> +    printf("%20s%20s%20s\n", "Start", "Len", "Used");
>> +    while (1) {
>> +        ret = get_chunks(fd, &args);
>> +        if (ret < 0)
>> +            goto out;
>> +
>> +        /*
>> +         * it should not happen.
>> +         */
>> +        if (sk->nr_items == 0)
>> +            break;
>> +
>> +        off = 0;
>> +        memset(&bg_args, 0, sizeof(bg_args));
>> +        bg_sk = &bg_args.key;
>> +
>> +        /* For every chunk, look up 1 exact match for block group in
>> +         * the extent tree. */
>> +        bg_sk->tree_id = BTRFS_EXTENT_TREE_OBJECTID;
>> +        bg_sk->min_type = BTRFS_BLOCK_GROUP_ITEM_KEY;
>> +        bg_sk->max_type = BTRFS_BLOCK_GROUP_ITEM_KEY;
>> +        bg_sk->min_transid =  0;
>> +        bg_sk->max_transid = (u64)-1;
>> +
>> +        for (i = 0; i < sk->nr_items; i++) {
>> +            header = (struct btrfs_ioctl_search_header *)(args.buf
>> +                                      + off);
>> +            off += sizeof(*header);
>> +            if (header->type == BTRFS_CHUNK_ITEM_KEY) {
>> +                chunk = (struct btrfs_chunk *)
>> +                    (args.buf + off);
>> +                ret = get_bg_info(fd, &bg_args, header->offset,
>> +                          chunk->length);
>> +                if (ret < 0)
>> +                    goto out;
>> +
>> +                /*
>> +                 * it should not happen.
>> +                 */
>> +                if (bg_sk->nr_items == 0)
>> +                    continue;
>> +
>> +                bg_off = 0;
>> +                bg_header = (struct btrfs_ioctl_search_header *)
>> +                        (bg_args.buf + bg_off);
>> +                bg_off += sizeof(*bg_header);
>> +                bg = (struct btrfs_block_group_item *)
>> +                     (bg_args.buf + bg_off);
>> +
>> +                flags = btrfs_block_group_flags(bg);
>> +                if (flags & BTRFS_BLOCK_GROUP_DATA) {
>> +                    used = btrfs_block_group_used(bg);
>> +                    nr_data_bgs++;
>> +                    printf("%20llu%20s%20s\n",
>> +                        bg_header->objectid,
>> +                        pretty_size(bg_header->offset),
>> +                        pretty_size(used));
>> +                    total_free += bg_header->offset - used;
>> +                    if (min_used >= used) {
>> +                        min_used = used;
>> +                        free_of_min_used =
>> +                            bg_header->offset - used;
>> +                        bg_of_min_used =
>> +                            bg_header->objectid;
>> +                    }
>> +                }
>> +            }
>> +
>> +            off += header->len;
>> +            sk->min_offset = header->offset + header->len;
>> +        }
>> +        sk->nr_items = 4096;
>> +
>> +    }
>> +
>> +    if (nr_data_bgs <= 1) {
>> +        printf("Data block groups in fs = %d, no need to do 
>> balance.\n",
>> +                nr_data_bgs);
>> +        ret = 1;
>> +        goto out;
>> +    }
>> +
>> +    printf("Total data bgs: %d\nTotal free space: %s\n"
>> +           "For min used bg %llu used = %s free = %s\n",
>> +        nr_data_bgs, pretty_size(total_free), bg_of_min_used,
>> +        pretty_size(min_used), pretty_size(free_of_min_used));
>> +
>> +    if (total_free - free_of_min_used > min_used) {
>> +        printf("run 'btrfs balance start -dvrange=%llu..%llu 
>> <mountpoint>'\n",
>> +                bg_of_min_used, bg_of_min_used + 1);
>> +        ret = 0;
>> +    } else {
>> +        printf("Please don't balance data block groups, no free 
>> space\n");
>> +        ret = 1;
>> +    }
>> +
>> +out:
>> +    close_file_or_dir(fd, dirstream);
>> +    return ret;
>> +}
>> +
>>  static int cmd_inspect_inode_resolve(int argc, char **argv)
>>  {
>>      int fd;
>> @@ -816,6 +961,8 @@ const struct cmd_group inspect_cmd_group = {
>>              cmd_inspect_min_dev_size_usage, NULL, 0 },
>>          { "bg_analysis", cmd_inspect_bg_analysis,
>>              cmd_inspect_bg_analysis_usage, NULL, 0 },
>> +        { "balance_check", cmd_inspect_balance_check,
>> +            cmd_inspect_balance_check_usage, NULL, 0 },
>>          { "dump-tree", cmd_inspect_dump_tree,
>>                  cmd_inspect_dump_tree_usage, NULL, 0 },
>>          { "dump-super", cmd_inspect_dump_super,
>>
>
>

Re: [PATCH 2/3] btrfs-progs: Add a command to show bg info

[Divya Indi]

On 10/17/2016 10:24 PM, Roman Mamedov wrote:
> On Tue, 18 Oct 2016 09:39:32 +0800
> Qu Wenruo <quwenruo@cn.fujitsu.com> wrote:
>
>>>   static const char * const cmd_inspect_inode_resolve_usage[] = {
>>>   	"btrfs inspect-internal inode-resolve [-v] <inode> <path>",
>>>   	"Get file system paths for the given inode",
>>> @@ -702,6 +814,8 @@ const struct cmd_group inspect_cmd_group = {
>>>   			0 },
>>>   		{ "min-dev-size", cmd_inspect_min_dev_size,
>>>   			cmd_inspect_min_dev_size_usage, NULL, 0 },
>>> +		{ "bg_analysis", cmd_inspect_bg_analysis,
>>> +			cmd_inspect_bg_analysis_usage, NULL, 0 },
>> Just naming preference, IMHO show-block-groups or dump-block-groups
>> seems better for me.
> And in any case please don't mix separation by "-" and "_" in the same command
> string. In btrfs tool the convention is to separate words in subcommand names
> using "-".
>
Noted, thanks! Will update the patch to correct this.

Re: [PATCH 3/3] btrfs-progs: Add command to check if balance op is req

[David Sterba]

On Wed, Oct 19, 2016 at 10:08:23AM -0700, Divya Indi wrote:
> On 10/17/2016 06:42 PM, Qu Wenruo wrote:
> > At 10/18/2016 08:35 AM, Divya Indi wrote:
> >> Add new subcommand to btrfs inspect-internal
> >>
> >> btrfs inspect-internal balance_check <path>
> >> Checks whether 'btrfs balance' can help creating more space (Only
> >> considers data block groups).
> >
> > I didn't think it's good to add a new subcommand just for that.
> >
> > Why not output such relocation sugguestion for you previous bg-analyze 
> > subcommand?
> > (It's better to make it a parameter to trigger such output)
> Or maybe as an option to btrfs balance start?
> Eg: btrfs balance start --check-only <path>

I tend to agree with this approach. The usecase, with some random sample
balance options:

 $ btrfs balance start --analyze -dusage=10 -musage=5 /path

returns if the command would be able to make any progress according to
the given filters and then

 $ btrfs balance start -dusage=10 -musage=5 /path

would actually do that. This is inherently racy, as the analysis will
happen in userspace and the filesystem block group layout could change
in the meantime.

This also implies that the filters are implemented in the userspace,
duplicating the kernel code. This is not neccesarily bad, as this would
work even on older kernels (with new progs), compared to a new analysis
mode of balance implemented in kernel.

Re: [PATCH 1/3] btrfs-progs: Generic functions to retrieve chunks and their bg info

[David Sterba]

On Mon, Oct 17, 2016 at 05:35:13PM -0700, Divya Indi wrote:
> An efficient alternative to retrieving block groups:
> get_chunks(): Walk the chunk tree to retrieve the chunks.
> get_bg_info(): For each retrieved chunk, lookup an exact match of block
> group in the extent tree.
> 
> Signed-off-by: Divya Indi <divya.indi@oracle.com>
> Reviewed-by: Ashish Samant <ashish.samant@oracle.com>
> Reviewed-by: Liu Bo <bo.li.liu@oracle.com>
> ---
>  cmds-inspect.c |   66 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 66 insertions(+), 0 deletions(-)
> 
> diff --git a/cmds-inspect.c b/cmds-inspect.c
> index 4b7cea0..f435ea9 100644
> --- a/cmds-inspect.c
> +++ b/cmds-inspect.c
> @@ -81,6 +81,72 @@ out:
>  	return !!ret;
>  }
>  
> +static void bg_flags_to_str(u64 flags, char *ret)
> +{
> +	int empty = 1;
> +
> +	if (flags & BTRFS_BLOCK_GROUP_DATA) {
> +		empty = 0;
> +		strcpy(ret, "DATA");
> +	}
> +	if (flags & BTRFS_BLOCK_GROUP_METADATA) {
> +		if (!empty)
> +			strcat(ret, "|");
> +		strcat(ret, "METADATA");
> +	}
> +	if (flags & BTRFS_BLOCK_GROUP_SYSTEM) {
> +		if (!empty)
> +			strcat(ret, "|");
> +		strcat(ret, "SYSTEM");
> +	}
> +}
> +
> +/* Walking through the chunk tree to retrieve chunks. */

No empty newline.

> +
> +static int get_chunks(int fd, struct btrfs_ioctl_search_args *chunk_args)
> +{
> +	struct btrfs_ioctl_search_key *sk;
> +	int ret;
> +	int e;
> +
> +	sk = &chunk_args->key;
> +
> +	sk->tree_id = BTRFS_CHUNK_TREE_OBJECTID;
> +	sk->min_objectid = sk->max_objectid = BTRFS_FIRST_CHUNK_TREE_OBJECTID;
> +	sk->max_type = sk->min_type = BTRFS_CHUNK_ITEM_KEY;

Please don't do multiple asignments in one statement.

> +	sk->nr_items = 4096;
> +
> +	ret = ioctl(fd, BTRFS_IOC_TREE_SEARCH, chunk_args);
> +	e = errno;

This is useless asignment, I've removed it from the code, please don't
reintrduce it.

> +	if (ret < 0) {
> +		fprintf(stderr, "ret %d error '%s'\n", ret,
> +				strerror(e));
> +	}
> +	return ret;
> +}


> +
> +/* Given the objectid, find the block group item in the extent tree */
> +static int get_bg_info(int fd, struct btrfs_ioctl_search_args *bg_args,
> +		       u64 objectid, unsigned long length)
> +{
> +	struct btrfs_ioctl_search_key *bg_sk;
> +	int ret;
> +	int e;
> +
> +	bg_sk = &bg_args->key;
> +
> +	bg_sk->min_objectid = bg_sk->max_objectid = objectid;
> +	bg_sk->nr_items = 1;
> +	bg_sk->min_offset = bg_sk->max_offset = length;

Same here.

> +
> +	ret = ioctl(fd, BTRFS_IOC_TREE_SEARCH, bg_args);
> +	e = errno;
> +	if (ret < 0) {
> +		fprintf(stderr, "ret %d error '%s'\n", ret,
> +				strerror(e));

Please take a look how the error messages are constructed when the tree
search ioctl fails, there are enough examples in the code.

> +	}
> +	return ret;
> +}
>  static const char * const cmd_inspect_inode_resolve_usage[] = {
>  	"btrfs inspect-internal inode-resolve [-v] <inode> <path>",
>  	"Get file system paths for the given inode",

Actually, I'm not sure if such functions should exist at all, as they
only hide the search ioctl but don't do any validation of the returned
keys and data.

Re: [PATCH 2/3] btrfs-progs: Add a command to show bg info

[David Sterba]

On Mon, Oct 17, 2016 at 05:35:14PM -0700, Divya Indi wrote:
> Add a new subcommand to btrfs inspect-internal
> 
> btrfs inspect-internal bg_analysis <path>
> Gives information about all the block groups.

The sample output from the cover letter should also go here (or just
here).

Below are some comments, but overall, I think the block group dumping
should be more advanced than just what this patch does. It's a good
start, but given the rich structure of the blockgroups and chunks, I'd
rather see the basics done right from the beginning.

The blockgroups and chunks can be viewed in a logical or physical way.
I've sent a patch some time agou, that dumps the physical structure, but
got reminded that the balance analysis is more interesting on the
logical level.

Ideally I'd like to keep both ways to show the information. The physical
way is easier, just iterate the device extents and print start, lenght
etc. The logical is more tricky as it's a tree structure, when one
logical chunk could comprised of several physical chunks. And this must
reflect all current raid profiles, where we're mixing mirorring and
striping, and chunks of special kind (parity).

Now, I'm not asking you to implement all of that, but I want to make
sure that code that touches the area of interest does not block further
extensions. I understand that you want to add the ability to optimize
the balance.

> Signed-off-by: Divya Indi <divya.indi@oracle.com>
> Reviewed-by: Ashish Samant <ashish.samant@oracle.com>
> Reviewed-by: Liu Bo <bo.li.liu@oracle.com>
> ---
>  cmds-inspect.c |  114 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 114 insertions(+), 0 deletions(-)
> 
> diff --git a/cmds-inspect.c b/cmds-inspect.c
> index f435ea9..0e2f15a 100644
> --- a/cmds-inspect.c
> +++ b/cmds-inspect.c
> @@ -147,6 +147,118 @@ static int get_bg_info(int fd, struct btrfs_ioctl_search_args *bg_args,
>  	}
>  	return ret;
>  }
> +
> +static const char * const cmd_inspect_bg_analysis_usage[] = {
> +	"btrfs inspect-internal bg_analysis <path> ",
> +	"Get information about all block groups",
> +	"",
> +	"",
> +	NULL
> +};
> +
> +static int cmd_inspect_bg_analysis(int argc, char **argv)
> +{
> +	struct btrfs_ioctl_search_args args;
> +	struct btrfs_ioctl_search_args bg_args;
> +	struct btrfs_ioctl_search_header *header;
> +	struct btrfs_ioctl_search_header *bg_header;
> +	struct btrfs_ioctl_search_key *sk;
> +	struct btrfs_ioctl_search_key *bg_sk;
> +	struct btrfs_block_group_item *bg;
> +	struct btrfs_chunk *chunk;
> +	unsigned long off = 0;
> +	unsigned long bg_off = 0;
> +	DIR *dirstream = NULL;
> +	int fd;
> +	int i;
> +	int ret = 0;
> +	u64 used;
> +	u64 flags;
> +	char bg_type[32] = {0};
> +
> +	if (check_argc_exact(argc, 2))
> +		usage(cmd_inspect_bg_analysis_usage);
> +
> +	fd = btrfs_open_dir(argv[optind], &dirstream, 1);
> +	if (fd < 0)
> +		return 1;
> +
> +	memset(&args, 0, sizeof(args));
> +	sk = &args.key;
> +	sk->min_offset = sk->min_transid = 0;
> +	sk->max_offset = sk->max_transid = (u64)-1;
> +	printf("%20s%20s%20s%20s\n", "Type", "Start", "Len", "Used");
> +	while (1) {
> +
> +		/* Walk through the chunk tree and retrieve all the chunks */
> +		ret = get_chunks(fd, &args);
> +		if (ret < 0)
> +			goto out;
> +
> +		/*
> +		 * it should not happen.
> +		 */
> +		if (sk->nr_items == 0)
> +			break;

So is this an error condition? If yes, then it should be handled.

> +
> +		off = 0;
> +		memset(&bg_args, 0, sizeof(bg_args));
> +		bg_sk = &bg_args.key;
> +
> +		bg_sk->tree_id = BTRFS_EXTENT_TREE_OBJECTID;
> +		bg_sk->min_type = BTRFS_BLOCK_GROUP_ITEM_KEY;
> +		bg_sk->max_type = BTRFS_BLOCK_GROUP_ITEM_KEY;
> +		bg_sk->min_transid =  0;
> +		bg_sk->max_transid = (u64)-1;
> +
> +		for (i = 0; i < sk->nr_items; i++) {
> +			header = (struct btrfs_ioctl_search_header *)(args.buf
> +								      + off);
> +			off += sizeof(*header);
> +			if (header->type == BTRFS_CHUNK_ITEM_KEY) {
> +				chunk = (struct btrfs_chunk *)
> +					(args.buf + off);
> +
> +				/* For every chunk lookup an exact match(bg) in
> +				 * the extent tree and read its used values */

Comment formatting

> +				ret = get_bg_info(fd, &bg_args, header->offset,
> +						  chunk->length);
> +				if (ret < 0)
> +					goto out;
> +
> +				/*
> +				 * it should not happen.
> +				 */
> +				if (bg_sk->nr_items == 0)
> +					continue;
> +
> +				bg_off = 0;
> +				bg_header = (struct btrfs_ioctl_search_header *)
> +					    (bg_args.buf + bg_off);
> +				bg_off += sizeof(*bg_header);
> +				bg = (struct btrfs_block_group_item *)
> +				     (bg_args.buf + bg_off);
> +
> +				used = btrfs_block_group_used(bg);
> +				memset(&bg_type, 0, 32);

This should be sizeof(bg_type), no?

> +				flags = btrfs_block_group_flags(bg);
> +				bg_flags_to_str(flags, bg_type);
> +				printf("%20s%20llu%20s%20s\n",
> +					bg_type,
> +					bg_header->objectid,
> +					pretty_size(bg_header->offset),
> +					pretty_size(used));
> +			}
> +			off += header->len;
> +			sk->min_offset = header->offset + header->len;
> +		}
> +		sk->nr_items = 4096;

Better put that right before the search ioctls.

> +	}
> +out:
> +	close_file_or_dir(fd, dirstream);
> +	return ret;
> +}
> +
>  static const char * const cmd_inspect_inode_resolve_usage[] = {
>  	"btrfs inspect-internal inode-resolve [-v] <inode> <path>",
>  	"Get file system paths for the given inode",
> @@ -702,6 +814,8 @@ const struct cmd_group inspect_cmd_group = {
>  			0 },
>  		{ "min-dev-size", cmd_inspect_min_dev_size,
>  			cmd_inspect_min_dev_size_usage, NULL, 0 },
> +		{ "bg_analysis", cmd_inspect_bg_analysis,
> +			cmd_inspect_bg_analysis_usage, NULL, 0 },
>  		{ "dump-tree", cmd_inspect_dump_tree,
>  				cmd_inspect_dump_tree_usage, NULL, 0 },
>  		{ "dump-super", cmd_inspect_dump_super,
> -- 
> 1.7.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3] btrfs-progs: Add command to check if balance op is req

[Graham Cobb]

On 28/10/16 16:20, David Sterba wrote:
> I tend to agree with this approach. The usecase, with some random sample
> balance options:
> 
>  $ btrfs balance start --analyze -dusage=10 -musage=5 /path

Wouldn't a "balance analyze" command be better than "balance start
--analyze"? I would have guessed the latter started the balance but
printed some analysis as well (before or, probably more usefully,
afterwards).

There might, of course, be some point in a (future)

$ btrfs balance start --if-needed -dusage=10 -musage=5 /path

command.

Re: [PATCH 3/3] btrfs-progs: Add command to check if balance op is req

[Qu Wenruo]

On 10/28/2016 11:20 PM, David Sterba wrote:
> On Wed, Oct 19, 2016 at 10:08:23AM -0700, Divya Indi wrote:
>> On 10/17/2016 06:42 PM, Qu Wenruo wrote:
>>> At 10/18/2016 08:35 AM, Divya Indi wrote:
>>>> Add new subcommand to btrfs inspect-internal
>>>>
>>>> btrfs inspect-internal balance_check <path>
>>>> Checks whether 'btrfs balance' can help creating more space (Only
>>>> considers data block groups).
>>>
>>> I didn't think it's good to add a new subcommand just for that.
>>>
>>> Why not output such relocation sugguestion for you previous bg-analyze
>>> subcommand?
>>> (It's better to make it a parameter to trigger such output)
>> Or maybe as an option to btrfs balance start?
>> Eg: btrfs balance start --check-only <path>
>
> I tend to agree with this approach. The usecase, with some random sample
> balance options:
>
>  $ btrfs balance start --analyze -dusage=10 -musage=5 /path

This use case is better than my original idea.
It is really useful then.

+1 for this approach.

Thanks,
Qu
>
> returns if the command would be able to make any progress according to
> the given filters and then
>
>  $ btrfs balance start -dusage=10 -musage=5 /path
>
> would actually do that. This is inherently racy, as the analysis will
> happen in userspace and the filesystem block group layout could change
> in the meantime.
>
> This also implies that the filters are implemented in the userspace,
> duplicating the kernel code. This is not neccesarily bad, as this would
> work even on older kernels (with new progs), compared to a new analysis
> mode of balance implemented in kernel.

Re: [PATCH 3/3] btrfs-progs: Add command to check if balance op is req

[David Sterba]

On Fri, Oct 28, 2016 at 05:29:45PM +0100, Graham Cobb wrote:
> On 28/10/16 16:20, David Sterba wrote:
> > I tend to agree with this approach. The usecase, with some random sample
> > balance options:
> > 
> >  $ btrfs balance start --analyze -dusage=10 -musage=5 /path
> 
> Wouldn't a "balance analyze" command be better than "balance start
> --analyze"? I would have guessed the latter started the balance but
> printed some analysis as well (before or, probably more usefully,
> afterwards).

Right, separate command seems better.

Re: [PATCH 3/3] btrfs-progs: Add command to check if balance op is req

[divya.indi]

On 10/31/2016 09:33 AM, David Sterba wrote:
> On Fri, Oct 28, 2016 at 05:29:45PM +0100, Graham Cobb wrote:
>> On 28/10/16 16:20, David Sterba wrote:
>>> I tend to agree with this approach. The usecase, with some random sample
>>> balance options:
>>>
>>>   $ btrfs balance start --analyze -dusage=10 -musage=5 /path
>> Wouldn't a "balance analyze" command be better than "balance start
>> --analyze"? I would have guessed the latter started the balance but
>> printed some analysis as well (before or, probably more usefully,
>> afterwards).
> Right, separate command seems better.
What about btrfs inspect-internal bg_analysis (new name: 
show-block-groups)? It can still be a subcommand for inspect-internal?
So, wel have 2 new sub commands:
1) btrfs balance analyze
2) btrfs inspect-internal show-block-groups
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/3] btrfs-progs: Generic functions to retrieve chunks and their bg info

[divya.indi]

On 10/28/2016 08:44 AM, David Sterba wrote:
> On Mon, Oct 17, 2016 at 05:35:13PM -0700, Divya Indi wrote:
>> An efficient alternative to retrieving block groups:
>> get_chunks(): Walk the chunk tree to retrieve the chunks.
>> get_bg_info(): For each retrieved chunk, lookup an exact match of block
>> group in the extent tree.
>>
>> Signed-off-by: Divya Indi <divya.indi@oracle.com>
>> Reviewed-by: Ashish Samant <ashish.samant@oracle.com>
>> Reviewed-by: Liu Bo <bo.li.liu@oracle.com>
>> ---
>>   cmds-inspect.c |   66 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>   1 files changed, 66 insertions(+), 0 deletions(-)
>>
>> diff --git a/cmds-inspect.c b/cmds-inspect.c
>> index 4b7cea0..f435ea9 100644
>> --- a/cmds-inspect.c
>> +++ b/cmds-inspect.c
>> @@ -81,6 +81,72 @@ out:
>>   	return !!ret;
>>   }
>>   
>> +static void bg_flags_to_str(u64 flags, char *ret)
>> +{
>> +	int empty = 1;
>> +
>> +	if (flags & BTRFS_BLOCK_GROUP_DATA) {
>> +		empty = 0;
>> +		strcpy(ret, "DATA");
>> +	}
>> +	if (flags & BTRFS_BLOCK_GROUP_METADATA) {
>> +		if (!empty)
>> +			strcat(ret, "|");
>> +		strcat(ret, "METADATA");
>> +	}
>> +	if (flags & BTRFS_BLOCK_GROUP_SYSTEM) {
>> +		if (!empty)
>> +			strcat(ret, "|");
>> +		strcat(ret, "SYSTEM");
>> +	}
>> +}
>> +
>> +/* Walking through the chunk tree to retrieve chunks. */
> No empty newline.
>
>> +
>> +static int get_chunks(int fd, struct btrfs_ioctl_search_args *chunk_args)
>> +{
>> +	struct btrfs_ioctl_search_key *sk;
>> +	int ret;
>> +	int e;
>> +
>> +	sk = &chunk_args->key;
>> +
>> +	sk->tree_id = BTRFS_CHUNK_TREE_OBJECTID;
>> +	sk->min_objectid = sk->max_objectid = BTRFS_FIRST_CHUNK_TREE_OBJECTID;
>> +	sk->max_type = sk->min_type = BTRFS_CHUNK_ITEM_KEY;
> Please don't do multiple asignments in one statement.
>
>> +	sk->nr_items = 4096;
>> +
>> +	ret = ioctl(fd, BTRFS_IOC_TREE_SEARCH, chunk_args);
>> +	e = errno;
> This is useless asignment, I've removed it from the code, please don't
> reintrduce it.
>
>> +	if (ret < 0) {
>> +		fprintf(stderr, "ret %d error '%s'\n", ret,
>> +				strerror(e));
>> +	}
>> +	return ret;
>> +}
>
>> +
>> +/* Given the objectid, find the block group item in the extent tree */
>> +static int get_bg_info(int fd, struct btrfs_ioctl_search_args *bg_args,
>> +		       u64 objectid, unsigned long length)
>> +{
>> +	struct btrfs_ioctl_search_key *bg_sk;
>> +	int ret;
>> +	int e;
>> +
>> +	bg_sk = &bg_args->key;
>> +
>> +	bg_sk->min_objectid = bg_sk->max_objectid = objectid;
>> +	bg_sk->nr_items = 1;
>> +	bg_sk->min_offset = bg_sk->max_offset = length;
> Same here.
>
>> +
>> +	ret = ioctl(fd, BTRFS_IOC_TREE_SEARCH, bg_args);
>> +	e = errno;
>> +	if (ret < 0) {
>> +		fprintf(stderr, "ret %d error '%s'\n", ret,
>> +				strerror(e));
> Please take a look how the error messages are constructed when the tree
> search ioctl fails, there are enough examples in the code.
>
>> +	}
>> +	return ret;
>> +}
>>   static const char * const cmd_inspect_inode_resolve_usage[] = {
>>   	"btrfs inspect-internal inode-resolve [-v] <inode> <path>",
>>   	"Get file system paths for the given inode",
> Actually, I'm not sure if such functions should exist at all, as they
> only hide the search ioctl but don't do any validation of the returned
> keys and data.
The intent was to avoid the same assignments and calls in both the sub 
commands, but I see your point.
  Noted- for v2.

Re: [PATCH 2/3] btrfs-progs: Add a command to show bg info

[divya.indi]

On 10/28/2016 09:00 AM, David Sterba wrote:
> On Mon, Oct 17, 2016 at 05:35:14PM -0700, Divya Indi wrote:
>> Add a new subcommand to btrfs inspect-internal
>>
>> btrfs inspect-internal bg_analysis <path>
>> Gives information about all the block groups.
> The sample output from the cover letter should also go here (or just
> here).
>
> Below are some comments, but overall, I think the block group dumping
> should be more advanced than just what this patch does. It's a good
> start, but given the rich structure of the blockgroups and chunks, I'd
> rather see the basics done right from the beginning.
>
> The blockgroups and chunks can be viewed in a logical or physical way.
> I've sent a patch some time agou, that dumps the physical structure, but
> got reminded that the balance analysis is more interesting on the
> logical level.
>
> Ideally I'd like to keep both ways to show the information. The physical
> way is easier, just iterate the device extents and print start, lenght
> etc. The logical is more tricky as it's a tree structure, when one
> logical chunk could comprised of several physical chunks. And this must
> reflect all current raid profiles, where we're mixing mirorring and
> striping, and chunks of special kind (parity).
Since most of this information is available through the chunk, we can 
try to implement these as part of v2.
>
> Now, I'm not asking you to implement all of that, but I want to make
> sure that code that touches the area of interest does not block further
> extensions. I understand that you want to add the ability to optimize
> the balance.
>
>> Signed-off-by: Divya Indi <divya.indi@oracle.com>
>> Reviewed-by: Ashish Samant <ashish.samant@oracle.com>
>> Reviewed-by: Liu Bo <bo.li.liu@oracle.com>
>> ---
>>   cmds-inspect.c |  114 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>   1 files changed, 114 insertions(+), 0 deletions(-)
>>
>> diff --git a/cmds-inspect.c b/cmds-inspect.c
>> index f435ea9..0e2f15a 100644
>> --- a/cmds-inspect.c
>> +++ b/cmds-inspect.c
>> @@ -147,6 +147,118 @@ static int get_bg_info(int fd, struct btrfs_ioctl_search_args *bg_args,
>>   	}
>>   	return ret;
>>   }
>> +
>> +static const char * const cmd_inspect_bg_analysis_usage[] = {
>> +	"btrfs inspect-internal bg_analysis <path> ",
>> +	"Get information about all block groups",
>> +	"",
>> +	"",
>> +	NULL
>> +};
>> +
>> +static int cmd_inspect_bg_analysis(int argc, char **argv)
>> +{
>> +	struct btrfs_ioctl_search_args args;
>> +	struct btrfs_ioctl_search_args bg_args;
>> +	struct btrfs_ioctl_search_header *header;
>> +	struct btrfs_ioctl_search_header *bg_header;
>> +	struct btrfs_ioctl_search_key *sk;
>> +	struct btrfs_ioctl_search_key *bg_sk;
>> +	struct btrfs_block_group_item *bg;
>> +	struct btrfs_chunk *chunk;
>> +	unsigned long off = 0;
>> +	unsigned long bg_off = 0;
>> +	DIR *dirstream = NULL;
>> +	int fd;
>> +	int i;
>> +	int ret = 0;
>> +	u64 used;
>> +	u64 flags;
>> +	char bg_type[32] = {0};
>> +
>> +	if (check_argc_exact(argc, 2))
>> +		usage(cmd_inspect_bg_analysis_usage);
>> +
>> +	fd = btrfs_open_dir(argv[optind], &dirstream, 1);
>> +	if (fd < 0)
>> +		return 1;
>> +
>> +	memset(&args, 0, sizeof(args));
>> +	sk = &args.key;
>> +	sk->min_offset = sk->min_transid = 0;
>> +	sk->max_offset = sk->max_transid = (u64)-1;
>> +	printf("%20s%20s%20s%20s\n", "Type", "Start", "Len", "Used");
>> +	while (1) {
>> +
>> +		/* Walk through the chunk tree and retrieve all the chunks */
>> +		ret = get_chunks(fd, &args);
>> +		if (ret < 0)
>> +			goto out;
>> +
>> +		/*
>> +		 * it should not happen.
>> +		 */
>> +		if (sk->nr_items == 0)
>> +			break;
> So is this an error condition? If yes, then it should be handled.
>
>> +
>> +		off = 0;
>> +		memset(&bg_args, 0, sizeof(bg_args));
>> +		bg_sk = &bg_args.key;
>> +
>> +		bg_sk->tree_id = BTRFS_EXTENT_TREE_OBJECTID;
>> +		bg_sk->min_type = BTRFS_BLOCK_GROUP_ITEM_KEY;
>> +		bg_sk->max_type = BTRFS_BLOCK_GROUP_ITEM_KEY;
>> +		bg_sk->min_transid =  0;
>> +		bg_sk->max_transid = (u64)-1;
>> +
>> +		for (i = 0; i < sk->nr_items; i++) {
>> +			header = (struct btrfs_ioctl_search_header *)(args.buf
>> +								      + off);
>> +			off += sizeof(*header);
>> +			if (header->type == BTRFS_CHUNK_ITEM_KEY) {
>> +				chunk = (struct btrfs_chunk *)
>> +					(args.buf + off);
>> +
>> +				/* For every chunk lookup an exact match(bg) in
>> +				 * the extent tree and read its used values */
> Comment formatting
>
>> +				ret = get_bg_info(fd, &bg_args, header->offset,
>> +						  chunk->length);
>> +				if (ret < 0)
>> +					goto out;
>> +
>> +				/*
>> +				 * it should not happen.
>> +				 */
>> +				if (bg_sk->nr_items == 0)
>> +					continue;
>> +
>> +				bg_off = 0;
>> +				bg_header = (struct btrfs_ioctl_search_header *)
>> +					    (bg_args.buf + bg_off);
>> +				bg_off += sizeof(*bg_header);
>> +				bg = (struct btrfs_block_group_item *)
>> +				     (bg_args.buf + bg_off);
>> +
>> +				used = btrfs_block_group_used(bg);
>> +				memset(&bg_type, 0, 32);
> This should be sizeof(bg_type), no?
>
>> +				flags = btrfs_block_group_flags(bg);
>> +				bg_flags_to_str(flags, bg_type);
>> +				printf("%20s%20llu%20s%20s\n",
>> +					bg_type,
>> +					bg_header->objectid,
>> +					pretty_size(bg_header->offset),
>> +					pretty_size(used));
>> +			}
>> +			off += header->len;
>> +			sk->min_offset = header->offset + header->len;
>> +		}
>> +		sk->nr_items = 4096;
> Better put that right before the search ioctls.
>
>> +	}
>> +out:
>> +	close_file_or_dir(fd, dirstream);
>> +	return ret;
>> +}
>> +
>>   static const char * const cmd_inspect_inode_resolve_usage[] = {
>>   	"btrfs inspect-internal inode-resolve [-v] <inode> <path>",
>>   	"Get file system paths for the given inode",
>> @@ -702,6 +814,8 @@ const struct cmd_group inspect_cmd_group = {
>>   			0 },
>>   		{ "min-dev-size", cmd_inspect_min_dev_size,
>>   			cmd_inspect_min_dev_size_usage, NULL, 0 },
>> +		{ "bg_analysis", cmd_inspect_bg_analysis,
>> +			cmd_inspect_bg_analysis_usage, NULL, 0 },
>>   		{ "dump-tree", cmd_inspect_dump_tree,
>>   				cmd_inspect_dump_tree_usage, NULL, 0 },
>>   		{ "dump-super", cmd_inspect_dump_super,
>> -- 
>> 1.7.1
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/3] btrfs-progs: Generic functions to retrieve chunks and their bg info

[Goffredo Baroncelli]

Hi,

any news about these commands ?

BR
G.Baroncelli

On 2016-10-28 17:44, David Sterba wrote:
> On Mon, Oct 17, 2016 at 05:35:13PM -0700, Divya Indi wrote:
>> An efficient alternative to retrieving block groups:
>> get_chunks(): Walk the chunk tree to retrieve the chunks.
>> get_bg_info(): For each retrieved chunk, lookup an exact match of block
>> group in the extent tree.
>>
>> Signed-off-by: Divya Indi <divya.indi@oracle.com>
>> Reviewed-by: Ashish Samant <ashish.samant@oracle.com>
>> Reviewed-by: Liu Bo <bo.li.liu@oracle.com>
>> ---
>>  cmds-inspect.c |   66 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  1 files changed, 66 insertions(+), 0 deletions(-)
>>
>> diff --git a/cmds-inspect.c b/cmds-inspect.c
>> index 4b7cea0..f435ea9 100644
>> --- a/cmds-inspect.c
>> +++ b/cmds-inspect.c
>> @@ -81,6 +81,72 @@ out:
>>  	return !!ret;
>>  }
>>  
>> +static void bg_flags_to_str(u64 flags, char *ret)
>> +{
>> +	int empty = 1;
>> +
>> +	if (flags & BTRFS_BLOCK_GROUP_DATA) {
>> +		empty = 0;
>> +		strcpy(ret, "DATA");
>> +	}
>> +	if (flags & BTRFS_BLOCK_GROUP_METADATA) {
>> +		if (!empty)
>> +			strcat(ret, "|");
>> +		strcat(ret, "METADATA");
>> +	}
>> +	if (flags & BTRFS_BLOCK_GROUP_SYSTEM) {
>> +		if (!empty)
>> +			strcat(ret, "|");
>> +		strcat(ret, "SYSTEM");
>> +	}
>> +}
>> +
>> +/* Walking through the chunk tree to retrieve chunks. */
> 
> No empty newline.
> 
>> +
>> +static int get_chunks(int fd, struct btrfs_ioctl_search_args *chunk_args)
>> +{
>> +	struct btrfs_ioctl_search_key *sk;
>> +	int ret;
>> +	int e;
>> +
>> +	sk = &chunk_args->key;
>> +
>> +	sk->tree_id = BTRFS_CHUNK_TREE_OBJECTID;
>> +	sk->min_objectid = sk->max_objectid = BTRFS_FIRST_CHUNK_TREE_OBJECTID;
>> +	sk->max_type = sk->min_type = BTRFS_CHUNK_ITEM_KEY;
> 
> Please don't do multiple asignments in one statement.
> 
>> +	sk->nr_items = 4096;
>> +
>> +	ret = ioctl(fd, BTRFS_IOC_TREE_SEARCH, chunk_args);
>> +	e = errno;
> 
> This is useless asignment, I've removed it from the code, please don't
> reintrduce it.
> 
>> +	if (ret < 0) {
>> +		fprintf(stderr, "ret %d error '%s'\n", ret,
>> +				strerror(e));
>> +	}
>> +	return ret;
>> +}
> 
> 
>> +
>> +/* Given the objectid, find the block group item in the extent tree */
>> +static int get_bg_info(int fd, struct btrfs_ioctl_search_args *bg_args,
>> +		       u64 objectid, unsigned long length)
>> +{
>> +	struct btrfs_ioctl_search_key *bg_sk;
>> +	int ret;
>> +	int e;
>> +
>> +	bg_sk = &bg_args->key;
>> +
>> +	bg_sk->min_objectid = bg_sk->max_objectid = objectid;
>> +	bg_sk->nr_items = 1;
>> +	bg_sk->min_offset = bg_sk->max_offset = length;
> 
> Same here.
> 
>> +
>> +	ret = ioctl(fd, BTRFS_IOC_TREE_SEARCH, bg_args);
>> +	e = errno;
>> +	if (ret < 0) {
>> +		fprintf(stderr, "ret %d error '%s'\n", ret,
>> +				strerror(e));
> 
> Please take a look how the error messages are constructed when the tree
> search ioctl fails, there are enough examples in the code.
> 
>> +	}
>> +	return ret;
>> +}
>>  static const char * const cmd_inspect_inode_resolve_usage[] = {
>>  	"btrfs inspect-internal inode-resolve [-v] <inode> <path>",
>>  	"Get file system paths for the given inode",
> 
> Actually, I'm not sure if such functions should exist at all, as they
> only hide the search ioctl but don't do any validation of the returned
> keys and data.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 


-- 
gpg @keyserver.linux.it: Goffredo Baroncelli <kreijackATinwind.it>
Key fingerprint BBF5 1610 0B64 DAC6 5F7D  17B2 0EDA 9B37 8B82 E0B5

Re: [PATCH 1/3] btrfs-progs: Generic functions to retrieve chunks and their bg info

[divya.indi]

Hi,

Working on a v2 of this patch based on the comments received.

Thanks,

Divya


On 06/07/2017 10:03 AM, Goffredo Baroncelli wrote:
> Hi,
>
> any news about these commands ?
>
> BR
> G.Baroncelli
>
> On 2016-10-28 17:44, David Sterba wrote:
>> On Mon, Oct 17, 2016 at 05:35:13PM -0700, Divya Indi wrote:
>>> An efficient alternative to retrieving block groups:
>>> get_chunks(): Walk the chunk tree to retrieve the chunks.
>>> get_bg_info(): For each retrieved chunk, lookup an exact match of block
>>> group in the extent tree.
>>>
>>> Signed-off-by: Divya Indi <divya.indi@oracle.com>
>>> Reviewed-by: Ashish Samant <ashish.samant@oracle.com>
>>> Reviewed-by: Liu Bo <bo.li.liu@oracle.com>
>>> ---
>>>   cmds-inspect.c |   66 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>   1 files changed, 66 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/cmds-inspect.c b/cmds-inspect.c
>>> index 4b7cea0..f435ea9 100644
>>> --- a/cmds-inspect.c
>>> +++ b/cmds-inspect.c
>>> @@ -81,6 +81,72 @@ out:
>>>   	return !!ret;
>>>   }
>>>   
>>> +static void bg_flags_to_str(u64 flags, char *ret)
>>> +{
>>> +	int empty = 1;
>>> +
>>> +	if (flags & BTRFS_BLOCK_GROUP_DATA) {
>>> +		empty = 0;
>>> +		strcpy(ret, "DATA");
>>> +	}
>>> +	if (flags & BTRFS_BLOCK_GROUP_METADATA) {
>>> +		if (!empty)
>>> +			strcat(ret, "|");
>>> +		strcat(ret, "METADATA");
>>> +	}
>>> +	if (flags & BTRFS_BLOCK_GROUP_SYSTEM) {
>>> +		if (!empty)
>>> +			strcat(ret, "|");
>>> +		strcat(ret, "SYSTEM");
>>> +	}
>>> +}
>>> +
>>> +/* Walking through the chunk tree to retrieve chunks. */
>> No empty newline.
>>
>>> +
>>> +static int get_chunks(int fd, struct btrfs_ioctl_search_args *chunk_args)
>>> +{
>>> +	struct btrfs_ioctl_search_key *sk;
>>> +	int ret;
>>> +	int e;
>>> +
>>> +	sk = &chunk_args->key;
>>> +
>>> +	sk->tree_id = BTRFS_CHUNK_TREE_OBJECTID;
>>> +	sk->min_objectid = sk->max_objectid = BTRFS_FIRST_CHUNK_TREE_OBJECTID;
>>> +	sk->max_type = sk->min_type = BTRFS_CHUNK_ITEM_KEY;
>> Please don't do multiple asignments in one statement.
>>
>>> +	sk->nr_items = 4096;
>>> +
>>> +	ret = ioctl(fd, BTRFS_IOC_TREE_SEARCH, chunk_args);
>>> +	e = errno;
>> This is useless asignment, I've removed it from the code, please don't
>> reintrduce it.
>>
>>> +	if (ret < 0) {
>>> +		fprintf(stderr, "ret %d error '%s'\n", ret,
>>> +				strerror(e));
>>> +	}
>>> +	return ret;
>>> +}
>>
>>> +
>>> +/* Given the objectid, find the block group item in the extent tree */
>>> +static int get_bg_info(int fd, struct btrfs_ioctl_search_args *bg_args,
>>> +		       u64 objectid, unsigned long length)
>>> +{
>>> +	struct btrfs_ioctl_search_key *bg_sk;
>>> +	int ret;
>>> +	int e;
>>> +
>>> +	bg_sk = &bg_args->key;
>>> +
>>> +	bg_sk->min_objectid = bg_sk->max_objectid = objectid;
>>> +	bg_sk->nr_items = 1;
>>> +	bg_sk->min_offset = bg_sk->max_offset = length;
>> Same here.
>>
>>> +
>>> +	ret = ioctl(fd, BTRFS_IOC_TREE_SEARCH, bg_args);
>>> +	e = errno;
>>> +	if (ret < 0) {
>>> +		fprintf(stderr, "ret %d error '%s'\n", ret,
>>> +				strerror(e));
>> Please take a look how the error messages are constructed when the tree
>> search ioctl fails, there are enough examples in the code.
>>
>>> +	}
>>> +	return ret;
>>> +}
>>>   static const char * const cmd_inspect_inode_resolve_usage[] = {
>>>   	"btrfs inspect-internal inode-resolve [-v] <inode> <path>",
>>>   	"Get file system paths for the given inode",
>> Actually, I'm not sure if such functions should exist at all, as they
>> only hide the search ioctl but don't do any validation of the returned
>> keys and data.
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
>