Commit c5e1bfd8 authored by Adam Kropelin's avatar Adam Kropelin Committed by Linus Torvalds

[PATCH] cpqarray SMP deadlock fix

This fixes some critical bugs in cpqarray in 2.5.  One of the fixes
essentially backs out the block queue stop/start behavior that was added
recently.  This code as it stands is buggy and locks up under even light
SMP workloads.  Certainly we want the performance benefits of proper
block queue plugging, but the driver needs some work before it will fit
nicely.

Some of these fixes do theoretically hurt performance, but when you
consider that the driver is unusable under SMP as-is, I think it is
right to get correctness first.

Specifically, this patch does the following:

* Adds locking to proc queue-walking code for debugging use.  Note that
  the proc registration is still broken and I've left it that way since
  this stuff should probably migrate to driverfs anyway.

* Moves interrupt enabling so queue lock is initialized before
  interrupts are enabled.  Otherwise if we get a quick interrupt we oops
  the machine.

* Removes unconditional IRQ enabling in do_ida_request().  The block
  layer takes the spinlock with irq_save so if we're going to play this
  trick then we need to irq_restore.  For now, just eliminate the
  unlocked region.

* Remove block queue stop/start logic since it can leave the queue
  stopped with no outstanding completions to start it again.  Plugging
  logic can come back but it should go hand-in-hand with a cleanup of
  the driver's request handling algorithm.  If nobody screams about this
  patch I'll go ahead and start making those improvments.
parent 801b25c2
...@@ -200,6 +200,7 @@ static int ida_proc_get_info(char *buffer, char **start, off_t offset, int lengt ...@@ -200,6 +200,7 @@ static int ida_proc_get_info(char *buffer, char **start, off_t offset, int lengt
drv_info_t *drv; drv_info_t *drv;
#ifdef CPQ_PROC_PRINT_QUEUES #ifdef CPQ_PROC_PRINT_QUEUES
cmdlist_t *c; cmdlist_t *c;
unsigned long flags;
#endif #endif
ctlr = h->ctlr; ctlr = h->ctlr;
...@@ -236,6 +237,7 @@ static int ida_proc_get_info(char *buffer, char **start, off_t offset, int lengt ...@@ -236,6 +237,7 @@ static int ida_proc_get_info(char *buffer, char **start, off_t offset, int lengt
} }
#ifdef CPQ_PROC_PRINT_QUEUES #ifdef CPQ_PROC_PRINT_QUEUES
spin_lock_irqsave(IDA_LOCK(h->ctlr), flags);
size = sprintf(buffer+len, "\nCurrent Queues:\n"); size = sprintf(buffer+len, "\nCurrent Queues:\n");
pos += size; len += size; pos += size; len += size;
...@@ -258,6 +260,7 @@ static int ida_proc_get_info(char *buffer, char **start, off_t offset, int lengt ...@@ -258,6 +260,7 @@ static int ida_proc_get_info(char *buffer, char **start, off_t offset, int lengt
} }
size = sprintf(buffer+len, "\n"); pos += size; len += size; size = sprintf(buffer+len, "\n"); pos += size; len += size;
spin_unlock_irqrestore(IDA_LOCK(h->ctlr), flags);
#endif #endif
size = sprintf(buffer+len, "nr_allocs = %d\nnr_frees = %d\n", size = sprintf(buffer+len, "nr_allocs = %d\nnr_frees = %d\n",
h->nr_allocs, h->nr_frees); h->nr_allocs, h->nr_frees);
...@@ -373,8 +376,6 @@ static int __init cpqarray_init(void) ...@@ -373,8 +376,6 @@ static int __init cpqarray_init(void)
getgeometry(i); getgeometry(i);
start_fwbk(i); start_fwbk(i);
hba[i]->access.set_intr_mask(hba[i], FIFO_NOT_EMPTY);
ida_procinit(i); ida_procinit(i);
q = BLK_DEFAULT_QUEUE(MAJOR_NR + i); q = BLK_DEFAULT_QUEUE(MAJOR_NR + i);
...@@ -395,6 +396,9 @@ static int __init cpqarray_init(void) ...@@ -395,6 +396,9 @@ static int __init cpqarray_init(void)
hba[i]->timer.function = ida_timer; hba[i]->timer.function = ida_timer;
add_timer(&hba[i]->timer); add_timer(&hba[i]->timer);
/* Enable IRQ now that spinlock and rate limit timer are set up */
hba[i]->access.set_intr_mask(hba[i], FIFO_NOT_EMPTY);
for(j=0; j<NWD; j++) { for(j=0; j<NWD; j++) {
struct gendisk *disk = ida_gendisk[i][j]; struct gendisk *disk = ida_gendisk[i][j];
drv_info_t *drv = &hba[i]->drv[j]; drv_info_t *drv = &hba[i]->drv[j];
...@@ -809,8 +813,6 @@ static void do_ida_request(request_queue_t *q) ...@@ -809,8 +813,6 @@ static void do_ida_request(request_queue_t *q)
blkdev_dequeue_request(creq); blkdev_dequeue_request(creq);
spin_unlock_irq(q->queue_lock);
c->ctlr = h->ctlr; c->ctlr = h->ctlr;
c->hdr.unit = minor(creq->rq_dev) >> NWD_SHIFT; c->hdr.unit = minor(creq->rq_dev) >> NWD_SHIFT;
c->hdr.size = sizeof(rblk_t) >> 2; c->hdr.size = sizeof(rblk_t) >> 2;
...@@ -842,8 +844,6 @@ DBGPX( printk("Submitting %d sectors in %d segments\n", creq->nr_sectors, seg); ...@@ -842,8 +844,6 @@ DBGPX( printk("Submitting %d sectors in %d segments\n", creq->nr_sectors, seg);
c->req.hdr.cmd = (rq_data_dir(creq) == READ) ? IDA_READ : IDA_WRITE; c->req.hdr.cmd = (rq_data_dir(creq) == READ) ? IDA_READ : IDA_WRITE;
c->type = CMD_RWREQ; c->type = CMD_RWREQ;
spin_lock_irq(q->queue_lock);
/* Put the request on the tail of the request queue */ /* Put the request on the tail of the request queue */
addQ(&h->reqQ, c); addQ(&h->reqQ, c);
h->Qdepth++; h->Qdepth++;
...@@ -853,7 +853,6 @@ DBGPX( printk("Submitting %d sectors in %d segments\n", creq->nr_sectors, seg); ...@@ -853,7 +853,6 @@ DBGPX( printk("Submitting %d sectors in %d segments\n", creq->nr_sectors, seg);
goto queue_next; goto queue_next;
startio: startio:
__blk_stop_queue(q);
start_io(h); start_io(h);
} }
...@@ -1006,8 +1005,8 @@ static void do_ida_intr(int irq, void *dev_id, struct pt_regs *regs) ...@@ -1006,8 +1005,8 @@ static void do_ida_intr(int irq, void *dev_id, struct pt_regs *regs)
/* /*
* See if we can queue up some more IO * See if we can queue up some more IO
*/ */
spin_unlock_irqrestore(IDA_LOCK(h->ctlr), flags); do_ida_request(BLK_DEFAULT_QUEUE(MAJOR_NR+h->ctlr));
blk_start_queue(BLK_DEFAULT_QUEUE(MAJOR_NR + h->ctlr)); spin_unlock_irqrestore(IDA_LOCK(h->ctlr), flags);
} }
/* /*
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment