Re: [PATCH 18/40] btrfs: move more work into btrfs_end_bioc

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

On 2022/3/22 23:55, Christoph Hellwig wrote:
> Assign ->mirror_num and ->bi_status in btrfs_end_bioc instead of
> duplicating the logic in the callers.  Also remove the bio argument as
> it always must be bioc->orig_bio and the now pointless bioc_error that
> did nothing but assign bi_sector to the same value just sampled in the
> caller.

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

It may be better to rename @first_bio or the @bio parameter, as it takes
me several seconds to realize that @bio get reused for RAID1*/DUP bio
cloned submission.

Thanks,
Qu

>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>   fs/btrfs/volumes.c | 68 ++++++++++++++--------------------------------
>   1 file changed, 20 insertions(+), 48 deletions(-)
>
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index 4dd54b80dac81..9d1f8c27eff33 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -6659,19 +6659,29 @@ int btrfs_map_sblock(struct btrfs_fs_info *fs_info, enum btrfs_map_op op,
>   	return __btrfs_map_block(fs_info, op, logical, length, bioc_ret, 0, 1);
>   }
>
> -static inline void btrfs_end_bioc(struct btrfs_io_context *bioc, struct bio *bio)
> +static inline void btrfs_end_bioc(struct btrfs_io_context *bioc)
>   {
> +	struct bio *bio = bioc->orig_bio;
> +
> +	btrfs_bio(bio)->mirror_num = bioc->mirror_num;
>   	bio->bi_private = bioc->private;
>   	bio->bi_end_io = bioc->end_io;
> -	bio_endio(bio);
>
> +	/*
> +	 * Only send an error to the higher layers if it is beyond the tolerance
> +	 * threshold.
> +	 */
> +	if (atomic_read(&bioc->error) > bioc->max_errors)
> +		bio->bi_status = BLK_STS_IOERR;
> +	else
> +		bio->bi_status = BLK_STS_OK;
> +	bio_endio(bio);
>   	btrfs_put_bioc(bioc);
>   }
>
>   static void btrfs_end_bio(struct bio *bio)
>   {
>   	struct btrfs_io_context *bioc = bio->bi_private;
> -	int is_orig_bio = 0;
>
>   	if (bio->bi_status) {
>   		atomic_inc(&bioc->error);
> @@ -6692,35 +6702,12 @@ static void btrfs_end_bio(struct bio *bio)
>   		}
>   	}
>
> -	if (bio == bioc->orig_bio)
> -		is_orig_bio = 1;
> +	if (bio != bioc->orig_bio)
> +		bio_put(bio);
>
>   	btrfs_bio_counter_dec(bioc->fs_info);
> -
> -	if (atomic_dec_and_test(&bioc->stripes_pending)) {
> -		if (!is_orig_bio) {
> -			bio_put(bio);
> -			bio = bioc->orig_bio;
> -		}
> -
> -		btrfs_bio(bio)->mirror_num = bioc->mirror_num;
> -		/* only send an error to the higher layers if it is
> -		 * beyond the tolerance of the btrfs bio
> -		 */
> -		if (atomic_read(&bioc->error) > bioc->max_errors) {
> -			bio->bi_status = BLK_STS_IOERR;
> -		} else {
> -			/*
> -			 * this bio is actually up to date, we didn't
> -			 * go over the max number of errors
> -			 */
> -			bio->bi_status = BLK_STS_OK;
> -		}
> -
> -		btrfs_end_bioc(bioc, bio);
> -	} else if (!is_orig_bio) {
> -		bio_put(bio);
> -	}
> +	if (atomic_dec_and_test(&bioc->stripes_pending))
> +		btrfs_end_bioc(bioc);
>   }
>
>   static void submit_stripe_bio(struct btrfs_io_context *bioc, struct bio *bio,
> @@ -6758,23 +6745,6 @@ static void submit_stripe_bio(struct btrfs_io_context *bioc, struct bio *bio,
>   	submit_bio(bio);
>   }
>
> -static void bioc_error(struct btrfs_io_context *bioc, struct bio *bio, u64 logical)
> -{
> -	atomic_inc(&bioc->error);
> -	if (atomic_dec_and_test(&bioc->stripes_pending)) {
> -		/* Should be the original bio. */
> -		WARN_ON(bio != bioc->orig_bio);
> -
> -		btrfs_bio(bio)->mirror_num = bioc->mirror_num;
> -		bio->bi_iter.bi_sector = logical >> 9;
> -		if (atomic_read(&bioc->error) > bioc->max_errors)
> -			bio->bi_status = BLK_STS_IOERR;
> -		else
> -			bio->bi_status = BLK_STS_OK;
> -		btrfs_end_bioc(bioc, bio);
> -	}
> -}
> -
>   blk_status_t btrfs_map_bio(struct btrfs_fs_info *fs_info, struct bio *bio,
>   			   int mirror_num)
>   {
> @@ -6833,7 +6803,9 @@ blk_status_t btrfs_map_bio(struct btrfs_fs_info *fs_info, struct bio *bio,
>   						   &dev->dev_state) ||
>   		    (btrfs_op(first_bio) == BTRFS_MAP_WRITE &&
>   		    !test_bit(BTRFS_DEV_STATE_WRITEABLE, &dev->dev_state))) {
> -			bioc_error(bioc, first_bio, logical);
> +			atomic_inc(&bioc->error);
> +			if (atomic_dec_and_test(&bioc->stripes_pending))
> +				btrfs_end_bioc(bioc);
>   			continue;
>   		}
>