Re: [PATCH RFC v2 01/10] block: factor out a helper bio_submit_split_bioset()

[Damien Le Moal <dlemoal@kernel.org>]

On 8/28/25 15:57, Yu Kuai wrote:
> From: Yu Kuai <yukuai3@huawei.com>
> 
> No functional changes are intended, some drivers like mdraid will split
> bio by internal processing, prepare to unify bio split codes.
> 
> Signed-off-by: Yu Kuai <yukuai3@huawei.com>

Looks good to me. A few nits below.

> ---
>  block/blk-merge.c      | 63 ++++++++++++++++++++++++++++--------------
>  include/linux/blkdev.h |  2 ++
>  2 files changed, 44 insertions(+), 21 deletions(-)
> 
> diff --git a/block/blk-merge.c b/block/blk-merge.c
> index 70d704615be5..3d6dc9cc4f61 100644
> --- a/block/blk-merge.c
> +++ b/block/blk-merge.c
> @@ -104,34 +104,55 @@ static unsigned int bio_allowed_max_sectors(const struct queue_limits *lim)
>  	return round_down(UINT_MAX, lim->logical_block_size) >> SECTOR_SHIFT;
>  }
>  
> +/**
> + * bio_submit_split_bioset - Submit a bio, splitting it at a designated sector
> + * @bio:		the original bio to be submitted and split
> + * @split_sectors:	the sector count at which to split
> + * @bs:			the bio set used for allocating the new split bio
> + *
> + * The original bio is modified to contain the remaining sectors and submitted.
> + * The caller is responsible for submitting the returned bio.
> + *
> + * If succeed, the newly allocated bio representing the initial part will be
> + * returned, on failure NULL will be returned and original bio will fail.
> + */
> +struct bio *bio_submit_split_bioset(struct bio *bio, int split_sectors,
> +				    struct bio_set *bs)

While at it, it would be nice to have split_sectors be unsigned. That would
avoid the check in bio_submit_split().

> +{
> +	struct bio *split = bio_split(bio, split_sectors, GFP_NOIO, bs);
> +
> +	if (IS_ERR(split)) {
> +		bio->bi_status = errno_to_blk_status(PTR_ERR(split));
> +		bio_endio(bio);
> +		return NULL;
> +	}
> +
> +	blkcg_bio_issue_init(split);
> +	bio_chain(split, bio);
> +	trace_block_split(split, bio->bi_iter.bi_sector);
> +	WARN_ON_ONCE(bio_zone_write_plugging(bio));
> +	submit_bio_noacct(bio);
> +
> +	return split;
> +}
> +EXPORT_SYMBOL_GPL(bio_submit_split_bioset);
> +
>  static struct bio *bio_submit_split(struct bio *bio, int split_sectors)
>  {
> -	if (unlikely(split_sectors < 0))
> -		goto error;
> +	if (unlikely(split_sectors < 0)) {
> +		bio->bi_status = errno_to_blk_status(split_sectors);
> +		bio_endio(bio);
> +		return NULL;
> +	}

See above.

>  
>  	if (split_sectors) {
> -		struct bio *split;
> -
> -		split = bio_split(bio, split_sectors, GFP_NOIO,
> -				&bio->bi_bdev->bd_disk->bio_split);
> -		if (IS_ERR(split)) {
> -			split_sectors = PTR_ERR(split);
> -			goto error;
> -		}
> -		split->bi_opf |= REQ_NOMERGE;
> -		blkcg_bio_issue_init(split);
> -		bio_chain(split, bio);
> -		trace_block_split(split, bio->bi_iter.bi_sector);
> -		WARN_ON_ONCE(bio_zone_write_plugging(bio));
> -		submit_bio_noacct(bio);
> -		return split;
> +		bio = bio_submit_split_bioset(bio, split_sectors,
> +					 &bio->bi_bdev->bd_disk->bio_split);
> +		if (bio)
> +			bio->bi_opf |= REQ_NOMERGE;

I think that setting REQ_NOMERGE should be done in bio_submit_split_bioset().

>  	}
>  
>  	return bio;
> -error:
> -	bio->bi_status = errno_to_blk_status(split_sectors);
> -	bio_endio(bio);
> -	return NULL;
>  }
>  
>  struct bio *bio_split_discard(struct bio *bio, const struct queue_limits *lim,
> diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
> index fe1797bbec42..be4b3adf3989 100644
> --- a/include/linux/blkdev.h
> +++ b/include/linux/blkdev.h
> @@ -999,6 +999,8 @@ extern int blk_register_queue(struct gendisk *disk);
>  extern void blk_unregister_queue(struct gendisk *disk);
>  void submit_bio_noacct(struct bio *bio);
>  struct bio *bio_split_to_limits(struct bio *bio);
> +struct bio *bio_submit_split_bioset(struct bio *bio, int split_sectors,
> +				    struct bio_set *bs);
>  
>  extern int blk_lld_busy(struct request_queue *q);
>  extern int blk_queue_enter(struct request_queue *q, blk_mq_req_flags_t flags);


-- 
Damien Le Moal
Western Digital Research