[PATCH 0/6] btrfs: qgroup: Separate qgroup reservation types

[PATCH 0/6] btrfs: qgroup: Separate qgroup reservation types

[Qu Wenruo]

This patchset will separate original qgroup->reserved into a new
structure, btrfs_qgroup_rsv, which has different reservation for
different types.

Currently it will only includes data and meta type.

The main advantage is:

1) Better underflow detection
   Now it can detection which reservation type is causing the underflow.

2) Easier to expend
   All interface is updated to access reservation type, it will get
   super easy to be expended, especially for later delalloc reservation.

3) Better encapsulation
   No longer need to manually trace underflow or add trace events, all
   encapsulated into 2 functions.

Despite of the qgroup reservation refactor, also fix a bug where qgroup
relationship update modifies parent qgroup reservation wrongly.

Although I tend to agree with Jeff's idea to remove support of
multi-level qgroup, at least fix what I exposed during coding.

Qu Wenruo (6):
  btrfs: qgroup: Skeleton to support separate qgroup reservation type
  btrfs: qgroup: Introduce helpers to update and access new qgroup rsv
  btrfs: qgroup: Make qgroup_reserve and its callers to use separate
    reservation type
  btrfs: qgroup: Fix wrong qgroup reservation inheritance for
    relationship update
  btrfs: qgroup: Update trace events to use new separate rsv types
  btrfs: qgroup: Cleanup the remaining old reservation counters

 fs/btrfs/qgroup.c            | 160 +++++++++++++++++++++++++++++--------------
 fs/btrfs/qgroup.h            |  28 +++++++-
 include/trace/events/btrfs.h |  17 +++--
 3 files changed, 145 insertions(+), 60 deletions(-)

-- 
2.14.2

[PATCH 1/6] btrfs: qgroup: Skeleton to support separate qgroup reservation type

[Qu Wenruo]

Instead of single qgroup->reserved, use a new structure btrfs_qgroup_rsv
to restore different types of reservation.

This patch only updates the header and needed modification to pass
compile.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/qgroup.c | 16 ++++++++++------
 fs/btrfs/qgroup.h | 27 +++++++++++++++++++++++++--
 2 files changed, 35 insertions(+), 8 deletions(-)

diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
index e172d4843eae..fe3adb996883 100644
--- a/fs/btrfs/qgroup.c
+++ b/fs/btrfs/qgroup.c
@@ -2444,7 +2444,8 @@ static int qgroup_reserve(struct btrfs_root *root, u64 num_bytes, bool enforce)
 }
 
 void btrfs_qgroup_free_refroot(struct btrfs_fs_info *fs_info,
-			       u64 ref_root, u64 num_bytes)
+			       u64 ref_root, u64 num_bytes,
+			       enum btrfs_qgroup_rsv_type type)
 {
 	struct btrfs_root *quota_root;
 	struct btrfs_qgroup *qgroup;
@@ -2936,7 +2937,8 @@ static int qgroup_free_reserved_data(struct inode *inode,
 			goto out;
 		freed += changeset.bytes_changed;
 	}
-	btrfs_qgroup_free_refroot(root->fs_info, root->objectid, freed);
+	btrfs_qgroup_free_refroot(root->fs_info, root->objectid, freed,
+				  BTRFS_QGROUP_RSV_DATA);
 	ret = freed;
 out:
 	extent_changeset_release(&changeset);
@@ -2968,7 +2970,7 @@ static int __btrfs_qgroup_release_data(struct inode *inode,
 	if (free)
 		btrfs_qgroup_free_refroot(BTRFS_I(inode)->root->fs_info,
 				BTRFS_I(inode)->root->objectid,
-				changeset.bytes_changed);
+				changeset.bytes_changed, BTRFS_QGROUP_RSV_DATA);
 	ret = changeset.bytes_changed;
 out:
 	extent_changeset_release(&changeset);
@@ -3045,7 +3047,8 @@ void btrfs_qgroup_free_meta_all(struct btrfs_root *root)
 	if (reserved == 0)
 		return;
 	trace_qgroup_meta_reserve(root, -(s64)reserved);
-	btrfs_qgroup_free_refroot(fs_info, root->objectid, reserved);
+	btrfs_qgroup_free_refroot(fs_info, root->objectid, reserved,
+				  BTRFS_QGROUP_RSV_META);
 }
 
 void btrfs_qgroup_free_meta(struct btrfs_root *root, int num_bytes)
@@ -3060,7 +3063,8 @@ void btrfs_qgroup_free_meta(struct btrfs_root *root, int num_bytes)
 	WARN_ON(atomic64_read(&root->qgroup_meta_rsv) < num_bytes);
 	atomic64_sub(num_bytes, &root->qgroup_meta_rsv);
 	trace_qgroup_meta_reserve(root, -(s64)num_bytes);
-	btrfs_qgroup_free_refroot(fs_info, root->objectid, num_bytes);
+	btrfs_qgroup_free_refroot(fs_info, root->objectid, num_bytes,
+				  BTRFS_QGROUP_RSV_META);
 }
 
 /*
@@ -3088,7 +3092,7 @@ void btrfs_qgroup_check_reserved_leak(struct inode *inode)
 		}
 		btrfs_qgroup_free_refroot(BTRFS_I(inode)->root->fs_info,
 				BTRFS_I(inode)->root->objectid,
-				changeset.bytes_changed);
+				changeset.bytes_changed, BTRFS_QGROUP_RSV_DATA);
 
 	}
 	extent_changeset_release(&changeset);
diff --git a/fs/btrfs/qgroup.h b/fs/btrfs/qgroup.h
index d9984e87cddf..0b04cbc5b5ce 100644
--- a/fs/btrfs/qgroup.h
+++ b/fs/btrfs/qgroup.h
@@ -61,6 +61,26 @@ struct btrfs_qgroup_extent_record {
 	struct ulist *old_roots;
 };
 
+enum btrfs_qgroup_rsv_type {
+	BTRFS_QGROUP_RSV_DATA = 0,
+	BTRFS_QGROUP_RSV_META = 1,
+	BTRFS_QGROUP_RSV_TYPES = 1,
+};
+
+/*
+ * Represents how many bytes we reserved for this qgroup.
+ *
+ * Each type should have different reservation behavior.
+ * E.g, data follows its io_tree flag modification, while
+ * *currently* meta is just reserve-and-clear during transcation.
+ *
+ * TODO: Add new type for delalloc, which can exist across several
+ * transaction.
+ */
+struct btrfs_qgroup_rsv {
+	u64 values[BTRFS_QGROUP_RSV_TYPES + 1];
+};
+
 /*
  * one struct for each qgroup, organized in fs_info->qgroup_tree.
  */
