[PATCH 1/2] ata: allow toggling fua parameter at runtime

[PATCH 1/2] ata: allow toggling fua parameter at runtime

[Maciej S. Szmigiero]

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

Signed-off-by: Maciej S. Szmigiero <maciej.szmigiero@oracle.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 75b86913db1ac..b322006c85806 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;

[PATCH 2/2] ata: allow enabling FUA support in Kconfig

[Maciej S. Szmigiero]

From: "Maciej S. Szmigiero" <maciej.szmigiero@oracle.com>

Currently, if one wants to make use of FUA support in libata it is
necessary to provide an explicit kernel command line parameter in order to
enable it (for drives that report such support).

In terms of Git archaeology: FUA support was enabled by default in early
libata versions but was disabled soon after.
Since then there were a few attempts to enable this support by default:
[1] (for NCQ drives only), [2] (for all drives).
However, the second change had to be reverted after a report came of
an incompatibility with the HDD in 2011 Mac Mini.

Enabling FUA avoids having to emulate it by issuing an extra drive cache
flush for every request that have this flag set.
Since FUA support is required by the ATA/ATAPI spec for any drive that
supports LBA48 and so these days should be pretty widespread let's provide
an ability to enable it by default in Kconfig.

[1]: https://lore.kernel.org/lkml/45CFFF82.4030301@shaw.ca/
[2]: https://lore.kernel.org/lkml/1336447443-4685-1-git-send-email-wenqing.lz@taobao.com/

Signed-off-by: Maciej S. Szmigiero <maciej.szmigiero@oracle.com>
---
 drivers/ata/Kconfig       | 15 +++++++++++++++
 drivers/ata/libata-core.c |  5 +++--
 2 files changed, 18 insertions(+), 2 deletions(-)

diff --git a/drivers/ata/Kconfig b/drivers/ata/Kconfig
index 36833a8629980..fd39bb22963a3 100644
--- a/drivers/ata/Kconfig
+++ b/drivers/ata/Kconfig
@@ -102,6 +102,21 @@ config SATA_PMP
 	  This option adds support for SATA Port Multipliers
 	  (the SATA version of an ethernet hub, or SAS expander).
 
+config ATA_FUA
+	bool "ATA FUA support"
+	help
+	  Enables Forced Unit Access (FUA) support by default for drives that
+	  have it.
+	  Otherwise the FUA flag has to be emulated by flushing the drive cache.
+
+	  Regardless of this option, you can enable or disable such support at
+	  kernel boot time by providing libata.fua=1 or libata.fua=0 kernel
+	  command line parameter.
+
+	  If building a kernel for yourself, say Y.
+	  If building a kernel for a distro that's supposed to run out of the
+	  box on old and broken hardware say N.
+
 if HAS_DMA
 
 comment "Controllers with non-SFF native interface"
diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index b322006c85806..941836c4b5eda 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -127,9 +127,10 @@ 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 = IS_ENABLED(CONFIG_ATA_FUA);
 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 " __stringify(IS_ENABLED(CONFIG_ATA_FUA)));
 
 static int ata_ignore_hpa;
 module_param_named(ignore_hpa, ata_ignore_hpa, int, 0644);

Re: [PATCH 2/2] ata: allow enabling FUA support in Kconfig

[Damien Le Moal]

On 9/27/22 04:51, Maciej S. Szmigiero wrote:
> From: "Maciej S. Szmigiero" <maciej.szmigiero@oracle.com>
> 
> Currently, if one wants to make use of FUA support in libata it is
> necessary to provide an explicit kernel command line parameter in order to
> enable it (for drives that report such support).
> 
> In terms of Git archaeology: FUA support was enabled by default in early
> libata versions but was disabled soon after.
> Since then there were a few attempts to enable this support by default:
> [1] (for NCQ drives only), [2] (for all drives).
> However, the second change had to be reverted after a report came of
> an incompatibility with the HDD in 2011 Mac Mini.
> 
> Enabling FUA avoids having to emulate it by issuing an extra drive cache
> flush for every request that have this flag set.
> Since FUA support is required by the ATA/ATAPI spec for any drive that
> supports LBA48 and so these days should be pretty widespread let's provide
> an ability to enable it by default in Kconfig.

This can be done by adding "libata.fua=1" to the CONFIG_CMDLINE option. So
I do not see the need to add yet another config option.

Patch 1 looks good. I will queue it up once rc1 is out.

