[PATCH] scsi_lib: don't decrement busy counters when inserting commands

[PATCH] scsi_lib: don't decrement busy counters when inserting commands

[michaelc]

From: Mike Christie <michaelc@cs.wisc.edu>

This patch fixes a regression in scsi-misc introduced with:
312efe5efcdb02d604ea37a41d965f32ca276d6a
[SCSI] simplify scsi_io_completion().

The problem is that in previous kernels scsi_io_completion would call
scsi_requeue_command, but now it can call scsi_queue_insert for
something like a UNIT_ATTENTION (for netapp targets we get
UNIT_ATTENTION when restarting a iscsi session). And scsi_queue_insert
will call scsi_device_unbusy, but in the scsi_io_completion code path
scsi_finish_command has already called this so we now end up
with invalid host, target and device busy values.

Patch was made over scsi-misc.

Signed-off-by: Mike Christie <michaelc@cs.wisc.edu>
---
 drivers/scsi/scsi_lib.c |   16 +++++++++-------
 include/scsi/scsi.h     |    9 +++++++++
 2 files changed, 18 insertions(+), 7 deletions(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 111f9e9..a47fcac 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -139,6 +139,7 @@ int scsi_queue_insert(struct scsi_cmnd *cmd, int reason)
 		host->host_blocked = host->max_host_blocked;
 		break;
 	case SCSI_MLQUEUE_DEVICE_BUSY:
+	case SCSI_IO_COMPL_DEV_BUSY:
 		device->device_blocked = device->max_device_blocked;
 		break;
 	case SCSI_MLQUEUE_TARGET_BUSY:
@@ -146,11 +147,12 @@ int scsi_queue_insert(struct scsi_cmnd *cmd, int reason)
 		break;
 	}
 
-	/*
-	 * Decrement the counters, since these commands are no longer
-	 * active on the host/device.
-	 */
-	scsi_device_unbusy(device);
+	if (!SCSI_IO_COMPL_RET(reason))
+		/*
+		 * Decrement the counters, since these commands are no longer
+		 * active on the host/device.
+		 */
+		scsi_device_unbusy(device);
 
 	/*
 	 * Requeue this command.  It will go before all other commands
@@ -1071,11 +1073,11 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
 		break;
 	case ACTION_RETRY:
 		/* Retry the same command immediately */
-		scsi_queue_insert(cmd, SCSI_MLQUEUE_EH_RETRY);
+		scsi_queue_insert(cmd, SCSI_IO_COMPL_EH_RETRY);
 		break;
 	case ACTION_DELAYED_RETRY:
 		/* Retry the same command after a delay */
-		scsi_queue_insert(cmd, SCSI_MLQUEUE_DEVICE_BUSY);
+		scsi_queue_insert(cmd, SCSI_IO_COMPL_DEV_BUSY);
 		break;
 	}
 }
