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

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

Good catch.

Thanks,

-Jeff

-- 
Jeff Mahoney
SUSE Labs



signature.asc
Description: OpenPGP digital signature


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

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

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

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

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

-Jeff

-- 
Jeff Mahoney
SUSE Labs



signature.asc
Description: OpenPGP digital signature


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

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

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

>   break;
>   }
>  
> @@ -1198,14 +1180,14 @@ static int __qgroups_search(int fd, struct 
> qgroup_lookup *qgroup_lookup)
>* read the root_ref item it contains
>*/
>   for (i = 0; i < sk->nr_items; i++) {
> - sh = (struct btrfs_ioctl_search_header *)(args.buf +
> + sh = (struct btrfs_ioctl_search_header *)(args->buf +
> off);
>   off += sizeof(*sh);
>  
>   switch (btrfs_search_header_type(sh)) {
>   case BTRFS_QGROUP_STATUS_KEY:
>   si = (struct btrfs_qgroup_status_item *)
> -  (args.buf + off);
> +  (args->buf + off);
>   flags = btrfs_stack_qgroup_status_flags(si);
>  
>   print_status_flag_warning(flags);
> @@ -1213,7 +1195,7 @@ static int __qgroups_search(int fd, struct 
> qgroup_lookup *qgroup_lookup)
>   case BTRFS_QGROUP_INFO_KEY:
>   qgroupid = btrfs_search_header_offset(sh);
>   info = (struct btrfs_qgroup_info_item *)
> -(args.buf + off);
> +(args->buf + off);
>  
>   ret = update_qgroup_info(fd, qgroup_lookup,
>qgroupid, info);
> @@ -1221,7 +1203,7 @@ static int __qgroups_search(int fd, struct 
> qgroup_lookup *qgroup_lookup)
>   case BTRFS_QGROUP_LIMIT_KEY:
>   qgroupid = btrfs_search_header_offset(sh);
>   limit = (struct btrfs_qgroup_limit_item *)
> - (args.buf + off);
> + (args->buf + off);
>  
>   ret = update_qgroup_limit(fd, qgroup_lookup,
> qgroupid, limit);
> @@ -1267,6 +1249,66 @@ static int __qgroups_search(int fd, struct 
> qgroup_lookup *qgroup_lookup)
>   

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

2018-03-06 Thread Qu Wenruo


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

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

2018-03-06 Thread Qu Wenruo


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