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

From: Jason Yan <yanaijie@huawei.com>
To: Bart Van Assche <bvanassche@acm.org>,
	"Martin K . Petersen" <martin.petersen@oracle.com>
Cc: <linux-scsi@vger.kernel.org>, Jaegeuk Kim <jaegeuk@kernel.org>,
	Hannes Reinecke <hare@suse.de>, Christoph Hellwig <hch@lst.de>,
	Ming Lei <ming.lei@redhat.com>,
	John Garry <john.garry@huawei.com>,
	Yves-Alexis Perez <corsac@debian.org>,
	"James E.J. Bottomley" <jejb@linux.ibm.com>,
	"Gustavo A. R. Silva" <gustavoars@kernel.org>,
	Yufen Yu <yuyufen@huawei.com>
Subject: Re: [PATCH 02/21] libsas: Only abort commands from inside the error handler
Date: Sat, 3 Jul 2021 10:32:06 +0800	[thread overview]
Message-ID: <779caa02-6c9f-183b-2f3d-2b7dc5c877ef@huawei.com> (raw)
In-Reply-To: <20210701211224.17070-3-bvanassche@acm.org>

Hi Bart,

在 2021/7/2 5:12, Bart Van Assche 写道:
> 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 <ming.lei@redhat.com>
> Cc: John Garry <john.garry@huawei.com>
> Cc: Yves-Alexis Perez <corsac@debian.org>
> 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.

Thanks,
Jason

> @@ -731,6 +732,7 @@ void sas_scsi_recover_host(struct Scsi_Host *shost)
>   	tries++;
>   	retry = true;
>   	spin_lock_irq(shost->host_lock);
> +	ha->eh_running = true;
>   	list_splice_init(&shost->eh_cmd_q, &eh_work_q);
>   	spin_unlock_irq(shost->host_lock);
>   
> @@ -767,6 +769,7 @@ void sas_scsi_recover_host(struct Scsi_Host *shost)
>   
>   	/* check if any new eh work was scheduled during the last run */
>   	spin_lock_irq(&ha->lock);
> +	ha->eh_running = false;
>   	if (ha->eh_active == 0) {
>   		shost->host_eh_scheduled = 0;
>   		retry = false;
> diff --git a/include/scsi/libsas.h b/include/scsi/libsas.h
> index 6fe125a71b60..4a8fb324140e 100644
> --- a/include/scsi/libsas.h
> +++ b/include/scsi/libsas.h
> @@ -364,6 +364,7 @@ struct sas_ha_struct {
>   	struct mutex	  drain_mutex;
>   	unsigned long	  state;
>   	spinlock_t	  lock;
> +	bool		  eh_running;
>   	int		  eh_active;
>   	wait_queue_head_t eh_wait_q;
>   	struct list_head  eh_dev_q;
> .
> 