FWD: [BK PATCH] SCSI host num allocation improvement

FWD: [BK PATCH] SCSI host num allocation improvement

[James Bottomley]

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

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

James


[-- Attachment #2: Type: message/rfc822, Size: 7004 bytes --]

From: Ben Collins <bcollins@debian.org>
To: James Bottomley <James.Bottomley@steeleye.com>
Cc: Linux Kernel <linux-kernel@vger.kernel.org>
Subject: [BK PATCH] SCSI host num allocation improvement
Date: Thu, 26 Feb 2004 18:54:12 -0500
Message-ID: <20040226235412.GA819@phunnypharm.org>

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.

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


ChangeSet@1.1589, 2004-02-26 18:40:58-05:00, bcollins@debian.org
  [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
--- a/drivers/scsi/hosts.c	Thu Feb 26 18:52:00 2004
+++ b/drivers/scsi/hosts.c	Thu Feb 26 18:52:00 2004
@@ -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@@     
 

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

Re: FWD: [BK PATCH] SCSI host num allocation improvement

[Matthew Dharm]

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

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.

Matt

On Thu, Feb 26, 2004 at 06:40:43PM -0600, James Bottomley wrote:
> 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 <bcollins@debian.org>
> Subject: [BK PATCH] SCSI host num allocation improvement
> To: James Bottomley <James.Bottomley@steeleye.com>
> Cc: Linux Kernel <linux-kernel@vger.kernel.org>
> 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.
> 
> ===================================================================
> 
> 
> ChangeSet@1.1589, 2004-02-26 18:40:58-05:00, bcollins@debian.org
>   [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
> --- a/drivers/scsi/hosts.c	Thu Feb 26 18:52:00 2004
> +++ b/drivers/scsi/hosts.c	Thu Feb 26 18:52:00 2004
> @@ -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@@     
>  
> 
> -- 
> Debian     - http://www.debian.org/
> Linux 1394 - http://www.linux1394.org/
> Subversion - http://subversion.tigris.org/
> WatchGuard - http://www.watchguard.com/


-- 
Matthew Dharm                              Home: mdharm-usb@one-eyed-alien.net 
Maintainer, Linux USB Mass Storage Driver

I don't have a left mouse button.  I only have one mouse and it's on my right.
					-- Customer
User Friendly, 2/13/1999

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: FWD: [BK PATCH] SCSI host num allocation improvement

[Ben Collins]

On Thu, Feb 26, 2004 at 04:56:01PM -0800, Matthew Dharm wrote:
> 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.

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...).

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

Re: FWD: [BK PATCH] SCSI host num allocation improvement

[Arjan van de Ven]

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

On Fri, 2004-02-27 at 02:04, Ben Collins wrote:
> On Thu, Feb 26, 2004 at 04:56:01PM -0800, Matthew Dharm wrote:
> > 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.
> 
> 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...

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: FWD: [BK PATCH] SCSI host num allocation improvement

[Ben Collins]

On Fri, Feb 27, 2004 at 08:56:22AM +0100, Arjan van de Ven wrote:
> On Fri, 2004-02-27 at 02:04, Ben Collins wrote:
> > On Thu, Feb 26, 2004 at 04:56:01PM -0800, Matthew Dharm wrote:
> > > 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.
> > 
> > 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...

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


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

Re: FWD: [BK PATCH] SCSI host num allocation improvement

[Matthew Wilcox]

On Fri, Feb 27, 2004 at 07:30:09AM -0500, Ben Collins wrote:
> On Fri, Feb 27, 2004 at 08:56:22AM +0100, Arjan van de Ven wrote:
> > 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.

-- 
"Next the statesmen will invent cheap lies, putting the blame upon 
the nation that is attacked, and every man will be glad of those
conscience-soothing falsities, and will diligently study them, and refuse
to examine any refutations of them; and thus he will by and by convince 
himself that the war is just, and will thank God for the better sleep 
he enjoys after this process of grotesque self-deception." -- Mark Twain

Re: FWD: [BK PATCH] SCSI host num allocation improvement

[Ben Collins]

On Fri, Feb 27, 2004 at 12:39:06PM +0000, Matthew Wilcox wrote:
> On Fri, Feb 27, 2004 at 07:30:09AM -0500, Ben Collins wrote:
> > On Fri, Feb 27, 2004 at 08:56:22AM +0100, Arjan van de Ven wrote:
> > > 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.

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

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

Re: FWD: [BK PATCH] SCSI host num allocation improvement

[Christoph Hellwig]

On Fri, Feb 27, 2004 at 07:43:15AM -0500, Ben Collins wrote:
> > 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.

Sounds like cdrecord needs fixing.  Or even better replacing..

Re: FWD: [BK PATCH] SCSI host num allocation improvement

[Christoph Hellwig]

On Thu, Feb 26, 2004 at 06:40:43PM -0600, James Bottomley wrote:
> 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?

Re: FWD: [BK PATCH] SCSI host num allocation improvement

[Ben Collins]

On Fri, Feb 27, 2004 at 12:48:11PM +0000, Christoph Hellwig wrote:
> On Thu, Feb 26, 2004 at 06:40:43PM -0600, James Bottomley wrote:
> > 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?

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.

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

Re: FWD: [BK PATCH] SCSI host num allocation improvement

[Mike Anderson]

Ben Collins [bcollins@debian.org] wrote:
> On Fri, Feb 27, 2004 at 12:48:11PM +0000, Christoph Hellwig wrote:
> > On Thu, Feb 26, 2004 at 06:40:43PM -0600, James Bottomley wrote:
> > > 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?
> 
> 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.

-andmike
--
Michael Anderson
andmike@us.ibm.com

Re: FWD: [BK PATCH] SCSI host num allocation improvement

[Ben Collins]

On Fri, Feb 27, 2004 at 08:49:25AM -0800, Mike Anderson wrote:
> Ben Collins [bcollins@debian.org] wrote:
> > On Fri, Feb 27, 2004 at 12:48:11PM +0000, Christoph Hellwig wrote:
> > > On Thu, Feb 26, 2004 at 06:40:43PM -0600, James Bottomley wrote:
> > > > 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?
> > 
> > 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.

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.

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

Re: FWD: [BK PATCH] SCSI host num allocation improvement

[Mike Anderson]

Ben Collins [bcollins@debian.org] wrote:
> On Fri, Feb 27, 2004 at 08:49:25AM -0800, Mike Anderson wrote:
> > Ben Collins [bcollins@debian.org] wrote:
> > > On Fri, Feb 27, 2004 at 12:48:11PM +0000, Christoph Hellwig wrote:
> > > > On Thu, Feb 26, 2004 at 06:40:43PM -0600, James Bottomley wrote:
> > > > > 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?
> > > 
> > > 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.
> 
> 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.
> 

I guess I was directing it at you trying to understand the issue of
device naming working the "right" way and default kernel names picked
for hosts / devices. I can see the possible legacy issues of host /
device numbers reaching beyond the bounds the applications expects.
There is also the theoretical issue of scsi host numbers wrapping which
I believe has been mentioned in a old thread. I do not see the issue if
someone is using udev naming along with a increasing scsi host number.

-andmike
--
Michael Anderson
andmike@us.ibm.com

Re: FWD: [BK PATCH] SCSI host num allocation improvement

[James Bottomley]

On Fri, 2004-02-27 at 06:48, Christoph Hellwig wrote:
> 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?

James

Re: FWD: [BK PATCH] SCSI host num allocation improvement

[Ben Collins]

On Fri, Feb 27, 2004 at 09:08:18AM -0600, James Bottomley wrote:
> On Fri, 2004-02-27 at 06:48, Christoph Hellwig wrote:
> > 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.

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

Re: FWD: [BK PATCH] SCSI host num allocation improvement

[James Bottomley]

On Fri, 2004-02-27 at 09:15, Ben Collins wrote:
> 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.

James

Re: FWD: [BK PATCH] SCSI host num allocation improvement

[Ben Collins]

On Fri, Feb 27, 2004 at 09:29:34AM -0600, James Bottomley wrote:
> On Fri, 2004-02-27 at 09:15, Ben Collins wrote:
> > 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).

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

Re: FWD: [BK PATCH] SCSI host num allocation improvement

[James Bottomley]

On Fri, 2004-02-27 at 09:32, Ben Collins wrote:
> 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.

James

Re: FWD: [BK PATCH] SCSI host num allocation improvement

[Ben Collins]

On Fri, Feb 27, 2004 at 10:37:13AM -0600, James Bottomley wrote:
> On Fri, 2004-02-27 at 09:32, Ben Collins wrote:
> > 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.

The problems you keep raising already exist and aren't going to be
increased by this patch.

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

Re: FWD: [BK PATCH] SCSI host num allocation improvement

[David.Egolf]

On Fri, 2004-02-27 at 02:04, Ben Collins wrote:
> On Thu, Feb 26, 2004 at 04:56:01PM -0800, Matthew Dharm wrote:
> > 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.
>
> 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...).

An administrator should certainly know when devices are connected or
disconnected.  Unfortunately, failed devices can behave as if they were
disconnected.  I agree that the same thing could be said for scsi device
allocation.  It is also a problem there.  It happens to be a problem with
ethernet interfaces, as well.

We have systems with lots of HBA's and lots of redundant paths.  If the
names remain invariant, then we could continue to boot correctly and run
our applications.

On our legacy mainframes, the physical configuration is explicitly declared
by the administrator in a configuration map.  The software probes
components based on this map and simply skips over the components which are
not present or not responding, but the names remain invariant.  An added
advantage is that the OS is informed which of the visible hardware
components belong to it.  We do not have to resort to hardware fencing or
zoning strategies.  A third advantage to an explicit configuration is that
the OS, when it boots, can inform the administrator of discrepancies from
what was expected.  In a complex configuration it serves as a check on the
physical configuration.

When an OS, such as Linux, elects to "discover" the configuration and
assigns names as it finds components, then it is very difficult to maintain
a large configuration.  If the 12th component (of any type) out of 37 does
not respond, then all the components beginning at the 13th suddenly have
different names.