On Tue, Oct 29, 2019 at 12:27:45AM -0700, Christoph Hellwig wrote:
> On Tue, Oct 29, 2019 at 03:06:21PM +0800, Ming Lei wrote:
> > __blk_queue_split() may be a bit heavy for small BS(such as 512B, or
>
> Maybe spell out block size. BS has another much less nice connotation.
OK.
>
> > bch_bio_map() should be the only one which doesn't use bio_add_page(),
> > so force to mark bio built via bch_bio_map() as MULTI_PAGE.
>
> We really need to fix that up. I had patches back in the day which
> Kent didn't particularly like for non-technical reason, that might serve
> as a starting point.
>
> > @@ -789,6 +794,10 @@ void __bio_add_page(struct bio *bio, struct page *page,
> > bio->bi_iter.bi_size += len;
> > bio->bi_vcnt++;
> >
> > + if (!bio_flagged(bio, BIO_MULTI_PAGE) && (bio->bi_vcnt >= 2 ||
> > + (bio->bi_vcnt == 1 && len > PAGE_SIZE)))
> > + bio_set_flag(bio, BIO_MULTI_PAGE);
>
> This looks pretty ugly and does more (and more confusing) checks than
> actually needed Maybe we need a little bio_is_multi_page helper to clean
> this up a bit:
>
> /*
> * Check if the bio contains more than a page and thus needs special
> * treatment in the bio splitting code.
> */
> static inline bool bio_is_multi_page(struct bio *bio)
> {
> return bio->bi_vcnt > 1 || bio->bi_io_vec[0].bv_len > PAGE_SIZE;
> }
>
> and then this becomes:
>
> if (!bio_flagged(bio, BIO_MULTI_PAGE) && bio_is_multi_page(bio))
>
> Then again these checks are so cheap that we can just use the
> bio_is_multi_page helper directly and skip the flag entirely.
I'd suggest to not add this helper:
1) there is only one user
2) the helper has to refer to bio->bi_io_vec
However, the above check can be simplified as:
if (!bio_flagged(bio, BIO_MULTI_PAGE) && (bio->bi_vcnt >= 2 ||
bv->bv_len > PAGE_SIZE))
bio_set_flag(bio, BIO_MULTI_PAGE);
Then the check has basically zero cost since all the checked variables
are just written or read in __bio_add_page() before the check.
Thanks,
Ming
