Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Paolo Valente <paolo.valente@linaro.org>
Content-Type: text/plain;
        charset=us-ascii
Content-Transfer-Encoding: quoted-printable
Mime-Version: 1.0 (Mac OS X Mail 12.4 \(3445.104.11\))
Subject: block: wrong return value by bio_end_sector?
Message-Id: <D4FC5552-B3C8-4118-B3C8-C6BF20C793B4@linaro.org>
Date:   Fri, 30 Sep 2022 17:59:26 +0200
To:     Arie van der Hoeven <arie.vanderhoeven@seagate.com>,
        Tyler Erickson <tyler.erickson@seagate.com>,
        Muhammad Ahmad <muhammad.ahmad@seagate.com>,
        "linux-block@vger.kernel.org" <linux-block@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        Jan Kara <jack@suse.cz>,
        "andrea.righi@canonical.com" <andrea.righi@canonical.com>,
        "glen.valante@linaro.org" <glen.valante@linaro.org>,
        "axboe@kernel.dk" <axboe@kernel.dk>,
        Michael English <michael.english@seagate.com>,
        Andrew Ring <andrew.ring@seagate.com>,
        Varun Boddu <varunreddy.boddu@seagate.com>,
        Damien Le Moal <damien.lemoal@wdc.com>
Precedence: bulk

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.

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. 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:
=3D=3D=3D=3DConcurrent Positioning Ranges=3D=3D=3D=3D

Range#     #Elements            Lowest LBA          # of LBAs     =20
  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?

Thanks,
Paolo

[1] https://www.spinics.net/lists/kernel/msg4507408.html=