unplugging fix

PATCH In review Andrew Morton 2 participants 9 messages Jun 1, 2002 – Jun 4, 2002 Block layer

Share & tools

Andrew Morton

author

2002-06-01 08:36 UTC #0 lore raw

PATCH 1/16 unplugging fix

There's a plugging bug in 2.5.19. Once you start pushing several disks hard, the new unplug code gets confused and queues are left in plugged state, but not on the plug list. They never get unplugged and the machine dies mysteriously.

I exchanged a stream of emails and patches with Jens Thursday/Friday, but none of it worked.

This patch makes all the locking and initialisation paths much more defensive. It does fix the bug, but I'm not actually sure where the bug is. This is an "unofficial" patch - Jens is offline for a few days. But if you plan to release a kernel soon I'd suggest that it include this patch.

=====================================

drivers/block/ll_rw_blk.c renamed from drivers/block/ll_rw_blk.c~blk-plug +24 ~~-34~~

@@ -49,7 +49,7 @@ extern int mac_floppy_init(void);
  */
 static kmem_cache_t *request_cachep;
 
-static struct list_head blk_plug_list;
+static LIST_HEAD(blk_plug_list);
 static spinlock_t blk_plug_lock __cacheline_aligned_in_smp = SPIN_LOCK_UNLOCKED;
 
 /* blk_dev_struct is:

@@ -156,6 +156,7 @@ void blk_queue_make_request(request_queu
 	 */
 	blk_queue_bounce_limit(q, BLK_BOUNCE_HIGH);
 
+	INIT_LIST_HEAD(&q->plug_list);
 	init_waitqueue_head(&q->queue_wait);
 }

