| Message ID | 20250409125724.145597-1-frank.li@vivo.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] btrfs: convert to spinlock guards in btrfs_update_ioctl_balance_args() | expand |
On Wed, Apr 09, 2025 at 06:57:22AM -0600, Yangtao Li wrote: > To simplify handling, use the guard helper to let the compiler care for > unlocking. > > Signed-off-by: Yangtao Li <frank.li@vivo.com> > --- > fs/btrfs/ioctl.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c > index 63aeacc54945..7cec105a4cb0 100644 > --- a/fs/btrfs/ioctl.c > +++ b/fs/btrfs/ioctl.c > @@ -3438,9 +3438,8 @@ void btrfs_update_ioctl_balance_args(struct btrfs_fs_info *fs_info, > memcpy(&bargs->meta, &bctl->meta, sizeof(bargs->meta)); > memcpy(&bargs->sys, &bctl->sys, sizeof(bargs->sys)); > > - spin_lock(&fs_info->balance_lock); > + guard(spinlock)(&fs_info->balance_lock); 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. I personally hate it as it totally disrupts the perception of visibly demarcated critical sections. I don't see the benefit comparing saved lines of code vs clear and understandable code. We rarely have a clear case for the guards, I found a few where it could be used but then this means we have 2 styles of locking, yet another code pattern to learn.
> 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?
Thx,
Yangtao
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.
diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c index 63aeacc54945..7cec105a4cb0 100644 --- a/fs/btrfs/ioctl.c +++ b/fs/btrfs/ioctl.c @@ -3438,9 +3438,8 @@ void btrfs_update_ioctl_balance_args(struct btrfs_fs_info *fs_info, memcpy(&bargs->meta, &bctl->meta, sizeof(bargs->meta)); memcpy(&bargs->sys, &bctl->sys, sizeof(bargs->sys)); - spin_lock(&fs_info->balance_lock); + guard(spinlock)(&fs_info->balance_lock); memcpy(&bargs->stat, &bctl->stat, sizeof(bargs->stat)); - spin_unlock(&fs_info->balance_lock); } /*
To simplify handling, use the guard helper to let the compiler care for unlocking. Signed-off-by: Yangtao Li <frank.li@vivo.com> --- fs/btrfs/ioctl.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)