[PATCH v2 0/3] Improve libata support for FUA

[PATCH v2 0/3] Improve libata support for FUA

[Damien Le Moal]

These patches cleanup and improve libata support for the FUA device
feature. Patch 3 enables FUA support by default for any drive that
reports supporting the feature.

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

Damien Le Moal (3):
  ata: libata: cleanup fua handling
  ata: libata: blacklist FUA support for known buggy drives
  ata: libata: Enable fua support by default

 .../admin-guide/kernel-parameters.txt         |  3 ++
 drivers/ata/libata-core.c                     | 36 +++++++++++++++++--
 drivers/ata/libata-scsi.c                     | 30 ++--------------
 include/linux/libata.h                        |  8 +++--
 4 files changed, 43 insertions(+), 34 deletions(-)

-- 
2.37.3

[PATCH v2 1/3] ata: libata: cleanup fua handling

[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 a device FUA
support 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 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 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 advertizes support for the WRITE DMA FUA EXT command,
* The device supports LBA48 and is not restricted to single block PIO

Note: Enabling or diabling libata fua support for all devices by default
can now by done using either the "fua" module parameter or the
"force=[port[.device]][no]fua" module parameter when libata.fua==1.

Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
Reviewed-by: Hannes Reinecke <hare@suse.de>
---
 .../admin-guide/kernel-parameters.txt         |  3 ++
 drivers/ata/libata-core.c                     | 29 +++++++++++++++++-
 drivers/ata/libata-scsi.c                     | 30 ++-----------------
 include/linux/libata.h                        |  8 +++--
 4 files changed, 38 insertions(+), 32 deletions(-)

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index a465d5242774..f9724642c703 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -2786,6 +2786,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 884ae73b11ea..6008f7ed1c42 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -2420,6 +2420,27 @@ 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 globally disabled */
+	if (!libata_fua)
+		goto nofua;
+
+	/* Ignore devices without support and known bad devices */
+	if (!ata_id_has_fua(dev->id) || (dev->horkage & ATA_HORKAGE_NO_FUA))
+		goto nofua;
+
+	/* Limit FUA support to LBA48 without PIO restriction */
+	if ((dev->flags & ATA_DFLAG_LBA48) &&
+	    (!(dev->flags & ATA_DFLAG_PIO) || dev->multi_count)) {
+		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;
@@ -2508,7 +2529,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" : "",
@@ -2669,6 +2691,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);
@@ -4103,6 +4126,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 */
 	{ }
 };
@@ -6214,6 +6240,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 4cb914103382..69948e2a8f6d 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 af4953b95f76..81d863d751e1 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.37.3

[PATCH v2 2/3] 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
rpoblems 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>
---
 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 6008f7ed1c42..27aec8e63a8c 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -4128,6 +4128,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.37.3

Re: [PATCH v2 2/3] ata: libata: blacklist FUA support for known buggy drives

[Hannes Reinecke]

On 10/24/22 09:26, Damien Le Moal wrote:
> 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
> rpoblems 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>
> ---
>   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 6008f7ed1c42..27aec8e63a8c 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -4128,6 +4128,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 */
>   	{ }
Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		           Kernel Storage Architect
hare@suse.de			                  +49 911 74053 688
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), GF: Felix Imendörffer

[PATCH v2 3/3] 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. This parameter
description is also updated to indicate it is deprecated and that
libata.force=[no]fua should be used to control fua support.

Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
Reviewed-by: Hannes Reinecke <hare@suse.de>
---
 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 27aec8e63a8c..867e2c423627 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.37.3

Re: [PATCH v2 3/3] ata: libata: Enable fua support by default

[Sergey Shtylyov]

Hello!

On 10/24/22 10:26 AM, Damien Le Moal wrote:

> Change the default value of the fua module parameter to 1 to enable fua
> support by default for all devices supporting it. This parameter
> description is also updated to indicate it is deprecated and that
> libata.force=[no]fua should be used to control fua support.

   Mhm, where is that change? You only seem to change the default...

> 
> Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
> Reviewed-by: Hannes Reinecke <hare@suse.de>
> ---
>  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 27aec8e63a8c..867e2c423627 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);

MBR, Sergey

Re: [PATCH v2 3/3] ata: libata: Enable fua support by default

[Damien Le Moal]

On 10/24/22 19:16, Sergey Shtylyov wrote:
> Hello!
> 
> On 10/24/22 10:26 AM, Damien Le Moal wrote:
> 
>> Change the default value of the fua module parameter to 1 to enable fua
>> support by default for all devices supporting it. This parameter
>> description is also updated to indicate it is deprecated and that
>> libata.force=[no]fua should be used to control fua support.
> 
>    Mhm, where is that change? You only seem to change the default...

Oops. Forgot to update the commit message. I thought there was a way to
mark a module argument as deprecated, but there is not. Need to simplify
this commit message.

> 
>>
>> Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
>> Reviewed-by: Hannes Reinecke <hare@suse.de>
>> ---
>>  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 27aec8e63a8c..867e2c423627 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);
> 
> MBR, Sergey

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v2 0/3] Improve libata support for FUA

[Maciej S. Szmigiero]

On 24.10.2022 09:26, Damien Le Moal wrote:
> These patches cleanup and improve libata support for the FUA device
> feature. Patch 3 enables FUA support by default for any drive that
> reports supporting the feature.
> 
> Changes from v1:
>   - Removed Maciej's patch 2. Instead, blacklist drives which are known
>     to have a buggy FUA support.
> 
> Damien Le Moal (3):
>    ata: libata: cleanup fua handling
>    ata: libata: blacklist FUA support for known buggy drives
>    ata: libata: Enable fua support by default
> 

Thanks for the updated series.

In general (besides the small commit message thing that Sergey had
already mentioned) it looks good to me, so:
Reviewed-by: Maciej S. Szmigiero <maciej.szmigiero@oracle.com>

Thanks,
Maciej

Re: [PATCH v2 0/3] Improve libata support for FUA

[Damien Le Moal]

On 10/25/22 03:48, Maciej S. Szmigiero wrote:
> On 24.10.2022 09:26, Damien Le Moal wrote:
>> These patches cleanup and improve libata support for the FUA device
>> feature. Patch 3 enables FUA support by default for any drive that
>> reports supporting the feature.
>>
>> Changes from v1:
>>   - Removed Maciej's patch 2. Instead, blacklist drives which are known
>>     to have a buggy FUA support.
>>
>> Damien Le Moal (3):
>>    ata: libata: cleanup fua handling
>>    ata: libata: blacklist FUA support for known buggy drives
>>    ata: libata: Enable fua support by default
>>
> 
> Thanks for the updated series.
> 
> In general (besides the small commit message thing that Sergey had
> already mentioned) it looks good to me, so:
> Reviewed-by: Maciej S. Szmigiero <maciej.szmigiero@oracle.com>

Thanks. I need to do some more testing using some very old drives I found.
So far, no issues: detection works, some drives have FUA, other not. For
the ones that have FUA, I am running fstests (ext4 and xfs) to check for
weird behavior with REQ_FUA writes. Once I complete all tests I will queue
this.

> 
> Thanks,
> Maciej
> 

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v2 0/3] Improve libata support for FUA

[Damien Le Moal]

On 10/25/22 07:09, Damien Le Moal wrote:
> On 10/25/22 03:48, Maciej S. Szmigiero wrote:
>> On 24.10.2022 09:26, Damien Le Moal wrote:
>>> These patches cleanup and improve libata support for the FUA device
>>> feature. Patch 3 enables FUA support by default for any drive that
>>> reports supporting the feature.
>>>
>>> Changes from v1:
>>>   - Removed Maciej's patch 2. Instead, blacklist drives which are known
>>>     to have a buggy FUA support.
>>>
>>> Damien Le Moal (3):
>>>    ata: libata: cleanup fua handling
>>>    ata: libata: blacklist FUA support for known buggy drives
>>>    ata: libata: Enable fua support by default
>>>
>>
>> Thanks for the updated series.
>>
>> In general (besides the small commit message thing that Sergey had
>> already mentioned) it looks good to me, so:
>> Reviewed-by: Maciej S. Szmigiero <maciej.szmigiero@oracle.com>
> 
> Thanks. I need to do some more testing using some very old drives I found.
> So far, no issues: detection works, some drives have FUA, other not. For
> the ones that have FUA, I am running fstests (ext4 and xfs) to check for
> weird behavior with REQ_FUA writes. Once I complete all tests I will queue
> this.

Actually, I need to take this back. Checking again the code, I found an
issue with this entire FUA support: for a drive that does not support NCQ,
or one that has NCQ but has its queue depth set to one, then for a REQ_FUA
write request, ATA_CMD_WRITE_MULTI_FUA_EXT or ATA_CMD_WRITE_FUA_EXT will
be used. All good, BUT ! sd.c may also send read requests with the FUA bit
set if the read request has REQ_FUA set. For read commands, the regular,
non FUA commands ATA_CMD_READ_MULTI, ATA_CMD_READ_MULTI_EXT, ATA_CMD_READ
or ATA_CMD_READ_EXT will be used since ATA does not define a FUA version
of these. This means that the REQ_FUA flag will be ignored: this entire
code is broken as it is assuming that the read command processing on the
drive is consistent with executions of ATA_CMD_WRITE_MULTI_FUA_EXT or
ATA_CMD_WRITE_FUA_EXT. I do not want to bet on that, especially with old
drives.

I would be tempted to restrict FUA support to drives that support NCQ,
given that with NCQ, both READ FPDMA QUEUED and READ FPDMA WRITE have the
FUA bit. But then, the problem is that if the user changes the queue depth
of the drive to 1 through sysfs, ncq is turned off and we are back to
using the EXT read & write commands, that is, only write has FUA.

So if we want a solid ata FUA support, we would need to always use NCQ
regardless of the drive max queue depth setting...

Thoughts ?

> 
>>
>> Thanks,
>> Maciej
>>
> 

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v2 0/3] Improve libata support for FUA

[Damien Le Moal]

