btrfs: qgroup: Deprecate the ability to manually inherit rfer/excl numbers
PATCH v2 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.
---
changelog:
v2:
Don't return -ENOTTY, but just ignoring value set in num_ref/excl_copies, suggested by Nikolay.
Don't modify the UAPI members, but add comment to deprecate them, suggested by Omar.
And add Omar as first reporter.
--- fs/btrfs/qgroup.c | 54 +++++++++++++--------------------------------- include/uapi/linux/btrfs.h | 4 ++-- 2 files changed, 17 insertions(+), 41 deletions(-)
fs/btrfs/qgroup.c +15 -39
@@ -2158,9 +2158,22 @@ 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, along with qgroup relationship added) +* +* 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. +* +* Here we just ignore any value set in num_excl/rfer_copies, +* and only handles qgroup relationship specified by +* @num_qgroups. +*/ 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 +2299,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, will just be ignored */ +__u64 num_excl_copies;/* DEPRECATED, will just be ignored */ struct btrfs_qgroup_limit lim;__u64 qgroups[0 ]; };
Signature · 1 line
2.15.1
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 8 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 Omar Sandoval <[email protected]>Reported-by Nikolay Borisov <[email protected]>Signed-off-by Qu Wenruo <[email protected]>
Overall I'm ok with the patch but I'd like to hear the opinion of other developer about whether we can rename the fields.
Quoted 102 lines
---
changelog:
v2:
Don't return-ENOTTY, but just ignoring value set in num_ref/excl_copies, suggested by Nikolay.
Don't modify the UAPI members, but add comment to deprecate them, suggested by Omar.
And add Omar as first reporter.--- fs/btrfs/qgroup.c | 54 +++++++++++++--------------------------------- include/uapi/linux/btrfs.h | 4 ++-- 2 files changed, 17 insertions(+), 41 deletions(-)fs/btrfs/qgroup.c +15
-39@@ -2158,9 +2158,22 @@ 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, along with qgroup relationship added) +* +* 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. +* +* Here we just ignore any value set in num_excl/rfer_copies, +* and only handles qgroup relationship specified by +* @num_qgroups. +*/ 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 +2299,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, will just be ignored */ +__u64 num_excl_copies;/* DEPRECATED, will just be ignored */ struct btrfs_qgroup_limit lim;__u64 qgroups[0 ]; };