mbox series

[v2,00/35] Allow scsi_execute users to control retries

Message ID 20220929025407.119804-1-michael.christie@oracle.com
Headers show
Series Allow scsi_execute users to control retries | expand

Message

Mike Christie Sept. 29, 2022, 2:53 a.m. UTC 
The following patches made over a combo of linus's tree and Martin's
6.1-queue tree (they are both missing patches so I couldn't build
against just one) allow scsi_execute* users to control exactly which
errors are retried, so we can reduce the sense/sshdr handling they have 
to do.

The patches allow scsi_execute* users to pass in an array of failures
which they want retried and also specify how many times they want them
retried. If we hit an error that the user did not specify then we drop
down to the default behavior. This allows us to remove almost all the
retry logic from scsi_execute* users.

We then only need to drive retries from the caller for:

1. wants to sleep between retries or had strict timings like in
sd_spinup_disk or ufs.
2. needed to set some internal state between retries like in
scsi_test_unit_ready)
3. retried based on the error code and it's internal state like in the
alua rtpg handling.

v2:
- Rename scsi_prep_sense
- Change scsi_check_passthrough's loop and added some fixes
- Modified scsi_execute* so it uses a struct to pass in args

Comments

Christoph Hellwig Sept. 29, 2022, 7:39 a.m. UTC | #1 
Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>
Martin Wilck Sept. 29, 2022, 2:12 p.m. UTC | #2 
On Wed, 2022-09-28 at 21:53 -0500, Mike Christie wrote:
> This has read_capacity_16 have scsi-ml retry errors instead of
> driving
> them itself.
> 
> Signed-off-by: Mike Christie <michael.christie@oracle.com>
> ---
>  drivers/scsi/sd.c | 82 +++++++++++++++++++++++++--------------------
> --
>  1 file changed, 43 insertions(+), 39 deletions(-)
> 
> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index 37eafa968116..a35c089c3097 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -2283,55 +2283,59 @@ static int read_capacity_16(struct scsi_disk
> *sdkp, struct scsi_device *sdp,
>         struct scsi_sense_hdr sshdr;
>         int sense_valid = 0;
>         int the_result;
> -       int retries = 3, reset_retries =
> READ_CAPACITY_RETRIES_ON_RESET;
>         unsigned int alignment;
>         unsigned long long lba;
>         unsigned sector_size;
> +       struct scsi_failure failures[] = {
> +               {
> +                       .sense = UNIT_ATTENTION,
> +                       .asc = 0x29,
> +                       .ascq = 0,
> +                       /* Device reset might occur several times */
> +                       .allowed = READ_CAPACITY_RETRIES_ON_RESET,
> +                       .result = SAM_STAT_CHECK_CONDITION,
> +               },
> +               {
> +                       .result = SCMD_FAILURE_ANY,
> +                       .allowed = 3,
> +               },
> +               {},
> +       };

I first wondered whether this was correct, until I realized that
the logic in patch 02/35 actually treats the counts for different
failures independently, so that the maximum overall retry count is
the sum of the individual retry counts.

I wonder if we should give callers the chance to set a limit for the
overall retry count in addition to the retry counts for individual 
failures.

Martin
Mike Christie Sept. 29, 2022, 4:39 p.m. UTC | #3 
On 9/29/22 6:00 AM, Martin Wilck wrote:
>> +               if (!failure->result)
>> +                       break;
>> +
>> +               if (failure->result == SCMD_FAILURE_ANY)
>> +                       goto maybe_retry;
>> +
>> +               if (host_byte(scmd->result) & host_byte(failure-
>>> result)) {
>> +                       goto maybe_retry;
> 
> Using "&" here needs explanation. The host byte is not a bit mask.
> 
>> +               } else if (get_status_byte(scmd) &
>> +                          __get_status_byte(failure->result)) {
> 
> See above.
> 
I had a brain far for host/status bytes. Will fix throughout the set.
Bart Van Assche Sept. 29, 2022, 5:02 p.m. UTC | #4 
On 9/28/22 19:53, Mike Christie wrote:
> +/* Make sure any sense buffer is the correct size. */
> +#define scsi_exec_req(_args)						\
> +({									\
> +	BUILD_BUG_ON(_args.sense &&					\
> +		     _args.sense_len != SCSI_SENSE_BUFFERSIZE);		\
> +	__scsi_exec_req(&_args);					\
> +})

Hi Mike,

I will take a close look at the entire patch series when I have the time.
So far I only have one question: if _args would be surrounded with
parentheses in the above macro, would that allow to leave out one pair of
parentheses from the code that uses this macro? Would that allow to write
scsi_exec_req((struct scsi_exec_args){...}) instead of
scsi_exec_req(((struct scsi_exec_args){...}))?

Thanks,

Bart.
Mike Christie Sept. 29, 2022, 8:24 p.m. UTC | #5 
On 9/29/22 12:02 PM, Bart Van Assche wrote:
> On 9/28/22 19:53, Mike Christie wrote:
>> +/* Make sure any sense buffer is the correct size. */
>> +#define scsi_exec_req(_args)                        \
>> +({                                    \
>> +    BUILD_BUG_ON(_args.sense &&                    \
>> +             _args.sense_len != SCSI_SENSE_BUFFERSIZE);        \
>> +    __scsi_exec_req(&_args);                    \
>> +})
> 
> Hi Mike,
> 
> I will take a close look at the entire patch series when I have the time.
> So far I only have one question: if _args would be surrounded with
> parentheses in the above macro, would that allow to leave out one pair of
> parentheses from the code that uses this macro? Would that allow to write
> scsi_exec_req((struct scsi_exec_args){...}) instead of
> scsi_exec_req(((struct scsi_exec_args){...}))?
> 

You mean like:

#define scsi_exec_req(_args)                                            \
({                                                                      \
        BUILD_BUG_ON((_args).sense &&                                   \
                     (_args).sense_len != SCSI_SENSE_BUFFERSIZE);       \
        __scsi_exec_req(&(_args));                                      \
})

right? That didn't help. You still get the error:

error: macro "scsi_exec_req" passed 8 arguments, but takes just 1
Bart Van Assche Sept. 30, 2022, 12:05 a.m. UTC | #6 
On 9/28/22 19:53, Mike Christie wrote:
> This breaks out the sense prep so it can be used in helper that will be
> added in this patchset for passthrough commands.

Reviewed-by: Bart Van Assche <bvanassche@acm.org>
Bart Van Assche Sept. 30, 2022, 12:12 a.m. UTC | #7 
On 9/28/22 19:53, Mike Christie wrote:
> +int __scsi_exec_req(struct scsi_exec_args *args)

Has it been considered to change the argument list into "const struct
scsi_exec_args *args"?

> -#define scsi_execute(sdev, cmd, data_direction, buffer, bufflen, sense,	\
> -		     sshdr, timeout, retries, flags, rq_flags, resid)	\
> +#define scsi_execute(_sdev, _cmd, _data_dir, _buffer, _bufflen, _sense,	\
> +		     _sshdr, _timeout, _retries, _flags, _rq_flags,	\
> +		     _resid)						\

