diff mbox series

[v6,7/7] ata: libata: Enable fua support by default

Message ID 20221108055544.1481583-8-damien.lemoal@opensource.wdc.com (mailing list archive)
State New, archived
Headers show
Series Improve libata support for FUA | expand

Commit Message

Damien Le Moal Nov. 8, 2022, 5:55 a.m. UTC
Change the default value of the fua module parameter to 1 to enable fua
support by default for all devices supporting it.

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

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

Comments

Johannes Thumshirn Nov. 8, 2022, 7:45 a.m. UTC | #1
Looks good,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
Niklas Cassel Dec. 30, 2022, 10:54 a.m. UTC | #2
On Tue, Nov 08, 2022 at 02:55:44PM +0900, 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.
> 
> FUA support can be disabled for individual drives using the
> force=[ID]nofua libata module argument.
> 
> Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
> Reviewed-by: Hannes Reinecke <hare@suse.de>
> Reviewed-by: Maciej S. Szmigiero <maciej.szmigiero@oracle.com>
> Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>
> ---
>  drivers/ata/libata-core.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index 97ade977b830..2967671131d2 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -127,9 +127,9 @@ int atapi_passthru16 = 1;
>  module_param(atapi_passthru16, int, 0444);
>  MODULE_PARM_DESC(atapi_passthru16, "Enable ATA_16 passthru for ATAPI devices (0=off, 1=on [default])");
>  
> -int libata_fua = 0;
> +int libata_fua = 1;
>  module_param_named(fua, libata_fua, int, 0444);
> -MODULE_PARM_DESC(fua, "FUA support (0=off [default], 1=on)");
> +MODULE_PARM_DESC(fua, "FUA support (0=off, 1=on [default])");
>  
>  static int ata_ignore_hpa;
>  module_param_named(ignore_hpa, ata_ignore_hpa, int, 0644);
> -- 
> 2.38.1
> 

Before this commit:
-For a SCSI mode sense:
 FUA bit will not get set in the simulated SCSI cmd output buffer,
 since ATA_DFLAG_FUA is not be set, since libata_fua == 0.
-For a SCSI WRITE_16 / WRITE_16 command:
 ata_scsi_rw_xlat() sets ATA_TFLAG_FUA, regardless if ATA_DFLAG_FUA
 is set or not. ata_set_rwcmd_protocol() will return a FUA command
 as long as ATA_TFLAG_FUA is set.

After this commit:
-For a SCSI mode sense:
 FUA bit will get set in the simulated SCSI cmd output buffer,
 since ATA_DFLAG_FUA is set, since libata_fua == 1.
-For a SCSI WRITE_16 / WRITE_16 command:
 ata_scsi_rw_xlat() sets ATA_TFLAG_FUA, regardless if ATA_DFLAG_FUA
 is set or not. ata_set_rwcmd_protocol() will return a FUA command
 as long as ATA_TFLAG_FUA is set.


Perhaps this commit should more clearly say that this commit only affects
the simulated output for a SCSI mode sense command?

Currently, the commit message is a bit misleading, since it makes the
reader think that a SCSI write command with the FUA bit was not propagated
to the device before this commit, which AFAICT, is not true.

If the intention of this series was to only send a ATA write command to
the device, if the device has the ATA_DFLAG_FUA flag set, then perhaps the
series should include a new commit which either:
-Adds a check to ata_scsi_rw_xlat() so that it only sets ATA_TFLAG_FUA if
ATA_DFLAG_FUA is set;
or
-Adds a check to ata_scsi_rw_xlat() which returns an error if the SCSI
command has the FUA bit set, but ATA_DFLAG_FUA is not set.