On 10/25/22 08:26, Damien Le Moal wrote:
> On 10/25/22 07:09, Damien Le Moal wrote:
>> On 10/25/22 03:48, Maciej S. Szmigiero wrote:
>>> On 24.10.2022 09:26, Damien Le Moal wrote:
>>>> These patches cleanup and improve libata support for the FUA device
>>>> feature. Patch 3 enables FUA support by default for any drive that
>>>> reports supporting the feature.
>>>>
>>>> Changes from v1:
>>>>   - Removed Maciej's patch 2. Instead, blacklist drives which are known
>>>>     to have a buggy FUA support.
>>>>
>>>> Damien Le Moal (3):
>>>>    ata: libata: cleanup fua handling
>>>>    ata: libata: blacklist FUA support for known buggy drives
>>>>    ata: libata: Enable fua support by default
>>>>
>>>
>>> Thanks for the updated series.
>>>
>>> In general (besides the small commit message thing that Sergey had
>>> already mentioned) it looks good to me, so:
>>> Reviewed-by: Maciej S. Szmigiero <maciej.szmigiero@oracle.com>
>>
>> Thanks. I need to do some more testing using some very old drives I found.
>> So far, no issues: detection works, some drives have FUA, other not. For
>> the ones that have FUA, I am running fstests (ext4 and xfs) to check for
>> weird behavior with REQ_FUA writes. Once I complete all tests I will queue
>> this.
> 
> Actually, I need to take this back. Checking again the code, I found an
> issue with this entire FUA support: for a drive that does not support NCQ,
> or one that has NCQ but has its queue depth set to one, then for a REQ_FUA
> write request, ATA_CMD_WRITE_MULTI_FUA_EXT or ATA_CMD_WRITE_FUA_EXT will
> be used. All good, BUT ! sd.c may also send read requests with the FUA bit
> set if the read request has REQ_FUA set. For read commands, the regular,
> non FUA commands ATA_CMD_READ_MULTI, ATA_CMD_READ_MULTI_EXT, ATA_CMD_READ
> or ATA_CMD_READ_EXT will be used since ATA does not define a FUA version
> of these. This means that the REQ_FUA flag will be ignored: this entire
> code is broken as it is assuming that the read command processing on the
> drive is consistent with executions of ATA_CMD_WRITE_MULTI_FUA_EXT or
> ATA_CMD_WRITE_FUA_EXT. I do not want to bet on that, especially with old
> drives.

Correction here: a REQ_FUA reads would end up being mapped to the "0"
command as returned by ata_rwcmd_protocol(), resulting in the user getting
back an EIO for any FUA read if libata FUA is enabled. With libata fua
disabled that was not happening. Digging further though, I do not see any
in-kernel code using REQ_FUA for reads. But it does not look like that is
forbidden either. Applications using SG_IO may use it anyway (e.g. to do
write verify type operations).

> 
> I would be tempted to restrict FUA support to drives that support NCQ,
> given that with NCQ, both READ FPDMA QUEUED and READ FPDMA WRITE have the
> FUA bit. But then, the problem is that if the user changes the queue depth
> of the drive to 1 through sysfs, ncq is turned off and we are back to
> using the EXT read & write commands, that is, only write has FUA.
> 
> So if we want a solid ata FUA support, we would need to always use NCQ
> regardless of the drive max queue depth setting...
> 
> Thoughts ?
> 
>>
>>>
>>> Thanks,
>>> Maciej
>>>
>>
> 

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v2 0/3] Improve libata support for FUA

[Hannes Reinecke]

On 10/25/22 01:26, Damien Le Moal wrote:
> On 10/25/22 07:09, Damien Le Moal wrote:
>> On 10/25/22 03:48, Maciej S. Szmigiero wrote:
>>> On 24.10.2022 09:26, Damien Le Moal wrote:
>>>> These patches cleanup and improve libata support for the FUA device
>>>> feature. Patch 3 enables FUA support by default for any drive that
>>>> reports supporting the feature.
>>>>
>>>> Changes from v1:
>>>>    - Removed Maciej's patch 2. Instead, blacklist drives which are known
>>>>      to have a buggy FUA support.
>>>>
>>>> Damien Le Moal (3):
>>>>     ata: libata: cleanup fua handling
>>>>     ata: libata: blacklist FUA support for known buggy drives
>>>>     ata: libata: Enable fua support by default
>>>>
>>>
>>> Thanks for the updated series.
>>>
>>> In general (besides the small commit message thing that Sergey had
>>> already mentioned) it looks good to me, so:
>>> Reviewed-by: Maciej S. Szmigiero <maciej.szmigiero@oracle.com>
>>
>> Thanks. I need to do some more testing using some very old drives I found.
>> So far, no issues: detection works, some drives have FUA, other not. For
>> the ones that have FUA, I am running fstests (ext4 and xfs) to check for
>> weird behavior with REQ_FUA writes. Once I complete all tests I will queue
>> this.
> 
> Actually, I need to take this back. Checking again the code, I found an
> issue with this entire FUA support: for a drive that does not support NCQ,
> or one that has NCQ but has its queue depth set to one, then for a REQ_FUA
> write request, ATA_CMD_WRITE_MULTI_FUA_EXT or ATA_CMD_WRITE_FUA_EXT will
> be used. All good, BUT ! sd.c may also send read requests with the FUA bit
> set if the read request has REQ_FUA set. For read commands, the regular,
> non FUA commands ATA_CMD_READ_MULTI, ATA_CMD_READ_MULTI_EXT, ATA_CMD_READ
> or ATA_CMD_READ_EXT will be used since ATA does not define a FUA version
> of these. This means that the REQ_FUA flag will be ignored: this entire
> code is broken as it is assuming that the read command processing on the
> drive is consistent with executions of ATA_CMD_WRITE_MULTI_FUA_EXT or
> ATA_CMD_WRITE_FUA_EXT. I do not want to bet on that, especially with old
> drives.
> 
Now you got me confused.
What exactly would be the semantics of a READ request with the FUA bit 
set? Ignore the cache and read from disk?
That would only make sense if the cache went out of sync with the drive, 
which really shouldn't happen, no?

