Re: 回复: [PATCH 1/2] btrfs: convert to spinlock guards in btrfs_update_ioctl_balance_args()

[David Sterba]

On Thu, Apr 10, 2025 at 11:11:11AM +0000, 李扬韬 wrote:
> > Please don't do the guard() conversions in fs/btrfs/, the explicit locking is the preferred style. If other subsystems use the scoped locking guards then let them do it.
> 
> OK, is there anything we can do quickly in the btrfs code currently?

Yeah, there always is. The open coded rb_tree searches can be converted
to the rb_find() helpers. It ends up as the same asm code due to
inlining and reads a bit better when there's just one rb_find instead of
the while loop and left/right tree moves.

I have some WIP for that but I havent't tested it at all, but it should
give a good idea:

--- a/fs/btrfs/qgroup.c
+++ b/fs/btrfs/qgroup.c
@@ -160,23 +160,27 @@ qgroup_rescan_init(struct btrfs_fs_info *fs_info, u64 progress_objectid,
 		   int init_flags);
 static void qgroup_rescan_zero_tracking(struct btrfs_fs_info *fs_info);
 
+static int qgroup_qgroupid_cmp(const void *key, const struct rb_node *node)
+{
+	const u64 *qgroupid = key;
+	const struct btrfs_qgroup *qgroup = rb_entry(n, struct btrfs_qgroup, node);
+
+	if (qgroup->qgroupid < qgroupid)
+		return -1;
+	else if (qgroup->qgroupid > qgroupid)
+		return 1;
+	return 0;
+}
+
 /* must be called with qgroup_ioctl_lock held */
 static struct btrfs_qgroup *find_qgroup_rb(const struct btrfs_fs_info *fs_info,
 					   u64 qgroupid)
 {
-	struct rb_node *n = fs_info->qgroup_tree.rb_node;
-	struct btrfs_qgroup *qgroup;
+	struct rb_node *node;
 
-	while (n) {
-		qgroup = rb_entry(n, struct btrfs_qgroup, node);
-		if (qgroup->qgroupid < qgroupid)
-			n = n->rb_left;
-		else if (qgroup->qgroupid > qgroupid)
-			n = n->rb_right;
-		else
-			return qgroup;
-	}
-	return NULL;
+	node = rb_find(&qgroupid, fs_info->qgroup_tree, qgroup_qgroupid_cmp);
+
+	return rb_entry_safe(n, struct btrfs_qgroup, node);
 }
 
 /*
---

So basically:
- add a comparator function
- replace the while loop with rb_find
- make sure it is equivalent and test it

There are easy conversions like __btrfs_lookup_delayed_item(),
potentially convertible too but with a comparator that takes an extra
parameter like prelim_ref_compare() or compare_inode_defrag(). There are
many more that do some additional things like remembering the last
parent pointer and for that the rb-tree API is not convenient so you can
skip that and do the straigtforward cases first.

If you decide not to take it then it's also fine, it's a cleanup and the
code will work as-is.