Re: [PATCH 02/21] libsas: Only abort commands from inside the error handler

On 7/3/21 4:32 AM, Jason Yan wrote:

Hi Bart,

在 2021/7/2 5:12, Bart Van Assche 写道:

quoted

According to the information I found in patch commit descriptions, for 
SATA
devices commands must be aborted from inside the libsas error handler.
Check host->ehandler to determine whether or not running inside the error
handler since host->host_eh_scheduled != 0 indicates that the SCSI error
handler has been scheduled but does not mean that is already running. 
This
patch restores code that was removed by commit 909657615d9b ("scsi: 
libsas:
allow async aborts").

Cc: Hannes Reinecke <hare@suse.de>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Ming Lei <redacted>
Cc: John Garry <redacted>
Cc: Yves-Alexis Perez <redacted>
Fixes: c9f926000fe3 ("scsi: libsas: Disable asynchronous aborts for 
SATA devices")
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
  drivers/scsi/libsas/sas_scsi_host.c | 5 ++++-
  include/scsi/libsas.h               | 1 +
  2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/libsas/sas_scsi_host.c 

b/drivers/scsi/libsas/sas_scsi_host.c
index ee44a0d7730b..95e4d58ab9d4 100644

--- a/drivers/scsi/libsas/sas_scsi_host.c
+++ b/drivers/scsi/libsas/sas_scsi_host.c

@@ -462,6 +462,7 @@ int sas_eh_abort_handler(struct scsi_cmnd *cmd)

      int res = TMF_RESP_FUNC_FAILED;
      struct sas_task *task = TO_SAS_TASK(cmd);
      struct Scsi_Host *host = cmd->device->host;
+    struct sas_ha_struct *ha = SHOST_TO_SAS_HA(host);
      struct domain_device *dev = cmd_to_domain_dev(cmd);
      struct sas_internal *i = to_sas_internal(host->transportt);
      unsigned long flags;

@@ -471,7 +472,7 @@ int sas_eh_abort_handler(struct scsi_cmnd *cmd)

      spin_lock_irqsave(host->host_lock, flags);
      /* We cannot do async aborts for SATA devices */
-    if (dev_is_sata(dev) && !host->host_eh_scheduled) {
+    if (dev_is_sata(dev) && !ha->eh_running) {
          spin_unlock_irqrestore(host->host_lock, flags);
          return FAILED;
      }

No idea why sas_eh_abort_handler() is only used by isci. The other
libsas drivers just let the error handler do the aborting work. So my
question is can the isci driver delete this callback and let the error
handler do this? If so, then we cann remove this function directly.

That, indeed, is a good point. SATA commands are being handled by the 
sas_ata_strategy_handler (as they don't really match the SAM logic wrt TMF).
And actually there is no 'abort' command on SATA; the best you can do is 
'abort queue', but that's more like an 'ABORT TASK SET' in SAM.
So either way that callback is pointless.

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: Felix Imendörffer