Re: RFC: Retrying SCSI pass-through commands

[Damien Le Moal <dlemoal@kernel.org>]

On 8/2/24 05:12, Bart Van Assche wrote:
> On 7/31/24 8:33 PM, Damien Le Moal wrote:
>> Looking at the code, e.g. sd_start_stop_device():
>>
>> 	res = scsi_execute_cmd(sdp, cmd, REQ_OP_DRV_IN, NULL, 0, SD_TIMEOUT,
>>                                 sdkp->max_retries, &exec_args);
>>
>> It seems that it is expected that the retry count will be honored. But that
>> indeed is not the case as scsi_noretry_cmd() will always return false for
>> REQ_OP_DRV_* commands.
>>
>> So may be we should have a RQF_USER_OP_DRV flag to differentiate user
>> REQ_OP_DRV_* passthrough commands from internally issued REQ_OP_DRV_* commands.
>> Or the reverse flag, e.g. RQF_INTERNAL_OP_DRV, that we can set in e.g.
>> scsi_execute_cmnd().
> 
> Hmm ... how about using the simplest possible patch? The patch below seems
> to work fine.
> 
> Thanks,
> 
> Bart.
> 
> [PATCH] scsi: core: Retry commands submitted by the SCSI core
> 
> Pass-through commands either come from user space (SG_IO ioctl,
> SCSI_IOCTL_*, BSG), from the SCSI core or from a SCSI driver (ch,
> cxlflash, pktcdvd, scsi_scan, scsi_dh_*, scsi_transport_spi, sd,
> sg, st, virtio_scsi). All this code sets scsi_cmnd.allowed to the
> maximum number of allowed retries. Hence, removing the
> blk_rq_is_passthrough() check from scsi_noretry_cmd() has the
> effect that scsi_cmnd.allowed is respected for pass-through
> commands.
> 
> diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
> index 612489afe8d2..c09f65cfa898 100644
> --- a/drivers/scsi/scsi_error.c
> +++ b/drivers/scsi/scsi_error.c
> @@ -1855,7 +1855,7 @@ bool scsi_noretry_cmd(struct scsi_cmnd *scmd)
>   	 * assume caller has checked sense and determined
>   	 * the check condition was retryable.
>   	 */
> -	if (req->cmd_flags & REQ_FAILFAST_DEV || blk_rq_is_passthrough(req))
> +	if (req->cmd_flags & REQ_FAILFAST_DEV)
>   		return true;

But that will cause *all* passthrough requests to be retried, including those
issued from userspace, no ? I do not think that such change would be acceptable
as that can break userspace using passthrough and expecting to deal with errors
and handling them however they see fit, which may not be retrying commands.

> 
>   	return false;
> 

-- 
Damien Le Moal
Western Digital Research