[patch 1/16] unplugging fix

[patch 1/16] unplugging fix

[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.

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.



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

--- 2.5.19/drivers/block/ll_rw_blk.c~blk-plug	Sat Jun  1 01:18:04 2002
+++ 2.5.19-akpm/drivers/block/ll_rw_blk.c	Sat Jun  1 01:18:04 2002
@@ -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

-

Re: [patch 1/16] unplugging fix

[Andrew Morton]

Andrew Morton wrote:
> 
> 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.

-

Re: [patch 1/16] unplugging fix

[Jens Axboe]

On Sun, Jun 02 2002, Andrew Morton wrote:
> Andrew Morton wrote:
> > 
> > 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.

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...

-- 
Jens Axboe

Re: [patch 1/16] unplugging fix

[Jens Axboe]

On Sun, Jun 02 2002, Jens Axboe wrote:
> On Sun, Jun 02 2002, Andrew Morton wrote:
> > Andrew Morton wrote:
> > > 
> > > 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.
> 
> 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...

--- /opt/kernel/linux-2.5.20/drivers/block/ll_rw_blk.c	2002-06-03 10:35:35.000000000 +0200
+++ linux/drivers/block/ll_rw_blk.c	2002-06-03 10:37:26.000000000 +0200
@@ -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);
 	}
 }
 

-- 
Jens Axboe

Re: [patch 1/16] unplugging fix

[Andrew Morton]

Jens Axboe wrote:
> 
> ...
> 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().

-

Re: [patch 1/16] unplugging fix

[Jens Axboe]

On Mon, Jun 03 2002, Andrew Morton wrote:
> Jens Axboe wrote:
> > 
> > ...
> > 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.

> 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?

--- /opt/kernel/linux-2.5.20/drivers/block/ll_rw_blk.c	2002-06-03 10:35:35.000000000 +0200
+++ linux/drivers/block/ll_rw_blk.c	2002-06-03 11:40:35.000000000 +0200
@@ -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);
 	}
 }
 


-- 
Jens Axboe

Re: [patch 1/16] unplugging fix

[Andrew Morton]

Jens Axboe wrote:
> 
> On Mon, Jun 03 2002, Andrew Morton wrote:
> > Jens Axboe wrote:
> > >
> > > ...
> > > 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 :(

> > 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?

-

Re: [patch 1/16] unplugging fix

[Jens Axboe]

On Mon, Jun 03 2002, Andrew Morton wrote:
> Jens Axboe wrote:
> > 
> > On Mon, Jun 03 2002, Andrew Morton wrote:
> > > Jens Axboe wrote:
> > > >
> > > > ...
> > > > 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 :(

Still good?

> > > 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?

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.

--- /opt/kernel/linux-2.5.20/drivers/block/ll_rw_blk.c	2002-06-03 10:35:35.000000000 +0200
+++ linux/drivers/block/ll_rw_blk.c	2002-06-04 09:23:12.000000000 +0200
@@ -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);
 	}
 }
 

-- 
Jens Axboe

Re: [patch 1/16] unplugging fix

[Jens Axboe]

On Tue, Jun 04 2002, Jens Axboe wrote:
> 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.

--- /opt/kernel/linux-2.5.20/drivers/block/ll_rw_blk.c	2002-06-03 10:35:35.000000000 +0200
+++ linux/drivers/block/ll_rw_blk.c	2002-06-04 09:23:12.000000000 +0200
@@ -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);
 	}
 }
 
--- /opt/kernel/linux-2.5.20/include/linux/blkdev.h	2002-06-03 10:35:40.000000000 +0200
+++ linux/include/linux/blkdev.h	2002-06-04 09:17:46.000000000 +0200
@@ -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)

-- 
Jens Axboe