Re: [OOPS] 2.6.11 - NMI lockup with CFQ scheduler

[James Bottomley <James.Bottomley@SteelEye.com>]

On Thu, 2005-04-07 at 08:49 +0200, Jens Axboe wrote:
> On Wed, Apr 06 2005, James Bottomley wrote:
> > My proposal is to correct this by moving the data back to the correct
> > object, and make any object using it hold a reference, so this would
> > make the provider of the block request_fn hold a reference to the queue
> > and initialise the queue lock pointer with the lock currently in the
> > queue.  Drivers that still use a global lock would be unaffected.  This
> 
> But this is the current requirement, as long as you use the queue you
> must hold a reference to it.

Exactly! that's why I think this solution must work independently of
subsystem.

> What do you think of the attached, then? Allow NULL lock to be passed
> in, in which case we use the queue private lock (that no one should ever
> ever touch). It looks a little confusing that
> sdev->request_queue->queue_lock now protects some sdev structures, if
> you want we can retain ->sdev_lock but as a pointer to the queue lock
> instead.

Looks good.  How about the attached modification?  It makes sdev_lock a
pointer that uses the queue lock which we null out when we release it
(not that I don't trust SCSI or anything ;-)

James

===== drivers/block/ll_rw_blk.c 1.287 vs edited =====
--- 1.287/drivers/block/ll_rw_blk.c	2005-03-11 15:32:27 -05:00
+++ edited/drivers/block/ll_rw_blk.c	2005-04-07 09:05:19 -04:00
@@ -1714,6 +1714,15 @@
 	if (blk_init_free_list(q))
 		goto out_init;
 
+	/*
+	 * if caller didn't supply a lock, they get per-queue locking with
+	 * our embedded lock
+	 */
+	if (!lock) {
+		spin_lock_init(&q->__queue_lock);
+		lock = &q->__queue_lock;
+	}
+
 	q->request_fn		= rfn;
 	q->back_merge_fn       	= ll_back_merge_fn;
 	q->front_merge_fn      	= ll_front_merge_fn;
===== drivers/scsi/scsi_lib.c 1.151 vs edited =====
--- 1.151/drivers/scsi/scsi_lib.c	2005-02-17 14:17:22 -05:00
+++ edited/drivers/scsi/scsi_lib.c	2005-04-07 09:10:31 -04:00
@@ -349,9 +349,9 @@
 		     shost->host_failed))
 		scsi_eh_wakeup(shost);
 	spin_unlock(shost->host_lock);
-	spin_lock(&sdev->sdev_lock);
+	spin_lock(sdev->sdev_lock);
 	sdev->device_busy--;
-	spin_unlock_irqrestore(&sdev->sdev_lock, flags);
+	spin_unlock_irqrestore(sdev->sdev_lock, flags);
 }
 
 /*
@@ -1391,7 +1391,7 @@
 	struct Scsi_Host *shost = sdev->host;
 	struct request_queue *q;
 
-	q = blk_init_queue(scsi_request_fn, &sdev->sdev_lock);
+	q = blk_init_queue(scsi_request_fn, NULL);
 	if (!q)
 		return NULL;
 
===== drivers/scsi/scsi_scan.c 1.142 vs edited =====
--- 1.142/drivers/scsi/scsi_scan.c	2005-02-16 13:21:35 -05:00
+++ edited/drivers/scsi/scsi_scan.c	2005-04-07 09:10:01 -04:00
@@ -249,7 +249,6 @@
 	 */
 	sdev->borken = 1;
 
-	spin_lock_init(&sdev->sdev_lock);
 	sdev->request_queue = scsi_alloc_queue(sdev);
 	if (!sdev->request_queue) {
 		/* release fn is set up in scsi_sysfs_device_initialise, so
@@ -257,7 +256,8 @@
 		put_device(&starget->dev);
 		goto out;
 	}
-
+	/* Now make the sdev_lock point to the request queue lock */
+	sdev->sdev_lock = q->queue_lock;
 	sdev->request_queue->queuedata = sdev;
 	scsi_adjust_queue_depth(sdev, 0, sdev->host->cmd_per_lun);
 
===== drivers/scsi/scsi_sysfs.c 1.69 vs edited =====
--- 1.69/drivers/scsi/scsi_sysfs.c	2005-02-16 20:05:37 -05:00
+++ edited/drivers/scsi/scsi_sysfs.c	2005-04-07 09:12:17 -04:00
@@ -168,6 +168,9 @@
 	list_del(&sdev->starved_entry);
 	spin_unlock_irqrestore(sdev->host->host_lock, flags);
 
+	/* After releasing the queue we may no longer access its lock */
+	BUG_ON(spin_is_locked(sdev->sdev_lock));
+	sdev->sdev_lock = NULL;
 	if (sdev->request_queue)
 		scsi_free_queue(sdev->request_queue);
 
===== include/linux/blkdev.h 1.161 vs edited =====
--- 1.161/include/linux/blkdev.h	2005-03-09 03:03:24 -05:00
+++ edited/include/linux/blkdev.h	2005-04-07 09:05:21 -04:00
@@ -355,8 +355,11 @@
 	unsigned long		queue_flags;
 
 	/*
-	 * protects queue structures from reentrancy
+	 * protects queue structures from reentrancy. ->__queue_lock should
+	 * _never_ be used directly, it is queue private. always use
+	 * ->queue_lock.
 	 */
+	spinlock_t		__queue_lock;
 	spinlock_t		*queue_lock;
 
 	/*
===== include/scsi/scsi_device.h 1.32 vs edited =====
--- 1.32/include/scsi/scsi_device.h	2005-02-17 14:15:51 -05:00
+++ edited/include/scsi/scsi_device.h	2005-04-07 09:08:25 -04:00
@@ -44,7 +44,7 @@
 	struct list_head    same_target_siblings; /* just the devices sharing same target id */
 
 	volatile unsigned short device_busy;	/* commands actually active on low-level */
-	spinlock_t sdev_lock;           /* also the request queue_lock */
+	spinlock_t *sdev_lock;           /* pointer to the request queue_lock */
 	spinlock_t list_lock;
 	struct list_head cmd_list;	/* queue of in use SCSI Command structures */
 	struct list_head starved_entry;