SCSI host num allocation improvement

I'm forwarding this to linux-scsi, which is the appropriate list to scrutinise it.

James

Date: Thu, 26 Feb 2004 18:54:12 -0500
From: Ben Collins <[email protected]>
Subject: [BK PATCH] SCSI host num allocation improvement
To: James Bottomley <[email protected]>
Cc: Linux Kernel <[email protected]>
X-Spam-Status: No, hits=-5.9 required=5.0
tests=BAYES_20,PATCH_UNIFIED_DIFF,USER_AGENT_MUTT autolearn=ham version=2.55

After doing a large round of debugging and fixups for ieee1394, I started getting really aggrivated with the way that the scsi layer allocates host numbers by forever incrementing from the last one.

Before too long, I was getting scsi137, etc. Not only did it look bad, it was bad. Cdrecord only scans bus 0-15, which means after a few hotplugs with your USB or Firewire CD/DVD recorder, you wont be able to burn to it. Yes, I know cdrecord needs to be fixed aswell, but I've had my fill of trying to convince the author of anything, so I wont be sending him a patch.

Anyway, this patch enables a race free (I hope) allocation of host numbers. And it will pick the lowest available number first. I've only tested it with ieee1394 sbp2, so real scsi users should give it a go.

You can import this changeset into BK by piping this whole message to:
'| bk receive [path to repository]' or apply the patch as usual.

===================================================================

[email protected], 2004-02-26 18:40:58-05:00, [email protected]
[SCSI]: Add a scsi_alloc_host_num function for race free, and non-incremental host ids

 hosts.c |   44 ++++++++++++++++++++++++++++++++++++++++----
 1 files changed, 40 insertions(+), 4 deletions(-)

diff -Nru a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c

drivers/scsi/hosts.c +40 -4

@@ -38,9 +38,6 @@
 #include "scsi_logging.h"
 
 
-static int scsi_host_next_hn;		/* host_no for next new host */
-
-
 static void scsi_host_cls_release(struct class_device *class_dev)
 {
 	put_device(&class_to_shost(class_dev)->shost_gendev);

@@ -166,6 +163,38 @@
 	kfree(shost);
 }
 
+static DECLARE_MUTEX(host_num_lock);
+/**
+ * scsi_host_num_alloc - allocate a unique host number for a scsi host.
+ *
+ * Note:
+ *      Must hold host_num_lock when calling this, and continue holding it
+ *      till after the host is added to the shost_class.
+ *
+ * Return value:
+ *      A unique host number.
+ **/
+static int scsi_host_num_alloc(void)
+{
+	int host_num = 0;
+	struct class *class = &shost_class;
+	struct class_device *cdev;
+	struct Scsi_Host *shost;
+
+	down_read(&class->subsys.rwsem);
+next_host_num_try:
+	list_for_each_entry(cdev, &class->children, node) {
+		shost = class_to_shost(cdev);
+		if (shost->host_no == host_num) {
+			host_num++;
+			goto next_host_num_try;
+		}
+	}
+	up_read(&class->subsys.rwsem);
+
+	return host_num;
+}
+
 /**
  * scsi_host_alloc - register a scsi host adapter instance.
  * @sht:	pointer to scsi host template

@@ -214,7 +243,6 @@
 
 	init_MUTEX(&shost->scan_mutex);
 
-	shost->host_no = scsi_host_next_hn++; /* XXX(hch): still racy */
 	shost->dma_channel = 0xff;
 
 	/* These three are default values which can be overridden */

@@ -263,6 +291,12 @@
 	if (rval)
 		goto fail_kfree;
 
+	/* Hold this lock until after we've added this to the scsi_host
+	 * class, to avoid race condititons with the host number
+	 * allocation scheme. */
+	down(&host_num_lock);
+	shost->host_no = scsi_host_num_alloc();
+
 	device_initialize(&shost->shost_gendev);
 	snprintf(shost->shost_gendev.bus_id, BUS_ID_SIZE, "host%d",
 		shost->host_no);

@@ -273,6 +307,8 @@
 	shost->shost_classdev.class = &shost_class;
 	snprintf(shost->shost_classdev.class_id, BUS_ID_SIZE, "host%d",
 		  shost->host_no);
+
+	up(&host_num_lock);
 
 	shost->eh_notify = &complete;
 	rval = kernel_thread(scsi_error_handler, shost, 0);

===================================================================

This BitKeeper patch contains the following changesets:
1.1589
## Wrapped with gzip_uu ##