diff --git a/include/scsi/scsi.h b/include/scsi/scsi.h
index a109165..e9cc4fc 100644
--- a/include/scsi/scsi.h
+++ b/include/scsi/scsi.h
@@ -434,6 +434,15 @@ static inline int scsi_is_wlun(unsigned int lun)
 #define SCSI_MLQUEUE_TARGET_BUSY 0x1058
 
 /*
+ * Midlevel io completeion path return values
+ */
+#define SCSI_IO_COMPL_EH_RETRY	 0x1059
+#define SCSI_IO_COMPL_DEV_BUSY	 0x1060
+
+#define SCSI_IO_COMPL_RET(_ret) \
+	(_ret == SCSI_IO_COMPL_EH_RETRY || _ret == SCSI_IO_COMPL_DEV_BUSY)
+
+/*
  *  Use these to separate status msg and our bytes
  *
  *  These are set by:
-- 
1.6.0.4

Re: [PATCH] scsi_lib: don't decrement busy counters when inserting commands

[Andrew Vasquez]

On Tue, 16 Dec 2008, michaelc@cs.wisc.edu wrote:

> This patch fixes a regression in scsi-misc introduced with:
> 312efe5efcdb02d604ea37a41d965f32ca276d6a
> [SCSI] simplify scsi_io_completion().
> 
> The problem is that in previous kernels scsi_io_completion would call
> scsi_requeue_command, but now it can call scsi_queue_insert for
> something like a UNIT_ATTENTION (for netapp targets we get
> UNIT_ATTENTION when restarting a iscsi session). And scsi_queue_insert
> will call scsi_device_unbusy, but in the scsi_io_completion code path
> scsi_finish_command has already called this so we now end up
> with invalid host, target and device busy values.

Thanks for finding this, as we've run into this problem ourselves.
After applying your fix, at least I/O appears to continue...  But
there's still some residual problems left over with the changes from
scsi_requeue_command() -> scsi_queue_insert() in
312efe5efcdb02d604ea37a41d965f32ca276d6a that are causing commands
returned by the LLD with a DID_RESET status to be reissued with
cleared cmd->sdb data which in our tests are manifesting in firmware
detected overruns.  Here's a snippet of a READ_10 scsi_cmnd upon
completion by the storage:

	[  107.083085] scsi(4:0:0): OVERRUN status detected 0x7-0x400
	[  107.087081] CDB: 0x28 0x0 0x0 0x0 0x28 0x0
	[  107.087081] CDB: 0x0 0x4 0x0 0x0 0x0 0x0
	[  107.087081] PID=0x82d req=0x0 xtra=0x80000 -- returning DID_ERROR status!
	[  107.087081] cmd: serial=82d scsi_bufflen=0 retries=0 allowed=5 sc_data_direction=2
	[  107.087081] cmd->sdb: length=0 resid=0
	[  107.087081] cmd->sdb.table: sgl=0000000000000000 nents=0 orig_nents=0
	[  107.087081] req: cmd_flags=83c60 hard_nr_sections=0x400 nr_sectors=0x400 data_len=0x80000

the CDB in question contains a valid transfer-length of 400h blocks,
but the scsi_bufflen() of the scsi_cmnd is set to 0.

With the new 'simplify' code, what appears to be happening is the
scsi_cmnd->sdb structure is never being re-populated within
scsi_io_completion() after the scsi_release_buffers() call as
scsi_queue_insert() simply reissues the request with the previously
allocated scsi_cmnd and the request's REQ_DONTPREP bit set...

The original 'pre-simplified' code, as Mike C. notes, would call
scsi_requeue_command() which tore-down the request's scsi_cmnd prior
to retry.

I've tried this small patch:

	--- a/drivers/scsi/scsi_lib.c
	+++ b/drivers/scsi/scsi_lib.c
	@@ -970,7 +973,7 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
			 * reasons.  Just retry the command and see what
			 * happens.
			 */
	-		action = ACTION_RETRY;
	+		action = ACTION_REPREP;
		} else if (sense_valid && !sense_deferred) {
			switch (sshdr.sense_key) {
			case UNIT_ATTENTION:

but, it doesn't appear to solve the problem either.  Additionally,
it's not going to cover any other cases which ultimately result in
scsi_queue_insert() being used for retries...

Any thoughts on how to handle this???

Re: [PATCH] scsi_lib: don't decrement busy counters when inserting commands

[James Bottomley]

On Tue, 2008-12-16 at 23:55 -0600, michaelc@cs.wisc.edu wrote:
> From: Mike Christie <michaelc@cs.wisc.edu>
> 
> This patch fixes a regression in scsi-misc introduced with:
> 312efe5efcdb02d604ea37a41d965f32ca276d6a
> [SCSI] simplify scsi_io_completion().
> 
> The problem is that in previous kernels scsi_io_completion would call
> scsi_requeue_command, but now it can call scsi_queue_insert for
> something like a UNIT_ATTENTION (for netapp targets we get
> UNIT_ATTENTION when restarting a iscsi session). And scsi_queue_insert
> will call scsi_device_unbusy, but in the scsi_io_completion code path
> scsi_finish_command has already called this so we now end up
> with invalid host, target and device busy values.
> 
> Patch was made over scsi-misc.

This is a bad bug, but not quite the way I'd like to fix it.  Whether
the queue should be unbusied or not is really separate from the block
action, so it should have its own flag (plus it's not really a flag many
people should be using).  Since, really, the wrong use is confined to
the defining file, how about this:

James

---

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index f2f51e0..c6d48eb 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -91,26 +91,19 @@ static void scsi_unprep_request(struct request *req)
 	scsi_put_command(cmd);
 }
 
-/*
- * Function:    scsi_queue_insert()
- *
- * Purpose:     Insert a command in the midlevel queue.
- *
- * Arguments:   cmd    - command that we are adding to queue.
- *              reason - why we are inserting command to queue.
- *
- * Lock status: Assumed that lock is not held upon entry.
- *
- * Returns:     Nothing.
- *
- * Notes:       We do this for one of two cases.  Either the host is busy
- *              and it cannot accept any more commands for the time being,
- *              or the device returned QUEUE_FULL and can accept no more
- *              commands.
- * Notes:       This could be called either from an interrupt context or a
- *              normal process context.
+/**
+ * __scsi_queue_insert - private queue insertion
+ * @cmd: The SCSI command being requeued
+ * @reason:  The reason for the requeue
+ * @unbusy: Whether the queue should be unbusied
+ *
+ * This is a private queue insertion.  The public interface
+ * scsi_queue_insert() always assumes the queue should be unbusied
+ * because it's always called before the completion.  This function is
+ * for a requeue after completion, which should only occur in this
+ * file.
  */
-int scsi_queue_insert(struct scsi_cmnd *cmd, int reason)
+static int __scsi_queue_insert(struct scsi_cmnd *cmd, int reason, int unbusy)
 {
 	struct Scsi_Host *host = cmd->device->host;
 	struct scsi_device *device = cmd->device;
@@ -150,7 +143,8 @@ int scsi_queue_insert(struct scsi_cmnd *cmd, int reason)
 	 * Decrement the counters, since these commands are no longer
 	 * active on the host/device.
 	 */
-	scsi_device_unbusy(device);
+	if (unbusy)
+		scsi_device_unbusy(device);
 
 	/*
 	 * Requeue this command.  It will go before all other commands
@@ -172,6 +166,29 @@ int scsi_queue_insert(struct scsi_cmnd *cmd, int reason)
 	return 0;
 }
 
+/*
+ * Function:    scsi_queue_insert()
+ *
+ * Purpose:     Insert a command in the midlevel queue.
+ *
+ * Arguments:   cmd    - command that we are adding to queue.
+ *              reason - why we are inserting command to queue.
+ *
+ * Lock status: Assumed that lock is not held upon entry.
+ *
+ * Returns:     Nothing.
+ *
+ * Notes:       We do this for one of two cases.  Either the host is busy
+ *              and it cannot accept any more commands for the time being,
+ *              or the device returned QUEUE_FULL and can accept no more
+ *              commands.
+ * Notes:       This could be called either from an interrupt context or a
+ *              normal process context.
+ */
+int scsi_queue_insert(struct scsi_cmnd *cmd, int reason)
+{
+	return __scsi_queue_insert(cmd, reason, 1);
+}
 /**
  * scsi_execute - insert request and wait for the result
  * @sdev:	scsi device
@@ -1071,11 +1088,11 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
 		break;
 	case ACTION_RETRY:
 		/* Retry the same command immediately */
-		scsi_queue_insert(cmd, SCSI_MLQUEUE_EH_RETRY);
+		__scsi_queue_insert(cmd, SCSI_MLQUEUE_EH_RETRY, 0);
 		break;
 	case ACTION_DELAYED_RETRY:
 		/* Retry the same command after a delay */
-		scsi_queue_insert(cmd, SCSI_MLQUEUE_DEVICE_BUSY);
+		__scsi_queue_insert(cmd, SCSI_MLQUEUE_DEVICE_BUSY, 0);
 		break;
 	}
 }

