Skip to contentSkip to conversation

btrfs: qgroup: Separate qgroup reservation types

PATCH In review Qu Wenruo 3 participants 9 messages Oct 25, 2017 – Oct 26, 2017

Subsystems (2)
Btrfs file system Tracing
lore
Share & tools
lore

Qu Wenruo

author
#0 lore raw

v2 cover btrfs: qgroup: Separate qgroup reservation types

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.

Changelog:
v2:
Use _MAX naming, as _TYPES naming reusing the last type makes it quite easier to screw up space allocation, since we need +1 to handle _TYPES. Suggested by Nikolay, Edmund, Jeff and already experienced such problem by myself.

Rename the qgroup_rsv_increase/decrease() to qgroup_rsv_add/release(), to follow the block_rsv API, suggested by Nikolay, Jeff and Edmund.

Better description for the bug in __qgroup_excl_accounting(), suggested by Nikolay.

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 update for relationship
modification
btrfs: qgroup: Update trace events to use new separate rsv types
btrfs: qgroup: Cleanup the remaining old reservation counters

 fs/btrfs/qgroup.c            | 169 ++++++++++++++++++++++++++++++-------------
 fs/btrfs/qgroup.h            |  28 ++++++-
 include/trace/events/btrfs.h |  17 +++--
 3 files changed, 154 insertions(+), 60 deletions(-)
Signature · 1 line
2.14.2
author
#1 lore raw

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

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 <[email protected]>
---
 fs/btrfs/qgroup.c | 16 ++++++++++------
 fs/btrfs/qgroup.h | 27 +++++++++++++++++++++++++--
 2 files changed, 35 insertions(+), 8 deletions(-)
