[PATCH] for Deadlock in transport_fc

[PATCH] for Deadlock in transport_fc

[James.Smart]

[-- Attachment #1: Type: text/plain, Size: 2768 bytes --]


> currently I'm trying to use the new transport_fc to read the
> very often changing FibreChannel configuration in a test system.
> 
> To avoid a growing list of consistent binding entries (which
> make no sense in this special case), I tried to switch off this
> feature by
>      "echo none > /sys/class/fc_host/host1/tgtid_bind_type"
> 
> Unfortunately, the system stalls immediately, I guess the reason
> is store_fc_private_host_tgtid_bind_type() calling
> fc_rport_terminate() while holding host_lock.

Yep. A rather blatant lock bug that slipped through due to testing
on a non-smp box.  Try the attached patch.

> 
> If I understand the code correctly, even if tgtid_bind_type
> would work correctly, still the rport-nummer and scsi-target-id
> would count up on configuration changes. In the lpfc-driver, I
> saw:
> #define MAX_FCP_TARGET              256     /* max num of FCP 
> targets supported */
> Will this result in problems after 256 configuration changes?
> If so, what could I do?

Yes, it will. Once the target id assignment became larger than 256,
scsi scans won't see the remote port.

I admit, a more difficult implementation is possible if this is a
goal. In general, a production system will always manage devices
by wwpn assignments, and will usually use fabric zoning to minimize
it's view. Thus, a configuration such as yours, with high variability
in the fabric, is unusual.

I'm open to a different implementation if deemed necessary.

> BTW: My Emulex boards do not recognize a change behind the
> FibreChannel switch. So I force them to scan the configuration
> using "echo [01] >/sys/class/scsi_host/host1/board_online".
> Is there a better way to do this?

This is an issue worth noting. The lpfc driver registers for RSCN
events, so it should be seeing changes. There could be switch issues
in not posting the RSCN's (rare, long-time ago). The driver does
qualify it's nameserver request by FC4 type of FCP. Is the device in
question registering as an FCP type device with the fabric ?
Please follow up on this. This should not be happening.

Also - tweaking the lpfc-specific board_online attribute is a little
odd to make things scan. It resets and restarts the entire adapter.
For a link rescan, we recommend that bounce the link via
"echo 1 > sys/class/scsi_host/host1/issue_lip". If all you needed was
a scsi scan - try "echo "- - -"  > /sys/class/scsi_host/host2/scan ".

-- James


> 
> Regards
> Bodo
> 
> P.S.: Please CC me, I'm not on the list.
> -
> To unsubscribe from this list: send the line "unsubscribe 
> linux-scsi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

[-- Attachment #2: patch.fc_xpt --]
[-- Type: application/octet-stream, Size: 1455 bytes --]

diff -upNr linux-2.6.12-rc6/drivers/scsi/scsi_transport_fc.c fixed/drivers/scsi/scsi_transport_fc.c
--- linux-2.6.12-rc6/drivers/scsi/scsi_transport_fc.c	2005-06-07 15:18:58.000000000 -0400
+++ fixed/drivers/scsi/scsi_transport_fc.c	2005-06-08 22:49:42.000000000 -0400
@@ -817,12 +817,15 @@ show_fc_private_host_tgtid_bind_type(str
 	return snprintf(buf, FC_BINDTYPE_MAX_NAMELEN, "%s\n", name);
 }
 
+#define get_list_head_entry(pos, head, member) 		\
+	pos = list_entry((head)->next, typeof(*pos), member)
+
 static ssize_t
 store_fc_private_host_tgtid_bind_type(struct class_device *cdev,
 	const char *buf, size_t count)
 {
 	struct Scsi_Host *shost = transport_class_to_shost(cdev);
-	struct fc_rport *rport, *next_rport;
+	struct fc_rport *rport;
  	enum fc_tgtid_binding_type val;
 	unsigned long flags;
 
@@ -832,9 +835,13 @@ store_fc_private_host_tgtid_bind_type(st
 	/* if changing bind type, purge all unused consistent bindings */
 	if (val != fc_host_tgtid_bind_type(shost)) {
 		spin_lock_irqsave(shost->host_lock, flags);
-		list_for_each_entry_safe(rport, next_rport,
-				&fc_host_rport_bindings(shost), peers)
+		while (!list_empty(&fc_host_rport_bindings(shost))) {
+			get_list_head_entry(rport,
+				&fc_host_rport_bindings(shost), peers);
+			spin_unlock_irqrestore(shost->host_lock, flags);
 			fc_rport_terminate(rport);
+			spin_lock_irqsave(shost->host_lock, flags);
+		}
 		spin_unlock_irqrestore(shost->host_lock, flags);
 	}
 

Re: [PATCH] for Deadlock in transport_fc

[Bodo Stroesser]

James.Smart@Emulex.Com wrote:
>>currently I'm trying to use the new transport_fc to read the
>>very often changing FibreChannel configuration in a test system.
>>
>>To avoid a growing list of consistent binding entries (which
>>make no sense in this special case), I tried to switch off this
>>feature by
>>     "echo none > /sys/class/fc_host/host1/tgtid_bind_type"
>>
>>Unfortunately, the system stalls immediately, I guess the reason
>>is store_fc_private_host_tgtid_bind_type() calling
>>fc_rport_terminate() while holding host_lock.
> 
> 
> Yep. A rather blatant lock bug that slipped through due to testing
> on a non-smp box.  Try the attached patch.

Thank you, it works fine.

> 
> 
>>If I understand the code correctly, even if tgtid_bind_type
>>would work correctly, still the rport-nummer and scsi-target-id
>>would count up on configuration changes. In the lpfc-driver, I
>>saw:
>>#define MAX_FCP_TARGET              256     /* max num of FCP 
>>targets supported */
>>Will this result in problems after 256 configuration changes?
>>If so, what could I do?
> 
> 
> Yes, it will. Once the target id assignment became larger than 256,
> scsi scans won't see the remote port.
> 
> I admit, a more difficult implementation is possible if this is a
> goal. In general, a production system will always manage devices
> by wwpn assignments, and will usually use fabric zoning to minimize
> it's view. Thus, a configuration such as yours, with high variability
> in the fabric, is unusual.
> 
> I'm open to a different implementation if deemed necessary.

Meanwhile I understood, that using "port_id" instead of "none"
does the trick for us. Now the scsi target numbers are restricted to
the number of fabric-ports used in the test. That always is less than
256 in our configuration.

So, for me there is no need to change anything.

> 
> 
>>BTW: My Emulex boards do not recognize a change behind the
>>FibreChannel switch. So I force them to scan the configuration
>>using "echo [01] >/sys/class/scsi_host/host1/board_online".
>>Is there a better way to do this?
> 
> 
> This is an issue worth noting. The lpfc driver registers for RSCN
> events, so it should be seeing changes. There could be switch issues
> in not posting the RSCN's (rare, long-time ago). The driver does
> qualify it's nameserver request by FC4 type of FCP. Is the device in
> question registering as an FCP type device with the fabric ?
> Please follow up on this. This should not be happening.
> 
> Also - tweaking the lpfc-specific board_online attribute is a little
> odd to make things scan. It resets and restarts the entire adapter.
> For a link rescan, we recommend that bounce the link via
> "echo 1 > sys/class/scsi_host/host1/issue_lip". If all you needed was
> a scsi scan - try "echo "- - -"  > /sys/class/scsi_host/host2/scan ".

Thank you for the explanations. We tested a bit more and found out, that
one has to wait lpfc_nodev_tmo seconds between disconnecting one target
from the fabric and connecting the other.
As this might be done quite fast in our tests, we now set lpfc_nodev_tmo
to 1, and it seems to work fine. Is there any risk with such a short
timer?
For my understanding: what is "echo 1 > sys/class/scsi_host/host1/issue_lip"?
When we used it after not having waited for nodev_tmo, it didn't cause a
rescan (i.e., the target still had the old wwpn).

BTW: at least once when we disconnected one target and connected the other
without waiting for nodev_tmo, we saw the situation, that the new target was
accessible, while sysfs still showed us the old wwpn.

	Bodo
> 
> -- James
> 
> 
> 
>>Regards
>>Bodo
>>
>>P.S.: Please CC me, I'm not on the list.
>>-
>>To unsubscribe from this list: send the line "unsubscribe 
>>linux-scsi" in
>>the body of a message to majordomo@vger.kernel.org
>>More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 

RE: [PATCH] for Deadlock in transport_fc

[James.Smart]

> Thank you for the explanations. We tested a bit more and 
> found out, that
> one has to wait lpfc_nodev_tmo seconds between disconnecting 
> one target
> from the fabric and connecting the other.
> As this might be done quite fast in our tests, we now set 
> lpfc_nodev_tmo
> to 1, and it seems to work fine. Is there any risk with such a short
> timer?

The risk of setting this too low is that normal links may occur where
the port disappears for longer than 1 second. If so, the lpfc driver
will notify the transport of a loss of the port, which will tear down
the target and all luns. In your test case, it's what you wanted.
In otherwise normal scenarios, it may not be.  If it did occur, and
the port came back, discovery should see it, re-add the port with the
transport, kick off the scsi scan, and enumerate the target and luns
again.  Beware though- if you are not using udev, name shift may occur.

> For my understanding: what is "echo 1 > 
> sys/class/scsi_host/host1/issue_lip"?
> When we used it after not having waited for nodev_tmo, it 
> didn't cause a
> rescan (i.e., the target still had the old wwpn).

This either injects a LIP if the topology is loop, or bounces the link if
the topology is pt-2-pt. Discovery has to then kick in and complete before
changes would be seen.

> 
> BTW: at least once when we disconnected one target and 
> connected the other
> without waiting for nodev_tmo, we saw the situation, that the 
> new target was
> accessible, while sysfs still showed us the old wwpn.

We ought to track this further offline. There's more detail I'd like to
get from you to ensure there isn't an error here.

-- james s

Re: [PATCH] for Deadlock in transport_fc

[Bodo Stroesser]

James.Smart@Emulex.Com wrote:
> 
>>BTW: at least once when we disconnected one target and 
>>connected the other
>>without waiting for nodev_tmo, we saw the situation, that the 
>>new target was
>>accessible, while sysfs still showed us the old wwpn.
> 
> 
> We ought to track this further offline. There's more detail I'd like to
> get from you to ensure there isn't an error here.
> 
We'll try to reproduce this. If we succeed, we will let you know the
recipe.

	Bodo