[PATCH v1 0/2] scsi: libsas: An improvement on error handle and tidy-up

[PATCH v1 0/2] scsi: libsas: An improvement on error handle and tidy-up

[Luo Jiaxing]

This patch introduces an improvement to reduce error handle time and a
tidy-up, including: 
- postreset() is deleted from sas_sata_ops. 
- Do not perform hard reset and delayed retry on a removed SATA disk. This
can effectively reduce the error handle duration of hot unplug a SATA disk
with traffic(reduce about 30s depending on the delay setting of libata). 

Both John garry and Jason Yan participated in the review of the solution
and provided good suggestions during the development. 

Luo Jiaxing (2):
  {topost} scsi: libsas: delete postreset at sas_sata_ops
  {topost} scsi: libsas: check link status at ATA prereset() ops

 drivers/scsi/libsas/sas_ata.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

-- 
2.7.4

[PATCH v1 1/2] {topost} scsi: libsas: delete postreset at sas_sata_ops

[Luo Jiaxing]

We fill postreset with ata_std_postreset() at sas_sata_ops before, but we
found out that ata_std_postreset() call sata_scr_read()/sata_scr_write()
which need to access SCR register. Actually we don't own these kind of
register, so sata_scr_read()/sata_scr_write always return -EOPNOTSUPP.

We drop ata_std_postreset() at sas_sata_ops.

Signed-off-by: Luo Jiaxing <luojiaxing@huawei.com>
Reviewed-by: John Garry <john.garry@huawei.com>
---
 drivers/scsi/libsas/sas_ata.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/scsi/libsas/sas_ata.c b/drivers/scsi/libsas/sas_ata.c