Kind regards,
Niklas
Damien Le Moal Dec. 30, 2022, 11:21 a.m. UTC | #3
On 12/30/22 19:54, Niklas Cassel wrote:
> On Tue, Nov 08, 2022 at 02:55:44PM +0900, 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.
>>
>> FUA support can be disabled for individual drives using the
>> force=[ID]nofua libata module argument.
>>
>> Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
>> Reviewed-by: Hannes Reinecke <hare@suse.de>
>> Reviewed-by: Maciej S. Szmigiero <maciej.szmigiero@oracle.com>
>> Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>
>> ---
>>  drivers/ata/libata-core.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
>> index 97ade977b830..2967671131d2 100644
>> --- a/drivers/ata/libata-core.c
>> +++ b/drivers/ata/libata-core.c
>> @@ -127,9 +127,9 @@ int atapi_passthru16 = 1;
>>  module_param(atapi_passthru16, int, 0444);
>>  MODULE_PARM_DESC(atapi_passthru16, "Enable ATA_16 passthru for ATAPI devices (0=off, 1=on [default])");
>>  
>> -int libata_fua = 0;
>> +int libata_fua = 1;
>>  module_param_named(fua, libata_fua, int, 0444);
>> -MODULE_PARM_DESC(fua, "FUA support (0=off [default], 1=on)");
>> +MODULE_PARM_DESC(fua, "FUA support (0=off, 1=on [default])");
>>  
>>  static int ata_ignore_hpa;
>>  module_param_named(ignore_hpa, ata_ignore_hpa, int, 0644);
>> -- 
>> 2.38.1
>>
> 
> Before this commit:
> -For a SCSI mode sense:
>  FUA bit will not get set in the simulated SCSI cmd output buffer,
>  since ATA_DFLAG_FUA is not be set, since libata_fua == 0.
> -For a SCSI WRITE_16 / WRITE_16 command:
>  ata_scsi_rw_xlat() sets ATA_TFLAG_FUA, regardless if ATA_DFLAG_FUA
>  is set or not. ata_set_rwcmd_protocol() will return a FUA command
>  as long as ATA_TFLAG_FUA is set.
> 
> After this commit:
> -For a SCSI mode sense:
>  FUA bit will get set in the simulated SCSI cmd output buffer,
>  since ATA_DFLAG_FUA is set, since libata_fua == 1.
> -For a SCSI WRITE_16 / WRITE_16 command:
>  ata_scsi_rw_xlat() sets ATA_TFLAG_FUA, regardless if ATA_DFLAG_FUA
>  is set or not. ata_set_rwcmd_protocol() will return a FUA command
>  as long as ATA_TFLAG_FUA is set.
> 
> 
> Perhaps this commit should more clearly say that this commit only affects
> the simulated output for a SCSI mode sense command?
> 
> Currently, the commit message is a bit misleading, since it makes the
> reader think that a SCSI write command with the FUA bit was not propagated
> to the device before this commit, which AFAICT, is not true.

It was not with the default libata.fua = 0, since the drive would never be
exposed as having FUA support. See ata_dev_supports_fua() and its test for
"!libata_fua" and how that function was used in ata_scsiop_mode_sense().

The confusing thing here is that indeed there is no code preventing
propagating FUA bit to the device in libata-core/libata-scsi for any
REQ_FUA request. But the fact that devices would never report FUA support
means that the block layer would never issue a request with REQ_FUA set.

> 
> If the intention of this series was to only send a ATA write command to
> the device, if the device has the ATA_DFLAG_FUA flag set, then perhaps the
> series should include a new commit which either:
> -Adds a check to ata_scsi_rw_xlat() so that it only sets ATA_TFLAG_FUA if
> ATA_DFLAG_FUA is set;

See above. I do not think that is needed since the block layer will never
ever send a REQ_FUA request when ATA_DFLAG_FUA is not set.

> or
> -Adds a check to ata_scsi_rw_xlat() which returns an error if the SCSI
> command has the FUA bit set, but ATA_DFLAG_FUA is not set.

That would be possible for passthrough commands only. What is going to
happen is that we'll now get an error for that write if the drive does not
support NCQ or has NCQ disabled.

> 
> 
> Kind regards,
> Niklas
Niklas Cassel Dec. 30, 2022, 12:41 p.m. UTC | #4
On Fri, Dec 30, 2022 at 08:21:59PM +0900, Damien Le Moal wrote:
> On 12/30/22 19:54, Niklas Cassel wrote:
> >
> > Perhaps this commit should more clearly say that this commit only affects
> > the simulated output for a SCSI mode sense command?
> >
> > Currently, the commit message is a bit misleading, since it makes the
> > reader think that a SCSI write command with the FUA bit was not propagated
> > to the device before this commit, which AFAICT, is not true.
>
> It was not with the default libata.fua = 0, since the drive would never be
> exposed as having FUA support. See ata_dev_supports_fua() and its test for
> "!libata_fua" and how that function was used in ata_scsiop_mode_sense().