> I would be tempted to restrict FUA support to drives that support NCQ,
> given that with NCQ, both READ FPDMA QUEUED and READ FPDMA WRITE have the
> FUA bit. But then, the problem is that if the user changes the queue depth
> of the drive to 1 through sysfs, ncq is turned off and we are back to
> using the EXT read & write commands, that is, only write has FUA.
> 
Hmm. Is this a requirement? We _could_ use the NCQ variants even with a 
queue depth of 1, no?

> So if we want a solid ata FUA support, we would need to always use NCQ
> regardless of the drive max queue depth setting...
> 
Sure, that would be the way I would be going.
If the drive supports NCQ we should always be using the FPDMA variants, 
irrespective of the queue depth.
Additionally we _might_ make FUA dependent on NCQ, and disallow FUA for 
non-NCQ drives.
(Where it's questionable anyway; if you only have a single command 
outstanding the pressure on any internal cache is far less as with NCQ.)

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Ivo Totev, Andrew
Myers, Andrew McDonald, Martje Boudien Moerman

Re: [PATCH v2 0/3] Improve libata support for FUA

[Damien Le Moal]

On 10/25/22 16:05, Hannes Reinecke wrote:
> On 10/25/22 01:26, Damien Le Moal wrote:
>> On 10/25/22 07:09, Damien Le Moal wrote:
>>> On 10/25/22 03:48, Maciej S. Szmigiero wrote:
>>>> On 24.10.2022 09:26, Damien Le Moal wrote:
>>>>> These patches cleanup and improve libata support for the FUA device
>>>>> feature. Patch 3 enables FUA support by default for any drive that
>>>>> reports supporting the feature.
>>>>>
>>>>> Changes from v1:
>>>>>    - Removed Maciej's patch 2. Instead, blacklist drives which are known
>>>>>      to have a buggy FUA support.
>>>>>
>>>>> Damien Le Moal (3):
>>>>>     ata: libata: cleanup fua handling
>>>>>     ata: libata: blacklist FUA support for known buggy drives
>>>>>     ata: libata: Enable fua support by default
>>>>>
>>>>
>>>> Thanks for the updated series.
>>>>
>>>> In general (besides the small commit message thing that Sergey had
>>>> already mentioned) it looks good to me, so:
>>>> Reviewed-by: Maciej S. Szmigiero <maciej.szmigiero@oracle.com>
>>>
>>> Thanks. I need to do some more testing using some very old drives I found.
>>> So far, no issues: detection works, some drives have FUA, other not. For
>>> the ones that have FUA, I am running fstests (ext4 and xfs) to check for
>>> weird behavior with REQ_FUA writes. Once I complete all tests I will queue
>>> this.
>>
>> Actually, I need to take this back. Checking again the code, I found an
>> issue with this entire FUA support: for a drive that does not support NCQ,
>> or one that has NCQ but has its queue depth set to one, then for a REQ_FUA
>> write request, ATA_CMD_WRITE_MULTI_FUA_EXT or ATA_CMD_WRITE_FUA_EXT will
>> be used. All good, BUT ! sd.c may also send read requests with the FUA bit
>> set if the read request has REQ_FUA set. For read commands, the regular,
>> non FUA commands ATA_CMD_READ_MULTI, ATA_CMD_READ_MULTI_EXT, ATA_CMD_READ
>> or ATA_CMD_READ_EXT will be used since ATA does not define a FUA version
>> of these. This means that the REQ_FUA flag will be ignored: this entire
>> code is broken as it is assuming that the read command processing on the
>> drive is consistent with executions of ATA_CMD_WRITE_MULTI_FUA_EXT or
>> ATA_CMD_WRITE_FUA_EXT. I do not want to bet on that, especially with old
>> drives.
>>
> Now you got me confused.
> What exactly would be the semantics of a READ request with the FUA bit 
> set? Ignore the cache and read from disk?
> That would only make sense if the cache went out of sync with the drive, 
> which really shouldn't happen, no?

For the NCQ read command, FUA text says:

If the Forced Unit Access (FUA) bit is set to one, the device shall
retrieve the data from the non-volatile media regardless of whether the
device holds the requested information in the volatile cache. If the
device holds a modified copy of the requested data as a result of having
volatile cached writes, the modified data shall be written to the
non-volatile media before being retrieved from the non-volatile media as
part of this operation. If the FUA bit is cleared to zero, the data shall
be retrieved either from the device's non-volatile media or cache.

