[PATCH] btrfs-progs: qgroup: add sync option to 'qgroup show'

[PATCH] btrfs-progs: qgroup: add sync option to 'qgroup show'

[Tsutomu Itoh]

The 'qgroup show' command does not synchronize filesystem.
Therefore, 'qgroup show' may not display the correct value unless
synchronized with 'filesystem sync' command etc.

So add the '--sync' and '--no-sync' options so that we can choose
whether or not to synchronize when executing the command.

Signed-off-by: Tsutomu Itoh <t-itoh@jp.fujitsu.com>
---
 Documentation/btrfs-qgroup.asciidoc |  5 +++++
 cmds-qgroup.c                       | 24 ++++++++++++++++++++++--
 2 files changed, 27 insertions(+), 2 deletions(-)

diff --git a/Documentation/btrfs-qgroup.asciidoc b/Documentation/btrfs-qgroup.asciidoc
index 438dbc7..dd133ec 100644
--- a/Documentation/btrfs-qgroup.asciidoc
+++ b/Documentation/btrfs-qgroup.asciidoc
@@ -126,6 +126,11 @@ Prefix \'+' means ascending order and \'-' means descending order of <attr>.
 If no prefix is given, use ascending order by default.
 +
 If multiple <attr>s is given, use comma to separate.
++
+--sync::::
+invoke sync before getting info.
+--no-sync::::
+do not invoke sync before getting info (default).
 
 EXIT STATUS
 -----------
diff --git a/cmds-qgroup.c b/cmds-qgroup.c
index bc15077..ac339f3 100644
--- a/cmds-qgroup.c
+++ b/cmds-qgroup.c
@@ -272,8 +272,7 @@ static int cmd_qgroup_destroy(int argc, char **argv)
 }
 
 static const char * const cmd_qgroup_show_usage[] = {
-	"btrfs qgroup show -pcreFf "
-	"[--sort=qgroupid,rfer,excl,max_rfer,max_excl] <path>",
+	"btrfs qgroup show [options] <path>",
 	"Show subvolume quota groups.",
 	"-p             print parent qgroup id",
 	"-c             print child qgroup id",
@@ -288,6 +287,8 @@ static const char * const cmd_qgroup_show_usage[] = {
 	"               list qgroups sorted by specified items",
 	"               you can use '+' or '-' in front of each item.",
 	"               (+:ascending, -:descending, ascending default)",
+	"--sync         invoke sync before getting info",
+	"--no-sync      do not invoke sync before getting info (default)",
 	NULL
 };
 
@@ -301,6 +302,7 @@ static int cmd_qgroup_show(int argc, char **argv)
 	u64 qgroupid;
 	int filter_flag = 0;
 	unsigned unit_mode;
+	int sync = 0;
 
 	struct btrfs_qgroup_comparer_set *comparer_set;
 	struct btrfs_qgroup_filter_set *filter_set;
@@ -313,6 +315,8 @@ static int cmd_qgroup_show(int argc, char **argv)
 		int c;
 		static const struct option long_options[] = {
 			{"sort", required_argument, NULL, 'S'},
+			{"sync", no_argument, NULL, 'Y'},
+			{"no-sync", no_argument, NULL, 'N'},
 			{ NULL, 0, NULL, 0 }
 		};
 
@@ -348,6 +352,12 @@ static int cmd_qgroup_show(int argc, char **argv)
 			if (ret)
 				usage(cmd_qgroup_show_usage);
 			break;
+		case 'Y':
+			sync = 1;
+			break;
+		case 'N':
+			sync = 0;
+			break;
 		default:
 			usage(cmd_qgroup_show_usage);
 		}
@@ -365,6 +375,16 @@ static int cmd_qgroup_show(int argc, char **argv)
 		return 1;
 	}
 