I don't think that the added underscores are necessary. Has it been
considered not to change the argument names?

Thanks,

Bart.
Mike Christie Sept. 30, 2022, 2:43 a.m. UTC | #8 
On 9/29/22 7:12 PM, Bart Van Assche wrote:
> On 9/28/22 19:53, Mike Christie wrote:
>> +int __scsi_exec_req(struct scsi_exec_args *args)
> 
> Has it been considered to change the argument list into "const struct
> scsi_exec_args *args"?

Yeah I meant to ask you about this. We do end up updating resid, sense
buf, and sshdr, but because those are pointers I can make it
"const struct scsi_exec_args *args" and it's fine since we are not
updating fields like buf_len.

I was thinking you wanted fields like cmd const though. So do you want

1. "const struct scsi_exec_args *args"

plus

2. pointers on that struct that we don't modify like cmd and sdev also
const.

?

> 
>> -#define scsi_execute(sdev, cmd, data_direction, buffer, bufflen, sense,    \
>> -             sshdr, timeout, retries, flags, rq_flags, resid)    \
>> +#define scsi_execute(_sdev, _cmd, _data_dir, _buffer, _bufflen, _sense,    \
>> +             _sshdr, _timeout, _retries, _flags, _rq_flags,    \
>> +             _resid)                        \
> 
> I don't think that the added underscores are necessary. Has it been
> considered not to change the argument names?

I can do that.
Bart Van Assche Sept. 30, 2022, 5:29 p.m. UTC | #9 
On 9/29/22 19:43, Mike Christie wrote:
> On 9/29/22 7:12 PM, Bart Van Assche wrote:
>> On 9/28/22 19:53, Mike Christie wrote:
>>> +int __scsi_exec_req(struct scsi_exec_args *args)
>>
>> Has it been considered to change the argument list into "const struct
>> scsi_exec_args *args"?
> 
> Yeah I meant to ask you about this. We do end up updating resid, sense
> buf, and sshdr, but because those are pointers I can make it
> "const struct scsi_exec_args *args" and it's fine since we are not
> updating fields like buf_len.
> 
> I was thinking you wanted fields like cmd const though. So do you want
> 
> 1. "const struct scsi_exec_args *args"
> 
> plus
> 
> 2. pointers on that struct that we don't modify like cmd and sdev also
> const.
> 
> ?
  Hi Mike,

I care more about (1) than about (2). The following code:

__scsi_exec_req(&((struct scsi_exec_args){...}));

creates a temporary struct on the stack and passes the address of that 
temporary to __scsi_exec_req(). Changing the argument type of 
__scsi_exec_req() from struct scsi_exec_args *args into const struct 
scsi_exec_args *args ensures that __scsi_exec_req() cannot modify *args 
if 'args' points at a temporary data structure on the stack.

Thanks,

Bart.