On 8/27/24 02:37, Christoph Hellwig wrote:
> The current setup with bio_may_exceed_limit and __bio_split_to_limits
> is a bit of a mess.
>
> Change it so that __bio_split_to_limits does all the work and is just
> a variant of bio_split_to_limits that returns nr_segs. This is done
> by inlining it and instead have the various bio_split_* helpers directly
> submit the potentially split bios.
>
> To support btrfs, the rw version has a lower level helper split out
> that just returns the offset to split. This turns out to nicely clean
> up the btrfs flow as well.
>
> Signed-off-by: Christoph Hellwig <hch@xxxxxx>
> ---
> block/blk-merge.c | 146 +++++++++++++++++---------------------------
> block/blk-mq.c | 11 ++--
> block/blk.h | 63 +++++++++++++------
> fs/btrfs/bio.c | 30 +++++----
> include/linux/bio.h | 4 +-
> 5 files changed, 125 insertions(+), 129 deletions(-)
>
> diff --git a/block/blk-merge.c b/block/blk-merge.c
> index de5281bcadc538..c7222c4685e060 100644
> --- a/block/blk-merge.c
> +++ b/block/blk-merge.c
> @@ -105,9 +105,33 @@ static unsigned int bio_allowed_max_sectors(const struct queue_limits *lim)
> return round_down(UINT_MAX, lim->logical_block_size) >> SECTOR_SHIFT;
> }
>
> -static struct bio *bio_split_discard(struct bio *bio,
> - const struct queue_limits *lim,
> - unsigned *nsegs, struct bio_set *bs)
> +static struct bio *bio_submit_split(struct bio *bio, int split_sectors)
Why not "unsigned int" for split_sectors ? That would avoid the need for the
first "if" of the function. Note that bio_split() also takes an int for the
sector count and also checks for < 0 count with a BUG_ON(). We can clean that up
too. BIOs sector count is unsigned int...
> +{
> + if (unlikely(split_sectors < 0)) {
> + bio->bi_status = errno_to_blk_status(split_sectors);
> + bio_endio(bio);
> + return NULL;
> + }
> +
> + if (split_sectors) {
May be the simple case should come first ? E.g.:
if (!split_sectors)
return bio;
But shouldn't this check be:
if (split_sectors >= bio_sectors(bio))
return bio;
?
[...]
> +/**
> + * __bio_split_to_limits - split a bio to fit the queue limits
> + * @bio: bio to be split
> + * @lim: queue limits to split based on
> + * @nr_segs: returns the number of segments in the returned bio
> + *
> + * Check if @bio needs splitting based on the queue limits, and if so split off
> + * a bio fitting the limits from the beginning of @bio and return it. @bio is
> + * shortened to the remainder and re-submitted.
> + *
> + * The split bio is allocated from @q->bio_split, which is provided by the
> + * block layer.
> + */
> +static inline struct bio *__bio_split_to_limits(struct bio *bio,
> + const struct queue_limits *lim, unsigned int *nr_segs)
> {
> switch (bio_op(bio)) {
> + default:
> + if (bio_may_need_split(bio, lim))
> + return bio_split_rw(bio, lim, nr_segs);
> + *nr_segs = 1;
> + return bio;
Wouldn't it be safer to move the if check inside bio_split_rw(), so that here we
can have:
return bio_split_rw(bio, lim, nr_segs);
And at the top of bio_split_rw() add:
if (!bio_may_need_split(bio, lim)) {
*nr_segs = 1;
return bio;
}
so that bio_split_rw() always does the right thing ?
> case REQ_OP_DISCARD:
> case REQ_OP_SECURE_ERASE:
> + return bio_split_discard(bio, lim, nr_segs);
> case REQ_OP_WRITE_ZEROES:
> - return true; /* non-trivial splitting decisions */
> - default:
> - break;
> + return bio_split_write_zeroes(bio, lim, nr_segs);
> }
> -
> - /*
> - * All drivers must accept single-segments bios that are <= PAGE_SIZE.
> - * This is a quick and dirty check that relies on the fact that
> - * bi_io_vec[0] is always valid if a bio has data. The check might
> - * lead to occasional false negatives when bios are cloned, but compared
> - * to the performance impact of cloned bios themselves the loop below
> - * doesn't matter anyway.
> - */
> - return lim->chunk_sectors || bio->bi_vcnt != 1 ||
> - bio->bi_io_vec->bv_len + bio->bi_io_vec->bv_offset > PAGE_SIZE;
> }
--
Damien Le Moal
Western Digital Research
