Re: [PATCH 1/3] btrfs: cleanup raid56_parity_write

From: Qu Wenruo <quwenruo.btrfs@gmx.com>
To: Christoph Hellwig <hch@lst.de>, Chris Mason <clm@fb.com>,
	Josef Bacik <josef@toxicpanda.com>,
	David Sterba <dsterba@suse.com>
Cc: Qu Wenruo <wqu@suse.com>, linux-btrfs@vger.kernel.org
Subject: Re: [PATCH 1/3] btrfs: cleanup raid56_parity_write
Date: Mon, 12 Dec 2022 15:34:07 +0800	[thread overview]
Message-ID: <0b8647ed-a987-4648-630e-a544888420dc@gmx.com> (raw)
In-Reply-To: <20221212070611.5209-2-hch@lst.de>

On 2022/12/12 15:06, Christoph Hellwig wrote:
> Handle the error return on alloc_rbio failure directly instead of using
> a goto, and remove the queue_rbio goto label by moving the plugged
> check into the if branch.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Reviewed-by: Qu Wenruo <wqu@suse.com>

I originally thought the goto way would save some lines of code, but it 
turns out to be the opposite.

Thanks,
Qu

> ---
>   fs/btrfs/raid56.c | 37 +++++++++++++++----------------------
>   1 file changed, 15 insertions(+), 22 deletions(-)
> 
> diff --git a/fs/btrfs/raid56.c b/fs/btrfs/raid56.c
> index 2d90a6b5eb00e3..5603ba1af55584 100644
> --- a/fs/btrfs/raid56.c
> +++ b/fs/btrfs/raid56.c
> @@ -1660,12 +1660,12 @@ void raid56_parity_write(struct bio *bio, struct btrfs_io_context *bioc)
>   	struct btrfs_raid_bio *rbio;
>   	struct btrfs_plug_cb *plug = NULL;
>   	struct blk_plug_cb *cb;
> -	int ret = 0;
>   
>   	rbio = alloc_rbio(fs_info, bioc);
>   	if (IS_ERR(rbio)) {
> -		ret = PTR_ERR(rbio);
> -		goto fail;
> +		bio->bi_status = errno_to_blk_status(PTR_ERR(rbio));
> +		bio_endio(bio);
> +		return;
>   	}
>   	rbio->operation = BTRFS_RBIO_WRITE;
>   	rbio_add_bio(rbio, bio);
> @@ -1674,31 +1674,24 @@ void raid56_parity_write(struct bio *bio, struct btrfs_io_context *bioc)
>   	 * Don't plug on full rbios, just get them out the door
>   	 * as quickly as we can
>   	 */
> -	if (rbio_is_full(rbio))
> -		goto queue_rbio;
> -
> -	cb = blk_check_plugged(raid_unplug, fs_info, sizeof(*plug));
> -	if (cb) {
> -		plug = container_of(cb, struct btrfs_plug_cb, cb);
> -		if (!plug->info) {
> -			plug->info = fs_info;
> -			INIT_LIST_HEAD(&plug->rbio_list);
> +	if (!rbio_is_full(rbio)) {
> +		cb = blk_check_plugged(raid_unplug, fs_info, sizeof(*plug));
> +		if (cb) {
> +			plug = container_of(cb, struct btrfs_plug_cb, cb);
> +			if (!plug->info) {
> +				plug->info = fs_info;
> +				INIT_LIST_HEAD(&plug->rbio_list);
> +			}
> +			list_add_tail(&rbio->plug_list, &plug->rbio_list);
> +			return;
>   		}
> -		list_add_tail(&rbio->plug_list, &plug->rbio_list);
> -		return;
>   	}
> -queue_rbio:
> +
>   	/*
>   	 * Either we don't have any existing plug, or we're doing a full stripe,
> -	 * can queue the rmw work now.
> +	 * queue the rmw work now.
>   	 */
>   	start_async_work(rbio, rmw_rbio_work);
> -
> -	return;
> -
> -fail:
> -	bio->bi_status = errno_to_blk_status(ret);
> -	bio_endio(bio);
>   }
>   
>   static int verify_one_sector(struct btrfs_raid_bio *rbio,