Re: [PATCH v2 03/18] scsi: core: allow libata to complete successful commands via EH

[Hannes Reinecke <hare@suse.de>]

On 1/12/23 15:03, Niklas Cassel wrote:
> In SCSI, we get the sense data as part of the completion, for ATA
> however, we need to fetch the sense data as an extra step. For an
> aborted ATA command the sense data is fetched via libata's
> ->eh_strategy_handler().
> 
> For Command Duration Limits policy 0xD:
> The device shall complete the command without error with the additional
> sense code set to DATA CURRENTLY UNAVAILABLE.
> 
> In order to handle this policy in libata, we intend to send a successful
> command via SCSI EH, and let libata's ->eh_strategy_handler() fetch the
> sense data for the good command. This is similar to how we handle an
> aborted ATA command, just that we need to read the Successful NCQ
> Commands log instead of the NCQ Command Error log.
> 
> When we get a SATA completion with successful commands, ATA_SENSE will
> be set, indicating that some commands in the completion have sense data.
> 
> The sense_valid bitmask in the Sense Data for Successful NCQ Commands
> log will inform exactly which commands that had sense data, which might
> be a subset of all the commands that was completed in the same
> completion. (Yet all will have ATA_SENSE set, since the status is per
> completion.)
> 
> The successful commands that have e.g. a "DATA CURRENTLY UNAVAILABLE"
> sense data will have a SCSI ML byte set, so scsi_eh_flush_done_q() will
> not set the scmd->result to DID_TIME_OUT for these commands. However,
> the successful commands that did not have sense data, must not get their
> result marked as DID_TIME_OUT by SCSI EH.
> 
> Add a new flag SCMD_EH_SUCCESS_CMD, which tells SCSI EH to not mark a
> command as DID_TIME_OUT, even if it has scmd->result == SAM_STAT_GOOD.
> 
> This will be used by libata in a follow-up patch.
> 
> Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
> ---
>   drivers/scsi/scsi_error.c | 3 ++-
>   include/scsi/scsi_cmnd.h  | 5 +++++
>   2 files changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
> index 2aa2c2aee6e7..51aa5c1e31b5 100644
> --- a/drivers/scsi/scsi_error.c
> +++ b/drivers/scsi/scsi_error.c
> @@ -2165,7 +2165,8 @@ void scsi_eh_flush_done_q(struct list_head *done_q)
>   			 * scsi_eh_get_sense), scmd->result is already
>   			 * set, do not set DID_TIME_OUT.
>   			 */
> -			if (!scmd->result)
> +			if (!scmd->result &&
> +			    !(scmd->flags & SCMD_EH_SUCCESS_CMD))
>   				scmd->result |= (DID_TIME_OUT << 16);
>   			SCSI_LOG_ERROR_RECOVERY(3,
>   				scmd_printk(KERN_INFO, scmd,
Wouldn't it be better to use '!scmd->result && !scsi_sense_valid(scmd)' 
instead of a new flag?
After all, if we have a valid sense code we _have_ been able to 
communicate with the device. And as we did so it's questionable whether 
it should count as a command time out ...

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Ivo Totev, Andrew
Myers, Andrew McDonald, Martje Boudien Moerman