serialize bus scanning

Synchronize all scanning activity, this fixes long-standing races vs /proc/scsi/scsi and sysfs addition and deletion of devices.

Note that this does not serialize removing, the lists will get their own locking soon.

diff -Nru a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
@@ -30,6 +30,7 @@
 #include <linux/moduleparam.h>
 #include <linux/init.h>
 #include <linux/blkdev.h>
+#include <asm/semaphore.h>
 
 #include "scsi.h"
 #include "hosts.h"

@@ -94,6 +95,13 @@
 		 " between 1 and 16384)");
 #endif
 
+/*
+ * This mutex serializes all scsi scanning activity from kernel- and
+ * userspace.  It could easily be made per-host but I'd like to avoid
+ * the overhead for now.
+ */
+static DECLARE_MUTEX(scsi_scan_mutex);
+
 /**
  * scsi_unlock_floptical - unlock device via a special MODE SENSE command
  * @sreq:	used to send the command

@@ -1067,9 +1075,12 @@
 	struct scsi_device *sdev;
 	int res;
 
+	down(&scsi_scan_mutex);
 	res = scsi_probe_and_add_lun(shost, channel, id, lun, NULL, &sdev, 1);
 	if (res != SCSI_SCAN_LUN_PRESENT)
 		sdev = ERR_PTR(-ENODEV);
+	up(&scsi_scan_mutex);
+
 	return sdev;
 }
 

@@ -1191,11 +1202,14 @@
 	    ((lun != SCAN_WILD_CARD) && (lun > shost->max_lun)))
 		return -EINVAL;
 
+	down(&scsi_scan_mutex);
 	if (channel == SCAN_WILD_CARD) 
 		for (channel = 0; channel <= shost->max_channel; channel++)
 			scsi_scan_channel(shost, channel, id, lun, rescan);
 	else
 		scsi_scan_channel(shost, channel, id, lun, rescan);
+	up(&scsi_scan_mutex);
+
 	return 0;
 }
 

Hi,

Ive started playing with parallel probe in the lab (on some large ppc64 boxes). I know we have a lot of work to do before this will all work, but would it be possible to make this semaphore per host?

Even with a per host semaphore, it annoys me how long it takes to spin up 7 disks especially if a few of them are dead. Longer term are we planning to overlap spinup of disks on the same host?

(We have a property in our device tree that says how quickly you can spin disks up on the same host, eg 1sec apart. We should also be able to do this in parallel on all hosts. We could easily read this property in a small initramfs program and then issue probe commands via /sys)

Anton

you can just add a per-host mutex and lock it in the same places as indicated by the comment in the patch.

I have no plans to do that but if someone comes up with a proper patch that would be nice.

How does this look? This restores the ability to do parallel scsi scan via sysfs.

Anton

--

Change scsi_scan_mutex from global to per host, which is needed for parallel SCSI probe.

 gr16_work-anton/drivers/scsi/hosts.c     |    2 ++
 gr16_work-anton/drivers/scsi/scsi_scan.c |   15 ++++-----------
 gr16_work-anton/include/scsi/scsi_host.h |    6 ++++++
 3 files changed, 12 insertions(+), 11 deletions(-)

diff -puN drivers/scsi/scsi_scan.c~per_host_scan_mutex drivers/scsi/scsi_scan.c
@@ -95,13 +95,6 @@ MODULE_PARM_DESC(max_report_luns,
 		 " between 1 and 16384)");
 #endif
 
-/*
- * This mutex serializes all scsi scanning activity from kernel- and
- * userspace.  It could easily be made per-host but I'd like to avoid
- * the overhead for now.
- */
-static DECLARE_MUTEX(scsi_scan_mutex);
-
 /**
  * scsi_unlock_floptical - unlock device via a special MODE SENSE command
  * @sreq:	used to send the command

@@ -1075,11 +1068,11 @@ struct scsi_device *scsi_add_device(stru
 	struct scsi_device *sdev;
 	int res;
 
-	down(&scsi_scan_mutex);
+	down(&shost->scan_mutex);
 	res = scsi_probe_and_add_lun(shost, channel, id, lun, NULL, &sdev, 1);
 	if (res != SCSI_SCAN_LUN_PRESENT)
 		sdev = ERR_PTR(-ENODEV);
-	up(&scsi_scan_mutex);
+	up(&shost->scan_mutex);
 
 	return sdev;
 }

@@ -1202,13 +1195,13 @@ int scsi_scan_host_selected(struct Scsi_
 	    ((lun != SCAN_WILD_CARD) && (lun > shost->max_lun)))
 		return -EINVAL;
 
-	down(&scsi_scan_mutex);
+	down(&shost->scan_mutex);
 	if (channel == SCAN_WILD_CARD) 
 		for (channel = 0; channel <= shost->max_channel; channel++)
 			scsi_scan_channel(shost, channel, id, lun, rescan);
 	else
 		scsi_scan_channel(shost, channel, id, lun, rescan);
-	up(&scsi_scan_mutex);
+	up(&shost->scan_mutex);
 
 	return 0;
 }

diff -puN include/scsi/scsi_host.h~per_host_scan_mutex include/scsi/scsi_host.h
@@ -480,6 +480,12 @@ struct Scsi_Host {
 	struct list_head sht_legacy_list;
 
 	/*
+	 * This mutex serializes all scsi scanning activity from kernel- and
+	 * userspace.
+	 */
+	struct semaphore scan_mutex;
+
+	/*
 	 * We should ensure that this is aligned, both for better performance
 	 * and also because some compilers (m68k) don't automatically force
 	 * alignment to a long boundary.

diff -puN drivers/scsi/hosts.c~per_host_scan_mutex drivers/scsi/hosts.c
@@ -214,6 +214,8 @@ struct Scsi_Host *scsi_host_alloc(struct
 	INIT_LIST_HEAD(&shost->starved_list);
 	init_waitqueue_head(&shost->host_wait);
 
+	init_MUTEX(&shost->scan_mutex);
+
 	shost->host_no = scsi_host_next_hn++; /* XXX(hch): still racy */
 	shost->dma_channel = 0xff;
 

