Some fixes for the SRP driver

PATCH Matthew Wilcox 2 participants 2 messages May 13, 2006 – May 17, 2006 SCSI

Share & tools

Matthew Wilcox

author

2006-05-13 04:44 UTC #0 lore raw

Hi Roland,

I happened to glance at the SRP driver this evening and found some problems, so here's a patch to fix them. Only lightly compile-tested, but the fixes seem obvious enough.

First, the host has a mutex_lock to protect the list of targets. This seems very wasteful as it's normally only used to protect a simple list add/delete operation, and the one case that isn't isn't going to take long either.

Second, you use list_for_each_entry_safe() when you aren't modifying the list. I changed it to just list_for_each_entry().

Third, SCAN_WILD_CARD is indeed available from <scsi/scsi.h>, which you already include.

I have some further suggestions:

- You might consider moving this driver to drivers/scsi. It's actually fairly small compared to most scsi drivers, and basically you've got two files plus Kbuild/Kconfig machinery. Seems a bit silly, plus people would notice this scsi driver more readily.
- I'm not convinced you need the target list/lock I mention above. The Scsi_Host structure has a list of associated targets, although I don't see a good iterator for them right now.

Anyway, feel free to use whichever pieces of this patch interest you:

Signed-off-by Matthew Wilcox <[email protected]>

Index: drivers/infiniband/ulp/srp/ib_srp.c
===================================================================
RCS file: /var/cvs/linux-2.6/drivers/infiniband/ulp/srp/ib_srp.c,v retrieving revision 1.9

infiniband/ulp/srp/ib_srp.c +9 ~~-11~~

diff -u -p -r1.9 ib_srp.c
@@ -357,9 +357,9 @@ static void srp_remove_work(void *target
 	target->state = SRP_TARGET_REMOVED;
 	spin_unlock_irq(target->scsi_host->host_lock);
 
-	mutex_lock(&target->srp_host->target_mutex);
+	spin_lock(&target->srp_host->target_lock);
 	list_del(&target->list);
-	mutex_unlock(&target->srp_host->target_mutex);
+	spin_unlock(&target->srp_host->target_lock);
 
 	scsi_remove_host(target->scsi_host);
 	ib_destroy_cm_id(target->cm_id);

@@ -1339,15 +1339,14 @@ static int srp_add_target(struct srp_hos
 	if (scsi_add_host(target->scsi_host, host->dev->dma_device))
 		return -ENODEV;
 
-	mutex_lock(&host->target_mutex);
+	spin_lock(&host->target_lock);
 	list_add_tail(&target->list, &host->target_list);
-	mutex_unlock(&host->target_mutex);
+	spin_unlock(&host->target_lock);
 
 	target->state = SRP_TARGET_LIVE;
 
-	/* XXX: are we supposed to have a definition of SCAN_WILD_CARD ?? */
 	scsi_scan_target(&target->scsi_host->shost_gendev,
-			 0, target->scsi_id, ~0, 0);
+			 0, target->scsi_id, SCAN_WILD_CARD, 0);
 
 	return 0;
 }

@@ -1612,7 +1611,7 @@ static struct srp_host *srp_add_port(str
 		return NULL;
 
 	INIT_LIST_HEAD(&host->target_list);
-	mutex_init(&host->target_mutex);
+	spin_lock_init(&host->target_lock);
 	init_completion(&host->released);
 	host->dev  = device;
 	host->port = port;

@@ -1713,15 +1712,14 @@ static void srp_remove_one(struct ib_dev
 		 * Mark all target ports as removed, so we stop queueing
 		 * commands and don't try to reconnect.
 		 */
-		mutex_lock(&host->target_mutex);
-		list_for_each_entry_safe(target, tmp_target,
-					 &host->target_list, list) {
+		spin_lock(&host->target_lock);
+		list_for_each_entry(target, &host->target_list, list) {
 			spin_lock_irqsave(target->scsi_host->host_lock, flags);
 			if (target->state != SRP_TARGET_REMOVED)
 				target->state = SRP_TARGET_REMOVED;
 			spin_unlock_irqrestore(target->scsi_host->host_lock, flags);
 		}
-		mutex_unlock(&host->target_mutex);
+		spin_unlock(&host->target_lock);
 
 		/*
 		 * Wait for any reconnection tasks that may have

Index: drivers/infiniband/ulp/srp/ib_srp.h
===================================================================
RCS file: /var/cvs/linux-2.6/drivers/infiniband/ulp/srp/ib_srp.h,v retrieving revision 1.5

infiniband/ulp/srp/ib_srp.h +2 -2

diff -u -p -r1.5 ib_srp.h
@@ -37,7 +37,7 @@
 
 #include <linux/types.h>
 #include <linux/list.h>
-#include <linux/mutex.h>
+#include <linux/spinlock.h>
 #include <linux/scatterlist.h>
 
 #include <scsi/scsi_host.h>

@@ -85,7 +85,7 @@ struct srp_host {
 	struct ib_mr	       *mr;
 	struct class_device	class_dev;
 	struct list_head	target_list;
-	struct mutex            target_mutex;
+	spinlock_t              target_lock;
 	struct completion	released;
 	struct list_head	list;
 };

Roland Dreier

2006-05-17 14:54 UTC #1 lore raw

Thanks, I queued up most of this for 2.6.18.

Quoted 4 lines

- You might consider moving this driver to drivers/scsi. It's actually fairly small compared to most scsi drivers, and basically you've got two files plus Kbuild/Kconfig machinery. Seems a bit silly, plus people would notice this scsi driver more readily.

Yeah, it's a valid point. On the other hand, it's also an infiniband driver. It's kind of like the ieee1394 sbp2 driver -- both fish and fowl ;)

Quoted 3 lines

- I'm not convinced you need the target list/lock I mention above. The Scsi_Host structure has a list of associated targets, although I don't see a good iterator for them right now.

It's a little different than that. Confusingly enough, the SRP driver uses a scsi_host for each target port it connects to, and then has a higher-level private structure for each host adapter with a list of target ports. That list is what's being protected.

The reason for using a full scsi_host for a target port is so that we can limit the number of commands sent to a target port from the midlayer (because the number of outstanding work requests on a given connection is what is limited).

- R.