Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
IronPort-SDR: w4LqwiLt6UcyeXymJ4qKmfaW51OQiDrWSi1yakFmk3Nz0WCfz1K0i9GsAbrzT4cum44c0Z38Fx
 pGYPRAtPrH0w72r4+PPrlehTzItuThEM6hNcPAJcXSWj+0J95FoKGug26ZZePeXQ6Wt5WR5u81
 RG3qJpeHVygImQEzjU9Nw15TRDeZZ3iKnYh/7O/M7/TIyrr/3Q8LO8W1FURd327XYgzKbYC0gm
 lkQfdoFrKZqRYWOar1h3Peaw9/7zqvbNsmrKmMQIp7DPUeAJPAa19FPjW4QIQjxc35PFMHHoXV
 Htk=
From:   Damien Le Moal <Damien.LeMoal@wdc.com>
To:     Changheun Lee <nanich.lee@samsung.com>
CC:     Johannes Thumshirn <Johannes.Thumshirn@wdc.com>,
        "axboe@kernel.dk" <axboe@kernel.dk>,
        "jisoo2146.oh@samsung.com" <jisoo2146.oh@samsung.com>,
        "junho89.kim@samsung.com" <junho89.kim@samsung.com>,
        "linux-block@vger.kernel.org" <linux-block@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "mj0123.lee@samsung.com" <mj0123.lee@samsung.com>,
        "seunghwan.hyun@samsung.com" <seunghwan.hyun@samsung.com>,
        "sookwan7.kim@samsung.com" <sookwan7.kim@samsung.com>,
        "tj@kernel.org" <tj@kernel.org>,
        "yt0928.kim@samsung.com" <yt0928.kim@samsung.com>
Subject: Re: [PATCH] bio: limit bio max size.
Thread-Topic: [PATCH] bio: limit bio max size.
Thread-Index: AQHW6UnJBbWZvTKpx0iP4pbQi8BDkg==
Date:   Wed, 13 Jan 2021 01:16:57 +0000
Message-ID: <CH2PR04MB65225EDDA7069CCD47A459A5E7A90@CH2PR04MB6522.namprd04.prod.outlook.com>
References: <BL0PR04MB6514F7F944D70DE024494544E7AA0@BL0PR04MB6514.namprd04.prod.outlook.com>
 <CGME20210112121356epcas1p124baa93f10eb3400539ba4db27c18955@epcas1p1.samsung.com>
 <20210112115846.31415-1-nanich.lee@samsung.com>
Accept-Language: en-US
Content-Language: en-US
wdcipoutbound: EOP-TRUE
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: 643e6648-38e5-46d6-5830-08d8b760ec31
X-MS-Exchange-CrossTenant-originalarrivaltime: 13 Jan 2021 01:16:57.9588
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: b61c8803-16f3-4c35-9b17-6f65f441df86
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: pmmsX3cTtJAR5RaHa5eaWGPuMvHLWE3n+vhmoYKDHjM4XM43ZTTaKjpLohVc7f/vD8lxLFMRwIJusiT1oPWp5Q==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CH2PR04MB7095
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On 2021/01/12 21:14, Changheun Lee wrote:=0A=
>> On 2021/01/12 17:52, Changheun Lee wrote:=0A=
>>> From: "Changheun Lee" <nanich.lee@samsung.com>=0A=
>>>=0A=
>>> bio size can grow up to 4GB when muli-page bvec is enabled.=0A=
>>> but sometimes it would lead to inefficient behaviors.=0A=
>>> in case of large chunk direct I/O, - 64MB chunk read in user space -=0A=
>>> all pages for 64MB would be merged to a bio structure if memory address=
 is=0A=
>>> continued phsycally. it makes some delay to submit until merge complete=
.=0A=
>>> bio max size should be limited as a proper size.=0A=
>>=0A=
>> But merging physically contiguous pages into the same bvec + later autom=
atic bio=0A=
>> split on submit should give you better throughput for large IOs compared=
 to=0A=