> 
> [1]: https://lore.kernel.org/lkml/45CFFF82.4030301@shaw.ca/
> [2]: https://lore.kernel.org/lkml/1336447443-4685-1-git-send-email-wenqing.lz@taobao.com/
> 
> Signed-off-by: Maciej S. Szmigiero <maciej.szmigiero@oracle.com>
> ---
>  drivers/ata/Kconfig       | 15 +++++++++++++++
>  drivers/ata/libata-core.c |  5 +++--
>  2 files changed, 18 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/ata/Kconfig b/drivers/ata/Kconfig
> index 36833a8629980..fd39bb22963a3 100644
> --- a/drivers/ata/Kconfig
> +++ b/drivers/ata/Kconfig
> @@ -102,6 +102,21 @@ config SATA_PMP
>  	  This option adds support for SATA Port Multipliers
>  	  (the SATA version of an ethernet hub, or SAS expander).
>  
> +config ATA_FUA
> +	bool "ATA FUA support"
> +	help
> +	  Enables Forced Unit Access (FUA) support by default for drives that
> +	  have it.
> +	  Otherwise the FUA flag has to be emulated by flushing the drive cache.
> +
> +	  Regardless of this option, you can enable or disable such support at
> +	  kernel boot time by providing libata.fua=1 or libata.fua=0 kernel
> +	  command line parameter.
> +
> +	  If building a kernel for yourself, say Y.
> +	  If building a kernel for a distro that's supposed to run out of the
> +	  box on old and broken hardware say N.
> +
>  if HAS_DMA
>  
>  comment "Controllers with non-SFF native interface"
> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index b322006c85806..941836c4b5eda 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -127,9 +127,10 @@ 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 = IS_ENABLED(CONFIG_ATA_FUA);
>  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 " __stringify(IS_ENABLED(CONFIG_ATA_FUA)));
>  
>  static int ata_ignore_hpa;
>  module_param_named(ignore_hpa, ata_ignore_hpa, int, 0644);

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 2/2] ata: allow enabling FUA support in Kconfig

[Maciej S. Szmigiero]

On 6.10.2022 01:38, Damien Le Moal wrote:
> On 9/27/22 04:51, Maciej S. Szmigiero wrote:
>> From: "Maciej S. Szmigiero" <maciej.szmigiero@oracle.com>
>>
>> Currently, if one wants to make use of FUA support in libata it is
>> necessary to provide an explicit kernel command line parameter in order to
>> enable it (for drives that report such support).
>>
>> In terms of Git archaeology: FUA support was enabled by default in early
>> libata versions but was disabled soon after.
>> Since then there were a few attempts to enable this support by default:
>> [1] (for NCQ drives only), [2] (for all drives).
>> However, the second change had to be reverted after a report came of
>> an incompatibility with the HDD in 2011 Mac Mini.
>>
>> Enabling FUA avoids having to emulate it by issuing an extra drive cache
>> flush for every request that have this flag set.
>> Since FUA support is required by the ATA/ATAPI spec for any drive that
>> supports LBA48 and so these days should be pretty widespread let's provide
>> an ability to enable it by default in Kconfig.
> 
> This can be done by adding "libata.fua=1" to the CONFIG_CMDLINE option. So
> I do not see the need to add yet another config option.

A specific Kconfig option is more structured than a free-form
CONFIG_CMDLINE (which is also technically a per-arch option, but seems
to be widely supported across arches).

That's why there is a lot (100+) of similar Kconfig default-changing
options, a quick sample of these (in no particular order):
SOUND_OSS_CORE_PRECLAIM, SND_INTEL_BYT_PREFER_SOF, LSM,
SECURITY_SELINUX_CHECKREQPROT_VALUE, SECURITY_LOADPIN_ENFORCE,
SECURITY_APPARMOR_DEBUG_MESSAGES, IP_VS_TAB_BITS, IP_SET_MAX,
MAC80211_HAS_RC, SLUB_DEBUG_ON, KFENCE_SAMPLE_INTERVAL, PRINTK_TIME,
DEBUG_OBJECTS_ENABLE_DEFAULT, RCU_NOCB_CPU_DEFAULT_ALL, ...

libata currently has only one similar option: SATA_MOBILE_LPM_POLICY,
so it's not like a person performing kernel configuration is
overloaded with questions here.

But at the same time, I respect your decision as a maintainer of
this code.

> 
> Patch 1 looks good. I will queue it up once rc1 is out.

Thanks,
Maciej

Re: [PATCH 2/2] ata: allow enabling FUA support in Kconfig

[Damien Le Moal]

