On Wed, 2023-11-08 at 15:58 +0800, Ming Lei wrote:
> 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
We cannot predict the number of pages that can be filled in a bio as it
depends on the physical layout of memory buffer. The only option is to
limit the bio to the queue limit.
Even if I fill the bio with a large amount of pages, it will still be
split based on the queue limit at block layer. Therefore, I believe it
is appropriate to limit the bio size in this case.
We can enforce this limitation before extracting the page, which would
elimiate the need for iov_iter_revert.
Hi Christoph, what stage do you think would be better to impose the
limitaion?
--
Best Regards,
Ed Tsai
>
