[PATCH 0/8] btrfs-progs: qgroups usability [corrected]

[PATCH 0/8] btrfs-progs: qgroups usability [corrected]

[jeffm]

From: Jeff Mahoney <jeffm@suse.com>

Hi all -

The following series addresses some usability issues with the qgroups UI.

1) Adds -W option so we can wait on a rescan completing without starting one.
2) Adds qgroup information to 'btrfs subvolume show'
3) Adds a -P option to show pathnames for first-level qgroups (or member
   of nested qgroups with -v)
4) Allows exporting the qgroup table in JSON format for use by external
   programs/scripts.

-Jeff

Jeff Mahoney (8):
  btrfs-progs: quota: Add -W option to rescan to wait without starting
    rescan
  btrfs-progs: qgroups: fix misleading index check
  btrfs-progs: constify pathnames passed as arguments
  btrfs-progs: qgroups: add pathname to show output
  btrfs-progs: qgroups: introduce and use info and limit structures
  btrfs-progs: qgroups: introduce btrfs_qgroup_query
  btrfs-progs: subvolume: add quota info to btrfs sub show
  btrfs-progs: qgroups: export qgroups usage information as JSON

 Documentation/btrfs-qgroup.asciidoc |   8 +
 Documentation/btrfs-quota.asciidoc  |  10 +-
 Makefile.inc.in                     |   4 +-
 chunk-recover.c                     |   4 +-
 cmds-device.c                       |   2 +-
 cmds-fi-usage.c                     |   6 +-
 cmds-qgroup.c                       |  49 +++-
 cmds-quota.c                        |  21 +-
 cmds-rescue.c                       |   4 +-
 cmds-subvolume.c                    |  46 ++++
 configure.ac                        |   6 +
 kerncompat.h                        |   1 +
 qgroup.c                            | 526 ++++++++++++++++++++++++++++++------
 qgroup.h                            |  22 +-
 send-utils.c                        |   4 +-
 utils.c                             |  22 +-
 utils.h                             |   2 +
 17 files changed, 621 insertions(+), 116 deletions(-)

-- 
2.15.1

[PATCH 1/8] btrfs-progs: quota: Add -W option to rescan to wait without starting rescan

[jeffm]

From: Jeff Mahoney <jeffm@suse.com>

This patch adds a new -W option to wait for a rescan without starting a
new operation.  This is useful for things like xfstests where we want
do to do a "btrfs quota enable" and not continue until the subsequent
rescan has finished.

In addition to documenting the new option in the man page, I've cleaned
up the rescan entry to document the -w option a bit better.

Signed-off-by: Jeff Mahoney <jeffm@suse.com>
---
 Documentation/btrfs-quota.asciidoc | 10 +++++++---
 cmds-quota.c                       | 21 +++++++++++++++------
 2 files changed, 22 insertions(+), 9 deletions(-)

diff --git a/Documentation/btrfs-quota.asciidoc b/Documentation/btrfs-quota.asciidoc
index 85ebf729..0b64a69b 100644
--- a/Documentation/btrfs-quota.asciidoc
+++ b/Documentation/btrfs-quota.asciidoc
@@ -238,15 +238,19 @@ Disable subvolume quota support for a filesystem.
 *enable* <path>::
 Enable subvolume quota support for a filesystem.
 
-*rescan* [-s] <path>::
+*rescan* [-s|-w|-W] <path>::
 Trash all qgroup numbers and scan the metadata again with the current config.
 +
 `Options`
 +
 -s::::
-show status of a running rescan operation.
+Show status of a running rescan operation.
+
 -w::::
-wait for rescan operation to finish(can be already in progress).
+Start rescan operation and wait until it has finished before exiting.  If a rescan is already running, wait until it finishes and then exit without starting a new one.
+
+-W::::
+Wait for rescan operation to finish and then exit.  If a rescan is not already running, exit silently.
 
 EXIT STATUS
 -----------
diff --git a/cmds-quota.c b/cmds-quota.c
index 745889d1..fe6376ac 100644
--- a/cmds-quota.c
+++ b/cmds-quota.c
@@ -120,14 +120,20 @@ static int cmd_quota_rescan(int argc, char **argv)
 	int wait_for_completion = 0;
 
 	while (1) {
-		int c = getopt(argc, argv, "sw");
+		int c = getopt(argc, argv, "swW");
 		if (c < 0)
 			break;
 		switch (c) {
 		case 's':
 			ioctlnum = BTRFS_IOC_QUOTA_RESCAN_STATUS;
 			break;
+		case 'W':
+			ioctlnum = 0;
+			wait_for_completion = 1;
+			break;
 		case 'w':
+			/* Reset it in case the user did both -W and -w */
+			ioctlnum = BTRFS_IOC_QUOTA_RESCAN;
 			wait_for_completion = 1;
 			break;
 		default:
@@ -135,8 +141,9 @@ static int cmd_quota_rescan(int argc, char **argv)
 		}
 	}
 
