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:
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
