* [PATCHSET 0/3] little bit of love to scsi_io_completion
@ 2010-01-04 12:11 Boaz Harrosh
2010-01-04 12:13 ` [PATCH 1/3] scsi_lib: request_queue is only needed inside scsi_requeue_command Boaz Harrosh
` (2 more replies)
0 siblings, 3 replies; 16+ messages in thread
From: Boaz Harrosh @ 2010-01-04 12:11 UTC (permalink / raw
To: James Bottomley, Alan Stern, Mike Christie, Hannes Reinecke,
linux-scsi
Three small incremental patches to clean up scsi_io_completion and friends.
There should be absolutely no functional change after this patchset, only
readability improvements.
This patchset should open up the stage for the fixes Alan needs and farther
enhancements.
(Mike, Hannes, I know you have pending work here, please point me to patches and
I'll rebase them for you)
List of patches
[PATCH 1/3] scsi_lib: request_queue is only needed inside scsi_requeue_command
[PATCH 2/3] scsi_lib: Remove that __scsi_release_buffers contraption
[PATCH 3/3] scsi_lib: Collapse scsi_end_request into only user
Are on top of scsi-misc Please consider for inclusion
Boaz
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 1/3] scsi_lib: request_queue is only needed inside scsi_requeue_command
2010-01-04 12:11 [PATCHSET 0/3] little bit of love to scsi_io_completion Boaz Harrosh
@ 2010-01-04 12:13 ` Boaz Harrosh
2010-01-04 12:15 ` [PATCH 2/3] scsi_lib: Remove that __scsi_release_buffers contraption Boaz Harrosh
2010-01-04 12:17 ` [PATCH 3/3] scsi_lib: Collapse scsi_end_request into only user Boaz Harrosh
2 siblings, 0 replies; 16+ messages in thread
From: Boaz Harrosh @ 2010-01-04 12:13 UTC (permalink / raw
To: James Bottomley, Alan Stern, Mike Christie, Hannes Reinecke,
linux-scsi
This is a pure cleanup. Just starting the engines for things
to come.
Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
---
drivers/scsi/scsi_lib.c | 11 +++++------
1 files changed, 5 insertions(+), 6 deletions(-)
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index c664242..05f988a 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -475,9 +475,10 @@ static void scsi_run_queue(struct request_queue *q)
* sector.
* Notes: Upon return, cmd is a stale pointer.
*/
-static void scsi_requeue_command(struct request_queue *q, struct scsi_cmnd *cmd)
+static void scsi_requeue_command(struct scsi_cmnd *cmd)
{
struct request *req = cmd->request;
+ struct request_queue *q = req->q;
unsigned long flags;
spin_lock_irqsave(q->queue_lock, flags);
@@ -538,7 +539,6 @@ static void __scsi_release_buffers(struct scsi_cmnd *, int);
static struct scsi_cmnd *scsi_end_request(struct scsi_cmnd *cmd, int error,
int bytes, int requeue)
{
- struct request_queue *q = cmd->device->request_queue;
struct request *req = cmd->request;
/*
@@ -557,7 +557,7 @@ static struct scsi_cmnd *scsi_end_request(struct scsi_cmnd *cmd, int error,
* queue, and goose the queue again.
*/
scsi_release_buffers(cmd);
- scsi_requeue_command(q, cmd);
+ scsi_requeue_command(cmd);
cmd = NULL;
}
return cmd;
@@ -706,7 +706,6 @@ EXPORT_SYMBOL(scsi_release_buffers);
void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
{
int result = cmd->result;
- struct request_queue *q = cmd->device->request_queue;
struct request *req = cmd->request;
int error = 0;
struct scsi_sense_hdr sshdr;
@@ -900,7 +899,7 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
scsi_print_command(cmd);
}
if (blk_end_request_err(req, error))
- scsi_requeue_command(q, cmd);
+ scsi_requeue_command(cmd);
else
scsi_next_command(cmd);
break;
@@ -909,7 +908,7 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
* A new command will be prepared and issued.
*/
scsi_release_buffers(cmd);
- scsi_requeue_command(q, cmd);
+ scsi_requeue_command(cmd);
break;
case ACTION_RETRY:
/* Retry the same command immediately */
--
1.6.6
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH 2/3] scsi_lib: Remove that __scsi_release_buffers contraption
2010-01-04 12:11 [PATCHSET 0/3] little bit of love to scsi_io_completion Boaz Harrosh
2010-01-04 12:13 ` [PATCH 1/3] scsi_lib: request_queue is only needed inside scsi_requeue_command Boaz Harrosh
@ 2010-01-04 12:15 ` Boaz Harrosh
2010-01-04 12:17 ` [PATCH 3/3] scsi_lib: Collapse scsi_end_request into only user Boaz Harrosh
2 siblings, 0 replies; 16+ messages in thread
From: Boaz Harrosh @ 2010-01-04 12:15 UTC (permalink / raw
To: James Bottomley, Alan Stern, Mike Christie, Hannes Reinecke,
linux-scsi
Remove "do_bidi_check" by checking and nullifying cmd->request
when blk_end_* indicates the request is no longer valid.
Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
---
drivers/scsi/scsi_lib.c | 41 +++++++++++++++++------------------------
1 files changed, 17 insertions(+), 24 deletions(-)
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 05f988a..8d8b4eb 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -512,8 +512,6 @@ 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()
*
@@ -564,11 +562,12 @@ static struct scsi_cmnd *scsi_end_request(struct scsi_cmnd *cmd, int error,
}
}
+ cmd->request = NULL;
/*
* 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_release_buffers(cmd);
scsi_next_command(cmd);
return NULL;
}
@@ -624,26 +623,6 @@ 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()
*
@@ -663,7 +642,21 @@ static void __scsi_release_buffers(struct scsi_cmnd *cmd, int do_bidi_check)
*/
void scsi_release_buffers(struct scsi_cmnd *cmd)
{
- __scsi_release_buffers(cmd, 1);
+ if (cmd->sdb.table.nents)
+ scsi_free_sgtable(&cmd->sdb);
+
+ memset(&cmd->sdb, 0, sizeof(cmd->sdb));
+
+ if (cmd->request && 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);
}
EXPORT_SYMBOL(scsi_release_buffers);
--
1.6.6
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH 3/3] scsi_lib: Collapse scsi_end_request into only user
2010-01-04 12:11 [PATCHSET 0/3] little bit of love to scsi_io_completion Boaz Harrosh
2010-01-04 12:13 ` [PATCH 1/3] scsi_lib: request_queue is only needed inside scsi_requeue_command Boaz Harrosh
2010-01-04 12:15 ` [PATCH 2/3] scsi_lib: Remove that __scsi_release_buffers contraption Boaz Harrosh
@ 2010-01-04 12:17 ` Boaz Harrosh
2010-01-04 13:23 ` Matthew Wilcox
` (2 more replies)
2 siblings, 3 replies; 16+ messages in thread
From: Boaz Harrosh @ 2010-01-04 12:17 UTC (permalink / raw
To: James Bottomley, Alan Stern, Mike Christie, Hannes Reinecke,
linux-scsi
Embedding scsi_end_request() into scsi_io_completion actually simplifies the
code and makes it clearer what's going on.
There is absolutely no functional and/or side effects changes after this patch.
Patch was inspired by Alan Stern.
CC: Alan Stern <stern@rowland.harvard.edu>
Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
---
drivers/scsi/scsi_lib.c | 90 +++++++++++-----------------------------------
1 files changed, 22 insertions(+), 68 deletions(-)
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 8d8b4eb..326b228 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -512,66 +512,6 @@ void scsi_run_host_queues(struct Scsi_Host *shost)
scsi_run_queue(sdev->request_queue);
}
-/*
- * Function: scsi_end_request()
- *
- * Purpose: Post-processing of completed commands (usually invoked at end
- * of upper level post-processing and scsi_io_completion).
- *
- * Arguments: cmd - command that is complete.
- * error - 0 if I/O indicates success, < 0 for I/O error.
- * bytes - number of bytes of completed I/O
- * requeue - indicates whether we should requeue leftovers.
- *
- * Lock status: Assumed that lock is not held upon entry.
- *
- * Returns: cmd if requeue required, NULL otherwise.
- *
- * Notes: This is called for block device requests in order to
- * mark some number of sectors as complete.
- *
- * We are guaranteeing that the request queue will be goosed
- * at some point during this call.
- * Notes: If cmd was requeued, upon return it will be a stale pointer.
- */
-static struct scsi_cmnd *scsi_end_request(struct scsi_cmnd *cmd, int error,
- int bytes, int requeue)
-{
- struct request *req = cmd->request;
-
- /*
- * If there are blocks left over at the end, set up the command
- * to queue the remainder of them.
- */
- if (blk_end_request(req, error, bytes)) {
- /* kill remainder if no retrys */
- if (error && scsi_noretry_cmd(cmd))
- blk_end_request_all(req, error);
- else {
- if (requeue) {
- /*
- * Bleah. Leftovers again. Stick the
- * leftovers in the front of the
- * queue, and goose the queue again.
- */
- scsi_release_buffers(cmd);
- scsi_requeue_command(cmd);
- cmd = NULL;
- }
- return cmd;
- }
- }
-
- cmd->request = NULL;
- /*
- * 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);
- scsi_next_command(cmd);
- return NULL;
-}
-
static inline unsigned int scsi_sgtable_index(unsigned short nents)
{
unsigned int index;
@@ -704,7 +644,7 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
struct scsi_sense_hdr sshdr;
int sense_valid = 0;
int sense_deferred = 0;
- enum {ACTION_FAIL, ACTION_REPREP, ACTION_RETRY,
+ enum {ACTION_FAIL, ACTION_REPREP, ACTION_NEXT_CMND, ACTION_RETRY,
ACTION_DELAYED_RETRY} action;
char *description = NULL;
@@ -773,13 +713,22 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
error = 0;
}
- /*
- * A number of bytes were successfully read. If there
- * 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)
- return;
+ if (!blk_end_request(req, error, good_bytes)) {
+ cmd->request = NULL;
+ action = ACTION_NEXT_CMND;
+ goto do_action;
+ }
+
+ if (error && scsi_noretry_cmd(cmd)) {
+ /* kill remainder if no retrys */
+ blk_end_request_all(req, error);
+ cmd->request = NULL;
+ action = ACTION_NEXT_CMND;
+ goto do_action;
+ } else if (result == 0) {
+ action = ACTION_REPREP;
+ goto do_action;
+ }
error = -EIO;
@@ -878,6 +827,7 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
action = ACTION_FAIL;
}
+do_action:
switch (action) {
case ACTION_FAIL:
/* Give up and fail the remainder of the request */
@@ -896,6 +846,10 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
else
scsi_next_command(cmd);
break;
+ case ACTION_NEXT_CMND:
+ scsi_release_buffers(cmd);
+ scsi_next_command(cmd);
+ break;
case ACTION_REPREP:
/* Unprep the request and put it back at the head of the queue.
* A new command will be prepared and issued.
--
1.6.6
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH 3/3] scsi_lib: Collapse scsi_end_request into only user
2010-01-04 12:17 ` [PATCH 3/3] scsi_lib: Collapse scsi_end_request into only user Boaz Harrosh
@ 2010-01-04 13:23 ` Matthew Wilcox
2010-01-04 13:56 ` Boaz Harrosh
2010-01-04 14:07 ` [PATCH 3/3 version 2] " Boaz Harrosh
2010-01-05 9:07 ` [PATCH 3/3 version 4] " Boaz Harrosh
2 siblings, 1 reply; 16+ messages in thread
From: Matthew Wilcox @ 2010-01-04 13:23 UTC (permalink / raw
To: Boaz Harrosh
Cc: James Bottomley, Alan Stern, Mike Christie, Hannes Reinecke,
linux-scsi
On Mon, Jan 04, 2010 at 02:17:06PM +0200, Boaz Harrosh wrote:
> Embedding scsi_end_request() into scsi_io_completion actually simplifies the
> code and makes it clearer what's going on.
I'm not entirely convinced about that -- scsi_io_completion is currently
over 200 lines long and needs to be made shorter, not longer. That said,
I see no reason that the current factoring of scsi_io_completion() makes
sense; pushing the decoding of the sense key into a separate function
looks like a more profitable idea. It would also let you do without
the nasty gotos you add in this patch.
> There is absolutely no functional and/or side effects changes after this patch.
>
> Patch was inspired by Alan Stern.
>
> CC: Alan Stern <stern@rowland.harvard.edu>
> Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
> ---
> drivers/scsi/scsi_lib.c | 90 +++++++++++-----------------------------------
> 1 files changed, 22 insertions(+), 68 deletions(-)
>
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index 8d8b4eb..326b228 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -512,66 +512,6 @@ void scsi_run_host_queues(struct Scsi_Host *shost)
> scsi_run_queue(sdev->request_queue);
> }
>
> -/*
> - * Function: scsi_end_request()
> - *
> - * Purpose: Post-processing of completed commands (usually invoked at end
> - * of upper level post-processing and scsi_io_completion).
> - *
> - * Arguments: cmd - command that is complete.
> - * error - 0 if I/O indicates success, < 0 for I/O error.
> - * bytes - number of bytes of completed I/O
> - * requeue - indicates whether we should requeue leftovers.
> - *
> - * Lock status: Assumed that lock is not held upon entry.
> - *
> - * Returns: cmd if requeue required, NULL otherwise.
> - *
> - * Notes: This is called for block device requests in order to
> - * mark some number of sectors as complete.
> - *
> - * We are guaranteeing that the request queue will be goosed
> - * at some point during this call.
> - * Notes: If cmd was requeued, upon return it will be a stale pointer.
> - */
> -static struct scsi_cmnd *scsi_end_request(struct scsi_cmnd *cmd, int error,
> - int bytes, int requeue)
> -{
> - struct request *req = cmd->request;
> -
> - /*
> - * If there are blocks left over at the end, set up the command
> - * to queue the remainder of them.
> - */
> - if (blk_end_request(req, error, bytes)) {
> - /* kill remainder if no retrys */
> - if (error && scsi_noretry_cmd(cmd))
> - blk_end_request_all(req, error);
> - else {
> - if (requeue) {
> - /*
> - * Bleah. Leftovers again. Stick the
> - * leftovers in the front of the
> - * queue, and goose the queue again.
> - */
> - scsi_release_buffers(cmd);
> - scsi_requeue_command(cmd);
> - cmd = NULL;
> - }
> - return cmd;
> - }
> - }
> -
> - cmd->request = NULL;
> - /*
> - * 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);
> - scsi_next_command(cmd);
> - return NULL;
> -}
> -
> static inline unsigned int scsi_sgtable_index(unsigned short nents)
> {
> unsigned int index;
> @@ -704,7 +644,7 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
> struct scsi_sense_hdr sshdr;
> int sense_valid = 0;
> int sense_deferred = 0;
> - enum {ACTION_FAIL, ACTION_REPREP, ACTION_RETRY,
> + enum {ACTION_FAIL, ACTION_REPREP, ACTION_NEXT_CMND, ACTION_RETRY,
> ACTION_DELAYED_RETRY} action;
> char *description = NULL;
>
> @@ -773,13 +713,22 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
> error = 0;
> }
>
> - /*
> - * A number of bytes were successfully read. If there
> - * 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)
> - return;
> + if (!blk_end_request(req, error, good_bytes)) {
> + cmd->request = NULL;
> + action = ACTION_NEXT_CMND;
> + goto do_action;
> + }
> +
> + if (error && scsi_noretry_cmd(cmd)) {
> + /* kill remainder if no retrys */
> + blk_end_request_all(req, error);
> + cmd->request = NULL;
> + action = ACTION_NEXT_CMND;
> + goto do_action;
> + } else if (result == 0) {
> + action = ACTION_REPREP;
> + goto do_action;
> + }
>
> error = -EIO;
>
> @@ -878,6 +827,7 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
> action = ACTION_FAIL;
> }
>
> +do_action:
> switch (action) {
> case ACTION_FAIL:
> /* Give up and fail the remainder of the request */
> @@ -896,6 +846,10 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
> else
> scsi_next_command(cmd);
> break;
> + case ACTION_NEXT_CMND:
> + scsi_release_buffers(cmd);
> + scsi_next_command(cmd);
> + break;
> case ACTION_REPREP:
> /* Unprep the request and put it back at the head of the queue.
> * A new command will be prepared and issued.
> --
> 1.6.6
>
>
> --
> 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
--
Matthew Wilcox Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours. We can't possibly take such
a retrograde step."
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 3/3] scsi_lib: Collapse scsi_end_request into only user
2010-01-04 13:23 ` Matthew Wilcox
@ 2010-01-04 13:56 ` Boaz Harrosh
0 siblings, 0 replies; 16+ messages in thread
From: Boaz Harrosh @ 2010-01-04 13:56 UTC (permalink / raw
To: Matthew Wilcox
Cc: James Bottomley, Alan Stern, Mike Christie, Hannes Reinecke,
linux-scsi
On 01/04/2010 03:23 PM, Matthew Wilcox wrote:
> On Mon, Jan 04, 2010 at 02:17:06PM +0200, Boaz Harrosh wrote:
>> Embedding scsi_end_request() into scsi_io_completion actually simplifies the
>> code and makes it clearer what's going on.
>
> I'm not entirely convinced about that -- scsi_io_completion is currently
> over 200 lines long and needs to be made shorter, not longer. That said,
> I see no reason that the current factoring of scsi_io_completion() makes
> sense; pushing the decoding of the sense key into a separate function
> looks like a more profitable idea. It would also let you do without
> the nasty gotos you add in this patch.
>
I agree, but I want that Alan's retries handling, Hannes's sense for FS_COMMANDS
and mainly the question about good_bytes!=0 and retries are answered first.
Then we can see what the code is really like and make a clean cut.
I'll attempt a second version without the goto.
>> There is absolutely no functional and/or side effects changes after this patch.
>>
>> Patch was inspired by Alan Stern.
>>
>> CC: Alan Stern <stern@rowland.harvard.edu>
>> Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
<snip>
Boaz
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 3/3 version 2] scsi_lib: Collapse scsi_end_request into only user
2010-01-04 12:17 ` [PATCH 3/3] scsi_lib: Collapse scsi_end_request into only user Boaz Harrosh
2010-01-04 13:23 ` Matthew Wilcox
@ 2010-01-04 14:07 ` Boaz Harrosh
2010-01-04 14:12 ` Boaz Harrosh
` (2 more replies)
2010-01-05 9:07 ` [PATCH 3/3 version 4] " Boaz Harrosh
2 siblings, 3 replies; 16+ messages in thread
From: Boaz Harrosh @ 2010-01-04 14:07 UTC (permalink / raw
To: James Bottomley, Alan Stern, Mike Christie, Hannes Reinecke,
linux-scsi
Embedding scsi_end_request() into scsi_io_completion actually simplifies the
code and makes it clearer what's going on.
There is absolutely no functional and/or side effects changes after this patch.
Patch was inspired by Alan Stern. (version 2 comments by Matthew Wilcox)
CC: Alan Stern <stern@rowland.harvard.edu>
CC: Matthew Wilcox <matthew@wil.cx>
Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
---
drivers/scsi/scsi_lib.c | 90 +++++++++--------------------------------------
1 files changed, 17 insertions(+), 73 deletions(-)
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 8d8b4eb..48ebc96 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -512,66 +512,6 @@ void scsi_run_host_queues(struct Scsi_Host *shost)
scsi_run_queue(sdev->request_queue);
}
-/*
- * Function: scsi_end_request()
- *
- * Purpose: Post-processing of completed commands (usually invoked at end
- * of upper level post-processing and scsi_io_completion).
- *
- * Arguments: cmd - command that is complete.
- * error - 0 if I/O indicates success, < 0 for I/O error.
- * bytes - number of bytes of completed I/O
- * requeue - indicates whether we should requeue leftovers.
- *
- * Lock status: Assumed that lock is not held upon entry.
- *
- * Returns: cmd if requeue required, NULL otherwise.
- *
- * Notes: This is called for block device requests in order to
- * mark some number of sectors as complete.
- *
- * We are guaranteeing that the request queue will be goosed
- * at some point during this call.
- * Notes: If cmd was requeued, upon return it will be a stale pointer.
- */
-static struct scsi_cmnd *scsi_end_request(struct scsi_cmnd *cmd, int error,
- int bytes, int requeue)
-{
- struct request *req = cmd->request;
-
- /*
- * If there are blocks left over at the end, set up the command
- * to queue the remainder of them.
- */
- if (blk_end_request(req, error, bytes)) {
- /* kill remainder if no retrys */
- if (error && scsi_noretry_cmd(cmd))
- blk_end_request_all(req, error);
- else {
- if (requeue) {
- /*
- * Bleah. Leftovers again. Stick the
- * leftovers in the front of the
- * queue, and goose the queue again.
- */
- scsi_release_buffers(cmd);
- scsi_requeue_command(cmd);
- cmd = NULL;
- }
- return cmd;
- }
- }
-
- cmd->request = NULL;
- /*
- * 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);
- scsi_next_command(cmd);
- return NULL;
-}
-
static inline unsigned int scsi_sgtable_index(unsigned short nents)
{
unsigned int index;
@@ -704,7 +644,7 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
struct scsi_sense_hdr sshdr;
int sense_valid = 0;
int sense_deferred = 0;
- enum {ACTION_FAIL, ACTION_REPREP, ACTION_RETRY,
+ enum {ACTION_FAIL, ACTION_REPREP, ACTION_NEXT_CMND, ACTION_RETRY,
ACTION_DELAYED_RETRY} action;
char *description = NULL;
@@ -773,17 +713,17 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
error = 0;
}
- /*
- * A number of bytes were successfully read. If there
- * 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)
- return;
-
- error = -EIO;
-
- if (host_byte(result) == DID_RESET) {
+ if (likely(!blk_end_request(req, error, good_bytes))) {
+ cmd->request = NULL;
+ action = ACTION_NEXT_CMND;
+ } else if (error && scsi_noretry_cmd(cmd)) {
+ /* kill remainder if no retrys */
+ blk_end_request_all(req, error);
+ cmd->request = NULL;
+ action = ACTION_NEXT_CMND;
+ } else if (result == 0) {
+ action = ACTION_REPREP;
+ } else if (host_byte(result) == DID_RESET) {
/* Third party bus reset or reset for error recovery
* reasons. Just retry the command and see what
* happens.
@@ -891,11 +831,15 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
scsi_print_sense("", cmd);
scsi_print_command(cmd);
}
- if (blk_end_request_err(req, error))
+ if (blk_end_request_err(req, error ? error : -EIO))
scsi_requeue_command(cmd);
else
scsi_next_command(cmd);
break;
+ case ACTION_NEXT_CMND:
+ scsi_release_buffers(cmd);
+ scsi_next_command(cmd);
+ break;
case ACTION_REPREP:
/* Unprep the request and put it back at the head of the queue.
* A new command will be prepared and issued.
--
1.6.6
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH 3/3 version 2] scsi_lib: Collapse scsi_end_request into only user
2010-01-04 14:07 ` [PATCH 3/3 version 2] " Boaz Harrosh
@ 2010-01-04 14:12 ` Boaz Harrosh
2010-01-04 18:23 ` Alan Stern
2010-01-05 7:53 ` [PATCH 3/3 version 3] " Boaz Harrosh
2 siblings, 0 replies; 16+ messages in thread
From: Boaz Harrosh @ 2010-01-04 14:12 UTC (permalink / raw
To: Matthew Wilcox
Cc: James Bottomley, Alan Stern, Mike Christie, Hannes Reinecke,
linux-scsi
On 01/04/2010 04:07 PM, Boaz Harrosh wrote:
>
> Embedding scsi_end_request() into scsi_io_completion actually simplifies the
> code and makes it clearer what's going on.
>
> There is absolutely no functional and/or side effects changes after this patch.
>
> Patch was inspired by Alan Stern. (version 2 comments by Matthew Wilcox)
>
Matthew please check me out here. I want absolutely equivalent code. I think
Alan's mistake was to introduce enhancements together with cleanups.
The issue is with "error = -EIO" but since error is only used at blk_end_request_err
I think this approach is correct and is goto-less.
Thanks for reviewing
Boaz
> CC: Alan Stern <stern@rowland.harvard.edu>
> CC: Matthew Wilcox <matthew@wil.cx>
> Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
> ---
> drivers/scsi/scsi_lib.c | 90 +++++++++--------------------------------------
> 1 files changed, 17 insertions(+), 73 deletions(-)
>
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index 8d8b4eb..48ebc96 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -512,66 +512,6 @@ void scsi_run_host_queues(struct Scsi_Host *shost)
> scsi_run_queue(sdev->request_queue);
> }
>
> -/*
> - * Function: scsi_end_request()
> - *
> - * Purpose: Post-processing of completed commands (usually invoked at end
> - * of upper level post-processing and scsi_io_completion).
> - *
> - * Arguments: cmd - command that is complete.
> - * error - 0 if I/O indicates success, < 0 for I/O error.
> - * bytes - number of bytes of completed I/O
> - * requeue - indicates whether we should requeue leftovers.
> - *
> - * Lock status: Assumed that lock is not held upon entry.
> - *
> - * Returns: cmd if requeue required, NULL otherwise.
> - *
> - * Notes: This is called for block device requests in order to
> - * mark some number of sectors as complete.
> - *
> - * We are guaranteeing that the request queue will be goosed
> - * at some point during this call.
> - * Notes: If cmd was requeued, upon return it will be a stale pointer.
> - */
> -static struct scsi_cmnd *scsi_end_request(struct scsi_cmnd *cmd, int error,
> - int bytes, int requeue)
> -{
> - struct request *req = cmd->request;
> -
> - /*
> - * If there are blocks left over at the end, set up the command
> - * to queue the remainder of them.
> - */
> - if (blk_end_request(req, error, bytes)) {
> - /* kill remainder if no retrys */
> - if (error && scsi_noretry_cmd(cmd))
> - blk_end_request_all(req, error);
> - else {
> - if (requeue) {
> - /*
> - * Bleah. Leftovers again. Stick the
> - * leftovers in the front of the
> - * queue, and goose the queue again.
> - */
> - scsi_release_buffers(cmd);
> - scsi_requeue_command(cmd);
> - cmd = NULL;
> - }
> - return cmd;
> - }
> - }
> -
> - cmd->request = NULL;
> - /*
> - * 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);
> - scsi_next_command(cmd);
> - return NULL;
> -}
> -
> static inline unsigned int scsi_sgtable_index(unsigned short nents)
> {
> unsigned int index;
> @@ -704,7 +644,7 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
> struct scsi_sense_hdr sshdr;
> int sense_valid = 0;
> int sense_deferred = 0;
> - enum {ACTION_FAIL, ACTION_REPREP, ACTION_RETRY,
> + enum {ACTION_FAIL, ACTION_REPREP, ACTION_NEXT_CMND, ACTION_RETRY,
> ACTION_DELAYED_RETRY} action;
> char *description = NULL;
>
> @@ -773,17 +713,17 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
> error = 0;
> }
>
> - /*
> - * A number of bytes were successfully read. If there
> - * 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)
> - return;
> -
> - error = -EIO;
> -
> - if (host_byte(result) == DID_RESET) {
> + if (likely(!blk_end_request(req, error, good_bytes))) {
> + cmd->request = NULL;
> + action = ACTION_NEXT_CMND;
> + } else if (error && scsi_noretry_cmd(cmd)) {
> + /* kill remainder if no retrys */
> + blk_end_request_all(req, error);
> + cmd->request = NULL;
> + action = ACTION_NEXT_CMND;
> + } else if (result == 0) {
> + action = ACTION_REPREP;
> + } else if (host_byte(result) == DID_RESET) {
> /* Third party bus reset or reset for error recovery
> * reasons. Just retry the command and see what
> * happens.
> @@ -891,11 +831,15 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
> scsi_print_sense("", cmd);
> scsi_print_command(cmd);
> }
> - if (blk_end_request_err(req, error))
> + if (blk_end_request_err(req, error ? error : -EIO))
> scsi_requeue_command(cmd);
> else
> scsi_next_command(cmd);
> break;
> + case ACTION_NEXT_CMND:
> + scsi_release_buffers(cmd);
> + scsi_next_command(cmd);
> + break;
> case ACTION_REPREP:
> /* Unprep the request and put it back at the head of the queue.
> * A new command will be prepared and issued.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 3/3 version 2] scsi_lib: Collapse scsi_end_request into only user
2010-01-04 14:07 ` [PATCH 3/3 version 2] " Boaz Harrosh
2010-01-04 14:12 ` Boaz Harrosh
@ 2010-01-04 18:23 ` Alan Stern
2010-01-05 7:27 ` Boaz Harrosh
2010-01-05 7:53 ` [PATCH 3/3 version 3] " Boaz Harrosh
2 siblings, 1 reply; 16+ messages in thread
From: Alan Stern @ 2010-01-04 18:23 UTC (permalink / raw
To: Boaz Harrosh
Cc: James Bottomley, Mike Christie, Hannes Reinecke, linux-scsi,
Matthew Wilcox
On Mon, 4 Jan 2010, Boaz Harrosh wrote:
> Embedding scsi_end_request() into scsi_io_completion actually simplifies the
> code and makes it clearer what's going on.
>
> There is absolutely no functional and/or side effects changes after this patch.
>
> Patch was inspired by Alan Stern. (version 2 comments by Matthew Wilcox)
> - /*
> - * A number of bytes were successfully read. If there
> - * 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)
> - return;
> -
> - error = -EIO;
> -
> - if (host_byte(result) == DID_RESET) {
> + if (likely(!blk_end_request(req, error, good_bytes))) {
> + cmd->request = NULL;
> + action = ACTION_NEXT_CMND;
> + } else if (error && scsi_noretry_cmd(cmd)) {
> + /* kill remainder if no retrys */
> + blk_end_request_all(req, error);
> + cmd->request = NULL;
> + action = ACTION_NEXT_CMND;
> + } else if (result == 0) {
> + action = ACTION_REPREP;
> + } else if (host_byte(result) == DID_RESET) {
A few comments in these new "if" cases would help readers to understand
the logic here.
My personal preference is to reverse the order of the "if (error &&
scsi_noretry_cmd(cmd))" and the "if (result == 0)" sections. It would
make more sense; that way we'd have:
If the request is finished [blk_end_request() == 0]
...
else if the request was successful but has more work to do
(result == 0)
...
else if there was an error and retries are disallowed
...
else ... [other error handling]
Interchanging the order wouldn't make any functional difference,
because the code above this point guarantees that we can never have
result == 0 and error != 0.
Apart from these small issues, it looks perfect.
Alan Stern
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 3/3 version 2] scsi_lib: Collapse scsi_end_request into only user
2010-01-04 18:23 ` Alan Stern
@ 2010-01-05 7:27 ` Boaz Harrosh
0 siblings, 0 replies; 16+ messages in thread
From: Boaz Harrosh @ 2010-01-05 7:27 UTC (permalink / raw
To: Alan Stern
Cc: James Bottomley, Mike Christie, Hannes Reinecke, linux-scsi,
Matthew Wilcox
On 01/04/2010 08:23 PM, Alan Stern wrote:
> On Mon, 4 Jan 2010, Boaz Harrosh wrote:
>
>> Embedding scsi_end_request() into scsi_io_completion actually simplifies the
>> code and makes it clearer what's going on.
>>
>> There is absolutely no functional and/or side effects changes after this patch.
>>
>> Patch was inspired by Alan Stern. (version 2 comments by Matthew Wilcox)
>
>> - /*
>> - * A number of bytes were successfully read. If there
>> - * 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)
>> - return;
>> -
>> - error = -EIO;
>> -
>> - if (host_byte(result) == DID_RESET) {
>> + if (likely(!blk_end_request(req, error, good_bytes))) {
>> + cmd->request = NULL;
>> + action = ACTION_NEXT_CMND;
>> + } else if (error && scsi_noretry_cmd(cmd)) {
>> + /* kill remainder if no retrys */
>> + blk_end_request_all(req, error);
>> + cmd->request = NULL;
>> + action = ACTION_NEXT_CMND;
>> + } else if (result == 0) {
>> + action = ACTION_REPREP;
>> + } else if (host_byte(result) == DID_RESET) {
>
> A few comments in these new "if" cases would help readers to understand
> the logic here.
>
> My personal preference is to reverse the order of the "if (error &&
> scsi_noretry_cmd(cmd))" and the "if (result == 0)" sections. It would
> make more sense; that way we'd have:
>
> If the request is finished [blk_end_request() == 0]
> ...
> else if the request was successful but has more work to do
> (result == 0)
> ...
> else if there was an error and retries are disallowed
> ...
> else ... [other error handling]
>
> Interchanging the order wouldn't make any functional difference,
> because the code above this point guarantees that we can never have
> result == 0 and error != 0.
>
> Apart from these small issues, it looks perfect.
>
> Alan Stern
>
Thanks Alan.
I agree with your comments and will send a new version.
Boaz
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 3/3 version 3] scsi_lib: Collapse scsi_end_request into only user
2010-01-04 14:07 ` [PATCH 3/3 version 2] " Boaz Harrosh
2010-01-04 14:12 ` Boaz Harrosh
2010-01-04 18:23 ` Alan Stern
@ 2010-01-05 7:53 ` Boaz Harrosh
2010-01-05 8:49 ` Boaz Harrosh
2 siblings, 1 reply; 16+ messages in thread
From: Boaz Harrosh @ 2010-01-05 7:53 UTC (permalink / raw
To: James Bottomley, Alan Stern, Mike Christie, Hannes Reinecke,
linux-scsi
Embedding scsi_end_request() into scsi_io_completion actually simplifies
the code and makes it clearer what's going on.
There is absolutely no functional and/or side effects changes after this
patch.
Patch was inspired by Alan Stern.
CC: Alan Stern <stern@rowland.harvard.edu>
CC: Matthew Wilcox <matthew@wil.cx>
Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
---
drivers/scsi/scsi_lib.c | 92 ++++++++++-------------------------------------
1 files changed, 19 insertions(+), 73 deletions(-)
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 8d8b4eb..41df4e8 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -512,66 +512,6 @@ void scsi_run_host_queues(struct Scsi_Host *shost)
scsi_run_queue(sdev->request_queue);
}
-/*
- * Function: scsi_end_request()
- *
- * Purpose: Post-processing of completed commands (usually invoked at end
- * of upper level post-processing and scsi_io_completion).
- *
- * Arguments: cmd - command that is complete.
- * error - 0 if I/O indicates success, < 0 for I/O error.
- * bytes - number of bytes of completed I/O
- * requeue - indicates whether we should requeue leftovers.
- *
- * Lock status: Assumed that lock is not held upon entry.
- *
- * Returns: cmd if requeue required, NULL otherwise.
- *
- * Notes: This is called for block device requests in order to
- * mark some number of sectors as complete.
- *
- * We are guaranteeing that the request queue will be goosed
- * at some point during this call.
- * Notes: If cmd was requeued, upon return it will be a stale pointer.
- */
-static struct scsi_cmnd *scsi_end_request(struct scsi_cmnd *cmd, int error,
- int bytes, int requeue)
-{
- struct request *req = cmd->request;
-
- /*
- * If there are blocks left over at the end, set up the command
- * to queue the remainder of them.
- */
- if (blk_end_request(req, error, bytes)) {
- /* kill remainder if no retrys */
- if (error && scsi_noretry_cmd(cmd))
- blk_end_request_all(req, error);
- else {
- if (requeue) {
- /*
- * Bleah. Leftovers again. Stick the
- * leftovers in the front of the
- * queue, and goose the queue again.
- */
- scsi_release_buffers(cmd);
- scsi_requeue_command(cmd);
- cmd = NULL;
- }
- return cmd;
- }
- }
-
- cmd->request = NULL;
- /*
- * 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);
- scsi_next_command(cmd);
- return NULL;
-}
-
static inline unsigned int scsi_sgtable_index(unsigned short nents)
{
unsigned int index;
@@ -704,7 +644,7 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
struct scsi_sense_hdr sshdr;
int sense_valid = 0;
int sense_deferred = 0;
- enum {ACTION_FAIL, ACTION_REPREP, ACTION_RETRY,
+ enum {ACTION_FAIL, ACTION_REPREP, ACTION_NEXT_CMND, ACTION_RETRY,
ACTION_DELAYED_RETRY} action;
char *description = NULL;
@@ -773,17 +713,19 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
error = 0;
}
- /*
- * A number of bytes were successfully read. If there
- * 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)
- return;
-
- error = -EIO;
-
- if (host_byte(result) == DID_RESET) {
+ if (likely(0 == blk_end_request(req, error, good_bytes))) {
+ /* All is done and good move to next command */
+ cmd->request = NULL;
+ action = ACTION_NEXT_CMND;
+ } else if (result == 0) {
+ /* Wrote some bytes but request was split */
+ action = ACTION_REPREP;
+ } else if (error && scsi_noretry_cmd(cmd)) {
+ /* kill remainder if no retries */
+ blk_end_request_all(req, error);
+ cmd->request = NULL;
+ action = ACTION_NEXT_CMND;
+ } else if (host_byte(result) == DID_RESET) {
/* Third party bus reset or reset for error recovery
* reasons. Just retry the command and see what
* happens.
@@ -891,11 +833,15 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
scsi_print_sense("", cmd);
scsi_print_command(cmd);
}
- if (blk_end_request_err(req, error))
+ if (blk_end_request_err(req, error ? error : -EIO))
scsi_requeue_command(cmd);
else
scsi_next_command(cmd);
break;
+ case ACTION_NEXT_CMND:
+ scsi_release_buffers(cmd);
+ scsi_next_command(cmd);
+ break;
case ACTION_REPREP:
/* Unprep the request and put it back at the head of the queue.
* A new command will be prepared and issued.
--
1.6.6
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH 3/3 version 3] scsi_lib: Collapse scsi_end_request into only user
2010-01-05 7:53 ` [PATCH 3/3 version 3] " Boaz Harrosh
@ 2010-01-05 8:49 ` Boaz Harrosh
0 siblings, 0 replies; 16+ messages in thread
From: Boaz Harrosh @ 2010-01-05 8:49 UTC (permalink / raw
To: James Bottomley, Alan Stern, Mike Christie, Hannes Reinecke,
linux-scsi
On 01/05/2010 09:53 AM, Boaz Harrosh wrote:
>
> Embedding scsi_end_request() into scsi_io_completion actually simplifies
> the code and makes it clearer what's going on.
>
> There is absolutely no functional and/or side effects changes after this
> patch.
>
> Patch was inspired by Alan Stern.
>
> CC: Alan Stern <stern@rowland.harvard.edu>
> CC: Matthew Wilcox <matthew@wil.cx>
> Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
> ---
> drivers/scsi/scsi_lib.c | 92 ++++++++++-------------------------------------
> 1 files changed, 19 insertions(+), 73 deletions(-)
>
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index 8d8b4eb..41df4e8 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -512,66 +512,6 @@ void scsi_run_host_queues(struct Scsi_Host *shost)
> scsi_run_queue(sdev->request_queue);
> }
>
> -/*
> - * Function: scsi_end_request()
> - *
> - * Purpose: Post-processing of completed commands (usually invoked at end
> - * of upper level post-processing and scsi_io_completion).
> - *
> - * Arguments: cmd - command that is complete.
> - * error - 0 if I/O indicates success, < 0 for I/O error.
> - * bytes - number of bytes of completed I/O
> - * requeue - indicates whether we should requeue leftovers.
> - *
> - * Lock status: Assumed that lock is not held upon entry.
> - *
> - * Returns: cmd if requeue required, NULL otherwise.
> - *
> - * Notes: This is called for block device requests in order to
> - * mark some number of sectors as complete.
> - *
> - * We are guaranteeing that the request queue will be goosed
> - * at some point during this call.
> - * Notes: If cmd was requeued, upon return it will be a stale pointer.
> - */
> -static struct scsi_cmnd *scsi_end_request(struct scsi_cmnd *cmd, int error,
> - int bytes, int requeue)
> -{
> - struct request *req = cmd->request;
> -
> - /*
> - * If there are blocks left over at the end, set up the command
> - * to queue the remainder of them.
> - */
> - if (blk_end_request(req, error, bytes)) {
> - /* kill remainder if no retrys */
> - if (error && scsi_noretry_cmd(cmd))
> - blk_end_request_all(req, error);
> - else {
> - if (requeue) {
> - /*
> - * Bleah. Leftovers again. Stick the
> - * leftovers in the front of the
> - * queue, and goose the queue again.
> - */
> - scsi_release_buffers(cmd);
> - scsi_requeue_command(cmd);
> - cmd = NULL;
> - }
> - return cmd;
> - }
> - }
> -
> - cmd->request = NULL;
> - /*
> - * 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);
> - scsi_next_command(cmd);
> - return NULL;
> -}
> -
> static inline unsigned int scsi_sgtable_index(unsigned short nents)
> {
> unsigned int index;
> @@ -704,7 +644,7 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
> struct scsi_sense_hdr sshdr;
> int sense_valid = 0;
> int sense_deferred = 0;
> - enum {ACTION_FAIL, ACTION_REPREP, ACTION_RETRY,
> + enum {ACTION_FAIL, ACTION_REPREP, ACTION_NEXT_CMND, ACTION_RETRY,
> ACTION_DELAYED_RETRY} action;
> char *description = NULL;
>
> @@ -773,17 +713,19 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
> error = 0;
> }
>
> - /*
> - * A number of bytes were successfully read. If there
> - * 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)
> - return;
> -
> - error = -EIO;
> -
> - if (host_byte(result) == DID_RESET) {
> + if (likely(0 == blk_end_request(req, error, good_bytes))) {
> + /* All is done and good move to next command */
> + cmd->request = NULL;
> + action = ACTION_NEXT_CMND;
> + } else if (result == 0) {
> + /* Wrote some bytes but request was split */
> + action = ACTION_REPREP;
> + } else if (error && scsi_noretry_cmd(cmd)) {
> + /* kill remainder if no retries */
> + blk_end_request_all(req, error);
> + cmd->request = NULL;
> + action = ACTION_NEXT_CMND;
> + } else if (host_byte(result) == DID_RESET) {
> /* Third party bus reset or reset for error recovery
> * reasons. Just retry the command and see what
> * happens.
> @@ -891,11 +833,15 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
> scsi_print_sense("", cmd);
> scsi_print_command(cmd);
> }
> - if (blk_end_request_err(req, error))
> + if (blk_end_request_err(req, error ? error : -EIO))
> scsi_requeue_command(cmd);
> else
> scsi_next_command(cmd);
> break;
> + case ACTION_NEXT_CMND:
> + scsi_release_buffers(cmd);
> + scsi_next_command(cmd);
> + break;
In a switch expression, Does the first case have an advantage?
I would think that even if the branch predictor does it's job the cache proximity
to the head of the switch would matter. Is there a likely() for case statements?
In anyway I thinks I'll send a 4th version with this case at the top of the switch
Boaz
> case ACTION_REPREP:
> /* Unprep the request and put it back at the head of the queue.
> * A new command will be prepared and issued.
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 3/3 version 4] scsi_lib: Collapse scsi_end_request into only user
2010-01-04 12:17 ` [PATCH 3/3] scsi_lib: Collapse scsi_end_request into only user Boaz Harrosh
2010-01-04 13:23 ` Matthew Wilcox
2010-01-04 14:07 ` [PATCH 3/3 version 2] " Boaz Harrosh
@ 2010-01-05 9:07 ` Boaz Harrosh
2010-01-05 15:20 ` Alan Stern
2 siblings, 1 reply; 16+ messages in thread
From: Boaz Harrosh @ 2010-01-05 9:07 UTC (permalink / raw
To: James Bottomley, Alan Stern, Mike Christie, Hannes Reinecke,
linux-scsi
Embedding scsi_end_request() into scsi_io_completion actually simplifies
the code and makes it clearer what's going on.
There is absolutely no functional and/or side effects changes after this
patch.
Patch was inspired by Alan Stern.
CC: Alan Stern <stern@rowland.harvard.edu>
CC: Matthew Wilcox <matthew@wil.cx>
Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
---
drivers/scsi/scsi_lib.c | 92 ++++++++++-------------------------------------
1 files changed, 19 insertions(+), 73 deletions(-)
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 8d8b4eb..523453f 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -512,66 +512,6 @@ void scsi_run_host_queues(struct Scsi_Host *shost)
scsi_run_queue(sdev->request_queue);
}
-/*
- * Function: scsi_end_request()
- *
- * Purpose: Post-processing of completed commands (usually invoked at end
- * of upper level post-processing and scsi_io_completion).
- *
- * Arguments: cmd - command that is complete.
- * error - 0 if I/O indicates success, < 0 for I/O error.
- * bytes - number of bytes of completed I/O
- * requeue - indicates whether we should requeue leftovers.
- *
- * Lock status: Assumed that lock is not held upon entry.
- *
- * Returns: cmd if requeue required, NULL otherwise.
- *
- * Notes: This is called for block device requests in order to
- * mark some number of sectors as complete.
- *
- * We are guaranteeing that the request queue will be goosed
- * at some point during this call.
- * Notes: If cmd was requeued, upon return it will be a stale pointer.
- */
-static struct scsi_cmnd *scsi_end_request(struct scsi_cmnd *cmd, int error,
- int bytes, int requeue)
-{
- struct request *req = cmd->request;
-
- /*
- * If there are blocks left over at the end, set up the command
- * to queue the remainder of them.
- */
- if (blk_end_request(req, error, bytes)) {
- /* kill remainder if no retrys */
- if (error && scsi_noretry_cmd(cmd))
- blk_end_request_all(req, error);
- else {
- if (requeue) {
- /*
- * Bleah. Leftovers again. Stick the
- * leftovers in the front of the
- * queue, and goose the queue again.
- */
- scsi_release_buffers(cmd);
- scsi_requeue_command(cmd);
- cmd = NULL;
- }
- return cmd;
- }
- }
-
- cmd->request = NULL;
- /*
- * 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);
- scsi_next_command(cmd);
- return NULL;
-}
-
static inline unsigned int scsi_sgtable_index(unsigned short nents)
{
unsigned int index;
@@ -704,7 +644,7 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
struct scsi_sense_hdr sshdr;
int sense_valid = 0;
int sense_deferred = 0;
- enum {ACTION_FAIL, ACTION_REPREP, ACTION_RETRY,
+ enum {ACTION_NEXT_CMND, ACTION_FAIL, ACTION_REPREP, ACTION_RETRY,
ACTION_DELAYED_RETRY} action;
char *description = NULL;
@@ -773,17 +713,19 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
error = 0;
}
- /*
- * A number of bytes were successfully read. If there
- * 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)
- return;
-
- error = -EIO;
-
- if (host_byte(result) == DID_RESET) {
+ if (likely(0 == blk_end_request(req, error, good_bytes))) {
+ /* All is done and good move to next command */
+ cmd->request = NULL;
+ action = ACTION_NEXT_CMND;
+ } else if (result == 0) {
+ /* Wrote some bytes but request was split */
+ action = ACTION_REPREP;
+ } else if (error && scsi_noretry_cmd(cmd)) {
+ /* kill remainder if no retries */
+ blk_end_request_all(req, error);
+ cmd->request = NULL;
+ action = ACTION_NEXT_CMND;
+ } else if (host_byte(result) == DID_RESET) {
/* Third party bus reset or reset for error recovery
* reasons. Just retry the command and see what
* happens.
@@ -879,6 +821,10 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
}
switch (action) {
+ case ACTION_NEXT_CMND :
+ scsi_release_buffers(cmd);
+ scsi_next_command(cmd);
+ break;
case ACTION_FAIL:
/* Give up and fail the remainder of the request */
scsi_release_buffers(cmd);
@@ -891,7 +837,7 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
scsi_print_sense("", cmd);
scsi_print_command(cmd);
}
- if (blk_end_request_err(req, error))
+ if (blk_end_request_err(req, error ? error : -EIO))
scsi_requeue_command(cmd);
else
scsi_next_command(cmd);
--
1.6.6
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH 3/3 version 4] scsi_lib: Collapse scsi_end_request into only user
2010-01-05 9:07 ` [PATCH 3/3 version 4] " Boaz Harrosh
@ 2010-01-05 15:20 ` Alan Stern
2010-01-05 16:23 ` Boaz Harrosh
0 siblings, 1 reply; 16+ messages in thread
From: Alan Stern @ 2010-01-05 15:20 UTC (permalink / raw
To: Boaz Harrosh; +Cc: James Bottomley, Mike Christie, Hannes Reinecke, linux-scsi
On Tue, 5 Jan 2010, Boaz Harrosh wrote:
> Embedding scsi_end_request() into scsi_io_completion actually simplifies
> the code and makes it clearer what's going on.
>
> There is absolutely no functional and/or side effects changes after this
> patch.
Here are some suggestions for changes to the comments. These are quite
minor and you might not want to bother updating the patch yet again...
> + if (likely(0 == blk_end_request(req, error, good_bytes))) {
> + /* All is done and good move to next command */
/* The command completed successfully; move on to the next. */
> + cmd->request = NULL;
> + action = ACTION_NEXT_CMND;
> + } else if (result == 0) {
> + /* Wrote some bytes but request was split */
/* The command was successful but not all the data was
* transferred; re-prep the command to handle the rest.
*/
> + action = ACTION_REPREP;
> + } else if (error && scsi_noretry_cmd(cmd)) {
> + /* kill remainder if no retries */
/* Error. Kill the request since retries are disallowed. */
> + blk_end_request_all(req, error);
> + cmd->request = NULL;
> + action = ACTION_NEXT_CMND;
> + } else if (host_byte(result) == DID_RESET) {
Alan Stern
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 3/3 version 4] scsi_lib: Collapse scsi_end_request into only user
2010-01-05 15:20 ` Alan Stern
@ 2010-01-05 16:23 ` Boaz Harrosh
2010-01-05 16:33 ` Alan Stern
0 siblings, 1 reply; 16+ messages in thread
From: Boaz Harrosh @ 2010-01-05 16:23 UTC (permalink / raw
To: Alan Stern; +Cc: James Bottomley, Mike Christie, Hannes Reinecke, linux-scsi
On 01/05/2010 05:20 PM, Alan Stern wrote:
> On Tue, 5 Jan 2010, Boaz Harrosh wrote:
>
>> Embedding scsi_end_request() into scsi_io_completion actually simplifies
>> the code and makes it clearer what's going on.
>>
>> There is absolutely no functional and/or side effects changes after this
>> patch.
>
> Here are some suggestions for changes to the comments. These are quite
> minor and you might not want to bother updating the patch yet again...
>
Sure, NP. thanks for checking me out, coming from Hebrew I do need support
in these matters. Will update tomorrow.
Alan can I add your Review-by: this time around?
>> + if (likely(0 == blk_end_request(req, error, good_bytes))) {
>> + /* All is done and good move to next command */
>
> /* The command completed successfully; move on to the next. */
>
>> + cmd->request = NULL;
>> + action = ACTION_NEXT_CMND;
>> + } else if (result == 0) {
>> + /* Wrote some bytes but request was split */
>
> /* The command was successful but not all the data was
> * transferred; re-prep the command to handle the rest.
> */
>
>> + action = ACTION_REPREP;
>> + } else if (error && scsi_noretry_cmd(cmd)) {
>> + /* kill remainder if no retries */
>
> /* Error. Kill the request since retries are disallowed. */
>
>> + blk_end_request_all(req, error);
>> + cmd->request = NULL;
>> + action = ACTION_NEXT_CMND;
>> + } else if (host_byte(result) == DID_RESET) {
>
> Alan Stern
>
Thanks
Boaz
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 3/3 version 4] scsi_lib: Collapse scsi_end_request into only user
2010-01-05 16:23 ` Boaz Harrosh
@ 2010-01-05 16:33 ` Alan Stern
0 siblings, 0 replies; 16+ messages in thread
From: Alan Stern @ 2010-01-05 16:33 UTC (permalink / raw
To: Boaz Harrosh; +Cc: James Bottomley, Mike Christie, Hannes Reinecke, linux-scsi
On Tue, 5 Jan 2010, Boaz Harrosh wrote:
> On 01/05/2010 05:20 PM, Alan Stern wrote:
> > On Tue, 5 Jan 2010, Boaz Harrosh wrote:
> >
> >> Embedding scsi_end_request() into scsi_io_completion actually simplifies
> >> the code and makes it clearer what's going on.
> >>
> >> There is absolutely no functional and/or side effects changes after this
> >> patch.
> >
> > Here are some suggestions for changes to the comments. These are quite
> > minor and you might not want to bother updating the patch yet again...
> >
>
> Sure, NP. thanks for checking me out, coming from Hebrew I do need support
> in these matters. Will update tomorrow.
>
> Alan can I add your Review-by: this time around?
Yes. This is a good step. It will make adding functional changes a
lot easier, and it will help if anyone wants to split up
scsi_io_completion() into several smaller routines.
Alan Stern
^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2010-01-05 16:33 UTC | newest]
Thread overview: 16+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2010-01-04 12:11 [PATCHSET 0/3] little bit of love to scsi_io_completion Boaz Harrosh
2010-01-04 12:13 ` [PATCH 1/3] scsi_lib: request_queue is only needed inside scsi_requeue_command Boaz Harrosh
2010-01-04 12:15 ` [PATCH 2/3] scsi_lib: Remove that __scsi_release_buffers contraption Boaz Harrosh
2010-01-04 12:17 ` [PATCH 3/3] scsi_lib: Collapse scsi_end_request into only user Boaz Harrosh
2010-01-04 13:23 ` Matthew Wilcox
2010-01-04 13:56 ` Boaz Harrosh
2010-01-04 14:07 ` [PATCH 3/3 version 2] " Boaz Harrosh
2010-01-04 14:12 ` Boaz Harrosh
2010-01-04 18:23 ` Alan Stern
2010-01-05 7:27 ` Boaz Harrosh
2010-01-05 7:53 ` [PATCH 3/3 version 3] " Boaz Harrosh
2010-01-05 8:49 ` Boaz Harrosh
2010-01-05 9:07 ` [PATCH 3/3 version 4] " Boaz Harrosh
2010-01-05 15:20 ` Alan Stern
2010-01-05 16:23 ` Boaz Harrosh
2010-01-05 16:33 ` Alan Stern
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).