fc transport: pre-emptively terminate i/o upon dev_loss_tmo

PATCH Ralph Wuerthner 3 participants 6 messages Apr 9, 2009 – Apr 22, 2009 SCSI

Share & tools

Ralph Wuerthner

author

2009-04-09 11:04 UTC #0 lore raw

Quoted 89 lines · Mike Christie

Quoted 26 lines · James Smart
Pre-emptively terminate i/o on the rport if dev_loss_tmo has fired. The desire is to terminate everything, so that the i/o is cleaned up prior to the sdev's being unblocked, thus any outstanding timeouts/aborts are avoided.
Also, we do this early enough such that the rport's port_id field is still valid. FCOE libFC code needs this info to find the i/o's to terminate.
-- james s
Signed-off-by James Smart <[email protected]>
---
 scsi_transport_fc.c |   11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)
diff -upNr a/drivers/scsi/scsi_transport_fc.c b/drivers/scsi/scsi_transport_fc.c
drivers/scsi/scsi_transport_fc.c +0 -0
@@ -3012,6 +3012,16 @@ fc_timeout_deleted_rport(struct work_str
 	rport->port_state = FC_PORTSTATE_NOTPRESENT;
 	rport->flags &= ~FC_RPORT_FAST_FAIL_TIMEDOUT;
 
drivers/scsi/scsi_transport_fc.c fc_timeout_deleted_rport(struct work_str · line 3024
Quoted 10 lines
drivers/scsi/scsi_transport_fc.c +10 -0
+	/*
+	 * Pre-emptively kill I/O rather than waiting for the work queue
+	 * item to teardown the starget. (FCOE libFC folks prefer this
+	 * and to have the rport_port_id still set when it's done).
+	 */
+	spin_unlock_irqrestore(shost->host_lock, flags);
+	fc_terminate_rport_io(rport);
+
+	BUG_ON(rport->port_state != FC_PORTSTATE_NOTPRESENT);
+
I think we could this this bug_on or we might hit a problem below where this thread is changing the port_id but some other thread is calling fc_remote_port_add and changging the port id.
I think the problem is that fc_remote_port_add only calls fc_flush_work which flushes the fc_host work_q:
struct fc_rport *
() (struct Scsi_Host *shost, int channel,
struct fc_rport_identifiers *ids)
{
         struct fc_internal *fci = to_fc_internal(shost->transportt);
         struct fc_host_attrs *fc_host = shost_to_fc_host(shost);
         struct fc_rport *rport;
         unsigned long flags;
         int match = 0;

         /* ensure any stgt delete functions are done */
         fc_flush_work(shost);
But fc_timeout_deleted_rport dev_loss_work is running on the devloss_work_q.
So if fc_timeout_deleted_rport grabs the host lock first, it will set the port state to FC_PORTSTATE_NOTPRESENT, then drop the lock.
Once fc_timeout_delete_rport drops the lock, fc_remote_port_add could have already passed the fc_flush_work() call because there was nothing on it. fc_remote_port_add would then drop down to the list search on the bindings array and see the remote port. It would then start setting the port state, id and port_name and node_name, but at the same time, because the host lock no longer guards it, fc_timedout_deleted_rport could be fiddling with the same fields and we could end up a mix of values or it could be running over the BUG_ON.
Is that right?
If so do we just want to flush the devloss_work_queue in fc_remote_port_add?
Quoted 9 lines
drivers/scsi/scsi_transport_fc.c +0 -1
 	/* remove the identifiers that aren't used in the consisting binding */
 	switch (fc_host->tgtid_bind_type) {
 	case FC_TGTID_BIND_BY_WWPN:
@@ -3035,7 +3045,6 @@ fc_timeout_deleted_rport(struct work_str
 	 * went away and didn't come back - we'll remove
 	 * all attached scsi devices.
 	 */
-	spin_unlock_irqrestore(shost->host_lock, flags);
 

Quoted 3 lines

drivers/scsi/scsi_transport_fc.c fc_timeout_deleted_rport(struct work_str · line 3050
Quoted 3 lines
drivers/scsi/scsi_transport_fc.c +0 -0
 	scsi_target_unblock(&rport->dev);
 	fc_queue_work(shost, &rport->stgt_delete_work);

One of our tester hit the problem Michael describes above: right after releasing the shost->host_lock lock in fc_timeout_deleted_rport() the remote port was rediscovered by the LLDD (in our case zfcp), fc_remote_port_add() was called and BUG_ON() triggered.

But flushing the devloss_work_queue in fc_remote_port_add() won't help either because it still would be possible that right after the flush_workqueue() call a timer could trigger and fc_timeout_deleted_rport() can be called on a different cpu before fc_remote_port_add() grabs the lock and we end up again with inconsistent data.

To me the purpose of the BUG_ON() is not clear: re-adding a remote port should be possible at any time even after the devloss timeout triggered. And as Michael already mentioned, modifying the identifiers without holding the lock can lead to inconsistent remote port data. What about the following solution:

---
 drivers/scsi/scsi_transport_fc.c |   36 +++++++++++++++++++-----------------
 1 file changed, 19 insertions(+), 17 deletions(-)

drivers/scsi/scsi_transport_fc.c +19 ~~-17~~

Index: linux-2.5/drivers/scsi/scsi_transport_fc.c
===================================================================
@@ -3027,25 +3027,27 @@ fc_timeout_deleted_rport(struct work_str
 	spin_unlock_irqrestore(shost->host_lock, flags);
 	fc_terminate_rport_io(rport);
 
-	BUG_ON(rport->port_state != FC_PORTSTATE_NOTPRESENT);
-
 	/* remove the identifiers that aren't used in the consisting binding */
-	switch (fc_host->tgtid_bind_type) {
-	case FC_TGTID_BIND_BY_WWPN:
-		rport->node_name = -1;
-		rport->port_id = -1;
-		break;
-	case FC_TGTID_BIND_BY_WWNN:
-		rport->port_name = -1;
-		rport->port_id = -1;
-		break;
-	case FC_TGTID_BIND_BY_ID:
-		rport->node_name = -1;
-		rport->port_name = -1;
-		break;
-	case FC_TGTID_BIND_NONE:	/* to keep compiler happy */
-		break;
+	spin_lock_irqsave(shost->host_lock, flags);
+	if (rport->port_state == FC_PORTSTATE_NOTPRESENT) {
+		switch (fc_host->tgtid_bind_type) {
+		case FC_TGTID_BIND_BY_WWPN:
+			rport->node_name = -1;
+			rport->port_id = -1;
+			break;
+		case FC_TGTID_BIND_BY_WWNN:
+			rport->port_name = -1;
+			rport->port_id = -1;
+			break;
+		case FC_TGTID_BIND_BY_ID:
+			rport->node_name = -1;
+			rport->port_name = -1;
+			break;
+		case FC_TGTID_BIND_NONE:	/* to keep compiler happy */
+			break;
+		}
 	}
+	spin_unlock_irqrestore(shost->host_lock, flags);
 
 	/*
 	 * As this only occurs if the remote port (scsi target)

--
Ralph Wuerthner

Mike Christie

2009-04-21 21:43 UTC #1 lore raw

Quoted 87 lines · Ralph Wuerthner

Quoted 77 lines · Mike Christie
Quoted 26 lines · James Smart
Pre-emptively terminate i/o on the rport if dev_loss_tmo has fired. The desire is to terminate everything, so that the i/o is cleaned up prior to the sdev's being unblocked, thus any outstanding timeouts/aborts are avoided.
Also, we do this early enough such that the rport's port_id field is still valid. FCOE libFC code needs this info to find the i/o's to terminate.
-- james s
Signed-off-by James Smart <[email protected]>
---
 scsi_transport_fc.c |   11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)
diff -upNr a/drivers/scsi/scsi_transport_fc.c b/drivers/scsi/scsi_transport_fc.c
drivers/scsi/scsi_transport_fc.c +0 -0
@@ -3012,6 +3012,16 @@ fc_timeout_deleted_rport(struct work_str
 	rport->port_state = FC_PORTSTATE_NOTPRESENT;
 	rport->flags &= ~FC_RPORT_FAST_FAIL_TIMEDOUT;
 
drivers/scsi/scsi_transport_fc.c fc_timeout_deleted_rport(struct work_str · line 3024
Quoted 10 lines
drivers/scsi/scsi_transport_fc.c +10 -0
+	/*
+	 * Pre-emptively kill I/O rather than waiting for the work queue
+	 * item to teardown the starget. (FCOE libFC folks prefer this
+	 * and to have the rport_port_id still set when it's done).
+	 */
+	spin_unlock_irqrestore(shost->host_lock, flags);
+	fc_terminate_rport_io(rport);
+
+	BUG_ON(rport->port_state != FC_PORTSTATE_NOTPRESENT);
+
I think we could this this bug_on or we might hit a problem below where this thread is changing the port_id but some other thread is calling fc_remote_port_add and changging the port id.
I think the problem is that fc_remote_port_add only calls fc_flush_work which flushes the fc_host work_q:
struct fc_rport *
() (struct Scsi_Host *shost, int channel,
struct fc_rport_identifiers *ids)
{
         struct fc_internal *fci = to_fc_internal(shost->transportt);
         struct fc_host_attrs *fc_host = shost_to_fc_host(shost);
         struct fc_rport *rport;
         unsigned long flags;
         int match = 0;

         /* ensure any stgt delete functions are done */
         fc_flush_work(shost);
But fc_timeout_deleted_rport dev_loss_work is running on the devloss_work_q.
So if fc_timeout_deleted_rport grabs the host lock first, it will set the port state to FC_PORTSTATE_NOTPRESENT, then drop the lock.
Once fc_timeout_delete_rport drops the lock, fc_remote_port_add could have already passed the fc_flush_work() call because there was nothing on it. fc_remote_port_add would then drop down to the list search on the bindings array and see the remote port. It would then start setting the port state, id and port_name and node_name, but at the same time, because the host lock no longer guards it, fc_timedout_deleted_rport could be fiddling with the same fields and we could end up a mix of values or it could be running over the BUG_ON.
Is that right?
If so do we just want to flush the devloss_work_queue in fc_remote_port_add?
Quoted 9 lines
drivers/scsi/scsi_transport_fc.c +0 -1
 	/* remove the identifiers that aren't used in the consisting binding */
 	switch (fc_host->tgtid_bind_type) {
 	case FC_TGTID_BIND_BY_WWPN:
@@ -3035,7 +3045,6 @@ fc_timeout_deleted_rport(struct work_str
 	 * went away and didn't come back - we'll remove
 	 * all attached scsi devices.
 	 */
-	spin_unlock_irqrestore(shost->host_lock, flags);
 
Quoted 3 lines
drivers/scsi/scsi_transport_fc.c fc_timeout_deleted_rport(struct work_str · line 3050
Quoted 3 lines
drivers/scsi/scsi_transport_fc.c +0 -0
 	scsi_target_unblock(&rport->dev);
 	fc_queue_work(shost, &rport->stgt_delete_work);
One of our tester hit the problem Michael describes above: right after releasing the shost->host_lock lock in fc_timeout_deleted_rport() the remote port was rediscovered by the LLDD (in our case zfcp), fc_remote_port_add() was called and BUG_ON() triggered.

Quoted 3 lines

But flushing the devloss_work_queue in fc_remote_port_add() won't help either because it still would be possible that right after the flush_workqueue() call a timer could trigger and fc_timeout_deleted_rport()

What timer could that be? Only the dev_loss_work delayed work calls fc_timeout_deleted_rport.

Are you thinking about fc_rport_final_delete?

If you do the

                if (!cancel_delayed_work(&rport->dev_loss_work))
                        fc_flush_devloss(shost);

sequence we should be ok I think, because after that point it should have either been canceled or the work has completed.

If not then I think we might have problems in other places.

Mike Christie

2009-04-21 21:53 UTC #2 lore raw

Quoted 98 lines · Mike Christie

Quoted 90 lines · Ralph Wuerthner
Quoted 80 lines · Mike Christie
Quoted 28 lines · James Smart
Pre-emptively terminate i/o on the rport if dev_loss_tmo has fired. The desire is to terminate everything, so that the i/o is cleaned up prior to the sdev's being unblocked, thus any outstanding timeouts/aborts are avoided.
Also, we do this early enough such that the rport's port_id field is still valid. FCOE libFC code needs this info to find the i/o's to terminate.
-- james s
Signed-off-by James Smart <[email protected]>
---
 scsi_transport_fc.c |   11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)
diff -upNr a/drivers/scsi/scsi_transport_fc.c b/drivers/scsi/scsi_transport_fc.c
--- a/drivers/scsi/scsi_transport_fc.c 2008-10-18
10:32:52.000000000 -0400
+++ b/drivers/scsi/scsi_transport_fc.c 2008-12-05
12:13:54.000000000 -0500
(quoted diff) +0 -0
@@ -3012,6 +3012,16 @@ fc_timeout_deleted_rport(struct work_str
     rport->port_state = FC_PORTSTATE_NOTPRESENT;
     rport->flags &= ~FC_RPORT_FAST_FAIL_TIMEDOUT;
 
Review fc_timeout_deleted_rport(struct work_str · line 3024
Quoted 10 lines
(quoted diff) +10 -0
+    /*
+     * Pre-emptively kill I/O rather than waiting for the work queue
+     * item to teardown the starget. (FCOE libFC folks prefer this
+     * and to have the rport_port_id still set when it's done).
+     */
+    spin_unlock_irqrestore(shost->host_lock, flags);
+    fc_terminate_rport_io(rport);
+
+    BUG_ON(rport->port_state != FC_PORTSTATE_NOTPRESENT);
+
I think we could this this bug_on or we might hit a problem below where this thread is changing the port_id but some other thread is calling fc_remote_port_add and changging the port id.
I think the problem is that fc_remote_port_add only calls fc_flush_work which flushes the fc_host work_q:
struct fc_rport *
() (struct Scsi_Host *shost, int channel,
struct fc_rport_identifiers *ids)
{
         struct fc_internal *fci = to_fc_internal(shost->transportt);
         struct fc_host_attrs *fc_host = shost_to_fc_host(shost);
         struct fc_rport *rport;
         unsigned long flags;
         int match = 0;

         /* ensure any stgt delete functions are done */
         fc_flush_work(shost);
But fc_timeout_deleted_rport dev_loss_work is running on the devloss_work_q.
So if fc_timeout_deleted_rport grabs the host lock first, it will set the port state to FC_PORTSTATE_NOTPRESENT, then drop the lock.
Once fc_timeout_delete_rport drops the lock, fc_remote_port_add could have already passed the fc_flush_work() call because there was nothing on it. fc_remote_port_add would then drop down to the list search on the bindings array and see the remote port. It would then start setting the port state, id and port_name and node_name, but at the same time, because the host lock no longer guards it, fc_timedout_deleted_rport could be fiddling with the same fields and we could end up a mix of values or it could be running over the BUG_ON.
Is that right?
If so do we just want to flush the devloss_work_queue in fc_remote_port_add?
Quoted 10 lines
(quoted diff) +0 -0
     /* remove the identifiers that aren't used in the consisting 
binding */
     switch (fc_host->tgtid_bind_type) {
     case FC_TGTID_BIND_BY_WWPN:
(quoted diff) +0 -1
@@ -3035,7 +3045,6 @@ fc_timeout_deleted_rport(struct work_str
      * went away and didn't come back - we'll remove
      * all attached scsi devices.
      */
-    spin_unlock_irqrestore(shost->host_lock, flags);
 
Quoted 3 lines
Review fc_timeout_deleted_rport(struct work_str · line 3050
Quoted 3 lines
(quoted diff) +0 -0
     scsi_target_unblock(&rport->dev);
     fc_queue_work(shost, &rport->stgt_delete_work);
One of our tester hit the problem Michael describes above: right after releasing the shost->host_lock lock in fc_timeout_deleted_rport() the remote port was rediscovered by the LLDD (in our case zfcp), fc_remote_port_add() was called and BUG_ON() triggered.
Quoted 3 lines
But flushing the devloss_work_queue in fc_remote_port_add() won't help either because it still would be possible that right after the flush_workqueue() call a timer could trigger and fc_timeout_deleted_rport()
What timer could that be? Only the dev_loss_work delayed work calls fc_timeout_deleted_rport.

Quoted 8 lines

Are you thinking about fc_rport_final_delete?
If you do the
                if (!cancel_delayed_work(&rport->dev_loss_work))
                        fc_flush_devloss(shost);
sequence we should be ok I think, because after that point it should have either been canceled or the work has completed.

I did this in the attached patch. It also will prevent drivers from having to worry about about if the terminate rport IO or dev loss callback is running at the same time fc_remote_port_add is run.

drivers/scsi/scsi_transport_fc.c +4 -0

index a152f89..a45c5ed 100644
@@ -2564,6 +2564,10 @@ fc_remote_port_add(struct Scsi_Host *shost, int channel,
 	/* ensure any stgt delete functions are done */
 	fc_flush_work(shost);
 
+	/* ensure dev loss or fast io fail are done */
+	if (!cancel_delayed_work(&rport->dev_loss_work))
+		fc_flush_devloss(shost);
+
 	/*
 	 * Search the list of "active" rports, for an rport that has been
 	 * deleted, but we've held off the real delete while the target

Mike Christie

2009-04-21 22:12 UTC #3 lore raw

Quoted 101 lines · Mike Christie

Quoted 100 lines · Mike Christie
Quoted 90 lines · Ralph Wuerthner
Quoted 80 lines · Mike Christie
Quoted 28 lines · James Smart
Pre-emptively terminate i/o on the rport if dev_loss_tmo has fired. The desire is to terminate everything, so that the i/o is cleaned up prior to the sdev's being unblocked, thus any outstanding timeouts/aborts are avoided.
Also, we do this early enough such that the rport's port_id field is still valid. FCOE libFC code needs this info to find the i/o's to terminate.
-- james s
Signed-off-by James Smart <[email protected]>
---
 scsi_transport_fc.c |   11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)
diff -upNr a/drivers/scsi/scsi_transport_fc.c b/drivers/scsi/scsi_transport_fc.c
--- a/drivers/scsi/scsi_transport_fc.c 2008-10-18
10:32:52.000000000 -0400
+++ b/drivers/scsi/scsi_transport_fc.c 2008-12-05
12:13:54.000000000 -0500
(quoted diff) +0 -0
@@ -3012,6 +3012,16 @@ fc_timeout_deleted_rport(struct work_str
     rport->port_state = FC_PORTSTATE_NOTPRESENT;
     rport->flags &= ~FC_RPORT_FAST_FAIL_TIMEDOUT;
 
Review fc_timeout_deleted_rport(struct work_str · line 3024
Quoted 10 lines
(quoted diff) +10 -0
+    /*
+     * Pre-emptively kill I/O rather than waiting for the work queue
+     * item to teardown the starget. (FCOE libFC folks prefer this
+     * and to have the rport_port_id still set when it's done).
+     */
+    spin_unlock_irqrestore(shost->host_lock, flags);
+    fc_terminate_rport_io(rport);
+
+    BUG_ON(rport->port_state != FC_PORTSTATE_NOTPRESENT);
+
I think we could this this bug_on or we might hit a problem below where this thread is changing the port_id but some other thread is calling fc_remote_port_add and changging the port id.
I think the problem is that fc_remote_port_add only calls fc_flush_work which flushes the fc_host work_q:
struct fc_rport *
() (struct Scsi_Host *shost, int channel,
struct fc_rport_identifiers *ids)
{
         struct fc_internal *fci = to_fc_internal(shost->transportt);
         struct fc_host_attrs *fc_host = shost_to_fc_host(shost);
         struct fc_rport *rport;
         unsigned long flags;
         int match = 0;

         /* ensure any stgt delete functions are done */
         fc_flush_work(shost);
But fc_timeout_deleted_rport dev_loss_work is running on the devloss_work_q.
So if fc_timeout_deleted_rport grabs the host lock first, it will set the port state to FC_PORTSTATE_NOTPRESENT, then drop the lock.
Once fc_timeout_delete_rport drops the lock, fc_remote_port_add could have already passed the fc_flush_work() call because there was nothing on it. fc_remote_port_add would then drop down to the list search on the bindings array and see the remote port. It would then start setting the port state, id and port_name and node_name, but at the same time, because the host lock no longer guards it, fc_timedout_deleted_rport could be fiddling with the same fields and we could end up a mix of values or it could be running over the BUG_ON.
Is that right?
If so do we just want to flush the devloss_work_queue in fc_remote_port_add?
Quoted 10 lines
(quoted diff) +0 -0
     /* remove the identifiers that aren't used in the consisting 
binding */
     switch (fc_host->tgtid_bind_type) {
     case FC_TGTID_BIND_BY_WWPN:
(quoted diff) +0 -1
@@ -3035,7 +3045,6 @@ fc_timeout_deleted_rport(struct work_str
      * went away and didn't come back - we'll remove
      * all attached scsi devices.
      */
-    spin_unlock_irqrestore(shost->host_lock, flags);
 
Quoted 3 lines
Review fc_timeout_deleted_rport(struct work_str · line 3050
Quoted 3 lines
(quoted diff) +0 -0
     scsi_target_unblock(&rport->dev);
     fc_queue_work(shost, &rport->stgt_delete_work);
One of our tester hit the problem Michael describes above: right after releasing the shost->host_lock lock in fc_timeout_deleted_rport() the remote port was rediscovered by the LLDD (in our case zfcp), fc_remote_port_add() was called and BUG_ON() triggered.
Quoted 3 lines
But flushing the devloss_work_queue in fc_remote_port_add() won't help either because it still would be possible that right after the flush_workqueue() call a timer could trigger and fc_timeout_deleted_rport()
What timer could that be? Only the dev_loss_work delayed work calls fc_timeout_deleted_rport.
Are you thinking about fc_rport_final_delete?

Quoted 9 lines

Quoted 6 lines
If you do the
                if (!cancel_delayed_work(&rport->dev_loss_work))
                        fc_flush_devloss(shost);
sequence we should be ok I think, because after that point it should have either been canceled or the work has completed.
I did this in the attached patch. It also will prevent drivers from having to worry about about if the terminate rport IO or dev loss callback is running at the same time fc_remote_port_add is run.

I forgot to cancel the fail io work too. I guess we can also remove the same calls later in that function.

drivers/scsi/scsi_transport_fc.c +8 ~~-10~~

index a152f89..e7bbc65 100644
@@ -2564,6 +2564,14 @@ fc_remote_port_add(struct Scsi_Host *shost, int channel,
 	/* ensure any stgt delete functions are done */
 	fc_flush_work(shost);
 
+	/* ensure fail fast is done */
+	if (!cancel_delayed_work(&rport->fail_io_work))
+		fc_flush_devloss(shost);
+
+	/* ensure dev loss is done */
+	if (!cancel_delayed_work(&rport->dev_loss_work))
+		fc_flush_devloss(shost);
+
 	/*
 	 * Search the list of "active" rports, for an rport that has been
 	 * deleted, but we've held off the real delete while the target

@@ -2630,16 +2638,6 @@ fc_remote_port_add(struct Scsi_Host *shost, int channel,
 				    (!(ids->roles & FC_PORT_ROLE_FCP_TARGET)))
 					return rport;
 
-				/*
-				 * Stop the fail io and dev_loss timers.
-				 * If they flush, the port_state will
-				 * be checked and will NOOP the function.
-				 */
-				if (!cancel_delayed_work(&rport->fail_io_work))
-					fc_flush_devloss(shost);
-				if (!cancel_delayed_work(&rport->dev_loss_work))
-					fc_flush_devloss(shost);
-
 				spin_lock_irqsave(shost->host_lock, flags);
 
 				rport->flags &= ~(FC_RPORT_FAST_FAIL_TIMEDOUT |

Ralph Wuerthner

author

2009-04-22 12:13 UTC #4 lore raw

Quoted 176 lines · On Tue, 21 Apr 2009 17:12:18 -0500 Mike Christie <[email protected]> wrote:

Quoted 127 lines · Mike Christie
Quoted 126 lines · Mike Christie
Quoted 110 lines · Ralph Wuerthner
Quoted 99 lines · Mike Christie
Quoted 35 lines · James Smart
Pre-emptively terminate i/o on the rport if dev_loss_tmo has fired. The desire is to terminate everything, so that the i/o is cleaned up prior to the sdev's being unblocked, thus any outstanding timeouts/aborts
are avoided.
Also, we do this early enough such that the rport's port_id field is still valid. FCOE libFC code needs this info to find the i/o's to terminate.
-- james s
Signed-off-by James Smart <[email protected]>
---
 scsi_transport_fc.c |   11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)
diff -upNr a/drivers/scsi/scsi_transport_fc.c
b/drivers/scsi/scsi_transport_fc.c
--- a/drivers/scsi/scsi_transport_fc.c 2008-10-18
10:32:52.000000000 -0400
+++ b/drivers/scsi/scsi_transport_fc.c 2008-12-05
12:13:54.000000000 -0500
(quoted diff) +2 -0
@@ -3012,6 +3012,16 @@ fc_timeout_deleted_rport(struct work_str
     rport->port_state = FC_PORTSTATE_NOTPRESENT;
     rport->flags &= ~FC_RPORT_FAST_FAIL_TIMEDOUT;
 
+    /*
+     * Pre-emptively kill I/O rather than waiting for the work
queue
+ * item to teardown the starget. (FCOE libFC folks prefer
this
Review
Quoted 7 lines
(quoted diff) +7 -0
+     * and to have the rport_port_id still set when it's done).
+     */
+    spin_unlock_irqrestore(shost->host_lock, flags);
+    fc_terminate_rport_io(rport);
+
+    BUG_ON(rport->port_state != FC_PORTSTATE_NOTPRESENT);
+
I think we could this this bug_on or we might hit a problem below where this thread is changing the port_id but some other thread is calling fc_remote_port_add and changging the port id.
I think the problem is that fc_remote_port_add only calls fc_flush_work which flushes the fc_host work_q:
struct fc_rport *
() (struct Scsi_Host *shost, int channel,
struct fc_rport_identifiers *ids)
{
struct fc_internal *fci =
to_fc_internal(shost->transportt); struct fc_host_attrs *fc_host
= shost_to_fc_host(shost); struct fc_rport *rport;
         unsigned long flags;
         int match = 0;

         /* ensure any stgt delete functions are done */
         fc_flush_work(shost);
But fc_timeout_deleted_rport dev_loss_work is running on the devloss_work_q.
So if fc_timeout_deleted_rport grabs the host lock first, it will set the port state to FC_PORTSTATE_NOTPRESENT, then drop the lock.
Once fc_timeout_delete_rport drops the lock, fc_remote_port_add could have already passed the fc_flush_work() call because there was nothing on it. fc_remote_port_add would then drop down to the list search on the bindings array and see the remote port. It would then start setting the port state, id and port_name and node_name, but at the same time, because the host lock no longer guards it, fc_timedout_deleted_rport could be fiddling with the same fields and we could end up a mix of values or it could be running over the BUG_ON.
Is that right?
If so do we just want to flush the devloss_work_queue in fc_remote_port_add?
Quoted 10 lines
     /* remove the identifiers that aren't used in the
consisting binding */
     switch (fc_host->tgtid_bind_type) {
     case FC_TGTID_BIND_BY_WWPN:
(quoted diff) +0 -1
@@ -3035,7 +3045,6 @@ fc_timeout_deleted_rport(struct work_str
      * went away and didn't come back - we'll remove
      * all attached scsi devices.
      */
-    spin_unlock_irqrestore(shost->host_lock, flags);
 
Quoted 3 lines
Review fc_timeout_deleted_rport(struct work_str · line 3050
Quoted 3 lines
(quoted diff) +0 -0
     scsi_target_unblock(&rport->dev);
     fc_queue_work(shost, &rport->stgt_delete_work);
One of our tester hit the problem Michael describes above: right after releasing the shost->host_lock lock in fc_timeout_deleted_rport() the remote port was rediscovered by the LLDD (in our case zfcp), fc_remote_port_add() was called and BUG_ON() triggered.
Quoted 4 lines
But flushing the devloss_work_queue in fc_remote_port_add() won't help either because it still would be possible that right after the flush_workqueue() call a timer could trigger and fc_timeout_deleted_rport()
What timer could that be? Only the dev_loss_work delayed work calls fc_timeout_deleted_rport.
Are you thinking about fc_rport_final_delete?
If you do the
                if (!cancel_delayed_work(&rport->dev_loss_work))
                        fc_flush_devloss(shost);
Quoted 8 lines
Quoted 3 lines
sequence we should be ok I think, because after that point it should have either been canceled or the work has completed.
I did this in the attached patch. It also will prevent drivers from having to worry about about if the terminate rport IO or dev loss callback is running at the same time fc_remote_port_add is run.
I forgot to cancel the fail io work too. I guess we can also remove the same calls later in that function.>
drivers/scsi/scsi_transport_fc.c +8 ~~-10~~
index a152f89..e7bbc65 100644
@@ -2564,6 +2564,14 @@ fc_remote_port_add(struct Scsi_Host *shost, int channel,
 	/* ensure any stgt delete functions are done */
 	fc_flush_work(shost);
 
+	/* ensure fail fast is done */
+	if (!cancel_delayed_work(&rport->fail_io_work))
+		fc_flush_devloss(shost);
+
+	/* ensure dev loss is done */
+	if (!cancel_delayed_work(&rport->dev_loss_work))
+		fc_flush_devloss(shost);
+
 	/*
 	 * Search the list of "active" rports, for an rport that has been
 	 * deleted, but we've held off the real delete while the target
@@ -2630,16 +2638,6 @@ fc_remote_port_add(struct Scsi_Host *shost, int channel,
 				    (!(ids->roles & FC_PORT_ROLE_FCP_TARGET)))
 					return rport;
 
-				/*
-				 * Stop the fail io and dev_loss timers.
-				 * If they flush, the port_state will
-				 * be checked and will NOOP the function.
-				 */
-				if (!cancel_delayed_work(&rport->fail_io_work))
-					fc_flush_devloss(shost);
-				if (!cancel_delayed_work(&rport->dev_loss_work))
-					fc_flush_devloss(shost);
-
 				spin_lock_irqsave(shost->host_lock, flags);
 
 				rport->flags &=

Quoted 1 line

~(FC_RPORT_FAST_FAIL_TIMEDOUT |

Your patch does not work: you are canceling the fast_fail work and dev_loss work without initializing rport first.

We have to make sure that fc_remote_port_add(), fc_remote_port_delete(), and fc_timeout_deleted_rport() do not interfere with each other. Unfortunately I don't know yet how. I have to think more about this.

Signature · 1 line

Ralph Wuerthner

James Smart

2009-04-22 16:19 UTC #5 lore raw

All the flushes and workq stuff is very tricky. We (Mike Reed and I) had a devil of a time with it on large SMP systems. So adding these quickly is not advised.

I believe what we need to do is add a synchronization between the delete and the add sequences, which right now, expect the workq and flushing to provide it, but left other code paths, where it had to release the locks post the state change, at risk. Let me put something together.

-- james s

Quoted 9 lines · Ralph Wuerthner

Your patch does not work: you are canceling the fast_fail work and dev_loss work without initializing rport first.
We have to make sure that fc_remote_port_add(), fc_remote_port_delete(), and fc_timeout_deleted_rport() do not interfere with each other. Unfortunately I don't know yet how. I have to think more about this.