Message-ID: <8d5ffebe-39dc-b811-ce7c-9df4b5d061c1@opensource.wdc.com>
Date:   Sat, 1 Oct 2022 09:50:13 +0900
From:   Damien Le Moal <damien.lemoal@...nsource.wdc.com>
To:     Paolo Valente <paolo.valente@...aro.org>,
        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: block: wrong return value by bio_end_sector?

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

Powered by blists - more mailing lists