>> having to issue a bio chain of smaller BIOs that are arbitrarily sized a=
nd will=0A=
>> likely need splitting anyway (because of DMA boundaries etc).=0A=
>>=0A=
>> Do you have a specific case where you see higher performance with this p=
atch=0A=
>> applied ? On Intel, BIO_MAX_SIZE would be 1MB... That is arbitrary and t=
oo small=0A=
>> considering that many hardware can execute larger IOs than that.=0A=
>>=0A=
> =0A=
> When I tested 32MB chunk read with O_DIRECT in android, all pages of 32MB=
=0A=
> is merged into a bio structure.=0A=
> And elapsed time to merge complete was about 2ms.=0A=
> It means first bio-submit is after 2ms.=0A=
> If bio size is limited with 1MB with this patch, first bio-submit is abou=
t=0A=
> 100us by bio_full operation.=0A=
=0A=
bio_submit() will split the large BIO case into multiple requests while the=
=0A=
small BIO case will likely result one or two requests only. That likely exp=
lain=0A=
the time difference here. However, for the large case, the 2ms will issue A=
LL=0A=
requests needed for processing the entire 32MB user IO while the 1MB bio ca=
se=0A=
will need 32 different bio_submit() calls. So what is the actual total late=
ncy=0A=
difference for the entire 32MB user IO ? That is I think what needs to be=
=0A=
compared here.=0A=
=0A=
Also, what is your device max_sectors_kb and max queue depth ?=0A=
=0A=
> It's not large delay and can't be observed with low speed device.=0A=
> But it's needed to reduce merge delay for high speed device.=0A=
> I improved 512MB sequential read performance from 1900MB/s to 2000MB/s=0A=
> with this patch on android platform.=0A=
> As you said, 1MB might be small for some device.=0A=
> But method is needed to re-size, or select the bio max size.=0A=
=0A=
At the very least, I think that such limit should not be arbitrary as your =
patch=0A=
proposes but rely on the device characteristics (e.g.=0A=
max_hw_sectors_kb/max_sectors_kb and queue depth).=0A=
=0A=
> =0A=
>>=0A=
>>>=0A=
>>> Signed-off-by: Changheun Lee <nanich.lee@samsung.com>=0A=
>>> ---=0A=
>>>  block/bio.c         | 2 +-=0A=
>>>  include/linux/bio.h | 3 ++-=0A=
>>>  2 files changed, 3 insertions(+), 2 deletions(-)=0A=
>>>=0A=
>>> diff --git a/block/bio.c b/block/bio.c=0A=
>>> index 1f2cc1fbe283..dbe14d675f28 100644=0A=
>>> --- a/block/bio.c=0A=
>>> +++ b/block/bio.c=0A=
>>> @@ -877,7 +877,7 @@ bool __bio_try_merge_page(struct bio *bio, struct p=
age *page,=0A=
>>>  		struct bio_vec *bv =3D &bio->bi_io_vec[bio->bi_vcnt - 1];=0A=
>>>  =0A=
>>>  		if (page_is_mergeable(bv, page, len, off, same_page)) {=0A=
>>> -			if (bio->bi_iter.bi_size > UINT_MAX - len) {=0A=
>>> +			if (bio->bi_iter.bi_size > BIO_MAX_SIZE - len) {=0A=
>>>  				*same_page =3D false;=0A=
>>>  				return false;=0A=
>>>  			}=0A=
>>> diff --git a/include/linux/bio.h b/include/linux/bio.h=0A=
>>> index 1edda614f7ce..0f49b354b1f6 100644=0A=
>>> --- a/include/linux/bio.h=0A=
>>> +++ b/include/linux/bio.h=0A=
>>> @@ -20,6 +20,7 @@=0A=
>>>  #endif=0A=
>>>  =0A=
>>>  #define BIO_MAX_PAGES		256=0A=
>>> +#define BIO_MAX_SIZE		(BIO_MAX_PAGES * PAGE_SIZE)=0A=
>>>  =0A=
>>>  #define bio_prio(bio)			(bio)->bi_ioprio=0A=
>>>  #define bio_set_prio(bio, prio)		((bio)->bi_ioprio =3D prio)=0A=
>>> @@ -113,7 +114,7 @@ static inline bool bio_full(struct bio *bio, unsign=
ed len)=0A=
>>>  	if (bio->bi_vcnt >=3D bio->bi_max_vecs)=0A=
>>>  		return true;=0A=
>>>  =0A=
>>> -	if (bio->bi_iter.bi_size > UINT_MAX - len)=0A=
>>> +	if (bio->bi_iter.bi_size > BIO_MAX_SIZE - len)=0A=
>>>  		return true;=0A=
>>>  =0A=
>>>  	return false;=0A=
>>>=0A=
>>=0A=
>>=0A=
>> -- =0A=
>> Damien Le Moal=0A=
>> Western Digital Research=0A=
> =0A=
=0A=
=0A=
-- =0A=
Damien Le Moal=0A=
Western Digital Research=0A=