Re: [PATCH 5/8] btrfs: cleanup scrub_rbio

[Qu Wenruo <quwenruo.btrfs@gmx.com>]

On 2022/12/13 16:41, Christoph Hellwig wrote:
> The bio_list is only filled by scrub_assemble_read_bios when
> successful, so don't try to walk it and put the bios on any
> failure before the successful call to recover_assemble_read_bios.
> Also initialize bio_list at initialization time.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>   fs/btrfs/raid56.c | 21 ++++++++-------------
>   1 file changed, 8 insertions(+), 13 deletions(-)
> 
> diff --git a/fs/btrfs/raid56.c b/fs/btrfs/raid56.c
> index 5dab0685e17dd5..2a5857d42fff20 100644
> --- a/fs/btrfs/raid56.c
> +++ b/fs/btrfs/raid56.c
> @@ -2754,31 +2754,32 @@ static int scrub_assemble_read_bios(struct btrfs_raid_bio *rbio,
>   
>   static int scrub_rbio(struct btrfs_raid_bio *rbio)
>   {
> +	struct bio_list bio_list = BIO_EMPTY_LIST;
>   	bool need_check = false;
> -	struct bio_list bio_list;
>   	int sector_nr;
>   	int ret;
>   	struct bio *bio;
>   
> -	bio_list_init(&bio_list);
> -
>   	ret = alloc_rbio_essential_pages(rbio);
>   	if (ret)
> -		goto cleanup;
> +		return ret;
>   
>   	bitmap_clear(rbio->error_bitmap, 0, rbio->nr_sectors);
>   
>   	ret = scrub_assemble_read_bios(rbio, &bio_list);
>   	if (ret < 0)
> -		goto cleanup;
> +		return ret;
>   
>   	submit_read_bios(rbio, &bio_list);
>   	wait_event(rbio->io_wait, atomic_read(&rbio->stripes_pending) == 0);
>   
>   	/* We may have some failures, recover the failed sectors first. */
>   	ret = recover_scrub_rbio(rbio);
> -	if (ret < 0)
> -		goto cleanup;
> +	if (ret < 0) {
> +		while ((bio = bio_list_pop(&bio_list)))
> +			bio_put(bio);
> +		return ret;
> +	}

Do we still need the cleanup? IIRC after submit_read_bios() (or be more 
safe, after wait_event()), we should no longer touch @bio_list anymore.

Thus the cleanup itself is no longer needed AFAIK.

Thanks,
Qu

>   
>   	/*
>   	 * We have every sector properly prepared. Can finish the scrub
> @@ -2796,12 +2797,6 @@ static int scrub_rbio(struct btrfs_raid_bio *rbio)
>   		}
>   	}
>   	return ret;
> -
> -cleanup:
> -	while ((bio = bio_list_pop(&bio_list)))
> -		bio_put(bio);
> -
> -	return ret;
>   }
>   
>   static void scrub_rbio_work_locked(struct work_struct *work)