[PATCH] Eliminate error handler overload of the SCSI serial number

[PATCH] Eliminate error handler overload of the SCSI serial number

[James Bottomley]

The error handler is using the test cmd->serial_number == 0 in the
abort routines to signal that the command to be aborted has already
completed normally.  This design was to close a race window in the
original error handler where a command could go through the normal
completion routines after it timed out but before error handling was
started.

Mike Anderson pointed out that when we converted our timeout and
softirq completions, we picked up atomicity here because the block
layer now mediates this with the REQ_ATOM_COMPLETE flag and guarantees
that *either* the command times out or our done routine is called, but
ensures we can't get both occurring.  That makes the serial number
zero check redundant and it can be removed.

Signed-off-by: James Bottomley <James.Bottomley@suse.de>
---
 drivers/scsi/scsi_error.c |   26 ++------------------------
 drivers/scsi/scsi_lib.c   |    5 -----
 2 files changed, 2 insertions(+), 29 deletions(-)

diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index 824b8fc..30ac116 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -615,7 +615,7 @@ static int scsi_try_bus_device_reset(struct scsi_cmnd *scmd)
 	return rtn;
 }
 
-static int __scsi_try_to_abort_cmd(struct scsi_cmnd *scmd)
+static int scsi_try_to_abort_cmd(struct scsi_cmnd *scmd)
 {
 	if (!scmd->device->host->hostt->eh_abort_handler)
 		return FAILED;
@@ -623,31 +623,9 @@ static int __scsi_try_to_abort_cmd(struct scsi_cmnd *scmd)
 	return scmd->device->host->hostt->eh_abort_handler(scmd);
 }
 
