linux-kernel - Re: [RFC PATCH 8/8] btrfs: simplify cleanup in btrfs_add_qgroup

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: <ec7e5f41-6585-4817-a901-e7a1e7e7b760@gmx.com>
Date: Thu, 13 Nov 2025 07:16:41 +1030
From: Qu Wenruo <quwenruo.btrfs@....com>
To: Gladyshev Ilya <foxido@...ido.dev>
Cc: Chris Mason <clm@...com>, David Sterba <dsterba@...e.com>,
 linux-btrfs@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH 8/8] btrfs: simplify cleanup in
 btrfs_add_qgroup_relation



在 2025/11/13 05:19, Gladyshev Ilya 写道:
> Remove from cleanup path mutex_unlock via guard() and kfree via
> __free(kfree) macro. With those two cleanups gone, we can remove `out`
> label and replace all gotos with direct returns.
> 
> Signed-off-by: Gladyshev Ilya <foxido@...ido.dev>
> ---
>   fs/btrfs/qgroup.c | 42 ++++++++++++++++--------------------------
>   1 file changed, 16 insertions(+), 26 deletions(-)
> 
> diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
> index 9b2f2c8ca505..238c17c7d969 100644
> --- a/fs/btrfs/qgroup.c
> +++ b/fs/btrfs/qgroup.c
> @@ -1528,65 +1528,55 @@ static int quick_update_accounting(struct btrfs_fs_info *fs_info,
>    * callers and transferred here (either used or freed on error).
>    */
>   int btrfs_add_qgroup_relation(struct btrfs_trans_handle *trans, u64 src, u64 dst,
> -			      struct btrfs_qgroup_list *prealloc)
> +			      struct btrfs_qgroup_list *_prealloc)

We don't use '_' prefix. Even '__' usage is discouraged.

>   {
>   	struct btrfs_fs_info *fs_info = trans->fs_info;
>   	struct btrfs_qgroup *parent;
>   	struct btrfs_qgroup *member;
>   	struct btrfs_qgroup_list *list;
> +	struct btrfs_qgroup_list *prealloc __free(kfree) = _prealloc;

I do not think preallocation is a good use case for scope based cleanup, 
especially when there are two similarly named variables.

This reduces the readability.

Thanks,
Qu

>   	int ret = 0;
>   
>   	ASSERT(prealloc);
>   
>   	/* Check the level of src and dst first */
>   	if (btrfs_qgroup_level(src) >= btrfs_qgroup_level(dst)) {
> -		kfree(prealloc);
>   		return -EINVAL;
>   	}
>   
> -	mutex_lock(&fs_info->qgroup_ioctl_lock);
> -	if (!fs_info->quota_root) {
> -		ret = -ENOTCONN;
> -		goto out;
> -	}
> +	guard(mutex)(&fs_info->qgroup_ioctl_lock);
> +
> +	if (!fs_info->quota_root)
> +		return -ENOTCONN;
> +
>   	member = find_qgroup_rb(fs_info, src);
>   	parent = find_qgroup_rb(fs_info, dst);
> -	if (!member || !parent) {
> -		ret = -EINVAL;
> -		goto out;
> -	}
> +	if (!member || !parent)
> +		return -EINVAL;
>   
>   	/* check if such qgroup relation exist firstly */
>   	list_for_each_entry(list, &member->groups, next_group) {
> -		if (list->group == parent) {
> -			ret = -EEXIST;
> -			goto out;
> -		}
> +		if (list->group == parent)
> +			return -EEXIST;
>   	}
>   
>   	ret = add_qgroup_relation_item(trans, src, dst);
>   	if (ret)
> -		goto out;
> +		return ret;
>   
>   	ret = add_qgroup_relation_item(trans, dst, src);
>   	if (ret) {
>   		del_qgroup_relation_item(trans, src, dst);
> -		goto out;
> +		return ret;
>   	}
>   
> -	spin_lock(&fs_info->qgroup_lock);
> +	guard(spinlock)(&fs_info->qgroup_lock);
>   	ret = __add_relation_rb(prealloc, member, parent);
>   	prealloc = NULL;
>   	if (ret < 0) {
> -		spin_unlock(&fs_info->qgroup_lock);
> -		goto out;
> +		return ret;
>   	}
> -	ret = quick_update_accounting(fs_info, src, dst, 1);
> -	spin_unlock(&fs_info->qgroup_lock);
> -out:
> -	kfree(prealloc);
> -	mutex_unlock(&fs_info->qgroup_ioctl_lock);
> -	return ret;
> +	return quick_update_accounting(fs_info, src, dst, 1);
>   }
>   
>   static int __del_qgroup_relation(struct btrfs_trans_handle *trans, u64 src,