So all well for NCQ, everything as expected, no surprises at all here.

But no equivalent behavior defined for non-NCQ read commands, and with
libata.fua enabled, ata_rwcmd_protocol() will cause any REQ_FUA read to
fail, which is the safe thing to do, but that is not what one gets when
libata.fua is disabled. So enabling libata fua by default could
potentially break some use cases out there of people using REQ_FUA reads
without realizing that it is doing nothing in non-NCQ case.

I have not checked the block layer though. Does the preflush/sync
machinery also triggers for reads ? I do not think so. I think REQ_FUA is
ignored for reads.

> 
>> I would be tempted to restrict FUA support to drives that support NCQ,
>> given that with NCQ, both READ FPDMA QUEUED and READ FPDMA WRITE have the
>> FUA bit. But then, the problem is that if the user changes the queue depth
>> of the drive to 1 through sysfs, ncq is turned off and we are back to
>> using the EXT read & write commands, that is, only write has FUA.
>>
> Hmm. Is this a requirement? We _could_ use the NCQ variants even with a 
> queue depth of 1, no?

Absolutely, yes. Running NCQ commands at QD = 1 is perfectly fine.
I think that this "do not use NCQ when QD == 1" is there mostly to deal
with drives that have a buggy NCQ implementation. Not sure. It has been
like this forever. Would need to do some git archeology about that one.

>> So if we want a solid ata FUA support, we would need to always use NCQ
>> regardless of the drive max queue depth setting...
>>
> Sure, that would be the way I would be going.
> If the drive supports NCQ we should always be using the FPDMA variants, 
> irrespective of the queue depth.
> Additionally we _might_ make FUA dependent on NCQ, and disallow FUA for 
> non-NCQ drives.
> (Where it's questionable anyway; if you only have a single command 
> outstanding the pressure on any internal cache is far less as with NCQ.)

Yes, that is my thinking too. Will try this and see how it looks.

> 
> Cheers,
> 
> Hannes

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v2 0/3] Improve libata support for FUA

[Niklas Cassel]

On Tue, Oct 25, 2022 at 05:59:07PM +0900, Damien Le Moal wrote:
> > Sure, that would be the way I would be going.
> > If the drive supports NCQ we should always be using the FPDMA variants, 
> > irrespective of the queue depth.
> > Additionally we _might_ make FUA dependent on NCQ, and disallow FUA for 
> > non-NCQ drives.
> > (Where it's questionable anyway; if you only have a single command 
> > outstanding the pressure on any internal cache is far less as with NCQ.)
> 
> Yes, that is my thinking too. Will try this and see how it looks.

When there are too many NCQ errors, flag ATA_DFLAG_NCQ_OFF will get set
(but queue depth will be left unchanged).

Currently, one way to re-enable NCQ is to change queue_depth to 1 in
sysfs, and then set it back to 32.

It would be nice if this method, or some other method to re-enable NCQ
after encountering too many NCQ errors, would still be available after
your suggested changes.



And if we change to always use NCQ commands regardless of queue depth
(for devices supporting it), because it provides us with additional
fields not available in the non-NCQ versions...
then I think that we should also consider always using WRITE DMA EXT
(on devices supporting 48-bit addresses), when NCQ is not supported
(or disabled because of too many errors), since it also has additional
fields not available in the regular WRITE DMA command (especially
considering that they should have the exact same performance).


Kind regards,
Niklas

Re: [PATCH v2 0/3] Improve libata support for FUA

[Maciej S. Szmigiero]

