On 7/12/20 6:13 AM, Bart Van Assche wrote:
On 2020-07-05 23:22, Hannes Reinecke wrote:
That will still have a duplicate call as scsi_target_block() has been called already (cf scsi_target_block() in line 539 right at the start of srp_reconnect_rport()).
(And I don't think we need the WARN_ON_ONCE() here; we are checking for rport->state == SRP_RPORT_BLOCKED just before that line...)
How about the patch below? The approach implemented by that
patch is only to call scsi_target_block() after a successful
transition to the SRP_RPORT_BLOCKED state and to only call
scsi_target_unblock() when leaving the SRP_RPORT_BLOCKED state.
Thanks,
Bart.
diff --git a/drivers/scsi/scsi_transport_srp.c b/drivers/scsi/scsi_transport_srp.c
index d4d1104fac99..0334f86f0879 100644
--- a/drivers/scsi/scsi_transport_srp.c
+++ b/drivers/scsi/scsi_transport_srp.c
@@ -402,13 +402,9 @@ static void __rport_fail_io_fast(struct srp_rport *rport)
lockdep_assert_held(&rport->mutex);
+ WARN_ON_ONCE(rport->state != SRP_RPORT_BLOCKED);
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. */
@@ -541,7 +537,8 @@ int srp_reconnect_rport(struct srp_rport *rport)
res = mutex_lock_interruptible(&rport->mutex);
if (res)
goto out;
- scsi_target_block(&shost->shost_gendev);
+ if (srp_rport_set_state(rport, SRP_RPORT_BLOCKED) == 0)
+ scsi_target_block(&shost->shost_gendev);
res = rport->state != SRP_RPORT_LOST ? i->f->reconnect(rport) : -ENODEV;
pr_debug("%s (state %d): transport.reconnect() returned %d\n",
dev_name(&shost->shost_gendev), rport->state, res);
@@ -569,9 +566,9 @@ int srp_reconnect_rport(struct srp_rport *rport)
* and dev_loss off. Mark the port as failed and start the TL
* failure timers if these had not yet been started.
*/
+ WARN_ON_ONCE(srp_rport_set_state(rport, SRP_RPORT_BLOCKED));
+ scsi_target_block(rport->dev.parent);
__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,
That doesn't look correct.
We just set the portstate to 'blocked' in the first hunk.
So the only way for this bit to make any sense would be if the portstate
would _not_ blocked, _and_ we have a valid state transition to 'blocked'.
But this cannot happen, as the state can't change in between those two
calls, and the first state change didn't succeed. So this state change
won't succeed, either, and the WARN_ON will always trigger here.
Plus this whole hunk is reached from an if condition:
} else if (rport->state == SRP_RPORT_RUNNING) {
which (after the first hunk) is never viable, as the transition RUNNINNG
-> BLOCKED is allowed. Hence the first hunk will always transition to
BLOCKED, and this whole block can never be reached.
I think this should be sufficient:
diff --git a/drivers/scsi/scsi_transport_srp.c
b/drivers/scsi/scsi_transport_srp.c
index d4d1104fac99..180b323f46b8 100644
--- a/drivers/scsi/scsi_transport_srp.c
+++ b/drivers/scsi/scsi_transport_srp.c
@@ -404,11 +404,6 @@ 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 +565,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,
Cheers,
Hannes
--
Dr. Hannes Reinecke Teamlead Storage & Networking
hare@xxxxxxx +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer
