[PATCH 0/1] Update scsi hosts to use idr for host number mgmt

[PATCH 0/1] Update scsi hosts to use idr for host number mgmt

[Lee Duncan]

This is an updated version of a patch I submitted a few days ago.

The previous versions of this patch updated the scsi hosts module
to use the ida helper routines to allocate host numbers, instead
of using a simple integer.

Based on a suggestion by Christoph Hellwig, I decided to use the
idr helper routines instead of the ida set, since the idr functions
can also associate a pointer with an index.

This patch uses idr to manage the host number index as well
as to simplify looking up a host based on that index.

No helper routines were supplied/needed since the users
of these APIs do not seem to have uniform calling sequences.

A separate patch sequence follows which will add helper routines
for the ida index functions.

Lee Duncan (1):
  Update scsi hosts to use idr for host number mgmt

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

-- 
2.1.4

[PATCH 1/1] Update scsi hosts to use idr for host number mgmt

[Lee Duncan]

Each Scsi_host instance gets a host number starting
at 0, but this was implemented with an atomic integer,
and rollover wasn't considered. Another problem with
this design is that scsi host numbers used by iscsi
are never reused, thereby making rollover more likely.
This patch converts Scsi_host instances to use idr
to manage their instance numbers and to simplify
instance number to pointer lookups.

This also means that host instance numbers will be
reused, when available.

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

diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
index 8bb173e01084..1127a50e5942 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_SPINLOCK(host_index_lock);
+static DEFINE_IDR(host_index_idr);
 
 
 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 idr_ret;
 
 	if (sht->unchecked_isa_dma && privsize)
 		gfp_mask |= __GFP_DMA;
@@ -388,11 +393,14 @@ 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);
+	idr_ret = idr_alloc(&host_index_idr, shost, 0, 0, GFP_NOWAIT);
+	spin_unlock(&host_index_lock);
+	idr_preload_end();
+	if (idr_ret < 0)
+		goto fail_kfree;
+	shost->host_no = idr_ret;
 	shost->dma_channel = 0xff;
 
 	/* These three are default values which can be overridden */
@@ -477,7 +485,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 +501,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 +534,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 +584,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: [PATCH 1/1] Update scsi hosts to use idr for host number mgmt

[Sagi Grimberg]

On 9/5/2015 11:44 PM, Lee Duncan wrote:
> Each Scsi_host instance gets a host number starting
> at 0, but this was implemented with an atomic integer,
> and rollover wasn't considered. Another problem with
> this design is that scsi host numbers used by iscsi
> are never reused, thereby making rollover more likely.
> This patch converts Scsi_host instances to use idr
> to manage their instance numbers and to simplify
> instance number to pointer lookups.
>
> This also means that host instance numbers will be
> reused, when available.
>
> Signed-off-by: Lee Duncan <lduncan@suse.com>
> ---
>   drivers/scsi/hosts.c | 59 +++++++++++++++++++++++++---------------------------
>   1 file changed, 28 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
> index 8bb173e01084..1127a50e5942 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_SPINLOCK(host_index_lock);
> +static DEFINE_IDR(host_index_idr);
>
>
>   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);
> +

Did you change your mind on having host_[get|put]_idx() helpers?

Re: [PATCH 1/1] Update scsi hosts to use idr for host number mgmt

[Lee Duncan]

On 09/06/2015 12:34 AM, Sagi Grimberg wrote:
> On 9/5/2015 11:44 PM, Lee Duncan wrote:
>> Each Scsi_host instance gets a host number starting
>> at 0, but this was implemented with an atomic integer,
>> and rollover wasn't considered. Another problem with
>> this design is that scsi host numbers used by iscsi
>> are never reused, thereby making rollover more likely.
>> This patch converts Scsi_host instances to use idr
>> to manage their instance numbers and to simplify
>> instance number to pointer lookups.
>>
>> This also means that host instance numbers will be
>> reused, when available.
>>
>> Signed-off-by: Lee Duncan <lduncan@suse.com>
>> ---
>>   drivers/scsi/hosts.c | 59
>> +++++++++++++++++++++++++---------------------------
>>   1 file changed, 28 insertions(+), 31 deletions(-)
>>
>> diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
>> index 8bb173e01084..1127a50e5942 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_SPINLOCK(host_index_lock);
>> +static DEFINE_IDR(host_index_idr);
>>
>>
>>   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);
>> +
> 
> Did you change your mind on having host_[get|put]_idx() helpers?

No. As I said on the description:

