Re: [patch 1/16] unplugging fix

Jens Axboe
2002-06-03T08:40:02Z

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