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

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

[Yangtao Li]

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);
 	memcpy(&bargs->stat, &bctl->stat, sizeof(bargs->stat));
-	spin_unlock(&fs_info->balance_lock);
 }
 
 /*
-- 
2.39.0

[PATCH 2/2] btrfs: convert to mutex guard in btrfs_ioctl_balance_progress()

[Yangtao Li]

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 | 20 ++++++++------------
 1 file changed, 8 insertions(+), 12 deletions(-)

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 7cec105a4cb0..1d8c28aa84d2 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -3620,22 +3620,19 @@ static long btrfs_ioctl_balance_progress(struct btrfs_fs_info *fs_info,
 					 void __user *arg)
 {
 	struct btrfs_ioctl_balance_args *bargs;
-	int ret = 0;
+	int ret;
 
 	if (!capable(CAP_SYS_ADMIN))
 		return -EPERM;
 
-	mutex_lock(&fs_info->balance_mutex);
-	if (!fs_info->balance_ctl) {
-		ret = -ENOTCONN;
-		goto out;
-	}
+	guard(mutex)(&fs_info->balance_mutex);
+
+	if (!fs_info->balance_ctl)
+		return -ENOTCONN;
 
 	bargs = kzalloc(sizeof(*bargs), GFP_KERNEL);
-	if (!bargs) {
-		ret = -ENOMEM;
-		goto out;
-	}
+	if (!bargs)
+		return -ENOMEM;
 
 	btrfs_update_ioctl_balance_args(fs_info, bargs);
 
@@ -3643,8 +3640,7 @@ static long btrfs_ioctl_balance_progress(struct btrfs_fs_info *fs_info,
 		ret = -EFAULT;
 
 	kfree(bargs);
-out:
-	mutex_unlock(&fs_info->balance_mutex);
+
 	return ret;
 }
 
-- 
2.39.0

Re: [PATCH 2/2] btrfs: convert to mutex guard in btrfs_ioctl_balance_progress()

[kernel test robot]

Hi Yangtao,

kernel test robot noticed the following build warnings:

[auto build test WARNING on kdave/for-next]
[also build test WARNING on linus/master v6.15-rc1 next-20250410]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Yangtao-Li/btrfs-convert-to-mutex-guard-in-btrfs_ioctl_balance_progress/20250409-204204
base:   https://git.kernel.org/pub/scm/linux/kernel/git/kdave/linux.git for-next
patch link:    https://lore.kernel.org/r/20250409125724.145597-2-frank.li%40vivo.com
patch subject: [PATCH 2/2] btrfs: convert to mutex guard in btrfs_ioctl_balance_progress()
config: arm-randconfig-001-20250410 (https://download.01.org/0day-ci/archive/20250410/202504102206.gpAU9chA-lkp@intel.com/config)
compiler: clang version 21.0.0git (https://github.com/llvm/llvm-project f819f46284f2a79790038e1f6649172789734ae8)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250410/202504102206.gpAU9chA-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202504102206.gpAU9chA-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> fs/btrfs/ioctl.c:3639:6: warning: variable 'ret' is used uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized]
    3639 |         if (copy_to_user(arg, bargs, sizeof(*bargs)))
         |             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   fs/btrfs/ioctl.c:3644:9: note: uninitialized use occurs here
    3644 |         return ret;
         |                ^~~
   fs/btrfs/ioctl.c:3639:2: note: remove the 'if' if its condition is always true
    3639 |         if (copy_to_user(arg, bargs, sizeof(*bargs)))
         |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    3640 |                 ret = -EFAULT;
   fs/btrfs/ioctl.c:3623:9: note: initialize the variable 'ret' to silence this warning
    3623 |         int ret;
         |                ^
         |                 = 0
   1 warning generated.


vim +3639 fs/btrfs/ioctl.c

837d5b6e46d1a4 Ilya Dryomov 2012-01-16  3618  
2ff7e61e0d30ff Jeff Mahoney 2016-06-22  3619  static long btrfs_ioctl_balance_progress(struct btrfs_fs_info *fs_info,
19a39dce3b9bf0 Ilya Dryomov 2012-01-16  3620  					 void __user *arg)
19a39dce3b9bf0 Ilya Dryomov 2012-01-16  3621  {
19a39dce3b9bf0 Ilya Dryomov 2012-01-16  3622  	struct btrfs_ioctl_balance_args *bargs;
68395a7bf00486 Yangtao Li   2025-04-09  3623  	int ret;
19a39dce3b9bf0 Ilya Dryomov 2012-01-16  3624  
19a39dce3b9bf0 Ilya Dryomov 2012-01-16  3625  	if (!capable(CAP_SYS_ADMIN))
19a39dce3b9bf0 Ilya Dryomov 2012-01-16  3626  		return -EPERM;
19a39dce3b9bf0 Ilya Dryomov 2012-01-16  3627  
68395a7bf00486 Yangtao Li   2025-04-09  3628  	guard(mutex)(&fs_info->balance_mutex);
68395a7bf00486 Yangtao Li   2025-04-09  3629  
68395a7bf00486 Yangtao Li   2025-04-09  3630  	if (!fs_info->balance_ctl)
68395a7bf00486 Yangtao Li   2025-04-09  3631  		return -ENOTCONN;
19a39dce3b9bf0 Ilya Dryomov 2012-01-16  3632  
8d2db7855e7b65 David Sterba 2015-11-04  3633  	bargs = kzalloc(sizeof(*bargs), GFP_KERNEL);
68395a7bf00486 Yangtao Li   2025-04-09  3634  	if (!bargs)
68395a7bf00486 Yangtao Li   2025-04-09  3635  		return -ENOMEM;
19a39dce3b9bf0 Ilya Dryomov 2012-01-16  3636  
008ef0969dd966 David Sterba 2018-03-21  3637  	btrfs_update_ioctl_balance_args(fs_info, bargs);
19a39dce3b9bf0 Ilya Dryomov 2012-01-16  3638  
19a39dce3b9bf0 Ilya Dryomov 2012-01-16 @3639  	if (copy_to_user(arg, bargs, sizeof(*bargs)))
19a39dce3b9bf0 Ilya Dryomov 2012-01-16  3640  		ret = -EFAULT;
19a39dce3b9bf0 Ilya Dryomov 2012-01-16  3641  
19a39dce3b9bf0 Ilya Dryomov 2012-01-16  3642  	kfree(bargs);
68395a7bf00486 Yangtao Li   2025-04-09  3643  
19a39dce3b9bf0 Ilya Dryomov 2012-01-16  3644  	return ret;
19a39dce3b9bf0 Ilya Dryomov 2012-01-16  3645  }
19a39dce3b9bf0 Ilya Dryomov 2012-01-16  3646  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

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

[David Sterba]

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.

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

[李扬韬]

> 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

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.

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

[李扬韬]

> 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.

OK, thank you very much for the detailed idea!

I will spend time on this.

MBR,
Yangtao