Re: RFC: Retrying SCSI pass-through commands

[Bart Van Assche <bvanassche@acm.org>]

On 8/1/24 5:47 PM, Damien Le Moal wrote:
> On 8/2/24 05:12, Bart Van Assche wrote:
>> [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.

In the SG_IO ioctl implementation I found the following assignment:

	scmd->allowed = 0;

In drivers/scsi/sg.c I found the following (SG_DEFAULT_RETRIES == 0):

	scmd->allowed = SG_DEFAULT_RETRIES;

Isn't that sufficient to prevent retries in the scsi_noretry_cmd() callers that
check scmd->allowed? The only scsi_noretry_cmd() that does not check
scmd->allowed is scsi_io_completion(). I propose to add an explicit
blk_rq_is_passthrough() check in that function next to the scsi_noretry_cmd()
call.

Thanks,

Bart.