Re: [RFC] Prevent infinite retries due to DID_RESET return status

[Christoph Hellwig <hch@infradead.org>]

On Mon, Dec 11, 2006 at 03:42:34PM -0600, Michael Reed wrote:
> Due to a firmware mismatch between a host and target (names withheld to
> protect the innocent?), the LLDD was returning DID_RESET for every
> i/o command.  This patch modifies the scsi layer to take into account
> when the command which received DID_RESET was issued and eventually
> give up on it instead of unconditionally reissuing it forever
> when it receives a DID_RESET.  With this patch, on my test system,
> the command receiving the constant DID_RESET times out after about
> 360 seconds.
> 
> The premise for this patch is that no command should have an infinite
> lifetime.  The impetus for this patch was a system which would not
> reach a command prompt without disconnecting the storage from the
> host.
> 
> The significant change in this patch is to call scsi_retry_command()
> instead of scsi_requeue_command() if the command which receives a
> DID_RESET did not complete any i/o (good_bytes==0).  scsi_retry_command()
> does not release the command and regenerate it like scsi_requeue_command()
> does, hence jiffies_at_alloc reflects when the command was first issued.

Generally this patch looks good to me.  Some comments:


> -extern int scsi_retry_command(struct scsi_cmnd *cmd);
> +extern int scsi_retry_command(struct scsi_cmnd *cmd, int reason);

	I've just sent a patch to kill scsi_retry_command.  Use
	scsi_queue_insert directly instead.

> +	scsi_release_buffers(cmd);

	Can you please separate out the moving of the scsi_release_buffer
	calls into a separate patch so it can be audited better?

> @@ -961,9 +992,20 @@ void scsi_io_completion(struct scsi_cmnd
>  		/* Third party bus reset or reset for error recovery
>  		 * reasons.  Just retry the request and see what
>  		 * happens.
> +		 * If no data was transferred, just reissue this
> +		 * command.  If data was transferred, regenerate
> +		 * the command to transfer only untransferred data.
>  		 */

The whole comment should look more like:

		/*
		 * Third party bus reset or reset for error recovery reasons.
		 * If no data was transferred, just reissue this command.
		 * If data was transferred, regenerate the command to transfer
		 * only untransferred data.
		 */

> -		scsi_requeue_command(q, cmd);
> -		return;
> +		if (!good_bytes) {
> +			if (!(scsi_command_expired(cmd))) {
> +				scsi_retry_command(cmd, SCSI_MLQUEUE_DID_RESET);
> +				return;
> +			}
> +		}
> +		else {
> +			scsi_requeue_command(q, cmd);
> +			return;
> +		}

	With this code we now fallthrough if we don't have any good bytes
	and the command has expired.  Is this the expected behaviour? If
	yes we need a good comment describing it.