[PATCH] scsi_requeuest_fn

[PATCH] scsi_requeuest_fn

[Christoph Hellwig]

Okay, when doing some other stuff I looked over this one, and it's
a bit confusing to read:

 - using a goto completed where a simple break would be sufficient
 - using for (;;) for a perfectly fine while loop
 - ...

but what's more interesting is that the spinlock handling in here,
when we switch from sdev_lock/queue_lock to host_lock we
do a spin_unlock_irq followed by a spin_lock_irqsave - but we
we just enabled interrupts so the save isn't nessecary at all, even
more we can just do spin_unlock/spin_lock when keeping them
disabled.  Also we drop host_lock in the middle of this function,
just to reacquire it a tad later in scsi_dispatch_cmd, but fixing
that need a bit more thinking as there's another caller for
scsi_dispatch_cmd.


--- 1.84/drivers/scsi/scsi_lib.c	Mon Apr 21 10:17:33 2003
+++ edited/drivers/scsi/scsi_lib.c	Sun Apr 27 14:19:35 2003
@@ -1140,66 +1128,61 @@
  *
  * Lock status: IO request lock assumed to be held when called.
  */
-static void scsi_request_fn(request_queue_t *q)
+static void scsi_request_fn(struct request_queue *q)
 {
 	struct scsi_device *sdev = q->queuedata;
 	struct Scsi_Host *shost = sdev->host;
 	struct scsi_cmnd *cmd;
 	struct request *req;
-	unsigned long flags;
 
 	/*
 	 * To start with, we keep looping until the queue is empty, or until
 	 * the host is no longer able to accept any more requests.
 	 */
-	for (;;) {
-		if (blk_queue_plugged(q))
-			goto completed;
-
+	while (!blk_queue_plugged(q)) {
 		/*
 		 * get next queueable request.  We do this early to make sure
 		 * that the request is fully prepared even if we cannot 
 		 * accept it.
 		 */
 		req = elv_next_request(q);
-
-		if (!req)
-			goto completed;
-
-		if (!scsi_dev_queue_ready(q, sdev))
-			goto completed;
+		if (!req || !scsi_dev_queue_ready(q, sdev))
+			break;
 
 		/*
 		 * Remove the request from the request list.
 		 */
-		if (!(blk_queue_tagged(q) && (blk_queue_start_tag(q, req) == 0)))
+		if (!(blk_queue_tagged(q) && !blk_queue_start_tag(q, req)))
 			blkdev_dequeue_request(req);
-
 		sdev->device_busy++;
-		spin_unlock_irq(q->queue_lock);
 
-		spin_lock_irqsave(shost->host_lock, flags);
-		if (!scsi_host_queue_ready(q, shost, sdev))
-			goto host_lock_held;
+		spin_unlock(q->queue_lock);
+		spin_lock(shost->host_lock);
 
+		if (!scsi_host_queue_ready(q, shost, sdev))
+			goto not_ready;
 		if (sdev->single_lun) {
 			if (sdev->sdev_target->starget_sdev_user &&
-			    (sdev->sdev_target->starget_sdev_user != sdev))
-				goto host_lock_held;
-			else
-				sdev->sdev_target->starget_sdev_user = sdev;
+			    sdev->sdev_target->starget_sdev_user != sdev)
+				goto not_ready;
+			sdev->sdev_target->starget_sdev_user = sdev;
 		}
-
 		shost->host_busy++;
-		spin_unlock_irqrestore(shost->host_lock, flags);
-
-		cmd = req->special;
 
 		/*
-		 * Should be impossible for a correctly prepared request
-		 * please mail the stack trace to linux-scsi@vger.kernel.org
+		 * XXX(hch): This is rather suboptimal, scsi_dispatch_cmd will
+		 *		take the lock again.
 		 */
-		BUG_ON(!cmd);
+		spin_unlock_irq(shost->host_lock);
+
+		cmd = req->special;
+		if (unlikely(cmd == NULL)) {
+			printk(KERN_CRIT "impossible request in %s.\n"
+					 "please mail a stack trace to "
+					 "linux-scsi@vger.kernel.org",
+					 __FUNCTION__);
+			BUG();
+		}
 
 		/*
 		 * Finally, initialize any error handling parameters, and set up
@@ -1211,18 +1194,14 @@
 		 * Dispatch the command to the low-level driver.
 		 */
 		scsi_dispatch_cmd(cmd);
-
-		/*
-		 * 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(q->queue_lock);
 	}
-completed:
+
 	return;
 
-host_lock_held:
-	spin_unlock_irqrestore(shost->host_lock, flags);
+ not_ready:
+	spin_unlock_irq(shost->host_lock);
+
 	/*
 	 * lock q, handle tag, requeue req, and decrement device_busy. We
 	 * must return with queue_lock held.

Re: [PATCH] scsi_requeuest_fn

[Patrick Mansfield]

On Sun, Apr 27, 2003 at 05:04:40PM +0200, Christoph Hellwig wrote:

> --- 1.84/drivers/scsi/scsi_lib.c	Mon Apr 21 10:17:33 2003
> +++ edited/drivers/scsi/scsi_lib.c	Sun Apr 27 14:19:35 2003
> @@ -1140,66 +1128,61 @@
>   *
>   * Lock status: IO request lock assumed to be held when called.
>   */
> -static void scsi_request_fn(request_queue_t *q)
> +static void scsi_request_fn(struct request_queue *q)
>  {
>  	struct scsi_device *sdev = q->queuedata;
>  	struct Scsi_Host *shost = sdev->host;
>  	struct scsi_cmnd *cmd;
>  	struct request *req;
> -	unsigned long flags;
>  
>  	/*
>  	 * To start with, we keep looping until the queue is empty, or until
>  	 * the host is no longer able to accept any more requests.
>  	 */

You might as well remove all or most of the comments, none of them add
much information, the comments might as well say "we are about to
loop" or "we are about to call scsi_dispatch_cmd".

> -		BUG_ON(!cmd);
> +		spin_unlock_irq(shost->host_lock);
> +
> +		cmd = req->special;
> +		if (unlikely(cmd == NULL)) {
> +			printk(KERN_CRIT "impossible request in %s.\n"
> +					 "please mail a stack trace to "
> +					 "linux-scsi@vger.kernel.org",
> +					 __FUNCTION__);
> +			BUG();
> +		}

The scsi_init_cmd_errh(cmd) will oops anyway if cmd == NULL, so just remove
the check.

Also - do you have a patch rollup somewhere (in case these recent
patches aren't applied soon)?

-- Patrick Mansfield

Re: [PATCH] scsi_requeuest_fn

[Christoph Hellwig]

On Sun, Apr 27, 2003 at 11:07:11AM -0700, Patrick Mansfield wrote:
> You might as well remove all or most of the comments, none of them add
> much information, the comments might as well say "we are about to
> loop" or "we are about to call scsi_dispatch_cmd".

> The scsi_init_cmd_errh(cmd) will oops anyway if cmd == NULL, so just remove
> the check.
>
Okay, updated patch below.

> Also - do you have a patch rollup somewhere (in case these recent
> patches aren't applied soon)?

Currently I don't have.  I'll merge everything together if this isn't
applied until tomorrow.


--- 1.84/drivers/scsi/scsi_lib.c	Mon Apr 21 10:17:33 2003
+++ edited/drivers/scsi/scsi_lib.c	Sun Apr 27 18:54:46 2003
@@ -1140,89 +1128,56 @@
  *
  * Lock status: IO request lock assumed to be held when called.
  */
-static void scsi_request_fn(request_queue_t *q)
+static void scsi_request_fn(struct request_queue *q)
 {
 	struct scsi_device *sdev = q->queuedata;
 	struct Scsi_Host *shost = sdev->host;
 	struct scsi_cmnd *cmd;
 	struct request *req;
-	unsigned long flags;
-
-	/*
-	 * To start with, we keep looping until the queue is empty, or until
-	 * the host is no longer able to accept any more requests.
-	 */
-	for (;;) {
-		if (blk_queue_plugged(q))
-			goto completed;
 
+	while (!blk_queue_plugged(q)) {
 		/*
 		 * get next queueable request.  We do this early to make sure
 		 * that the request is fully prepared even if we cannot 
 		 * accept it.
 		 */
 		req = elv_next_request(q);
-
-		if (!req)
-			goto completed;
-
-		if (!scsi_dev_queue_ready(q, sdev))
-			goto completed;
-
-		/*
-		 * Remove the request from the request list.
-		 */
-		if (!(blk_queue_tagged(q) && (blk_queue_start_tag(q, req) == 0)))
+		if (!req || !scsi_dev_queue_ready(q, sdev))
+			break;
+		if (!(blk_queue_tagged(q) && !blk_queue_start_tag(q, req)))
 			blkdev_dequeue_request(req);
-
 		sdev->device_busy++;
-		spin_unlock_irq(q->queue_lock);
 
-		spin_lock_irqsave(shost->host_lock, flags);
-		if (!scsi_host_queue_ready(q, shost, sdev))
-			goto host_lock_held;
+		spin_unlock(q->queue_lock);
+		spin_lock(shost->host_lock);
 
+		if (!scsi_host_queue_ready(q, shost, sdev))
+			goto not_ready;
 		if (sdev->single_lun) {
 			if (sdev->sdev_target->starget_sdev_user &&
-			    (sdev->sdev_target->starget_sdev_user != sdev))
-				goto host_lock_held;
-			else
-				sdev->sdev_target->starget_sdev_user = sdev;
+			    sdev->sdev_target->starget_sdev_user != sdev)
+				goto not_ready;
+			sdev->sdev_target->starget_sdev_user = sdev;
 		}
 
 		shost->host_busy++;
-		spin_unlock_irqrestore(shost->host_lock, flags);
-
-		cmd = req->special;
 
 		/*
-		 * Should be impossible for a correctly prepared request
-		 * please mail the stack trace to linux-scsi@vger.kernel.org
-		 */
-		BUG_ON(!cmd);
-
-		/*
-		 * Finally, initialize any error handling parameters, and set up
-		 * the timers for timeouts.
+		 * XXX(hch): This is rather suboptimal, scsi_dispatch_cmd will
+		 *		take the lock again.
 		 */
+		spin_unlock_irq(shost->host_lock);
+		cmd = req->special;
 		scsi_init_cmd_errh(cmd);
-
-		/*
-		 * Dispatch the command to the low-level driver.
-		 */
 		scsi_dispatch_cmd(cmd);
-
-		/*
-		 * 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(q->queue_lock);
 	}
-completed:
+
 	return;
 
-host_lock_held:
-	spin_unlock_irqrestore(shost->host_lock, flags);
+ not_ready:
+	spin_unlock_irq(shost->host_lock);
+
 	/*
 	 * lock q, handle tag, requeue req, and decrement device_busy. We
 	 * must return with queue_lock held.

Re: [PATCH] scsi_requeuest_fn

[Patrick Mansfield]

On Sun, Apr 27, 2003 at 08:53:03PM +0200, Christoph Hellwig wrote:
> 
> > Also - do you have a patch rollup somewhere (in case these recent
> > patches aren't applied soon)?
> 
> Currently I don't have.  I'll merge everything together if this isn't
> applied until tomorrow.
> 

FYI -

I compiled and booted (on one machine with aic driver) with all but the
first (the header split up) and last patch (changes to drivers I don't
use) applied.

The header split up had a conflict with the nuke externs, plus the
missing include mentioned in a separate email.

-- Patrick Mansfield

Re: [PATCH] scsi_requeuest_fn

[Christoph Hellwig]

On Mon, Apr 28, 2003 at 10:14:11AM -0700, Patrick Mansfield wrote:
> I compiled and booted (on one machine with aic driver) with all but the
> first (the header split up) and last patch (changes to drivers I don't
> use) applied.
> 
> The header split up had a conflict with the nuke externs, plus the
> missing include mentioned in a separate email.

oh well, that happens if you need to split up the huge delta of
your working tree vs the upstream tree manually :P  I'll only hope
that James could apply some of the patches so this delta gets down a bit..