On 25.10.2022 10:59, Damien Le Moal wrote:
> On 10/25/22 16:05, Hannes Reinecke wrote:
>> On 10/25/22 01:26, Damien Le Moal wrote:
>>> On 10/25/22 07:09, Damien Le Moal wrote:
>>>> On 10/25/22 03:48, Maciej S. Szmigiero wrote:
>>>>> On 24.10.2022 09:26, Damien Le Moal wrote:
>>>>>> These patches cleanup and improve libata support for the FUA device
>>>>>> feature. Patch 3 enables FUA support by default for any drive that
>>>>>> reports supporting the feature.
>>>>>>
>>>>>> Changes from v1:
>>>>>>     - Removed Maciej's patch 2. Instead, blacklist drives which are known
>>>>>>       to have a buggy FUA support.
>>>>>>
>>>>>> Damien Le Moal (3):
>>>>>>      ata: libata: cleanup fua handling
>>>>>>      ata: libata: blacklist FUA support for known buggy drives
>>>>>>      ata: libata: Enable fua support by default
>>>>>>
>>>>>
>>>>> Thanks for the updated series.
>>>>>
>>>>> In general (besides the small commit message thing that Sergey had
>>>>> already mentioned) it looks good to me, so:
>>>>> Reviewed-by: Maciej S. Szmigiero <maciej.szmigiero@oracle.com>
>>>>
>>>> Thanks. I need to do some more testing using some very old drives I found.
>>>> So far, no issues: detection works, some drives have FUA, other not. For
>>>> the ones that have FUA, I am running fstests (ext4 and xfs) to check for
>>>> weird behavior with REQ_FUA writes. Once I complete all tests I will queue
>>>> this.
>>>
>>> Actually, I need to take this back. Checking again the code, I found an
>>> issue with this entire FUA support: for a drive that does not support NCQ,
>>> or one that has NCQ but has its queue depth set to one, then for a REQ_FUA
>>> write request, ATA_CMD_WRITE_MULTI_FUA_EXT or ATA_CMD_WRITE_FUA_EXT will
>>> be used. All good, BUT ! sd.c may also send read requests with the FUA bit
>>> set if the read request has REQ_FUA set. For read commands, the regular,
>>> non FUA commands ATA_CMD_READ_MULTI, ATA_CMD_READ_MULTI_EXT, ATA_CMD_READ
>>> or ATA_CMD_READ_EXT will be used since ATA does not define a FUA version
>>> of these. This means that the REQ_FUA flag will be ignored: this entire
>>> code is broken as it is assuming that the read command processing on the
>>> drive is consistent with executions of ATA_CMD_WRITE_MULTI_FUA_EXT or
>>> ATA_CMD_WRITE_FUA_EXT. I do not want to bet on that, especially with old
>>> drives.
>>>
>> Now you got me confused.
>> What exactly would be the semantics of a READ request with the FUA bit
>> set? Ignore the cache and read from disk?
>> That would only make sense if the cache went out of sync with the drive,
>> which really shouldn't happen, no?
> 
> For the NCQ read command, FUA text says:
> 
> If the Forced Unit Access (FUA) bit is set to one, the device shall
> retrieve the data from the non-volatile media regardless of whether the
> device holds the requested information in the volatile cache. If the
> device holds a modified copy of the requested data as a result of having
> volatile cached writes, the modified data shall be written to the
> non-volatile media before being retrieved from the non-volatile media as
> part of this operation. If the FUA bit is cleared to zero, the data shall
> be retrieved either from the device's non-volatile media or cache.
> 
> So all well for NCQ, everything as expected, no surprises at all here.
> 
> But no equivalent behavior defined for non-NCQ read commands, and with
> libata.fua enabled, ata_rwcmd_protocol() will cause any REQ_FUA read to
> fail, which is the safe thing to do, but that is not what one gets when
> libata.fua is disabled. So enabling libata fua by default could
> potentially break some use cases out there of people using REQ_FUA reads
> without realizing that it is doing nothing in non-NCQ case.
> 
> I have not checked the block layer though. Does the preflush/sync
> machinery also triggers for reads ? I do not think so. I think REQ_FUA is
> ignored for reads.

It seems like the block layer is indeed capable of doing a {pre,post}-flush
on reads - neither op_is_flush() nor blk_flush_policy() or
blk_insert_flush() have any checks for request direction.

But, as you mentioned, no higher level kernel code seems to actually issue
such FUA reads.

>>
>>> I would be tempted to restrict FUA support to drives that support NCQ,
>>> given that with NCQ, both READ FPDMA QUEUED and READ FPDMA WRITE have the
>>> FUA bit. But then, the problem is that if the user changes the queue depth
>>> of the drive to 1 through sysfs, ncq is turned off and we are back to
>>> using the EXT read & write commands, that is, only write has FUA.
>>>
>> Hmm. Is this a requirement? We _could_ use the NCQ variants even with a
>> queue depth of 1, no?
> 
> Absolutely, yes. Running NCQ commands at QD = 1 is perfectly fine.
> I think that this "do not use NCQ when QD == 1" is there mostly to deal
> with drives that have a buggy NCQ implementation. Not sure. It has been
> like this forever. Would need to do some git archeology about that one.

As Niklas has mentioned the NCQ will be disabled if there are too many
errors (for buggy implementations), so it should be safe to always make
use of it when it is supported by the drive.

>>> So if we want a solid ata FUA support, we would need to always use NCQ
>>> regardless of the drive max queue depth setting...
>>>
>> Sure, that would be the way I would be going.
>> If the drive supports NCQ we should always be using the FPDMA variants,
>> irrespective of the queue depth.
>> Additionally we _might_ make FUA dependent on NCQ, and disallow FUA for
>> non-NCQ drives.
>> (Where it's questionable anyway; if you only have a single command
>> outstanding the pressure on any internal cache is far less as with NCQ.)
> 
> Yes, that is my thinking too. Will try this and see how it looks.

In my opinion, having FUA dependent on NCQ totally makes sense
considering that there are no ATA_CMD_READ_FUA{,_EXT}.

In this case the NCQ disabling code in ata_eh_speed_down() will probably
need to clear the queue FUA flag too.

Thanks,
Maciej

Re: [PATCH v2 0/3] Improve libata support for FUA

[Damien Le Moal]

