| Message ID | 20221107005021.1327692-6-damien.lemoal@opensource.wdc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Improve libata support for FUA | expand |
On 11/6/2022 4:50 PM, 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 tests if LBA 28 can be used > based on the write command sector and number of blocks. 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. > > Given that the WRITE MULTI FUA EXT command is marked as obsolete iin the > ATA specification since ACS-3 (published in 2013), remove the > ATA_CMD_WRITE_MULTI_FUA_EXT command from the ata_rw_cmds array. > > Finally, since the block layer should never issue a FUA read > request, warn in ata_build_rw_tf() if we see such request. > > Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com> > Reviewed-by: Hannes Reinecke <hare@suse.de> > --- Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com> -ck
On Mon, Nov 07, 2022 at 09:50:19AM +0900, Damien Le Moal wrote: > Finally, since the block layer should never issue a FUA read > request, warn in ata_build_rw_tf() if we see such request. Couldn't this be triggered using SG_IO passthrough with a SCSI WRITE* command that has the FUA bit set?
On 11/7/22 14:50, Christoph Hellwig wrote: > On Mon, Nov 07, 2022 at 09:50:19AM +0900, Damien Le Moal wrote: >> Finally, since the block layer should never issue a FUA read >> request, warn in ata_build_rw_tf() if we see such request. > > Couldn't this be triggered using SG_IO passthrough with a SCSI > WRITE* command that has the FUA bit set? Yes indeed. Should I drop the warn ?
On Mon, Nov 07, 2022 at 09:12:57PM +0900, Damien Le Moal wrote: > On 11/7/22 14:50, Christoph Hellwig wrote: > > On Mon, Nov 07, 2022 at 09:50:19AM +0900, Damien Le Moal wrote: > >> Finally, since the block layer should never issue a FUA read > >> request, warn in ata_build_rw_tf() if we see such request. > > > > Couldn't this be triggered using SG_IO passthrough with a SCSI > > WRITE* command that has the FUA bit set? > > Yes indeed. Should I drop the warn ? I think the warn needs to go. But don't we also need to handle the non-NCQ fua case if we don't want to break pure passthrough appliations? Or do we simply not care? In the latter case we'll at least need a comment documenting that tradeoff.
On 11/7/22 21:16, Christoph Hellwig wrote: > On Mon, Nov 07, 2022 at 09:12:57PM +0900, Damien Le Moal wrote: >> On 11/7/22 14:50, Christoph Hellwig wrote: >>> On Mon, Nov 07, 2022 at 09:50:19AM +0900, Damien Le Moal wrote: >>>> Finally, since the block layer should never issue a FUA read >>>> request, warn in ata_build_rw_tf() if we see such request. >>> >>> Couldn't this be triggered using SG_IO passthrough with a SCSI >>> WRITE* command that has the FUA bit set? >> >> Yes indeed. Should I drop the warn ? > > I think the warn needs to go. But don't we also need to handle the > non-NCQ fua case if we don't want to break pure passthrough appliations? > Or do we simply not care? In the latter case we'll at least need a > comment documenting that tradeoff. I am tempted to say "not care" since it has been like this since forever, silently letting FUA read through that do not do FUA at all... I am also tempted to say "let's add a check and fail those FUA reads that are not supported", that is, essentially stop hiding errors to the user. Bad passthrough applications that were working would stop working. Is that considered breaking the app if it was bad in the first place ?
On 11/7/22 21:16, Christoph Hellwig wrote: > On Mon, Nov 07, 2022 at 09:12:57PM +0900, Damien Le Moal wrote: >> On 11/7/22 14:50, Christoph Hellwig wrote: >>> On Mon, Nov 07, 2022 at 09:50:19AM +0900, Damien Le Moal wrote: >>>> Finally, since the block layer should never issue a FUA read >>>> request, warn in ata_build_rw_tf() if we see such request. >>> >>> Couldn't this be triggered using SG_IO passthrough with a SCSI >>> WRITE* command that has the FUA bit set? >> >> Yes indeed. Should I drop the warn ? > > I think the warn needs to go. But don't we also need to handle the > non-NCQ fua case if we don't want to break pure passthrough appliations? > Or do we simply not care? In the latter case we'll at least need a > comment documenting that tradeoff. Actually, this was already handled: for an FUA read without ncq enabed, ata_set_rwcmd_protocol() will return false, leading to ata_build_rw_tf() to return EINVAL, which is then translated to ILLEGAL REQUEST/INVALID FIELD IN CDB in libata-scsi. So passthrough applications attempting FUA reads without NCQ will see an error, as they already did before (modulo the bug with lba28, that is, the error was only for reads to higher LBAs or larger commands). So no additional checks needed I think.
diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c index 30adae16efff..83bea8591b08 100644 --- a/drivers/ata/libata-core.c +++ b/drivers/ata/libata-core.c @@ -552,7 +552,7 @@ static const u8 ata_rw_cmds[] = { 0, 0, 0, - ATA_CMD_WRITE_MULTI_FUA_EXT, + 0, /* pio */ ATA_CMD_PIO_READ, ATA_CMD_PIO_WRITE, @@ -693,6 +693,10 @@ int ata_build_rw_tf(struct ata_queued_cmd *qc, u64 block, u32 n_block, tf->flags |= ATA_TFLAG_ISADDR | ATA_TFLAG_DEVICE; tf->flags |= tf_flags; + /* We should never get a FUA read */ + WARN_ON_ONCE((tf->flags & ATA_TFLAG_FUA) && + !(tf->flags & ATA_TFLAG_WRITE)); + if (ata_ncq_enabled(dev)) { /* yay, NCQ */ if (!lba_48_ok(block, n_block)) @@ -727,7 +731,8 @@ int ata_build_rw_tf(struct ata_queued_cmd *qc, u64 block, u32 n_block, } else if (dev->flags & ATA_DFLAG_LBA) { tf->flags |= ATA_TFLAG_LBA; - if (lba_28_ok(block, n_block)) { + /* We need LBA48 for FUA writes */ + if (!(tf->flags & ATA_TFLAG_FUA) && lba_28_ok(block, n_block)) { /* use LBA28 */ tf->device |= (block >> 24) & 0xf; } else if (lba_48_ok(block, n_block)) { @@ -742,9 +747,10 @@ int ata_build_rw_tf(struct ata_queued_cmd *qc, u64 block, u32 n_block, tf->hob_lbah = (block >> 40) & 0xff; tf->hob_lbam = (block >> 32) & 0xff; tf->hob_lbal = (block >> 24) & 0xff; - } else + } else { /* request too large even for LBA48 */ return -ERANGE; + } if (unlikely(!ata_set_rwcmd_protocol(dev, tf))) return -EINVAL;