* [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 related [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, other threads:[~2020-07-31 2:15 UTC | newest]
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
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).