| Message ID | f89fe3933ae899dc01f2eddd2a0cefac3383cf3c.1444241058.git.lduncan@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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> -- 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
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 -- 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
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.
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 -- 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
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.
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
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 > > > >
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 -- 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
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" == 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.
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
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" == 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?
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?
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 -- 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
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.
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 -- 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
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
>>>>> "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.
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.
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" == Lee Duncan <lduncan@suse.com> writes:
Lee> Do you need me to resubmit this patch now that it's accepted?
Please resend.
Thanks!
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.
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)
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(-)