Re: [PATCH 36/51] scsi: Use scsi_target as argument for eh_target_reset_handler()

[Steffen Maier <maier@linux.ibm.com>]

On 8/17/21 11:14 AM, Hannes Reinecke wrote:
> The target reset function should only depend on the scsi target,
> not the scsi command.
> 
> Signed-off-by: Hannes Reinecke <hare@suse.com> > ---
>   Documentation/scsi/scsi_eh.rst              | 10 ++++
>   Documentation/scsi/scsi_mid_low_api.rst     | 18 +++++++

>   drivers/s390/scsi/zfcp_scsi.c               |  7 ++-

>   drivers/scsi/scsi_error.c                   |  5 +-

>   include/scsi/scsi_host.h                    |  2 +-
>   34 files changed, 226 insertions(+), 192 deletions(-)
> 
> diff --git a/Documentation/scsi/scsi_eh.rst b/Documentation/scsi/scsi_eh.rst
> index cf0649e0c3a9..e09c81a54702 100644
> --- a/Documentation/scsi/scsi_eh.rst
> +++ b/Documentation/scsi/scsi_eh.rst
> @@ -215,6 +215,7 @@ considered to fail always.
> 
>       int (* eh_abort_handler)(struct scsi_cmnd *);
>       int (* eh_device_reset_handler)(struct scsi_cmnd *);
> +    int (* eh_target_reset_handler)(struct scsi_target *);
>       int (* eh_bus_reset_handler)(struct Scsi_Host *, int);
>       int (* eh_host_reset_handler)(struct Scsi_Host *);
> 
> @@ -410,6 +411,15 @@ scmd->allowed.

> 	2. If !list_empty(&eh_work_q), invoke scsi_eh_bus_device_reset().
> 
> 	``scsi_eh_bus_device_reset``
> 
> 	    This action is very similar to scsi_eh_stu() except that,
> 	    instead of issuing STU, hostt->eh_device_reset_handler()
> 	    is used.  Also, as we're not issuing SCSI commands and

>   	    resetting clears all scmds on the sdev, there is no need
>   	    to choose error-completed scmds.
> 
> +	2. If !list_empty(&eh_work_q), invoke scsi_eh_target_reset().

I think we have item 2 twice now, as this is preded by BDR as step 2.
Should target reset be the newly inserted step 3 and subsequent ones be 
incremented by 1?

> +
> +	``scsi_eh_target_reset``
> +
> +	    hostt->eh_target_reset_handler() is invoked for each target
> +	    with failed scmds.  If bus reset succeeds, all failed

bus reset => target reset

(I reckon you copy&pasted from scsi_eh_bus_reset ;-))

> +	    scmds on all ready or offline sdevs on the target are
> +	    EH-finished.
> +
>   	3. If !list_empty(&eh_work_q), invoke scsi_eh_bus_reset()
> 
>   	``scsi_eh_bus_reset``

> 
> 	    hostt->eh_bus_reset_handler() is invoked for each channel
> 	    with failed scmds.  If bus reset succeeds, all failed
> 	    scmds on all ready or offline sdevs on the channel are
> 	    EH-finished.
> 
> 	4. If !list_empty(&eh_work_q), invoke scsi_eh_host_reset()
> 
> 	``scsi_eh_host_reset``
> 
> 	    This is the last resort.  hostt->eh_host_reset_handler()
> 	    is invoked.  If host reset succeeds, all failed scmds on
> 	    all ready or offline sdevs on the host are EH-finished.
> 
> 	5. If !list_empty(&eh_work_q), invoke scsi_eh_offline_sdevs()
> 
> 	``scsi_eh_offline_sdevs``
> 
> 	    Take all sdevs which still have unrecovered scmds offline
> 	    and EH-finish the scmds.


> diff --git a/Documentation/scsi/scsi_mid_low_api.rst b/Documentation/scsi/scsi_mid_low_api.rst
> index 1b1c37445580..0afc1b4f89af 100644
> --- a/Documentation/scsi/scsi_mid_low_api.rst
> +++ b/Documentation/scsi/scsi_mid_low_api.rst

Missing an entry in the Summary section preceding the Details?:

> Summary:
> 
>   - bios_param - fetch head, sector, cylinder info for a disk
>   - eh_timed_out - notify the host that a command timer expired
>   - eh_abort_handler - abort given command
>   - eh_bus_reset_handler - issue SCSI bus reset
>   - eh_device_reset_handler - issue SCSI device reset
>   - eh_host_reset_handler - reset host (host bus adapter)

+  - eh_target_reset_handler - issue SCSI target reset

> @@ -758,6 +758,24 @@ Details::
>   	int eh_bus_reset_handler(struct Scsi_Host * host, int channel)
> 
> 
> +    /**
> +    *      eh_target_reset_handler - issue SCSI target reset
> +    *      @starget: identifies SCSI target to be reset
> +    *
> +    *      Returns SUCCESS if command aborted else FAILED
> +    *
> +    *      Locks: None held
> +    *
> +    *      Calling context: kernel thread
> +    *
> +    *      Notes: Invoked from scsi_eh thread. No other commands will be
> +    *      queued on current host during eh.
> +    *
> +    *      Optionally defined in: LLD
> +    **/
> +	int eh_target_reset_handler(struct scsi_target * starget)
> +
> +

