Re: [PATCH 5/6] block: enable multi-page bvec for passthrough IO

[Ming Lei <ming.lei@redhat.com>]

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