Skip to contentSkip to conversation

btrfs: qgroup: Deprecate the ability to manually inherit rfer/excl numbers

PATCH In review Qu Wenruo 2 participants 2 messages Dec 20, 2017 Btrfs file system

lore
Share & tools
lore

Qu Wenruo

author
#0 lore raw

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
index 168fd03ca3ac..42c6b35d8d7f 100644
@@ -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
index ce615b75e855..a10342d491bc 100644
@@ -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
#1 lore raw
Quoted 8 lines · Qu Wenruo

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.

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.

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
index 168fd03ca3ac..42c6b35d8d7f 100644
@@ -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
index ce615b75e855..a10342d491bc 100644
@@ -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];
 };