linux-kernel - Re: [PATCH] btrfs: reuse exit helper in btrfs_bioset

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] [day] [month] [year] [list]

Message-ID: <20250414190321.GF16750@suse.cz>
Date: Mon, 14 Apr 2025 21:03:21 +0200
From: David Sterba <dsterba@...e.cz>
To: Yangtao Li <frank.li@...o.com>
Cc: clm@...com, josef@...icpanda.com, dsterba@...e.com,
	linux-btrfs@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] btrfs: reuse exit helper in btrfs_bioset_init()

On Mon, Apr 14, 2025 at 06:44:01AM -0600, Yangtao Li wrote:
> As David Sterba said before:
> 
>   This is partially duplicating btrfs_delayed_ref_exit(), I'd rather reuse
>   the exit helper.
> 
>   I've checked if this can be done elsewhere, seems that there's only one
>   other case btrfs_bioset_init(), which is partially duplicating
>   btrfs_bioset_exit(). All other init/exit functions are trivial and
>   allocate one structure. So if you want to do that cleanup, please update
>   btrfs_bioset_init() to the preferred pattern. Thanks.

Please write the changelogs as standalone text without the references or
copied text from some suggestions.

Mentions, credits or Suggested-by make most sense if there's some
groundbreaking idea implemented and not mentioning the author would be
percieved as stealing it. Otherwise, suggestions are part of the
review process and should be transformed into useful text in the
changelog.

> So let's convert it.
> 
> Signed-off-by: Yangtao Li <frank.li@...o.com>
> ---
>  fs/btrfs/bio.c | 30 +++++++++++++-----------------
>  1 file changed, 13 insertions(+), 17 deletions(-)
> 
> diff --git a/fs/btrfs/bio.c b/fs/btrfs/bio.c
> index 8c2eee1f1878..f6f84837d62b 100644
> --- a/fs/btrfs/bio.c
> +++ b/fs/btrfs/bio.c
> @@ -892,6 +892,14 @@ void btrfs_submit_repair_write(struct btrfs_bio *bbio, int mirror_num, bool dev_
>  	btrfs_bio_end_io(bbio, errno_to_blk_status(ret));
>  }
>  
> +void __cold btrfs_bioset_exit(void)
> +{
> +	mempool_exit(&btrfs_failed_bio_pool);
> +	bioset_exit(&btrfs_repair_bioset);
> +	bioset_exit(&btrfs_clone_bioset);
> +	bioset_exit(&btrfs_bioset);
> +}

This function is public and you don't need to move it.

> +
>  int __init btrfs_bioset_init(void)
>  {
>  	if (bioset_init(&btrfs_bioset, BIO_POOL_SIZE,
> @@ -900,29 +908,17 @@ int __init btrfs_bioset_init(void)
>  		return -ENOMEM;
>  	if (bioset_init(&btrfs_clone_bioset, BIO_POOL_SIZE,
>  			offsetof(struct btrfs_bio, bio), 0))
> -		goto out_free_bioset;
> +		goto out;
>  	if (bioset_init(&btrfs_repair_bioset, BIO_POOL_SIZE,
>  			offsetof(struct btrfs_bio, bio),
>  			BIOSET_NEED_BVECS))
> -		goto out_free_clone_bioset;
> +		goto out;
>  	if (mempool_init_kmalloc_pool(&btrfs_failed_bio_pool, BIO_POOL_SIZE,
>  				      sizeof(struct btrfs_failed_bio)))
> -		goto out_free_repair_bioset;
> +		goto out;
>  	return 0;
>  
> -out_free_repair_bioset:
> -	bioset_exit(&btrfs_repair_bioset);
> -out_free_clone_bioset:
> -	bioset_exit(&btrfs_clone_bioset);
> -out_free_bioset:
> -	bioset_exit(&btrfs_bioset);
> +out:
> +	btrfs_bioset_exit();
>  	return -ENOMEM;
>  }
> -
> -void __cold btrfs_bioset_exit(void)
> -{
> -	mempool_exit(&btrfs_failed_bio_pool);
> -	bioset_exit(&btrfs_repair_bioset);
> -	bioset_exit(&btrfs_clone_bioset);
> -	bioset_exit(&btrfs_bioset);
> -}
> -- 
> 2.39.0
>