linux-kernel - Re: 回复: [PATCH 1/2] btrfs: convert to spinlock guards in btrfs_update_ioctl_balance

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20250415175307.GK16750@twin.jikos.cz>
Date: Tue, 15 Apr 2025 19:53:07 +0200
From: David Sterba <dsterba@...e.cz>
To: 李扬韬 <frank.li@...o.com>
Cc: "dsterba@...e.cz" <dsterba@...e.cz>, "clm@...com" <clm@...com>,
	"josef@...icpanda.com" <josef@...icpanda.com>,
	"dsterba@...e.com" <dsterba@...e.com>,
	"linux-btrfs@...r.kernel.org" <linux-btrfs@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: 回复: [PATCH 1/2] btrfs: convert to spinlock guards in
 btrfs_update_ioctl_balance_args()

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.