Linux-SCSI Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] scsi_transport_srp: sanitize scsi_target_block/unblock sequences
@ 2020-07-28 13:48 Hannes Reinecke
  2020-07-28 18:43 ` Laurence Oberman
                   ` (2 more replies)
  0 siblings, 3 replies; 6+ messages in thread
From: Hannes Reinecke @ 2020-07-28 13:48 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: Martin K. Petersen, James Bottomley, Bart van Assche, linux-scsi,
	Hannes Reinecke

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,
-- 
2.16.4


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] scsi_transport_srp: sanitize scsi_target_block/unblock sequences
  2020-07-28 13:48 [PATCH] scsi_transport_srp: sanitize scsi_target_block/unblock sequences Hannes Reinecke
@ 2020-07-28 18:43 ` Laurence Oberman
  2020-07-28 19:50   ` Laurence Oberman
  2020-07-30  3:27 ` Bart Van Assche
  2020-07-31  2:14 ` Martin K. Petersen
  2 siblings, 1 reply; 6+ messages in thread
From: Laurence Oberman @ 2020-07-28 18:43 UTC (permalink / raw)
  To: Hannes Reinecke, Christoph Hellwig
  Cc: Martin K. Petersen, James Bottomley, Bart van Assche, linux-scsi

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>


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] scsi_transport_srp: sanitize scsi_target_block/unblock sequences
  2020-07-28 18:43 ` Laurence Oberman
@ 2020-07-28 19:50   ` Laurence Oberman
  2020-07-29  6:09     ` Hannes Reinecke
  0 siblings, 1 reply; 6+ messages in thread
From: Laurence Oberman @ 2020-07-28 19:50 UTC (permalink / raw)
  To: Hannes Reinecke, Christoph Hellwig
  Cc: Martin K. Petersen, James Bottomley, Bart van Assche, linux-scsi

On Tue, 2020-07-28 at 14:43 -0400, Laurence Oberman wrote:
> 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>
> 

Ouch, my last message saw my bad touch typing creep in. Let me try
again.

This looks OK to me but I guess its always worked by just ignoring it
being called or I would have seen issues already.
I test that stuff pretty heavily and regularly.

Reviewed-by: Laurence Oberman <loberman@redhat.com>



^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] scsi_transport_srp: sanitize scsi_target_block/unblock sequences
  2020-07-28 19:50   ` Laurence Oberman
@ 2020-07-29  6:09     ` Hannes Reinecke
  0 siblings, 0 replies; 6+ messages in thread
From: Hannes Reinecke @ 2020-07-29  6:09 UTC (permalink / raw)
  To: Laurence Oberman, Christoph Hellwig
  Cc: Martin K. Petersen, James Bottomley, Bart van Assche, linux-scsi

On 7/28/20 9:50 PM, Laurence Oberman wrote:
> On Tue, 2020-07-28 at 14:43 -0400, Laurence Oberman wrote:
>> 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>
>>
> 
> Ouch, my last message saw my bad touch typing creep in. Let me try
> again.
> 
> This looks OK to me but I guess its always worked by just ignoring it
> being called or I would have seen issues already.
> I test that stuff pretty heavily and regularly.
> 
> Reviewed-by: Laurence Oberman <loberman@redhat.com>
> 
> 
Well, we've had a customer running into it.
And they do _heavy_ tests, too.

I haven't said it's a regular occurrence :-)

Cheers,

Hannes
-- 
Dr. Hannes Reinecke            Teamlead Storage & Networking
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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] scsi_transport_srp: sanitize scsi_target_block/unblock sequences
  2020-07-28 13:48 [PATCH] scsi_transport_srp: sanitize scsi_target_block/unblock sequences Hannes Reinecke
  2020-07-28 18:43 ` Laurence Oberman
@ 2020-07-30  3:27 ` Bart Van Assche
  2020-07-31  2:14 ` Martin K. Petersen
  2 siblings, 0 replies; 6+ messages in thread
From: Bart Van Assche @ 2020-07-30  3:27 UTC (permalink / raw)
  To: Hannes Reinecke, Christoph Hellwig
  Cc: Martin K. Petersen, James Bottomley, linux-scsi

On 2020-07-28 06:48, 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.
 Reviewed-by: Bart Van Assche <bvanassche@acm.org>

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] scsi_transport_srp: sanitize scsi_target_block/unblock sequences
  2020-07-28 13:48 [PATCH] scsi_transport_srp: sanitize scsi_target_block/unblock sequences Hannes Reinecke
  2020-07-28 18:43 ` Laurence Oberman
  2020-07-30  3:27 ` Bart Van Assche
@ 2020-07-31  2:14 ` Martin K. Petersen
  2 siblings, 0 replies; 6+ messages in thread
From: Martin K. Petersen @ 2020-07-31  2:14 UTC (permalink / raw)
  To: Hannes Reinecke, Christoph Hellwig
  Cc: Martin K . Petersen, James Bottomley, linux-scsi, Bart van Assche

On Tue, 28 Jul 2020 15:48:33 +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.

Applied to 5.9/scsi-queue, thanks!

[1/1] scsi: scsi_transport_srp: Sanitize scsi_target_block/unblock sequences
      https://git.kernel.org/mkp/scsi/c/bf1a28f92a8b

-- 
Martin K. Petersen	Oracle Linux Engineering

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, back to index

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-28 13:48 [PATCH] scsi_transport_srp: sanitize scsi_target_block/unblock sequences Hannes Reinecke
2020-07-28 18:43 ` Laurence Oberman
2020-07-28 19:50   ` Laurence Oberman
2020-07-29  6:09     ` Hannes Reinecke
2020-07-30  3:27 ` Bart Van Assche
2020-07-31  2:14 ` Martin K. Petersen

Linux-SCSI Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-scsi/0 linux-scsi/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-scsi linux-scsi/ https://lore.kernel.org/linux-scsi \
		linux-scsi@vger.kernel.org
	public-inbox-index linux-scsi

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-scsi


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git