Message-ID: <SJ0PR20MB44099E58A9EF256E849C8FF4A0549@SJ0PR20MB4409.namprd20.prod.outlook.com>
Date:   Wed, 28 Sep 2022 01:02:13 +0000
From:   Rory Chen <rory.c.chen@...gate.com>
To:     Arie van der Hoeven <arie.vanderhoeven@...gate.com>,
        Paolo Valente <paolo.valente@...aro.org>
CC:     Tyler Erickson <tyler.erickson@...gate.com>,
        Muhammad Ahmad <muhammad.ahmad@...gate.com>,
        "linux-block@...r.kernel.org" <linux-block@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Jan Kara <jack@...e.cz>,
        "andrea.righi@...onical.com" <andrea.righi@...onical.com>,
        "glen.valante@...aro.org" <glen.valante@...aro.org>,
        "axboe@...nel.dk" <axboe@...nel.dk>,
        Michael English <michael.english@...gate.com>,
        Andrew Ring <andrew.ring@...gate.com>,
        Varun Boddu <varunreddy.boddu@...gate.com>,
        Damien Le Moal <damien.lemoal@....com>
Subject: RE: [PATCH 0/8] block, bfq: extend bfq to support multi-actuator
 drives

Hi Arie,

I have no experience to submit the change into kernel. Can Paolo or other maintainers help submit?


Seagate Internal

-----Original Message-----
From: Arie van der Hoeven <arie.vanderhoeven@...gate.com> 
Sent: Wednesday, September 28, 2022 12:59 AM
To: Rory Chen <rory.c.chen@...gate.com>; Paolo Valente <paolo.valente@...aro.org>
Cc: Tyler Erickson <tyler.erickson@...gate.com>; Muhammad Ahmad <muhammad.ahmad@...gate.com>; linux-block@...r.kernel.org; linux-kernel@...r.kernel.org; Jan Kara <jack@...e.cz>; andrea.righi@...onical.com; glen.valante@...aro.org; axboe@...nel.dk; Michael English <michael.english@...gate.com>; Andrew Ring <andrew.ring@...gate.com>; Varun Boddu <varunreddy.boddu@...gate.com>; Damien Le Moal <damien.lemoal@....com>
Subject: Re: [PATCH 0/8] block, bfq: extend bfq to support multi-actuator drives

Chiming in as we have customers who are having very good results with these BFQ patches and are planning to pilot NAS solutions in early 2023. This bug is not a blocker for us, but we do need the BFQ patches included in Linux 6.0.

Rory can you submit your changes or is it the maintainer's responsibility?

Regards,  --Arie


From: Rory Chen <mailto:rory.c.chen@...gate.com>
Sent: Thursday, September 8, 2022 4:34 AM
To: Paolo Valente <mailto:paolo.valente@...aro.org>
Cc: Tyler Erickson <mailto:tyler.erickson@...gate.com>; Arie van der Hoeven <mailto:arie.vanderhoeven@...gate.com>; Muhammad Ahmad <mailto:muhammad.ahmad@...gate.com>; mailto:linux-block@...r.kernel.org <mailto:linux-block@...r.kernel.org>; mailto:linux-kernel@...r.kernel.org <mailto:linux-kernel@...r.kernel.org>; Jan Kara <mailto:jack@...e.cz>; mailto:andrea.righi@...onical.com <mailto:andrea.righi@...onical.com>; mailto:glen.valante@...aro.org <mailto:glen.valante@...aro.org>; mailto:axboe@...nel.dk <mailto:axboe@...nel.dk>; Michael English <mailto:michael.english@...gate.com>; Andrew Ring <mailto:andrew.ring@...gate.com>; Varun Boddu <mailto:varunreddy.boddu@...gate.com>; Damien Le Moal <mailto:damien.lemoal@....com>
Subject: Re: [PATCH 0/8] block, bfq: extend bfq to support multi-actuator drives


Oops, I attach wrong code change. Here's the right change made by me.

<               if (end >= iar->sector + 1 && end < iar->sector + iar->nr_sectors + 1)  //Changed code
>               if (end >= iar->sector && end < iar->sector + 
> iar->nr_sectors) // Original code

Unfortunately, the crash is still existing and I can't find any clue from /var/log/messages