-/**
- * scsi_try_to_abort_cmd - Ask host to abort a running command.
- * @scmd:	SCSI cmd to abort from Lower Level.
- *
- * Notes:
- *    This function will not return until the user's completion function
- *    has been called.  there is no timeout on this operation.  if the
- *    author of the low-level driver wishes this operation to be timed,
- *    they can provide this facility themselves.  helper functions in
- *    scsi_error.c can be supplied to make this easier to do.
- */
-static int scsi_try_to_abort_cmd(struct scsi_cmnd *scmd)
-{
-	/*
-	 * scsi_done was called just after the command timed out and before
-	 * we had a chance to process it. (db)
-	 */
-	if (scmd->serial_number == 0)
-		return SUCCESS;
-	return __scsi_try_to_abort_cmd(scmd);
-}
-
 static void scsi_abort_eh_cmnd(struct scsi_cmnd *scmd)
 {
-	if (__scsi_try_to_abort_cmd(scmd) != SUCCESS)
+	if (scsi_try_to_abort_cmd(scmd) != SUCCESS)
 		if (scsi_try_bus_device_reset(scmd) != SUCCESS)
 			if (scsi_try_target_reset(scmd) != SUCCESS)
 				if (scsi_try_bus_reset(scmd) != SUCCESS)
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index eafeeda..5b6bbae 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -1403,11 +1403,6 @@ static void scsi_softirq_done(struct request *rq)
 
 	INIT_LIST_HEAD(&cmd->eh_entry);
 
-	/*
-	 * Set the serial numbers back to zero
-	 */
-	cmd->serial_number = 0;
-
 	atomic_inc(&cmd->device->iodone_cnt);
 	if (cmd->result)
 		atomic_inc(&cmd->device->ioerr_cnt);
-- 
1.7.2.3

Re: [PATCH] Eliminate error handler overload of the SCSI serial number

[Nicholas A. Bellinger]

On Wed, 2010-11-17 at 10:10 -0600, James Bottomley wrote:
> The error handler is using the test cmd->serial_number == 0 in the
> abort routines to signal that the command to be aborted has already
> completed normally.  This design was to close a race window in the
> original error handler where a command could go through the normal
> completion routines after it timed out but before error handling was
> started.
> 
> Mike Anderson pointed out that when we converted our timeout and
> softirq completions, we picked up atomicity here because the block
> layer now mediates this with the REQ_ATOM_COMPLETE flag and guarantees
> that *either* the command times out or our done routine is called, but
> ensures we can't get both occurring.  That makes the serial number
> zero check redundant and it can be removed.
> 
> Signed-off-by: James Bottomley <James.Bottomley@suse.de>

Looks good to me.

Signed-off-by: Nicholas A. Bellinger <nab@linux-iscsi.org>

And a big thanks to andmike for sorting this legacy bit out..  So with
this patch I will go ahead and drop the following from lio-core-2.6.git:

scsi: Convert scsi_host->cmd_serial_number to odd numbered atomic_t counter

Thanks James and Co!

> ---
>  drivers/scsi/scsi_error.c |   26 ++------------------------
>  drivers/scsi/scsi_lib.c   |    5 -----
>  2 files changed, 2 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
> index 824b8fc..30ac116 100644
> --- a/drivers/scsi/scsi_error.c
> +++ b/drivers/scsi/scsi_error.c
> @@ -615,7 +615,7 @@ static int scsi_try_bus_device_reset(struct scsi_cmnd *scmd)
>  	return rtn;
>  }
>  
> -static int __scsi_try_to_abort_cmd(struct scsi_cmnd *scmd)
> +static int scsi_try_to_abort_cmd(struct scsi_cmnd *scmd)
>  {
>  	if (!scmd->device->host->hostt->eh_abort_handler)
>  		return FAILED;
> @@ -623,31 +623,9 @@ static int __scsi_try_to_abort_cmd(struct scsi_cmnd *scmd)
>  	return scmd->device->host->hostt->eh_abort_handler(scmd);
>  }
>  
> -/**
> - * scsi_try_to_abort_cmd - Ask host to abort a running command.
> - * @scmd:	SCSI cmd to abort from Lower Level.
> - *
> - * Notes:
> - *    This function will not return until the user's completion function
> - *    has been called.  there is no timeout on this operation.  if the
> - *    author of the low-level driver wishes this operation to be timed,
> - *    they can provide this facility themselves.  helper functions in
> - *    scsi_error.c can be supplied to make this easier to do.
> - */
> -static int scsi_try_to_abort_cmd(struct scsi_cmnd *scmd)
> -{
> -	/*
> -	 * scsi_done was called just after the command timed out and before
> -	 * we had a chance to process it. (db)
> -	 */
> -	if (scmd->serial_number == 0)
> -		return SUCCESS;
> -	return __scsi_try_to_abort_cmd(scmd);
> -}
> -
>  static void scsi_abort_eh_cmnd(struct scsi_cmnd *scmd)
>  {
> -	if (__scsi_try_to_abort_cmd(scmd) != SUCCESS)
> +	if (scsi_try_to_abort_cmd(scmd) != SUCCESS)
>  		if (scsi_try_bus_device_reset(scmd) != SUCCESS)
>  			if (scsi_try_target_reset(scmd) != SUCCESS)
>  				if (scsi_try_bus_reset(scmd) != SUCCESS)
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index eafeeda..5b6bbae 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -1403,11 +1403,6 @@ static void scsi_softirq_done(struct request *rq)
>  
>  	INIT_LIST_HEAD(&cmd->eh_entry);
>  
> -	/*
> -	 * Set the serial numbers back to zero
> -	 */
> -	cmd->serial_number = 0;
> -
>  	atomic_inc(&cmd->device->iodone_cnt);
>  	if (cmd->result)
>  		atomic_inc(&cmd->device->ioerr_cnt);

Re: [PATCH] Eliminate error handler overload of the SCSI serial number

[Mike Christie]

On 11/17/2010 10:10 AM, James Bottomley wrote:
> -static int scsi_try_to_abort_cmd(struct scsi_cmnd *scmd)
> -{
> -	/*
> -	 * scsi_done was called just after the command timed out and before
> -	 * we had a chance to process it. (db)
> -	 */
> -	if (scmd->serial_number == 0)
> -		return SUCCESS;
> -	return __scsi_try_to_abort_cmd(scmd);
> -}

Does it matter that with this patch we now call the driver's abort 
handler when the command has been added to the eh list when going 
through scsi_softirq_done-> scsi_decide_disposition returns FAILED -> 
scsi_eh_scmd_add?

It looks like most drivers will not send an abort, because they check 
something like the scsi_cmnd SCp pointer for their internal info and 
when it is not there they assume the command completed and just return 
(do some return FAILED and some return SUCCESS through). I did not check 
all drivers.

It seems like this is a really rare case that we get FAILED from 
scsi_decide_disposition so I thought it might not matter.

Re: [PATCH] Eliminate error handler overload of the SCSI serial number

[Mike Christie]

On 11/18/2010 01:36 PM, Mike Christie wrote:
> On 11/17/2010 10:10 AM, James Bottomley wrote:
>> -static int scsi_try_to_abort_cmd(struct scsi_cmnd *scmd)
>> -{
>> - /*
>> - * scsi_done was called just after the command timed out and before
>> - * we had a chance to process it. (db)
>> - */
>> - if (scmd->serial_number == 0)
>> - return SUCCESS;
>> - return __scsi_try_to_abort_cmd(scmd);
>> -}
>
> Does it matter that with this patch we now call the driver's abort
> handler when the command has been added to the eh list when going
> through scsi_softirq_done-> scsi_decide_disposition returns FAILED ->
> scsi_eh_scmd_add?
>

Ignore this. I see scsi_eh_scmd_add passes the 0 flag, so the abort 
handler is not called.