On 10/28/22 00:22, Damien Le Moal wrote:
On 10/27/22 18:42, Hannes Reinecke wrote:
On 10/27/22 09:50, Damien Le Moal wrote:
If a user issues a write command with the FUA bit set for a device with
NCQ support disabled (that is, the device queue depth was set to 1), the
LBA 48 command WRITE DMA FUA EXT must be used. However,
ata_build_rw_tf() ignores this and first test if LBA 28 can be used.
That is, for small FUA writes at low LBAs, ata_rwcmd_protocol() will
cause the write to fail.
Fix this by preventing the use of LBA 28 for any FUA write request.
While at it, also early test if the request is a FUA read and fail these
requests for the NCQ-disabled case instead of relying on
ata_rwcmd_protocol() returning an error.
Signed-off-by: Damien Le Moal <damien.lemoal@xxxxxxxxxxxxxxxxxx>
---
drivers/ata/libata-core.c | 17 +++++++++++++++--
1 file changed, 15 insertions(+), 2 deletions(-)
diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 81b20ffb1554..fea06f41f371 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -725,9 +725,21 @@ int ata_build_rw_tf(struct ata_queued_cmd *qc, u64 block, u32 n_block,
class == IOPRIO_CLASS_RT)
tf->hob_nsect |= ATA_PRIO_HIGH << ATA_SHIFT_PRIO;
} else if (dev->flags & ATA_DFLAG_LBA) {
+ bool lba28_ok;
+
+ if (tf->flags & ATA_TFLAG_FUA) {
+ /* FUA reads are not defined */
+ if (!(tf->flags & ATA_TFLAG_WRITE))
+ return -EINVAL;
+ /* We need LBA48 / WRITE DMA FUA EXT for FUA writes */
+ lba28_ok = false;
+ } else {
+ lba28_ok = lba_28_ok(block, n_block);
+ }
+
tf->flags |= ATA_TFLAG_LBA;
- if (lba_28_ok(block, n_block)) {
+ if (lba28_ok) {
/* use LBA28 */
tf->device |= (block >> 24) & 0xf;
} else if (lba_48_ok(block, n_block)) {
I am still skeptical about this change.
Having checked the code I don't think that we ever issue a
REQ_READ|REQ_FUA; but at the same time there doesn't seem to be a strict
rule. I wonder if we shouldn't move that check into the block layer, and
error out any attempts to issue such?
That definitely would be good for ATA, but potentially restrictive for
scsi ? Not sure about NVMe, I have not checked the specs. That said, the
only reasonable reason to do an FUA read that I can think of would be a
"scrub" like write-and-verify feature. And I do not think that any FS
implement their scrub process using FUA.
But that was the point.
_If_ the block layer never issues a REQ_READ|REQ_FUA we could make this
a restriction of the block layer, and would be perfectly fine to error
that out in the libata stack, too.
So we wouldn't need to worry on how to map that command, seeing that
we'll never get it.
Cheers,
Hannes
--
Dr. Hannes Reinecke Kernel Storage Architect
hare@xxxxxxx +49 911 74053 688
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), GF: Felix Imendörffer