fs/btrfs/qgroup.c +10 -6
index e172d4843eae..fe3adb996883 100644
@@ -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);
fs/btrfs/qgroup.h +25 -2
index d9984e87cddf..fead9955af72 100644
@@ -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,
+	BTRFS_QGROUP_RSV_LAST,
+};
+
+/*
+ * Represents how many bytes we have 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 reservation which can survive transaction commit.
+ * Currect metadata reservation behavior is not suitable for such case.
+ */
+struct btrfs_qgroup_rsv {
+	u64 values[BTRFS_QGROUP_RSV_LAST];
+};
+
 /*
  * 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
Signature · 1 line
2.14.2
author
#2 lore raw

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

In reply to Qu Wenruo

Introduce helpers to:

1) Get total reserved space
For limit calculation
2) Add/release reserved space for given type With underflow detection and warning
3) Add/release reserved space according to child qgroup

Signed-off-by Qu Wenruo <[email protected]>
---
 fs/btrfs/qgroup.c | 68 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 68 insertions(+)
fs/btrfs/qgroup.c +68 -0
index fe3adb996883..161f16c40264 100644
@@ -47,6 +47,74 @@
  *  - 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_LAST; i++)
+		ret += qgroup->rsv.values[i];
+
+	return ret;
+}
+
+#ifdef CONFIG_BTRFS_DEBUG
+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;
+}
+#endif
+
+static void qgroup_rsv_add(struct btrfs_qgroup *qgroup, u64 num_bytes,
+			   enum btrfs_qgroup_rsv_type type)
+{
+	qgroup->rsv.values[type] += num_bytes;
+}
+
+static void qgroup_rsv_release(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_add_by_qgroup(struct btrfs_qgroup *dest,
+					  struct btrfs_qgroup *src)
+{
+	int i;
+
+	for (i = 0; i < BTRFS_QGROUP_RSV_LAST; i++)
+		qgroup_rsv_add(dest, src->rsv.values[i], i);
+}
+
+static void qgroup_rsv_release_by_qgroup(struct btrfs_qgroup *dest,
+					  struct btrfs_qgroup *src)
+{
+	int i;
+
+	for (i = 0; i < BTRFS_QGROUP_RSV_LAST; i++)
+		qgroup_rsv_release(dest, src->rsv.values[i], i);
+}
+
 static void btrfs_qgroup_update_old_refcnt(struct btrfs_qgroup *qg, u64 seq,
 					   int mod)
 {
Signature · 1 line
2.14.2
author
#3 lore raw

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

In reply to 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 <[email protected]>
---
 fs/btrfs/qgroup.c | 20 +++++++++-----------
 1 file changed, 9 insertions(+), 11 deletions(-)
fs/btrfs/qgroup.c +9 -11
index 161f16c40264..2c690aa19d84 100644
@@ -2401,17 +2401,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;

@@ -2463,7 +2464,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);

@@ -2503,7 +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);
-		qg->reserved += num_bytes;
+		qgroup_rsv_add(qg, num_bytes, type);
 	}
 
 out:

@@ -2550,10 +2551,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_release(qg, num_bytes, type);
 
 		list_for_each_entry(glist, &qg->groups, next_group) {
 			ret = ulist_add(fs_info->qgroup_ulist,

@@ -2941,7 +2939,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;
 

@@ -3095,7 +3093,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);
Signature · 1 line
2.14.2
author
#4 lore raw

PATCH v2 4/6 btrfs: qgroup: Fix wrong qgroup reservation update for relationship modification

In reply to 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 adding/substracting exclusive and reference number for parent qgroup, since the source (child) qgroup only have exclusive extents, destination (parent) qgroup will also own or lose these extents exclusively.

So should be the same for reservation, since later reservation adding/releasing will also affect parent qgroup, without the servation carried from child, parent will underflow reservation or have dead reservation which will never be freed.

However original code doesn't do the same thing for reservation. It handles qgroup reservation quite differently:

It removes qgroup reservation, as it's allocating space from the reserved qgroup for relationship adding.
But does nothing for qgroup reservation if we're removing a qgroup relationship.

According to the original code, it looks just like because we're adding qgroup->rfer, the code assumes we're writing new data, so it's follows the normal write routine, by reducing qgroup->reserved and adding qgroup->rfer/excl.

This old behavior is wrong, and should be fixed to follow the same excl/rfer behavior.

Just fix it by using the correct behavior described above.

Fixes 31193213f1f9 ("Btrfs: qgroup: Introduce a may_use to account space_info->bytes_may_use.")
Signed-off-by Qu Wenruo <[email protected]>
---
 fs/btrfs/qgroup.c | 44 +++++++++++++++++++++++---------------------
 1 file changed, 23 insertions(+), 21 deletions(-)
fs/btrfs/qgroup.c +23 -21
index 2c690aa19d84..cbc31cfabf44 100644
@@ -1071,21 +1071,30 @@ 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 has exclusive extents.
+ *
+ * In this case, all exclsuive extents will also be exlusive for parent, so
+ * excl/rfer just get added/removed.
+ *
+ * So is qgroup reservation space, which should also be added/removed to
+ * parent.
+ * Or when child tries to release reservation space, parent will underflow its
+ * reservation (for relationship adding case).
  *
  * 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);

@@ -1098,13 +1107,11 @@ 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;
-	}
+
+	if (sign > 0)
+		qgroup_rsv_add_by_qgroup(qgroup, src);
+	else
+		qgroup_rsv_release_by_qgroup(qgroup, src);
 
 	qgroup_dirty(fs_info, qgroup);
 

@@ -1124,15 +1131,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_add_by_qgroup(qgroup, src);
+		else
+			qgroup_rsv_release_by_qgroup(qgroup, src);
 		qgroup->excl_cmpr += sign * num_bytes;
 		qgroup_dirty(fs_info, qgroup);
 

@@ -1175,7 +1177,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;
Signature · 1 line
2.14.2
#5 lore raw

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

Quoted 6 lines · 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 adding/substracting exclusive and reference number for parent qgroup, since the source (child) qgroup only have exclusive extents, destination (parent) qgroup will also own or lose these

^ what
does own or lose mean how is it decided whether it's losing them or owning them ??

Quoted 3 lines

extents exclusively.

So should be the same for reservation, since later reservation

What does the 'same' refer to here - the "will also own or lose these extents exclusively" ? It's a bit ambiguous given the surrounding context.

Quoted 1 line

adding/releasing will also affect parent qgroup, without the servation

nit: s/servation/reservation

Quoted 4 lines

carried from child, parent will underflow reservation or have dead reservation which will never be freed.

However original code doesn't do the same thing for reservation.

what does "the same" refer to here?

Quoted 9 lines

It handles qgroup reservation quite differently:

It removes qgroup reservation, as it's allocating space from the reserved qgroup for relationship adding.
But does nothing for qgroup reservation if we're removing a qgroup relationship.

According to the original code, it looks just like because we're adding qgroup->rfer, the code assumes we're writing new data, so it's follows

nit:s/it's/it/

It might not be worth it doing a resend of the series with those fixes, I guess David could fix them up while committing them but just answering my question might help in fleshing out e clearer commit message.

Quoted 102 lines

the normal write routine, by reducing qgroup->reserved and adding qgroup->rfer/excl.

This old behavior is wrong, and should be fixed to follow the same excl/rfer behavior.

Just fix it by using the correct behavior described above.

Fixes 31193213f1f9 ("Btrfs: qgroup: Introduce a may_use to account space_info->bytes_may_use.")
Signed-off-by Qu Wenruo <[email protected]>
---
 fs/btrfs/qgroup.c | 44 +++++++++++++++++++++++---------------------
 1 file changed, 23 insertions(+), 21 deletions(-)
fs/btrfs/qgroup.c +23 -21
index 2c690aa19d84..cbc31cfabf44 100644
@@ -1071,21 +1071,30 @@ 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 has exclusive extents.
+ *
+ * In this case, all exclsuive extents will also be exlusive for parent, so
+ * excl/rfer just get added/removed.
+ *
+ * So is qgroup reservation space, which should also be added/removed to
+ * parent.
+ * Or when child tries to release reservation space, parent will underflow its
+ * reservation (for relationship adding case).
  *
  * 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);

@@ -1098,13 +1107,11 @@ 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;
-	}
+
+	if (sign > 0)
+		qgroup_rsv_add_by_qgroup(qgroup, src);
+	else
+		qgroup_rsv_release_by_qgroup(qgroup, src);
 
 	qgroup_dirty(fs_info, qgroup);
 

@@ -1124,15 +1131,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_add_by_qgroup(qgroup, src);
+		else
+			qgroup_rsv_release_by_qgroup(qgroup, src);
 		qgroup->excl_cmpr += sign * num_bytes;
 		qgroup_dirty(fs_info, qgroup);
 

@@ -1175,7 +1177,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;
#6 lore raw

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

Quoted 6 lines · Nikolay Borisov
Quoted 3 lines · Qu Wenruo

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

Quoted 6 lines
Quoted 3 lines

In quick update path, we will adding/substracting exclusive and reference number for parent qgroup, since the source (child) qgroup only have exclusive extents, destination (parent) qgroup will also own or lose these

^ what
does own or lose mean how is it decided whether it's losing them or owning them ??

Adding a child qgroup to parent, then the parent qgroup *owns* all these extents of the child groups.
And vice verse (lose).

Of course, only for "exclusive-only" child qgroup, whose extents are all exclusive.

Quoted 7 lines
Quoted 3 lines

extents exclusively.

So should be the same for reservation, since later reservation

What does the 'same' refer to here - the "will also own or lose these extents exclusively" ? It's a bit ambiguous given the surrounding context.

Quoted 1 line

adding/releasing will also affect parent qgroup, without the servation

Quoted 8 lines

nit: s/servation/reservation

Quoted 4 lines

carried from child, parent will underflow reservation or have dead reservation which will never be freed.

However original code doesn't do the same thing for reservation.

what does "the same" refer to here?

"The same" here means, the old code doesn't carry the reservation from child, unlike it carries all rfer/excl from child.

Quoted 8 lines
Quoted 8 lines

It handles qgroup reservation quite differently:

It removes qgroup reservation, as it's allocating space from the reserved qgroup for relationship adding.
But does nothing for qgroup reservation if we're removing a qgroup relationship.

Quoted 8 lines
Quoted 2 lines

According to the original code, it looks just like because we're adding qgroup->rfer, the code assumes we're writing new data, so it's follows

nit:s/it's/it/

It might not be worth it doing a resend of the series with those fixes, I guess David could fix them up while committing them but just answering my question might help in fleshing out e clearer commit message.

No problem,

And answering your question can also help me to refine not only the commit message the term used and the logic.

Thanks,
Qu

Quoted 108 lines
Quoted 95 lines

the normal write routine, by reducing qgroup->reserved and adding qgroup->rfer/excl.

This old behavior is wrong, and should be fixed to follow the same excl/rfer behavior.

Just fix it by using the correct behavior described above.

Fixes 31193213f1f9 ("Btrfs: qgroup: Introduce a may_use to account space_info->bytes_may_use.")
Signed-off-by Qu Wenruo <[email protected]>
---
 fs/btrfs/qgroup.c | 44 +++++++++++++++++++++++---------------------
 1 file changed, 23 insertions(+), 21 deletions(-)
fs/btrfs/qgroup.c +22 -20
index 2c690aa19d84..cbc31cfabf44 100644
@@ -1071,21 +1071,30 @@ 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 has exclusive extents.
+ *
+ * In this case, all exclsuive extents will also be exlusive for parent, so
+ * excl/rfer just get added/removed.
+ *
+ * So is qgroup reservation space, which should also be added/removed to
+ * parent.
+ * Or when child tries to release reservation space, parent will underflow its
+ * reservation (for relationship adding case).
  *
  * 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);

@@ -1098,13 +1107,11 @@ 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;
-	}
+
+	if (sign > 0)
+		qgroup_rsv_add_by_qgroup(qgroup, src);
+	else
+		qgroup_rsv_release_by_qgroup(qgroup, src);
 
 	qgroup_dirty(fs_info, qgroup);
 

@@ -1124,15 +1131,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_add_by_qgroup(qgroup, src);
+		else
+			qgroup_rsv_release_by_qgroup(qgroup, src);
 		qgroup->excl_cmpr += sign * num_bytes;
 		qgroup_dirty(fs_info, qgroup);

fs/btrfs/qgroup.c static int quick_update_accounting(struct btrfs_fs_info *fs_info, · line 1183

Quoted 8 lines
fs/btrfs/qgroup.c +1 -1
@@ -1175,7 +1177,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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

author
#7 lore raw

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

In reply to Qu Wenruo
Signed-off-by Qu Wenruo <[email protected]>
---
 fs/btrfs/qgroup.c            | 36 +++++++++++++++++++++---------------
 include/trace/events/btrfs.h | 17 ++++++++++++-----
 2 files changed, 33 insertions(+), 20 deletions(-)
fs/btrfs/qgroup.c +21 -15
index cbc31cfabf44..0a38e65f55f7 100644
@@ -75,15 +75,19 @@ static const char *qgroup_rsv_type_str(enum btrfs_qgroup_rsv_type type)
 }
 #endif
 
-static void qgroup_rsv_add(struct btrfs_qgroup *qgroup, u64 num_bytes,
+static void qgroup_rsv_add(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_release(struct btrfs_qgroup *qgroup, u64 num_bytes,
+static void qgroup_rsv_release(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;

@@ -97,22 +101,24 @@ static void qgroup_rsv_release(struct btrfs_qgroup *qgroup, u64 num_bytes,
 	qgroup->rsv.values[type] = 0;
 }
 
-static void qgroup_rsv_add_by_qgroup(struct btrfs_qgroup *dest,
-					  struct btrfs_qgroup *src)
+static void qgroup_rsv_add_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_LAST; i++)
-		qgroup_rsv_add(dest, src->rsv.values[i], i);
+		qgroup_rsv_add(fs_info, dest, src->rsv.values[i], i);
 }
 
-static void qgroup_rsv_release_by_qgroup(struct btrfs_qgroup *dest,
+static void qgroup_rsv_release_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_LAST; i++)
-		qgroup_rsv_release(dest, src->rsv.values[i], i);
+		qgroup_rsv_release(fs_info, dest, src->rsv.values[i], i);
 }
 
 static void btrfs_qgroup_update_old_refcnt(struct btrfs_qgroup *qg, u64 seq,

@@ -1109,9 +1115,9 @@ static int __qgroup_excl_accounting(struct btrfs_fs_info *fs_info,
 	qgroup->excl_cmpr += sign * num_bytes;
 
 	if (sign > 0)
-		qgroup_rsv_add_by_qgroup(qgroup, src);
+		qgroup_rsv_add_by_qgroup(fs_info, qgroup, src);
 	else
-		qgroup_rsv_release_by_qgroup(qgroup, src);
+		qgroup_rsv_release_by_qgroup(fs_info, qgroup, src);
 
 	qgroup_dirty(fs_info, qgroup);
 

@@ -1132,9 +1138,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_add_by_qgroup(qgroup, src);
+			qgroup_rsv_add_by_qgroup(fs_info, qgroup, src);
 		else
-			qgroup_rsv_release_by_qgroup(qgroup, src);
+			qgroup_rsv_release_by_qgroup(fs_info, qgroup, src);
 		qgroup->excl_cmpr += sign * num_bytes;
 		qgroup_dirty(fs_info, qgroup);
 

@@ -2505,8 +2511,8 @@ 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_add(qg, num_bytes, type);
+		trace_qgroup_update_reserve(fs_info, qg, num_bytes, type);
+		qgroup_rsv_add(fs_info, qg, num_bytes, type);
 	}
 
 out:

@@ -2552,8 +2558,8 @@ 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_release(qg, num_bytes, type);
+		trace_qgroup_update_reserve(fs_info, qg, -(s64)num_bytes, type);
+		qgroup_rsv_release(fs_info, qg, num_bytes, type);
 
 		list_for_each_entry(glist, &qg->groups, next_group) {
 			ret = ulist_add(fs_info->qgroup_ulist,
include/trace/events/btrfs.h +12 -5
index dc1d0df91e0b..68c4d11f55fa 100644
@@ -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,
Signature · 1 line
2.14.2
author
#8 lore raw

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

In reply to Qu Wenruo

So qgroup is switched to new separate types reservation system.

Signed-off-by Qu Wenruo <[email protected]>
---
 fs/btrfs/qgroup.c | 13 -------------
 fs/btrfs/qgroup.h |  1 -
 2 files changed, 14 deletions(-)
fs/btrfs/qgroup.c +0 -13
index 0a38e65f55f7..4063004ae9d5 100644
@@ -1065,19 +1065,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 has exclusive extents.
fs/btrfs/qgroup.h +0 -1
index fead9955af72..c8c81b923674 100644
@@ -107,7 +107,6 @@ struct btrfs_qgroup {
 	/*
 	 * reservation tracking
 	 */
-	u64 reserved;
 	struct btrfs_qgroup_rsv rsv;
 
 	/*
Signature · 1 line
2.14.2