index 5d716d3..a7d16d2 100644
--- a/drivers/scsi/libsas/sas_ata.c
+++ b/drivers/scsi/libsas/sas_ata.c
@@ -510,7 +510,6 @@ void sas_ata_end_eh(struct ata_port *ap)
 static struct ata_port_operations sas_sata_ops = {
 	.prereset		= ata_std_prereset,
 	.hardreset		= sas_ata_hard_reset,
-	.postreset		= ata_std_postreset,
 	.error_handler		= ata_std_error_handler,
 	.post_internal_cmd	= sas_ata_post_internal,
 	.qc_defer               = ata_std_qc_defer,
-- 
2.7.4

Re: [PATCH v1 1/2] {topost} scsi: libsas: delete postreset at sas_sata_ops

[Jason Yan]

在 2020/7/22 17:04, Luo Jiaxing 写道:
> We fill postreset with ata_std_postreset() at sas_sata_ops before, but we
> found out that ata_std_postreset() call sata_scr_read()/sata_scr_write()
> which need to access SCR register. Actually we don't own these kind of
> register, so sata_scr_read()/sata_scr_write always return -EOPNOTSUPP.
> 
> We drop ata_std_postreset() at sas_sata_ops.
> 
> Signed-off-by: Luo Jiaxing <luojiaxing@huawei.com>
> Reviewed-by: John Garry <john.garry@huawei.com>
> ---
>   drivers/scsi/libsas/sas_ata.c | 1 -
>   1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/scsi/libsas/sas_ata.c b/drivers/scsi/libsas/sas_ata.c
> index 5d716d3..a7d16d2 100644
> --- a/drivers/scsi/libsas/sas_ata.c
> +++ b/drivers/scsi/libsas/sas_ata.c
> @@ -510,7 +510,6 @@ void sas_ata_end_eh(struct ata_port *ap)
>   static struct ata_port_operations sas_sata_ops = {
>   	.prereset		= ata_std_prereset,
>   	.hardreset		= sas_ata_hard_reset,
> -	.postreset		= ata_std_postreset,
>   	.error_handler		= ata_std_error_handler,
>   	.post_internal_cmd	= sas_ata_post_internal,
>   	.qc_defer               = ata_std_qc_defer,
> 

Hi Luo,

Please remove the "{topost}" in the subject, other than that:

Reviewed-by: Jason Yan <yanaijie@huawei.com>

[PATCH v1 2/2] {topost} scsi: libsas: check link status at ATA prereset() ops

[Luo Jiaxing]

We found out that libata will retry reset even if SATA disk is unpluged. We
should report offline to libata to avoid meaningless reset on the disk.
Libata provide an ops of prereset() for this purpose, it was called by
ata_eh_reset() only and used to decide whether to skip reset base on the
return value of it.

We check status of phy and disk at prereset(). If disk is already offline
or phy is disabled, we return -ENOENT to libata to skip disk reset.

As prereset() should be best-effort, we should continue to try disk reset
beyond the situation we mentioned before.

Signed-off-by: Luo Jiaxing <luojiaxing@huawei.com>
Reviewed-by: John Garry <john.garry@huawei.com>
---
 drivers/scsi/libsas/sas_ata.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/libsas/sas_ata.c b/drivers/scsi/libsas/sas_ata.c
index a7d16d2..1b93332 100644
--- a/drivers/scsi/libsas/sas_ata.c
+++ b/drivers/scsi/libsas/sas_ata.c
@@ -507,8 +507,22 @@ void sas_ata_end_eh(struct ata_port *ap)
 	spin_unlock_irqrestore(&ha->lock, flags);
 }
 
+static int sas_ata_prereset(struct ata_link *link, unsigned long deadline)
+{
+	struct ata_port *ap = link->ap;
+	struct domain_device *dev = ap->private_data;
+	struct sas_phy *local_phy = sas_get_local_phy(dev);
+	int res = 0;
+
+	if (!local_phy->enabled || test_bit(SAS_DEV_GONE, &dev->state))
+		res = -ENOENT;
+	sas_put_local_phy(local_phy);
+
+	return res;
+}
+
 static struct ata_port_operations sas_sata_ops = {
-	.prereset		= ata_std_prereset,
+	.prereset		= sas_ata_prereset,
 	.hardreset		= sas_ata_hard_reset,
 	.error_handler		= ata_std_error_handler,
 	.post_internal_cmd	= sas_ata_post_internal,
-- 
2.7.4

Re: [PATCH v1 2/2] {topost} scsi: libsas: check link status at ATA prereset() ops

[Jason Yan]

在 2020/7/22 17:04, Luo Jiaxing 写道:
> We found out that libata will retry reset even if SATA disk is unpluged. We
> should report offline to libata to avoid meaningless reset on the disk.
> Libata provide an ops of prereset() for this purpose, it was called by
> ata_eh_reset() only and used to decide whether to skip reset base on the
> return value of it.
> 
> We check status of phy and disk at prereset(). If disk is already offline
> or phy is disabled, we return -ENOENT to libata to skip disk reset.
> 
> As prereset() should be best-effort, we should continue to try disk reset
> beyond the situation we mentioned before.
> 
> Signed-off-by: Luo Jiaxing <luojiaxing@huawei.com>
> Reviewed-by: John Garry <john.garry@huawei.com>
> ---
>   drivers/scsi/libsas/sas_ata.c | 16 +++++++++++++++-
>   1 file changed, 15 insertions(+), 1 deletion(-)
> 

The same as the first one, after fix the  subject:

Reviewed-by: Jason Yan <yanaijie@huawei.com>

Re: [PATCH v1 0/2] scsi: libsas: An improvement on error handle and tidy-up

[Martin K. Petersen]

On Wed, 22 Jul 2020 17:04:01 +0800, Luo Jiaxing wrote:

> This patch introduces an improvement to reduce error handle time and a
> tidy-up, including:
> - postreset() is deleted from sas_sata_ops.
> - Do not perform hard reset and delayed retry on a removed SATA disk. This
> can effectively reduce the error handle duration of hot unplug a SATA disk
> with traffic(reduce about 30s depending on the delay setting of libata).
> 
> [...]

Applied to 5.9/scsi-queue, thanks!

[1/2] scsi: libsas: Remove postreset from sas_sata_ops
      https://git.kernel.org/mkp/scsi/c/3a243c2c3500
[2/2] scsi: libsas: Check link status in ATA prereset()
      https://git.kernel.org/mkp/scsi/c/386533796574

-- 
Martin K. Petersen	Oracle Linux Engineering