On 10/6/22 22:06, Maciej S. Szmigiero wrote:
> On 6.10.2022 01:38, Damien Le Moal wrote:
>> On 9/27/22 04:51, Maciej S. Szmigiero wrote:
>>> From: "Maciej S. Szmigiero" <maciej.szmigiero@oracle.com>
>>>
>>> Currently, if one wants to make use of FUA support in libata it is
>>> necessary to provide an explicit kernel command line parameter in order to
>>> enable it (for drives that report such support).
>>>
>>> In terms of Git archaeology: FUA support was enabled by default in early
>>> libata versions but was disabled soon after.
>>> Since then there were a few attempts to enable this support by default:
>>> [1] (for NCQ drives only), [2] (for all drives).
>>> However, the second change had to be reverted after a report came of
>>> an incompatibility with the HDD in 2011 Mac Mini.
>>>
>>> Enabling FUA avoids having to emulate it by issuing an extra drive cache
>>> flush for every request that have this flag set.
>>> Since FUA support is required by the ATA/ATAPI spec for any drive that
>>> supports LBA48 and so these days should be pretty widespread let's provide
>>> an ability to enable it by default in Kconfig.
>>
>> This can be done by adding "libata.fua=1" to the CONFIG_CMDLINE option. So
>> I do not see the need to add yet another config option.
> 
> A specific Kconfig option is more structured than a free-form
> CONFIG_CMDLINE (which is also technically a per-arch option, but seems
> to be widely supported across arches).
> 
> That's why there is a lot (100+) of similar Kconfig default-changing
> options, a quick sample of these (in no particular order):
> SOUND_OSS_CORE_PRECLAIM, SND_INTEL_BYT_PREFER_SOF, LSM,
> SECURITY_SELINUX_CHECKREQPROT_VALUE, SECURITY_LOADPIN_ENFORCE,
> SECURITY_APPARMOR_DEBUG_MESSAGES, IP_VS_TAB_BITS, IP_SET_MAX,
> MAC80211_HAS_RC, SLUB_DEBUG_ON, KFENCE_SAMPLE_INTERVAL, PRINTK_TIME,
> DEBUG_OBJECTS_ENABLE_DEFAULT, RCU_NOCB_CPU_DEFAULT_ALL, ...
> 
> libata currently has only one similar option: SATA_MOBILE_LPM_POLICY,
> so it's not like a person performing kernel configuration is
> overloaded with questions here.
> 
> But at the same time, I respect your decision as a maintainer of
> this code.

I am not dead set on pushing back on this, but as usual, whenever we can
avoid adding config options, we should. Given that libata has had fua
disabled forever, I am not convinced yet that there is a strong need for
that new option. But if distros prefer the config option approach, we can
make that happen.

If anything, I would be tempted to switch fua support to on by default
after some time if we do not get many reports of broken drives. You did
mention that old mac minis drives did not like it, but these are not even
blacklisted in libata-scsi. They should. Only one model of maxtor drives
is. We should add an ATA_HORKAGE_NO_FUA flag and start a proper blacklist
of drives not liking fua. Without that in place, switching to on by
default as your config option allows could break many (old) systems.


> 
>>
>> Patch 1 looks good. I will queue it up once rc1 is out.
> 
> Thanks,
> Maciej
> 
> 

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 2/2] ata: allow enabling FUA support in Kconfig

[Damien Le Moal]

On 10/7/22 07:20, Damien Le Moal wrote:
> On 10/6/22 22:06, Maciej S. Szmigiero wrote:
>> On 6.10.2022 01:38, Damien Le Moal wrote:
>>> On 9/27/22 04:51, Maciej S. Szmigiero wrote:
>>>> From: "Maciej S. Szmigiero" <maciej.szmigiero@oracle.com>
>>>>
>>>> Currently, if one wants to make use of FUA support in libata it is
>>>> necessary to provide an explicit kernel command line parameter in order to
>>>> enable it (for drives that report such support).
>>>>
>>>> In terms of Git archaeology: FUA support was enabled by default in early
>>>> libata versions but was disabled soon after.
>>>> Since then there were a few attempts to enable this support by default:
>>>> [1] (for NCQ drives only), [2] (for all drives).
>>>> However, the second change had to be reverted after a report came of
>>>> an incompatibility with the HDD in 2011 Mac Mini.
>>>>
>>>> Enabling FUA avoids having to emulate it by issuing an extra drive cache
>>>> flush for every request that have this flag set.
>>>> Since FUA support is required by the ATA/ATAPI spec for any drive that
>>>> supports LBA48 and so these days should be pretty widespread let's provide
>>>> an ability to enable it by default in Kconfig.
>>>
>>> This can be done by adding "libata.fua=1" to the CONFIG_CMDLINE option. So
>>> I do not see the need to add yet another config option.
>>
>> A specific Kconfig option is more structured than a free-form
>> CONFIG_CMDLINE (which is also technically a per-arch option, but seems
>> to be widely supported across arches).
>>
>> That's why there is a lot (100+) of similar Kconfig default-changing
>> options, a quick sample of these (in no particular order):
>> SOUND_OSS_CORE_PRECLAIM, SND_INTEL_BYT_PREFER_SOF, LSM,
>> SECURITY_SELINUX_CHECKREQPROT_VALUE, SECURITY_LOADPIN_ENFORCE,
>> SECURITY_APPARMOR_DEBUG_MESSAGES, IP_VS_TAB_BITS, IP_SET_MAX,
>> MAC80211_HAS_RC, SLUB_DEBUG_ON, KFENCE_SAMPLE_INTERVAL, PRINTK_TIME,
>> DEBUG_OBJECTS_ENABLE_DEFAULT, RCU_NOCB_CPU_DEFAULT_ALL, ...
>>
>> libata currently has only one similar option: SATA_MOBILE_LPM_POLICY,
>> so it's not like a person performing kernel configuration is
>> overloaded with questions here.
>>
>> But at the same time, I respect your decision as a maintainer of
>> this code.
> 
> I am not dead set on pushing back on this, but as usual, whenever we can
> avoid adding config options, we should. Given that libata has had fua
> disabled forever, I am not convinced yet that there is a strong need for
> that new option. But if distros prefer the config option approach, we can
> make that happen.
> 
> If anything, I would be tempted to switch fua support to on by default
> after some time if we do not get many reports of broken drives. You did
> mention that old mac minis drives did not like it, but these are not even
> blacklisted in libata-scsi. They should. Only one model of maxtor drives
> is. We should add an ATA_HORKAGE_NO_FUA flag and start a proper blacklist
> of drives not liking fua. Without that in place, switching to on by
> default as your config option allows could break many (old) systems.