M'XL( *"&/D [56:V_;-A3]+/Z*"Q3('">6J*<=!0[2)<$6;-V"9 $&=(5!
MDTQ$1*8RDK(;S/WO(ZG*>=1ML:*3!4BZO(]S[SF4_ JN-5=E,*=-70NIT2OX MN=&F#!B?"R+#1MU:TV736%/4:A5I1:,[KB2O(^O?OA\E88&LRP4QM((E5[H, MXC#=6,S#/2^#R[.?KG]]?8G0= HG%9&W_(H;F$Z1:=22U$P?$U/5C0R-(E(O MN"$A;1;KC>LZP3BQOSP>IS@OUG&!L_&:QBR.219SAI-L4F2(D25?''?PPOG= M7!@=2AO]+$^&DR3&*4[B\3K/\GB"3B$.XWQR #B+<!(E!<23,L-E/AGAO,08 M^ND</TX%]F(88?0C?-\.3A"%MU<G5^?O2GC-&!#05(L9J>N&SBK+S$RV"[AI M)36BD7#3*%"$<KA1G.\#D0QD(T="4L477!I2@PL"P33Z!?(,CS&Z>&0 C?[C M@1 F&!U]I6FFA!-"Y*!'KKX.Z9/^,XSMY).DR-9S/)GS<4YRRT:6L*V3_FPZ M1V21I.E!DJ]3'"=C+Z]MWE]7VK=#1N3N?G'<:%8[L&_[,N\^FS*-DQ3C/,^L M_I(TCC.OOX/Q,_6E!V62?TE]&891]K_([[O+KN/F=QBIE3^MC"ZVTO0-<CS- M8DC1>5Q,($V0-L0("J=G)_9E<S9[<_W'V9^#'O[,]G*W>XBBX1#!L&MPL^8[ MA1'X*S'<3J"5XN^6=XU8ESE7ON]N--X<VCPNU6^-X:6[\<>;U@943<W@6658 M55P")8[*6S"5T-W@:".-?9%R'^*6A-FD,J*N@=P86]I4'Z$(#80QSBSSWJA] M%5H3K7L\E]RT2H+51?L$U^LM';F(8=3/34BS;2R#92/8+OH'!<YAHX8IX$,4 M:*-::L#7AV%WF<+.$U0OG&:,+X65SI#:F\>U*U?7?7I@Z&,/T5\H8,U*SA0G M;+#C8T='NIWK!QVJE>8+RZ7D[\TC6J,>2A34PCY:IF:<T&IFU:@>!J[6/O1) M:"5JIKC<MZIE?!=L9X$O:I%W&$TS\P8?:.L$@;B!@3>-CKIZC7VI;(;1Y0CZ MQ[T]%Q+<-I:C3R"ZI0_(G>W]%YNS U =DWWX(?I@K:?VRP4Q.D^*' H41$/[ MR;9J<Y("K[36*JK7S8K_L.2]8IQ'+YN>9Q18@7@ ^VZ-.+*[S6V5R801II$:
M5L)4CQKLQ.,C/VX8]U;0M+*[/P0K*,_<8.?EU@M>3G"KW'SKY\DXA\3-H+W_ <--'F[X4M2>]TNYBFN#@@\W&!_@4R$Z&RT@@

Signature · 4 lines

Debian     - http://www.debian.org/
Linux 1394 - http://www.linux1394.org/
Subversion - http://subversion.tigris.org/
WatchGuard - http://www.watchguard.com/

My only concern with something like this is the userspace race problem. i.e the following sequence of events:

(1) User uses cdrecord -scanbus to locate their device (host:bus:dev:lun)
(2) HBA is disconnected
(3) New HBA is added
(4) User issues command to host:bus:dev:lun, and addresses the wrong device

While the current system doesn't prevent this, it makes it a much more difficult situation to happen.

Quoted 8 lines · Matthew Dharm

My only concern with something like this is the userspace race problem. i.e the following sequence of events:

(1) User uses cdrecord -scanbus to locate their device (host:bus:dev:lun)
(2) HBA is disconnected
(3) New HBA is added
(4) User issues command to host:bus:dev:lun, and addresses the wrong device

Quoted 2 lines

While the current system doesn't prevent this, it makes it a much more difficult situation to happen.

Surely the user would have to have known that one device was disconnected and another connected. Same thing could already be said for the scsi device allocation (sda, sdb, and sg0, sg1, sg2, etc...).

Quoted 12 lines · Ben Collins

Quoted 10 lines · Matthew Dharm

My only concern with something like this is the userspace race problem. i.e the following sequence of events:

(1) User uses cdrecord -scanbus to locate their device (host:bus:dev:lun)
(2) HBA is disconnected
(3) New HBA is added
(4) User issues command to host:bus:dev:lun, and addresses the wrong device

While the current system doesn't prevent this, it makes it a much more difficult situation to happen.

Quoted 3 lines

Surely the user would have to have known that one device was disconnected and another connected. Same thing could already be said for the scsi device allocation (sda, sdb, and sg0, sg1, sg2, etc...).

surely everyone is using cdrecord -dev=/dev/scd5 by now and not the host:bus:dev crap...

Quoted 2 lines · Arjan van de Ven

surely everyone is using cdrecord -dev=/dev/scd5 by now and not the host:bus:dev crap...

scsidev: '/dev/sg1'
devname: '/dev/sg1'
scsibus: -2 target: -2 lun: -2
Warning: Open by 'devname' is unintentional and not supported.

Quoted 8 lines · Ben Collins

Quoted 2 lines · Arjan van de Ven

surely everyone is using cdrecord -dev=/dev/scd5 by now and not the host:bus:dev crap...

scsidev: '/dev/sg1'
devname: '/dev/sg1'
scsibus: -2 target: -2 lun: -2
Warning: Open by 'devname' is unintentional and not supported.

Yeah, we need to patch that warning out of the debian cdrecord packages.

Quoted 1 line

Yeah, we need to patch that warning out of the debian cdrecord packages.

Even still, if the real bus number is > 15, it doesn't recognize the device.

I'm forwarding this to linux-scsi, which is the appropriate list to scrutinise it.

Quoted 2 lines · James Bottomley

I'm forwarding this to linux-scsi, which is the appropriate list to scrutinise it.

Well, the last patch looks sane if we want to do that. But didn't we declare the mononically increasing host numbers a feature?

Quoted 4 lines · Christoph Hellwig

Quoted 2 lines · James Bottomley

I'm forwarding this to linux-scsi, which is the appropriate list to scrutinise it.

Quoted 2 lines

Well, the last patch looks sane if we want to do that. But didn't we declare the mononically increasing host numbers a feature?

I can't see how things like the device naming can work the "right" way, but making the host numbers work in an increasing fashion would be a feature or even a benefit.

Quoted 5 lines · Ben Collins [[email protected]] wrote:

Quoted 4 lines · Christoph Hellwig

Quoted 2 lines · James Bottomley

I'm forwarding this to linux-scsi, which is the appropriate list to scrutinise it.

Quoted 7 lines

Quoted 2 lines

Well, the last patch looks sane if we want to do that. But didn't we declare the mononically increasing host numbers a feature?

I can't see how things like the device naming can work the "right" way, but making the host numbers work in an increasing fashion would be a feature or even a benefit.

I thought the direction was that default kernels names where headed in the direction of being depreciated. Are you using udev or something else for you naming policy. Is your naming policy based on position? Are the devices you are attaching unable to return a unique ID. Device naming based on immutable values are better.

Quoted 14 lines · Mike Anderson

Quoted 5 lines · Ben Collins [[email protected]] wrote:

Quoted 4 lines · Christoph Hellwig

Quoted 2 lines · James Bottomley

I'm forwarding this to linux-scsi, which is the appropriate list to scrutinise it.

Quoted 7 lines

Quoted 2 lines

Well, the last patch looks sane if we want to do that. But didn't we declare the mononically increasing host numbers a feature?

I can't see how things like the device naming can work the "right" way, but making the host numbers work in an increasing fashion would be a feature or even a benefit.

Quoted 5 lines

I thought the direction was that default kernels names where headed in the direction of being depreciated. Are you using udev or something else for you naming policy. Is your naming policy based on position? Are the devices you are attaching unable to return a unique ID. Device naming based on immutable values are better.

Not sure if you are directing this at me(firewire), but the ieee1394 devices are immutable naming that comes from the device. I don't have any control over how the scsi layer sets up the scsi host numbers and device names though.

Well, the last patch looks sane if we want to do that. But didn't we declare the mononically increasing host numbers a feature?

Quoted 2 lines · Christoph Hellwig

Well, the last patch looks sane if we want to do that. But didn't we declare the mononically increasing host numbers a feature?

This is the part that worries me too. At the moment with monotonically increasing host numbers, the operation of scsi_host_lookup() is provably safe. Can we prove that all uses of it are still safe if we reuse the host numbers?

If a driver somehow removes it's scsi host and then later attempts a scsi_host_lookup for the id, I think that driver is broken anyway. IOW, it is safe as long as you aren't broken in the first place.

Quoted 3 lines · Ben Collins

If a driver somehow removes it's scsi host and then later attempts a scsi_host_lookup for the id, I think that driver is broken anyway. IOW, it is safe as long as you aren't broken in the first place.

I'm not thinking about the driver, I'm thinking about the hotplug system. Hotplug events are queued and not locked. The event identifies the scsi device by the (host number, channel number, target, lun) quadruple. The reason it has to do that is that the /proc/scsi/scsi add/remove device operate on these numbers.

If we get a sequence of racing connects and disconnects, we could mis identify the devices because of the host number reuse. That's the race I want assurance of making safe if we move to reusing the numbers.

Obviously, one solution would be to fix the /proc add/remove, but it's a bit late in 2.6 to change the interface, I think.

On the same token, if you hot plug/unplug a single device (not a host) on the same host, you will already have this problem. This wont make the problem any worse or better. Devices are more likely to get plugged/unplugged anyway (well, in the case of usb/firewire you plug/unplug a host/dev combination, but still).

Quoted 5 lines · Ben Collins

On the same token, if you hot plug/unplug a single device (not a host) on the same host, you will already have this problem. This wont make the problem any worse or better. Devices are more likely to get plugged/unplugged anyway (well, in the case of usb/firewire you plug/unplug a host/dev combination, but still).

Simply having races is not an excuse for introducing more, particularly when they're under our control.

This represents a behaviour change in a stable kernel introducing a potential problem in hotplug and solving a fixable issue in a single user application tool.

What I'm not yet convinced of is that the benefits outweigh the risks.