Message-Id: <09055269-6707-402C-9A69-2BA720886E17@linaro.org>
Date:   Fri, 30 Sep 2022 17:48:40 +0200
From:   Paolo Valente <paolo.valente@...aro.org>
To:     Rory Chen <rory.c.chen@...gate.com>
Cc:     Arie van der Hoeven <arie.vanderhoeven@...gate.com>,
        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



> Il giorno 28 set 2022, alle ore 03:02, Rory Chen <rory.c.chen@...gate.com> ha scritto:
> 
> Hi Arie,
> 
> I have no experience to submit the change into kernel. Can Paolo or other maintainers help submit?
> 
> 

Hi Rory,
the problem here is that your change simply switches off the warning
in BFQ, but the offending sector number is still produced by
bio_end_sector.  What we need here is some feedback by Damien or Jens.
Maybe they are not following this thread.  I'll try to get their
attention by creating a new thread on the topic.  Cross your fingers :)

Thanks,
Paolo

> 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