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-i...@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) { >>>> >>> >>> -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html