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.
