* Re: [PATCH] btrfs-progs: qgroup: add sync option to 'qgroup show'
2016-12-07 3:07 [PATCH] btrfs-progs: qgroup: add sync option to 'qgroup show' Tsutomu Itoh
@ 2016-12-07 3:24 ` Qu Wenruo
2016-12-07 4:31 ` Tsutomu Itoh
2016-12-07 7:55 ` [PATCH v2] " Tsutomu Itoh
` (3 subsequent siblings)
4 siblings, 1 reply; 14+ messages in thread
From: Qu Wenruo @ 2016-12-07 3:24 UTC (permalink / raw)
To: Tsutomu Itoh, linux-btrfs
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) {
>
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH] btrfs-progs: qgroup: add sync option to 'qgroup show'
2016-12-07 3:24 ` Qu Wenruo
@ 2016-12-07 4:31 ` Tsutomu Itoh
2016-12-07 4:59 ` Qu Wenruo
0 siblings, 1 reply; 14+ messages in thread
From: Tsutomu Itoh @ 2016-12-07 4:31 UTC (permalink / raw)
To: Qu Wenruo, linux-btrfs
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
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH] btrfs-progs: qgroup: add sync option to 'qgroup show'
2016-12-07 4:31 ` Tsutomu Itoh
@ 2016-12-07 4:59 ` Qu Wenruo
2016-12-07 5:41 ` Tsutomu Itoh
0 siblings, 1 reply; 14+ messages in thread
From: Qu Wenruo @ 2016-12-07 4:59 UTC (permalink / raw)
To: Tsutomu Itoh, linux-btrfs
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
>
>
>
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH] btrfs-progs: qgroup: add sync option to 'qgroup show'
2016-12-07 4:59 ` Qu Wenruo
@ 2016-12-07 5:41 ` Tsutomu Itoh
0 siblings, 0 replies; 14+ messages in thread
From: Tsutomu Itoh @ 2016-12-07 5:41 UTC (permalink / raw)
To: Qu Wenruo; +Cc: linux-btrfs
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) {
>>>>
>>>
>>>
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v2] btrfs-progs: qgroup: add sync option to 'qgroup show'
2016-12-07 3:07 [PATCH] btrfs-progs: qgroup: add sync option to 'qgroup show' Tsutomu Itoh
2016-12-07 3:24 ` Qu Wenruo
@ 2016-12-07 7:55 ` Tsutomu Itoh
2016-12-14 10:54 ` David Sterba
2016-12-08 5:44 ` [PATCH] btrfs-progs: tests: add test for --sync option of qgroup show Tsutomu Itoh
` (2 subsequent siblings)
4 siblings, 1 reply; 14+ messages in thread
From: Tsutomu Itoh @ 2016-12-07 7:55 UTC (permalink / raw)
To: linux-btrfs; +Cc: Qu Wenruo
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
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [PATCH v2] btrfs-progs: qgroup: add sync option to 'qgroup show'
2016-12-07 7:55 ` [PATCH v2] " Tsutomu Itoh
@ 2016-12-14 10:54 ` David Sterba
2016-12-15 0:02 ` Tsutomu Itoh
0 siblings, 1 reply; 14+ messages in thread
From: David Sterba @ 2016-12-14 10:54 UTC (permalink / raw)
To: Tsutomu Itoh; +Cc: linux-btrfs, Qu Wenruo
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 }
> };
>
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH v2] btrfs-progs: qgroup: add sync option to 'qgroup show'
2016-12-14 10:54 ` David Sterba
@ 2016-12-15 0:02 ` Tsutomu Itoh
0 siblings, 0 replies; 14+ messages in thread
From: Tsutomu Itoh @ 2016-12-15 0:02 UTC (permalink / raw)
To: dsterba; +Cc: linux-btrfs, Qu Wenruo
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 }
>> };
>>
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH] btrfs-progs: tests: add test for --sync option of qgroup show
2016-12-07 3:07 [PATCH] btrfs-progs: qgroup: add sync option to 'qgroup show' Tsutomu Itoh
2016-12-07 3:24 ` Qu Wenruo
2016-12-07 7:55 ` [PATCH v2] " Tsutomu Itoh
@ 2016-12-08 5:44 ` Tsutomu Itoh
2016-12-15 4:33 ` [PATCH v2] " Tsutomu Itoh
2016-12-15 4:29 ` [PATCH v3 1/2] btrfs-progs: qgroup: add sync option to 'qgroup show' Tsutomu Itoh
2016-12-15 4:30 ` [PATCH v3 2/2] btrfs-progs: qgroup: change the value of sort option Tsutomu Itoh
4 siblings, 1 reply; 14+ messages in thread
From: Tsutomu Itoh @ 2016-12-08 5:44 UTC (permalink / raw)
To: linux-btrfs
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
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH v2] btrfs-progs: tests: add test for --sync option of qgroup show
2016-12-08 5:44 ` [PATCH] btrfs-progs: tests: add test for --sync option of qgroup show Tsutomu Itoh
@ 2016-12-15 4:33 ` Tsutomu Itoh
2017-01-24 17:17 ` David Sterba
0 siblings, 1 reply; 14+ messages in thread
From: Tsutomu Itoh @ 2016-12-15 4:33 UTC (permalink / raw)
To: linux-btrfs; +Cc: dsterba
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
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v2] btrfs-progs: tests: add test for --sync option of qgroup show
2016-12-15 4:33 ` [PATCH v2] " Tsutomu Itoh
@ 2017-01-24 17:17 ` David Sterba
0 siblings, 0 replies; 14+ messages in thread
From: David Sterba @ 2017-01-24 17:17 UTC (permalink / raw)
To: Tsutomu Itoh; +Cc: linux-btrfs, dsterba
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.
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v3 1/2] btrfs-progs: qgroup: add sync option to 'qgroup show'
2016-12-07 3:07 [PATCH] btrfs-progs: qgroup: add sync option to 'qgroup show' Tsutomu Itoh
` (2 preceding siblings ...)
2016-12-08 5:44 ` [PATCH] btrfs-progs: tests: add test for --sync option of qgroup show Tsutomu Itoh
@ 2016-12-15 4:29 ` Tsutomu Itoh
2017-01-24 16:42 ` David Sterba
2016-12-15 4:30 ` [PATCH v3 2/2] btrfs-progs: qgroup: change the value of sort option Tsutomu Itoh
4 siblings, 1 reply; 14+ messages in thread
From: Tsutomu Itoh @ 2016-12-15 4:29 UTC (permalink / raw)
To: linux-btrfs; +Cc: dsterba
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
^ permalink raw reply related [flat|nested] 14+ messages in thread* [PATCH v3 2/2] btrfs-progs: qgroup: change the value of sort option
2016-12-07 3:07 [PATCH] btrfs-progs: qgroup: add sync option to 'qgroup show' Tsutomu Itoh
` (3 preceding siblings ...)
2016-12-15 4:29 ` [PATCH v3 1/2] btrfs-progs: qgroup: add sync option to 'qgroup show' Tsutomu Itoh
@ 2016-12-15 4:30 ` Tsutomu Itoh
4 siblings, 0 replies; 14+ messages in thread
From: Tsutomu Itoh @ 2016-12-15 4:30 UTC (permalink / raw)
To: linux-btrfs; +Cc: dsterba
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
^ permalink raw reply related [flat|nested] 14+ messages in thread