Re: [PATCH 1/4] block: rework bio splitting

[Christoph Hellwig <hch@lst.de>]

On Tue, Aug 27, 2024 at 07:26:40AM +0900, Damien Le Moal wrote:
> > +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...

Because we need to handle the case where we do no want to submit any
bio and error out as well, and a negative error code works nicely
for that.  Note that the bios has an unsigned int byte count, so we
have the extra precision for the sign bit.

> But shouldn't this check be:
> 
> 	if (split_sectors >= bio_sectors(bio))
> 		return bio;

The  API is to return the split position or 0 if no split is needed.
That is needed because splits are usually done for limits singnificantly
smaller than what the bio could take.

> > +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:

The !bio_may_need_split case is the existing fast path optimization
without a function call that I wanted to keep because it made a
difference from some workloads that Jens was testing.

> 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 ?

Note that bio_split_rw still always does the right thing, it'll just
do it a little slower than this fast path optimization.