@@ -88,6 +108,7 @@ struct btrfs_qgroup {
 	 * reservation tracking
 	 */
 	u64 reserved;
+	struct btrfs_qgroup_rsv rsv;
 
 	/*
 	 * lists
@@ -228,12 +249,14 @@ int btrfs_qgroup_inherit(struct btrfs_trans_handle *trans,
 			 struct btrfs_fs_info *fs_info, u64 srcid, u64 objectid,
 			 struct btrfs_qgroup_inherit *inherit);
 void btrfs_qgroup_free_refroot(struct btrfs_fs_info *fs_info,
-			       u64 ref_root, u64 num_bytes);
+			       u64 ref_root, u64 num_bytes,
+			       enum btrfs_qgroup_rsv_type type);
 static inline void btrfs_qgroup_free_delayed_ref(struct btrfs_fs_info *fs_info,
 						 u64 ref_root, u64 num_bytes)
 {
 	trace_btrfs_qgroup_free_delayed_ref(fs_info, ref_root, num_bytes);
-	btrfs_qgroup_free_refroot(fs_info, ref_root, num_bytes);
+	btrfs_qgroup_free_refroot(fs_info, ref_root, num_bytes,
+				  BTRFS_QGROUP_RSV_DATA);
 }
 
 #ifdef CONFIG_BTRFS_FS_RUN_SANITY_TESTS
-- 
2.14.2

[PATCH 2/6] btrfs: qgroup: Introduce helpers to update and access new qgroup rsv

[Qu Wenruo]

Introduce helpers to:

1) Get total reserved space
   For limit calculation

2) Increase reserved space for given type
2) Decrease reserved space for given type

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/qgroup.c | 66 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 66 insertions(+)

diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
index fe3adb996883..04fd145516ad 100644
--- a/fs/btrfs/qgroup.c
+++ b/fs/btrfs/qgroup.c
@@ -47,6 +47,72 @@
  *  - check all ioctl parameters
  */
 
+/*
+ * Helpers to access qgroup reservation
+ *
+ * Callers should ensure the lock context and type are valid
+ */
+
+static u64 qgroup_rsv_total(const struct btrfs_qgroup *qgroup)
+{
+	u64 ret = 0;
+	int i;
+
+	for (i = 0; i <= BTRFS_QGROUP_RSV_TYPES; i++)
+		ret += qgroup->rsv.values[i];
+
+	return ret;
+}
+
+static const char *qgroup_rsv_type_str(enum btrfs_qgroup_rsv_type type)
+{
+	if (type == BTRFS_QGROUP_RSV_DATA)
+		return "data";
+	if (type == BTRFS_QGROUP_RSV_META)
+		return "meta";
+	return NULL;
+}
+
+static void qgroup_rsv_increase(struct btrfs_qgroup *qgroup, u64 num_bytes,
+				enum btrfs_qgroup_rsv_type type)
+{
+	qgroup->rsv.values[type] += num_bytes;
+}
+
+static void qgroup_rsv_decrease(struct btrfs_qgroup *qgroup, u64 num_bytes,
+				enum btrfs_qgroup_rsv_type type)
+{
+	if (qgroup->rsv.values[type] >= num_bytes) {
+		qgroup->rsv.values[type] -= num_bytes;
+		return;
+	}
+#ifdef CONFIG_BTRFS_DEBUG
+	WARN_RATELIMIT(1,
+		"qgroup %llu %s reserved space underflow, have %llu to free %llu",
+		qgroup->qgroupid, qgroup_rsv_type_str(type),
+		qgroup->rsv.values[type], num_bytes);
+#endif
+	qgroup->rsv.values[type] = 0;
+}
+
+static void qgroup_rsv_increase_by_qgroup(struct btrfs_qgroup *dest,
+					  struct btrfs_qgroup *src)
+{
+	int i;
+
+	for (i = 0; i <= BTRFS_QGROUP_RSV_TYPES; i++)
+		qgroup_rsv_increase(dest, src->rsv.values[i], i);
+}
+
+static void qgroup_rsv_decrease_by_qgroup(struct btrfs_qgroup *dest,
+					  struct btrfs_qgroup *src)
+{
+	int i;
+
+	for (i = 0; i <= BTRFS_QGROUP_RSV_TYPES; i++)
+		qgroup_rsv_decrease(dest, src->rsv.values[i], i);
+}
+
 static void btrfs_qgroup_update_old_refcnt(struct btrfs_qgroup *qg, u64 seq,
 					   int mod)
 {
-- 
2.14.2

[PATCH 3/6] btrfs: qgroup: Make qgroup_reserve and its callers to use separate reservation type

[Qu Wenruo]

Since most caller of qgroup_reserve() is already defined by type,
converting qgroup_reserve() is quite an easy work.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/qgroup.c | 20 +++++++++-----------
 1 file changed, 9 insertions(+), 11 deletions(-)

diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
index 04fd145516ad..7b89da9589c1 100644
--- a/fs/btrfs/qgroup.c
+++ b/fs/btrfs/qgroup.c
@@ -2399,17 +2399,18 @@ int btrfs_qgroup_inherit(struct btrfs_trans_handle *trans,
 static bool qgroup_check_limits(const struct btrfs_qgroup *qg, u64 num_bytes)
 {
 	if ((qg->lim_flags & BTRFS_QGROUP_LIMIT_MAX_RFER) &&
-	    qg->reserved + (s64)qg->rfer + num_bytes > qg->max_rfer)
+	    qgroup_rsv_total(qg) + (s64)qg->rfer + num_bytes > qg->max_rfer)
 		return false;
 
 	if ((qg->lim_flags & BTRFS_QGROUP_LIMIT_MAX_EXCL) &&
-	    qg->reserved + (s64)qg->excl + num_bytes > qg->max_excl)
+	    qgroup_rsv_total(qg) + (s64)qg->excl + num_bytes > qg->max_excl)
 		return false;
 
 	return true;
 }
 
-static int qgroup_reserve(struct btrfs_root *root, u64 num_bytes, bool enforce)
+static int qgroup_reserve(struct btrfs_root *root, u64 num_bytes, bool enforce,
+			  enum btrfs_qgroup_rsv_type type)
 {
 	struct btrfs_root *quota_root;
 	struct btrfs_qgroup *qgroup;
@@ -2461,7 +2462,7 @@ static int qgroup_reserve(struct btrfs_root *root, u64 num_bytes, bool enforce)
 			 * Commit the tree and retry, since we may have
 			 * deletions which would free up space.
 			 */
-			if (!retried && qg->reserved > 0) {
+			if (!retried && qgroup_rsv_total(qg) > 0) {
 				struct btrfs_trans_handle *trans;
 
 				spin_unlock(&fs_info->qgroup_lock);
@@ -2501,7 +2502,7 @@ static int qgroup_reserve(struct btrfs_root *root, u64 num_bytes, bool enforce)
 		qg = unode_aux_to_qgroup(unode);
 
 		trace_qgroup_update_reserve(fs_info, qg, num_bytes);
-		qg->reserved += num_bytes;
+		qgroup_rsv_increase(qg, num_bytes, type);
 	}
 
 out:
@@ -2548,10 +2549,7 @@ void btrfs_qgroup_free_refroot(struct btrfs_fs_info *fs_info,
 		qg = unode_aux_to_qgroup(unode);
 
 		trace_qgroup_update_reserve(fs_info, qg, -(s64)num_bytes);
-		if (qg->reserved < num_bytes)
-			report_reserved_underflow(fs_info, qg, num_bytes);
-		else
-			qg->reserved -= num_bytes;
+		qgroup_rsv_decrease(qg, num_bytes, type);
 
 		list_for_each_entry(glist, &qg->groups, next_group) {
 			ret = ulist_add(fs_info->qgroup_ulist,
@@ -2939,7 +2937,7 @@ int btrfs_qgroup_reserve_data(struct inode *inode,
 					to_reserve, QGROUP_RESERVE);
 	if (ret < 0)
 		goto cleanup;
-	ret = qgroup_reserve(root, to_reserve, true);
+	ret = qgroup_reserve(root, to_reserve, true, BTRFS_QGROUP_RSV_DATA);
 	if (ret < 0)
 		goto cleanup;
 
@@ -3093,7 +3091,7 @@ int btrfs_qgroup_reserve_meta(struct btrfs_root *root, int num_bytes,
 
 	BUG_ON(num_bytes != round_down(num_bytes, fs_info->nodesize));
 	trace_qgroup_meta_reserve(root, (s64)num_bytes);
-	ret = qgroup_reserve(root, num_bytes, enforce);
+	ret = qgroup_reserve(root, num_bytes, enforce, BTRFS_QGROUP_RSV_META);
 	if (ret < 0)
 		return ret;
 	atomic64_add(num_bytes, &root->qgroup_meta_rsv);
-- 
2.14.2

[PATCH 4/6] btrfs: qgroup: Fix wrong qgroup reservation inheritance for relationship update

[Qu Wenruo]

When modifying qgroup relationship, for qgroup which only owns exclusive
extents, we will go through quick update path.

In quick update path, we will just adding/removing exclusive and reference
number.

However we did the opposite for qgroup reservation from the very
beginning.

In fact, we should also inherit the qgroup reservation space, just like
exclusive and reference numbers.

Fix by using the newly introduced
qgroup_rsv_increase/decrease_by_qgroup() function call.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/qgroup.c | 39 ++++++++++++++++++---------------------
 1 file changed, 18 insertions(+), 21 deletions(-)

diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
index 7b89da9589c1..ba6f60fd0e96 100644
--- a/fs/btrfs/qgroup.c
+++ b/fs/btrfs/qgroup.c
@@ -1069,21 +1069,24 @@ static void report_reserved_underflow(struct btrfs_fs_info *fs_info,
 #endif
 	qgroup->reserved = 0;
 }
+
 /*
- * The easy accounting, if we are adding/removing the only ref for an extent
- * then this qgroup and all of the parent qgroups get their reference and
- * exclusive counts adjusted.
+ * The easy accounting, we're updating qgroup relationship whose child qgroup
+ * only have exclusive extents.
+ * In this case, we only need to update the rfer/excl, and inherit rsv from
+ * child qgroup (@src)
  *
  * Caller should hold fs_info->qgroup_lock.
  */
 static int __qgroup_excl_accounting(struct btrfs_fs_info *fs_info,
 				    struct ulist *tmp, u64 ref_root,
-				    u64 num_bytes, int sign)
+				    struct btrfs_qgroup *src, int sign)
 {
 	struct btrfs_qgroup *qgroup;
 	struct btrfs_qgroup_list *glist;
 	struct ulist_node *unode;
 	struct ulist_iterator uiter;
+	u64 num_bytes = src->excl;
 	int ret = 0;
 
 	qgroup = find_qgroup_rb(fs_info, ref_root);
@@ -1096,13 +1099,12 @@ static int __qgroup_excl_accounting(struct btrfs_fs_info *fs_info,
 	WARN_ON(sign < 0 && qgroup->excl < num_bytes);
 	qgroup->excl += sign * num_bytes;
 	qgroup->excl_cmpr += sign * num_bytes;
-	if (sign > 0) {
-		trace_qgroup_update_reserve(fs_info, qgroup, -(s64)num_bytes);
-		if (qgroup->reserved < num_bytes)
-			report_reserved_underflow(fs_info, qgroup, num_bytes);
-		else
-			qgroup->reserved -= num_bytes;
-	}
+
+	/* *Inherit* qgroup rsv info from @src */
+	if (sign > 0)
+		qgroup_rsv_increase_by_qgroup(qgroup, src);
+	else
+		qgroup_rsv_decrease_by_qgroup(qgroup, src);
 
 	qgroup_dirty(fs_info, qgroup);
 
@@ -1122,15 +1124,10 @@ static int __qgroup_excl_accounting(struct btrfs_fs_info *fs_info,
 		qgroup->rfer_cmpr += sign * num_bytes;
 		WARN_ON(sign < 0 && qgroup->excl < num_bytes);
 		qgroup->excl += sign * num_bytes;
-		if (sign > 0) {
-			trace_qgroup_update_reserve(fs_info, qgroup,
-						    -(s64)num_bytes);
-			if (qgroup->reserved < num_bytes)
-				report_reserved_underflow(fs_info, qgroup,
-							  num_bytes);
-			else
-				qgroup->reserved -= num_bytes;
-		}
+		if (sign > 0)
+			qgroup_rsv_increase_by_qgroup(qgroup, src);
+		else
+			qgroup_rsv_decrease_by_qgroup(qgroup, src);
 		qgroup->excl_cmpr += sign * num_bytes;
 		qgroup_dirty(fs_info, qgroup);
 
@@ -1173,7 +1170,7 @@ static int quick_update_accounting(struct btrfs_fs_info *fs_info,
 	if (qgroup->excl == qgroup->rfer) {
 		ret = 0;
 		err = __qgroup_excl_accounting(fs_info, tmp, dst,
-					       qgroup->excl, sign);
+					       qgroup, sign);
 		if (err < 0) {
 			ret = err;
 			goto out;
-- 
2.14.2

[PATCH 5/6] btrfs: qgroup: Update trace events to use new separate rsv types

[Qu Wenruo]

To make the trace point cleaner, remove the @fs_info parameter, and
integrate @fs_info to struct qgroup.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/qgroup.c            | 32 ++++++++++++++++++--------------
 include/trace/events/btrfs.h | 17 ++++++++++++-----
 2 files changed, 30 insertions(+), 19 deletions(-)

diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
index ba6f60fd0e96..2b587056b797 100644
--- a/fs/btrfs/qgroup.c
+++ b/fs/btrfs/qgroup.c
@@ -73,15 +73,19 @@ static const char *qgroup_rsv_type_str(enum btrfs_qgroup_rsv_type type)
 	return NULL;
 }
 
-static void qgroup_rsv_increase(struct btrfs_qgroup *qgroup, u64 num_bytes,
+static void qgroup_rsv_increase(struct btrfs_fs_info *fs_info,
+				struct btrfs_qgroup *qgroup, u64 num_bytes,
 				enum btrfs_qgroup_rsv_type type)
 {
+	trace_qgroup_update_reserve(fs_info, qgroup, num_bytes, type);
 	qgroup->rsv.values[type] += num_bytes;
 }
 
-static void qgroup_rsv_decrease(struct btrfs_qgroup *qgroup, u64 num_bytes,
+static void qgroup_rsv_decrease(struct btrfs_fs_info *fs_info,
+				struct btrfs_qgroup *qgroup, u64 num_bytes,
 				enum btrfs_qgroup_rsv_type type)
 {
+	trace_qgroup_update_reserve(fs_info, qgroup, -(s64)num_bytes, type);
 	if (qgroup->rsv.values[type] >= num_bytes) {
 		qgroup->rsv.values[type] -= num_bytes;
 		return;
@@ -95,22 +99,24 @@ static void qgroup_rsv_decrease(struct btrfs_qgroup *qgroup, u64 num_bytes,
 	qgroup->rsv.values[type] = 0;
 }
 
-static void qgroup_rsv_increase_by_qgroup(struct btrfs_qgroup *dest,
+static void qgroup_rsv_increase_by_qgroup(struct btrfs_fs_info *fs_info,
+					  struct btrfs_qgroup *dest,
 					  struct btrfs_qgroup *src)
 {
 	int i;
 
 	for (i = 0; i <= BTRFS_QGROUP_RSV_TYPES; i++)
-		qgroup_rsv_increase(dest, src->rsv.values[i], i);
+		qgroup_rsv_increase(fs_info, dest, src->rsv.values[i], i);
 }
 
-static void qgroup_rsv_decrease_by_qgroup(struct btrfs_qgroup *dest,
+static void qgroup_rsv_decrease_by_qgroup(struct btrfs_fs_info *fs_info,
+					  struct btrfs_qgroup *dest,
 					  struct btrfs_qgroup *src)
 {
 	int i;
 
 	for (i = 0; i <= BTRFS_QGROUP_RSV_TYPES; i++)
-		qgroup_rsv_decrease(dest, src->rsv.values[i], i);
+		qgroup_rsv_decrease(fs_info, dest, src->rsv.values[i], i);
 }
 
 static void btrfs_qgroup_update_old_refcnt(struct btrfs_qgroup *qg, u64 seq,
@@ -1102,9 +1108,9 @@ static int __qgroup_excl_accounting(struct btrfs_fs_info *fs_info,
 
 	/* *Inherit* qgroup rsv info from @src */
 	if (sign > 0)
-		qgroup_rsv_increase_by_qgroup(qgroup, src);
+		qgroup_rsv_increase_by_qgroup(fs_info, qgroup, src);
 	else
-		qgroup_rsv_decrease_by_qgroup(qgroup, src);
+		qgroup_rsv_decrease_by_qgroup(fs_info, qgroup, src);
 
 	qgroup_dirty(fs_info, qgroup);
 
@@ -1125,9 +1131,9 @@ static int __qgroup_excl_accounting(struct btrfs_fs_info *fs_info,
 		WARN_ON(sign < 0 && qgroup->excl < num_bytes);
 		qgroup->excl += sign * num_bytes;
 		if (sign > 0)
-			qgroup_rsv_increase_by_qgroup(qgroup, src);
+			qgroup_rsv_increase_by_qgroup(fs_info, qgroup, src);
 		else
-			qgroup_rsv_decrease_by_qgroup(qgroup, src);
+			qgroup_rsv_decrease_by_qgroup(fs_info, qgroup, src);
 		qgroup->excl_cmpr += sign * num_bytes;
 		qgroup_dirty(fs_info, qgroup);
 
@@ -2498,8 +2504,7 @@ static int qgroup_reserve(struct btrfs_root *root, u64 num_bytes, bool enforce,
 
 		qg = unode_aux_to_qgroup(unode);
 
-		trace_qgroup_update_reserve(fs_info, qg, num_bytes);
-		qgroup_rsv_increase(qg, num_bytes, type);
+		qgroup_rsv_increase(fs_info, qg, num_bytes, type);
 	}
 
 out:
@@ -2545,8 +2550,7 @@ void btrfs_qgroup_free_refroot(struct btrfs_fs_info *fs_info,
 
 		qg = unode_aux_to_qgroup(unode);
 
-		trace_qgroup_update_reserve(fs_info, qg, -(s64)num_bytes);
-		qgroup_rsv_decrease(qg, num_bytes, type);
+		qgroup_rsv_decrease(fs_info, qg, num_bytes, type);
 
 		list_for_each_entry(glist, &qg->groups, next_group) {
 			ret = ulist_add(fs_info->qgroup_ulist,
diff --git a/include/trace/events/btrfs.h b/include/trace/events/btrfs.h
index dc1d0df91e0b..68c4d11f55fa 100644
--- a/include/trace/events/btrfs.h
+++ b/include/trace/events/btrfs.h
@@ -63,6 +63,11 @@ struct prelim_ref;
 		 { BTRFS_FILE_EXTENT_REG,	"REG"	 },		\
 		 { BTRFS_FILE_EXTENT_PREALLOC,	"PREALLOC"})
 
+#define show_qgroup_rsv_type(type)					\
+	__print_symbolic(type,						\
+		{ BTRFS_QGROUP_RSV_DATA,	"DATA"	},		\
+		{ BTRFS_QGROUP_RSV_META,	"META"	})
+
 #define BTRFS_GROUP_FLAGS	\
 	{ BTRFS_BLOCK_GROUP_DATA,	"DATA"},	\
 	{ BTRFS_BLOCK_GROUP_SYSTEM,	"SYSTEM"},	\
@@ -1594,24 +1599,26 @@ TRACE_EVENT(qgroup_update_counters,
 TRACE_EVENT(qgroup_update_reserve,
 
 	TP_PROTO(struct btrfs_fs_info *fs_info, struct btrfs_qgroup *qgroup,
-		 s64 diff),
+		 s64 diff, int type),
 
-	TP_ARGS(fs_info, qgroup, diff),
+	TP_ARGS(fs_info, qgroup, diff, type),
 
 	TP_STRUCT__entry_btrfs(
 		__field(	u64,	qgid			)
 		__field(	u64,	cur_reserved		)
 		__field(	s64,	diff			)
+		__field(	int,	type			)
 	),
 
 	TP_fast_assign_btrfs(fs_info,
 		__entry->qgid		= qgroup->qgroupid;
-		__entry->cur_reserved	= qgroup->reserved;
+		__entry->cur_reserved	= qgroup->rsv.values[type];
 		__entry->diff		= diff;
 	),
 
-	TP_printk_btrfs("qgid=%llu cur_reserved=%llu diff=%lld",
-		__entry->qgid, __entry->cur_reserved, __entry->diff)
+	TP_printk_btrfs("qgid=%llu type=%s cur_reserved=%llu diff=%lld",
+		__entry->qgid, show_qgroup_rsv_type(__entry->type),
+		__entry->cur_reserved, __entry->diff)
 );
 
 TRACE_EVENT(qgroup_meta_reserve,
-- 
2.14.2

[PATCH 6/6] btrfs: qgroup: Cleanup the remaining old reservation counters

[Qu Wenruo]

So qgroup is switched to new separate types reservation system.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/qgroup.c | 13 -------------
 fs/btrfs/qgroup.h |  1 -
 2 files changed, 14 deletions(-)

diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
index 2b587056b797..c5936fc75a4d 100644
--- a/fs/btrfs/qgroup.c
+++ b/fs/btrfs/qgroup.c
@@ -1063,19 +1063,6 @@ static void qgroup_dirty(struct btrfs_fs_info *fs_info,
 		list_add(&qgroup->dirty, &fs_info->dirty_qgroups);
 }
 
-static void report_reserved_underflow(struct btrfs_fs_info *fs_info,
-				      struct btrfs_qgroup *qgroup,
-				      u64 num_bytes)
-{
-#ifdef CONFIG_BTRFS_DEBUG
-	WARN_ON(qgroup->reserved < num_bytes);
-	btrfs_debug(fs_info,
-		"qgroup %llu reserved space underflow, have: %llu, to free: %llu",
-		qgroup->qgroupid, qgroup->reserved, num_bytes);
-#endif
-	qgroup->reserved = 0;
-}
-
 /*
  * The easy accounting, we're updating qgroup relationship whose child qgroup
  * only have exclusive extents.
diff --git a/fs/btrfs/qgroup.h b/fs/btrfs/qgroup.h
index 0b04cbc5b5ce..7b21e61a399c 100644
--- a/fs/btrfs/qgroup.h
+++ b/fs/btrfs/qgroup.h
@@ -107,7 +107,6 @@ struct btrfs_qgroup {
 	/*
 	 * reservation tracking
 	 */
-	u64 reserved;
 	struct btrfs_qgroup_rsv rsv;
 
 	/*
-- 
2.14.2

Re: [PATCH 1/6] btrfs: qgroup: Skeleton to support separate qgroup reservation type

[Nikolay Borisov]

On 24.10.2017 11:39, Qu Wenruo wrote:
> Instead of single qgroup->reserved, use a new structure btrfs_qgroup_rsv
> to restore different types of reservation.
> 
> This patch only updates the header and needed modification to pass
> compile.
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>  fs/btrfs/qgroup.c | 16 ++++++++++------
>  fs/btrfs/qgroup.h | 27 +++++++++++++++++++++++++--
>  2 files changed, 35 insertions(+), 8 deletions(-)
> 
> diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
> index e172d4843eae..fe3adb996883 100644
> --- a/fs/btrfs/qgroup.c
> +++ b/fs/btrfs/qgroup.c
> @@ -2444,7 +2444,8 @@ static int qgroup_reserve(struct btrfs_root *root, u64 num_bytes, bool enforce)
>  }
>  
>  void btrfs_qgroup_free_refroot(struct btrfs_fs_info *fs_info,
> -			       u64 ref_root, u64 num_bytes)
> +			       u64 ref_root, u64 num_bytes,
> +			       enum btrfs_qgroup_rsv_type type)
>  {
>  	struct btrfs_root *quota_root;
>  	struct btrfs_qgroup *qgroup;
> @@ -2936,7 +2937,8 @@ static int qgroup_free_reserved_data(struct inode *inode,
>  			goto out;
>  		freed += changeset.bytes_changed;
>  	}
> -	btrfs_qgroup_free_refroot(root->fs_info, root->objectid, freed);
> +	btrfs_qgroup_free_refroot(root->fs_info, root->objectid, freed,
> +				  BTRFS_QGROUP_RSV_DATA);
>  	ret = freed;
>  out:
>  	extent_changeset_release(&changeset);
> @@ -2968,7 +2970,7 @@ static int __btrfs_qgroup_release_data(struct inode *inode,
>  	if (free)
>  		btrfs_qgroup_free_refroot(BTRFS_I(inode)->root->fs_info,
>  				BTRFS_I(inode)->root->objectid,
> -				changeset.bytes_changed);
> +				changeset.bytes_changed, BTRFS_QGROUP_RSV_DATA);
>  	ret = changeset.bytes_changed;
>  out:
>  	extent_changeset_release(&changeset);
> @@ -3045,7 +3047,8 @@ void btrfs_qgroup_free_meta_all(struct btrfs_root *root)
>  	if (reserved == 0)
>  		return;
>  	trace_qgroup_meta_reserve(root, -(s64)reserved);
> -	btrfs_qgroup_free_refroot(fs_info, root->objectid, reserved);
> +	btrfs_qgroup_free_refroot(fs_info, root->objectid, reserved,
> +				  BTRFS_QGROUP_RSV_META);
>  }
>  
>  void btrfs_qgroup_free_meta(struct btrfs_root *root, int num_bytes)
> @@ -3060,7 +3063,8 @@ void btrfs_qgroup_free_meta(struct btrfs_root *root, int num_bytes)
>  	WARN_ON(atomic64_read(&root->qgroup_meta_rsv) < num_bytes);
>  	atomic64_sub(num_bytes, &root->qgroup_meta_rsv);
>  	trace_qgroup_meta_reserve(root, -(s64)num_bytes);
> -	btrfs_qgroup_free_refroot(fs_info, root->objectid, num_bytes);
> +	btrfs_qgroup_free_refroot(fs_info, root->objectid, num_bytes,
> +				  BTRFS_QGROUP_RSV_META);
>  }
>  
>  /*
> @@ -3088,7 +3092,7 @@ void btrfs_qgroup_check_reserved_leak(struct inode *inode)
>  		}
>  		btrfs_qgroup_free_refroot(BTRFS_I(inode)->root->fs_info,
>  				BTRFS_I(inode)->root->objectid,
> -				changeset.bytes_changed);
> +				changeset.bytes_changed, BTRFS_QGROUP_RSV_DATA);
>  
>  	}
>  	extent_changeset_release(&changeset);
> diff --git a/fs/btrfs/qgroup.h b/fs/btrfs/qgroup.h
> index d9984e87cddf..0b04cbc5b5ce 100644
> --- a/fs/btrfs/qgroup.h
> +++ b/fs/btrfs/qgroup.h
> @@ -61,6 +61,26 @@ struct btrfs_qgroup_extent_record {
>  	struct ulist *old_roots;
>  };
>  
> +enum btrfs_qgroup_rsv_type {
> +	BTRFS_QGROUP_RSV_DATA = 0,
> +	BTRFS_QGROUP_RSV_META = 1,
> +	BTRFS_QGROUP_RSV_TYPES = 1,

nit: Why not BTRFS_QGROUP_RSV_TYPES_MAX = 2;

> +};
> +
> +/*
> + * Represents how many bytes we reserved for this qgroup.
> + *
> + * Each type should have different reservation behavior.
> + * E.g, data follows its io_tree flag modification, while
> + * *currently* meta is just reserve-and-clear during transcation.
> + *
> + * TODO: Add new type for delalloc, which can exist across several
> + * transaction.
> + */
> +struct btrfs_qgroup_rsv {
> +	u64 values[BTRFS_QGROUP_RSV_TYPES + 1];

nit: And here just BTRFS_QGROUP_RSV_TYPES_MAX rather than the +1 here,
seems more idiomatic to me.

> +};
> +
>  /*
>   * one struct for each qgroup, organized in fs_info->qgroup_tree.
>   */
> @@ -88,6 +108,7 @@ struct btrfs_qgroup {
>  	 * reservation tracking
>  	 */
>  	u64 reserved;
> +	struct btrfs_qgroup_rsv rsv;
>  
>  	/*
>  	 * lists
> @@ -228,12 +249,14 @@ int btrfs_qgroup_inherit(struct btrfs_trans_handle *trans,
>  			 struct btrfs_fs_info *fs_info, u64 srcid, u64 objectid,
>  			 struct btrfs_qgroup_inherit *inherit);
>  void btrfs_qgroup_free_refroot(struct btrfs_fs_info *fs_info,
> -			       u64 ref_root, u64 num_bytes);
> +			       u64 ref_root, u64 num_bytes,
> +			       enum btrfs_qgroup_rsv_type type);
>  static inline void btrfs_qgroup_free_delayed_ref(struct btrfs_fs_info *fs_info,
>  						 u64 ref_root, u64 num_bytes)
>  {
>  	trace_btrfs_qgroup_free_delayed_ref(fs_info, ref_root, num_bytes);
> -	btrfs_qgroup_free_refroot(fs_info, ref_root, num_bytes);
> +	btrfs_qgroup_free_refroot(fs_info, ref_root, num_bytes,
> +				  BTRFS_QGROUP_RSV_DATA);
>  }
>  
>  #ifdef CONFIG_BTRFS_FS_RUN_SANITY_TESTS
> 

Re: [PATCH 2/6] btrfs: qgroup: Introduce helpers to update and access new qgroup rsv

[Nikolay Borisov]

On 24.10.2017 11:39, Qu Wenruo wrote:
> Introduce helpers to:
> 
> 1) Get total reserved space
>    For limit calculation
> 
> 2) Increase reserved space for given type
> 2) Decrease reserved space for given type
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>  fs/btrfs/qgroup.c | 66 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 66 insertions(+)
> 
> diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
> index fe3adb996883..04fd145516ad 100644
> --- a/fs/btrfs/qgroup.c
> +++ b/fs/btrfs/qgroup.c
> @@ -47,6 +47,72 @@
>   *  - check all ioctl parameters
>   */
>  
> +/*
> + * Helpers to access qgroup reservation
> + *
> + * Callers should ensure the lock context and type are valid
> + */
> +
> +static u64 qgroup_rsv_total(const struct btrfs_qgroup *qgroup)
> +{
> +	u64 ret = 0;
> +	int i;
> +
> +	for (i = 0; i <= BTRFS_QGROUP_RSV_TYPES; i++)

So if you actually take up on my idea of having an explicit value which
denotes the end, the loop here would be just < *_END rather than the <=
which instantly looks suspicious of an off-by-one error.

> +		ret += qgroup->rsv.values[i];
> +
> +	return ret;
> +}
> +
> +static const char *qgroup_rsv_type_str(enum btrfs_qgroup_rsv_type type)
> +{
> +	if (type == BTRFS_QGROUP_RSV_DATA)
> +		return "data";
> +	if (type == BTRFS_QGROUP_RSV_META)
> +		return "meta";
> +	return NULL;
> +}
> +
> +static void qgroup_rsv_increase(struct btrfs_qgroup *qgroup, u64 num_bytes,
> +				enum btrfs_qgroup_rsv_type type)
> +{
> +	qgroup->rsv.values[type] += num_bytes;
> +}
> +
> +static void qgroup_rsv_decrease(struct btrfs_qgroup *qgroup, u64 num_bytes,
> +				enum btrfs_qgroup_rsv_type type)
> +{
> +	if (qgroup->rsv.values[type] >= num_bytes) {
> +		qgroup->rsv.values[type] -= num_bytes;
> +		return;
> +	}
> +#ifdef CONFIG_BTRFS_DEBUG
> +	WARN_RATELIMIT(1,
> +		"qgroup %llu %s reserved space underflow, have %llu to free %llu",
> +		qgroup->qgroupid, qgroup_rsv_type_str(type),
> +		qgroup->rsv.values[type], num_bytes);
> +#endif
> +	qgroup->rsv.values[type] = 0;
> +}


Perhaps these could be modelled after
block_rsv_use_bytes/block_rsv_add_bytes ? I'm not entirely sure of what
increasing the value actually does - reserving bytes or using already
reserved bytes, I guess it should be reserving. In this case what about
qgroup_bytes_reserve/qgroup_bytes_(free|unreserve) ?


> +
> +static void qgroup_rsv_increase_by_qgroup(struct btrfs_qgroup *dest,
> +					  struct btrfs_qgroup *src)
> +{
> +	int i;
> +
> +	for (i = 0; i <= BTRFS_QGROUP_RSV_TYPES; i++)
> +		qgroup_rsv_increase(dest, src->rsv.values[i], i);
> +}
> +
> +static void qgroup_rsv_decrease_by_qgroup(struct btrfs_qgroup *dest,
> +					  struct btrfs_qgroup *src)
> +{
> +	int i;
> +
> +	for (i = 0; i <= BTRFS_QGROUP_RSV_TYPES; i++)
> +		qgroup_rsv_decrease(dest, src->rsv.values[i], i);
> +}

Why don't you model these similar to what we already have for the block
rsv. In this case I believe those would correspond to the
btrfs_block_rsv_migrate. Admittedly we don't have a counterpart to
rsv_decrease_by_qgroup.


> +
>  static void btrfs_qgroup_update_old_refcnt(struct btrfs_qgroup *qg, u64 seq,
>  					   int mod)
>  {
> 

Re: [PATCH 1/6] btrfs: qgroup: Skeleton to support separate qgroup reservation type

[Qu Wenruo]

[-- Attachment #1.1: Type: text/plain, Size: 6108 bytes --]



On 2017年10月24日 19:00, Nikolay Borisov wrote:
> 
> 
> On 24.10.2017 11:39, Qu Wenruo wrote:
>> Instead of single qgroup->reserved, use a new structure btrfs_qgroup_rsv
>> to restore different types of reservation.
>>
>> This patch only updates the header and needed modification to pass
>> compile.
>>
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>> ---
>>  fs/btrfs/qgroup.c | 16 ++++++++++------
>>  fs/btrfs/qgroup.h | 27 +++++++++++++++++++++++++--
>>  2 files changed, 35 insertions(+), 8 deletions(-)
>>
>> diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
>> index e172d4843eae..fe3adb996883 100644
>> --- a/fs/btrfs/qgroup.c
>> +++ b/fs/btrfs/qgroup.c
>> @@ -2444,7 +2444,8 @@ static int qgroup_reserve(struct btrfs_root *root, u64 num_bytes, bool enforce)
>>  }
>>  
>>  void btrfs_qgroup_free_refroot(struct btrfs_fs_info *fs_info,
>> -			       u64 ref_root, u64 num_bytes)
>> +			       u64 ref_root, u64 num_bytes,
>> +			       enum btrfs_qgroup_rsv_type type)
>>  {
>>  	struct btrfs_root *quota_root;
>>  	struct btrfs_qgroup *qgroup;
>> @@ -2936,7 +2937,8 @@ static int qgroup_free_reserved_data(struct inode *inode,
>>  			goto out;
>>  		freed += changeset.bytes_changed;
>>  	}
>> -	btrfs_qgroup_free_refroot(root->fs_info, root->objectid, freed);
>> +	btrfs_qgroup_free_refroot(root->fs_info, root->objectid, freed,
>> +				  BTRFS_QGROUP_RSV_DATA);
>>  	ret = freed;
>>  out:
>>  	extent_changeset_release(&changeset);
>> @@ -2968,7 +2970,7 @@ static int __btrfs_qgroup_release_data(struct inode *inode,
>>  	if (free)
>>  		btrfs_qgroup_free_refroot(BTRFS_I(inode)->root->fs_info,
>>  				BTRFS_I(inode)->root->objectid,
>> -				changeset.bytes_changed);
>> +				changeset.bytes_changed, BTRFS_QGROUP_RSV_DATA);
>>  	ret = changeset.bytes_changed;
>>  out:
>>  	extent_changeset_release(&changeset);
>> @@ -3045,7 +3047,8 @@ void btrfs_qgroup_free_meta_all(struct btrfs_root *root)
>>  	if (reserved == 0)
>>  		return;
>>  	trace_qgroup_meta_reserve(root, -(s64)reserved);
>> -	btrfs_qgroup_free_refroot(fs_info, root->objectid, reserved);
>> +	btrfs_qgroup_free_refroot(fs_info, root->objectid, reserved,
>> +				  BTRFS_QGROUP_RSV_META);
>>  }
>>  
>>  void btrfs_qgroup_free_meta(struct btrfs_root *root, int num_bytes)
>> @@ -3060,7 +3063,8 @@ void btrfs_qgroup_free_meta(struct btrfs_root *root, int num_bytes)
>>  	WARN_ON(atomic64_read(&root->qgroup_meta_rsv) < num_bytes);
>>  	atomic64_sub(num_bytes, &root->qgroup_meta_rsv);
>>  	trace_qgroup_meta_reserve(root, -(s64)num_bytes);
>> -	btrfs_qgroup_free_refroot(fs_info, root->objectid, num_bytes);
>> +	btrfs_qgroup_free_refroot(fs_info, root->objectid, num_bytes,
>> +				  BTRFS_QGROUP_RSV_META);
>>  }
>>  
>>  /*
>> @@ -3088,7 +3092,7 @@ void btrfs_qgroup_check_reserved_leak(struct inode *inode)
>>  		}
>>  		btrfs_qgroup_free_refroot(BTRFS_I(inode)->root->fs_info,
>>  				BTRFS_I(inode)->root->objectid,
>> -				changeset.bytes_changed);
>> +				changeset.bytes_changed, BTRFS_QGROUP_RSV_DATA);
>>  
>>  	}
>>  	extent_changeset_release(&changeset);
>> diff --git a/fs/btrfs/qgroup.h b/fs/btrfs/qgroup.h
>> index d9984e87cddf..0b04cbc5b5ce 100644
>> --- a/fs/btrfs/qgroup.h
>> +++ b/fs/btrfs/qgroup.h
>> @@ -61,6 +61,26 @@ struct btrfs_qgroup_extent_record {
>>  	struct ulist *old_roots;
>>  };
>>  
>> +enum btrfs_qgroup_rsv_type {
>> +	BTRFS_QGROUP_RSV_DATA = 0,
>> +	BTRFS_QGROUP_RSV_META = 1,
>> +	BTRFS_QGROUP_RSV_TYPES = 1,
> 
> nit: Why not BTRFS_QGROUP_RSV_TYPES_MAX = 2;

My original plan is just as the same as yours.

However I still remember I did it before and David fixed it by using
TYPES, so I follow his naming schema here.

Kernel is also using this naming schema else where:
d91876496bcf ("btrfs: compress: put variables defined per compress type
in struct to make cache friendly")

> 
>> +};
>> +
>> +/*
>> + * Represents how many bytes we reserved for this qgroup.
>> + *
>> + * Each type should have different reservation behavior.
>> + * E.g, data follows its io_tree flag modification, while
>> + * *currently* meta is just reserve-and-clear during transcation.
>> + *
>> + * TODO: Add new type for delalloc, which can exist across several
>> + * transaction.
>> + */
>> +struct btrfs_qgroup_rsv {
>> +	u64 values[BTRFS_QGROUP_RSV_TYPES + 1];
> 
> nit: And here just BTRFS_QGROUP_RSV_TYPES_MAX rather than the +1 here,
> seems more idiomatic to me.

To follow same naming schema from David.
(IIRC it was about tree-checker patchset, checking file extent type part)

In fact, I crashed kernel several times due to the tiny +1, without even
a clue for hours just testing blindly, until latest gcc gives warning
about it.

Thanks,
Qu

> 
>> +};
>> +
>>  /*
>>   * one struct for each qgroup, organized in fs_info->qgroup_tree.
>>   */
>> @@ -88,6 +108,7 @@ struct btrfs_qgroup {
>>  	 * reservation tracking
>>  	 */
>>  	u64 reserved;
>> +	struct btrfs_qgroup_rsv rsv;
>>  
>>  	/*
>>  	 * lists
>> @@ -228,12 +249,14 @@ int btrfs_qgroup_inherit(struct btrfs_trans_handle *trans,
>>  			 struct btrfs_fs_info *fs_info, u64 srcid, u64 objectid,
>>  			 struct btrfs_qgroup_inherit *inherit);
>>  void btrfs_qgroup_free_refroot(struct btrfs_fs_info *fs_info,
>> -			       u64 ref_root, u64 num_bytes);
>> +			       u64 ref_root, u64 num_bytes,
>> +			       enum btrfs_qgroup_rsv_type type);
>>  static inline void btrfs_qgroup_free_delayed_ref(struct btrfs_fs_info *fs_info,
>>  						 u64 ref_root, u64 num_bytes)
>>  {
>>  	trace_btrfs_qgroup_free_delayed_ref(fs_info, ref_root, num_bytes);
>> -	btrfs_qgroup_free_refroot(fs_info, ref_root, num_bytes);
>> +	btrfs_qgroup_free_refroot(fs_info, ref_root, num_bytes,
>> +				  BTRFS_QGROUP_RSV_DATA);
>>  }
>>  
>>  #ifdef CONFIG_BTRFS_FS_RUN_SANITY_TESTS
>>
> --
> 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
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 520 bytes --]

Re: [PATCH 4/6] btrfs: qgroup: Fix wrong qgroup reservation inheritance for relationship update

[Nikolay Borisov]

On 24.10.2017 11:39, Qu Wenruo wrote:
> When modifying qgroup relationship, for qgroup which only owns exclusive
> extents, we will go through quick update path.
> 
> In quick update path, we will just adding/removing exclusive and reference
> number.
> 
> However we did the opposite for qgroup reservation from the very
> beginning.

I'm afraid this sentence doesn't give much information about what's
really going on.

> 
> In fact, we should also inherit the qgroup reservation space, just like
> exclusive and reference numbers.
> 
> Fix by using the newly introduced
> qgroup_rsv_increase/decrease_by_qgroup() function call.
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>  fs/btrfs/qgroup.c | 39 ++++++++++++++++++---------------------
>  1 file changed, 18 insertions(+), 21 deletions(-)
> 
> diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
> index 7b89da9589c1..ba6f60fd0e96 100644
> --- a/fs/btrfs/qgroup.c
> +++ b/fs/btrfs/qgroup.c
> @@ -1069,21 +1069,24 @@ static void report_reserved_underflow(struct btrfs_fs_info *fs_info,
>  #endif
>  	qgroup->reserved = 0;
>  }
> +
>  /*
> - * The easy accounting, if we are adding/removing the only ref for an extent
> - * then this qgroup and all of the parent qgroups get their reference and
> - * exclusive counts adjusted.
> + * The easy accounting, we're updating qgroup relationship whose child qgroup
> + * only have exclusive extents.
> + * In this case, we only need to update the rfer/excl, and inherit rsv from
> + * child qgroup (@src)
>   *
>   * Caller should hold fs_info->qgroup_lock.
>   */
>  static int __qgroup_excl_accounting(struct btrfs_fs_info *fs_info,
>  				    struct ulist *tmp, u64 ref_root,
> -				    u64 num_bytes, int sign)
> +				    struct btrfs_qgroup *src, int sign)
>  {
>  	struct btrfs_qgroup *qgroup;
>  	struct btrfs_qgroup_list *glist;
>  	struct ulist_node *unode;
>  	struct ulist_iterator uiter;
> +	u64 num_bytes = src->excl;
>  	int ret = 0;
>  
>  	qgroup = find_qgroup_rb(fs_info, ref_root);
> @@ -1096,13 +1099,12 @@ static int __qgroup_excl_accounting(struct btrfs_fs_info *fs_info,
>  	WARN_ON(sign < 0 && qgroup->excl < num_bytes);
>  	qgroup->excl += sign * num_bytes;
>  	qgroup->excl_cmpr += sign * num_bytes;
> -	if (sign > 0) {
> -		trace_qgroup_update_reserve(fs_info, qgroup, -(s64)num_bytes);
> -		if (qgroup->reserved < num_bytes)
> -			report_reserved_underflow(fs_info, qgroup, num_bytes);
> -		else
> -			qgroup->reserved -= num_bytes;
> -	}
> +
> +	/* *Inherit* qgroup rsv info from @src */
> +	if (sign > 0)
> +		qgroup_rsv_increase_by_qgroup(qgroup, src);
> +	else
> +		qgroup_rsv_decrease_by_qgroup(qgroup, src);


I'm a bit confused by the semantics of the 'sign' variable. So what you
are doing is that if sign is > 0 then you are "adding a relationship"
i.e. adding 'src reservation to 'qgroup', presumably because the src is
a child of qgroup? So you are handling both adding and deletion in the
if statement?

However, before that apparently only deleting a relation ship was
handled by that same if (And I believe that was wrong since if sign > 0
then we should be adding bytes but here we are subtracting). SO the bug
being fixed by this commit are actually 2 bugs:

1. Completely missing the "adding a relation ship case"
2. Incorrect hanlding of sign < 0, since this was handled by the sign >
0 case?



>  
>  	qgroup_dirty(fs_info, qgroup);
>  
> @@ -1122,15 +1124,10 @@ static int __qgroup_excl_accounting(struct btrfs_fs_info *fs_info,
>  		qgroup->rfer_cmpr += sign * num_bytes;
>  		WARN_ON(sign < 0 && qgroup->excl < num_bytes);
>  		qgroup->excl += sign * num_bytes;
> -		if (sign > 0) {
> -			trace_qgroup_update_reserve(fs_info, qgroup,
> -						    -(s64)num_bytes);
> -			if (qgroup->reserved < num_bytes)
> -				report_reserved_underflow(fs_info, qgroup,
> -							  num_bytes);
> -			else
> -				qgroup->reserved -= num_bytes;
> -		}
> +		if (sign > 0)
> +			qgroup_rsv_increase_by_qgroup(qgroup, src);
> +		else
> +			qgroup_rsv_decrease_by_qgroup(qgroup, src);
>  		qgroup->excl_cmpr += sign * num_bytes;
>  		qgroup_dirty(fs_info, qgroup);
>  
> @@ -1173,7 +1170,7 @@ static int quick_update_accounting(struct btrfs_fs_info *fs_info,
>  	if (qgroup->excl == qgroup->rfer) {
>  		ret = 0;
>  		err = __qgroup_excl_accounting(fs_info, tmp, dst,
> -					       qgroup->excl, sign);
> +					       qgroup, sign);
>  		if (err < 0) {
>  			ret = err;
>  			goto out;
> 

Re: [PATCH 2/6] btrfs: qgroup: Introduce helpers to update and access new qgroup rsv

[Qu Wenruo]

[-- Attachment #1.1: Type: text/plain, Size: 5128 bytes --]



On 2017年10月24日 19:07, Nikolay Borisov wrote:
> 
> 
> On 24.10.2017 11:39, Qu Wenruo wrote:
>> Introduce helpers to:
>>
>> 1) Get total reserved space
>>    For limit calculation
>>
>> 2) Increase reserved space for given type
>> 2) Decrease reserved space for given type
>>
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>> ---
>>  fs/btrfs/qgroup.c | 66 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 66 insertions(+)
>>
>> diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
>> index fe3adb996883..04fd145516ad 100644
>> --- a/fs/btrfs/qgroup.c
>> +++ b/fs/btrfs/qgroup.c
>> @@ -47,6 +47,72 @@
>>   *  - check all ioctl parameters
>>   */
>>  
>> +/*
>> + * Helpers to access qgroup reservation
>> + *
>> + * Callers should ensure the lock context and type are valid
>> + */
>> +
>> +static u64 qgroup_rsv_total(const struct btrfs_qgroup *qgroup)
>> +{
>> +	u64 ret = 0;
>> +	int i;
>> +
>> +	for (i = 0; i <= BTRFS_QGROUP_RSV_TYPES; i++)
> 
> So if you actually take up on my idea of having an explicit value which
> denotes the end, the loop here would be just < *_END rather than the <=
> which instantly looks suspicious of an off-by-one error.

Yeah, I really like to do it, as mentioned in previous mail.

But to follow the schema used elsewhere, I had no choice.

> 
>> +		ret += qgroup->rsv.values[i];
>> +
>> +	return ret;
>> +}
>> +
>> +static const char *qgroup_rsv_type_str(enum btrfs_qgroup_rsv_type type)
>> +{
>> +	if (type == BTRFS_QGROUP_RSV_DATA)
>> +		return "data";
>> +	if (type == BTRFS_QGROUP_RSV_META)
>> +		return "meta";
>> +	return NULL;
>> +}
>> +
>> +static void qgroup_rsv_increase(struct btrfs_qgroup *qgroup, u64 num_bytes,
>> +				enum btrfs_qgroup_rsv_type type)
>> +{
>> +	qgroup->rsv.values[type] += num_bytes;
>> +}
>> +
>> +static void qgroup_rsv_decrease(struct btrfs_qgroup *qgroup, u64 num_bytes,
>> +				enum btrfs_qgroup_rsv_type type)
>> +{
>> +	if (qgroup->rsv.values[type] >= num_bytes) {
>> +		qgroup->rsv.values[type] -= num_bytes;
>> +		return;
>> +	}
>> +#ifdef CONFIG_BTRFS_DEBUG
>> +	WARN_RATELIMIT(1,
>> +		"qgroup %llu %s reserved space underflow, have %llu to free %llu",
>> +		qgroup->qgroupid, qgroup_rsv_type_str(type),
>> +		qgroup->rsv.values[type], num_bytes);
>> +#endif
>> +	qgroup->rsv.values[type] = 0;
>> +}
> 
> 
> Perhaps these could be modelled after
> block_rsv_use_bytes/block_rsv_add_bytes ? I'm not entirely sure of what
> increasing the value actually does - reserving bytes or using already
> reserved bytes, I guess it should be reserving. In this case what about
> qgroup_bytes_reserve/qgroup_bytes_(free|unreserve) ?

I'm really bad at naming functions.
My original idea is qgroup_rsv_add() and qgroup_rsv_rename(), but
"rename" is 3 characters longer than "add", which makes me quite
uncomfortable.
(Maybe 'add' and 'sub' better?)

For the "reserve|free", it can be easily confused with
btrfs_qgroup_reserve|release_data().

But in fact, this qgroup_rsv_* functions are only used to maintain
qgroup->rsv, so it's less meaningful compared to other functions, like
btrfs_qgroup_reserve|release_data().

The value itself represents how many bytes it has already reserved for
later operation.
So qgroup_rsv_increase() normally means qgroup is reserving more space,
while qgroup_rsv_decrease() means qgroup reserved space is decreasing,
either released or freed.

Hopes above explanation could inspire you about better naming ideas.
(Because I really have no idea how to name it better while keeping the
name the same length)

> 
> 
>> +
>> +static void qgroup_rsv_increase_by_qgroup(struct btrfs_qgroup *dest,
>> +					  struct btrfs_qgroup *src)
>> +{
>> +	int i;
>> +
>> +	for (i = 0; i <= BTRFS_QGROUP_RSV_TYPES; i++)
>> +		qgroup_rsv_increase(dest, src->rsv.values[i], i);
>> +}
>> +
>> +static void qgroup_rsv_decrease_by_qgroup(struct btrfs_qgroup *dest,
>> +					  struct btrfs_qgroup *src)
>> +{
>> +	int i;
>> +
>> +	for (i = 0; i <= BTRFS_QGROUP_RSV_TYPES; i++)
>> +		qgroup_rsv_decrease(dest, src->rsv.values[i], i);
>> +}
> 
> Why don't you model these similar to what we already have for the block
> rsv. In this case I believe those would correspond to the
> btrfs_block_rsv_migrate. Admittedly we don't have a counterpart to
> rsv_decrease_by_qgroup.

Not sure about 'migrate', I think it's more like 'inherit', since @src
is not modified at all.

'increase' and 'decrease' are preferred mainly because they are the same
length, and represents the value change obviously enough.

I'm completely open to better naming schema.
(But it's better to have same length, I'm kind of paranoid about this)

Thanks,
Qu

> 
> 
>> +
>>  static void btrfs_qgroup_update_old_refcnt(struct btrfs_qgroup *qg, u64 seq,
>>  					   int mod)
>>  {
>>
> --
> 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
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 504 bytes --]

Re: [PATCH 4/6] btrfs: qgroup: Fix wrong qgroup reservation inheritance for relationship update

[Qu Wenruo]

[-- Attachment #1.1: Type: text/plain, Size: 6103 bytes --]



On 2017年10月24日 20:01, Nikolay Borisov wrote:
> 
> 
> On 24.10.2017 11:39, Qu Wenruo wrote:
>> When modifying qgroup relationship, for qgroup which only owns exclusive
>> extents, we will go through quick update path.
>>
>> In quick update path, we will just adding/removing exclusive and reference
>> number.
>>
>> However we did the opposite for qgroup reservation from the very
>> beginning.
> 
> I'm afraid this sentence doesn't give much information about what's
> really going on.

I'll try to reorganize it to give a better explanation on this.

> 
>>
>> In fact, we should also inherit the qgroup reservation space, just like
>> exclusive and reference numbers.
>>
>> Fix by using the newly introduced
>> qgroup_rsv_increase/decrease_by_qgroup() function call.
>>
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>> ---
>>  fs/btrfs/qgroup.c | 39 ++++++++++++++++++---------------------
>>  1 file changed, 18 insertions(+), 21 deletions(-)
>>
>> diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
>> index 7b89da9589c1..ba6f60fd0e96 100644
>> --- a/fs/btrfs/qgroup.c
>> +++ b/fs/btrfs/qgroup.c
>> @@ -1069,21 +1069,24 @@ static void report_reserved_underflow(struct btrfs_fs_info *fs_info,
>>  #endif
>>  	qgroup->reserved = 0;
>>  }
>> +
>>  /*
>> - * The easy accounting, if we are adding/removing the only ref for an extent
>> - * then this qgroup and all of the parent qgroups get their reference and
>> - * exclusive counts adjusted.
>> + * The easy accounting, we're updating qgroup relationship whose child qgroup
>> + * only have exclusive extents.
>> + * In this case, we only need to update the rfer/excl, and inherit rsv from
>> + * child qgroup (@src)
>>   *
>>   * Caller should hold fs_info->qgroup_lock.
>>   */
>>  static int __qgroup_excl_accounting(struct btrfs_fs_info *fs_info,
>>  				    struct ulist *tmp, u64 ref_root,
>> -				    u64 num_bytes, int sign)
>> +				    struct btrfs_qgroup *src, int sign)
>>  {
>>  	struct btrfs_qgroup *qgroup;
>>  	struct btrfs_qgroup_list *glist;
>>  	struct ulist_node *unode;
>>  	struct ulist_iterator uiter;
>> +	u64 num_bytes = src->excl;
>>  	int ret = 0;
>>  
>>  	qgroup = find_qgroup_rb(fs_info, ref_root);
>> @@ -1096,13 +1099,12 @@ static int __qgroup_excl_accounting(struct btrfs_fs_info *fs_info,
>>  	WARN_ON(sign < 0 && qgroup->excl < num_bytes);
>>  	qgroup->excl += sign * num_bytes;
>>  	qgroup->excl_cmpr += sign * num_bytes;
>> -	if (sign > 0) {
>> -		trace_qgroup_update_reserve(fs_info, qgroup, -(s64)num_bytes);
>> -		if (qgroup->reserved < num_bytes)
>> -			report_reserved_underflow(fs_info, qgroup, num_bytes);
>> -		else
>> -			qgroup->reserved -= num_bytes;
>> -	}
>> +
>> +	/* *Inherit* qgroup rsv info from @src */
>> +	if (sign > 0)
>> +		qgroup_rsv_increase_by_qgroup(qgroup, src);
>> +	else
>> +		qgroup_rsv_decrease_by_qgroup(qgroup, src);
> 
> 
> I'm a bit confused by the semantics of the 'sign' variable. So what you
> are doing is that if sign is > 0 then you are "adding a relationship"
> i.e. adding 'src reservation to 'qgroup', presumably because the src is
> a child of qgroup? So you are handling both adding and deletion in the
> if statement?

Yes, the original design of @sign is to allow single function to handle
both relationship adding and deleting.
just like the rest code, which uses @sign to handle both adding and
deleting without using if.

> 
> However, before that apparently only deleting a relation ship was
> handled by that same if (And I believe that was wrong since if sign > 0
> then we should be adding bytes but here we are subtracting). SO the bug
> being fixed by this commit are actually 2 bugs:
> 
> 1. Completely missing the "adding a relation ship case"
> 2. Incorrect hanlding of sign < 0, since this was handled by the sign >
> 0 case?

Yes, in fact 2 bugs.

Although the original code is acting like it's allocating space inside
the new parent, so it reduces parent's reserved, and adding new excl/refer.

However it's not the case, it should do inheriting, not allocating from
parent.

For sign > 0, (adding relationship) parent should inherit all excl/rfer
and reserved space.
For sign < 0, (deleting relationshio) parent should have all its
excl/rfer along with reserved space removed.

^^^ This should be the correct behavior.

The original code is just a copy of older code, as you can see in commit
9c8b35b1ba21 ("btrfs: quota: Automatically update related qgroups or
mark INCONSISTENT flags when assigning/deleting a qgroup relations.").

So it's a bug dating back to ancient days and it's my fault I didn't
expose it in the very beginning.

Thanks,
Qu
> 
> 
> 
>>  
>>  	qgroup_dirty(fs_info, qgroup);
>>  
>> @@ -1122,15 +1124,10 @@ static int __qgroup_excl_accounting(struct btrfs_fs_info *fs_info,
>>  		qgroup->rfer_cmpr += sign * num_bytes;
>>  		WARN_ON(sign < 0 && qgroup->excl < num_bytes);
>>  		qgroup->excl += sign * num_bytes;
>> -		if (sign > 0) {
>> -			trace_qgroup_update_reserve(fs_info, qgroup,
>> -						    -(s64)num_bytes);
>> -			if (qgroup->reserved < num_bytes)
>> -				report_reserved_underflow(fs_info, qgroup,
>> -							  num_bytes);
>> -			else
>> -				qgroup->reserved -= num_bytes;
>> -		}
>> +		if (sign > 0)
>> +			qgroup_rsv_increase_by_qgroup(qgroup, src);
>> +		else
>> +			qgroup_rsv_decrease_by_qgroup(qgroup, src);
>>  		qgroup->excl_cmpr += sign * num_bytes;
>>  		qgroup_dirty(fs_info, qgroup);
>>  
>> @@ -1173,7 +1170,7 @@ static int quick_update_accounting(struct btrfs_fs_info *fs_info,
>>  	if (qgroup->excl == qgroup->rfer) {
>>  		ret = 0;
>>  		err = __qgroup_excl_accounting(fs_info, tmp, dst,
>> -					       qgroup->excl, sign);
>> +					       qgroup, sign);
>>  		if (err < 0) {
>>  			ret = err;
>>  			goto out;
>>
> --
> 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
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 520 bytes --]

Re: [PATCH 1/6] btrfs: qgroup: Skeleton to support separate qgroup reservation type

[Nikolay Borisov]

On 24.10.2017 14:51, Qu Wenruo wrote:
> 
> 
> On 2017年10月24日 19:00, Nikolay Borisov wrote:
>>
>>
>> On 24.10.2017 11:39, Qu Wenruo wrote:
>>> Instead of single qgroup->reserved, use a new structure btrfs_qgroup_rsv
>>> to restore different types of reservation.
>>>
>>> This patch only updates the header and needed modification to pass
>>> compile.
>>>
>>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>>> ---
>>>  fs/btrfs/qgroup.c | 16 ++++++++++------
>>>  fs/btrfs/qgroup.h | 27 +++++++++++++++++++++++++--
>>>  2 files changed, 35 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
>>> index e172d4843eae..fe3adb996883 100644
>>> --- a/fs/btrfs/qgroup.c
>>> +++ b/fs/btrfs/qgroup.c
>>> @@ -2444,7 +2444,8 @@ static int qgroup_reserve(struct btrfs_root *root, u64 num_bytes, bool enforce)
>>>  }
>>>  
>>>  void btrfs_qgroup_free_refroot(struct btrfs_fs_info *fs_info,
>>> -			       u64 ref_root, u64 num_bytes)
>>> +			       u64 ref_root, u64 num_bytes,
>>> +			       enum btrfs_qgroup_rsv_type type)
>>>  {
>>>  	struct btrfs_root *quota_root;
>>>  	struct btrfs_qgroup *qgroup;
>>> @@ -2936,7 +2937,8 @@ static int qgroup_free_reserved_data(struct inode *inode,
>>>  			goto out;
>>>  		freed += changeset.bytes_changed;
>>>  	}
>>> -	btrfs_qgroup_free_refroot(root->fs_info, root->objectid, freed);
>>> +	btrfs_qgroup_free_refroot(root->fs_info, root->objectid, freed,
>>> +				  BTRFS_QGROUP_RSV_DATA);
>>>  	ret = freed;
>>>  out:
>>>  	extent_changeset_release(&changeset);
>>> @@ -2968,7 +2970,7 @@ static int __btrfs_qgroup_release_data(struct inode *inode,
>>>  	if (free)
>>>  		btrfs_qgroup_free_refroot(BTRFS_I(inode)->root->fs_info,
>>>  				BTRFS_I(inode)->root->objectid,
>>> -				changeset.bytes_changed);
>>> +				changeset.bytes_changed, BTRFS_QGROUP_RSV_DATA);
>>>  	ret = changeset.bytes_changed;
>>>  out:
>>>  	extent_changeset_release(&changeset);
>>> @@ -3045,7 +3047,8 @@ void btrfs_qgroup_free_meta_all(struct btrfs_root *root)
>>>  	if (reserved == 0)
>>>  		return;
>>>  	trace_qgroup_meta_reserve(root, -(s64)reserved);
>>> -	btrfs_qgroup_free_refroot(fs_info, root->objectid, reserved);
>>> +	btrfs_qgroup_free_refroot(fs_info, root->objectid, reserved,
>>> +				  BTRFS_QGROUP_RSV_META);
>>>  }
>>>  
>>>  void btrfs_qgroup_free_meta(struct btrfs_root *root, int num_bytes)
>>> @@ -3060,7 +3063,8 @@ void btrfs_qgroup_free_meta(struct btrfs_root *root, int num_bytes)
>>>  	WARN_ON(atomic64_read(&root->qgroup_meta_rsv) < num_bytes);
>>>  	atomic64_sub(num_bytes, &root->qgroup_meta_rsv);
>>>  	trace_qgroup_meta_reserve(root, -(s64)num_bytes);
>>> -	btrfs_qgroup_free_refroot(fs_info, root->objectid, num_bytes);
>>> +	btrfs_qgroup_free_refroot(fs_info, root->objectid, num_bytes,
>>> +				  BTRFS_QGROUP_RSV_META);
>>>  }
>>>  
>>>  /*
>>> @@ -3088,7 +3092,7 @@ void btrfs_qgroup_check_reserved_leak(struct inode *inode)
>>>  		}
>>>  		btrfs_qgroup_free_refroot(BTRFS_I(inode)->root->fs_info,
>>>  				BTRFS_I(inode)->root->objectid,
>>> -				changeset.bytes_changed);
>>> +				changeset.bytes_changed, BTRFS_QGROUP_RSV_DATA);
>>>  
>>>  	}
>>>  	extent_changeset_release(&changeset);
>>> diff --git a/fs/btrfs/qgroup.h b/fs/btrfs/qgroup.h
>>> index d9984e87cddf..0b04cbc5b5ce 100644
>>> --- a/fs/btrfs/qgroup.h
>>> +++ b/fs/btrfs/qgroup.h
>>> @@ -61,6 +61,26 @@ struct btrfs_qgroup_extent_record {
>>>  	struct ulist *old_roots;
>>>  };
>>>  
>>> +enum btrfs_qgroup_rsv_type {
>>> +	BTRFS_QGROUP_RSV_DATA = 0,
>>> +	BTRFS_QGROUP_RSV_META = 1,
>>> +	BTRFS_QGROUP_RSV_TYPES = 1,
>>
>> nit: Why not BTRFS_QGROUP_RSV_TYPES_MAX = 2;
> 
> My original plan is just as the same as yours.
> 
> However I still remember I did it before and David fixed it by using
> TYPES, so I follow his naming schema here.

I don't really care about whether it's called TYPES or _MAX. The more
important thing is the fact that we have types set to the same value as
the last valid value and thus when writing loops or checking ranges we
need to use <= and + 1 respectively which IMO is a bit
counter-intuitive. But as I said it's a nit so I don't have strong
preference either way if this is the convention of btrfs so be it.
> 
> Kernel is also using this naming schema else where:
> d91876496bcf ("btrfs: compress: put variables defined per compress type
> in struct to make cache friendly")
> 
>>
>>> +};
>>> +
>>> +/*
>>> + * Represents how many bytes we reserved for this qgroup.
>>> + *
>>> + * Each type should have different reservation behavior.
>>> + * E.g, data follows its io_tree flag modification, while
>>> + * *currently* meta is just reserve-and-clear during transcation.
>>> + *
>>> + * TODO: Add new type for delalloc, which can exist across several
>>> + * transaction.
>>> + */
>>> +struct btrfs_qgroup_rsv {
>>> +	u64 values[BTRFS_QGROUP_RSV_TYPES + 1];
>>
>> nit: And here just BTRFS_QGROUP_RSV_TYPES_MAX rather than the +1 here,
>> seems more idiomatic to me.
> 
> To follow same naming schema from David.
> (IIRC it was about tree-checker patchset, checking file extent type part)
> 
> In fact, I crashed kernel several times due to the tiny +1, without even
> a clue for hours just testing blindly, until latest gcc gives warning
> about it.

So you crashed the kernel because you were missing it or not ?

> 
> Thanks,
> Qu
> 
>>
>>> +};
>>> +
>>>  /*
>>>   * one struct for each qgroup, organized in fs_info->qgroup_tree.
>>>   */
>>> @@ -88,6 +108,7 @@ struct btrfs_qgroup {
>>>  	 * reservation tracking
>>>  	 */
>>>  	u64 reserved;
>>> +	struct btrfs_qgroup_rsv rsv;
>>>  
>>>  	/*
>>>  	 * lists
>>> @@ -228,12 +249,14 @@ int btrfs_qgroup_inherit(struct btrfs_trans_handle *trans,
>>>  			 struct btrfs_fs_info *fs_info, u64 srcid, u64 objectid,
>>>  			 struct btrfs_qgroup_inherit *inherit);
>>>  void btrfs_qgroup_free_refroot(struct btrfs_fs_info *fs_info,
>>> -			       u64 ref_root, u64 num_bytes);
>>> +			       u64 ref_root, u64 num_bytes,
>>> +			       enum btrfs_qgroup_rsv_type type);
>>>  static inline void btrfs_qgroup_free_delayed_ref(struct btrfs_fs_info *fs_info,
>>>  						 u64 ref_root, u64 num_bytes)
>>>  {
>>>  	trace_btrfs_qgroup_free_delayed_ref(fs_info, ref_root, num_bytes);
>>> -	btrfs_qgroup_free_refroot(fs_info, ref_root, num_bytes);
>>> +	btrfs_qgroup_free_refroot(fs_info, ref_root, num_bytes,
>>> +				  BTRFS_QGROUP_RSV_DATA);
>>>  }
>>>  
>>>  #ifdef CONFIG_BTRFS_FS_RUN_SANITY_TESTS
>>>
>> --
>> 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
>>
> 

Re: [PATCH 4/6] btrfs: qgroup: Fix wrong qgroup reservation inheritance for relationship update

[Nikolay Borisov]

On 24.10.2017 15:19, Qu Wenruo wrote:
> 
> 
> On 2017年10月24日 20:01, Nikolay Borisov wrote:
>>
>>
>> On 24.10.2017 11:39, Qu Wenruo wrote:
>>> When modifying qgroup relationship, for qgroup which only owns exclusive
>>> extents, we will go through quick update path.
>>>
>>> In quick update path, we will just adding/removing exclusive and reference
>>> number.
>>>
>>> However we did the opposite for qgroup reservation from the very
>>> beginning.
>>
>> I'm afraid this sentence doesn't give much information about what's
>> really going on.
> 
> I'll try to reorganize it to give a better explanation on this.
> 
>>
>>>
>>> In fact, we should also inherit the qgroup reservation space, just like
>>> exclusive and reference numbers.
>>>
>>> Fix by using the newly introduced
>>> qgroup_rsv_increase/decrease_by_qgroup() function call.
>>>
>>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>>> ---
>>>  fs/btrfs/qgroup.c | 39 ++++++++++++++++++---------------------
>>>  1 file changed, 18 insertions(+), 21 deletions(-)
>>>
>>> diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
>>> index 7b89da9589c1..ba6f60fd0e96 100644
>>> --- a/fs/btrfs/qgroup.c
>>> +++ b/fs/btrfs/qgroup.c
>>> @@ -1069,21 +1069,24 @@ static void report_reserved_underflow(struct btrfs_fs_info *fs_info,
>>>  #endif
>>>  	qgroup->reserved = 0;
>>>  }
>>> +
>>>  /*
>>> - * The easy accounting, if we are adding/removing the only ref for an extent
>>> - * then this qgroup and all of the parent qgroups get their reference and
>>> - * exclusive counts adjusted.
>>> + * The easy accounting, we're updating qgroup relationship whose child qgroup
>>> + * only have exclusive extents.
>>> + * In this case, we only need to update the rfer/excl, and inherit rsv from
>>> + * child qgroup (@src)
>>>   *
>>>   * Caller should hold fs_info->qgroup_lock.
>>>   */
>>>  static int __qgroup_excl_accounting(struct btrfs_fs_info *fs_info,
>>>  				    struct ulist *tmp, u64 ref_root,
>>> -				    u64 num_bytes, int sign)
>>> +				    struct btrfs_qgroup *src, int sign)
>>>  {
>>>  	struct btrfs_qgroup *qgroup;
>>>  	struct btrfs_qgroup_list *glist;
>>>  	struct ulist_node *unode;
>>>  	struct ulist_iterator uiter;
>>> +	u64 num_bytes = src->excl;
>>>  	int ret = 0;
>>>  
>>>  	qgroup = find_qgroup_rb(fs_info, ref_root);
>>> @@ -1096,13 +1099,12 @@ static int __qgroup_excl_accounting(struct btrfs_fs_info *fs_info,
>>>  	WARN_ON(sign < 0 && qgroup->excl < num_bytes);
>>>  	qgroup->excl += sign * num_bytes;
>>>  	qgroup->excl_cmpr += sign * num_bytes;
>>> -	if (sign > 0) {
>>> -		trace_qgroup_update_reserve(fs_info, qgroup, -(s64)num_bytes);
>>> -		if (qgroup->reserved < num_bytes)
>>> -			report_reserved_underflow(fs_info, qgroup, num_bytes);
>>> -		else
>>> -			qgroup->reserved -= num_bytes;
>>> -	}
>>> +
>>> +	/* *Inherit* qgroup rsv info from @src */
>>> +	if (sign > 0)
>>> +		qgroup_rsv_increase_by_qgroup(qgroup, src);
>>> +	else
>>> +		qgroup_rsv_decrease_by_qgroup(qgroup, src);
>>
>>
>> I'm a bit confused by the semantics of the 'sign' variable. So what you
>> are doing is that if sign is > 0 then you are "adding a relationship"
>> i.e. adding 'src reservation to 'qgroup', presumably because the src is
>> a child of qgroup? So you are handling both adding and deletion in the
>> if statement?
> 
> Yes, the original design of @sign is to allow single function to handle
> both relationship adding and deleting.
> just like the rest code, which uses @sign to handle both adding and
> deleting without using if.
> 
>>
>> However, before that apparently only deleting a relation ship was
>> handled by that same if (And I believe that was wrong since if sign > 0
>> then we should be adding bytes but here we are subtracting). SO the bug
>> being fixed by this commit are actually 2 bugs:
>>
>> 1. Completely missing the "adding a relation ship case"
>> 2. Incorrect hanlding of sign < 0, since this was handled by the sign >
>> 0 case?
> 
> Yes, in fact 2 bugs.
> 
> Although the original code is acting like it's allocating space inside
> the new parent, so it reduces parent's reserved, and adding new excl/refer.
> 
> However it's not the case, it should do inheriting, not allocating from
> parent.
> 
> For sign > 0, (adding relationship) parent should inherit all excl/rfer
> and reserved space.
> For sign < 0, (deleting relationshio) parent should have all its
> excl/rfer along with reserved space removed.
> 
> ^^^ This should be the correct behavior.

In that case I think this explanation needs to go into the commit
message itself.

> 
> The original code is just a copy of older code, as you can see in commit
> 9c8b35b1ba21 ("btrfs: quota: Automatically update related qgroups or
> mark INCONSISTENT flags when assigning/deleting a qgroup relations.").

You can also add this about how the bug got introduced in the first place.

> 
> So it's a bug dating back to ancient days and it's my fault I didn't
> expose it in the very beginning.
> 
> Thanks,
> Qu
>>
>>
>>
>>>  
>>>  	qgroup_dirty(fs_info, qgroup);
>>>  
>>> @@ -1122,15 +1124,10 @@ static int __qgroup_excl_accounting(struct btrfs_fs_info *fs_info,
>>>  		qgroup->rfer_cmpr += sign * num_bytes;
>>>  		WARN_ON(sign < 0 && qgroup->excl < num_bytes);
>>>  		qgroup->excl += sign * num_bytes;
>>> -		if (sign > 0) {
>>> -			trace_qgroup_update_reserve(fs_info, qgroup,
>>> -						    -(s64)num_bytes);
>>> -			if (qgroup->reserved < num_bytes)
>>> -				report_reserved_underflow(fs_info, qgroup,
>>> -							  num_bytes);
>>> -			else
>>> -				qgroup->reserved -= num_bytes;
>>> -		}
>>> +		if (sign > 0)
>>> +			qgroup_rsv_increase_by_qgroup(qgroup, src);
>>> +		else
>>> +			qgroup_rsv_decrease_by_qgroup(qgroup, src);
>>>  		qgroup->excl_cmpr += sign * num_bytes;
>>>  		qgroup_dirty(fs_info, qgroup);
>>>  
>>> @@ -1173,7 +1170,7 @@ static int quick_update_accounting(struct btrfs_fs_info *fs_info,
>>>  	if (qgroup->excl == qgroup->rfer) {
>>>  		ret = 0;
>>>  		err = __qgroup_excl_accounting(fs_info, tmp, dst,
>>> -					       qgroup->excl, sign);
>>> +					       qgroup, sign);
>>>  		if (err < 0) {
>>>  			ret = err;
>>>  			goto out;
>>>
>> --
>> 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
>>
> 

Re: [PATCH 1/6] btrfs: qgroup: Skeleton to support separate qgroup reservation type

[Jeff Mahoney]

[-- Attachment #1.1: Type: text/plain, Size: 7346 bytes --]

On 10/24/17 7:51 AM, Qu Wenruo wrote:
> 
> 
> On 2017年10月24日 19:00, Nikolay Borisov wrote:
>>
>>
>> On 24.10.2017 11:39, Qu Wenruo wrote:
>>> Instead of single qgroup->reserved, use a new structure btrfs_qgroup_rsv
>>> to restore different types of reservation.
>>>
>>> This patch only updates the header and needed modification to pass
>>> compile.
>>>
>>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>>> ---
>>>  fs/btrfs/qgroup.c | 16 ++++++++++------
>>>  fs/btrfs/qgroup.h | 27 +++++++++++++++++++++++++--
>>>  2 files changed, 35 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
>>> index e172d4843eae..fe3adb996883 100644
>>> --- a/fs/btrfs/qgroup.c
>>> +++ b/fs/btrfs/qgroup.c
>>> @@ -2444,7 +2444,8 @@ static int qgroup_reserve(struct btrfs_root *root, u64 num_bytes, bool enforce)
>>>  }
>>>  
>>>  void btrfs_qgroup_free_refroot(struct btrfs_fs_info *fs_info,
>>> -			       u64 ref_root, u64 num_bytes)
>>> +			       u64 ref_root, u64 num_bytes,
>>> +			       enum btrfs_qgroup_rsv_type type)
>>>  {
>>>  	struct btrfs_root *quota_root;
>>>  	struct btrfs_qgroup *qgroup;
>>> @@ -2936,7 +2937,8 @@ static int qgroup_free_reserved_data(struct inode *inode,
>>>  			goto out;
>>>  		freed += changeset.bytes_changed;
>>>  	}
>>> -	btrfs_qgroup_free_refroot(root->fs_info, root->objectid, freed);
>>> +	btrfs_qgroup_free_refroot(root->fs_info, root->objectid, freed,
>>> +				  BTRFS_QGROUP_RSV_DATA);
>>>  	ret = freed;
>>>  out:
>>>  	extent_changeset_release(&changeset);
>>> @@ -2968,7 +2970,7 @@ static int __btrfs_qgroup_release_data(struct inode *inode,
>>>  	if (free)
>>>  		btrfs_qgroup_free_refroot(BTRFS_I(inode)->root->fs_info,
>>>  				BTRFS_I(inode)->root->objectid,
>>> -				changeset.bytes_changed);
>>> +				changeset.bytes_changed, BTRFS_QGROUP_RSV_DATA);
>>>  	ret = changeset.bytes_changed;
>>>  out:
>>>  	extent_changeset_release(&changeset);
>>> @@ -3045,7 +3047,8 @@ void btrfs_qgroup_free_meta_all(struct btrfs_root *root)
>>>  	if (reserved == 0)
>>>  		return;
>>>  	trace_qgroup_meta_reserve(root, -(s64)reserved);
>>> -	btrfs_qgroup_free_refroot(fs_info, root->objectid, reserved);
>>> +	btrfs_qgroup_free_refroot(fs_info, root->objectid, reserved,
>>> +				  BTRFS_QGROUP_RSV_META);
>>>  }
>>>  
>>>  void btrfs_qgroup_free_meta(struct btrfs_root *root, int num_bytes)
>>> @@ -3060,7 +3063,8 @@ void btrfs_qgroup_free_meta(struct btrfs_root *root, int num_bytes)
>>>  	WARN_ON(atomic64_read(&root->qgroup_meta_rsv) < num_bytes);
>>>  	atomic64_sub(num_bytes, &root->qgroup_meta_rsv);
>>>  	trace_qgroup_meta_reserve(root, -(s64)num_bytes);
>>> -	btrfs_qgroup_free_refroot(fs_info, root->objectid, num_bytes);
>>> +	btrfs_qgroup_free_refroot(fs_info, root->objectid, num_bytes,
>>> +				  BTRFS_QGROUP_RSV_META);
>>>  }
>>>  
>>>  /*
>>> @@ -3088,7 +3092,7 @@ void btrfs_qgroup_check_reserved_leak(struct inode *inode)
>>>  		}
>>>  		btrfs_qgroup_free_refroot(BTRFS_I(inode)->root->fs_info,
>>>  				BTRFS_I(inode)->root->objectid,
>>> -				changeset.bytes_changed);
>>> +				changeset.bytes_changed, BTRFS_QGROUP_RSV_DATA);
>>>  
>>>  	}
>>>  	extent_changeset_release(&changeset);
>>> diff --git a/fs/btrfs/qgroup.h b/fs/btrfs/qgroup.h
>>> index d9984e87cddf..0b04cbc5b5ce 100644
>>> --- a/fs/btrfs/qgroup.h
>>> +++ b/fs/btrfs/qgroup.h
>>> @@ -61,6 +61,26 @@ struct btrfs_qgroup_extent_record {
>>>  	struct ulist *old_roots;
>>>  };
>>>  
>>> +enum btrfs_qgroup_rsv_type {
>>> +	BTRFS_QGROUP_RSV_DATA = 0,
>>> +	BTRFS_QGROUP_RSV_META = 1,
>>> +	BTRFS_QGROUP_RSV_TYPES = 1,
>>
>> nit: Why not BTRFS_QGROUP_RSV_TYPES_MAX = 2;
> 
> My original plan is just as the same as yours.
> 
> However I still remember I did it before and David fixed it by using
> TYPES, so I follow his naming schema here.
> 
> Kernel is also using this naming schema else where:
> d91876496bcf ("btrfs: compress: put variables defined per compress type
> in struct to make cache friendly")

The COMPRESS_TYPES pattern isn't the right pattern to follow here.
That's a special case since there's a _NONE that doesn't have anything
associated with it, so we don't need to take a slot in the array.

We also don't care about any of the specific values, just that they
start at 0.  The BTRFS_COMPRESS_TYPES example also has a
BTRFS_COMPRESS_LAST item in the enum, which serves the same purpose as
MAX.  I don't have a strong opinion on the naming, just that we don't
play games with +1 when handling arrays since, as you say, that's just
waiting for subtle bugs later.

enum btrfs_qgroup_rsv_type {
	BTRFS_QGROUP_RSV_DATA = 0,
	BTRFS_QGROUP_RSV_META,
	BTRFS_QGROUP_RSV_LAST,
};

>>> +};
>>> +
>>> +/*
>>> + * Represents how many bytes we reserved for this qgroup.
>>> + *
>>> + * Each type should have different reservation behavior.
>>> + * E.g, data follows its io_tree flag modification, while
>>> + * *currently* meta is just reserve-and-clear during transcation.
>>> + *
>>> + * TODO: Add new type for delalloc, which can exist across several
>>> + * transaction.
>>> + */

Minor nit: It's not just delalloc.  Delayed items and inodes can as
well.  The general rule is that qgroup reservations aren't essentially
different from block reservations and follow the same usage patterns
when operating on leaf nodes.

>>> +struct btrfs_qgroup_rsv {
>>> +	u64 values[BTRFS_QGROUP_RSV_TYPES + 1];
>>
>> nit: And here just BTRFS_QGROUP_RSV_TYPES_MAX rather than the +1 here,
>> seems more idiomatic to me.
> 
> To follow same naming schema from David.
> (IIRC it was about tree-checker patchset, checking file extent type part)
> 
> In fact, I crashed kernel several times due to the tiny +1, without even
> a clue for hours just testing blindly, until latest gcc gives warning
> about it.

BTRFS_QGROUP_RSV_LAST would do the job here.

-Jeff

>>
>>> +};
>>> +
>>>  /*
>>>   * one struct for each qgroup, organized in fs_info->qgroup_tree.
>>>   */
>>> @@ -88,6 +108,7 @@ struct btrfs_qgroup {
>>>  	 * reservation tracking
>>>  	 */
>>>  	u64 reserved;
>>> +	struct btrfs_qgroup_rsv rsv;
>>>  
>>>  	/*
>>>  	 * lists
>>> @@ -228,12 +249,14 @@ int btrfs_qgroup_inherit(struct btrfs_trans_handle *trans,
>>>  			 struct btrfs_fs_info *fs_info, u64 srcid, u64 objectid,
>>>  			 struct btrfs_qgroup_inherit *inherit);
>>>  void btrfs_qgroup_free_refroot(struct btrfs_fs_info *fs_info,
>>> -			       u64 ref_root, u64 num_bytes);
>>> +			       u64 ref_root, u64 num_bytes,
>>> +			       enum btrfs_qgroup_rsv_type type);
>>>  static inline void btrfs_qgroup_free_delayed_ref(struct btrfs_fs_info *fs_info,
>>>  						 u64 ref_root, u64 num_bytes)
>>>  {
>>>  	trace_btrfs_qgroup_free_delayed_ref(fs_info, ref_root, num_bytes);
>>> -	btrfs_qgroup_free_refroot(fs_info, ref_root, num_bytes);
>>> +	btrfs_qgroup_free_refroot(fs_info, ref_root, num_bytes,
>>> +				  BTRFS_QGROUP_RSV_DATA);
>>>  }
>>>  
>>>  #ifdef CONFIG_BTRFS_FS_RUN_SANITY_TESTS
>>>
>> --
>> 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
>>
> 


-- 
Jeff Mahoney
SUSE Labs


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 1/6] btrfs: qgroup: Skeleton to support separate qgroup reservation type

[Qu Wenruo]

[-- Attachment #1.1: Type: text/plain, Size: 7160 bytes --]



On 2017年10月24日 20:23, Nikolay Borisov wrote:
> 
> 
> On 24.10.2017 14:51, Qu Wenruo wrote:
>>
>>
>> On 2017年10月24日 19:00, Nikolay Borisov wrote:
>>>
>>>
>>> On 24.10.2017 11:39, Qu Wenruo wrote:
>>>> Instead of single qgroup->reserved, use a new structure btrfs_qgroup_rsv
>>>> to restore different types of reservation.
>>>>
>>>> This patch only updates the header and needed modification to pass
>>>> compile.
>>>>
>>>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>>>> ---
>>>>  fs/btrfs/qgroup.c | 16 ++++++++++------
>>>>  fs/btrfs/qgroup.h | 27 +++++++++++++++++++++++++--
>>>>  2 files changed, 35 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
>>>> index e172d4843eae..fe3adb996883 100644
>>>> --- a/fs/btrfs/qgroup.c
>>>> +++ b/fs/btrfs/qgroup.c
>>>> @@ -2444,7 +2444,8 @@ static int qgroup_reserve(struct btrfs_root *root, u64 num_bytes, bool enforce)
>>>>  }
>>>>  
>>>>  void btrfs_qgroup_free_refroot(struct btrfs_fs_info *fs_info,
>>>> -			       u64 ref_root, u64 num_bytes)
>>>> +			       u64 ref_root, u64 num_bytes,
>>>> +			       enum btrfs_qgroup_rsv_type type)
>>>>  {
>>>>  	struct btrfs_root *quota_root;
>>>>  	struct btrfs_qgroup *qgroup;
>>>> @@ -2936,7 +2937,8 @@ static int qgroup_free_reserved_data(struct inode *inode,
>>>>  			goto out;
>>>>  		freed += changeset.bytes_changed;
>>>>  	}
>>>> -	btrfs_qgroup_free_refroot(root->fs_info, root->objectid, freed);
>>>> +	btrfs_qgroup_free_refroot(root->fs_info, root->objectid, freed,
>>>> +				  BTRFS_QGROUP_RSV_DATA);
>>>>  	ret = freed;
>>>>  out:
>>>>  	extent_changeset_release(&changeset);
>>>> @@ -2968,7 +2970,7 @@ static int __btrfs_qgroup_release_data(struct inode *inode,
>>>>  	if (free)
>>>>  		btrfs_qgroup_free_refroot(BTRFS_I(inode)->root->fs_info,
>>>>  				BTRFS_I(inode)->root->objectid,
>>>> -				changeset.bytes_changed);
>>>> +				changeset.bytes_changed, BTRFS_QGROUP_RSV_DATA);
>>>>  	ret = changeset.bytes_changed;
>>>>  out:
>>>>  	extent_changeset_release(&changeset);
>>>> @@ -3045,7 +3047,8 @@ void btrfs_qgroup_free_meta_all(struct btrfs_root *root)
>>>>  	if (reserved == 0)
>>>>  		return;
>>>>  	trace_qgroup_meta_reserve(root, -(s64)reserved);
>>>> -	btrfs_qgroup_free_refroot(fs_info, root->objectid, reserved);
>>>> +	btrfs_qgroup_free_refroot(fs_info, root->objectid, reserved,
>>>> +				  BTRFS_QGROUP_RSV_META);
>>>>  }
>>>>  
>>>>  void btrfs_qgroup_free_meta(struct btrfs_root *root, int num_bytes)
>>>> @@ -3060,7 +3063,8 @@ void btrfs_qgroup_free_meta(struct btrfs_root *root, int num_bytes)
>>>>  	WARN_ON(atomic64_read(&root->qgroup_meta_rsv) < num_bytes);
>>>>  	atomic64_sub(num_bytes, &root->qgroup_meta_rsv);
>>>>  	trace_qgroup_meta_reserve(root, -(s64)num_bytes);
>>>> -	btrfs_qgroup_free_refroot(fs_info, root->objectid, num_bytes);
>>>> +	btrfs_qgroup_free_refroot(fs_info, root->objectid, num_bytes,
>>>> +				  BTRFS_QGROUP_RSV_META);
>>>>  }
>>>>  
>>>>  /*
>>>> @@ -3088,7 +3092,7 @@ void btrfs_qgroup_check_reserved_leak(struct inode *inode)
>>>>  		}
>>>>  		btrfs_qgroup_free_refroot(BTRFS_I(inode)->root->fs_info,
>>>>  				BTRFS_I(inode)->root->objectid,
>>>> -				changeset.bytes_changed);
>>>> +				changeset.bytes_changed, BTRFS_QGROUP_RSV_DATA);
>>>>  
>>>>  	}
>>>>  	extent_changeset_release(&changeset);
>>>> diff --git a/fs/btrfs/qgroup.h b/fs/btrfs/qgroup.h
>>>> index d9984e87cddf..0b04cbc5b5ce 100644
>>>> --- a/fs/btrfs/qgroup.h
>>>> +++ b/fs/btrfs/qgroup.h
>>>> @@ -61,6 +61,26 @@ struct btrfs_qgroup_extent_record {
>>>>  	struct ulist *old_roots;
>>>>  };
>>>>  
>>>> +enum btrfs_qgroup_rsv_type {
>>>> +	BTRFS_QGROUP_RSV_DATA = 0,
>>>> +	BTRFS_QGROUP_RSV_META = 1,
>>>> +	BTRFS_QGROUP_RSV_TYPES = 1,
>>>
>>> nit: Why not BTRFS_QGROUP_RSV_TYPES_MAX = 2;
>>
>> My original plan is just as the same as yours.
>>
>> However I still remember I did it before and David fixed it by using
>> TYPES, so I follow his naming schema here.
> 
> I don't really care about whether it's called TYPES or _MAX. The more
> important thing is the fact that we have types set to the same value as
> the last valid value and thus when writing loops or checking ranges we
> need to use <= and + 1 respectively which IMO is a bit
> counter-intuitive. But as I said it's a nit so I don't have strong
> preference either way if this is the convention of btrfs so be it.
>>
>> Kernel is also using this naming schema else where:
>> d91876496bcf ("btrfs: compress: put variables defined per compress type
>> in struct to make cache friendly")
>>
>>>
>>>> +};
>>>> +
>>>> +/*
>>>> + * Represents how many bytes we reserved for this qgroup.
>>>> + *
>>>> + * Each type should have different reservation behavior.
>>>> + * E.g, data follows its io_tree flag modification, while
>>>> + * *currently* meta is just reserve-and-clear during transcation.
>>>> + *
>>>> + * TODO: Add new type for delalloc, which can exist across several
>>>> + * transaction.
>>>> + */
>>>> +struct btrfs_qgroup_rsv {
>>>> +	u64 values[BTRFS_QGROUP_RSV_TYPES + 1];
>>>
>>> nit: And here just BTRFS_QGROUP_RSV_TYPES_MAX rather than the +1 here,
>>> seems more idiomatic to me.
>>
>> To follow same naming schema from David.
>> (IIRC it was about tree-checker patchset, checking file extent type part)
>>
>> In fact, I crashed kernel several times due to the tiny +1, without even
>> a clue for hours just testing blindly, until latest gcc gives warning
>> about it.
> 
> So you crashed the kernel because you were missing it or not ?

Crashed it just because missing the +1.

So I'd better go the _LAST solution.

Thanks,
Qu
> 
>>
>> Thanks,
>> Qu
>>
>>>
>>>> +};
>>>> +
>>>>  /*
>>>>   * one struct for each qgroup, organized in fs_info->qgroup_tree.
>>>>   */
>>>> @@ -88,6 +108,7 @@ struct btrfs_qgroup {
>>>>  	 * reservation tracking
>>>>  	 */
>>>>  	u64 reserved;
>>>> +	struct btrfs_qgroup_rsv rsv;
>>>>  
>>>>  	/*
>>>>  	 * lists
>>>> @@ -228,12 +249,14 @@ int btrfs_qgroup_inherit(struct btrfs_trans_handle *trans,
>>>>  			 struct btrfs_fs_info *fs_info, u64 srcid, u64 objectid,
>>>>  			 struct btrfs_qgroup_inherit *inherit);
>>>>  void btrfs_qgroup_free_refroot(struct btrfs_fs_info *fs_info,
>>>> -			       u64 ref_root, u64 num_bytes);
>>>> +			       u64 ref_root, u64 num_bytes,
>>>> +			       enum btrfs_qgroup_rsv_type type);
>>>>  static inline void btrfs_qgroup_free_delayed_ref(struct btrfs_fs_info *fs_info,
>>>>  						 u64 ref_root, u64 num_bytes)
>>>>  {
>>>>  	trace_btrfs_qgroup_free_delayed_ref(fs_info, ref_root, num_bytes);
>>>> -	btrfs_qgroup_free_refroot(fs_info, ref_root, num_bytes);
>>>> +	btrfs_qgroup_free_refroot(fs_info, ref_root, num_bytes,
>>>> +				  BTRFS_QGROUP_RSV_DATA);
>>>>  }
>>>>  
>>>>  #ifdef CONFIG_BTRFS_FS_RUN_SANITY_TESTS
>>>>
>>> --
>>> 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
>>>
>>


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 520 bytes --]

Re: [PATCH 1/6] btrfs: qgroup: Skeleton to support separate qgroup reservation type

[Jeff Mahoney]

[-- Attachment #1.1: Type: text/plain, Size: 1628 bytes --]

On 10/24/17 8:29 AM, Jeff Mahoney wrote:
> On 10/24/17 7:51 AM, Qu Wenruo wrote:
>>
>>
>> On 2017年10月24日 19:00, Nikolay Borisov wrote:
>>> nit: Why not BTRFS_QGROUP_RSV_TYPES_MAX = 2;
>>
>> My original plan is just as the same as yours.
>>
>> However I still remember I did it before and David fixed it by using
>> TYPES, so I follow his naming schema here.
>>
>> Kernel is also using this naming schema else where:
>> d91876496bcf ("btrfs: compress: put variables defined per compress type
>> in struct to make cache friendly")
> 
> The COMPRESS_TYPES pattern isn't the right pattern to follow here.
> That's a special case since there's a _NONE that doesn't have anything
> associated with it, so we don't need to take a slot in the array.
> 
> We also don't care about any of the specific values, just that they
> start at 0.  The BTRFS_COMPRESS_TYPES example also has a
> BTRFS_COMPRESS_LAST item in the enum, which serves the same purpose as
> MAX.  I don't have a strong opinion on the naming, just that we don't
> play games with +1 when handling arrays since, as you say, that's just
> waiting for subtle bugs later.
> 
> enum btrfs_qgroup_rsv_type {
> 	BTRFS_QGROUP_RSV_DATA = 0,
> 	BTRFS_QGROUP_RSV_META,
> 	BTRFS_QGROUP_RSV_LAST,
> };

It turns out that BTRFS_COMPRESS_LAST did exist until very recently:

commit dc2f29212a2648164b054016dc5b948bf0fc92d5
Author: Anand Jain <anand.jain@oracle.com>
Date:   Sun Aug 13 12:02:41 2017 +0800

    btrfs: remove unused BTRFS_COMPRESS_LAST

... so it was there, but just wasn't used.

-Jeff

-- 
Jeff Mahoney
SUSE Labs


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 1/6] btrfs: qgroup: Skeleton to support separate qgroup reservation type

[Qu Wenruo]

[-- Attachment #1.1: Type: text/plain, Size: 8102 bytes --]



On 2017年10月24日 20:29, Jeff Mahoney wrote:
> On 10/24/17 7:51 AM, Qu Wenruo wrote:
>>
>>
>> On 2017年10月24日 19:00, Nikolay Borisov wrote:
>>>
>>>
>>> On 24.10.2017 11:39, Qu Wenruo wrote:
>>>> Instead of single qgroup->reserved, use a new structure btrfs_qgroup_rsv
>>>> to restore different types of reservation.
>>>>
>>>> This patch only updates the header and needed modification to pass
>>>> compile.
>>>>
>>>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>>>> ---
>>>>  fs/btrfs/qgroup.c | 16 ++++++++++------
>>>>  fs/btrfs/qgroup.h | 27 +++++++++++++++++++++++++--
>>>>  2 files changed, 35 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
>>>> index e172d4843eae..fe3adb996883 100644
>>>> --- a/fs/btrfs/qgroup.c
>>>> +++ b/fs/btrfs/qgroup.c
>>>> @@ -2444,7 +2444,8 @@ static int qgroup_reserve(struct btrfs_root *root, u64 num_bytes, bool enforce)
>>>>  }
>>>>  
>>>>  void btrfs_qgroup_free_refroot(struct btrfs_fs_info *fs_info,
>>>> -			       u64 ref_root, u64 num_bytes)
>>>> +			       u64 ref_root, u64 num_bytes,
>>>> +			       enum btrfs_qgroup_rsv_type type)
>>>>  {
>>>>  	struct btrfs_root *quota_root;
>>>>  	struct btrfs_qgroup *qgroup;
>>>> @@ -2936,7 +2937,8 @@ static int qgroup_free_reserved_data(struct inode *inode,
>>>>  			goto out;
>>>>  		freed += changeset.bytes_changed;
>>>>  	}
>>>> -	btrfs_qgroup_free_refroot(root->fs_info, root->objectid, freed);
>>>> +	btrfs_qgroup_free_refroot(root->fs_info, root->objectid, freed,
>>>> +				  BTRFS_QGROUP_RSV_DATA);
>>>>  	ret = freed;
>>>>  out:
>>>>  	extent_changeset_release(&changeset);
>>>> @@ -2968,7 +2970,7 @@ static int __btrfs_qgroup_release_data(struct inode *inode,
>>>>  	if (free)
>>>>  		btrfs_qgroup_free_refroot(BTRFS_I(inode)->root->fs_info,
>>>>  				BTRFS_I(inode)->root->objectid,
>>>> -				changeset.bytes_changed);
>>>> +				changeset.bytes_changed, BTRFS_QGROUP_RSV_DATA);
>>>>  	ret = changeset.bytes_changed;
>>>>  out:
>>>>  	extent_changeset_release(&changeset);
>>>> @@ -3045,7 +3047,8 @@ void btrfs_qgroup_free_meta_all(struct btrfs_root *root)
>>>>  	if (reserved == 0)
>>>>  		return;
>>>>  	trace_qgroup_meta_reserve(root, -(s64)reserved);
>>>> -	btrfs_qgroup_free_refroot(fs_info, root->objectid, reserved);
>>>> +	btrfs_qgroup_free_refroot(fs_info, root->objectid, reserved,
>>>> +				  BTRFS_QGROUP_RSV_META);
>>>>  }
>>>>  
>>>>  void btrfs_qgroup_free_meta(struct btrfs_root *root, int num_bytes)
>>>> @@ -3060,7 +3063,8 @@ void btrfs_qgroup_free_meta(struct btrfs_root *root, int num_bytes)
>>>>  	WARN_ON(atomic64_read(&root->qgroup_meta_rsv) < num_bytes);
>>>>  	atomic64_sub(num_bytes, &root->qgroup_meta_rsv);
>>>>  	trace_qgroup_meta_reserve(root, -(s64)num_bytes);
>>>> -	btrfs_qgroup_free_refroot(fs_info, root->objectid, num_bytes);
>>>> +	btrfs_qgroup_free_refroot(fs_info, root->objectid, num_bytes,
>>>> +				  BTRFS_QGROUP_RSV_META);
>>>>  }
>>>>  
>>>>  /*
>>>> @@ -3088,7 +3092,7 @@ void btrfs_qgroup_check_reserved_leak(struct inode *inode)
>>>>  		}
>>>>  		btrfs_qgroup_free_refroot(BTRFS_I(inode)->root->fs_info,
>>>>  				BTRFS_I(inode)->root->objectid,
>>>> -				changeset.bytes_changed);
>>>> +				changeset.bytes_changed, BTRFS_QGROUP_RSV_DATA);
>>>>  
>>>>  	}
>>>>  	extent_changeset_release(&changeset);
>>>> diff --git a/fs/btrfs/qgroup.h b/fs/btrfs/qgroup.h
>>>> index d9984e87cddf..0b04cbc5b5ce 100644
>>>> --- a/fs/btrfs/qgroup.h
>>>> +++ b/fs/btrfs/qgroup.h
>>>> @@ -61,6 +61,26 @@ struct btrfs_qgroup_extent_record {
>>>>  	struct ulist *old_roots;
>>>>  };
>>>>  
>>>> +enum btrfs_qgroup_rsv_type {
>>>> +	BTRFS_QGROUP_RSV_DATA = 0,
>>>> +	BTRFS_QGROUP_RSV_META = 1,
>>>> +	BTRFS_QGROUP_RSV_TYPES = 1,
>>>
>>> nit: Why not BTRFS_QGROUP_RSV_TYPES_MAX = 2;
>>
>> My original plan is just as the same as yours.
>>
>> However I still remember I did it before and David fixed it by using
>> TYPES, so I follow his naming schema here.
>>
>> Kernel is also using this naming schema else where:
>> d91876496bcf ("btrfs: compress: put variables defined per compress type
>> in struct to make cache friendly")
> 
> The COMPRESS_TYPES pattern isn't the right pattern to follow here.
> That's a special case since there's a _NONE that doesn't have anything
> associated with it, so we don't need to take a slot in the array.
> 
> We also don't care about any of the specific values, just that they
> start at 0.  The BTRFS_COMPRESS_TYPES example also has a
> BTRFS_COMPRESS_LAST item in the enum, which serves the same purpose as
> MAX.

At least in latest mainline kernel, there is no COMPRESS_LAST now.
---
enum btrfs_compression_type {
	BTRFS_COMPRESS_NONE  = 0,
	BTRFS_COMPRESS_ZLIB  = 1,
	BTRFS_COMPRESS_LZO   = 2,
	BTRFS_COMPRESS_ZSTD  = 3,
	BTRFS_COMPRESS_TYPES = 3,
};
---

>  I don't have a strong opinion on the naming, just that we don't
> play games with +1 when handling arrays since, as you say, that's just
> waiting for subtle bugs later.

Not waiting, already experienced it during coding.

> 
> enum btrfs_qgroup_rsv_type {
> 	BTRFS_QGROUP_RSV_DATA = 0,
> 	BTRFS_QGROUP_RSV_META,
> 	BTRFS_QGROUP_RSV_LAST,
> };
> 
>>>> +};
>>>> +
>>>> +/*
>>>> + * Represents how many bytes we reserved for this qgroup.
>>>> + *
>>>> + * Each type should have different reservation behavior.
>>>> + * E.g, data follows its io_tree flag modification, while
>>>> + * *currently* meta is just reserve-and-clear during transcation.
>>>> + *
>>>> + * TODO: Add new type for delalloc, which can exist across several
>>>> + * transaction.
>>>> + */
> 
> Minor nit: It's not just delalloc.  Delayed items and inodes can as
> well.  The general rule is that qgroup reservations aren't essentially
> different from block reservations and follow the same usage patterns
> when operating on leaf nodes.

Yep, any reserveration who can survive transaction commit should go here
with a new reservation type.

I'll change the TODO description here.

> 
>>>> +struct btrfs_qgroup_rsv {
>>>> +	u64 values[BTRFS_QGROUP_RSV_TYPES + 1];
>>>
>>> nit: And here just BTRFS_QGROUP_RSV_TYPES_MAX rather than the +1 here,
>>> seems more idiomatic to me.
>>
>> To follow same naming schema from David.
>> (IIRC it was about tree-checker patchset, checking file extent type part)
>>
>> In fact, I crashed kernel several times due to the tiny +1, without even
>> a clue for hours just testing blindly, until latest gcc gives warning
>> about it.
> 
> BTRFS_QGROUP_RSV_LAST would do the job here.

I'll go with this method.

Thanks,
Qu

> 
> -Jeff
> 
>>>
>>>> +};
>>>> +
>>>>  /*
>>>>   * one struct for each qgroup, organized in fs_info->qgroup_tree.
>>>>   */
>>>> @@ -88,6 +108,7 @@ struct btrfs_qgroup {
>>>>  	 * reservation tracking
>>>>  	 */
>>>>  	u64 reserved;
>>>> +	struct btrfs_qgroup_rsv rsv;
>>>>  
>>>>  	/*
>>>>  	 * lists
>>>> @@ -228,12 +249,14 @@ int btrfs_qgroup_inherit(struct btrfs_trans_handle *trans,
>>>>  			 struct btrfs_fs_info *fs_info, u64 srcid, u64 objectid,
>>>>  			 struct btrfs_qgroup_inherit *inherit);
>>>>  void btrfs_qgroup_free_refroot(struct btrfs_fs_info *fs_info,
>>>> -			       u64 ref_root, u64 num_bytes);
>>>> +			       u64 ref_root, u64 num_bytes,
>>>> +			       enum btrfs_qgroup_rsv_type type);
>>>>  static inline void btrfs_qgroup_free_delayed_ref(struct btrfs_fs_info *fs_info,
>>>>  						 u64 ref_root, u64 num_bytes)
>>>>  {
>>>>  	trace_btrfs_qgroup_free_delayed_ref(fs_info, ref_root, num_bytes);
>>>> -	btrfs_qgroup_free_refroot(fs_info, ref_root, num_bytes);
>>>> +	btrfs_qgroup_free_refroot(fs_info, ref_root, num_bytes,
>>>> +				  BTRFS_QGROUP_RSV_DATA);
>>>>  }
>>>>  
>>>>  #ifdef CONFIG_BTRFS_FS_RUN_SANITY_TESTS
>>>>
>>> --
>>> 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
>>>
>>
> 
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 520 bytes --]

Re: [PATCH 2/6] btrfs: qgroup: Introduce helpers to update and access new qgroup rsv

[Edmund Nadolski]

On 10/24/2017 06:05 AM, Qu Wenruo wrote:
> 
> 
> On 2017年10月24日 19:07, Nikolay Borisov wrote:
>>
>>
>> On 24.10.2017 11:39, Qu Wenruo wrote:
>>> Introduce helpers to:
>>>
>>> 1) Get total reserved space
>>>    For limit calculation
>>>
>>> 2) Increase reserved space for given type
>>> 2) Decrease reserved space for given type
>>>
>>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>>> ---
>>>  fs/btrfs/qgroup.c | 66 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>  1 file changed, 66 insertions(+)
>>>
>>> diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
>>> index fe3adb996883..04fd145516ad 100644
>>> --- a/fs/btrfs/qgroup.c
>>> +++ b/fs/btrfs/qgroup.c
>>> @@ -47,6 +47,72 @@
>>>   *  - check all ioctl parameters
>>>   */
>>>  
>>> +/*
>>> + * Helpers to access qgroup reservation
>>> + *
>>> + * Callers should ensure the lock context and type are valid
>>> + */
>>> +
>>> +static u64 qgroup_rsv_total(const struct btrfs_qgroup *qgroup)
>>> +{
>>> +	u64 ret = 0;
>>> +	int i;
>>> +
>>> +	for (i = 0; i <= BTRFS_QGROUP_RSV_TYPES; i++)
>>
>> So if you actually take up on my idea of having an explicit value which
>> denotes the end, the loop here would be just < *_END rather than the <=
>> which instantly looks suspicious of an off-by-one error.
> 
> Yeah, I really like to do it, as mentioned in previous mail.
> 
> But to follow the schema used elsewhere, I had no choice.
> 
>>
>>> +		ret += qgroup->rsv.values[i];
>>> +
>>> +	return ret;
>>> +}
>>> +
>>> +static const char *qgroup_rsv_type_str(enum btrfs_qgroup_rsv_type type)
>>> +{
>>> +	if (type == BTRFS_QGROUP_RSV_DATA)
>>> +		return "data";
>>> +	if (type == BTRFS_QGROUP_RSV_META)
>>> +		return "meta";
>>> +	return NULL;
>>> +}
>>> +
>>> +static void qgroup_rsv_increase(struct btrfs_qgroup *qgroup, u64 num_bytes,
>>> +				enum btrfs_qgroup_rsv_type type)
>>> +{
>>> +	qgroup->rsv.values[type] += num_bytes;
>>> +}
>>> +
>>> +static void qgroup_rsv_decrease(struct btrfs_qgroup *qgroup, u64 num_bytes,
>>> +				enum btrfs_qgroup_rsv_type type)
>>> +{
>>> +	if (qgroup->rsv.values[type] >= num_bytes) {
>>> +		qgroup->rsv.values[type] -= num_bytes;
>>> +		return;
>>> +	}
>>> +#ifdef CONFIG_BTRFS_DEBUG
>>> +	WARN_RATELIMIT(1,
>>> +		"qgroup %llu %s reserved space underflow, have %llu to free %llu",
>>> +		qgroup->qgroupid, qgroup_rsv_type_str(type),
>>> +		qgroup->rsv.values[type], num_bytes);
>>> +#endif
>>> +	qgroup->rsv.values[type] = 0;
>>> +}
>>
>>
>> Perhaps these could be modelled after
>> block_rsv_use_bytes/block_rsv_add_bytes ? I'm not entirely sure of what
>> increasing the value actually does - reserving bytes or using already
>> reserved bytes, I guess it should be reserving. In this case what about
>> qgroup_bytes_reserve/qgroup_bytes_(free|unreserve) ?
> 
> I'm really bad at naming functions.
> My original idea is qgroup_rsv_add() and qgroup_rsv_rename(), but
> "rename" is 3 characters longer than "add", which makes me quite
> uncomfortable.
> (Maybe 'add' and 'sub' better?)
> 
> For the "reserve|free", it can be easily confused with
> btrfs_qgroup_reserve|release_data().
> 
> But in fact, this qgroup_rsv_* functions are only used to maintain
> qgroup->rsv, so it's less meaningful compared to other functions, like
> btrfs_qgroup_reserve|release_data().
> 
> The value itself represents how many bytes it has already reserved for
> later operation.
> So qgroup_rsv_increase() normally means qgroup is reserving more space,
> while qgroup_rsv_decrease() means qgroup reserved space is decreasing,
> either released or freed.
> 
> Hopes above explanation could inspire you about better naming ideas.
> (Because I really have no idea how to name it better while keeping the
> name the same length)

I'm not sure I understand the concern about the length.  These names do
not seem excessive to me, so a few extra characters for the sake of
clarity would be well worth it.

Ed

>>> +
>>> +static void qgroup_rsv_increase_by_qgroup(struct btrfs_qgroup *dest,
>>> +					  struct btrfs_qgroup *src)
>>> +{
>>> +	int i;
>>> +
>>> +	for (i = 0; i <= BTRFS_QGROUP_RSV_TYPES; i++)
>>> +		qgroup_rsv_increase(dest, src->rsv.values[i], i);
>>> +}
>>> +
>>> +static void qgroup_rsv_decrease_by_qgroup(struct btrfs_qgroup *dest,
>>> +					  struct btrfs_qgroup *src)
>>> +{
>>> +	int i;
>>> +
>>> +	for (i = 0; i <= BTRFS_QGROUP_RSV_TYPES; i++)
>>> +		qgroup_rsv_decrease(dest, src->rsv.values[i], i);
>>> +}
>>
>> Why don't you model these similar to what we already have for the block
>> rsv. In this case I believe those would correspond to the
>> btrfs_block_rsv_migrate. Admittedly we don't have a counterpart to
>> rsv_decrease_by_qgroup.
> 
> Not sure about 'migrate', I think it's more like 'inherit', since @src
> is not modified at all.
> 
> 'increase' and 'decrease' are preferred mainly because they are the same
> length, and represents the value change obviously enough.
> 
> I'm completely open to better naming schema.
> (But it's better to have same length, I'm kind of paranoid about this)
> 
> Thanks,
> Qu
> 
>>
>>
>>> +
>>>  static void btrfs_qgroup_update_old_refcnt(struct btrfs_qgroup *qg, u64 seq,
>>>  					   int mod)
>>>  {
>>>
>> --
>> 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
>>
> 

Re: [PATCH 4/6] btrfs: qgroup: Fix wrong qgroup reservation inheritance for relationship update

[Edmund Nadolski]

On 10/24/2017 02:39 AM, Qu Wenruo wrote:
> When modifying qgroup relationship, for qgroup which only owns exclusive
> extents, we will go through quick update path.
> 
> In quick update path, we will just adding/removing exclusive and reference
> number.
> 
> However we did the opposite for qgroup reservation from the very
> beginning.
> 
> In fact, we should also inherit the qgroup reservation space, just like
> exclusive and reference numbers.
> 
> Fix by using the newly introduced
> qgroup_rsv_increase/decrease_by_qgroup() function call.
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>  fs/btrfs/qgroup.c | 39 ++++++++++++++++++---------------------
>  1 file changed, 18 insertions(+), 21 deletions(-)
> 
> diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
> index 7b89da9589c1..ba6f60fd0e96 100644
> --- a/fs/btrfs/qgroup.c
> +++ b/fs/btrfs/qgroup.c
> @@ -1069,21 +1069,24 @@ static void report_reserved_underflow(struct btrfs_fs_info *fs_info,
>  #endif
>  	qgroup->reserved = 0;
>  }
> +
>  /*
> - * The easy accounting, if we are adding/removing the only ref for an extent
> - * then this qgroup and all of the parent qgroups get their reference and
> - * exclusive counts adjusted.
> + * The easy accounting, we're updating qgroup relationship whose child qgroup
> + * only have exclusive extents.
> + * In this case, we only need to update the rfer/excl, and inherit rsv from
> + * child qgroup (@src)

I'm not sure I understand this inheritance model.  Typically a child
will inherit from one (or more) parents, but this seems to go the
opposite way.  (Perhaps it's just terminology, but I may have missed
something here.)

Can these relationships form a hierarchy (i.e., grandparents,
grandchildren, etc.)?

Thanks,
Ed


>   *
>   * Caller should hold fs_info->qgroup_lock.
>   */
>  static int __qgroup_excl_accounting(struct btrfs_fs_info *fs_info,
>  				    struct ulist *tmp, u64 ref_root,
> -				    u64 num_bytes, int sign)
> +				    struct btrfs_qgroup *src, int sign)
>  {
>  	struct btrfs_qgroup *qgroup;
>  	struct btrfs_qgroup_list *glist;
>  	struct ulist_node *unode;
>  	struct ulist_iterator uiter;
> +	u64 num_bytes = src->excl;
>  	int ret = 0;
>  
>  	qgroup = find_qgroup_rb(fs_info, ref_root);
> @@ -1096,13 +1099,12 @@ static int __qgroup_excl_accounting(struct btrfs_fs_info *fs_info,
>  	WARN_ON(sign < 0 && qgroup->excl < num_bytes);
>  	qgroup->excl += sign * num_bytes;
>  	qgroup->excl_cmpr += sign * num_bytes;
> -	if (sign > 0) {
> -		trace_qgroup_update_reserve(fs_info, qgroup, -(s64)num_bytes);
> -		if (qgroup->reserved < num_bytes)
> -			report_reserved_underflow(fs_info, qgroup, num_bytes);
> -		else
> -			qgroup->reserved -= num_bytes;
> -	}
> +
> +	/* *Inherit* qgroup rsv info from @src */
> +	if (sign > 0)
> +		qgroup_rsv_increase_by_qgroup(qgroup, src);
> +	else
> +		qgroup_rsv_decrease_by_qgroup(qgroup, src);
>  
>  	qgroup_dirty(fs_info, qgroup);
>  
> @@ -1122,15 +1124,10 @@ static int __qgroup_excl_accounting(struct btrfs_fs_info *fs_info,
>  		qgroup->rfer_cmpr += sign * num_bytes;
>  		WARN_ON(sign < 0 && qgroup->excl < num_bytes);
>  		qgroup->excl += sign * num_bytes;
> -		if (sign > 0) {
> -			trace_qgroup_update_reserve(fs_info, qgroup,
> -						    -(s64)num_bytes);
> -			if (qgroup->reserved < num_bytes)
> -				report_reserved_underflow(fs_info, qgroup,
> -							  num_bytes);
> -			else
> -				qgroup->reserved -= num_bytes;
> -		}
> +		if (sign > 0)
> +			qgroup_rsv_increase_by_qgroup(qgroup, src);
> +		else
> +			qgroup_rsv_decrease_by_qgroup(qgroup, src);
>  		qgroup->excl_cmpr += sign * num_bytes;
>  		qgroup_dirty(fs_info, qgroup);
>  
> @@ -1173,7 +1170,7 @@ static int quick_update_accounting(struct btrfs_fs_info *fs_info,
>  	if (qgroup->excl == qgroup->rfer) {
>  		ret = 0;
>  		err = __qgroup_excl_accounting(fs_info, tmp, dst,
> -					       qgroup->excl, sign);
> +					       qgroup, sign);
>  		if (err < 0) {
>  			ret = err;
>  			goto out;
> 

Re: [PATCH 4/6] btrfs: qgroup: Fix wrong qgroup reservation inheritance for relationship update

[Qu Wenruo]

[-- Attachment #1.1: Type: text/plain, Size: 4654 bytes --]



On 2017年10月25日 01:11, Edmund Nadolski wrote:
> 
> 
> On 10/24/2017 02:39 AM, Qu Wenruo wrote:
>> When modifying qgroup relationship, for qgroup which only owns exclusive
>> extents, we will go through quick update path.
>>
>> In quick update path, we will just adding/removing exclusive and reference
>> number.
>>
>> However we did the opposite for qgroup reservation from the very
>> beginning.
>>
>> In fact, we should also inherit the qgroup reservation space, just like
>> exclusive and reference numbers.
>>
>> Fix by using the newly introduced
>> qgroup_rsv_increase/decrease_by_qgroup() function call.
>>
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>> ---
>>  fs/btrfs/qgroup.c | 39 ++++++++++++++++++---------------------
>>  1 file changed, 18 insertions(+), 21 deletions(-)
>>
>> diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
>> index 7b89da9589c1..ba6f60fd0e96 100644
>> --- a/fs/btrfs/qgroup.c
>> +++ b/fs/btrfs/qgroup.c
>> @@ -1069,21 +1069,24 @@ static void report_reserved_underflow(struct btrfs_fs_info *fs_info,
>>  #endif
>>  	qgroup->reserved = 0;
>>  }
>> +
>>  /*
>> - * The easy accounting, if we are adding/removing the only ref for an extent
>> - * then this qgroup and all of the parent qgroups get their reference and
>> - * exclusive counts adjusted.
>> + * The easy accounting, we're updating qgroup relationship whose child qgroup
>> + * only have exclusive extents.
>> + * In this case, we only need to update the rfer/excl, and inherit rsv from
>> + * child qgroup (@src)
> 
> I'm not sure I understand this inheritance model.  Typically a child
> will inherit from one (or more) parents, but this seems to go the
> opposite way.  (Perhaps it's just terminology, but I may have missed
> something here.)

Btrfs qgroup, for exclusive qgroup, it's more like a subset.

If a exclusive qgroup belongs to a parent qgroup, then parent qgroup
should have all the exclusive extents and its reservation.

So it's still like inheritance, but it's parent inheriting things from
child.
Yeah, it's opposite, and I don't have better idea to explain this.

> 
> Can these relationships form a hierarchy (i.e., grandparents,
> grandchildren, etc.)?

Yes, it's possible.

Thanks,
Qu

> 
> Thanks,
> Ed
> 
> 
>>   *
>>   * Caller should hold fs_info->qgroup_lock.
>>   */
>>  static int __qgroup_excl_accounting(struct btrfs_fs_info *fs_info,
>>  				    struct ulist *tmp, u64 ref_root,
>> -				    u64 num_bytes, int sign)
>> +				    struct btrfs_qgroup *src, int sign)
>>  {
>>  	struct btrfs_qgroup *qgroup;
>>  	struct btrfs_qgroup_list *glist;
>>  	struct ulist_node *unode;
>>  	struct ulist_iterator uiter;
>> +	u64 num_bytes = src->excl;
>>  	int ret = 0;
>>  
>>  	qgroup = find_qgroup_rb(fs_info, ref_root);
>> @@ -1096,13 +1099,12 @@ static int __qgroup_excl_accounting(struct btrfs_fs_info *fs_info,
>>  	WARN_ON(sign < 0 && qgroup->excl < num_bytes);
>>  	qgroup->excl += sign * num_bytes;
>>  	qgroup->excl_cmpr += sign * num_bytes;
>> -	if (sign > 0) {
>> -		trace_qgroup_update_reserve(fs_info, qgroup, -(s64)num_bytes);
>> -		if (qgroup->reserved < num_bytes)
>> -			report_reserved_underflow(fs_info, qgroup, num_bytes);
>> -		else
>> -			qgroup->reserved -= num_bytes;
>> -	}
>> +
>> +	/* *Inherit* qgroup rsv info from @src */
>> +	if (sign > 0)
>> +		qgroup_rsv_increase_by_qgroup(qgroup, src);
>> +	else
>> +		qgroup_rsv_decrease_by_qgroup(qgroup, src);
>>  
>>  	qgroup_dirty(fs_info, qgroup);
>>  
>> @@ -1122,15 +1124,10 @@ static int __qgroup_excl_accounting(struct btrfs_fs_info *fs_info,
>>  		qgroup->rfer_cmpr += sign * num_bytes;
>>  		WARN_ON(sign < 0 && qgroup->excl < num_bytes);
>>  		qgroup->excl += sign * num_bytes;
>> -		if (sign > 0) {
>> -			trace_qgroup_update_reserve(fs_info, qgroup,
>> -						    -(s64)num_bytes);
>> -			if (qgroup->reserved < num_bytes)
>> -				report_reserved_underflow(fs_info, qgroup,
>> -							  num_bytes);
>> -			else
>> -				qgroup->reserved -= num_bytes;
>> -		}
>> +		if (sign > 0)
>> +			qgroup_rsv_increase_by_qgroup(qgroup, src);
>> +		else
>> +			qgroup_rsv_decrease_by_qgroup(qgroup, src);
>>  		qgroup->excl_cmpr += sign * num_bytes;
>>  		qgroup_dirty(fs_info, qgroup);
>>  
>> @@ -1173,7 +1170,7 @@ static int quick_update_accounting(struct btrfs_fs_info *fs_info,
>>  	if (qgroup->excl == qgroup->rfer) {
>>  		ret = 0;
>>  		err = __qgroup_excl_accounting(fs_info, tmp, dst,
>> -					       qgroup->excl, sign);
>> +					       qgroup, sign);
>>  		if (err < 0) {
>>  			ret = err;
>>  			goto out;
>>


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 504 bytes --]

Re: [PATCH 2/6] btrfs: qgroup: Introduce helpers to update and access new qgroup rsv

[Qu Wenruo]

[-- Attachment #1.1: Type: text/plain, Size: 6317 bytes --]



On 2017年10月25日 00:22, Edmund Nadolski wrote:
> 
> 
> On 10/24/2017 06:05 AM, Qu Wenruo wrote:
>>
>>
>> On 2017年10月24日 19:07, Nikolay Borisov wrote:
>>>
>>>
>>> On 24.10.2017 11:39, Qu Wenruo wrote:
>>>> Introduce helpers to:
>>>>
>>>> 1) Get total reserved space
>>>>    For limit calculation
>>>>
>>>> 2) Increase reserved space for given type
>>>> 2) Decrease reserved space for given type
>>>>
>>>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>>>> ---
>>>>  fs/btrfs/qgroup.c | 66 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>  1 file changed, 66 insertions(+)
>>>>
>>>> diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
>>>> index fe3adb996883..04fd145516ad 100644
>>>> --- a/fs/btrfs/qgroup.c
>>>> +++ b/fs/btrfs/qgroup.c
>>>> @@ -47,6 +47,72 @@
>>>>   *  - check all ioctl parameters
>>>>   */
>>>>  
>>>> +/*
>>>> + * Helpers to access qgroup reservation
>>>> + *
>>>> + * Callers should ensure the lock context and type are valid
>>>> + */
>>>> +
>>>> +static u64 qgroup_rsv_total(const struct btrfs_qgroup *qgroup)
>>>> +{
>>>> +	u64 ret = 0;
>>>> +	int i;
>>>> +
>>>> +	for (i = 0; i <= BTRFS_QGROUP_RSV_TYPES; i++)
>>>
>>> So if you actually take up on my idea of having an explicit value which
>>> denotes the end, the loop here would be just < *_END rather than the <=
>>> which instantly looks suspicious of an off-by-one error.
>>
>> Yeah, I really like to do it, as mentioned in previous mail.
>>
>> But to follow the schema used elsewhere, I had no choice.
>>
>>>
>>>> +		ret += qgroup->rsv.values[i];
>>>> +
>>>> +	return ret;
>>>> +}
>>>> +
>>>> +static const char *qgroup_rsv_type_str(enum btrfs_qgroup_rsv_type type)
>>>> +{
>>>> +	if (type == BTRFS_QGROUP_RSV_DATA)
>>>> +		return "data";
>>>> +	if (type == BTRFS_QGROUP_RSV_META)
>>>> +		return "meta";
>>>> +	return NULL;
>>>> +}
>>>> +
>>>> +static void qgroup_rsv_increase(struct btrfs_qgroup *qgroup, u64 num_bytes,
>>>> +				enum btrfs_qgroup_rsv_type type)
>>>> +{
>>>> +	qgroup->rsv.values[type] += num_bytes;
>>>> +}
>>>> +
>>>> +static void qgroup_rsv_decrease(struct btrfs_qgroup *qgroup, u64 num_bytes,
>>>> +				enum btrfs_qgroup_rsv_type type)
>>>> +{
>>>> +	if (qgroup->rsv.values[type] >= num_bytes) {
>>>> +		qgroup->rsv.values[type] -= num_bytes;
>>>> +		return;
>>>> +	}
>>>> +#ifdef CONFIG_BTRFS_DEBUG
>>>> +	WARN_RATELIMIT(1,
>>>> +		"qgroup %llu %s reserved space underflow, have %llu to free %llu",
>>>> +		qgroup->qgroupid, qgroup_rsv_type_str(type),
>>>> +		qgroup->rsv.values[type], num_bytes);
>>>> +#endif
>>>> +	qgroup->rsv.values[type] = 0;
>>>> +}
>>>
>>>
>>> Perhaps these could be modelled after
>>> block_rsv_use_bytes/block_rsv_add_bytes ? I'm not entirely sure of what
>>> increasing the value actually does - reserving bytes or using already
>>> reserved bytes, I guess it should be reserving. In this case what about
>>> qgroup_bytes_reserve/qgroup_bytes_(free|unreserve) ?
>>
>> I'm really bad at naming functions.
>> My original idea is qgroup_rsv_add() and qgroup_rsv_rename(), but
>> "rename" is 3 characters longer than "add", which makes me quite
>> uncomfortable.
>> (Maybe 'add' and 'sub' better?)
>>
>> For the "reserve|free", it can be easily confused with
>> btrfs_qgroup_reserve|release_data().
>>
>> But in fact, this qgroup_rsv_* functions are only used to maintain
>> qgroup->rsv, so it's less meaningful compared to other functions, like
>> btrfs_qgroup_reserve|release_data().
>>
>> The value itself represents how many bytes it has already reserved for
>> later operation.
>> So qgroup_rsv_increase() normally means qgroup is reserving more space,
>> while qgroup_rsv_decrease() means qgroup reserved space is decreasing,
>> either released or freed.
>>
>> Hopes above explanation could inspire you about better naming ideas.
>> (Because I really have no idea how to name it better while keeping the
>> name the same length)
> 
> I'm not sure I understand the concern about the length.  These names do
> not seem excessive to me, so a few extra characters for the sake of
> clarity would be well worth it.

What about qgroup_rsv_add/remove()?

These functions are just designed to increase/decrease the value(s) of
btrfs_qgroup_rsv structure, and should not be accessed outside of
qgroup_reserve() and some special call sides.

Maybe __qgroup_rsv_add/remove() will be better considering it's only
used internally.

Thanks,
Qu

> 
> Ed
> 
>>>> +
>>>> +static void qgroup_rsv_increase_by_qgroup(struct btrfs_qgroup *dest,
>>>> +					  struct btrfs_qgroup *src)
>>>> +{
>>>> +	int i;
>>>> +
>>>> +	for (i = 0; i <= BTRFS_QGROUP_RSV_TYPES; i++)
>>>> +		qgroup_rsv_increase(dest, src->rsv.values[i], i);
>>>> +}
>>>> +
>>>> +static void qgroup_rsv_decrease_by_qgroup(struct btrfs_qgroup *dest,
>>>> +					  struct btrfs_qgroup *src)
>>>> +{
>>>> +	int i;
>>>> +
>>>> +	for (i = 0; i <= BTRFS_QGROUP_RSV_TYPES; i++)
>>>> +		qgroup_rsv_decrease(dest, src->rsv.values[i], i);
>>>> +}
>>>
>>> Why don't you model these similar to what we already have for the block
>>> rsv. In this case I believe those would correspond to the
>>> btrfs_block_rsv_migrate. Admittedly we don't have a counterpart to
>>> rsv_decrease_by_qgroup.
>>
>> Not sure about 'migrate', I think it's more like 'inherit', since @src
>> is not modified at all.
>>
>> 'increase' and 'decrease' are preferred mainly because they are the same
>> length, and represents the value change obviously enough.
>>
>> I'm completely open to better naming schema.
>> (But it's better to have same length, I'm kind of paranoid about this)
>>
>> Thanks,
>> Qu
>>
>>>
>>>
>>>> +
>>>>  static void btrfs_qgroup_update_old_refcnt(struct btrfs_qgroup *qg, u64 seq,
>>>>  					   int mod)
>>>>  {
>>>>
>>> --
>>> 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
>>>
>>
> --
> 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
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 520 bytes --]