Re: [PATCH] scsi_lib: don't decrement busy counters when inserting commands

[Alan Stern]

On Fri, 2 Jan 2009, James Bottomley wrote:

> On Tue, 2008-12-16 at 23:55 -0600, michaelc@cs.wisc.edu wrote:
> > From: Mike Christie <michaelc@cs.wisc.edu>
> > 
> > This patch fixes a regression in scsi-misc introduced with:
> > 312efe5efcdb02d604ea37a41d965f32ca276d6a
> > [SCSI] simplify scsi_io_completion().
> > 
> > The problem is that in previous kernels scsi_io_completion would call
> > scsi_requeue_command, but now it can call scsi_queue_insert for
> > something like a UNIT_ATTENTION (for netapp targets we get
> > UNIT_ATTENTION when restarting a iscsi session). And scsi_queue_insert
> > will call scsi_device_unbusy, but in the scsi_io_completion code path
> > scsi_finish_command has already called this so we now end up
> > with invalid host, target and device busy values.
> > 
> > Patch was made over scsi-misc.
> 
> This is a bad bug, but not quite the way I'd like to fix it.  Whether
> the queue should be unbusied or not is really separate from the block
> action, so it should have its own flag (plus it's not really a flag many
> people should be using).  Since, really, the wrong use is confined to
> the defining file, how about this:

