btrfs: qgroup: Deprecate the ability to manually inherit rfer/excl numbers
RFC PATCH 1/1 btrfs: qgroup: Deprecate the ability to manually inherit rfer/excl numbers
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.
---
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(-)
fs/btrfs/qgroup.c +17 -39
@@ -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:
include/uapi/linux/btrfs.h +2 -2
@@ -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 ]; };
Signature · 1 line
2.15.1
Quoted 108 lines · Qu Wenruo
btrfs_qgroup_inheritstructure has two members,num_ref_copiesandnum_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 <[email protected]>Signed-off-by Qu Wenruo <[email protected]>---
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(-)fs/btrfs/qgroup.c +17
-39@@ -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:include/uapi/linux/btrfs.h +0
-0
include/uapi/linux/btrfs.h struct btrfs_qgroup_limit { · line 84
Quoted 7 lines
include/uapi/linux/btrfs.h +2
-2@@ -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 ...
Quoted 4 lines
include/uapi/linux/btrfs.h +0
-0struct btrfs_qgroup_limit lim;__u64 qgroups[0 ]; };
Quoted 114 lines · Nikolay Borisov
Quoted 111 lines · Qu Wenruo
btrfs_qgroup_inheritstructure has two members,num_ref_copiesandnum_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 <[email protected]>Signed-off-by Qu Wenruo <[email protected]>---
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(-)fs/btrfs/qgroup.c +17
-39@@ -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:include/uapi/linux/btrfs.h +0
-0@@ -80,8 +80,8 @@ struct btrfs_qgroup_limit {struct btrfs_qgroup_inherit {__u64 flags;__u64 num_qgroups;
Quoted 8 lines
include/uapi/linux/btrfs.h struct btrfs_qgroup_limit { · line 84
Quoted 4 lines
include/uapi/linux/btrfs.h +2
-2-__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
Quoted 10 lines
include/uapi/linux/btrfs.h struct btrfs_qgroup_limit { · line 87
Quoted 5 lines
include/uapi/linux/btrfs.h +0
-0struct 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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 4 lines · Qu Wenruo
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.
Quoted 2 lines
Thanks,
Quoted 6 lines · Nikolay Borisov
Quoted 3 lines · Qu Wenruo
Naming like pad1/2 will make the check in
btrfs_qgroup_inherit()look quite weird.
Quoted 6 lines
Quoted 1 line
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_inheritstruct 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
Quoted 3 lines
Quoted 3 lines
Thanks,
Quoted 8 lines · Qu Wenruo
btrfs_qgroup_inheritstructure has two members,num_ref_copiesandnum_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.
Quoted 6 lines
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 <[email protected]>
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,
Quoted 9 lines
Signed-off-by Qu Wenruo <[email protected]>---
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!
Quoted 4 lines
--- fs/btrfs/qgroup.c | 56 ++++++++++++++-------------------------------- include/uapi/linux/btrfs.h | 4 ++-- 2 files changed, 19 insertions(+), 41 deletions(-)
Quoted 3 lines · Omar Sandoval
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?
Quoted 8 lines · Nikolay Borisov
Quoted 3 lines · Omar Sandoval
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.
Quoted 8 lines · Omar Sandoval
Quoted 7 lines · Nikolay Borisov
Quoted 3 lines · Omar Sandoval
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,
Quoted 6 lines
Quoted 1 line
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 :)
Quoted 8 lines · Omar Sandoval
Quoted 7 lines · Nikolay Borisov
Quoted 3 lines · Omar Sandoval
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,
Quoted 6 lines
Quoted 1 line
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 ...