On 10/26/22 03:13, Maciej S. Szmigiero wrote:
> On 25.10.2022 10:59, Damien Le Moal wrote:
>> On 10/25/22 16:05, Hannes Reinecke wrote:
>>> On 10/25/22 01:26, Damien Le Moal wrote:
>>>> On 10/25/22 07:09, Damien Le Moal wrote:
>>>>> On 10/25/22 03:48, Maciej S. Szmigiero wrote:
>>>>>> On 24.10.2022 09:26, Damien Le Moal wrote:
>>>>>>> These patches cleanup and improve libata support for the FUA device
>>>>>>> feature. Patch 3 enables FUA support by default for any drive that
>>>>>>> reports supporting the feature.
>>>>>>>
>>>>>>> Changes from v1:
>>>>>>>     - Removed Maciej's patch 2. Instead, blacklist drives which are known
>>>>>>>       to have a buggy FUA support.
>>>>>>>
>>>>>>> Damien Le Moal (3):
>>>>>>>      ata: libata: cleanup fua handling
>>>>>>>      ata: libata: blacklist FUA support for known buggy drives
>>>>>>>      ata: libata: Enable fua support by default
>>>>>>>
>>>>>>
>>>>>> Thanks for the updated series.
>>>>>>
>>>>>> In general (besides the small commit message thing that Sergey had
>>>>>> already mentioned) it looks good to me, so:
>>>>>> Reviewed-by: Maciej S. Szmigiero <maciej.szmigiero@oracle.com>
>>>>>
>>>>> Thanks. I need to do some more testing using some very old drives I found.
>>>>> So far, no issues: detection works, some drives have FUA, other not. For
>>>>> the ones that have FUA, I am running fstests (ext4 and xfs) to check for
>>>>> weird behavior with REQ_FUA writes. Once I complete all tests I will queue
>>>>> this.
>>>>
>>>> Actually, I need to take this back. Checking again the code, I found an
>>>> issue with this entire FUA support: for a drive that does not support NCQ,
>>>> or one that has NCQ but has its queue depth set to one, then for a REQ_FUA
>>>> write request, ATA_CMD_WRITE_MULTI_FUA_EXT or ATA_CMD_WRITE_FUA_EXT will
>>>> be used. All good, BUT ! sd.c may also send read requests with the FUA bit
>>>> set if the read request has REQ_FUA set. For read commands, the regular,
>>>> non FUA commands ATA_CMD_READ_MULTI, ATA_CMD_READ_MULTI_EXT, ATA_CMD_READ
>>>> or ATA_CMD_READ_EXT will be used since ATA does not define a FUA version
>>>> of these. This means that the REQ_FUA flag will be ignored: this entire
>>>> code is broken as it is assuming that the read command processing on the
>>>> drive is consistent with executions of ATA_CMD_WRITE_MULTI_FUA_EXT or
>>>> ATA_CMD_WRITE_FUA_EXT. I do not want to bet on that, especially with old
>>>> drives.
>>>>
>>> Now you got me confused.
>>> What exactly would be the semantics of a READ request with the FUA bit
>>> set? Ignore the cache and read from disk?
>>> That would only make sense if the cache went out of sync with the drive,
>>> which really shouldn't happen, no?
>>
>> For the NCQ read command, FUA text says:
>>
>> If the Forced Unit Access (FUA) bit is set to one, the device shall
>> retrieve the data from the non-volatile media regardless of whether the
>> device holds the requested information in the volatile cache. If the
>> device holds a modified copy of the requested data as a result of having
>> volatile cached writes, the modified data shall be written to the
>> non-volatile media before being retrieved from the non-volatile media as
>> part of this operation. If the FUA bit is cleared to zero, the data shall
>> be retrieved either from the device's non-volatile media or cache.
>>
>> So all well for NCQ, everything as expected, no surprises at all here.
>>
>> But no equivalent behavior defined for non-NCQ read commands, and with
>> libata.fua enabled, ata_rwcmd_protocol() will cause any REQ_FUA read to
>> fail, which is the safe thing to do, but that is not what one gets when
>> libata.fua is disabled. So enabling libata fua by default could
>> potentially break some use cases out there of people using REQ_FUA reads
>> without realizing that it is doing nothing in non-NCQ case.
>>
>> I have not checked the block layer though. Does the preflush/sync
>> machinery also triggers for reads ? I do not think so. I think REQ_FUA is
>> ignored for reads.
> 
> It seems like the block layer is indeed capable of doing a {pre,post}-flush
> on reads - neither op_is_flush() nor blk_flush_policy() or
> blk_insert_flush() have any checks for request direction.

OK. But the problem remains that if fua is enabled, this machinery will
not trigger and we end up with a read error in ata.

> But, as you mentioned, no higher level kernel code seems to actually issue
> such FUA reads.

Which leaves SG_IO applications: translating a read with FUA bit set needs
to use NCQ read, always. But since that was not working before (the user
would get a read error), and nobody complained, I guess this is OK.

> 
>>>
>>>> I would be tempted to restrict FUA support to drives that support NCQ,
>>>> given that with NCQ, both READ FPDMA QUEUED and READ FPDMA WRITE have the
>>>> FUA bit. But then, the problem is that if the user changes the queue depth
>>>> of the drive to 1 through sysfs, ncq is turned off and we are back to
>>>> using the EXT read & write commands, that is, only write has FUA.
>>>>
>>> Hmm. Is this a requirement? We _could_ use the NCQ variants even with a
>>> queue depth of 1, no?
>>
>> Absolutely, yes. Running NCQ commands at QD = 1 is perfectly fine.
>> I think that this "do not use NCQ when QD == 1" is there mostly to deal
>> with drives that have a buggy NCQ implementation. Not sure. It has been
>> like this forever. Would need to do some git archeology about that one.
> 
> As Niklas has mentioned the NCQ will be disabled if there are too many
> errors (for buggy implementations), so it should be safe to always make
> use of it when it is supported by the drive.