> A separate patch sequence follows which will add helper routines
> for the ida index functions.

I'll be sending out that patch series today (I hope).

I *do* believe it would be useful to add some ida helper routines, since
callers of these routines seem mostly to use a uniform calling sequence.
But even so some of the callers do things differently enough so that I
was not comfortable changing them to use the helper routines.

But the "idr" routines, i.e. the ones that manage both an index *and* a
pointer, which are the ones I'm using in hosts.c, are not called so
uniformly, so helper routines did not seem like a good idea.
-- 
Lee Duncan

Re: [PATCH 1/1] Update scsi hosts to use idr for host number mgmt

[Lee Duncan]

On 09/06/2015 08:16 AM, Lee Duncan wrote:
> On 09/06/2015 12:34 AM, Sagi Grimberg wrote:
>> On 9/5/2015 11:44 PM, Lee Duncan wrote:
>>> Each Scsi_host instance gets a host number starting
>>> at 0, but this was implemented with an atomic integer,
>>> and rollover wasn't considered. Another problem with
>>> this design is that scsi host numbers used by iscsi
>>> are never reused, thereby making rollover more likely.
>>> This patch converts Scsi_host instances to use idr
>>> to manage their instance numbers and to simplify
>>> instance number to pointer lookups.
>>>
>>> This also means that host instance numbers will be
>>> reused, when available.
>>>
>>> Signed-off-by: Lee Duncan <lduncan@suse.com>
>>> ---
>>>   drivers/scsi/hosts.c | 59
>>> +++++++++++++++++++++++++---------------------------
>>>   1 file changed, 28 insertions(+), 31 deletions(-)
>>>
>>> diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
>>> index 8bb173e01084..1127a50e5942 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_SPINLOCK(host_index_lock);
>>> +static DEFINE_IDR(host_index_idr);
>>>
>>>
>>>   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);
>>> +
>>
>> Did you change your mind on having host_[get|put]_idx() helpers?
> 

Sagi: I may have answered you incorrectly.

Yes, when I switched from using ida routines to using idr, I dropped the
helper functions. But I think you are right, the code is cleaner with
them present, so I will resubmit.

> No. As I said on the description:
> 
>> A separate patch sequence follows which will add helper routines
>> for the ida index functions.
> 
> I'll be sending out that patch series today (I hope).
> 
> I *do* believe it would be useful to add some ida helper routines, since
> callers of these routines seem mostly to use a uniform calling sequence.
> But even so some of the callers do things differently enough so that I
> was not comfortable changing them to use the helper routines.
> 
> But the "idr" routines, i.e. the ones that manage both an index *and* a
> pointer, which are the ones I'm using in hosts.c, are not called so
> uniformly, so helper routines did not seem like a good idea.
> 

-- 
Lee Duncan
SUSE Labs

Re: [PATCH 0/1] Update scsi hosts to use idr for host number mgmt

[Sagi Grimberg]

On 9/5/2015 11:44 PM, Lee Duncan wrote:
> This is an updated version of a patch I submitted a few days ago.
>
> The previous versions of this patch updated the scsi hosts module
> to use the ida helper routines to allocate host numbers, instead
> of using a simple integer.
>
> Based on a suggestion by Christoph Hellwig, I decided to use the
> idr helper routines instead of the ida set, since the idr functions
> can also associate a pointer with an index.

While we're at it, why not include sd index conversion?

Re: [PATCH 0/1] Update scsi hosts to use idr for host number mgmt

[Lee Duncan]

On 09/06/2015 12:32 AM, Sagi Grimberg wrote:
> On 9/5/2015 11:44 PM, Lee Duncan wrote:
>> This is an updated version of a patch I submitted a few days ago.
>>
>> The previous versions of this patch updated the scsi hosts module
>> to use the ida helper routines to allocate host numbers, instead
>> of using a simple integer.
>>
>> Based on a suggestion by Christoph Hellwig, I decided to use the
>> idr helper routines instead of the ida set, since the idr functions
>> can also associate a pointer with an index.
> 
> While we're at it, why not include sd index conversion?
> 

I'm not sure what you mean here.

Are you suggesting sd.c be updated to use idr index routines instead of
ida routines so that sd.c could associate instance pointers with
instance numbers, much like I did for hosts.c?

If so, looking over sd.c, I don't think that would be feasible. The sd.c
driver does not do any searches for a pointer based on the index number.
Could you be more specific?

And do you have any objections to this patch?
-- 
Lee Duncan