[PATCH 0/5] btrfs: qgroup: reduce GFP_ATOMIC usage for ulist

[PATCH 0/5] btrfs: qgroup: reduce GFP_ATOMIC usage for ulist

[Qu Wenruo]

[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 (5):
  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()

 fs/btrfs/qgroup.c | 252 ++++++++++++++++------------------------------
 fs/btrfs/qgroup.h |   9 ++
 2 files changed, 94 insertions(+), 167 deletions(-)

-- 
2.41.0

[PATCH 1/5] 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.

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

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

[Boris Burkov]

On Wed, Aug 30, 2023 at 06:37:23PM +0800, Qu Wenruo wrote:
> 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.

This looks great, thanks! I like it more than my array/ulist hybrid
since it never allocates :)

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.

Can you also add a note that this relies on global exclusion under the
qgroup spin lock?

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

[PATCH 2/5] 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.

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 3/5] 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.

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 4/5] 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.

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

[PATCH 5/5] 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.

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 5/5] btrfs: qgroup: use qgroup_iterator facility to replace @tmp ulist of qgroup_update_refcnt()

[Boris Burkov]

On Wed, Aug 30, 2023 at 06:37:27PM +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.
> 
> 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);

Thinking out loud, could we use the same trick and put another global
list on the qgroups to handle this one? Or some other "single global
allocation up to #qgroups" trick.

>  				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 5/5] btrfs: qgroup: use qgroup_iterator facility to replace @tmp ulist of qgroup_update_refcnt()

[Qu Wenruo]

On 2023/8/31 06:09, Boris Burkov wrote:
> On Wed, Aug 30, 2023 at 06:37:27PM +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.
>>
>> 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);
>
> Thinking out loud, could we use the same trick and put another global
> list on the qgroups to handle this one? Or some other "single global
> allocation up to #qgroups" trick.

I'm considering this as the last resort method.

Currently I tried to move this qgroups collecting code into one dedicate
function other than doing two jobs inside one function.

But that idea doesn't work as expected, I'm hoping to at least
understand why before adding a new list_head.

Thanks,
Qu

>
>>   				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 5/5] btrfs: qgroup: use qgroup_iterator facility to replace @tmp ulist of qgroup_update_refcnt()

[David Sterba]

Regarding style guidelines, the subject should not be overly long, like
in this patch, "btrfs: qgroup: use qgroup_iterator in qgroup_update_refcnt"
would work too.

Re: [PATCH 0/5] btrfs: qgroup: reduce GFP_ATOMIC usage for ulist

[Boris Burkov]

On Wed, Aug 30, 2023 at 06:37:22PM +0800, Qu Wenruo wrote:
> [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.

These all LGTM, thanks!

Reviewed-by: Boris Burkov <boris@bur.io>

> 
> Qu Wenruo (5):
>   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()
> 
>  fs/btrfs/qgroup.c | 252 ++++++++++++++++------------------------------
>  fs/btrfs/qgroup.h |   9 ++
>  2 files changed, 94 insertions(+), 167 deletions(-)
> 
> -- 
> 2.41.0
>