I see, drivers/scsi/sd.c does a SCSI mode sense to check if FUA is reported
as being supported, and then calls the block layer setter for this.

So changing the simulated SCSI cmd output is sufficient to tell the block
layer that it can send FUA requests to us.

>
> The confusing thing here is that indeed there is no code preventing
> propagating FUA bit to the device in libata-core/libata-scsi for any
> REQ_FUA request. But the fact that devices would never report FUA support
> means that the block layer would never issue a request with REQ_FUA set.

Well, ata_scsi_rw_xlat() is used both for passthrough commands and commands
issued by the block layer.

Just like how ata_scsiop_mode_sense() will be called regardless if it was a
passthrough command or a command issued by the block layer.

ata_scsiop_mode_sense() will not behave differently if it was a command
issued by the block layer or if it was e.g. a SG command.

I would argue that it makes sense (pun intended) for ata_scsi_rw_xlat() to
also behave the same, regardless if it was a command issued by the block
layer of if it was e.g. a SG command, but I will leave that to you to decide.


> That would be possible for passthrough commands only. What is going to
> happen is that we'll now get an error for that write if the drive does not
> support NCQ or has NCQ disabled.

Why would it give an error on a drive that has NCQ disabled?

Your new code looks like this:

static void ata_dev_config_fua(struct ata_device *dev)
{
	/* Ignore FUA support if its use is disabled globally */
	if (!libata_fua)
		goto nofua;

	/* Ignore devices without support for WRITE DMA FUA EXT */
	if (!(dev->flags & ATA_DFLAG_LBA48) || !ata_id_has_fua(dev->id))
		goto nofua;

	/* Ignore known bad devices and devices that lack NCQ support */
	if (!ata_ncq_supported(dev) || (dev->horkage & ATA_HORKAGE_NO_FUA))
		goto nofua;

	dev->flags |= ATA_DFLAG_FUA;

	return;

nofua:
	dev->flags &= ~ATA_DFLAG_FUA;
}

So it should only give an error for a drive where NCQ is not supported.
If NCQ is supported, but disabled, you will still send a ATA_CMD_WRITE_FUA_EXT
command.


Kind regards,
Niklas
Damien Le Moal Dec. 30, 2022, 12:55 p.m. UTC | #5
On 12/30/22 21:41, Niklas Cassel wrote:
> On Fri, Dec 30, 2022 at 08:21:59PM +0900, Damien Le Moal wrote:
>> On 12/30/22 19:54, Niklas Cassel wrote:
>>>
>>> Perhaps this commit should more clearly say that this commit only affects
>>> the simulated output for a SCSI mode sense command?
>>>
>>> Currently, the commit message is a bit misleading, since it makes the
>>> reader think that a SCSI write command with the FUA bit was not propagated
>>> to the device before this commit, which AFAICT, is not true.
>>
>> It was not with the default libata.fua = 0, since the drive would never be
>> exposed as having FUA support. See ata_dev_supports_fua() and its test for
>> "!libata_fua" and how that function was used in ata_scsiop_mode_sense().
> 
> I see, drivers/scsi/sd.c does a SCSI mode sense to check if FUA is reported
> as being supported, and then calls the block layer setter for this.
> 
> So changing the simulated SCSI cmd output is sufficient to tell the block
> layer that it can send FUA requests to us.
> 
>>
>> The confusing thing here is that indeed there is no code preventing
>> propagating FUA bit to the device in libata-core/libata-scsi for any
>> REQ_FUA request. But the fact that devices would never report FUA support
>> means that the block layer would never issue a request with REQ_FUA set.
> 
> Well, ata_scsi_rw_xlat() is used both for passthrough commands and commands
> issued by the block layer.
> 
> Just like how ata_scsiop_mode_sense() will be called regardless if it was a
> passthrough command or a command issued by the block layer.
> 
> ata_scsiop_mode_sense() will not behave differently if it was a command
> issued by the block layer or if it was e.g. a SG command.
> 
> I would argue that it makes sense (pun intended) for ata_scsi_rw_xlat() to
> also behave the same, regardless if it was a command issued by the block
> layer of if it was e.g. a SG command, but I will leave that to you to decide.

