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;
From:   Changheun Lee <nanich.lee@samsung.com>
To:     damien.lemoal@wdc.com
Cc:     Johannes.Thumshirn@wdc.com, axboe@kernel.dk,
        jisoo2146.oh@samsung.com, junho89.kim@samsung.com,
        linux-block@vger.kernel.org, linux-kernel@vger.kernel.org,
        ming.lei@redhat.com, mj0123.lee@samsung.com,
        nanich.lee@samsung.com, seunghwan.hyun@samsung.com,
        sookwan7.kim@samsung.com, tj@kernel.org, tom.leiming@gmail.com,
        woosung2.lee@samsung.com, yt0928.kim@samsung.com
Subject: Re: [PATCH] bio: limit bio max size.
Date:   Thu, 14 Jan 2021 13:35:10 +0900
Message-Id: <20210114043510.7379-1-nanich.lee@samsung.com>
In-Reply-To: <BL0PR04MB651470CA7B3AA843E57AF85EE7A80@BL0PR04MB6514.namprd04.prod.outlook.com>
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Content-Type: text/plain; charset="utf-8"
CMS-TYPE: 101P
DLP-Filter: Pass
References: <BL0PR04MB651470CA7B3AA843E57AF85EE7A80@BL0PR04MB6514.namprd04.prod.outlook.com>
        <CGME20210114045019epcas1p16d4f5f258c2a3b290540ac640745764d@epcas1p1.samsung.com>
Precedence: bulk

>On 2021/01/14 12:53, Ming Lei wrote:
>> On Wed, Jan 13, 2021 at 12:02:44PM +0000, Damien Le Moal wrote:
>>> On 2021/01/13 20:48, Ming Lei wrote:
>>>> On Wed, Jan 13, 2021 at 11:16:11AM +0000, Damien Le Moal wrote:
>>>>> On 2021/01/13 19:25, Ming Lei wrote:
>>>>>> On Wed, Jan 13, 2021 at 09:28:02AM +0000, Damien Le Moal wrote:
>>>>>>> On 2021/01/13 18:19, Ming Lei wrote:
>>>>>>>> On Wed, Jan 13, 2021 at 12:09 PM Changheun Lee <nanich.lee@samsung.com> wrote:
>>>>>>>>>
>>>>>>>>>> On 2021/01/12 21:14, Changheun Lee wrote:
>>>>>>>>>>>> On 2021/01/12 17:52, Changheun Lee wrote:
>>>>>>>>>>>>> From: "Changheun Lee" <nanich.lee@samsung.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 ?
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> 32MB total latency is about 19ms including merge time without this patch.
>>>>>>>>> But with this patch, total latency is about 17ms including merge time too.
>>>>>>>>
>>>>>>>> 19ms looks too big just for preparing one 32MB sized bio, which isn't
>>>>>>>> supposed to
>>>>>>>> take so long.  Can you investigate where the 19ms is taken just for
>>>>>>>> preparing one
>>>>>>>> 32MB sized bio?
>>>>>>>
>>>>>>> Changheun mentioned that the device side IO latency is 16.7ms out of the 19ms
>>>>>>> total. So the BIO handling, submission+completion takes about 2.3ms, and
>>>>>>> Changheun points above to 2ms for the submission part.
>>>>>>
>>>>>> OK, looks I misunderstood the data.
>>>>>>
>>>>>>>
>>>>>>>>
>>>>>>>> It might be iov_iter_get_pages() for handling page fault. If yes, one suggestion
>>>>>>>> is to enable THP(Transparent HugePage Support) in your application.
>>>>>>>
>>>>>>> But if that was due to page faults, the same large-ish time would be taken for
>>>>>>> the preparing the size-limited BIOs too, no ? No matter how the BIOs are diced,
>>>>>>> all 32MB of pages of the user IO are referenced...
>>>>>>
>>>>>> If bio size is reduced to 1MB, just 256 pages need to be faulted before submitting this
>>>>>> bio, instead of 256*32 pages, that is why the following words are mentioned:
>>>>>>
>>>>>> 	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.
>>>>>
>>>>> Yes, but eventually, all pages for the 32MB IO will be faulted in, just not in
>>>>> one go. Overall number of page faults is likely the same as with the large BIO
>>>>> preparation. So I think we are back to my previous point, that is, reducing the
>>>>> device idle time by starting a BIO more quickly, even a small one, leads to
>>>>> overlap between CPU time needed for the next BIO preparation and previous BIO
>>>>> execution, reducing overall the latency for the entire 32MB user IO.
>>>>
>>>> When bio size is reduced from 32M to 1M:
>>>>
>>>> 1MB/(P(1M) + D(1M)) may become bigger than 32MB/(P(1M) + D(1M)), so
>>>> throughput is improved.
>>>
>>> I think that the reason is that P(1M) < D(1M) and so there is overlap between P
>>> and D: P of the next BIO is done on the CPU while D of the previous BIO is
>>> ongoing on the device, assuming there is no plugging.
>> 
>> Looks you are talking about AIO. IMO, if AIO is used in Changheun's
>> test, the UFS controller pipeline can be saturated easily by many
>> enough(> 8 or more) 32M requests(preparing each takes 2ms, and device need
>> 16ms to handle 32MB req), then there shouldn't be such issue.
>> 
>> So I guess Changheun uses sync dio, and the 2ms preparing time is added
>> to bio submission delay every time.
>> 
>> Changheun, can you talk about your 32MB block size direct IO test in a
>> bit detail? AIO or sync dio? Do you have fio command line to reproduce
>> this issue?
>
>Maybe also provide a blktrace output of for one 32MB IO execution ?

When 32MB chunk read with direct I/O option is comming 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.

>
>> 
>> 
>> Thanks, 
>> Ming
>> 
>> 
>
>
>-- 
>Damien Le Moal
>Western Digital Research
>