nit-picky: It looks like this file had the reset_handlers sorted alphabetically 
(rather than semantically in order of eh escalation) both in its sections 
"Summary" and "Details". So the eh_target_reset_handler would go after 
eh_host_reset_handler? (also to match order in Summary)

>       /**
>       *      eh_device_reset_handler - issue SCSI device reset
>       *      @scp: identifies SCSI device to be reset

In fact, the Documentation changes look like a different patch to me, namely 
adding missing information about the target reset escalation step. That seems 
independent of this patch which changes the callback signature.


> diff --git a/drivers/s390/scsi/zfcp_scsi.c b/drivers/s390/scsi/zfcp_scsi.c
> index 8bfa8ffd9ff6..6492c3b1b12f 100644
> --- a/drivers/s390/scsi/zfcp_scsi.c
> +++ b/drivers/s390/scsi/zfcp_scsi.c
> @@ -340,9 +340,12 @@ static int zfcp_scsi_eh_device_reset_handler(struct scsi_cmnd *scpnt)
>   	return zfcp_scsi_task_mgmt_function(sdev, FCP_TMF_LUN_RESET);
>   }
> 
> -static int zfcp_scsi_eh_target_reset_handler(struct scsi_cmnd *scpnt)

> +/*
> + * Note: We need to select a LUN as the storage array doesn't
> + * necessarily supports LUN 0 and might refuse the target reset.
> + */

While the storage itself might be another condition, there is the following 
necessary pre-condition about the FCP channel:

v4.18 commit 674595d8519f ("scsi: zfcp: decouple our scsi_eh callbacks from 
scsi_cmnd")
> zfcp_scsi_eh_target_reset_handler() is special: The FCP channel requires a
> valid LUN handle so we try to find ourselves a stand-in scsi_device as
> suggested by Hannes Reinecke. If it cannot find a stand-in scsi device,
> trace a record

(cf. v4.10 commit 6f2ce1c6af37 ("scsi: zfcp: fix rport unblock race with LUN 
recovery") I mentioned in the reply to patch 08/51)

If you think that commit description is insufficient, you could borrow text 
from there for the code comment.

> +static int zfcp_scsi_eh_target_reset_handler(struct scsi_target *starget)
>   {
> -	struct scsi_target *starget = scsi_target(scpnt->device);
>   	struct fc_rport *rport = starget_to_rport(starget);
>   	struct Scsi_Host *shost = rport_to_shost(rport);
>   	struct scsi_device *sdev = NULL, *tmp_sdev;

The code change looks good.


> diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
> index d5e36488c87b..1d8e2f655833 100644
> --- a/drivers/scsi/scsi_error.c
> +++ b/drivers/scsi/scsi_error.c
> @@ -876,14 +876,15 @@ static enum scsi_disposition scsi_try_target_reset(struct scsi_cmnd *scmd)
>   	enum scsi_disposition rtn;
>   	struct Scsi_Host *host = scmd->device->host;
>   	struct scsi_host_template *hostt = host->hostt;
> +	struct scsi_target *starget = scsi_target(scmd->device);
> 
>   	if (!hostt->eh_target_reset_handler)
>   		return FAILED;
> 
> -	rtn = hostt->eh_target_reset_handler(scmd);
> +	rtn = hostt->eh_target_reset_handler(starget);
>   	if (rtn == SUCCESS) {
>   		spin_lock_irqsave(host->host_lock, flags);
> -		__starget_for_each_device(scsi_target(scmd->device), NULL,
> +		__starget_for_each_device(starget, NULL,
>   					  __scsi_report_device_reset);
>   		spin_unlock_irqrestore(host->host_lock, flags);
>   	}

> diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
> index e0a102339317..8fcced0d98bd 100644
> --- a/include/scsi/scsi_host.h
> +++ b/include/scsi/scsi_host.h
> @@ -140,7 +140,7 @@ struct scsi_host_template {
>   	 */
>   	int (* eh_abort_handler)(struct scsi_cmnd *);
>   	int (* eh_device_reset_handler)(struct scsi_cmnd *);
> -	int (* eh_target_reset_handler)(struct scsi_cmnd *);
> +	int (* eh_target_reset_handler)(struct scsi_target *);
>   	int (* eh_bus_reset_handler)(struct Scsi_Host *, int);
>   	int (* eh_host_reset_handler)(struct Scsi_Host *);
> 


-- 
Mit freundlichen Gruessen / Kind regards
Steffen Maier

Linux on IBM Z Development

https://www.ibm.com/privacy/us/en/
IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Matthias Hartmann
Geschaeftsfuehrung: Dirk Wittkopp
Sitz der Gesellschaft: Boeblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294