[PATCH v2] scsi_error: do not queue pointless abort workqueue functions

[PATCH v2] scsi_error: do not queue pointless abort workqueue functions

[Niklas Cassel]

From: Hannes Reinecke <hare@suse.de>

If a host template doesn't implement the .eh_abort_handler()
there is no point in queueing the abort workqueue function;
all it does is invoking SCSI EH anyway.
So return 'FAILED' from scsi_abort_command() if the .eh_abort_handler()
is not implemented and save us from having to wait for the
abort workqueue function to complete.

Cc: Niklas Cassel <niklas.cassel@wdc.com>
Cc: Damien Le Moal <damien.lemoal@opensource.wdc.com>
Cc: John Garry <john.g.garry@oracle.com>
Signed-off-by: Hannes Reinecke <hare@suse.de>
[niklas: moved the check to the top of scsi_abort_command()]
Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
---
Changes since v1:
-moved the check to the top of scsi_abort_command(), as there is no need to
 perform the SCSI_EH_ABORT_SCHEDULED check if there is no eh_abort_handler.

I know that John gave a review comment on V1 that it is possible to not
allocate the shost->tmf_work_q in case there is no eh_abort_handler,
however, that is more of a micro optimization. This patch significantly
reduces the latency before SCSI EH is invoked for libata.

It would be nice if we could get this patched merged for 6.2, for which
the merge window opens soon.

 drivers/scsi/scsi_error.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index a7960ad2d386..2aa2c2aee6e7 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -231,6 +231,11 @@ scsi_abort_command(struct scsi_cmnd *scmd)
 	struct Scsi_Host *shost = sdev->host;
 	unsigned long flags;
 
+	if (!shost->hostt->eh_abort_handler) {
+		/* No abort handler, fail command directly */
+		return FAILED;
+	}
+
 	if (scmd->eh_eflags & SCSI_EH_ABORT_SCHEDULED) {
 		/*
 		 * Retry after abort failed, escalate to next level.
-- 
2.38.1

Re: [PATCH v2] scsi_error: do not queue pointless abort workqueue functions

[John Garry]

On 06/12/2022 13:13, Niklas Cassel wrote:
> From: Hannes Reinecke <hare@suse.de>
> 
> If a host template doesn't implement the .eh_abort_handler()
> there is no point in queueing the abort workqueue function;
> all it does is invoking SCSI EH anyway.
> So return 'FAILED' from scsi_abort_command() if the .eh_abort_handler()
> is not implemented and save us from having to wait for the
> abort workqueue function to complete.
> 
> Cc: Niklas Cassel <niklas.cassel@wdc.com>
> Cc: Damien Le Moal <damien.lemoal@opensource.wdc.com>
> Cc: John Garry <john.g.garry@oracle.com>
> Signed-off-by: Hannes Reinecke <hare@suse.de>
> [niklas: moved the check to the top of scsi_abort_command()]
> Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>

FWIW,

Reviewed-by: John Garry <john.g.garry@oracle.com>

> ---
> Changes since v1:
> -moved the check to the top of scsi_abort_command(), as there is no need to
>   perform the SCSI_EH_ABORT_SCHEDULED check if there is no eh_abort_handler.
> 
> I know that John gave a review comment on V1 that it is possible to not
> allocate the shost->tmf_work_q in case there is no eh_abort_handler,
> however, that is more of a micro optimization. 

I'd say that would be a separate change.

> This patch significantly
> reduces the latency before SCSI EH is invoked for libata.
> 
> It would be nice if we could get this patched merged for 6.2, for which
> the merge window opens soon.
> 
>   drivers/scsi/scsi_error.c | 5 +++++
>   1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
> index a7960ad2d386..2aa2c2aee6e7 100644
> --- a/drivers/scsi/scsi_error.c
> +++ b/drivers/scsi/scsi_error.c
> @@ -231,6 +231,11 @@ scsi_abort_command(struct scsi_cmnd *scmd)
>   	struct Scsi_Host *shost = sdev->host;
>   	unsigned long flags;
>   
> +	if (!shost->hostt->eh_abort_handler) {
> +		/* No abort handler, fail command directly */
> +		return FAILED;
> +	}
> +
>   	if (scmd->eh_eflags & SCSI_EH_ABORT_SCHEDULED) {
>   		/*
>   		 * Retry after abort failed, escalate to next level.

Re: [PATCH v2] scsi_error: do not queue pointless abort workqueue functions

[Martin K. Petersen]

Niklas,

> If a host template doesn't implement the .eh_abort_handler() there is
> no point in queueing the abort workqueue function; all it does is
> invoking SCSI EH anyway.  So return 'FAILED' from scsi_abort_command()
> if the .eh_abort_handler() is not implemented and save us from having
> to wait for the abort workqueue function to complete.

Applied to 6.2/scsi-staging, thanks!

-- 
Martin K. Petersen	Oracle Linux Engineering