[PATCHv4 0/1] Update SCSI hosts to use ida for host number mgmt

[PATCHv4 0/1] Update SCSI hosts to use ida for host number mgmt

[Lee Duncan]

This patch updates the SCSI hosts module to use the ida
index-management routines to manage its host_no index instead
of using an ATOMIC integer. This means that host numbers
can now be reclaimed and re-used.

NOTE: it was not feasible to use idr_*() functions instead of
ida_*() functions, since using idr_find() without additional
locking to find our Scsi_Host structure left a window between
idr lookup and host structure deletion that would have
required additional locking to close.

Changes from v3:
 - Switched from idr to ida since managing our instance
   pointer required extra locking
Changes from v2 and v1:
 - First two version used idr instead of ida

Lee Duncan (1):
  SCSI: hosts: update to use ida_simple for host_no management

 drivers/scsi/hosts.c | 22 ++++++++++++++--------
 1 file changed, 14 insertions(+), 8 deletions(-)

-- 
2.1.4

[PATCHv4 1/1] SCSI: hosts: update to use ida_simple for host_no management

[Lee Duncan]

Update the SCSI hosts module to use the ida_simple*() routines
to manage its host_no index instead of an ATOMIC integer. This
means that the SCSI host number will now be reclaimable.

Signed-off-by: Lee Duncan <lduncan@suse.com>
---
 drivers/scsi/hosts.c | 22 ++++++++++++++--------
 1 file changed, 14 insertions(+), 8 deletions(-)

diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
index 8bb173e01084..b6a5ffa886b7 100644
--- a/drivers/scsi/hosts.c
+++ b/drivers/scsi/hosts.c
@@ -33,7 +33,7 @@
 #include <linux/transport_class.h>
 #include <linux/platform_device.h>
 #include <linux/pm_runtime.h>
-
+#include <linux/idr.h>
 #include <scsi/scsi_device.h>
 #include <scsi/scsi_host.h>
 #include <scsi/scsi_transport.h>
@@ -42,7 +42,7 @@
 #include "scsi_logging.h"
 
 
-static atomic_t scsi_host_next_hn = ATOMIC_INIT(0);	/* host_no for next new host */
+static DEFINE_IDA(host_index_ida);
 
 
 static void scsi_host_cls_release(struct device *dev)
@@ -337,6 +337,8 @@ static void scsi_host_dev_release(struct device *dev)
 
 	kfree(shost->shost_data);
 
+	ida_simple_remove(&host_index_ida, shost->host_no);
+
 	if (parent)
 		put_device(parent);
 	kfree(shost);
