Re: [PATCH 0/8] block, bfq: extend bfq to support multi-actuator drives

[Paolo Valente]

> Il giorno 8 set 2022, alle ore 04:46, Rory Chen <rory.c.chen@xxxxxxxxxxx> 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://lwn.net/ml/linux-block/20210909023545.1101672-2-damien.lemoal@xxxxxxx/

Thanks,
Paolo

> but  bio_end_sector deducting 1 said by Tyler seems to make sense.
> 
> From: Paolo Valente <paolo.valente@xxxxxxxxxx>
> Sent: Thursday, August 25, 2022 10:45 PM
> To: Tyler Erickson <tyler.erickson@xxxxxxxxxxx>
> Cc: Rory Chen <rory.c.chen@xxxxxxxxxxx>; Arie van der Hoeven <arie.vanderhoeven@xxxxxxxxxxx>; Muhammad Ahmad <muhammad.ahmad@xxxxxxxxxxx>; linux-block@xxxxxxxxxxxxxxx <linux-block@xxxxxxxxxxxxxxx>; linux-kernel@xxxxxxxxxxxxxxx <linux-kernel@xxxxxxxxxxxxxxx>; Jan Kara <jack@xxxxxxx>; andrea.righi@xxxxxxxxxxxxx <andrea.righi@xxxxxxxxxxxxx>; glen.valante@xxxxxxxxxx <glen.valante@xxxxxxxxxx>; axboe@xxxxxxxxx <axboe@xxxxxxxxx>; Michael English <michael.english@xxxxxxxxxxx>; Andrew Ring <andrew.ring@xxxxxxxxxxx>; Varun Boddu <varunreddy.boddu@xxxxxxxxxxx>
> 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 <tyler.erickson@xxxxxxxxxxx> 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 <rory.c.chen@xxxxxxxxxxx>
>> Sent: Wednesday, August 10, 2022 6:59 AM
>> To: Paolo Valente <paolo.valente@xxxxxxxxxx>
>> Cc: Arie van der Hoeven <arie.vanderhoeven@xxxxxxxxxxx>; Muhammad Ahmad <muhammad.ahmad@xxxxxxxxxxx>; linux-block@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; Jan Kara <jack@xxxxxxx>; andrea.righi@xxxxxxxxxxxxx; glen.valante@xxxxxxxxxx; axboe@xxxxxxxxx; Tyler Erickson <tyler.erickson@xxxxxxxxxxx>; Michael English <michael.english@xxxxxxxxxxx>; Andrew Ring <andrew.ring@xxxxxxxxxxx>; Varun Boddu <varunreddy.boddu@xxxxxxxxxxx>
>> 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 <paolo.valente@xxxxxxxxxx>
>> Sent: Wednesday, August 10, 2022 6:22 PM
>> To: Rory Chen <rory.c.chen@xxxxxxxxxxx>
>> Cc: Arie van der Hoeven <arie.vanderhoeven@xxxxxxxxxxx>; Muhammad Ahmad <muhammad.ahmad@xxxxxxxxxxx>; linux-block@xxxxxxxxxxxxxxx <linux-block@xxxxxxxxxxxxxxx>; linux-kernel@xxxxxxxxxxxxxxx <linux-kernel@xxxxxxxxxxxxxxx>; Jan Kara <jack@xxxxxxx>; andrea.righi@xxxxxxxxxxxxx <andrea.righi@xxxxxxxxxxxxx>; glen.valante@xxxxxxxxxx <glen.valante@xxxxxxxxxx>; axboe@xxxxxxxxx <axboe@xxxxxxxxx>; Tyler Erickson <tyler.erickson@xxxxxxxxxxx>; Michael English <michael.english@xxxxxxxxxxx>; Andrew Ring <andrew.ring@xxxxxxxxxxx>; Varun Boddu <varunreddy.boddu@xxxxxxxxxxx>
>> 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 <rory.c.chen@xxxxxxxxxxx> 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 <paolo.valente@xxxxxxxxxx>
>>> Sent: Thursday, June 23, 2022 8:53 AM
>>> To: Jens Axboe <axboe@xxxxxxxxx>
>>> Cc: linux-block@xxxxxxxxxxxxxxx <linux-block@xxxxxxxxxxxxxxx>;
>>> linux-kernel@xxxxxxxxxxxxxxx <linux-kernel@xxxxxxxxxxxxxxx>;
>>> jack@xxxxxxx <jack@xxxxxxx>; andrea.righi@xxxxxxxxxxxxx
>>> <andrea.righi@xxxxxxxxxxxxx>; glen.valante@xxxxxxxxxx
>>> <glen.valante@xxxxxxxxxx>; Arie van der Hoeven
>>> <arie.vanderhoeven@xxxxxxxxxxx>; Paolo Valente
>>> <paolo.valente@xxxxxxxxxx>
>>> 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_LE8szirQwbQI0fBpYeP
>>> rA0GTWfyuQyl0GpZaOn32xMSkNT0BUQWloDHFzZ23aYDZdi8NfdrEFLY9pQDBblIvn08LR
>>> iTVoIOUC8zWSG_r2PCyLtx3ppZq5cWOib_8azxteRRcbKWGdbLPSqg9hfSJSqltth0ByLO
>>> NHEoI3p3e9QNIn6nVAeQbsT3aOQe-F95XrQvaPrFJXx6RGL9kDXyfkbXIHcdcLBf895gYB
>>> Fn5S2WjBDQq2kzDzZOlc1HekRUhg0qDQcFY6NydVfrqNfLbpAHAth6KyREscQhVTMVREEV
>>> a1b6bQByX6grF5pn3pTIo0lODyfX6yRmcbReSYNfOZ65ZPvp-nH530FQ-5nXoRxFf74WIK
>>> DrNTALs3xQvg03DH4jLez-T2M9xEu-sfEDAEdTGF7BcnmBW6vrPO4_p3k4/https%3A%2F
>>> %2Fwww.linaro.org%2Fblog%2Fbudget-fair-queueing-bfq-linux-io-scheduler
>>> -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