>Please drop the "." at the end of the patch title. > >> bio size can grow up to 4GB when muli-page bvec is enabled. >> but sometimes it would lead to inefficient behaviors. >> in case of large chunk direct I/O, - 32MB chunk read in user space - >> all pages for 32MB would be merged to a bio structure if memory address is >> continued phsycally. it makes some delay to submit until merge complete. > >s/if memory address is continued phsycally/if the pages physical addresses are >contiguous/ > >> bio max size should be limited as a proper size. > >s/as/to/
Thank you for advice. :) > >> >> When 32MB chunk read with direct I/O option is coming from userspace, >> kernel behavior is below now. it's timeline. >> >> | bio merge for 32MB. total 8,192 pages are merged. >> | total elapsed time is over 2ms. >> |------------------ ... ----------------------->| >> | 8,192 pages merged a bio. >> | at this time, first bio >> submit is done. >> | 1 bio is split to 32 read >> request and issue. >> |---------------> >> |---------------> >> |---------------> >> ...... >> >> |---------------> >> >> |--------------->| >> total 19ms elapsed to complete 32MB read done from >> device. | >> >> If bio max size is limited with 1MB, behavior is changed below. >> >> | bio merge for 1MB. 256 pages are merged for each bio. >> | total 32 bio will be made. >> | total elapsed time is over 2ms. it's same. >> | but, first bio submit timing is fast. about 100us. >> |--->|--->|--->|---> ... -->|--->|--->|--->|--->| >> | 256 pages merged a bio. >> | at this time, first bio submit is done. >> | and 1 read request is issued for 1 bio. >> |---------------> >> |---------------> >> |---------------> >> ...... >> |---------------> >> |--------------->| >> total 17ms elapsed to complete 32MB read done from device. | >> >> As a result, read request issue timing is faster if bio max size is limited. >> Current kernel behavior with multipage bvec, super large bio can be created. >> And it lead to delay first I/O request issue. >> >> Signed-off-by: Changheun Lee <[email protected]> >> >> --- >> block/bio.c | 17 ++++++++++++++++- >> include/linux/bio.h | 13 +++---------- >> include/linux/blk_types.h | 1 + >> 3 files changed, 20 insertions(+), 11 deletions(-) >> >> diff --git a/block/bio.c b/block/bio.c >> index 1f2cc1fbe283..027503c2e2e7 100644 >> --- a/block/bio.c >> +++ b/block/bio.c >> @@ -284,9 +284,24 @@ void bio_init(struct bio *bio, struct bio_vec *table, >> >> bio->bi_io_vec = table; >> bio->bi_max_vecs = max_vecs; >> + bio->bi_max_size = UINT_MAX; >> } >> EXPORT_SYMBOL(bio_init); >> >> +void bio_set_dev(struct bio *bio, struct block_device *bdev) >> +{ >> + if (bio->bi_disk != bdev->bd_disk) >> + bio_clear_flag(bio, BIO_THROTTLED); >> + >> + bio->bi_disk = bdev->bd_disk; >> + bio->bi_partno = bdev->bd_partno; >> + bio->bi_max_size = blk_queue_get_max_sectors(bio->bi_disk->queue, >> + bio_op(bio)) << SECTOR_SHIFT; >> + >> + bio_associate_blkg(bio); >> +} >> +EXPORT_SYMBOL(bio_set_dev); >> + >> /** >> * bio_reset - reinitialize a bio >> * @bio: bio to reset >> @@ -877,7 +892,7 @@ bool __bio_try_merge_page(struct bio *bio, struct page >> *page, >> struct bio_vec *bv = &bio->bi_io_vec[bio->bi_vcnt - 1]; >> >> if (page_is_mergeable(bv, page, len, off, same_page)) { >> - if (bio->bi_iter.bi_size > UINT_MAX - len) { >> + if (bio->bi_iter.bi_size > bio->bi_max_size - len) >> *same_page = false; >> return false; >> } >> diff --git a/include/linux/bio.h b/include/linux/bio.h >> index 1edda614f7ce..b9803e80c259 100644 >> --- a/include/linux/bio.h >> +++ b/include/linux/bio.h >> @@ -113,7 +113,7 @@ static inline bool bio_full(struct bio *bio, unsigned >> len) >> if (bio->bi_vcnt >= bio->bi_max_vecs) >> return true; >> >> - if (bio->bi_iter.bi_size > UINT_MAX - len) >> + if (bio->bi_iter.bi_size > bio->bi_max_size - len) >> return true; >> >> return false; >> @@ -482,20 +482,13 @@ extern struct bio_vec *bvec_alloc(gfp_t, int, unsigned >> long *, mempool_t *); >> extern void bvec_free(mempool_t *, struct bio_vec *, unsigned int); >> extern unsigned int bvec_nr_vecs(unsigned short idx); >> extern const char *bio_devname(struct bio *bio, char *buffer); >> - >> -#define bio_set_dev(bio, bdev) \ >> -do { \ >> - if ((bio)->bi_disk != (bdev)->bd_disk) \ >> - bio_clear_flag(bio, BIO_THROTTLED);\ >> - (bio)->bi_disk = (bdev)->bd_disk; \ >> - (bio)->bi_partno = (bdev)->bd_partno; \ >> - bio_associate_blkg(bio); \ >> -} while (0) >> +extern void bio_set_dev(struct bio *bio, struct block_device *bdev); >> >> #define bio_copy_dev(dst, src) \ >> do { \ >> (dst)->bi_disk = (src)->bi_disk; \ >> (dst)->bi_partno = (src)->bi_partno; \ >> + (dst)->bi_max_size = (src)->bi_max_size;\ >> bio_clone_blkg_association(dst, src); \ >> } while (0) >> >> diff --git a/include/linux/blk_types.h b/include/linux/blk_types.h >> index 866f74261b3b..e5dd5b7d8fc1 100644 >> --- a/include/linux/blk_types.h >> +++ b/include/linux/blk_types.h >> @@ -270,6 +270,7 @@ struct bio { >> */ >> >> unsigned short bi_max_vecs; /* max bvl_vecs we can hold */ >> + unsigned int bi_max_size; /* max data size we can hold */ >> >> atomic_t __bi_cnt; /* pin count */ > >This modification comes at the cost of increasing the bio structure size to >simply tell the block layer "do not delay BIO splitting"... > >I think there is a much simpler approach. What about: > >1) Use a request queue flag to indicate "limit BIO size" >2) modify __bio_try_merge_page() to look at that flag to disallow page merging >if the bio size exceeds blk_queue_get_max_sectors(), or more ideally a version >of it that takes into account the bio start sector. >3) Set the "limit bio size" queue flag in the driver of the device that benefit >from this change. Eventually, that could also be controlled through sysfs. > >With such change, you will get the same result without having to increase the >BIO structure size. I have a qustion. Is adding new variable in bio not possible? Additional check for every page merge like as below is inefficient I think. bool __bio_try_merge_page(struct bio *bio, struct page *page, unsigned int len, unsigned int off, bool *same_page) { ... if (page_is_mergeable(bv, page, len, off, same_page)) { if (bio->bi_iter.bi_size > UINT_MAX - len) { *same_page = false; return false; } + if (blk_queue_limit_bio_max_size(bio) && + (bio->bi_iter.bi_size > blk_queue_get_bio_max_size(bio) - len)) { + *same_page = false; + return false; + } bv->bv_len += len; bio->bi_iter.bi_size += len; return true; } ... } static inline bool bio_full(struct bio *bio, unsigned len) { ... if (bio->bi_iter.bi_size > UINT_MAX - len) return true; + if (blk_queue_limit_bio_max_size(bio) && + (bio->bi_iter.bi_size > blk_queue_get_bio_max_size(bio) - len)) + return true; ... } Page merge is CPU-bound job as you said. How about below with adding of bi_max_size in bio? bool __bio_try_merge_page(struct bio *bio, struct page *page, unsigned int len, unsigned int off, bool *same_page) { ... if (page_is_mergeable(bv, page, len, off, same_page)) { - if (bio->bi_iter.bi_size > UINT_MAX - len) { + if (bio->bi_iter.bi_size > bio->bi_max_size - len) { *same_page = false; return false; } bv->bv_len += len; bio->bi_iter.bi_size += len; return true; } ... } static inline bool bio_full(struct bio *bio, unsigned len) { ... - if (bio->bi_iter.bi_size > UINT_MAX - len) + if (bio->bi_iter.bi_size > bio->bi_max_size - len) return true; ... } +void bio_set_dev(struct bio *bio, struct block_device *bdev) +{ + if (bio->bi_disk != bdev->bd_disk) + bio_clear_flag(bio, BIO_THROTTLED); + + bio->bi_disk = bdev->bd_disk; + bio->bi_partno = bdev->bd_partno; + if (blk_queue_limit_bio_max_size(bio)) + bio->bi_max_size = blk_queue_get_bio_max_size(bio); + + bio_associate_blkg(bio); +} +EXPORT_SYMBOL(bio_set_dev); >-- >Damien Le Moal >Western Digital Research --- Changheun Lee Samsung Electronics