+	if (sync) {
+		ret = ioctl(fd, BTRFS_IOC_SYNC);
+		if (ret < 0) {
+			error("sync ioctl failed on '%s': %s", path,
+			      strerror(errno));
+			close_file_or_dir(fd, dirstream);
+			goto out;
+		}
+	}
+
 	if (filter_flag) {
 		ret = lookup_path_rootid(fd, &qgroupid);
 		if (ret < 0) {
-- 
2.9.3

Re: [PATCH] btrfs-progs: qgroup: add sync option to 'qgroup show'

[Qu Wenruo]

At 12/07/2016 11:07 AM, Tsutomu Itoh wrote:
> The 'qgroup show' command does not synchronize filesystem.
> Therefore, 'qgroup show' may not display the correct value unless
> synchronized with 'filesystem sync' command etc.
>
> So add the '--sync' and '--no-sync' options so that we can choose
> whether or not to synchronize when executing the command.

Indeed, --sync would help in a lot of cases.

>
> Signed-off-by: Tsutomu Itoh <t-itoh@jp.fujitsu.com>
> ---
>  Documentation/btrfs-qgroup.asciidoc |  5 +++++
>  cmds-qgroup.c                       | 24 ++++++++++++++++++++++--
>  2 files changed, 27 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/btrfs-qgroup.asciidoc b/Documentation/btrfs-qgroup.asciidoc
> index 438dbc7..dd133ec 100644
> --- a/Documentation/btrfs-qgroup.asciidoc
> +++ b/Documentation/btrfs-qgroup.asciidoc
> @@ -126,6 +126,11 @@ Prefix \'+' means ascending order and \'-' means descending order of <attr>.
>  If no prefix is given, use ascending order by default.
>  +
>  If multiple <attr>s is given, use comma to separate.
> ++
> +--sync::::
> +invoke sync before getting info.

A little more words would help, to info user that qgroup will only 
update after sync.

> +--no-sync::::
> +do not invoke sync before getting info (default).
>
>  EXIT STATUS
>  -----------
> diff --git a/cmds-qgroup.c b/cmds-qgroup.c
> index bc15077..ac339f3 100644
> --- a/cmds-qgroup.c
> +++ b/cmds-qgroup.c
> @@ -272,8 +272,7 @@ static int cmd_qgroup_destroy(int argc, char **argv)
>  }
>
>  static const char * const cmd_qgroup_show_usage[] = {
> -	"btrfs qgroup show -pcreFf "
> -	"[--sort=qgroupid,rfer,excl,max_rfer,max_excl] <path>",
> +	"btrfs qgroup show [options] <path>",
>  	"Show subvolume quota groups.",
>  	"-p             print parent qgroup id",
>  	"-c             print child qgroup id",
> @@ -288,6 +287,8 @@ static const char * const cmd_qgroup_show_usage[] = {
>  	"               list qgroups sorted by specified items",
>  	"               you can use '+' or '-' in front of each item.",
>  	"               (+:ascending, -:descending, ascending default)",
> +	"--sync         invoke sync before getting info",
> +	"--no-sync      do not invoke sync before getting info (default)",

I see you're using -Y and -N option, it's better also to show
the short option together, if we will use that short option.

>  	NULL
>  };
>
> @@ -301,6 +302,7 @@ static int cmd_qgroup_show(int argc, char **argv)
>  	u64 qgroupid;
>  	int filter_flag = 0;
>  	unsigned unit_mode;
> +	int sync = 0;
>
>  	struct btrfs_qgroup_comparer_set *comparer_set;
>  	struct btrfs_qgroup_filter_set *filter_set;
> @@ -313,6 +315,8 @@ static int cmd_qgroup_show(int argc, char **argv)
>  		int c;
>  		static const struct option long_options[] = {
>  			{"sort", required_argument, NULL, 'S'},
> +			{"sync", no_argument, NULL, 'Y'},
> +			{"no-sync", no_argument, NULL, 'N'},

Although I'm not sure if "-Y" and "-N" is proper here.
IMHO, it's more like something to say all "Yes" or "No" to any 
interactive confirmation.

Maybe non-charactor option will be better.

Other part looks good to me.

Thanks,
Qu

>  			{ NULL, 0, NULL, 0 }
>  		};
>
> @@ -348,6 +352,12 @@ static int cmd_qgroup_show(int argc, char **argv)
>  			if (ret)
>  				usage(cmd_qgroup_show_usage);
>  			break;
> +		case 'Y':
> +			sync = 1;
> +			break;
> +		case 'N':
> +			sync = 0;
> +			break;
>  		default:
>  			usage(cmd_qgroup_show_usage);
>  		}
> @@ -365,6 +375,16 @@ static int cmd_qgroup_show(int argc, char **argv)
>  		return 1;
>  	}
>
> +	if (sync) {
> +		ret = ioctl(fd, BTRFS_IOC_SYNC);
> +		if (ret < 0) {
> +			error("sync ioctl failed on '%s': %s", path,
> +			      strerror(errno));
> +			close_file_or_dir(fd, dirstream);
> +			goto out;
> +		}
> +	}
> +
>  	if (filter_flag) {
>  		ret = lookup_path_rootid(fd, &qgroupid);
>  		if (ret < 0) {
>

Re: [PATCH] btrfs-progs: qgroup: add sync option to 'qgroup show'

[Tsutomu Itoh]

Hi Qu,

Thanks for the review.

On 2016/12/07 12:24, Qu Wenruo wrote:
> 
> 
> At 12/07/2016 11:07 AM, Tsutomu Itoh wrote:
>> The 'qgroup show' command does not synchronize filesystem.
>> Therefore, 'qgroup show' may not display the correct value unless
>> synchronized with 'filesystem sync' command etc.
>>
>> So add the '--sync' and '--no-sync' options so that we can choose
>> whether or not to synchronize when executing the command.
> 
> Indeed, --sync would help in a lot of cases.
> 
>>
>> Signed-off-by: Tsutomu Itoh <t-itoh@jp.fujitsu.com>
>> ---
>>  Documentation/btrfs-qgroup.asciidoc |  5 +++++
>>  cmds-qgroup.c                       | 24 ++++++++++++++++++++++--
>>  2 files changed, 27 insertions(+), 2 deletions(-)
>>
>> diff --git a/Documentation/btrfs-qgroup.asciidoc b/Documentation/btrfs-qgroup.asciidoc
>> index 438dbc7..dd133ec 100644
>> --- a/Documentation/btrfs-qgroup.asciidoc
>> +++ b/Documentation/btrfs-qgroup.asciidoc
>> @@ -126,6 +126,11 @@ Prefix \'+' means ascending order and \'-' means descending order of <attr>.
>>  If no prefix is given, use ascending order by default.
>>  +
>>  If multiple <attr>s is given, use comma to separate.
>> ++
>> +--sync::::
>> +invoke sync before getting info.
> 
> A little more words would help, to info user that qgroup will only update after sync.
> 
>> +--no-sync::::
>> +do not invoke sync before getting info (default).
>>
>>  EXIT STATUS
>>  -----------
>> diff --git a/cmds-qgroup.c b/cmds-qgroup.c
>> index bc15077..ac339f3 100644
>> --- a/cmds-qgroup.c
>> +++ b/cmds-qgroup.c
>> @@ -272,8 +272,7 @@ static int cmd_qgroup_destroy(int argc, char **argv)
>>  }
>>
>>  static const char * const cmd_qgroup_show_usage[] = {
>> -    "btrfs qgroup show -pcreFf "
>> -    "[--sort=qgroupid,rfer,excl,max_rfer,max_excl] <path>",
>> +    "btrfs qgroup show [options] <path>",
>>      "Show subvolume quota groups.",
>>      "-p             print parent qgroup id",
>>      "-c             print child qgroup id",
>> @@ -288,6 +287,8 @@ static const char * const cmd_qgroup_show_usage[] = {
>>      "               list qgroups sorted by specified items",
>>      "               you can use '+' or '-' in front of each item.",
>>      "               (+:ascending, -:descending, ascending default)",
>> +    "--sync         invoke sync before getting info",
>> +    "--no-sync      do not invoke sync before getting info (default)",
> 
> I see you're using -Y and -N option, it's better also to show
> the short option together, if we will use that short option.

Do you think that a short option is necessary?
I thought it was not necessary...

> 
>>      NULL
>>  };
>>
>> @@ -301,6 +302,7 @@ static int cmd_qgroup_show(int argc, char **argv)
>>      u64 qgroupid;
>>      int filter_flag = 0;
>>      unsigned unit_mode;
>> +    int sync = 0;
>>
>>      struct btrfs_qgroup_comparer_set *comparer_set;
>>      struct btrfs_qgroup_filter_set *filter_set;
>> @@ -313,6 +315,8 @@ static int cmd_qgroup_show(int argc, char **argv)
>>          int c;
>>          static const struct option long_options[] = {
>>              {"sort", required_argument, NULL, 'S'},
>> +            {"sync", no_argument, NULL, 'Y'},
>> +            {"no-sync", no_argument, NULL, 'N'},
> 
> Although I'm not sure if "-Y" and "-N" is proper here.
> IMHO, it's more like something to say all "Yes" or "No" to any interactive confirmation.
> 
> Maybe non-charactor option will be better.

Umm, these mean s(Y)nc/(N)osync, and the user can not specify short options...
Still would you rather change to another character?

Thanks,
Tsutomu

> 
> Other part looks good to me.
> 
> Thanks,
> Qu
> 
>>              { NULL, 0, NULL, 0 }
>>          };
>>
>> @@ -348,6 +352,12 @@ static int cmd_qgroup_show(int argc, char **argv)
>>              if (ret)
>>                  usage(cmd_qgroup_show_usage);
>>              break;
>> +        case 'Y':
>> +            sync = 1;
>> +            break;
>> +        case 'N':
>> +            sync = 0;
>> +            break;
>>          default:
>>              usage(cmd_qgroup_show_usage);
>>          }
>> @@ -365,6 +375,16 @@ static int cmd_qgroup_show(int argc, char **argv)
>>          return 1;
>>      }
>>
>> +    if (sync) {
>> +        ret = ioctl(fd, BTRFS_IOC_SYNC);
>> +        if (ret < 0) {
>> +            error("sync ioctl failed on '%s': %s", path,
>> +                  strerror(errno));
>> +            close_file_or_dir(fd, dirstream);
>> +            goto out;
>> +        }
>> +    }
>> +
>>      if (filter_flag) {
>>          ret = lookup_path_rootid(fd, &qgroupid);
>>          if (ret < 0) {
>>
> 
> 
> -- 
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] btrfs-progs: qgroup: add sync option to 'qgroup show'

[Qu Wenruo]

At 12/07/2016 12:31 PM, Tsutomu Itoh wrote:
> Hi Qu,
>
> Thanks for the review.
>
> On 2016/12/07 12:24, Qu Wenruo wrote:
>>
>>
>> At 12/07/2016 11:07 AM, Tsutomu Itoh wrote:
>>> The 'qgroup show' command does not synchronize filesystem.
>>> Therefore, 'qgroup show' may not display the correct value unless
>>> synchronized with 'filesystem sync' command etc.
>>>
>>> So add the '--sync' and '--no-sync' options so that we can choose
>>> whether or not to synchronize when executing the command.
>>
>> Indeed, --sync would help in a lot of cases.
>>
>>>
>>> Signed-off-by: Tsutomu Itoh <t-itoh@jp.fujitsu.com>
>>> ---
>>>  Documentation/btrfs-qgroup.asciidoc |  5 +++++
>>>  cmds-qgroup.c                       | 24 ++++++++++++++++++++++--
>>>  2 files changed, 27 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/Documentation/btrfs-qgroup.asciidoc b/Documentation/btrfs-qgroup.asciidoc
>>> index 438dbc7..dd133ec 100644
>>> --- a/Documentation/btrfs-qgroup.asciidoc
>>> +++ b/Documentation/btrfs-qgroup.asciidoc
>>> @@ -126,6 +126,11 @@ Prefix \'+' means ascending order and \'-' means descending order of <attr>.
>>>  If no prefix is given, use ascending order by default.
>>>  +
>>>  If multiple <attr>s is given, use comma to separate.
>>> ++
>>> +--sync::::
>>> +invoke sync before getting info.
>>
>> A little more words would help, to info user that qgroup will only update after sync.
>>
>>> +--no-sync::::
>>> +do not invoke sync before getting info (default).
>>>
>>>  EXIT STATUS
>>>  -----------
>>> diff --git a/cmds-qgroup.c b/cmds-qgroup.c
>>> index bc15077..ac339f3 100644
>>> --- a/cmds-qgroup.c
>>> +++ b/cmds-qgroup.c
>>> @@ -272,8 +272,7 @@ static int cmd_qgroup_destroy(int argc, char **argv)
>>>  }
>>>
>>>  static const char * const cmd_qgroup_show_usage[] = {
>>> -    "btrfs qgroup show -pcreFf "
>>> -    "[--sort=qgroupid,rfer,excl,max_rfer,max_excl] <path>",
>>> +    "btrfs qgroup show [options] <path>",
>>>      "Show subvolume quota groups.",
>>>      "-p             print parent qgroup id",
>>>      "-c             print child qgroup id",
>>> @@ -288,6 +287,8 @@ static const char * const cmd_qgroup_show_usage[] = {
>>>      "               list qgroups sorted by specified items",
>>>      "               you can use '+' or '-' in front of each item.",
>>>      "               (+:ascending, -:descending, ascending default)",
>>> +    "--sync         invoke sync before getting info",
>>> +    "--no-sync      do not invoke sync before getting info (default)",
>>
>> I see you're using -Y and -N option, it's better also to show
>> the short option together, if we will use that short option.
>
> Do you think that a short option is necessary?
> I thought it was not necessary...

Just mean if we use short option in getopt, it's better to mention it in 
both man page and help string.

>
>>
>>>      NULL
>>>  };
>>>
>>> @@ -301,6 +302,7 @@ static int cmd_qgroup_show(int argc, char **argv)
>>>      u64 qgroupid;
>>>      int filter_flag = 0;
>>>      unsigned unit_mode;
>>> +    int sync = 0;
>>>
>>>      struct btrfs_qgroup_comparer_set *comparer_set;
>>>      struct btrfs_qgroup_filter_set *filter_set;
>>> @@ -313,6 +315,8 @@ static int cmd_qgroup_show(int argc, char **argv)
>>>          int c;
>>>          static const struct option long_options[] = {
>>>              {"sort", required_argument, NULL, 'S'},
>>> +            {"sync", no_argument, NULL, 'Y'},
>>> +            {"no-sync", no_argument, NULL, 'N'},
>>
>> Although I'm not sure if "-Y" and "-N" is proper here.
>> IMHO, it's more like something to say all "Yes" or "No" to any interactive confirmation.
>>
>> Maybe non-charactor option will be better.
>
> Umm, these mean s(Y)nc/(N)osync, and the user can not specify short options...
> Still would you rather change to another character?

If not using short option, it's better to use non-character value 
instead of 'Y' and 'N'.

Use any number larger than 256 would do the trick, just like we did in 
qgroup assign:

enum { GETOPT_VAL_RESCAN = 256, GETOPT_VAL_NO_RESCAN };
static const struct option long_options[] = {
         { "rescan", no_argument, NULL, GETOPT_VAL_RESCAN },
         { "no-rescan", no_argument, NULL, GETOPT_VAL_NO_RESCAN },
         { NULL, 0, NULL, 0 }
};
int c = getopt_long(argc, argv, "", long_options, NULL);

BTW, I'm completely OK not to use short option.
Just the "Y" and "N" a little confusing to me since they are not 
mentioned anywhere.

Thanks,
Qu
>
> Thanks,
> Tsutomu
>
>>
>> Other part looks good to me.
>>
>> Thanks,
>> Qu
>>
>>>              { NULL, 0, NULL, 0 }
>>>          };
>>>
>>> @@ -348,6 +352,12 @@ static int cmd_qgroup_show(int argc, char **argv)
>>>              if (ret)
>>>                  usage(cmd_qgroup_show_usage);
>>>              break;
>>> +        case 'Y':
>>> +            sync = 1;
>>> +            break;
>>> +        case 'N':
>>> +            sync = 0;
>>> +            break;
>>>          default:
>>>              usage(cmd_qgroup_show_usage);
>>>          }
>>> @@ -365,6 +375,16 @@ static int cmd_qgroup_show(int argc, char **argv)
>>>          return 1;
>>>      }
>>>
>>> +    if (sync) {
>>> +        ret = ioctl(fd, BTRFS_IOC_SYNC);
>>> +        if (ret < 0) {
>>> +            error("sync ioctl failed on '%s': %s", path,
>>> +                  strerror(errno));
>>> +            close_file_or_dir(fd, dirstream);
>>> +            goto out;
>>> +        }
>>> +    }
>>> +
>>>      if (filter_flag) {
>>>          ret = lookup_path_rootid(fd, &qgroupid);
>>>          if (ret < 0) {
>>>
>>
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
>
>

Re: [PATCH] btrfs-progs: qgroup: add sync option to 'qgroup show'

[Tsutomu Itoh]

On 2016/12/07 13:59, Qu Wenruo wrote:
> 
> 
> At 12/07/2016 12:31 PM, Tsutomu Itoh wrote:
>> Hi Qu,
>>
>> Thanks for the review.
>>
>> On 2016/12/07 12:24, Qu Wenruo wrote:
>>>
>>>
>>> At 12/07/2016 11:07 AM, Tsutomu Itoh wrote:
>>>> The 'qgroup show' command does not synchronize filesystem.
>>>> Therefore, 'qgroup show' may not display the correct value unless
>>>> synchronized with 'filesystem sync' command etc.
>>>>
>>>> So add the '--sync' and '--no-sync' options so that we can choose
>>>> whether or not to synchronize when executing the command.
>>>
>>> Indeed, --sync would help in a lot of cases.
>>>
>>>>
>>>> Signed-off-by: Tsutomu Itoh <t-itoh@jp.fujitsu.com>
>>>> ---
>>>>  Documentation/btrfs-qgroup.asciidoc |  5 +++++
>>>>  cmds-qgroup.c                       | 24 ++++++++++++++++++++++--
>>>>  2 files changed, 27 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/Documentation/btrfs-qgroup.asciidoc b/Documentation/btrfs-qgroup.asciidoc
>>>> index 438dbc7..dd133ec 100644
>>>> --- a/Documentation/btrfs-qgroup.asciidoc
>>>> +++ b/Documentation/btrfs-qgroup.asciidoc
>>>> @@ -126,6 +126,11 @@ Prefix \'+' means ascending order and \'-' means descending order of <attr>.
>>>>  If no prefix is given, use ascending order by default.
>>>>  +
>>>>  If multiple <attr>s is given, use comma to separate.
>>>> ++
>>>> +--sync::::
>>>> +invoke sync before getting info.
>>>
>>> A little more words would help, to info user that qgroup will only update after sync.
>>>
>>>> +--no-sync::::
>>>> +do not invoke sync before getting info (default).
>>>>
>>>>  EXIT STATUS
>>>>  -----------
>>>> diff --git a/cmds-qgroup.c b/cmds-qgroup.c
>>>> index bc15077..ac339f3 100644
>>>> --- a/cmds-qgroup.c
>>>> +++ b/cmds-qgroup.c
>>>> @@ -272,8 +272,7 @@ static int cmd_qgroup_destroy(int argc, char **argv)
>>>>  }
>>>>
>>>>  static const char * const cmd_qgroup_show_usage[] = {
>>>> -    "btrfs qgroup show -pcreFf "
>>>> -    "[--sort=qgroupid,rfer,excl,max_rfer,max_excl] <path>",
>>>> +    "btrfs qgroup show [options] <path>",
>>>>      "Show subvolume quota groups.",
>>>>      "-p             print parent qgroup id",
>>>>      "-c             print child qgroup id",
>>>> @@ -288,6 +287,8 @@ static const char * const cmd_qgroup_show_usage[] = {
>>>>      "               list qgroups sorted by specified items",
>>>>      "               you can use '+' or '-' in front of each item.",
>>>>      "               (+:ascending, -:descending, ascending default)",
>>>> +    "--sync         invoke sync before getting info",
>>>> +    "--no-sync      do not invoke sync before getting info (default)",
>>>
>>> I see you're using -Y and -N option, it's better also to show
>>> the short option together, if we will use that short option.
>>
>> Do you think that a short option is necessary?
>> I thought it was not necessary...
> 
> Just mean if we use short option in getopt, it's better to mention it in both man page and help string.
> 
>>
>>>
>>>>      NULL
>>>>  };
>>>>
>>>> @@ -301,6 +302,7 @@ static int cmd_qgroup_show(int argc, char **argv)
>>>>      u64 qgroupid;
>>>>      int filter_flag = 0;
>>>>      unsigned unit_mode;
>>>> +    int sync = 0;
>>>>
>>>>      struct btrfs_qgroup_comparer_set *comparer_set;
>>>>      struct btrfs_qgroup_filter_set *filter_set;
>>>> @@ -313,6 +315,8 @@ static int cmd_qgroup_show(int argc, char **argv)
>>>>          int c;
>>>>          static const struct option long_options[] = {
>>>>              {"sort", required_argument, NULL, 'S'},
>>>> +            {"sync", no_argument, NULL, 'Y'},
>>>> +            {"no-sync", no_argument, NULL, 'N'},
>>>
>>> Although I'm not sure if "-Y" and "-N" is proper here.
>>> IMHO, it's more like something to say all "Yes" or "No" to any interactive confirmation.
>>>
>>> Maybe non-charactor option will be better.
>>
>> Umm, these mean s(Y)nc/(N)osync, and the user can not specify short options...
>> Still would you rather change to another character?
> 
> If not using short option, it's better to use non-character value instead of 'Y' and 'N'.
> 
> Use any number larger than 256 would do the trick, just like we did in qgroup assign:
> 
> enum { GETOPT_VAL_RESCAN = 256, GETOPT_VAL_NO_RESCAN };
> static const struct option long_options[] = {
>         { "rescan", no_argument, NULL, GETOPT_VAL_RESCAN },
>         { "no-rescan", no_argument, NULL, GETOPT_VAL_NO_RESCAN },
>         { NULL, 0, NULL, 0 }
> };
> int c = getopt_long(argc, argv, "", long_options, NULL);
> 
> BTW, I'm completely OK not to use short option.
> Just the "Y" and "N" a little confusing to me since they are not mentioned anywhere.

OK, thanks. I'll post v2 patch soon.

Thanks,
Tsutomu

> 
> Thanks,
> Qu
>>
>> Thanks,
>> Tsutomu
>>
>>>
>>> Other part looks good to me.
>>>
>>> Thanks,
>>> Qu
>>>
>>>>              { NULL, 0, NULL, 0 }
>>>>          };
>>>>
>>>> @@ -348,6 +352,12 @@ static int cmd_qgroup_show(int argc, char **argv)
>>>>              if (ret)
>>>>                  usage(cmd_qgroup_show_usage);
>>>>              break;
>>>> +        case 'Y':
>>>> +            sync = 1;
>>>> +            break;
>>>> +        case 'N':
>>>> +            sync = 0;
>>>> +            break;
>>>>          default:
>>>>              usage(cmd_qgroup_show_usage);
>>>>          }
>>>> @@ -365,6 +375,16 @@ static int cmd_qgroup_show(int argc, char **argv)
>>>>          return 1;
>>>>      }
>>>>
>>>> +    if (sync) {
>>>> +        ret = ioctl(fd, BTRFS_IOC_SYNC);
>>>> +        if (ret < 0) {
>>>> +            error("sync ioctl failed on '%s': %s", path,
>>>> +                  strerror(errno));
>>>> +            close_file_or_dir(fd, dirstream);
>>>> +            goto out;
>>>> +        }
>>>> +    }
>>>> +
>>>>      if (filter_flag) {
>>>>          ret = lookup_path_rootid(fd, &qgroupid);
>>>>          if (ret < 0) {
>>>>
>>>
>>>

[PATCH v2] btrfs-progs: qgroup: add sync option to 'qgroup show'

[Tsutomu Itoh]

The 'qgroup show' command does not synchronize filesystem.
Therefore, 'qgroup show' may not display the correct value unless
synchronized with 'filesystem sync' command etc.

So add the '--sync' and '--no-sync' options so that we can choose
whether or not to synchronize when executing the command.

Signed-off-by: Tsutomu Itoh <t-itoh@jp.fujitsu.com>
---
v2: use getopt_long with enum instead of single letter (suggested by Qu)
---
 Documentation/btrfs-qgroup.asciidoc |  6 ++++++
 cmds-qgroup.c                       | 33 +++++++++++++++++++++++++++++----
 2 files changed, 35 insertions(+), 4 deletions(-)

diff --git a/Documentation/btrfs-qgroup.asciidoc b/Documentation/btrfs-qgroup.asciidoc
index 438dbc7..9c65795 100644
--- a/Documentation/btrfs-qgroup.asciidoc
+++ b/Documentation/btrfs-qgroup.asciidoc
@@ -126,6 +126,12 @@ Prefix \'+' means ascending order and \'-' means descending order of <attr>.
 If no prefix is given, use ascending order by default.
 +
 If multiple <attr>s is given, use comma to separate.
++
+--sync::::
+To retrieve information after updating the status of qgroups,
+invoke sync before getting information.
+--no-sync::::
+Do not invoke sync before getting information (default).
 
 EXIT STATUS
 -----------
diff --git a/cmds-qgroup.c b/cmds-qgroup.c
index bc15077..b494f6f 100644
--- a/cmds-qgroup.c
+++ b/cmds-qgroup.c
@@ -272,8 +272,7 @@ static int cmd_qgroup_destroy(int argc, char **argv)
 }
 
 static const char * const cmd_qgroup_show_usage[] = {
-	"btrfs qgroup show -pcreFf "
-	"[--sort=qgroupid,rfer,excl,max_rfer,max_excl] <path>",
+	"btrfs qgroup show [options] <path>",
 	"Show subvolume quota groups.",
 	"-p             print parent qgroup id",
 	"-c             print child qgroup id",
@@ -288,6 +287,8 @@ static const char * const cmd_qgroup_show_usage[] = {
 	"               list qgroups sorted by specified items",
 	"               you can use '+' or '-' in front of each item.",
 	"               (+:ascending, -:descending, ascending default)",
+	"--sync         invoke sync before getting info",
+	"--no-sync      do not invoke sync before getting info (default)",
 	NULL
 };
 
@@ -301,6 +302,7 @@ static int cmd_qgroup_show(int argc, char **argv)
 	u64 qgroupid;
 	int filter_flag = 0;
 	unsigned unit_mode;
+	int sync = 0;
 
 	struct btrfs_qgroup_comparer_set *comparer_set;
 	struct btrfs_qgroup_filter_set *filter_set;
@@ -311,8 +313,15 @@ static int cmd_qgroup_show(int argc, char **argv)
 
 	while (1) {
 		int c;
+		enum {
+			GETOPT_VAL_SORT = 256,
+			GETOPT_VAL_SYNC,
+			GETOPT_VAL_NO_SYNC
+		};
 		static const struct option long_options[] = {
-			{"sort", required_argument, NULL, 'S'},
+			{"sort", required_argument, NULL, GETOPT_VAL_SORT},
+			{"sync", no_argument, NULL, GETOPT_VAL_SYNC},
+			{"no-sync", no_argument, NULL, GETOPT_VAL_NO_SYNC},
 			{ NULL, 0, NULL, 0 }
 		};
 
@@ -342,12 +351,18 @@ static int cmd_qgroup_show(int argc, char **argv)
 		case 'f':
 			filter_flag |= 0x2;
 			break;
-		case 'S':
+		case GETOPT_VAL_SORT:
 			ret = btrfs_qgroup_parse_sort_string(optarg,
 							     &comparer_set);
 			if (ret)
 				usage(cmd_qgroup_show_usage);
 			break;
+		case GETOPT_VAL_SYNC:
+			sync = 1;
+			break;
+		case GETOPT_VAL_NO_SYNC:
+			sync = 0;
+			break;
 		default:
 			usage(cmd_qgroup_show_usage);
 		}
@@ -365,6 +380,16 @@ static int cmd_qgroup_show(int argc, char **argv)
 		return 1;
 	}
 
+	if (sync) {
+		ret = ioctl(fd, BTRFS_IOC_SYNC);
+		if (ret < 0) {
+			error("sync ioctl failed on '%s': %s", path,
+			      strerror(errno));
+			close_file_or_dir(fd, dirstream);
+			goto out;
+		}
+	}
+
 	if (filter_flag) {
 		ret = lookup_path_rootid(fd, &qgroupid);
 		if (ret < 0) {
-- 
2.9.3

[PATCH] btrfs-progs: tests: add test for --sync option of qgroup show

[Tsutomu Itoh]

Simple test script for the following patch.

   btrfs-progs: qgroup: add sync option to 'qgroup show'

Signed-off-by: Tsutomu Itoh <t-itoh@jp.fujitsu.com>
---
 tests/cli-tests/005-qgroup-show-sync/test.sh | 30 ++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)
 create mode 100755 tests/cli-tests/005-qgroup-show-sync/test.sh

diff --git a/tests/cli-tests/005-qgroup-show-sync/test.sh b/tests/cli-tests/005-qgroup-show-sync/test.sh
new file mode 100755
index 0000000..2be684d
--- /dev/null
+++ b/tests/cli-tests/005-qgroup-show-sync/test.sh
@@ -0,0 +1,30 @@
+#!/bin/bash
+#
+# simple test of qgroup show --sync and --no-sync options
+
+source $TOP/tests/common
+
+check_prereq mkfs.btrfs
+check_prereq btrfs
+
+setup_root_helper
+prepare_test_dev 1g
+
+run_check $TOP/mkfs.btrfs -f $IMAGE
+run_check_mount_test_dev
+
+run_check $SUDO_HELPER $TOP/btrfs subvolume create $TEST_MNT/Sub
+run_check $SUDO_HELPER $TOP/btrfs quota enable $TEST_MNT/Sub
+
+for opt in '' '--' '--sync' '--no-sync'; do
+	run_check $SUDO_HELPER $TOP/btrfs qgroup limit 300M $TEST_MNT/Sub
+	run_check $SUDU_HELPER dd if=/dev/zero of=$TEST_MNT/Sub/file bs=1M count=200
+
+	run_check $SUDO_HELPER $TOP/btrfs qgroup show -re $opt $TEST_MNT/Sub
+
+	run_check $SUDO_HELPER $TOP/btrfs qgroup limit none $TEST_MNT/Sub
+	run_check rm -f $TEST_MNT/Sub/file
+	run_check $TOP/btrfs filesystem sync $TEST_MNT/Sub
+done
+
+run_check_umount_test_dev
-- 
2.9.3

Re: [PATCH v2] btrfs-progs: qgroup: add sync option to 'qgroup show'

[David Sterba]

On Wed, Dec 07, 2016 at 04:55:15PM +0900, Tsutomu Itoh wrote:
> The 'qgroup show' command does not synchronize filesystem.
> Therefore, 'qgroup show' may not display the correct value unless
> synchronized with 'filesystem sync' command etc.
> 
> So add the '--sync' and '--no-sync' options so that we can choose
> whether or not to synchronize when executing the command.
> 
> Signed-off-by: Tsutomu Itoh <t-itoh@jp.fujitsu.com>
> ---
> v2: use getopt_long with enum instead of single letter (suggested by Qu)
> ---
>  Documentation/btrfs-qgroup.asciidoc |  6 ++++++
>  cmds-qgroup.c                       | 33 +++++++++++++++++++++++++++++----
>  2 files changed, 35 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/btrfs-qgroup.asciidoc b/Documentation/btrfs-qgroup.asciidoc
> index 438dbc7..9c65795 100644
> --- a/Documentation/btrfs-qgroup.asciidoc
> +++ b/Documentation/btrfs-qgroup.asciidoc
> @@ -126,6 +126,12 @@ Prefix \'+' means ascending order and \'-' means descending order of <attr>.
>  If no prefix is given, use ascending order by default.
>  +
>  If multiple <attr>s is given, use comma to separate.
> ++
> +--sync::::
> +To retrieve information after updating the status of qgroups,
> +invoke sync before getting information.

This could be more specific, that it's a filesystem sync.

> +--no-sync::::
> +Do not invoke sync before getting information (default).

I'm not sure we need this option, how is it supposed to be used?

> @@ -311,8 +313,15 @@ static int cmd_qgroup_show(int argc, char **argv)
>  
>  	while (1) {
>  		int c;
> +		enum {
> +			GETOPT_VAL_SORT = 256,
> +			GETOPT_VAL_SYNC,
> +			GETOPT_VAL_NO_SYNC
> +		};
>  		static const struct option long_options[] = {
> -			{"sort", required_argument, NULL, 'S'},
> +			{"sort", required_argument, NULL, GETOPT_VAL_SORT},

This change is unrelated to the patch, please make a separate patch for
that.

Otherwise looks good.

> +			{"sync", no_argument, NULL, GETOPT_VAL_SYNC},
> +			{"no-sync", no_argument, NULL, GETOPT_VAL_NO_SYNC},
>  			{ NULL, 0, NULL, 0 }
>  		};
>  

Re: [PATCH v2] btrfs-progs: qgroup: add sync option to 'qgroup show'

[Tsutomu Itoh]

Hi David,

Thanks for the review.

On 2016/12/14 19:54, David Sterba wrote:
> On Wed, Dec 07, 2016 at 04:55:15PM +0900, Tsutomu Itoh wrote:
>> The 'qgroup show' command does not synchronize filesystem.
>> Therefore, 'qgroup show' may not display the correct value unless
>> synchronized with 'filesystem sync' command etc.
>>
>> So add the '--sync' and '--no-sync' options so that we can choose
>> whether or not to synchronize when executing the command.
>>
>> Signed-off-by: Tsutomu Itoh <t-itoh@jp.fujitsu.com>
>> ---
>> v2: use getopt_long with enum instead of single letter (suggested by Qu)
>> ---
>>  Documentation/btrfs-qgroup.asciidoc |  6 ++++++
>>  cmds-qgroup.c                       | 33 +++++++++++++++++++++++++++++----
>>  2 files changed, 35 insertions(+), 4 deletions(-)
>>
>> diff --git a/Documentation/btrfs-qgroup.asciidoc b/Documentation/btrfs-qgroup.asciidoc
>> index 438dbc7..9c65795 100644
>> --- a/Documentation/btrfs-qgroup.asciidoc
>> +++ b/Documentation/btrfs-qgroup.asciidoc
>> @@ -126,6 +126,12 @@ Prefix \'+' means ascending order and \'-' means descending order of <attr>.
>>  If no prefix is given, use ascending order by default.
>>  +
>>  If multiple <attr>s is given, use comma to separate.
>> ++
>> +--sync::::
>> +To retrieve information after updating the status of qgroups,
>> +invoke sync before getting information.
> 
> This could be more specific, that it's a filesystem sync.
> 
>> +--no-sync::::
>> +Do not invoke sync before getting information (default).
> 
> I'm not sure we need this option, how is it supposed to be used?

I made it to pair with --sync, but there is no use case in particular.
So, I would like to drop this with the next patch.

> 
>> @@ -311,8 +313,15 @@ static int cmd_qgroup_show(int argc, char **argv)
>>  
>>  	while (1) {
>>  		int c;
>> +		enum {
>> +			GETOPT_VAL_SORT = 256,
>> +			GETOPT_VAL_SYNC,
>> +			GETOPT_VAL_NO_SYNC
>> +		};
>>  		static const struct option long_options[] = {
>> -			{"sort", required_argument, NULL, 'S'},
>> +			{"sort", required_argument, NULL, GETOPT_VAL_SORT},
> 
> This change is unrelated to the patch, please make a separate patch for
> that.

OK. I'll separate this with the next patch.

Thanks,
Tsutomu

> 
> Otherwise looks good.
> 
>> +			{"sync", no_argument, NULL, GETOPT_VAL_SYNC},
>> +			{"no-sync", no_argument, NULL, GETOPT_VAL_NO_SYNC},
>>  			{ NULL, 0, NULL, 0 }
>>  		};
>>  

[PATCH v3 1/2] btrfs-progs: qgroup: add sync option to 'qgroup show'

[Tsutomu Itoh]

The 'qgroup show' command does not synchronize filesystem.
Therefore, 'qgroup show' may not display the correct value unless
synchronized with 'filesystem sync' command etc.

So add the '--sync' option so that we can choose whether or not
to synchronize when executing the command.

Signed-off-by: Tsutomu Itoh <t-itoh@jp.fujitsu.com>
---
v2: use getopt_long with enum instead of single letter (suggested by Qu)
v3: dropped the --no-sync option and separated the patch of sort
    option (suggested by David)
---
 Documentation/btrfs-qgroup.asciidoc |  4 ++++
 cmds-qgroup.c                       | 22 ++++++++++++++++++++--
 2 files changed, 24 insertions(+), 2 deletions(-)

diff --git a/Documentation/btrfs-qgroup.asciidoc b/Documentation/btrfs-qgroup.asciidoc
index 438dbc7..3053f2e 100644
--- a/Documentation/btrfs-qgroup.asciidoc
+++ b/Documentation/btrfs-qgroup.asciidoc
@@ -126,6 +126,10 @@ Prefix \'+' means ascending order and \'-' means descending order of <attr>.
 If no prefix is given, use ascending order by default.
 +
 If multiple <attr>s is given, use comma to separate.
++
+--sync::::
+To retrieve information after updating the state of qgroups,
+force sync of the filesystem identified by <path> before getting information.
 
 EXIT STATUS
 -----------
diff --git a/cmds-qgroup.c b/cmds-qgroup.c
index bc15077..2a10c97 100644
--- a/cmds-qgroup.c
+++ b/cmds-qgroup.c
@@ -272,8 +272,7 @@ static int cmd_qgroup_destroy(int argc, char **argv)
 }
 
 static const char * const cmd_qgroup_show_usage[] = {
-	"btrfs qgroup show -pcreFf "
-	"[--sort=qgroupid,rfer,excl,max_rfer,max_excl] <path>",
+	"btrfs qgroup show [options] <path>",
 	"Show subvolume quota groups.",
 	"-p             print parent qgroup id",
 	"-c             print child qgroup id",
@@ -288,6 +287,7 @@ static const char * const cmd_qgroup_show_usage[] = {
 	"               list qgroups sorted by specified items",
 	"               you can use '+' or '-' in front of each item.",
 	"               (+:ascending, -:descending, ascending default)",
+	"--sync         force sync of the filesystem before getting info",
 	NULL
 };
 
@@ -301,6 +301,7 @@ static int cmd_qgroup_show(int argc, char **argv)
 	u64 qgroupid;
 	int filter_flag = 0;
 	unsigned unit_mode;
+	int sync = 0;
 
 	struct btrfs_qgroup_comparer_set *comparer_set;
 	struct btrfs_qgroup_filter_set *filter_set;
@@ -311,8 +312,12 @@ static int cmd_qgroup_show(int argc, char **argv)
 
 	while (1) {
 		int c;
+		enum {
+			GETOPT_VAL_SYNC = 256
+		};
 		static const struct option long_options[] = {
 			{"sort", required_argument, NULL, 'S'},
+			{"sync", no_argument, NULL, GETOPT_VAL_SYNC},
 			{ NULL, 0, NULL, 0 }
 		};
 
@@ -348,6 +353,9 @@ static int cmd_qgroup_show(int argc, char **argv)
 			if (ret)
 				usage(cmd_qgroup_show_usage);
 			break;
+		case GETOPT_VAL_SYNC:
+			sync = 1;
+			break;
 		default:
 			usage(cmd_qgroup_show_usage);
 		}
@@ -365,6 +373,16 @@ static int cmd_qgroup_show(int argc, char **argv)
 		return 1;
 	}
 
+	if (sync) {
+		ret = ioctl(fd, BTRFS_IOC_SYNC);
+		if (ret < 0) {
+			error("sync ioctl failed on '%s': %s", path,
+			      strerror(errno));
+			close_file_or_dir(fd, dirstream);
+			goto out;
+		}
+	}
+
 	if (filter_flag) {
 		ret = lookup_path_rootid(fd, &qgroupid);
 		if (ret < 0) {
-- 
2.9.3

[PATCH v3 2/2] btrfs-progs: qgroup: change the value of sort option

[Tsutomu Itoh]

The value of sort option ('S') is not used for option letter.
Therefore, I'll change the single letter to non-character.

Signed-off-by: Tsutomu Itoh <t-itoh@jp.fujitsu.com>
---
This patch is separated from patch of --sync option.
---
 cmds-qgroup.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/cmds-qgroup.c b/cmds-qgroup.c
index 2a10c97..34e3bcc 100644
--- a/cmds-qgroup.c
+++ b/cmds-qgroup.c
@@ -313,10 +313,11 @@ static int cmd_qgroup_show(int argc, char **argv)
 	while (1) {
 		int c;
 		enum {
-			GETOPT_VAL_SYNC = 256
+			GETOPT_VAL_SORT = 256,
+			GETOPT_VAL_SYNC
 		};
 		static const struct option long_options[] = {
-			{"sort", required_argument, NULL, 'S'},
+			{"sort", required_argument, NULL, GETOPT_VAL_SORT},
 			{"sync", no_argument, NULL, GETOPT_VAL_SYNC},
 			{ NULL, 0, NULL, 0 }
 		};
@@ -347,7 +348,7 @@ static int cmd_qgroup_show(int argc, char **argv)
 		case 'f':
 			filter_flag |= 0x2;
 			break;
-		case 'S':
+		case GETOPT_VAL_SORT:
 			ret = btrfs_qgroup_parse_sort_string(optarg,
 							     &comparer_set);
 			if (ret)
-- 
2.9.3

[PATCH v2] btrfs-progs: tests: add test for --sync option of qgroup show

[Tsutomu Itoh]

Simple test script for the following patch.

   btrfs-progs: qgroup: add sync option to 'qgroup show'

Signed-off-by: Tsutomu Itoh <t-itoh@jp.fujitsu.com>
---
v2: dropped the test of --no-sync
---
 tests/cli-tests/005-qgroup-show-sync/test.sh | 30 ++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)
 create mode 100755 tests/cli-tests/005-qgroup-show-sync/test.sh

diff --git a/tests/cli-tests/005-qgroup-show-sync/test.sh b/tests/cli-tests/005-qgroup-show-sync/test.sh
new file mode 100755
index 0000000..a325b48
--- /dev/null
+++ b/tests/cli-tests/005-qgroup-show-sync/test.sh
@@ -0,0 +1,30 @@
+#!/bin/bash
+#
+# simple test of qgroup show --sync option
+
+source $TOP/tests/common
+
+check_prereq mkfs.btrfs
+check_prereq btrfs
+
+setup_root_helper
+prepare_test_dev 1g
+
+run_check $TOP/mkfs.btrfs -f $IMAGE
+run_check_mount_test_dev
+
+run_check $SUDO_HELPER $TOP/btrfs subvolume create $TEST_MNT/Sub
+run_check $SUDO_HELPER $TOP/btrfs quota enable $TEST_MNT/Sub
+
+for opt in '' '--' '--sync'; do
+	run_check $SUDO_HELPER $TOP/btrfs qgroup limit 300M $TEST_MNT/Sub
+	run_check $SUDU_HELPER dd if=/dev/zero of=$TEST_MNT/Sub/file bs=1M count=200
+
+	run_check $SUDO_HELPER $TOP/btrfs qgroup show -re $opt $TEST_MNT/Sub
+
+	run_check $SUDO_HELPER $TOP/btrfs qgroup limit none $TEST_MNT/Sub
+	run_check rm -f $TEST_MNT/Sub/file
+	run_check $TOP/btrfs filesystem sync $TEST_MNT/Sub
+done
+
+run_check_umount_test_dev
-- 
2.9.3

Re: [PATCH v3 1/2] btrfs-progs: qgroup: add sync option to 'qgroup show'

[David Sterba]

On Thu, Dec 15, 2016 at 01:29:28PM +0900, Tsutomu Itoh wrote:
> The 'qgroup show' command does not synchronize filesystem.
> Therefore, 'qgroup show' may not display the correct value unless
> synchronized with 'filesystem sync' command etc.
> 
> So add the '--sync' option so that we can choose whether or not
> to synchronize when executing the command.
> 
> Signed-off-by: Tsutomu Itoh <t-itoh@jp.fujitsu.com>

1 and 2 applied, thanks.

Re: [PATCH v2] btrfs-progs: tests: add test for --sync option of qgroup show

[David Sterba]

On Thu, Dec 15, 2016 at 01:33:05PM +0900, Tsutomu Itoh wrote:
> Simple test script for the following patch.
> 
>    btrfs-progs: qgroup: add sync option to 'qgroup show'
> 
> Signed-off-by: Tsutomu Itoh <t-itoh@jp.fujitsu.com>

Applied, thanks.