Re: [PATCH v11 07/34] scsi: sd: Have scsi-ml retry read_capacity_16 errors

[Mike Christie <michael.christie@oracle.com>]

On 9/19/23 4:07 AM, Martin Wilck wrote:
> On Mon, 2023-09-18 at 13:45 -0500, Mike Christie wrote:
>> On 9/18/23 11:48 AM, Martin Wilck wrote:
>>> On Sun, 2023-09-17 at 19:35 -0500, Mike Christie wrote:
>>>> On 9/15/23 4:34 PM, Martin Wilck wrote:
>>>>> On Fri, 2023-09-15 at 22:21 +0200, Martin Wilck wrote:
>>>>>> On Tue, 2023-09-05 at 18:15 -0500, Mike Christie wrote:
>>>>>>> This has read_capacity_16 have scsi-ml retry errors instead
>>>>>>> of
>>>>>>> driving
>>>>>>> them itself.
>>>>>>>
>>>>>>> There are 2 behavior changes with this patch:
>>>>>>> 1. There is one behavior change where we no longer retry
>>>>>>> when
>>>>>>> scsi_execute_cmd returns < 0, but we should be ok. We don't
>>>>>>> need to
>>>>>>> retry
>>>>>>> for failures like the queue being removed, and for the case
>>>>>>> where
>>>>>>> there
>>>>>>> are no tags/reqs since the block layer waits/retries for
>>>>>>> us.
>>>>>>> For
>>>>>>> possible
>>>>>>> memory allocation failures from blk_rq_map_kern we use
>>>>>>> GFP_NOIO, so
>>>>>>> retrying will probably not help.
>>>>>>> 2. For the specific UAs we checked for and retried, we
>>>>>>> would
>>>>>>> get
>>>>>>> READ_CAPACITY_RETRIES_ON_RESET retries plus whatever
>>>>>>> retries
>>>>>>> were
>>>>>>> left
>>>>>>> from the loop's retries. Each UA now gets
>>>>>>> READ_CAPACITY_RETRIES_ON_RESET
>>>>>>> reties, and the other errors (not including medium not
>>>>>>> present)
>>>>>>> get
>>>>>>> up
>>>>>>> to 3 retries.
>>>>>>
>>>>>> This is ok, but - just a thought - would it make sense to add
>>>>>> a
>>>>>> field
>>>>>> for maximum total retry count (summed over all failures)?
>>>>>> That
>>>>>> would
>>>>>> allow us to minimize behavior changes also in other cases.
>>>>>
>>>>> Could we perhaps use scmd->allowed for this purpose?
>>>>>
>>>>> I noted that several callers of scsi_execute_cmd() in your
>>>>> patch
>>>>> set
>>>>> set the allowed parameter, e.g. to sdkp->max_retries in 07/34.
>>>>> But allowed doesn't seem to be used at all in the passthrough
>>>>> case,
>>>>
>>>> I think scmd->allowed is still used for errors that don't hit the
>>>> check_type goto in scsi_no_retry_cmd.
>>>>
>>>> If the user sets up scsi failures for only UAs, and we got a
>>>> DID_TRANSPORT_DISRUPTED then we we do scsi_cmd_retry_allowed
>>>> which
>>>> checks scmd->allowed.
>>>>
>>>> In scsi_noretry_cmd we then hit the:
>>>>
>>>>         if (!scsi_status_is_check_condition(scmd->result))
>>>>                 return false;
>>>>
>>>> and will retry.
>>>
>>> Right. But that's pretty confusing. Actually, I am confused about
>>> some
>>> other things as well. 
>>>
>>> I apologize for taking a step back and asking some more questions
>>> and
>>> presenting some more thoughts about your patch set as a whole.
>>>
>>> For passthrough commands, the simplified logic is now:
>>>
>>> scsi_decide_disposition() 
>>> {
>>>          if (!scsi_device_online)
>>>                 return SUCCESS;
>>>          if ((rtn = scsi_check_passthrough(scmd)) !=
>>> SCSI_RETURN_NOT_HANDLED)
>>>                 return rtn;
>>>
>>>          /* handle host byte for regular commands, 
>>>             may return SUCESS, NEEDS_RETRY/ADD_TO_MLQUEUE, 
>>>             FAILED, fall through, or jump to maybe_retry */
>>>
>>>          /* handle status byte for regular commands, likewise */
>>>           
>>>  maybe_retry: /* [2] */
>>>          if (scsi_cmd_retry_allowed(scmd) &&
>>> !scsi_noretry_cmd(scmd))
>>>                 return NEEDS_RETRY;
>>>          else
>>>                 return SUCCESS;
>>> }
>>>
>>> where scsi_noretry_cmd() has special treatment for passthrough
>>> commands
>>> in certain cases (DID_TIME_OUT and CHECK_CONDITION).
>>>
>>> Because you put the call to scsi_check_passthrough() before the
>>> standard checks, some retries that the mid layer used to do will
>>> now
>>> not be done if scsi_check_passthrough() returns SUCCESS. Also,
>>
>> Yeah, I did that on purpose to give scsi_execute_cmd more control
>> over
>> whether to retry or not. I think you are looking at this more like
>> we want to be able to retry when scsi-ml decided not to.
> 
> I am not saying that giving the failure checks more control is wrong; I
> lack experience to judge that. I was just pointing out a change in
> behavior that wasn't obvious to me from the outset. IOW, I'd appreciate
> a clear separation between the  formal change that moves failure
> treatment for passthrough commands into the mid layer, and the semantic
> changes regarding the ordering and precedence of the various cases in
> scsi_decide_disposition().

Yeah, agree now. The email is a mess because I wrote that below. I
should have written that here to save you time :)


> 
>> For example, I was thinking multipath related code like the scsi_dh
>> handlers
>> would want to fail for cases scsi-ml was currently retrying. Right
>> now they
>> are setting REQ_FAILFAST_TRANSPORT but for most drivers that's not
>> doing what
>> they think it does. Only DID_BUS_BUSY fast fails and I think the
>> scsi_dh
>> failover code wants other errors like DID_TRANSPORT_DISRUPTED to fail
>> so the
>> caller can decide based on something like the dm-multipath pg
>> retries.
> 
> This makes sense for device handlers, but not so much for other
> callers. Do we need to discuss a separate approach for commands sent by
> device handlers?
> 
> Current device handlers have been written with the standard ML retry
> handling in mind. The device handler changes in your patch set apply to
> special CHECK CONDITION situations. These checks are executed before
> the host byte checks in scsi_decide_disposition(). It's not obvious
> from the code that none of these conditions can trigger if the host
> byte is set [1], which would seem wrong. Perhaps we need separate
> scsi_check_passthrough() handlers for host byte and status byte?

I think we are talking about different things. I can do what you want
first then go for the behavior change later. We can discuss that later.
The second/other patchset will be smaller so it will be easier to
discuss.


Snip the chunk below because I agree with you.

And for the total retry issue, since I'm going to try and keep the
existing behavior as much as possible, I'll add code for that. It
will be easier to discuss code then.