To be extra clear, I think that this fua module parameter is silly. If a
drive says it supports fua, we should use it and not have a global
parameter to disable it for all drives. So no config option needed for it.

That is also why I am not keen on taking that config option. It is not
really improving anything at all and would prefer nuking the fua module
argument and have a proper blacklisting of buggy drives.

But such a change is painful as we'll be able to update the blacklist with
users getting corrupted FSes on buggy drives. The time may have come to do
this change though as the number of buggy drives out there is hopefully
small enough now.

> 
> 
>>
>>>
>>> Patch 1 looks good. I will queue it up once rc1 is out.
>>
>> Thanks,
>> Maciej
>>
>>
> 

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 2/2] ata: allow enabling FUA support in Kconfig

[Maciej S. Szmigiero]

On 7.10.2022 00:53, Damien Le Moal wrote:
> On 10/7/22 07:20, Damien Le Moal wrote:
>> On 10/6/22 22:06, Maciej S. Szmigiero wrote:
>>> On 6.10.2022 01:38, Damien Le Moal wrote:
>>>> On 9/27/22 04:51, Maciej S. Szmigiero wrote:
>>>>> From: "Maciej S. Szmigiero" <maciej.szmigiero@oracle.com>
>>>>>
>>>>> Currently, if one wants to make use of FUA support in libata it is
>>>>> necessary to provide an explicit kernel command line parameter in order to
>>>>> enable it (for drives that report such support).
>>>>>
>>>>> In terms of Git archaeology: FUA support was enabled by default in early
>>>>> libata versions but was disabled soon after.
>>>>> Since then there were a few attempts to enable this support by default:
>>>>> [1] (for NCQ drives only), [2] (for all drives).
>>>>> However, the second change had to be reverted after a report came of
>>>>> an incompatibility with the HDD in 2011 Mac Mini.
>>>>>
>>>>> Enabling FUA avoids having to emulate it by issuing an extra drive cache
>>>>> flush for every request that have this flag set.
>>>>> Since FUA support is required by the ATA/ATAPI spec for any drive that
>>>>> supports LBA48 and so these days should be pretty widespread let's provide
>>>>> an ability to enable it by default in Kconfig.
>>>>
>>>> This can be done by adding "libata.fua=1" to the CONFIG_CMDLINE option. So
>>>> I do not see the need to add yet another config option.
>>>
>>> A specific Kconfig option is more structured than a free-form
>>> CONFIG_CMDLINE (which is also technically a per-arch option, but seems
>>> to be widely supported across arches).
>>>
>>> That's why there is a lot (100+) of similar Kconfig default-changing
>>> options, a quick sample of these (in no particular order):
>>> SOUND_OSS_CORE_PRECLAIM, SND_INTEL_BYT_PREFER_SOF, LSM,
>>> SECURITY_SELINUX_CHECKREQPROT_VALUE, SECURITY_LOADPIN_ENFORCE,
>>> SECURITY_APPARMOR_DEBUG_MESSAGES, IP_VS_TAB_BITS, IP_SET_MAX,
>>> MAC80211_HAS_RC, SLUB_DEBUG_ON, KFENCE_SAMPLE_INTERVAL, PRINTK_TIME,
>>> DEBUG_OBJECTS_ENABLE_DEFAULT, RCU_NOCB_CPU_DEFAULT_ALL, ...
>>>
>>> libata currently has only one similar option: SATA_MOBILE_LPM_POLICY,
>>> so it's not like a person performing kernel configuration is
>>> overloaded with questions here.
>>>
>>> But at the same time, I respect your decision as a maintainer of
>>> this code.
>>
>> I am not dead set on pushing back on this, but as usual, whenever we can
>> avoid adding config options, we should. Given that libata has had fua
>> disabled forever, I am not convinced yet that there is a strong need for
>> that new option. But if distros prefer the config option approach, we can
>> make that happen.
>>
>> If anything, I would be tempted to switch fua support to on by default
>> after some time if we do not get many reports of broken drives. You did
>> mention that old mac minis drives did not like it, but these are not even
>> blacklisted in libata-scsi. They should. Only one model of maxtor drives
>> is. We should add an ATA_HORKAGE_NO_FUA flag and start a proper blacklist
>> of drives not liking fua. Without that in place, switching to on by
>> default as your config option allows could break many (old) systems.
> 
> To be extra clear, I think that this fua module parameter is silly. If a
> drive says it supports fua, we should use it and not have a global
> parameter to disable it for all drives. So no config option needed for it.
> 
> That is also why I am not keen on taking that config option. It is not
> really improving anything at all and would prefer nuking the fua module
> argument and have a proper blacklisting of buggy drives.
> 
> But such a change is painful as we'll be able to update the blacklist with
> users getting corrupted FSes on buggy drives. The time may have come to do
> this change though as the number of buggy drives out there is hopefully
> small enough now.

