linux-kernel - Re: [PATCH] bio: limit bio max size.

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CH2PR04MB65225EDDA7069CCD47A459A5E7A90@CH2PR04MB6522.namprd04.prod.outlook.com>
Date:   Wed, 13 Jan 2021 01:16:57 +0000
From:   Damien Le Moal <Damien.LeMoal@....com>
To:     Changheun Lee <nanich.lee@...sung.com>
CC:     Johannes Thumshirn <Johannes.Thumshirn@....com>,
        "axboe@...nel.dk" <axboe@...nel.dk>,
        "jisoo2146.oh@...sung.com" <jisoo2146.oh@...sung.com>,
        "junho89.kim@...sung.com" <junho89.kim@...sung.com>,
        "linux-block@...r.kernel.org" <linux-block@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "mj0123.lee@...sung.com" <mj0123.lee@...sung.com>,
        "seunghwan.hyun@...sung.com" <seunghwan.hyun@...sung.com>,
        "sookwan7.kim@...sung.com" <sookwan7.kim@...sung.com>,
        "tj@...nel.org" <tj@...nel.org>,
        "yt0928.kim@...sung.com" <yt0928.kim@...sung.com>
Subject: Re: [PATCH] bio: limit bio max size.

On 2021/01/12 21:14, Changheun Lee wrote:
>> On 2021/01/12 17:52, Changheun Lee wrote:
>>> From: "Changheun Lee" <nanich.lee@...sung.com>
>>>
>>> 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, - 64MB chunk read in user space -
>>> all pages for 64MB would be merged to a bio structure if memory address is
>>> continued phsycally. it makes some delay to submit until merge complete.
>>> bio max size should be limited as a proper size.
>>
>> But merging physically contiguous pages into the same bvec + later automatic bio
>> split on submit should give you better throughput for large IOs compared to
>> having to issue a bio chain of smaller BIOs that are arbitrarily sized and will
>> likely need splitting anyway (because of DMA boundaries etc).
>>
>> Do you have a specific case where you see higher performance with this patch
>> applied ? On Intel, BIO_MAX_SIZE would be 1MB... That is arbitrary and too small
>> considering that many hardware can execute larger IOs than that.
>>
> 
> When I tested 32MB chunk read with O_DIRECT in android, all pages of 32MB
> is merged into a bio structure.
> And elapsed time to merge complete was about 2ms.
> It means first bio-submit is after 2ms.
> If bio size is limited with 1MB with this patch, first bio-submit is about
> 100us by bio_full operation.

bio_submit() will split the large BIO case into multiple requests while the
small BIO case will likely result one or two requests only. That likely explain
the time difference here. However, for the large case, the 2ms will issue ALL
requests needed for processing the entire 32MB user IO while the 1MB bio case
will need 32 different bio_submit() calls. So what is the actual total latency
difference for the entire 32MB user IO ? That is I think what needs to be
compared here.

Also, what is your device max_sectors_kb and max queue depth ?

> It's not large delay and can't be observed with low speed device.
> But it's needed to reduce merge delay for high speed device.
> I improved 512MB sequential read performance from 1900MB/s to 2000MB/s
> with this patch on android platform.
> As you said, 1MB might be small for some device.
> But method is needed to re-size, or select the bio max size.

At the very least, I think that such limit should not be arbitrary as your patch
proposes but rely on the device characteristics (e.g.
max_hw_sectors_kb/max_sectors_kb and queue depth).

> 
>>
>>>
>>> Signed-off-by: Changheun Lee <nanich.lee@...sung.com>
>>> ---
>>>  block/bio.c         | 2 +-
>>>  include/linux/bio.h | 3 ++-
>>>  2 files changed, 3 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/block/bio.c b/block/bio.c
>>> index 1f2cc1fbe283..dbe14d675f28 100644
>>> --- a/block/bio.c
>>> +++ b/block/bio.c
>>> @@ -877,7 +877,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_MAX_SIZE - len) {
>>>  				*same_page = false;
>>>  				return false;
>>>  			}
>>> diff --git a/include/linux/bio.h b/include/linux/bio.h
>>> index 1edda614f7ce..0f49b354b1f6 100644
>>> --- a/include/linux/bio.h
>>> +++ b/include/linux/bio.h
>>> @@ -20,6 +20,7 @@
>>>  #endif
>>>  
>>>  #define BIO_MAX_PAGES		256
>>> +#define BIO_MAX_SIZE		(BIO_MAX_PAGES * PAGE_SIZE)
>>>  
>>>  #define bio_prio(bio)			(bio)->bi_ioprio
>>>  #define bio_set_prio(bio, prio)		((bio)->bi_ioprio = prio)
>>> @@ -113,7 +114,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_MAX_SIZE - len)
>>>  		return true;
>>>  
>>>  	return false;
>>>
>>
>>
>> -- 
>> Damien Le Moal
>> Western Digital Research
> 


-- 
Damien Le Moal
Western Digital Research