Re: [PATCH v2] bio: limit bio max size
From: Changheun Lee
Date: Thu Jan 21 2021 - 04:53:51 EST
>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 <nanich.lee@xxxxxxxxxxx>
>>
>> ---
>> 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