Update SCSI hosts to use ida for host number mgmt

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 <[email protected]>

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

drivers/scsi/hosts.c +1 -2

index 8bb173e01084..b6a5ffa886b7 100644
@@ -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 */

drivers/scsi/hosts.c +3 -0

+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);

drivers/scsi/hosts.c +1 -0

 	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)

drivers/scsi/hosts.c +0 -2

 		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

drivers/scsi/hosts.c +6 -4

-	 * 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;
 	}

drivers/scsi/hosts.c +2 -0

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

drivers/scsi/hosts.c +0 -0

 	return NULL;

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.

Quoted 3 lines · 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.

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

Quoted 13 lines · James Bottomley

Quoted 3 lines · 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.

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.

Quoted 2 lines

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.

Quoted 27 lines · Lee Duncan

Quoted 13 lines · James Bottomley

Quoted 3 lines · 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.

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.

Quoted 2 lines

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

Quoted 1 line

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.

Quoted 2 lines

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

Quoted 27 lines · Lee Duncan

Quoted 13 lines · James Bottomley

Quoted 3 lines · 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.

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.

Quoted 2 lines

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

Quoted 1 line

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.

Quoted 2 lines

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.

Quoted 27 lines · Lee Duncan

Quoted 13 lines · James Bottomley

Quoted 3 lines · 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.

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.

Quoted 2 lines

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

Quoted 1 line

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.

Quoted 2 lines

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

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.

Quoted 44 lines · James Bottomley

Quoted 27 lines · Lee Duncan

Quoted 13 lines · James Bottomley

Quoted 3 lines · 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.

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.

Quoted 2 lines

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

Quoted 1 line

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.

Quoted 2 lines

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.

Quoted 27 lines · Lee Duncan

Quoted 13 lines · James Bottomley

Quoted 3 lines · 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.

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.

Quoted 2 lines

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

Quoted 1 line

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.

Quoted 2 lines

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

Quoted 6 lines

Quoted 1 line

Quoted 1 line

Quoted 1 line

"Lee" == Lee Duncan <[email protected]> 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.

Quoted 6 lines

Quoted 6 lines

Quoted 1 line

Quoted 1 line

Quoted 1 line

"Lee" == Lee Duncan <[email protected]> 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.

Quoted 10 lines · Martin K. Petersen

Quoted 6 lines

Quoted 6 lines

Quoted 1 line

Quoted 1 line

Quoted 1 line

"Lee" == Lee Duncan <[email protected]> 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

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

"Lee" == Lee Duncan <[email protected]> writes:

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

"Lee" == Lee Duncan <[email protected]> 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?

Quoted 5 lines · Martin K. Petersen

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

"Lee" == Lee Duncan <[email protected]> writes:

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

Quoted 6 lines

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?

Quoted 6 lines · Lee Duncan

Quoted 5 lines · Martin K. Petersen

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

"Lee" == Lee Duncan <[email protected]> writes:

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

Quoted 8 lines

Quoted 6 lines

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

Quoted 25 lines · Ewan Milne

Quoted 6 lines · Lee Duncan

Quoted 5 lines · Martin K. Petersen

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

"Lee" == Lee Duncan <[email protected]> writes:

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

Quoted 8 lines

Quoted 6 lines

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.

Quoted 6 lines

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.

Quoted 35 lines · Lee Duncan

Quoted 25 lines · Ewan Milne

Quoted 6 lines · Lee Duncan

Quoted 5 lines · Martin K. Petersen

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

"Lee" == Lee Duncan <[email protected]> writes:

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

Quoted 8 lines

Quoted 6 lines

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.

Quoted 6 lines

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:

Quoted 8 lines

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.

Quoted 6 lines

Quoted 1 line

Quoted 1 line

Quoted 1 line

"Hannes" == Hannes Reinecke <[email protected]> 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.

Quoted 6 lines

Quoted 6 lines

Quoted 1 line

Quoted 1 line

Quoted 1 line

"Hannes" == Hannes Reinecke <[email protected]> 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.

Quoted 15 lines · Martin K. Petersen

Quoted 6 lines

Quoted 6 lines

Quoted 1 line

Quoted 1 line

Quoted 1 line

"Hannes" == Hannes Reinecke <[email protected]> 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.

Quoted 3 lines

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.

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

"Lee" == Lee Duncan <[email protected]> writes:

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

"Lee" == Lee Duncan <[email protected]> writes:

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

Please resend.

Thanks!