Seems entirely reasonable.  This points out the potential for trouble
when a non-expert (like me!) tries to modify a complicated subsystem.  
Still, many eyeballs and all that...

Thanks to both of you,

Alan Stern

Re: [PATCH] scsi_lib: don't decrement busy counters when inserting commands

[Mike Christie]

James Bottomley wrote:
> On Tue, 2008-12-16 at 23:55 -0600, michaelc@cs.wisc.edu wrote:
>> From: Mike Christie <michaelc@cs.wisc.edu>
>>
>> This patch fixes a regression in scsi-misc introduced with:
>> 312efe5efcdb02d604ea37a41d965f32ca276d6a
>> [SCSI] simplify scsi_io_completion().
>>
>> The problem is that in previous kernels scsi_io_completion would call
>> scsi_requeue_command, but now it can call scsi_queue_insert for
>> something like a UNIT_ATTENTION (for netapp targets we get
>> UNIT_ATTENTION when restarting a iscsi session). And scsi_queue_insert
>> will call scsi_device_unbusy, but in the scsi_io_completion code path
>> scsi_finish_command has already called this so we now end up
>> with invalid host, target and device busy values.
>>
>> Patch was made over scsi-misc.
> 
> This is a bad bug, but not quite the way I'd like to fix it.  Whether
> the queue should be unbusied or not is really separate from the block
> action, so it should have its own flag (plus it's not really a flag many
> people should be using).  Since, really, the wrong use is confined to
> the defining file, how about this:
> 

Patch fixed my problem here. Thanks.

Re: [PATCH] scsi_lib: don't decrement busy counters when inserting commands

[Andrew Vasquez]

On Fri, 02 Jan 2009, James Bottomley wrote:

> On Tue, 2008-12-16 at 23:55 -0600, michaelc@cs.wisc.edu wrote:
> > From: Mike Christie <michaelc@cs.wisc.edu>
> > 
> > This patch fixes a regression in scsi-misc introduced with:
> > 312efe5efcdb02d604ea37a41d965f32ca276d6a
> > [SCSI] simplify scsi_io_completion().
> > 
> > The problem is that in previous kernels scsi_io_completion would call
> > scsi_requeue_command, but now it can call scsi_queue_insert for
> > something like a UNIT_ATTENTION (for netapp targets we get
> > UNIT_ATTENTION when restarting a iscsi session). And scsi_queue_insert
> > will call scsi_device_unbusy, but in the scsi_io_completion code path
> > scsi_finish_command has already called this so we now end up
> > with invalid host, target and device busy values.
> > 
> > Patch was made over scsi-misc.
> 
> This is a bad bug, but not quite the way I'd like to fix it.  Whether
> the queue should be unbusied or not is really separate from the block
> action, so it should have its own flag (plus it's not really a flag many
> people should be using).  Since, really, the wrong use is confined to
> the defining file, how about this:

