On Tue, Nov 07, 2023 at 11:28:30PM -0800, Christoph Hellwig wrote:
> On Tue, Nov 07, 2023 at 06:01:40PM +0800, Ming Lei wrote:
> > In case of big chunk sequential IO, bio's size is often not aligned with
> > this queue's max request size because of multipage bvec, then small sized
> > bio can be made by bio split, so try to align bio with max io size if
> > it isn't the last one.
>
> I have a hard time parsing this long sentence.
It covers the reasons(multipage bvec, bio split, big sequential IO) and solution(
align bio), or any suggestion to simplify it further?
>
> > + /*
> > + * If we still have data and bio is full, this bio size may not be
> > + * aligned with max io size, small bio can be caused by split, try
> > + * to avoid this situation by aligning bio with max io size.
> > + *
> > + * Big chunk of sequential IO workload can benefit from this way.
> > + */
> > + if (!ret && iov_iter_count(iter) && bio->bi_bdev &&
> > + bio_op(bio) != REQ_OP_ZONE_APPEND) {
> > + unsigned trim = bio_align_with_io_size(bio);
>
> Besides the fact that bi_bdev should always be set, this really does
> thing backwards.
Looks it is true after your patch passes bdev to bio_alloc*(), but
bio_add_page() is just fine without bio->bi_bdev.
Also this way follows the check for aligning with block size.
But seems safe to remove the check.
> Instead of rewding things which is really
> expensive don't add it in the first place.
The rewind may not be avoided because iov_iter_extract_pages() can often
return less pages than requested. Also rewind is only done after the bio
becomes full(at least 1MB bytes are added) and there is still data left,
so it shouldn't be too expensive.
I will provide 'size' hint to iov_iter_extract_pages() to try to make
it aligned from the beginning in next version, but rewind may not be
avoided.
Thanks,
Ming
