Re: [PATCH 06/40] btrfs: split submit_bio from btrfsic checking

[Qu Wenruo <quwenruo.btrfs@xxxxxxx>]

On 2022/3/22 23:55, Christoph Hellwig wrote:
> Require a separate call to the integrity checking helpers from the
> actual bio submission.

All-in for this!

I can't remember how many times I feel embarrassed when the bio
submission is done inside a function which by its name should only do
sanity checks.

Reviewed-by: Qu Wenruo <wqu@xxxxxxxx>

Thanks,
Qu
> Signed-off-by: Christoph Hellwig <hch@xxxxxx>
> ---
>   fs/btrfs/check-integrity.c | 14 +-------------
>   fs/btrfs/check-integrity.h |  8 ++++----
>   fs/btrfs/disk-io.c         |  6 ++++--
>   fs/btrfs/extent_io.c       |  3 ++-
>   fs/btrfs/scrub.c           | 12 ++++++++----
>   fs/btrfs/volumes.c         |  3 ++-
>   6 files changed, 21 insertions(+), 25 deletions(-)
>
> diff --git a/fs/btrfs/check-integrity.c b/fs/btrfs/check-integrity.c
> index 9efc1feb6cb08..49f9954f1438f 100644
> --- a/fs/btrfs/check-integrity.c
> +++ b/fs/btrfs/check-integrity.c
> @@ -2703,7 +2703,7 @@ static void btrfsic_check_flush_bio(struct bio *bio,
>   	}
>   }
>
> -static void __btrfsic_submit_bio(struct bio *bio)
> +void btrfsic_check_bio(struct bio *bio)
>   {
>   	struct btrfsic_dev_state *dev_state;
>
> @@ -2725,18 +2725,6 @@ static void __btrfsic_submit_bio(struct bio *bio)
>   	mutex_unlock(&btrfsic_mutex);
>   }
>
> -void btrfsic_submit_bio(struct bio *bio)
> -{
> -	__btrfsic_submit_bio(bio);
> -	submit_bio(bio);
> -}
> -
> -int btrfsic_submit_bio_wait(struct bio *bio)
> -{
> -	__btrfsic_submit_bio(bio);
> -	return submit_bio_wait(bio);
> -}
> -
>   int btrfsic_mount(struct btrfs_fs_info *fs_info,
>   		  struct btrfs_fs_devices *fs_devices,
>   		  int including_extent_data, u32 print_mask)
> diff --git a/fs/btrfs/check-integrity.h b/fs/btrfs/check-integrity.h
> index bcc730a06cb58..ed115e0f2ebbd 100644
> --- a/fs/btrfs/check-integrity.h
> +++ b/fs/btrfs/check-integrity.h
> @@ -7,11 +7,11 @@
>   #define BTRFS_CHECK_INTEGRITY_H
>
>   #ifdef CONFIG_BTRFS_FS_CHECK_INTEGRITY
> -void btrfsic_submit_bio(struct bio *bio);
> -int btrfsic_submit_bio_wait(struct bio *bio);
> +void btrfsic_check_bio(struct bio *bio);
>   #else
> -#define btrfsic_submit_bio submit_bio
> -#define btrfsic_submit_bio_wait submit_bio_wait
> +static inline void btrfsic_check_bio(struct bio *bio)
> +{
> +}
>   #endif
>
>   int btrfsic_mount(struct btrfs_fs_info *fs_info,
> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> index c245e1b131964..9b8ee74144910 100644
> --- a/fs/btrfs/disk-io.c
> +++ b/fs/btrfs/disk-io.c
> @@ -4048,7 +4048,8 @@ static int write_dev_supers(struct btrfs_device *device,
>   		if (i == 0 && !btrfs_test_opt(device->fs_info, NOBARRIER))
>   			bio->bi_opf |= REQ_FUA;
>
> -		btrfsic_submit_bio(bio);
> +		btrfsic_check_bio(bio);
> +		submit_bio(bio);
>
>   		if (btrfs_advance_sb_log(device, i))
>   			errors++;
> @@ -4161,7 +4162,8 @@ static void write_dev_flush(struct btrfs_device *device)
>   	init_completion(&device->flush_wait);
>   	bio->bi_private = &device->flush_wait;
>
> -	btrfsic_submit_bio(bio);
> +	btrfsic_check_bio(bio);
> +	submit_bio(bio);
>   	set_bit(BTRFS_DEV_STATE_FLUSH_SENT, &device->dev_state);
>   }
>
> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> index e789676373ab0..1a39b9ffdd180 100644
> --- a/fs/btrfs/extent_io.c
> +++ b/fs/btrfs/extent_io.c
> @@ -2370,7 +2370,8 @@ static int repair_io_failure(struct btrfs_fs_info *fs_info, u64 ino, u64 start,
>   	bio->bi_opf = REQ_OP_WRITE | REQ_SYNC;
>   	bio_add_page(bio, page, length, pg_offset);
>
> -	if (btrfsic_submit_bio_wait(bio)) {
> +	btrfsic_check_bio(bio);
> +	if (submit_bio_wait(bio)) {
>   		/* try to remap that extent elsewhere? */
>   		btrfs_bio_counter_dec(fs_info);
>   		bio_put(bio);
> diff --git a/fs/btrfs/scrub.c b/fs/btrfs/scrub.c
> index 2e9a322773f28..605ecc675ba7c 100644
> --- a/fs/btrfs/scrub.c
> +++ b/fs/btrfs/scrub.c
> @@ -1479,7 +1479,8 @@ static void scrub_recheck_block(struct btrfs_fs_info *fs_info,
>   		bio->bi_iter.bi_sector = spage->physical >> 9;
>   		bio->bi_opf = REQ_OP_READ;
>
> -		if (btrfsic_submit_bio_wait(bio)) {
> +		btrfsic_check_bio(bio);
> +		if (submit_bio_wait(bio)) {
>   			spage->io_error = 1;
>   			sblock->no_io_error_seen = 0;
>   		}
> @@ -1565,7 +1566,8 @@ static int scrub_repair_page_from_good_copy(struct scrub_block *sblock_bad,
>   			return -EIO;
>   		}
>
> -		if (btrfsic_submit_bio_wait(bio)) {
> +		btrfsic_check_bio(bio);
> +		if (submit_bio_wait(bio)) {
>   			btrfs_dev_stat_inc_and_print(spage_bad->dev,
>   				BTRFS_DEV_STAT_WRITE_ERRS);
>   			atomic64_inc(&fs_info->dev_replace.num_write_errors);
> @@ -1723,7 +1725,8 @@ static void scrub_wr_submit(struct scrub_ctx *sctx)
>   	 * orders the requests before sending them to the driver which
>   	 * doubled the write performance on spinning disks when measured
>   	 * with Linux 3.5 */
> -	btrfsic_submit_bio(sbio->bio);
> +	btrfsic_check_bio(sbio->bio);
> +	submit_bio(sbio->bio);
>
>   	if (btrfs_is_zoned(sctx->fs_info))
>   		sctx->write_pointer = sbio->physical + sbio->page_count *
> @@ -2057,7 +2060,8 @@ static void scrub_submit(struct scrub_ctx *sctx)
>   	sbio = sctx->bios[sctx->curr];
>   	sctx->curr = -1;
>   	scrub_pending_bio_inc(sctx);
> -	btrfsic_submit_bio(sbio->bio);
> +	btrfsic_check_bio(sbio->bio);
> +	submit_bio(sbio->bio);
>   }
>
>   static int scrub_add_page_to_rd_bio(struct scrub_ctx *sctx,
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index b07d382d53a86..bfa8e825e5047 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -6755,7 +6755,8 @@ static void submit_stripe_bio(struct btrfs_io_context *bioc, struct bio *bio,
>
>   	btrfs_bio_counter_inc_noblocked(fs_info);
>
> -	btrfsic_submit_bio(bio);
> +	btrfsic_check_bio(bio);
> +	submit_bio(bio);
>   }
>
>   static void bioc_error(struct btrfs_io_context *bioc, struct bio *bio, u64 logical)