diff mbox series

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

Message ID ce02f493bf56e540679db37393a9ca243b20c012.1693441298.git.wqu@suse.com (mailing list archive)
State New, archived
Headers show
Series btrfs: qgroup: remove GFP_ATOMIC usage for ulist | expand

Commit Message

Qu Wenruo Aug. 31, 2023, 12:30 a.m. UTC 
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(-)

Comments

David Sterba Sept. 1, 2023, 1:16 p.m. UTC | #1 
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>
Qu Wenruo Sept. 2, 2023, 12:16 a.m. UTC | #2 
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>
David Sterba Sept. 5, 2023, 1:02 p.m. UTC | #3 
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.
diff mbox series

Patch

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);