-	if (ioctlnum != BTRFS_IOC_QUOTA_RESCAN && wait_for_completion) {
-		error("switch -w cannot be used with -s");
+	if (ioctlnum == BTRFS_IOC_QUOTA_RESCAN_STATUS && wait_for_completion) {
+		error("switch -%c cannot be used with -s",
+		      ioctlnum ? 'w' : 'W');
 		return 1;
 	}
 
@@ -150,8 +157,10 @@ static int cmd_quota_rescan(int argc, char **argv)
 	if (fd < 0)
 		return 1;
 
-	ret = ioctl(fd, ioctlnum, &args);
-	e = errno;
+	if (ioctlnum) {
+		ret = ioctl(fd, ioctlnum, &args);
+		e = errno;
+	}
 
 	if (ioctlnum == BTRFS_IOC_QUOTA_RESCAN_STATUS) {
 		close_file_or_dir(fd, dirstream);
@@ -167,7 +176,7 @@ static int cmd_quota_rescan(int argc, char **argv)
 		return 0;
 	}
 
-	if (ret == 0) {
+	if (ioctlnum == BTRFS_IOC_QUOTA_RESCAN && ret == 0) {
 		printf("quota rescan started\n");
 		fflush(stdout);
 	} else if (ret < 0 && (!wait_for_completion || e != EINPROGRESS)) {
-- 
2.15.1

Re: [PATCH 1/8] btrfs-progs: quota: Add -W option to rescan to wait without starting rescan

[Nikolay Borisov]

On  2.03.2018 20:46, jeffm@suse.com wrote:
> From: Jeff Mahoney <jeffm@suse.com>
> 
> This patch adds a new -W option to wait for a rescan without starting a
> new operation.  This is useful for things like xfstests where we want
> do to do a "btrfs quota enable" and not continue until the subsequent
> rescan has finished.
> 
> In addition to documenting the new option in the man page, I've cleaned
> up the rescan entry to document the -w option a bit better.
> 
> Signed-off-by: Jeff Mahoney <jeffm@suse.com>
> ---
>  Documentation/btrfs-quota.asciidoc | 10 +++++++---
>  cmds-quota.c                       | 21 +++++++++++++++------
>  2 files changed, 22 insertions(+), 9 deletions(-)
> 
> diff --git a/Documentation/btrfs-quota.asciidoc b/Documentation/btrfs-quota.asciidoc
> index 85ebf729..0b64a69b 100644
> --- a/Documentation/btrfs-quota.asciidoc
> +++ b/Documentation/btrfs-quota.asciidoc
> @@ -238,15 +238,19 @@ Disable subvolume quota support for a filesystem.
>  *enable* <path>::
>  Enable subvolume quota support for a filesystem.
>  
> -*rescan* [-s] <path>::
> +*rescan* [-s|-w|-W] <path>::
>  Trash all qgroup numbers and scan the metadata again with the current config.
>  +
>  `Options`
>  +
>  -s::::
> -show status of a running rescan operation.
> +Show status of a running rescan operation.
> +
>  -w::::
> -wait for rescan operation to finish(can be already in progress).
> +Start rescan operation and wait until it has finished before exiting.  If a rescan is already running, wait until it finishes and then exit without starting a new one.
> +
> +-W::::
> +Wait for rescan operation to finish and then exit.  If a rescan is not already running, exit silently.
>  
>  EXIT STATUS
>  -----------
> diff --git a/cmds-quota.c b/cmds-quota.c
> index 745889d1..fe6376ac 100644
> --- a/cmds-quota.c
> +++ b/cmds-quota.c
> @@ -120,14 +120,20 @@ static int cmd_quota_rescan(int argc, char **argv)
>  	int wait_for_completion = 0;
>  
>  	while (1) {
> -		int c = getopt(argc, argv, "sw");
> +		int c = getopt(argc, argv, "swW");
>  		if (c < 0)
>  			break;
>  		switch (c) {
>  		case 's':
>  			ioctlnum = BTRFS_IOC_QUOTA_RESCAN_STATUS;
>  			break;
> +		case 'W':
> +			ioctlnum = 0;
> +			wait_for_completion = 1;
> +			break;
>  		case 'w':
> +			/* Reset it in case the user did both -W and -w */
> +			ioctlnum = BTRFS_IOC_QUOTA_RESCAN;
>  			wait_for_completion = 1;
>  			break;
>  		default:
> @@ -135,8 +141,9 @@ static int cmd_quota_rescan(int argc, char **argv)
>  		}
>  	}
>  
> -	if (ioctlnum != BTRFS_IOC_QUOTA_RESCAN && wait_for_completion) {
> -		error("switch -w cannot be used with -s");
> +	if (ioctlnum == BTRFS_IOC_QUOTA_RESCAN_STATUS && wait_for_completion) {
> +		error("switch -%c cannot be used with -s",
> +		      ioctlnum ? 'w' : 'W');

You can't really distinguish between w/W in this context, since ioctlnum
will be RESCAN_STATUS. So just harcode the w/W in the text message itself?

>  		return 1;
>  	}
>  
> @@ -150,8 +157,10 @@ static int cmd_quota_rescan(int argc, char **argv)
>  	if (fd < 0)
>  		return 1;
>  
> -	ret = ioctl(fd, ioctlnum, &args);
> -	e = errno;
> +	if (ioctlnum) {
> +		ret = ioctl(fd, ioctlnum, &args);
> +		e = errno;
> +	}
>  
>  	if (ioctlnum == BTRFS_IOC_QUOTA_RESCAN_STATUS) {
>  		close_file_or_dir(fd, dirstream);
> @@ -167,7 +176,7 @@ static int cmd_quota_rescan(int argc, char **argv)
>  		return 0;
>  	}
>  
> -	if (ret == 0) {
> +	if (ioctlnum == BTRFS_IOC_QUOTA_RESCAN && ret == 0) {
>  		printf("quota rescan started\n");
>  		fflush(stdout);
>  	} else if (ret < 0 && (!wait_for_completion || e != EINPROGRESS)) {
> 

Re: [PATCH 1/8] btrfs-progs: quota: Add -W option to rescan to wait without starting rescan

[Jeff Mahoney]

[-- Attachment #1.1: Type: text/plain, Size: 747 bytes --]

On 3/2/18 1:59 PM, Nikolay Borisov wrote:
> 
> 
> On  2.03.2018 20:46, jeffm@suse.com wrote:
>> From: Jeff Mahoney <jeffm@suse.com>
>> @@ -135,8 +141,9 @@ static int cmd_quota_rescan(int argc, char **argv)
>>  		}
>>  	}
>>  
>> -	if (ioctlnum != BTRFS_IOC_QUOTA_RESCAN && wait_for_completion) {
>> -		error("switch -w cannot be used with -s");
>> +	if (ioctlnum == BTRFS_IOC_QUOTA_RESCAN_STATUS && wait_for_completion) {
>> +		error("switch -%c cannot be used with -s",
>> +		      ioctlnum ? 'w' : 'W');
> 
> You can't really distinguish between w/W in this context, since ioctlnum
> will be RESCAN_STATUS. So just harcode the w/W in the text message itself?

Yep.  Derp.

Thanks,

-Jeff

-- 
Jeff Mahoney
SUSE Labs


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 2/8] btrfs-progs: qgroups: fix misleading index check

[jeffm]

From: Jeff Mahoney <jeffm@suse.com>

In print_single_qgroup_table we check the loop index against
BTRFS_QGROUP_CHILD, but what we really mean is "last column."  Since
we have an enum value to indicate the last value, use that instead
of assuming that BTRFS_QGROUP_CHILD is always last.

Signed-off-by: Jeff Mahoney <jeffm@suse.com>
---
 qgroup.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/qgroup.c b/qgroup.c
index 11659e83..67bc0738 100644
--- a/qgroup.c
+++ b/qgroup.c
@@ -267,7 +267,7 @@ static void print_single_qgroup_table(struct btrfs_qgroup *qgroup)
 			continue;
 		print_qgroup_column(qgroup, i);
 
-		if (i != BTRFS_QGROUP_CHILD)
+		if (i != BTRFS_QGROUP_ALL - 1)
 			printf(" ");
 	}
 	printf("\n");
-- 
2.15.1

Re: [PATCH 2/8] btrfs-progs: qgroups: fix misleading index check

[Nikolay Borisov]

On  2.03.2018 20:46, jeffm@suse.com wrote:
> From: Jeff Mahoney <jeffm@suse.com>
> 
> In print_single_qgroup_table we check the loop index against
> BTRFS_QGROUP_CHILD, but what we really mean is "last column."  Since
> we have an enum value to indicate the last value, use that instead
> of assuming that BTRFS_QGROUP_CHILD is always last.
> 
> Signed-off-by: Jeff Mahoney <jeffm@suse.com>

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

> ---
>  qgroup.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/qgroup.c b/qgroup.c
> index 11659e83..67bc0738 100644
> --- a/qgroup.c
> +++ b/qgroup.c
> @@ -267,7 +267,7 @@ static void print_single_qgroup_table(struct btrfs_qgroup *qgroup)
>  			continue;
>  		print_qgroup_column(qgroup, i);
>  
> -		if (i != BTRFS_QGROUP_CHILD)
> +		if (i != BTRFS_QGROUP_ALL - 1)
>  			printf(" ");
>  	}
>  	printf("\n");
> 

[PATCH 3/8] btrfs-progs: constify pathnames passed as arguments

[jeffm]

From: Jeff Mahoney <jeffm@suse.com>

It's unlikely we're going to modify a pathname argument, so codify that
and use const.

Signed-off-by: Jeff Mahoney <jeffm@suse.com>
---
 chunk-recover.c | 4 ++--
 cmds-device.c   | 2 +-
 cmds-fi-usage.c | 6 +++---
 cmds-rescue.c   | 4 ++--
 send-utils.c    | 4 ++--
 5 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/chunk-recover.c b/chunk-recover.c
index 705bcf52..1d30db51 100644
--- a/chunk-recover.c
+++ b/chunk-recover.c
@@ -1492,7 +1492,7 @@ out:
 	return ERR_PTR(ret);
 }
 
-static int recover_prepare(struct recover_control *rc, char *path)
+static int recover_prepare(struct recover_control *rc, const char *path)
 {
 	int ret;
 	int fd;
@@ -2296,7 +2296,7 @@ static void validate_rebuild_chunks(struct recover_control *rc)
 /*
  * Return 0 when successful, < 0 on error and > 0 if aborted by user
  */
-int btrfs_recover_chunk_tree(char *path, int verbose, int yes)
+int btrfs_recover_chunk_tree(const char *path, int verbose, int yes)
 {
 	int ret = 0;
 	struct btrfs_root *root = NULL;
diff --git a/cmds-device.c b/cmds-device.c
index 86459d1b..a49c9d9d 100644
--- a/cmds-device.c
+++ b/cmds-device.c
@@ -526,7 +526,7 @@ static const char * const cmd_device_usage_usage[] = {
 	NULL
 };
 
-static int _cmd_device_usage(int fd, char *path, unsigned unit_mode)
+static int _cmd_device_usage(int fd, const char *path, unsigned unit_mode)
 {
 	int i;
 	int ret = 0;
diff --git a/cmds-fi-usage.c b/cmds-fi-usage.c
index de7ad668..9a1c76ab 100644
--- a/cmds-fi-usage.c
+++ b/cmds-fi-usage.c
@@ -227,7 +227,7 @@ static int cmp_btrfs_ioctl_space_info(const void *a, const void *b)
 /*
  * This function load all the information about the space usage
  */
-static struct btrfs_ioctl_space_args *load_space_info(int fd, char *path)
+static struct btrfs_ioctl_space_args *load_space_info(int fd, const char *path)
 {
 	struct btrfs_ioctl_space_args *sargs = NULL, *sargs_orig = NULL;
 	int ret, count;
@@ -305,7 +305,7 @@ static void get_raid56_used(struct chunk_info *chunks, int chunkcount,
 #define	MIN_UNALOCATED_THRESH	SZ_16M
 static int print_filesystem_usage_overall(int fd, struct chunk_info *chunkinfo,
 		int chunkcount, struct device_info *devinfo, int devcount,
-		char *path, unsigned unit_mode)
+		const char *path, unsigned unit_mode)
 {
 	struct btrfs_ioctl_space_args *sargs = NULL;
 	int i;
@@ -931,7 +931,7 @@ static void _cmd_filesystem_usage_linear(unsigned unit_mode,
 static int print_filesystem_usage_by_chunk(int fd,
 		struct chunk_info *chunkinfo, int chunkcount,
 		struct device_info *devinfo, int devcount,
-		char *path, unsigned unit_mode, int tabular)
+		const char *path, unsigned unit_mode, int tabular)
 {
 	struct btrfs_ioctl_space_args *sargs;
 	int ret = 0;
diff --git a/cmds-rescue.c b/cmds-rescue.c
index c40088ad..c61145bc 100644
--- a/cmds-rescue.c
+++ b/cmds-rescue.c
@@ -32,8 +32,8 @@ static const char * const rescue_cmd_group_usage[] = {
 	NULL
 };
 
-int btrfs_recover_chunk_tree(char *path, int verbose, int yes);
-int btrfs_recover_superblocks(char *path, int verbose, int yes);
+int btrfs_recover_chunk_tree(const char *path, int verbose, int yes);
+int btrfs_recover_superblocks(const char *path, int verbose, int yes);
 
 static const char * const cmd_rescue_chunk_recover_usage[] = {
 	"btrfs rescue chunk-recover [options] <device>",
diff --git a/send-utils.c b/send-utils.c
index b5289e76..8ce94de1 100644
--- a/send-utils.c
+++ b/send-utils.c
@@ -28,8 +28,8 @@
 #include "ioctl.h"
 #include "btrfs-list.h"
 
-static int btrfs_subvolid_resolve_sub(int fd, char *path, size_t *path_len,
-				      u64 subvol_id);
+static int btrfs_subvolid_resolve_sub(int fd, char *path,
+				      size_t *path_len, u64 subvol_id);
 
 static int btrfs_get_root_id_by_sub_path(int mnt_fd, const char *sub_path,
 					 u64 *root_id)
-- 
2.15.1

Re: [PATCH 3/8] btrfs-progs: constify pathnames passed as arguments

[Nikolay Borisov]

On  2.03.2018 20:46, jeffm@suse.com wrote:
> From: Jeff Mahoney <jeffm@suse.com>
> 
> It's unlikely we're going to modify a pathname argument, so codify that
> and use const.
> 
> Signed-off-by: Jeff Mahoney <jeffm@suse.com>
> ---
>  chunk-recover.c | 4 ++--
>  cmds-device.c   | 2 +-
>  cmds-fi-usage.c | 6 +++---
>  cmds-rescue.c   | 4 ++--
>  send-utils.c    | 4 ++--
>  5 files changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/chunk-recover.c b/chunk-recover.c
> index 705bcf52..1d30db51 100644
> --- a/chunk-recover.c
> +++ b/chunk-recover.c
> @@ -1492,7 +1492,7 @@ out:
>  	return ERR_PTR(ret);
>  }
>  
> -static int recover_prepare(struct recover_control *rc, char *path)
> +static int recover_prepare(struct recover_control *rc, const char *path)
>  {
>  	int ret;
>  	int fd;
> @@ -2296,7 +2296,7 @@ static void validate_rebuild_chunks(struct recover_control *rc)
>  /*
>   * Return 0 when successful, < 0 on error and > 0 if aborted by user
>   */
> -int btrfs_recover_chunk_tree(char *path, int verbose, int yes)
> +int btrfs_recover_chunk_tree(const char *path, int verbose, int yes)
>  {
>  	int ret = 0;
>  	struct btrfs_root *root = NULL;
> diff --git a/cmds-device.c b/cmds-device.c
> index 86459d1b..a49c9d9d 100644
> --- a/cmds-device.c
> +++ b/cmds-device.c
> @@ -526,7 +526,7 @@ static const char * const cmd_device_usage_usage[] = {
>  	NULL
>  };
>  
> -static int _cmd_device_usage(int fd, char *path, unsigned unit_mode)
> +static int _cmd_device_usage(int fd, const char *path, unsigned unit_mode)

Actually the path parameter is not used in this function at all, I'd say
just remove it.

>  {
>  	int i;
>  	int ret = 0;> diff --git a/cmds-fi-usage.c b/cmds-fi-usage.c
> index de7ad668..9a1c76ab 100644
> --- a/cmds-fi-usage.c
> +++ b/cmds-fi-usage.c
> @@ -227,7 +227,7 @@ static int cmp_btrfs_ioctl_space_info(const void *a, const void *b)
>  /*
>   * This function load all the information about the space usage
>   */
> -static struct btrfs_ioctl_space_args *load_space_info(int fd, char *path)
> +static struct btrfs_ioctl_space_args *load_space_info(int fd, const char *path)
>  {
>  	struct btrfs_ioctl_space_args *sargs = NULL, *sargs_orig = NULL;
>  	int ret, count;
> @@ -305,7 +305,7 @@ static void get_raid56_used(struct chunk_info *chunks, int chunkcount,
>  #define	MIN_UNALOCATED_THRESH	SZ_16M
>  static int print_filesystem_usage_overall(int fd, struct chunk_info *chunkinfo,
>  		int chunkcount, struct device_info *devinfo, int devcount,
> -		char *path, unsigned unit_mode)
> +		const char *path, unsigned unit_mode)
>  {
>  	struct btrfs_ioctl_space_args *sargs = NULL;
>  	int i;
> @@ -931,7 +931,7 @@ static void _cmd_filesystem_usage_linear(unsigned unit_mode,
>  static int print_filesystem_usage_by_chunk(int fd,
>  		struct chunk_info *chunkinfo, int chunkcount,
>  		struct device_info *devinfo, int devcount,
> -		char *path, unsigned unit_mode, int tabular)
> +		const char *path, unsigned unit_mode, int tabular)
>  {
>  	struct btrfs_ioctl_space_args *sargs;
>  	int ret = 0;
> diff --git a/cmds-rescue.c b/cmds-rescue.c
> index c40088ad..c61145bc 100644
> --- a/cmds-rescue.c
> +++ b/cmds-rescue.c
> @@ -32,8 +32,8 @@ static const char * const rescue_cmd_group_usage[] = {
>  	NULL
>  };
>  
> -int btrfs_recover_chunk_tree(char *path, int verbose, int yes);
> -int btrfs_recover_superblocks(char *path, int verbose, int yes);
> +int btrfs_recover_chunk_tree(const char *path, int verbose, int yes);

That path argument is being passed to recover_prepare which can alo use
a const to its path parameter

> +int btrfs_recover_superblocks(const char *path, int verbose, int yes);
>  
>  static const char * const cmd_rescue_chunk_recover_usage[] = {
>  	"btrfs rescue chunk-recover [options] <device>",
> diff --git a/send-utils.c b/send-utils.c
> index b5289e76..8ce94de1 100644
> --- a/send-utils.c
> +++ b/send-utils.c
> @@ -28,8 +28,8 @@
>  #include "ioctl.h"
>  #include "btrfs-list.h"
>  
> -static int btrfs_subvolid_resolve_sub(int fd, char *path, size_t *path_len,
> -				      u64 subvol_id);
> +static int btrfs_subvolid_resolve_sub(int fd, char *path,
> +				      size_t *path_len, u64 subvol_id);

This seems like an unrelated change. As a matter of fact
btrfs_subvolid_resolve_sub is used only by btrfs_subvolid_resolve. So if
you move the latter after the former then you can drop the declaration
at the beginning of the file altogether.

>  
>  static int btrfs_get_root_id_by_sub_path(int mnt_fd, const char *sub_path,
>  					 u64 *root_id)
> 

Re: [PATCH 3/8] btrfs-progs: constify pathnames passed as arguments

[Jeff Mahoney]

[-- Attachment #1.1: Type: text/plain, Size: 5020 bytes --]

On 3/7/18 3:17 AM, Nikolay Borisov wrote:
> 
> 
> On  2.03.2018 20:46, jeffm@suse.com wrote:
>> From: Jeff Mahoney <jeffm@suse.com>
>>
>> It's unlikely we're going to modify a pathname argument, so codify that
>> and use const.
>>
>> Signed-off-by: Jeff Mahoney <jeffm@suse.com>
>> ---
>>  chunk-recover.c | 4 ++--
>>  cmds-device.c   | 2 +-
>>  cmds-fi-usage.c | 6 +++---
>>  cmds-rescue.c   | 4 ++--
>>  send-utils.c    | 4 ++--
>>  5 files changed, 10 insertions(+), 10 deletions(-)
>>
>> diff --git a/chunk-recover.c b/chunk-recover.c
>> index 705bcf52..1d30db51 100644
>> --- a/chunk-recover.c
>> +++ b/chunk-recover.c
>> @@ -1492,7 +1492,7 @@ out:
>>  	return ERR_PTR(ret);
>>  }
>>  
>> -static int recover_prepare(struct recover_control *rc, char *path)
>> +static int recover_prepare(struct recover_control *rc, const char *path)
>>  {
>>  	int ret;
>>  	int fd;
>> @@ -2296,7 +2296,7 @@ static void validate_rebuild_chunks(struct recover_control *rc)
>>  /*
>>   * Return 0 when successful, < 0 on error and > 0 if aborted by user
>>   */
>> -int btrfs_recover_chunk_tree(char *path, int verbose, int yes)
>> +int btrfs_recover_chunk_tree(const char *path, int verbose, int yes)
>>  {
>>  	int ret = 0;
>>  	struct btrfs_root *root = NULL;
>> diff --git a/cmds-device.c b/cmds-device.c
>> index 86459d1b..a49c9d9d 100644
>> --- a/cmds-device.c
>> +++ b/cmds-device.c
>> @@ -526,7 +526,7 @@ static const char * const cmd_device_usage_usage[] = {
>>  	NULL
>>  };
>>  
>> -static int _cmd_device_usage(int fd, char *path, unsigned unit_mode)
>> +static int _cmd_device_usage(int fd, const char *path, unsigned unit_mode)
> 
> Actually the path parameter is not used in this function at all, I'd say
> just remove it.

Yep, it's unused, but that's a different project.  Add
-Wunused-parameter and see what shakes out. :)

>>  {
>>  	int i;
>>  	int ret = 0;> diff --git a/cmds-fi-usage.c b/cmds-fi-usage.c
>> index de7ad668..9a1c76ab 100644
>> --- a/cmds-fi-usage.c
>> +++ b/cmds-fi-usage.c
>> @@ -227,7 +227,7 @@ static int cmp_btrfs_ioctl_space_info(const void *a, const void *b)
>>  /*
>>   * This function load all the information about the space usage
>>   */
>> -static struct btrfs_ioctl_space_args *load_space_info(int fd, char *path)
>> +static struct btrfs_ioctl_space_args *load_space_info(int fd, const char *path)
>>  {
>>  	struct btrfs_ioctl_space_args *sargs = NULL, *sargs_orig = NULL;
>>  	int ret, count;
>> @@ -305,7 +305,7 @@ static void get_raid56_used(struct chunk_info *chunks, int chunkcount,
>>  #define	MIN_UNALOCATED_THRESH	SZ_16M
>>  static int print_filesystem_usage_overall(int fd, struct chunk_info *chunkinfo,
>>  		int chunkcount, struct device_info *devinfo, int devcount,
>> -		char *path, unsigned unit_mode)
>> +		const char *path, unsigned unit_mode)
>>  {
>>  	struct btrfs_ioctl_space_args *sargs = NULL;
>>  	int i;
>> @@ -931,7 +931,7 @@ static void _cmd_filesystem_usage_linear(unsigned unit_mode,
>>  static int print_filesystem_usage_by_chunk(int fd,
>>  		struct chunk_info *chunkinfo, int chunkcount,
>>  		struct device_info *devinfo, int devcount,
>> -		char *path, unsigned unit_mode, int tabular)
>> +		const char *path, unsigned unit_mode, int tabular)
>>  {
>>  	struct btrfs_ioctl_space_args *sargs;
>>  	int ret = 0;
>> diff --git a/cmds-rescue.c b/cmds-rescue.c
>> index c40088ad..c61145bc 100644
>> --- a/cmds-rescue.c
>> +++ b/cmds-rescue.c
>> @@ -32,8 +32,8 @@ static const char * const rescue_cmd_group_usage[] = {
>>  	NULL
>>  };
>>  
>> -int btrfs_recover_chunk_tree(char *path, int verbose, int yes);
>> -int btrfs_recover_superblocks(char *path, int verbose, int yes);
>> +int btrfs_recover_chunk_tree(const char *path, int verbose, int yes);
> 
> That path argument is being passed to recover_prepare which can alo use
> a const to its path parameter

Yep, and it was in the first chunk.

>> +int btrfs_recover_superblocks(const char *path, int verbose, int yes);
>>  
>>  static const char * const cmd_rescue_chunk_recover_usage[] = {
>>  	"btrfs rescue chunk-recover [options] <device>",
>> diff --git a/send-utils.c b/send-utils.c
>> index b5289e76..8ce94de1 100644
>> --- a/send-utils.c
>> +++ b/send-utils.c
>> @@ -28,8 +28,8 @@
>>  #include "ioctl.h"
>>  #include "btrfs-list.h"
>>  
>> -static int btrfs_subvolid_resolve_sub(int fd, char *path, size_t *path_len,
>> -				      u64 subvol_id);
>> +static int btrfs_subvolid_resolve_sub(int fd, char *path,
>> +				      size_t *path_len, u64 subvol_id);
> 
> This seems like an unrelated change. As a matter of fact
> btrfs_subvolid_resolve_sub is used only by btrfs_subvolid_resolve. So if
> you move the latter after the former then you can drop the declaration
> at the beginning of the file altogether.

Ah, yep.  That's the fallout from adding const, reformatting, and
removing it.  I'll just skip it entirely.

-Jeff

-- 
Jeff Mahoney
SUSE Labs


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 854 bytes --]

[PATCH 4/8] btrfs-progs: qgroups: add pathname to show output

[jeffm]

From: Jeff Mahoney <jeffm@suse.com>

The btrfs qgroup show command currently only exports qgroup IDs,
forcing the user to resolve which subvolume each corresponds to.

This patch adds pathname resolution to qgroup show so that when
the -P option is used, the last column contains the pathname of
the root of the subvolume it describes.  In the case of nested
qgroups, it will show the number of member qgroups or the paths
of the members if the -v option is used.

Pathname can also be used as a sort parameter.

Signed-off-by: Jeff Mahoney <jeffm@suse.com>
---
 Documentation/btrfs-qgroup.asciidoc |   4 +
 cmds-qgroup.c                       |  17 ++++-
 kerncompat.h                        |   1 +
 qgroup.c                            | 142 ++++++++++++++++++++++++++++++++----
 qgroup.h                            |   4 +-
 utils.c                             |  22 ++++--
 utils.h                             |   2 +
 7 files changed, 166 insertions(+), 26 deletions(-)

diff --git a/Documentation/btrfs-qgroup.asciidoc b/Documentation/btrfs-qgroup.asciidoc
index 3108457c..360b3269 100644
--- a/Documentation/btrfs-qgroup.asciidoc
+++ b/Documentation/btrfs-qgroup.asciidoc
@@ -97,10 +97,14 @@ print child qgroup id.
 print limit of referenced size of qgroup.
 -e::::
 print limit of exclusive size of qgroup.
+-P::::
+print pathname to the root of the subvolume managed by qgroup.  For nested qgroups, the number of members will be printed unless -v is specified.
 -F::::
 list all qgroups which impact the given path(include ancestral qgroups)
 -f::::
 list all qgroups which impact the given path(exclude ancestral qgroups)
+-v::::
+Be more verbose.  Print pathnames of member qgroups when nested.
 --raw::::
 raw numbers in bytes, without the 'B' suffix.
 --human-readable::::
diff --git a/cmds-qgroup.c b/cmds-qgroup.c
index 48686436..94cd0fd3 100644
--- a/cmds-qgroup.c
+++ b/cmds-qgroup.c
@@ -280,8 +280,10 @@ static const char * const cmd_qgroup_show_usage[] = {
 	"               (including ancestral qgroups)",
 	"-f             list all qgroups which impact the given path",
 	"               (excluding ancestral qgroups)",
+	"-P             print first-level qgroups using pathname",
+	"-v             verbose, prints all nested subvolumes",
 	HELPINFO_UNITS_LONG,
-	"--sort=qgroupid,rfer,excl,max_rfer,max_excl",
+	"--sort=qgroupid,rfer,excl,max_rfer,max_excl,pathname",
 	"               list qgroups sorted by specified items",
 	"               you can use '+' or '-' in front of each item.",
 	"               (+:ascending, -:descending, ascending default)",
@@ -299,6 +301,7 @@ static int cmd_qgroup_show(int argc, char **argv)
 	int filter_flag = 0;
 	unsigned unit_mode;
 	int sync = 0;
+	bool verbose = false;
 
 	struct btrfs_qgroup_comparer_set *comparer_set;
 	struct btrfs_qgroup_filter_set *filter_set;
@@ -316,10 +319,11 @@ static int cmd_qgroup_show(int argc, char **argv)
 		static const struct option long_options[] = {
 			{"sort", required_argument, NULL, GETOPT_VAL_SORT},
 			{"sync", no_argument, NULL, GETOPT_VAL_SYNC},
+			{"verbose", no_argument, NULL, 'v'},
 			{ NULL, 0, NULL, 0 }
 		};
 
-		c = getopt_long(argc, argv, "pcreFf", long_options, NULL);
+		c = getopt_long(argc, argv, "pPcreFfv", long_options, NULL);
 		if (c < 0)
 			break;
 		switch (c) {
@@ -327,6 +331,10 @@ static int cmd_qgroup_show(int argc, char **argv)
 			btrfs_qgroup_setup_print_column(
 				BTRFS_QGROUP_PARENT);
 			break;
+		case 'P':
+			btrfs_qgroup_setup_print_column(
+				BTRFS_QGROUP_PATHNAME);
+			break;
 		case 'c':
 			btrfs_qgroup_setup_print_column(
 				BTRFS_QGROUP_CHILD);
@@ -354,6 +362,9 @@ static int cmd_qgroup_show(int argc, char **argv)
 		case GETOPT_VAL_SYNC:
 			sync = 1;
 			break;
+		case 'v':
+			verbose = true;
+			break;
 		default:
 			usage(cmd_qgroup_show_usage);
 		}
@@ -394,7 +405,7 @@ static int cmd_qgroup_show(int argc, char **argv)
 					BTRFS_QGROUP_FILTER_PARENT,
 					qgroupid);
 	}
-	ret = btrfs_show_qgroups(fd, filter_set, comparer_set);
+	ret = btrfs_show_qgroups(fd, filter_set, comparer_set, verbose);
 	close_file_or_dir(fd, dirstream);
 	free(filter_set);
 	free(comparer_set);
diff --git a/kerncompat.h b/kerncompat.h
index fa96715f..f97495ee 100644
--- a/kerncompat.h
+++ b/kerncompat.h
@@ -29,6 +29,7 @@
 #include <stddef.h>
 #include <linux/types.h>
 #include <stdint.h>
+#include <stdbool.h>
 
 #include <features.h>
 
diff --git a/qgroup.c b/qgroup.c
index 67bc0738..83918134 100644
--- a/qgroup.c
+++ b/qgroup.c
@@ -40,6 +40,9 @@ struct btrfs_qgroup {
 	struct rb_node all_parent_node;
 	u64 qgroupid;
 
+	/* NULL for qgroups with level > 0 */
+	const char *pathname;
+
 	/*
 	 * info_item
 	 */
@@ -133,6 +136,13 @@ static struct {
 		.unit_mode	= 0,
 		.max_len	= 5,
 	},
+	{
+		.name		= "pathname",
+		.column_name	= "pathname",
+		.need_print	= 0,
+		.unit_mode	= 0,
+		.max_len	= 10,
+	},
 	{
 		.name		= NULL,
 		.column_name	= NULL,
@@ -210,8 +220,42 @@ static void print_qgroup_column_add_blank(enum btrfs_qgroup_column_enum column,
 		printf(" ");
 }
 
+void print_pathname_column(struct btrfs_qgroup *qgroup, bool verbose)
+{
+	struct btrfs_qgroup_list *list = NULL;
+
+	fputs("  ", stdout);
+	if (btrfs_qgroup_level(qgroup->qgroupid) > 0) {
+		int count = 0;
+		list_for_each_entry(list, &qgroup->qgroups,
+				    next_qgroup) {
+			if (verbose) {
+				struct btrfs_qgroup *member = list->qgroup;
+				u64 level = btrfs_qgroup_level(member->qgroupid);
+				u64 sid = btrfs_qgroup_subvid(member->qgroupid);
+				if (count)
+					fputs(" ", stdout);
+				if (btrfs_qgroup_level(member->qgroupid) == 0)
+					fputs(member->pathname, stdout);
+				else
+					printf("%llu/%llu", level, sid);
+			}
+			count++;
+		}
+		if (!count)
+			fputs("<empty>", stdout);
+		else if (!verbose)
+			printf("<%u member qgroup%c>", count,
+			       count != 1 ? 's' : '\0');
+	} else if (qgroup->pathname)
+		fputs(qgroup->pathname, stdout);
+	else
+		fputs("<missing>", stdout);
+}
+
 static void print_qgroup_column(struct btrfs_qgroup *qgroup,
-				enum btrfs_qgroup_column_enum column)
+				enum btrfs_qgroup_column_enum column,
+				bool verbose)
 {
 	int len;
 	int unit_mode = btrfs_qgroup_columns[column].unit_mode;
@@ -253,19 +297,22 @@ static void print_qgroup_column(struct btrfs_qgroup *qgroup,
 		len = print_child_column(qgroup);
 		print_qgroup_column_add_blank(BTRFS_QGROUP_CHILD, len);
 		break;
+	case BTRFS_QGROUP_PATHNAME:
+		print_pathname_column(qgroup, verbose);
+		break;
 	default:
 		break;
 	}
 }
 
-static void print_single_qgroup_table(struct btrfs_qgroup *qgroup)
+static void print_single_qgroup_table(struct btrfs_qgroup *qgroup, bool verbose)
 {
 	int i;
 
 	for (i = 0; i < BTRFS_QGROUP_ALL; i++) {
 		if (!btrfs_qgroup_columns[i].need_print)
 			continue;
-		print_qgroup_column(qgroup, i);
+		print_qgroup_column(qgroup, i, verbose);
 
 		if (i != BTRFS_QGROUP_ALL - 1)
 			printf(" ");
@@ -338,6 +385,47 @@ static int comp_entry_with_qgroupid(struct btrfs_qgroup *entry1,
 	return is_descending ? -ret : ret;
 }
 
+/* Sorts first-level qgroups by pathname and nested qgroups by qgroupid */
+static int comp_entry_with_pathname(struct btrfs_qgroup *entry1,
+				    struct btrfs_qgroup *entry2,
+				    int is_descending)
+{
+	int ret = 0;
+	const char *p1 = entry1->pathname;
+	const char *p2 = entry2->pathname;
+
+	u64 level1 = btrfs_qgroup_level(entry1->qgroupid);
+	u64 level2 = btrfs_qgroup_level(entry2->qgroupid);
+
+	if (level1 != level2) {
+		if (entry1->qgroupid > entry2->qgroupid)
+			ret = 1;
+		else if (entry1->qgroupid < entry2->qgroupid)
+			ret = -1;
+	}
+
+	if (ret)
+		goto out;
+
+	while (*p1 && *p2) {
+		if (*p1 != *p2)
+			break;
+		p1++;
+		p2++;
+	}
+
+	if (*p1 == '/')
+		ret = 1;
+	else if (*p2 == '/')
+	      ret = -1;
+	else if (*p1 > *p2)
+	      ret = 1;
+	else if (*p1 < *p2)
+		ret = -1;
+out:
+	return is_descending ? -ret : ret;
+}
+
 static int comp_entry_with_rfer(struct btrfs_qgroup *entry1,
 				struct btrfs_qgroup *entry2,
 				int is_descending)
@@ -404,6 +492,7 @@ static int comp_entry_with_max_excl(struct btrfs_qgroup *entry1,
 
 static btrfs_qgroup_comp_func all_comp_funcs[] = {
 	[BTRFS_QGROUP_COMP_QGROUPID]	= comp_entry_with_qgroupid,
+	[BTRFS_QGROUP_COMP_PATHNAME]	= comp_entry_with_pathname,
 	[BTRFS_QGROUP_COMP_RFER]	= comp_entry_with_rfer,
 	[BTRFS_QGROUP_COMP_EXCL]	= comp_entry_with_excl,
 	[BTRFS_QGROUP_COMP_MAX_RFER]	= comp_entry_with_max_rfer,
@@ -412,6 +501,7 @@ static btrfs_qgroup_comp_func all_comp_funcs[] = {
 
 static char *all_sort_items[] = {
 	[BTRFS_QGROUP_COMP_QGROUPID]	= "qgroupid",
+	[BTRFS_QGROUP_COMP_PATHNAME]	= "pathname",
 	[BTRFS_QGROUP_COMP_RFER]	= "rfer",
 	[BTRFS_QGROUP_COMP_EXCL]	= "excl",
 	[BTRFS_QGROUP_COMP_MAX_RFER]	= "max_rfer",
@@ -578,6 +668,25 @@ static struct btrfs_qgroup *qgroup_tree_search(struct qgroup_lookup *root_tree,
 	return NULL;
 }
 
+static const char *qgroup_pathname(int fd, u64 qgroupid)
+{
+	struct root_info root_info;
+	int ret;
+	char *pathname;
+
+	ret = get_subvol_info_by_rootid_fd(fd, &root_info, qgroupid);
+	if (ret)
+		return NULL;
+
+	ret = asprintf(&pathname, "%s%s",
+		       root_info.full_path[0] == '/' ? "" : "/",
+		       root_info.full_path);
+	if (ret < 0)
+		return NULL;
+
+	return pathname;
+}
+
 /*
  * Lookup or insert btrfs_qgroup into qgroup_lookup.
  *
@@ -588,7 +697,7 @@ static struct btrfs_qgroup *qgroup_tree_search(struct qgroup_lookup *root_tree,
  * Return the pointer to the btrfs_qgroup if found or if inserted successfully.
  * Return ERR_PTR if any error occurred.
  */
-static struct btrfs_qgroup *get_or_add_qgroup(
+static struct btrfs_qgroup *get_or_add_qgroup(int fd,
 		struct qgroup_lookup *qgroup_lookup, u64 qgroupid)
 {
 	struct btrfs_qgroup *bq;
@@ -608,6 +717,8 @@ static struct btrfs_qgroup *get_or_add_qgroup(
 	INIT_LIST_HEAD(&bq->qgroups);
 	INIT_LIST_HEAD(&bq->members);
 
+	bq->pathname = qgroup_pathname(fd, qgroupid);
+
 	ret = qgroup_tree_insert(qgroup_lookup, bq);
 	if (ret) {
 		error("failed to insert %llu into tree: %s",
@@ -619,12 +730,12 @@ static struct btrfs_qgroup *get_or_add_qgroup(
 	return bq;
 }
 
-static int update_qgroup_info(struct qgroup_lookup *qgroup_lookup, u64 qgroupid,
-			      struct btrfs_qgroup_info_item *info)
+static int update_qgroup_info(int fd, struct qgroup_lookup *qgroup_lookup,
+			      u64 qgroupid, struct btrfs_qgroup_info_item *info)
 {
 	struct btrfs_qgroup *bq;
 
-	bq = get_or_add_qgroup(qgroup_lookup, qgroupid);
+	bq = get_or_add_qgroup(fd, qgroup_lookup, qgroupid);
 	if (IS_ERR_OR_NULL(bq))
 		return PTR_ERR(bq);
 
@@ -637,13 +748,13 @@ static int update_qgroup_info(struct qgroup_lookup *qgroup_lookup, u64 qgroupid,
 	return 0;
 }
 
-static int update_qgroup_limit(struct qgroup_lookup *qgroup_lookup,
+static int update_qgroup_limit(int fd, struct qgroup_lookup *qgroup_lookup,
 			       u64 qgroupid,
 			       struct btrfs_qgroup_limit_item *limit)
 {
 	struct btrfs_qgroup *bq;
 
-	bq = get_or_add_qgroup(qgroup_lookup, qgroupid);
+	bq = get_or_add_qgroup(fd, qgroup_lookup, qgroupid);
 	if (IS_ERR_OR_NULL(bq))
 		return PTR_ERR(bq);
 
@@ -1107,7 +1218,7 @@ static int __qgroups_search(int fd, struct qgroup_lookup *qgroup_lookup)
 				info = (struct btrfs_qgroup_info_item *)
 				       (args.buf + off);
 
-				ret = update_qgroup_info(qgroup_lookup,
+				ret = update_qgroup_info(fd, qgroup_lookup,
 							 qgroupid, info);
 				break;
 			case BTRFS_QGROUP_LIMIT_KEY:
@@ -1115,7 +1226,7 @@ static int __qgroups_search(int fd, struct qgroup_lookup *qgroup_lookup)
 				limit = (struct btrfs_qgroup_limit_item *)
 					(args.buf + off);
 
-				ret = update_qgroup_limit(qgroup_lookup,
+				ret = update_qgroup_limit(fd, qgroup_lookup,
 							  qgroupid, limit);
 				break;
 			case BTRFS_QGROUP_RELATION_KEY:
@@ -1159,7 +1270,7 @@ static int __qgroups_search(int fd, struct qgroup_lookup *qgroup_lookup)
 	return ret;
 }
 
-static void print_all_qgroups(struct qgroup_lookup *qgroup_lookup)
+static void print_all_qgroups(struct qgroup_lookup *qgroup_lookup, bool verbose)
 {
 
 	struct rb_node *n;
@@ -1170,14 +1281,15 @@ static void print_all_qgroups(struct qgroup_lookup *qgroup_lookup)
 	n = rb_first(&qgroup_lookup->root);
 	while (n) {
 		entry = rb_entry(n, struct btrfs_qgroup, sort_node);
-		print_single_qgroup_table(entry);
+		print_single_qgroup_table(entry, verbose);
 		n = rb_next(n);
 	}
 }
 
 int btrfs_show_qgroups(int fd,
 		       struct btrfs_qgroup_filter_set *filter_set,
-		       struct btrfs_qgroup_comparer_set *comp_set)
+		       struct btrfs_qgroup_comparer_set *comp_set,
+		       bool verbose)
 {
 
 	struct qgroup_lookup qgroup_lookup;
@@ -1189,7 +1301,7 @@ int btrfs_show_qgroups(int fd,
 		return ret;
 	__filter_and_sort_qgroups(&qgroup_lookup, &sort_tree,
 				  filter_set, comp_set);
-	print_all_qgroups(&sort_tree);
+	print_all_qgroups(&sort_tree, verbose);
 
 	__free_all_qgroups(&qgroup_lookup);
 	return ret;
diff --git a/qgroup.h b/qgroup.h
index 875fbdf3..f7ab7de5 100644
--- a/qgroup.h
+++ b/qgroup.h
@@ -59,11 +59,13 @@ enum btrfs_qgroup_column_enum {
 	BTRFS_QGROUP_MAX_EXCL,
 	BTRFS_QGROUP_PARENT,
 	BTRFS_QGROUP_CHILD,
+	BTRFS_QGROUP_PATHNAME,
 	BTRFS_QGROUP_ALL,
 };
 
 enum btrfs_qgroup_comp_enum {
 	BTRFS_QGROUP_COMP_QGROUPID,
+	BTRFS_QGROUP_COMP_PATHNAME,
 	BTRFS_QGROUP_COMP_RFER,
 	BTRFS_QGROUP_COMP_EXCL,
 	BTRFS_QGROUP_COMP_MAX_RFER,
@@ -80,7 +82,7 @@ enum btrfs_qgroup_filter_enum {
 int btrfs_qgroup_parse_sort_string(const char *opt_arg,
 				struct btrfs_qgroup_comparer_set **comps);
 int btrfs_show_qgroups(int fd, struct btrfs_qgroup_filter_set *,
-		       struct btrfs_qgroup_comparer_set *);
+		       struct btrfs_qgroup_comparer_set *, bool verbose);
 void btrfs_qgroup_setup_print_column(enum btrfs_qgroup_column_enum column);
 void btrfs_qgroup_setup_units(unsigned unit_mode);
 struct btrfs_qgroup_filter_set *btrfs_qgroup_alloc_filter_set(void);
diff --git a/utils.c b/utils.c
index e9cb3a82..68202142 100644
--- a/utils.c
+++ b/utils.c
@@ -2556,15 +2556,9 @@ out:
 	return ret;
 }
 
-int get_subvol_info_by_rootid(const char *mnt, struct root_info *get_ri, u64 r_id)
+int get_subvol_info_by_rootid_fd(int fd, struct root_info *get_ri, u64 r_id)
 {
-	int fd;
 	int ret;
-	DIR *dirstream = NULL;
-
-	fd = btrfs_open_dir(mnt, &dirstream, 1);
-	if (fd < 0)
-		return -EINVAL;
 
 	memset(get_ri, 0, sizeof(*get_ri));
 	get_ri->root_id = r_id;
@@ -2574,6 +2568,20 @@ int get_subvol_info_by_rootid(const char *mnt, struct root_info *get_ri, u64 r_i
 	else
 		ret = btrfs_get_subvol(fd, get_ri);
 
+	return ret;
+}
+
+int get_subvol_info_by_rootid(const char *mnt, struct root_info *get_ri, u64 r_id)
+{
+	int fd;
+	int ret;
+	DIR *dirstream = NULL;
+
+	fd = btrfs_open_dir(mnt, &dirstream, 1);
+	if (fd < 0)
+		return -EINVAL;
+
+	ret = get_subvol_info_by_rootid_fd(fd, get_ri, r_id);
 	if (ret)
 		error("can't find rootid '%llu' on '%s': %d", r_id, mnt, ret);
 
diff --git a/utils.h b/utils.h
index b871c9ff..722d3c48 100644
--- a/utils.h
+++ b/utils.h
@@ -154,6 +154,8 @@ int test_isdir(const char *path);
 
 const char *subvol_strip_mountpoint(const char *mnt, const char *full_path);
 int get_subvol_info(const char *fullpath, struct root_info *get_ri);
+int get_subvol_info_by_rootid_fd(int fd, struct root_info *get_ri,
+				 u64 rootid_arg);
 int get_subvol_info_by_rootid(const char *mnt, struct root_info *get_ri,
 							u64 rootid_arg);
 int get_subvol_info_by_uuid(const char *mnt, struct root_info *get_ri,
-- 
2.15.1

Re: [PATCH 4/8] btrfs-progs: qgroups: add pathname to show output

[Qu Wenruo]

[-- Attachment #1.1: Type: text/plain, Size: 17495 bytes --]



On 2018年03月03日 02:47, jeffm@suse.com wrote:
> From: Jeff Mahoney <jeffm@suse.com>
> 
> The btrfs qgroup show command currently only exports qgroup IDs,
> forcing the user to resolve which subvolume each corresponds to.
> 
> This patch adds pathname resolution to qgroup show so that when
> the -P option is used, the last column contains the pathname of
> the root of the subvolume it describes.  In the case of nested
> qgroups, it will show the number of member qgroups or the paths
> of the members if the -v option is used.
> 
> Pathname can also be used as a sort parameter.
> 
> Signed-off-by: Jeff Mahoney <jeffm@suse.com>
> ---
>  Documentation/btrfs-qgroup.asciidoc |   4 +
>  cmds-qgroup.c                       |  17 ++++-
>  kerncompat.h                        |   1 +
>  qgroup.c                            | 142 ++++++++++++++++++++++++++++++++----
>  qgroup.h                            |   4 +-
>  utils.c                             |  22 ++++--
>  utils.h                             |   2 +
>  7 files changed, 166 insertions(+), 26 deletions(-)
> 
> diff --git a/Documentation/btrfs-qgroup.asciidoc b/Documentation/btrfs-qgroup.asciidoc
> index 3108457c..360b3269 100644
> --- a/Documentation/btrfs-qgroup.asciidoc
> +++ b/Documentation/btrfs-qgroup.asciidoc
> @@ -97,10 +97,14 @@ print child qgroup id.
>  print limit of referenced size of qgroup.
>  -e::::
>  print limit of exclusive size of qgroup.
> +-P::::
> +print pathname to the root of the subvolume managed by qgroup.  For nested qgroups, the number of members will be printed unless -v is specified.
>  -F::::
>  list all qgroups which impact the given path(include ancestral qgroups)
>  -f::::
>  list all qgroups which impact the given path(exclude ancestral qgroups)
> +-v::::
> +Be more verbose.  Print pathnames of member qgroups when nested.
>  --raw::::
>  raw numbers in bytes, without the 'B' suffix.
>  --human-readable::::
> diff --git a/cmds-qgroup.c b/cmds-qgroup.c
> index 48686436..94cd0fd3 100644
> --- a/cmds-qgroup.c
> +++ b/cmds-qgroup.c
> @@ -280,8 +280,10 @@ static const char * const cmd_qgroup_show_usage[] = {
>  	"               (including ancestral qgroups)",
>  	"-f             list all qgroups which impact the given path",
>  	"               (excluding ancestral qgroups)",
> +	"-P             print first-level qgroups using pathname",
> +	"-v             verbose, prints all nested subvolumes",

Did you mean the subvolume paths of all children qgroups?

>  	HELPINFO_UNITS_LONG,
> -	"--sort=qgroupid,rfer,excl,max_rfer,max_excl",
> +	"--sort=qgroupid,rfer,excl,max_rfer,max_excl,pathname",
>  	"               list qgroups sorted by specified items",
>  	"               you can use '+' or '-' in front of each item.",
>  	"               (+:ascending, -:descending, ascending default)",
> @@ -299,6 +301,7 @@ static int cmd_qgroup_show(int argc, char **argv)
>  	int filter_flag = 0;
>  	unsigned unit_mode;
>  	int sync = 0;
> +	bool verbose = false;
>  
>  	struct btrfs_qgroup_comparer_set *comparer_set;
>  	struct btrfs_qgroup_filter_set *filter_set;
> @@ -316,10 +319,11 @@ static int cmd_qgroup_show(int argc, char **argv)
>  		static const struct option long_options[] = {
>  			{"sort", required_argument, NULL, GETOPT_VAL_SORT},
>  			{"sync", no_argument, NULL, GETOPT_VAL_SYNC},
> +			{"verbose", no_argument, NULL, 'v'},
>  			{ NULL, 0, NULL, 0 }
>  		};
>  
> -		c = getopt_long(argc, argv, "pcreFf", long_options, NULL);
> +		c = getopt_long(argc, argv, "pPcreFfv", long_options, NULL);
>  		if (c < 0)
>  			break;
>  		switch (c) {
> @@ -327,6 +331,10 @@ static int cmd_qgroup_show(int argc, char **argv)
>  			btrfs_qgroup_setup_print_column(
>  				BTRFS_QGROUP_PARENT);
>  			break;
> +		case 'P':
> +			btrfs_qgroup_setup_print_column(
> +				BTRFS_QGROUP_PATHNAME);
> +			break;
>  		case 'c':
>  			btrfs_qgroup_setup_print_column(
>  				BTRFS_QGROUP_CHILD);
> @@ -354,6 +362,9 @@ static int cmd_qgroup_show(int argc, char **argv)
>  		case GETOPT_VAL_SYNC:
>  			sync = 1;
>  			break;
> +		case 'v':
> +			verbose = true;
> +			break;
>  		default:
>  			usage(cmd_qgroup_show_usage);
>  		}
> @@ -394,7 +405,7 @@ static int cmd_qgroup_show(int argc, char **argv)
>  					BTRFS_QGROUP_FILTER_PARENT,
>  					qgroupid);
>  	}
> -	ret = btrfs_show_qgroups(fd, filter_set, comparer_set);
> +	ret = btrfs_show_qgroups(fd, filter_set, comparer_set, verbose);
>  	close_file_or_dir(fd, dirstream);
>  	free(filter_set);
>  	free(comparer_set);
> diff --git a/kerncompat.h b/kerncompat.h
> index fa96715f..f97495ee 100644
> --- a/kerncompat.h
> +++ b/kerncompat.h
> @@ -29,6 +29,7 @@
>  #include <stddef.h>
>  #include <linux/types.h>
>  #include <stdint.h>
> +#include <stdbool.h>
>  
>  #include <features.h>
>  
> diff --git a/qgroup.c b/qgroup.c
> index 67bc0738..83918134 100644
> --- a/qgroup.c
> +++ b/qgroup.c
> @@ -40,6 +40,9 @@ struct btrfs_qgroup {
>  	struct rb_node all_parent_node;
>  	u64 qgroupid;
>  
> +	/* NULL for qgroups with level > 0 */
> +	const char *pathname;
> +
>  	/*
>  	 * info_item
>  	 */
> @@ -133,6 +136,13 @@ static struct {
>  		.unit_mode	= 0,
>  		.max_len	= 5,
>  	},
> +	{
> +		.name		= "pathname",
> +		.column_name	= "pathname",
> +		.need_print	= 0,
> +		.unit_mode	= 0,
> +		.max_len	= 10,
> +	},
>  	{
>  		.name		= NULL,
>  		.column_name	= NULL,
> @@ -210,8 +220,42 @@ static void print_qgroup_column_add_blank(enum btrfs_qgroup_column_enum column,
>  		printf(" ");
>  }
>  
> +void print_pathname_column(struct btrfs_qgroup *qgroup, bool verbose)
> +{
> +	struct btrfs_qgroup_list *list = NULL;
> +
> +	fputs("  ", stdout);
> +	if (btrfs_qgroup_level(qgroup->qgroupid) > 0) {
> +		int count = 0;

Newline after declaration please.

And declaration in if() {} block doesn't pass checkpath IIRC.

> +		list_for_each_entry(list, &qgroup->qgroups,
> +				    next_qgroup) {
> +			if (verbose) {
> +				struct btrfs_qgroup *member = list->qgroup;

Same coding style problem here.

> +				u64 level = btrfs_qgroup_level(member->qgroupid);
> +				u64 sid = btrfs_qgroup_subvid(member->qgroupid);
> +				if (count)
> +					fputs(" ", stdout);
> +				if (btrfs_qgroup_level(member->qgroupid) == 0)
> +					fputs(member->pathname, stdout);

What about checking member->pathname before outputting?
As it could be missing.

> +				else
> +					printf("%llu/%llu", level, sid);
> +			}
> +			count++;
> +		}
> +		if (!count)
> +			fputs("<empty>", stdout);
> +		else if (!verbose)
> +			printf("<%u member qgroup%c>", count,
> +			       count != 1 ? 's' : '\0');
> +	} else if (qgroup->pathname)
> +		fputs(qgroup->pathname, stdout);
> +	else
> +		fputs("<missing>", stdout);
> +}
> +
>  static void print_qgroup_column(struct btrfs_qgroup *qgroup,
> -				enum btrfs_qgroup_column_enum column)
> +				enum btrfs_qgroup_column_enum column,
> +				bool verbose)
>  {
>  	int len;
>  	int unit_mode = btrfs_qgroup_columns[column].unit_mode;
> @@ -253,19 +297,22 @@ static void print_qgroup_column(struct btrfs_qgroup *qgroup,
>  		len = print_child_column(qgroup);
>  		print_qgroup_column_add_blank(BTRFS_QGROUP_CHILD, len);
>  		break;
> +	case BTRFS_QGROUP_PATHNAME:
> +		print_pathname_column(qgroup, verbose);
> +		break;
>  	default:
>  		break;
>  	}
>  }
>  
> -static void print_single_qgroup_table(struct btrfs_qgroup *qgroup)
> +static void print_single_qgroup_table(struct btrfs_qgroup *qgroup, bool verbose)
>  {
>  	int i;
>  
>  	for (i = 0; i < BTRFS_QGROUP_ALL; i++) {
>  		if (!btrfs_qgroup_columns[i].need_print)
>  			continue;
> -		print_qgroup_column(qgroup, i);
> +		print_qgroup_column(qgroup, i, verbose);
>  
>  		if (i != BTRFS_QGROUP_ALL - 1)
>  			printf(" ");
> @@ -338,6 +385,47 @@ static int comp_entry_with_qgroupid(struct btrfs_qgroup *entry1,
>  	return is_descending ? -ret : ret;
>  }
>  
> +/* Sorts first-level qgroups by pathname and nested qgroups by qgroupid */
> +static int comp_entry_with_pathname(struct btrfs_qgroup *entry1,
> +				    struct btrfs_qgroup *entry2,
> +				    int is_descending)
> +{
> +	int ret = 0;
> +	const char *p1 = entry1->pathname;
> +	const char *p2 = entry2->pathname;
> +
> +	u64 level1 = btrfs_qgroup_level(entry1->qgroupid);
> +	u64 level2 = btrfs_qgroup_level(entry2->qgroupid);
> +
> +	if (level1 != level2) {
> +		if (entry1->qgroupid > entry2->qgroupid)
> +			ret = 1;
> +		else if (entry1->qgroupid < entry2->qgroupid)
> +			ret = -1;
> +	}
> +
> +	if (ret)
> +		goto out;
> +
> +	while (*p1 && *p2) {
> +		if (*p1 != *p2)
> +			break;
> +		p1++;
> +		p2++;
> +	}
> +
> +	if (*p1 == '/')
> +		ret = 1;
> +	else if (*p2 == '/')
> +	      ret = -1;
> +	else if (*p1 > *p2)
> +	      ret = 1;
> +	else if (*p1 < *p2)
> +		ret = -1;
> +out:
> +	return is_descending ? -ret : ret;
> +}
> +
>  static int comp_entry_with_rfer(struct btrfs_qgroup *entry1,
>  				struct btrfs_qgroup *entry2,
>  				int is_descending)
> @@ -404,6 +492,7 @@ static int comp_entry_with_max_excl(struct btrfs_qgroup *entry1,
>  
>  static btrfs_qgroup_comp_func all_comp_funcs[] = {
>  	[BTRFS_QGROUP_COMP_QGROUPID]	= comp_entry_with_qgroupid,
> +	[BTRFS_QGROUP_COMP_PATHNAME]	= comp_entry_with_pathname,
>  	[BTRFS_QGROUP_COMP_RFER]	= comp_entry_with_rfer,
>  	[BTRFS_QGROUP_COMP_EXCL]	= comp_entry_with_excl,
>  	[BTRFS_QGROUP_COMP_MAX_RFER]	= comp_entry_with_max_rfer,
> @@ -412,6 +501,7 @@ static btrfs_qgroup_comp_func all_comp_funcs[] = {
>  
>  static char *all_sort_items[] = {
>  	[BTRFS_QGROUP_COMP_QGROUPID]	= "qgroupid",
> +	[BTRFS_QGROUP_COMP_PATHNAME]	= "pathname",
>  	[BTRFS_QGROUP_COMP_RFER]	= "rfer",
>  	[BTRFS_QGROUP_COMP_EXCL]	= "excl",
>  	[BTRFS_QGROUP_COMP_MAX_RFER]	= "max_rfer",
> @@ -578,6 +668,25 @@ static struct btrfs_qgroup *qgroup_tree_search(struct qgroup_lookup *root_tree,
>  	return NULL;
>  }
>  
> +static const char *qgroup_pathname(int fd, u64 qgroupid)
> +{
> +	struct root_info root_info;
> +	int ret;
> +	char *pathname;
> +
> +	ret = get_subvol_info_by_rootid_fd(fd, &root_info, qgroupid);
> +	if (ret)
> +		return NULL;
> +
> +	ret = asprintf(&pathname, "%s%s",
> +		       root_info.full_path[0] == '/' ? "" : "/",
> +		       root_info.full_path);
> +	if (ret < 0)
> +		return NULL;
> +
> +	return pathname;
> +}
> +
>  /*
>   * Lookup or insert btrfs_qgroup into qgroup_lookup.
>   *
> @@ -588,7 +697,7 @@ static struct btrfs_qgroup *qgroup_tree_search(struct qgroup_lookup *root_tree,
>   * Return the pointer to the btrfs_qgroup if found or if inserted successfully.
>   * Return ERR_PTR if any error occurred.
>   */
> -static struct btrfs_qgroup *get_or_add_qgroup(
> +static struct btrfs_qgroup *get_or_add_qgroup(int fd,
>  		struct qgroup_lookup *qgroup_lookup, u64 qgroupid)
>  {
>  	struct btrfs_qgroup *bq;
> @@ -608,6 +717,8 @@ static struct btrfs_qgroup *get_or_add_qgroup(
>  	INIT_LIST_HEAD(&bq->qgroups);
>  	INIT_LIST_HEAD(&bq->members);
>  
> +	bq->pathname = qgroup_pathname(fd, qgroupid);
> +

Here qgroup_pathname() will allocate memory, but no one is freeing this
memory.

The cleaner should be in __free_btrfs_qgroup() but there is no
modification to that function.

Thanks,
Qu

>  	ret = qgroup_tree_insert(qgroup_lookup, bq);
>  	if (ret) {
>  		error("failed to insert %llu into tree: %s",
> @@ -619,12 +730,12 @@ static struct btrfs_qgroup *get_or_add_qgroup(
>  	return bq;
>  }
>  
> -static int update_qgroup_info(struct qgroup_lookup *qgroup_lookup, u64 qgroupid,
> -			      struct btrfs_qgroup_info_item *info)
> +static int update_qgroup_info(int fd, struct qgroup_lookup *qgroup_lookup,
> +			      u64 qgroupid, struct btrfs_qgroup_info_item *info)
>  {
>  	struct btrfs_qgroup *bq;
>  
> -	bq = get_or_add_qgroup(qgroup_lookup, qgroupid);
> +	bq = get_or_add_qgroup(fd, qgroup_lookup, qgroupid);
>  	if (IS_ERR_OR_NULL(bq))
>  		return PTR_ERR(bq);
>  
> @@ -637,13 +748,13 @@ static int update_qgroup_info(struct qgroup_lookup *qgroup_lookup, u64 qgroupid,
>  	return 0;
>  }
>  
> -static int update_qgroup_limit(struct qgroup_lookup *qgroup_lookup,
> +static int update_qgroup_limit(int fd, struct qgroup_lookup *qgroup_lookup,
>  			       u64 qgroupid,
>  			       struct btrfs_qgroup_limit_item *limit)
>  {
>  	struct btrfs_qgroup *bq;
>  
> -	bq = get_or_add_qgroup(qgroup_lookup, qgroupid);
> +	bq = get_or_add_qgroup(fd, qgroup_lookup, qgroupid);
>  	if (IS_ERR_OR_NULL(bq))
>  		return PTR_ERR(bq);
>  
> @@ -1107,7 +1218,7 @@ static int __qgroups_search(int fd, struct qgroup_lookup *qgroup_lookup)
>  				info = (struct btrfs_qgroup_info_item *)
>  				       (args.buf + off);
>  
> -				ret = update_qgroup_info(qgroup_lookup,
> +				ret = update_qgroup_info(fd, qgroup_lookup,
>  							 qgroupid, info);
>  				break;
>  			case BTRFS_QGROUP_LIMIT_KEY:
> @@ -1115,7 +1226,7 @@ static int __qgroups_search(int fd, struct qgroup_lookup *qgroup_lookup)
>  				limit = (struct btrfs_qgroup_limit_item *)
>  					(args.buf + off);
>  
> -				ret = update_qgroup_limit(qgroup_lookup,
> +				ret = update_qgroup_limit(fd, qgroup_lookup,
>  							  qgroupid, limit);
>  				break;
>  			case BTRFS_QGROUP_RELATION_KEY:
> @@ -1159,7 +1270,7 @@ static int __qgroups_search(int fd, struct qgroup_lookup *qgroup_lookup)
>  	return ret;
>  }
>  
> -static void print_all_qgroups(struct qgroup_lookup *qgroup_lookup)
> +static void print_all_qgroups(struct qgroup_lookup *qgroup_lookup, bool verbose)
>  {
>  
>  	struct rb_node *n;
> @@ -1170,14 +1281,15 @@ static void print_all_qgroups(struct qgroup_lookup *qgroup_lookup)
>  	n = rb_first(&qgroup_lookup->root);
>  	while (n) {
>  		entry = rb_entry(n, struct btrfs_qgroup, sort_node);
> -		print_single_qgroup_table(entry);
> +		print_single_qgroup_table(entry, verbose);
>  		n = rb_next(n);
>  	}
>  }
>  
>  int btrfs_show_qgroups(int fd,
>  		       struct btrfs_qgroup_filter_set *filter_set,
> -		       struct btrfs_qgroup_comparer_set *comp_set)
> +		       struct btrfs_qgroup_comparer_set *comp_set,
> +		       bool verbose)
>  {
>  
>  	struct qgroup_lookup qgroup_lookup;
> @@ -1189,7 +1301,7 @@ int btrfs_show_qgroups(int fd,
>  		return ret;
>  	__filter_and_sort_qgroups(&qgroup_lookup, &sort_tree,
>  				  filter_set, comp_set);
> -	print_all_qgroups(&sort_tree);
> +	print_all_qgroups(&sort_tree, verbose);
>  
>  	__free_all_qgroups(&qgroup_lookup);
>  	return ret;
> diff --git a/qgroup.h b/qgroup.h
> index 875fbdf3..f7ab7de5 100644
> --- a/qgroup.h
> +++ b/qgroup.h
> @@ -59,11 +59,13 @@ enum btrfs_qgroup_column_enum {
>  	BTRFS_QGROUP_MAX_EXCL,
>  	BTRFS_QGROUP_PARENT,
>  	BTRFS_QGROUP_CHILD,
> +	BTRFS_QGROUP_PATHNAME,
>  	BTRFS_QGROUP_ALL,
>  };
>  
>  enum btrfs_qgroup_comp_enum {
>  	BTRFS_QGROUP_COMP_QGROUPID,
> +	BTRFS_QGROUP_COMP_PATHNAME,
>  	BTRFS_QGROUP_COMP_RFER,
>  	BTRFS_QGROUP_COMP_EXCL,
>  	BTRFS_QGROUP_COMP_MAX_RFER,
> @@ -80,7 +82,7 @@ enum btrfs_qgroup_filter_enum {
>  int btrfs_qgroup_parse_sort_string(const char *opt_arg,
>  				struct btrfs_qgroup_comparer_set **comps);
>  int btrfs_show_qgroups(int fd, struct btrfs_qgroup_filter_set *,
> -		       struct btrfs_qgroup_comparer_set *);
> +		       struct btrfs_qgroup_comparer_set *, bool verbose);
>  void btrfs_qgroup_setup_print_column(enum btrfs_qgroup_column_enum column);
>  void btrfs_qgroup_setup_units(unsigned unit_mode);
>  struct btrfs_qgroup_filter_set *btrfs_qgroup_alloc_filter_set(void);
> diff --git a/utils.c b/utils.c
> index e9cb3a82..68202142 100644
> --- a/utils.c
> +++ b/utils.c
> @@ -2556,15 +2556,9 @@ out:
>  	return ret;
>  }
>  
> -int get_subvol_info_by_rootid(const char *mnt, struct root_info *get_ri, u64 r_id)
> +int get_subvol_info_by_rootid_fd(int fd, struct root_info *get_ri, u64 r_id)
>  {
> -	int fd;
>  	int ret;
> -	DIR *dirstream = NULL;
> -
> -	fd = btrfs_open_dir(mnt, &dirstream, 1);
> -	if (fd < 0)
> -		return -EINVAL;
>  
>  	memset(get_ri, 0, sizeof(*get_ri));
>  	get_ri->root_id = r_id;
> @@ -2574,6 +2568,20 @@ int get_subvol_info_by_rootid(const char *mnt, struct root_info *get_ri, u64 r_i
>  	else
>  		ret = btrfs_get_subvol(fd, get_ri);
>  
> +	return ret;
> +}
> +
> +int get_subvol_info_by_rootid(const char *mnt, struct root_info *get_ri, u64 r_id)
> +{
> +	int fd;
> +	int ret;
> +	DIR *dirstream = NULL;
> +
> +	fd = btrfs_open_dir(mnt, &dirstream, 1);
> +	if (fd < 0)
> +		return -EINVAL;
> +
> +	ret = get_subvol_info_by_rootid_fd(fd, get_ri, r_id);
>  	if (ret)
>  		error("can't find rootid '%llu' on '%s': %d", r_id, mnt, ret);
>  
> diff --git a/utils.h b/utils.h
> index b871c9ff..722d3c48 100644
> --- a/utils.h
> +++ b/utils.h
> @@ -154,6 +154,8 @@ int test_isdir(const char *path);
>  
>  const char *subvol_strip_mountpoint(const char *mnt, const char *full_path);
>  int get_subvol_info(const char *fullpath, struct root_info *get_ri);
> +int get_subvol_info_by_rootid_fd(int fd, struct root_info *get_ri,
> +				 u64 rootid_arg);
>  int get_subvol_info_by_rootid(const char *mnt, struct root_info *get_ri,
>  							u64 rootid_arg);
>  int get_subvol_info_by_uuid(const char *mnt, struct root_info *get_ri,
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 520 bytes --]

Re: [PATCH 4/8] btrfs-progs: qgroups: add pathname to show output

[Jeff Mahoney]

[-- Attachment #1.1: Type: text/plain, Size: 3918 bytes --]

On 3/7/18 12:45 AM, Qu Wenruo wrote:
> 
> 
> On 2018年03月03日 02:47, jeffm@suse.com wrote:
>> diff --git a/cmds-qgroup.c b/cmds-qgroup.c
>> index 48686436..94cd0fd3 100644
>> --- a/cmds-qgroup.c
>> +++ b/cmds-qgroup.c
>> @@ -280,8 +280,10 @@ static const char * const cmd_qgroup_show_usage[] = {
>>  	"               (including ancestral qgroups)",
>>  	"-f             list all qgroups which impact the given path",
>>  	"               (excluding ancestral qgroups)",
>> +	"-P             print first-level qgroups using pathname",
>> +	"-v             verbose, prints all nested subvolumes",
> 
> Did you mean the subvolume paths of all children qgroups?

Yes.  I'll make that clearer.

>>  	HELPINFO_UNITS_LONG,
>> -	"--sort=qgroupid,rfer,excl,max_rfer,max_excl",
>> +	"--sort=qgroupid,rfer,excl,max_rfer,max_excl,pathname",
>>  	"               list qgroups sorted by specified items",
>>  	"               you can use '+' or '-' in front of each item.",
>>  	"               (+:ascending, -:descending, ascending default)",

>> diff --git a/qgroup.c b/qgroup.c
>> index 67bc0738..83918134 100644
>> --- a/qgroup.c
>> +++ b/qgroup.c
>> @@ -210,8 +220,42 @@ static void print_qgroup_column_add_blank(enum btrfs_qgroup_column_enum column,
>>  		printf(" ");
>>  }
>>  
>> +void print_pathname_column(struct btrfs_qgroup *qgroup, bool verbose)
>> +{
>> +	struct btrfs_qgroup_list *list = NULL;
>> +
>> +	fputs("  ", stdout);
>> +	if (btrfs_qgroup_level(qgroup->qgroupid) > 0) {
>> +		int count = 0;
> 
> Newline after declaration please.

Ack.

> And declaration in if() {} block doesn't pass checkpath IIRC.

Declarations in if () {} are fine.

>> +		list_for_each_entry(list, &qgroup->qgroups,
>> +				    next_qgroup) {
>> +			if (verbose) {
>> +				struct btrfs_qgroup *member = list->qgroup;
> 
> Same coding style problem here.

Ack.

>> +				u64 level = btrfs_qgroup_level(member->qgroupid);
>> +				u64 sid = btrfs_qgroup_subvid(member->qgroupid);
>> +				if (count)
>> +					fputs(" ", stdout);
>> +				if (btrfs_qgroup_level(member->qgroupid) == 0)
>> +					fputs(member->pathname, stdout);
> 
> What about checking member->pathname before outputting?
> As it could be missing.

Yep.

>> +static const char *qgroup_pathname(int fd, u64 qgroupid)
>> +{
>> +	struct root_info root_info;
>> +	int ret;
>> +	char *pathname;
>> +
>> +	ret = get_subvol_info_by_rootid_fd(fd, &root_info, qgroupid);

This is a leak too.  Callers are responsible for freeing the root_info
paths.  With this and your review fixed, valgrind passes with 0 leaks
for btrfs qgroups show -P.

>> +	if (ret)
>> +		return NULL;
>> +
>> +	ret = asprintf(&pathname, "%s%s",
>> +		       root_info.full_path[0] == '/' ? "" : "/",
>> +		       root_info.full_path);
>> +	if (ret < 0)
>> +		return NULL;
>> +
>> +	return pathname;
>> +}
>> +
>>  /*
>>   * Lookup or insert btrfs_qgroup into qgroup_lookup.
>>   *
>> @@ -588,7 +697,7 @@ static struct btrfs_qgroup *qgroup_tree_search(struct qgroup_lookup *root_tree,
>>   * Return the pointer to the btrfs_qgroup if found or if inserted successfully.
>>   * Return ERR_PTR if any error occurred.
>>   */
>> -static struct btrfs_qgroup *get_or_add_qgroup(
>> +static struct btrfs_qgroup *get_or_add_qgroup(int fd,
>>  		struct qgroup_lookup *qgroup_lookup, u64 qgroupid)
>>  {
>>  	struct btrfs_qgroup *bq;
>> @@ -608,6 +717,8 @@ static struct btrfs_qgroup *get_or_add_qgroup(
>>  	INIT_LIST_HEAD(&bq->qgroups);
>>  	INIT_LIST_HEAD(&bq->members);
>>  
>> +	bq->pathname = qgroup_pathname(fd, qgroupid);
>> +
> 
> Here qgroup_pathname() will allocate memory, but no one is freeing this
> memory.
> 
> The cleaner should be in __free_btrfs_qgroup() but there is no
> modification to that function.

Ack.

Thanks for the review,

-Jeff

-- 
Jeff Mahoney
SUSE Labs


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 854 bytes --]

[PATCH 5/8] btrfs-progs: qgroups: introduce and use info and limit structures

[jeffm]

From: Jeff Mahoney <jeffm@suse.com>

We use structures to pass the info and limit from the kernel as items
but store the individual values separately in btrfs_qgroup.  We already
have a btrfs_qgroup_limit structure that's used for setting the limit.

This patch introduces a btrfs_qgroup_info structure and uses that and
btrfs_qgroup_limit in btrfs_qgroup.

Signed-off-by: Jeff Mahoney <jeffm@suse.com>
---
 qgroup.c | 73 +++++++++++++++++++++++++++++++---------------------------------
 qgroup.h |  8 +++++++
 2 files changed, 43 insertions(+), 38 deletions(-)

diff --git a/qgroup.c b/qgroup.c
index 83918134..b1be3311 100644
--- a/qgroup.c
+++ b/qgroup.c
@@ -46,20 +46,12 @@ struct btrfs_qgroup {
 	/*
 	 * info_item
 	 */
-	u64 generation;
-	u64 rfer;	/*referenced*/
-	u64 rfer_cmpr;	/*referenced compressed*/
-	u64 excl;	/*exclusive*/
-	u64 excl_cmpr;	/*exclusive compressed*/
+	struct btrfs_qgroup_info info;
 
 	/*
 	 *limit_item
 	 */
-	u64 flags;	/*which limits are set*/
-	u64 max_rfer;
-	u64 max_excl;
-	u64 rsv_rfer;
-	u64 rsv_excl;
+	struct btrfs_qgroup_limit limit;
 
 	/*qgroups this group is member of*/
 	struct list_head qgroups;
@@ -272,24 +264,24 @@ static void print_qgroup_column(struct btrfs_qgroup *qgroup,
 		print_qgroup_column_add_blank(BTRFS_QGROUP_QGROUPID, len);
 		break;
 	case BTRFS_QGROUP_RFER:
-		len = printf("%*s", max_len, pretty_size_mode(qgroup->rfer, unit_mode));
+		len = printf("%*s", max_len, pretty_size_mode(qgroup->info.referenced, unit_mode));
 		break;
 	case BTRFS_QGROUP_EXCL:
-		len = printf("%*s", max_len, pretty_size_mode(qgroup->excl, unit_mode));
+		len = printf("%*s", max_len, pretty_size_mode(qgroup->info.exclusive, unit_mode));
 		break;
 	case BTRFS_QGROUP_PARENT:
 		len = print_parent_column(qgroup);
 		print_qgroup_column_add_blank(BTRFS_QGROUP_PARENT, len);
 		break;
 	case BTRFS_QGROUP_MAX_RFER:
-		if (qgroup->flags & BTRFS_QGROUP_LIMIT_MAX_RFER)
-			len = printf("%*s", max_len, pretty_size_mode(qgroup->max_rfer, unit_mode));
+		if (qgroup->limit.flags & BTRFS_QGROUP_LIMIT_MAX_RFER)
+			len = printf("%*s", max_len, pretty_size_mode(qgroup->limit.max_referenced, unit_mode));
 		else
 			len = printf("%*s", max_len, "none");
 		break;
 	case BTRFS_QGROUP_MAX_EXCL:
-		if (qgroup->flags & BTRFS_QGROUP_LIMIT_MAX_EXCL)
-			len = printf("%*s", max_len, pretty_size_mode(qgroup->max_excl, unit_mode));
+		if (qgroup->limit.flags & BTRFS_QGROUP_LIMIT_MAX_EXCL)
+			len = printf("%*s", max_len, pretty_size_mode(qgroup->limit.max_exclusive, unit_mode));
 		else
 			len = printf("%*s", max_len, "none");
 		break;
@@ -432,9 +424,9 @@ static int comp_entry_with_rfer(struct btrfs_qgroup *entry1,
 {
 	int ret;
 
-	if (entry1->rfer > entry2->rfer)
+	if (entry1->info.referenced > entry2->info.referenced)
 		ret = 1;
-	else if (entry1->rfer < entry2->rfer)
+	else if (entry1->info.referenced < entry2->info.referenced)
 		ret = -1;
 	else
 		ret = 0;
@@ -448,9 +440,9 @@ static int comp_entry_with_excl(struct btrfs_qgroup *entry1,
 {
 	int ret;
 
-	if (entry1->excl > entry2->excl)
+	if (entry1->info.exclusive > entry2->info.exclusive)
 		ret = 1;
-	else if (entry1->excl < entry2->excl)
+	else if (entry1->info.exclusive < entry2->info.exclusive)
 		ret = -1;
 	else
 		ret = 0;
@@ -464,9 +456,9 @@ static int comp_entry_with_max_rfer(struct btrfs_qgroup *entry1,
 {
 	int ret;
 
-	if (entry1->max_rfer > entry2->max_rfer)
+	if (entry1->limit.max_referenced > entry2->limit.max_referenced)
 		ret = 1;
-	else if (entry1->max_rfer < entry2->max_rfer)
+	else if (entry1->limit.max_referenced < entry2->limit.max_referenced)
 		ret = -1;
 	else
 		ret = 0;
@@ -480,9 +472,9 @@ static int comp_entry_with_max_excl(struct btrfs_qgroup *entry1,
 {
 	int ret;
 
-	if (entry1->max_excl > entry2->max_excl)
+	if (entry1->limit.max_exclusive > entry2->limit.max_exclusive)
 		ret = 1;
-	else if (entry1->max_excl < entry2->max_excl)
+	else if (entry1->limit.max_exclusive < entry2->limit.max_exclusive)
 		ret = -1;
 	else
 		ret = 0;
@@ -739,11 +731,13 @@ static int update_qgroup_info(int fd, struct qgroup_lookup *qgroup_lookup,
 	if (IS_ERR_OR_NULL(bq))
 		return PTR_ERR(bq);
 
-	bq->generation = btrfs_stack_qgroup_info_generation(info);
-	bq->rfer = btrfs_stack_qgroup_info_referenced(info);
-	bq->rfer_cmpr = btrfs_stack_qgroup_info_referenced_compressed(info);
-	bq->excl = btrfs_stack_qgroup_info_exclusive(info);
-	bq->excl_cmpr = btrfs_stack_qgroup_info_exclusive_compressed(info);
+	bq->info.generation = btrfs_stack_qgroup_info_generation(info);
+	bq->info.referenced = btrfs_stack_qgroup_info_referenced(info);
+	bq->info.referenced_compressed =
+			btrfs_stack_qgroup_info_referenced_compressed(info);
+	bq->info.exclusive = btrfs_stack_qgroup_info_exclusive(info);
+	bq->info.exclusive_compressed =
+			btrfs_stack_qgroup_info_exclusive_compressed(info);
 
 	return 0;
 }
@@ -758,11 +752,14 @@ static int update_qgroup_limit(int fd, struct qgroup_lookup *qgroup_lookup,
 	if (IS_ERR_OR_NULL(bq))
 		return PTR_ERR(bq);
 
-	bq->flags = btrfs_stack_qgroup_limit_flags(limit);
-	bq->max_rfer = btrfs_stack_qgroup_limit_max_referenced(limit);
-	bq->max_excl = btrfs_stack_qgroup_limit_max_exclusive(limit);
-	bq->rsv_rfer = btrfs_stack_qgroup_limit_rsv_referenced(limit);
-	bq->rsv_excl = btrfs_stack_qgroup_limit_rsv_exclusive(limit);
+	bq->limit.flags = btrfs_stack_qgroup_limit_flags(limit);
+	bq->limit.max_referenced =
+			btrfs_stack_qgroup_limit_max_referenced(limit);
+	bq->limit.max_exclusive =
+			btrfs_stack_qgroup_limit_max_exclusive(limit);
+	bq->limit.rsv_referenced =
+			btrfs_stack_qgroup_limit_rsv_referenced(limit);
+	bq->limit.rsv_exclusive = btrfs_stack_qgroup_limit_rsv_exclusive(limit);
 
 	return 0;
 }
@@ -1053,22 +1050,22 @@ static void __update_columns_max_len(struct btrfs_qgroup *bq,
 			btrfs_qgroup_columns[column].max_len = len;
 		break;
 	case BTRFS_QGROUP_RFER:
-		len = strlen(pretty_size_mode(bq->rfer, unit_mode));
+		len = strlen(pretty_size_mode(bq->info.referenced, unit_mode));
 		if (btrfs_qgroup_columns[column].max_len < len)
 			btrfs_qgroup_columns[column].max_len = len;
 		break;
 	case BTRFS_QGROUP_EXCL:
-		len = strlen(pretty_size_mode(bq->excl, unit_mode));
+		len = strlen(pretty_size_mode(bq->info.exclusive, unit_mode));
 		if (btrfs_qgroup_columns[column].max_len < len)
 			btrfs_qgroup_columns[column].max_len = len;
 		break;
 	case BTRFS_QGROUP_MAX_RFER:
-		len = strlen(pretty_size_mode(bq->max_rfer, unit_mode));
+		len = strlen(pretty_size_mode(bq->limit.max_referenced, unit_mode));
 		if (btrfs_qgroup_columns[column].max_len < len)
 			btrfs_qgroup_columns[column].max_len = len;
 		break;
 	case BTRFS_QGROUP_MAX_EXCL:
-		len = strlen(pretty_size_mode(bq->max_excl, unit_mode));
+		len = strlen(pretty_size_mode(bq->limit.max_exclusive, unit_mode));
 		if (btrfs_qgroup_columns[column].max_len < len)
 			btrfs_qgroup_columns[column].max_len = len;
 		break;
diff --git a/qgroup.h b/qgroup.h
index f7ab7de5..5e71349c 100644
--- a/qgroup.h
+++ b/qgroup.h
@@ -79,6 +79,14 @@ enum btrfs_qgroup_filter_enum {
 	BTRFS_QGROUP_FILTER_MAX,
 };
 
+struct btrfs_qgroup_info {
+	u64 generation;
+	u64 referenced;
+	u64 referenced_compressed;
+	u64 exclusive;
+	u64 exclusive_compressed;
+};
+
 int btrfs_qgroup_parse_sort_string(const char *opt_arg,
 				struct btrfs_qgroup_comparer_set **comps);
 int btrfs_show_qgroups(int fd, struct btrfs_qgroup_filter_set *,
-- 
2.15.1

Re: [PATCH 5/8] btrfs-progs: qgroups: introduce and use info and limit structures

[Nikolay Borisov]

On  2.03.2018 20:47, jeffm@suse.com wrote:
> From: Jeff Mahoney <jeffm@suse.com>
> 
> We use structures to pass the info and limit from the kernel as items
> but store the individual values separately in btrfs_qgroup.  We already
> have a btrfs_qgroup_limit structure that's used for setting the limit.
> 
> This patch introduces a btrfs_qgroup_info structure and uses that and
> btrfs_qgroup_limit in btrfs_qgroup.
> 
> Signed-off-by: Jeff Mahoney <jeffm@suse.com>

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

[PATCH 6/8] btrfs-progs: qgroups: introduce btrfs_qgroup_query

[jeffm]

From: Jeff Mahoney <jeffm@suse.com>

The only mechanism we have in the progs for searching qgroups is to load
all of them and filter the results.  This works for qgroup show but
to add quota information to 'btrfs subvoluem show' it's pretty wasteful.

This patch splits out setting up the search and performing the search so
we can search for a single qgroupid more easily.

Signed-off-by: Jeff Mahoney <jeffm@suse.com>
---
 qgroup.c | 98 +++++++++++++++++++++++++++++++++++++++++++++-------------------
 qgroup.h |  7 +++++
 2 files changed, 77 insertions(+), 28 deletions(-)

diff --git a/qgroup.c b/qgroup.c
index b1be3311..2d0a6947 100644
--- a/qgroup.c
+++ b/qgroup.c
@@ -1146,11 +1146,11 @@ static inline void print_status_flag_warning(u64 flags)
 		warning("qgroup data inconsistent, rescan recommended");
 }
 
-static int __qgroups_search(int fd, struct qgroup_lookup *qgroup_lookup)
+static int __qgroups_search(int fd, struct btrfs_ioctl_search_args *args,
+			    struct qgroup_lookup *qgroup_lookup)
 {
 	int ret;
-	struct btrfs_ioctl_search_args args;
-	struct btrfs_ioctl_search_key *sk = &args.key;
+	struct btrfs_ioctl_search_key *sk = &args->key;
 	struct btrfs_ioctl_search_header *sh;
 	unsigned long off = 0;
 	unsigned int i;
@@ -1161,30 +1161,12 @@ static int __qgroups_search(int fd, struct qgroup_lookup *qgroup_lookup)
 	u64 qgroupid;
 	u64 qgroupid1;
 
-	memset(&args, 0, sizeof(args));
-
-	sk->tree_id = BTRFS_QUOTA_TREE_OBJECTID;
-	sk->max_type = BTRFS_QGROUP_RELATION_KEY;
-	sk->min_type = BTRFS_QGROUP_STATUS_KEY;
-	sk->max_objectid = (u64)-1;
-	sk->max_offset = (u64)-1;
-	sk->max_transid = (u64)-1;
-	sk->nr_items = 4096;
-
 	qgroup_lookup_init(qgroup_lookup);
 
 	while (1) {
-		ret = ioctl(fd, BTRFS_IOC_TREE_SEARCH, &args);
+		ret = ioctl(fd, BTRFS_IOC_TREE_SEARCH, args);
 		if (ret < 0) {
-			if (errno == ENOENT) {
-				error("can't list qgroups: quotas not enabled");
-				ret = -ENOTTY;
-			} else {
-				error("can't list qgroups: %s",
-				       strerror(errno));
-				ret = -errno;
-			}
-
+			ret = -errno;
 			break;
 		}
 
@@ -1198,14 +1180,14 @@ static int __qgroups_search(int fd, struct qgroup_lookup *qgroup_lookup)
 		 * read the root_ref item it contains
 		 */
 		for (i = 0; i < sk->nr_items; i++) {
-			sh = (struct btrfs_ioctl_search_header *)(args.buf +
+			sh = (struct btrfs_ioctl_search_header *)(args->buf +
 								  off);
 			off += sizeof(*sh);
 
 			switch (btrfs_search_header_type(sh)) {
 			case BTRFS_QGROUP_STATUS_KEY:
 				si = (struct btrfs_qgroup_status_item *)
-				     (args.buf + off);
+				     (args->buf + off);
 				flags = btrfs_stack_qgroup_status_flags(si);
 
 				print_status_flag_warning(flags);
@@ -1213,7 +1195,7 @@ static int __qgroups_search(int fd, struct qgroup_lookup *qgroup_lookup)
 			case BTRFS_QGROUP_INFO_KEY:
 				qgroupid = btrfs_search_header_offset(sh);
 				info = (struct btrfs_qgroup_info_item *)
-				       (args.buf + off);
+				       (args->buf + off);
 
 				ret = update_qgroup_info(fd, qgroup_lookup,
 							 qgroupid, info);
@@ -1221,7 +1203,7 @@ static int __qgroups_search(int fd, struct qgroup_lookup *qgroup_lookup)
 			case BTRFS_QGROUP_LIMIT_KEY:
 				qgroupid = btrfs_search_header_offset(sh);
 				limit = (struct btrfs_qgroup_limit_item *)
-					(args.buf + off);
+					(args->buf + off);
 
 				ret = update_qgroup_limit(fd, qgroup_lookup,
 							  qgroupid, limit);
@@ -1267,6 +1249,66 @@ static int __qgroups_search(int fd, struct qgroup_lookup *qgroup_lookup)
 	return ret;
 }
 
+static int qgroups_search_all(int fd, struct qgroup_lookup *qgroup_lookup)
+{
+	struct btrfs_ioctl_search_args args = {
+		.key = {
+			.tree_id = BTRFS_QUOTA_TREE_OBJECTID,
+			.max_type = BTRFS_QGROUP_RELATION_KEY,
+			.min_type = BTRFS_QGROUP_STATUS_KEY,
+			.max_objectid = (u64)-1,
+			.max_offset = (u64)-1,
+			.max_transid = (u64)-1,
+			.nr_items = 4096,
+		},
+	};
+	int ret;
+
+	ret = __qgroups_search(fd, &args, qgroup_lookup);
+	if (ret == -ENOTTY)
+		error("can't list qgroups: quotas not enabled");
+	else if (ret < 0)
+		error("can't list qgroups: %s", strerror(-ret));
+	return ret;
+}
+
+int btrfs_qgroup_query(int fd, u64 qgroupid, struct btrfs_qgroup_stats *stats)
+{
+	struct btrfs_ioctl_search_args args = {
+		.key = {
+			.tree_id = BTRFS_QUOTA_TREE_OBJECTID,
+			.min_type = BTRFS_QGROUP_INFO_KEY,
+			.max_type = BTRFS_QGROUP_LIMIT_KEY,
+			.max_objectid = 0,
+			.max_offset = qgroupid,
+			.max_transid = (u64)-1,
+			.nr_items = 4096, /* should be 2, i think */
+		},
+	};
+	struct qgroup_lookup qgroup_lookup;
+	struct btrfs_qgroup *qgroup;
+	struct rb_node *n;
+	int ret;
+
+	ret = __qgroups_search(fd, &args, &qgroup_lookup);
+	if (ret < 0)
+		return ret;
+
+	ret = -ENODATA;
+	n = rb_first(&qgroup_lookup.root);
+	if (n) {
+		qgroup = rb_entry(n, struct btrfs_qgroup, rb_node);
+		stats->qgroupid = qgroup->qgroupid;
+		stats->info = qgroup->info;
+		stats->limit = qgroup->limit;
+
+		ret = 0;
+	}
+
+	__free_all_qgroups(&qgroup_lookup);
+	return ret;
+}
+
 static void print_all_qgroups(struct qgroup_lookup *qgroup_lookup, bool verbose)
 {
 
@@ -1293,7 +1335,7 @@ int btrfs_show_qgroups(int fd,
 	struct qgroup_lookup sort_tree;
 	int ret;
 
-	ret = __qgroups_search(fd, &qgroup_lookup);
+	ret = qgroups_search_all(fd, &qgroup_lookup);
 	if (ret)
 		return ret;
 	__filter_and_sort_qgroups(&qgroup_lookup, &sort_tree,
diff --git a/qgroup.h b/qgroup.h
index 5e71349c..688f92b2 100644
--- a/qgroup.h
+++ b/qgroup.h
@@ -87,6 +87,12 @@ struct btrfs_qgroup_info {
 	u64 exclusive_compressed;
 };
 
+struct btrfs_qgroup_stats {
+	u64 qgroupid;
+	struct btrfs_qgroup_info info;
+	struct btrfs_qgroup_limit limit;
+};
+
 int btrfs_qgroup_parse_sort_string(const char *opt_arg,
 				struct btrfs_qgroup_comparer_set **comps);
 int btrfs_show_qgroups(int fd, struct btrfs_qgroup_filter_set *,
@@ -105,4 +111,5 @@ int qgroup_inherit_add_group(struct btrfs_qgroup_inherit **inherit, char *arg);
 int qgroup_inherit_add_copy(struct btrfs_qgroup_inherit **inherit, char *arg,
 			    int type);
 
+int btrfs_qgroup_query(int fd, u64 qgroupid, struct btrfs_qgroup_stats *stats);
 #endif
-- 
2.15.1

Re: [PATCH 6/8] btrfs-progs: qgroups: introduce btrfs_qgroup_query

[Qu Wenruo]

[-- Attachment #1.1: Type: text/plain, Size: 7806 bytes --]



On 2018年03月03日 02:47, jeffm@suse.com wrote:
> From: Jeff Mahoney <jeffm@suse.com>
> 
> The only mechanism we have in the progs for searching qgroups is to load
> all of them and filter the results.  This works for qgroup show but
> to add quota information to 'btrfs subvoluem show' it's pretty wasteful.
> 
> This patch splits out setting up the search and performing the search so
> we can search for a single qgroupid more easily.
> 
> Signed-off-by: Jeff Mahoney <jeffm@suse.com>
> ---
>  qgroup.c | 98 +++++++++++++++++++++++++++++++++++++++++++++-------------------
>  qgroup.h |  7 +++++
>  2 files changed, 77 insertions(+), 28 deletions(-)
> 
> diff --git a/qgroup.c b/qgroup.c
> index b1be3311..2d0a6947 100644
> --- a/qgroup.c
> +++ b/qgroup.c
> @@ -1146,11 +1146,11 @@ static inline void print_status_flag_warning(u64 flags)
>  		warning("qgroup data inconsistent, rescan recommended");
>  }
>  
> -static int __qgroups_search(int fd, struct qgroup_lookup *qgroup_lookup)
> +static int __qgroups_search(int fd, struct btrfs_ioctl_search_args *args,
> +			    struct qgroup_lookup *qgroup_lookup)
>  {
>  	int ret;
> -	struct btrfs_ioctl_search_args args;
> -	struct btrfs_ioctl_search_key *sk = &args.key;
> +	struct btrfs_ioctl_search_key *sk = &args->key;
>  	struct btrfs_ioctl_search_header *sh;
>  	unsigned long off = 0;
>  	unsigned int i;
> @@ -1161,30 +1161,12 @@ static int __qgroups_search(int fd, struct qgroup_lookup *qgroup_lookup)
>  	u64 qgroupid;
>  	u64 qgroupid1;
>  
> -	memset(&args, 0, sizeof(args));
> -
> -	sk->tree_id = BTRFS_QUOTA_TREE_OBJECTID;
> -	sk->max_type = BTRFS_QGROUP_RELATION_KEY;
> -	sk->min_type = BTRFS_QGROUP_STATUS_KEY;
> -	sk->max_objectid = (u64)-1;
> -	sk->max_offset = (u64)-1;
> -	sk->max_transid = (u64)-1;
> -	sk->nr_items = 4096;
> -
>  	qgroup_lookup_init(qgroup_lookup);
>  
>  	while (1) {
> -		ret = ioctl(fd, BTRFS_IOC_TREE_SEARCH, &args);
> +		ret = ioctl(fd, BTRFS_IOC_TREE_SEARCH, args);
>  		if (ret < 0) {
> -			if (errno == ENOENT) {
> -				error("can't list qgroups: quotas not enabled");
> -				ret = -ENOTTY;
> -			} else {
> -				error("can't list qgroups: %s",
> -				       strerror(errno));
> -				ret = -errno;
> -			}
> -
> +			ret = -errno;
>  			break;
>  		}
>  
> @@ -1198,14 +1180,14 @@ static int __qgroups_search(int fd, struct qgroup_lookup *qgroup_lookup)
>  		 * read the root_ref item it contains
>  		 */
>  		for (i = 0; i < sk->nr_items; i++) {
> -			sh = (struct btrfs_ioctl_search_header *)(args.buf +
> +			sh = (struct btrfs_ioctl_search_header *)(args->buf +
>  								  off);
>  			off += sizeof(*sh);
>  
>  			switch (btrfs_search_header_type(sh)) {
>  			case BTRFS_QGROUP_STATUS_KEY:
>  				si = (struct btrfs_qgroup_status_item *)
> -				     (args.buf + off);
> +				     (args->buf + off);
>  				flags = btrfs_stack_qgroup_status_flags(si);
>  
>  				print_status_flag_warning(flags);
> @@ -1213,7 +1195,7 @@ static int __qgroups_search(int fd, struct qgroup_lookup *qgroup_lookup)
>  			case BTRFS_QGROUP_INFO_KEY:
>  				qgroupid = btrfs_search_header_offset(sh);
>  				info = (struct btrfs_qgroup_info_item *)
> -				       (args.buf + off);
> +				       (args->buf + off);
>  
>  				ret = update_qgroup_info(fd, qgroup_lookup,
>  							 qgroupid, info);
> @@ -1221,7 +1203,7 @@ static int __qgroups_search(int fd, struct qgroup_lookup *qgroup_lookup)
>  			case BTRFS_QGROUP_LIMIT_KEY:
>  				qgroupid = btrfs_search_header_offset(sh);
>  				limit = (struct btrfs_qgroup_limit_item *)
> -					(args.buf + off);
> +					(args->buf + off);
>  
>  				ret = update_qgroup_limit(fd, qgroup_lookup,
>  							  qgroupid, limit);
> @@ -1267,6 +1249,66 @@ static int __qgroups_search(int fd, struct qgroup_lookup *qgroup_lookup)
>  	return ret;
>  }
>  
> +static int qgroups_search_all(int fd, struct qgroup_lookup *qgroup_lookup)
> +{
> +	struct btrfs_ioctl_search_args args = {
> +		.key = {
> +			.tree_id = BTRFS_QUOTA_TREE_OBJECTID,
> +			.max_type = BTRFS_QGROUP_RELATION_KEY,
> +			.min_type = BTRFS_QGROUP_STATUS_KEY,
> +			.max_objectid = (u64)-1,
> +			.max_offset = (u64)-1,
> +			.max_transid = (u64)-1,
> +			.nr_items = 4096,
> +		},
> +	};
> +	int ret;
> +
> +	ret = __qgroups_search(fd, &args, qgroup_lookup);
> +	if (ret == -ENOTTY)
> +		error("can't list qgroups: quotas not enabled");
> +	else if (ret < 0)
> +		error("can't list qgroups: %s", strerror(-ret));
> +	return ret;
> +}
> +
> +int btrfs_qgroup_query(int fd, u64 qgroupid, struct btrfs_qgroup_stats *stats)
> +{
> +	struct btrfs_ioctl_search_args args = {
> +		.key = {
> +			.tree_id = BTRFS_QUOTA_TREE_OBJECTID,
> +			.min_type = BTRFS_QGROUP_INFO_KEY,
> +			.max_type = BTRFS_QGROUP_LIMIT_KEY,
> +			.max_objectid = 0,
> +			.max_offset = qgroupid,
> +			.max_transid = (u64)-1,
> +			.nr_items = 4096, /* should be 2, i think */

2 is not correct in fact.

As QGROUP_INFO is smaller than QGROUP_LIMIT, to get a slice of all what
we need, we need to include all other unrelated items.

One example will be:
	item 1 key (0 QGROUP_INFO 0/5) itemoff 16211 itemsize 40
	item 2 key (0 QGROUP_INFO 0/257) itemoff 16171 itemsize 40
	item 3 key (0 QGROUP_INFO 1/1) itemoff 16131 itemsize 40
	item 4 key (0 QGROUP_LIMIT 0/5) itemoff 16091 itemsize 40
	item 5 key (0 QGROUP_LIMIT 0/257) itemoff 16051 itemsize 40
	item 6 key (0 QGROUP_LIMIT 1/1) itemoff 16011 itemsize 40

To query qgroup info about 0/257, above setup will get the following slice:
	item 1 key (0 QGROUP_INFO 0/5) itemoff 16211 itemsize 40
	item 2 key (0 QGROUP_INFO 0/257) itemoff 16171 itemsize 40
	item 3 key (0 QGROUP_INFO 1/1) itemoff 16131 itemsize 40
	item 4 key (0 QGROUP_LIMIT 0/5) itemoff 16091 itemsize 40
	item 5 key (0 QGROUP_LIMIT 0/257) itemoff 16051 itemsize 40
So we still need that large @nr_items.

Despite this comment it looks good.

Thanks,
Qu

> +		},
> +	};
> +	struct qgroup_lookup qgroup_lookup;
> +	struct btrfs_qgroup *qgroup;
> +	struct rb_node *n;
> +	int ret;
> +
> +	ret = __qgroups_search(fd, &args, &qgroup_lookup);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = -ENODATA;
> +	n = rb_first(&qgroup_lookup.root);
> +	if (n) {
> +		qgroup = rb_entry(n, struct btrfs_qgroup, rb_node);
> +		stats->qgroupid = qgroup->qgroupid;
> +		stats->info = qgroup->info;
> +		stats->limit = qgroup->limit;
> +
> +		ret = 0;
> +	}
> +
> +	__free_all_qgroups(&qgroup_lookup);
> +	return ret;
> +}
> +
>  static void print_all_qgroups(struct qgroup_lookup *qgroup_lookup, bool verbose)
>  {
>  
> @@ -1293,7 +1335,7 @@ int btrfs_show_qgroups(int fd,
>  	struct qgroup_lookup sort_tree;
>  	int ret;
>  
> -	ret = __qgroups_search(fd, &qgroup_lookup);
> +	ret = qgroups_search_all(fd, &qgroup_lookup);
>  	if (ret)
>  		return ret;
>  	__filter_and_sort_qgroups(&qgroup_lookup, &sort_tree,
> diff --git a/qgroup.h b/qgroup.h
> index 5e71349c..688f92b2 100644
> --- a/qgroup.h
> +++ b/qgroup.h
> @@ -87,6 +87,12 @@ struct btrfs_qgroup_info {
>  	u64 exclusive_compressed;
>  };
>  
> +struct btrfs_qgroup_stats {
> +	u64 qgroupid;
> +	struct btrfs_qgroup_info info;
> +	struct btrfs_qgroup_limit limit;
> +};
> +
>  int btrfs_qgroup_parse_sort_string(const char *opt_arg,
>  				struct btrfs_qgroup_comparer_set **comps);
>  int btrfs_show_qgroups(int fd, struct btrfs_qgroup_filter_set *,
> @@ -105,4 +111,5 @@ int qgroup_inherit_add_group(struct btrfs_qgroup_inherit **inherit, char *arg);
>  int qgroup_inherit_add_copy(struct btrfs_qgroup_inherit **inherit, char *arg,
>  			    int type);
>  
> +int btrfs_qgroup_query(int fd, u64 qgroupid, struct btrfs_qgroup_stats *stats);
>  #endif
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 520 bytes --]

Re: [PATCH 6/8] btrfs-progs: qgroups: introduce btrfs_qgroup_query

[Jeff Mahoney]

[-- Attachment #1.1: Type: text/plain, Size: 3475 bytes --]

On 3/7/18 12:58 AM, Qu Wenruo wrote:
> 
> 
> On 2018年03月03日 02:47, jeffm@suse.com wrote:
>> diff --git a/qgroup.c b/qgroup.c
>> index b1be3311..2d0a6947 100644
>> --- a/qgroup.c
>> +++ b/qgroup.c
>> @@ -1267,6 +1249,66 @@ static int __qgroups_search(int fd, struct qgroup_lookup *qgroup_lookup)
>>  	return ret;
>>  }
>>  
>> +static int qgroups_search_all(int fd, struct qgroup_lookup *qgroup_lookup)
>> +{
>> +	struct btrfs_ioctl_search_args args = {
>> +		.key = {
>> +			.tree_id = BTRFS_QUOTA_TREE_OBJECTID,
>> +			.max_type = BTRFS_QGROUP_RELATION_KEY,
>> +			.min_type = BTRFS_QGROUP_STATUS_KEY,
>> +			.max_objectid = (u64)-1,
>> +			.max_offset = (u64)-1,
>> +			.max_transid = (u64)-1,
>> +			.nr_items = 4096,
>> +		},
>> +	};
>> +	int ret;
>> +
>> +	ret = __qgroups_search(fd, &args, qgroup_lookup);
>> +	if (ret == -ENOTTY)
>> +		error("can't list qgroups: quotas not enabled");
>> +	else if (ret < 0)
>> +		error("can't list qgroups: %s", strerror(-ret));
>> +	return ret;
>> +}
>> +
>> +int btrfs_qgroup_query(int fd, u64 qgroupid, struct btrfs_qgroup_stats *stats)
>> +{
>> +	struct btrfs_ioctl_search_args args = {
>> +		.key = {
>> +			.tree_id = BTRFS_QUOTA_TREE_OBJECTID,
>> +			.min_type = BTRFS_QGROUP_INFO_KEY,
>> +			.max_type = BTRFS_QGROUP_LIMIT_KEY,
>> +			.max_objectid = 0,
>> +			.max_offset = qgroupid,
>> +			.max_transid = (u64)-1,
>> +			.nr_items = 4096, /* should be 2, i think */
> 
> 2 is not correct in fact.
> 
> As QGROUP_INFO is smaller than QGROUP_LIMIT, to get a slice of all what
> we need, we need to include all other unrelated items.
> 
> One example will be:
> 	item 1 key (0 QGROUP_INFO 0/5) itemoff 16211 itemsize 40
> 	item 2 key (0 QGROUP_INFO 0/257) itemoff 16171 itemsize 40
> 	item 3 key (0 QGROUP_INFO 1/1) itemoff 16131 itemsize 40
> 	item 4 key (0 QGROUP_LIMIT 0/5) itemoff 16091 itemsize 40
> 	item 5 key (0 QGROUP_LIMIT 0/257) itemoff 16051 itemsize 40
> 	item 6 key (0 QGROUP_LIMIT 1/1) itemoff 16011 itemsize 40
> 
> To query qgroup info about 0/257, above setup will get the following slice:
> 	item 1 key (0 QGROUP_INFO 0/5) itemoff 16211 itemsize 40
> 	item 2 key (0 QGROUP_INFO 0/257) itemoff 16171 itemsize 40
> 	item 3 key (0 QGROUP_INFO 1/1) itemoff 16131 itemsize 40
> 	item 4 key (0 QGROUP_LIMIT 0/5) itemoff 16091 itemsize 40
> 	item 5 key (0 QGROUP_LIMIT 0/257) itemoff 16051 itemsize 40
> So we still need that large @nr_items.
> 
> Despite this comment it looks good.

Of course.  I use TREE_SEARCH so infrequently that I forget about this
every time so the pain is always fresh.

It should be .min_offset = qgroupid, .nr_items = 2, but of course that
doesn't work either for different reasons.  __qgroups_search's loop will
loop until it comes back with no more results and it sets the nr_items
itself to 4096 at the end of the loop.  The key comparison in the ioctl
only does a regular key comparison and offset doesn't get evaluated if
the types aren't equal.  That works fine when doing tree insertion or
searches for a single key but is wrong for searching for a range.  I
have a TREE_SEARCH_V3 lying around somewhere to address this ridiculous
behavior and should probably finish it up at some point.

This hasn't mattered for __qgroup_search until now since it hasn't used
anything other than -1 for the offset and objectid so I'll just add a
filter there.

-Jeff

-- 
Jeff Mahoney
SUSE Labs


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 854 bytes --]

Re: [PATCH 6/8] btrfs-progs: qgroups: introduce btrfs_qgroup_query

[Qu Wenruo]

[-- Attachment #1.1: Type: text/plain, Size: 7079 bytes --]



On 2018年03月03日 02:47, jeffm@suse.com wrote:
> From: Jeff Mahoney <jeffm@suse.com>
> 
> The only mechanism we have in the progs for searching qgroups is to load
> all of them and filter the results.  This works for qgroup show but
> to add quota information to 'btrfs subvoluem show' it's pretty wasteful.
> 
> This patch splits out setting up the search and performing the search so
> we can search for a single qgroupid more easily.
> 
> Signed-off-by: Jeff Mahoney <jeffm@suse.com>
> ---
>  qgroup.c | 98 +++++++++++++++++++++++++++++++++++++++++++++-------------------
>  qgroup.h |  7 +++++
>  2 files changed, 77 insertions(+), 28 deletions(-)
> 
> diff --git a/qgroup.c b/qgroup.c
> index b1be3311..2d0a6947 100644
> --- a/qgroup.c
> +++ b/qgroup.c
> @@ -1146,11 +1146,11 @@ static inline void print_status_flag_warning(u64 flags)
>  		warning("qgroup data inconsistent, rescan recommended");
>  }
>  
> -static int __qgroups_search(int fd, struct qgroup_lookup *qgroup_lookup)
> +static int __qgroups_search(int fd, struct btrfs_ioctl_search_args *args,
> +			    struct qgroup_lookup *qgroup_lookup)
>  {
>  	int ret;
> -	struct btrfs_ioctl_search_args args;
> -	struct btrfs_ioctl_search_key *sk = &args.key;
> +	struct btrfs_ioctl_search_key *sk = &args->key;
>  	struct btrfs_ioctl_search_header *sh;
>  	unsigned long off = 0;
>  	unsigned int i;
> @@ -1161,30 +1161,12 @@ static int __qgroups_search(int fd, struct qgroup_lookup *qgroup_lookup)
>  	u64 qgroupid;
>  	u64 qgroupid1;
>  
> -	memset(&args, 0, sizeof(args));
> -
> -	sk->tree_id = BTRFS_QUOTA_TREE_OBJECTID;
> -	sk->max_type = BTRFS_QGROUP_RELATION_KEY;
> -	sk->min_type = BTRFS_QGROUP_STATUS_KEY;
> -	sk->max_objectid = (u64)-1;
> -	sk->max_offset = (u64)-1;
> -	sk->max_transid = (u64)-1;
> -	sk->nr_items = 4096;
> -
>  	qgroup_lookup_init(qgroup_lookup);
>  
>  	while (1) {
> -		ret = ioctl(fd, BTRFS_IOC_TREE_SEARCH, &args);
> +		ret = ioctl(fd, BTRFS_IOC_TREE_SEARCH, args);
>  		if (ret < 0) {
> -			if (errno == ENOENT) {
> -				error("can't list qgroups: quotas not enabled");
> -				ret = -ENOTTY;
> -			} else {
> -				error("can't list qgroups: %s",
> -				       strerror(errno));
> -				ret = -errno;
> -			}
> -
> +			ret = -errno;
>  			break;
>  		}
>  
> @@ -1198,14 +1180,14 @@ static int __qgroups_search(int fd, struct qgroup_lookup *qgroup_lookup)
>  		 * read the root_ref item it contains
>  		 */
>  		for (i = 0; i < sk->nr_items; i++) {
> -			sh = (struct btrfs_ioctl_search_header *)(args.buf +
> +			sh = (struct btrfs_ioctl_search_header *)(args->buf +
>  								  off);
>  			off += sizeof(*sh);
>  
>  			switch (btrfs_search_header_type(sh)) {
>  			case BTRFS_QGROUP_STATUS_KEY:
>  				si = (struct btrfs_qgroup_status_item *)
> -				     (args.buf + off);
> +				     (args->buf + off);
>  				flags = btrfs_stack_qgroup_status_flags(si);
>  
>  				print_status_flag_warning(flags);
> @@ -1213,7 +1195,7 @@ static int __qgroups_search(int fd, struct qgroup_lookup *qgroup_lookup)
>  			case BTRFS_QGROUP_INFO_KEY:
>  				qgroupid = btrfs_search_header_offset(sh);
>  				info = (struct btrfs_qgroup_info_item *)
> -				       (args.buf + off);
> +				       (args->buf + off);
>  
>  				ret = update_qgroup_info(fd, qgroup_lookup,
>  							 qgroupid, info);
> @@ -1221,7 +1203,7 @@ static int __qgroups_search(int fd, struct qgroup_lookup *qgroup_lookup)
>  			case BTRFS_QGROUP_LIMIT_KEY:
>  				qgroupid = btrfs_search_header_offset(sh);
>  				limit = (struct btrfs_qgroup_limit_item *)
> -					(args.buf + off);
> +					(args->buf + off);
>  
>  				ret = update_qgroup_limit(fd, qgroup_lookup,
>  							  qgroupid, limit);
> @@ -1267,6 +1249,66 @@ static int __qgroups_search(int fd, struct qgroup_lookup *qgroup_lookup)
>  	return ret;
>  }
>  
> +static int qgroups_search_all(int fd, struct qgroup_lookup *qgroup_lookup)
> +{
> +	struct btrfs_ioctl_search_args args = {
> +		.key = {
> +			.tree_id = BTRFS_QUOTA_TREE_OBJECTID,
> +			.max_type = BTRFS_QGROUP_RELATION_KEY,
> +			.min_type = BTRFS_QGROUP_STATUS_KEY,
> +			.max_objectid = (u64)-1,
> +			.max_offset = (u64)-1,
> +			.max_transid = (u64)-1,
> +			.nr_items = 4096,
> +		},
> +	};
> +	int ret;
> +
> +	ret = __qgroups_search(fd, &args, qgroup_lookup);
> +	if (ret == -ENOTTY)
> +		error("can't list qgroups: quotas not enabled");
> +	else if (ret < 0)
> +		error("can't list qgroups: %s", strerror(-ret));
> +	return ret;
> +}
> +
> +int btrfs_qgroup_query(int fd, u64 qgroupid, struct btrfs_qgroup_stats *stats)
> +{
> +	struct btrfs_ioctl_search_args args = {
> +		.key = {
> +			.tree_id = BTRFS_QUOTA_TREE_OBJECTID,
> +			.min_type = BTRFS_QGROUP_INFO_KEY,
> +			.max_type = BTRFS_QGROUP_LIMIT_KEY,
> +			.max_objectid = 0,
> +			.max_offset = qgroupid,
> +			.max_transid = (u64)-1,
> +			.nr_items = 4096, /* should be 2, i think */
> +		},
> +	};
> +	struct qgroup_lookup qgroup_lookup;
> +	struct btrfs_qgroup *qgroup;
> +	struct rb_node *n;
> +	int ret;
> +
> +	ret = __qgroups_search(fd, &args, &qgroup_lookup);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = -ENODATA;
> +	n = rb_first(&qgroup_lookup.root);
> +	if (n) {
> +		qgroup = rb_entry(n, struct btrfs_qgroup, rb_node);
> +		stats->qgroupid = qgroup->qgroupid;
> +		stats->info = qgroup->info;
> +		stats->limit = qgroup->limit;

I just missed this.

Even we are just doing single query, just as explained in previous
reply, we could get a splice containing other info of other qgroups.

So here the first qgroup could not be the one we want.
We need to do extra search to find the qgroup we need.

Thanks,
Qu

> +
> +		ret = 0;
> +	}
> +
> +	__free_all_qgroups(&qgroup_lookup);
> +	return ret;
> +}
> +
>  static void print_all_qgroups(struct qgroup_lookup *qgroup_lookup, bool verbose)
>  {
>  
> @@ -1293,7 +1335,7 @@ int btrfs_show_qgroups(int fd,
>  	struct qgroup_lookup sort_tree;
>  	int ret;
>  
> -	ret = __qgroups_search(fd, &qgroup_lookup);
> +	ret = qgroups_search_all(fd, &qgroup_lookup);
>  	if (ret)
>  		return ret;
>  	__filter_and_sort_qgroups(&qgroup_lookup, &sort_tree,
> diff --git a/qgroup.h b/qgroup.h
> index 5e71349c..688f92b2 100644
> --- a/qgroup.h
> +++ b/qgroup.h
> @@ -87,6 +87,12 @@ struct btrfs_qgroup_info {
>  	u64 exclusive_compressed;
>  };
>  
> +struct btrfs_qgroup_stats {
> +	u64 qgroupid;
> +	struct btrfs_qgroup_info info;
> +	struct btrfs_qgroup_limit limit;
> +};
> +
>  int btrfs_qgroup_parse_sort_string(const char *opt_arg,
>  				struct btrfs_qgroup_comparer_set **comps);
>  int btrfs_show_qgroups(int fd, struct btrfs_qgroup_filter_set *,
> @@ -105,4 +111,5 @@ int qgroup_inherit_add_group(struct btrfs_qgroup_inherit **inherit, char *arg);
>  int qgroup_inherit_add_copy(struct btrfs_qgroup_inherit **inherit, char *arg,
>  			    int type);
>  
> +int btrfs_qgroup_query(int fd, u64 qgroupid, struct btrfs_qgroup_stats *stats);
>  #endif
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 520 bytes --]

Re: [PATCH 6/8] btrfs-progs: qgroups: introduce btrfs_qgroup_query

[Misono, Tomohiro]

On 2018/03/03 3:47, jeffm@suse.com wrote:
> From: Jeff Mahoney <jeffm@suse.com>
> 
> The only mechanism we have in the progs for searching qgroups is to load
> all of them and filter the results.  This works for qgroup show but
> to add quota information to 'btrfs subvoluem show' it's pretty wasteful.
> 
> This patch splits out setting up the search and performing the search so
> we can search for a single qgroupid more easily.
> 
> Signed-off-by: Jeff Mahoney <jeffm@suse.com>
> ---
>  qgroup.c | 98 +++++++++++++++++++++++++++++++++++++++++++++-------------------
>  qgroup.h |  7 +++++
>  2 files changed, 77 insertions(+), 28 deletions(-)
> 
> diff --git a/qgroup.c b/qgroup.c
> index b1be3311..2d0a6947 100644
> --- a/qgroup.c
> +++ b/qgroup.c
> @@ -1146,11 +1146,11 @@ static inline void print_status_flag_warning(u64 flags)
>  		warning("qgroup data inconsistent, rescan recommended");
>  }
>  
> -static int __qgroups_search(int fd, struct qgroup_lookup *qgroup_lookup)
> +static int __qgroups_search(int fd, struct btrfs_ioctl_search_args *args,
> +			    struct qgroup_lookup *qgroup_lookup)
>  {
>  	int ret;
> -	struct btrfs_ioctl_search_args args;
> -	struct btrfs_ioctl_search_key *sk = &args.key;
> +	struct btrfs_ioctl_search_key *sk = &args->key;
>  	struct btrfs_ioctl_search_header *sh;
>  	unsigned long off = 0;
>  	unsigned int i;
> @@ -1161,30 +1161,12 @@ static int __qgroups_search(int fd, struct qgroup_lookup *qgroup_lookup)
>  	u64 qgroupid;
>  	u64 qgroupid1;
>  
> -	memset(&args, 0, sizeof(args));
> -
> -	sk->tree_id = BTRFS_QUOTA_TREE_OBJECTID;
> -	sk->max_type = BTRFS_QGROUP_RELATION_KEY;
> -	sk->min_type = BTRFS_QGROUP_STATUS_KEY;
> -	sk->max_objectid = (u64)-1;
> -	sk->max_offset = (u64)-1;
> -	sk->max_transid = (u64)-1;
> -	sk->nr_items = 4096;
> -
>  	qgroup_lookup_init(qgroup_lookup);
>  
>  	while (1) {
> -		ret = ioctl(fd, BTRFS_IOC_TREE_SEARCH, &args);
> +		ret = ioctl(fd, BTRFS_IOC_TREE_SEARCH, args);
>  		if (ret < 0) {
> -			if (errno == ENOENT) {
> -				error("can't list qgroups: quotas not enabled");
> -				ret = -ENOTTY;
> -			} else {
> -				error("can't list qgroups: %s",
> -				       strerror(errno));
> -				ret = -errno;
> -			}
> -
> +			ret = -errno;

Originally, -ENOTTY would be returned when qgroup is disabled
but this changes to return -ENOENT. so, it seems that error check
in 7th patch would not work correctly when qgroup is disabled.

>  			break;
>  		}
>  
> @@ -1198,14 +1180,14 @@ static int __qgroups_search(int fd, struct qgroup_lookup *qgroup_lookup)
>  		 * read the root_ref item it contains
>  		 */
>  		for (i = 0; i < sk->nr_items; i++) {
> -			sh = (struct btrfs_ioctl_search_header *)(args.buf +
> +			sh = (struct btrfs_ioctl_search_header *)(args->buf +
>  								  off);
>  			off += sizeof(*sh);
>  
>  			switch (btrfs_search_header_type(sh)) {
>  			case BTRFS_QGROUP_STATUS_KEY:
>  				si = (struct btrfs_qgroup_status_item *)
> -				     (args.buf + off);
> +				     (args->buf + off);
>  				flags = btrfs_stack_qgroup_status_flags(si);
>  
>  				print_status_flag_warning(flags);
> @@ -1213,7 +1195,7 @@ static int __qgroups_search(int fd, struct qgroup_lookup *qgroup_lookup)
>  			case BTRFS_QGROUP_INFO_KEY:
>  				qgroupid = btrfs_search_header_offset(sh);
>  				info = (struct btrfs_qgroup_info_item *)
> -				       (args.buf + off);
> +				       (args->buf + off);
>  
>  				ret = update_qgroup_info(fd, qgroup_lookup,
>  							 qgroupid, info);
> @@ -1221,7 +1203,7 @@ static int __qgroups_search(int fd, struct qgroup_lookup *qgroup_lookup)
>  			case BTRFS_QGROUP_LIMIT_KEY:
>  				qgroupid = btrfs_search_header_offset(sh);
>  				limit = (struct btrfs_qgroup_limit_item *)
> -					(args.buf + off);
> +					(args->buf + off);
>  
>  				ret = update_qgroup_limit(fd, qgroup_lookup,
>  							  qgroupid, limit);
> @@ -1267,6 +1249,66 @@ static int __qgroups_search(int fd, struct qgroup_lookup *qgroup_lookup)
>  	return ret;
>  }
>  
> +static int qgroups_search_all(int fd, struct qgroup_lookup *qgroup_lookup)
> +{
> +	struct btrfs_ioctl_search_args args = {
> +		.key = {
> +			.tree_id = BTRFS_QUOTA_TREE_OBJECTID,
> +			.max_type = BTRFS_QGROUP_RELATION_KEY,
> +			.min_type = BTRFS_QGROUP_STATUS_KEY,
> +			.max_objectid = (u64)-1,
> +			.max_offset = (u64)-1,
> +			.max_transid = (u64)-1,
> +			.nr_items = 4096,
> +		},
> +	};
> +	int ret;
> +
> +	ret = __qgroups_search(fd, &args, qgroup_lookup);
> +	if (ret == -ENOTTY)
> +		error("can't list qgroups: quotas not enabled");
> +	else if (ret < 0)
> +		error("can't list qgroups: %s", strerror(-ret));
> +	return ret;
> +}
> +
> +int btrfs_qgroup_query(int fd, u64 qgroupid, struct btrfs_qgroup_stats *stats)
> +{
> +	struct btrfs_ioctl_search_args args = {
> +		.key = {
> +			.tree_id = BTRFS_QUOTA_TREE_OBJECTID,
> +			.min_type = BTRFS_QGROUP_INFO_KEY,
> +			.max_type = BTRFS_QGROUP_LIMIT_KEY,
> +			.max_objectid = 0,
> +			.max_offset = qgroupid,
> +			.max_transid = (u64)-1,
> +			.nr_items = 4096, /* should be 2, i think */
> +		},
> +	};
> +	struct qgroup_lookup qgroup_lookup;
> +	struct btrfs_qgroup *qgroup;
> +	struct rb_node *n;
> +	int ret;
> +
> +	ret = __qgroups_search(fd, &args, &qgroup_lookup);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = -ENODATA;
> +	n = rb_first(&qgroup_lookup.root);
> +	if (n) {
> +		qgroup = rb_entry(n, struct btrfs_qgroup, rb_node);
> +		stats->qgroupid = qgroup->qgroupid;
> +		stats->info = qgroup->info;
> +		stats->limit = qgroup->limit;
> +
> +		ret = 0;
> +	}
> +
> +	__free_all_qgroups(&qgroup_lookup);
> +	return ret;
> +}
> +
>  static void print_all_qgroups(struct qgroup_lookup *qgroup_lookup, bool verbose)
>  {
>  
> @@ -1293,7 +1335,7 @@ int btrfs_show_qgroups(int fd,
>  	struct qgroup_lookup sort_tree;
>  	int ret;
>  
> -	ret = __qgroups_search(fd, &qgroup_lookup);
> +	ret = qgroups_search_all(fd, &qgroup_lookup);
>  	if (ret)
>  		return ret;
>  	__filter_and_sort_qgroups(&qgroup_lookup, &sort_tree,
> diff --git a/qgroup.h b/qgroup.h
> index 5e71349c..688f92b2 100644
> --- a/qgroup.h
> +++ b/qgroup.h
> @@ -87,6 +87,12 @@ struct btrfs_qgroup_info {
>  	u64 exclusive_compressed;
>  };
>  
> +struct btrfs_qgroup_stats {
> +	u64 qgroupid;
> +	struct btrfs_qgroup_info info;
> +	struct btrfs_qgroup_limit limit;
> +};
> +
>  int btrfs_qgroup_parse_sort_string(const char *opt_arg,
>  				struct btrfs_qgroup_comparer_set **comps);
>  int btrfs_show_qgroups(int fd, struct btrfs_qgroup_filter_set *,
> @@ -105,4 +111,5 @@ int qgroup_inherit_add_group(struct btrfs_qgroup_inherit **inherit, char *arg);
>  int qgroup_inherit_add_copy(struct btrfs_qgroup_inherit **inherit, char *arg,
>  			    int type);
>  
> +int btrfs_qgroup_query(int fd, u64 qgroupid, struct btrfs_qgroup_stats *stats);
>  #endif
> 

Re: [PATCH 6/8] btrfs-progs: qgroups: introduce btrfs_qgroup_query

[Jeff Mahoney]

[-- Attachment #1.1: Type: text/plain, Size: 2670 bytes --]

On 3/7/18 3:02 AM, Misono, Tomohiro wrote:
> On 2018/03/03 3:47, jeffm@suse.com wrote:
>> From: Jeff Mahoney <jeffm@suse.com>
>>
>> The only mechanism we have in the progs for searching qgroups is to load
>> all of them and filter the results.  This works for qgroup show but
>> to add quota information to 'btrfs subvoluem show' it's pretty wasteful.
>>
>> This patch splits out setting up the search and performing the search so
>> we can search for a single qgroupid more easily.
>>
>> Signed-off-by: Jeff Mahoney <jeffm@suse.com>
>> ---
>>  qgroup.c | 98 +++++++++++++++++++++++++++++++++++++++++++++-------------------
>>  qgroup.h |  7 +++++
>>  2 files changed, 77 insertions(+), 28 deletions(-)
>>
>> diff --git a/qgroup.c b/qgroup.c
>> index b1be3311..2d0a6947 100644
>> --- a/qgroup.c
>> +++ b/qgroup.c
>> @@ -1146,11 +1146,11 @@ static inline void print_status_flag_warning(u64 flags)
>>  		warning("qgroup data inconsistent, rescan recommended");
>>  }
>>  
>> -static int __qgroups_search(int fd, struct qgroup_lookup *qgroup_lookup)
>> +static int __qgroups_search(int fd, struct btrfs_ioctl_search_args *args,
>> +			    struct qgroup_lookup *qgroup_lookup)
>>  {
>>  	int ret;
>> -	struct btrfs_ioctl_search_args args;
>> -	struct btrfs_ioctl_search_key *sk = &args.key;
>> +	struct btrfs_ioctl_search_key *sk = &args->key;
>>  	struct btrfs_ioctl_search_header *sh;
>>  	unsigned long off = 0;
>>  	unsigned int i;
>> @@ -1161,30 +1161,12 @@ static int __qgroups_search(int fd, struct qgroup_lookup *qgroup_lookup)
>>  	u64 qgroupid;
>>  	u64 qgroupid1;
>>  
>> -	memset(&args, 0, sizeof(args));
>> -
>> -	sk->tree_id = BTRFS_QUOTA_TREE_OBJECTID;
>> -	sk->max_type = BTRFS_QGROUP_RELATION_KEY;
>> -	sk->min_type = BTRFS_QGROUP_STATUS_KEY;
>> -	sk->max_objectid = (u64)-1;
>> -	sk->max_offset = (u64)-1;
>> -	sk->max_transid = (u64)-1;
>> -	sk->nr_items = 4096;
>> -
>>  	qgroup_lookup_init(qgroup_lookup);
>>  
>>  	while (1) {
>> -		ret = ioctl(fd, BTRFS_IOC_TREE_SEARCH, &args);
>> +		ret = ioctl(fd, BTRFS_IOC_TREE_SEARCH, args);
>>  		if (ret < 0) {
>> -			if (errno == ENOENT) {
>> -				error("can't list qgroups: quotas not enabled");
>> -				ret = -ENOTTY;
>> -			} else {
>> -				error("can't list qgroups: %s",
>> -				       strerror(errno));
>> -				ret = -errno;
>> -			}
>> -
>> +			ret = -errno;
> 
> Originally, -ENOTTY would be returned when qgroup is disabled
> but this changes to return -ENOENT. so, it seems that error check
> in 7th patch would not work correctly when qgroup is disabled.
> 

Good catch.

Thanks,

-Jeff

-- 
Jeff Mahoney
SUSE Labs


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 854 bytes --]

[PATCH 7/8] btrfs-progs: subvolume: add quota info to btrfs sub show

[jeffm]

From: Jeff Mahoney <jeffm@suse.com>

This patch reports on the first-level qgroup, if any, associated with
a particular subvolume.  It displays the usage and limit, subject
to the usual unit parameters.

Signed-off-by: Jeff Mahoney <jeffm@suse.com>
---
 cmds-subvolume.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 46 insertions(+)

diff --git a/cmds-subvolume.c b/cmds-subvolume.c
index 8a473f7a..29d0e0e5 100644
--- a/cmds-subvolume.c
+++ b/cmds-subvolume.c
@@ -972,6 +972,7 @@ static const char * const cmd_subvol_show_usage[] = {
 	"Show more information about the subvolume",
 	"-r|--rootid   rootid of the subvolume",
 	"-u|--uuid     uuid of the subvolume",
+	HELPINFO_UNITS_SHORT_LONG,
 	"",
 	"If no option is specified, <subvol-path> will be shown, otherwise",
 	"the rootid or uuid are resolved relative to the <mnt> path.",
@@ -993,6 +994,13 @@ static int cmd_subvol_show(int argc, char **argv)
 	int by_uuid = 0;
 	u64 rootid_arg;
 	u8 uuid_arg[BTRFS_UUID_SIZE];
+	struct btrfs_qgroup_stats stats;
+	unsigned int unit_mode;
+	const char *referenced_size;
+	const char *referenced_limit_size = "-";
+	unsigned field_width = 0;
+
+	unit_mode = get_unit_mode_from_arg(&argc, argv, 1);
 
 	while (1) {
 		int c;
@@ -1112,6 +1120,44 @@ static int cmd_subvol_show(int argc, char **argv)
 	btrfs_list_subvols_print(fd, filter_set, NULL, BTRFS_LIST_LAYOUT_RAW,
 			1, raw_prefix);
 
+	ret = btrfs_qgroup_query(fd, get_ri.root_id, &stats);
+	if (ret < 0) {
+		if (ret == -ENODATA)
+			printf("Quotas must be enabled for per-subvolume usage\n");
+		else if (ret != -ENOTTY)
+			fprintf(stderr,
+				"\nERROR: BTRFS_IOC_QUOTA_QUERY failed: %s\n",
+				strerror(errno));
+		goto out;
+	}
+
+	printf("\tQuota Usage:\t\t");
+	fflush(stdout);
+
+	referenced_size = pretty_size_mode(stats.info.referenced, unit_mode);
+	if (stats.limit.max_referenced)
+	       referenced_limit_size = pretty_size_mode(
+						stats.limit.max_referenced,
+						unit_mode);
+	field_width = max(strlen(referenced_size),
+			  strlen(referenced_limit_size));
+
+	printf("%-*s referenced, %s exclusive\n ", field_width,
+	       referenced_size,
+	       pretty_size_mode(stats.info.exclusive, unit_mode));
+
+	printf("\tQuota Limits:\t\t");
+	if (stats.limit.max_referenced || stats.limit.max_exclusive) {
+		const char *excl = "-";
+
+		if (stats.limit.max_exclusive)
+		       excl = pretty_size_mode(stats.limit.max_exclusive,
+					       unit_mode);
+		printf("%-*s referenced, %s exclusive\n", field_width,
+		       referenced_limit_size, excl);
+	} else
+		printf("None\n");
+
 out:
 	/* clean up */
 	free(get_ri.path);
-- 
2.15.1

Re: [PATCH 7/8] btrfs-progs: subvolume: add quota info to btrfs sub show

[Qu Wenruo]

[-- Attachment #1.1: Type: text/plain, Size: 3114 bytes --]



On 2018年03月03日 02:47, jeffm@suse.com wrote:
> From: Jeff Mahoney <jeffm@suse.com>
> 
> This patch reports on the first-level qgroup, if any, associated with
> a particular subvolume.  It displays the usage and limit, subject
> to the usual unit parameters.
> 
> Signed-off-by: Jeff Mahoney <jeffm@suse.com>
> ---
>  cmds-subvolume.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 46 insertions(+)
> 
> diff --git a/cmds-subvolume.c b/cmds-subvolume.c
> index 8a473f7a..29d0e0e5 100644
> --- a/cmds-subvolume.c
> +++ b/cmds-subvolume.c
> @@ -972,6 +972,7 @@ static const char * const cmd_subvol_show_usage[] = {
>  	"Show more information about the subvolume",
>  	"-r|--rootid   rootid of the subvolume",
>  	"-u|--uuid     uuid of the subvolume",
> +	HELPINFO_UNITS_SHORT_LONG,
>  	"",
>  	"If no option is specified, <subvol-path> will be shown, otherwise",
>  	"the rootid or uuid are resolved relative to the <mnt> path.",
> @@ -993,6 +994,13 @@ static int cmd_subvol_show(int argc, char **argv)
>  	int by_uuid = 0;
>  	u64 rootid_arg;
>  	u8 uuid_arg[BTRFS_UUID_SIZE];
> +	struct btrfs_qgroup_stats stats;
> +	unsigned int unit_mode;
> +	const char *referenced_size;
> +	const char *referenced_limit_size = "-";
> +	unsigned field_width = 0;
> +
> +	unit_mode = get_unit_mode_from_arg(&argc, argv, 1);
>  
>  	while (1) {
>  		int c;
> @@ -1112,6 +1120,44 @@ static int cmd_subvol_show(int argc, char **argv)
>  	btrfs_list_subvols_print(fd, filter_set, NULL, BTRFS_LIST_LAYOUT_RAW,
>  			1, raw_prefix);
>  
> +	ret = btrfs_qgroup_query(fd, get_ri.root_id, &stats);
> +	if (ret < 0) {
> +		if (ret == -ENODATA)
> +			printf("Quotas must be enabled for per-subvolume usage\n");

This seems a little confusing.
If quota is disabled, we get ENOTTY not ENODATA.

For ENODATA we know quota is enabled but just no info for this qgroup.

Thanks,
Qu

> +		else if (ret != -ENOTTY)
> +			fprintf(stderr,
> +				"\nERROR: BTRFS_IOC_QUOTA_QUERY failed: %s\n",
> +				strerror(errno));
> +		goto out;
> +	}
> +
> +	printf("\tQuota Usage:\t\t");
> +	fflush(stdout);
> +
> +	referenced_size = pretty_size_mode(stats.info.referenced, unit_mode);
> +	if (stats.limit.max_referenced)
> +	       referenced_limit_size = pretty_size_mode(
> +						stats.limit.max_referenced,
> +						unit_mode);
> +	field_width = max(strlen(referenced_size),
> +			  strlen(referenced_limit_size));
> +
> +	printf("%-*s referenced, %s exclusive\n ", field_width,
> +	       referenced_size,
> +	       pretty_size_mode(stats.info.exclusive, unit_mode));
> +
> +	printf("\tQuota Limits:\t\t");
> +	if (stats.limit.max_referenced || stats.limit.max_exclusive) {
> +		const char *excl = "-";
> +
> +		if (stats.limit.max_exclusive)
> +		       excl = pretty_size_mode(stats.limit.max_exclusive,
> +					       unit_mode);
> +		printf("%-*s referenced, %s exclusive\n", field_width,
> +		       referenced_limit_size, excl);
> +	} else
> +		printf("None\n");
> +
>  out:
>  	/* clean up */
>  	free(get_ri.path);
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 520 bytes --]

Re: [PATCH 7/8] btrfs-progs: subvolume: add quota info to btrfs sub show

[Jeff Mahoney]

[-- Attachment #1.1: Type: text/plain, Size: 3320 bytes --]

On 3/7/18 1:09 AM, Qu Wenruo wrote:
> 
> 
> On 2018年03月03日 02:47, jeffm@suse.com wrote:
>> From: Jeff Mahoney <jeffm@suse.com>
>>
>> This patch reports on the first-level qgroup, if any, associated with
>> a particular subvolume.  It displays the usage and limit, subject
>> to the usual unit parameters.
>>
>> Signed-off-by: Jeff Mahoney <jeffm@suse.com>
>> ---
>>  cmds-subvolume.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 46 insertions(+)
>>
>> diff --git a/cmds-subvolume.c b/cmds-subvolume.c
>> index 8a473f7a..29d0e0e5 100644
>> --- a/cmds-subvolume.c
>> +++ b/cmds-subvolume.c
>> @@ -972,6 +972,7 @@ static const char * const cmd_subvol_show_usage[] = {
>>  	"Show more information about the subvolume",
>>  	"-r|--rootid   rootid of the subvolume",
>>  	"-u|--uuid     uuid of the subvolume",
>> +	HELPINFO_UNITS_SHORT_LONG,
>>  	"",
>>  	"If no option is specified, <subvol-path> will be shown, otherwise",
>>  	"the rootid or uuid are resolved relative to the <mnt> path.",
>> @@ -993,6 +994,13 @@ static int cmd_subvol_show(int argc, char **argv)
>>  	int by_uuid = 0;
>>  	u64 rootid_arg;
>>  	u8 uuid_arg[BTRFS_UUID_SIZE];
>> +	struct btrfs_qgroup_stats stats;
>> +	unsigned int unit_mode;
>> +	const char *referenced_size;
>> +	const char *referenced_limit_size = "-";
>> +	unsigned field_width = 0;
>> +
>> +	unit_mode = get_unit_mode_from_arg(&argc, argv, 1);
>>  
>>  	while (1) {
>>  		int c;
>> @@ -1112,6 +1120,44 @@ static int cmd_subvol_show(int argc, char **argv)
>>  	btrfs_list_subvols_print(fd, filter_set, NULL, BTRFS_LIST_LAYOUT_RAW,
>>  			1, raw_prefix);
>>  
>> +	ret = btrfs_qgroup_query(fd, get_ri.root_id, &stats);
>> +	if (ret < 0) {
>> +		if (ret == -ENODATA)
>> +			printf("Quotas must be enabled for per-subvolume usage\n");
> 
> This seems a little confusing.
> If quota is disabled, we get ENOTTY not ENODATA.
> 
> For ENODATA we know quota is enabled but just no info for this qgroup.

Yep.

Thanks,

-Jeff


> Thanks,
> Qu
> 
>> +		else if (ret != -ENOTTY)
>> +			fprintf(stderr,
>> +				"\nERROR: BTRFS_IOC_QUOTA_QUERY failed: %s\n",
>> +				strerror(errno));
>> +		goto out;
>> +	}
>> +
>> +	printf("\tQuota Usage:\t\t");
>> +	fflush(stdout);
>> +
>> +	referenced_size = pretty_size_mode(stats.info.referenced, unit_mode);
>> +	if (stats.limit.max_referenced)
>> +	       referenced_limit_size = pretty_size_mode(
>> +						stats.limit.max_referenced,
>> +						unit_mode);
>> +	field_width = max(strlen(referenced_size),
>> +			  strlen(referenced_limit_size));
>> +
>> +	printf("%-*s referenced, %s exclusive\n ", field_width,
>> +	       referenced_size,
>> +	       pretty_size_mode(stats.info.exclusive, unit_mode));
>> +
>> +	printf("\tQuota Limits:\t\t");
>> +	if (stats.limit.max_referenced || stats.limit.max_exclusive) {
>> +		const char *excl = "-";
>> +
>> +		if (stats.limit.max_exclusive)
>> +		       excl = pretty_size_mode(stats.limit.max_exclusive,
>> +					       unit_mode);
>> +		printf("%-*s referenced, %s exclusive\n", field_width,
>> +		       referenced_limit_size, excl);
>> +	} else
>> +		printf("None\n");
>> +
>>  out:
>>  	/* clean up */
>>  	free(get_ri.path);
>>
> 


-- 
Jeff Mahoney
SUSE Labs


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 854 bytes --]

[PATCH 8/8] btrfs-progs: qgroups: export qgroups usage information as JSON

[jeffm]

From: Jeff Mahoney <jeffm@suse.com>

One of the common requests I receive is for 'df' like facilities
for subvolume usage.  Really, the request is for monitoring tools to be
able to understand when subvolumes may be approaching quota in the same
manner traditional file systems approach ENOSPC.

This patch allows us to export the qgroups data in a machine-readable
format so that monitoring tools can parse it easily.

There are two modes since JSON can technically handle 64-bit numbers
but JavaScript proper cannot.  show -j enables JSON mode using 64-bit
integers directly.  --json-compat presents 64-bit numbers as an array
of two 32-bit numbers (high followed by low).

Signed-off-by: Jeff Mahoney <jeffm@suse.com>
---
 Documentation/btrfs-qgroup.asciidoc |   4 +
 Makefile.inc.in                     |   4 +-
 cmds-qgroup.c                       |  36 +++++-
 configure.ac                        |   6 +
 qgroup.c                            | 211 ++++++++++++++++++++++++++++++++++++
 qgroup.h                            |   3 +
 6 files changed, 258 insertions(+), 6 deletions(-)

diff --git a/Documentation/btrfs-qgroup.asciidoc b/Documentation/btrfs-qgroup.asciidoc
index 360b3269..22a9c2a7 100644
--- a/Documentation/btrfs-qgroup.asciidoc
+++ b/Documentation/btrfs-qgroup.asciidoc
@@ -105,6 +105,10 @@ list all qgroups which impact the given path(include ancestral qgroups)
 list all qgroups which impact the given path(exclude ancestral qgroups)
 -v::::
 Be more verbose.  Print pathnames of member qgroups when nested.
+-j::::
+If enabled, export qgroup usage information in JSON format.  This implies --raw.
+--json-compat::::
+By default, JSON output contains full 64-bit integers, which may be incompatible with some JSON parsers.  This option exports those values as an array of 32-bit numbers in [high, low] format.
 --raw::::
 raw numbers in bytes, without the 'B' suffix.
 --human-readable::::
diff --git a/Makefile.inc.in b/Makefile.inc.in
index 56271903..68bddbed 100644
--- a/Makefile.inc.in
+++ b/Makefile.inc.in
@@ -18,9 +18,9 @@ BTRFSRESTORE_ZSTD = @BTRFSRESTORE_ZSTD@
 SUBST_CFLAGS = @CFLAGS@
 SUBST_LDFLAGS = @LDFLAGS@
 
-LIBS_BASE = @UUID_LIBS@ @BLKID_LIBS@ -L. -pthread
+LIBS_BASE = @UUID_LIBS@ @BLKID_LIBS@ @JSON_LIBS@ -L. -pthread
 LIBS_COMP = @ZLIB_LIBS@ @LZO2_LIBS@ @ZSTD_LIBS@
-STATIC_LIBS_BASE = @UUID_LIBS_STATIC@ @BLKID_LIBS_STATIC@ -L. -pthread
+STATIC_LIBS_BASE = @UUID_LIBS_STATIC@ @BLKID_LIBS_STATIC@ @JSON_LIBS_STATIC@ -L. -pthread
 STATIC_LIBS_COMP = @ZLIB_LIBS_STATIC@ @LZO2_LIBS_STATIC@ @ZSTD_LIBS_STATIC@
 
 prefix ?= @prefix@
diff --git a/cmds-qgroup.c b/cmds-qgroup.c
index 94cd0fd3..eee15ef1 100644
--- a/cmds-qgroup.c
+++ b/cmds-qgroup.c
@@ -282,6 +282,10 @@ static const char * const cmd_qgroup_show_usage[] = {
 	"               (excluding ancestral qgroups)",
 	"-P             print first-level qgroups using pathname",
 	"-v             verbose, prints all nested subvolumes",
+#ifdef HAVE_JSON
+	"-j             export in JSON format",
+	"--json-compat  export in JSON compatibility mode",
+#endif
 	HELPINFO_UNITS_LONG,
 	"--sort=qgroupid,rfer,excl,max_rfer,max_excl,pathname",
 	"               list qgroups sorted by specified items",
@@ -302,6 +306,8 @@ static int cmd_qgroup_show(int argc, char **argv)
 	unsigned unit_mode;
 	int sync = 0;
 	bool verbose = false;
+	bool export_json = false;
+	bool compat_json = false;
 
 	struct btrfs_qgroup_comparer_set *comparer_set;
 	struct btrfs_qgroup_filter_set *filter_set;
@@ -314,16 +320,26 @@ static int cmd_qgroup_show(int argc, char **argv)
 		int c;
 		enum {
 			GETOPT_VAL_SORT = 256,
-			GETOPT_VAL_SYNC
+			GETOPT_VAL_SYNC,
+			GETOPT_VAL_JSCOMPAT,
 		};
 		static const struct option long_options[] = {
 			{"sort", required_argument, NULL, GETOPT_VAL_SORT},
 			{"sync", no_argument, NULL, GETOPT_VAL_SYNC},
 			{"verbose", no_argument, NULL, 'v'},
+#ifdef HAVE_JSON
+			{"json-compat", no_argument, NULL, GETOPT_VAL_JSCOMPAT},
+#endif
 			{ NULL, 0, NULL, 0 }
 		};
-
-		c = getopt_long(argc, argv, "pPcreFfv", long_options, NULL);
+		const char getopt_chars[] = {
+			'p', 'P', 'c', 'r', 'e', 'F', 'f', 'v',
+#ifdef HAVE_JSON
+			'j',
+#endif
+			'\0' };
+
+		c = getopt_long(argc, argv, getopt_chars, long_options, NULL);
 		if (c < 0)
 			break;
 		switch (c) {
@@ -353,6 +369,14 @@ static int cmd_qgroup_show(int argc, char **argv)
 		case 'f':
 			filter_flag |= 0x2;
 			break;
+#ifdef HAVE_JSON
+		case GETOPT_VAL_JSCOMPAT:
+			compat_json = true;
+		case 'j':
+			unit_mode = UNITS_RAW;
+			export_json = true;
+			break;
+#endif
 		case GETOPT_VAL_SORT:
 			ret = btrfs_qgroup_parse_sort_string(optarg,
 							     &comparer_set);
@@ -405,7 +429,11 @@ static int cmd_qgroup_show(int argc, char **argv)
 					BTRFS_QGROUP_FILTER_PARENT,
 					qgroupid);
 	}
-	ret = btrfs_show_qgroups(fd, filter_set, comparer_set, verbose);
+	if (export_json)
+		ret = btrfs_export_qgroups_json(fd, filter_set, comparer_set,
+						compat_json);
+	else
+		ret = btrfs_show_qgroups(fd, filter_set, comparer_set, verbose);
 	close_file_or_dir(fd, dirstream);
 	free(filter_set);
 	free(comparer_set);
diff --git a/configure.ac b/configure.ac
index 56d17c3a..6aec672a 100644
--- a/configure.ac
+++ b/configure.ac
@@ -197,6 +197,12 @@ PKG_STATIC(UUID_LIBS_STATIC, [uuid])
 PKG_CHECK_MODULES(ZLIB, [zlib])
 PKG_STATIC(ZLIB_LIBS_STATIC, [zlib])
 
+PKG_CHECK_MODULES(JSON, [json-c], [
+	AC_DEFINE(HAVE_JSON, [1], [Have JSON]),
+	PKG_STATIC(JSON_LIBS_STATIC, [json-c], [
+		AC_DEFINE(HAVE_JSON_STATIC, [1], [Have JSON static])], [true])
+	], [true])
+
 AC_ARG_ENABLE([zstd],
 	AS_HELP_STRING([--disable-zstd], [build without zstd support]),
 	[], [enable_zstd=yes]
diff --git a/qgroup.c b/qgroup.c
index 2d0a6947..f632a45c 100644
--- a/qgroup.c
+++ b/qgroup.c
@@ -16,12 +16,16 @@
  * Boston, MA 021110-1307, USA.
  */
 
+#include "version.h"
 #include "qgroup.h"
 #include <sys/ioctl.h>
 #include "ctree.h"
 #include "ioctl.h"
 #include "utils.h"
 #include <errno.h>
+#ifdef HAVE_JSON
+#include <json-c/json.h>
+#endif
 
 #define BTRFS_QGROUP_NFILTERS_INCREASE (2 * BTRFS_QGROUP_FILTER_MAX)
 #define BTRFS_QGROUP_NCOMPS_INCREASE (2 * BTRFS_QGROUP_COMP_MAX)
@@ -1346,6 +1350,213 @@ int btrfs_show_qgroups(int fd,
 	return ret;
 }
 
+#ifdef HAVE_JSON
+static void format_qgroupid(char *buf, size_t size, u64 qgroupid)
+{
+	int ret;
+
+	ret = snprintf(buf, size, "%llu/%llu",
+		       btrfs_qgroup_level(qgroupid),
+		       btrfs_qgroup_subvid(qgroupid));
+	ASSERT(ret < sizeof(buf));
+}
+
+static json_object *export_one_u64(u64 value, bool compat)
+{
+	json_object *array, *tmp;
+
+	if (!compat)
+		return json_object_new_int64(value);
+
+	array = json_object_new_array();
+	if (!array)
+		return NULL;
+
+	tmp = json_object_new_int(value >> 32);
+	if (!tmp)
+		goto failure;
+	json_object_array_add(array, tmp);
+
+	tmp = json_object_new_int(value & 0xffffffff);
+	if (!tmp)
+		goto failure;
+	json_object_array_add(array, tmp);
+
+	return array;
+failure:
+	json_object_put(array);
+	return NULL;
+}
+
+static bool export_one_qgroup(json_object *container,
+			     const struct btrfs_qgroup *qgroup, bool compat)
+{
+	json_object *obj = json_object_new_object();
+	json_object *tmp;
+	char buf[42];
+
+	format_qgroupid(buf, sizeof(buf), qgroup->qgroupid);
+	tmp = json_object_new_string(buf);
+	if (!tmp)
+		return false;
+	json_object_object_add(obj, "qgroupid", tmp);
+
+	tmp = export_one_u64(qgroup->qgroupid, compat);
+	if (!tmp)
+		goto failure;
+	json_object_object_add(obj, "qgroupid_raw", tmp);
+
+	tmp = export_one_u64(qgroup->info.generation, compat);
+	if (!tmp)
+		goto failure;
+	json_object_object_add(obj, "generation", tmp);
+
+	tmp = export_one_u64(qgroup->info.referenced, compat);
+	if (!tmp)
+		goto failure;
+	json_object_object_add(obj, "referenced_bytes", tmp);
+
+	tmp = export_one_u64(qgroup->info.exclusive, compat);
+	if (!tmp)
+		goto failure;
+	json_object_object_add(obj, "exclusive_bytes", tmp);
+
+	tmp = export_one_u64(qgroup->limit.max_referenced, compat);
+	if (!tmp)
+		goto failure;
+	json_object_object_add(obj, "referenced_limit_bytes", tmp);
+
+	tmp = export_one_u64(qgroup->limit.max_exclusive, compat);
+	if (!tmp)
+		goto failure;
+	json_object_object_add(obj, "exclusive_limit_bytes", tmp);
+
+	if (btrfs_qgroup_level(qgroup->qgroupid) == 0) {
+		tmp = json_object_new_string(qgroup->pathname);
+		if (!tmp)
+			goto failure;
+		json_object_object_add(obj, "pathname", tmp);
+	} else {
+		json_object *array = json_object_new_array();
+		struct btrfs_qgroup_list *list = NULL;
+		if (!array)
+			goto failure;
+		json_object_object_add(obj, "members", array);
+
+		list_for_each_entry(list, &qgroup->qgroups, next_qgroup) {
+			struct btrfs_qgroup *member = list->qgroup;
+			char buf2[42];
+
+			format_qgroupid(buf2, sizeof(buf2), member->qgroupid);
+			tmp = json_object_new_string(buf2);
+			if (!tmp)
+				goto failure;
+
+			json_object_array_add(array, tmp);
+		}
+	}
+
+	json_object_object_add(container, buf, obj);
+	return true;
+failure:
+	json_object_put(obj);
+	return false;
+}
+
+#define BTRFS_JSON_WARNING \
+"This data contains 64-bit values that are incompatible with Javascript. Export in compatibility mode using --json-compat."
+
+static void export_all_qgroups(const struct qgroup_lookup *qgroup_lookup,
+			       bool compat)
+{
+
+	struct rb_node *n;
+	const char *json;
+	json_object *container, *dict, *obj;
+	struct btrfs_qgroup *entry;
+
+	container = json_object_new_object();
+	if (!container)
+		goto failure_msg;
+
+	obj = json_object_new_string(BTRFS_BUILD_VERSION);
+	if (!obj)
+		goto failure;
+	json_object_object_add(container, "exporter", obj);
+
+	if (!compat) {
+		obj = json_object_new_string(BTRFS_JSON_WARNING);
+		if (!obj)
+			goto failure;
+		json_object_object_add(container, "compatibility-warning", obj);
+
+		obj = json_object_new_string("64-bit");
+		if (!obj)
+			goto failure;
+		json_object_object_add(container, "u64-format", obj);
+	} else {
+		obj = json_object_new_string("array");
+		if (!obj)
+			goto failure;
+		json_object_object_add(container, "u64-format", obj);
+	}
+
+	dict = json_object_new_object();
+	if (!dict)
+		goto failure;
+	json_object_object_add(container, "qgroup_data", dict);
+
+	n = rb_first(&qgroup_lookup->root);
+	while (n) {
+		entry = rb_entry(n, struct btrfs_qgroup, sort_node);
+		if (!export_one_qgroup(dict, entry, compat))
+			goto failure;
+		n = rb_next(n);
+	}
+
+	json = json_object_to_json_string(container);
+	if (!json)
+		goto failure;
+
+	puts(json);
+
+	/* clean up container */
+	json_object_put(container);
+	return;
+
+failure:
+	json_object_put(container);
+failure_msg:
+	error("Failed to create JSON object.");
+}
+#endif
+
+int btrfs_export_qgroups_json(int fd,
+			      struct btrfs_qgroup_filter_set *filter_set,
+			      struct btrfs_qgroup_comparer_set *comp_set,
+			      bool compat)
+{
+
+#ifdef HAVE_JSON
+	struct qgroup_lookup qgroup_lookup;
+	struct qgroup_lookup sort_tree;
+	int ret = 0;
+
+	ret = qgroups_search_all(fd, &qgroup_lookup);
+	if (ret)
+		return ret;
+	__filter_and_sort_qgroups(&qgroup_lookup, &sort_tree,
+				  filter_set, comp_set);
+	export_all_qgroups(&sort_tree, compat);
+
+	__free_all_qgroups(&qgroup_lookup);
+
+	return ret;
+#else
+	return 0;
+#endif
+}
+
 int btrfs_qgroup_parse_sort_string(const char *opt_arg,
 				   struct btrfs_qgroup_comparer_set **comps)
 {
diff --git a/qgroup.h b/qgroup.h
index 688f92b2..2883727b 100644
--- a/qgroup.h
+++ b/qgroup.h
@@ -97,6 +97,9 @@ int btrfs_qgroup_parse_sort_string(const char *opt_arg,
 				struct btrfs_qgroup_comparer_set **comps);
 int btrfs_show_qgroups(int fd, struct btrfs_qgroup_filter_set *,
 		       struct btrfs_qgroup_comparer_set *, bool verbose);
+int btrfs_export_qgroups_json(int fd, struct btrfs_qgroup_filter_set *,
+			      struct btrfs_qgroup_comparer_set *,
+			      bool compat);
 void btrfs_qgroup_setup_print_column(enum btrfs_qgroup_column_enum column);
 void btrfs_qgroup_setup_units(unsigned unit_mode);
 struct btrfs_qgroup_filter_set *btrfs_qgroup_alloc_filter_set(void);
-- 
2.15.1

Re: [PATCH 8/8] btrfs-progs: qgroups: export qgroups usage information as JSON

[Qu Wenruo]

[-- Attachment #1.1: Type: text/plain, Size: 14087 bytes --]



On 2018年03月03日 02:47, jeffm@suse.com wrote:
> From: Jeff Mahoney <jeffm@suse.com>
> 
> One of the common requests I receive is for 'df' like facilities
> for subvolume usage.  Really, the request is for monitoring tools to be
> able to understand when subvolumes may be approaching quota in the same
> manner traditional file systems approach ENOSPC.
> 
> This patch allows us to export the qgroups data in a machine-readable
> format so that monitoring tools can parse it easily.
> 
> There are two modes since JSON can technically handle 64-bit numbers
> but JavaScript proper cannot.  show -j enables JSON mode using 64-bit
> integers directly.  --json-compat presents 64-bit numbers as an array
> of two 32-bit numbers (high followed by low).
> 
> Signed-off-by: Jeff Mahoney <jeffm@suse.com>
> ---
>  Documentation/btrfs-qgroup.asciidoc |   4 +
>  Makefile.inc.in                     |   4 +-
>  cmds-qgroup.c                       |  36 +++++-
>  configure.ac                        |   6 +
>  qgroup.c                            | 211 ++++++++++++++++++++++++++++++++++++
>  qgroup.h                            |   3 +
>  6 files changed, 258 insertions(+), 6 deletions(-)
> 
> diff --git a/Documentation/btrfs-qgroup.asciidoc b/Documentation/btrfs-qgroup.asciidoc
> index 360b3269..22a9c2a7 100644
> --- a/Documentation/btrfs-qgroup.asciidoc
> +++ b/Documentation/btrfs-qgroup.asciidoc
> @@ -105,6 +105,10 @@ list all qgroups which impact the given path(include ancestral qgroups)
>  list all qgroups which impact the given path(exclude ancestral qgroups)
>  -v::::
>  Be more verbose.  Print pathnames of member qgroups when nested.
> +-j::::
> +If enabled, export qgroup usage information in JSON format.  This implies --raw.
> +--json-compat::::
> +By default, JSON output contains full 64-bit integers, which may be incompatible with some JSON parsers.  This option exports those values as an array of 32-bit numbers in [high, low] format.
>  --raw::::
>  raw numbers in bytes, without the 'B' suffix.
>  --human-readable::::
> diff --git a/Makefile.inc.in b/Makefile.inc.in
> index 56271903..68bddbed 100644
> --- a/Makefile.inc.in
> +++ b/Makefile.inc.in
> @@ -18,9 +18,9 @@ BTRFSRESTORE_ZSTD = @BTRFSRESTORE_ZSTD@
>  SUBST_CFLAGS = @CFLAGS@
>  SUBST_LDFLAGS = @LDFLAGS@
>  
> -LIBS_BASE = @UUID_LIBS@ @BLKID_LIBS@ -L. -pthread
> +LIBS_BASE = @UUID_LIBS@ @BLKID_LIBS@ @JSON_LIBS@ -L. -pthread
>  LIBS_COMP = @ZLIB_LIBS@ @LZO2_LIBS@ @ZSTD_LIBS@
> -STATIC_LIBS_BASE = @UUID_LIBS_STATIC@ @BLKID_LIBS_STATIC@ -L. -pthread
> +STATIC_LIBS_BASE = @UUID_LIBS_STATIC@ @BLKID_LIBS_STATIC@ @JSON_LIBS_STATIC@ -L. -pthread
>  STATIC_LIBS_COMP = @ZLIB_LIBS_STATIC@ @LZO2_LIBS_STATIC@ @ZSTD_LIBS_STATIC@
>  
>  prefix ?= @prefix@
> diff --git a/cmds-qgroup.c b/cmds-qgroup.c
> index 94cd0fd3..eee15ef1 100644
> --- a/cmds-qgroup.c
> +++ b/cmds-qgroup.c
> @@ -282,6 +282,10 @@ static const char * const cmd_qgroup_show_usage[] = {
>  	"               (excluding ancestral qgroups)",
>  	"-P             print first-level qgroups using pathname",
>  	"-v             verbose, prints all nested subvolumes",
> +#ifdef HAVE_JSON
> +	"-j             export in JSON format",
> +	"--json-compat  export in JSON compatibility mode",
> +#endif
>  	HELPINFO_UNITS_LONG,
>  	"--sort=qgroupid,rfer,excl,max_rfer,max_excl,pathname",
>  	"               list qgroups sorted by specified items",
> @@ -302,6 +306,8 @@ static int cmd_qgroup_show(int argc, char **argv)
>  	unsigned unit_mode;
>  	int sync = 0;
>  	bool verbose = false;
> +	bool export_json = false;
> +	bool compat_json = false;
>  
>  	struct btrfs_qgroup_comparer_set *comparer_set;
>  	struct btrfs_qgroup_filter_set *filter_set;
> @@ -314,16 +320,26 @@ static int cmd_qgroup_show(int argc, char **argv)
>  		int c;
>  		enum {
>  			GETOPT_VAL_SORT = 256,
> -			GETOPT_VAL_SYNC
> +			GETOPT_VAL_SYNC,
> +			GETOPT_VAL_JSCOMPAT,
>  		};
>  		static const struct option long_options[] = {
>  			{"sort", required_argument, NULL, GETOPT_VAL_SORT},
>  			{"sync", no_argument, NULL, GETOPT_VAL_SYNC},
>  			{"verbose", no_argument, NULL, 'v'},
> +#ifdef HAVE_JSON
> +			{"json-compat", no_argument, NULL, GETOPT_VAL_JSCOMPAT},
> +#endif
>  			{ NULL, 0, NULL, 0 }
>  		};
> -
> -		c = getopt_long(argc, argv, "pPcreFfv", long_options, NULL);
> +		const char getopt_chars[] = {
> +			'p', 'P', 'c', 'r', 'e', 'F', 'f', 'v',
> +#ifdef HAVE_JSON
> +			'j',
> +#endif
> +			'\0' };
> +
> +		c = getopt_long(argc, argv, getopt_chars, long_options, NULL);
>  		if (c < 0)
>  			break;
>  		switch (c) {
> @@ -353,6 +369,14 @@ static int cmd_qgroup_show(int argc, char **argv)
>  		case 'f':
>  			filter_flag |= 0x2;
>  			break;
> +#ifdef HAVE_JSON
> +		case GETOPT_VAL_JSCOMPAT:
> +			compat_json = true;
> +		case 'j':
> +			unit_mode = UNITS_RAW;
> +			export_json = true;
> +			break;
> +#endif
>  		case GETOPT_VAL_SORT:
>  			ret = btrfs_qgroup_parse_sort_string(optarg,
>  							     &comparer_set);
> @@ -405,7 +429,11 @@ static int cmd_qgroup_show(int argc, char **argv)
>  					BTRFS_QGROUP_FILTER_PARENT,
>  					qgroupid);
>  	}
> -	ret = btrfs_show_qgroups(fd, filter_set, comparer_set, verbose);
> +	if (export_json)
> +		ret = btrfs_export_qgroups_json(fd, filter_set, comparer_set,
> +						compat_json);
> +	else
> +		ret = btrfs_show_qgroups(fd, filter_set, comparer_set, verbose);
>  	close_file_or_dir(fd, dirstream);
>  	free(filter_set);
>  	free(comparer_set);
> diff --git a/configure.ac b/configure.ac
> index 56d17c3a..6aec672a 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -197,6 +197,12 @@ PKG_STATIC(UUID_LIBS_STATIC, [uuid])
>  PKG_CHECK_MODULES(ZLIB, [zlib])
>  PKG_STATIC(ZLIB_LIBS_STATIC, [zlib])
>  
> +PKG_CHECK_MODULES(JSON, [json-c], [

Json-c is quite common and also used by cryptsetup, so pretty good
library choice.

> +	AC_DEFINE(HAVE_JSON, [1], [Have JSON]),
> +	PKG_STATIC(JSON_LIBS_STATIC, [json-c], [
> +		AC_DEFINE(HAVE_JSON_STATIC, [1], [Have JSON static])], [true])
> +	], [true])
> +
>  AC_ARG_ENABLE([zstd],
>  	AS_HELP_STRING([--disable-zstd], [build without zstd support]),
>  	[], [enable_zstd=yes]
> diff --git a/qgroup.c b/qgroup.c
> index 2d0a6947..f632a45c 100644
> --- a/qgroup.c
> +++ b/qgroup.c
> @@ -16,12 +16,16 @@
>   * Boston, MA 021110-1307, USA.
>   */
>  
> +#include "version.h"
>  #include "qgroup.h"
>  #include <sys/ioctl.h>
>  #include "ctree.h"
>  #include "ioctl.h"
>  #include "utils.h"
>  #include <errno.h>
> +#ifdef HAVE_JSON
> +#include <json-c/json.h>
> +#endif
>  
>  #define BTRFS_QGROUP_NFILTERS_INCREASE (2 * BTRFS_QGROUP_FILTER_MAX)
>  #define BTRFS_QGROUP_NCOMPS_INCREASE (2 * BTRFS_QGROUP_COMP_MAX)
> @@ -1346,6 +1350,213 @@ int btrfs_show_qgroups(int fd,
>  	return ret;
>  }
>  
> +#ifdef HAVE_JSON
> +static void format_qgroupid(char *buf, size_t size, u64 qgroupid)
> +{
> +	int ret;
> +
> +	ret = snprintf(buf, size, "%llu/%llu",
> +		       btrfs_qgroup_level(qgroupid),
> +		       btrfs_qgroup_subvid(qgroupid));
> +	ASSERT(ret < sizeof(buf));

This is designed to catch truncated snprintf(), right?
This can be addressed by setting up the @buf properly.
(See below)

And in fact, due to that super magic number, we won't hit this ASSERT()
anyway.

> +}
> +
> +static json_object *export_one_u64(u64 value, bool compat)
> +{
> +	json_object *array, *tmp;
> +
> +	if (!compat)
> +		return json_object_new_int64(value);
> +
> +	array = json_object_new_array();
> +	if (!array)
> +		return NULL;
> +
> +	tmp = json_object_new_int(value >> 32);
> +	if (!tmp)
> +		goto failure;
> +	json_object_array_add(array, tmp);
> +
> +	tmp = json_object_new_int(value & 0xffffffff);
> +	if (!tmp)
> +		goto failure;
> +	json_object_array_add(array, tmp);
> +
> +	return array;
> +failure:
> +	json_object_put(array);
> +	return NULL;
> +}
> +
> +static bool export_one_qgroup(json_object *container,
> +			     const struct btrfs_qgroup *qgroup, bool compat)
> +{
> +	json_object *obj = json_object_new_object();
> +	json_object *tmp;
> +	char buf[42];

Answer to the ultimate question of life, the universe, and everything. :)

Although according to the format level/subvolid, it should be
count_digits(MAX_U16) + 1 + count_digits(MAX_U48) + 1. (1 for '/' and 1
for '\n')

Could be defined as a macro as:
#define QGROUP_FORMAT_BUF_LEN (count_digits(1ULL<<16) + 1 + \
                               count_digits(1ULL<<48) + 1)

BTW, the result is just 22.

Despite that looks good.

Thanks,
Qu

> +
> +	format_qgroupid(buf, sizeof(buf), qgroup->qgroupid);
> +	tmp = json_object_new_string(buf);
> +	if (!tmp)
> +		return false;
> +	json_object_object_add(obj, "qgroupid", tmp);
> +
> +	tmp = export_one_u64(qgroup->qgroupid, compat);
> +	if (!tmp)
> +		goto failure;
> +	json_object_object_add(obj, "qgroupid_raw", tmp);> +
> +	tmp = export_one_u64(qgroup->info.generation, compat);
> +	if (!tmp)
> +		goto failure;
> +	json_object_object_add(obj, "generation", tmp);
> +
> +	tmp = export_one_u64(qgroup->info.referenced, compat);
> +	if (!tmp)
> +		goto failure;
> +	json_object_object_add(obj, "referenced_bytes", tmp);
> +
> +	tmp = export_one_u64(qgroup->info.exclusive, compat);
> +	if (!tmp)
> +		goto failure;
> +	json_object_object_add(obj, "exclusive_bytes", tmp);
> +
> +	tmp = export_one_u64(qgroup->limit.max_referenced, compat);
> +	if (!tmp)
> +		goto failure;
> +	json_object_object_add(obj, "referenced_limit_bytes", tmp);
> +
> +	tmp = export_one_u64(qgroup->limit.max_exclusive, compat);
> +	if (!tmp)
> +		goto failure;
> +	json_object_object_add(obj, "exclusive_limit_bytes", tmp);
> +
> +	if (btrfs_qgroup_level(qgroup->qgroupid) == 0) {
> +		tmp = json_object_new_string(qgroup->pathname);
> +		if (!tmp)
> +			goto failure;
> +		json_object_object_add(obj, "pathname", tmp);
> +	} else {
> +		json_object *array = json_object_new_array();
> +		struct btrfs_qgroup_list *list = NULL;
> +		if (!array)
> +			goto failure;
> +		json_object_object_add(obj, "members", array);
> +
> +		list_for_each_entry(list, &qgroup->qgroups, next_qgroup) {
> +			struct btrfs_qgroup *member = list->qgroup;
> +			char buf2[42];
> +
> +			format_qgroupid(buf2, sizeof(buf2), member->qgroupid);
> +			tmp = json_object_new_string(buf2);
> +			if (!tmp)
> +				goto failure;
> +
> +			json_object_array_add(array, tmp);
> +		}
> +	}
> +
> +	json_object_object_add(container, buf, obj);
> +	return true;
> +failure:
> +	json_object_put(obj);
> +	return false;
> +}
> +
> +#define BTRFS_JSON_WARNING \
> +"This data contains 64-bit values that are incompatible with Javascript. Export in compatibility mode using --json-compat."
> +
> +static void export_all_qgroups(const struct qgroup_lookup *qgroup_lookup,
> +			       bool compat)
> +{
> +
> +	struct rb_node *n;
> +	const char *json;
> +	json_object *container, *dict, *obj;
> +	struct btrfs_qgroup *entry;
> +
> +	container = json_object_new_object();
> +	if (!container)
> +		goto failure_msg;
> +
> +	obj = json_object_new_string(BTRFS_BUILD_VERSION);
> +	if (!obj)
> +		goto failure;
> +	json_object_object_add(container, "exporter", obj);
> +
> +	if (!compat) {
> +		obj = json_object_new_string(BTRFS_JSON_WARNING);
> +		if (!obj)
> +			goto failure;
> +		json_object_object_add(container, "compatibility-warning", obj);
> +
> +		obj = json_object_new_string("64-bit");
> +		if (!obj)
> +			goto failure;
> +		json_object_object_add(container, "u64-format", obj);
> +	} else {
> +		obj = json_object_new_string("array");
> +		if (!obj)
> +			goto failure;
> +		json_object_object_add(container, "u64-format", obj);
> +	}
> +
> +	dict = json_object_new_object();
> +	if (!dict)
> +		goto failure;
> +	json_object_object_add(container, "qgroup_data", dict);
> +
> +	n = rb_first(&qgroup_lookup->root);
> +	while (n) {
> +		entry = rb_entry(n, struct btrfs_qgroup, sort_node);
> +		if (!export_one_qgroup(dict, entry, compat))
> +			goto failure;
> +		n = rb_next(n);
> +	}
> +
> +	json = json_object_to_json_string(container);
> +	if (!json)
> +		goto failure;
> +
> +	puts(json);
> +
> +	/* clean up container */
> +	json_object_put(container);
> +	return;
> +
> +failure:
> +	json_object_put(container);
> +failure_msg:
> +	error("Failed to create JSON object.");
> +}
> +#endif
> +
> +int btrfs_export_qgroups_json(int fd,
> +			      struct btrfs_qgroup_filter_set *filter_set,
> +			      struct btrfs_qgroup_comparer_set *comp_set,
> +			      bool compat)
> +{
> +
> +#ifdef HAVE_JSON
> +	struct qgroup_lookup qgroup_lookup;
> +	struct qgroup_lookup sort_tree;
> +	int ret = 0;
> +
> +	ret = qgroups_search_all(fd, &qgroup_lookup);
> +	if (ret)
> +		return ret;
> +	__filter_and_sort_qgroups(&qgroup_lookup, &sort_tree,
> +				  filter_set, comp_set);
> +	export_all_qgroups(&sort_tree, compat);
> +
> +	__free_all_qgroups(&qgroup_lookup);
> +
> +	return ret;
> +#else
> +	return 0;
> +#endif
> +}
> +
>  int btrfs_qgroup_parse_sort_string(const char *opt_arg,
>  				   struct btrfs_qgroup_comparer_set **comps)
>  {
> diff --git a/qgroup.h b/qgroup.h
> index 688f92b2..2883727b 100644
> --- a/qgroup.h
> +++ b/qgroup.h
> @@ -97,6 +97,9 @@ int btrfs_qgroup_parse_sort_string(const char *opt_arg,
>  				struct btrfs_qgroup_comparer_set **comps);
>  int btrfs_show_qgroups(int fd, struct btrfs_qgroup_filter_set *,
>  		       struct btrfs_qgroup_comparer_set *, bool verbose);
> +int btrfs_export_qgroups_json(int fd, struct btrfs_qgroup_filter_set *,
> +			      struct btrfs_qgroup_comparer_set *,
> +			      bool compat);
>  void btrfs_qgroup_setup_print_column(enum btrfs_qgroup_column_enum column);
>  void btrfs_qgroup_setup_units(unsigned unit_mode);
>  struct btrfs_qgroup_filter_set *btrfs_qgroup_alloc_filter_set(void);
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 520 bytes --]

Re: [PATCH 8/8] btrfs-progs: qgroups: export qgroups usage information as JSON

[Jeff Mahoney]

[-- Attachment #1.1: Type: text/plain, Size: 2478 bytes --]

On 3/7/18 1:34 AM, Qu Wenruo wrote:
> 
> 
> On 2018年03月03日 02:47, jeffm@suse.com wrote:
>> diff --git a/configure.ac b/configure.ac
>> index 56d17c3a..6aec672a 100644
>> --- a/configure.ac
>> +++ b/configure.ac
>> @@ -197,6 +197,12 @@ PKG_STATIC(UUID_LIBS_STATIC, [uuid])
>>  PKG_CHECK_MODULES(ZLIB, [zlib])
>>  PKG_STATIC(ZLIB_LIBS_STATIC, [zlib])
>>  
>> +PKG_CHECK_MODULES(JSON, [json-c], [
> 
> Json-c is quite common and also used by cryptsetup, so pretty good
> library choice.

Yep, so that puts it in the base system packages of most distros.

>> diff --git a/qgroup.c b/qgroup.c
>> index 2d0a6947..f632a45c 100644
>> --- a/qgroup.c
>> +++ b/qgroup.c
>>  	return ret;
>>  }
>>  
>> +#ifdef HAVE_JSON
>> +static void format_qgroupid(char *buf, size_t size, u64 qgroupid)
>> +{
>> +	int ret;
>> +
>> +	ret = snprintf(buf, size, "%llu/%llu",
>> +		       btrfs_qgroup_level(qgroupid),
>> +		       btrfs_qgroup_subvid(qgroupid));
>> +	ASSERT(ret < sizeof(buf));
> 
> This is designed to catch truncated snprintf(), right?
> This can be addressed by setting up the @buf properly.
> (See below)
> 
> And in fact, due to that super magic number, we won't hit this ASSERT()
> anyway.

Yep, but ASSERTs are there to detect/prevent developer mistakes.  This
should only trigger due to a simple bug, so we ASSERT rather than handle
the error gracefully.

[...]

>> +static bool export_one_qgroup(json_object *container,
>> +			     const struct btrfs_qgroup *qgroup, bool compat)
>> +{
>> +	json_object *obj = json_object_new_object();
>> +	json_object *tmp;
>> +	char buf[42];
> 
> Answer to the ultimate question of life, the universe, and everything. :)
> 
> Although according to the format level/subvolid, it should be
> count_digits(MAX_U16) + 1 + count_digits(MAX_U48) + 1. (1 for '/' and 1
> for '\n')
> 
> Could be defined as a macro as:
> #define QGROUP_FORMAT_BUF_LEN (count_digits(1ULL<<16) + 1 + \
>                                count_digits(1ULL<<48) + 1)

Which would mean we'd execute count_digits twice for every qgroup to
resolve a constant.  I'll replace the magic number with a define though.

> BTW, the result is just 22.
It's a worst-case.  We're using %llu, so 42 is the length of two 64-bit
numbers in base ten, plus the slash and nul terminator.  In practice we
won't hit the limit, but it doesn't hurt.

Thanks for the review.

-Jeff

-- 
Jeff Mahoney
SUSE Labs


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 854 bytes --]

Re: [PATCH 0/8] btrfs-progs: qgroups usability [corrected]

[Qu Wenruo]

[-- Attachment #1.1: Type: text/plain, Size: 2240 bytes --]



On 2018年03月03日 02:46, jeffm@suse.com wrote:
> From: Jeff Mahoney <jeffm@suse.com>
> 
> Hi all -
> 
> The following series addresses some usability issues with the qgroups UI.
> 
> 1) Adds -W option so we can wait on a rescan completing without starting one.
> 2) Adds qgroup information to 'btrfs subvolume show'
> 3) Adds a -P option to show pathnames for first-level qgroups (or member
>    of nested qgroups with -v)
> 4) Allows exporting the qgroup table in JSON format for use by external
>    programs/scripts.

Going to review the patchset in the following days, but I'm pretty
curious about this feature.

Is there any plan to implement similar json interface for other tools?
Or just qgroup only yet?

Thanks,
Qu

> 
> -Jeff
> 
> Jeff Mahoney (8):
>   btrfs-progs: quota: Add -W option to rescan to wait without starting
>     rescan
>   btrfs-progs: qgroups: fix misleading index check
>   btrfs-progs: constify pathnames passed as arguments
>   btrfs-progs: qgroups: add pathname to show output
>   btrfs-progs: qgroups: introduce and use info and limit structures
>   btrfs-progs: qgroups: introduce btrfs_qgroup_query
>   btrfs-progs: subvolume: add quota info to btrfs sub show
>   btrfs-progs: qgroups: export qgroups usage information as JSON
> 
>  Documentation/btrfs-qgroup.asciidoc |   8 +
>  Documentation/btrfs-quota.asciidoc  |  10 +-
>  Makefile.inc.in                     |   4 +-
>  chunk-recover.c                     |   4 +-
>  cmds-device.c                       |   2 +-
>  cmds-fi-usage.c                     |   6 +-
>  cmds-qgroup.c                       |  49 +++-
>  cmds-quota.c                        |  21 +-
>  cmds-rescue.c                       |   4 +-
>  cmds-subvolume.c                    |  46 ++++
>  configure.ac                        |   6 +
>  kerncompat.h                        |   1 +
>  qgroup.c                            | 526 ++++++++++++++++++++++++++++++------
>  qgroup.h                            |  22 +-
>  send-utils.c                        |   4 +-
>  utils.c                             |  22 +-
>  utils.h                             |   2 +
>  17 files changed, 621 insertions(+), 116 deletions(-)
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 520 bytes --]

Re: [PATCH 0/8] btrfs-progs: qgroups usability [corrected]

[Jeffrey Mahoney]

[-- Attachment #1.1: Type: text/plain, Size: 3618 bytes --]

On 3/6/18 7:10 AM, Qu Wenruo wrote:
> 
> 
> On 2018年03月03日 02:46, jeffm@suse.com wrote:
>> From: Jeff Mahoney <jeffm@suse.com>
>>
>> Hi all -
>>
>> The following series addresses some usability issues with the qgroups UI.
>>
>> 1) Adds -W option so we can wait on a rescan completing without starting one.
>> 2) Adds qgroup information to 'btrfs subvolume show'
>> 3) Adds a -P option to show pathnames for first-level qgroups (or member
>>    of nested qgroups with -v)
>> 4) Allows exporting the qgroup table in JSON format for use by external
>>    programs/scripts.
> 
> Going to review the patchset in the following days, but I'm pretty
> curious about this feature.
> 
> Is there any plan to implement similar json interface for other tools?
> Or just qgroup only yet?

Dave and I talked about this off-list yesterday.  I had asked if perhaps
we'd want things like "btrfs subvolume list" and "btrfs subvolume show",
among other commands, to also offer JSON output.  We agreed that the
answer is "yes" and that we should use something like a global option
like "--format=json" to do that, e.g. "btrfs --format=json qgroup show."
   I have patches partially worked up to implement that.  The idea is
that commands would define in their flags which output formats they
accept.  If the user requests an unsupported format, they would receive
an error with usage, listing the accepted formats.  Each command is
responsible for outputting in a given format, but that doesn't mean we
couldn't standardize on a common library for most commands.  Dave and I
discussed using libsmartcols for output since it supports tabular and
JSON output.  For qgroups show, where we want to provide different data
structures for level 0 qgroups vs nested qgroups, it's unsuitable.  For
simple tables like 'subvolume show' or 'subvolume list' it could
probably work well.

One question I had was whether errors should be reported in the
requested format.  I'm inclined to say no and that's what my code does:
errors are still reported in plaintext with a nonzero error code.

-Jeff

>> Jeff Mahoney (8):
>>   btrfs-progs: quota: Add -W option to rescan to wait without starting
>>     rescan
>>   btrfs-progs: qgroups: fix misleading index check
>>   btrfs-progs: constify pathnames passed as arguments
>>   btrfs-progs: qgroups: add pathname to show output
>>   btrfs-progs: qgroups: introduce and use info and limit structures
>>   btrfs-progs: qgroups: introduce btrfs_qgroup_query
>>   btrfs-progs: subvolume: add quota info to btrfs sub show
>>   btrfs-progs: qgroups: export qgroups usage information as JSON
>>
>>  Documentation/btrfs-qgroup.asciidoc |   8 +
>>  Documentation/btrfs-quota.asciidoc  |  10 +-
>>  Makefile.inc.in                     |   4 +-
>>  chunk-recover.c                     |   4 +-
>>  cmds-device.c                       |   2 +-
>>  cmds-fi-usage.c                     |   6 +-
>>  cmds-qgroup.c                       |  49 +++-
>>  cmds-quota.c                        |  21 +-
>>  cmds-rescue.c                       |   4 +-
>>  cmds-subvolume.c                    |  46 ++++
>>  configure.ac                        |   6 +
>>  kerncompat.h                        |   1 +
>>  qgroup.c                            | 526 ++++++++++++++++++++++++++++++------
>>  qgroup.h                            |  22 +-
>>  send-utils.c                        |   4 +-
>>  utils.c                             |  22 +-
>>  utils.h                             |   2 +
>>  17 files changed, 621 insertions(+), 116 deletions(-)
>>
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 854 bytes --]

Re: [PATCH 0/8] btrfs-progs: qgroups usability [corrected]

[Qu Wenruo]

[-- Attachment #1.1: Type: text/plain, Size: 2111 bytes --]



On 2018年03月03日 02:46, jeffm@suse.com wrote:
> From: Jeff Mahoney <jeffm@suse.com>
> 
> Hi all -
> 
> The following series addresses some usability issues with the qgroups UI.
> 
> 1) Adds -W option so we can wait on a rescan completing without starting one.
> 2) Adds qgroup information to 'btrfs subvolume show'
> 3) Adds a -P option to show pathnames for first-level qgroups (or member
>    of nested qgroups with -v)
> 4) Allows exporting the qgroup table in JSON format for use by external
>    programs/scripts.
> 
> -Jeff
> 
> Jeff Mahoney (8):
>   btrfs-progs: quota: Add -W option to rescan to wait without starting
>     rescan
>   btrfs-progs: qgroups: fix misleading index check
>   btrfs-progs: constify pathnames passed as arguments

For patch 1~3 looks good.

Reviewed-by: Qu Wenruo <wqu@suse.com>

Thanks,
Qu

>   btrfs-progs: qgroups: add pathname to show output
>   btrfs-progs: qgroups: introduce and use info and limit structures
>   btrfs-progs: qgroups: introduce btrfs_qgroup_query
>   btrfs-progs: subvolume: add quota info to btrfs sub show
>   btrfs-progs: qgroups: export qgroups usage information as JSON
> 
>  Documentation/btrfs-qgroup.asciidoc |   8 +
>  Documentation/btrfs-quota.asciidoc  |  10 +-
>  Makefile.inc.in                     |   4 +-
>  chunk-recover.c                     |   4 +-
>  cmds-device.c                       |   2 +-
>  cmds-fi-usage.c                     |   6 +-
>  cmds-qgroup.c                       |  49 +++-
>  cmds-quota.c                        |  21 +-
>  cmds-rescue.c                       |   4 +-
>  cmds-subvolume.c                    |  46 ++++
>  configure.ac                        |   6 +
>  kerncompat.h                        |   1 +
>  qgroup.c                            | 526 ++++++++++++++++++++++++++++++------
>  qgroup.h                            |  22 +-
>  send-utils.c                        |   4 +-
>  utils.c                             |  22 +-
>  utils.h                             |   2 +
>  17 files changed, 621 insertions(+), 116 deletions(-)
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 520 bytes --]

[PATCH 0/8] btrfs-progs: qgroups usability

[jeffm]

From: Jeff Mahoney <jeffm@suse.com>

Hi all -

The following series addresses some usability issues with the qgroups UI.

1) Adds -W option so we can wait on a rescan completing without starting one.
2) Adds qgroup information to 'btrfs subvolume show'
3) Adds a -P option to show pathnames for first-level qgroups (or member
   of nested qgroups with -v)
4) Allows exporting the qgroup table in JSON format for use by external
   programs/scripts.

-Jeff

Jeff Mahoney (8):
  btrfs-progs: quota: Add -W option to rescan to wait without starting
    rescan
  btrfs-progs: qgroups: fix misleading index check
  btrfs-progs: constify pathnames passed as arguments
  btrfs-progs: qgroups: add pathname to show output
  btrfs-progs: qgroups: introduce and use info and limit structures
  btrfs-progs: qgroups: introduce btrfs_qgroup_query
  btrfs-progs: subvolume: add quota info to btrfs sub show
  btrfs-progs: qgroups: export qgroups usage information as JSON

 Documentation/btrfs-qgroup.asciidoc |   8 +
 Documentation/btrfs-quota.asciidoc  |  10 +-
 Makefile.inc.in                     |   4 +-
 chunk-recover.c                     |   4 +-
 cmds-device.c                       |   2 +-
 cmds-fi-usage.c                     |   6 +-
 cmds-qgroup.c                       |  49 +++-
 cmds-quota.c                        |  21 +-
 cmds-rescue.c                       |   4 +-
 cmds-subvolume.c                    |  46 ++++
 configure.ac                        |   6 +
 kerncompat.h                        |   1 +
 qgroup.c                            | 526 ++++++++++++++++++++++++++++++------
 qgroup.h                            |  22 +-
 send-utils.c                        |   4 +-
 utils.c                             |  22 +-
 utils.h                             |   2 +
 17 files changed, 621 insertions(+), 116 deletions(-)

-- 
2.15.1

[PATCH 1/8] btrfs-progs: quota: Add -W option to rescan to wait without starting rescan

[jeffm]

From: Jeff Mahoney <jeffm@suse.com>

This patch adds a new -W option to wait for a rescan without starting a
new operation.  This is useful for things like xfstests where we want
do to do a "btrfs quota enable" and not continue until the subsequent
rescan has finished.

In addition to documenting the new option in the man page, I've cleaned
up the rescan entry to document the -w option a bit better.

Signed-off-by: Jeff Mahoney <jeffm@suse.com>
---
 Documentation/btrfs-quota.asciidoc | 10 +++++++---
 cmds-quota.c                       | 21 +++++++++++++++------
 2 files changed, 22 insertions(+), 9 deletions(-)

diff --git a/Documentation/btrfs-quota.asciidoc b/Documentation/btrfs-quota.asciidoc
index 85ebf729..0b64a69b 100644
--- a/Documentation/btrfs-quota.asciidoc
+++ b/Documentation/btrfs-quota.asciidoc
@@ -238,15 +238,19 @@ Disable subvolume quota support for a filesystem.
 *enable* <path>::
 Enable subvolume quota support for a filesystem.
 
-*rescan* [-s] <path>::
+*rescan* [-s|-w|-W] <path>::
 Trash all qgroup numbers and scan the metadata again with the current config.
 +
 `Options`
 +
 -s::::
-show status of a running rescan operation.
+Show status of a running rescan operation.
+
 -w::::
-wait for rescan operation to finish(can be already in progress).
+Start rescan operation and wait until it has finished before exiting.  If a rescan is already running, wait until it finishes and then exit without starting a new one.
+
+-W::::
+Wait for rescan operation to finish and then exit.  If a rescan is not already running, exit silently.
 
 EXIT STATUS
 -----------
diff --git a/cmds-quota.c b/cmds-quota.c
index 745889d1..fe6376ac 100644
--- a/cmds-quota.c
+++ b/cmds-quota.c
@@ -120,14 +120,20 @@ static int cmd_quota_rescan(int argc, char **argv)
 	int wait_for_completion = 0;
 
 	while (1) {
-		int c = getopt(argc, argv, "sw");
+		int c = getopt(argc, argv, "swW");
 		if (c < 0)
 			break;
 		switch (c) {
 		case 's':
 			ioctlnum = BTRFS_IOC_QUOTA_RESCAN_STATUS;
 			break;
+		case 'W':
+			ioctlnum = 0;
+			wait_for_completion = 1;
+			break;
 		case 'w':
+			/* Reset it in case the user did both -W and -w */
+			ioctlnum = BTRFS_IOC_QUOTA_RESCAN;
 			wait_for_completion = 1;
 			break;
 		default:
@@ -135,8 +141,9 @@ static int cmd_quota_rescan(int argc, char **argv)
 		}
 	}
 
-	if (ioctlnum != BTRFS_IOC_QUOTA_RESCAN && wait_for_completion) {
-		error("switch -w cannot be used with -s");
+	if (ioctlnum == BTRFS_IOC_QUOTA_RESCAN_STATUS && wait_for_completion) {
+		error("switch -%c cannot be used with -s",
+		      ioctlnum ? 'w' : 'W');
 		return 1;
 	}
 
@@ -150,8 +157,10 @@ static int cmd_quota_rescan(int argc, char **argv)
 	if (fd < 0)
 		return 1;
 
-	ret = ioctl(fd, ioctlnum, &args);
-	e = errno;
+	if (ioctlnum) {
+		ret = ioctl(fd, ioctlnum, &args);
+		e = errno;
+	}
 
 	if (ioctlnum == BTRFS_IOC_QUOTA_RESCAN_STATUS) {
 		close_file_or_dir(fd, dirstream);
@@ -167,7 +176,7 @@ static int cmd_quota_rescan(int argc, char **argv)
 		return 0;
 	}
 
-	if (ret == 0) {
+	if (ioctlnum == BTRFS_IOC_QUOTA_RESCAN && ret == 0) {
 		printf("quota rescan started\n");
 		fflush(stdout);
 	} else if (ret < 0 && (!wait_for_completion || e != EINPROGRESS)) {
-- 
2.15.1