Message ID | 20200728134833.42547-1-hare@suse.de (mailing list archive) |
---|---|
State | Mainlined |
Commit | bf1a28f92a8b00ee8ce48cc11338980e31ddb7b3 |
Headers | show |
Series | scsi_transport_srp: sanitize scsi_target_block/unblock sequences | expand |
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>
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>
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
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>
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
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,
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(-)