Hmm, with this patch I'm still running into the problems I described
here:

http://article.gmane.org/gmane.linux.scsi/47029

basically, I/Os returned with a DID_RESET status are being requeued in
an incomplete state:

	[ 2362.068342] scsi(5:0:13): OVERRUN status detected 0x7-0x400
	[ 2362.068345] CDB: 0x2a 0x0 0x0 0x0 0xe 0x70
	[ 2362.068347] PID=0x81eeb req=0x0 xtra=0x1c00 -- returning DID_ERROR status!
	[ 2362.068411] scsi(5:0:13): OVERRUN status detected 0x7-0x400
	[ 2362.068413] CDB: 0x2a 0x0 0x0 0x0 0xe 0x70
	[ 2362.068415] PID=0x81eec req=0x0 xtra=0x1c00 -- returning DID_ERROR status!

:(

Thoughts?

Re: [PATCH] scsi_lib: don't decrement busy counters when inserting commands

[James Bottomley]

On Mon, 2009-01-05 at 15:28 -0800, Andrew Vasquez wrote:
> On Fri, 02 Jan 2009, James Bottomley wrote:
> 
> > On Tue, 2008-12-16 at 23:55 -0600, michaelc@cs.wisc.edu wrote:
> > > From: Mike Christie <michaelc@cs.wisc.edu>
> > > 
> > > This patch fixes a regression in scsi-misc introduced with:
> > > 312efe5efcdb02d604ea37a41d965f32ca276d6a
> > > [SCSI] simplify scsi_io_completion().
> > > 
> > > The problem is that in previous kernels scsi_io_completion would call
> > > scsi_requeue_command, but now it can call scsi_queue_insert for
> > > something like a UNIT_ATTENTION (for netapp targets we get
> > > UNIT_ATTENTION when restarting a iscsi session). And scsi_queue_insert
> > > will call scsi_device_unbusy, but in the scsi_io_completion code path
> > > scsi_finish_command has already called this so we now end up
> > > with invalid host, target and device busy values.
> > > 
> > > Patch was made over scsi-misc.
> > 
> > This is a bad bug, but not quite the way I'd like to fix it.  Whether
> > the queue should be unbusied or not is really separate from the block
> > action, so it should have its own flag (plus it's not really a flag many
> > people should be using).  Since, really, the wrong use is confined to
> > the defining file, how about this:
> 
> Hmm, with this patch I'm still running into the problems I described
> here:
> 
> http://article.gmane.org/gmane.linux.scsi/47029
> 
> basically, I/Os returned with a DID_RESET status are being requeued in
> an incomplete state:
> 
> 	[ 2362.068342] scsi(5:0:13): OVERRUN status detected 0x7-0x400
> 	[ 2362.068345] CDB: 0x2a 0x0 0x0 0x0 0xe 0x70
> 	[ 2362.068347] PID=0x81eeb req=0x0 xtra=0x1c00 -- returning DID_ERROR status!
> 	[ 2362.068411] scsi(5:0:13): OVERRUN status detected 0x7-0x400
> 	[ 2362.068413] CDB: 0x2a 0x0 0x0 0x0 0xe 0x70
> 	[ 2362.068415] PID=0x81eec req=0x0 xtra=0x1c00 -- returning DID_ERROR status!
> 
> :(
> 
> Thoughts?

Hmm, brown paper bag for me on the review, I think.

The problem is that the buffers are released before the requeue; this is
wrong.  The fix might be to release them later.  Does this work?

James

---

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index cc613ba..0de4834 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -962,7 +962,6 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
 	}
 
 	BUG_ON(blk_bidi_rq(req)); /* bidi not support for !blk_pc_request yet */
-	scsi_release_buffers(cmd);
 
 	/*
 	 * Next deal with any sectors which we were able to correctly
@@ -976,8 +975,10 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
 	 * are leftovers and there is some kind of error
 	 * (result != 0), retry the rest.
 	 */
-	if (scsi_end_request(cmd, error, good_bytes, result == 0) == NULL)
+	if (scsi_end_request(cmd, error, good_bytes, result == 0) == NULL) {
+		scsi_release_buffers(cmd);
 		return;
+	}
 	this_count = blk_rq_bytes(req);
 
 	error = -EIO;
@@ -1080,6 +1081,7 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
 	switch (action) {
 	case ACTION_FAIL:
 		/* Give up and fail the remainder of the request */
+		scsi_release_buffers(cmd);
 		if (!(req->cmd_flags & REQ_QUIET)) {
 			if (description)
 				scmd_printk(KERN_INFO, cmd, "%s\n",
@@ -1095,6 +1097,7 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
 		/* Unprep the request and put it back at the head of the queue.
 		 * A new command will be prepared and issued.
 		 */
+		scsi_release_buffers(cmd);
 		scsi_requeue_command(q, cmd);
 		break;
 	case ACTION_RETRY:

Re: [PATCH] scsi_lib: don't decrement busy counters when inserting commands

[Andrew Vasquez]

On Mon, 05 Jan 2009, James Bottomley wrote:

> Hmm, brown paper bag for me on the review, I think.
> 
> The problem is that the buffers are released before the requeue; this is
> wrong.  The fix might be to release them later.  Does this work?

Hmm, no...

<snip>
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index cc613ba..0de4834 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -962,7 +962,6 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
>  	}
>  
>  	BUG_ON(blk_bidi_rq(req)); /* bidi not support for !blk_pc_request yet */
> -	scsi_release_buffers(cmd);
>  
>  	/*
>  	 * Next deal with any sectors which we were able to correctly
> @@ -976,8 +975,10 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
>  	 * are leftovers and there is some kind of error
>  	 * (result != 0), retry the rest.
>  	 */
> -	if (scsi_end_request(cmd, error, good_bytes, result == 0) == NULL)
> +	if (scsi_end_request(cmd, error, good_bytes, result == 0) == NULL) {
> +		scsi_release_buffers(cmd);
>  		return;
> +	}

This hunk is causing a panic during early SCSI init-time of the server
boot disk -- sg_free_table+0x51 (RIP), scsi_release_buffers+0xd4,
scsi_io_completion+0x324.  IAC, in looking through the code
scsi_end_request() returns NULL when the command has already been
requeued (via scsi_requeue_command()).  Modifying your original patch
to release-buffers prior to scsi_end_request()'s call to
scsi_requeue_command() and dropping the post-scsi_end_request() call
to scsi_release_buffers() fixes *both* the panic as well as the original
problem I reported regarding commands returned with a DID_RESET status
being requeued in an incomplete state.

Here's the updated patch.  Any other holes I missed?

--
av

---

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index cc613ba..9b626af 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -749,6 +749,7 @@ static struct scsi_cmnd *scsi_end_request(struct scsi_cmnd *cmd, int error,
 				 * leftovers in the front of the
 				 * queue, and goose the queue again.
 				 */
+				scsi_release_buffers(cmd);
 				scsi_requeue_command(q, cmd);
 				cmd = NULL;
 			}
@@ -962,7 +963,6 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
 	}
 
 	BUG_ON(blk_bidi_rq(req)); /* bidi not support for !blk_pc_request yet */
-	scsi_release_buffers(cmd);
 
 	/*
 	 * Next deal with any sectors which we were able to correctly
@@ -1080,6 +1080,7 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
 	switch (action) {
 	case ACTION_FAIL:
 		/* Give up and fail the remainder of the request */
+		scsi_release_buffers(cmd);
 		if (!(req->cmd_flags & REQ_QUIET)) {
 			if (description)
 				scmd_printk(KERN_INFO, cmd, "%s\n",
@@ -1095,6 +1096,7 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
 		/* Unprep the request and put it back at the head of the queue.
 		 * A new command will be prepared and issued.
 		 */
+		scsi_release_buffers(cmd);
 		scsi_requeue_command(q, cmd);
 		break;
 	case ACTION_RETRY:

Re: [PATCH] scsi_lib: don't decrement busy counters when inserting commands

[James Bottomley]

On Mon, 2009-01-05 at 23:01 -0800, Andrew Vasquez wrote:
> On Mon, 05 Jan 2009, James Bottomley wrote:
> 
> > Hmm, brown paper bag for me on the review, I think.
> > 
> > The problem is that the buffers are released before the requeue; this is
> > wrong.  The fix might be to release them later.  Does this work?
> 
> Hmm, no...
> 
> <snip>
> > diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> > index cc613ba..0de4834 100644
> > --- a/drivers/scsi/scsi_lib.c
> > +++ b/drivers/scsi/scsi_lib.c
> > @@ -962,7 +962,6 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
> >  	}
> >  
> >  	BUG_ON(blk_bidi_rq(req)); /* bidi not support for !blk_pc_request yet */
> > -	scsi_release_buffers(cmd);
> >  
> >  	/*
> >  	 * Next deal with any sectors which we were able to correctly
> > @@ -976,8 +975,10 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
> >  	 * are leftovers and there is some kind of error
> >  	 * (result != 0), retry the rest.
> >  	 */
> > -	if (scsi_end_request(cmd, error, good_bytes, result == 0) == NULL)
> > +	if (scsi_end_request(cmd, error, good_bytes, result == 0) == NULL) {
> > +		scsi_release_buffers(cmd);
> >  		return;
> > +	}
> 
> This hunk is causing a panic during early SCSI init-time of the server
> boot disk -- sg_free_table+0x51 (RIP), scsi_release_buffers+0xd4,
> scsi_io_completion+0x324.  IAC, in looking through the code
> scsi_end_request() returns NULL when the command has already been
> requeued (via scsi_requeue_command()).  Modifying your original patch
> to release-buffers prior to scsi_end_request()'s call to
> scsi_requeue_command() and dropping the post-scsi_end_request() call
> to scsi_release_buffers() fixes *both* the panic as well as the original
> problem I reported regarding commands returned with a DID_RESET status
> being requeued in an incomplete state.

Gosh this is a subtle nasty mess.

> Here's the updated patch.  Any other holes I missed?

Yes, I'm afraid ... if scsi_release_buffers() is moved into
scsi_end_request() then it has to be called for every NULL returning
path otherwise we leak buffers.  The missing release is the NULL return
at the bottom of the function.  However, you can't call
scsi_release_buffers() there because we've lost the request and the
scsi_bidi_cmd() check tries to touch it.

Hopefully this version has all the holes plugged, if you could give it a
spin...

James

---

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index cc613ba..940dc32 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -701,6 +701,8 @@ void scsi_run_host_queues(struct Scsi_Host *shost)
 		scsi_run_queue(sdev->request_queue);
 }
 
+static void __scsi_release_buffers(struct scsi_cmnd *, int);
+
 /*
  * Function:    scsi_end_request()
  *
@@ -749,6 +751,7 @@ static struct scsi_cmnd *scsi_end_request(struct scsi_cmnd *cmd, int error,
 				 * leftovers in the front of the
 				 * queue, and goose the queue again.
 				 */
+				scsi_release_buffers(cmd);
 				scsi_requeue_command(q, cmd);
 				cmd = NULL;
 			}
@@ -760,6 +763,7 @@ static struct scsi_cmnd *scsi_end_request(struct scsi_cmnd *cmd, int error,
 	 * This will goose the queue request function at the end, so we don't
 	 * need to worry about launching another command.
 	 */
+	__scsi_release_buffers(cmd, 0);
 	scsi_next_command(cmd);
 	return NULL;
 }
@@ -815,6 +819,26 @@ static void scsi_free_sgtable(struct scsi_data_buffer *sdb)
 	__sg_free_table(&sdb->table, SCSI_MAX_SG_SEGMENTS, scsi_sg_free);
 }
 
