| Message ID | 20171219104511.3563-1-wqu@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 19.12.2017 12:45, Qu Wenruo wrote: > btrfs_qgroup_inherit structure has two members, num_ref_copies and > num_excl_copies, to info btrfs kernel modules to inherit (copy) > rfer/excl numbers at snapshot/subvolume creation time. > > Since qgroup number is already hard to maintain for multi-level qgroup > scenario, allowing user to manually manipulate qgroup inherit is quite > easy to screw up qgroup numbers. > > Although btrfs-progs supports such inheritance specification, the > options are hidden from user and not documented. > So there is no need to allow user to manually specify inheritance in > kernel. > > Reported-by: Nikolay Borisov <nborisov@suse.com> > Signed-off-by: Qu Wenruo <wqu@suse.com> > --- > The only concern is, currently we don't have good tool to handle > inheritance of multi-level qgroups. > The only method to get qgroup numbers correct is to run a quota rescan. > > So there may be some case where experienced (well, mostly a developer) > user can use the hidden btrfs-progs options or manually craft an ioctl > to handle multi-level qgroups without costly rescan. > --- > fs/btrfs/qgroup.c | 56 ++++++++++++++-------------------------------- > include/uapi/linux/btrfs.h | 4 ++-- > 2 files changed, 19 insertions(+), 41 deletions(-) > > diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c > index 168fd03ca3ac..d8a2413272f9 100644 > --- a/fs/btrfs/qgroup.c > +++ b/fs/btrfs/qgroup.c > @@ -2158,9 +2158,24 @@ int btrfs_qgroup_inherit(struct btrfs_trans_handle *trans, > } > > if (inherit) { > + /* > + * num_excl/rfer_copies indicate how many qgroup pairs needs > + * to be manually inherited (copy rfer or excl from src > + * qgroup to dst) > + * > + * Allowing user to manipulate inheritance can easily cause > + * problem in multi-level qgroup scenario. > + * And the ioctl interface is hidden in btrfs-progs for a long > + * time, deprecate them should not be a big problem. > + */ > + if (inherit->__num_excl_copies || inherit->__num_ref_copies) { > + ret = -ENOTTY; > + btrfs_warn(fs_info, > + "manually inherit excl/rfer is no longer supported"); > + goto out; > + } > i_qgroups = (u64 *)(inherit + 1); > - nums = inherit->num_qgroups + 2 * inherit->num_ref_copies + > - 2 * inherit->num_excl_copies; > + nums = inherit->num_qgroups; > for (i = 0; i < nums; ++i) { > srcgroup = find_qgroup_rb(fs_info, *i_qgroups); > > @@ -2286,43 +2301,6 @@ int btrfs_qgroup_inherit(struct btrfs_trans_handle *trans, > ++i_qgroups; > } > > - for (i = 0; i < inherit->num_ref_copies; ++i, i_qgroups += 2) { > - struct btrfs_qgroup *src; > - struct btrfs_qgroup *dst; > - > - if (!i_qgroups[0] || !i_qgroups[1]) > - continue; > - > - src = find_qgroup_rb(fs_info, i_qgroups[0]); > - dst = find_qgroup_rb(fs_info, i_qgroups[1]); > - > - if (!src || !dst) { > - ret = -EINVAL; > - goto unlock; > - } > - > - dst->rfer = src->rfer - level_size; > - dst->rfer_cmpr = src->rfer_cmpr - level_size; > - } > - for (i = 0; i < inherit->num_excl_copies; ++i, i_qgroups += 2) { > - struct btrfs_qgroup *src; > - struct btrfs_qgroup *dst; > - > - if (!i_qgroups[0] || !i_qgroups[1]) > - continue; > - > - src = find_qgroup_rb(fs_info, i_qgroups[0]); > - dst = find_qgroup_rb(fs_info, i_qgroups[1]); > - > - if (!src || !dst) { > - ret = -EINVAL; > - goto unlock; > - } > - > - dst->excl = src->excl + level_size; > - dst->excl_cmpr = src->excl_cmpr + level_size; > - } > - > unlock: > spin_unlock(&fs_info->qgroup_lock); > out: > diff --git a/include/uapi/linux/btrfs.h b/include/uapi/linux/btrfs.h > index ce615b75e855..099e088414d6 100644 > --- a/include/uapi/linux/btrfs.h > +++ b/include/uapi/linux/btrfs.h > @@ -80,8 +80,8 @@ struct btrfs_qgroup_limit { > struct btrfs_qgroup_inherit { > __u64 flags; > __u64 num_qgroups; > - __u64 num_ref_copies; > - __u64 num_excl_copies; > + __u64 __num_ref_copies; /* DEPRECATED */ > + __u64 __num_excl_copies; /* DEPRECATED */ I'd prefer we name them something even more generic i.e. : pad1, pad2 or unused1, unused2 to really deter any efforts to use them. I guess this could shouldn't have been merged in the first place ... > struct btrfs_qgroup_limit lim; > __u64 qgroups[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
On 2017年12月19日 19:12, Nikolay Borisov wrote: > > > On 19.12.2017 12:45, Qu Wenruo wrote: >> btrfs_qgroup_inherit structure has two members, num_ref_copies and >> num_excl_copies, to info btrfs kernel modules to inherit (copy) >> rfer/excl numbers at snapshot/subvolume creation time. >> >> Since qgroup number is already hard to maintain for multi-level qgroup >> scenario, allowing user to manually manipulate qgroup inherit is quite >> easy to screw up qgroup numbers. >> >> Although btrfs-progs supports such inheritance specification, the >> options are hidden from user and not documented. >> So there is no need to allow user to manually specify inheritance in >> kernel. >> >> Reported-by: Nikolay Borisov <nborisov@suse.com> >> Signed-off-by: Qu Wenruo <wqu@suse.com> >> --- >> The only concern is, currently we don't have good tool to handle >> inheritance of multi-level qgroups. >> The only method to get qgroup numbers correct is to run a quota rescan. >> >> So there may be some case where experienced (well, mostly a developer) >> user can use the hidden btrfs-progs options or manually craft an ioctl >> to handle multi-level qgroups without costly rescan. >> --- >> fs/btrfs/qgroup.c | 56 ++++++++++++++-------------------------------- >> include/uapi/linux/btrfs.h | 4 ++-- >> 2 files changed, 19 insertions(+), 41 deletions(-) >> >> diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c >> index 168fd03ca3ac..d8a2413272f9 100644 >> --- a/fs/btrfs/qgroup.c >> +++ b/fs/btrfs/qgroup.c >> @@ -2158,9 +2158,24 @@ int btrfs_qgroup_inherit(struct btrfs_trans_handle *trans, >> } >> >> if (inherit) { >> + /* >> + * num_excl/rfer_copies indicate how many qgroup pairs needs >> + * to be manually inherited (copy rfer or excl from src >> + * qgroup to dst) >> + * >> + * Allowing user to manipulate inheritance can easily cause >> + * problem in multi-level qgroup scenario. >> + * And the ioctl interface is hidden in btrfs-progs for a long >> + * time, deprecate them should not be a big problem. >> + */ >> + if (inherit->__num_excl_copies || inherit->__num_ref_copies) { >> + ret = -ENOTTY; >> + btrfs_warn(fs_info, >> + "manually inherit excl/rfer is no longer supported"); >> + goto out; >> + } >> i_qgroups = (u64 *)(inherit + 1); >> - nums = inherit->num_qgroups + 2 * inherit->num_ref_copies + >> - 2 * inherit->num_excl_copies; >> + nums = inherit->num_qgroups; >> for (i = 0; i < nums; ++i) { >> srcgroup = find_qgroup_rb(fs_info, *i_qgroups); >> >> @@ -2286,43 +2301,6 @@ int btrfs_qgroup_inherit(struct btrfs_trans_handle *trans, >> ++i_qgroups; >> } >> >> - for (i = 0; i < inherit->num_ref_copies; ++i, i_qgroups += 2) { >> - struct btrfs_qgroup *src; >> - struct btrfs_qgroup *dst; >> - >> - if (!i_qgroups[0] || !i_qgroups[1]) >> - continue; >> - >> - src = find_qgroup_rb(fs_info, i_qgroups[0]); >> - dst = find_qgroup_rb(fs_info, i_qgroups[1]); >> - >> - if (!src || !dst) { >> - ret = -EINVAL; >> - goto unlock; >> - } >> - >> - dst->rfer = src->rfer - level_size; >> - dst->rfer_cmpr = src->rfer_cmpr - level_size; >> - } >> - for (i = 0; i < inherit->num_excl_copies; ++i, i_qgroups += 2) { >> - struct btrfs_qgroup *src; >> - struct btrfs_qgroup *dst; >> - >> - if (!i_qgroups[0] || !i_qgroups[1]) >> - continue; >> - >> - src = find_qgroup_rb(fs_info, i_qgroups[0]); >> - dst = find_qgroup_rb(fs_info, i_qgroups[1]); >> - >> - if (!src || !dst) { >> - ret = -EINVAL; >> - goto unlock; >> - } >> - >> - dst->excl = src->excl + level_size; >> - dst->excl_cmpr = src->excl_cmpr + level_size; >> - } >> - >> unlock: >> spin_unlock(&fs_info->qgroup_lock); >> out: >> diff --git a/include/uapi/linux/btrfs.h b/include/uapi/linux/btrfs.h >> index ce615b75e855..099e088414d6 100644 >> --- a/include/uapi/linux/btrfs.h >> +++ b/include/uapi/linux/btrfs.h >> @@ -80,8 +80,8 @@ struct btrfs_qgroup_limit { >> struct btrfs_qgroup_inherit { >> __u64 flags; >> __u64 num_qgroups; >> - __u64 num_ref_copies; >> - __u64 num_excl_copies; >> + __u64 __num_ref_copies; /* DEPRECATED */ >> + __u64 __num_excl_copies; /* DEPRECATED */ > > I'd prefer we name them something even more generic i.e. : > pad1, pad2 or unused1, unused2 to really deter any efforts to use them. > I guess this could shouldn't have been merged in the first place ... Naming like pad1/2 will make the check in btrfs_qgroup_inherit() look quite weird. Although I don't have any better idea, so I'm mostly fine with such rename. Thanks, Qu > >> struct btrfs_qgroup_limit lim; >> __u64 qgroups[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 >
On 19.12.2017 13:20, Qu Wenruo wrote: > Naming like pad1/2 will make the check in btrfs_qgroup_inherit() look > quite weird. > > Although I don't have any better idea, so I'm mostly fine with such rename. Right, I saw the check now, missed it the first time. I think we should remove it and the code should completely ignore these values. Ideally, we would have removed them from qgroup_inherit struct as well but this means breaking the on-disk format. So let's ignore them altogether. > > Thanks, -- 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
On 2017年12月19日 19:24, Nikolay Borisov wrote: > > > On 19.12.2017 13:20, Qu Wenruo wrote: >> Naming like pad1/2 will make the check in btrfs_qgroup_inherit() look >> quite weird. >> >> Although I don't have any better idea, so I'm mostly fine with such rename. > > Right, I saw the check now, missed it the first time. I think we should > remove it and the code should completely ignore these values. Ideally, > we would have removed them from qgroup_inherit struct as well but this > means breaking the on-disk format. So let's ignore them altogether. Ignoring the values completely seems quite reasonable. I'll update the patch. Thanks, Qu > >> >> Thanks,
On Tue, Dec 19, 2017 at 06:45:11PM +0800, Qu Wenruo wrote: > btrfs_qgroup_inherit structure has two members, num_ref_copies and > num_excl_copies, to info btrfs kernel modules to inherit (copy) > rfer/excl numbers at snapshot/subvolume creation time. > > Since qgroup number is already hard to maintain for multi-level qgroup > scenario, allowing user to manually manipulate qgroup inherit is quite > easy to screw up qgroup numbers. > > Although btrfs-progs supports such inheritance specification, the > options are hidden from user and not documented. > So there is no need to allow user to manually specify inheritance in > kernel. > > Reported-by: Nikolay Borisov <nborisov@suse.com> And reported to Nikolay by me ;) My only objection is that we shouldn't rename the field names in the UAPI header. Let's just add a comment that the two counters are ignored. Besides that, Reviewed-by: Omar Sandoval <osandov@fb.com> > Signed-off-by: Qu Wenruo <wqu@suse.com> > --- > The only concern is, currently we don't have good tool to handle > inheritance of multi-level qgroups. > The only method to get qgroup numbers correct is to run a quota rescan. > > So there may be some case where experienced (well, mostly a developer) > user can use the hidden btrfs-progs options or manually craft an ioctl > to handle multi-level qgroups without costly rescan. If we want this functionality, we should add a specific ioctl for it instead of piggy-backing off of subvolume creation. Thanks! > --- > fs/btrfs/qgroup.c | 56 ++++++++++++++-------------------------------- > include/uapi/linux/btrfs.h | 4 ++-- > 2 files changed, 19 insertions(+), 41 deletions(-) -- 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
On 19.12.2017 21:01, Omar Sandoval wrote: > My only objection is that we shouldn't rename the field names in the > UAPI header. Let's just add a comment that the two counters are ignored. > Besides that, Why is that? -- 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
On Tue, Dec 19, 2017 at 09:19:10PM +0200, Nikolay Borisov wrote: > > > On 19.12.2017 21:01, Omar Sandoval wrote: > > My only objection is that we shouldn't rename the field names in the > > UAPI header. Let's just add a comment that the two counters are ignored. > > Besides that, > > Why is that? We don't know if anyone is including the UAPI header and referring to these fields for whatever reason. A quick Google search doesn't turn up anything, but it has been there forever so I think we should err on the side of not breaking the API. -- 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
On Tue, Dec 19, 2017 at 11:36:58AM -0800, Omar Sandoval wrote: > On Tue, Dec 19, 2017 at 09:19:10PM +0200, Nikolay Borisov wrote: > > > > > > On 19.12.2017 21:01, Omar Sandoval wrote: > > > My only objection is that we shouldn't rename the field names in the > > > UAPI header. Let's just add a comment that the two counters are ignored. > > > Besides that, > > > > Why is that? > > We don't know if anyone is including the UAPI header and referring to > these fields for whatever reason. A quick Google search doesn't turn up > anything, but it has been there forever so I think we should err on the > side of not breaking the API. At least snapper sets these: https://github.com/openSUSE/snapper/blob/3c126c92bf2bd25a952800b2efb18754148ac227/snapper/BtrfsUtils.cc#L147 They get it from the libbtrfs header, but we might some day sync it to the UAPI header. So it's not completely theoretical :) -- 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
On 19.12.2017 21:36, Omar Sandoval wrote: > On Tue, Dec 19, 2017 at 09:19:10PM +0200, Nikolay Borisov wrote: >> >> >> On 19.12.2017 21:01, Omar Sandoval wrote: >>> My only objection is that we shouldn't rename the field names in the >>> UAPI header. Let's just add a comment that the two counters are ignored. >>> Besides that, >> >> Why is that? > > We don't know if anyone is including the UAPI header and referring to > these fields for whatever reason. A quick Google search doesn't turn up > anything, but it has been there forever so I think we should err on the > side of not breaking the API. So it makes sense, but I think we can be brave in this case and really obscure them. Since from where I'm standing it looks someone made a _really_ bad decision in making this debug aid part of the on-disk format and now we have to live with that decision for eternity. I think teh fact this was not documented provides enough safety for allowing us to rename the members. What if in the future we'd like to repurpose those values and use them for something else it would make sense to rename an unused member to something meaningful. "I know we have the don't break userspace" rule but in this case I'm more inclined to disregard for the sake of long-term maintainability. In the end it's not _that_ big of an issue so I won't be anal about it but I really hate the situation we find ourselves in - shit is merged and subsequently we can't really fix it ... -- 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
diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c index 168fd03ca3ac..d8a2413272f9 100644 --- a/fs/btrfs/qgroup.c +++ b/fs/btrfs/qgroup.c @@ -2158,9 +2158,24 @@ int btrfs_qgroup_inherit(struct btrfs_trans_handle *trans, } if (inherit) { + /* + * num_excl/rfer_copies indicate how many qgroup pairs needs + * to be manually inherited (copy rfer or excl from src + * qgroup to dst) + * + * Allowing user to manipulate inheritance can easily cause + * problem in multi-level qgroup scenario. + * And the ioctl interface is hidden in btrfs-progs for a long + * time, deprecate them should not be a big problem. + */ + if (inherit->__num_excl_copies || inherit->__num_ref_copies) { + ret = -ENOTTY; + btrfs_warn(fs_info, + "manually inherit excl/rfer is no longer supported"); + goto out; + } i_qgroups = (u64 *)(inherit + 1); - nums = inherit->num_qgroups + 2 * inherit->num_ref_copies + - 2 * inherit->num_excl_copies; + nums = inherit->num_qgroups; for (i = 0; i < nums; ++i) { srcgroup = find_qgroup_rb(fs_info, *i_qgroups); @@ -2286,43 +2301,6 @@ int btrfs_qgroup_inherit(struct btrfs_trans_handle *trans, ++i_qgroups; } - for (i = 0; i < inherit->num_ref_copies; ++i, i_qgroups += 2) { - struct btrfs_qgroup *src; - struct btrfs_qgroup *dst; - - if (!i_qgroups[0] || !i_qgroups[1]) - continue; - - src = find_qgroup_rb(fs_info, i_qgroups[0]); - dst = find_qgroup_rb(fs_info, i_qgroups[1]); - - if (!src || !dst) { - ret = -EINVAL; - goto unlock; - } - - dst->rfer = src->rfer - level_size; - dst->rfer_cmpr = src->rfer_cmpr - level_size; - } - for (i = 0; i < inherit->num_excl_copies; ++i, i_qgroups += 2) { - struct btrfs_qgroup *src; - struct btrfs_qgroup *dst; - - if (!i_qgroups[0] || !i_qgroups[1]) - continue; - - src = find_qgroup_rb(fs_info, i_qgroups[0]); - dst = find_qgroup_rb(fs_info, i_qgroups[1]); - - if (!src || !dst) { - ret = -EINVAL; - goto unlock; - } - - dst->excl = src->excl + level_size; - dst->excl_cmpr = src->excl_cmpr + level_size; - } - unlock: spin_unlock(&fs_info->qgroup_lock); out: diff --git a/include/uapi/linux/btrfs.h b/include/uapi/linux/btrfs.h index ce615b75e855..099e088414d6 100644 --- a/include/uapi/linux/btrfs.h +++ b/include/uapi/linux/btrfs.h @@ -80,8 +80,8 @@ struct btrfs_qgroup_limit { struct btrfs_qgroup_inherit { __u64 flags; __u64 num_qgroups; - __u64 num_ref_copies; - __u64 num_excl_copies; + __u64 __num_ref_copies; /* DEPRECATED */ + __u64 __num_excl_copies; /* DEPRECATED */ struct btrfs_qgroup_limit lim; __u64 qgroups[0]; };
btrfs_qgroup_inherit structure has two members, num_ref_copies and num_excl_copies, to info btrfs kernel modules to inherit (copy) rfer/excl numbers at snapshot/subvolume creation time. Since qgroup number is already hard to maintain for multi-level qgroup scenario, allowing user to manually manipulate qgroup inherit is quite easy to screw up qgroup numbers. Although btrfs-progs supports such inheritance specification, the options are hidden from user and not documented. So there is no need to allow user to manually specify inheritance in kernel. Reported-by: Nikolay Borisov <nborisov@suse.com> Signed-off-by: Qu Wenruo <wqu@suse.com> --- The only concern is, currently we don't have good tool to handle inheritance of multi-level qgroups. The only method to get qgroup numbers correct is to run a quota rescan. So there may be some case where experienced (well, mostly a developer) user can use the hidden btrfs-progs options or manually craft an ioctl to handle multi-level qgroups without costly rescan. --- fs/btrfs/qgroup.c | 56 ++++++++++++++-------------------------------- include/uapi/linux/btrfs.h | 4 ++-- 2 files changed, 19 insertions(+), 41 deletions(-)