[RFC v2 2/6] scsi: xarray, iterations on scsi_target

[Douglas Gilbert <dgilbert@interlog.com>]

For reasons unknown, the supplied functions for iterating through all
scsi_device objects that belonged to a scsi_target involved going
up to the scsi_host object (i.e. the target's parent) and iterating
through all scsi_device objects belonging to that scsi_host, and only
selecting those scsi_device objects that belonged to the original
scsi_target object. While correct, that is very inefficient when it
is noted that each scsi_target object has a 'devices' xarray which
contains pointers to the scsi_device object it owns.

Modify starget_for_each_device() and __starget_for_each_device()
to take this more efficient route. Add starg_for_each_device() and
__starg_for_each_device() macros in scsi_device.h that are finer
grained versions of the existing shost_for_each_device() and
__shost_for_each_device() macros.

The new __scsi_target_iterate_devices() helper function takes
the host_lock to be consistent with the existing
__scsi_iterate_devices() function's locking. At this stage the user
is _not_ being encouraged to use per scsi_target locks.

Signed-off-by: Douglas Gilbert <dgilbert@interlog.com>
---
 drivers/scsi/scsi.c        | 68 ++++++++++++++++++++++++++---------
 drivers/scsi/scsi_scan.c   |  3 +-
 include/scsi/scsi_device.h | 73 +++++++++++++++++++++++++++++++++++++-
 3 files changed, 125 insertions(+), 19 deletions(-)

diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index 61aa68083f67..aa3240f7aed9 100644
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -588,9 +588,50 @@ struct scsi_device *__scsi_iterate_devices(struct Scsi_Host *shost,
 }
 EXPORT_SYMBOL(__scsi_iterate_devices);
 
+/* helper for starg_for_each_device, see that for documentation */
+struct scsi_device *__scsi_target_iterate_devices(struct scsi_target *starg,
+						  struct scsi_device *prev)
+{
+	struct Scsi_Host *shost = starg_to_shost(starg);
+	struct scsi_device *next = prev;
+	unsigned long flags, l_idx;
+
+	if (!shost)
+		return NULL;
+	spin_lock_irqsave(shost->host_lock, flags);
+	if (xa_empty(&starg->devices)) {
+		next = NULL;
+		goto unlock;
+	}
+	do {
+		if (!next) {	/* get first element iff first iteration */
+			l_idx = 0;
+			next = xa_find(&starg->devices, &l_idx,
+				       scsi_xa_limit_16b.max, XA_PRESENT);
+		} else {
+			l_idx = next->sh_idx,
+			next = xa_find_after(&starg->devices, &l_idx,
+					     scsi_xa_limit_16b.max,
+					     XA_PRESENT);
+		}
+		if (next) {
+			/* skip devices that we can't get a reference to */
+			if (!scsi_device_get(next))
+				break;
+		}
+	} while (next);
+unlock:
+	spin_unlock_irqrestore(shost->host_lock, flags);
+
+	if (prev)
+		scsi_device_put(prev);
+	return next;
+}
+EXPORT_SYMBOL(__scsi_target_iterate_devices);
+
 /**
  * starget_for_each_device  -  helper to walk all devices of a target
- * @starget:	target whose devices we want to iterate over.
+ * @starg:	target whose devices we want to iterate over.
  * @data:	Opaque passed to each function call.
  * @fn:		Function to call on each device
  *
@@ -598,23 +639,20 @@ EXPORT_SYMBOL(__scsi_iterate_devices);
  * a reference that must be released by scsi_host_put when breaking
  * out of the loop.
  */
-void starget_for_each_device(struct scsi_target *starget, void *data,
-		     void (*fn)(struct scsi_device *, void *))
+void starget_for_each_device(struct scsi_target *starg, void *data,
+			     void (*fn)(struct scsi_device *, void *))
 {
-	struct Scsi_Host *shost = starg_to_shost(starget);
 	struct scsi_device *sdev;
 
-	shost_for_each_device(sdev, shost) {
-		if ((sdev->channel == starget->channel) &&
-		    (sdev->id == starget->id))
-			fn(sdev, data);
-	}
+	/* XARRAY: this looks like recursion, but isn't. Rename function? */
+	sstarg_for_each_device(sdev, starg)
+		fn(sdev, data);
 }
 EXPORT_SYMBOL(starget_for_each_device);
 
 /**
  * __starget_for_each_device - helper to walk all devices of a target (UNLOCKED)
- * @starget:	target whose devices we want to iterate over.
+ * @starg:	target whose devices we want to iterate over.
  * @data:	parameter for callback @fn()
  * @fn:		callback function that is invoked for each device
  *
@@ -626,17 +664,13 @@ EXPORT_SYMBOL(starget_for_each_device);
  * they need to access the device list in irq context.  Otherwise you
  * really want to use starget_for_each_device instead.
  **/
-void __starget_for_each_device(struct scsi_target *starget, void *data,
+void __starget_for_each_device(struct scsi_target *starg, void *data,
 			       void (*fn)(struct scsi_device *, void *))
 {
-	struct Scsi_Host *shost = starg_to_shost(starget);
 	struct scsi_device *sdev;
 
-	__shost_for_each_device(sdev, shost) {
-		if ((sdev->channel == starget->channel) &&
-		    (sdev->id == starget->id))
-			fn(sdev, data);
-	}
+	__sstarg_for_each_device(sdev, starg)
+		fn(sdev, data);
 }
 EXPORT_SYMBOL(__starget_for_each_device);
 
diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
index b8f5850c5a04..72a0064a879a 100644
--- a/drivers/scsi/scsi_scan.c
+++ b/drivers/scsi/scsi_scan.c
@@ -403,7 +403,8 @@ static void scsi_target_reap_ref_put(struct scsi_target *starget)
 
 /**
  * scsi_alloc_target - allocate a new or find an existing target
- * @parent:	parent of the target (need not be a scsi host)
+ * @parent:	may point to the parent shost, or an intermediate object
+ *		that dev_to_shost() can resolve to the parent shost
  * @channel:	target channel number (zero if no channels)
  * @id:		target id number
  *
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index dc9de4cc0e79..4219b8d1b94c 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -295,6 +295,7 @@ struct scsi_target {
 				     * scsi_device.id eventually */
 	struct xarray devices;	/* scsi_device objects owned by this target */
 	struct device		dev;
+
 	unsigned int		create:1; /* signal that it needs to be added */
 	unsigned int		single_lun:1;	/* Indicates we should only
 						 * allow I/O to one of the luns
@@ -361,6 +362,7 @@ 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 *,
 							  u64);
+/* XARRAY: these visitor names too close to sstarg_for_each_device macros? */
 extern void starget_for_each_device(struct scsi_target *, void *,
 		     void (*fn)(struct scsi_device *, void *));
 extern void __starget_for_each_device(struct scsi_target *, void *,
@@ -370,6 +372,10 @@ extern void __starget_for_each_device(struct scsi_target *, void *,
 /* only exposed to implement shost_for_each_device */
 extern struct scsi_device *__scsi_iterate_devices(struct Scsi_Host *,
 						  struct scsi_device *);
+/* only exposed to implement starg_for_each_device */
+extern struct scsi_device *__scsi_target_iterate_devices
+					(struct scsi_target *starg,
+					 struct scsi_device *sdev);
 
 /**
  * shost_for_each_device - iterate over all devices of a host
@@ -423,13 +429,78 @@ static inline struct scsi_device *__scsi_h2d_next_it(struct Scsi_Host *shost,
  *
  * Note 2: The iteration won't fail but dereferencing sdev might. To access
  * the xarray features (e.g. marks) associated with sdev safely use:
- * xa_for_each(&shost->__devices, l_idx, next) directly then use l_idx.
+ * xa_for_each(&shost->__devices, l_idx, sdev) directly then use l_idx.
  */
 #define __shost_for_each_device(sdev, shost) \
 	for ((sdev) = __scsi_h2d_1st_it((shost)); \
 	     (sdev); \
 	     (sdev) = __scsi_h2d_next_it((shost), (sdev)))
 
+/**
+ * sstarg_for_each_device - iterate over all devices of a target
+ * @sdev: the &struct scsi_device to use as a cursor
+ * @starg: the &struct scsi_target to iterate over
+ *
+ * Iterator that returns each device attached to @starg.  This loop
+ * takes a reference on each device and releases it at the end.  If
+ * you break out of the loop, you must call scsi_device_put(sdev).
+ *
+ * Note: the leading double "ss" is to lessen the similarity between this
+ * macro and the starget_for_each_device() function declared above.
+ */
+#define sstarg_for_each_device(sdev, starg) \
+	for ((sdev) = __scsi_target_iterate_devices((starg), NULL); \
+	     (sdev); \
+	     (sdev) = __scsi_target_iterate_devices((starg), (sdev)))
+
+/* helper for __sstarg_for_each_device */
+static inline struct scsi_device *__scsi_t2d_1st_it(struct scsi_target *starg)
+{
+	unsigned long l_idx = 0;
+
+	return xa_find(&starg->devices, &l_idx, scsi_xa_limit_16b.max,
+		       XA_PRESENT);
+}
+
+/* helper for __sstarg_for_each_device */
+static inline struct scsi_device *__scsi_t2d_next_it(struct scsi_target *starg,
+						     struct scsi_device *prev)
+{
+	unsigned long l_idx;
+
+	if (prev) {
+		l_idx = prev->starg_idx,
+		prev = xa_find_after(&starg->devices, &l_idx,
+				     scsi_xa_limit_16b.max, XA_PRESENT);
+	}
+	return prev;	/* now either _next_ or NULL */
+}
+
+/**
+ * __sstarg_for_each_device - iterate over all devices of a target (UNLOCKED)
+ * @sdev: the &struct scsi_device to use as a cursor
+ * @starg: the &struct scsi_target to iterate over
+ *
+ * Iterator that returns each device attached to @starg.  It does _not_
+ * take a reference on the scsi_device, so the whole loop must be
+ * protected by shost->host_lock.
+ *
+ * Note: The only reason to use this is because you need to access the
+ * device list in interrupt context.  Otherwise you really want to use
+ * starg_for_each_device instead.
+ *
+ * Note 2: The iteration won't fail but dereferencing sdev might. To access
+ * the xarray features (e.g. marks) associated with sdev safely use:
+ * xa_for_each(&starg->devices, l_idx, sdev) directly then use l_idx.
+ *
+ * Note 3: the leading double "ss" is to lessen the similarity between this
+ * macro and the __starget_for_each_device() function declared above.
+ */
+#define __sstarg_for_each_device(sdev, starg) \
+	for ((sdev) = __scsi_t2d_1st_it((starg)); \
+	     (sdev); \
+	     (sdev) = __scsi_t2d_next_it((starg), (sdev)))
+
 extern int scsi_change_queue_depth(struct scsi_device *, int);
 extern int scsi_track_queue_full(struct scsi_device *, int);
 
-- 
2.25.1