| Message ID | 20220929025407.119804-1-michael.christie@oracle.com |
|---|---|
| Headers | show |
| Series | Allow scsi_execute users to control retries | expand |
Looks good:
Reviewed-by: Christoph Hellwig <hch@lst.de>
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
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.
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.
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
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>
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.
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.
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.