Update SCSI hosts to use idr for host number mgmt
v3 cover Update SCSI hosts to use idr for host number mgmt
This patch updates the SCSI hosts module to use the idr 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.
It also updates the hosts module to use the idr routine idr_find() to lookup hosts based on the host number, hopefully speeding up said lookup.
After noticing that my idr calling sequences where very close to those in other modules, I considered creating some idr helper functions (and using them), but because idr usage almost always requires the caller to manage their own locks, I gave up on this approach (as suggested by Tejon -- thank you).
Changes from v1:
- no longer using helper routines
Changes from v2:
- added back missing scsi_host_get() in scsi_host_lookup()
Lee Duncan (1):
SCSI: update hosts module to use idr index management
drivers/scsi/hosts.c | 61 ++++++++++++++++++++++++++-------------------------- 1 file changed, 31 insertions(+), 30 deletions(-)
Signature · 1 line
2.1.4
PATCH v3 1/1 SCSI: update hosts module to use idr index management
Update the SCSI hosts module to use idr to manage its host_no index instead of an ATOMIC integer. This also allows using idr_find() to look up the SCSI host structure given the host number.
This means that the SCSI host number will now be reclaimable.
--- drivers/scsi/hosts.c | 61 ++++++++++++++++++++++++++-------------------------- 1 file changed, 31 insertions(+), 30 deletions(-)
drivers/scsi/hosts.c +31 -30
@@ -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>
@@ -41,8 +41,8 @@#include "scsi_priv.h" #include "scsi_logging.h" - -static atomic_t scsi_host_next_hn =ATOMIC_INIT (0 );/* host_no for next new host */ +static DEFINE_IDR (host_index_idr); +static DEFINE_SPINLOCK (host_index_lock);static void scsi_host_cls_release (struct device *dev)
@@ -337,6 +337,10 @@ static void scsi_host_dev_release(struct device *dev)kfree (shost->shost_data); +spin_lock (&host_index_lock); +idr_remove (&host_index_idr, shost->host_no); +spin_unlock (&host_index_lock); +if (parent)put_device (parent);kfree (shost);
@@ -370,6 +374,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 +393,15 @@ 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 ; +idr_preload (GFP_KERNEL ); +spin_lock (&host_index_lock); + index =idr_alloc (&host_index_idr, shost,0 ,0 ,GFP_NOWAIT ); +spin_unlock (&host_index_lock); +idr_preload_end (); +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 +486,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_idr_remove; } shost->tmf_work_q = alloc_workqueue("scsi_tmf_%d",
@@ -493,6 +502,10 @@ struct Scsi_Host *scsi_host_alloc(struct scsi_host_template *sht, int privsize) fail_kthread:kthread_stop (shost->ehandler); + fail_idr_remove: +spin_lock (&host_index_lock); +idr_remove (&host_index_idr, shost->host_no); +spin_unlock (&host_index_lock); fail_kfree:kfree (shost);return NULL ;
@@ -522,15 +535,6 @@ void scsi_unregister(struct Scsi_Host *shost) }EXPORT_SYMBOL (scsi_unregister); -static int __scsi_host_match (struct device *dev,const void *data) -{ -struct Scsi_Host *p; -const unsigned short *hostnum = data; - - p =class_to_shost (dev); -return p->host_no == *hostnum; -} -/** * scsi_host_lookup - get a reference to a Scsi_Host by host no * @hostnum: host number to locate
@@ -539,21 +543,17 @@ static int __scsi_host_match(struct device *dev, const void *data)* A pointer to located Scsi_Host or NULL. * * The caller must do a scsi_host_put() to drop the reference -* that scsi_host_get() took. The put_device() below dropped -* the reference from class_find_device(). +* that scsi_host_get() takes. **/ struct Scsi_Host *scsi_host_lookup (unsigned short hostnum) { -struct device *cdev; -struct Scsi_Host *shost =NULL ; - - cdev =class_find_device (&shost_class,NULL , &hostnum, - __scsi_host_match); -if (cdev) { - shost =scsi_host_get (class_to_shost (cdev)); -put_device (cdev); - } -return shost; +struct Scsi_Host *shost; + +spin_lock (&host_index_lock); + shost =idr_find (&host_index_idr, hostnum); +spin_unlock (&host_index_lock); + +return shost ?scsi_host_get (shost) :NULL ; }EXPORT_SYMBOL (scsi_host_lookup);
@@ -588,6 +588,7 @@ int scsi_init_hosts(void)void scsi_exit_hosts (void ) {class_unregister (&shost_class); +idr_destroy (&host_index_idr); }int scsi_is_host_device (const struct device *dev)
Signature · 1 line
2.1.4
James Bottomley
maintainerRe: [PATCHv3 1/1] SCSI: update hosts module to use idr index management
Quoted 18 lines · Lee Duncan
Update the SCSI hosts module to use idr to manage its
host_noindex instead of an ATOMIC integer. This also allows usingidr_find()to look up the SCSI host structure given the host number.This means that the SCSI host number will now be reclaimable.
Signed-off-by Lee Duncan <[email protected]>Reviewed-by Hannes Reinecke <[email protected]>--- drivers/scsi/hosts.c | 61 ++++++++++++++++++++++++++-------------------------- 1 file changed, 31 insertions(+), 30 deletions(-drivers/scsi/hosts.c +0
-0
[...]
drivers/scsi/hosts.c static int __scsi_host_match(struct device *dev, const void *data) · line 556
Quoted 5 lines
drivers/scsi/hosts.c +5
-0+spin_lock (&host_index_lock); + shost =idr_find (&host_index_idr, hostnum); +spin_unlock (&host_index_lock); + +return shost ?scsi_host_get (shost) :NULL ;
So the thing I don't like here is that there's a race between scsi_host_get() and the final put. What could happen is that idr_find() returns the host just before but scsi_host_dev_release() is executed before the return. In that instance, we'll reference freed memory in scsi_host_get() ... probably completely harmlessly, but it will show up occasionally on some of the traces ... particularly the ones doing a fuzz/stress test around host create/destroy.
James
Re: [PATCHv3 1/1] SCSI: update hosts module to use idr index management
Quoted 34 lines · James Bottomley
Quoted 18 lines · Lee Duncan
Update the SCSI hosts module to use idr to manage its
host_noindex instead of an ATOMIC integer. This also allows usingidr_find()to look up the SCSI host structure given the host number.This means that the SCSI host number will now be reclaimable.
Signed-off-by Lee Duncan <[email protected]>Reviewed-by Hannes Reinecke <[email protected]>--- drivers/scsi/hosts.c | 61 ++++++++++++++++++++++++++-------------------------- 1 file changed, 31 insertions(+), 30 deletions(-drivers/scsi/hosts.c +0
-0[...]
drivers/scsi/hosts.c static int __scsi_host_match(struct device *dev, const void *data) · line 556
Quoted 5 lines
drivers/scsi/hosts.c +5
-0+spin_lock (&host_index_lock); + shost =idr_find (&host_index_idr, hostnum); +spin_unlock (&host_index_lock); + +return shost ?scsi_host_get (shost) :NULL ;So the thing I don't like here is that there's a race between
scsi_host_get()and the final put. What could happen is thatidr_find()returns the host just before butscsi_host_dev_release()is executed before the return. In that instance, we'll reference freed memory inscsi_host_get()... probably completely harmlessly, but it will show up occasionally on some of the traces ... particularly the ones doing a fuzz/stress test around host create/destroy.
Quoted 1 line
James
Good point. The scenario you mention may be a corner case, but I also don't like it. I cannot see another good way to synchronize lookup and removal other than adding a new lock, which would be dumb.
I will submit my "host number" patch again, without the change to scsi_host_lookup().
Signature · 1 line
Lee Duncan
Duplicate email, please ignore
Quoted 26 lines · Lee Duncan
This patch updates the SCSI hosts module to use the idr index-management routines to manage its
host_noindex instead of using an ATOMIC integer. This means that host numbers can now be reclaimed and re-used.It also updates the hosts module to use the idr routine
idr_find()to lookup hosts based on the host number, hopefully speeding up said lookup.After noticing that my idr calling sequences where very close to those in other modules, I considered creating some idr helper functions (and using them), but because idr usage almost always requires the caller to manage their own locks, I gave up on this approach (as suggested by Tejon -- thank you).
Changes from v1:
- no longer using helper routines
Changes from v2:
- added back missingscsi_host_get()inscsi_host_lookup()Lee Duncan (1):
SCSI: update hosts module to use idr index managementdrivers/scsi/hosts.c | 61 ++++++++++++++++++++++++++-------------------------- 1 file changed, 31 insertions(+), 30 deletions(-)
Signature · 2 lines
Lee Duncan SUSE Labs