So your proposal is basically to switch the existing fua option default
to "on" and deal with the fallout (hopefully minimal) by blacklisting
misbehaving drives as they get reported, right?

In this case, my vote would be to still keep the "libata.fua" parameter
available (at least for the time being) so people have some way of
working broken drives around without having to recompile their kernel
(maybe also print a kernel log message if libata.fua=0 is provided asking
people to report these drive models to linux-ide@).

Thanks,
Maciej

Re: [PATCH 2/2] ata: allow enabling FUA support in Kconfig

[Damien Le Moal]

On 10/7/22 23:14, Maciej S. Szmigiero wrote:
> On 7.10.2022 00:53, Damien Le Moal wrote:
>> On 10/7/22 07:20, Damien Le Moal wrote:
>>> On 10/6/22 22:06, Maciej S. Szmigiero wrote:
>>>> On 6.10.2022 01:38, Damien Le Moal wrote:
>>>>> On 9/27/22 04:51, Maciej S. Szmigiero wrote:
>>>>>> From: "Maciej S. Szmigiero" <maciej.szmigiero@oracle.com>
>>>>>>
>>>>>> Currently, if one wants to make use of FUA support in libata it is
>>>>>> necessary to provide an explicit kernel command line parameter in order to
>>>>>> enable it (for drives that report such support).
>>>>>>
>>>>>> In terms of Git archaeology: FUA support was enabled by default in early
>>>>>> libata versions but was disabled soon after.
>>>>>> Since then there were a few attempts to enable this support by default:
>>>>>> [1] (for NCQ drives only), [2] (for all drives).
>>>>>> However, the second change had to be reverted after a report came of
>>>>>> an incompatibility with the HDD in 2011 Mac Mini.
>>>>>>
>>>>>> Enabling FUA avoids having to emulate it by issuing an extra drive cache
>>>>>> flush for every request that have this flag set.
>>>>>> Since FUA support is required by the ATA/ATAPI spec for any drive that
>>>>>> supports LBA48 and so these days should be pretty widespread let's provide
>>>>>> an ability to enable it by default in Kconfig.
>>>>>
>>>>> This can be done by adding "libata.fua=1" to the CONFIG_CMDLINE option. So
>>>>> I do not see the need to add yet another config option.
>>>>
>>>> A specific Kconfig option is more structured than a free-form
>>>> CONFIG_CMDLINE (which is also technically a per-arch option, but seems
>>>> to be widely supported across arches).
>>>>
>>>> That's why there is a lot (100+) of similar Kconfig default-changing
>>>> options, a quick sample of these (in no particular order):
>>>> SOUND_OSS_CORE_PRECLAIM, SND_INTEL_BYT_PREFER_SOF, LSM,
>>>> SECURITY_SELINUX_CHECKREQPROT_VALUE, SECURITY_LOADPIN_ENFORCE,
>>>> SECURITY_APPARMOR_DEBUG_MESSAGES, IP_VS_TAB_BITS, IP_SET_MAX,
>>>> MAC80211_HAS_RC, SLUB_DEBUG_ON, KFENCE_SAMPLE_INTERVAL, PRINTK_TIME,
>>>> DEBUG_OBJECTS_ENABLE_DEFAULT, RCU_NOCB_CPU_DEFAULT_ALL, ...
>>>>
>>>> libata currently has only one similar option: SATA_MOBILE_LPM_POLICY,
>>>> so it's not like a person performing kernel configuration is
>>>> overloaded with questions here.
>>>>
>>>> But at the same time, I respect your decision as a maintainer of
>>>> this code.
>>>
>>> I am not dead set on pushing back on this, but as usual, whenever we can
>>> avoid adding config options, we should. Given that libata has had fua
>>> disabled forever, I am not convinced yet that there is a strong need for
>>> that new option. But if distros prefer the config option approach, we can
>>> make that happen.
>>>
>>> If anything, I would be tempted to switch fua support to on by default
>>> after some time if we do not get many reports of broken drives. You did
>>> mention that old mac minis drives did not like it, but these are not even
>>> blacklisted in libata-scsi. They should. Only one model of maxtor drives
>>> is. We should add an ATA_HORKAGE_NO_FUA flag and start a proper blacklist
>>> of drives not liking fua. Without that in place, switching to on by
>>> default as your config option allows could break many (old) systems.
>>
>> To be extra clear, I think that this fua module parameter is silly. If a
>> drive says it supports fua, we should use it and not have a global
>> parameter to disable it for all drives. So no config option needed for it.
>>
>> That is also why I am not keen on taking that config option. It is not
>> really improving anything at all and would prefer nuking the fua module
>> argument and have a proper blacklisting of buggy drives.
>>
>> But such a change is painful as we'll be able to update the blacklist with
>> users getting corrupted FSes on buggy drives. The time may have come to do
>> this change though as the number of buggy drives out there is hopefully
>> small enough now.
> 
> So your proposal is basically to switch the existing fua option default
> to "on" and deal with the fallout (hopefully minimal) by blacklisting
> misbehaving drives as they get reported, right?

