On 10/1/22 00:59, Paolo Valente wrote: > Hi Jens, Damien, all other possibly interested people, this is to raise > attention on a mistake that has emerged in a thread on a bfq extension > for multi-actuary drives [1]. > > The mistake is apparently in the macro bio_end_sector (defined in > include/linux/bio.h), which seems to be translated (incorrectly) as > sector+size, and not as sector+size-1. This has been like this for a long time, I think. > > For your convenience, I'm pasting a detailed description of the > problem, by Tyler (description taken from the above thread [1]). > > 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. Well, yes. LBA 0 + drive capacity is also an incorrect LBA. If the code assumes that it is, you have a classic off-by-one bug. > 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. Yes. And ? Where is the issue ? > > 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. > > 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. 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. > > Jens, Damien, can you shed a light on this? I am not clear on what the problem is exactly. This all sound like a simple off-by-one issue if bfq support code. No ? > > Thanks, Paolo > > [1] https://www.spinics.net/lists/kernel/msg4507408.html -- Damien Le Moal Western Digital Research
