[RFC PATCH 0/4] scsi: use xarray for devices and targets

[RFC PATCH 0/4] scsi: use xarray for devices and targets

[Hannes Reinecke]

Hi all,

based on the ideas from Doug Gilbert here's now my take on using
xarrays for devices and targets.
It revolves around two ideas:
- 'channel' and 'id' are never ever used to the full 32 bit range;
  'channels' are well below 10, and no driver is using more than
  16 bits for the id. So we can reduce the type of 'channel' and
  'id' to 16 bits, and use the 32 bit value 'channel << 16 | id'
  as the index into the target xarray.
- Most SCSI LUNs are below 256 (to ensure compability with older
  systems). So there we can use the LUN number as the index into
  the xarray; for larger LUN numbers we'll allocate a separate
  index.

With these change we can implement an efficient lookup mechanism,
devolving into direct lookup for most cases.
And iteration should be as efficient as the current, list-based,
approach.

This is compile-tested only, to give you an impression of the
overall idea and to get the discussion rolling.

Hannes Reinecke (4):
  scsi: convert target lookup to xarray
  target_core_pscsi: use __scsi_device_lookup()
  scsi: move target device list to xarray
  scsi: remove direct device lookup per host

 drivers/scsi/hosts.c               |   3 +-
 drivers/scsi/scsi.c                | 129 ++++++++++++++++++++++++++++---------
 drivers/scsi/scsi_lib.c            |   8 +--
 drivers/scsi/scsi_scan.c           |  66 +++++++++----------
 drivers/scsi/scsi_sysfs.c          |  39 +++++++----
 drivers/target/target_core_pscsi.c |   8 +--
 include/scsi/scsi_device.h         |  21 +++---
 include/scsi/scsi_host.h           |   5 +-
 8 files changed, 175 insertions(+), 104 deletions(-)

-- 
2.16.4

[PATCH 1/4] scsi: convert target lookup to xarray

[Hannes Reinecke]

Use an xarray instead of lists for holding the scsi targets.
I've also shortened the 'channel' and 'id' values to 16 bit
as none of the drivers requires a full 32bit range for either
of them, and by shortening them we can use them as the index
into the xarray for storing the scsi_target pointer.

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/scsi/hosts.c       |  2 +-
 drivers/scsi/scsi.c        | 35 ++++++++++++++++++++++++-----------
 drivers/scsi/scsi_scan.c   | 39 +++++++++++++--------------------------
 drivers/scsi/scsi_sysfs.c  | 15 +++++++++++----
 include/scsi/scsi_device.h |  4 ++--
 include/scsi/scsi_host.h   |  2 +-
 6 files changed, 52 insertions(+), 45 deletions(-)

diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
index 7ec91c3a66ca..7109afad0183 100644
--- a/drivers/scsi/hosts.c
+++ b/drivers/scsi/hosts.c
@@ -383,7 +383,7 @@ struct Scsi_Host *scsi_host_alloc(struct scsi_host_template *sht, int privsize)
 	spin_lock_init(shost->host_lock);
 	shost->shost_state = SHOST_CREATED;
 	INIT_LIST_HEAD(&shost->__devices);
-	INIT_LIST_HEAD(&shost->__targets);
+	xa_init(&shost->__targets);
 	INIT_LIST_HEAD(&shost->eh_cmd_q);
 	INIT_LIST_HEAD(&shost->starved_list);
 	init_waitqueue_head(&shost->host_wait);
diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index 56c24a73e0c7..25c815355d1a 100644
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -575,6 +575,19 @@ struct scsi_device *__scsi_iterate_devices(struct Scsi_Host *shost,
 }
 EXPORT_SYMBOL(__scsi_iterate_devices);
 
+/**
+ * __scsi_target_lookup  -  find a target based on channel and target id
+ * @shost:	SCSI host pointer
+ * @channel:	channel number of the target
+ * @id:		ID of the target
+ *
+ */
+static struct scsi_target *__scsi_target_lookup(struct Scsi_Host *shost,
+					 u16 channel, u16 id)
+{
+	return xa_load(&shost->__targets, (channel << 16) | id);
+}
+
 /**
  * starget_for_each_device  -  helper to walk all devices of a target
  * @starget:	target whose devices we want to iterate over.
@@ -670,8 +683,8 @@ EXPORT_SYMBOL(__scsi_device_lookup_by_target);
 struct scsi_device *scsi_device_lookup_by_target(struct scsi_target *starget,
 						 u64 lun)
 {
-	struct scsi_device *sdev;
 	struct Scsi_Host *shost = dev_to_shost(starget->dev.parent);
+	struct scsi_device *sdev;
 	unsigned long flags;
 
 	spin_lock_irqsave(shost->host_lock, flags);
@@ -701,19 +714,19 @@ EXPORT_SYMBOL(scsi_device_lookup_by_target);
  * really want to use scsi_device_lookup instead.
  **/
 struct scsi_device *__scsi_device_lookup(struct Scsi_Host *shost,
-		uint channel, uint id, u64 lun)
+		u16 channel, u16 id, u64 lun)
 {
+	struct scsi_target *starget;
 	struct scsi_device *sdev;
 
-	list_for_each_entry(sdev, &shost->__devices, siblings) {
-		if (sdev->sdev_state == SDEV_DEL)
-			continue;
-		if (sdev->channel == channel && sdev->id == id &&
-				sdev->lun ==lun)
-			return sdev;
-	}
+	starget = __scsi_target_lookup(shost, channel, id);
+	if (!starget)
+		return NULL;
+	sdev = __scsi_device_lookup_by_target(starget, lun);
+	if (sdev && sdev->sdev_state == SDEV_DEL)
+		sdev = NULL;
 
-	return NULL;
+	return sdev;
 }
 EXPORT_SYMBOL(__scsi_device_lookup);
 
@@ -729,7 +742,7 @@ EXPORT_SYMBOL(__scsi_device_lookup);
  * needs to be released with scsi_device_put once you're done with it.
  **/
 struct scsi_device *scsi_device_lookup(struct Scsi_Host *shost,
-		uint channel, uint id, u64 lun)
+		u16 channel, u16 id, u64 lun)
 {
 	struct scsi_device *sdev;
 	unsigned long flags;
diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
index f2437a7570ce..c47cddef1839 100644
--- a/drivers/scsi/scsi_scan.c
+++ b/drivers/scsi/scsi_scan.c
@@ -304,11 +304,15 @@ static struct scsi_device *scsi_alloc_sdev(struct scsi_target *starget,
 	return NULL;
 }
 
+#define scsi_target_index(s) \
+	((((unsigned long)(s)->channel) << 16) | (s)->id)
+
 static void scsi_target_destroy(struct scsi_target *starget)
 {
 	struct device *dev = &starget->dev;
 	struct Scsi_Host *shost = dev_to_shost(dev->parent);
 	unsigned long flags;
+	unsigned long tid = scsi_target_index(starget);
 
 	BUG_ON(starget->state == STARGET_DEL);
 	starget->state = STARGET_DEL;
@@ -316,7 +320,7 @@ static void scsi_target_destroy(struct scsi_target *starget)
 	spin_lock_irqsave(shost->host_lock, flags);
 	if (shost->hostt->target_destroy)
 		shost->hostt->target_destroy(starget);
-	list_del_init(&starget->siblings);
+	xa_erase(&shost->__targets, tid);
 	spin_unlock_irqrestore(shost->host_lock, flags);
 	put_device(dev);
 }
@@ -341,27 +345,6 @@ int scsi_is_target_device(const struct device *dev)
 }
 EXPORT_SYMBOL(scsi_is_target_device);
 
-static struct scsi_target *__scsi_find_target(struct device *parent,
-					      int channel, uint id)
-{
-	struct scsi_target *starget, *found_starget = NULL;
-	struct Scsi_Host *shost = dev_to_shost(parent);
-	/*
-	 * Search for an existing target for this sdev.
-	 */
-	list_for_each_entry(starget, &shost->__targets, siblings) {
-		if (starget->id == id &&
-		    starget->channel == channel) {
-			found_starget = starget;
-			break;
-		}
-	}
-	if (found_starget)
-		get_device(&found_starget->dev);
-
-	return found_starget;
-}
-
 /**
  * scsi_target_reap_ref_release - remove target from visibility
  * @kref: the reap_ref in the target being released
@@ -417,6 +400,7 @@ static struct scsi_target *scsi_alloc_target(struct device *parent,
 	struct scsi_target *starget;
 	struct scsi_target *found_target;
 	int error, ref_got;
+	unsigned long tid;
 
 	starget = kzalloc(size, GFP_KERNEL);
 	if (!starget) {
@@ -433,19 +417,22 @@ static struct scsi_target *scsi_alloc_target(struct device *parent,
 	starget->id = id;
 	starget->channel = channel;
 	starget->can_queue = 0;
-	INIT_LIST_HEAD(&starget->siblings);
 	INIT_LIST_HEAD(&starget->devices);
 	starget->state = STARGET_CREATED;
 	starget->scsi_level = SCSI_2;
 	starget->max_target_blocked = SCSI_DEFAULT_TARGET_BLOCKED;
+	tid = scsi_target_index(starget);
  retry:
 	spin_lock_irqsave(shost->host_lock, flags);
 
-	found_target = __scsi_find_target(parent, channel, id);
+	found_target = xa_load(&shost->__targets, tid);
 	if (found_target)
 		goto found;
-
-	list_add_tail(&starget->siblings, &shost->__targets);
+	if (xa_insert(&shost->__targets, tid, starget, GFP_KERNEL)) {
+		dev_printk(KERN_ERR, dev, "target index busy\n");
+		kfree(starget);
+		return NULL;
+	}
 	spin_unlock_irqrestore(shost->host_lock, flags);
 	/* allocate and add */
 	transport_setup_device(dev);
diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
index 163dbcb741c1..95aaa96ce03b 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -1512,15 +1512,19 @@ void scsi_remove_target(struct device *dev)
 {
 	struct Scsi_Host *shost = dev_to_shost(dev->parent);
 	struct scsi_target *starget;
+	unsigned long tid = 0;
 	unsigned long flags;
 
-restart:
 	spin_lock_irqsave(shost->host_lock, flags);
-	list_for_each_entry(starget, &shost->__targets, siblings) {
+	starget = xa_find(&shost->__targets, &tid, ULONG_MAX, XA_PRESENT);
+	while (starget) {
 		if (starget->state == STARGET_DEL ||
 		    starget->state == STARGET_REMOVE ||
-		    starget->state == STARGET_CREATED_REMOVE)
+		    starget->state == STARGET_CREATED_REMOVE) {
+			starget = xa_find_after(&shost->__targets, &tid,
+						ULONG_MAX, XA_PRESENT);
 			continue;
+		}
 		if (starget->dev.parent == dev || &starget->dev == dev) {
 			kref_get(&starget->reap_ref);
 			if (starget->state == STARGET_CREATED)
@@ -1530,7 +1534,10 @@ void scsi_remove_target(struct device *dev)
 			spin_unlock_irqrestore(shost->host_lock, flags);
 			__scsi_remove_target(starget);
 			scsi_target_reap(starget);
-			goto restart;
+			spin_lock_irqsave(shost->host_lock, flags);
+			starget = xa_find_after(&shost->__targets, &tid,
+						ULONG_MAX, XA_PRESENT);
+			continue;
 		}
 	}
 	spin_unlock_irqrestore(shost->host_lock, flags);
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index c3cba2aaf934..28034cc0fce5 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -345,9 +345,9 @@ extern struct scsi_device *scsi_device_from_queue(struct request_queue *q);
 extern int __must_check scsi_device_get(struct scsi_device *);
 extern void scsi_device_put(struct scsi_device *);
 extern struct scsi_device *scsi_device_lookup(struct Scsi_Host *,
-					      uint, uint, u64);
+					      u16, u16, u64);
 extern struct scsi_device *__scsi_device_lookup(struct Scsi_Host *,
-						uint, uint, u64);
+						u16, u16, u64);
 extern struct scsi_device *scsi_device_lookup_by_target(struct scsi_target *,
 							u64);
 extern struct scsi_device *__scsi_device_lookup_by_target(struct scsi_target *,
diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
index 822e8cda8d9b..b9395676c75b 100644
--- a/include/scsi/scsi_host.h
+++ b/include/scsi/scsi_host.h
@@ -521,7 +521,7 @@ struct Scsi_Host {
 	 * access this list directly from a driver.
 	 */
 	struct list_head	__devices;
-	struct list_head	__targets;
+	struct xarray		__targets;
 	
 	struct list_head	starved_list;
 
-- 
2.16.4

Re: [PATCH 1/4] scsi: convert target lookup to xarray

[Johannes Thumshirn]

On 27/05/2020 16:14, Hannes Reinecke wrote:
[...]
> @@ -670,8 +683,8 @@ EXPORT_SYMBOL(__scsi_device_lookup_by_target);
>  struct scsi_device *scsi_device_lookup_by_target(struct scsi_target *starget,
>  						 u64 lun)
>  {
> -	struct scsi_device *sdev;
>  	struct Scsi_Host *shost = dev_to_shost(starget->dev.parent);
> +	struct scsi_device *sdev;
>  	unsigned long flags;

This looks unrelated.

>  
>  	spin_lock_irqsave(shost->host_lock, flags);
> @@ -701,19 +714,19 @@ EXPORT_SYMBOL(scsi_device_lookup_by_target);
>   * really want to use scsi_device_lookup instead.
>   **/
>  struct scsi_device *__scsi_device_lookup(struct Scsi_Host *shost,
> -		uint channel, uint id, u64 lun)
> +		u16 channel, u16 id, u64 lun)
>  {
> +	struct scsi_target *starget;
>  	struct scsi_device *sdev;
>  
> -	list_for_each_entry(sdev, &shost->__devices, siblings) {
> -		if (sdev->sdev_state == SDEV_DEL)
> -			continue;
> -		if (sdev->channel == channel && sdev->id == id &&
> -				sdev->lun ==lun)
> -			return sdev;
> -	}
> +	starget = __scsi_target_lookup(shost, channel, id);
> +	if (!starget)
> +		return NULL;
> +	sdev = __scsi_device_lookup_by_target(starget, lun);
> +	if (sdev && sdev->sdev_state == SDEV_DEL)
> +		sdev = NULL;
>  

I think the above if is unneeded as __scsi_device_lookup_by_target() does:
	list_for_each_entry(sdev, &starget->devices, same_target_siblings) {
		if (sdev->sdev_state == SDEV_DEL)
			continue;

So 'sdev != NULL && sdev->sdev_state == SDEV_DEL' would never evaluate to true,
wouldn't it?

Re: [PATCH 1/4] scsi: convert target lookup to xarray

[Hannes Reinecke]

On 5/27/20 4:57 PM, Johannes Thumshirn wrote:
> On 27/05/2020 16:14, Hannes Reinecke wrote:
> [...]
>> @@ -670,8 +683,8 @@ EXPORT_SYMBOL(__scsi_device_lookup_by_target);
>>   struct scsi_device *scsi_device_lookup_by_target(struct scsi_target *starget,
>>   						 u64 lun)
>>   {
>> -	struct scsi_device *sdev;
>>   	struct Scsi_Host *shost = dev_to_shost(starget->dev.parent);
>> +	struct scsi_device *sdev;
>>   	unsigned long flags;
> 
> This looks unrelated.
> 
>>   
>>   	spin_lock_irqsave(shost->host_lock, flags);
>> @@ -701,19 +714,19 @@ EXPORT_SYMBOL(scsi_device_lookup_by_target);
>>    * really want to use scsi_device_lookup instead.
>>    **/
>>   struct scsi_device *__scsi_device_lookup(struct Scsi_Host *shost,
>> -		uint channel, uint id, u64 lun)
>> +		u16 channel, u16 id, u64 lun)
>>   {
>> +	struct scsi_target *starget;
>>   	struct scsi_device *sdev;
>>   
>> -	list_for_each_entry(sdev, &shost->__devices, siblings) {
>> -		if (sdev->sdev_state == SDEV_DEL)
>> -			continue;
>> -		if (sdev->channel == channel && sdev->id == id &&
>> -				sdev->lun ==lun)
>> -			return sdev;
>> -	}
>> +	starget = __scsi_target_lookup(shost, channel, id);
>> +	if (!starget)
>> +		return NULL;
>> +	sdev = __scsi_device_lookup_by_target(starget, lun);
>> +	if (sdev && sdev->sdev_state == SDEV_DEL)
>> +		sdev = NULL;
>>   
> 
> I think the above if is unneeded as __scsi_device_lookup_by_target() does:
> 	list_for_each_entry(sdev, &starget->devices, same_target_siblings) {
> 		if (sdev->sdev_state == SDEV_DEL)
> 			continue;
> 
> So 'sdev != NULL && sdev->sdev_state == SDEV_DEL' would never evaluate to true,
> wouldn't it?
> 
Oh. indeed. So that can be simplified.
Will do for the next version.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke            Teamlead Storage & Networking
hare@suse.de                               +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer

Re: [PATCH 1/4] scsi: convert target lookup to xarray

[Daniel Wagner]

On Wed, May 27, 2020 at 04:13:57PM +0200, Hannes Reinecke wrote:
> --- a/drivers/scsi/scsi_sysfs.c
> +++ b/drivers/scsi/scsi_sysfs.c
> @@ -1512,15 +1512,19 @@ void scsi_remove_target(struct device *dev)
>  {
>  	struct Scsi_Host *shost = dev_to_shost(dev->parent);
>  	struct scsi_target *starget;
> +	unsigned long tid = 0;
>  	unsigned long flags;
>  
> -restart:
>  	spin_lock_irqsave(shost->host_lock, flags);
> -	list_for_each_entry(starget, &shost->__targets, siblings) {
> +	starget = xa_find(&shost->__targets, &tid, ULONG_MAX, XA_PRESENT);
> +	while (starget) {
>  		if (starget->state == STARGET_DEL ||
>  		    starget->state == STARGET_REMOVE ||
> -		    starget->state == STARGET_CREATED_REMOVE)
> +		    starget->state == STARGET_CREATED_REMOVE) {
> +			starget = xa_find_after(&shost->__targets, &tid,
> +						ULONG_MAX, XA_PRESENT);
>  			continue;
> +		}
>  		if (starget->dev.parent == dev || &starget->dev == dev) {
>  			kref_get(&starget->reap_ref);
>  			if (starget->state == STARGET_CREATED)
> @@ -1530,7 +1534,10 @@ void scsi_remove_target(struct device *dev)
>  			spin_unlock_irqrestore(shost->host_lock, flags);
>  			__scsi_remove_target(starget);
>  			scsi_target_reap(starget);
> -			goto restart;
> +			spin_lock_irqsave(shost->host_lock, flags);
> +			starget = xa_find_after(&shost->__targets, &tid,
> +						ULONG_MAX, XA_PRESENT);
> +			continue;
>  		}

Is there a special reason why don't use xa_for_each to iterate through all
entries?

Re: [PATCH 1/4] scsi: convert target lookup to xarray

[Hannes Reinecke]

On 5/28/20 9:24 AM, Daniel Wagner wrote:
> On Wed, May 27, 2020 at 04:13:57PM +0200, Hannes Reinecke wrote:
>> --- a/drivers/scsi/scsi_sysfs.c
>> +++ b/drivers/scsi/scsi_sysfs.c
>> @@ -1512,15 +1512,19 @@ void scsi_remove_target(struct device *dev)
>>   {
>>   	struct Scsi_Host *shost = dev_to_shost(dev->parent);
>>   	struct scsi_target *starget;
>> +	unsigned long tid = 0;
>>   	unsigned long flags;
>>   
>> -restart:
>>   	spin_lock_irqsave(shost->host_lock, flags);
>> -	list_for_each_entry(starget, &shost->__targets, siblings) {
>> +	starget = xa_find(&shost->__targets, &tid, ULONG_MAX, XA_PRESENT);
>> +	while (starget) {
>>   		if (starget->state == STARGET_DEL ||
>>   		    starget->state == STARGET_REMOVE ||
>> -		    starget->state == STARGET_CREATED_REMOVE)
>> +		    starget->state == STARGET_CREATED_REMOVE) {
>> +			starget = xa_find_after(&shost->__targets, &tid,
>> +						ULONG_MAX, XA_PRESENT);
>>   			continue;
>> +		}
>>   		if (starget->dev.parent == dev || &starget->dev == dev) {
>>   			kref_get(&starget->reap_ref);
>>   			if (starget->state == STARGET_CREATED)
>> @@ -1530,7 +1534,10 @@ void scsi_remove_target(struct device *dev)
>>   			spin_unlock_irqrestore(shost->host_lock, flags);
>>   			__scsi_remove_target(starget);
>>   			scsi_target_reap(starget);
>> -			goto restart;
>> +			spin_lock_irqsave(shost->host_lock, flags);
>> +			starget = xa_find_after(&shost->__targets, &tid,
>> +						ULONG_MAX, XA_PRESENT);
>> +			continue;
>>   		}
> 
> Is there a special reason why don't use xa_for_each to iterate through all
> entries?
> 
This entire function is completely daft.
It says 'scsi_remove_target()', but for some weird reason fails to pass 
in the target it want to delete, so it has to go round in circles trying 
to figure out which target to delete.
There probably had been an obscure reason for this, but with the current 
code it's just pointless.
So that's the next thing to fix (after all of this): use a struct 
scsi_target as argument for this function, then this entire loop can go.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke            Teamlead Storage & Networking
hare@suse.de                               +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer

Re: [PATCH 1/4] scsi: convert target lookup to xarray

[Bart Van Assche]

On 2020-05-28 00:52, Hannes Reinecke wrote:
> On 5/28/20 9:24 AM, Daniel Wagner wrote:
>> Is there a special reason why don't use xa_for_each to iterate through
>> all
>> entries?
>>
> This entire function is completely daft.
> It says 'scsi_remove_target()', but for some weird reason fails to pass
> in the target it want to delete, so it has to go round in circles trying
> to figure out which target to delete.
> There probably had been an obscure reason for this, but with the current
> code it's just pointless.
> So that's the next thing to fix (after all of this): use a struct
> scsi_target as argument for this function, then this entire loop can go.

Please be careful when modifying this code. I remember that it took
multiple iterations to get this code right. See e.g. commit 81b6c9998979
("scsi: core: check for device state in __scsi_remove_target()"). See
also commit fbce4d97fd43 ("scsi: fixup kernel warning during rmmod()").
See also commit f9279c968c25 ("scsi: Add STARGET_CREATED_REMOVE state to
scsi_target_state").

The only reason I can think of why that loop exists is that @dev may be
associated with multiple targets (struct scsi_target) at the same time.
Is that something that can happen?

Thanks,

Bart.

Re: [PATCH 1/4] scsi: convert target lookup to xarray

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 11065 bytes --]

Hi Hannes,

I love your patch! Perhaps something to improve:

[auto build test WARNING on mkp-scsi/for-next]
[also build test WARNING on scsi/for-next v5.7-rc7 next-20200526]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Hannes-Reinecke/scsi-use-xarray-for-devices-and-targets/20200527-231824
base:   https://git.kernel.org/pub/scm/linux/kernel/git/mkp/scsi.git for-next
:::::: branch date: 12 hours ago
:::::: commit date: 12 hours ago
config: i386-randconfig-s001-20200528 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-13) 9.3.0
reproduce:
        # apt-get install sparse
        # sparse version: v0.6.1-240-gf0fe1cd9-dirty
        # save the attached .config to linux build tree
        make W=1 C=1 ARCH=i386 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__'

If you fix the issue, kindly add following tag as appropriate
Reported-by: kbuild test robot <lkp@intel.com>


sparse warnings: (new ones prefixed by >>)

>> drivers/scsi/scsi_scan.c:392:27: sparse: sparse: context imbalance in 'scsi_alloc_target' - different lock contexts for basic block

# https://github.com/0day-ci/linux/commit/45b149b239ea9a86968ddbd8ecda1e6c44937b68
git remote add linux-review https://github.com/0day-ci/linux
git remote update linux-review
git checkout 45b149b239ea9a86968ddbd8ecda1e6c44937b68
vim +/scsi_alloc_target +392 drivers/scsi/scsi_scan.c

e63ed0d7a98014 James Bottomley        2014-01-21  379  
884d25cc4fda20 James Bottomley        2006-09-05  380  /**
884d25cc4fda20 James Bottomley        2006-09-05  381   * scsi_alloc_target - allocate a new or find an existing target
884d25cc4fda20 James Bottomley        2006-09-05  382   * @parent:	parent of the target (need not be a scsi host)
884d25cc4fda20 James Bottomley        2006-09-05  383   * @channel:	target channel number (zero if no channels)
884d25cc4fda20 James Bottomley        2006-09-05  384   * @id:		target id number
884d25cc4fda20 James Bottomley        2006-09-05  385   *
884d25cc4fda20 James Bottomley        2006-09-05  386   * Return an existing target if one exists, provided it hasn't already
884d25cc4fda20 James Bottomley        2006-09-05  387   * gone into STARGET_DEL state, otherwise allocate a new target.
884d25cc4fda20 James Bottomley        2006-09-05  388   *
884d25cc4fda20 James Bottomley        2006-09-05  389   * The target is returned with an incremented reference, so the caller
884d25cc4fda20 James Bottomley        2006-09-05  390   * is responsible for both reaping and doing a last put
884d25cc4fda20 James Bottomley        2006-09-05  391   */
^1da177e4c3f41 Linus Torvalds         2005-04-16 @392  static struct scsi_target *scsi_alloc_target(struct device *parent,
^1da177e4c3f41 Linus Torvalds         2005-04-16  393  					     int channel, uint id)
^1da177e4c3f41 Linus Torvalds         2005-04-16  394  {
^1da177e4c3f41 Linus Torvalds         2005-04-16  395  	struct Scsi_Host *shost = dev_to_shost(parent);
^1da177e4c3f41 Linus Torvalds         2005-04-16  396  	struct device *dev = NULL;
^1da177e4c3f41 Linus Torvalds         2005-04-16  397  	unsigned long flags;
^1da177e4c3f41 Linus Torvalds         2005-04-16  398  	const int size = sizeof(struct scsi_target)
^1da177e4c3f41 Linus Torvalds         2005-04-16  399  		+ shost->transportt->target_size;
5c44cd2afad3f7 James.Smart@Emulex.Com 2005-06-10  400  	struct scsi_target *starget;
^1da177e4c3f41 Linus Torvalds         2005-04-16  401  	struct scsi_target *found_target;
e63ed0d7a98014 James Bottomley        2014-01-21  402  	int error, ref_got;
45b149b239ea9a Hannes Reinecke        2020-05-27  403  	unsigned long tid;
^1da177e4c3f41 Linus Torvalds         2005-04-16  404  
24669f75a3231f Jes Sorensen           2006-01-16  405  	starget = kzalloc(size, GFP_KERNEL);
^1da177e4c3f41 Linus Torvalds         2005-04-16  406  	if (!starget) {
cadbd4a5e36dde Harvey Harrison        2008-07-03  407  		printk(KERN_ERR "%s: allocation failure\n", __func__);
^1da177e4c3f41 Linus Torvalds         2005-04-16  408  		return NULL;
^1da177e4c3f41 Linus Torvalds         2005-04-16  409  	}
^1da177e4c3f41 Linus Torvalds         2005-04-16  410  	dev = &starget->dev;
^1da177e4c3f41 Linus Torvalds         2005-04-16  411  	device_initialize(dev);
e63ed0d7a98014 James Bottomley        2014-01-21  412  	kref_init(&starget->reap_ref);
^1da177e4c3f41 Linus Torvalds         2005-04-16  413  	dev->parent = get_device(parent);
71610f55fa4db6 Kay Sievers            2008-12-03  414  	dev_set_name(dev, "target%d:%d:%d", shost->host_no, channel, id);
b0ed43360fdca2 Hannes Reinecke        2008-03-18  415  	dev->bus = &scsi_bus_type;
b0ed43360fdca2 Hannes Reinecke        2008-03-18  416  	dev->type = &scsi_target_type;
^1da177e4c3f41 Linus Torvalds         2005-04-16  417  	starget->id = id;
^1da177e4c3f41 Linus Torvalds         2005-04-16  418  	starget->channel = channel;
f0c0a376d0fcd4 Mike Christie          2008-08-17  419  	starget->can_queue = 0;
^1da177e4c3f41 Linus Torvalds         2005-04-16  420  	INIT_LIST_HEAD(&starget->devices);
643eb2d932c97a James Bottomley        2008-03-22  421  	starget->state = STARGET_CREATED;
7c9d6f16f50d3a Alan Stern             2007-01-08  422  	starget->scsi_level = SCSI_2;
c53a284f8be237 Edward Goggin          2009-04-09  423  	starget->max_target_blocked = SCSI_DEFAULT_TARGET_BLOCKED;
45b149b239ea9a Hannes Reinecke        2020-05-27  424  	tid = scsi_target_index(starget);
ffedb4522571ac James Bottomley        2006-02-23  425   retry:
^1da177e4c3f41 Linus Torvalds         2005-04-16  426  	spin_lock_irqsave(shost->host_lock, flags);
^1da177e4c3f41 Linus Torvalds         2005-04-16  427  
45b149b239ea9a Hannes Reinecke        2020-05-27  428  	found_target = xa_load(&shost->__targets, tid);
^1da177e4c3f41 Linus Torvalds         2005-04-16  429  	if (found_target)
^1da177e4c3f41 Linus Torvalds         2005-04-16  430  		goto found;
45b149b239ea9a Hannes Reinecke        2020-05-27  431  	if (xa_insert(&shost->__targets, tid, starget, GFP_KERNEL)) {
45b149b239ea9a Hannes Reinecke        2020-05-27  432  		dev_printk(KERN_ERR, dev, "target index busy\n");
45b149b239ea9a Hannes Reinecke        2020-05-27  433  		kfree(starget);
45b149b239ea9a Hannes Reinecke        2020-05-27  434  		return NULL;
45b149b239ea9a Hannes Reinecke        2020-05-27  435  	}
^1da177e4c3f41 Linus Torvalds         2005-04-16  436  	spin_unlock_irqrestore(shost->host_lock, flags);
^1da177e4c3f41 Linus Torvalds         2005-04-16  437  	/* allocate and add */
a283bd37d00e92 James Bottomley        2005-05-24  438  	transport_setup_device(dev);
a283bd37d00e92 James Bottomley        2005-05-24  439  	if (shost->hostt->target_alloc) {
32f95792500794 Brian King             2006-02-22  440  		error = shost->hostt->target_alloc(starget);
a283bd37d00e92 James Bottomley        2005-05-24  441  
a283bd37d00e92 James Bottomley        2005-05-24  442  		if(error) {
a283bd37d00e92 James Bottomley        2005-05-24  443  			dev_printk(KERN_ERR, dev, "target allocation failed, error %d\n", error);
a283bd37d00e92 James Bottomley        2005-05-24  444  			/* don't want scsi_target_reap to do the final
a283bd37d00e92 James Bottomley        2005-05-24  445  			 * put because it will be under the host lock */
643eb2d932c97a James Bottomley        2008-03-22  446  			scsi_target_destroy(starget);
a283bd37d00e92 James Bottomley        2005-05-24  447  			return NULL;
a283bd37d00e92 James Bottomley        2005-05-24  448  		}
a283bd37d00e92 James Bottomley        2005-05-24  449  	}
884d25cc4fda20 James Bottomley        2006-09-05  450  	get_device(dev);
a283bd37d00e92 James Bottomley        2005-05-24  451  
^1da177e4c3f41 Linus Torvalds         2005-04-16  452  	return starget;
^1da177e4c3f41 Linus Torvalds         2005-04-16  453  
^1da177e4c3f41 Linus Torvalds         2005-04-16  454   found:
e63ed0d7a98014 James Bottomley        2014-01-21  455  	/*
e63ed0d7a98014 James Bottomley        2014-01-21  456  	 * release routine already fired if kref is zero, so if we can still
e63ed0d7a98014 James Bottomley        2014-01-21  457  	 * take the reference, the target must be alive.  If we can't, it must
e63ed0d7a98014 James Bottomley        2014-01-21  458  	 * be dying and we need to wait for a new target
e63ed0d7a98014 James Bottomley        2014-01-21  459  	 */
e63ed0d7a98014 James Bottomley        2014-01-21  460  	ref_got = kref_get_unless_zero(&found_target->reap_ref);
e63ed0d7a98014 James Bottomley        2014-01-21  461  
^1da177e4c3f41 Linus Torvalds         2005-04-16  462  	spin_unlock_irqrestore(shost->host_lock, flags);
e63ed0d7a98014 James Bottomley        2014-01-21  463  	if (ref_got) {
12fb8c1574d7d0 Alan Stern             2010-03-18  464  		put_device(dev);
^1da177e4c3f41 Linus Torvalds         2005-04-16  465  		return found_target;
^1da177e4c3f41 Linus Torvalds         2005-04-16  466  	}
e63ed0d7a98014 James Bottomley        2014-01-21  467  	/*
e63ed0d7a98014 James Bottomley        2014-01-21  468  	 * Unfortunately, we found a dying target; need to wait until it's
e63ed0d7a98014 James Bottomley        2014-01-21  469  	 * dead before we can get a new one.  There is an anomaly here.  We
e63ed0d7a98014 James Bottomley        2014-01-21  470  	 * *should* call scsi_target_reap() to balance the kref_get() of the
e63ed0d7a98014 James Bottomley        2014-01-21  471  	 * reap_ref above.  However, since the target being released, it's
e63ed0d7a98014 James Bottomley        2014-01-21  472  	 * already invisible and the reap_ref is irrelevant.  If we call
e63ed0d7a98014 James Bottomley        2014-01-21  473  	 * scsi_target_reap() we might spuriously do another device_del() on
e63ed0d7a98014 James Bottomley        2014-01-21  474  	 * an already invisible target.
e63ed0d7a98014 James Bottomley        2014-01-21  475  	 */
ffedb4522571ac James Bottomley        2006-02-23  476  	put_device(&found_target->dev);
e63ed0d7a98014 James Bottomley        2014-01-21  477  	/*
e63ed0d7a98014 James Bottomley        2014-01-21  478  	 * length of time is irrelevant here, we just want to yield the CPU
e63ed0d7a98014 James Bottomley        2014-01-21  479  	 * for a tick to avoid busy waiting for the target to die.
e63ed0d7a98014 James Bottomley        2014-01-21  480  	 */
e63ed0d7a98014 James Bottomley        2014-01-21  481  	msleep(1);
ffedb4522571ac James Bottomley        2006-02-23  482  	goto retry;
ffedb4522571ac James Bottomley        2006-02-23  483  }
^1da177e4c3f41 Linus Torvalds         2005-04-16  484  

:::::: The code at line 392 was first introduced by commit
:::::: 1da177e4c3f41524e886b7f1b8a0c1fc7321cac2 Linux-2.6.12-rc2

:::::: TO: Linus Torvalds <torvalds@ppc970.osdl.org>
:::::: CC: Linus Torvalds <torvalds@ppc970.osdl.org>

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 35320 bytes --]

Re: [PATCH 1/4] scsi: convert target lookup to xarray

[Dan Carpenter]

[-- Attachment #1: Type: text/plain, Size: 9488 bytes --]

Hi Hannes,

[auto build test WARNING on mkp-scsi/for-next]
[also build test WARNING on scsi/for-next v5.7-rc7 next-20200529]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Hannes-Reinecke/scsi-use-xarray-for-devices-and-targets/20200527-231824
base:   https://git.kernel.org/pub/scm/linux/kernel/git/mkp/scsi.git for-next
config: x86_64-randconfig-m001-20200529 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-13) 9.3.0

If you fix the issue, kindly add following tag as appropriate
Reported-by: kbuild test robot <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>

smatch warnings:
drivers/scsi/scsi_scan.c:482 scsi_alloc_target() warn: inconsistent returns '*shost->host_lock'.
drivers/scsi/scsi_scan.c:482 scsi_alloc_target() warn: inconsistent returns 'flags'.

# https://github.com/0day-ci/linux/commit/45b149b239ea9a86968ddbd8ecda1e6c44937b68
git remote add linux-review https://github.com/0day-ci/linux
git remote update linux-review
git checkout 45b149b239ea9a86968ddbd8ecda1e6c44937b68
vim +482 drivers/scsi/scsi_scan.c

^1da177e4c3f41 Linus Torvalds         2005-04-16  392  static struct scsi_target *scsi_alloc_target(struct device *parent,
^1da177e4c3f41 Linus Torvalds         2005-04-16  393  					     int channel, uint id)
^1da177e4c3f41 Linus Torvalds         2005-04-16  394  {
^1da177e4c3f41 Linus Torvalds         2005-04-16  395  	struct Scsi_Host *shost = dev_to_shost(parent);
^1da177e4c3f41 Linus Torvalds         2005-04-16  396  	struct device *dev = NULL;
^1da177e4c3f41 Linus Torvalds         2005-04-16  397  	unsigned long flags;
^1da177e4c3f41 Linus Torvalds         2005-04-16  398  	const int size = sizeof(struct scsi_target)
^1da177e4c3f41 Linus Torvalds         2005-04-16  399  		+ shost->transportt->target_size;
5c44cd2afad3f7 James.Smart@Emulex.Com 2005-06-10  400  	struct scsi_target *starget;
^1da177e4c3f41 Linus Torvalds         2005-04-16  401  	struct scsi_target *found_target;
e63ed0d7a98014 James Bottomley        2014-01-21  402  	int error, ref_got;
45b149b239ea9a Hannes Reinecke        2020-05-27  403  	unsigned long tid;
^1da177e4c3f41 Linus Torvalds         2005-04-16  404  
24669f75a3231f Jes Sorensen           2006-01-16  405  	starget = kzalloc(size, GFP_KERNEL);
^1da177e4c3f41 Linus Torvalds         2005-04-16  406  	if (!starget) {
cadbd4a5e36dde Harvey Harrison        2008-07-03  407  		printk(KERN_ERR "%s: allocation failure\n", __func__);
^1da177e4c3f41 Linus Torvalds         2005-04-16  408  		return NULL;
^1da177e4c3f41 Linus Torvalds         2005-04-16  409  	}
^1da177e4c3f41 Linus Torvalds         2005-04-16  410  	dev = &starget->dev;
^1da177e4c3f41 Linus Torvalds         2005-04-16  411  	device_initialize(dev);
e63ed0d7a98014 James Bottomley        2014-01-21  412  	kref_init(&starget->reap_ref);
^1da177e4c3f41 Linus Torvalds         2005-04-16  413  	dev->parent = get_device(parent);
71610f55fa4db6 Kay Sievers            2008-12-03  414  	dev_set_name(dev, "target%d:%d:%d", shost->host_no, channel, id);
b0ed43360fdca2 Hannes Reinecke        2008-03-18  415  	dev->bus = &scsi_bus_type;
b0ed43360fdca2 Hannes Reinecke        2008-03-18  416  	dev->type = &scsi_target_type;
^1da177e4c3f41 Linus Torvalds         2005-04-16  417  	starget->id = id;
^1da177e4c3f41 Linus Torvalds         2005-04-16  418  	starget->channel = channel;
f0c0a376d0fcd4 Mike Christie          2008-08-17  419  	starget->can_queue = 0;
^1da177e4c3f41 Linus Torvalds         2005-04-16  420  	INIT_LIST_HEAD(&starget->devices);
643eb2d932c97a James Bottomley        2008-03-22  421  	starget->state = STARGET_CREATED;
7c9d6f16f50d3a Alan Stern             2007-01-08  422  	starget->scsi_level = SCSI_2;
c53a284f8be237 Edward Goggin          2009-04-09  423  	starget->max_target_blocked = SCSI_DEFAULT_TARGET_BLOCKED;
45b149b239ea9a Hannes Reinecke        2020-05-27  424  	tid = scsi_target_index(starget);
ffedb4522571ac James Bottomley        2006-02-23  425   retry:
^1da177e4c3f41 Linus Torvalds         2005-04-16  426  	spin_lock_irqsave(shost->host_lock, flags);
                                                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
^1da177e4c3f41 Linus Torvalds         2005-04-16  427  
45b149b239ea9a Hannes Reinecke        2020-05-27  428  	found_target = xa_load(&shost->__targets, tid);
^1da177e4c3f41 Linus Torvalds         2005-04-16  429  	if (found_target)
^1da177e4c3f41 Linus Torvalds         2005-04-16  430  		goto found;
45b149b239ea9a Hannes Reinecke        2020-05-27  431  	if (xa_insert(&shost->__targets, tid, starget, GFP_KERNEL)) {
45b149b239ea9a Hannes Reinecke        2020-05-27  432  		dev_printk(KERN_ERR, dev, "target index busy\n");
45b149b239ea9a Hannes Reinecke        2020-05-27  433  		kfree(starget);
45b149b239ea9a Hannes Reinecke        2020-05-27  434  		return NULL;
                                                                ^^^^^^^^^^^

Need to drop the lock.

45b149b239ea9a Hannes Reinecke        2020-05-27  435  	}
^1da177e4c3f41 Linus Torvalds         2005-04-16  436  	spin_unlock_irqrestore(shost->host_lock, flags);
^1da177e4c3f41 Linus Torvalds         2005-04-16  437  	/* allocate and add */
a283bd37d00e92 James Bottomley        2005-05-24  438  	transport_setup_device(dev);
a283bd37d00e92 James Bottomley        2005-05-24  439  	if (shost->hostt->target_alloc) {
32f95792500794 Brian King             2006-02-22  440  		error = shost->hostt->target_alloc(starget);
a283bd37d00e92 James Bottomley        2005-05-24  441  
a283bd37d00e92 James Bottomley        2005-05-24  442  		if(error) {
a283bd37d00e92 James Bottomley        2005-05-24  443  			dev_printk(KERN_ERR, dev, "target allocation failed, error %d\n", error);
a283bd37d00e92 James Bottomley        2005-05-24  444  			/* don't want scsi_target_reap to do the final
a283bd37d00e92 James Bottomley        2005-05-24  445  			 * put because it will be under the host lock */
643eb2d932c97a James Bottomley        2008-03-22  446  			scsi_target_destroy(starget);
a283bd37d00e92 James Bottomley        2005-05-24  447  			return NULL;
a283bd37d00e92 James Bottomley        2005-05-24  448  		}
a283bd37d00e92 James Bottomley        2005-05-24  449  	}
884d25cc4fda20 James Bottomley        2006-09-05  450  	get_device(dev);
a283bd37d00e92 James Bottomley        2005-05-24  451  
^1da177e4c3f41 Linus Torvalds         2005-04-16  452  	return starget;
^1da177e4c3f41 Linus Torvalds         2005-04-16  453  
^1da177e4c3f41 Linus Torvalds         2005-04-16  454   found:
e63ed0d7a98014 James Bottomley        2014-01-21  455  	/*
e63ed0d7a98014 James Bottomley        2014-01-21  456  	 * release routine already fired if kref is zero, so if we can still
e63ed0d7a98014 James Bottomley        2014-01-21  457  	 * take the reference, the target must be alive.  If we can't, it must
e63ed0d7a98014 James Bottomley        2014-01-21  458  	 * be dying and we need to wait for a new target
e63ed0d7a98014 James Bottomley        2014-01-21  459  	 */
e63ed0d7a98014 James Bottomley        2014-01-21  460  	ref_got = kref_get_unless_zero(&found_target->reap_ref);
e63ed0d7a98014 James Bottomley        2014-01-21  461  
^1da177e4c3f41 Linus Torvalds         2005-04-16  462  	spin_unlock_irqrestore(shost->host_lock, flags);
e63ed0d7a98014 James Bottomley        2014-01-21  463  	if (ref_got) {
12fb8c1574d7d0 Alan Stern             2010-03-18  464  		put_device(dev);
^1da177e4c3f41 Linus Torvalds         2005-04-16  465  		return found_target;
^1da177e4c3f41 Linus Torvalds         2005-04-16  466  	}
e63ed0d7a98014 James Bottomley        2014-01-21  467  	/*
e63ed0d7a98014 James Bottomley        2014-01-21  468  	 * Unfortunately, we found a dying target; need to wait until it's
e63ed0d7a98014 James Bottomley        2014-01-21  469  	 * dead before we can get a new one.  There is an anomaly here.  We
e63ed0d7a98014 James Bottomley        2014-01-21  470  	 * *should* call scsi_target_reap() to balance the kref_get() of the
e63ed0d7a98014 James Bottomley        2014-01-21  471  	 * reap_ref above.  However, since the target being released, it's
e63ed0d7a98014 James Bottomley        2014-01-21  472  	 * already invisible and the reap_ref is irrelevant.  If we call
e63ed0d7a98014 James Bottomley        2014-01-21  473  	 * scsi_target_reap() we might spuriously do another device_del() on
e63ed0d7a98014 James Bottomley        2014-01-21  474  	 * an already invisible target.
e63ed0d7a98014 James Bottomley        2014-01-21  475  	 */
ffedb4522571ac James Bottomley        2006-02-23  476  	put_device(&found_target->dev);
e63ed0d7a98014 James Bottomley        2014-01-21  477  	/*
e63ed0d7a98014 James Bottomley        2014-01-21  478  	 * length of time is irrelevant here, we just want to yield the CPU
e63ed0d7a98014 James Bottomley        2014-01-21  479  	 * for a tick to avoid busy waiting for the target to die.
e63ed0d7a98014 James Bottomley        2014-01-21  480  	 */
e63ed0d7a98014 James Bottomley        2014-01-21  481  	msleep(1);
ffedb4522571ac James Bottomley        2006-02-23 @482  	goto retry;
ffedb4522571ac James Bottomley        2006-02-23  483  }

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 37380 bytes --]

[PATCH 2/4] target_core_pscsi: use __scsi_device_lookup()

[Hannes Reinecke]

Instead of walking the list of devices manually use the helper
function to return the device directly.

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/target/target_core_pscsi.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/target/target_core_pscsi.c b/drivers/target/target_core_pscsi.c
index 4e37fa9b409d..38799e47b590 100644
--- a/drivers/target/target_core_pscsi.c
+++ b/drivers/target/target_core_pscsi.c
@@ -496,11 +496,9 @@ static int pscsi_configure_device(struct se_device *dev)
 	}
 
 	spin_lock_irq(sh->host_lock);
-	list_for_each_entry(sd, &sh->__devices, siblings) {
-		if ((pdv->pdv_channel_id != sd->channel) ||
-		    (pdv->pdv_target_id != sd->id) ||
-		    (pdv->pdv_lun_id != sd->lun))
-			continue;
+	sd = __scsi_device_lookup(sh, pdv->pdv_channel_id,
+				  pdv->pdv_target_id, pdv->pdv_lun_id);
+	if (sd) {
 		/*
 		 * Functions will release the held struct scsi_host->host_lock
 		 * before calling calling pscsi_add_device_to_list() to register
-- 
2.16.4

Re: [PATCH 2/4] target_core_pscsi: use __scsi_device_lookup()

[Johannes Thumshirn]

On 27/05/2020 16:14, Hannes Reinecke wrote:
> Instead of walking the list of devices manually use the helper
> function to return the device directly.

I think this a nice cleanup irrespective of the rest of the series.

Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

[PATCH 3/4] scsi: move target device list to xarray

[Hannes Reinecke]

Use xarray for device lookup by target. LUNs below 256 are linear,
and can be used directly as the index into the xarray.
LUNs above 256 have a distinct LUN format, and are not necessarily
linear. They'll be stored in indices above 256 in the xarray, with
the next free index in the xarray.

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/scsi/scsi.c        | 28 +++++++++++++++-------------
 drivers/scsi/scsi_lib.c    |  8 +++-----
 drivers/scsi/scsi_scan.c   |  7 +++++--
 drivers/scsi/scsi_sysfs.c  | 15 +++++++++++++--
 include/scsi/scsi_device.h |  4 ++--
 5 files changed, 38 insertions(+), 24 deletions(-)

diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index 25c815355d1a..36aec2b37caa 100644
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -601,13 +601,14 @@ static struct scsi_target *__scsi_target_lookup(struct Scsi_Host *shost,
 void starget_for_each_device(struct scsi_target *starget, void *data,
 		     void (*fn)(struct scsi_device *, void *))
 {
-	struct Scsi_Host *shost = dev_to_shost(starget->dev.parent);
 	struct scsi_device *sdev;
+	unsigned long lun_id = 0;
 
-	shost_for_each_device(sdev, shost) {
-		if ((sdev->channel == starget->channel) &&
-		    (sdev->id == starget->id))
-			fn(sdev, data);
+	xa_for_each(&starget->devices, lun_id, sdev) {
+		if (scsi_device_get(sdev))
+			continue;
+		fn(sdev, data);
+		scsi_device_put(sdev);
 	}
 }
 EXPORT_SYMBOL(starget_for_each_device);
@@ -629,14 +630,11 @@ EXPORT_SYMBOL(starget_for_each_device);
 void __starget_for_each_device(struct scsi_target *starget, void *data,
 			       void (*fn)(struct scsi_device *, void *))
 {
-	struct Scsi_Host *shost = dev_to_shost(starget->dev.parent);
 	struct scsi_device *sdev;
+	unsigned long lun_id = 0;
 
-	__shost_for_each_device(sdev, shost) {
-		if ((sdev->channel == starget->channel) &&
-		    (sdev->id == starget->id))
-			fn(sdev, data);
-	}
+	xa_for_each(&starget->devices, lun_id, sdev)
+		fn(sdev, data);
 }
 EXPORT_SYMBOL(__starget_for_each_device);
 
@@ -659,11 +657,15 @@ struct scsi_device *__scsi_device_lookup_by_target(struct scsi_target *starget,
 						   u64 lun)
 {
 	struct scsi_device *sdev;
+	unsigned long lun_idx = 0;
+
+	if (lun < 256)
+		return xa_load(&starget->devices, lun);
 
-	list_for_each_entry(sdev, &starget->devices, same_target_siblings) {
+	xa_for_each(&starget->devices, lun_idx, sdev) {
 		if (sdev->sdev_state == SDEV_DEL)
 			continue;
-		if (sdev->lun ==lun)
+		if (sdev->lun == lun)
 			return sdev;
 	}
 
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index c163fa22267c..23c99fbe47d5 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -361,9 +361,9 @@ static void scsi_kick_queue(struct request_queue *q)
 static void scsi_single_lun_run(struct scsi_device *current_sdev)
 {
 	struct Scsi_Host *shost = current_sdev->host;
-	struct scsi_device *sdev, *tmp;
+	struct scsi_device *sdev;
 	struct scsi_target *starget = scsi_target(current_sdev);
-	unsigned long flags;
+	unsigned long flags, lun_id = 0;
 
 	spin_lock_irqsave(shost->host_lock, flags);
 	starget->starget_sdev_user = NULL;
@@ -380,8 +380,7 @@ static void scsi_single_lun_run(struct scsi_device *current_sdev)
 	spin_lock_irqsave(shost->host_lock, flags);
 	if (starget->starget_sdev_user)
 		goto out;
-	list_for_each_entry_safe(sdev, tmp, &starget->devices,
-			same_target_siblings) {
+	xa_for_each(&starget->devices, lun_id, sdev) {
 		if (sdev == current_sdev)
 			continue;
 		if (scsi_device_get(sdev))
@@ -390,7 +389,6 @@ static void scsi_single_lun_run(struct scsi_device *current_sdev)
 		spin_unlock_irqrestore(shost->host_lock, flags);
 		scsi_kick_queue(sdev->request_queue);
 		spin_lock_irqsave(shost->host_lock, flags);
-	
 		scsi_device_put(sdev);
 	}
  out:
diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
index c47cddef1839..6fce5fe6ef32 100644
--- a/drivers/scsi/scsi_scan.c
+++ b/drivers/scsi/scsi_scan.c
@@ -232,10 +232,13 @@ static struct scsi_device *scsi_alloc_sdev(struct scsi_target *starget,
 	sdev->id = starget->id;
 	sdev->lun = lun;
 	sdev->channel = starget->channel;
+	if (lun < 256)
+		sdev->lun_idx = lun;
+	else
+		sdev->lun_idx = (unsigned int)-1;
 	mutex_init(&sdev->state_mutex);
 	sdev->sdev_state = SDEV_CREATED;
 	INIT_LIST_HEAD(&sdev->siblings);
-	INIT_LIST_HEAD(&sdev->same_target_siblings);
 	INIT_LIST_HEAD(&sdev->starved_entry);
 	INIT_LIST_HEAD(&sdev->event_list);
 	spin_lock_init(&sdev->list_lock);
@@ -417,7 +420,7 @@ static struct scsi_target *scsi_alloc_target(struct device *parent,
 	starget->id = id;
 	starget->channel = channel;
 	starget->can_queue = 0;
-	INIT_LIST_HEAD(&starget->devices);
+	xa_init(&starget->devices);
 	starget->state = STARGET_CREATED;
 	starget->scsi_level = SCSI_2;
 	starget->max_target_blocked = SCSI_DEFAULT_TARGET_BLOCKED;
diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
index 95aaa96ce03b..b9ed56d6bd95 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -434,6 +434,7 @@ static void scsi_device_cls_release(struct device *class_dev)
 static void scsi_device_dev_release_usercontext(struct work_struct *work)
 {
 	struct scsi_device *sdev;
+	struct scsi_target *starget = sdev->sdev_target;
 	struct device *parent;
 	struct list_head *this, *tmp;
 	struct scsi_vpd *vpd_pg80 = NULL, *vpd_pg83 = NULL;
@@ -448,7 +449,7 @@ static void scsi_device_dev_release_usercontext(struct work_struct *work)
 
 	spin_lock_irqsave(sdev->host->host_lock, flags);
 	list_del(&sdev->siblings);
-	list_del(&sdev->same_target_siblings);
+	xa_erase(&starget->devices, sdev->lun_idx);
 	list_del(&sdev->starved_entry);
 	spin_unlock_irqrestore(sdev->host->host_lock, flags);
 
@@ -1621,7 +1622,17 @@ void scsi_sysfs_device_initialize(struct scsi_device *sdev)
 
 	transport_setup_device(&sdev->sdev_gendev);
 	spin_lock_irqsave(shost->host_lock, flags);
-	list_add_tail(&sdev->same_target_siblings, &starget->devices);
+	if (sdev->lun_idx != (unsigned long)-1)
+		WARN_ON(!xa_insert(&starget->devices, sdev->lun_idx,
+				   sdev, GFP_KERNEL));
+	else {
+		struct xa_limit scsi_lun_limit = {
+			.min = 256,
+			.max = UINT_MAX,
+		};
+		WARN_ON(!xa_alloc(&starget->devices, &sdev->lun_idx,
+				  sdev, scsi_lun_limit, GFP_KERNEL));
+	}
 	list_add_tail(&sdev->siblings, &shost->__devices);
 	spin_unlock_irqrestore(shost->host_lock, flags);
 	/*
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index 28034cc0fce5..2c6b9d8bc40e 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -104,7 +104,6 @@ struct scsi_device {
 
 	/* the next two are protected by the host->host_lock */
 	struct list_head    siblings;   /* list of all devices on this host */
-	struct list_head    same_target_siblings; /* just the devices sharing same target id */
 
 	atomic_t device_busy;		/* commands actually active on LLDD */
 	atomic_t device_blocked;	/* Device returned QUEUE_FULL. */
@@ -123,6 +122,7 @@ struct scsi_device {
 
 	unsigned int id, channel;
 	u64 lun;
+	unsigned int lun_idx;		/* Index into target device xarray */
 	unsigned int manufacturer;	/* Manufacturer of device, for using 
 					 * vendor-specific cmd's */
 	unsigned sector_size;	/* size in bytes */
@@ -285,7 +285,7 @@ enum scsi_target_state {
 struct scsi_target {
 	struct scsi_device	*starget_sdev_user;
 	struct list_head	siblings;
-	struct list_head	devices;
+	struct xarray		devices;
 	struct device		dev;
 	struct kref		reap_ref; /* last put renders target invisible */
 	unsigned int		channel;
-- 
2.16.4

Re: [PATCH 3/4] scsi: move target device list to xarray

[Johannes Thumshirn]

Looks good (I think),
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

Re: [PATCH 3/4] scsi: move target device list to xarray

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 4193 bytes --]

Hi Hannes,

I love your patch! Perhaps something to improve:

[auto build test WARNING on mkp-scsi/for-next]
[also build test WARNING on scsi/for-next v5.7-rc7 next-20200526]
[cannot apply to hch-configfs/for-next]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Hannes-Reinecke/scsi-use-xarray-for-devices-and-targets/20200527-231824
base:   https://git.kernel.org/pub/scm/linux/kernel/git/mkp/scsi.git for-next
config: ia64-allmodconfig (attached as .config)
compiler: ia64-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=ia64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kbuild test robot <lkp@intel.com>

All warnings (new ones prefixed by >>, old ones prefixed by <<):

drivers/scsi/scsi_sysfs.c: In function 'scsi_sysfs_device_initialize':
>> drivers/scsi/scsi_sysfs.c:1625:20: warning: comparison is always true due to limited range of data type [-Wtype-limits]
1625 |  if (sdev->lun_idx != (unsigned long)-1)
|                    ^~
drivers/scsi/scsi_sysfs.c: In function 'scsi_device_dev_release_usercontext':
drivers/scsi/scsi_sysfs.c:437:22: warning: 'sdev' is used uninitialized in this function [-Wuninitialized]
437 |  struct scsi_target *starget = sdev->sdev_target;
|                      ^~~~~~~

vim +1625 drivers/scsi/scsi_sysfs.c

  1592	
  1593	void scsi_sysfs_device_initialize(struct scsi_device *sdev)
  1594	{
  1595		unsigned long flags;
  1596		struct Scsi_Host *shost = sdev->host;
  1597		struct scsi_target  *starget = sdev->sdev_target;
  1598	
  1599		device_initialize(&sdev->sdev_gendev);
  1600		sdev->sdev_gendev.bus = &scsi_bus_type;
  1601		sdev->sdev_gendev.type = &scsi_dev_type;
  1602		dev_set_name(&sdev->sdev_gendev, "%d:%d:%d:%llu",
  1603			     sdev->host->host_no, sdev->channel, sdev->id, sdev->lun);
  1604	
  1605		device_initialize(&sdev->sdev_dev);
  1606		sdev->sdev_dev.parent = get_device(&sdev->sdev_gendev);
  1607		sdev->sdev_dev.class = &sdev_class;
  1608		dev_set_name(&sdev->sdev_dev, "%d:%d:%d:%llu",
  1609			     sdev->host->host_no, sdev->channel, sdev->id, sdev->lun);
  1610		/*
  1611		 * Get a default scsi_level from the target (derived from sibling
  1612		 * devices).  This is the best we can do for guessing how to set
  1613		 * sdev->lun_in_cdb for the initial INQUIRY command.  For LUN 0 the
  1614		 * setting doesn't matter, because all the bits are zero anyway.
  1615		 * But it does matter for higher LUNs.
  1616		 */
  1617		sdev->scsi_level = starget->scsi_level;
  1618		if (sdev->scsi_level <= SCSI_2 &&
  1619				sdev->scsi_level != SCSI_UNKNOWN &&
  1620				!shost->no_scsi2_lun_in_cdb)
  1621			sdev->lun_in_cdb = 1;
  1622	
  1623		transport_setup_device(&sdev->sdev_gendev);
  1624		spin_lock_irqsave(shost->host_lock, flags);
> 1625		if (sdev->lun_idx != (unsigned long)-1)
  1626			WARN_ON(!xa_insert(&starget->devices, sdev->lun_idx,
  1627					   sdev, GFP_KERNEL));
  1628		else {
  1629			struct xa_limit scsi_lun_limit = {
  1630				.min = 256,
  1631				.max = UINT_MAX,
  1632			};
  1633			WARN_ON(!xa_alloc(&starget->devices, &sdev->lun_idx,
  1634					  sdev, scsi_lun_limit, GFP_KERNEL));
  1635		}
  1636		list_add_tail(&sdev->siblings, &shost->__devices);
  1637		spin_unlock_irqrestore(shost->host_lock, flags);
  1638		/*
  1639		 * device can now only be removed via __scsi_remove_device() so hold
  1640		 * the target.  Target will be held in CREATED state until something
  1641		 * beneath it becomes visible (in which case it moves to RUNNING)
  1642		 */
  1643		kref_get(&starget->reap_ref);
  1644	}
  1645	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 57752 bytes --]

Re: [PATCH 3/4] scsi: move target device list to xarray

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 3559 bytes --]

Hi Hannes,

I love your patch! Perhaps something to improve:

[auto build test WARNING on mkp-scsi/for-next]
[also build test WARNING on scsi/for-next v5.7-rc7 next-20200529]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Hannes-Reinecke/scsi-use-xarray-for-devices-and-targets/20200527-231824
base:   https://git.kernel.org/pub/scm/linux/kernel/git/mkp/scsi.git for-next
config: x86_64-randconfig-m001-20200529 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-13) 9.3.0

If you fix the issue, kindly add following tag as appropriate
Reported-by: kbuild test robot <lkp@intel.com>

New smatch warnings:
drivers/scsi/scsi_sysfs.c:1625 scsi_sysfs_device_initialize() warn: always true condition '(sdev->lun_idx != -1) => (0-u32max != u64max)'

Old smatch warnings:
drivers/scsi/scsi_sysfs.c:437 scsi_device_dev_release_usercontext() error: potentially dereferencing uninitialized 'sdev'.

vim +1625 drivers/scsi/scsi_sysfs.c

  1592	
  1593	void scsi_sysfs_device_initialize(struct scsi_device *sdev)
  1594	{
  1595		unsigned long flags;
  1596		struct Scsi_Host *shost = sdev->host;
  1597		struct scsi_target  *starget = sdev->sdev_target;
  1598	
  1599		device_initialize(&sdev->sdev_gendev);
  1600		sdev->sdev_gendev.bus = &scsi_bus_type;
  1601		sdev->sdev_gendev.type = &scsi_dev_type;
  1602		dev_set_name(&sdev->sdev_gendev, "%d:%d:%d:%llu",
  1603			     sdev->host->host_no, sdev->channel, sdev->id, sdev->lun);
  1604	
  1605		device_initialize(&sdev->sdev_dev);
  1606		sdev->sdev_dev.parent = get_device(&sdev->sdev_gendev);
  1607		sdev->sdev_dev.class = &sdev_class;
  1608		dev_set_name(&sdev->sdev_dev, "%d:%d:%d:%llu",
  1609			     sdev->host->host_no, sdev->channel, sdev->id, sdev->lun);
  1610		/*
  1611		 * Get a default scsi_level from the target (derived from sibling
  1612		 * devices).  This is the best we can do for guessing how to set
  1613		 * sdev->lun_in_cdb for the initial INQUIRY command.  For LUN 0 the
  1614		 * setting doesn't matter, because all the bits are zero anyway.
  1615		 * But it does matter for higher LUNs.
  1616		 */
  1617		sdev->scsi_level = starget->scsi_level;
  1618		if (sdev->scsi_level <= SCSI_2 &&
  1619				sdev->scsi_level != SCSI_UNKNOWN &&
  1620				!shost->no_scsi2_lun_in_cdb)
  1621			sdev->lun_in_cdb = 1;
  1622	
  1623		transport_setup_device(&sdev->sdev_gendev);
  1624		spin_lock_irqsave(shost->host_lock, flags);
> 1625		if (sdev->lun_idx != (unsigned long)-1)
  1626			WARN_ON(!xa_insert(&starget->devices, sdev->lun_idx,
  1627					   sdev, GFP_KERNEL));
  1628		else {
  1629			struct xa_limit scsi_lun_limit = {
  1630				.min = 256,
  1631				.max = UINT_MAX,
  1632			};
  1633			WARN_ON(!xa_alloc(&starget->devices, &sdev->lun_idx,
  1634					  sdev, scsi_lun_limit, GFP_KERNEL));
  1635		}
  1636		list_add_tail(&sdev->siblings, &shost->__devices);
  1637		spin_unlock_irqrestore(shost->host_lock, flags);
  1638		/*
  1639		 * device can now only be removed via __scsi_remove_device() so hold
  1640		 * the target.  Target will be held in CREATED state until something
  1641		 * beneath it becomes visible (in which case it moves to RUNNING)
  1642		 */
  1643		kref_get(&starget->reap_ref);
  1644	}
  1645	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 37380 bytes --]

[PATCH 4/4] scsi: remove direct device lookup per host

[Hannes Reinecke]

Drop the per-host device list for direct lookup and iterate
over the targets and devices xarrays instead.
As both are now using xarrays the lookup is more efficient
as we can use the provided indices based on the HCTL id
to do a direct lookup instead of traversing lists.

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/scsi/hosts.c       |  1 -
 drivers/scsi/scsi.c        | 68 +++++++++++++++++++++++++++++++++++++++++-----
 drivers/scsi/scsi_scan.c   | 20 ++++++++------
 drivers/scsi/scsi_sysfs.c  | 13 ++-------
 include/scsi/scsi_device.h | 13 +++++----
 include/scsi/scsi_host.h   |  3 +-
 6 files changed, 85 insertions(+), 33 deletions(-)

diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
index 7109afad0183..004a50a95560 100644
--- a/drivers/scsi/hosts.c
+++ b/drivers/scsi/hosts.c
@@ -382,7 +382,6 @@ struct Scsi_Host *scsi_host_alloc(struct scsi_host_template *sht, int privsize)
 	shost->host_lock = &shost->default_lock;
 	spin_lock_init(shost->host_lock);
 	shost->shost_state = SHOST_CREATED;
-	INIT_LIST_HEAD(&shost->__devices);
 	xa_init(&shost->__targets);
 	INIT_LIST_HEAD(&shost->eh_cmd_q);
 	INIT_LIST_HEAD(&shost->starved_list);
diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index 36aec2b37caa..172767e7ee71 100644
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -554,18 +554,39 @@ EXPORT_SYMBOL(scsi_device_put);
 struct scsi_device *__scsi_iterate_devices(struct Scsi_Host *shost,
 					   struct scsi_device *prev)
 {
-	struct list_head *list = (prev ? &prev->siblings : &shost->__devices);
+	struct scsi_target *starget;
 	struct scsi_device *next = NULL;
 	unsigned long flags;
+	unsigned long tid = 0, lun_idx = 0;
 
 	spin_lock_irqsave(shost->host_lock, flags);
-	while (list->next != &shost->__devices) {
-		next = list_entry(list->next, struct scsi_device, siblings);
-		/* skip devices that we can't get a reference to */
+	if (!prev) {
+		starget = xa_find(&shost->__targets, &tid,
+				       ULONG_MAX, XA_PRESENT);
+		if (starget) {
+			next = xa_find(&starget->devices, &lun_idx,
+				       ULONG_MAX, XA_PRESENT);
+			if (!scsi_device_get(next)) {
+				spin_unlock_irqrestore(shost->host_lock, flags);
+				return next;
+			}
+		}
+	} else {
+		starget = prev->sdev_target;
+		lun_idx = prev->lun_idx;
+	}
+	while (starget) {
+		tid = (starget->channel << 16) | starget->id;
+		next = xa_find_after(&starget->devices, &tid,
+				     ULONG_MAX, XA_PRESENT);
+		if (!next) {
+			lun_idx = 0;
+			starget = xa_find_after(&shost->__targets, &tid,
+						ULONG_MAX, XA_PRESENT);
+			continue;
+		}
 		if (!scsi_device_get(next))
 			break;
-		next = NULL;
-		list = list->next;
 	}
 	spin_unlock_irqrestore(shost->host_lock, flags);
 
@@ -575,6 +596,38 @@ struct scsi_device *__scsi_iterate_devices(struct Scsi_Host *shost,
 }
 EXPORT_SYMBOL(__scsi_iterate_devices);
 
+/* helper for __shost_for_each_device, see that for documentation */
+struct scsi_device *__scsi_iterate_devices_unlocked(struct Scsi_Host *shost,
+						    struct scsi_device *prev)
+{
+	struct scsi_target *starget;
+	struct scsi_device *next = NULL;
+	unsigned long tid = 0, lun_idx = 0;
+
+	if (!prev) {
+		starget = xa_find(&shost->__targets, &tid,
+				       ULONG_MAX, XA_PRESENT);
+		if (starget)
+			return xa_find(&starget->devices, &lun_idx,
+				       ULONG_MAX, XA_PRESENT);
+	} else {
+		starget = prev->sdev_target;
+		lun_idx = prev->lun_idx;
+	}
+	while (starget) {
+		tid = (starget->channel << 16) | starget->id;
+		next = xa_find_after(&starget->devices, &tid,
+				     ULONG_MAX, XA_PRESENT);
+		if (next)
+			return next;
+		lun_idx = 0;
+		starget = xa_find_after(&shost->__targets, &tid,
+					ULONG_MAX, XA_PRESENT);
+	}
+	return NULL;
+}
+EXPORT_SYMBOL(__scsi_iterate_devices_unlocked);
+
 /**
  * __scsi_target_lookup  -  find a target based on channel and target id
  * @shost:	SCSI host pointer
@@ -582,11 +635,12 @@ EXPORT_SYMBOL(__scsi_iterate_devices);
  * @id:		ID of the target
  *
  */
-static struct scsi_target *__scsi_target_lookup(struct Scsi_Host *shost,
+struct scsi_target *__scsi_target_lookup(struct Scsi_Host *shost,
 					 u16 channel, u16 id)
 {
 	return xa_load(&shost->__targets, (channel << 16) | id);
 }
+EXPORT_SYMBOL(__scsi_target_lookup);
 
 /**
  * starget_for_each_device  -  helper to walk all devices of a target
diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
index 6fce5fe6ef32..24179591848b 100644
--- a/drivers/scsi/scsi_scan.c
+++ b/drivers/scsi/scsi_scan.c
@@ -238,7 +238,6 @@ static struct scsi_device *scsi_alloc_sdev(struct scsi_target *starget,
 		sdev->lun_idx = (unsigned int)-1;
 	mutex_init(&sdev->state_mutex);
 	sdev->sdev_state = SDEV_CREATED;
-	INIT_LIST_HEAD(&sdev->siblings);
 	INIT_LIST_HEAD(&sdev->starved_entry);
 	INIT_LIST_HEAD(&sdev->event_list);
 	spin_lock_init(&sdev->list_lock);
@@ -1849,17 +1848,22 @@ EXPORT_SYMBOL(scsi_scan_host);
 
 void scsi_forget_host(struct Scsi_Host *shost)
 {
+	struct scsi_target *starget;
 	struct scsi_device *sdev;
 	unsigned long flags;
+	unsigned long tid = 0;
 
- restart:
 	spin_lock_irqsave(shost->host_lock, flags);
-	list_for_each_entry(sdev, &shost->__devices, siblings) {
-		if (sdev->sdev_state == SDEV_DEL)
-			continue;
-		spin_unlock_irqrestore(shost->host_lock, flags);
-		__scsi_remove_device(sdev);
-		goto restart;
+	xa_for_each(&shost->__targets, tid, starget) {
+		unsigned long lun_id = 0;
+
+		xa_for_each(&starget->devices, lun_id, sdev) {
+			if (sdev->sdev_state == SDEV_DEL)
+				continue;
+			spin_unlock_irqrestore(shost->host_lock, flags);
+			__scsi_remove_device(sdev);
+			spin_lock_irqsave(shost->host_lock, flags);
+		}
 	}
 	spin_unlock_irqrestore(shost->host_lock, flags);
 }
diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
index b9ed56d6bd95..24822f6ee136 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -434,7 +434,6 @@ static void scsi_device_cls_release(struct device *class_dev)
 static void scsi_device_dev_release_usercontext(struct work_struct *work)
 {
 	struct scsi_device *sdev;
-	struct scsi_target *starget = sdev->sdev_target;
 	struct device *parent;
 	struct list_head *this, *tmp;
 	struct scsi_vpd *vpd_pg80 = NULL, *vpd_pg83 = NULL;
@@ -448,8 +447,7 @@ static void scsi_device_dev_release_usercontext(struct work_struct *work)
 	parent = sdev->sdev_gendev.parent;
 
 	spin_lock_irqsave(sdev->host->host_lock, flags);
-	list_del(&sdev->siblings);
-	xa_erase(&starget->devices, sdev->lun_idx);
+	xa_erase(&sdev->sdev_target->devices, sdev->lun_idx);
 	list_del(&sdev->starved_entry);
 	spin_unlock_irqrestore(sdev->host->host_lock, flags);
 
@@ -1475,19 +1473,16 @@ static void __scsi_remove_target(struct scsi_target *starget)
 	struct Scsi_Host *shost = dev_to_shost(starget->dev.parent);
 	unsigned long flags;
 	struct scsi_device *sdev;
+	unsigned long lun_idx = 0;
 
 	spin_lock_irqsave(shost->host_lock, flags);
- restart:
-	list_for_each_entry(sdev, &shost->__devices, siblings) {
+	xa_for_each(&starget->devices, lun_idx, sdev) {
 		/*
 		 * We cannot call scsi_device_get() here, as
 		 * we might've been called from rmmod() causing
 		 * scsi_device_get() to fail the module_is_live()
 		 * check.
 		 */
-		if (sdev->channel != starget->channel ||
-		    sdev->id != starget->id)
-			continue;
 		if (sdev->sdev_state == SDEV_DEL ||
 		    sdev->sdev_state == SDEV_CANCEL ||
 		    !get_device(&sdev->sdev_gendev))
@@ -1496,7 +1491,6 @@ static void __scsi_remove_target(struct scsi_target *starget)
 		scsi_remove_device(sdev);
 		put_device(&sdev->sdev_gendev);
 		spin_lock_irqsave(shost->host_lock, flags);
-		goto restart;
 	}
 	spin_unlock_irqrestore(shost->host_lock, flags);
 }
@@ -1633,7 +1627,6 @@ void scsi_sysfs_device_initialize(struct scsi_device *sdev)
 		WARN_ON(!xa_alloc(&starget->devices, &sdev->lun_idx,
 				  sdev, scsi_lun_limit, GFP_KERNEL));
 	}
-	list_add_tail(&sdev->siblings, &shost->__devices);
 	spin_unlock_irqrestore(shost->host_lock, flags);
 	/*
 	 * device can now only be removed via __scsi_remove_device() so hold
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index 2c6b9d8bc40e..b811b5e3adfe 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -102,9 +102,6 @@ struct scsi_device {
 	struct Scsi_Host *host;
 	struct request_queue *request_queue;
 
-	/* the next two are protected by the host->host_lock */
-	struct list_head    siblings;   /* list of all devices on this host */
-
 	atomic_t device_busy;		/* commands actually active on LLDD */
 	atomic_t device_blocked;	/* Device returned QUEUE_FULL. */
 
@@ -376,6 +373,10 @@ extern struct scsi_device *__scsi_iterate_devices(struct Scsi_Host *,
 	     (sdev); \
 	     (sdev) = __scsi_iterate_devices((shost), (sdev)))
 
+/* only exposed to implement shost_for_each_device */
+struct scsi_device *__scsi_iterate_devices_unlocked(struct Scsi_Host *,
+						    struct scsi_device *);
+
 /**
  * __shost_for_each_device - iterate over all devices of a host (UNLOCKED)
  * @sdev: the &struct scsi_device to use as a cursor
@@ -389,8 +390,10 @@ extern struct scsi_device *__scsi_iterate_devices(struct Scsi_Host *,
  * device list in interrupt context.  Otherwise you really want to use
  * shost_for_each_device instead.
  */
-#define __shost_for_each_device(sdev, shost) \
-	list_for_each_entry((sdev), &((shost)->__devices), siblings)
+#define __shost_for_each_device(sdev, shost)				\
+	for((sdev) = __scsi_iterate_devices_unlocked((shost), NULL);	\
+	    (sdev);							\
+	    (sdev) = __scsi_iterate_devices_unlocked((shost),(sdev)))
 
 extern int scsi_change_queue_depth(struct scsi_device *, int);
 extern int scsi_track_queue_full(struct scsi_device *, int);
diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
index b9395676c75b..ee0b72075e9f 100644
--- a/include/scsi/scsi_host.h
+++ b/include/scsi/scsi_host.h
@@ -520,9 +520,8 @@ struct Scsi_Host {
 	 * their __ prefixed variants with the lock held. NEVER
 	 * access this list directly from a driver.
 	 */
-	struct list_head	__devices;
 	struct xarray		__targets;
-	
+
 	struct list_head	starved_list;
 
 	spinlock_t		default_lock;
-- 
2.16.4

Re: [PATCH 4/4] scsi: remove direct device lookup per host

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1836 bytes --]

Hi Hannes,

I love your patch! Perhaps something to improve:

[auto build test WARNING on mkp-scsi/for-next]
[also build test WARNING on scsi/for-next v5.7-rc7 next-20200526]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Hannes-Reinecke/scsi-use-xarray-for-devices-and-targets/20200527-231824
base:   https://git.kernel.org/pub/scm/linux/kernel/git/mkp/scsi.git for-next
config: x86_64-lkp (attached as .config)
compiler: gcc-8 (Debian 8.3.0-6) 8.3.0
reproduce (this is a W=1 build):
        # save the attached .config to linux build tree
        make W=1 ARCH=x86_64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kbuild test robot <lkp@intel.com>

All warnings (new ones prefixed by >>, old ones prefixed by <<):

>> drivers/scsi/scsi.c:638:21: warning: no previous prototype for '__scsi_target_lookup' [-Wmissing-prototypes]
struct scsi_target *__scsi_target_lookup(struct Scsi_Host *shost,
^~~~~~~~~~~~~~~~~~~~

vim +/__scsi_target_lookup +638 drivers/scsi/scsi.c

   630	
   631	/**
   632	 * __scsi_target_lookup  -  find a target based on channel and target id
   633	 * @shost:	SCSI host pointer
   634	 * @channel:	channel number of the target
   635	 * @id:		ID of the target
   636	 *
   637	 */
 > 638	struct scsi_target *__scsi_target_lookup(struct Scsi_Host *shost,
   639						 u16 channel, u16 id)
   640	{
   641		return xa_load(&shost->__targets, (channel << 16) | id);
   642	}
   643	EXPORT_SYMBOL(__scsi_target_lookup);
   644	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 28414 bytes --]

Re: [RFC PATCH 0/4] scsi: use xarray for devices and targets

[Bart Van Assche]

On 2020-05-27 07:13, Hannes Reinecke wrote:
> Hi all,
> 
> based on the ideas from Doug Gilbert here's now my take on using
> xarrays for devices and targets.
> It revolves around two ideas:
> - 'channel' and 'id' are never ever used to the full 32 bit range;
>   'channels' are well below 10, and no driver is using more than
>   16 bits for the id. So we can reduce the type of 'channel' and
>   'id' to 16 bits, and use the 32 bit value 'channel << 16 | id'
>   as the index into the target xarray.
> - Most SCSI LUNs are below 256 (to ensure compability with older
>   systems). So there we can use the LUN number as the index into
>   the xarray; for larger LUN numbers we'll allocate a separate
>   index.
> 
> With these change we can implement an efficient lookup mechanism,
> devolving into direct lookup for most cases.
> And iteration should be as efficient as the current, list-based,
> approach.
> 
> This is compile-tested only, to give you an impression of the
> overall idea and to get the discussion rolling.

Hi Hannes,

My understanding of the xarray concept is that it provides two
advantages over using linked lists:
- Faster lookups.
- Requires less memory.

Will we benefit from any of these advantages in the SCSI code? Hadn't
James Bottomley already brought up that lookup by (channel, target, lun)
only happens from some LLDs and from the procfs code?

Are there any use cases where the number of SCSI devices is large enough
to benefit from the memory reduction?

Thanks,

Bart.

Re: [RFC PATCH 0/4] scsi: use xarray for devices and targets

[Hannes Reinecke]

On 5/27/20 6:36 PM, Bart Van Assche wrote:
> On 2020-05-27 07:13, Hannes Reinecke wrote:
>> Hi all,
>>
>> based on the ideas from Doug Gilbert here's now my take on using
>> xarrays for devices and targets.
>> It revolves around two ideas:
>> - 'channel' and 'id' are never ever used to the full 32 bit range;
>>    'channels' are well below 10, and no driver is using more than
>>    16 bits for the id. So we can reduce the type of 'channel' and
>>    'id' to 16 bits, and use the 32 bit value 'channel << 16 | id'
>>    as the index into the target xarray.
>> - Most SCSI LUNs are below 256 (to ensure compability with older
>>    systems). So there we can use the LUN number as the index into
>>    the xarray; for larger LUN numbers we'll allocate a separate
>>    index.
>>
>> With these change we can implement an efficient lookup mechanism,
>> devolving into direct lookup for most cases.
>> And iteration should be as efficient as the current, list-based,
>> approach.
>>
>> This is compile-tested only, to give you an impression of the
>> overall idea and to get the discussion rolling.
> 
> Hi Hannes,
> 
> My understanding of the xarray concept is that it provides two
> advantages over using linked lists:
> - Faster lookups.
> - Requires less memory.
> 
> Will we benefit from any of these advantages in the SCSI code? Hadn't
> James Bottomley already brought up that lookup by (channel, target, lun)
> only happens from some LLDs and from the procfs code?
> 
It's not only lookup, it's iteration in general.
Which affects scanning and device removal; especially the latter is 
_very_ error prone (just look at scsi_target_reap etc), so any reduction 
in complexity is a good thing in general methinks.

> Are there any use cases where the number of SCSI devices is large enough
> to benefit from the memory reduction?
> 
I would assume that we're seeing benefits as soon as we're in the range 
of tens to hundreds of devices; then list lookup will be eating up more 
time and space as xarrays.

And the big benefit of using xarrays is that we will be alerted if an 
element with the same indices is being added; we've already had issues 
in the past here which are notoriously difficult to track down.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke            Teamlead Storage & Networking
hare@suse.de                               +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer

Re: [RFC PATCH 0/4] scsi: use xarray for devices and targets

[Douglas Gilbert]

On 2020-05-27 12:36 p.m., Bart Van Assche wrote:
> On 2020-05-27 07:13, Hannes Reinecke wrote:
>> Hi all,
>>
>> based on the ideas from Doug Gilbert here's now my take on using
>> xarrays for devices and targets.
>> It revolves around two ideas:
>> - 'channel' and 'id' are never ever used to the full 32 bit range;
>>    'channels' are well below 10, and no driver is using more than
>>    16 bits for the id. So we can reduce the type of 'channel' and
>>    'id' to 16 bits, and use the 32 bit value 'channel << 16 | id'
>>    as the index into the target xarray.
>> - Most SCSI LUNs are below 256 (to ensure compability with older
>>    systems). So there we can use the LUN number as the index into
>>    the xarray; for larger LUN numbers we'll allocate a separate
>>    index.
>>
>> With these change we can implement an efficient lookup mechanism,
>> devolving into direct lookup for most cases.
>> And iteration should be as efficient as the current, list-based,
>> approach.
>>
>> This is compile-tested only, to give you an impression of the
>> overall idea and to get the discussion rolling.
> 
> Hi Hannes,
> 
> My understanding of the xarray concept is that it provides two
> advantages over using linked lists:
> - Faster lookups.
> - Requires less memory.

Bart,
You might add these:
   - sane deletion semantics
   - inbuilt locking
   - inherently safer iterations, especially if marks are used

Matthew can probably add more.

> Will we benefit from any of these advantages in the SCSI code? Hadn't
> James Bottomley already brought up that lookup by (channel, target, lun)
> only happens from some LLDs and from the procfs code?

The way that the SCSI object tree hangs together with doubly linked
lists may have been a coherent design 20 years ago when JB wrote it,
but it has been white-anted big time since then.

The current state of that code is hard to defend. I have between 10 and 20
more examples of patently stupid things the current code does. See my
exchange with JB concerning the starget iterator over its sdevs that decided
to check on _every_ sdev in that host. That is done in several places.

The redundant sdevs in a shost collection is probably the most glaring
current design flaw.

> Are there any use cases where the number of SCSI devices is large enough
> to benefit from the memory reduction?

I don't believe that overall memory usage is a problem. Fitting the sdev_s
of hot devices in a smaller number of cache lines would help. That is
where Ming Lei was looking that kicked off this exercise that has
morphed into using xarray.

Doug Gilbert

[PATCHv2 0/4]

[Hannes Reinecke]

Hi all,

based on the ideas from Doug Gilbert here's now my take on using
xarrays for devices and targets.
It revolves around two ideas:

 - The scsi target 'channel' and 'id' numbers are never ever used
   to the full 32 bit range; channels are well below 10, and no
   driver is using more than 16 bits for the id. So we can reduce
   the type of 'channel' and 'id' to 16 bits, and use the 32 bit
   value 'channel << 16 | id' as the index into the target xarray.
 - Most SCSI LUNs are below 256 (to ensure compability with older
   systems). So there we can use the LUN number as the index into
   the xarray; for larger LUN numbers we'll allocate a separate
   index.

With these changes we can implement an efficient lookup mechanism,
devolving into direct lookup for most cases.
And iteration should be as efficient as the current, list-based,
approach.

This patchset now survives basic testing, hence I've removed
the 'RFC' tag from the initial patchset.

Changes to v1:
- Fixup __scsi_iterate_devices()
- Include reviews from Johannes
- Minor fixes
- Include comments from Doug

Hannes Reinecke (4):
  scsi: convert target lookup to xarray
  target_core_pscsi: use __scsi_device_lookup()
  scsi: move target device list to xarray
  scsi: remove direct device lookup per host

 drivers/scsi/hosts.c               |   3 +-
 drivers/scsi/scsi.c                | 131 ++++++++++++++++++++++++++++---------
 drivers/scsi/scsi_lib.c            |   9 ++-
 drivers/scsi/scsi_scan.c           |  66 ++++++++-----------
 drivers/scsi/scsi_sysfs.c          |  42 ++++++++----
 drivers/target/target_core_pscsi.c |   8 +--
 include/scsi/scsi_device.h         |  21 +++---
 include/scsi/scsi_host.h           |   5 +-
 8 files changed, 179 insertions(+), 106 deletions(-)

-- 
2.16.4

[PATCH 1/4] scsi: convert target lookup to xarray

[Hannes Reinecke]

Use an xarray instead of lists for holding the scsi targets.
I've also shortened the 'channel' and 'id' values to 16 bit
as none of the drivers requires a full 32bit range for either
of them, and by shortening them we can use them as the index
into the xarray for storing the scsi_target pointer.

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/scsi/hosts.c       |  2 +-
 drivers/scsi/scsi.c        | 33 +++++++++++++++++++++++----------
 drivers/scsi/scsi_scan.c   | 43 ++++++++++++++++---------------------------
 drivers/scsi/scsi_sysfs.c  | 15 +++++++++++----
 include/scsi/scsi_device.h |  4 ++--
 include/scsi/scsi_host.h   |  2 +-
 6 files changed, 54 insertions(+), 45 deletions(-)

diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
index 7ec91c3a66ca..7109afad0183 100644
--- a/drivers/scsi/hosts.c
+++ b/drivers/scsi/hosts.c
@@ -383,7 +383,7 @@ struct Scsi_Host *scsi_host_alloc(struct scsi_host_template *sht, int privsize)
 	spin_lock_init(shost->host_lock);
 	shost->shost_state = SHOST_CREATED;
 	INIT_LIST_HEAD(&shost->__devices);
-	INIT_LIST_HEAD(&shost->__targets);
+	xa_init(&shost->__targets);
 	INIT_LIST_HEAD(&shost->eh_cmd_q);
 	INIT_LIST_HEAD(&shost->starved_list);
 	init_waitqueue_head(&shost->host_wait);
diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index 56c24a73e0c7..d119ee7177e0 100644
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -575,6 +575,19 @@ struct scsi_device *__scsi_iterate_devices(struct Scsi_Host *shost,
 }
 EXPORT_SYMBOL(__scsi_iterate_devices);
 
+/**
+ * __scsi_target_lookup  -  find a target based on channel and target id
+ * @shost:	SCSI host pointer
+ * @channel:	channel number of the target
+ * @id:		ID of the target
+ *
+ */
+static struct scsi_target *__scsi_target_lookup(struct Scsi_Host *shost,
+					 u16 channel, u16 id)
+{
+	return xa_load(&shost->__targets, (channel << 16) | id);
+}
+
 /**
  * starget_for_each_device  -  helper to walk all devices of a target
  * @starget:	target whose devices we want to iterate over.
@@ -701,19 +714,19 @@ EXPORT_SYMBOL(scsi_device_lookup_by_target);
  * really want to use scsi_device_lookup instead.
  **/
 struct scsi_device *__scsi_device_lookup(struct Scsi_Host *shost,
-		uint channel, uint id, u64 lun)
+		u16 channel, u16 id, u64 lun)
 {
+	struct scsi_target *starget;
 	struct scsi_device *sdev;
 
-	list_for_each_entry(sdev, &shost->__devices, siblings) {
-		if (sdev->sdev_state == SDEV_DEL)
-			continue;
-		if (sdev->channel == channel && sdev->id == id &&
-				sdev->lun ==lun)
-			return sdev;
-	}
+	starget = __scsi_target_lookup(shost, channel, id);
+	if (!starget)
+		return NULL;
+	sdev = __scsi_device_lookup_by_target(starget, lun);
+	if (sdev && sdev->sdev_state == SDEV_DEL)
+		sdev = NULL;
 
-	return NULL;
+	return sdev;
 }
 EXPORT_SYMBOL(__scsi_device_lookup);
 
@@ -729,7 +742,7 @@ EXPORT_SYMBOL(__scsi_device_lookup);
  * needs to be released with scsi_device_put once you're done with it.
  **/
 struct scsi_device *scsi_device_lookup(struct Scsi_Host *shost,
-		uint channel, uint id, u64 lun)
+		u16 channel, u16 id, u64 lun)
 {
 	struct scsi_device *sdev;
 	unsigned long flags;
diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
index f2437a7570ce..dc2656df495b 100644
--- a/drivers/scsi/scsi_scan.c
+++ b/drivers/scsi/scsi_scan.c
@@ -304,11 +304,15 @@ static struct scsi_device *scsi_alloc_sdev(struct scsi_target *starget,
 	return NULL;
 }
 
+#define scsi_target_index(s) \
+	((((unsigned long)(s)->channel) << 16) | (s)->id)
+
 static void scsi_target_destroy(struct scsi_target *starget)
 {
 	struct device *dev = &starget->dev;
 	struct Scsi_Host *shost = dev_to_shost(dev->parent);
 	unsigned long flags;
+	unsigned long tid = scsi_target_index(starget);
 
 	BUG_ON(starget->state == STARGET_DEL);
 	starget->state = STARGET_DEL;
@@ -316,7 +320,7 @@ static void scsi_target_destroy(struct scsi_target *starget)
 	spin_lock_irqsave(shost->host_lock, flags);
 	if (shost->hostt->target_destroy)
 		shost->hostt->target_destroy(starget);
-	list_del_init(&starget->siblings);
+	xa_erase(&shost->__targets, tid);
 	spin_unlock_irqrestore(shost->host_lock, flags);
 	put_device(dev);
 }
@@ -341,27 +345,6 @@ int scsi_is_target_device(const struct device *dev)
 }
 EXPORT_SYMBOL(scsi_is_target_device);
 
-static struct scsi_target *__scsi_find_target(struct device *parent,
-					      int channel, uint id)
-{
-	struct scsi_target *starget, *found_starget = NULL;
-	struct Scsi_Host *shost = dev_to_shost(parent);
-	/*
-	 * Search for an existing target for this sdev.
-	 */
-	list_for_each_entry(starget, &shost->__targets, siblings) {
-		if (starget->id == id &&
-		    starget->channel == channel) {
-			found_starget = starget;
-			break;
-		}
-	}
-	if (found_starget)
-		get_device(&found_starget->dev);
-
-	return found_starget;
-}
-
 /**
  * scsi_target_reap_ref_release - remove target from visibility
  * @kref: the reap_ref in the target being released
@@ -417,6 +400,7 @@ static struct scsi_target *scsi_alloc_target(struct device *parent,
 	struct scsi_target *starget;
 	struct scsi_target *found_target;
 	int error, ref_got;
+	unsigned long tid;
 
 	starget = kzalloc(size, GFP_KERNEL);
 	if (!starget) {
@@ -433,19 +417,24 @@ static struct scsi_target *scsi_alloc_target(struct device *parent,
 	starget->id = id;
 	starget->channel = channel;
 	starget->can_queue = 0;
-	INIT_LIST_HEAD(&starget->siblings);
 	INIT_LIST_HEAD(&starget->devices);
 	starget->state = STARGET_CREATED;
 	starget->scsi_level = SCSI_2;
 	starget->max_target_blocked = SCSI_DEFAULT_TARGET_BLOCKED;
+	tid = scsi_target_index(starget);
  retry:
 	spin_lock_irqsave(shost->host_lock, flags);
 
-	found_target = __scsi_find_target(parent, channel, id);
-	if (found_target)
+	found_target = xa_load(&shost->__targets, tid);
+	if (found_target) {
+		get_device(&found_target->dev);
 		goto found;
-
-	list_add_tail(&starget->siblings, &shost->__targets);
+	}
+	if (xa_insert(&shost->__targets, tid, starget, GFP_KERNEL)) {
+		dev_printk(KERN_ERR, dev, "target index busy\n");
+		kfree(starget);
+		return NULL;
+	}
 	spin_unlock_irqrestore(shost->host_lock, flags);
 	/* allocate and add */
 	transport_setup_device(dev);
diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
index 163dbcb741c1..95aaa96ce03b 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -1512,15 +1512,19 @@ void scsi_remove_target(struct device *dev)
 {
 	struct Scsi_Host *shost = dev_to_shost(dev->parent);
 	struct scsi_target *starget;
+	unsigned long tid = 0;
 	unsigned long flags;
 
-restart:
 	spin_lock_irqsave(shost->host_lock, flags);
-	list_for_each_entry(starget, &shost->__targets, siblings) {
+	starget = xa_find(&shost->__targets, &tid, ULONG_MAX, XA_PRESENT);
+	while (starget) {
 		if (starget->state == STARGET_DEL ||
 		    starget->state == STARGET_REMOVE ||
-		    starget->state == STARGET_CREATED_REMOVE)
+		    starget->state == STARGET_CREATED_REMOVE) {
+			starget = xa_find_after(&shost->__targets, &tid,
+						ULONG_MAX, XA_PRESENT);
 			continue;
+		}
 		if (starget->dev.parent == dev || &starget->dev == dev) {
 			kref_get(&starget->reap_ref);
 			if (starget->state == STARGET_CREATED)
@@ -1530,7 +1534,10 @@ void scsi_remove_target(struct device *dev)
 			spin_unlock_irqrestore(shost->host_lock, flags);
 			__scsi_remove_target(starget);
 			scsi_target_reap(starget);
-			goto restart;
+			spin_lock_irqsave(shost->host_lock, flags);
+			starget = xa_find_after(&shost->__targets, &tid,
+						ULONG_MAX, XA_PRESENT);
+			continue;
 		}
 	}
 	spin_unlock_irqrestore(shost->host_lock, flags);
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index c3cba2aaf934..28034cc0fce5 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -345,9 +345,9 @@ extern struct scsi_device *scsi_device_from_queue(struct request_queue *q);
 extern int __must_check scsi_device_get(struct scsi_device *);
 extern void scsi_device_put(struct scsi_device *);
 extern struct scsi_device *scsi_device_lookup(struct Scsi_Host *,
-					      uint, uint, u64);
+					      u16, u16, u64);
 extern struct scsi_device *__scsi_device_lookup(struct Scsi_Host *,
-						uint, uint, u64);
+						u16, u16, u64);
 extern struct scsi_device *scsi_device_lookup_by_target(struct scsi_target *,
 							u64);
 extern struct scsi_device *__scsi_device_lookup_by_target(struct scsi_target *,
diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
index 822e8cda8d9b..b9395676c75b 100644
--- a/include/scsi/scsi_host.h
+++ b/include/scsi/scsi_host.h
@@ -521,7 +521,7 @@ struct Scsi_Host {
 	 * access this list directly from a driver.
 	 */
 	struct list_head	__devices;
-	struct list_head	__targets;
+	struct xarray		__targets;
 	
 	struct list_head	starved_list;
 
-- 
2.16.4

Re: [PATCH 1/4] scsi: convert target lookup to xarray

[Johannes Thumshirn]

On 28/05/2020 10:53, Hannes Reinecke wrote:
> +	sdev = __scsi_device_lookup_by_target(starget, lun);
> +	if (sdev && sdev->sdev_state == SDEV_DEL)
> +		sdev = NULL;

Nope, still there.

[PATCHv3 0/4] scsi: use xarray for devices and targets

[Hannes Reinecke]

Hi all,

based on the ideas from Doug Gilbert here's now my take on using
xarrays for devices and targets.
It revolves around two ideas:

 - The scsi target 'channel' and 'id' numbers are never ever used
   to the full 32 bit range; channels are well below 10, and no
   driver is using more than 16 bits for the id. So we can reduce
   the type of 'channel' and 'id' to 16 bits, and use the 32 bit
   value 'channel << 16 | id' as the index into the target xarray.
 - Most SCSI LUNs are below 256 (to ensure compability with older
   systems). So there we can use the LUN number as the index into
   the xarray; for larger LUN numbers we'll allocate a separate
   index.

With these changes we can implement an efficient lookup mechanism,
devolving into direct lookup for most cases.
And iteration should be as efficient as the current, list-based,
approach.

This patchset now survives basic testing, hence I've removed
the 'RFC' tag from the initial patchset.

Changes to v1:
- Fixup __scsi_iterate_devices()
- Include reviews from Johannes
- Minor fixes
- Include comments from Doug

Changes to v2:
- Reshuffle hunks as per suggestion from Johannes

Hannes Reinecke (4):
  scsi: convert target lookup to xarray
  target_core_pscsi: use __scsi_device_lookup()
  scsi: move target device list to xarray
  scsi: remove direct device lookup per host

 drivers/scsi/hosts.c               |   3 +-
 drivers/scsi/scsi.c                | 131 ++++++++++++++++++++++++++++---------
 drivers/scsi/scsi_lib.c            |   9 ++-
 drivers/scsi/scsi_scan.c           |  66 ++++++++-----------
 drivers/scsi/scsi_sysfs.c          |  42 ++++++++----
 drivers/target/target_core_pscsi.c |   8 +--
 include/scsi/scsi_device.h         |  21 +++---
 include/scsi/scsi_host.h           |   5 +-
 8 files changed, 179 insertions(+), 106 deletions(-)

-- 
2.16.4

[PATCH 1/4] scsi: convert target lookup to xarray

[Hannes Reinecke]

Use an xarray instead of lists for holding the scsi targets.
I've also shortened the 'channel' and 'id' values to 16 bit
as none of the drivers requires a full 32bit range for either
of them, and by shortening them we can use them as the index
into the xarray for storing the scsi_target pointer.

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/scsi/hosts.c       |  2 +-
 drivers/scsi/scsi.c        | 32 ++++++++++++++++++++------------
 drivers/scsi/scsi_scan.c   | 43 ++++++++++++++++---------------------------
 drivers/scsi/scsi_sysfs.c  | 15 +++++++++++----
 include/scsi/scsi_device.h |  4 ++--
 include/scsi/scsi_host.h   |  2 +-
 6 files changed, 51 insertions(+), 47 deletions(-)

diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
index 7ec91c3a66ca..7109afad0183 100644
--- a/drivers/scsi/hosts.c
+++ b/drivers/scsi/hosts.c
@@ -383,7 +383,7 @@ struct Scsi_Host *scsi_host_alloc(struct scsi_host_template *sht, int privsize)
 	spin_lock_init(shost->host_lock);
 	shost->shost_state = SHOST_CREATED;
 	INIT_LIST_HEAD(&shost->__devices);
-	INIT_LIST_HEAD(&shost->__targets);
+	xa_init(&shost->__targets);
 	INIT_LIST_HEAD(&shost->eh_cmd_q);
 	INIT_LIST_HEAD(&shost->starved_list);
 	init_waitqueue_head(&shost->host_wait);
diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index 56c24a73e0c7..d601424e32b2 100644
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -575,6 +575,19 @@ struct scsi_device *__scsi_iterate_devices(struct Scsi_Host *shost,
 }
 EXPORT_SYMBOL(__scsi_iterate_devices);
 
+/**
+ * __scsi_target_lookup  -  find a target based on channel and target id
+ * @shost:	SCSI host pointer
+ * @channel:	channel number of the target
+ * @id:		ID of the target
+ *
+ */
+static struct scsi_target *__scsi_target_lookup(struct Scsi_Host *shost,
+					 u16 channel, u16 id)
+{
+	return xa_load(&shost->__targets, (channel << 16) | id);
+}
+
 /**
  * starget_for_each_device  -  helper to walk all devices of a target
  * @starget:	target whose devices we want to iterate over.
@@ -701,19 +714,14 @@ EXPORT_SYMBOL(scsi_device_lookup_by_target);
  * really want to use scsi_device_lookup instead.
  **/
 struct scsi_device *__scsi_device_lookup(struct Scsi_Host *shost,
-		uint channel, uint id, u64 lun)
+		u16 channel, u16 id, u64 lun)
 {
-	struct scsi_device *sdev;
+	struct scsi_target *starget;
 
-	list_for_each_entry(sdev, &shost->__devices, siblings) {
-		if (sdev->sdev_state == SDEV_DEL)
-			continue;
-		if (sdev->channel == channel && sdev->id == id &&
-				sdev->lun ==lun)
-			return sdev;
-	}
-
-	return NULL;
+	starget = __scsi_target_lookup(shost, channel, id);
+	if (!starget)
+		return NULL;
+	return __scsi_device_lookup_by_target(starget, lun);
 }
 EXPORT_SYMBOL(__scsi_device_lookup);
 
@@ -729,7 +737,7 @@ EXPORT_SYMBOL(__scsi_device_lookup);
  * needs to be released with scsi_device_put once you're done with it.
  **/
 struct scsi_device *scsi_device_lookup(struct Scsi_Host *shost,
-		uint channel, uint id, u64 lun)
+		u16 channel, u16 id, u64 lun)
 {
 	struct scsi_device *sdev;
 	unsigned long flags;
diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
index f2437a7570ce..dc2656df495b 100644
--- a/drivers/scsi/scsi_scan.c
+++ b/drivers/scsi/scsi_scan.c
@@ -304,11 +304,15 @@ static struct scsi_device *scsi_alloc_sdev(struct scsi_target *starget,
 	return NULL;
 }
 
+#define scsi_target_index(s) \
+	((((unsigned long)(s)->channel) << 16) | (s)->id)
+
 static void scsi_target_destroy(struct scsi_target *starget)
 {
 	struct device *dev = &starget->dev;
 	struct Scsi_Host *shost = dev_to_shost(dev->parent);
 	unsigned long flags;
+	unsigned long tid = scsi_target_index(starget);
 
 	BUG_ON(starget->state == STARGET_DEL);
 	starget->state = STARGET_DEL;
@@ -316,7 +320,7 @@ static void scsi_target_destroy(struct scsi_target *starget)
 	spin_lock_irqsave(shost->host_lock, flags);
 	if (shost->hostt->target_destroy)
 		shost->hostt->target_destroy(starget);
-	list_del_init(&starget->siblings);
+	xa_erase(&shost->__targets, tid);
 	spin_unlock_irqrestore(shost->host_lock, flags);
 	put_device(dev);
 }
@@ -341,27 +345,6 @@ int scsi_is_target_device(const struct device *dev)
 }
 EXPORT_SYMBOL(scsi_is_target_device);
 
-static struct scsi_target *__scsi_find_target(struct device *parent,
-					      int channel, uint id)
-{
-	struct scsi_target *starget, *found_starget = NULL;
-	struct Scsi_Host *shost = dev_to_shost(parent);
-	/*
-	 * Search for an existing target for this sdev.
-	 */
-	list_for_each_entry(starget, &shost->__targets, siblings) {
-		if (starget->id == id &&
-		    starget->channel == channel) {
-			found_starget = starget;
-			break;
-		}
-	}
-	if (found_starget)
-		get_device(&found_starget->dev);
-
-	return found_starget;
-}
-
 /**
  * scsi_target_reap_ref_release - remove target from visibility
  * @kref: the reap_ref in the target being released
@@ -417,6 +400,7 @@ static struct scsi_target *scsi_alloc_target(struct device *parent,
 	struct scsi_target *starget;
 	struct scsi_target *found_target;
 	int error, ref_got;
+	unsigned long tid;
 
 	starget = kzalloc(size, GFP_KERNEL);
 	if (!starget) {
@@ -433,19 +417,24 @@ static struct scsi_target *scsi_alloc_target(struct device *parent,
 	starget->id = id;
 	starget->channel = channel;
 	starget->can_queue = 0;
-	INIT_LIST_HEAD(&starget->siblings);
 	INIT_LIST_HEAD(&starget->devices);
 	starget->state = STARGET_CREATED;
 	starget->scsi_level = SCSI_2;
 	starget->max_target_blocked = SCSI_DEFAULT_TARGET_BLOCKED;
+	tid = scsi_target_index(starget);
  retry:
 	spin_lock_irqsave(shost->host_lock, flags);
 
-	found_target = __scsi_find_target(parent, channel, id);
-	if (found_target)
+	found_target = xa_load(&shost->__targets, tid);
+	if (found_target) {
+		get_device(&found_target->dev);
 		goto found;
-
-	list_add_tail(&starget->siblings, &shost->__targets);
+	}
+	if (xa_insert(&shost->__targets, tid, starget, GFP_KERNEL)) {
+		dev_printk(KERN_ERR, dev, "target index busy\n");
+		kfree(starget);
+		return NULL;
+	}
 	spin_unlock_irqrestore(shost->host_lock, flags);
 	/* allocate and add */
 	transport_setup_device(dev);
diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
index 163dbcb741c1..95aaa96ce03b 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -1512,15 +1512,19 @@ void scsi_remove_target(struct device *dev)
 {
 	struct Scsi_Host *shost = dev_to_shost(dev->parent);
 	struct scsi_target *starget;
+	unsigned long tid = 0;
 	unsigned long flags;
 
-restart:
 	spin_lock_irqsave(shost->host_lock, flags);
-	list_for_each_entry(starget, &shost->__targets, siblings) {
+	starget = xa_find(&shost->__targets, &tid, ULONG_MAX, XA_PRESENT);
+	while (starget) {
 		if (starget->state == STARGET_DEL ||
 		    starget->state == STARGET_REMOVE ||
-		    starget->state == STARGET_CREATED_REMOVE)
+		    starget->state == STARGET_CREATED_REMOVE) {
+			starget = xa_find_after(&shost->__targets, &tid,
+						ULONG_MAX, XA_PRESENT);
 			continue;
+		}
 		if (starget->dev.parent == dev || &starget->dev == dev) {
 			kref_get(&starget->reap_ref);
 			if (starget->state == STARGET_CREATED)
@@ -1530,7 +1534,10 @@ void scsi_remove_target(struct device *dev)
 			spin_unlock_irqrestore(shost->host_lock, flags);
 			__scsi_remove_target(starget);
 			scsi_target_reap(starget);
-			goto restart;
+			spin_lock_irqsave(shost->host_lock, flags);
+			starget = xa_find_after(&shost->__targets, &tid,
+						ULONG_MAX, XA_PRESENT);
+			continue;
 		}
 	}
 	spin_unlock_irqrestore(shost->host_lock, flags);
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index c3cba2aaf934..28034cc0fce5 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -345,9 +345,9 @@ extern struct scsi_device *scsi_device_from_queue(struct request_queue *q);
 extern int __must_check scsi_device_get(struct scsi_device *);
 extern void scsi_device_put(struct scsi_device *);
 extern struct scsi_device *scsi_device_lookup(struct Scsi_Host *,
-					      uint, uint, u64);
+					      u16, u16, u64);
 extern struct scsi_device *__scsi_device_lookup(struct Scsi_Host *,
-						uint, uint, u64);
+						u16, u16, u64);
 extern struct scsi_device *scsi_device_lookup_by_target(struct scsi_target *,
 							u64);
 extern struct scsi_device *__scsi_device_lookup_by_target(struct scsi_target *,
diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
index 822e8cda8d9b..b9395676c75b 100644
--- a/include/scsi/scsi_host.h
+++ b/include/scsi/scsi_host.h
@@ -521,7 +521,7 @@ struct Scsi_Host {
 	 * access this list directly from a driver.
 	 */
 	struct list_head	__devices;
-	struct list_head	__targets;
+	struct xarray		__targets;
 	
 	struct list_head	starved_list;
 
-- 
2.16.4

Re: [PATCH 1/4] scsi: convert target lookup to xarray

[Douglas Gilbert]

The following suggestions are based on trying to _run_ this code ...
foolish, I know.

On 2020-05-28 12:36 p.m., Hannes Reinecke wrote:
> Use an xarray instead of lists for holding the scsi targets.
> I've also shortened the 'channel' and 'id' values to 16 bit
> as none of the drivers requires a full 32bit range for either
> of them, and by shortening them we can use them as the index
> into the xarray for storing the scsi_target pointer.
> 
> Signed-off-by: Hannes Reinecke <hare@suse.de>
> ---
>   drivers/scsi/hosts.c       |  2 +-
>   drivers/scsi/scsi.c        | 32 ++++++++++++++++++++------------
>   drivers/scsi/scsi_scan.c   | 43 ++++++++++++++++---------------------------
>   drivers/scsi/scsi_sysfs.c  | 15 +++++++++++----
>   include/scsi/scsi_device.h |  4 ++--
>   include/scsi/scsi_host.h   |  2 +-
>   6 files changed, 51 insertions(+), 47 deletions(-)
> 
> diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
> index 7ec91c3a66ca..7109afad0183 100644
> --- a/drivers/scsi/hosts.c
> +++ b/drivers/scsi/hosts.c
> @@ -383,7 +383,7 @@ struct Scsi_Host *scsi_host_alloc(struct scsi_host_template *sht, int privsize)
>   	spin_lock_init(shost->host_lock);
>   	shost->shost_state = SHOST_CREATED;
>   	INIT_LIST_HEAD(&shost->__devices);
> -	INIT_LIST_HEAD(&shost->__targets);
> +	xa_init(&shost->__targets);

Please use: xa_init_flags(&shost->__targets, XA_FLAGS_LOCK_IRQ);

If you ever touch (modify?) that array in interrupt context then I
believe you need this. See the first 50 or so lines in lib/xarray.c
to see that it is used for xarray internals (e.g. xas_lock_type() ).

The xarray interface (include/linux/xarray.h) also has *_irq() and
*_bh() variants (e.g. xa_insert(), xa_insert_bh(), xa_insert_irq() )
but setting that flag in the xa_init_flags() call seems to be a
safer approach. IOWs it is an alternative to using the *_irq()
variants on all the _modifying_ xarray calls.

>   	INIT_LIST_HEAD(&shost->eh_cmd_q);
>   	INIT_LIST_HEAD(&shost->starved_list);
>   	init_waitqueue_head(&shost->host_wait);
> diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
> index 56c24a73e0c7..d601424e32b2 100644
> --- a/drivers/scsi/scsi.c
> +++ b/drivers/scsi/scsi.c
> @@ -575,6 +575,19 @@ struct scsi_device *__scsi_iterate_devices(struct Scsi_Host *shost,
>   }
>   EXPORT_SYMBOL(__scsi_iterate_devices);
>   
> +/**
> + * __scsi_target_lookup  -  find a target based on channel and target id
> + * @shost:	SCSI host pointer
> + * @channel:	channel number of the target
> + * @id:		ID of the target
> + *
> + */
> +static struct scsi_target *__scsi_target_lookup(struct Scsi_Host *shost,
> +					 u16 channel, u16 id)
> +{
> +	return xa_load(&shost->__targets, (channel << 16) | id);
> +}
> +
>   /**
>    * starget_for_each_device  -  helper to walk all devices of a target
>    * @starget:	target whose devices we want to iterate over.
> @@ -701,19 +714,14 @@ EXPORT_SYMBOL(scsi_device_lookup_by_target);
>    * really want to use scsi_device_lookup instead.
>    **/
>   struct scsi_device *__scsi_device_lookup(struct Scsi_Host *shost,
> -		uint channel, uint id, u64 lun)
> +		u16 channel, u16 id, u64 lun)
>   {
> -	struct scsi_device *sdev;
> +	struct scsi_target *starget;
>   
> -	list_for_each_entry(sdev, &shost->__devices, siblings) {
> -		if (sdev->sdev_state == SDEV_DEL)
> -			continue;
> -		if (sdev->channel == channel && sdev->id == id &&
> -				sdev->lun ==lun)
> -			return sdev;
> -	}
> -
> -	return NULL;
> +	starget = __scsi_target_lookup(shost, channel, id);
> +	if (!starget)
> +		return NULL;
> +	return __scsi_device_lookup_by_target(starget, lun);
>   }
>   EXPORT_SYMBOL(__scsi_device_lookup);
>   
> @@ -729,7 +737,7 @@ EXPORT_SYMBOL(__scsi_device_lookup);
>    * needs to be released with scsi_device_put once you're done with it.
>    **/
>   struct scsi_device *scsi_device_lookup(struct Scsi_Host *shost,
> -		uint channel, uint id, u64 lun)
> +		u16 channel, u16 id, u64 lun)
>   {
>   	struct scsi_device *sdev;
>   	unsigned long flags;
> diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
> index f2437a7570ce..dc2656df495b 100644
> --- a/drivers/scsi/scsi_scan.c
> +++ b/drivers/scsi/scsi_scan.c
> @@ -304,11 +304,15 @@ static struct scsi_device *scsi_alloc_sdev(struct scsi_target *starget,
>   	return NULL;
>   }
>   
> +#define scsi_target_index(s) \
> +	((((unsigned long)(s)->channel) << 16) | (s)->id)
> +
>   static void scsi_target_destroy(struct scsi_target *starget)
>   {
>   	struct device *dev = &starget->dev;
>   	struct Scsi_Host *shost = dev_to_shost(dev->parent);
>   	unsigned long flags;
> +	unsigned long tid = scsi_target_index(starget);
>   
>   	BUG_ON(starget->state == STARGET_DEL);
>   	starget->state = STARGET_DEL;
> @@ -316,7 +320,7 @@ static void scsi_target_destroy(struct scsi_target *starget)
>   	spin_lock_irqsave(shost->host_lock, flags);
>   	if (shost->hostt->target_destroy)
>   		shost->hostt->target_destroy(starget);
> -	list_del_init(&starget->siblings);
> +	xa_erase(&shost->__targets, tid);
>   	spin_unlock_irqrestore(shost->host_lock, flags);
>   	put_device(dev);
>   }
> @@ -341,27 +345,6 @@ int scsi_is_target_device(const struct device *dev)
>   }
>   EXPORT_SYMBOL(scsi_is_target_device);
>   
> -static struct scsi_target *__scsi_find_target(struct device *parent,
> -					      int channel, uint id)
> -{
> -	struct scsi_target *starget, *found_starget = NULL;
> -	struct Scsi_Host *shost = dev_to_shost(parent);
> -	/*
> -	 * Search for an existing target for this sdev.
> -	 */
> -	list_for_each_entry(starget, &shost->__targets, siblings) {
> -		if (starget->id == id &&
> -		    starget->channel == channel) {
> -			found_starget = starget;
> -			break;
> -		}
> -	}
> -	if (found_starget)
> -		get_device(&found_starget->dev);
> -
> -	return found_starget;
> -}
> -
>   /**
>    * scsi_target_reap_ref_release - remove target from visibility
>    * @kref: the reap_ref in the target being released
> @@ -417,6 +400,7 @@ static struct scsi_target *scsi_alloc_target(struct device *parent,
>   	struct scsi_target *starget;
>   	struct scsi_target *found_target;
>   	int error, ref_got;
> +	unsigned long tid;
>   
>   	starget = kzalloc(size, GFP_KERNEL);
>   	if (!starget) {
> @@ -433,19 +417,24 @@ static struct scsi_target *scsi_alloc_target(struct device *parent,
>   	starget->id = id;
>   	starget->channel = channel;
>   	starget->can_queue = 0;
> -	INIT_LIST_HEAD(&starget->siblings);
>   	INIT_LIST_HEAD(&starget->devices);
>   	starget->state = STARGET_CREATED;
>   	starget->scsi_level = SCSI_2;
>   	starget->max_target_blocked = SCSI_DEFAULT_TARGET_BLOCKED;
> +	tid = scsi_target_index(starget);
>    retry:
>   	spin_lock_irqsave(shost->host_lock, flags);
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Interrupts now masked on this CPU.

>   
> -	found_target = __scsi_find_target(parent, channel, id);
> -	if (found_target)
> +	found_target = xa_load(&shost->__targets, tid);
> +	if (found_target) {
> +		get_device(&found_target->dev);
>   		goto found;
> -
> -	list_add_tail(&starget->siblings, &shost->__targets);
> +	}
> +	if (xa_insert(&shost->__targets, tid, starget, GFP_KERNEL)) {

You have interrupts masked (on this CPU), you can't use GFP_KERNEL.
If you run this code, you will be told :-)
In this case, you must use GFP_ATOMIC.

> +		dev_printk(KERN_ERR, dev, "target index busy\n");
> +		kfree(starget);
> +		return NULL;
> +	}
>   	spin_unlock_irqrestore(shost->host_lock, flags);
^^^^^^^^^^^^^^^^^^^^
Now you can use GFP_KERNEL, if you want.

>   	/* allocate and add */
>   	transport_setup_device(dev);
> diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
> index 163dbcb741c1..95aaa96ce03b 100644
> --- a/drivers/scsi/scsi_sysfs.c
> +++ b/drivers/scsi/scsi_sysfs.c
> @@ -1512,15 +1512,19 @@ void scsi_remove_target(struct device *dev)
>   {
>   	struct Scsi_Host *shost = dev_to_shost(dev->parent);
>   	struct scsi_target *starget;
> +	unsigned long tid = 0;
>   	unsigned long flags;
>   
> -restart:
>   	spin_lock_irqsave(shost->host_lock, flags);
> -	list_for_each_entry(starget, &shost->__targets, siblings) {
> +	starget = xa_find(&shost->__targets, &tid, ULONG_MAX, XA_PRESENT);
> +	while (starget) {
>   		if (starget->state == STARGET_DEL ||
>   		    starget->state == STARGET_REMOVE ||
> -		    starget->state == STARGET_CREATED_REMOVE)
> +		    starget->state == STARGET_CREATED_REMOVE) {
> +			starget = xa_find_after(&shost->__targets, &tid,
> +						ULONG_MAX, XA_PRESENT);
>   			continue;
> +		}
>   		if (starget->dev.parent == dev || &starget->dev == dev) {
>   			kref_get(&starget->reap_ref);
>   			if (starget->state == STARGET_CREATED)
> @@ -1530,7 +1534,10 @@ void scsi_remove_target(struct device *dev)
>   			spin_unlock_irqrestore(shost->host_lock, flags);
>   			__scsi_remove_target(starget);
>   			scsi_target_reap(starget);
> -			goto restart;
> +			spin_lock_irqsave(shost->host_lock, flags);
> +			starget = xa_find_after(&shost->__targets, &tid,
> +						ULONG_MAX, XA_PRESENT);
> +			continue;
>   		}
>   	}
>   	spin_unlock_irqrestore(shost->host_lock, flags);
> diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
> index c3cba2aaf934..28034cc0fce5 100644
> --- a/include/scsi/scsi_device.h
> +++ b/include/scsi/scsi_device.h
> @@ -345,9 +345,9 @@ extern struct scsi_device *scsi_device_from_queue(struct request_queue *q);
>   extern int __must_check scsi_device_get(struct scsi_device *);
>   extern void scsi_device_put(struct scsi_device *);
>   extern struct scsi_device *scsi_device_lookup(struct Scsi_Host *,
> -					      uint, uint, u64);
> +					      u16, u16, u64);
>   extern struct scsi_device *__scsi_device_lookup(struct Scsi_Host *,
> -						uint, uint, u64);
> +						u16, u16, u64);
>   extern struct scsi_device *scsi_device_lookup_by_target(struct scsi_target *,
>   							u64);
>   extern struct scsi_device *__scsi_device_lookup_by_target(struct scsi_target *,
> diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
> index 822e8cda8d9b..b9395676c75b 100644
> --- a/include/scsi/scsi_host.h
> +++ b/include/scsi/scsi_host.h
> @@ -521,7 +521,7 @@ struct Scsi_Host {
>   	 * access this list directly from a driver.
>   	 */
>   	struct list_head	__devices;
> -	struct list_head	__targets;
> +	struct xarray		__targets;
>   	
>   	struct list_head	starved_list;
>   
> 

Re: [PATCH 1/4] scsi: convert target lookup to xarray

[Douglas Gilbert]

On 2020-05-28 1:18 p.m., Douglas Gilbert wrote:
> The following suggestions are based on trying to _run_ this code ...
> foolish, I know.

Now with the patches I've suggested it runs. So I ran my modprobe,
rmmod test building gradually out to 6000 odd devices. It ran
for a few minutes then got clobbered by the OOM killer.

Perhaps there are some misplaced xa_destroy()s, I thought.
After grep-ing, "un-placed" would be a better description.

So Hannes, for each xa_init*() call made, there needs to be a
xa_destroy() at the appropriate place and time, perferably before
the OOM killer strikes :-)

Doug Gilbert

> On 2020-05-28 12:36 p.m., Hannes Reinecke wrote:
>> Use an xarray instead of lists for holding the scsi targets.
>> I've also shortened the 'channel' and 'id' values to 16 bit
>> as none of the drivers requires a full 32bit range for either
>> of them, and by shortening them we can use them as the index
>> into the xarray for storing the scsi_target pointer.
>>
>> Signed-off-by: Hannes Reinecke <hare@suse.de>
>> ---
>>   drivers/scsi/hosts.c       |  2 +-
>>   drivers/scsi/scsi.c        | 32 ++++++++++++++++++++------------
>>   drivers/scsi/scsi_scan.c   | 43 ++++++++++++++++---------------------------
>>   drivers/scsi/scsi_sysfs.c  | 15 +++++++++++----
>>   include/scsi/scsi_device.h |  4 ++--
>>   include/scsi/scsi_host.h   |  2 +-
>>   6 files changed, 51 insertions(+), 47 deletions(-)
>>
>> diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
>> index 7ec91c3a66ca..7109afad0183 100644
>> --- a/drivers/scsi/hosts.c
>> +++ b/drivers/scsi/hosts.c
>> @@ -383,7 +383,7 @@ struct Scsi_Host *scsi_host_alloc(struct 
>> scsi_host_template *sht, int privsize)
>>       spin_lock_init(shost->host_lock);
>>       shost->shost_state = SHOST_CREATED;
>>       INIT_LIST_HEAD(&shost->__devices);
>> -    INIT_LIST_HEAD(&shost->__targets);
>> +    xa_init(&shost->__targets);
> 
> Please use: xa_init_flags(&shost->__targets, XA_FLAGS_LOCK_IRQ);
> 
> If you ever touch (modify?) that array in interrupt context then I
> believe you need this. See the first 50 or so lines in lib/xarray.c
> to see that it is used for xarray internals (e.g. xas_lock_type() ).
> 
> The xarray interface (include/linux/xarray.h) also has *_irq() and
> *_bh() variants (e.g. xa_insert(), xa_insert_bh(), xa_insert_irq() )
> but setting that flag in the xa_init_flags() call seems to be a
> safer approach. IOWs it is an alternative to using the *_irq()
> variants on all the _modifying_ xarray calls.
> 
>>       INIT_LIST_HEAD(&shost->eh_cmd_q);
>>       INIT_LIST_HEAD(&shost->starved_list);
>>       init_waitqueue_head(&shost->host_wait);
>> diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
>> index 56c24a73e0c7..d601424e32b2 100644
>> --- a/drivers/scsi/scsi.c
>> +++ b/drivers/scsi/scsi.c
>> @@ -575,6 +575,19 @@ struct scsi_device *__scsi_iterate_devices(struct 
>> Scsi_Host *shost,
>>   }
>>   EXPORT_SYMBOL(__scsi_iterate_devices);
>> +/**
>> + * __scsi_target_lookup  -  find a target based on channel and target id
>> + * @shost:    SCSI host pointer
>> + * @channel:    channel number of the target
>> + * @id:        ID of the target
>> + *
>> + */
>> +static struct scsi_target *__scsi_target_lookup(struct Scsi_Host *shost,
>> +                     u16 channel, u16 id)
>> +{
>> +    return xa_load(&shost->__targets, (channel << 16) | id);
>> +}
>> +
>>   /**
>>    * starget_for_each_device  -  helper to walk all devices of a target
>>    * @starget:    target whose devices we want to iterate over.
>> @@ -701,19 +714,14 @@ EXPORT_SYMBOL(scsi_device_lookup_by_target);
>>    * really want to use scsi_device_lookup instead.
>>    **/
>>   struct scsi_device *__scsi_device_lookup(struct Scsi_Host *shost,
>> -        uint channel, uint id, u64 lun)
>> +        u16 channel, u16 id, u64 lun)
>>   {
>> -    struct scsi_device *sdev;
>> +    struct scsi_target *starget;
>> -    list_for_each_entry(sdev, &shost->__devices, siblings) {
>> -        if (sdev->sdev_state == SDEV_DEL)
>> -            continue;
>> -        if (sdev->channel == channel && sdev->id == id &&
>> -                sdev->lun ==lun)
>> -            return sdev;
>> -    }
>> -
>> -    return NULL;
>> +    starget = __scsi_target_lookup(shost, channel, id);
>> +    if (!starget)
>> +        return NULL;
>> +    return __scsi_device_lookup_by_target(starget, lun);
>>   }
>>   EXPORT_SYMBOL(__scsi_device_lookup);
>> @@ -729,7 +737,7 @@ EXPORT_SYMBOL(__scsi_device_lookup);
>>    * needs to be released with scsi_device_put once you're done with it.
>>    **/
>>   struct scsi_device *scsi_device_lookup(struct Scsi_Host *shost,
>> -        uint channel, uint id, u64 lun)
>> +        u16 channel, u16 id, u64 lun)
>>   {
>>       struct scsi_device *sdev;
>>       unsigned long flags;
>> diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
>> index f2437a7570ce..dc2656df495b 100644
>> --- a/drivers/scsi/scsi_scan.c
>> +++ b/drivers/scsi/scsi_scan.c
>> @@ -304,11 +304,15 @@ static struct scsi_device *scsi_alloc_sdev(struct 
>> scsi_target *starget,
>>       return NULL;
>>   }
>> +#define scsi_target_index(s) \
>> +    ((((unsigned long)(s)->channel) << 16) | (s)->id)
>> +
>>   static void scsi_target_destroy(struct scsi_target *starget)
>>   {
>>       struct device *dev = &starget->dev;
>>       struct Scsi_Host *shost = dev_to_shost(dev->parent);
>>       unsigned long flags;
>> +    unsigned long tid = scsi_target_index(starget);
>>       BUG_ON(starget->state == STARGET_DEL);
>>       starget->state = STARGET_DEL;
>> @@ -316,7 +320,7 @@ static void scsi_target_destroy(struct scsi_target *starget)
>>       spin_lock_irqsave(shost->host_lock, flags);
>>       if (shost->hostt->target_destroy)
>>           shost->hostt->target_destroy(starget);
>> -    list_del_init(&starget->siblings);
>> +    xa_erase(&shost->__targets, tid);
>>       spin_unlock_irqrestore(shost->host_lock, flags);
>>       put_device(dev);
>>   }
>> @@ -341,27 +345,6 @@ int scsi_is_target_device(const struct device *dev)
>>   }
>>   EXPORT_SYMBOL(scsi_is_target_device);
>> -static struct scsi_target *__scsi_find_target(struct device *parent,
>> -                          int channel, uint id)
>> -{
>> -    struct scsi_target *starget, *found_starget = NULL;
>> -    struct Scsi_Host *shost = dev_to_shost(parent);
>> -    /*
>> -     * Search for an existing target for this sdev.
>> -     */
>> -    list_for_each_entry(starget, &shost->__targets, siblings) {
>> -        if (starget->id == id &&
>> -            starget->channel == channel) {
>> -            found_starget = starget;
>> -            break;
>> -        }
>> -    }
>> -    if (found_starget)
>> -        get_device(&found_starget->dev);
>> -
>> -    return found_starget;
>> -}
>> -
>>   /**
>>    * scsi_target_reap_ref_release - remove target from visibility
>>    * @kref: the reap_ref in the target being released
>> @@ -417,6 +400,7 @@ static struct scsi_target *scsi_alloc_target(struct device 
>> *parent,
>>       struct scsi_target *starget;
>>       struct scsi_target *found_target;
>>       int error, ref_got;
>> +    unsigned long tid;
>>       starget = kzalloc(size, GFP_KERNEL);
>>       if (!starget) {
>> @@ -433,19 +417,24 @@ static struct scsi_target *scsi_alloc_target(struct 
>> device *parent,
>>       starget->id = id;
>>       starget->channel = channel;
>>       starget->can_queue = 0;
>> -    INIT_LIST_HEAD(&starget->siblings);
>>       INIT_LIST_HEAD(&starget->devices);
>>       starget->state = STARGET_CREATED;
>>       starget->scsi_level = SCSI_2;
>>       starget->max_target_blocked = SCSI_DEFAULT_TARGET_BLOCKED;
>> +    tid = scsi_target_index(starget);
>>    retry:
>>       spin_lock_irqsave(shost->host_lock, flags);
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> Interrupts now masked on this CPU.
> 
>> -    found_target = __scsi_find_target(parent, channel, id);
>> -    if (found_target)
>> +    found_target = xa_load(&shost->__targets, tid);
>> +    if (found_target) {
>> +        get_device(&found_target->dev);
>>           goto found;
>> -
>> -    list_add_tail(&starget->siblings, &shost->__targets);
>> +    }
>> +    if (xa_insert(&shost->__targets, tid, starget, GFP_KERNEL)) {
> 
> You have interrupts masked (on this CPU), you can't use GFP_KERNEL.
> If you run this code, you will be told :-)
> In this case, you must use GFP_ATOMIC.
> 
>> +        dev_printk(KERN_ERR, dev, "target index busy\n");
>> +        kfree(starget);
>> +        return NULL;
>> +    }
>>       spin_unlock_irqrestore(shost->host_lock, flags);
> ^^^^^^^^^^^^^^^^^^^^
> Now you can use GFP_KERNEL, if you want.
> 
>>       /* allocate and add */
>>       transport_setup_device(dev);
>> diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
>> index 163dbcb741c1..95aaa96ce03b 100644
>> --- a/drivers/scsi/scsi_sysfs.c
>> +++ b/drivers/scsi/scsi_sysfs.c
>> @@ -1512,15 +1512,19 @@ void scsi_remove_target(struct device *dev)
>>   {
>>       struct Scsi_Host *shost = dev_to_shost(dev->parent);
>>       struct scsi_target *starget;
>> +    unsigned long tid = 0;
>>       unsigned long flags;
>> -restart:
>>       spin_lock_irqsave(shost->host_lock, flags);
>> -    list_for_each_entry(starget, &shost->__targets, siblings) {
>> +    starget = xa_find(&shost->__targets, &tid, ULONG_MAX, XA_PRESENT);
>> +    while (starget) {
>>           if (starget->state == STARGET_DEL ||
>>               starget->state == STARGET_REMOVE ||
>> -            starget->state == STARGET_CREATED_REMOVE)
>> +            starget->state == STARGET_CREATED_REMOVE) {
>> +            starget = xa_find_after(&shost->__targets, &tid,
>> +                        ULONG_MAX, XA_PRESENT);
>>               continue;
>> +        }
>>           if (starget->dev.parent == dev || &starget->dev == dev) {
>>               kref_get(&starget->reap_ref);
>>               if (starget->state == STARGET_CREATED)
>> @@ -1530,7 +1534,10 @@ void scsi_remove_target(struct device *dev)
>>               spin_unlock_irqrestore(shost->host_lock, flags);
>>               __scsi_remove_target(starget);
>>               scsi_target_reap(starget);
>> -            goto restart;
>> +            spin_lock_irqsave(shost->host_lock, flags);
>> +            starget = xa_find_after(&shost->__targets, &tid,
>> +                        ULONG_MAX, XA_PRESENT);
>> +            continue;
>>           }
>>       }
>>       spin_unlock_irqrestore(shost->host_lock, flags);
>> diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
>> index c3cba2aaf934..28034cc0fce5 100644
>> --- a/include/scsi/scsi_device.h
>> +++ b/include/scsi/scsi_device.h
>> @@ -345,9 +345,9 @@ extern struct scsi_device *scsi_device_from_queue(struct 
>> request_queue *q);
>>   extern int __must_check scsi_device_get(struct scsi_device *);
>>   extern void scsi_device_put(struct scsi_device *);
>>   extern struct scsi_device *scsi_device_lookup(struct Scsi_Host *,
>> -                          uint, uint, u64);
>> +                          u16, u16, u64);
>>   extern struct scsi_device *__scsi_device_lookup(struct Scsi_Host *,
>> -                        uint, uint, u64);
>> +                        u16, u16, u64);
>>   extern struct scsi_device *scsi_device_lookup_by_target(struct scsi_target *,
>>                               u64);
>>   extern struct scsi_device *__scsi_device_lookup_by_target(struct scsi_target *,
>> diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
>> index 822e8cda8d9b..b9395676c75b 100644
>> --- a/include/scsi/scsi_host.h
>> +++ b/include/scsi/scsi_host.h
>> @@ -521,7 +521,7 @@ struct Scsi_Host {
>>        * access this list directly from a driver.
>>        */
>>       struct list_head    __devices;
>> -    struct list_head    __targets;
>> +    struct xarray        __targets;
>>
>>       struct list_head    starved_list;
>>
> 

Re: [PATCH 1/4] scsi: convert target lookup to xarray

[Bart Van Assche]

On 2020-05-28 09:36, Hannes Reinecke wrote:
> +#define scsi_target_index(s) \
> +	((((unsigned long)(s)->channel) << 16) | (s)->id)

Please define scsi_target_index() as an inline function instead of a macro.

> +	if (xa_insert(&shost->__targets, tid, starget, GFP_KERNEL)) {
> +		dev_printk(KERN_ERR, dev, "target index busy\n");
> +		kfree(starget);
> +		return NULL;
> +	}

So the above code passes GFP_KERNEL to xa_insert() while holding a
spinlock? That doesn't seem correct to me. Since xa_insert() provides
locking itself, can the spin_lock_irqsave(shost->host_lock, flags) /
spin_unlock_irqrestore(shost->host_lock, flags) pair be removed and can
the xa_load() and xa_insert() calls be combined into a single
xa_insert() call? I think xa_insert() returns -EBUSY if an entry already
exists.

> -restart:
>  	spin_lock_irqsave(shost->host_lock, flags);
> -	list_for_each_entry(starget, &shost->__targets, siblings) {
> +	starget = xa_find(&shost->__targets, &tid, ULONG_MAX, XA_PRESENT);
> +	while (starget) {
>  		if (starget->state == STARGET_DEL ||
>  		    starget->state == STARGET_REMOVE ||
> -		    starget->state == STARGET_CREATED_REMOVE)
> +		    starget->state == STARGET_CREATED_REMOVE) {
> +			starget = xa_find_after(&shost->__targets, &tid,
> +						ULONG_MAX, XA_PRESENT);
>  			continue;
> +		}
>  		if (starget->dev.parent == dev || &starget->dev == dev) {
>  			kref_get(&starget->reap_ref);
>  			if (starget->state == STARGET_CREATED)
> @@ -1530,7 +1534,10 @@ void scsi_remove_target(struct device *dev)
>  			spin_unlock_irqrestore(shost->host_lock, flags);
>  			__scsi_remove_target(starget);
>  			scsi_target_reap(starget);
> -			goto restart;
> +			spin_lock_irqsave(shost->host_lock, flags);
> +			starget = xa_find_after(&shost->__targets, &tid,
> +						ULONG_MAX, XA_PRESENT);
> +			continue;
>  		}
>  	}

How about using a for loop instead of a while loop such that the
xa_find_after() statement does not have to be duplicated?

Thanks,

Bart.

Re: [PATCH 1/4] scsi: convert target lookup to xarray

[Hannes Reinecke]

On 5/28/20 7:48 PM, Bart Van Assche wrote:
> On 2020-05-28 09:36, Hannes Reinecke wrote:
>> +#define scsi_target_index(s) \
>> +	((((unsigned long)(s)->channel) << 16) | (s)->id)
> 
> Please define scsi_target_index() as an inline function instead of a macro.
> 
>> +	if (xa_insert(&shost->__targets, tid, starget, GFP_KERNEL)) {
>> +		dev_printk(KERN_ERR, dev, "target index busy\n");
>> +		kfree(starget);
>> +		return NULL;
>> +	}
> 
> So the above code passes GFP_KERNEL to xa_insert() while holding a
> spinlock? That doesn't seem correct to me. Since xa_insert() provides
> locking itself, can the spin_lock_irqsave(shost->host_lock, flags) /
> spin_unlock_irqrestore(shost->host_lock, flags) pair be removed and can
> the xa_load() and xa_insert() calls be combined into a single
> xa_insert() call? I think xa_insert() returns -EBUSY if an entry already
> exists.
> 
>> -restart:
>>   	spin_lock_irqsave(shost->host_lock, flags);
>> -	list_for_each_entry(starget, &shost->__targets, siblings) {
>> +	starget = xa_find(&shost->__targets, &tid, ULONG_MAX, XA_PRESENT);
>> +	while (starget) {
>>   		if (starget->state == STARGET_DEL ||
>>   		    starget->state == STARGET_REMOVE ||
>> -		    starget->state == STARGET_CREATED_REMOVE)
>> +		    starget->state == STARGET_CREATED_REMOVE) {
>> +			starget = xa_find_after(&shost->__targets, &tid,
>> +						ULONG_MAX, XA_PRESENT);
>>   			continue;
>> +		}
>>   		if (starget->dev.parent == dev || &starget->dev == dev) {
>>   			kref_get(&starget->reap_ref);
>>   			if (starget->state == STARGET_CREATED)
>> @@ -1530,7 +1534,10 @@ void scsi_remove_target(struct device *dev)
>>   			spin_unlock_irqrestore(shost->host_lock, flags);
>>   			__scsi_remove_target(starget);
>>   			scsi_target_reap(starget);
>> -			goto restart;
>> +			spin_lock_irqsave(shost->host_lock, flags);
>> +			starget = xa_find_after(&shost->__targets, &tid,
>> +						ULONG_MAX, XA_PRESENT);
>> +			continue;
>>   		}
>>   	}
> 
> How about using a for loop instead of a while loop such that the
> xa_find_after() statement does not have to be duplicated?
> 
Ok, will do for the next round.

Cheers,

Hannes