* 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