-Eric
----- Original Message -----
From: "Gérard Roudier" <groudier@free.fr>
To: "Jonathan Lahr" <lahr@us.ibm.com>
Cc: <linux-kernel@vger.kernel.org>; <linux-scsi@vger.kernel.org>
Sent: Thursday, August 30, 2001 5:32 PM
Subject: Re: io_request_lock/queue_lock patch
>
> Here are my welcome comments. :)
>
> In my opinion, it would well be a miracle if your patch does not introduce
> new race conditions, at least for drivers that still use the old scsi done
> method.
>
> Gérard.
>
> On Thu, 30 Aug 2001, Jonathan Lahr wrote:
>
> > Included below is a snapshot of a patch I am developing to reduce
> > io_request_lock contention in 2.4.
> >
> > This patch changes __make_request to take the device-specific queue_lock
> > instead of io_request_lock and changes generic_unplug_device to take
> > queue_lock in addition to io_request_lock to serialize access to the
> > request queue. Also, locking in various functions has been modified
> > in accordance with this new scheme.
> >
> > I have done some testing of this with aic7xxx, qla2x00, and lpfc
adapters.
> >
> > To apply it to 2.4.6 or 2.4.7, from linux/ "patch -p1 < iorlv0_247".
> >
> > Comments and suggestions are welcome and invited.
> >
> > Jonathan
> >
> > ----- patch -----
> > diff -Naur linux.base/drivers/block/ll_rw_blk.c
linux.mod/drivers/block/ll_rw_blk.c
> > --- linux.base/drivers/block/ll_rw_blk.c Thu Jul 19 20:51:23 2001
> > +++ linux.mod/drivers/block/ll_rw_blk.c Fri Aug 24 10:17:38 2001
> > @@ -343,11 +343,13 @@
> > void generic_unplug_device(void *data)
> > {
> > request_queue_t *q = (request_queue_t *) data;
> > - unsigned long flags;
> > + unsigned long flags[2];
> >
> > - spin_lock_irqsave(&io_request_lock, flags);
> > + spin_lock_irqsave(&io_request_lock, flags[0]);
> > + spin_lock_irqsave(&q->queue_lock, flags[1]);
> > __generic_unplug_device(q);
> > - spin_unlock_irqrestore(&io_request_lock, flags);
> > + spin_unlock_irqrestore(&q->queue_lock, flags[1]);
> > + spin_unlock_irqrestore(&io_request_lock, flags[0]);
> > }
> >
> > static void blk_init_free_list(request_queue_t *q)
> > @@ -470,9 +472,9 @@
> > add_wait_queue_exclusive(&q->wait_for_request, &wait);
> > for (;;) {
> > __set_current_state(TASK_UNINTERRUPTIBLE);
> > - spin_lock_irq(&io_request_lock);
> > + spin_lock_irq(&q->queue_lock);
> > rq = get_request(q, rw);
> > - spin_unlock_irq(&io_request_lock);
> > + spin_unlock_irq(&q->queue_lock);
> > if (rq)
> > break;
> > generic_unplug_device(q);
> > @@ -487,9 +489,9 @@
> > {
> > register struct request *rq;
> >
> > - spin_lock_irq(&io_request_lock);
> > + spin_lock_irq(&q->queue_lock);
> > rq = get_request(q, rw);
> > - spin_unlock_irq(&io_request_lock);
> > + spin_unlock_irq(&q->queue_lock);
> > if (rq)
> > return rq;
> > return __get_request_wait(q, rw);
> > @@ -555,7 +557,7 @@
> > drive_stat_acct(req->rq_dev, req->cmd, req->nr_sectors, 1);
> >
> > if (!q->plugged && q->head_active && insert_here == &q->queue_head) {
> > - spin_unlock_irq(&io_request_lock);
> > + spin_unlock_irq(&q->queue_lock);
> > BUG();
> > }
> >
> > @@ -727,7 +729,7 @@
> > * Now we acquire the request spinlock, we have to be mega careful
> > * not to schedule or do something nonatomic
> > */
> > - spin_lock_irq(&io_request_lock);
> > + spin_lock_irq(&q->queue_lock);
> >
> > insert_here = head->prev;
> > if (list_empty(head)) {
> > @@ -794,7 +796,7 @@
> > req = freereq;
> > freereq = NULL;
> > } else if ((req = get_request(q, rw)) == NULL) {
> > - spin_unlock_irq(&io_request_lock);
> > + spin_unlock_irq(&q->queue_lock);
> > if (rw_ahead)
> > goto end_io;
> >
> > @@ -821,7 +823,7 @@
> > out:
> > if (freereq)
> > blkdev_release_request(freereq);
> > - spin_unlock_irq(&io_request_lock);
> > + spin_unlock_irq(&q->queue_lock);
> > return 0;
> > end_io:
> > bh->b_end_io(bh, test_bit(BH_Uptodate, &bh->b_state));
> > diff -Naur linux.base/drivers/scsi/scsi.c linux.mod/drivers/scsi/scsi.c
> > --- linux.base/drivers/scsi/scsi.c Thu Jul 19 21:07:04 2001
> > +++ linux.mod/drivers/scsi/scsi.c Fri Aug 24 11:06:25 2001
> > @@ -678,41 +678,33 @@
> > * passes a meaningful return value.
> > */
> > if (host->hostt->use_new_eh_code) {
> > - spin_lock_irqsave(&io_request_lock, flags);
> > rtn = host->hostt->queuecommand(SCpnt, scsi_done);
> > - spin_unlock_irqrestore(&io_request_lock,
flags);
> > if (rtn != 0) {
> > scsi_delete_timer(SCpnt);
> > scsi_mlqueue_insert(SCpnt, SCSI_MLQUEUE_HOST_BUSY);
> > SCSI_LOG_MLQUEUE(3,
printk("queuecommand : request rejected\n"));
> > }
> > } else {
> > - spin_lock_irqsave(&io_request_lock, flags);
> > host->hostt->queuecommand(SCpnt, scsi_old_done);
> > - spin_unlock_irqrestore(&io_request_lock,
flags);
> > }
> > } else {
> > int temp;
> >
> > SCSI_LOG_MLQUEUE(3, printk("command() : routine at %p\n",
host->hostt->command));
> > - spin_lock_irqsave(&io_request_lock, flags);
> > temp = host->hostt->command(SCpnt);
> > SCpnt->result = temp;
> > #ifdef DEBUG_DELAY
> > - spin_unlock_irqrestore(&io_request_lock, flags);
> > clock = jiffies + 4 * HZ;
> > while (time_before(jiffies, clock))
> > barrier();
> > printk("done(host = %d, result = %04x) : routine at %p\n",
> > host->host_no, temp, host->hostt->command);
> > - spin_lock_irqsave(&io_request_lock, flags);
> > #endif
> > if (host->hostt->use_new_eh_code) {
> > scsi_done(SCpnt);
> > } else {
> > scsi_old_done(SCpnt);
> > }
> > - spin_unlock_irqrestore(&io_request_lock, flags);
> > }
> > SCSI_LOG_MLQUEUE(3, printk("leaving scsi_dispatch_cmnd()\n"));
> > return rtn;
> > diff -Naur linux.base/drivers/scsi/scsi_lib.c
linux.mod/drivers/scsi/scsi_lib.c
> > --- linux.base/drivers/scsi/scsi_lib.c Thu Jul 19 20:48:04 2001
> > +++ linux.mod/drivers/scsi/scsi_lib.c Fri Aug 24 10:19:50 2001
> > @@ -68,7 +68,7 @@
> > static void __scsi_insert_special(request_queue_t *q, struct request
*rq,
> > void *data, int at_head)
> > {
> > - unsigned long flags;
> > + unsigned long flags[2];
> >
> > ASSERT_LOCK(&io_request_lock, 0);
> >
> > @@ -84,7 +84,8 @@
> > * head of the queue for things like a QUEUE_FULL message from a
> > * device, or a host that is unable to accept a particular command.
> > */
> > - spin_lock_irqsave(&io_request_lock, flags);
> > + spin_lock_irqsave(&io_request_lock, flags[0]);
> > + spin_lock_irqsave(&q->queue_lock, flags[1]);
> >
> > if (at_head)
> > list_add(&rq->queue, &q->queue_head);
> > @@ -92,7 +93,8 @@
> > list_add_tail(&rq->queue, &q->queue_head);
> >
> > q->request_fn(q);
> > - spin_unlock_irqrestore(&io_request_lock, flags);
> > + spin_unlock_irqrestore(&q->queue_lock, flags[1]);
> > + spin_unlock_irqrestore(&io_request_lock, flags[0]);
> > }
> >
> >
> > @@ -246,13 +248,14 @@
> > void scsi_queue_next_request(request_queue_t * q, Scsi_Cmnd * SCpnt)
> > {
> > int all_clear;
> > - unsigned long flags;
> > + unsigned long flags[2];
> > Scsi_Device *SDpnt;
> > struct Scsi_Host *SHpnt;
> >
> > ASSERT_LOCK(&io_request_lock, 0);
> >
> > - spin_lock_irqsave(&io_request_lock, flags);
> > + spin_lock_irqsave(&io_request_lock, flags[0]);
> > + spin_lock_irqsave(&q->queue_lock, flags[1]);
> > if (SCpnt != NULL) {
> >
> > /*
> > @@ -328,7 +331,8 @@
> > SHpnt->some_device_starved = 0;
> > }
> > }
> > - spin_unlock_irqrestore(&io_request_lock, flags);
> > + spin_unlock_irqrestore(&q->queue_lock, flags[1]);
> > + spin_unlock_irqrestore(&io_request_lock, flags[0]);
> > }
> >
> > /*
> > @@ -821,6 +825,7 @@
> > struct Scsi_Device_Template *STpnt;
> >
> > ASSERT_LOCK(&io_request_lock, 1);
> > + ASSERT_LOCK(&q->queue_lock, 1);
> >
> > SDpnt = (Scsi_Device *) q->queuedata;
> > if (!SDpnt) {
> > @@ -828,6 +833,7 @@
> > }
> > SHpnt = SDpnt->host;
> >
> > +
> > /*
> > * To start with, we keep looping until the queue is empty, or until
> > * the host is no longer able to accept any more requests.
> > @@ -839,7 +845,7 @@
> > * we need to check to see if the queue is plugged or not.
> > */
> > if (SHpnt->in_recovery || q->plugged)
> > - return;
> > + break;
> >
> > /*
> > * If the device cannot accept another request, then quit.
> > @@ -887,9 +893,11 @@
> > */
> > SDpnt->was_reset = 0;
> > if (SDpnt->removable && !in_interrupt()) {
> > + spin_unlock_irq(&q->queue_lock);
> > spin_unlock_irq(&io_request_lock);
> > scsi_ioctl(SDpnt, SCSI_IOCTL_DOORLOCK, 0);
> > spin_lock_irq(&io_request_lock);
> > + spin_lock_irq(&q->queue_lock);
> > continue;
> > }
> > }
> > @@ -998,8 +1006,6 @@
> > * another.
> > */
> > req = NULL;
> > - spin_unlock_irq(&io_request_lock);
> > -
> > if (SCpnt->request.cmd != SPECIAL) {
> > /*
> > * This will do a couple of things:
> > @@ -1028,7 +1034,6 @@
> > {
> > panic("Should not have leftover blocks\n");
> > }
> > - spin_lock_irq(&io_request_lock);
> > SHpnt->host_busy--;
> > SDpnt->device_busy--;
> > continue;
> > @@ -1044,7 +1049,6 @@
> > {
> > panic("Should not have leftover blocks\n");
> > }
> > - spin_lock_irq(&io_request_lock);
> > SHpnt->host_busy--;
> > SDpnt->device_busy--;
> > continue;
> > @@ -1065,7 +1069,6 @@
> > * Now we need to grab the lock again. We are about to mess
> > * with the request queue and try to find another command.
> > */
> > - spin_lock_irq(&io_request_lock);
> > }
> > }
> >
> > -
> > To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
>
> -
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/