Skip to contentSkip to conversation

scsi in 2.5.48

DISCUSSION Andrew Morton 4 participants 6 messages Nov 18, 2002 – Nov 19, 2002

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

Appears to be DOA. Just a simple mke2fs hangs in get_request_wait(). Running an `ls -lR' against an IDE disk gets it going again. Seems that the driver is failing to call into the elevator to fetch more requests. The unplug activity from the IDE reads is sufficient to keep the SCSI driver limping along.

The driver is aic7xxx.

#1 lore raw
Quoted 2 lines · Andrew Morton

Appears to be DOA. Just a simple mke2fs hangs in get_request_wait().

This makes it work again.

drivers/scsi/scsi_lib.c renamed from drivers/scsi/scsi_lib.c~scsi-plug +0 -1
@@ -1024,7 +1024,6 @@ void scsi_request_fn(request_queue_t * q
 			/* can happen if the prep fails 
 			 * FIXME: elv_next_request() should be plugging the
 			 * queue */
-			blk_plug_device(q);
 			break;
 		}
 

_

#2 lore raw
Quoted 10 lines · Andrew Morton
Quoted 2 lines · Andrew Morton

Appears to be DOA. Just a simple mke2fs hangs in get_request_wait().

This makes it work again.

drivers/scsi/scsi_lib.c void scsi_request_fn(request_queue_t * q · line 1028

Quoted 6 lines
drivers/scsi/scsi_lib.c renamed from drivers/scsi/scsi_lib.c~scsi-plug +0 -1
@@ -1024,7 +1024,6 @@ void scsi_request_fn(request_queue_t * q
 			/* can happen if the prep fails 
 			 * FIXME: elv_next_request() should be plugging the
 			 * queue */
-			blk_plug_device(q);
 			break;
 		}

Right fix would be something ala:

===== drivers/block/ll_rw_blk.c 1.143 vs edited =====

drivers/block/ll_rw_blk.c +11 -0
@@ -1038,6 +1038,16 @@
 }
 
 /**
+ * blk_run_queue - run a single device queue
+ * @q	The queue to run
+ */
+void __blk_run_queue(request_queue_t *q)
+{
+	blk_remove_plug(q);
+	q->request_fn(q);
+}
+
+/**
  * blk_run_queues - fire all plugged queues
  *
  * Description:
@@ -2198,4 +2211,5 @@
 EXPORT_SYMBOL(blk_start_queue);
 EXPORT_SYMBOL(blk_stop_queue);
 EXPORT_SYMBOL(__blk_stop_queue);
+EXPORT_SYMBOL(__blk_run_queue);
 EXPORT_SYMBOL(blk_run_queues);

===== drivers/scsi/scsi_lib.c 1.46 vs edited =====

drivers/scsi/scsi_lib.c +3 -8
@@ -259,7 +259,7 @@
 	/*
 	 * Just hit the requeue function for the queue.
 	 */
-	q->request_fn(q);
+	__blk_run_queue(q);
 
 	SDpnt = (Scsi_Device *) q->queuedata;
 	SHpnt = SDpnt->host;
@@ -272,8 +272,6 @@
 	 * use function pointers to pick the right one.
 	 */
 	if (SDpnt->single_lun && blk_queue_empty(q) && SDpnt->device_busy ==0) {
-		request_queue_t *q;
-
 		for (SDpnt = SHpnt->host_queue; SDpnt; SDpnt = SDpnt->next) {
 			if (((SHpnt->can_queue > 0)
 			     && (SHpnt->host_busy >= SHpnt->can_queue))
@@ -283,8 +281,7 @@
 				break;
 			}
 
-			q = &SDpnt->request_queue;
-			q->request_fn(q);
+			__blk_run_queue(&SDpnt->request_queue);
 		}
 	}
 
@@ -299,7 +296,6 @@
 	all_clear = 1;
 	if (SHpnt->some_device_starved) {
 		for (SDpnt = SHpnt->host_queue; SDpnt; SDpnt = SDpnt->next) {
-			request_queue_t *q;
 			if ((SHpnt->can_queue > 0 && (SHpnt->host_busy >= SHpnt->can_queue))
 			    || (SHpnt->host_blocked) 
 			    || (SHpnt->host_self_blocked)) {
@@ -308,8 +304,7 @@
 			if (SDpnt->device_blocked || !SDpnt->starved) {
 				continue;
 			}
-			q = &SDpnt->request_queue;
-			q->request_fn(q);
+			__blk_run_queue(&SDpnt->request_queue);
 			all_clear = 0;
 		}
 		if (SDpnt == NULL && all_clear) {

===== drivers/scsi/scsi_error.c 1.21 vs edited =====

drivers/scsi/scsi_error.c +1 -3
@@ -1479,8 +1479,6 @@
 	 */
 	spin_lock_irqsave(shost->host_lock, flags);
 	for (sdev = shost->host_queue; sdev; sdev = sdev->next) {
-		request_queue_t *q = &sdev->request_queue;
-
 		if ((shost->can_queue > 0 &&
 		     (shost->host_busy >= shost->can_queue))
 		    || (shost->host_blocked)
@@ -1488,7 +1486,7 @@
 			break;
 		}
 
-		q->request_fn(q);
+		__blk_run_queue(&sdev->request_queue);
 	}
 	spin_unlock_irqrestore(shost->host_lock, flags);
 }

===== include/linux/blkdev.h 1.93 vs edited =====

include/linux/blkdev.h +1 -0
@@ -321,6 +321,7 @@
 extern void blk_start_queue(request_queue_t *q);
 extern void blk_stop_queue(request_queue_t *q);
 extern void __blk_stop_queue(request_queue_t *q);
+extern void __blk_run_queue(request_queue_t *q);
 
 static inline request_queue_t *bdev_get_queue(struct block_device *bdev)
 {
Signature · 1 line
Jens Axboe
#3 lore raw
Quoted 4 lines · [email protected] said:
Quoted 3 lines

Andrew Morton wrote:

Appears to be DOA. Just a simple mke2fs hangs in get_request_wait().

This makes it work again.

That can't be entirely right, because we can't exit the request function with pending requests and something like an unplugged queue to start them. It will work for the adaptec because the prep defer is caused by the huge queue depth running us out of command blocks, so the queues would get re-run by a returning command. If the failure were caused by zero outstanding commands, this would hang the system forever.

Quoted 1 line · [email protected] said:

Right fix would be something ala:

That looks about right. On returning I/O we need to unplug and restart.

However, with regard to Andrew's problem, how can the queue plug indefinitely?
I thought it was guaranteed to be unplugged and run eventually?

James

#4 lore raw
In reply to Jens Axboe
Quoted 25 lines · Jens Axboe
Quoted 10 lines · Andrew Morton
Quoted 2 lines · Andrew Morton

Appears to be DOA. Just a simple mke2fs hangs in get_request_wait().

This makes it work again.

drivers/scsi/scsi_lib.c Mon Nov 18 01:42:44 2002 void scsi_request_fn(request_queue_t * q · line 1028

Quoted 6 lines
drivers/scsi/scsi_lib.c Mon Nov 18 01:42:44 2002 renamed from drivers/scsi/scsi_lib.c~scsi-plug Mon Nov 18 01:42:40 2002 +0 -1
@@ -1024,7 +1024,6 @@ void scsi_request_fn(request_queue_t * q
                      /* can happen if the prep fails
                       * FIXME: elv_next_request() should be plugging the
                       * queue */
-                     blk_plug_device(q);
                      break;
              }

Right fix would be something ala:

===== drivers/block/ll_rw_blk.c 1.143 vs edited =====

drivers/block/ll_rw_blk.c Mon Nov 18 14:45:55 2002 line 1043

Quoted 6 lines
drivers/block/ll_rw_blk.c Mon Nov 18 14:45:55 2002 renamed from drivers/block/ll_rw_blk.c Mon Nov 18 08:28:08 2002 +3 -0
@@ -1038,6 +1038,16 @@
 }
 
 /**
+ * blk_run_queue - run a single device queue
+ * @q  The queue to run
+ */

That fixes it for me, thanks.

Now, I had me a little bug in blk_congestion_wait() - it was forgetting to wait. The net effect of this bug was to increase plug/unplug traffic by a factor of about 200. And the 4-way was oopsing about once per gigabyte of IO, in blk_run_queues(). Always due to a garbage q->unplug_fn.

That local list needs spinlock protection, because blk_remove_plug() will actually take queues off that local list while another CPU (or this one) is walking it.

Maybe that's a logic error, but I feel a ton safer with this patch in place, and it stopped the oopses.

It also adds a few debug checks and uninlines stuff ;)

 drivers/block/ll_rw_blk.c |   27 ++++++++++++++-------------
 1 files changed, 14 insertions(+), 13 deletions(-)
drivers/block/ll_rw_blk.c renamed from drivers/block/ll_rw_blk.c~plugbug +14 -13
@@ -737,7 +737,7 @@ new_segment:
 }
 
 
-inline int blk_phys_contig_segment(request_queue_t *q, struct bio *bio,
+int blk_phys_contig_segment(request_queue_t *q, struct bio *bio,
 				   struct bio *nxt)
 {
 	if (!(q->queue_flags & (1 << QUEUE_FLAG_CLUSTER)))
@@ -758,7 +758,7 @@ inline int blk_phys_contig_segment(reque
 	return 0;
 }
 
-inline int blk_hw_contig_segment(request_queue_t *q, struct bio *bio,
+int blk_hw_contig_segment(request_queue_t *q, struct bio *bio,
 				 struct bio *nxt)
 {
 	if (!(q->queue_flags & (1 << QUEUE_FLAG_CLUSTER)))
@@ -956,6 +956,7 @@ static int ll_merge_requests_fn(request_
  */
 void blk_plug_device(request_queue_t *q)
 {
+	WARN_ON(!irqs_disabled());
 	if (!blk_queue_plugged(q)) {
 		spin_lock(&blk_plug_lock);
 		list_add_tail(&q->plug_list, &blk_plug_list);
@@ -967,8 +968,9 @@ void blk_plug_device(request_queue_t *q)
  * remove the queue from the plugged list, if present. called with
  * queue lock held and interrupts disabled.
  */
-inline int blk_remove_plug(request_queue_t *q)
+int blk_remove_plug(request_queue_t *q)
 {
+	WARN_ON(!irqs_disabled());
 	if (blk_queue_plugged(q)) {
 		spin_lock(&blk_plug_lock);
 		list_del_init(&q->plug_list);
@@ -1096,28 +1098,27 @@ void __blk_run_queue(request_queue_t *q)
 #define blk_plug_entry(entry) list_entry((entry), request_queue_t, plug_list)
 void blk_run_queues(void)
 {
-	struct list_head local_plug_list;
-
-	INIT_LIST_HEAD(&local_plug_list);
+	LIST_HEAD(local_plug_list);
 
 	spin_lock_irq(&blk_plug_lock);
 
 	/*
 	 * this will happen fairly often
 	 */
-	if (list_empty(&blk_plug_list)) {
-		spin_unlock_irq(&blk_plug_lock);
-		return;
-	}
+	if (list_empty(&blk_plug_list))
+		goto out;
 
 	list_splice_init(&blk_plug_list, &local_plug_list);
-	spin_unlock_irq(&blk_plug_lock);
 	
 	while (!list_empty(&local_plug_list)) {
 		request_queue_t *q = blk_plug_entry(local_plug_list.next);
 
+		spin_unlock_irq(&blk_plug_lock);
 		q->unplug_fn(q);
+		spin_lock_irq(&blk_plug_lock);
 	}
+out:
+	spin_unlock_irq(&blk_plug_lock);
 }
 
 static int __blk_cleanup_queue(struct request_list *list)
@@ -1959,7 +1960,7 @@ int submit_bio(int rw, struct bio *bio)
 	return 1;
 }
 
-inline void blk_recalc_rq_segments(struct request *rq)
+void blk_recalc_rq_segments(struct request *rq)
 {
 	struct bio *bio;
 	int nr_phys_segs, nr_hw_segs;
@@ -1982,7 +1983,7 @@ inline void blk_recalc_rq_segments(struc
 	rq->nr_hw_segments = nr_hw_segs;
 }
 
-inline void blk_recalc_rq_sectors(struct request *rq, int nsect)
+void blk_recalc_rq_sectors(struct request *rq, int nsect)
 {
 	if (blk_fs_request(rq)) {
 		rq->hard_sector += nsect;

_

#5 lore raw
In reply to Jens Axboe
Quoted 1 line · Andrew Morton
Quoted 1 line
Quoted 1 line

Appears to be DOA. Just a simple mke2fs hangs in get_request_wait().

Quoted 1 line · Andrew Morton
Quoted 1 line

This makes it work again.

Quoted 1 line · Jens Axboe

Right fix would be something ala:

This solves my free_initmem() deadlock, which for some reason I assumed to be a problem of a lower-level nature.

Bill