[PATCH v2 0/6] btrfs: qgroup: remove GFP_ATOMIC usage for ulist

[PATCH v2 0/6] btrfs: qgroup: remove GFP_ATOMIC usage for ulist

[Qu Wenruo]

[CHANGELOG]
v2:
- Remove the final GFP_ATOMIC ulist usage
  This is done by introducing a new list_head, btrfs_qgroup::iterator2,
  allowing us to do nested iteration.

  This extra nested facility is needed as even if we move the qgroups
  collection into one dedicated function, we're reusing the list for
  iteration which can lead to unnecessary re-visit of the same qgroup.

  Thus we have to support one level of nested iteration.

- Add reviewed by tags from Boris

[REPO]
https://github.com/adam900710/linux/tree/qgroup_mutex

Yep, the branch name shows my previous failed attempt to solve the
problem.

[PROBLEM]
There are quite some GFP_ATOMIC usage for btrfs qgroups, most of them
are for ulists.

Those ulists are just used as a temporary memory to trace the involved
qgroups.

[ENHANCEMENT]
This patchset would address the problem by adding a new list_head called
iterator for btrfs_qgroup.

And all call sites but one of ulist allocation can be migrated to use
the new qgroup_iterator facility to iterate qgroups without any memory
allocation.

The only remaining ulist call site is @qgroups ulist utilized inside
btrfs_qgroup_account_extent(), which is utilized to get all involved
qgroups for both old and new roots.

I tried to extract the qgroups collection code into a dedicate loop
out of qgroup_update_refcnt(), but it would lead to test case failure of
btrfs/028 (accounts underflow).

Thus for now only the safe part is sent to the list.

And BTW since we can skip quite some memory allocation failure handling
(since there is no memory allocation), we also save some lines of code.

Qu Wenruo (6):
  btrfs: qgroup: iterate qgroups without memory allocation for
    qgroup_reserve()
  btrfs: qgroup: use qgroup_iterator facility for
    btrfs_qgroup_free_refroot()
  btrfs: qgroup: use qgroup_iterator facility for qgroup_convert_meta()
  btrfs: qgroup: use qgroup_iterator facility for
    __qgroup_excl_accounting()
  btrfs: qgroup: use qgroup_iterator facility to replace @tmp ulist of
    qgroup_update_refcnt()
  btrfs: qgroup: use qgroup_iterator2 facility to replace @qgroups ulist
    of qgroup_update_refcnt()

 fs/btrfs/qgroup.c | 325 +++++++++++++++++-----------------------------
 fs/btrfs/qgroup.h |  12 ++
 2 files changed, 130 insertions(+), 207 deletions(-)

-- 
2.41.0

[PATCH v2 1/6] btrfs: qgroup: iterate qgroups without memory allocation for qgroup_reserve()

[Qu Wenruo]

Qgroup heavily relies on ulist to go through all the involved
qgroups, but since we're using ulist inside fs_info->qgroup_lock
spinlock, this means we're doing a lot of GFP_ATOMIC allocation.

This patch would reduce the GFP_ATOMIC usage for qgroup_reserve() by
eliminating the memory allocation completely.

This is done by moving the needed memory to btrfs_qgroup::iterator
list_head, so that we can put all the involved qgroup into a on-stack list, thus
eliminate the need to allocate memory holding spinlock.

The only cost is the slightly higher memory usage, but considering the
reduce GFP_ATOMIC during a hot path, it should still be acceptable.

Function qgroup_reserve() is the perfect start point for this
conversion.

Reviewed-by: Boris Burkov <boris@bur.io>
Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/qgroup.c | 68 +++++++++++++++++++++++------------------------
 fs/btrfs/qgroup.h |  9 +++++++
 2 files changed, 42 insertions(+), 35 deletions(-)

diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
index 74244b4bb0e9..de34e2aef710 100644
--- a/fs/btrfs/qgroup.c
+++ b/fs/btrfs/qgroup.c
@@ -216,6 +216,7 @@ static struct btrfs_qgroup *add_qgroup_rb(struct btrfs_fs_info *fs_info,
 	INIT_LIST_HEAD(&qgroup->groups);
 	INIT_LIST_HEAD(&qgroup->members);
 	INIT_LIST_HEAD(&qgroup->dirty);
+	INIT_LIST_HEAD(&qgroup->iterator);
 
 	rb_link_node(&qgroup->node, parent, p);
 	rb_insert_color(&qgroup->node, &fs_info->qgroup_tree);
@@ -1367,6 +1368,25 @@ static void qgroup_dirty(struct btrfs_fs_info *fs_info,
 		list_add(&qgroup->dirty, &fs_info->dirty_qgroups);
 }
 
+static void qgroup_iterator_add(struct list_head *head, struct btrfs_qgroup *qgroup)
+{
+	if (!list_empty(&qgroup->iterator))
+		return;
+
+	list_add_tail(&qgroup->iterator, head);
+}
+
+static void qgroup_iterator_clean(struct list_head *head)
+{
+
+	while (!list_empty(head)) {
+		struct btrfs_qgroup *qgroup;
+
+		qgroup = list_first_entry(head, struct btrfs_qgroup, iterator);
+		list_del_init(&qgroup->iterator);
+	}
+}
+
 /*
  * The easy accounting, we're updating qgroup relationship whose child qgroup
  * only has exclusive extents.
@@ -3154,12 +3174,11 @@ static bool qgroup_check_limits(const struct btrfs_qgroup *qg, u64 num_bytes)
 static int qgroup_reserve(struct btrfs_root *root, u64 num_bytes, bool enforce,
 			  enum btrfs_qgroup_rsv_type type)
 {
-	struct btrfs_qgroup *qgroup;
+	struct btrfs_qgroup *cur;
 	struct btrfs_fs_info *fs_info = root->fs_info;
 	u64 ref_root = root->root_key.objectid;
 	int ret = 0;
-	struct ulist_node *unode;
-	struct ulist_iterator uiter;
+	LIST_HEAD(qgroup_list);
 
 	if (!is_fstree(ref_root))
 		return 0;
@@ -3175,53 +3194,32 @@ static int qgroup_reserve(struct btrfs_root *root, u64 num_bytes, bool enforce,
 	if (!fs_info->quota_root)
 		goto out;
 
-	qgroup = find_qgroup_rb(fs_info, ref_root);
-	if (!qgroup)
+	cur = find_qgroup_rb(fs_info, ref_root);
+	if (!cur)
 		goto out;
 
-	/*
-	 * in a first step, we check all affected qgroups if any limits would
-	 * be exceeded
-	 */
-	ulist_reinit(fs_info->qgroup_ulist);
-	ret = ulist_add(fs_info->qgroup_ulist, qgroup->qgroupid,
-			qgroup_to_aux(qgroup), GFP_ATOMIC);
-	if (ret < 0)
-		goto out;
-	ULIST_ITER_INIT(&uiter);
-	while ((unode = ulist_next(fs_info->qgroup_ulist, &uiter))) {
-		struct btrfs_qgroup *qg;
+	qgroup_iterator_add(&qgroup_list, cur);
+	list_for_each_entry(cur, &qgroup_list, iterator) {
 		struct btrfs_qgroup_list *glist;
 
-		qg = unode_aux_to_qgroup(unode);
-
-		if (enforce && !qgroup_check_limits(qg, num_bytes)) {
+		if (enforce && !qgroup_check_limits(cur, num_bytes)) {
 			ret = -EDQUOT;
 			goto out;
 		}
 
-		list_for_each_entry(glist, &qg->groups, next_group) {
-			ret = ulist_add(fs_info->qgroup_ulist,
-					glist->group->qgroupid,
-					qgroup_to_aux(glist->group), GFP_ATOMIC);
-			if (ret < 0)
-				goto out;
-		}
+		list_for_each_entry(glist, &cur->groups, next_group)
+			qgroup_iterator_add(&qgroup_list, glist->group);
 	}
+
 	ret = 0;
 	/*
 	 * no limits exceeded, now record the reservation into all qgroups
 	 */
-	ULIST_ITER_INIT(&uiter);
-	while ((unode = ulist_next(fs_info->qgroup_ulist, &uiter))) {
-		struct btrfs_qgroup *qg;
-
-		qg = unode_aux_to_qgroup(unode);
-
-		qgroup_rsv_add(fs_info, qg, num_bytes, type);
-	}
+	list_for_each_entry(cur, &qgroup_list, iterator)
+		qgroup_rsv_add(fs_info, cur, num_bytes, type);
 
 out:
+	qgroup_iterator_clean(&qgroup_list);
 	spin_unlock(&fs_info->qgroup_lock);
 	return ret;
 }
diff --git a/fs/btrfs/qgroup.h b/fs/btrfs/qgroup.h
index 7bffa10589d6..5dc0583622c3 100644
--- a/fs/btrfs/qgroup.h
+++ b/fs/btrfs/qgroup.h
@@ -220,6 +220,15 @@ struct btrfs_qgroup {
 	struct list_head groups;  /* groups this group is member of */
 	struct list_head members; /* groups that are members of this group */
 	struct list_head dirty;   /* dirty groups */
+
+	/*
+	 * For qgroup iteration usage.
+	 *
+	 * The iteration list should always be empty until
+	 * qgroup_iterator_add() is called.
+	 * And should be reset to empty after the iteration is finished.
+	 */
+	struct list_head iterator;
 	struct rb_node node;	  /* tree of qgroups */
 
 	/*
-- 
2.41.0

[PATCH v2 2/6] btrfs: qgroup: use qgroup_iterator facility for btrfs_qgroup_free_refroot()

[Qu Wenruo]

With the new qgroup_iterator_add() and qgroup_iterator_clean(), we can
get rid of the ulist and its GFP_ATOMIC memory allocation.

Reviewed-by: Boris Burkov <boris@bur.io>
Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/qgroup.c | 37 +++++++++++--------------------------
 1 file changed, 11 insertions(+), 26 deletions(-)

diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
index de34e2aef710..c2a1173d9f00 100644
--- a/fs/btrfs/qgroup.c
+++ b/fs/btrfs/qgroup.c
@@ -3237,10 +3237,8 @@ void btrfs_qgroup_free_refroot(struct btrfs_fs_info *fs_info,
 			       u64 ref_root, u64 num_bytes,
 			       enum btrfs_qgroup_rsv_type type)
 {
-	struct btrfs_qgroup *qgroup;
-	struct ulist_node *unode;
-	struct ulist_iterator uiter;
-	int ret = 0;
+	struct btrfs_qgroup *cur;
+	LIST_HEAD(qgroup_list);
 
 	if (!is_fstree(ref_root))
 		return;
@@ -3257,8 +3255,8 @@ void btrfs_qgroup_free_refroot(struct btrfs_fs_info *fs_info,
 	if (!fs_info->quota_root)
 		goto out;
 
-	qgroup = find_qgroup_rb(fs_info, ref_root);
-	if (!qgroup)
+	cur = find_qgroup_rb(fs_info, ref_root);
+	if (!cur)
 		goto out;
 
 	if (num_bytes == (u64)-1)
@@ -3266,32 +3264,19 @@ void btrfs_qgroup_free_refroot(struct btrfs_fs_info *fs_info,
 		 * We're freeing all pertrans rsv, get reserved value from
 		 * level 0 qgroup as real num_bytes to free.
 		 */
-		num_bytes = qgroup->rsv.values[type];
+		num_bytes = cur->rsv.values[type];
 
-	ulist_reinit(fs_info->qgroup_ulist);
-	ret = ulist_add(fs_info->qgroup_ulist, qgroup->qgroupid,
-			qgroup_to_aux(qgroup), GFP_ATOMIC);
-	if (ret < 0)
-		goto out;
-	ULIST_ITER_INIT(&uiter);
-	while ((unode = ulist_next(fs_info->qgroup_ulist, &uiter))) {
-		struct btrfs_qgroup *qg;
+	qgroup_iterator_add(&qgroup_list, cur);
+	list_for_each_entry(cur, &qgroup_list, iterator) {
 		struct btrfs_qgroup_list *glist;
 
-		qg = unode_aux_to_qgroup(unode);
-
-		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,
-					glist->group->qgroupid,
-					qgroup_to_aux(glist->group), GFP_ATOMIC);
-			if (ret < 0)
-				goto out;
+		qgroup_rsv_release(fs_info, cur, num_bytes, type);
+		list_for_each_entry(glist, &cur->groups, next_group) {
+			qgroup_iterator_add(&qgroup_list, glist->group);
 		}
 	}
-
 out:
+	qgroup_iterator_clean(&qgroup_list);
 	spin_unlock(&fs_info->qgroup_lock);
 }
 
-- 
2.41.0

[PATCH v2 3/6] btrfs: qgroup: use qgroup_iterator facility for qgroup_convert_meta()

[Qu Wenruo]

With the new qgroup_iterator_add() and qgroup_iterator_clean(), we can
get rid of the ulist and its GFP_ATOMIC memory allocation.

Reviewed-by: Boris Burkov <boris@bur.io>
Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/qgroup.c | 38 +++++++++++++-------------------------
 1 file changed, 13 insertions(+), 25 deletions(-)

diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
index c2a1173d9f00..aa8e9e7be4f8 100644
--- a/fs/btrfs/qgroup.c
+++ b/fs/btrfs/qgroup.c
@@ -4119,10 +4119,8 @@ void __btrfs_qgroup_free_meta(struct btrfs_root *root, int num_bytes,
 static void qgroup_convert_meta(struct btrfs_fs_info *fs_info, u64 ref_root,
 				int num_bytes)
 {
-	struct btrfs_qgroup *qgroup;
-	struct ulist_node *unode;
-	struct ulist_iterator uiter;
-	int ret = 0;
+	struct btrfs_qgroup *cur;
+	LIST_HEAD(qgroup_list);
 
 	if (num_bytes == 0)
 		return;
@@ -4130,34 +4128,24 @@ static void qgroup_convert_meta(struct btrfs_fs_info *fs_info, u64 ref_root,
 		return;
 
 	spin_lock(&fs_info->qgroup_lock);
-	qgroup = find_qgroup_rb(fs_info, ref_root);
-	if (!qgroup)
+	cur = find_qgroup_rb(fs_info, ref_root);
+	if (!cur)
 		goto out;
-	ulist_reinit(fs_info->qgroup_ulist);
-	ret = ulist_add(fs_info->qgroup_ulist, qgroup->qgroupid,
-		       qgroup_to_aux(qgroup), GFP_ATOMIC);
-	if (ret < 0)
-		goto out;
-	ULIST_ITER_INIT(&uiter);
-	while ((unode = ulist_next(fs_info->qgroup_ulist, &uiter))) {
-		struct btrfs_qgroup *qg;
+
+	qgroup_iterator_add(&qgroup_list, cur);
+	list_for_each_entry(cur, &qgroup_list, iterator) {
 		struct btrfs_qgroup_list *glist;
 
-		qg = unode_aux_to_qgroup(unode);
-
-		qgroup_rsv_release(fs_info, qg, num_bytes,
+		qgroup_rsv_release(fs_info, cur, num_bytes,
 				BTRFS_QGROUP_RSV_META_PREALLOC);
-		qgroup_rsv_add(fs_info, qg, num_bytes,
+		qgroup_rsv_add(fs_info, cur, num_bytes,
 				BTRFS_QGROUP_RSV_META_PERTRANS);
-		list_for_each_entry(glist, &qg->groups, next_group) {
-			ret = ulist_add(fs_info->qgroup_ulist,
-					glist->group->qgroupid,
-					qgroup_to_aux(glist->group), GFP_ATOMIC);
-			if (ret < 0)
-				goto out;
-		}
+
+		list_for_each_entry(glist, &cur->groups, next_group)
+			qgroup_iterator_add(&qgroup_list, glist->group);
 	}
 out:
+	qgroup_iterator_clean(&qgroup_list);
 	spin_unlock(&fs_info->qgroup_lock);
 }
 
-- 
2.41.0

[PATCH v2 4/6] btrfs: qgroup: use qgroup_iterator facility for __qgroup_excl_accounting()

[Qu Wenruo]

With the new qgroup_iterator_add() and qgroup_iterator_clean(), we can
get rid of the ulist and its GFP_ATOMIC memory allocation.

Furthermore we can merge the code handling the initial and parent
qgroups into one loop, and drop the @tmp ulist parameter for involved
call sites.

Reviewed-by: Boris Burkov <boris@bur.io>
Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/qgroup.c | 72 +++++++++++------------------------------------
 1 file changed, 17 insertions(+), 55 deletions(-)

diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
index aa8e9e7be4f8..08f4fc622180 100644
--- a/fs/btrfs/qgroup.c
+++ b/fs/btrfs/qgroup.c
@@ -1401,14 +1401,12 @@ static void qgroup_iterator_clean(struct list_head *head)
  *
  * Caller should hold fs_info->qgroup_lock.
  */
-static int __qgroup_excl_accounting(struct btrfs_fs_info *fs_info,
-				    struct ulist *tmp, u64 ref_root,
+static int __qgroup_excl_accounting(struct btrfs_fs_info *fs_info, u64 ref_root,
 				    struct btrfs_qgroup *src, int sign)
 {
 	struct btrfs_qgroup *qgroup;
-	struct btrfs_qgroup_list *glist;
-	struct ulist_node *unode;
-	struct ulist_iterator uiter;
+	struct btrfs_qgroup *cur;
+	LIST_HEAD(qgroup_list);
 	u64 num_bytes = src->excl;
 	int ret = 0;
 
@@ -1416,53 +1414,30 @@ static int __qgroup_excl_accounting(struct btrfs_fs_info *fs_info,
 	if (!qgroup)
 		goto out;
 
-	qgroup->rfer += sign * num_bytes;
-	qgroup->rfer_cmpr += sign * num_bytes;
+	qgroup_iterator_add(&qgroup_list, qgroup);
+	list_for_each_entry(cur, &qgroup_list, iterator) {
+		struct btrfs_qgroup_list *glist;
 
-	WARN_ON(sign < 0 && qgroup->excl < num_bytes);
-	qgroup->excl += sign * num_bytes;
-	qgroup->excl_cmpr += sign * num_bytes;
-
-	if (sign > 0)
-		qgroup_rsv_add_by_qgroup(fs_info, qgroup, src);
-	else
-		qgroup_rsv_release_by_qgroup(fs_info, qgroup, src);
-
-	qgroup_dirty(fs_info, qgroup);
-
-	/* Get all of the parent groups that contain this qgroup */
-	list_for_each_entry(glist, &qgroup->groups, next_group) {
-		ret = ulist_add(tmp, glist->group->qgroupid,
-				qgroup_to_aux(glist->group), GFP_ATOMIC);
-		if (ret < 0)
-			goto out;
-	}
-
-	/* Iterate all of the parents and adjust their reference counts */
-	ULIST_ITER_INIT(&uiter);
-	while ((unode = ulist_next(tmp, &uiter))) {
-		qgroup = unode_aux_to_qgroup(unode);
 		qgroup->rfer += sign * num_bytes;
 		qgroup->rfer_cmpr += sign * num_bytes;
+
 		WARN_ON(sign < 0 && qgroup->excl < num_bytes);
 		qgroup->excl += sign * num_bytes;
+		qgroup->excl_cmpr += sign * num_bytes;
+
 		if (sign > 0)
 			qgroup_rsv_add_by_qgroup(fs_info, qgroup, src);
 		else
 			qgroup_rsv_release_by_qgroup(fs_info, qgroup, src);
-		qgroup->excl_cmpr += sign * num_bytes;
 		qgroup_dirty(fs_info, qgroup);
 
-		/* Add any parents of the parents */
-		list_for_each_entry(glist, &qgroup->groups, next_group) {
-			ret = ulist_add(tmp, glist->group->qgroupid,
-					qgroup_to_aux(glist->group), GFP_ATOMIC);
-			if (ret < 0)
-				goto out;
-		}
+		/* Append parent qgroups to @qgroup_list. */
+		list_for_each_entry(glist, &qgroup->groups, next_group)
+			qgroup_iterator_add(&qgroup_list, glist->group);
 	}
 	ret = 0;
 out:
+	qgroup_iterator_clean(&qgroup_list);
 	return ret;
 }
 
@@ -1479,8 +1454,7 @@ static int __qgroup_excl_accounting(struct btrfs_fs_info *fs_info,
  * Return < 0 for other error.
  */
 static int quick_update_accounting(struct btrfs_fs_info *fs_info,
-				   struct ulist *tmp, u64 src, u64 dst,
-				   int sign)
+				   u64 src, u64 dst, int sign)
 {
 	struct btrfs_qgroup *qgroup;
 	int ret = 1;
@@ -1491,8 +1465,7 @@ static int quick_update_accounting(struct btrfs_fs_info *fs_info,
 		goto out;
 	if (qgroup->excl == qgroup->rfer) {
 		ret = 0;
-		err = __qgroup_excl_accounting(fs_info, tmp, dst,
-					       qgroup, sign);
+		err = __qgroup_excl_accounting(fs_info, dst, qgroup, sign);
 		if (err < 0) {
 			ret = err;
 			goto out;
@@ -1511,7 +1484,6 @@ int btrfs_add_qgroup_relation(struct btrfs_trans_handle *trans, u64 src,
 	struct btrfs_qgroup *parent;
 	struct btrfs_qgroup *member;
 	struct btrfs_qgroup_list *list;
-	struct ulist *tmp;
 	unsigned int nofs_flag;
 	int ret = 0;
 
@@ -1521,10 +1493,7 @@ int btrfs_add_qgroup_relation(struct btrfs_trans_handle *trans, u64 src,
 
 	/* We hold a transaction handle open, must do a NOFS allocation. */
 	nofs_flag = memalloc_nofs_save();
-	tmp = ulist_alloc(GFP_KERNEL);
 	memalloc_nofs_restore(nofs_flag);
-	if (!tmp)
-		return -ENOMEM;
 
 	mutex_lock(&fs_info->qgroup_ioctl_lock);
 	if (!fs_info->quota_root) {
@@ -1562,11 +1531,10 @@ int btrfs_add_qgroup_relation(struct btrfs_trans_handle *trans, u64 src,
 		spin_unlock(&fs_info->qgroup_lock);
 		goto out;
 	}
-	ret = quick_update_accounting(fs_info, tmp, src, dst, 1);
+	ret = quick_update_accounting(fs_info, src, dst, 1);
 	spin_unlock(&fs_info->qgroup_lock);
 out:
 	mutex_unlock(&fs_info->qgroup_ioctl_lock);
-	ulist_free(tmp);
 	return ret;
 }
 
@@ -1577,7 +1545,6 @@ static int __del_qgroup_relation(struct btrfs_trans_handle *trans, u64 src,
 	struct btrfs_qgroup *parent;
 	struct btrfs_qgroup *member;
 	struct btrfs_qgroup_list *list;
-	struct ulist *tmp;
 	bool found = false;
 	unsigned int nofs_flag;
 	int ret = 0;
@@ -1585,11 +1552,7 @@ static int __del_qgroup_relation(struct btrfs_trans_handle *trans, u64 src,
 
 	/* We hold a transaction handle open, must do a NOFS allocation. */
 	nofs_flag = memalloc_nofs_save();
-	tmp = ulist_alloc(GFP_KERNEL);
 	memalloc_nofs_restore(nofs_flag);
-	if (!tmp)
-		return -ENOMEM;
-
 	if (!fs_info->quota_root) {
 		ret = -ENOTCONN;
 		goto out;
@@ -1627,11 +1590,10 @@ static int __del_qgroup_relation(struct btrfs_trans_handle *trans, u64 src,
 	if (found) {
 		spin_lock(&fs_info->qgroup_lock);
 		del_relation_rb(fs_info, src, dst);
-		ret = quick_update_accounting(fs_info, tmp, src, dst, -1);
+		ret = quick_update_accounting(fs_info, src, dst, -1);
 		spin_unlock(&fs_info->qgroup_lock);
 	}
 out:
-	ulist_free(tmp);
 	return ret;
 }
 
-- 
2.41.0

Re: [PATCH v2 4/6] btrfs: qgroup: use qgroup_iterator facility for __qgroup_excl_accounting()

[David Sterba]

On Thu, Aug 31, 2023 at 08:30:35AM +0800, Qu Wenruo wrote:
> With the new qgroup_iterator_add() and qgroup_iterator_clean(), we can
> get rid of the ulist and its GFP_ATOMIC memory allocation.
> 
> Furthermore we can merge the code handling the initial and parent
> qgroups into one loop, and drop the @tmp ulist parameter for involved
> call sites.
> 
> Reviewed-by: Boris Burkov <boris@bur.io>
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>  fs/btrfs/qgroup.c | 72 +++++++++++------------------------------------
>  1 file changed, 17 insertions(+), 55 deletions(-)
> 
> diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
> index aa8e9e7be4f8..08f4fc622180 100644
> --- a/fs/btrfs/qgroup.c
> +++ b/fs/btrfs/qgroup.c
> @@ -1401,14 +1401,12 @@ static void qgroup_iterator_clean(struct list_head *head)
>   *
>   * Caller should hold fs_info->qgroup_lock.
>   */
> -static int __qgroup_excl_accounting(struct btrfs_fs_info *fs_info,
> -				    struct ulist *tmp, u64 ref_root,
> +static int __qgroup_excl_accounting(struct btrfs_fs_info *fs_info, u64 ref_root,
>  				    struct btrfs_qgroup *src, int sign)
>  {
>  	struct btrfs_qgroup *qgroup;
> -	struct btrfs_qgroup_list *glist;
> -	struct ulist_node *unode;
> -	struct ulist_iterator uiter;
> +	struct btrfs_qgroup *cur;
> +	LIST_HEAD(qgroup_list);
>  	u64 num_bytes = src->excl;
>  	int ret = 0;
>  
> @@ -1416,53 +1414,30 @@ static int __qgroup_excl_accounting(struct btrfs_fs_info *fs_info,
>  	if (!qgroup)
>  		goto out;
>  
> -	qgroup->rfer += sign * num_bytes;
> -	qgroup->rfer_cmpr += sign * num_bytes;
> +	qgroup_iterator_add(&qgroup_list, qgroup);
> +	list_for_each_entry(cur, &qgroup_list, iterator) {
> +		struct btrfs_qgroup_list *glist;
>  
> -	WARN_ON(sign < 0 && qgroup->excl < num_bytes);
> -	qgroup->excl += sign * num_bytes;
> -	qgroup->excl_cmpr += sign * num_bytes;
> -
> -	if (sign > 0)
> -		qgroup_rsv_add_by_qgroup(fs_info, qgroup, src);
> -	else
> -		qgroup_rsv_release_by_qgroup(fs_info, qgroup, src);
> -
> -	qgroup_dirty(fs_info, qgroup);
> -
> -	/* Get all of the parent groups that contain this qgroup */
> -	list_for_each_entry(glist, &qgroup->groups, next_group) {
> -		ret = ulist_add(tmp, glist->group->qgroupid,
> -				qgroup_to_aux(glist->group), GFP_ATOMIC);
> -		if (ret < 0)
> -			goto out;
> -	}
> -
> -	/* Iterate all of the parents and adjust their reference counts */
> -	ULIST_ITER_INIT(&uiter);
> -	while ((unode = ulist_next(tmp, &uiter))) {
> -		qgroup = unode_aux_to_qgroup(unode);
>  		qgroup->rfer += sign * num_bytes;
>  		qgroup->rfer_cmpr += sign * num_bytes;
> +
>  		WARN_ON(sign < 0 && qgroup->excl < num_bytes);
>  		qgroup->excl += sign * num_bytes;
> +		qgroup->excl_cmpr += sign * num_bytes;
> +
>  		if (sign > 0)
>  			qgroup_rsv_add_by_qgroup(fs_info, qgroup, src);
>  		else
>  			qgroup_rsv_release_by_qgroup(fs_info, qgroup, src);
> -		qgroup->excl_cmpr += sign * num_bytes;
>  		qgroup_dirty(fs_info, qgroup);
>  
> -		/* Add any parents of the parents */
> -		list_for_each_entry(glist, &qgroup->groups, next_group) {
> -			ret = ulist_add(tmp, glist->group->qgroupid,
> -					qgroup_to_aux(glist->group), GFP_ATOMIC);
> -			if (ret < 0)
> -				goto out;
> -		}
> +		/* Append parent qgroups to @qgroup_list. */
> +		list_for_each_entry(glist, &qgroup->groups, next_group)
> +			qgroup_iterator_add(&qgroup_list, glist->group);
>  	}
>  	ret = 0;
>  out:
> +	qgroup_iterator_clean(&qgroup_list);
>  	return ret;
>  }
>  
> @@ -1479,8 +1454,7 @@ static int __qgroup_excl_accounting(struct btrfs_fs_info *fs_info,
>   * Return < 0 for other error.
>   */
>  static int quick_update_accounting(struct btrfs_fs_info *fs_info,
> -				   struct ulist *tmp, u64 src, u64 dst,
> -				   int sign)
> +				   u64 src, u64 dst, int sign)
>  {
>  	struct btrfs_qgroup *qgroup;
>  	int ret = 1;
> @@ -1491,8 +1465,7 @@ static int quick_update_accounting(struct btrfs_fs_info *fs_info,
>  		goto out;
>  	if (qgroup->excl == qgroup->rfer) {
>  		ret = 0;
> -		err = __qgroup_excl_accounting(fs_info, tmp, dst,
> -					       qgroup, sign);
> +		err = __qgroup_excl_accounting(fs_info, dst, qgroup, sign);
>  		if (err < 0) {
>  			ret = err;
>  			goto out;
> @@ -1511,7 +1484,6 @@ int btrfs_add_qgroup_relation(struct btrfs_trans_handle *trans, u64 src,
>  	struct btrfs_qgroup *parent;
>  	struct btrfs_qgroup *member;
>  	struct btrfs_qgroup_list *list;
> -	struct ulist *tmp;
>  	unsigned int nofs_flag;
>  	int ret = 0;
>  
> @@ -1521,10 +1493,7 @@ int btrfs_add_qgroup_relation(struct btrfs_trans_handle *trans, u64 src,
>  
>  	/* We hold a transaction handle open, must do a NOFS allocation. */
>  	nofs_flag = memalloc_nofs_save();
> -	tmp = ulist_alloc(GFP_KERNEL);
>  	memalloc_nofs_restore(nofs_flag);

You should have removed the memalloc_nofs_* calls too.

> -	if (!tmp)
> -		return -ENOMEM;
>  
>  	mutex_lock(&fs_info->qgroup_ioctl_lock);
>  	if (!fs_info->quota_root) {

> @@ -1585,11 +1552,7 @@ static int __del_qgroup_relation(struct btrfs_trans_handle *trans, u64 src,
>  
>  	/* We hold a transaction handle open, must do a NOFS allocation. */
>  	nofs_flag = memalloc_nofs_save();
> -	tmp = ulist_alloc(GFP_KERNEL);
>  	memalloc_nofs_restore(nofs_flag);

And here.

> -	if (!tmp)
> -		return -ENOMEM;
> -
>  	if (!fs_info->quota_root) {
>  		ret = -ENOTCONN;
>  		goto out;

[PATCH v2 5/6] btrfs: qgroup: use qgroup_iterator facility to replace @tmp ulist of qgroup_update_refcnt()

[Qu Wenruo]

For function qgroup_update_refcnt(), we use @tmp list to iterate all the
involved qgroups of a subvolume.

It's a perfect match for qgroup_iterator facility, as that @tmp ulist
has a very limited lifespan (just inside the while() loop).

By migrating to qgroup_iterator, we can get rid of the GFP_ATOMIC memory
allocation and no more error handling needed.

Reviewed-by: Boris Burkov <boris@bur.io>
Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/qgroup.c | 37 +++++++++++--------------------------
 1 file changed, 11 insertions(+), 26 deletions(-)

diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
index 08f4fc622180..6fcf302744e2 100644
--- a/fs/btrfs/qgroup.c
+++ b/fs/btrfs/qgroup.c
@@ -2463,13 +2463,11 @@ int btrfs_qgroup_trace_subtree(struct btrfs_trans_handle *trans,
  * Walk all of the roots that points to the bytenr and adjust their refcnts.
  */
 static int qgroup_update_refcnt(struct btrfs_fs_info *fs_info,
-				struct ulist *roots, struct ulist *tmp,
-				struct ulist *qgroups, u64 seq, int update_old)
+				struct ulist *roots, struct ulist *qgroups,
+				u64 seq, int update_old)
 {
 	struct ulist_node *unode;
 	struct ulist_iterator uiter;
-	struct ulist_node *tmp_unode;
-	struct ulist_iterator tmp_uiter;
 	struct btrfs_qgroup *qg;
 	int ret = 0;
 
@@ -2477,40 +2475,35 @@ static int qgroup_update_refcnt(struct btrfs_fs_info *fs_info,
 		return 0;
 	ULIST_ITER_INIT(&uiter);
 	while ((unode = ulist_next(roots, &uiter))) {
+		LIST_HEAD(tmp);
+
 		qg = find_qgroup_rb(fs_info, unode->val);
 		if (!qg)
 			continue;
 
-		ulist_reinit(tmp);
 		ret = ulist_add(qgroups, qg->qgroupid, qgroup_to_aux(qg),
 				GFP_ATOMIC);
 		if (ret < 0)
 			return ret;
-		ret = ulist_add(tmp, qg->qgroupid, qgroup_to_aux(qg), GFP_ATOMIC);
-		if (ret < 0)
-			return ret;
-		ULIST_ITER_INIT(&tmp_uiter);
-		while ((tmp_unode = ulist_next(tmp, &tmp_uiter))) {
+		qgroup_iterator_add(&tmp, qg);
+		list_for_each_entry(qg, &tmp, iterator) {
 			struct btrfs_qgroup_list *glist;
 
-			qg = unode_aux_to_qgroup(tmp_unode);
 			if (update_old)
 				btrfs_qgroup_update_old_refcnt(qg, seq, 1);
 			else
 				btrfs_qgroup_update_new_refcnt(qg, seq, 1);
+
 			list_for_each_entry(glist, &qg->groups, next_group) {
 				ret = ulist_add(qgroups, glist->group->qgroupid,
 						qgroup_to_aux(glist->group),
 						GFP_ATOMIC);
 				if (ret < 0)
 					return ret;
-				ret = ulist_add(tmp, glist->group->qgroupid,
-						qgroup_to_aux(glist->group),
-						GFP_ATOMIC);
-				if (ret < 0)
-					return ret;
+				qgroup_iterator_add(&tmp, glist->group);
 			}
 		}
+		qgroup_iterator_clean(&tmp);
 	}
 	return 0;
 }
@@ -2675,7 +2668,6 @@ int btrfs_qgroup_account_extent(struct btrfs_trans_handle *trans, u64 bytenr,
 {
 	struct btrfs_fs_info *fs_info = trans->fs_info;
 	struct ulist *qgroups = NULL;
-	struct ulist *tmp = NULL;
 	u64 seq;
 	u64 nr_new_roots = 0;
 	u64 nr_old_roots = 0;
@@ -2714,12 +2706,6 @@ int btrfs_qgroup_account_extent(struct btrfs_trans_handle *trans, u64 bytenr,
 		ret = -ENOMEM;
 		goto out_free;
 	}
-	tmp = ulist_alloc(GFP_NOFS);
-	if (!tmp) {
-		ret = -ENOMEM;
-		goto out_free;
-	}
-
 	mutex_lock(&fs_info->qgroup_rescan_lock);
 	if (fs_info->qgroup_flags & BTRFS_QGROUP_STATUS_FLAG_RESCAN) {
 		if (fs_info->qgroup_rescan_progress.objectid <= bytenr) {
@@ -2734,13 +2720,13 @@ int btrfs_qgroup_account_extent(struct btrfs_trans_handle *trans, u64 bytenr,
 	seq = fs_info->qgroup_seq;
 
 	/* Update old refcnts using old_roots */
-	ret = qgroup_update_refcnt(fs_info, old_roots, tmp, qgroups, seq,
+	ret = qgroup_update_refcnt(fs_info, old_roots, qgroups, seq,
 				   UPDATE_OLD);
 	if (ret < 0)
 		goto out;
 
 	/* Update new refcnts using new_roots */
-	ret = qgroup_update_refcnt(fs_info, new_roots, tmp, qgroups, seq,
+	ret = qgroup_update_refcnt(fs_info, new_roots, qgroups, seq,
 				   UPDATE_NEW);
 	if (ret < 0)
 		goto out;
@@ -2755,7 +2741,6 @@ int btrfs_qgroup_account_extent(struct btrfs_trans_handle *trans, u64 bytenr,
 out:
 	spin_unlock(&fs_info->qgroup_lock);
 out_free:
-	ulist_free(tmp);
 	ulist_free(qgroups);
 	ulist_free(old_roots);
 	ulist_free(new_roots);
-- 
2.41.0

Re: [PATCH v2 5/6] btrfs: qgroup: use qgroup_iterator facility to replace @tmp ulist of qgroup_update_refcnt()

[David Sterba]

Please don't put the @ in the variable references in the subjects.

On Thu, Aug 31, 2023 at 08:30:36AM +0800, Qu Wenruo wrote:
> For function qgroup_update_refcnt(), we use @tmp list to iterate all the
> involved qgroups of a subvolume.
> 
> It's a perfect match for qgroup_iterator facility, as that @tmp ulist
> has a very limited lifespan (just inside the while() loop).
> 
> By migrating to qgroup_iterator, we can get rid of the GFP_ATOMIC memory
> allocation and no more error handling needed.
> 
> Reviewed-by: Boris Burkov <boris@bur.io>
> Signed-off-by: Qu Wenruo <wqu@suse.com>

Re: [PATCH v2 5/6] btrfs: qgroup: use qgroup_iterator facility to replace @tmp ulist of qgroup_update_refcnt()

[Qu Wenruo]

On 2023/9/1 21:16, David Sterba wrote:
> Please don't put the @ in the variable references in the subjects.

Now removed, but I'm wondering what's the guideline for referring the
variable names in the commit message/subject line?

I didn't remember when but I have seen such "@" prefix for variable
names in the mailing list, thus I followed the scheme.

If there is a guideline it can help me from causing more damage in the
future.

Thanks,
Qu
>
> On Thu, Aug 31, 2023 at 08:30:36AM +0800, Qu Wenruo wrote:
>> For function qgroup_update_refcnt(), we use @tmp list to iterate all the
>> involved qgroups of a subvolume.
>>
>> It's a perfect match for qgroup_iterator facility, as that @tmp ulist
>> has a very limited lifespan (just inside the while() loop).
>>
>> By migrating to qgroup_iterator, we can get rid of the GFP_ATOMIC memory
>> allocation and no more error handling needed.
>>
>> Reviewed-by: Boris Burkov <boris@bur.io>
>> Signed-off-by: Qu Wenruo <wqu@suse.com>

Re: [PATCH v2 5/6] btrfs: qgroup: use qgroup_iterator facility to replace @tmp ulist of qgroup_update_refcnt()

[David Sterba]

On Sat, Sep 02, 2023 at 08:16:52AM +0800, Qu Wenruo wrote:
> 
> 
> On 2023/9/1 21:16, David Sterba wrote:
> > Please don't put the @ in the variable references in the subjects.
> 
> Now removed, but I'm wondering what's the guideline for referring the
> variable names in the commit message/subject line?

In subject it should be plain reference, unquoted and untagged.

> I didn't remember when but I have seen such "@" prefix for variable
> names in the mailing list, thus I followed the scheme.
>
> If there is a guideline it can help me from causing more damage in the
> future.

The guideline has been there for a few years at
https://archive.kernel.org/oldwiki/btrfs.wiki.kernel.org/index.php/Development_notes.html#Patches
after I reminded people to not do it, but this never sticks for long.

[PATCH v2 6/6] btrfs: qgroup: use qgroup_iterator2 facility to replace @qgroups ulist of qgroup_update_refcnt()

[Qu Wenruo]

The ulist @qgroups is utilized to record all involved qgroups from both
old and new roots inside btrfs_qgroup_account_extent().

Due to the fact that qgroup_update_refcnt() itself is already utilizing
qgroup_iterator, here we have to introduce another list_head,
btrfs_qgroup::iterator2, allowing nested iteartion.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/qgroup.c | 81 ++++++++++++++++++++++-------------------------
 fs/btrfs/qgroup.h |  3 ++
 2 files changed, 40 insertions(+), 44 deletions(-)

diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
index 6fcf302744e2..27debc645f97 100644
--- a/fs/btrfs/qgroup.c
+++ b/fs/btrfs/qgroup.c
@@ -217,6 +217,7 @@ static struct btrfs_qgroup *add_qgroup_rb(struct btrfs_fs_info *fs_info,
 	INIT_LIST_HEAD(&qgroup->members);
 	INIT_LIST_HEAD(&qgroup->dirty);
 	INIT_LIST_HEAD(&qgroup->iterator);
+	INIT_LIST_HEAD(&qgroup->iterator2);
 
 	rb_link_node(&qgroup->node, parent, p);
 	rb_insert_color(&qgroup->node, &fs_info->qgroup_tree);
@@ -2457,22 +2458,39 @@ int btrfs_qgroup_trace_subtree(struct btrfs_trans_handle *trans,
 	return ret;
 }
 
+static void qgroup_iterator2_add(struct list_head *head, struct btrfs_qgroup *qgroup)
+{
+	if (!list_empty(&qgroup->iterator2))
+		return;
+
+	list_add_tail(&qgroup->iterator2, head);
+}
+
+static void qgroup_iterator2_clean(struct list_head *head)
+{
+
+	while (!list_empty(head)) {
+		struct btrfs_qgroup *qgroup;
+
+		qgroup = list_first_entry(head, struct btrfs_qgroup, iterator2);
+		list_del_init(&qgroup->iterator2);
+	}
+}
 #define UPDATE_NEW	0
 #define UPDATE_OLD	1
 /*
  * Walk all of the roots that points to the bytenr and adjust their refcnts.
  */
-static int qgroup_update_refcnt(struct btrfs_fs_info *fs_info,
-				struct ulist *roots, struct ulist *qgroups,
-				u64 seq, int update_old)
+static void qgroup_update_refcnt(struct btrfs_fs_info *fs_info,
+				 struct ulist *roots, struct list_head *qgroups,
+				 u64 seq, int update_old)
 {
 	struct ulist_node *unode;
 	struct ulist_iterator uiter;
 	struct btrfs_qgroup *qg;
-	int ret = 0;
 
 	if (!roots)
-		return 0;
+		return;
 	ULIST_ITER_INIT(&uiter);
 	while ((unode = ulist_next(roots, &uiter))) {
 		LIST_HEAD(tmp);
@@ -2481,10 +2499,7 @@ static int qgroup_update_refcnt(struct btrfs_fs_info *fs_info,
 		if (!qg)
 			continue;
 
-		ret = ulist_add(qgroups, qg->qgroupid, qgroup_to_aux(qg),
-				GFP_ATOMIC);
-		if (ret < 0)
-			return ret;
+		qgroup_iterator2_add(qgroups, qg);
 		qgroup_iterator_add(&tmp, qg);
 		list_for_each_entry(qg, &tmp, iterator) {
 			struct btrfs_qgroup_list *glist;
@@ -2495,17 +2510,12 @@ static int qgroup_update_refcnt(struct btrfs_fs_info *fs_info,
 				btrfs_qgroup_update_new_refcnt(qg, seq, 1);
 
 			list_for_each_entry(glist, &qg->groups, next_group) {
-				ret = ulist_add(qgroups, glist->group->qgroupid,
-						qgroup_to_aux(glist->group),
-						GFP_ATOMIC);
-				if (ret < 0)
-					return ret;
+				qgroup_iterator2_add(qgroups, glist->group);
 				qgroup_iterator_add(&tmp, glist->group);
 			}
 		}
 		qgroup_iterator_clean(&tmp);
 	}
-	return 0;
 }
 
 /*
@@ -2544,22 +2554,18 @@ static int qgroup_update_refcnt(struct btrfs_fs_info *fs_info,
  * But this time we don't need to consider other things, the codes and logic
  * is easy to understand now.
  */
-static int qgroup_update_counters(struct btrfs_fs_info *fs_info,
-				  struct ulist *qgroups,
-				  u64 nr_old_roots,
-				  u64 nr_new_roots,
-				  u64 num_bytes, u64 seq)
+static void qgroup_update_counters(struct btrfs_fs_info *fs_info,
+				   struct list_head *qgroups,
+				   u64 nr_old_roots,
+				   u64 nr_new_roots,
+				   u64 num_bytes, u64 seq)
 {
-	struct ulist_node *unode;
-	struct ulist_iterator uiter;
 	struct btrfs_qgroup *qg;
-	u64 cur_new_count, cur_old_count;
 
-	ULIST_ITER_INIT(&uiter);
-	while ((unode = ulist_next(qgroups, &uiter))) {
+	list_for_each_entry(qg, qgroups, iterator2) {
+		u64 cur_new_count, cur_old_count;
 		bool dirty = false;
 
-		qg = unode_aux_to_qgroup(unode);
 		cur_old_count = btrfs_qgroup_get_old_refcnt(qg, seq);
 		cur_new_count = btrfs_qgroup_get_new_refcnt(qg, seq);
 
@@ -2630,7 +2636,6 @@ static int qgroup_update_counters(struct btrfs_fs_info *fs_info,
 		if (dirty)
 			qgroup_dirty(fs_info, qg);
 	}
-	return 0;
 }
 
 /*
@@ -2667,7 +2672,7 @@ int btrfs_qgroup_account_extent(struct btrfs_trans_handle *trans, u64 bytenr,
 				struct ulist *new_roots)
 {
 	struct btrfs_fs_info *fs_info = trans->fs_info;
-	struct ulist *qgroups = NULL;
+	LIST_HEAD(qgroups);
 	u64 seq;
 	u64 nr_new_roots = 0;
 	u64 nr_old_roots = 0;
@@ -2701,11 +2706,6 @@ int btrfs_qgroup_account_extent(struct btrfs_trans_handle *trans, u64 bytenr,
 	trace_btrfs_qgroup_account_extent(fs_info, trans->transid, bytenr,
 					num_bytes, nr_old_roots, nr_new_roots);
 
-	qgroups = ulist_alloc(GFP_NOFS);
-	if (!qgroups) {
-		ret = -ENOMEM;
-		goto out_free;
-	}
 	mutex_lock(&fs_info->qgroup_rescan_lock);
 	if (fs_info->qgroup_flags & BTRFS_QGROUP_STATUS_FLAG_RESCAN) {
 		if (fs_info->qgroup_rescan_progress.objectid <= bytenr) {
@@ -2720,28 +2720,21 @@ int btrfs_qgroup_account_extent(struct btrfs_trans_handle *trans, u64 bytenr,
 	seq = fs_info->qgroup_seq;
 
 	/* Update old refcnts using old_roots */
-	ret = qgroup_update_refcnt(fs_info, old_roots, qgroups, seq,
-				   UPDATE_OLD);
-	if (ret < 0)
-		goto out;
+	qgroup_update_refcnt(fs_info, old_roots, &qgroups, seq, UPDATE_OLD);
 
 	/* Update new refcnts using new_roots */
-	ret = qgroup_update_refcnt(fs_info, new_roots, qgroups, seq,
-				   UPDATE_NEW);
-	if (ret < 0)
-		goto out;
+	qgroup_update_refcnt(fs_info, new_roots, &qgroups, seq, UPDATE_NEW);
 
-	qgroup_update_counters(fs_info, qgroups, nr_old_roots, nr_new_roots,
+	qgroup_update_counters(fs_info, &qgroups, nr_old_roots, nr_new_roots,
 			       num_bytes, seq);
 
 	/*
 	 * Bump qgroup_seq to avoid seq overlap
 	 */
 	fs_info->qgroup_seq += max(nr_old_roots, nr_new_roots) + 1;
-out:
 	spin_unlock(&fs_info->qgroup_lock);
 out_free:
-	ulist_free(qgroups);
+	qgroup_iterator2_clean(&qgroups);
 	ulist_free(old_roots);
 	ulist_free(new_roots);
 	return ret;
diff --git a/fs/btrfs/qgroup.h b/fs/btrfs/qgroup.h
index 5dc0583622c3..1cce93585ff9 100644
--- a/fs/btrfs/qgroup.h
+++ b/fs/btrfs/qgroup.h
@@ -229,6 +229,9 @@ struct btrfs_qgroup {
 	 * And should be reset to empty after the iteration is finished.
 	 */
 	struct list_head iterator;
+
+	/* For nested iterator usage. */
+	struct list_head iterator2;
 	struct rb_node node;	  /* tree of qgroups */
 
 	/*
-- 
2.41.0

Re: [PATCH v2 6/6] btrfs: qgroup: use qgroup_iterator2 facility to replace @qgroups ulist of qgroup_update_refcnt()

[David Sterba]

On Thu, Aug 31, 2023 at 08:30:37AM +0800, Qu Wenruo wrote:
> The ulist @qgroups is utilized to record all involved qgroups from both
> old and new roots inside btrfs_qgroup_account_extent().
> 
> Due to the fact that qgroup_update_refcnt() itself is already utilizing
> qgroup_iterator, here we have to introduce another list_head,
> btrfs_qgroup::iterator2, allowing nested iteartion.

Please rename it to nested_iterator, the '2' is not very descriptive.

> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>  fs/btrfs/qgroup.c | 81 ++++++++++++++++++++++-------------------------
>  fs/btrfs/qgroup.h |  3 ++
>  2 files changed, 40 insertions(+), 44 deletions(-)
> 
> diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
> index 6fcf302744e2..27debc645f97 100644
> --- a/fs/btrfs/qgroup.c
> +++ b/fs/btrfs/qgroup.c
> @@ -217,6 +217,7 @@ static struct btrfs_qgroup *add_qgroup_rb(struct btrfs_fs_info *fs_info,
>  	INIT_LIST_HEAD(&qgroup->members);
>  	INIT_LIST_HEAD(&qgroup->dirty);
>  	INIT_LIST_HEAD(&qgroup->iterator);
> +	INIT_LIST_HEAD(&qgroup->iterator2);
>  
>  	rb_link_node(&qgroup->node, parent, p);
>  	rb_insert_color(&qgroup->node, &fs_info->qgroup_tree);
> @@ -2457,22 +2458,39 @@ int btrfs_qgroup_trace_subtree(struct btrfs_trans_handle *trans,
>  	return ret;
>  }
>  
> +static void qgroup_iterator2_add(struct list_head *head, struct btrfs_qgroup *qgroup)
> +{
> +	if (!list_empty(&qgroup->iterator2))
> +		return;
> +
> +	list_add_tail(&qgroup->iterator2, head);
> +}
> +
> +static void qgroup_iterator2_clean(struct list_head *head)
> +{
> +
> +	while (!list_empty(head)) {
> +		struct btrfs_qgroup *qgroup;
> +
> +		qgroup = list_first_entry(head, struct btrfs_qgroup, iterator2);
> +		list_del_init(&qgroup->iterator2);
> +	}
> +}
>  #define UPDATE_NEW	0
>  #define UPDATE_OLD	1
>  /*
>   * Walk all of the roots that points to the bytenr and adjust their refcnts.
>   */
> -static int qgroup_update_refcnt(struct btrfs_fs_info *fs_info,
> -				struct ulist *roots, struct ulist *qgroups,
> -				u64 seq, int update_old)
> +static void qgroup_update_refcnt(struct btrfs_fs_info *fs_info,
> +				 struct ulist *roots, struct list_head *qgroups,
> +				 u64 seq, int update_old)
>  {
>  	struct ulist_node *unode;
>  	struct ulist_iterator uiter;
>  	struct btrfs_qgroup *qg;
> -	int ret = 0;
>  
>  	if (!roots)
> -		return 0;
> +		return;
>  	ULIST_ITER_INIT(&uiter);
>  	while ((unode = ulist_next(roots, &uiter))) {
>  		LIST_HEAD(tmp);
> @@ -2481,10 +2499,7 @@ static int qgroup_update_refcnt(struct btrfs_fs_info *fs_info,
>  		if (!qg)
>  			continue;
>  
> -		ret = ulist_add(qgroups, qg->qgroupid, qgroup_to_aux(qg),
> -				GFP_ATOMIC);
> -		if (ret < 0)
> -			return ret;
> +		qgroup_iterator2_add(qgroups, qg);
>  		qgroup_iterator_add(&tmp, qg);
>  		list_for_each_entry(qg, &tmp, iterator) {
>  			struct btrfs_qgroup_list *glist;
> @@ -2495,17 +2510,12 @@ static int qgroup_update_refcnt(struct btrfs_fs_info *fs_info,
>  				btrfs_qgroup_update_new_refcnt(qg, seq, 1);
>  
>  			list_for_each_entry(glist, &qg->groups, next_group) {
> -				ret = ulist_add(qgroups, glist->group->qgroupid,
> -						qgroup_to_aux(glist->group),
> -						GFP_ATOMIC);
> -				if (ret < 0)
> -					return ret;
> +				qgroup_iterator2_add(qgroups, glist->group);
>  				qgroup_iterator_add(&tmp, glist->group);
>  			}
>  		}
>  		qgroup_iterator_clean(&tmp);
>  	}
> -	return 0;
>  }
>  
>  /*
> @@ -2544,22 +2554,18 @@ static int qgroup_update_refcnt(struct btrfs_fs_info *fs_info,
>   * But this time we don't need to consider other things, the codes and logic
>   * is easy to understand now.
>   */
> -static int qgroup_update_counters(struct btrfs_fs_info *fs_info,
> -				  struct ulist *qgroups,
> -				  u64 nr_old_roots,
> -				  u64 nr_new_roots,
> -				  u64 num_bytes, u64 seq)
> +static void qgroup_update_counters(struct btrfs_fs_info *fs_info,
> +				   struct list_head *qgroups,
> +				   u64 nr_old_roots,
> +				   u64 nr_new_roots,
> +				   u64 num_bytes, u64 seq)
>  {
> -	struct ulist_node *unode;
> -	struct ulist_iterator uiter;
>  	struct btrfs_qgroup *qg;
> -	u64 cur_new_count, cur_old_count;
>  
> -	ULIST_ITER_INIT(&uiter);
> -	while ((unode = ulist_next(qgroups, &uiter))) {
> +	list_for_each_entry(qg, qgroups, iterator2) {
> +		u64 cur_new_count, cur_old_count;
>  		bool dirty = false;
>  
> -		qg = unode_aux_to_qgroup(unode);
>  		cur_old_count = btrfs_qgroup_get_old_refcnt(qg, seq);
>  		cur_new_count = btrfs_qgroup_get_new_refcnt(qg, seq);
>  
> @@ -2630,7 +2636,6 @@ static int qgroup_update_counters(struct btrfs_fs_info *fs_info,
>  		if (dirty)
>  			qgroup_dirty(fs_info, qg);
>  	}
> -	return 0;
>  }
>  
>  /*
> @@ -2667,7 +2672,7 @@ int btrfs_qgroup_account_extent(struct btrfs_trans_handle *trans, u64 bytenr,
>  				struct ulist *new_roots)
>  {
>  	struct btrfs_fs_info *fs_info = trans->fs_info;
> -	struct ulist *qgroups = NULL;
> +	LIST_HEAD(qgroups);
>  	u64 seq;
>  	u64 nr_new_roots = 0;
>  	u64 nr_old_roots = 0;
> @@ -2701,11 +2706,6 @@ int btrfs_qgroup_account_extent(struct btrfs_trans_handle *trans, u64 bytenr,
>  	trace_btrfs_qgroup_account_extent(fs_info, trans->transid, bytenr,
>  					num_bytes, nr_old_roots, nr_new_roots);
>  
> -	qgroups = ulist_alloc(GFP_NOFS);
> -	if (!qgroups) {
> -		ret = -ENOMEM;
> -		goto out_free;
> -	}
>  	mutex_lock(&fs_info->qgroup_rescan_lock);
>  	if (fs_info->qgroup_flags & BTRFS_QGROUP_STATUS_FLAG_RESCAN) {
>  		if (fs_info->qgroup_rescan_progress.objectid <= bytenr) {
> @@ -2720,28 +2720,21 @@ int btrfs_qgroup_account_extent(struct btrfs_trans_handle *trans, u64 bytenr,
>  	seq = fs_info->qgroup_seq;
>  
>  	/* Update old refcnts using old_roots */
> -	ret = qgroup_update_refcnt(fs_info, old_roots, qgroups, seq,
> -				   UPDATE_OLD);
> -	if (ret < 0)
> -		goto out;
> +	qgroup_update_refcnt(fs_info, old_roots, &qgroups, seq, UPDATE_OLD);
>  
>  	/* Update new refcnts using new_roots */
> -	ret = qgroup_update_refcnt(fs_info, new_roots, qgroups, seq,
> -				   UPDATE_NEW);
> -	if (ret < 0)
> -		goto out;
> +	qgroup_update_refcnt(fs_info, new_roots, &qgroups, seq, UPDATE_NEW);
>  
> -	qgroup_update_counters(fs_info, qgroups, nr_old_roots, nr_new_roots,
> +	qgroup_update_counters(fs_info, &qgroups, nr_old_roots, nr_new_roots,
>  			       num_bytes, seq);
>  
>  	/*
>  	 * Bump qgroup_seq to avoid seq overlap
>  	 */
>  	fs_info->qgroup_seq += max(nr_old_roots, nr_new_roots) + 1;
> -out:
>  	spin_unlock(&fs_info->qgroup_lock);
>  out_free:
> -	ulist_free(qgroups);
> +	qgroup_iterator2_clean(&qgroups);
>  	ulist_free(old_roots);
>  	ulist_free(new_roots);
>  	return ret;
> diff --git a/fs/btrfs/qgroup.h b/fs/btrfs/qgroup.h
> index 5dc0583622c3..1cce93585ff9 100644
> --- a/fs/btrfs/qgroup.h
> +++ b/fs/btrfs/qgroup.h
> @@ -229,6 +229,9 @@ struct btrfs_qgroup {
>  	 * And should be reset to empty after the iteration is finished.
>  	 */
>  	struct list_head iterator;
> +
> +	/* For nested iterator usage. */

Please expand the comment e.g. describing in which context it's used or
why it must be used instead of the other iterator.

> +	struct list_head iterator2;
>  	struct rb_node node;	  /* tree of qgroups */
>  
>  	/*
> -- 
> 2.41.0