| Message ID | 20221104231927.9613-4-michael.christie@oracle.com |
|---|---|
| State | New |
| Headers | show |
| Series | Allow scsi_execute users to control retries | expand |
On 04/11/2022 23:18, Mike Christie wrote: > - req_flags_t rq_flags, int *resid) > +int __scsi_exec_req(const struct scsi_exec_args *args) nit: I would expect a req to be passed based on the name, like blk_execute_rq() > { > struct request *req; > struct scsi_cmnd *scmd; > int ret; > > - req = scsi_alloc_request(sdev->request_queue, > - data_direction == DMA_TO_DEVICE ? > - REQ_OP_DRV_OUT : REQ_OP_DRV_IN, > - rq_flags & RQF_PM ? BLK_MQ_REQ_PM : 0); > + req = scsi_alloc_request(args->sdev->request_queue, > + args->data_dir == DMA_TO_DEVICE ? > + REQ_OP_DRV_OUT : REQ_OP_DRV_IN, Did you ever consider just putting the scsi_alloc_request() opf arg in struct scsi_exec_args (rather than data_dir), i.e. have the caller evaluate? We already do it in other callers to scsi_alloc_request(). Current method means a store (in scsi_exec_args struct), a load, a comparison, and a mov value to register whose value depends on comparison. That's most relevant on performance being a concern. Thanks, John
On 11/10/22 03:15, John Garry wrote: > Current method means a store (in scsi_exec_args struct), a load, a > comparison, and a mov value to register whose value depends on > comparison. That's most relevant on performance being a concern. Hi John, Is there any code that calls scsi_execute() from a code path in which performance matters? Thanks, Bart.
On 10/11/2022 17:26, Bart Van Assche wrote: > n 11/10/22 03:15, John Garry wrote: >> Current method means a store (in scsi_exec_args struct), a load, a >> comparison, and a mov value to register whose value depends on >> comparison. That's most relevant on performance being a concern. > > Hi John, > > Is there any code that calls scsi_execute() from a code path in which > performance matters? Eh, I don't know. Does performance matter for the touched ioctls or probe lun code? Anyway, this code I mention is just the same as before. It just bugs me when I see code which accepts a hardcoded value (dma dir, in this case) and translates into another value, when the translated value could be just passed. Thanks, John
On 11/10/22 12:09 PM, John Garry wrote: > On 10/11/2022 17:26, Bart Van Assche wrote: >> n 11/10/22 03:15, John Garry wrote: >>> Current method means a store (in scsi_exec_args struct), a load, a comparison, and a mov value to register whose value depends on comparison. That's most relevant on performance being a concern. >> >> Hi John, >> >> Is there any code that calls scsi_execute() from a code path in which performance matters? > > Eh, I don't know. Does performance matter for the touched ioctls or probe lun code? No.
On 11/10/22 5:15 AM, John Garry wrote: > On 04/11/2022 23:18, Mike Christie wrote: >> - req_flags_t rq_flags, int *resid) >> +int __scsi_exec_req(const struct scsi_exec_args *args) > > nit: I would expect a req to be passed based on the name, like blk_execute_rq() > We have scsi_exeucute_req now which works like scsi_exec_req. I carried it over because it seemed nice that it reflects we are executing a request vs something like a TMF. I don't care either way if people have a preference. >> { >> struct request *req; >> struct scsi_cmnd *scmd; >> int ret; >> - req = scsi_alloc_request(sdev->request_queue, >> - data_direction == DMA_TO_DEVICE ? >> - REQ_OP_DRV_OUT : REQ_OP_DRV_IN, >> - rq_flags & RQF_PM ? BLK_MQ_REQ_PM : 0); >> + req = scsi_alloc_request(args->sdev->request_queue, >> + args->data_dir == DMA_TO_DEVICE ? >> + REQ_OP_DRV_OUT : REQ_OP_DRV_IN, > > Did you ever consider just putting the scsi_alloc_request() opf arg in struct scsi_exec_args (rather than data_dir), i.e. have the caller evaluate? We already do it in other callers to scsi_alloc_request(). I did, but this part of the patches just convert us to the args struct use. I tried to not change the types to keep the patchset down. I'll change the types in another patchset, but I think it's better to do in a separate patchset because I think we can do some more cleanup. The users that use scsi_allocate_request are kind of a mix match mess in that we use the scsi helper to allocate the request and scsi_cmnd, then setup the request directly and then use the blk_execute_rq helper. So I was thinking they use the flags directly since they are using the block layer APIs where the scsi_execute* users are trying to use a SCSI interface where the DMA values are also used (LLDs use DMA flags, ULDs use a mix because they convert between block and SCSI, and scsi-ml uses both as well).
On 11/10/22 12:40 PM, Mike Christie wrote: > On 11/10/22 5:15 AM, John Garry wrote: >> On 04/11/2022 23:18, Mike Christie wrote: >>> - req_flags_t rq_flags, int *resid) >>> +int __scsi_exec_req(const struct scsi_exec_args *args) >> >> nit: I would expect a req to be passed based on the name, like blk_execute_rq() >> > > We have scsi_exeucute_req now which works like scsi_exec_req. I carried it over > because it seemed nice that it reflects we are executing a request vs something > like a TMF. I don't care either way if people have a preference. > > >>> { >>> struct request *req; >>> struct scsi_cmnd *scmd; >>> int ret; >>> - req = scsi_alloc_request(sdev->request_queue, >>> - data_direction == DMA_TO_DEVICE ? >>> - REQ_OP_DRV_OUT : REQ_OP_DRV_IN, >>> - rq_flags & RQF_PM ? BLK_MQ_REQ_PM : 0); >>> + req = scsi_alloc_request(args->sdev->request_queue, >>> + args->data_dir == DMA_TO_DEVICE ? >>> + REQ_OP_DRV_OUT : REQ_OP_DRV_IN, >> >> Did you ever consider just putting the scsi_alloc_request() opf arg in struct scsi_exec_args (rather than data_dir), i.e. have the caller evaluate? We already do it in other callers to scsi_alloc_request(). > > I did, but this part of the patches just convert us to the args struct > use. I tried to not change the types to keep the patchset down. > > I'll change the types in another patchset, Oh wait, I could probably handle your comments in this set if we wanted to. scsi_execute could already take the block layer flags, so we already are doing that type of thing so when I was thinking the scsi_execute interface should be less block layer'y I was wrong. So, I could convert the users to use the REQ_OP_DRV instead of DMA values when I do the args conversion since we normally do just pass it in the directly (vs libata where we do some extra work). One thing that is weird to me is that scsi_execute_req is the more scsi'ish interface. I would have thought since it took the req flag since it has the req naming in it. I don't care either way.
On 11/10/22 11:26, Mike Christie wrote: > One thing that is weird to me is that scsi_execute_req is the more scsi'ish > interface. I would have thought since it took the req flag since it has the > req naming in it. > > I don't care either way. Although this is not a strong argument, it is an argument: passing a DMA direction is slightly more descriptive than passing REQ_OP_DRV_* since the information that no data is passed would be lost if the DMA direction argument would be changed into a REQ_OP_DRV_* argument. Bart.
On 10/11/2022 19:26, Mike Christie wrote: >> I'll change the types in another patchset, > Oh wait, I could probably handle your comments in this set if we > wanted to. hmmm...maybe it's better to be done in another series, as you might get bogged down here with other comments ... > scsi_execute could already take the block layer flags, so we already > are doing that type of thing so when I was thinking the scsi_execute > interface should be less block layer'y I was wrong. So, I could convert > the users to use the REQ_OP_DRV instead of DMA values when I do the args > conversion since we normally do just pass it in the directly (vs libata > where we do some extra work). Further to that, I do wonder why we even need a separate dma_dir flag at all. We already pass a blk_opt_t arg in flags, so why have a separate conduit to set REQ_OP_DRV_{IN/OUT}? A couple of other observations: a. why do we manually set req->cmd_flags from flags instead of passing flags directly to scsi_alloc_request() b. why pass a req_flags_t instead of a blk_mq_req_flags_t - as I see, rq_flags arg is only ever set to RQF_PM or 0, so no need for a conversion. > > One thing that is weird to me is that scsi_execute_req is the more scsi'ish > interface. I would have thought since it took the req flag since it has the > req naming in it. > > I don't care either way. Thanks, John
On Fri, Nov 11, 2022 at 12:07:26PM +0000, John Garry wrote: > We already pass a blk_opt_t arg in flags, so why have a separate conduit to > set REQ_OP_DRV_{IN/OUT}? A couple of other observations: > a. why do we manually set req->cmd_flags from flags instead of passing > flags directly to scsi_alloc_request() > b. why pass a req_flags_t instead of a blk_mq_req_flags_t - as I see, > rq_flags arg is only ever set to RQF_PM or 0, so no need for a conversion. Mostly historic I guess. But to me the fact that the blk_opf_t is passed is a good argument for only passing that and not an extra DMA direction. rq_flags is a mess - the flags to the request allocator are different from those at runtime, and the PM case needs both. We'll eventually need to clean this up in the block layer, but this is not the time.
On Fri, Nov 04, 2022 at 06:18:55PM -0500, Mike Christie wrote: > This begins to move the SCSI execution functions to use a struct for > passing in args. This patch adds the new struct, converts the low level > helpers and then adds a new helper the next patches will convert the rest > of the code to. > > Signed-off-by: Mike Christie <michael.christie@oracle.com> > Reviewed-by: Martin Wilck <mwilck@suse.com> > Reviewed-by: Bart Van Assche <bvanassche@acm.org> > --- > drivers/scsi/scsi_lib.c | 69 +++++++++++++++----------------------- > include/scsi/scsi_device.h | 69 ++++++++++++++++++++++++++++++-------- > 2 files changed, 82 insertions(+), 56 deletions(-) > > diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c > index fc1560981a03..f832befb50b0 100644 > --- a/drivers/scsi/scsi_lib.c > +++ b/drivers/scsi/scsi_lib.c > @@ -185,55 +185,39 @@ void scsi_queue_insert(struct scsi_cmnd *cmd, int reason) > __scsi_queue_insert(cmd, reason, true); > } > > - > /** > - * __scsi_execute - insert request and wait for the result > - * @sdev: scsi device > - * @cmd: scsi command > - * @data_direction: data direction > - * @buffer: data buffer > - * @bufflen: len of buffer > - * @sense: optional sense buffer > - * @sshdr: optional decoded sense header > - * @timeout: request timeout in HZ > - * @retries: number of times to retry request > - * @flags: flags for ->cmd_flags > - * @rq_flags: flags for ->rq_flags > - * @resid: optional residual length > + * __scsi_exec_req - insert request and wait for the result > + * @scsi_exec_args: See struct definition for description of each field > * > * Returns the scsi_cmnd result field if a command was executed, or a negative > * Linux error code if we didn't get that far. > */ > -int __scsi_execute(struct scsi_device *sdev, const unsigned char *cmd, > - int data_direction, void *buffer, unsigned bufflen, > - unsigned char *sense, struct scsi_sense_hdr *sshdr, > - int timeout, int retries, blk_opf_t flags, > - req_flags_t rq_flags, int *resid) > +int __scsi_exec_req(const struct scsi_exec_args *args) I find the struct for everyhing a somewhat confusing calling convention. So I'd pass the required arguments directly, and stuff all the optional bits into the struct. Based on the previous discussion maybe something like: int __scsi_execute_cmd(struct scsi_device *sdev, const unsigned char *cmd, blk_opf_t opf, void *buffer, unsigned int bufflen, int timeout, int retries, const struct scsi_exec_args *args) which would be a nice replacement for all the existing scsi_execute* interfaces.
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c index fc1560981a03..f832befb50b0 100644 --- a/drivers/scsi/scsi_lib.c +++ b/drivers/scsi/scsi_lib.c @@ -185,55 +185,39 @@ void scsi_queue_insert(struct scsi_cmnd *cmd, int reason) __scsi_queue_insert(cmd, reason, true); } - /** - * __scsi_execute - insert request and wait for the result - * @sdev: scsi device - * @cmd: scsi command - * @data_direction: data direction - * @buffer: data buffer - * @bufflen: len of buffer - * @sense: optional sense buffer - * @sshdr: optional decoded sense header - * @timeout: request timeout in HZ - * @retries: number of times to retry request - * @flags: flags for ->cmd_flags - * @rq_flags: flags for ->rq_flags - * @resid: optional residual length + * __scsi_exec_req - insert request and wait for the result + * @scsi_exec_args: See struct definition for description of each field * * Returns the scsi_cmnd result field if a command was executed, or a negative * Linux error code if we didn't get that far. */ -int __scsi_execute(struct scsi_device *sdev, const unsigned char *cmd, - int data_direction, void *buffer, unsigned bufflen, - unsigned char *sense, struct scsi_sense_hdr *sshdr, - int timeout, int retries, blk_opf_t flags, - req_flags_t rq_flags, int *resid) +int __scsi_exec_req(const struct scsi_exec_args *args) { struct request *req; struct scsi_cmnd *scmd; int ret; - req = scsi_alloc_request(sdev->request_queue, - data_direction == DMA_TO_DEVICE ? - REQ_OP_DRV_OUT : REQ_OP_DRV_IN, - rq_flags & RQF_PM ? BLK_MQ_REQ_PM : 0); + req = scsi_alloc_request(args->sdev->request_queue, + args->data_dir == DMA_TO_DEVICE ? + REQ_OP_DRV_OUT : REQ_OP_DRV_IN, + args->req_flags & RQF_PM ? BLK_MQ_REQ_PM : 0); if (IS_ERR(req)) return PTR_ERR(req); - if (bufflen) { - ret = blk_rq_map_kern(sdev->request_queue, req, - buffer, bufflen, GFP_NOIO); + if (args->buf) { + ret = blk_rq_map_kern(args->sdev->request_queue, req, args->buf, + args->buf_len, GFP_NOIO); if (ret) goto out; } scmd = blk_mq_rq_to_pdu(req); - scmd->cmd_len = COMMAND_SIZE(cmd[0]); - memcpy(scmd->cmnd, cmd, scmd->cmd_len); - scmd->allowed = retries; - req->timeout = timeout; - req->cmd_flags |= flags; - req->rq_flags |= rq_flags | RQF_QUIET; + scmd->cmd_len = COMMAND_SIZE(args->cmd[0]); + memcpy(scmd->cmnd, args->cmd, scmd->cmd_len); + scmd->allowed = args->retries; + req->timeout = args->timeout; + req->cmd_flags |= args->op_flags; + req->rq_flags |= args->req_flags | RQF_QUIET; /* * head injection *required* here otherwise quiesce won't work @@ -246,23 +230,24 @@ int __scsi_execute(struct scsi_device *sdev, const unsigned char *cmd, * is invalid. Prevent the garbage from being misinterpreted * and prevent security leaks by zeroing out the excess data. */ - if (unlikely(scmd->resid_len > 0 && scmd->resid_len <= bufflen)) - memset(buffer + bufflen - scmd->resid_len, 0, scmd->resid_len); - - if (resid) - *resid = scmd->resid_len; - if (sense && scmd->sense_len) - memcpy(sense, scmd->sense_buffer, SCSI_SENSE_BUFFERSIZE); - if (sshdr) + if (unlikely(scmd->resid_len > 0 && scmd->resid_len <= args->buf_len)) + memset(args->buf + args->buf_len - scmd->resid_len, 0, + scmd->resid_len); + + if (args->resid) + *args->resid = scmd->resid_len; + if (args->sense && scmd->sense_len) + memcpy(args->sense, scmd->sense_buffer, SCSI_SENSE_BUFFERSIZE); + if (args->sshdr) scsi_normalize_sense(scmd->sense_buffer, scmd->sense_len, - sshdr); + args->sshdr); ret = scmd->result; out: blk_mq_free_request(req); return ret; } -EXPORT_SYMBOL(__scsi_execute); +EXPORT_SYMBOL(__scsi_exec_req); /* * Wake up the error handler if necessary. Avoid as follows that the error diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h index 24bdbf7999ab..cd3957b80acd 100644 --- a/include/scsi/scsi_device.h +++ b/include/scsi/scsi_device.h @@ -454,28 +454,69 @@ extern const char *scsi_device_state_name(enum scsi_device_state); extern int scsi_is_sdev_device(const struct device *); extern int scsi_is_target_device(const struct device *); extern void scsi_sanitize_inquiry_string(unsigned char *s, int len); -extern int __scsi_execute(struct scsi_device *sdev, const unsigned char *cmd, - int data_direction, void *buffer, unsigned bufflen, - unsigned char *sense, struct scsi_sense_hdr *sshdr, - int timeout, int retries, blk_opf_t flags, - req_flags_t rq_flags, int *resid); + +struct scsi_exec_args { + struct scsi_device *sdev; /* scsi device */ + const unsigned char *cmd; /* scsi command */ + int data_dir; /* DMA direction */ + void *buf; /* data buffer */ + unsigned int buf_len; /* len of buffer */ + unsigned char *sense; /* optional sense buffer */ + unsigned int sense_len; /* optional sense buffer len */ + struct scsi_sense_hdr *sshdr; /* optional decoded sense header */ + int timeout; /* request timeout in HZ */ + int retries; /* number of times to retry request */ + blk_opf_t op_flags; /* flags for ->cmd_flags */ + req_flags_t req_flags; /* flags for ->rq_flags */ + int *resid; /* optional residual length */ +}; + +extern int __scsi_exec_req(const struct scsi_exec_args *args); +/* 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); \ +}) + /* Make sure any sense buffer is the correct size. */ -#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) \ ({ \ - BUILD_BUG_ON((sense) != NULL && \ - sizeof(sense) != SCSI_SENSE_BUFFERSIZE); \ - __scsi_execute(sdev, cmd, data_direction, buffer, bufflen, \ - sense, sshdr, timeout, retries, flags, rq_flags, \ - resid); \ + BUILD_BUG_ON((_sense) != NULL && \ + sizeof(_sense) != SCSI_SENSE_BUFFERSIZE); \ + __scsi_exec_req(&((struct scsi_exec_args) { \ + .sdev = _sdev, \ + .cmd = _cmd, \ + .data_dir = _data_dir, \ + .buf = _buffer, \ + .buf_len = _bufflen, \ + .sense = _sense, \ + .sshdr = _sshdr, \ + .timeout = _timeout, \ + .retries = _retries, \ + .op_flags = _flags, \ + .req_flags = _rq_flags, \ + .resid = _resid, })); \ }) + static inline int scsi_execute_req(struct scsi_device *sdev, const unsigned char *cmd, int data_direction, void *buffer, unsigned bufflen, struct scsi_sense_hdr *sshdr, int timeout, int retries, int *resid) { - return scsi_execute(sdev, cmd, data_direction, buffer, - bufflen, NULL, sshdr, timeout, retries, 0, 0, resid); + return __scsi_exec_req(&(struct scsi_exec_args) { + .sdev = sdev, + .cmd = cmd, + .data_dir = data_direction, + .buf = buffer, + .buf_len = bufflen, + .sshdr = sshdr, + .timeout = timeout, + .retries = retries, + .resid = resid }); } extern void sdev_disable_disk_events(struct scsi_device *sdev); extern void sdev_enable_disk_events(struct scsi_device *sdev);