Yes, we can improve that. The current series does not change the
relatively (and really old) bad handling of passthrough FUA commands. So
this is something we can do on top of the fua series.

> 
> 
>> That would be possible for passthrough commands only. What is going to
>> happen is that we'll now get an error for that write if the drive does not
>> support NCQ or has NCQ disabled.
> 
> Why would it give an error on a drive that has NCQ disabled?

My bad. It will not. In that case, WRITE FUA EXT will be used for an FUA
write.

> 
> Your new code looks like this:
> 
> static void ata_dev_config_fua(struct ata_device *dev)
> {
> 	/* Ignore FUA support if its use is disabled globally */
> 	if (!libata_fua)
> 		goto nofua;
> 
> 	/* Ignore devices without support for WRITE DMA FUA EXT */
> 	if (!(dev->flags & ATA_DFLAG_LBA48) || !ata_id_has_fua(dev->id))
> 		goto nofua;
> 
> 	/* Ignore known bad devices and devices that lack NCQ support */
> 	if (!ata_ncq_supported(dev) || (dev->horkage & ATA_HORKAGE_NO_FUA))
> 		goto nofua;
> 
> 	dev->flags |= ATA_DFLAG_FUA;
> 
> 	return;
> 
> nofua:
> 	dev->flags &= ~ATA_DFLAG_FUA;
> }
> 
> So it should only give an error for a drive where NCQ is not supported.
> If NCQ is supported, but disabled, you will still send a ATA_CMD_WRITE_FUA_EXT
> command.

Yes.

> 
> 
> Kind regards,
> Niklas
Niklas Cassel Dec. 30, 2022, 2:47 p.m. UTC | #6
On Tue, Nov 08, 2022 at 02:55:44PM +0900, 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.

Perhaps add the following to the commit message:

With this change, ata_dev_config_fua() will now set the flag
ATA_DFLAG_FUA by default for devices that support FUA.

This will cause ata_scsiop_mode_sense() to set the DPOFUA bit in
the DEVICE-SPECIFIC PARAMETER field in the mode parameter header.

The SCSI disk driver performs a MODE SENSE and looks at the
DPOFUA bit, it then calls blk_queue_write_cache() with the
value of the DPOFUA bit, to inform the the block layer if FUA
is supported or not.

The block layer will not issue REQ_FUA requests if it hasn't
been informed that the lower levels actually support FUA.

> 
> FUA support can be disabled for individual drives using the
> force=[ID]nofua libata module argument.
> 
> Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
> Reviewed-by: Hannes Reinecke <hare@suse.de>
> Reviewed-by: Maciej S. Szmigiero <maciej.szmigiero@oracle.com>
> Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>
> ---
>  drivers/ata/libata-core.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index 97ade977b830..2967671131d2 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -127,9 +127,9 @@ int atapi_passthru16 = 1;
>  module_param(atapi_passthru16, int, 0444);
>  MODULE_PARM_DESC(atapi_passthru16, "Enable ATA_16 passthru for ATAPI devices (0=off, 1=on [default])");
>  
> -int libata_fua = 0;
> +int libata_fua = 1;
>  module_param_named(fua, libata_fua, int, 0444);
> -MODULE_PARM_DESC(fua, "FUA support (0=off [default], 1=on)");
> +MODULE_PARM_DESC(fua, "FUA support (0=off, 1=on [default])");
>  
>  static int ata_ignore_hpa;
>  module_param_named(ignore_hpa, ata_ignore_hpa, int, 0644);
> -- 
> 2.38.1
> 

With a less spartan commit message:
Reviewed-by: Niklas Cassel <niklas.cassel@wdc.com>
diff mbox series

Patch

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