+static void __scsi_release_buffers(struct scsi_cmnd *cmd, int do_bidi_check)
+{
+
+	if (cmd->sdb.table.nents)
+		scsi_free_sgtable(&cmd->sdb);
+
+	memset(&cmd->sdb, 0, sizeof(cmd->sdb));
+
+	if (do_bidi_check && scsi_bidi_cmnd(cmd)) {
+		struct scsi_data_buffer *bidi_sdb =
+			cmd->request->next_rq->special;
+		scsi_free_sgtable(bidi_sdb);
+		kmem_cache_free(scsi_sdb_cache, bidi_sdb);
+		cmd->request->next_rq->special = NULL;
+	}
+
+	if (scsi_prot_sg_count(cmd))
+		scsi_free_sgtable(cmd->prot_sdb);
+}
+
 /*
  * Function:    scsi_release_buffers()
  *
@@ -834,21 +858,7 @@ static void scsi_free_sgtable(struct scsi_data_buffer *sdb)
  */
 void scsi_release_buffers(struct scsi_cmnd *cmd)
 {
-	if (cmd->sdb.table.nents)
-		scsi_free_sgtable(&cmd->sdb);
-
-	memset(&cmd->sdb, 0, sizeof(cmd->sdb));
-
-	if (scsi_bidi_cmnd(cmd)) {
-		struct scsi_data_buffer *bidi_sdb =
-			cmd->request->next_rq->special;
-		scsi_free_sgtable(bidi_sdb);
-		kmem_cache_free(scsi_sdb_cache, bidi_sdb);
-		cmd->request->next_rq->special = NULL;
-	}
-
-	if (scsi_prot_sg_count(cmd))
-		scsi_free_sgtable(cmd->prot_sdb);
+	__scsi_release_buffers(cmd, 1);
 }
 EXPORT_SYMBOL(scsi_release_buffers);
 