_

The patch looks fine to me, I hope James will apply it.

I think we might still have to allow only a scan or delete (per host), and not both in parallel - it looks like we could have some races otherwise, I'm not sure though (mainly where the driver model points to the parent device). Maybe a get of the host device during scanning is enough.

I tested it out, and ran some timings pre and post-patch, mainly out of curiousity.

I have:

[root@elm3b79 root]# ls -1d /sysfs/bus/scsi/devices/[01]* | wc

      6       6     192
[root@elm3b79 root]# ls -1d /sysfs/bus/scsi/devices/[2-5]* | wc
    288     288    9428

Really 72 separate LUNs attached to qlogic 2310 (FCP) host adapters (hosts 2-5), each with four paths. 40 disk drives (10 on a loop per enclosure), plus 32 LUNs connected to a disk array. All attached to a 1Gb switch.

I ran a simple script (attached) to scan via sysfs in parallel. Of course without the patch scanning is serialized.

System is a 8 cpu (two node) NUMAQ.

--------------------------------------------
Before the patch:

Rescanning the four qlogic adapters without finding any new devices, seemed to vary up to about 34 seconds:

[root@elm3b79 root]# time ./scanall.sh > xx

real    0m31.947s
user    0m0.007s
sys     0m0.113s

Rescan in parallel re-discovering all devices:

[root@elm3b79 root]# time ./scanall.sh > xx

real    0m34.189s
user    0m0.009s
sys     0m0.615s

--------------------------------------------
With the patch:

Generally went from 34 seconds down to 9.

Rescanning the four qlogic adapters without finding any new devices:

[root@elm3b79 root]# time ./scanall.sh > xx

real    0m8.057s
user    0m0.007s
sys     0m0.120s

Rescan in parallel re-discovering all devices:

[root@elm3b79 root]# time ./scanall.sh > xx

real    0m9.608s
user    0m0.008s
sys     0m0.783s

-----------------------------------------------------------

Also: time to sequentially remove all devices (on host 2-5, patch does not matter but I find this interesting):

time ./delall.sh > xx

real    0m7.276s
user    0m0.823s
sys     0m3.106s

And time to remove in parallel

[root@elm3b79 root]# time ./delall.sh > xx

real    0m4.586s
user    0m1.336s
sys     0m7.204s

-- Patrick Mansfield

Hi Pat,

Good to see someone else does parallel scan :)

Speaking of races... Ive been tripping one when parallel probing. I hit a poisoned list pointer in scsi_run_host_queues, which is being called out of the error handler thread. (Ive got a few scsi buses in this machine full of bad disks)

I havent had a chance to look into it but Im guessing we are racing with someone calling scsi_free_sdev. Im not sure how we should be protecting ->siblings but it looks like we have to do something.

Anton

Looks fine to me aswell. Not that I have a machine with more than one real HBA :)

->siblings needs protection. I've been working for this for a long time and should have a final patchkit soon.

Since I have not seen your tree I am speaking with lack of knowledge, but it seems odd that your device tree would contain spinup knowledge per host. The power issue you are trying to avoid is per disk enclosure / cabinet. A per host value seems like it could still lead to a power issue given a large enough configuration.

-andmike
--
Michael Anderson
[email protected]

Yeah you are right, its currently a global value. The firmware knows power domains so it could get finer grained but it would be at the host bridge level.

Anyway all this intelligence to parse this stuff should be in an initramfs program talking to sysfs. I was quite pleased with the sysfs rescan functionality recently, we had someone kick the power on a scsi drawer and were able to get linux to rescan and spin up the disks again without rebooting.

Anton