Yes. The risk though is that if the fallout are not minimal and we get too
many bug reports, we'll likely have to revert. So this needs to be
attempted early at the beginning of a cycle to get plenty of testing.

> In this case, my vote would be to still keep the "libata.fua" parameter
> available (at least for the time being) so people have some way of
> working broken drives around without having to recompile their kernel
> (maybe also print a kernel log message if libata.fua=0 is provided asking
> people to report these drive models to linux-ide@).

If we add a proper "nofua" horkage flag to blacklist buggy drives, we need
to move the fua parameter to be an argument of the force parameter so that
disabling fua can be done per drive (port) or for all drives, similarly to
other tweak (noncq, nodmalog, etc)

> 
> Thanks,
> Maciej
> 

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 2/2] ata: allow enabling FUA support in Kconfig

[Maciej S. Szmigiero]

On 8.10.2022 00:41, Damien Le Moal wrote:
> On 10/7/22 23:14, Maciej S. Szmigiero wrote:
>> On 7.10.2022 00:53, Damien Le Moal wrote:
>>> On 10/7/22 07:20, Damien Le Moal wrote:
>>>> On 10/6/22 22:06, Maciej S. Szmigiero wrote:
>>>>> On 6.10.2022 01:38, Damien Le Moal wrote:
>>>>>> On 9/27/22 04:51, Maciej S. Szmigiero wrote:
>>>>>>> From: "Maciej S. Szmigiero" <maciej.szmigiero@oracle.com>
>>>>>>>
>>>>>>> Currently, if one wants to make use of FUA support in libata it is
>>>>>>> necessary to provide an explicit kernel command line parameter in order to
>>>>>>> enable it (for drives that report such support).
>>>>>>>
>>>>>>> In terms of Git archaeology: FUA support was enabled by default in early
>>>>>>> libata versions but was disabled soon after.
>>>>>>> Since then there were a few attempts to enable this support by default:
>>>>>>> [1] (for NCQ drives only), [2] (for all drives).
>>>>>>> However, the second change had to be reverted after a report came of
>>>>>>> an incompatibility with the HDD in 2011 Mac Mini.
>>>>>>>
>>>>>>> Enabling FUA avoids having to emulate it by issuing an extra drive cache
>>>>>>> flush for every request that have this flag set.
>>>>>>> Since FUA support is required by the ATA/ATAPI spec for any drive that
>>>>>>> supports LBA48 and so these days should be pretty widespread let's provide
>>>>>>> an ability to enable it by default in Kconfig.
>>>>>>
>>>>>> This can be done by adding "libata.fua=1" to the CONFIG_CMDLINE option. So
>>>>>> I do not see the need to add yet another config option.
>>>>>
>>>>> A specific Kconfig option is more structured than a free-form
>>>>> CONFIG_CMDLINE (which is also technically a per-arch option, but seems
>>>>> to be widely supported across arches).
>>>>>
>>>>> That's why there is a lot (100+) of similar Kconfig default-changing
>>>>> options, a quick sample of these (in no particular order):
>>>>> SOUND_OSS_CORE_PRECLAIM, SND_INTEL_BYT_PREFER_SOF, LSM,
>>>>> SECURITY_SELINUX_CHECKREQPROT_VALUE, SECURITY_LOADPIN_ENFORCE,
>>>>> SECURITY_APPARMOR_DEBUG_MESSAGES, IP_VS_TAB_BITS, IP_SET_MAX,
>>>>> MAC80211_HAS_RC, SLUB_DEBUG_ON, KFENCE_SAMPLE_INTERVAL, PRINTK_TIME,
>>>>> DEBUG_OBJECTS_ENABLE_DEFAULT, RCU_NOCB_CPU_DEFAULT_ALL, ...
>>>>>
>>>>> libata currently has only one similar option: SATA_MOBILE_LPM_POLICY,
>>>>> so it's not like a person performing kernel configuration is
>>>>> overloaded with questions here.
>>>>>
>>>>> But at the same time, I respect your decision as a maintainer of
>>>>> this code.
>>>>
>>>> I am not dead set on pushing back on this, but as usual, whenever we can
>>>> avoid adding config options, we should. Given that libata has had fua
>>>> disabled forever, I am not convinced yet that there is a strong need for
>>>> that new option. But if distros prefer the config option approach, we can
>>>> make that happen.
>>>>
>>>> If anything, I would be tempted to switch fua support to on by default
>>>> after some time if we do not get many reports of broken drives. You did
>>>> mention that old mac minis drives did not like it, but these are not even
>>>> blacklisted in libata-scsi. They should. Only one model of maxtor drives
>>>> is. We should add an ATA_HORKAGE_NO_FUA flag and start a proper blacklist
>>>> of drives not liking fua. Without that in place, switching to on by
>>>> default as your config option allows could break many (old) systems.
>>>
>>> To be extra clear, I think that this fua module parameter is silly. If a
>>> drive says it supports fua, we should use it and not have a global
>>> parameter to disable it for all drives. So no config option needed for it.
>>>
>>> That is also why I am not keen on taking that config option. It is not
>>> really improving anything at all and would prefer nuking the fua module
>>> argument and have a proper blacklisting of buggy drives.
>>>
>>> But such a change is painful as we'll be able to update the blacklist with
>>> users getting corrupted FSes on buggy drives. The time may have come to do
>>> this change though as the number of buggy drives out there is hopefully
>>> small enough now.
>>
>> So your proposal is basically to switch the existing fua option default
>> to "on" and deal with the fallout (hopefully minimal) by blacklisting
>> misbehaving drives as they get reported, right?
> 
> Yes. The risk though is that if the fallout are not minimal and we get too
> many bug reports, we'll likely have to revert. So this needs to be
> attempted early at the beginning of a cycle to get plenty of testing.
> 
>> In this case, my vote would be to still keep the "libata.fua" parameter
>> available (at least for the time being) so people have some way of
>> working broken drives around without having to recompile their kernel
>> (maybe also print a kernel log message if libata.fua=0 is provided asking
>> people to report these drive models to linux-ide@).
> 
> If we add a proper "nofua" horkage flag to blacklist buggy drives, we need
> to move the fua parameter to be an argument of the force parameter so that
> disabling fua can be done per drive (port) or for all drives, similarly to
> other tweak (noncq, nodmalog, etc)