@@ -370,6 +372,7 @@ struct Scsi_Host *scsi_host_alloc(struct scsi_host_template *sht, int privsize)
 {
 	struct Scsi_Host *shost;
 	gfp_t gfp_mask = GFP_KERNEL;
+	int index;
 
 	if (sht->unchecked_isa_dma && privsize)
 		gfp_mask |= __GFP_DMA;
@@ -388,11 +391,11 @@ struct Scsi_Host *scsi_host_alloc(struct scsi_host_template *sht, int privsize)
 	init_waitqueue_head(&shost->host_wait);
 	mutex_init(&shost->scan_mutex);
 
-	/*
-	 * subtract one because we increment first then return, but we need to
-	 * know what the next host number was before increment
-	 */
-	shost->host_no = atomic_inc_return(&scsi_host_next_hn) - 1;
+	index = ida_simple_get(&host_index_ida, 0, 0, GFP_KERNEL);
+	if (index < 0)
+		goto fail_kfree;
+	shost->host_no = index;
+
 	shost->dma_channel = 0xff;
 
 	/* These three are default values which can be overridden */
@@ -477,7 +480,7 @@ struct Scsi_Host *scsi_host_alloc(struct scsi_host_template *sht, int privsize)
 		shost_printk(KERN_WARNING, shost,
 			"error handler thread failed to spawn, error = %ld\n",
 			PTR_ERR(shost->ehandler));
-		goto fail_kfree;
+		goto fail_index_remove;
 	}
 
 	shost->tmf_work_q = alloc_workqueue("scsi_tmf_%d",
@@ -493,6 +496,8 @@ struct Scsi_Host *scsi_host_alloc(struct scsi_host_template *sht, int privsize)
 
  fail_kthread:
 	kthread_stop(shost->ehandler);
+ fail_index_remove:
+	ida_simple_remove(&host_index_ida, shost->host_no);
  fail_kfree:
 	kfree(shost);
 	return NULL;
@@ -588,6 +593,7 @@ int scsi_init_hosts(void)
 void scsi_exit_hosts(void)
 {
 	class_unregister(&shost_class);
+	ida_destroy(&host_index_ida);
 }
 
 int scsi_is_host_device(const struct device *dev)
-- 
2.1.4

Re: [PATCHv4 1/1] SCSI: hosts: update to use ida_simple for host_no management

[Johannes Thumshirn]

On Wed, 2015-10-07 at 16:51 -0700, Lee Duncan wrote:
> Update the SCSI hosts module to use the ida_simple*() routines
> to manage its host_no index instead of an ATOMIC integer. This
> means that the SCSI host number will now be reclaimable.
> 
> Signed-off-by: Lee Duncan <lduncan@suse.com>
> ---
>  drivers/scsi/hosts.c | 22 ++++++++++++++--------
>  1 file changed, 14 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
> index 8bb173e01084..b6a5ffa886b7 100644
> --- a/drivers/scsi/hosts.c
> +++ b/drivers/scsi/hosts.c
> @@ -33,7 +33,7 @@
>  #include <linux/transport_class.h>
>  #include <linux/platform_device.h>
>  #include <linux/pm_runtime.h>
> -
> +#include <linux/idr.h>
>  #include <scsi/scsi_device.h>
>  #include <scsi/scsi_host.h>
>  #include <scsi/scsi_transport.h>
> @@ -42,7 +42,7 @@
>  #include "scsi_logging.h"
>  
>  
> -static atomic_t scsi_host_next_hn = ATOMIC_INIT(0);	/*
> host_no for next new host */
> +static DEFINE_IDA(host_index_ida);
>  
>  
>  static void scsi_host_cls_release(struct device *dev)
> @@ -337,6 +337,8 @@ static void scsi_host_dev_release(struct device
> *dev)
>  
>  	kfree(shost->shost_data);
>  
> +	ida_simple_remove(&host_index_ida, shost->host_no);
> +
>  	if (parent)
>  		put_device(parent);
>  	kfree(shost);
> @@ -370,6 +372,7 @@ struct Scsi_Host *scsi_host_alloc(struct
> scsi_host_template *sht, int privsize)
>  {
>  	struct Scsi_Host *shost;
>  	gfp_t gfp_mask = GFP_KERNEL;
> +	int index;
>  
>  	if (sht->unchecked_isa_dma && privsize)
>  		gfp_mask |= __GFP_DMA;
> @@ -388,11 +391,11 @@ struct Scsi_Host *scsi_host_alloc(struct
> scsi_host_template *sht, int privsize)
>  	init_waitqueue_head(&shost->host_wait);
>  	mutex_init(&shost->scan_mutex);
>  
> -	/*
> -	 * subtract one because we increment first then return, but
> we need to
> -	 * know what the next host number was before increment
> -	 */
> -	shost->host_no = atomic_inc_return(&scsi_host_next_hn) - 1;
> +	index = ida_simple_get(&host_index_ida, 0, 0, GFP_KERNEL);
> +	if (index < 0)
> +		goto fail_kfree;
> +	shost->host_no = index;
> +
>  	shost->dma_channel = 0xff;
>  
>  	/* These three are default values which can be overridden */
> @@ -477,7 +480,7 @@ struct Scsi_Host *scsi_host_alloc(struct
> scsi_host_template *sht, int privsize)
>  		shost_printk(KERN_WARNING, shost,
>  			"error handler thread failed to spawn, error
> = %ld\n",
>  			PTR_ERR(shost->ehandler));
> -		goto fail_kfree;
> +		goto fail_index_remove;
>  	}
>  
>  	shost->tmf_work_q = alloc_workqueue("scsi_tmf_%d",
> @@ -493,6 +496,8 @@ struct Scsi_Host *scsi_host_alloc(struct
> scsi_host_template *sht, int privsize)
>  
>   fail_kthread:
>  	kthread_stop(shost->ehandler);
> + fail_index_remove:
> +	ida_simple_remove(&host_index_ida, shost->host_no);
>   fail_kfree:
>  	kfree(shost);
>  	return NULL;
> @@ -588,6 +593,7 @@ int scsi_init_hosts(void)
>  void scsi_exit_hosts(void)
>  {
>  	class_unregister(&shost_class);
> +	ida_destroy(&host_index_ida);
>  }
>  
>  int scsi_is_host_device(const struct device *dev)

Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de>

Re: [PATCHv4 1/1] SCSI: hosts: update to use ida_simple for host_no management

[James Bottomley]

On Wed, 2015-10-07 at 16:51 -0700, Lee Duncan wrote:
> Update the SCSI hosts module to use the ida_simple*() routines
> to manage its host_no index instead of an ATOMIC integer. This
> means that the SCSI host number will now be reclaimable.

OK, but why would we want to do this?  We do it for sd because our minor
space for the device nodes is very constrained, so packing is essential.
For HBAs, there's no device space density to worry about, they're
largely statically allocated at boot time and not reusing the numbers
allows easy extraction of hotplug items for the logs (quite useful for
USB) because each separate hotplug has a separate and monotonically
increasing host number.

James

Re: [PATCHv4 1/1] SCSI: hosts: update to use ida_simple for host_no management

[Lee Duncan]

On 10/14/2015 06:55 AM, James Bottomley wrote:
> On Wed, 2015-10-07 at 16:51 -0700, Lee Duncan wrote:
>> Update the SCSI hosts module to use the ida_simple*() routines
>> to manage its host_no index instead of an ATOMIC integer. This
>> means that the SCSI host number will now be reclaimable.
> 
> OK, but why would we want to do this?  We do it for sd because our minor
> space for the device nodes is very constrained, so packing is essential.
> For HBAs, there's no device space density to worry about, they're
> largely statically allocated at boot time and not reusing the numbers
> allows easy extraction of hotplug items for the logs (quite useful for
> USB) because each separate hotplug has a separate and monotonically
> increasing host number.
> 
> James
> 

Good question, James. Apologies for not making the need clear.

The iSCSI subsystem uses a host structure for discovery, then throws it
away. So each time it does discovery it gets a new host structure. With
the current approach, that number is ever increasing. It's only a matter
of time until some user with a hundreds of disks and perhaps thousands
of LUNs, that likes to do periodic discovery (think super-computers)
will run out of host numbers. Or, worse yet, get a negative number
number (because the value is signed right now).

And this use case is a real one right now, by the way.

As you can see from the patch, it's a small amount of code to ensure
that the host number management is handled more cleanly.

-- 
Lee Duncan

Re: [PATCHv4 1/1] SCSI: hosts: update to use ida_simple for host_no management

[James Bottomley]

On Wed, 2015-10-14 at 11:34 -0700, Lee Duncan wrote:
> On 10/14/2015 06:55 AM, James Bottomley wrote:
> > On Wed, 2015-10-07 at 16:51 -0700, Lee Duncan wrote:
> >> Update the SCSI hosts module to use the ida_simple*() routines
> >> to manage its host_no index instead of an ATOMIC integer. This
> >> means that the SCSI host number will now be reclaimable.
> > 
> > OK, but why would we want to do this?  We do it for sd because our minor
> > space for the device nodes is very constrained, so packing is essential.
> > For HBAs, there's no device space density to worry about, they're
> > largely statically allocated at boot time and not reusing the numbers
> > allows easy extraction of hotplug items for the logs (quite useful for
> > USB) because each separate hotplug has a separate and monotonically
> > increasing host number.
> > 
> > James
> > 
> 
> Good question, James. Apologies for not making the need clear.
> 
> The iSCSI subsystem uses a host structure for discovery, then throws it
> away. So each time it does discovery it gets a new host structure. With
> the current approach, that number is ever increasing. It's only a matter
> of time until some user with a hundreds of disks and perhaps thousands
> of LUNs, that likes to do periodic discovery (think super-computers)
> will run out of host numbers. Or, worse yet, get a negative number
> number (because the value is signed right now).
> 
> And this use case is a real one right now, by the way.

Um, so even if you do discovery continuously, say one every second, it
still will take 68 years before we wrap the sign.

> As you can see from the patch, it's a small amount of code to ensure
> that the host number management is handled more cleanly.

Well, I'm a bit worried about the loss of a monotonically increasing
host number from the debugging perspective.  Right now, if you look at
any log, hostX always refers to one and only one incarnation throughout
the system lifetime for any given value of X.  With your patch, the
lowest host number gets continually reused ... probably for every hot
plug event.  If the USB and other hotplug system people don't mind this,
I suppose I can live with it, but I'd like to hear their view before
making this change.

James

Re: [PATCHv4 1/1] SCSI: hosts: update to use ida_simple for host_no management

[Lee Duncan]

On 10/14/2015 11:53 AM, James Bottomley wrote:
> On Wed, 2015-10-14 at 11:34 -0700, Lee Duncan wrote:
>> On 10/14/2015 06:55 AM, James Bottomley wrote:
>>> On Wed, 2015-10-07 at 16:51 -0700, Lee Duncan wrote:
>>>> Update the SCSI hosts module to use the ida_simple*() routines
>>>> to manage its host_no index instead of an ATOMIC integer. This
>>>> means that the SCSI host number will now be reclaimable.
>>>
>>> OK, but why would we want to do this?  We do it for sd because our minor
>>> space for the device nodes is very constrained, so packing is essential.
>>> For HBAs, there's no device space density to worry about, they're
>>> largely statically allocated at boot time and not reusing the numbers
>>> allows easy extraction of hotplug items for the logs (quite useful for
>>> USB) because each separate hotplug has a separate and monotonically
>>> increasing host number.
>>>
>>> James
>>>
>>
>> Good question, James. Apologies for not making the need clear.
>>
>> The iSCSI subsystem uses a host structure for discovery, then throws it
>> away. So each time it does discovery it gets a new host structure. With
>> the current approach, that number is ever increasing. It's only a matter
>> of time until some user with a hundreds of disks and perhaps thousands
>> of LUNs, that likes to do periodic discovery (think super-computers)
>> will run out of host numbers. Or, worse yet, get a negative number
>> number (because the value is signed right now).
>>
>> And this use case is a real one right now, by the way.
> 
> Um, so even if you do discovery continuously, say one every second, it
> still will take 68 years before we wrap the sign.
> 
>> As you can see from the patch, it's a small amount of code to ensure
>> that the host number management is handled more cleanly.
> 
> Well, I'm a bit worried about the loss of a monotonically increasing
> host number from the debugging perspective.  Right now, if you look at
> any log, hostX always refers to one and only one incarnation throughout
> the system lifetime for any given value of X.  With your patch, the
> lowest host number gets continually reused ... probably for every hot
> plug event.  If the USB and other hotplug system people don't mind this,
> I suppose I can live with it, but I'd like to hear their view before
> making this change.
> 
> James
> 

James:

Understand your point, but I've never seen the host number not repeating
be a benefit in debugging or testing. And Hannes suggested this fix, so
I can only assume he also did not see a benefit of unique host
numbering. And purely aesthetically, seeing "host4595483528" in sysfs
would not be very user-friendly.

But one possible solution to address your concern would be to increase
the host number until it ran out of room (or hit some large maximum),
and only then start re-using host numbers. This would preserve the
current monotonically-increasing behavior at least initially. But I
worry that having this bi-modal numbering scheme might confuse some.
-- 
Lee Duncan

Re: [PATCHv4 1/1] SCSI: hosts: update to use ida_simple for host_no management

[Hannes Reinecke]

On 10/14/2015 08:53 PM, James Bottomley wrote:
> On Wed, 2015-10-14 at 11:34 -0700, Lee Duncan wrote:
>> On 10/14/2015 06:55 AM, James Bottomley wrote:
>>> On Wed, 2015-10-07 at 16:51 -0700, Lee Duncan wrote:
>>>> Update the SCSI hosts module to use the ida_simple*() routines
>>>> to manage its host_no index instead of an ATOMIC integer. This
>>>> means that the SCSI host number will now be reclaimable.
>>>
>>> OK, but why would we want to do this?  We do it for sd because our minor
>>> space for the device nodes is very constrained, so packing is essential.
>>> For HBAs, there's no device space density to worry about, they're
>>> largely statically allocated at boot time and not reusing the numbers
>>> allows easy extraction of hotplug items for the logs (quite useful for
>>> USB) because each separate hotplug has a separate and monotonically
>>> increasing host number.
>>>
>>> James
>>>
>>
>> Good question, James. Apologies for not making the need clear.
>>
>> The iSCSI subsystem uses a host structure for discovery, then throws it
>> away. So each time it does discovery it gets a new host structure. With
>> the current approach, that number is ever increasing. It's only a matter
>> of time until some user with a hundreds of disks and perhaps thousands
>> of LUNs, that likes to do periodic discovery (think super-computers)
>> will run out of host numbers. Or, worse yet, get a negative number
>> number (because the value is signed right now).
>>
>> And this use case is a real one right now, by the way.
> 
> Um, so even if you do discovery continuously, say one every second, it
> still will take 68 years before we wrap the sign.
> 
>> As you can see from the patch, it's a small amount of code to ensure
>> that the host number management is handled more cleanly.
> 
> Well, I'm a bit worried about the loss of a monotonically increasing
> host number from the debugging perspective.  Right now, if you look at
> any log, hostX always refers to one and only one incarnation throughout
> the system lifetime for any given value of X.  With your patch, the
> lowest host number gets continually reused ... probably for every hot
> plug event.  If the USB and other hotplug system people don't mind this,
> I suppose I can live with it, but I'd like to hear their view before
> making this change.
> 
Typically host numbers are not a real issue; whenever I need to
debug something more often than not I need to figure out
informations about the scsi device. And not once in my entire career
I had any needs to rely on the SCSI host number.

Plus the SCSI host number is the only thing in the stack which
_does_ increase; everything else like bus/target/LUN
numbers are stable and won't change much, irrespective of the
number of rescans or unloads. Which always irritated me.

So I'm definitely in favour of reusing the SCSI host numbers.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		               zSeries & Storage
hare@suse.com			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

Re: [PATCHv4 1/1] SCSI: hosts: update to use ida_simple for host_no management

[Lee Duncan]

Adding linux-usb and linux-hotplug to cc list, in case they wish to comment.

Summary: I want to change SCSI host number so that it gets re-used, like
disk index numbers, instead of always increasing.

Please see below.

On 10/14/2015 11:53 AM, James Bottomley wrote:
> On Wed, 2015-10-14 at 11:34 -0700, Lee Duncan wrote:
>> On 10/14/2015 06:55 AM, James Bottomley wrote:
>>> On Wed, 2015-10-07 at 16:51 -0700, Lee Duncan wrote:
>>>> Update the SCSI hosts module to use the ida_simple*() routines
>>>> to manage its host_no index instead of an ATOMIC integer. This
>>>> means that the SCSI host number will now be reclaimable.
>>>
>>> OK, but why would we want to do this?  We do it for sd because our minor
>>> space for the device nodes is very constrained, so packing is essential.
>>> For HBAs, there's no device space density to worry about, they're
>>> largely statically allocated at boot time and not reusing the numbers
>>> allows easy extraction of hotplug items for the logs (quite useful for
>>> USB) because each separate hotplug has a separate and monotonically
>>> increasing host number.
>>>
>>> James
>>>
>>
>> Good question, James. Apologies for not making the need clear.
>>
>> The iSCSI subsystem uses a host structure for discovery, then throws it
>> away. So each time it does discovery it gets a new host structure. With
>> the current approach, that number is ever increasing. It's only a matter
>> of time until some user with a hundreds of disks and perhaps thousands
>> of LUNs, that likes to do periodic discovery (think super-computers)
>> will run out of host numbers. Or, worse yet, get a negative number
>> number (because the value is signed right now).
>>
>> And this use case is a real one right now, by the way.
> 
> Um, so even if you do discovery continuously, say one every second, it
> still will take 68 years before we wrap the sign.
> 
>> As you can see from the patch, it's a small amount of code to ensure
>> that the host number management is handled more cleanly.
> 
> Well, I'm a bit worried about the loss of a monotonically increasing
> host number from the debugging perspective.  Right now, if you look at
> any log, hostX always refers to one and only one incarnation throughout
> the system lifetime for any given value of X.  With your patch, the
> lowest host number gets continually reused ... probably for every hot
> plug event.  If the USB and other hotplug system people don't mind this,
> I suppose I can live with it, but I'd like to hear their view before
> making this change.
> 
> James
> 
> 
> 
> 

-- 
Lee Duncan
SUSE Labs

Re: [PATCHv4 1/1] SCSI: hosts: update to use ida_simple for host_no management

[Lee Duncan]

Adding linux-usb and linux-hotplug to cc list, in case they wish to comment.

Summary: I want to change SCSI host number so that it gets re-used, like
disk index numbers, instead of always increasing.

Please see below.

On 10/14/2015 11:53 AM, James Bottomley wrote:
> On Wed, 2015-10-14 at 11:34 -0700, Lee Duncan wrote:
>> On 10/14/2015 06:55 AM, James Bottomley wrote:
>>> On Wed, 2015-10-07 at 16:51 -0700, Lee Duncan wrote:
>>>> Update the SCSI hosts module to use the ida_simple*() routines
>>>> to manage its host_no index instead of an ATOMIC integer. This
>>>> means that the SCSI host number will now be reclaimable.
>>>
>>> OK, but why would we want to do this?  We do it for sd because our minor
>>> space for the device nodes is very constrained, so packing is essential.
>>> For HBAs, there's no device space density to worry about, they're
>>> largely statically allocated at boot time and not reusing the numbers
>>> allows easy extraction of hotplug items for the logs (quite useful for
>>> USB) because each separate hotplug has a separate and monotonically
>>> increasing host number.
>>>
>>> James
>>>
>>
>> Good question, James. Apologies for not making the need clear.
>>
>> The iSCSI subsystem uses a host structure for discovery, then throws it
>> away. So each time it does discovery it gets a new host structure. With
>> the current approach, that number is ever increasing. It's only a matter
>> of time until some user with a hundreds of disks and perhaps thousands
>> of LUNs, that likes to do periodic discovery (think super-computers)
>> will run out of host numbers. Or, worse yet, get a negative number
>> number (because the value is signed right now).
>>
>> And this use case is a real one right now, by the way.
> 
> Um, so even if you do discovery continuously, say one every second, it
> still will take 68 years before we wrap the sign.
> 
>> As you can see from the patch, it's a small amount of code to ensure
>> that the host number management is handled more cleanly.
> 
> Well, I'm a bit worried about the loss of a monotonically increasing
> host number from the debugging perspective.  Right now, if you look at
> any log, hostX always refers to one and only one incarnation throughout
> the system lifetime for any given value of X.  With your patch, the
> lowest host number gets continually reused ... probably for every hot
> plug event.  If the USB and other hotplug system people don't mind this,
> I suppose I can live with it, but I'd like to hear their view before
> making this change.
> 
> James
> 
> 
> 
> 

-- 
Lee Duncan
SUSE Labs

Re: [PATCHv4 1/1] SCSI: hosts: update to use ida_simple for host_no management

[Greg KH]

On Fri, Oct 16, 2015 at 01:03:42PM -0700, Lee Duncan wrote:
> Adding linux-usb and linux-hotplug to cc list, in case they wish to comment.
> 
> Summary: I want to change SCSI host number so that it gets re-used, like
> disk index numbers, instead of always increasing.
> 
> Please see below.
> 
> On 10/14/2015 11:53 AM, James Bottomley wrote:
> > On Wed, 2015-10-14 at 11:34 -0700, Lee Duncan wrote:
> >> On 10/14/2015 06:55 AM, James Bottomley wrote:
> >>> On Wed, 2015-10-07 at 16:51 -0700, Lee Duncan wrote:
> >>>> Update the SCSI hosts module to use the ida_simple*() routines
> >>>> to manage its host_no index instead of an ATOMIC integer. This
> >>>> means that the SCSI host number will now be reclaimable.
> >>>
> >>> OK, but why would we want to do this?  We do it for sd because our minor
> >>> space for the device nodes is very constrained, so packing is essential.
> >>> For HBAs, there's no device space density to worry about, they're
> >>> largely statically allocated at boot time and not reusing the numbers
> >>> allows easy extraction of hotplug items for the logs (quite useful for
> >>> USB) because each separate hotplug has a separate and monotonically
> >>> increasing host number.
> >>>
> >>> James
> >>>
> >>
> >> Good question, James. Apologies for not making the need clear.
> >>
> >> The iSCSI subsystem uses a host structure for discovery, then throws it
> >> away. So each time it does discovery it gets a new host structure. With
> >> the current approach, that number is ever increasing. It's only a matter
> >> of time until some user with a hundreds of disks and perhaps thousands
> >> of LUNs, that likes to do periodic discovery (think super-computers)
> >> will run out of host numbers. Or, worse yet, get a negative number
> >> number (because the value is signed right now).
> >>
> >> And this use case is a real one right now, by the way.
> > 
> > Um, so even if you do discovery continuously, say one every second, it
> > still will take 68 years before we wrap the sign.
> > 
> >> As you can see from the patch, it's a small amount of code to ensure
> >> that the host number management is handled more cleanly.
> > 
> > Well, I'm a bit worried about the loss of a monotonically increasing
> > host number from the debugging perspective.  Right now, if you look at
> > any log, hostX always refers to one and only one incarnation throughout
> > the system lifetime for any given value of X.  With your patch, the
> > lowest host number gets continually reused ... probably for every hot
> > plug event.  If the USB and other hotplug system people don't mind this,
> > I suppose I can live with it, but I'd like to hear their view before
> > making this change.

USB "people" don't care about this, why would we?  You can plug and
unplug and plug devices in lots of times and they get the "old" names
all the time, this is something that tools have had to deal with for
well over a decade.

thanks,

greg k-h

Re: [PATCHv4 1/1] SCSI: hosts: update to use ida_simple for host_no management

[Greg KH]

On Fri, Oct 16, 2015 at 01:03:42PM -0700, Lee Duncan wrote:
> Adding linux-usb and linux-hotplug to cc list, in case they wish to comment.
> 
> Summary: I want to change SCSI host number so that it gets re-used, like
> disk index numbers, instead of always increasing.
> 
> Please see below.
> 
> On 10/14/2015 11:53 AM, James Bottomley wrote:
> > On Wed, 2015-10-14 at 11:34 -0700, Lee Duncan wrote:
> >> On 10/14/2015 06:55 AM, James Bottomley wrote:
> >>> On Wed, 2015-10-07 at 16:51 -0700, Lee Duncan wrote:
> >>>> Update the SCSI hosts module to use the ida_simple*() routines
> >>>> to manage its host_no index instead of an ATOMIC integer. This
> >>>> means that the SCSI host number will now be reclaimable.
> >>>
> >>> OK, but why would we want to do this?  We do it for sd because our minor
> >>> space for the device nodes is very constrained, so packing is essential.
> >>> For HBAs, there's no device space density to worry about, they're
> >>> largely statically allocated at boot time and not reusing the numbers
> >>> allows easy extraction of hotplug items for the logs (quite useful for
> >>> USB) because each separate hotplug has a separate and monotonically
> >>> increasing host number.
> >>>
> >>> James
> >>>
> >>
> >> Good question, James. Apologies for not making the need clear.
> >>
> >> The iSCSI subsystem uses a host structure for discovery, then throws it
> >> away. So each time it does discovery it gets a new host structure. With
> >> the current approach, that number is ever increasing. It's only a matter
> >> of time until some user with a hundreds of disks and perhaps thousands
> >> of LUNs, that likes to do periodic discovery (think super-computers)
> >> will run out of host numbers. Or, worse yet, get a negative number
> >> number (because the value is signed right now).
> >>
> >> And this use case is a real one right now, by the way.
> > 
> > Um, so even if you do discovery continuously, say one every second, it
> > still will take 68 years before we wrap the sign.
> > 
> >> As you can see from the patch, it's a small amount of code to ensure
> >> that the host number management is handled more cleanly.
> > 
> > Well, I'm a bit worried about the loss of a monotonically increasing
> > host number from the debugging perspective.  Right now, if you look at
> > any log, hostX always refers to one and only one incarnation throughout
> > the system lifetime for any given value of X.  With your patch, the
> > lowest host number gets continually reused ... probably for every hot
> > plug event.  If the USB and other hotplug system people don't mind this,
> > I suppose I can live with it, but I'd like to hear their view before
> > making this change.

USB "people" don't care about this, why would we?  You can plug and
unplug and plug devices in lots of times and they get the "old" names
all the time, this is something that tools have had to deal with for
well over a decade.

thanks,

greg k-h

Re: [PATCHv4 1/1] SCSI: hosts: update to use ida_simple for host_no management

[Lee Duncan]

On 10/14/2015 08:53 PM, James Bottomley wrote:
> On Wed, 2015-10-14 at 11:34 -0700, Lee Duncan wrote:
>> On 10/14/2015 06:55 AM, James Bottomley wrote:
>>> On Wed, 2015-10-07 at 16:51 -0700, Lee Duncan wrote:
>>>> Update the SCSI hosts module to use the ida_simple*() routines
>>>> to manage its host_no index instead of an ATOMIC integer. This
>>>> means that the SCSI host number will now be reclaimable.
>>>
>>> OK, but why would we want to do this?  We do it for sd because our minor
>>> space for the device nodes is very constrained, so packing is essential.
>>> For HBAs, there's no device space density to worry about, they're
>>> largely statically allocated at boot time and not reusing the numbers
>>> allows easy extraction of hotplug items for the logs (quite useful for
>>> USB) because each separate hotplug has a separate and monotonically
>>> increasing host number.
>>>
>>> James
>>>
>>
>> Good question, James. Apologies for not making the need clear.
>>
>> The iSCSI subsystem uses a host structure for discovery, then throws it
>> away. So each time it does discovery it gets a new host structure. With
>> the current approach, that number is ever increasing. It's only a matter
>> of time until some user with a hundreds of disks and perhaps thousands
>> of LUNs, that likes to do periodic discovery (think super-computers)
>> will run out of host numbers. Or, worse yet, get a negative number
>> number (because the value is signed right now).
>>
>> And this use case is a real one right now, by the way.
> 
> Um, so even if you do discovery continuously, say one every second, it
> still will take 68 years before we wrap the sign.
> 
>> As you can see from the patch, it's a small amount of code to ensure
>> that the host number management is handled more cleanly.
> 
> Well, I'm a bit worried about the loss of a monotonically increasing
> host number from the debugging perspective.  Right now, if you look at
> any log, hostX always refers to one and only one incarnation throughout
> the system lifetime for any given value of X.  With your patch, the
> lowest host number gets continually reused ... probably for every hot
> plug event.  If the USB and other hotplug system people don't mind this,
> I suppose I can live with it, but I'd like to hear their view before
> making this change.
> 
> James

James?

It looks like both Hannes and GregKH agreed this was the proper approach.

-- 
Lee Duncan

Re: [PATCHv4 1/1] SCSI: hosts: update to use ida_simple for host_no management

[Martin K. Petersen]

>>>>> "Lee" == Lee Duncan <lduncan@suse.com> writes:

>> Well, I'm a bit worried about the loss of a monotonically increasing
>> host number from the debugging perspective.  Right now, if you look
>> at any log, hostX always refers to one and only one incarnation
>> throughout the system lifetime for any given value of X.

That's a feature that I would absolutely hate to lose. I spend a huge
amount of time looking at system logs.

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCHv4 1/1] SCSI: hosts: update to use ida_simple for host_no management

[Hannes Reinecke]

On 11/13/2015 10:54 PM, Martin K. Petersen wrote:
>>>>>> "Lee" == Lee Duncan <lduncan@suse.com> writes:
> 
>>> Well, I'm a bit worried about the loss of a monotonically increasing
>>> host number from the debugging perspective.  Right now, if you look
>>> at any log, hostX always refers to one and only one incarnation
>>> throughout the system lifetime for any given value of X.
> 
> That's a feature that I would absolutely hate to lose. I spend a huge
> amount of time looking at system logs.
> 
Right. Then have it enabled via a modprobe parameters.

We actually had customers running into a host_no overflow due to
excessive host allocations and freeing done by iSCSI.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		               zSeries & Storage
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

Re: [PATCHv4 1/1] SCSI: hosts: update to use ida_simple for host_no management

[Lee Duncan]

On 11/16/2015 04:10 AM, Hannes Reinecke wrote:
> On 11/13/2015 10:54 PM, Martin K. Petersen wrote:
>>>>>>> "Lee" == Lee Duncan <lduncan@suse.com> writes:
>>
>>>> Well, I'm a bit worried about the loss of a monotonically increasing
>>>> host number from the debugging perspective.  Right now, if you look
>>>> at any log, hostX always refers to one and only one incarnation
>>>> throughout the system lifetime for any given value of X.
>>
>> That's a feature that I would absolutely hate to lose. I spend a huge
>> amount of time looking at system logs.
>>
> Right. Then have it enabled via a modprobe parameters.
> 
> We actually had customers running into a host_no overflow due to
> excessive host allocations and freeing done by iSCSI.
> 
> Cheers,
> 
> Hannes
> 

Martin: I will be glad to update the patch, creating a modprobe
parameter as suggested, if you find this acceptable.
-- 
Lee Duncan
SUSE Labs

Re: [PATCHv4 1/1] SCSI: hosts: update to use ida_simple for host_no management

[Martin K. Petersen]

>>>>> "Lee" == Lee Duncan <lduncan@suse.com> writes:

Lee> Martin: I will be glad to update the patch, creating a modprobe
Lee> parameter as suggested, if you find this acceptable.

For development use a module parameter would be fine. But I am concerned
about our support folks that rely on the incrementing host number when
analyzing customer log files.

Ewan: How do you folks feel about this change?

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCHv4 1/1] SCSI: hosts: update to use ida_simple for host_no management

