* [PATCH 0/3] Improve libata support for FUA
@ 2022-10-21 5:38 Damien Le Moal
2022-10-21 5:38 ` [PATCH 1/3] ata: libata: cleanup fua handling Damien Le Moal
` (3 more replies)
0 siblings, 4 replies; 15+ messages in thread
From: Damien Le Moal @ 2022-10-21 5:38 UTC (permalink / raw)
To: linux-ide, Maciej S . Szmigiero; +Cc: Hannes Reinecke
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.
Damien Le Moal (2):
ata: libata: cleanup fua handling
ata: libata: Enable fua support by default
Maciej S. Szmigiero (1):
ata: libata: allow toggling fua parameter at runtime
.../admin-guide/kernel-parameters.txt | 3 ++
drivers/ata/libata-core.c | 35 ++++++++++++++++---
drivers/ata/libata-scsi.c | 30 ++--------------
include/linux/libata.h | 8 +++--
4 files changed, 41 insertions(+), 35 deletions(-)
--
2.37.3
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 1/3] ata: libata: cleanup fua handling
2022-10-21 5:38 [PATCH 0/3] Improve libata support for FUA Damien Le Moal
@ 2022-10-21 5:38 ` Damien Le Moal
2022-10-21 6:20 ` Hannes Reinecke
2022-10-21 5:38 ` [PATCH 2/3] ata: libata: allow toggling fua parameter at runtime Damien Le Moal
` (2 subsequent siblings)
3 siblings, 1 reply; 15+ messages in thread
From: Damien Le Moal @ 2022-10-21 5:38 UTC (permalink / raw)
To: linux-ide, Maciej S . Szmigiero; +Cc: Hannes Reinecke
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>
---
.../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
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH 2/3] ata: libata: allow toggling fua parameter at runtime
2022-10-21 5:38 [PATCH 0/3] Improve libata support for FUA Damien Le Moal
2022-10-21 5:38 ` [PATCH 1/3] ata: libata: cleanup fua handling Damien Le Moal
@ 2022-10-21 5:38 ` Damien Le Moal
2022-10-21 6:21 ` Hannes Reinecke
2022-10-21 5:38 ` [PATCH 3/3] ata: libata: Enable fua support by default Damien Le Moal
2022-10-21 21:02 ` [PATCH 0/3] Improve libata support for FUA Maciej S. Szmigiero
3 siblings, 1 reply; 15+ messages in thread
From: Damien Le Moal @ 2022-10-21 5:38 UTC (permalink / raw)
To: linux-ide, Maciej S . Szmigiero; +Cc: Hannes Reinecke
From: "Maciej S. Szmigiero" <maciej.szmigiero@oracle.com>
Currently, the libata.fua parameter isn't runtime-writable, so a
system restart is required in order to toggle it.
This unnecessarily complicates testing how drives behave with FUA on and
off.
Let's make this parameter R/W instead, like many others in the kernel.
Example usage:
Disable the parameter:
echo 0 >/sys/module/libata/parameters/fua
Revalidate disk cache settings:
F=/sys/class/scsi_disk/0\:0\:0\:0/cache_type; echo `cat $F` >$F
[Damien]
Enabling fua support by setting libata.fua to 1 will have no effect if
the libata module is loaded with libata.force=[ID]nofua, which disables
fua support for the ata device(s) identified with ID or all ata devices
if no ID is specified.
Signed-off-by: Maciej S. Szmigiero <maciej.szmigiero@oracle.com>
Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
---
drivers/ata/libata-core.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 6008f7ed1c42..1bb9616b10d9 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -128,7 +128,7 @@ 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;
-module_param_named(fua, libata_fua, int, 0444);
+module_param_named(fua, libata_fua, int, 0644);
MODULE_PARM_DESC(fua, "FUA support (0=off [default], 1=on)");
static int ata_ignore_hpa;
--
2.37.3
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH 3/3] ata: libata: Enable fua support by default
2022-10-21 5:38 [PATCH 0/3] Improve libata support for FUA Damien Le Moal
2022-10-21 5:38 ` [PATCH 1/3] ata: libata: cleanup fua handling Damien Le Moal
2022-10-21 5:38 ` [PATCH 2/3] ata: libata: allow toggling fua parameter at runtime Damien Le Moal
@ 2022-10-21 5:38 ` Damien Le Moal
2022-10-21 6:22 ` Hannes Reinecke
2022-10-21 21:02 ` [PATCH 0/3] Improve libata support for FUA Maciej S. Szmigiero
3 siblings, 1 reply; 15+ messages in thread
From: Damien Le Moal @ 2022-10-21 5:38 UTC (permalink / raw)
To: linux-ide, Maciej S . Szmigiero; +Cc: Hannes Reinecke
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>
---
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 1bb9616b10d9..140e7e7d7833 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, 0644);
-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
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH 1/3] ata: libata: cleanup fua handling
2022-10-21 5:38 ` [PATCH 1/3] ata: libata: cleanup fua handling Damien Le Moal
@ 2022-10-21 6:20 ` Hannes Reinecke
0 siblings, 0 replies; 15+ messages in thread
From: Hannes Reinecke @ 2022-10-21 6:20 UTC (permalink / raw)
To: Damien Le Moal, linux-ide, Maciej S . Szmigiero
On 10/21/22 07:38, Damien Le Moal wrote:
> 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>
> ---
> .../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(-)
>
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 GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Ivo Totev, Andrew
Myers, Andrew McDonald, Martje Boudien Moerman
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/3] ata: libata: allow toggling fua parameter at runtime
2022-10-21 5:38 ` [PATCH 2/3] ata: libata: allow toggling fua parameter at runtime Damien Le Moal
@ 2022-10-21 6:21 ` Hannes Reinecke
2022-10-21 6:50 ` Damien Le Moal
0 siblings, 1 reply; 15+ messages in thread
From: Hannes Reinecke @ 2022-10-21 6:21 UTC (permalink / raw)
To: Damien Le Moal, linux-ide, Maciej S . Szmigiero
On 10/21/22 07:38, Damien Le Moal wrote:
> From: "Maciej S. Szmigiero" <maciej.szmigiero@oracle.com>
>
> Currently, the libata.fua parameter isn't runtime-writable, so a
> system restart is required in order to toggle it.
> This unnecessarily complicates testing how drives behave with FUA on and
> off.
>
> Let's make this parameter R/W instead, like many others in the kernel.
>
> Example usage:
> Disable the parameter:
> echo 0 >/sys/module/libata/parameters/fua
>
> Revalidate disk cache settings:
> F=/sys/class/scsi_disk/0\:0\:0\:0/cache_type; echo `cat $F` >$F
>
> [Damien]
> Enabling fua support by setting libata.fua to 1 will have no effect if
> the libata module is loaded with libata.force=[ID]nofua, which disables
> fua support for the ata device(s) identified with ID or all ata devices
> if no ID is specified.
>
> Signed-off-by: Maciej S. Szmigiero <maciej.szmigiero@oracle.com>
> Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
> ---
> drivers/ata/libata-core.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index 6008f7ed1c42..1bb9616b10d9 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -128,7 +128,7 @@ 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;
> -module_param_named(fua, libata_fua, int, 0444);
> +module_param_named(fua, libata_fua, int, 0644);
> MODULE_PARM_DESC(fua, "FUA support (0=off [default], 1=on)");
>
> static int ata_ignore_hpa;
Hmm. I guess you'll need to revalidate the drive when changing that; but
this can be done in a later patch.
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 GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Ivo Totev, Andrew
Myers, Andrew McDonald, Martje Boudien Moerman
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 3/3] ata: libata: Enable fua support by default
2022-10-21 5:38 ` [PATCH 3/3] ata: libata: Enable fua support by default Damien Le Moal
@ 2022-10-21 6:22 ` Hannes Reinecke
0 siblings, 0 replies; 15+ messages in thread
From: Hannes Reinecke @ 2022-10-21 6:22 UTC (permalink / raw)
To: Damien Le Moal, linux-ide, Maciej S . Szmigiero
On 10/21/22 07:38, 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.
>
> Signed-off-by: Damien Le Moal <damien.lemoal@opensource.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 1bb9616b10d9..140e7e7d7833 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, 0644);
> -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);
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 GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Ivo Totev, Andrew
Myers, Andrew McDonald, Martje Boudien Moerman
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/3] ata: libata: allow toggling fua parameter at runtime
2022-10-21 6:21 ` Hannes Reinecke
@ 2022-10-21 6:50 ` Damien Le Moal
2022-10-21 8:00 ` Damien Le Moal
0 siblings, 1 reply; 15+ messages in thread
From: Damien Le Moal @ 2022-10-21 6:50 UTC (permalink / raw)
To: Hannes Reinecke, linux-ide, Maciej S . Szmigiero
On 10/21/22 15:21, Hannes Reinecke wrote:
> On 10/21/22 07:38, Damien Le Moal wrote:
>> From: "Maciej S. Szmigiero" <maciej.szmigiero@oracle.com>
>>
>> Currently, the libata.fua parameter isn't runtime-writable, so a
>> system restart is required in order to toggle it.
>> This unnecessarily complicates testing how drives behave with FUA on and
>> off.
>>
>> Let's make this parameter R/W instead, like many others in the kernel.
>>
>> Example usage:
>> Disable the parameter:
>> echo 0 >/sys/module/libata/parameters/fua
>>
>> Revalidate disk cache settings:
>> F=/sys/class/scsi_disk/0\:0\:0\:0/cache_type; echo `cat $F` >$F
>>
>> [Damien]
>> Enabling fua support by setting libata.fua to 1 will have no effect if
>> the libata module is loaded with libata.force=[ID]nofua, which disables
>> fua support for the ata device(s) identified with ID or all ata devices
>> if no ID is specified.
>>
>> Signed-off-by: Maciej S. Szmigiero <maciej.szmigiero@oracle.com>
>> Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
>> ---
>> drivers/ata/libata-core.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
>> index 6008f7ed1c42..1bb9616b10d9 100644
>> --- a/drivers/ata/libata-core.c
>> +++ b/drivers/ata/libata-core.c
>> @@ -128,7 +128,7 @@ 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;
>> -module_param_named(fua, libata_fua, int, 0444);
>> +module_param_named(fua, libata_fua, int, 0644);
>> MODULE_PARM_DESC(fua, "FUA support (0=off [default], 1=on)");
>>
>> static int ata_ignore_hpa;
> Hmm. I guess you'll need to revalidate the drive when changing that; but
> this can be done in a later patch.
Well, this is not sysfs, we cannot do this automatically easily...
And thinking about it now that you mention it, going from fua=1 to fua=0
can actually cause problems. The reverse not, since scsi side would still
see fua=0 until revalidation.
So... Unless we find a way to link the param write to reavlidation, we
should actually not allow this.
Maciej ? Thoughts ?
>
> Reviewed-by: Hannes Reinecke <hare@suse.de>
>
> Cheers,
>
> Hannes
--
Damien Le Moal
Western Digital Research
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/3] ata: libata: allow toggling fua parameter at runtime
2022-10-21 6:50 ` Damien Le Moal
@ 2022-10-21 8:00 ` Damien Le Moal
2022-10-21 8:45 ` Hannes Reinecke
0 siblings, 1 reply; 15+ messages in thread
From: Damien Le Moal @ 2022-10-21 8:00 UTC (permalink / raw)
To: Hannes Reinecke, linux-ide, Maciej S . Szmigiero
On 10/21/22 15:50, Damien Le Moal wrote:
> On 10/21/22 15:21, Hannes Reinecke wrote:
>> On 10/21/22 07:38, Damien Le Moal wrote:
>>> From: "Maciej S. Szmigiero" <maciej.szmigiero@oracle.com>
>>>
>>> Currently, the libata.fua parameter isn't runtime-writable, so a
>>> system restart is required in order to toggle it.
>>> This unnecessarily complicates testing how drives behave with FUA on and
>>> off.
>>>
>>> Let's make this parameter R/W instead, like many others in the kernel.
>>>
>>> Example usage:
>>> Disable the parameter:
>>> echo 0 >/sys/module/libata/parameters/fua
>>>
>>> Revalidate disk cache settings:
>>> F=/sys/class/scsi_disk/0\:0\:0\:0/cache_type; echo `cat $F` >$F
>>>
>>> [Damien]
>>> Enabling fua support by setting libata.fua to 1 will have no effect if
>>> the libata module is loaded with libata.force=[ID]nofua, which disables
>>> fua support for the ata device(s) identified with ID or all ata devices
>>> if no ID is specified.
>>>
>>> Signed-off-by: Maciej S. Szmigiero <maciej.szmigiero@oracle.com>
>>> Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
>>> ---
>>> drivers/ata/libata-core.c | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
>>> index 6008f7ed1c42..1bb9616b10d9 100644
>>> --- a/drivers/ata/libata-core.c
>>> +++ b/drivers/ata/libata-core.c
>>> @@ -128,7 +128,7 @@ 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;
>>> -module_param_named(fua, libata_fua, int, 0444);
>>> +module_param_named(fua, libata_fua, int, 0644);
>>> MODULE_PARM_DESC(fua, "FUA support (0=off [default], 1=on)");
>>>
>>> static int ata_ignore_hpa;
>> Hmm. I guess you'll need to revalidate the drive when changing that; but
>> this can be done in a later patch.
>
> Well, this is not sysfs, we cannot do this automatically easily...
> And thinking about it now that you mention it, going from fua=1 to fua=0
> can actually cause problems. The reverse not, since scsi side would still
> see fua=0 until revalidation.
>
> So... Unless we find a way to link the param write to reavlidation, we
> should actually not allow this.
> Maciej ? Thoughts ?
I looked at this a little more. We could define the operations (struct
kernel_param_ops) manually together with the fua parameter declaration,
but that would be really ugly...
Given that we are switching to fua=1 by default, do you still need a
dynamic argument ? I am now thinking that this patch should be dropped.
>
>>
>> Reviewed-by: Hannes Reinecke <hare@suse.de>
>>
>> Cheers,
>>
>> Hannes
>
--
Damien Le Moal
Western Digital Research
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/3] ata: libata: allow toggling fua parameter at runtime
2022-10-21 8:00 ` Damien Le Moal
@ 2022-10-21 8:45 ` Hannes Reinecke
2022-10-21 8:48 ` Damien Le Moal
0 siblings, 1 reply; 15+ messages in thread
From: Hannes Reinecke @ 2022-10-21 8:45 UTC (permalink / raw)
To: Damien Le Moal, linux-ide, Maciej S . Szmigiero
On 10/21/22 10:00, Damien Le Moal wrote:
> On 10/21/22 15:50, Damien Le Moal wrote:
>> On 10/21/22 15:21, Hannes Reinecke wrote:
>>> On 10/21/22 07:38, Damien Le Moal wrote:
>>>> From: "Maciej S. Szmigiero" <maciej.szmigiero@oracle.com>
>>>>
>>>> Currently, the libata.fua parameter isn't runtime-writable, so a
>>>> system restart is required in order to toggle it.
>>>> This unnecessarily complicates testing how drives behave with FUA on and
>>>> off.
>>>>
>>>> Let's make this parameter R/W instead, like many others in the kernel.
>>>>
>>>> Example usage:
>>>> Disable the parameter:
>>>> echo 0 >/sys/module/libata/parameters/fua
>>>>
>>>> Revalidate disk cache settings:
>>>> F=/sys/class/scsi_disk/0\:0\:0\:0/cache_type; echo `cat $F` >$F
>>>>
>>>> [Damien]
>>>> Enabling fua support by setting libata.fua to 1 will have no effect if
>>>> the libata module is loaded with libata.force=[ID]nofua, which disables
>>>> fua support for the ata device(s) identified with ID or all ata devices
>>>> if no ID is specified.
>>>>
>>>> Signed-off-by: Maciej S. Szmigiero <maciej.szmigiero@oracle.com>
>>>> Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
>>>> ---
>>>> drivers/ata/libata-core.c | 2 +-
>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
>>>> index 6008f7ed1c42..1bb9616b10d9 100644
>>>> --- a/drivers/ata/libata-core.c
>>>> +++ b/drivers/ata/libata-core.c
>>>> @@ -128,7 +128,7 @@ 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;
>>>> -module_param_named(fua, libata_fua, int, 0444);
>>>> +module_param_named(fua, libata_fua, int, 0644);
>>>> MODULE_PARM_DESC(fua, "FUA support (0=off [default], 1=on)");
>>>>
>>>> static int ata_ignore_hpa;
>>> Hmm. I guess you'll need to revalidate the drive when changing that; but
>>> this can be done in a later patch.
>>
>> Well, this is not sysfs, we cannot do this automatically easily...
>> And thinking about it now that you mention it, going from fua=1 to fua=0
>> can actually cause problems. The reverse not, since scsi side would still
>> see fua=0 until revalidation.
>>
>> So... Unless we find a way to link the param write to reavlidation, we
>> should actually not allow this.
>> Maciej ? Thoughts ?
>
> I looked at this a little more. We could define the operations (struct
> kernel_param_ops) manually together with the fua parameter declaration,
> but that would be really ugly...
>
> Given that we are switching to fua=1 by default, do you still need a
> dynamic argument ? I am now thinking that this patch should be dropped.
>
I'd kill it, and let users it handle via blacklist flags only.
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/3] ata: libata: allow toggling fua parameter at runtime
2022-10-21 8:45 ` Hannes Reinecke
@ 2022-10-21 8:48 ` Damien Le Moal
0 siblings, 0 replies; 15+ messages in thread
From: Damien Le Moal @ 2022-10-21 8:48 UTC (permalink / raw)
To: Hannes Reinecke, linux-ide, Maciej S . Szmigiero
On 10/21/22 17:45, Hannes Reinecke wrote:
> On 10/21/22 10:00, Damien Le Moal wrote:
>> On 10/21/22 15:50, Damien Le Moal wrote:
>>> On 10/21/22 15:21, Hannes Reinecke wrote:
>>>> On 10/21/22 07:38, Damien Le Moal wrote:
>>>>> From: "Maciej S. Szmigiero" <maciej.szmigiero@oracle.com>
>>>>>
>>>>> Currently, the libata.fua parameter isn't runtime-writable, so a
>>>>> system restart is required in order to toggle it.
>>>>> This unnecessarily complicates testing how drives behave with FUA on and
>>>>> off.
>>>>>
>>>>> Let's make this parameter R/W instead, like many others in the kernel.
>>>>>
>>>>> Example usage:
>>>>> Disable the parameter:
>>>>> echo 0 >/sys/module/libata/parameters/fua
>>>>>
>>>>> Revalidate disk cache settings:
>>>>> F=/sys/class/scsi_disk/0\:0\:0\:0/cache_type; echo `cat $F` >$F
>>>>>
>>>>> [Damien]
>>>>> Enabling fua support by setting libata.fua to 1 will have no effect if
>>>>> the libata module is loaded with libata.force=[ID]nofua, which disables
>>>>> fua support for the ata device(s) identified with ID or all ata devices
>>>>> if no ID is specified.
>>>>>
>>>>> Signed-off-by: Maciej S. Szmigiero <maciej.szmigiero@oracle.com>
>>>>> Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
>>>>> ---
>>>>> drivers/ata/libata-core.c | 2 +-
>>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
>>>>> index 6008f7ed1c42..1bb9616b10d9 100644
>>>>> --- a/drivers/ata/libata-core.c
>>>>> +++ b/drivers/ata/libata-core.c
>>>>> @@ -128,7 +128,7 @@ 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;
>>>>> -module_param_named(fua, libata_fua, int, 0444);
>>>>> +module_param_named(fua, libata_fua, int, 0644);
>>>>> MODULE_PARM_DESC(fua, "FUA support (0=off [default], 1=on)");
>>>>>
>>>>> static int ata_ignore_hpa;
>>>> Hmm. I guess you'll need to revalidate the drive when changing that; but
>>>> this can be done in a later patch.
>>>
>>> Well, this is not sysfs, we cannot do this automatically easily...
>>> And thinking about it now that you mention it, going from fua=1 to fua=0
>>> can actually cause problems. The reverse not, since scsi side would still
>>> see fua=0 until revalidation.
>>>
>>> So... Unless we find a way to link the param write to reavlidation, we
>>> should actually not allow this.
>>> Maciej ? Thoughts ?
>>
>> I looked at this a little more. We could define the operations (struct
>> kernel_param_ops) manually together with the fua parameter declaration,
>> but that would be really ugly...
>>
>> Given that we are switching to fua=1 by default, do you still need a
>> dynamic argument ? I am now thinking that this patch should be dropped.
>>
> I'd kill it, and let users it handle via blacklist flags only.
Yep, with the default set to 1 that is the goal. I kept the fua module
parameter for backward compatibility, in case some setups out there use
it. But the force=[ID]nofua or force=[ID]fua module parameters should be
the preferred way to control this now.
>
> Cheers,
>
> Hannes
--
Damien Le Moal
Western Digital Research
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 0/3] Improve libata support for FUA
2022-10-21 5:38 [PATCH 0/3] Improve libata support for FUA Damien Le Moal
` (2 preceding siblings ...)
2022-10-21 5:38 ` [PATCH 3/3] ata: libata: Enable fua support by default Damien Le Moal
@ 2022-10-21 21:02 ` Maciej S. Szmigiero
2022-10-21 22:45 ` Damien Le Moal
3 siblings, 1 reply; 15+ messages in thread
From: Maciej S. Szmigiero @ 2022-10-21 21:02 UTC (permalink / raw)
To: Damien Le Moal; +Cc: Hannes Reinecke, linux-ide
On 21.10.2022 07:38, 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.
>
> Damien Le Moal (2):
> ata: libata: cleanup fua handling
> ata: libata: Enable fua support by default
>
> Maciej S. Szmigiero (1):
> ata: libata: allow toggling fua parameter at runtime
>
Thanks Damien for the series!
I've looked at the code changes and have basically two points:
1) There seems to be no way to revalidate the FUA setting for an existing
disk, since it is now only taken into account in ata_dev_config_fua().
As far as I can see, this function is only called on probe paths
(and during exception handling), so if the "libata.fua" parameter is
toggled the new setting would only affect newly (re-)attached disks.
Previously, this parameter was read directly in ata_scsiop_mode_sense()
(specifically in ata_dev_supports_fua() called from there), which could
be called to re-compute the FUA setting for an existing disk by
re-writing the "cache_type" sysfs attribute (as described in my commit
message).
If that's indeed the case this severely limits the usefulness of having
this parameter runtime-writable, and I agree with your discussion with
Hannes that it isn't probably needed now (after all, probably nobody
has an explicit "libata.fua=0" in their kernel command line, since this
was the default setting anyway).
2) It would be good to collect known-broken disks from the similar FUA
enabling attempt in 2012 [1] and add them to the blacklist upfront, so
these users won't have to report them again.
The problematic disks reported in that thread were:
> ata1: SATA link up 1.5 Gbps (SStatus 113 SControl 300)
> ata1.00: ATA-7: WDC WD2500JS-41MVB1, 10.02E01, max UDMA/133
> ata1.00: 488397168 sectors, multi 16: LBA48
> ata1.00: configured for UDMA/133
> scsi 0:0:0:0: Direct-Access ATA WDC WD2500JS-41M 10.0 PQ: 0 ANSI: 5
> [ 2.845750] ata1.00: ATA-9: OCZ-VERTEX3 MI, 2.06, max UDMA/133
> [ 2.845754] ata1.00: 234441648 sectors, multi 16: LBA48 NCQ (depth 31/32), AA
> [ 2.865726] ata1.00: configured for UDMA/133
> [ 2.865955] scsi 0:0:0:0: Direct-Access ATA OCZ-VERTEX3 MI 2.06 PQ: 0 ANSI: 5
> [ 2.866722] sd 0:0:0:0: [sda] 234441648 512-byte logical blocks: (120 GB/111 GiB)
> [ 3.934157] ata1.00: ATA-9: INTEL SSDSC2CT120A3, 300i, max UDMA/133
> [ 3.934266] ata1.00: 234441648 sectors, multi 16: LBA48 NCQ (depth 0/32)
> [ 3.954145] ata1.00: configured for UDMA/133
> [ 3.954441] scsi 0:0:0:0: Direct-Access ATA INTEL SSDSC2CT12
> 300i PQ: 0 ANSI: 5
> [ 3.955233] sd 0:0:0:0: [sda] 234441648 512-byte logical blocks: (120
> GB/111 GiB)
Thanks,
Maciej
[1]: https://lore.kernel.org/lkml/CA+6av4=uxu_q5U_46HtpUt=FSgbh3pZuAEY54J5_xK=MKWq-YQ@mail.gmail.com/
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 0/3] Improve libata support for FUA
2022-10-21 21:02 ` [PATCH 0/3] Improve libata support for FUA Maciej S. Szmigiero
@ 2022-10-21 22:45 ` Damien Le Moal
2022-10-22 13:50 ` Hannes Reinecke
0 siblings, 1 reply; 15+ messages in thread
From: Damien Le Moal @ 2022-10-21 22:45 UTC (permalink / raw)
To: Maciej S. Szmigiero; +Cc: Hannes Reinecke, linux-ide
On 10/22/22 06:02, Maciej S. Szmigiero wrote:
> On 21.10.2022 07:38, 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.
>>
>> Damien Le Moal (2):
>> ata: libata: cleanup fua handling
>> ata: libata: Enable fua support by default
>>
>> Maciej S. Szmigiero (1):
>> ata: libata: allow toggling fua parameter at runtime
>>
>
> Thanks Damien for the series!
>
> I've looked at the code changes and have basically two points:
> 1) There seems to be no way to revalidate the FUA setting for an existing
> disk, since it is now only taken into account in ata_dev_config_fua().
>
> As far as I can see, this function is only called on probe paths
> (and during exception handling), so if the "libata.fua" parameter is
> toggled the new setting would only affect newly (re-)attached disks.
Yes. Indeed. Forcing an ATA revalidation needs some more trickery as the
regular sd_revalidate() does not lead to ata_dev_configure() being called
again.
> Previously, this parameter was read directly in ata_scsiop_mode_sense()
> (specifically in ata_dev_supports_fua() called from there), which could
> be called to re-compute the FUA setting for an existing disk by
> re-writing the "cache_type" sysfs attribute (as described in my commit
> message).
>
> If that's indeed the case this severely limits the usefulness of having
> this parameter runtime-writable, and I agree with your discussion with
> Hannes that it isn't probably needed now (after all, probably nobody
> has an explicit "libata.fua=0" in their kernel command line, since this
> was the default setting anyway).
OK. Then I will drop your patch. Safer that way.
> 2) It would be good to collect known-broken disks from the similar FUA
> enabling attempt in 2012 [1] and add them to the blacklist upfront, so
> these users won't have to report them again.
The code only had one Maxtor drive blacklisted for FUA. Patch one adds it
to the horkage table.
>
> The problematic disks reported in that thread were:
>> ata1: SATA link up 1.5 Gbps (SStatus 113 SControl 300)
>> ata1.00: ATA-7: WDC WD2500JS-41MVB1, 10.02E01, max UDMA/133
>> ata1.00: 488397168 sectors, multi 16: LBA48
>> ata1.00: configured for UDMA/133
>> scsi 0:0:0:0: Direct-Access ATA WDC WD2500JS-41M 10.0 PQ: 0 ANSI: 5
>
>> [ 2.845750] ata1.00: ATA-9: OCZ-VERTEX3 MI, 2.06, max UDMA/133
>> [ 2.845754] ata1.00: 234441648 sectors, multi 16: LBA48 NCQ (depth 31/32), AA
>> [ 2.865726] ata1.00: configured for UDMA/133
>> [ 2.865955] scsi 0:0:0:0: Direct-Access ATA OCZ-VERTEX3 MI 2.06 PQ: 0 ANSI: 5
>> [ 2.866722] sd 0:0:0:0: [sda] 234441648 512-byte logical blocks: (120 GB/111 GiB)
>
>> [ 3.934157] ata1.00: ATA-9: INTEL SSDSC2CT120A3, 300i, max UDMA/133
>> [ 3.934266] ata1.00: 234441648 sectors, multi 16: LBA48 NCQ (depth 0/32)
>> [ 3.954145] ata1.00: configured for UDMA/133
>> [ 3.954441] scsi 0:0:0:0: Direct-Access ATA INTEL SSDSC2CT12
>> 300i PQ: 0 ANSI: 5
>> [ 3.955233] sd 0:0:0:0: [sda] 234441648 512-byte logical blocks: (120
>> GB/111 GiB)
OK. I will check that thread and add these drives to the horkage list.
Thanks !
>
> Thanks,
> Maciej
>
> [1]: https://lore.kernel.org/lkml/CA+6av4=uxu_q5U_46HtpUt=FSgbh3pZuAEY54J5_xK=MKWq-YQ@mail.gmail.com/
>
--
Damien Le Moal
Western Digital Research
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 0/3] Improve libata support for FUA
2022-10-21 22:45 ` Damien Le Moal
@ 2022-10-22 13:50 ` Hannes Reinecke
2022-10-23 0:27 ` Damien Le Moal
0 siblings, 1 reply; 15+ messages in thread
From: Hannes Reinecke @ 2022-10-22 13:50 UTC (permalink / raw)
To: Damien Le Moal, Maciej S. Szmigiero; +Cc: linux-ide
On 10/22/22 00:45, Damien Le Moal wrote:
> On 10/22/22 06:02, Maciej S. Szmigiero wrote:
>> On 21.10.2022 07:38, 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.
>>>
>>> Damien Le Moal (2):
>>> ata: libata: cleanup fua handling
>>> ata: libata: Enable fua support by default
>>>
>>> Maciej S. Szmigiero (1):
>>> ata: libata: allow toggling fua parameter at runtime
>>>
>>
>> Thanks Damien for the series!
>>
>> I've looked at the code changes and have basically two points:
>> 1) There seems to be no way to revalidate the FUA setting for an existing
>> disk, since it is now only taken into account in ata_dev_config_fua().
>>
>> As far as I can see, this function is only called on probe paths
>> (and during exception handling), so if the "libata.fua" parameter is
>> toggled the new setting would only affect newly (re-)attached disks.
>
> Yes. Indeed. Forcing an ATA revalidation needs some more trickery as the
> regular sd_revalidate() does not lead to ata_dev_configure() being called
> again.
>
But shouldn't we rather fix that?
After Johns series of pre-allocating the SCSI devices we should be able
to call ata_dev_configure() from sd_revalidate() ...
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 0/3] Improve libata support for FUA
2022-10-22 13:50 ` Hannes Reinecke
@ 2022-10-23 0:27 ` Damien Le Moal
0 siblings, 0 replies; 15+ messages in thread
From: Damien Le Moal @ 2022-10-23 0:27 UTC (permalink / raw)
To: Hannes Reinecke, Maciej S. Szmigiero; +Cc: linux-ide
On 10/22/22 22:50, Hannes Reinecke wrote:
> On 10/22/22 00:45, Damien Le Moal wrote:
>> On 10/22/22 06:02, Maciej S. Szmigiero wrote:
>>> On 21.10.2022 07:38, 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.
>>>>
>>>> Damien Le Moal (2):
>>>> ata: libata: cleanup fua handling
>>>> ata: libata: Enable fua support by default
>>>>
>>>> Maciej S. Szmigiero (1):
>>>> ata: libata: allow toggling fua parameter at runtime
>>>>
>>>
>>> Thanks Damien for the series!
>>>
>>> I've looked at the code changes and have basically two points:
>>> 1) There seems to be no way to revalidate the FUA setting for an existing
>>> disk, since it is now only taken into account in ata_dev_config_fua().
>>>
>>> As far as I can see, this function is only called on probe paths
>>> (and during exception handling), so if the "libata.fua" parameter is
>>> toggled the new setting would only affect newly (re-)attached disks.
>>
>> Yes. Indeed. Forcing an ATA revalidation needs some more trickery as the
>> regular sd_revalidate() does not lead to ata_dev_configure() being called
>> again.
>>
> But shouldn't we rather fix that?
> After Johns series of pre-allocating the SCSI devices we should be able
> to call ata_dev_configure() from sd_revalidate() ...
Yes, that should work. Though I am not sure if we really want to call
ata_dev_configure() every time sd_revalidate() is called, given the
performance impact of going to EH to revalidate an ATA drive. On an
average distro, there are quite a lot of revalidate going on...
For this particular case though, changing libata fua module parameter
value at libata run-time should trigger a revalidate of *all* ata drives,
which is different from the regular per-device revalidate driven by events
or the user changing a drive config through sysfs.
--
Damien Le Moal
Western Digital Research
^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2022-10-23 0:27 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-10-21 5:38 [PATCH 0/3] Improve libata support for FUA Damien Le Moal
2022-10-21 5:38 ` [PATCH 1/3] ata: libata: cleanup fua handling Damien Le Moal
2022-10-21 6:20 ` Hannes Reinecke
2022-10-21 5:38 ` [PATCH 2/3] ata: libata: allow toggling fua parameter at runtime Damien Le Moal
2022-10-21 6:21 ` Hannes Reinecke
2022-10-21 6:50 ` Damien Le Moal
2022-10-21 8:00 ` Damien Le Moal
2022-10-21 8:45 ` Hannes Reinecke
2022-10-21 8:48 ` Damien Le Moal
2022-10-21 5:38 ` [PATCH 3/3] ata: libata: Enable fua support by default Damien Le Moal
2022-10-21 6:22 ` Hannes Reinecke
2022-10-21 21:02 ` [PATCH 0/3] Improve libata support for FUA Maciej S. Szmigiero
2022-10-21 22:45 ` Damien Le Moal
2022-10-22 13:50 ` Hannes Reinecke
2022-10-23 0:27 ` Damien Le Moal
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.