> Il giorno 28 set 2022, alle ore 03:02, Rory Chen <rory.c.chen@xxxxxxxxxxx> 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@xxxxxxxxxxx> > Sent: Wednesday, September 28, 2022 12:59 AM > To: Rory Chen <rory.c.chen@xxxxxxxxxxx>; Paolo Valente <paolo.valente@xxxxxxxxxx> > Cc: Tyler Erickson <tyler.erickson@xxxxxxxxxxx>; Muhammad Ahmad <muhammad.ahmad@xxxxxxxxxxx>; linux-block@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; Jan Kara <jack@xxxxxxx>; andrea.righi@xxxxxxxxxxxxx; glen.valante@xxxxxxxxxx; axboe@xxxxxxxxx; Michael English <michael.english@xxxxxxxxxxx>; Andrew Ring <andrew.ring@xxxxxxxxxxx>; Varun Boddu <varunreddy.boddu@xxxxxxxxxxx>; Damien Le Moal <damien.lemoal@xxxxxxx> > 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@xxxxxxxxxxx> > Sent: Thursday, September 8, 2022 4:34 AM > To: Paolo Valente <mailto:paolo.valente@xxxxxxxxxx> > Cc: Tyler Erickson <mailto:tyler.erickson@xxxxxxxxxxx>; Arie van der Hoeven <mailto:arie.vanderhoeven@xxxxxxxxxxx>; Muhammad Ahmad <mailto:muhammad.ahmad@xxxxxxxxxxx>; mailto:linux-block@xxxxxxxxxxxxxxx <mailto:linux-block@xxxxxxxxxxxxxxx>; mailto:linux-kernel@xxxxxxxxxxxxxxx <mailto:linux-kernel@xxxxxxxxxxxxxxx>; Jan Kara <mailto:jack@xxxxxxx>; mailto:andrea.righi@xxxxxxxxxxxxx <mailto:andrea.righi@xxxxxxxxxxxxx>; mailto:glen.valante@xxxxxxxxxx <mailto:glen.valante@xxxxxxxxxx>; mailto:axboe@xxxxxxxxx <mailto:axboe@xxxxxxxxx>; Michael English <mailto:michael.english@xxxxxxxxxxx>; Andrew Ring <mailto:andrew.ring@xxxxxxxxxxx>; Varun Boddu <mailto:varunreddy.boddu@xxxxxxxxxxx>; Damien Le Moal <mailto:damien.lemoal@xxxxxxx> > 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@xxxxxxxxxx> > Sent: Thursday, September 8, 2022 6:54 PM > To: Rory Chen <mailto:rory.c.chen@xxxxxxxxxxx> > Cc: Tyler Erickson <mailto:tyler.erickson@xxxxxxxxxxx>; Arie van der Hoeven <mailto:arie.vanderhoeven@xxxxxxxxxxx>; Muhammad Ahmad <mailto:muhammad.ahmad@xxxxxxxxxxx>; mailto:linux-block@xxxxxxxxxxxxxxx <mailto:linux-block@xxxxxxxxxxxxxxx>; mailto:linux-kernel@xxxxxxxxxxxxxxx <mailto:linux-kernel@xxxxxxxxxxxxxxx>; Jan Kara <mailto:jack@xxxxxxx>; mailto:andrea.righi@xxxxxxxxxxxxx <mailto:andrea.righi@xxxxxxxxxxxxx>; mailto:glen.valante@xxxxxxxxxx <mailto:glen.valante@xxxxxxxxxx>; mailto:axboe@xxxxxxxxx <mailto:axboe@xxxxxxxxx>; Michael English <mailto:michael.english@xxxxxxxxxxx>; Andrew Ring <mailto:andrew.ring@xxxxxxxxxxx>; Varun Boddu <mailto:varunreddy.boddu@xxxxxxxxxxx>; Damien Le Moal <mailto:damien.lemoal@xxxxxxx> > 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@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://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@xxxxxxxxxx> >> Sent: Thursday, August 25, 2022 10:45 PM >> To: Tyler Erickson <mailto:tyler.erickson@xxxxxxxxxxx> >> Cc: Rory Chen <mailto:rory.c.chen@xxxxxxxxxxx>; Arie van der Hoeven >> <mailto:arie.vanderhoeven@xxxxxxxxxxx>; Muhammad Ahmad >> <mailto:muhammad.ahmad@xxxxxxxxxxx>; mailto:linux-block@xxxxxxxxxxxxxxx >> <mailto:linux-block@xxxxxxxxxxxxxxx>; mailto:linux-kernel@xxxxxxxxxxxxxxx >> <mailto:linux-kernel@xxxxxxxxxxxxxxx>; Jan Kara <mailto:jack@xxxxxxx>; >> mailto:andrea.righi@xxxxxxxxxxxxx <mailto:andrea.righi@xxxxxxxxxxxxx>; >> mailto:glen.valante@xxxxxxxxxx <mailto:glen.valante@xxxxxxxxxx>; mailto:axboe@xxxxxxxxx >> <mailto:axboe@xxxxxxxxx>; Michael English <mailto:michael.english@xxxxxxxxxxx>; >> Andrew Ring <mailto:andrew.ring@xxxxxxxxxxx>; Varun Boddu >> <mailto: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 <mailto: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 <mailto:rory.c.chen@xxxxxxxxxxx> >>> Sent: Wednesday, August 10, 2022 6:59 AM >>> To: Paolo Valente <mailto:paolo.valente@xxxxxxxxxx> >>> Cc: Arie van der Hoeven <mailto:arie.vanderhoeven@xxxxxxxxxxx>; Muhammad >>> Ahmad <mailto:muhammad.ahmad@xxxxxxxxxxx>; mailto:linux-block@xxxxxxxxxxxxxxx; >>> mailto:linux-kernel@xxxxxxxxxxxxxxx; Jan Kara <mailto:jack@xxxxxxx>; >>> mailto:andrea.righi@xxxxxxxxxxxxx; mailto:glen.valante@xxxxxxxxxx; mailto:axboe@xxxxxxxxx; >>> Tyler Erickson <mailto:tyler.erickson@xxxxxxxxxxx>; Michael English >>> <mailto:michael.english@xxxxxxxxxxx>; Andrew Ring <mailto:andrew.ring@xxxxxxxxxxx>; >>> Varun Boddu <mailto: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 <mailto:paolo.valente@xxxxxxxxxx> >>> Sent: Wednesday, August 10, 2022 6:22 PM >>> To: Rory Chen <mailto:rory.c.chen@xxxxxxxxxxx> >>> Cc: Arie van der Hoeven <mailto:arie.vanderhoeven@xxxxxxxxxxx>; Muhammad >>> Ahmad <mailto:muhammad.ahmad@xxxxxxxxxxx>; mailto:linux-block@xxxxxxxxxxxxxxx >>> <mailto:linux-block@xxxxxxxxxxxxxxx>; mailto:linux-kernel@xxxxxxxxxxxxxxx >>> <mailto:linux-kernel@xxxxxxxxxxxxxxx>; Jan Kara <mailto:jack@xxxxxxx>; >>> mailto:andrea.righi@xxxxxxxxxxxxx <mailto:andrea.righi@xxxxxxxxxxxxx>; >>> mailto:glen.valante@xxxxxxxxxx <mailto:glen.valante@xxxxxxxxxx>; mailto:axboe@xxxxxxxxx >>> <mailto:axboe@xxxxxxxxx>; Tyler Erickson <mailto:tyler.erickson@xxxxxxxxxxx>; >>> Michael English <mailto:michael.english@xxxxxxxxxxx>; Andrew Ring >>> <mailto:andrew.ring@xxxxxxxxxxx>; Varun Boddu <mailto: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 <mailto: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 <mailto:paolo.valente@xxxxxxxxxx> >>>> Sent: Thursday, June 23, 2022 8:53 AM >>>> To: Jens Axboe <mailto:axboe@xxxxxxxxx> >>>> Cc: mailto:linux-block@xxxxxxxxxxxxxxx <mailto:linux-block@xxxxxxxxxxxxxxx>; >>>> mailto:linux-kernel@xxxxxxxxxxxxxxx <mailto:linux-kernel@xxxxxxxxxxxxxxx>; >>>> mailto:jack@xxxxxxx <mailto:jack@xxxxxxx>; mailto:andrea.righi@xxxxxxxxxxxxx >>>> <mailto:andrea.righi@xxxxxxxxxxxxx>; mailto:glen.valante@xxxxxxxxxx >>>> <mailto:glen.valante@xxxxxxxxxx>; Arie van der Hoeven >>>> <mailto:arie.vanderhoeven@xxxxxxxxxxx>; Paolo Valente >>>> <mailto: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_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