[Lee Duncan]

On 11/17/2015 03:20 PM, Martin K. Petersen wrote:
>>>>>> "Lee" == Lee Duncan <lduncan@suse.com> writes:
> 
> Lee> Martin: I will be glad to update the patch, creating a modprobe
> Lee> parameter as suggested, if you find this acceptable.
> 
> For development use a module parameter would be fine. But I am concerned
> about our support folks that rely on the incrementing host number when
> analyzing customer log files.
> 
> Ewan: How do you folks feel about this change?
> 

Ewan?

-- 
Lee Duncan

Re: [PATCHv4 1/1] SCSI: hosts: update to use ida_simple for host_no management

[Ewan Milne]

On Thu, 2015-12-10 at 13:48 -0800, Lee Duncan wrote:
> On 11/17/2015 03:20 PM, Martin K. Petersen wrote:
> >>>>>> "Lee" == Lee Duncan <lduncan@suse.com> writes:
> > 
> > Lee> Martin: I will be glad to update the patch, creating a modprobe
> > Lee> parameter as suggested, if you find this acceptable.
> > 
> > For development use a module parameter would be fine. But I am concerned
> > about our support folks that rely on the incrementing host number when
> > analyzing customer log files.
> > 
> > Ewan: How do you folks feel about this change?
> > 
> 
> Ewan?


