[PATCH v7 0/7] Improve libata support for FUA

[PATCH v7 0/7] Improve libata support for FUA

[Damien Le Moal]

These patches cleanup and improve libata support for ATA devices
supporting the FUA feature.

The first patch modifies the block layer to prevent the use of REQ_FUA
with read requests. This is necessary as the block layer code expect
REQ_FUA to be used with write requests (the flush machinery cannot
enforce access to the media for FUA read commands) and FUA is not
supported with ATA devices when NCQ is not enabled (device queue depth
set to 1).

Patch 2 and 3 are libata cleanup preparatory patches. Patch 4 cleans up
the detection for FUA support. Patch 5 fixes building a taskfile for FUA
write requests. Patch 6 prevents the use of FUA with known bad drives.

Finally, patch 7 enables FUA support by default in libata for devices
supporting this features.

Changes from v6:
 - Modified patch 1 to include checks for REQ_OP_ZONE_APPEND
 - Addressed comments from Niklas (patch 2 -> return false, patch 3 ->
   commit message typo, patch 7 -> more verbose commit message)

Changes from v5:
 - Removed WARN for FUA reads in patch 5.
 - Added reviewed-by tags.

Changes from v4:
 - Changed patch 1 to the one suggested by Christoph.
 - Added Hannes review tag.

Changes from v3:
 - Added patch 1 to prevent any block device user from issuing a
   REQ_FUA read.
 - Changed patch 5 to remove the check for REQ_FUA read and also remove 
   support for ATA_CMD_WRITE_MULTI_FUA_EXT as this command is obsolete
   in recent ACS specifications.

Changes from v2:
 - Added patch 1 and 2 as preparatory patches
 - Added patch 4 to fix FUA writes handling for the non-ncq case. Note
   that it is possible that the drives blacklisted in patch 5 are
   actually OK since the code back in 2012 had the issue with the wrong
   use of LBA 28 commands for FUA writes.

Changes from v1:
 - Removed Maciej's patch 2. Instead, blacklist drives which are known
   to have a buggy FUA support.

Christoph Hellwig (1):
  block: add a sanity check for non-write flush/fua bios

Damien Le Moal (6):
  ata: libata: Introduce ata_ncq_supported()
  ata: libata: Rename and cleanup ata_rwcmd_protocol()
  ata: libata: cleanup fua support detection
  ata: libata: Fix FUA handling in ata_build_rw_tf()
  ata: libata: blacklist FUA support for known buggy drives
  ata: libata: Enable fua support by default

 .../admin-guide/kernel-parameters.txt         |  3 +
 block/blk-core.c                              | 14 ++--
 drivers/ata/libata-core.c                     | 73 ++++++++++++++-----
 drivers/ata/libata-scsi.c                     | 30 +-------
 include/linux/libata.h                        | 36 ++++++---
 5 files changed, 94 insertions(+), 62 deletions(-)

-- 
2.39.0

[PATCH v7 1/7] block: add a sanity check for non-write flush/fua bios

[Damien Le Moal]

From: Christoph Hellwig <hch@infradead.org>

Check that the PREFUSH and FUA flags are only set on write bios,
given that the flush state machine expects that.

[Damien] The check is also extended to REQ_OP_ZONE_APPEND operations as
these are data write operations used by btrfs and zonefs and may also
have the REQ_FUA bit set.

Reported-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
Signed-off-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
---
 block/blk-core.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/block/blk-core.c b/block/blk-core.c
index 9321767470dc..c644aac498ef 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -744,12 +744,16 @@ void submit_bio_noacct(struct bio *bio)
 	 * Filter flush bio's early so that bio based drivers without flush
 	 * support don't have to worry about them.
 	 */