I checked with the ATA specs gurus, and NCQ FUA is always supported (if
NCQ is supported), regardless of support for WRITE DMA FUA EXT. Problem
is, that switching off NCQ has to go hand in hand with switching off FUA.
See below.

> 
>>>> So if we want a solid ata FUA support, we would need to always use NCQ
>>>> regardless of the drive max queue depth setting...
>>>>
>>> Sure, that would be the way I would be going.
>>> If the drive supports NCQ we should always be using the FPDMA variants,
>>> irrespective of the queue depth.
>>> Additionally we _might_ make FUA dependent on NCQ, and disallow FUA for
>>> non-NCQ drives.
>>> (Where it's questionable anyway; if you only have a single command
>>> outstanding the pressure on any internal cache is far less as with NCQ.)
>>
>> Yes, that is my thinking too. Will try this and see how it looks.
> 
> In my opinion, having FUA dependent on NCQ totally makes sense
> considering that there are no ATA_CMD_READ_FUA{,_EXT}.
> 
> In this case the NCQ disabling code in ata_eh_speed_down() will probably
> need to clear the queue FUA flag too.

Yes, that is the difficulty because this needs also a revalidation at scsi
layer. The flag is set there, not in libata. And if there is a mounted
file system on the drive, switching randomly from "have fua" to "sorry, no
more fua" might cause problems... Not 100% sure though.

Given that FUA reads do not seem to used at all, if we restrict FUA
support to drives that support both NCQ *and* WRITE DMA FUA EXT, then we
should be OK.

Let me rewrite the first patch for that and see how it looks.

> 
> Thanks,
> Maciej
> 

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v2 0/3] Improve libata support for FUA

[Niklas Cassel]

On Tue, Oct 25, 2022 at 09:05:02AM +0200, Hannes Reinecke wrote:
> On 10/25/22 01:26, Damien Le Moal wrote:
> > On 10/25/22 07:09, Damien Le Moal wrote:
> > > On 10/25/22 03:48, Maciej S. Szmigiero wrote:
> > > > On 24.10.2022 09:26, Damien Le Moal wrote:
> > > > > These patches cleanup and improve libata support for the FUA device
> > > > > feature. Patch 3 enables FUA support by default for any drive that
> > > > > reports supporting the feature.
> > > > > 
> > > > > Changes from v1:
> > > > >    - Removed Maciej's patch 2. Instead, blacklist drives which are known
> > > > >      to have a buggy FUA support.
> > > > > 
> > > > > Damien Le Moal (3):
> > > > >     ata: libata: cleanup fua handling
> > > > >     ata: libata: blacklist FUA support for known buggy drives
> > > > >     ata: libata: Enable fua support by default
> > > > > 
> > > > 
> > > > Thanks for the updated series.
> > > > 
> > > > In general (besides the small commit message thing that Sergey had
> > > > already mentioned) it looks good to me, so:
> > > > Reviewed-by: Maciej S. Szmigiero <maciej.szmigiero@oracle.com>
> > > 
> > > Thanks. I need to do some more testing using some very old drives I found.
> > > So far, no issues: detection works, some drives have FUA, other not. For
> > > the ones that have FUA, I am running fstests (ext4 and xfs) to check for
> > > weird behavior with REQ_FUA writes. Once I complete all tests I will queue
> > > this.
> > 
> > Actually, I need to take this back. Checking again the code, I found an
> > issue with this entire FUA support: for a drive that does not support NCQ,
> > or one that has NCQ but has its queue depth set to one, then for a REQ_FUA
> > write request, ATA_CMD_WRITE_MULTI_FUA_EXT or ATA_CMD_WRITE_FUA_EXT will
> > be used. All good, BUT ! sd.c may also send read requests with the FUA bit
> > set if the read request has REQ_FUA set. For read commands, the regular,
> > non FUA commands ATA_CMD_READ_MULTI, ATA_CMD_READ_MULTI_EXT, ATA_CMD_READ
> > or ATA_CMD_READ_EXT will be used since ATA does not define a FUA version
> > of these. This means that the REQ_FUA flag will be ignored: this entire
> > code is broken as it is assuming that the read command processing on the
> > drive is consistent with executions of ATA_CMD_WRITE_MULTI_FUA_EXT or
> > ATA_CMD_WRITE_FUA_EXT. I do not want to bet on that, especially with old
> > drives.
> > 
> Now you got me confused.
> What exactly would be the semantics of a READ request with the FUA bit set?
> Ignore the cache and read from disk?

The opposite:
"If the device holds a modified copy of the requested data as a result of
having volatile cached writes, the modified data shall be written to the
non-volatile media before being retrieved from the non-volatile media as
part of this operation."


Kind regards,
Niklas