Re: [PATCH 6/8] btrfs-progs: qgroups: introduce btrfs_qgroup_query
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
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
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
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
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 = { > +