-	if (op_is_flush(bio->bi_opf) &&
-	    !test_bit(QUEUE_FLAG_WC, &q->queue_flags)) {
-		bio->bi_opf &= ~(REQ_PREFLUSH | REQ_FUA);
-		if (!bio_sectors(bio)) {
-			status = BLK_STS_OK;
+	if (op_is_flush(bio->bi_opf)) {
+		if (WARN_ON_ONCE(bio_op(bio) != REQ_OP_WRITE &&
+				 bio_op(bio) != REQ_OP_ZONE_APPEND))
 			goto end_io;
+		if (!test_bit(QUEUE_FLAG_WC, &q->queue_flags)) {
+			bio->bi_opf &= ~(REQ_PREFLUSH | REQ_FUA);
+			if (!bio_sectors(bio)) {
+				status = BLK_STS_OK;
+				goto end_io;
+			}
 		}
 	}
 
-- 
2.39.0

[PATCH v7 2/7] ata: libata: Introduce ata_ncq_supported()

[Damien Le Moal]

Introduce the inline helper function ata_ncq_supported() to test if a
device supports NCQ commands. The function ata_ncq_enabled() is also
rewritten using this new helper function.

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>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
Reviewed-by: Niklas Cassel <niklas.cassel@wdc.com>
---
 include/linux/libata.h | 28 +++++++++++++++++++++-------
 1 file changed, 21 insertions(+), 7 deletions(-)

diff --git a/include/linux/libata.h b/include/linux/libata.h
index c9149ebe7423..650543afad32 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -1690,21 +1690,35 @@ extern struct ata_device *ata_dev_next(struct ata_device *dev,
 	     (dev) = ata_dev_next((dev), (link), ATA_DITER_##mode))
 
 /**
- *	ata_ncq_enabled - Test whether NCQ is enabled
- *	@dev: ATA device to test for
+ *	ata_ncq_supported - Test whether NCQ is supported
+ *	@dev: ATA device to test
  *
  *	LOCKING:
  *	spin_lock_irqsave(host lock)
  *
  *	RETURNS:
- *	1 if NCQ is enabled for @dev, 0 otherwise.
+ *	true if @dev supports NCQ, false otherwise.
  */
-static inline int ata_ncq_enabled(struct ata_device *dev)
+static inline bool ata_ncq_supported(struct ata_device *dev)
 {
 	if (!IS_ENABLED(CONFIG_SATA_HOST))
-		return 0;
-	return (dev->flags & (ATA_DFLAG_PIO | ATA_DFLAG_NCQ_OFF |
-			      ATA_DFLAG_NCQ)) == ATA_DFLAG_NCQ;
+		return false;
+	return (dev->flags & (ATA_DFLAG_PIO | ATA_DFLAG_NCQ)) == ATA_DFLAG_NCQ;
+}
+
+/**
+ *	ata_ncq_enabled - Test whether NCQ is enabled
+ *	@dev: ATA device to test
+ *
+ *	LOCKING:
+ *	spin_lock_irqsave(host lock)
+ *
+ *	RETURNS:
+ *	true if NCQ is enabled for @dev, false otherwise.
+ */
+static inline bool ata_ncq_enabled(struct ata_device *dev)
+{
+	return ata_ncq_supported(dev) && !(dev->flags & ATA_DFLAG_NCQ_OFF);
 }
 
 static inline bool ata_fpdma_dsm_supported(struct ata_device *dev)
-- 
2.39.0

[PATCH v7 3/7] ata: libata: Rename and cleanup ata_rwcmd_protocol()

[Damien Le Moal]

Rename ata_rwcmd_protocol() to ata_set_rwcmd_protocol() to better
reflect the fact that this function sets a task file command and
protocol. The arguments order is also reversed and the function return
type changed to a bool to indicate if the command and protocol were set
correctly (instead of returning a completely arbitrary "-1" value.

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>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
Reviewed-by: Niklas Cassel <niklas.cassel@wdc.com>
---
 drivers/ata/libata-core.c | 28 +++++++++++++++-------------
 1 file changed, 15 insertions(+), 13 deletions(-)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 884ae73b11ea..6ee1cbac3ab0 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -574,17 +574,18 @@ static const u8 ata_rw_cmds[] = {
 };
 
 /**
- *	ata_rwcmd_protocol - set taskfile r/w commands and protocol
- *	@tf: command to examine and configure
- *	@dev: device tf belongs to
+ *	ata_set_rwcmd_protocol - set taskfile r/w command and protocol
+ *	@dev: target device for the taskfile
+ *	@tf: taskfile to examine and configure
  *
- *	Examine the device configuration and tf->flags to calculate
- *	the proper read/write commands and protocol to use.
+ *	Examine the device configuration and tf->flags to determine
+ *	the proper read/write command and protocol to use for @tf.
  *
  *	LOCKING:
  *	caller.
  */
-static int ata_rwcmd_protocol(struct ata_taskfile *tf, struct ata_device *dev)
+static bool ata_set_rwcmd_protocol(struct ata_device *dev,
+				   struct ata_taskfile *tf)
 {
 	u8 cmd;
 
@@ -607,11 +608,12 @@ static int ata_rwcmd_protocol(struct ata_taskfile *tf, struct ata_device *dev)
 	}
 
 	cmd = ata_rw_cmds[index + fua + lba48 + write];
-	if (cmd) {
-		tf->command = cmd;
-		return 0;
-	}
-	return -1;
+	if (!cmd)
+		return false;
+
+	tf->command = cmd;
+
+	return true;
 }
 
 /**
@@ -744,7 +746,7 @@ int ata_build_rw_tf(struct ata_queued_cmd *qc, u64 block, u32 n_block,
 			/* request too large even for LBA48 */
 			return -ERANGE;
 
-		if (unlikely(ata_rwcmd_protocol(tf, dev) < 0))
+		if (unlikely(!ata_set_rwcmd_protocol(dev, tf)))
 			return -EINVAL;
 
 		tf->nsect = n_block & 0xff;
@@ -762,7 +764,7 @@ int ata_build_rw_tf(struct ata_queued_cmd *qc, u64 block, u32 n_block,
 		if (!lba_28_ok(block, n_block))
 			return -ERANGE;
 
-		if (unlikely(ata_rwcmd_protocol(tf, dev) < 0))
+		if (unlikely(!ata_set_rwcmd_protocol(dev, tf)))
 			return -EINVAL;
 
 		/* Convert LBA to CHS */
-- 
2.39.0

[PATCH v7 4/7] ata: libata: cleanup fua support detection

[Damien Le Moal]

Move the detection of a device FUA support from
ata_scsiop_mode_sense()/ata_dev_supports_fua() to device scan time in
ata_dev_configure().

The function ata_dev_config_fua() is introduced to detect if a device
supports FUA and this support is indicated using the new device flag
ATA_DFLAG_FUA.

In order to blacklist known buggy devices, the horkage flag
ATA_HORKAGE_NO_FUA is introduced. Similarly to other horkage flags, the
libata.force= arguments "fua" and "nofua" are also introduced to allow
a user to control this horkage flag through the "force" libata
module parameter.

The ATA_DFLAG_FUA device flag is set only and only if all the following
conditions are met:
* libata.fua module parameter is set to 1
* The device supports the WRITE DMA FUA EXT command,
* The device is not marked with the ATA_HORKAGE_NO_FUA flag, either from
  the blacklist or set by the user with libata.force=nofua
* The device supports NCQ (while this is not mandated by the standards,
  this restriction is introduced to avoid problems with older non-NCQ
  devices).

Enabling or diabling libata FUA support for all devices can now also be
done using the "force=[no]fua" module parameter when libata.fua is set
to 1.

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>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
Reviewed-by: Niklas Cassel <niklas.cassel@wdc.com>
---
 .../admin-guide/kernel-parameters.txt         |  3 ++
 drivers/ata/libata-core.c                     | 30 ++++++++++++++++++-
 drivers/ata/libata-scsi.c                     | 30 ++-----------------
 include/linux/libata.h                        |  8 +++--
 4 files changed, 39 insertions(+), 32 deletions(-)

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 6cfa6e3996cf..3d0f2de7dc03 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -2816,6 +2816,9 @@
 			* [no]setxfer: Indicate if transfer speed mode setting
 			  should be skipped.
 
+			* [no]fua: Disable or enable FUA (Force Unit Access)
+			  support for devices supporting this feature.
+
 			* dump_id: Dump IDENTIFY data.
 
 			* disable: Disable this device.
diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 6ee1cbac3ab0..30adae16efff 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -2422,6 +2422,28 @@ static void ata_dev_config_chs(struct ata_device *dev)
 			     dev->heads, dev->sectors);
 }
 
+static void ata_dev_config_fua(struct ata_device *dev)
+{
+	/* Ignore FUA support if its use is disabled globally */
+	if (!libata_fua)
+		goto nofua;
+
+	/* Ignore devices without support for WRITE DMA FUA EXT */
+	if (!(dev->flags & ATA_DFLAG_LBA48) || !ata_id_has_fua(dev->id))
+		goto nofua;
+
+	/* Ignore known bad devices and devices that lack NCQ support */
+	if (!ata_ncq_supported(dev) || (dev->horkage & ATA_HORKAGE_NO_FUA))
+		goto nofua;
+
+	dev->flags |= ATA_DFLAG_FUA;
+
+	return;
+
+nofua:
+	dev->flags &= ~ATA_DFLAG_FUA;
+}
+
 static void ata_dev_config_devslp(struct ata_device *dev)
 {
 	u8 *sata_setting = dev->link->ap->sector_buf;
@@ -2510,7 +2532,8 @@ static void ata_dev_print_features(struct ata_device *dev)
 		return;
 
 	ata_dev_info(dev,
-		     "Features:%s%s%s%s%s%s\n",
+		     "Features:%s%s%s%s%s%s%s\n",
+		     dev->flags & ATA_DFLAG_FUA ? " FUA" : "",
 		     dev->flags & ATA_DFLAG_TRUSTED ? " Trust" : "",
 		     dev->flags & ATA_DFLAG_DA ? " Dev-Attention" : "",
 		     dev->flags & ATA_DFLAG_DEVSLP ? " Dev-Sleep" : "",
@@ -2671,6 +2694,7 @@ int ata_dev_configure(struct ata_device *dev)
 			ata_dev_config_chs(dev);
 		}
 
+		ata_dev_config_fua(dev);
 		ata_dev_config_devslp(dev);
 		ata_dev_config_sense_reporting(dev);
 		ata_dev_config_zac(dev);
@@ -4105,6 +4129,9 @@ static const struct ata_blacklist_entry ata_device_blacklist [] = {
 	 */
 	{ "SATADOM-ML 3ME",		NULL,	ATA_HORKAGE_NO_LOG_DIR },
 
+	/* Buggy FUA */
+	{ "Maxtor",		"BANC1G10",	ATA_HORKAGE_NO_FUA },
+
 	/* End Marker */
 	{ }
 };
@@ -6216,6 +6243,7 @@ static const struct ata_force_param force_tbl[] __initconst = {
 	force_horkage_onoff(lpm,	ATA_HORKAGE_NOLPM),
 	force_horkage_onoff(setxfer,	ATA_HORKAGE_NOSETXFER),
 	force_horkage_on(dump_id,	ATA_HORKAGE_DUMP_ID),
+	force_horkage_onoff(fua,	ATA_HORKAGE_NO_FUA),
 
 	force_horkage_on(disable,	ATA_HORKAGE_DISABLE),
 };
diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index cbb3a7a50816..f1622b8cdb55 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -2240,30 +2240,6 @@ static unsigned int ata_msense_rw_recovery(u8 *buf, bool changeable)
 	return sizeof(def_rw_recovery_mpage);
 }
 
-/*
- * We can turn this into a real blacklist if it's needed, for now just
- * blacklist any Maxtor BANC1G10 revision firmware
- */
-static int ata_dev_supports_fua(u16 *id)
-{
-	unsigned char model[ATA_ID_PROD_LEN + 1], fw[ATA_ID_FW_REV_LEN + 1];
-
-	if (!libata_fua)
-		return 0;
-	if (!ata_id_has_fua(id))
-		return 0;
-
-	ata_id_c_string(id, model, ATA_ID_PROD, sizeof(model));
-	ata_id_c_string(id, fw, ATA_ID_FW_REV, sizeof(fw));
-
-	if (strcmp(model, "Maxtor"))
-		return 1;
-	if (strcmp(fw, "BANC1G10"))
-		return 1;
-
-	return 0; /* blacklisted */
-}
-
 /**
  *	ata_scsiop_mode_sense - Simulate MODE SENSE 6, 10 commands
  *	@args: device IDENTIFY data / SCSI command of interest.
@@ -2287,7 +2263,7 @@ static unsigned int ata_scsiop_mode_sense(struct ata_scsi_args *args, u8 *rbuf)
 	};
 	u8 pg, spg;
 	unsigned int ebd, page_control, six_byte;
-	u8 dpofua, bp = 0xff;
+	u8 dpofua = 0, bp = 0xff;
 	u16 fp;
 
 	six_byte = (scsicmd[0] == MODE_SENSE);
@@ -2350,9 +2326,7 @@ static unsigned int ata_scsiop_mode_sense(struct ata_scsi_args *args, u8 *rbuf)
 		goto invalid_fld;
 	}
 
-	dpofua = 0;
-	if (ata_dev_supports_fua(args->id) && (dev->flags & ATA_DFLAG_LBA48) &&
-	    (!(dev->flags & ATA_DFLAG_PIO) || dev->multi_count))
+	if (dev->flags & ATA_DFLAG_FUA)
 		dpofua = 1 << 4;
 
 	if (six_byte) {
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 650543afad32..6352f7f25a6d 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -91,6 +91,7 @@ enum {
 	ATA_DFLAG_AN		= (1 << 7), /* AN configured */
 	ATA_DFLAG_TRUSTED	= (1 << 8), /* device supports trusted send/recv */
 	ATA_DFLAG_DMADIR	= (1 << 10), /* device requires DMADIR */
+	ATA_DFLAG_FUA		= (1 << 11), /* device supports FUA */
 	ATA_DFLAG_CFG_MASK	= (1 << 12) - 1,
 
 	ATA_DFLAG_PIO		= (1 << 12), /* device limited to PIO mode */
@@ -113,9 +114,9 @@ enum {
 	ATA_DFLAG_D_SENSE	= (1 << 29), /* Descriptor sense requested */
 	ATA_DFLAG_ZAC		= (1 << 30), /* ZAC device */
 
-	ATA_DFLAG_FEATURES_MASK	= ATA_DFLAG_TRUSTED | ATA_DFLAG_DA | \
-				  ATA_DFLAG_DEVSLP | ATA_DFLAG_NCQ_SEND_RECV | \
-				  ATA_DFLAG_NCQ_PRIO,
+	ATA_DFLAG_FEATURES_MASK	= (ATA_DFLAG_TRUSTED | ATA_DFLAG_DA |	\
+				   ATA_DFLAG_DEVSLP | ATA_DFLAG_NCQ_SEND_RECV | \
+				   ATA_DFLAG_NCQ_PRIO | ATA_DFLAG_FUA),
 
 	ATA_DEV_UNKNOWN		= 0,	/* unknown device */
 	ATA_DEV_ATA		= 1,	/* ATA device */
@@ -381,6 +382,7 @@ enum {
 	ATA_HORKAGE_NO_NCQ_ON_ATI = (1 << 27),	/* Disable NCQ on ATI chipset */
 	ATA_HORKAGE_NO_ID_DEV_LOG = (1 << 28),	/* Identify device log missing */
 	ATA_HORKAGE_NO_LOG_DIR	= (1 << 29),	/* Do not read log directory */
+	ATA_HORKAGE_NO_FUA	= (1 << 30),	/* Do not use FUA */
 
 	 /* DMA mask for user DMA control: User visible values; DO NOT
 	    renumber */
-- 
2.39.0

[PATCH v7 5/7] ata: libata: Fix FUA handling in ata_build_rw_tf()

[Damien Le Moal]

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 in the
ATA specification since ACS-3 (published in 2013), remove the
ATA_CMD_WRITE_MULTI_FUA_EXT command from the ata_rw_cmds array.

Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
Reviewed-by: Niklas Cassel <niklas.cassel@wdc.com>
---
 drivers/ata/libata-core.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 30adae16efff..2b66fe529d81 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,
@@ -727,7 +727,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 +743,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;
-- 
2.39.0

[PATCH v7 6/7] ata: libata: blacklist FUA support for known buggy drives

[Damien Le Moal]

Thread [1] reported back in 2012 problems with enabling FUA for 3
different drives. Add these drives to ata_device_blacklist[] to mark
them with the ATA_HORKAGE_NO_FUA flag. To be conservative and avoid
problems on old systems, the model number for the three new entries
are defined as to widely match all drives in the same product line.

[1]: https://lore.kernel.org/lkml/CA+6av4=uxu_q5U_46HtpUt=FSgbh3pZuAEY54J5_xK=MKWq-YQ@mail.gmail.com/

Suggested-by: Maciej S. Szmigiero <mail@maciej.szmigiero.name>
Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
Reviewed-by: Hannes Reinecke <hare@suse.de>
Reviewed-by: Maciej S. Szmigiero <maciej.szmigiero@oracle.com>
Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
Reviewed-by: Niklas Cassel <niklas.cassel@wdc.com>
---
 drivers/ata/libata-core.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 2b66fe529d81..97ade977b830 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -4133,6 +4133,9 @@ static const struct ata_blacklist_entry ata_device_blacklist [] = {
 
 	/* Buggy FUA */
 	{ "Maxtor",		"BANC1G10",	ATA_HORKAGE_NO_FUA },
+	{ "WDC*WD2500J*",	NULL,		ATA_HORKAGE_NO_FUA },
+	{ "OCZ-VERTEX*",	NULL,		ATA_HORKAGE_NO_FUA },
+	{ "INTEL*SSDSC2CT*",	NULL,		ATA_HORKAGE_NO_FUA },
 
 	/* End Marker */
 	{ }
-- 
2.39.0

[PATCH v7 7/7] ata: libata: Enable fua support by default

[Damien Le Moal]

Change the default value of the fua module parameter to 1 to enable fua
support by default for all devices supporting it.

With this change, ata_dev_config_fua() will now set the flag
ATA_DFLAG_FUA by default for devices that support FUA. This will cause
ata_scsiop_mode_sense() to set the DPOFUA bit in the DEVICE-SPECIFIC
PARAMETER field in the mode parameter header. The SCSI disk driver
performs a MODE SENSE and looks at the DPOFUA bit, it then calls
blk_queue_write_cache() with the value of the DPOFUA bit, to inform the
block layer if FUA is supported or not. The block layer will not issue
REQ_FUA requests if it has not been informed that the device actually
support FUA.

FUA support can be disabled for all drives or for individual drives
using the force=[ID]nofua libata module argument.

Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
Reviewed-by: Hannes Reinecke <hare@suse.de>
Reviewed-by: Maciej S. Szmigiero <maciej.szmigiero@oracle.com>
Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
Reviewed-by: Niklas Cassel <niklas.cassel@wdc.com>
---
 drivers/ata/libata-core.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 97ade977b830..2967671131d2 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -127,9 +127,9 @@ int atapi_passthru16 = 1;
 module_param(atapi_passthru16, int, 0444);
 MODULE_PARM_DESC(atapi_passthru16, "Enable ATA_16 passthru for ATAPI devices (0=off, 1=on [default])");
 
-int libata_fua = 0;
+int libata_fua = 1;
 module_param_named(fua, libata_fua, int, 0444);
-MODULE_PARM_DESC(fua, "FUA support (0=off [default], 1=on)");
+MODULE_PARM_DESC(fua, "FUA support (0=off, 1=on [default])");
 
 static int ata_ignore_hpa;
 module_param_named(ignore_hpa, ata_ignore_hpa, int, 0644);
-- 
2.39.0

Re: [PATCH v7 1/7] block: add a sanity check for non-write flush/fua bios

[Niklas Cassel]

On Tue, Jan 03, 2023 at 02:19:18PM +0900, Damien Le Moal wrote:
> From: Christoph Hellwig <hch@infradead.org>
>
> Check that the PREFUSH and FUA flags are only set on write bios,
> given that the flush state machine expects that.
>
> [Damien] The check is also extended to REQ_OP_ZONE_APPEND operations as
> these are data write operations used by btrfs and zonefs and may also
> have the REQ_FUA bit set.
>
> Reported-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
> ---
>  block/blk-core.c | 14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
>
> diff --git a/block/blk-core.c b/block/blk-core.c
> index 9321767470dc..c644aac498ef 100644
> --- a/block/blk-core.c
> +++ b/block/blk-core.c
> @@ -744,12 +744,16 @@ void submit_bio_noacct(struct bio *bio)
>	 * Filter flush bio's early so that bio based drivers without flush
>	 * support don't have to worry about them.
>	 */
> -	if (op_is_flush(bio->bi_opf) &&
> -	    !test_bit(QUEUE_FLAG_WC, &q->queue_flags)) {
> -		bio->bi_opf &= ~(REQ_PREFLUSH | REQ_FUA);
> -		if (!bio_sectors(bio)) {
> -			status = BLK_STS_OK;
> +	if (op_is_flush(bio->bi_opf)) {
> +		if (WARN_ON_ONCE(bio_op(bio) != REQ_OP_WRITE &&
> +				 bio_op(bio) != REQ_OP_ZONE_APPEND))
>			goto end_io;
> +		if (!test_bit(QUEUE_FLAG_WC, &q->queue_flags)) {
> +			bio->bi_opf &= ~(REQ_PREFLUSH | REQ_FUA);
> +			if (!bio_sectors(bio)) {
> +				status = BLK_STS_OK;
> +				goto end_io;
> +			}
>		}
>	}

Hello Damien,

In a previous email I wrote:

> It seems that you can have flag WC set, without having flag FUA set.
>
> So should perhaps the line:
>
>> +             if (!test_bit(QUEUE_FLAG_WC, &q->queue_flags)) {
>
> instead be:
>
> if (!test_bit(QUEUE_FLAG_FUA, &q->queue_flags)) {

You replied with:
"Need both. If there is no write cache or write cache is off, FUA is
implied and is useless."

Did you change your mind since then?


Kind regards,
Niklas

Re: [PATCH v7 1/7] block: add a sanity check for non-write flush/fua bios

[Damien Le Moal]

On 1/3/23 17:05, Niklas Cassel wrote:
> On Tue, Jan 03, 2023 at 02:19:18PM +0900, Damien Le Moal wrote:
>> From: Christoph Hellwig <hch@infradead.org>
>>
>> Check that the PREFUSH and FUA flags are only set on write bios,
>> given that the flush state machine expects that.
>>
>> [Damien] The check is also extended to REQ_OP_ZONE_APPEND operations as
>> these are data write operations used by btrfs and zonefs and may also
>> have the REQ_FUA bit set.
>>
>> Reported-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
>> Signed-off-by: Christoph Hellwig <hch@lst.de>
>> Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
>> ---
>>  block/blk-core.c | 14 +++++++++-----
>>  1 file changed, 9 insertions(+), 5 deletions(-)
>>
>> diff --git a/block/blk-core.c b/block/blk-core.c
>> index 9321767470dc..c644aac498ef 100644
>> --- a/block/blk-core.c
>> +++ b/block/blk-core.c
>> @@ -744,12 +744,16 @@ void submit_bio_noacct(struct bio *bio)
>> 	 * Filter flush bio's early so that bio based drivers without flush
>> 	 * support don't have to worry about them.
>> 	 */
>> -	if (op_is_flush(bio->bi_opf) &&
>> -	    !test_bit(QUEUE_FLAG_WC, &q->queue_flags)) {
>> -		bio->bi_opf &= ~(REQ_PREFLUSH | REQ_FUA);
>> -		if (!bio_sectors(bio)) {
>> -			status = BLK_STS_OK;
>> +	if (op_is_flush(bio->bi_opf)) {
>> +		if (WARN_ON_ONCE(bio_op(bio) != REQ_OP_WRITE &&
>> +				 bio_op(bio) != REQ_OP_ZONE_APPEND))
>> 			goto end_io;
>> +		if (!test_bit(QUEUE_FLAG_WC, &q->queue_flags)) {
>> +			bio->bi_opf &= ~(REQ_PREFLUSH | REQ_FUA);
>> +			if (!bio_sectors(bio)) {
>> +				status = BLK_STS_OK;
>> +				goto end_io;
>> +			}
>> 		}
>> 	}
> 
> Hello Damien,
> 
> In a previous email I wrote:
> 
>> It seems that you can have flag WC set, without having flag FUA set.
>>
>> So should perhaps the line:
>>
>>> +             if (!test_bit(QUEUE_FLAG_WC, &q->queue_flags)) {
>>
>> instead be:
>>
>> if (!test_bit(QUEUE_FLAG_FUA, &q->queue_flags)) {
> 
> You replied with:
> "Need both. If there is no write cache or write cache is off, FUA is
> implied and is useless.">
> Did you change your mind since then?

I checked the flush machinery code again to be sure and we do not need to
check "if (!test_bit(QUEUE_FLAG_FUA, &q->queue_flags)) {" because this is
exactly what blk-flush.c code will handle: if the device support FUA, the
write is sent as is and if it does not, then the flush machinery sent a
regular write followed by a cache flush command. See the chain:

submit_bio_noacct() -> submit_bio_noacct_nocheck() ->
__submit_bio_noacct[_mq]() -> __submit_bio() -> blk_mq_submit_bio() ->
blk_insert_flush().

Then see blk_insert_flush() handling of the various cases based off the
device features and request.

So that QUEUE_FLAG_FUA test here does not make any sense.

Checking for the no write cache case does make sense though, as in that
case, all writes are FUA. So clearing the FUA & PREFLUSH flags for devices
that do not have write caching is the right thing to do.

> 
> 
> Kind regards,
> Niklas

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v7 1/7] block: add a sanity check for non-write flush/fua bios

[Niklas Cassel]

On Tue, Jan 03, 2023 at 02:19:18PM +0900, Damien Le Moal wrote:
> From: Christoph Hellwig <hch@infradead.org>
> 
> Check that the PREFUSH and FUA flags are only set on write bios,
> given that the flush state machine expects that.
> 
> [Damien] The check is also extended to REQ_OP_ZONE_APPEND operations as
> these are data write operations used by btrfs and zonefs and may also
> have the REQ_FUA bit set.
> 
> Reported-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
> ---
>  block/blk-core.c | 14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/block/blk-core.c b/block/blk-core.c
> index 9321767470dc..c644aac498ef 100644
> --- a/block/blk-core.c
> +++ b/block/blk-core.c
> @@ -744,12 +744,16 @@ void submit_bio_noacct(struct bio *bio)
>  	 * Filter flush bio's early so that bio based drivers without flush
>  	 * support don't have to worry about them.
>  	 */
> -	if (op_is_flush(bio->bi_opf) &&
> -	    !test_bit(QUEUE_FLAG_WC, &q->queue_flags)) {
> -		bio->bi_opf &= ~(REQ_PREFLUSH | REQ_FUA);
> -		if (!bio_sectors(bio)) {
> -			status = BLK_STS_OK;
> +	if (op_is_flush(bio->bi_opf)) {
> +		if (WARN_ON_ONCE(bio_op(bio) != REQ_OP_WRITE &&
> +				 bio_op(bio) != REQ_OP_ZONE_APPEND))
>  			goto end_io;
> +		if (!test_bit(QUEUE_FLAG_WC, &q->queue_flags)) {
> +			bio->bi_opf &= ~(REQ_PREFLUSH | REQ_FUA);
> +			if (!bio_sectors(bio)) {
> +				status = BLK_STS_OK;
> +				goto end_io;
> +			}
>  		}
>  	}
>  
> -- 
> 2.39.0
> 

Reviewed-by: Niklas Cassel <niklas.cassel@wdc.com>

Re: [PATCH v7 1/7] block: add a sanity check for non-write flush/fua bios

[Ming Lei]

On Tue, Jan 03, 2023 at 02:19:18PM +0900, Damien Le Moal wrote:
> From: Christoph Hellwig <hch@infradead.org>
> 
> Check that the PREFUSH and FUA flags are only set on write bios,
> given that the flush state machine expects that.

flush state machine is only for request based driver, but FUA is
used by bio drivers(dm, md, ...) too, just wondering if bio drivers
are covered for this change? If yes, can you add words in the
commit log?


Thanks,
Ming

Re: [PATCH v7 0/7] Improve libata support for FUA

[Tejun Heo]

Hello,

On Tue, Jan 03, 2023 at 02:19:17PM +0900, Damien Le Moal wrote:
> Finally, patch 7 enables FUA support by default in libata for devices
> supporting this features.

These optional features tend to be broken in various and subtle ways,
especially the ones which don't show clear and notable advantages and thus
don't get used by everybody. I'm not necessarily against enabling it by
default but we should have better justifications as we might unnecessarily
cause a bunch of painful and subtle failures which can take a while to sort
out.

* Can the advantages of using FUA be demonstrated in a realistic way? IOW,
  are there workloads which clearly benefit from FUA? My memory is hazy but
  we only really use FUA from flush sequence to turn flush, write, flush
  sequence into flush, FUA-write. As all the heavy lifting is done in the
  first flush anyway, I couldn't find a case where that optimization made a
  meaningful difference but I didn't look very hard.

* Do we know how widely FUA is used now? IOW, is windows using FUA by
  default now? If so, do we know whether they have a blocklist?

Thanks.

-- 
tejun

Re: [PATCH v7 0/7] Improve libata support for FUA

[Damien Le Moal]

On 1/5/23 01:49, Tejun Heo wrote:
> Hello,
> 
> On Tue, Jan 03, 2023 at 02:19:17PM +0900, Damien Le Moal wrote:
>> Finally, patch 7 enables FUA support by default in libata for devices
>> supporting this features.
> 
> These optional features tend to be broken in various and subtle ways,

FUA is not optional for any drive that supports NCQ. The FUA bit is a
mandatory part of the FPDMA READ/WRITE commands. The optional part is
support for the non-ncq WRITE FUA EXT command.

> especially the ones which don't show clear and notable advantages and thus
> don't get used by everybody. I'm not necessarily against enabling it by
> default but we should have better justifications as we might unnecessarily
> cause a bunch of painful and subtle failures which can take a while to sort
> out.

Avoiding regressions is always my highest priority. I know that there
are a lot of cheap ATA devices out there that have questionable ACS spec
compliance.

> * Can the advantages of using FUA be demonstrated in a realistic way? IOW,
>   are there workloads which clearly benefit from FUA? My memory is hazy but
>   we only really use FUA from flush sequence to turn flush, write, flush
>   sequence into flush, FUA-write. As all the heavy lifting is done in the
>   first flush anyway, I couldn't find a case where that optimization made a
>   meaningful difference but I didn't look very hard.

The main users in kernel are file systems, when committing
transactions/metadata journaling. Given that this is generally not
generating a lot of traffic, I do not think we can measure any
difference for HDDs. The devices are too slow to start with, so saving
one command will not matter much, unless the application is fsync()
crazy (and even then, not sure we'll see any difference). Even for SATA
SSDs it likely will be hard to see a difference I think.

Then we have applications using the drive block device file directly.
For these, it is hard to tell how much it matters. Enabling it by
default with a drive correctly supporting it will very much likely not
hurt though.

Maciej,

May be you did some experiments before asking for enabling FUA by
default ? Any interesting performance data you can share ?

> * Do we know how widely FUA is used now? IOW, is windows using FUA by
>   default now? If so, do we know whether they have a blocklist?

You mean "blacklist" ? I do not have any information about Windows, but
I can try to find out, at least for my employer's devices. But that will
not be very useful as I know these drives behave correctly.

More than Windows or the kernel, I think that looking at SAS HBAs is
more important here. SATA HDDs are the most widely used type of devices
with these, by far. These may have a SAT translating FUA scsi writes to
FUA NCQ FPDMA writes, resulting in FUA being extensively used. Modulo a
blacklist that results in the same as the kernel with a
flush/write/flush sequence. Hard to know as HBA's FW are not open. A bus
analyzer could tell us that though, but again I can look at that only
with the drives I have, which I know are working well with FUA.

I am OK with attempting enabling FUA by default for the following reasons:
1) The vast majority of drives in libata blacklist (all features) are
old models that are not sold anymore.
2) We are restricting FUA support to drives that also support NCQ, that
is, modern-ish ones that are supposed to process the FUA NCQ read/write
commands correctly, per specs.
3) For HDDs, which is the vast majority of ATA devices out there these
days, all recent drives I have tested are OK. Even older ones with NCQ
support that I have access to are fine.
4) We are at rc2, which gives us time to revert patch 7 if we see too
many bug reports.

One thing we could add to the patch series is an additional restriction
to enabling FUA by default to drives that support a recent standard. Say
ACS-4 and above. That will restrict this to recent devices, thus
reducing the risk of hitting bad apples. Thoughts ?

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v7 1/7] block: add a sanity check for non-write flush/fua bios

[Damien Le Moal]

On 1/4/23 23:23, Ming Lei wrote:
> On Tue, Jan 03, 2023 at 02:19:18PM +0900, Damien Le Moal wrote:
>> From: Christoph Hellwig <hch@infradead.org>
>>
>> Check that the PREFUSH and FUA flags are only set on write bios,
>> given that the flush state machine expects that.
> 
> flush state machine is only for request based driver, but FUA is
> used by bio drivers(dm, md, ...) too, just wondering if bio drivers
> are covered for this change? If yes, can you add words in the
> commit log?

I think they are since it already was the responsibility of
dm_submit_bio() and md_submit_bio() to handle PREFLUSH and FUA.

> 
> 
> Thanks,
> Ming
> 

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v7 0/7] Improve libata support for FUA

[Tejun Heo]

Hello,

On Thu, Jan 05, 2023 at 12:43:06PM +0900, Damien Le Moal wrote:
> > These optional features tend to be broken in various and subtle ways,
> 
> FUA is not optional for any drive that supports NCQ. The FUA bit is a
> mandatory part of the FPDMA READ/WRITE commands. The optional part is
> support for the non-ncq WRITE FUA EXT command.

Optional in the sense that it isn't essential in achieving the main function
of the device, which means that most don't end up using it.

> > especially the ones which don't show clear and notable advantages and thus
> > don't get used by everybody. I'm not necessarily against enabling it by
> > default but we should have better justifications as we might unnecessarily
> > cause a bunch of painful and subtle failures which can take a while to sort
> > out.
> 
> Avoiding regressions is always my highest priority. I know that there
> are a lot of cheap ATA devices out there that have questionable ACS spec
> compliance.

A lot of historical devices too which don't get much scrutiny or testing but
can still cause significant griefs for the users.

> > * Can the advantages of using FUA be demonstrated in a realistic way? IOW,
> >   are there workloads which clearly benefit from FUA? My memory is hazy but
> >   we only really use FUA from flush sequence to turn flush, write, flush
> >   sequence into flush, FUA-write. As all the heavy lifting is done in the
> >   first flush anyway, I couldn't find a case where that optimization made a
> >   meaningful difference but I didn't look very hard.
> 
> The main users in kernel are file systems, when committing
> transactions/metadata journaling. Given that this is generally not
> generating a lot of traffic, I do not think we can measure any
> difference for HDDs. The devices are too slow to start with, so saving
> one command will not matter much, unless the application is fsync()
> crazy (and even then, not sure we'll see any difference). Even for SATA
> SSDs it likely will be hard to see a difference I think.

On a quick glance, there are some uses of REQ_FUA w/o REQ_PREFLUSH which
indicates that there can be actual gains to be had. However, ext4 AFAICS
always pairs PREFLUSH w/ FUA, so a lot of use cases won't see any gain while
taking on the possible risk of being exposed to FUA commands.

> Then we have applications using the drive block device file directly.
> For these, it is hard to tell how much it matters. Enabling it by
> default with a drive correctly supporting it will very much likely not
> hurt though.
> 
> Maciej,
> 
> May be you did some experiments before asking for enabling FUA by
> default ? Any interesting performance data you can share ?
> 
> > * Do we know how widely FUA is used now? IOW, is windows using FUA by
> >   default now? If so, do we know whether they have a blocklist?
> 
> You mean "blacklist" ? I do not have any information about Windows, but

The PC thing to say now seems to be allowlist / blocklist instead of
whiltelist / blacklist, not that I mind either way.

> I can try to find out, at least for my employer's devices. But that will
> not be very useful as I know these drives behave correctly.

So, AFAIK, windows doesn't issue FUA for SATA devices, only SAS, but I could
be wrong. It'd be really useful to find out.

> More than Windows or the kernel, I think that looking at SAS HBAs is
> more important here. SATA HDDs are the most widely used type of devices
> with these, by far. These may have a SAT translating FUA scsi writes to
> FUA NCQ FPDMA writes, resulting in FUA being extensively used. Modulo a
> blacklist that results in the same as the kernel with a
> flush/write/flush sequence. Hard to know as HBA's FW are not open. A bus
> analyzer could tell us that though, but again I can look at that only
> with the drives I have, which I know are working well with FUA.
> 
> I am OK with attempting enabling FUA by default for the following reasons:
> 1) The vast majority of drives in libata blacklist (all features) are
> old models that are not sold anymore.

The context here is that we promptly found all of these devices struggle
with FUA (like locking up and dropping off the bus) shortly after we enabled
FUA by default, so the list is by no means exhaustive and is more an
indication that there at least were a whole lot of devices which choke on
FUA. On top, devices not sold anymore are even harder to debug and pay
attention to while being able to cause a lot of pain to configurations which
have been stable and happy for a long time.

> 2) We are restricting FUA support to drives that also support NCQ, that
> is, modern-ish ones that are supposed to process the FUA NCQ read/write
> commands correctly, per specs.

NCQ is really old now and our previous attempt at FUA was after NCQ was
widely available, so I'm not sure this holds.

> 3) For HDDs, which is the vast majority of ATA devices out there these
> days, all recent drives I have tested are OK. Even older ones with NCQ
> support that I have access to are fine.
> 4) We are at rc2, which gives us time to revert patch 7 if we see too
> many bug reports.

This sort of problems especially if affecting mostly old devices can be very
difficult to suss out and will definitely take way longer than a single
release cycle.

> One thing we could add to the patch series is an additional restriction
> to enabling FUA by default to drives that support a recent standard. Say
> ACS-4 and above. That will restrict this to recent devices, thus
> reducing the risk of hitting bad apples. Thoughts ?

Yeah, that'd help and also if SAS HBA SAT's have been issuing FUA's which
would be a meaningful verification of the feature, at least for rotating
hard disks.

I feel rather uneasy about enabling FUA by default given history. We can
improve its chances by restricting it to newer devices and maybe even just
hard disks, but it kinda comes back to the root question of why. Why would
we want to do this? What are the benefits? Right now, there are a bunch of
really tricky cons and not whole lot on the pro column.

Thanks.

-- 
tejun

Re: [PATCH v7 0/7] Improve libata support for FUA

[Damien Le Moal]

On 1/6/23 03:15, Tejun Heo wrote:
> Hello,
> 
> On Thu, Jan 05, 2023 at 12:43:06PM +0900, Damien Le Moal wrote:
>>> These optional features tend to be broken in various and subtle ways,
>>
>> FUA is not optional for any drive that supports NCQ. The FUA bit is a
>> mandatory part of the FPDMA READ/WRITE commands. The optional part is
>> support for the non-ncq WRITE FUA EXT command.
> 
> Optional in the sense that it isn't essential in achieving the main function
> of the device, which means that most don't end up using it.

OK. Granted. But for this particular case, scsi & nvme subsystem do not
treat FUA as "optional". If it is supported, it will be used even though
you are correct that we can work without it. I do not think we should
treat ATA devices any differently.

>>> especially the ones which don't show clear and notable advantages and thus
>>> don't get used by everybody. I'm not necessarily against enabling it by
>>> default but we should have better justifications as we might unnecessarily
>>> cause a bunch of painful and subtle failures which can take a while to sort
>>> out.
>>
>> Avoiding regressions is always my highest priority. I know that there
>> are a lot of cheap ATA devices out there that have questionable ACS spec
>> compliance.
> 
> A lot of historical devices too which don't get much scrutiny or testing but
> can still cause significant griefs for the users.

Yes. There are a lot of s****y old devices that do not correctly handle
synchronize cache, and likely fua too. Hence my propsal to limit
enabling FUA support to newer devices based on the standards version
supported. Note that this patch set excludes all ide/pata devices. These
will still operate with fua off by default since they do not support NCQ.

>>> * Can the advantages of using FUA be demonstrated in a realistic way? IOW,
>>>   are there workloads which clearly benefit from FUA? My memory is hazy but
>>>   we only really use FUA from flush sequence to turn flush, write, flush
>>>   sequence into flush, FUA-write. As all the heavy lifting is done in the
>>>   first flush anyway, I couldn't find a case where that optimization made a
>>>   meaningful difference but I didn't look very hard.
>>
>> The main users in kernel are file systems, when committing
>> transactions/metadata journaling. Given that this is generally not
>> generating a lot of traffic, I do not think we can measure any
>> difference for HDDs. The devices are too slow to start with, so saving
>> one command will not matter much, unless the application is fsync()
>> crazy (and even then, not sure we'll see any difference). Even for SATA
>> SSDs it likely will be hard to see a difference I think.
> 
> On a quick glance, there are some uses of REQ_FUA w/o REQ_PREFLUSH which
> indicates that there can be actual gains to be had. However, ext4 AFAICS
> always pairs PREFLUSH w/ FUA, so a lot of use cases won't see any gain while
> taking on the possible risk of being exposed to FUA commands.

Yes. Most FSes will do PREFLUSH | FUA. For the risk, see above.

>> Then we have applications using the drive block device file directly.
>> For these, it is hard to tell how much it matters. Enabling it by
>> default with a drive correctly supporting it will very much likely not
>> hurt though.
>>
>> Maciej,
>>
>> May be you did some experiments before asking for enabling FUA by
>> default ? Any interesting performance data you can share ?
>>
>>> * Do we know how widely FUA is used now? IOW, is windows using FUA by
>>>   default now? If so, do we know whether they have a blocklist?
>>
>> You mean "blacklist" ? I do not have any information about Windows, but
> 
> The PC thing to say now seems to be allowlist / blocklist instead of
> whiltelist / blacklist, not that I mind either way.

I was thinking "block == sector" :) yes, could patch the code to rename
blacklist to something like badlist. I find "block" confusing here given
that we are talking about block devices :)

>> I can try to find out, at least for my employer's devices. But that will
>> not be very useful as I know these drives behave correctly.
> 
> So, AFAIK, windows doesn't issue FUA for SATA devices, only SAS, but I could
> be wrong. It'd be really useful to find out.

Need to ping some people to see if I can find out.

>> More than Windows or the kernel, I think that looking at SAS HBAs is
>> more important here. SATA HDDs are the most widely used type of devices
>> with these, by far. These may have a SAT translating FUA scsi writes to
>> FUA NCQ FPDMA writes, resulting in FUA being extensively used. Modulo a
>> blacklist that results in the same as the kernel with a
>> flush/write/flush sequence. Hard to know as HBA's FW are not open. A bus
>> analyzer could tell us that though, but again I can look at that only
>> with the drives I have, which I know are working well with FUA.
>>
>> I am OK with attempting enabling FUA by default for the following reasons:
>> 1) The vast majority of drives in libata blacklist (all features) are
>> old models that are not sold anymore.
> 
> The context here is that we promptly found all of these devices struggle
> with FUA (like locking up and dropping off the bus) shortly after we enabled
> FUA by default, so the list is by no means exhaustive and is more an
> indication that there at least were a whole lot of devices which choke on
> FUA. On top, devices not sold anymore are even harder to debug and pay
> attention to while being able to cause a lot of pain to configurations which
> have been stable and happy for a long time.

Yes. Hence, again, the idea to limit this to recent drives. E.g ACS-4
(or 5) and above.

>> 2) We are restricting FUA support to drives that also support NCQ, that
>> is, modern-ish ones that are supposed to process the FUA NCQ read/write
>> commands correctly, per specs.
> 
> NCQ is really old now and our previous attempt at FUA was after NCQ was
> widely available, so I'm not sure this holds.
> 
>> 3) For HDDs, which is the vast majority of ATA devices out there these
>> days, all recent drives I have tested are OK. Even older ones with NCQ
>> support that I have access to are fine.
>> 4) We are at rc2, which gives us time to revert patch 7 if we see too
>> many bug reports.
> 
> This sort of problems especially if affecting mostly old devices can be very
> difficult to suss out and will definitely take way longer than a single
> release cycle.
> 
>> One thing we could add to the patch series is an additional restriction
>> to enabling FUA by default to drives that support a recent standard. Say
>> ACS-4 and above. That will restrict this to recent devices, thus
>> reducing the risk of hitting bad apples. Thoughts ?
> 
> Yeah, that'd help and also if SAS HBA SAT's have been issuing FUA's which
> would be a meaningful verification of the feature, at least for rotating
> hard disks.
> 
> I feel rather uneasy about enabling FUA by default given history. We can
> improve its chances by restricting it to newer devices and maybe even just
> hard disks, but it kinda comes back to the root question of why. Why would
> we want to do this? What are the benefits? Right now, there are a bunch of
> really tricky cons and not whole lot on the pro column.

OK. But again, why treat ATA devices differently from scsi/nvme/ufs ?
These have FUA used by default if it is supported.

We can take a big hammer here and start with enabling only ACS-5 and
above for now. That will represent the set of devices that are in
development right now, and only a few already released (I have some in
my test boxes and they are not even a few months old...).

Or simply remove patch 7 and let user choose to enable FUA themselves if
they are confident their devices are OK. That is the safest, but I am
not keen on keeping ATA subsystem in the 20th century...

> 
> Thanks.
> 

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v7 0/7] Improve libata support for FUA

[Tejun Heo]

Hello,

On Fri, Jan 06, 2023 at 03:51:48PM +0900, Damien Le Moal wrote:
> OK. Granted. But for this particular case, scsi & nvme subsystem do not
> treat FUA as "optional". If it is supported, it will be used even though
> you are correct that we can work without it. I do not think we should
> treat ATA devices any differently.

What matters isn't that they have a featured with the same name but the
actual circumstances. e.g. for nvme, FUA has been there from the beginning
and we used it from the beginning so we know that they're safe. For ATA,
it's something added later on and we know that there are a bunch of devices
which choke on it and we don't know whether anyone else is using it at any
scale.

> > On a quick glance, there are some uses of REQ_FUA w/o REQ_PREFLUSH which
> > indicates that there can be actual gains to be had. However, ext4 AFAICS
> > always pairs PREFLUSH w/ FUA, so a lot of use cases won't see any gain while
> > taking on the possible risk of being exposed to FUA commands.
> 
> Yes. Most FSes will do PREFLUSH | FUA. For the risk, see above.

Someone should benchmark it but it's likelyt that PREFLUSH | FUA vs.
PREFLUSH | WRITE | POSTFLUSH isn't gonna show any meaningful difference in
any realistic scenario. The main gain of NCQ'd FUA is that we don't have to
drain the in-flight commands but PREFLUSH needs that anyway.

> > I feel rather uneasy about enabling FUA by default given history. We can
> > improve its chances by restricting it to newer devices and maybe even just
> > hard disks, but it kinda comes back to the root question of why. Why would
> > we want to do this? What are the benefits? Right now, there are a bunch of
> > really tricky cons and not whole lot on the pro column.
> 
> OK. But again, why treat ATA devices differently from scsi/nvme/ufs ?
> These have FUA used by default if it is supported.

This part hopefully is answered above.

> We can take a big hammer here and start with enabling only ACS-5 and
> above for now. That will represent the set of devices that are in
> development right now, and only a few already released (I have some in
> my test boxes and they are not even a few months old...).

All that said, yeah, if we restrict it to only the newest devices, they're
more likely to be well behaved and a lot more visible when they misbehave.
That sounds reasonable to me.

Thanks.

-- 
tejun

Re: [PATCH v7 0/7] Improve libata support for FUA

[Damien Le Moal]

On 1/7/23 03:03, Tejun Heo wrote:
>> We can take a big hammer here and start with enabling only ACS-5 and
>> above for now. That will represent the set of devices that are in
>> development right now, and only a few already released (I have some in
>> my test boxes and they are not even a few months old...).
> 
> All that said, yeah, if we restrict it to only the newest devices, they're
> more likely to be well behaved and a lot more visible when they misbehave.
> That sounds reasonable to me.

I re-posted the series without patch 7 enabling FUA by default. This
maintains the current state of libata while still cleaning up nicely all
the code around FUA.

I will send 1 or 2 patches later after thinking a little more about how to
safely enable FUA by default only for recent drives or drives of interest.
E.g. SMR drives as the lack of FUA support for them forces the use of the
block layer flush machinery, which itself causes write reordering... That
needs to be addressed too, and will look at that.

Thanks for the feedback.

-- 
Damien Le Moal
Western Digital Research