Re: [PATCH] scsi_transport_srp: sanitize scsi_target_block/unblock sequences

[Laurence Oberman <loberman@redhat.com>]

On Tue, 2020-07-28 at 15:48 +0200, Hannes Reinecke wrote:
> The SCSI midlayer does not allow state transitions from SDEV_BLOCK
> to SDEV_BLOCK, so calling scsi_target_block() from
> __rport_fast_io_fail()
> is wrong as the port is already blocked.
> Similarly we don't need to call scsi_target_unblock() afterwards as
> the
> function has already done this.
> 
> Signed-off-by: Hannes Reinecke <hare@suse.de>
> ---
>  drivers/scsi/scsi_transport_srp.c | 12 +++++-------
>  1 file changed, 5 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/scsi/scsi_transport_srp.c
> b/drivers/scsi/scsi_transport_srp.c
> index d4d1104fac99..cba1cf6a1c12 100644
> --- a/drivers/scsi/scsi_transport_srp.c
> +++ b/drivers/scsi/scsi_transport_srp.c
> @@ -395,6 +395,10 @@ static void srp_reconnect_work(struct
> work_struct *work)
>  	}
>  }
>  
> +/*
> + * scsi_target_block() must have been called before this function is
> + * called to guarantee that no .queuecommand() calls are in
> progress.
> + */
>  static void __rport_fail_io_fast(struct srp_rport *rport)
>  {
>  	struct Scsi_Host *shost = rport_to_shost(rport);
> @@ -404,11 +408,7 @@ static void __rport_fail_io_fast(struct
> srp_rport *rport)
>  
>  	if (srp_rport_set_state(rport, SRP_RPORT_FAIL_FAST))
>  		return;
> -	/*
> -	 * Call scsi_target_block() to wait for ongoing shost-
> >queuecommand()
> -	 * calls before invoking i->f->terminate_rport_io().
> -	 */
> -	scsi_target_block(rport->dev.parent);
> +
>  	scsi_target_unblock(rport->dev.parent, SDEV_TRANSPORT_OFFLINE);
>  
>  	/* Involve the LLD if possible to terminate all I/O on the
> rport. */
> @@ -570,8 +570,6 @@ int srp_reconnect_rport(struct srp_rport *rport)
>  		 * failure timers if these had not yet been started.
>  		 */
>  		__rport_fail_io_fast(rport);
> -		scsi_target_unblock(&shost->shost_gendev,
> -				    SDEV_TRANSPORT_OFFLINE);
>  		__srp_start_tl_fail_timers(rport);
>  	} else if (rport->state != SRP_RPORT_BLOCKED) {
>  		scsi_target_unblock(&shost->shost_gendev,

This looks OK to me but I guess its alwasy worked by just ignoring it
being called or IU would have seenm issues.
I etest that stuff pretty heavily.
Reviewed-by: Laurence Oberman <loberman@redhat.com>