On Mon, Mar 11, 2019 at 03:35:54PM +0100, Christoph Hellwig wrote:
> > struct bio_vec *prev = &bio->bi_io_vec[bio->bi_vcnt - 1];
> >
> > + /* segment size is always >= PAGE_SIZE */
>
> I don't think that actually is true. We have various drivers with 4k
> segment size, for which this would not be true with a 64k page size
> system.
Please look at blk_queue_max_segment_size():
void blk_queue_max_segment_size(struct request_queue *q, unsigned int max_size)
{
if (max_size < PAGE_SIZE) {
max_size = PAGE_SIZE;
printk(KERN_INFO "%s: set to minimum %d\n",
__func__, max_size);
}
q->limits.max_segment_size = max_size;
}
But your concern is correct wrt. 4k segment size vs. 64k page size,
however, seems not see any such report.
>
> > if (page == prev->bv_page &&
> > offset == prev->bv_offset + prev->bv_len) {
> > if (put_same_page)
> > put_page(page);
> > + bvec_merge:
> > prev->bv_len += len;
> > bio->bi_iter.bi_size += len;
>
> Please throw in a cleanup patch that removes prev and and always uses
> bvec, then we can just move the done label up by two lines and handle
> the increment of bv_len and bi_size in a common branch.
Seems only one line of 'bio->bi_iter.bi_size += len;' can be shared.
>
> > done:
> > + bio->bi_phys_segments = bio->bi_vcnt;
> > + if (!bio_flagged(bio, BIO_SEG_VALID))
> > + bio_set_flag(bio, BIO_SEG_VALID);
>
> How would BIO_SEG_VALID get set previously? And even if it did
bio_add_pc_page() is run over each page.
> we wouldn't optimize much here, I'd just do it unconditionally.
OK.
>
> Btw, there are various comments in this function that either already
> were or have become out of data, like talking about REQ_PC or
> file system I/O - I think they could use some love.
OK.
Thanks,
Ming