From: Paolo Valente <mailto:paolo.valente@...aro.org>
Sent: Thursday, September 8, 2022 6:54 PM
To: Rory Chen <mailto:rory.c.chen@...gate.com>
Cc: Tyler Erickson <mailto:tyler.erickson@...gate.com>; Arie van der Hoeven <mailto:arie.vanderhoeven@...gate.com>; Muhammad Ahmad <mailto:muhammad.ahmad@...gate.com>; mailto:linux-block@...r.kernel.org <mailto:linux-block@...r.kernel.org>; mailto:linux-kernel@...r.kernel.org <mailto:linux-kernel@...r.kernel.org>; Jan Kara <mailto:jack@...e.cz>; mailto:andrea.righi@...onical.com <mailto:andrea.righi@...onical.com>; mailto:glen.valante@...aro.org <mailto:glen.valante@...aro.org>; mailto:axboe@...nel.dk <mailto:axboe@...nel.dk>; Michael English <mailto:michael.english@...gate.com>; Andrew Ring <mailto:andrew.ring@...gate.com>; Varun Boddu <mailto:varunreddy.boddu@...gate.com>; Damien Le Moal <mailto:damien.lemoal@....com>
Subject: Re: [PATCH 0/8] block, bfq: extend bfq to support multi-actuator drives


This message has originated from an External Source. Please use proper judgment and caution when opening attachments, clicking links, or responding to this email.


> Il giorno 8 set 2022, alle ore 04:46, Rory Chen <mailto:rory.c.chen@...gate.com> ha scritto:
>
> I change the comparison condition and it can eliminate the warning.

Yep. The crash you reported also goes away?

> <               if (end >= iar->sector + 1 && end < iar->sector + iar->nr_sectors + 1)
>>              if (end >= iar->sector && end < iar->sector + 
>> iar->nr_sectors)
>
> I don't know if this change is appropriate

Unfortunately your change conflicts with the standard code, taken from the original patches on access ranges [1].  I've CCed Damien, the author of this patch series.

[1] https://secure-web.cisco.com/12uvPqOwOjHJPiVGM6hJ7791jpWxxy8My3bFD1oA0pNh9m0W778f8IM7HPxjRUL8-94N0gKahHwtK-sEv1Tgk2Oo4H9GTAlLoml_uWF6BGktvDAlDp-zdNQUzCL7y1OCz_MJMaNlS5h0iwsE3q9m7tJsCFUWW0YEgcJE6LRTrZDQpFJhG3pGCLFgoPIuKa3o8B136dJoQvEtek7ZOQFKqesuZKbu4lvM4ds0HOLs5TIgJR_mSJ8UmhP5_M3a1CaDxdDzQ784H3EydkRN9a6v9-Oogo-wYUqS8fRq35rUyw1t2IblmgJzr6aoGazZsJHxBXPjpxA9DSEQqUtH7oT5RGM4qxLpEmYjgyzpJUZqhUCSXye7-lCTIQIB-SGzRuZDVbIqK5tZd3F_YK9LcAN0iVH_qfBM4zRe_4w4h5ikJdhc/https%3A%2F%2Flwn.net%2Fml%2Flinux-block%2F20210909023545.1101672-2-damien.lemoal%40wdc.com%2F

Thanks,
Paolo

