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

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

2016-12-06 Thread 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 
 ---
  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 .
  If no prefix is given, use ascending order by default.
  +
  If multiple 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] ",
 +"btrfs qgroup show [options] ",
  "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)

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

2016-12-06 Thread 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 
---
 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 .
 If no prefix is given, use ascending order by default.
 +
 If multiple 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] ",
+"btrfs qgroup show [options] ",
 "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) {

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

2016-12-06 Thread 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 
>> ---
>>  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 .
>>  If no prefix is given, use ascending order by default.
>>  +
>>  If multiple 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] ",
>> +"btrfs qgroup show [options] ",
>>  "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, );
>>  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

--
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

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

2016-12-06 Thread 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 
---
 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 .
 If no prefix is given, use ascending order by default.
 +
 If multiple 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] ",
+   "btrfs qgroup show [options] ",
"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, );
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

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

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

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

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

4 matches

Site Navigation

Mail list logo

Footer information