@@ -962,7 +972,6 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
 	}
 
 	BUG_ON(blk_bidi_rq(req)); /* bidi not support for !blk_pc_request yet */
-	scsi_release_buffers(cmd);
 
 	/*
 	 * Next deal with any sectors which we were able to correctly
@@ -1080,6 +1089,7 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
 	switch (action) {
 	case ACTION_FAIL:
 		/* Give up and fail the remainder of the request */
+		scsi_release_buffers(cmd);
 		if (!(req->cmd_flags & REQ_QUIET)) {
 			if (description)
 				scmd_printk(KERN_INFO, cmd, "%s\n",
@@ -1095,6 +1105,7 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
 		/* Unprep the request and put it back at the head of the queue.
 		 * A new command will be prepared and issued.
 		 */
+		scsi_release_buffers(cmd);
 		scsi_requeue_command(q, cmd);
 		break;
 	case ACTION_RETRY:

Re: [PATCH] scsi_lib: don't decrement busy counters when inserting commands

[Andrew Vasquez]

On Tue, 06 Jan 2009, James Bottomley wrote:

> On Mon, 2009-01-05 at 23:01 -0800, Andrew Vasquez wrote:
> > This hunk is causing a panic during early SCSI init-time of the server
> > boot disk -- sg_free_table+0x51 (RIP), scsi_release_buffers+0xd4,
> > scsi_io_completion+0x324.  IAC, in looking through the code
> > scsi_end_request() returns NULL when the command has already been
> > requeued (via scsi_requeue_command()).  Modifying your original patch
> > to release-buffers prior to scsi_end_request()'s call to
> > scsi_requeue_command() and dropping the post-scsi_end_request() call
> > to scsi_release_buffers() fixes *both* the panic as well as the original
> > problem I reported regarding commands returned with a DID_RESET status
> > being requeued in an incomplete state.
> 
> Gosh this is a subtle nasty mess.

:O

> > Here's the updated patch.  Any other holes I missed?
> 
> Yes, I'm afraid ... if scsi_release_buffers() is moved into
> scsi_end_request() then it has to be called for every NULL returning
> path otherwise we leak buffers.  The missing release is the NULL return
> at the bottom of the function.  However, you can't call
> scsi_release_buffers() there because we've lost the request and the
> scsi_bidi_cmd() check tries to touch it.
> 
> Hopefully this version has all the holes plugged, if you could give it a
> spin...

Ok... The results look promising -- no panics and I/Os are resuming
properly after being returned with DID_RESET statuses.  Thanks for
working through this.

--
av