[PATCHv2 0/1] Update SCSI hosts to use idr for host number mgmt

[PATCHv2 0/1] Update SCSI hosts to use idr for host number mgmt

[Lee Duncan]

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

Lee Duncan (1):
  SCSI: update hosts module to use idr index management

 drivers/scsi/hosts.c | 60 +++++++++++++++++++++++++---------------------------
 1 file changed, 29 insertions(+), 31 deletions(-)

-- 
2.1.4

[PATCHv2 1/1] SCSI: update hosts module to use idr index management

[Lee Duncan]

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.

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

diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
index 8bb173e01084..9dc8ff971f5a 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>
@@ -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,37 +535,21 @@ 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
  *
  * Return value:
  *	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().
  **/
 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);
-	}
+	struct Scsi_Host *shost;
+
+	spin_lock(&host_index_lock);
+	shost = idr_find(&host_index_idr, hostnum);
+	spin_unlock(&host_index_lock);
+
 	return shost;
 }
 EXPORT_SYMBOL(scsi_host_lookup);
@@ -588,6 +585,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)
-- 
2.1.4

Re: [PATCHv2 1/1] SCSI: update hosts module to use idr index management

[Christoph Hellwig]

>  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);
> -	}
> +	struct Scsi_Host *shost;
> +
> +	spin_lock(&host_index_lock);
> +	shost = idr_find(&host_index_idr, hostnum);
> +	spin_unlock(&host_index_lock);
> +
>  	return shost;

How does this actually grab a reference to the host?

Re: [PATCHv2 1/1] SCSI: update hosts module to use idr index management

[Hannes Reinecke]

On 10/06/2015 01:01 AM, Lee Duncan wrote:
> 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.
>
> Signed-off-by: Lee Duncan <lduncan@suse.com>
> ---
>   drivers/scsi/hosts.c | 60 +++++++++++++++++++++++++---------------------------
>   1 file changed, 29 insertions(+), 31 deletions(-)
>
Very nice.

Reviewed-by: Hannes Reinecke <hare@suse.com>

Cheers,

Hannes

Re: [PATCHv2 1/1] SCSI: update hosts module to use idr index management

[Lee Duncan]

On 10/06/2015 02:40 AM, Christoph Hellwig wrote:
>>  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);
>> -	}
>> +	struct Scsi_Host *shost;
>> +
>> +	spin_lock(&host_index_lock);
>> +	shost = idr_find(&host_index_idr, hostnum);
>> +	spin_unlock(&host_index_lock);
>> +
>>  	return shost;
> 
> How does this actually grab a reference to the host?

Good catch -- I should have noticed that.

I will resubmit the patch.
-- 
Lee Duncan

Re: [PATCHv2 1/1] SCSI: update hosts module to use idr index management

[James Bottomley]

On Tue, 2015-10-06 at 10:14 -0700, Lee Duncan wrote:
> On 10/06/2015 02:40 AM, Christoph Hellwig wrote:
> >>  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);
> >> -	}
> >> +	struct Scsi_Host *shost;
> >> +
> >> +	spin_lock(&host_index_lock);
> >> +	shost = idr_find(&host_index_idr, hostnum);
> >> +	spin_unlock(&host_index_lock);
> >> +
> >>  	return shost;
> > 
> > How does this actually grab a reference to the host?
> 
> Good catch -- I should have noticed that.
> 
> I will resubmit the patch.

I'll wait to see what you produce, but I don't think, using a separate
idr array, you can close the race window between lookup and get.  One of
the nice things about using the cdev iterator is that the get is part of
the lookup process.

James

Re: [PATCHv2 1/1] SCSI: update hosts module to use idr index management

[Hannes Reinecke]

On 10/06/2015 07:17 PM, James Bottomley wrote:
> On Tue, 2015-10-06 at 10:14 -0700, Lee Duncan wrote:
>> On 10/06/2015 02:40 AM, Christoph Hellwig wrote:
>>>>   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);
>>>> -	}
>>>> +	struct Scsi_Host *shost;
>>>> +
>>>> +	spin_lock(&host_index_lock);
>>>> +	shost = idr_find(&host_index_idr, hostnum);
>>>> +	spin_unlock(&host_index_lock);
>>>> +
>>>>   	return shost;
>>>
>>> How does this actually grab a reference to the host?
>>
>> Good catch -- I should have noticed that.
>>
>> I will resubmit the patch.
>
> I'll wait to see what you produce, but I don't think, using a separate
> idr array, you can close the race window between lookup and get.  One of
> the nice things about using the cdev iterator is that the get is part of
> the lookup process.
>
Hmm. Should be possible if you free up the IDR in scsi_remove_host(), 
just after setting the state to SHOST_DEL.

Cheers,

Hannes

Re: [PATCHv2 1/1] SCSI: update hosts module to use idr index management

[James Bottomley]

On Wed, 2015-10-07 at 22:23 +0200, Hannes Reinecke wrote:
> On 10/06/2015 07:17 PM, James Bottomley wrote:
> > On Tue, 2015-10-06 at 10:14 -0700, Lee Duncan wrote:
> >> On 10/06/2015 02:40 AM, Christoph Hellwig wrote:
> >>>>   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);
> >>>> -	}
> >>>> +	struct Scsi_Host *shost;
> >>>> +
> >>>> +	spin_lock(&host_index_lock);
> >>>> +	shost = idr_find(&host_index_idr, hostnum);
> >>>> +	spin_unlock(&host_index_lock);
> >>>> +
> >>>>   	return shost;
> >>>
> >>> How does this actually grab a reference to the host?
> >>
> >> Good catch -- I should have noticed that.
> >>
> >> I will resubmit the patch.
> >
> > I'll wait to see what you produce, but I don't think, using a separate
> > idr array, you can close the race window between lookup and get.  One of
> > the nice things about using the cdev iterator is that the get is part of
> > the lookup process.
> >
> Hmm. Should be possible if you free up the IDR in scsi_remove_host(), 
> just after setting the state to SHOST_DEL.

Then all lookups fail between _del and _release which is different
behaviour from current.  In theory that's a very short window because we
should have removed all the devices synchronously in _del, so I don't
think there's any adverse consequences but it's something that would
need investigating.

To be honest, with the host number patch, I'm starting to come to the
conclusion that if it isn't broken, don't fix it because of the risks.
What are we trying to fix, anyway?  The host number wrapping at 32 bits?

James