So would you like an updated patch set or do you prefer to do the changes
yourself?

Thanks,
Maciej

Re: [PATCH 2/2] ata: allow enabling FUA support in Kconfig

[Damien Le Moal]

On 10/15/22 01:44, Maciej S. Szmigiero wrote:
> On 8.10.2022 00:41, Damien Le Moal wrote:
>> On 10/7/22 23:14, Maciej S. Szmigiero wrote:
>>> On 7.10.2022 00:53, Damien Le Moal wrote:
>>>> On 10/7/22 07:20, Damien Le Moal wrote:
>>>>> On 10/6/22 22:06, Maciej S. Szmigiero wrote:
>>>>>> On 6.10.2022 01:38, Damien Le Moal wrote:
>>>>>>> On 9/27/22 04:51, Maciej S. Szmigiero wrote:
>>>>>>>> From: "Maciej S. Szmigiero" <maciej.szmigiero@oracle.com>
>>>>>>>>
>>>>>>>> Currently, if one wants to make use of FUA support in libata it is
>>>>>>>> necessary to provide an explicit kernel command line parameter in order to
>>>>>>>> enable it (for drives that report such support).
>>>>>>>>
>>>>>>>> In terms of Git archaeology: FUA support was enabled by default in early
>>>>>>>> libata versions but was disabled soon after.
>>>>>>>> Since then there were a few attempts to enable this support by default:
>>>>>>>> [1] (for NCQ drives only), [2] (for all drives).
>>>>>>>> However, the second change had to be reverted after a report came of
>>>>>>>> an incompatibility with the HDD in 2011 Mac Mini.
>>>>>>>>
>>>>>>>> Enabling FUA avoids having to emulate it by issuing an extra drive cache
>>>>>>>> flush for every request that have this flag set.
>>>>>>>> Since FUA support is required by the ATA/ATAPI spec for any drive that
>>>>>>>> supports LBA48 and so these days should be pretty widespread let's provide
>>>>>>>> an ability to enable it by default in Kconfig.
>>>>>>>
>>>>>>> This can be done by adding "libata.fua=1" to the CONFIG_CMDLINE option. So
>>>>>>> I do not see the need to add yet another config option.
>>>>>>
>>>>>> A specific Kconfig option is more structured than a free-form
>>>>>> CONFIG_CMDLINE (which is also technically a per-arch option, but seems
>>>>>> to be widely supported across arches).
>>>>>>
>>>>>> That's why there is a lot (100+) of similar Kconfig default-changing
>>>>>> options, a quick sample of these (in no particular order):
>>>>>> SOUND_OSS_CORE_PRECLAIM, SND_INTEL_BYT_PREFER_SOF, LSM,
>>>>>> SECURITY_SELINUX_CHECKREQPROT_VALUE, SECURITY_LOADPIN_ENFORCE,
>>>>>> SECURITY_APPARMOR_DEBUG_MESSAGES, IP_VS_TAB_BITS, IP_SET_MAX,
>>>>>> MAC80211_HAS_RC, SLUB_DEBUG_ON, KFENCE_SAMPLE_INTERVAL, PRINTK_TIME,
>>>>>> DEBUG_OBJECTS_ENABLE_DEFAULT, RCU_NOCB_CPU_DEFAULT_ALL, ...
>>>>>>
>>>>>> libata currently has only one similar option: SATA_MOBILE_LPM_POLICY,
>>>>>> so it's not like a person performing kernel configuration is
>>>>>> overloaded with questions here.
>>>>>>
>>>>>> But at the same time, I respect your decision as a maintainer of
>>>>>> this code.
>>>>>
>>>>> I am not dead set on pushing back on this, but as usual, whenever we can
>>>>> avoid adding config options, we should. Given that libata has had fua
>>>>> disabled forever, I am not convinced yet that there is a strong need for
>>>>> that new option. But if distros prefer the config option approach, we can
>>>>> make that happen.
>>>>>
>>>>> If anything, I would be tempted to switch fua support to on by default
>>>>> after some time if we do not get many reports of broken drives. You did
>>>>> mention that old mac minis drives did not like it, but these are not even
>>>>> blacklisted in libata-scsi. They should. Only one model of maxtor drives
>>>>> is. We should add an ATA_HORKAGE_NO_FUA flag and start a proper blacklist
>>>>> of drives not liking fua. Without that in place, switching to on by
>>>>> default as your config option allows could break many (old) systems.
>>>>
>>>> To be extra clear, I think that this fua module parameter is silly. If a
>>>> drive says it supports fua, we should use it and not have a global
>>>> parameter to disable it for all drives. So no config option needed for it.
>>>>
>>>> That is also why I am not keen on taking that config option. It is not
>>>> really improving anything at all and would prefer nuking the fua module
>>>> argument and have a proper blacklisting of buggy drives.
>>>>
>>>> But such a change is painful as we'll be able to update the blacklist with
>>>> users getting corrupted FSes on buggy drives. The time may have come to do
>>>> this change though as the number of buggy drives out there is hopefully
>>>> small enough now.
>>>
>>> So your proposal is basically to switch the existing fua option default
>>> to "on" and deal with the fallout (hopefully minimal) by blacklisting
>>> misbehaving drives as they get reported, right?
>>
>> Yes. The risk though is that if the fallout are not minimal and we get too
>> many bug reports, we'll likely have to revert. So this needs to be
>> attempted early at the beginning of a cycle to get plenty of testing.
>>
>>> In this case, my vote would be to still keep the "libata.fua" parameter
>>> available (at least for the time being) so people have some way of
>>> working broken drives around without having to recompile their kernel
>>> (maybe also print a kernel log message if libata.fua=0 is provided asking
>>> people to report these drive models to linux-ide@).
>>
>> If we add a proper "nofua" horkage flag to blacklist buggy drives, we need
>> to move the fua parameter to be an argument of the force parameter so that
>> disabling fua can be done per drive (port) or for all drives, similarly to
>> other tweak (noncq, nodmalog, etc)
> 
> So would you like an updated patch set or do you prefer to do the changes
> yourself?

Almost done already :) Need a lot of testing though.
Will post the patches when done.

Note that for now, I kept the fua module parameter, for compatibility with
existing setups. But I added the parameter force=[no]fua which allows
doing the same global enable/disable but also per drive enable/disable.
And known bad drives can be marked with the horkage flag.

> 
> Thanks,
> Maciej
> 

-- 
Damien Le Moal
Western Digital Research