@@ -782,17 +783,18 @@ static int ll_merge_requests_fn(request_
  */
 void blk_plug_device(request_queue_t *q)
 {
+	unsigned long flags;
+
 	/*
 	 * common case
 	 */
 	if (!elv_queue_empty(q))
 		return;
 
-	if (!test_and_set_bit(QUEUE_FLAG_PLUGGED, &q->queue_flags)) {
-		spin_lock(&blk_plug_lock);
+	spin_lock_irqsave(&blk_plug_lock, flags);
+	if (!test_and_set_bit(QUEUE_FLAG_PLUGGED, &q->queue_flags))
 		list_add_tail(&q->plug_list, &blk_plug_list);
-		spin_unlock(&blk_plug_lock);
-	}
+	spin_unlock_irqrestore(&blk_plug_lock, flags);
 }
 
 /*

@@ -803,7 +805,7 @@ static inline void __generic_unplug_devi
 	/*
 	 * not plugged
 	 */
-	if (!__test_and_clear_bit(QUEUE_FLAG_PLUGGED, &q->queue_flags))
+	if (!test_and_clear_bit(QUEUE_FLAG_PLUGGED, &q->queue_flags))
 		return;
 
 	if (test_bit(QUEUE_FLAG_STOPPED, &q->queue_flags))

@@ -866,44 +868,36 @@ void blk_stop_queue(request_queue_t *q)
  */
 void blk_run_queues(void)
 {
-	struct list_head *n, *tmp, local_plug_list;
+	LIST_HEAD(local_plug_list);
 	unsigned long flags;
 
-	INIT_LIST_HEAD(&local_plug_list);
-
 	/*
 	 * this will happen fairly often
 	 */
 	spin_lock_irqsave(&blk_plug_lock, flags);
-	if (list_empty(&blk_plug_list)) {
-		spin_unlock_irqrestore(&blk_plug_lock, flags);
-		return;
-	}
+	if (list_empty(&blk_plug_list))
+		goto out;
 
 	list_splice(&blk_plug_list, &local_plug_list);
 	INIT_LIST_HEAD(&blk_plug_list);
-	spin_unlock_irqrestore(&blk_plug_lock, flags);
 
-	/*
-	 * local_plug_list is now a private copy we can traverse lockless
-	 */
-	list_for_each_safe(n, tmp, &local_plug_list) {
-		request_queue_t *q = list_entry(n, request_queue_t, plug_list);
+	while (!list_empty(&local_plug_list)) {
+		request_queue_t *q;
 
-		if (!test_bit(QUEUE_FLAG_STOPPED, &q->queue_flags)) {
-			list_del(&q->plug_list);
+		q = list_entry(local_plug_list.next,
+				request_queue_t, plug_list);
+
+		list_del(&q->plug_list);
+		if (test_bit(QUEUE_FLAG_STOPPED, &q->queue_flags)) {
+			list_add_tail(&q->plug_list, &blk_plug_list);
+		} else {
+			spin_unlock_irqrestore(&blk_plug_lock, flags);
 			generic_unplug_device(q);
+			spin_lock_irqsave(&blk_plug_lock, flags);
 		}
 	}
-
-	/*
-	 * add any remaining queue back to plug list
-	 */
-	if (!list_empty(&local_plug_list)) {
-		spin_lock_irqsave(&blk_plug_lock, flags);
-		list_splice(&local_plug_list, &blk_plug_list);
-		spin_unlock_irqrestore(&blk_plug_lock, flags);
-	}
+out:
+	spin_unlock_irqrestore(&blk_plug_lock, flags);
 }
 
 static int __blk_cleanup_queue(struct request_list *list)

@@ -1056,8 +1050,6 @@ int blk_init_queue(request_queue_t *q, r
 	blk_queue_max_hw_segments(q, MAX_HW_SEGMENTS);
 	blk_queue_max_phys_segments(q, MAX_PHYS_SEGMENTS);
 
-	INIT_LIST_HEAD(&q->plug_list);
-
 	return 0;
 }

@@ -1938,8 +1930,6 @@ int __init blk_dev_init(void)
 	blk_max_low_pfn = max_low_pfn;
 	blk_max_pfn = max_pfn;
 
-	INIT_LIST_HEAD(&blk_plug_list);
-
 #if defined(CONFIG_IDE) && defined(CONFIG_BLK_DEV_HD)
 	hd_init();
 #endif

Andrew Morton

author

2002-06-02 07:35 UTC #1 lore raw

Quoted 6 lines · Andrew Morton

There's a plugging bug in 2.5.19. Once you start pushing several disks hard, the new unplug code gets confused and queues are left in plugged state, but not on the plug list. They never get unplugged and the machine dies mysteriously.

This patch didn't fix it. It made it tons better, but I again have a wedged box. Same symptoms - against IDE this time.

blk_plug_list is empty. queue_flags=0x03. Interestingly, q->plug_list is non-empty, non-zero, both list members pointing at a list which isn't either itself or blk_plug_list.

I note that the code isn't taking queues off the plug list when the queue is destroyed. Guess that doesn't matter - we never destroy a plugged queue...

This one is killing me.

Jens Axboe

2002-06-02 08:12 UTC #2 lore raw

In reply to Andrew Morton

Quoted 15 lines · Andrew Morton

Quoted 6 lines · Andrew Morton
There's a plugging bug in 2.5.19. Once you start pushing several disks hard, the new unplug code gets confused and queues are left in plugged state, but not on the plug list. They never get unplugged and the machine dies mysteriously.
This patch didn't fix it. It made it tons better, but I again have a wedged box. Same symptoms - against IDE this time.
blk_plug_list is empty. queue_flags=0x03. Interestingly, q->plug_list is non-empty, non-zero, both list members pointing at a list which isn't either itself or blk_plug_list.

Quoted 5 lines

I note that the code isn't taking queues off the plug list when the queue is destroyed. Guess that doesn't matter - we never destroy a plugged queue...
This one is killing me.

I've got a good handle on how to clean the whole plugging thing up, I suspect it will make this case easier to fix. I'll be back with that tomorrow, still got guests...

Signature · 1 line

Jens Axboe

Jens Axboe

2002-06-03 08:40 UTC #3 lore raw

In reply to Andrew Morton

Quoted 20 lines · Jens Axboe

Quoted 19 lines · Andrew Morton
Quoted 6 lines · Andrew Morton
There's a plugging bug in 2.5.19. Once you start pushing several disks hard, the new unplug code gets confused and queues are left in plugged state, but not on the plug list. They never get unplugged and the machine dies mysteriously.
This patch didn't fix it. It made it tons better, but I again have a wedged box. Same symptoms - against IDE this time.
blk_plug_list is empty. queue_flags=0x03. Interestingly, q->plug_list is non-empty, non-zero, both list members pointing at a list which isn't either itself or blk_plug_list.
I note that the code isn't taking queues off the plug list when the queue is destroyed. Guess that doesn't matter - we never destroy a plugged queue...

Quoted 5 lines

Quoted 1 line
This one is killing me.
I've got a good handle on how to clean the whole plugging thing up, I suspect it will make this case easier to fix. I'll be back with that tomorrow, still got guests...

Does this work? I can't poke holes in it, but then again...

drivers/block/ll_rw_blk.c renamed from opt/kernel/linux-2.5.20/drivers/block/ll_rw_blk.c +26 ~~-24~~

@@ -821,7 +821,7 @@
 	/*
 	 * not plugged
 	 */
-	if (!__test_and_clear_bit(QUEUE_FLAG_PLUGGED, &q->queue_flags))
+	if (!test_and_clear_bit(QUEUE_FLAG_PLUGGED, &q->queue_flags))
 		return;
 
 	if (test_bit(QUEUE_FLAG_STOPPED, &q->queue_flags))

@@ -893,6 +893,17 @@
  **/
 void blk_stop_queue(request_queue_t *q)
 {
+	/*
+	 * remove from the plugged list, queue must not be called.
+	 */
+	if (test_and_clear_bit(QUEUE_FLAG_PLUGGED, &q->queue_flags)) {
+		unsigned long flags;
+
+		spin_lock_irqsave(&blk_plug_lock, flags);
+		list_del(&q->plug_list);
+		spin_unlock_irqrestore(&blk_plug_lock, flags);
+	}
+
 	set_bit(QUEUE_FLAG_STOPPED, &q->queue_flags);
 }

@@ -904,45 +915,36 @@
  *   are currently stopped are ignored. This is equivalent to the older
  *   tq_disk task queue run.
  **/
+#define blk_plug_entry(entry) list_entry((entry), request_queue_t, plug_list)
 void blk_run_queues(void)
 {
-	struct list_head *n, *tmp, local_plug_list;
-	unsigned long flags;
+	struct list_head local_plug_list;
 
 	INIT_LIST_HEAD(&local_plug_list);
 
+	spin_lock_irq(&blk_plug_lock);
+
 	/*
 	 * this will happen fairly often
 	 */
-	spin_lock_irqsave(&blk_plug_lock, flags);
 	if (list_empty(&blk_plug_list)) {
-		spin_unlock_irqrestore(&blk_plug_lock, flags);
+		spin_unlock_irq(&blk_plug_lock);
 		return;
 	}
 
 	list_splice(&blk_plug_list, &local_plug_list);
 	INIT_LIST_HEAD(&blk_plug_list);
-	spin_unlock_irqrestore(&blk_plug_lock, flags);
+	spin_unlock_irq(&blk_plug_lock);
+	
+	while (!list_empty(&local_plug_list)) {
+		request_queue_t *q = blk_plug_entry(local_plug_list.next);
 
-	/*
-	 * local_plug_list is now a private copy we can traverse lockless
-	 */
-	list_for_each_safe(n, tmp, &local_plug_list) {
-		request_queue_t *q = list_entry(n, request_queue_t, plug_list);
-
-		if (!test_bit(QUEUE_FLAG_STOPPED, &q->queue_flags)) {
-			list_del(&q->plug_list);
-			generic_unplug_device(q);
-		}
-	}
+		BUG_ON(test_bit(QUEUE_FLAG_STOPPED, &q->queue_flags));
 
-	/*
-	 * add any remaining queue back to plug list
-	 */
-	if (!list_empty(&local_plug_list)) {
-		spin_lock_irqsave(&blk_plug_lock, flags);
-		list_splice(&local_plug_list, &blk_plug_list);
-		spin_unlock_irqrestore(&blk_plug_lock, flags);
+		spin_lock_irq(q->queue_lock);
+		list_del(&q->plug_list);
+		__generic_unplug_device(q);
+		spin_unlock_irq(q->queue_lock);
 	}
 }

Signature · 1 line

Jens Axboe

Andrew Morton

author

2002-06-03 09:11 UTC #4 lore raw

In reply to Andrew Morton

Quoted 3 lines · Jens Axboe

...
Does this work? I can't poke holes in it, but then again...

It survives a 30-minute test. It would not have done that before...

Are you sure blk_stop_queue() and blk_run_queues() can't race against each other? Seems there's a window where they could both do a list_del().

Jens Axboe

2002-06-03 09:41 UTC #5 lore raw

In reply to Andrew Morton

Quoted 7 lines · Andrew Morton

Quoted 3 lines · Jens Axboe
...
Does this work? I can't poke holes in it, but then again...
It survives a 30-minute test. It would not have done that before...

Excellent.

Quoted 3 lines

Are you sure blk_stop_queue() and blk_run_queues() can't race against each other? Seems there's a window where they could both do a list_del().

Hmm I'd prefer to just use the safe variant and not rely on the plugged flag when the lock isn't held, so here's my final version with just that change. Agree?

drivers/block/ll_rw_blk.c renamed from opt/kernel/linux-2.5.20/drivers/block/ll_rw_blk.c +29 ~~-24~~

@@ -821,7 +821,7 @@
 	/*
 	 * not plugged
 	 */
-	if (!__test_and_clear_bit(QUEUE_FLAG_PLUGGED, &q->queue_flags))
+	if (!test_and_clear_bit(QUEUE_FLAG_PLUGGED, &q->queue_flags))
 		return;
 
 	if (test_bit(QUEUE_FLAG_STOPPED, &q->queue_flags))

@@ -893,6 +893,20 @@
  **/
 void blk_stop_queue(request_queue_t *q)
 {
+	unsigned long flags;
+
+	spin_lock_irqsave(q->queue_lock, flags);
+
+	/*
+	 * remove from the plugged list, queue must not be called.
+	 */
+	if (test_and_clear_bit(QUEUE_FLAG_PLUGGED, &q->queue_flags)) {
+		spin_lock(&blk_plug_lock);
+		list_del(&q->plug_list);
+		spin_unlock(&blk_plug_lock);
+	}
+
+	spin_unlock_irqrestore(q->queue_lock, flags);
 	set_bit(QUEUE_FLAG_STOPPED, &q->queue_flags);
 }

@@ -904,45 +918,36 @@
  *   are currently stopped are ignored. This is equivalent to the older
  *   tq_disk task queue run.
  **/
+#define blk_plug_entry(entry) list_entry((entry), request_queue_t, plug_list)
 void blk_run_queues(void)
 {
-	struct list_head *n, *tmp, local_plug_list;
-	unsigned long flags;
+	struct list_head local_plug_list;
 
 	INIT_LIST_HEAD(&local_plug_list);
 
+	spin_lock_irq(&blk_plug_lock);
+
 	/*
 	 * this will happen fairly often
 	 */
-	spin_lock_irqsave(&blk_plug_lock, flags);
 	if (list_empty(&blk_plug_list)) {
-		spin_unlock_irqrestore(&blk_plug_lock, flags);
+		spin_unlock_irq(&blk_plug_lock);
 		return;
 	}
 
 	list_splice(&blk_plug_list, &local_plug_list);
 	INIT_LIST_HEAD(&blk_plug_list);
-	spin_unlock_irqrestore(&blk_plug_lock, flags);
+	spin_unlock_irq(&blk_plug_lock);
+	
+	while (!list_empty(&local_plug_list)) {
+		request_queue_t *q = blk_plug_entry(local_plug_list.next);
 
-	/*
-	 * local_plug_list is now a private copy we can traverse lockless
-	 */
-	list_for_each_safe(n, tmp, &local_plug_list) {
-		request_queue_t *q = list_entry(n, request_queue_t, plug_list);
-
-		if (!test_bit(QUEUE_FLAG_STOPPED, &q->queue_flags)) {
-			list_del(&q->plug_list);
-			generic_unplug_device(q);
-		}
-	}
+		BUG_ON(test_bit(QUEUE_FLAG_STOPPED, &q->queue_flags));
 
-	/*
-	 * add any remaining queue back to plug list
-	 */
-	if (!list_empty(&local_plug_list)) {
-		spin_lock_irqsave(&blk_plug_lock, flags);
-		list_splice(&local_plug_list, &blk_plug_list);
-		spin_unlock_irqrestore(&blk_plug_lock, flags);
+		spin_lock_irq(q->queue_lock);
+		list_del(&q->plug_list);
+		__generic_unplug_device(q);
+		spin_unlock_irq(q->queue_lock);
 	}
 }

Signature · 1 line

Jens Axboe

Andrew Morton

author

2002-06-03 19:28 UTC #6 lore raw

In reply to Andrew Morton

Quoted 11 lines · Jens Axboe

Quoted 7 lines · Andrew Morton
Quoted 3 lines · Jens Axboe
...
Does this work? I can't poke holes in it, but then again...
It survives a 30-minute test. It would not have done that before...
Excellent.

Hope so. My Friday-night-notfix wouild have survived that long :(

Quoted 7 lines

Quoted 3 lines
Are you sure blk_stop_queue() and blk_run_queues() can't race against each other? Seems there's a window where they could both do a list_del().
Hmm I'd prefer to just use the safe variant and not rely on the plugged flag when the lock isn't held, so here's my final version with just that change. Agree?

Not really ;)

There still seems to be a window where blk_run_queues() will assume the queue is on local_plug_list while not holding plug_list_lock. The QUEUE_PLUGGED flag is set, so blk_stop_queue() will remove the queue from local_plug_list. Then blk_run_queues() removes it as well. The new list_head debug code will rudely catch that.

I'd be more comfortable if the duplicated info in QUEUE_FLAG_PLUGGED and "presence on a list" were made fully atomic/coherent via blk_plug_lock?

Jens Axboe

2002-06-04 07:25 UTC #7 lore raw

In reply to Andrew Morton

Quoted 8 lines · Andrew Morton

Quoted 7 lines · Jens Axboe
Quoted 5 lines · Andrew Morton
Quoted 3 lines · Jens Axboe
...
Does this work? I can't poke holes in it, but then again...

Quoted 6 lines

Quoted 4 lines
Quoted 2 lines
It survives a 30-minute test. It would not have done that before...
Excellent.
Hope so. My Friday-night-notfix wouild have survived that long :(

Still good?

Quoted 17 lines

Quoted 7 lines
Quoted 3 lines
Are you sure blk_stop_queue() and blk_run_queues() can't race against each other? Seems there's a window where they could both do a list_del().
Hmm I'd prefer to just use the safe variant and not rely on the plugged flag when the lock isn't held, so here's my final version with just that change. Agree?
Not really ;)
There still seems to be a window where blk_run_queues() will assume the queue is on local_plug_list while not holding plug_list_lock. The QUEUE_PLUGGED flag is set, so blk_stop_queue() will remove the queue from local_plug_list. Then blk_run_queues() removes it as well. The new list_head debug code will rudely catch that.

Quoted 3 lines

I'd be more comfortable if the duplicated info in QUEUE_FLAG_PLUGGED and "presence on a list" were made fully atomic/coherent via blk_plug_lock?

I agree, at least that makes it more clear. Alright, here's the next version. I've removed the plugged flag since it really serves no purpose anymore when we can just look at the list status. AFAICS, it's safe to call blk_queue_plugged() with just the queue lock held, even if blk_plug_list changes underneath us we'll get the rigth result (it'll always return !list_empty regardless), only for insertion/deletion do we grab the blk_plug_lock.

drivers/block/ll_rw_blk.c renamed from opt/kernel/linux-2.5.20/drivers/block/ll_rw_blk.c +41 ~~-33~~

@@ -795,8 +795,8 @@
  * force the transfer to start only after we have put all the requests
  * on the list.
  *
- * This is called with interrupts off and no requests on the queue.
- * (and with the request spinlock acquired)
+ * This is called with interrupts off and no requests on the queue and
+ * with the queue lock held.
  */
 void blk_plug_device(request_queue_t *q)
 {

@@ -806,7 +806,7 @@
 	if (!elv_queue_empty(q))
 		return;
 
-	if (!test_and_set_bit(QUEUE_FLAG_PLUGGED, &q->queue_flags)) {
+	if (!blk_queue_plugged(q)) {
 		spin_lock(&blk_plug_lock);
 		list_add_tail(&q->plug_list, &blk_plug_list);
 		spin_unlock(&blk_plug_lock);

@@ -814,14 +814,27 @@
 }
 
 /*
+ * remove the queue from the plugged list, if present. called with
+ * queue lock held and interrupts disabled.
+ */
+static inline int blk_remove_plug(request_queue_t *q)
+{
+	if (blk_queue_plugged(q)) {
+		spin_lock(&blk_plug_lock);
+		list_del_init(&q->plug_list);
+		spin_unlock(&blk_plug_lock);
+		return 1;
+	}
+
+	return 0;
+}
+
+/*
  * remove the plug and let it rip..
  */
 static inline void __generic_unplug_device(request_queue_t *q)
 {
-	/*
-	 * not plugged
-	 */
-	if (!__test_and_clear_bit(QUEUE_FLAG_PLUGGED, &q->queue_flags))
+	if (!blk_remove_plug(q))
 		return;
 
 	if (test_bit(QUEUE_FLAG_STOPPED, &q->queue_flags))

@@ -849,11 +862,10 @@
 void generic_unplug_device(void *data)
 {
 	request_queue_t *q = data;
-	unsigned long flags;
 
-	spin_lock_irqsave(q->queue_lock, flags);
+	spin_lock_irq(q->queue_lock);
 	__generic_unplug_device(q);
-	spin_unlock_irqrestore(q->queue_lock, flags);
+	spin_unlock_irq(q->queue_lock);
 }
 
 /**

@@ -893,6 +905,12 @@
  **/
 void blk_stop_queue(request_queue_t *q)
 {
+	unsigned long flags;
+
+	spin_lock_irqsave(q->queue_lock, flags);
+	blk_remove_plug(q);
+	spin_unlock_irqrestore(q->queue_lock, flags);
+
 	set_bit(QUEUE_FLAG_STOPPED, &q->queue_flags);
 }

@@ -904,45 +922,35 @@
  *   are currently stopped are ignored. This is equivalent to the older
  *   tq_disk task queue run.
  **/
+#define blk_plug_entry(entry) list_entry((entry), request_queue_t, plug_list)
 void blk_run_queues(void)
 {
-	struct list_head *n, *tmp, local_plug_list;
-	unsigned long flags;
+	struct list_head local_plug_list;
 
 	INIT_LIST_HEAD(&local_plug_list);
 
+	spin_lock_irq(&blk_plug_lock);
+
 	/*
 	 * this will happen fairly often
 	 */
-	spin_lock_irqsave(&blk_plug_lock, flags);
 	if (list_empty(&blk_plug_list)) {
-		spin_unlock_irqrestore(&blk_plug_lock, flags);
+		spin_unlock_irq(&blk_plug_lock);
 		return;
 	}
 
 	list_splice(&blk_plug_list, &local_plug_list);
 	INIT_LIST_HEAD(&blk_plug_list);
-	spin_unlock_irqrestore(&blk_plug_lock, flags);
-
-	/*
-	 * local_plug_list is now a private copy we can traverse lockless
-	 */
-	list_for_each_safe(n, tmp, &local_plug_list) {
-		request_queue_t *q = list_entry(n, request_queue_t, 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);
 
-		if (!test_bit(QUEUE_FLAG_STOPPED, &q->queue_flags)) {
-			list_del(&q->plug_list);
-			generic_unplug_device(q);
-		}
-	}
+		BUG_ON(test_bit(QUEUE_FLAG_STOPPED, &q->queue_flags));
 
-	/*
-	 * add any remaining queue back to plug list
-	 */
-	if (!list_empty(&local_plug_list)) {
-		spin_lock_irqsave(&blk_plug_lock, flags);
-		list_splice(&local_plug_list, &blk_plug_list);
-		spin_unlock_irqrestore(&blk_plug_lock, flags);
+		spin_lock_irq(q->queue_lock);
+		__generic_unplug_device(q);
+		spin_unlock_irq(q->queue_lock);
 	}
 }

Signature · 1 line

Jens Axboe

Jens Axboe

2002-06-04 08:09 UTC #8 lore raw

In reply to Andrew Morton

Quoted 1 line · Jens Axboe

I agree, at least that makes it more clear. Alright, here's the next

[snip]

forgot to diff blkdev.h. here's a new complete version.

drivers/block/ll_rw_blk.c renamed from opt/kernel/linux-2.5.20/drivers/block/ll_rw_blk.c +41 ~~-33~~

@@ -795,8 +795,8 @@
  * force the transfer to start only after we have put all the requests
  * on the list.
  *
- * This is called with interrupts off and no requests on the queue.
- * (and with the request spinlock acquired)
+ * This is called with interrupts off and no requests on the queue and
+ * with the queue lock held.
  */
 void blk_plug_device(request_queue_t *q)
 {

@@ -806,7 +806,7 @@
 	if (!elv_queue_empty(q))
 		return;
 
-	if (!test_and_set_bit(QUEUE_FLAG_PLUGGED, &q->queue_flags)) {
+	if (!blk_queue_plugged(q)) {
 		spin_lock(&blk_plug_lock);
 		list_add_tail(&q->plug_list, &blk_plug_list);
 		spin_unlock(&blk_plug_lock);

@@ -814,14 +814,27 @@
 }
 
 /*
+ * remove the queue from the plugged list, if present. called with
+ * queue lock held and interrupts disabled.
+ */
+static inline int blk_remove_plug(request_queue_t *q)
+{
+	if (blk_queue_plugged(q)) {
+		spin_lock(&blk_plug_lock);
+		list_del_init(&q->plug_list);
+		spin_unlock(&blk_plug_lock);
+		return 1;
+	}
+
+	return 0;
+}
+
+/*
  * remove the plug and let it rip..
  */
 static inline void __generic_unplug_device(request_queue_t *q)
 {
-	/*
-	 * not plugged
-	 */
-	if (!__test_and_clear_bit(QUEUE_FLAG_PLUGGED, &q->queue_flags))
+	if (!blk_remove_plug(q))
 		return;
 
 	if (test_bit(QUEUE_FLAG_STOPPED, &q->queue_flags))

@@ -849,11 +862,10 @@
 void generic_unplug_device(void *data)
 {
 	request_queue_t *q = data;
-	unsigned long flags;
 
-	spin_lock_irqsave(q->queue_lock, flags);
+	spin_lock_irq(q->queue_lock);
 	__generic_unplug_device(q);
-	spin_unlock_irqrestore(q->queue_lock, flags);
+	spin_unlock_irq(q->queue_lock);
 }
 
 /**

@@ -893,6 +905,12 @@
  **/
 void blk_stop_queue(request_queue_t *q)
 {
+	unsigned long flags;
+
+	spin_lock_irqsave(q->queue_lock, flags);
+	blk_remove_plug(q);
+	spin_unlock_irqrestore(q->queue_lock, flags);
+
 	set_bit(QUEUE_FLAG_STOPPED, &q->queue_flags);
 }

@@ -904,45 +922,35 @@
  *   are currently stopped are ignored. This is equivalent to the older
  *   tq_disk task queue run.
  **/
+#define blk_plug_entry(entry) list_entry((entry), request_queue_t, plug_list)
 void blk_run_queues(void)
 {
-	struct list_head *n, *tmp, local_plug_list;
-	unsigned long flags;
+	struct list_head local_plug_list;
 
 	INIT_LIST_HEAD(&local_plug_list);
 
+	spin_lock_irq(&blk_plug_lock);
+
 	/*
 	 * this will happen fairly often
 	 */
-	spin_lock_irqsave(&blk_plug_lock, flags);
 	if (list_empty(&blk_plug_list)) {
-		spin_unlock_irqrestore(&blk_plug_lock, flags);
+		spin_unlock_irq(&blk_plug_lock);
 		return;
 	}
 
 	list_splice(&blk_plug_list, &local_plug_list);
 	INIT_LIST_HEAD(&blk_plug_list);
-	spin_unlock_irqrestore(&blk_plug_lock, flags);
-
-	/*
-	 * local_plug_list is now a private copy we can traverse lockless
-	 */
-	list_for_each_safe(n, tmp, &local_plug_list) {
-		request_queue_t *q = list_entry(n, request_queue_t, 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);
 
-		if (!test_bit(QUEUE_FLAG_STOPPED, &q->queue_flags)) {
-			list_del(&q->plug_list);
-			generic_unplug_device(q);
-		}
-	}
+		BUG_ON(test_bit(QUEUE_FLAG_STOPPED, &q->queue_flags));
 
-	/*
-	 * add any remaining queue back to plug list
-	 */
-	if (!list_empty(&local_plug_list)) {
-		spin_lock_irqsave(&blk_plug_lock, flags);
-		list_splice(&local_plug_list, &blk_plug_list);
-		spin_unlock_irqrestore(&blk_plug_lock, flags);
+		spin_lock_irq(q->queue_lock);
+		__generic_unplug_device(q);
+		spin_unlock_irq(q->queue_lock);
 	}
 }

include/linux/blkdev.h renamed from opt/kernel/linux-2.5.20/include/linux/blkdev.h +4 -6

@@ -209,13 +209,11 @@
 #define RQ_SCSI_DONE		0xfffe
 #define RQ_SCSI_DISCONNECTING	0xffe0
 
-#define QUEUE_FLAG_PLUGGED	0	/* queue is plugged */
-#define QUEUE_FLAG_CLUSTER	1	/* cluster several segments into 1 */
-#define QUEUE_FLAG_QUEUED	2	/* uses generic tag queueing */
-#define QUEUE_FLAG_STOPPED	3	/* queue is stopped */
+#define QUEUE_FLAG_CLUSTER	0	/* cluster several segments into 1 */
+#define QUEUE_FLAG_QUEUED	1	/* uses generic tag queueing */
+#define QUEUE_FLAG_STOPPED	2	/* queue is stopped */
 
-#define blk_queue_plugged(q)	test_bit(QUEUE_FLAG_PLUGGED, &(q)->queue_flags)
-#define blk_mark_plugged(q)	set_bit(QUEUE_FLAG_PLUGGED, &(q)->queue_flags)
+#define blk_queue_plugged(q)	!list_empty(&(q)->plug_list)
 #define blk_queue_tagged(q)	test_bit(QUEUE_FLAG_QUEUED, &(q)->queue_flags)
 #define blk_queue_empty(q)	elv_queue_empty(q)
 #define list_entry_rq(ptr)	list_entry((ptr), struct request, queuelist)

Signature · 1 line

Jens Axboe