aic94xx: Hotplug ex_change_count race fix

PATCH In review Alexis Bruemmer 6 participants 8 messages Sep 26, 2006 – Oct 4, 2006

Subsystems (2)

SCSI SCSI libsas

lore

Share & tools

lore

Alexis Bruemmer

author

2006-09-26 22:05 UTC #0 lore raw

PATCH 1/1 aic94xx: Hotplug ex_change_count race fix

In some cases while hotplugging disks on a system with an expander the broadcast primitive will be posted and begin processing before the expander change count is updated. This causes the device that triggered the broadcast to not be found.

--Alexis

Signed-off-by Alexis Bruemmer <[email protected]>

-------

drivers/scsi/libsas/sas_expander.c +27 ~~-15~~

diff -pNura linux-2.6.18-git4-orig/drivers/scsi/libsas/sas_expander.c linux-2.6.18-git4/drivers/scsi/libsas/sas_expander.c
@@ -24,6 +24,7 @@
 
 #include <linux/pci.h>
 #include <linux/scatterlist.h>
+#include <linux/delay.h>
 
 #include "sas_internal.h"

@@ -1760,23 +1761,34 @@ out:
 int sas_ex_revalidate_domain(struct domain_device *port_dev)
 {
 	int res;
+	int i;
 	struct domain_device *dev = NULL;
 
-	res = sas_find_bcast_dev(port_dev, &dev);
-	if (res)
-		goto out;
-	if (dev) {
-		struct expander_device *ex = &dev->ex_dev;
-		int i = 0, phy_id;
-
-		do {
-			phy_id = -1;
-			res = sas_find_bcast_phy(dev, &phy_id, i);
-			if (phy_id == -1)
-				break;
-			res = sas_rediscover(dev, phy_id);
-			i = phy_id + 1;
-		} while (i < ex->num_phys);
+	
+	/* Some expanders seem to generate a BROADCAST primitive before
+	 * they actually update their own "expander change count" field!
+	 * If we didn't find a device that caused the BROADCAST
+	 * primitive, let us wait and retry.
+	 */
+	for (i = 0; i < 2; i++) {
+		res = sas_find_bcast_dev(port_dev, &dev);
+		if (res)
+			goto out;
+		if (dev) {
+			struct expander_device *ex = &dev->ex_dev;
+			int i = 0, phy_id;
+
+			do {
+				phy_id = -1;
+				res = sas_find_bcast_phy(dev, &phy_id, i);
+				if (phy_id == -1)
+					break;
+				res = sas_rediscover(dev, phy_id);
+				i = phy_id + 1;
+			} while (i < ex->num_phys);
+			break;
+		}
+		ssleep(1);
 	}
 out:
 	return res;

James Bottomley

2006-10-03 14:19 UTC #1 lore raw

Quoted 4 lines · Alexis Bruemmer

In some cases while hotplugging disks on a system with an expander the broadcast primitive will be posted and begin processing before the expander change count is updated. This causes the device that triggered the broadcast to not be found.

Thanks; I'll stick this in.

However, it has always struck me that this broadcast code is fragile because of the way event processing works. If we get two fairly close together broadcast events, we'll amalgamate them into a single event and then stop processing as soon as we find one expander that changed, if you want to look at sorting that out ...

James

Alexis Bruemmer

author

2006-10-03 15:31 UTC #2 lore raw

Quoted 6 lines · James Bottomley

Quoted 4 lines · Alexis Bruemmer
In some cases while hotplugging disks on a system with an expander the broadcast primitive will be posted and begin processing before the expander change count is updated. This causes the device that triggered the broadcast to not be found.

Quoted 7 lines

Thanks; I'll stick this in.
However, it has always struck me that this broadcast code is fragile because of the way event processing works. If we get two fairly close together broadcast events, we'll amalgamate them into a single event and then stop processing as soon as we find one expander that changed, if you want to look at sorting that out ...

I will look into it.

--Alexis

Quoted 4 lines

James

[email protected]

2006-10-04 23:53 UTC #3 lore raw

Yes, I noticed that we stop processing as soon as we find a device that has change count. We should go through all the devices that have change counts. At least, in FCP world an RSCN (similar to BROADCAST in SAS) may be delayed by the Fabric to collect few changes.

Thanks, Malahal.

Quoted 18 lines · Alexis Bruemmer [[email protected]] wrote:

Quoted 6 lines · James Bottomley
Quoted 4 lines · Alexis Bruemmer
In some cases while hotplugging disks on a system with an expander the broadcast primitive will be posted and begin processing before the expander change count is updated. This causes the device that triggered the broadcast to not be found.
Quoted 7 lines
Thanks; I'll stick this in.
However, it has always struck me that this broadcast code is fragile because of the way event processing works. If we get two fairly close together broadcast events, we'll amalgamate them into a single event and then stop processing as soon as we find one expander that changed, if you want to look at sorting that out ...
I will look into it.
--Alexis

Luben Tuikov

2006-10-03 20:52 UTC #4 lore raw

In reply to James Bottomley

Quoted 6 lines · --- James Bottomley

Quoted 4 lines · Alexis Bruemmer
In some cases while hotplugging disks on a system with an expander the broadcast primitive will be posted and begin processing before the expander change count is updated. This causes the device that triggered the broadcast to not be found.

Quoted 7 lines

Thanks; I'll stick this in.
However, it has always struck me that this broadcast code is fragile because of the way event processing works. If we get two fairly close together broadcast events, we'll amalgamate them into a single event and then stop processing as soon as we find one expander that changed, if you want to look at sorting that out ...

You have to make a clarification: "the way event processing works" means the way it works after you removed all of event processing from my code the way I wrote it, and added a very naive "event processing" if it can be called that.

Event processing seems to work just fine with my version of my code the way I maintain it, and the way you had it originally on this list.

Just clarifying in case someone gets confused reading the code.

Quoted 9 lines

James
-
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Jeff Garzik

2006-10-03 21:19 UTC #5 lore raw

Quoted 9 lines · Luben Tuikov

You have to make a clarification: "the way event processing works" means the way it works after you removed all of event processing from my code the way I wrote it, and added a very naive "event processing" if it can be called that.
Event processing seems to work just fine with my version of my code the way I maintain it, and the way you had it originally on this list.
Just clarifying in case someone gets confused reading the code.

And we were all waiting breathlessly for this clarification too, I assure you.

In the dictionary under the phrase "sore loser", it says "see: Luben"

Jeff

Douglas Gilbert

2006-10-04 02:28 UTC #6 lore raw

Quoted 10 lines · Jeff Garzik

Quoted 9 lines · Luben Tuikov
You have to make a clarification: "the way event processing works" means the way it works after you removed all of event processing from my code the
way I wrote it, and added a very naive "event processing" if it can be called that.
Event processing seems to work just fine with my version of my code the way I maintain it, and the way you had it originally on this list.

Quoted 6 lines

Quoted 1 line
Just clarifying in case someone gets confused reading the code.
And we were all waiting breathlessly for this clarification too, I assure you.
In the dictionary under the phrase "sore loser", it says "see: Luben"

I work with both drivers and provide feedback to their maintainers. IMO the loser is "linux".

One handicap the official driver has is the SATL it has decided to use.

... back to my redmond os for testing ...

Doug Gilbert

Luben Tuikov

2006-10-04 07:39 UTC #7 lore raw

In reply to Jeff Garzik

Quoted 9 lines · --- Jeff Garzik

Quoted 8 lines · Luben Tuikov
You have to make a clarification: "the way event processing works" means the way it works after you removed all of event processing from my code the way I wrote it, and added a very naive "event processing" if it can be called that.
Event processing seems to work just fine with my version of my code the way I maintain it, and the way you had it originally on this list.

Quoted 6 lines

Quoted 1 line
Just clarifying in case someone gets confused reading the code.
And we were all waiting breathlessly for this clarification too, I assure you.
In the dictionary under the phrase "sore loser", it says "see: Luben"

Sorry that you feel this way Jeff and that your have to resort to personal attacks. It makes me doubt your professionalism.

Merely on technical grounds and the fact that the code still bears my copyright, I wanted to mention that all those bugs being posted against aic94xx and the SAS Stack, and "fixed" by Bottomley and LTC are not due to the reason that I wrote the code with bugs, but due to the fact of Bottomley blocking the code from going in, then him and LTC removing large portions of perfectly well working and tested code (SAS analysed and complient) with their own versions and interpretations, only later to "fix" the very code they submitted as a substition of a perfectly working code I wrote that they removed.

For example, event management is one such:
http://marc.theaimsgroup.com/?l=linux-scsi&m=115895097501842&w=2

Another example, commit 2908d778ab3e244900c310974e1fc1c69066e450,
http://kernel.org/git/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=2908d778ab3e244900c310974e1fc1c69066e450
has things in it like this:

    ...
    o Preliminary expander support for aic94xx
    ...

All the while aic94xx and the SAS Stack had always supported expanders. There is just a lot of misleading information, and I don't want the casual reader to believe that aic94xx and the SAS Stack as I posted them didn't support expanders all the while they did.

And I'm sure you don't want such confusion too and as a member of the open source community will do everything in your power to disambiguate that. For example mentioning the announcement emails of the very first posting of the SAS Stack and aic94xx:
http://marc.theaimsgroup.com/?l=linux-scsi&m=112629423714248&w=2
http://marc.theaimsgroup.com/?l=linux-scsi&m=112629509826900&w=2
http://marc.theaimsgroup.com/?l=linux-scsi&m=112629509318354&w=2

The tricky point is that aic94xx and the SAS Stack were *NOT* committed into git as I had posted them, but were first _edited_ and then "fixed" by what you see in commit 2908d778ab3e244900c310974e1fc1c69066e450,
http://kernel.org/git/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=2908d778ab3e244900c310974e1fc1c69066e450
and _then_ commited into git as you'd see them starting with the commit id quoted above.

Surely there are a lot of people just reading this mailing list, interested in where storage is going with Linux. You don't want to mislead them... Not to mention SATL support.

I should probably post my git trees somewhere? The SAS Stack now also includes enterprise-grade per-device SATL support. I'm sure you'd enjoy that the most. ;-)

Quoted 9 lines

Jeff
-
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html