> but  bio_end_sector deducting 1 said by Tyler seems to make sense.
>
> From: Paolo Valente <mailto:paolo.valente@...aro.org>
> Sent: Thursday, August 25, 2022 10:45 PM
> To: Tyler Erickson <mailto:tyler.erickson@...gate.com>
> Cc: Rory Chen <mailto:rory.c.chen@...gate.com>; Arie van der Hoeven 
> <mailto:arie.vanderhoeven@...gate.com>; Muhammad Ahmad 
> <mailto:muhammad.ahmad@...gate.com>; mailto:linux-block@...r.kernel.org 
> <mailto:linux-block@...r.kernel.org>; mailto:linux-kernel@...r.kernel.org 
> <mailto:linux-kernel@...r.kernel.org>; Jan Kara <mailto:jack@...e.cz>; 
> mailto:andrea.righi@...onical.com <mailto:andrea.righi@...onical.com>; 
> mailto:glen.valante@...aro.org <mailto:glen.valante@...aro.org>; mailto:axboe@...nel.dk 
> <mailto:axboe@...nel.dk>; Michael English <mailto:michael.english@...gate.com>; 
> Andrew Ring <mailto:andrew.ring@...gate.com>; Varun Boddu 
> <mailto:varunreddy.boddu@...gate.com>
> Subject: Re: [PATCH 0/8] block, bfq: extend bfq to support 
> multi-actuator drives
>
>
> This message has originated from an External Source. Please use proper judgment and caution when opening attachments, clicking links, or responding to this email.
>
>
> Hi
>
>> Il giorno 18 ago 2022, alle ore 17:40, Tyler Erickson <mailto:tyler.erickson@...gate.com> ha scritto:
>>
>> The libata layer is reporting correctly after the changes I submitted.
>>
>> The drive reports the actuator ranges as a starting LBA and a count of LBAs for the range.
>> If the code reading the reported values simply does startingLBA + range, this is an incorrect ending LBA for that actuator. This is because LBAs are zero indexed and this simple addition is not taking that into account.
>> The proper way to get the endingLBA is startingLBA + range - 1 to get the last LBA value for where to issue a final IO read/write to account for LBA values starting at zero rather than one.
>>
>> Here is an example from the output in SeaChest/openSeaChest:
>> ====Concurrent Positioning Ranges====
>>
>> Range#     #Elements            Lowest LBA          # of LBAs
>>  0            1                                               0           17578328064
>>  1            1                         17578328064           17578328064
>>
>> If using the incorrect formula to get the final LBA for actuator 0, you would get 17578328064, but this is the starting LBA reported by the drive for actuator 1.
>> So to be consistent for all ranges, the final LBA for a given actuator should be calculated as starting LBA + range - 1.
>>
>
> Ok
>
>> I had reached out to Seagate's T10 and T13 representatives for clarification and verification and this is most likely what is causing the error is a missing - 1 somewhere after getting the information reported by the device. They agreed that the reporting from the drive and the SCSI to ATA translation is correct.
>>
>> I'm not sure where this is being read and calculated, but it is not an error in the low-level libata or sd level of the kernel. It may be in bfq, or it may be in some other place after the sd layer.
>
> This apparent mistake is in the macro bio_end_sector (defined in 
> include/linux/bio.h), which seems to be translated as sector+size.
> Jens, can you shed a light on this point?
>
> Thanks,
> Paolo
>
>> I know there were some additions to read this and report it up the stack, but I did not think those were wrong as they seemed to pass the drive reported information up the stack.
>>
>> Tyler Erickson
>> Seagate Technology
>>
>>
>> Seagate Internal
>>
>> -----Original Message-----
>> From: Rory Chen <mailto:rory.c.chen@...gate.com>
>> Sent: Wednesday, August 10, 2022 6:59 AM
>> To: Paolo Valente <mailto:paolo.valente@...aro.org>
>> Cc: Arie van der Hoeven <mailto:arie.vanderhoeven@...gate.com>; Muhammad 
>> Ahmad <mailto:muhammad.ahmad@...gate.com>; mailto:linux-block@...r.kernel.org; 
>> mailto:linux-kernel@...r.kernel.org; Jan Kara <mailto:jack@...e.cz>; 
>> mailto:andrea.righi@...onical.com; mailto:glen.valante@...aro.org; mailto:axboe@...nel.dk; 
>> Tyler Erickson <mailto:tyler.erickson@...gate.com>; Michael English 
>> <mailto:michael.english@...gate.com>; Andrew Ring <mailto:andrew.ring@...gate.com>; 
>> Varun Boddu <mailto:varunreddy.boddu@...gate.com>
>> Subject: Re: [PATCH 0/8] block, bfq: extend bfq to support 
>> multi-actuator drives
>>
>> The block trace shows the start sector is 35156656120 and transfer length is 8 sectors, which is within the max LBA 35156656127 of drive. And this IO is completed successfully from the slice of parsed block trace though reporting the warning message.
>> 8,64   7       13     0.039401337 19176  Q  RA 35156656120 + 8 [systemd-udevd]
>> 8,64   7       15     0.039403946 19176  P   N [systemd-udevd]
>> 8,64   7       16     0.039405132 19176  I  RA 35156656120 + 8 [systemd-udevd]
>> 8,64   7       18     0.039411554 19176  D  RA 35156656120 + 8 [systemd-udevd]
>> 8,64   0       40     0.039479055     0  C  RA 35156656120 + 8 [0]
>>
>> It may need to know where calculate "bio_end_sector" value as 35156656128. I have patched libata and sd driver for Dual Actuator.
>>
>>
>>
>> From: Paolo Valente <mailto:paolo.valente@...aro.org>
>> Sent: Wednesday, August 10, 2022 6:22 PM
>> To: Rory Chen <mailto:rory.c.chen@...gate.com>
>> Cc: Arie van der Hoeven <mailto:arie.vanderhoeven@...gate.com>; Muhammad 
>> Ahmad <mailto:muhammad.ahmad@...gate.com>; mailto:linux-block@...r.kernel.org 
>> <mailto:linux-block@...r.kernel.org>; mailto:linux-kernel@...r.kernel.org 
>> <mailto:linux-kernel@...r.kernel.org>; Jan Kara <mailto:jack@...e.cz>; 
>> mailto:andrea.righi@...onical.com <mailto:andrea.righi@...onical.com>; 
>> mailto:glen.valante@...aro.org <mailto:glen.valante@...aro.org>; mailto:axboe@...nel.dk 
>> <mailto:axboe@...nel.dk>; Tyler Erickson <mailto:tyler.erickson@...gate.com>; 
>> Michael English <mailto:michael.english@...gate.com>; Andrew Ring 
>> <mailto:andrew.ring@...gate.com>; Varun Boddu <mailto:varunreddy.boddu@...gate.com>
>> Subject: Re: [PATCH 0/8] block, bfq: extend bfq to support 
>> multi-actuator drives
>>
>>
>> This message has originated from an External Source. Please use proper judgment and caution when opening attachments, clicking links, or responding to this email.
>>
>>
>>> Il giorno 9 ago 2022, alle ore 05:47, Rory Chen <mailto:rory.c.chen@...gate.com> ha scritto:
>>>
>>> Resend the mail as plain text because previous mail with rich text 
>>> makes some mess and forget to add others at Seagate who worked on 
>>> validating the patch as well(Muhammad, Michael, Andrew, Varun,Tyler)
>>>
>>> Hi Paolo,
>>>
>>
>> Hi
>>
>>> I am from Seagate China and face a problem when I'm evaluating the bfq patches. Could you please check?
>>> Thanks
>>>
>>> Issue statement
>>> When running performance test on bfq patch, I observed warning message "bfq_actuator_index: bio sector out of ranges: end=35156656128" and OS hung suddenly after some hours.
>>> The warning message is reported from function bfq_actuator_index which determines IO request is in which index of actuators.  The bio_end_sector is 35156656128 but the max LBA for the drive is 35156656127 so it's beyond the LBA range.
>>
>> Yep, this sanity check fails if the end sector of a new IO does not belong to any sector range.
>>
>>> I captured the block trace and didn't found request LBA 35156656128 instead only found max request LBA 35156656127.
>>
>> Maybe in the trace you see only start sectors?  The failed check si performed on end sectors instead.
>>
>> At any rate, there seems to be an off-by-one error in the value(s) stored in the sector field(s) of the blk_independent_access_range data structure.
>>
>> I guess we may need some help/feedback from people competent on this stuff.
>>
>>> I'm not sure if this warning message is related to later OS hung.
>>>
>>
>> Not easy to say.  At any rate, we can try with a development version of bfq.  It can help us detect the possible cause of this hang.  But let's see where we get with this sector error first.
>>
>> Thank you for testing this extended version of bfq, Paolo
>>
>>>
>>> Problem environment
>>> Kernel base is 5.18.9
>>> Test HDD drive is Seagate ST18000NM0092 dual actuator SATA.
>>> Actuator LBA mapping by reading VPD B9 Concurrent positioning ranges 
>>> VPD page:
>>> LBA range number:0
>>> number of storage elements:1
>>> starting LBA:0x0
>>> number of LBAs:0x417c00000 [17578328064] LBA range number:1 number 
>>> of storage elements:1 starting LBA:0x417c00000 number of 
>>> LBAs:0x417c00000 [17578328064]
>>>
>>>
>>>
>>>
>>>
>>> From: Paolo Valente <mailto:paolo.valente@...aro.org>
>>> Sent: Thursday, June 23, 2022 8:53 AM
>>> To: Jens Axboe <mailto:axboe@...nel.dk>
>>> Cc: mailto:linux-block@...r.kernel.org <mailto:linux-block@...r.kernel.org>; 
>>> mailto:linux-kernel@...r.kernel.org <mailto:linux-kernel@...r.kernel.org>; 
>>> mailto:jack@...e.cz <mailto:jack@...e.cz>; mailto:andrea.righi@...onical.com 
>>> <mailto:andrea.righi@...onical.com>; mailto:glen.valante@...aro.org 
>>> <mailto:glen.valante@...aro.org>; Arie van der Hoeven 
>>> <mailto:arie.vanderhoeven@...gate.com>; Paolo Valente 
>>> <mailto:paolo.valente@...aro.org>
>>> Subject: [PATCH 0/8] block, bfq: extend bfq to support 
>>> multi-actuator drives
>>>
>>>
>>> This message has originated from an External Source. Please use proper judgment and caution when opening attachments, clicking links, or responding to this email.
>>>
>>>
>>> Hi,
>>> this patch series extends BFQ so as to optimize I/O dispatch to 
>>> multi-actuator drives. In particular, this extension addresses the 
>>> following issue. Multi-actuator drives appear as a single device to 
>>> the I/O subsystem [1].  Yet they address commands to different 
>>> actuators internally, as a function of Logical Block Addressing 
>>> (LBAs). A given sector is reachable by only one of the actuators. 
>>> For example, Seagate's Serial Advanced Technology Attachment (SATA) 
>>> version contains two actuators and maps the lower half of the SATA 
>>> LBA space to the lower actuator and the upper half to the upper actuator.
>>>
>>> Evidently, to fully utilize actuators, no actuator must be left idle 
>>> or underutilized while there is pending I/O for it. To reach this 
>>> goal, the block layer must somehow control the load of each actuator 
>>> individually. This series enriches BFQ with such a per-actuator 
>>> control, as a first step. Then it also adds a simple mechanism for 
>>> guaranteeing that actuators with pending I/O are never left idle.
>>>
>>> See [1] for a more detailed overview of the problem and of the 
>>> solutions implemented in this patch series. There you will also find 
>>> some preliminary performance results.
>>>
>>> Thanks,
>>> Paolo
>>>
>>> [1]
>>> https://secure-web.cisco.com/1hcxnN1C3h1nW7mby7S66_LE8szirQwbQI0fBpY
>>> eP 
>>> rA0GTWfyuQyl0GpZaOn32xMSkNT0BUQWloDHFzZ23aYDZdi8NfdrEFLY9pQDBblIvn08
>>> LR 
>>> iTVoIOUC8zWSG_r2PCyLtx3ppZq5cWOib_8azxteRRcbKWGdbLPSqg9hfSJSqltth0By
>>> LO 
>>> NHEoI3p3e9QNIn6nVAeQbsT3aOQe-F95XrQvaPrFJXx6RGL9kDXyfkbXIHcdcLBf895g
>>> YB 
>>> Fn5S2WjBDQq2kzDzZOlc1HekRUhg0qDQcFY6NydVfrqNfLbpAHAth6KyREscQhVTMVRE
>>> EV 
>>> a1b6bQByX6grF5pn3pTIo0lODyfX6yRmcbReSYNfOZ65ZPvp-nH530FQ-5nXoRxFf74W
>>> IK 
>>> DrNTALs3xQvg03DH4jLez-T2M9xEu-sfEDAEdTGF7BcnmBW6vrPO4_p3k4/https%3A%
>>> 2F 
>>> %2Fwww.linaro.org%2Fblog%2Fbudget-fair-queueing-bfq-linux-io-schedul
>>> er -optimizations-for-multi-actuator-sata-hard-drives%2F
>>>
>>> Davide Zini (3):
>>> block, bfq: split also async bfq_queues on a per-actuator basis 
>>> block, bfq: inject I/O to underutilized actuators  block, bfq: 
>>> balance I/O injection among underutilized actuators
>>>
>>> Federico Gavioli (1):
>>> block, bfq: retrieve independent access ranges from request queue
>>>
>>> Paolo Valente (4):
>>> block, bfq: split sync bfq_queues on a per-actuator basis  block,
>>> bfq: forbid stable merging of queues associated with different  
>>> actuators block, bfq: turn scalar fields into arrays in bfq_io_cq  
>>> block, bfq:
>>> turn BFQ_NUM_ACTUATORS into BFQ_MAX_ACTUATORS
>>>
>>> block/bfq-cgroup.c  |  97 +++++----
>>> block/bfq-iosched.c | 488 
>>> +++++++++++++++++++++++++++++---------------
>>> block/bfq-iosched.h | 149 ++++++++++----
>>> block/bfq-wf2q.c    |   2 +-
>>> 4 files changed, 493 insertions(+), 243 deletions(-)
>>>
>>> --
>>> 2.20.1
>>>
>>>
>>> Seagate Internal
>>>
>>> Seagate Internal
>>
>> Seagate Internal
>
> Seagate Internal


Seagate Internal

Seagate Internal

Powered by blists - more mailing lists