Personally, I think having host numbers that increase essentially
without limit (I think I've seen this with iSCSI sessions) are a
problem, the numbers start to lose meaning for people when they
are not easily recognizable.  Yes, it can help when you're analyzing
a log file, but it seems to me that you would want to track the
host state throughout anyway, so you could just follow the number
as it changes.

If we change the behavior, we have to change documentation, and
our support people will get calls.  But that's not a reason not
to do it.

-Ewan

Re: [PATCHv4 1/1] SCSI: hosts: update to use ida_simple for host_no management

[Lee Duncan]

On 12/11/2015 07:31 AM, Ewan Milne wrote:
> On Thu, 2015-12-10 at 13:48 -0800, Lee Duncan wrote:
>> On 11/17/2015 03:20 PM, Martin K. Petersen wrote:
>>>>>>>> "Lee" == Lee Duncan <lduncan@suse.com> writes:
>>>
>>> Lee> Martin: I will be glad to update the patch, creating a modprobe
>>> Lee> parameter as suggested, if you find this acceptable.
>>>
>>> For development use a module parameter would be fine. But I am concerned
>>> about our support folks that rely on the incrementing host number when
>>> analyzing customer log files.
>>>
>>> Ewan: How do you folks feel about this change?
>>>
>>
>> Ewan?
> 
> 
> Personally, I think having host numbers that increase essentially
> without limit (I think I've seen this with iSCSI sessions) are a
> problem, the numbers start to lose meaning for people when they
> are not easily recognizable.  Yes, it can help when you're analyzing
> a log file, but it seems to me that you would want to track the
> host state throughout anyway, so you could just follow the number
> as it changes.
> 
> If we change the behavior, we have to change documentation, and
> our support people will get calls.  But that's not a reason not
> to do it.
> 
> -Ewan
> 

Ewan:

Thank you for your reply. I agree with you, which is why I generated
this patch.

If we *do* make this change, do you think it would be useful to have a
module option to revert to the old numbering behavior? I actually think
it would be more confusing to support two behaviors than it would be to
bite the bullet (so to speak) and make the change.

-- 
Lee Duncan

Re: [PATCHv4 1/1] SCSI: hosts: update to use ida_simple for host_no management

[Ewan Milne]

On Sun, 2015-12-13 at 11:16 -0800, Lee Duncan wrote:
> On 12/11/2015 07:31 AM, Ewan Milne wrote:
> > On Thu, 2015-12-10 at 13:48 -0800, Lee Duncan wrote:
> >> On 11/17/2015 03:20 PM, Martin K. Petersen wrote:
> >>>>>>>> "Lee" == Lee Duncan <lduncan@suse.com> writes:
> >>>
> >>> Lee> Martin: I will be glad to update the patch, creating a modprobe
> >>> Lee> parameter as suggested, if you find this acceptable.
> >>>
> >>> For development use a module parameter would be fine. But I am concerned
> >>> about our support folks that rely on the incrementing host number when
> >>> analyzing customer log files.
> >>>
> >>> Ewan: How do you folks feel about this change?
> >>>
> >>
> >> Ewan?
> > 
> > 
> > Personally, I think having host numbers that increase essentially
> > without limit (I think I've seen this with iSCSI sessions) are a
> > problem, the numbers start to lose meaning for people when they
> > are not easily recognizable.  Yes, it can help when you're analyzing
> > a log file, but it seems to me that you would want to track the
> > host state throughout anyway, so you could just follow the number
> > as it changes.
> > 
> > If we change the behavior, we have to change documentation, and
> > our support people will get calls.  But that's not a reason not
> > to do it.
> > 
> > -Ewan
> > 
> 
> Ewan:
> 
> Thank you for your reply. I agree with you, which is why I generated
> this patch.
> 
> If we *do* make this change, do you think it would be useful to have a
> module option to revert to the old numbering behavior? I actually think
> it would be more confusing to support two behaviors than it would be to
> bite the bullet (so to speak) and make the change.
> 

I'm not opposed to having the module option if others (Martin?) feel
they need it, but generally I think it's better to keep things as simple
as possible.  So, unless there are strong objections, I would say no.

-Ewan

Re: [PATCHv4 1/1] SCSI: hosts: update to use ida_simple for host_no management

[Hannes Reinecke]

On 12/14/2015 04:07 PM, Ewan Milne wrote:
> On Sun, 2015-12-13 at 11:16 -0800, Lee Duncan wrote:
>> On 12/11/2015 07:31 AM, Ewan Milne wrote:
>>> On Thu, 2015-12-10 at 13:48 -0800, Lee Duncan wrote:
>>>> On 11/17/2015 03:20 PM, Martin K. Petersen wrote:
>>>>>>>>>> "Lee" == Lee Duncan <lduncan@suse.com> writes:
>>>>>
>>>>> Lee> Martin: I will be glad to update the patch, creating a modprobe
>>>>> Lee> parameter as suggested, if you find this acceptable.
>>>>>
>>>>> For development use a module parameter would be fine. But I am concerned
>>>>> about our support folks that rely on the incrementing host number when
>>>>> analyzing customer log files.
>>>>>
>>>>> Ewan: How do you folks feel about this change?
>>>>>
>>>>
>>>> Ewan?
>>>
>>>
>>> Personally, I think having host numbers that increase essentially
>>> without limit (I think I've seen this with iSCSI sessions) are a
>>> problem, the numbers start to lose meaning for people when they
>>> are not easily recognizable.  Yes, it can help when you're analyzing
>>> a log file, but it seems to me that you would want to track the
>>> host state throughout anyway, so you could just follow the number
>>> as it changes.
>>>
>>> If we change the behavior, we have to change documentation, and
>>> our support people will get calls.  But that's not a reason not
>>> to do it.
>>>
>>> -Ewan
>>>
>>
>> Ewan:
>>
>> Thank you for your reply. I agree with you, which is why I generated
>> this patch.
>>
>> If we *do* make this change, do you think it would be useful to have a
>> module option to revert to the old numbering behavior? I actually think
>> it would be more confusing to support two behaviors than it would be to
>> bite the bullet (so to speak) and make the change.
>>
>
> I'm not opposed to having the module option if others (Martin?) feel
> they need it, but generally I think it's better to keep things as simple
> as possible.  So, unless there are strong objections, I would say no.
>
Agreeing with Ewan here.

Martin, I guess it's up to you to tell us whether you absolutely 
need a module parameter ...

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		               zSeries & Storage
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

Re: [PATCHv4 1/1] SCSI: hosts: update to use ida_simple for host_no management

[Hannes Reinecke]

On 12/14/2015 04:07 PM, Ewan Milne wrote:
> On Sun, 2015-12-13 at 11:16 -0800, Lee Duncan wrote:
>> On 12/11/2015 07:31 AM, Ewan Milne wrote:
>>> On Thu, 2015-12-10 at 13:48 -0800, Lee Duncan wrote:
>>>> On 11/17/2015 03:20 PM, Martin K. Petersen wrote:
>>>>>>>>>> "Lee" == Lee Duncan <lduncan@suse.com> writes:
>>>>>
>>>>> Lee> Martin: I will be glad to update the patch, creating a modprobe
>>>>> Lee> parameter as suggested, if you find this acceptable.
>>>>>
>>>>> For development use a module parameter would be fine. But I am concerned
>>>>> about our support folks that rely on the incrementing host number when
>>>>> analyzing customer log files.
>>>>>
>>>>> Ewan: How do you folks feel about this change?
>>>>>
>>>>
>>>> Ewan?
>>>
>>>
>>> Personally, I think having host numbers that increase essentially
>>> without limit (I think I've seen this with iSCSI sessions) are a
>>> problem, the numbers start to lose meaning for people when they
>>> are not easily recognizable.  Yes, it can help when you're analyzing
>>> a log file, but it seems to me that you would want to track the
>>> host state throughout anyway, so you could just follow the number
>>> as it changes.
>>>
>>> If we change the behavior, we have to change documentation, and
>>> our support people will get calls.  But that's not a reason not
>>> to do it.
>>>
>>> -Ewan
>>>
>>
>> Ewan:
>>
>> Thank you for your reply. I agree with you, which is why I generated
>> this patch.
>>
>> If we *do* make this change, do you think it would be useful to have a
>> module option to revert to the old numbering behavior? I actually think
>> it would be more confusing to support two behaviors than it would be to
>> bite the bullet (so to speak) and make the change.
>>
>
> I'm not opposed to having the module option if others (Martin?) feel
> they need it, but generally I think it's better to keep things as simple
> as possible.  So, unless there are strong objections, I would say no.
>
Agreeing with Ewan here.

Martin, I guess it's up to you to tell us whether you absolutely 
need a module parameter ...

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		               zSeries & Storage
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)
--
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: [PATCHv4 1/1] SCSI: hosts: update to use ida_simple for host_no management

[Martin K. Petersen]

>>>>> "Hannes" == Hannes Reinecke <hare@suse.de> writes:

>> I'm not opposed to having the module option if others (Martin?) feel
>> they need it, but generally I think it's better to keep things as
>> simple as possible.  So, unless there are strong objections, I would
>> say no.

Hannes> Agreeing with Ewan here.

Hannes> I guess it's up to you to tell us whether you absolutely need a
Hannes> module parameter ...

Still not a big ida fan but since the most people seem to be in favor of
this I guess I'll have to bite the bullet.

I don't see much value in the module parameter since it will require
customers to tweak their configs and reproduce. Not worth the hassle.

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCHv4 1/1] SCSI: hosts: update to use ida_simple for host_no management

[Lee Duncan]

On 12/14/2015 05:55 PM, Martin K. Petersen wrote:
>>>>>> "Hannes" == Hannes Reinecke <hare@suse.de> writes:
> 
>>> I'm not opposed to having the module option if others (Martin?) feel
>>> they need it, but generally I think it's better to keep things as
>>> simple as possible.  So, unless there are strong objections, I would
>>> say no.
> 
> Hannes> Agreeing with Ewan here.
> 
> Hannes> I guess it's up to you to tell us whether you absolutely need a
> Hannes> module parameter ...
> 
> Still not a big ida fan but since the most people seem to be in favor of
> this I guess I'll have to bite the bullet.
> 
> I don't see much value in the module parameter since it will require
> customers to tweak their configs and reproduce. Not worth the hassle.
> 

Thank you Martin. I'll look at further cleaning up the host module, but
I think this still much better than leaving the code as is.

-- 
Lee Duncan

Re: [PATCHv4 1/1] SCSI: hosts: update to use ida_simple for host_no management

[Lee Duncan]

On 12/17/2015 11:24 AM, Lee Duncan wrote:
> On 12/14/2015 05:55 PM, Martin K. Petersen wrote:
>>>>>>> "Hannes" == Hannes Reinecke <hare@suse.de> writes:
>>
>>>> I'm not opposed to having the module option if others (Martin?) feel
>>>> they need it, but generally I think it's better to keep things as
>>>> simple as possible.  So, unless there are strong objections, I would
>>>> say no.
>>
>> Hannes> Agreeing with Ewan here.
>>
>> Hannes> I guess it's up to you to tell us whether you absolutely need a
>> Hannes> module parameter ...
>>
>> Still not a big ida fan but since the most people seem to be in favor of
>> this I guess I'll have to bite the bullet.
>>
>> I don't see much value in the module parameter since it will require
>> customers to tweak their configs and reproduce. Not worth the hassle.
>>
> 
> Thank you Martin. I'll look at further cleaning up the host module, but
> I think this still much better than leaving the code as is.
> 

James:

Do you need me to resubmit this patch now that it's accepted?
-- 
Lee Duncan

Re: [PATCHv4 1/1] SCSI: hosts: update to use ida_simple for host_no management

[Martin K. Petersen]

>>>>> "Lee" == Lee Duncan <lduncan@suse.com> writes:

Lee> Do you need me to resubmit this patch now that it's accepted?

Please resend.

Thanks!

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCHv4 1/1] SCSI: hosts: update to use ida_simple for host_no management

[Lee Duncan]

On 01/05/2016 03:53 PM, Martin K. Petersen wrote:
>>>>>> "Lee" == Lee Duncan <lduncan@suse.com> writes:
>
> Lee> Do you need me to resubmit this patch now that it's accepted?
>
> Please resend.
>
> Thanks!
>

Done, submitted against scsi tree, misc branch.
-- 
Lee Duncan