Re: [PATCH v2] bio: limit bio max size

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



>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



[Index of Archives]     [Linux RAID]     [Linux SCSI]     [Linux ATA RAID]     [IDE]     [Linux Wireless]     [Linux Kernel]     [ATH6KL]     [Linux Bluetooth]     [Linux Netdev]     [Kernel Newbies]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Device Mapper]

  Powered by Linux