| Message ID | 20200703130122.111448-4-hare@suse.de (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | scsi: enable reserved commands for LLDDs | expand |
On 03/07/2020 14:01, Hannes Reinecke wrote: > Add helper functions to allow LLDDs to allocate and free > internal commands. > > Signed-off-by: Hannes Reinecke <hare@suse.de> Not sure how Christoph feels about this now, but FWIW: Reviewed-by: John Garry <john.garry@huawei.com> But a couple of comments, below. > --- > drivers/scsi/scsi_lib.c | 45 +++++++++++++++++++++++++++++++++++++++++++++ > include/scsi/scsi_device.h | 4 ++++ > 2 files changed, 49 insertions(+) > > diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c > index 0ba7a65e7c8d..1d5c1b9a1203 100644 > --- a/drivers/scsi/scsi_lib.c > +++ b/drivers/scsi/scsi_lib.c > @@ -1903,6 +1903,51 @@ void scsi_mq_destroy_tags(struct Scsi_Host *shost) > blk_mq_free_tag_set(&shost->tag_set); > } > > +/** > + * scsi_get_internal_cmd - allocate an internal SCSI command > + * @sdev: SCSI device from which to allocate the command > + * @data_direction: Data direction for the allocated command > + * @op_flags: request allocation flags > + * > + * Allocates a SCSI command for internal LLDD use. > + */ > +struct scsi_cmnd *scsi_get_internal_cmd(struct scsi_device *sdev, > + enum dma_data_direction data_direction, int op_flags) > +{ > + struct request *rq; > + struct scsi_cmnd *scmd; > + blk_mq_req_flags_t flags = 0; > + unsigned int op = REQ_INTERNAL | op_flags; nit: some people like ordering local variables in reverse Christmas tree style when possible, but I don't really care. > + > + op |= (data_direction == DMA_TO_DEVICE) ? > + REQ_OP_SCSI_OUT : REQ_OP_SCSI_IN; > + rq = blk_mq_alloc_request(sdev->request_queue, op, flags); > + if (IS_ERR(rq)) > + return NULL; > + scmd = blk_mq_rq_to_pdu(rq); > + scmd->request = rq; > + scmd->device = sdev; > + return scmd; > +} > +EXPORT_SYMBOL_GPL(scsi_get_internal_cmd); > + > +/** > + * scsi_put_internal_cmd - free an internal SCSI command > + * @scmd: SCSI command to be freed > + * > + * Check if @scmd is an internal command, and call > + * blk_mq_free_request() if true. > + */ > +void scsi_put_internal_cmd(struct scsi_cmnd *scmd) > +{ > + struct request *rq = blk_mq_rq_from_pdu(scmd); > + > + if (WARN_ON(!blk_rq_is_internal(rq))) > + return; > + blk_mq_free_request(rq); > +} > +EXPORT_SYMBOL_GPL(scsi_put_internal_cmd); > + > /** > * scsi_device_from_queue - return sdev associated with a request_queue > * @q: The request queue to return the sdev from > diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h > index bc5909033d13..2759a538adae 100644 > --- a/include/scsi/scsi_device.h > +++ b/include/scsi/scsi_device.h > @@ -8,6 +8,7 @@ > #include <linux/blkdev.h> > #include <scsi/scsi.h> > #include <linux/atomic.h> > +#include <linux/dma-direction.h> > > struct device; > struct request_queue; > @@ -460,6 +461,9 @@ static inline int scsi_execute_req(struct scsi_device *sdev, > return scsi_execute(sdev, cmd, data_direction, buffer, > bufflen, NULL, sshdr, timeout, retries, 0, 0, resid); > } > +struct scsi_cmnd *scsi_get_internal_cmd(struct scsi_device *sdev, > + enum dma_data_direction data_direction, int op_flags); > +void scsi_put_internal_cmd(struct scsi_cmnd *scmd); > extern void sdev_disable_disk_events(struct scsi_device *sdev); > extern void sdev_enable_disk_events(struct scsi_device *sdev); > extern int scsi_vpd_lun_id(struct scsi_device *, char *, size_t); If I go to delete all these externs so we can be consistent, will someone complain? >
On 03/07/2020 14:01, Hannes Reinecke wrote: > Add helper functions to allow LLDDs to allocate and free > internal commands. Hi Hannes, Is there any way to ensure that the request allocated is associated with some determined HW queue here? The reason for this requirement is that sometimes the LLDD must submit some internal IO (for which we allocate an "internal command") on a specific HW queue. An example of this is internal abort IO commands, which should be submitted on the same queue as the IO which we are attempting to abort was submitted. So, for sure, the LLDD does not have to honor the hwq associated with the request and submit on the desired queue, but then we lose the blk-mq CPU hotplug protection. And maybe other problems. One way to achieve this is to run scsi_get_internal_cmd() on a CPU associated with the desired HW queue, but that's a bit hacky. Not sure of another way. Thanks, John > > Signed-off-by: Hannes Reinecke <hare@suse.de> > --- > drivers/scsi/scsi_lib.c | 45 +++++++++++++++++++++++++++++++++++++++++++++ > include/scsi/scsi_device.h | 4 ++++ > 2 files changed, 49 insertions(+) > > diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c > index 0ba7a65e7c8d..1d5c1b9a1203 100644 > --- a/drivers/scsi/scsi_lib.c > +++ b/drivers/scsi/scsi_lib.c > @@ -1903,6 +1903,51 @@ void scsi_mq_destroy_tags(struct Scsi_Host *shost) > blk_mq_free_tag_set(&shost->tag_set); > } > > +/** > + * scsi_get_internal_cmd - allocate an internal SCSI command > + * @sdev: SCSI device from which to allocate the command > + * @data_direction: Data direction for the allocated command > + * @op_flags: request allocation flags > + * > + * Allocates a SCSI command for internal LLDD use. > + */ > +struct scsi_cmnd *scsi_get_internal_cmd(struct scsi_device *sdev, > + enum dma_data_direction data_direction, int op_flags) > +{ > + struct request *rq; > + struct scsi_cmnd *scmd; > + blk_mq_req_flags_t flags = 0; > + unsigned int op = REQ_INTERNAL | op_flags; > + > + op |= (data_direction == DMA_TO_DEVICE) ? > + REQ_OP_SCSI_OUT : REQ_OP_SCSI_IN; > + rq = blk_mq_alloc_request(sdev->request_queue, op, flags); > + if (IS_ERR(rq)) > + return NULL; > + scmd = blk_mq_rq_to_pdu(rq); > + scmd->request = rq; > + scmd->device = sdev; > + return scmd; > +} > +EXPORT_SYMBOL_GPL(scsi_get_internal_cmd); > + > +/** > + * scsi_put_internal_cmd - free an internal SCSI command > + * @scmd: SCSI command to be freed > + * > + * Check if @scmd is an internal command, and call > + * blk_mq_free_request() if true. > + */ > +void scsi_put_internal_cmd(struct scsi_cmnd *scmd) > +{ > + struct request *rq = blk_mq_rq_from_pdu(scmd); > + > + if (WARN_ON(!blk_rq_is_internal(rq))) > + return; > + blk_mq_free_request(rq); > +} > +EXPORT_SYMBOL_GPL(scsi_put_internal_cmd); > + > /** > * scsi_device_from_queue - return sdev associated with a request_queue > * @q: The request queue to return the sdev from > diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h > index bc5909033d13..2759a538adae 100644 > --- a/include/scsi/scsi_device.h > +++ b/include/scsi/scsi_device.h > @@ -8,6 +8,7 @@ > #include <linux/blkdev.h> > #include <scsi/scsi.h> > #include <linux/atomic.h> > +#include <linux/dma-direction.h> > > struct device; > struct request_queue; > @@ -460,6 +461,9 @@ static inline int scsi_execute_req(struct scsi_device *sdev, > return scsi_execute(sdev, cmd, data_direction, buffer, > bufflen, NULL, sshdr, timeout, retries, 0, 0, resid); > } > +struct scsi_cmnd *scsi_get_internal_cmd(struct scsi_device *sdev, > + enum dma_data_direction data_direction, int op_flags); > +void scsi_put_internal_cmd(struct scsi_cmnd *scmd); > extern void sdev_disable_disk_events(struct scsi_device *sdev); > extern void sdev_enable_disk_events(struct scsi_device *sdev); > extern int scsi_vpd_lun_id(struct scsi_device *, char *, size_t); >
On 11/16/20 9:56 AM, John Garry wrote: > On 03/07/2020 14:01, Hannes Reinecke wrote: >> Add helper functions to allow LLDDs to allocate and free >> internal commands. > > Hi Hannes, > > Is there any way to ensure that the request allocated is associated with > some determined HW queue here? > > The reason for this requirement is that sometimes the LLDD must submit > some internal IO (for which we allocate an "internal command") on a > specific HW queue. An example of this is internal abort IO commands, > which should be submitted on the same queue as the IO which we are > attempting to abort was submitted. > > So, for sure, the LLDD does not have to honor the hwq associated with > the request and submit on the desired queue, but then we lose the blk-mq > CPU hotplug protection. And maybe other problems. > > One way to achieve this is to run scsi_get_internal_cmd() on a CPU > associated with the desired HW queue, but that's a bit hacky. Not sure > of another way. > Hmm. You are correct for the 'abort' command; that typically needs to be submitted to a specific hwq. Let me think about it... Cheers, Hannes
On 16/11/2020 09:03, Hannes Reinecke wrote: >> Hi Hannes, >> >> Is there any way to ensure that the request allocated is associated >> with some determined HW queue here? >> >> The reason for this requirement is that sometimes the LLDD must submit >> some internal IO (for which we allocate an "internal command") on a >> specific HW queue. An example of this is internal abort IO commands, >> which should be submitted on the same queue as the IO which we are >> attempting to abort was submitted. >> >> So, for sure, the LLDD does not have to honor the hwq associated with >> the request and submit on the desired queue, but then we lose the >> blk-mq CPU hotplug protection. And maybe other problems. >> >> One way to achieve this is to run scsi_get_internal_cmd() on a CPU >> associated with the desired HW queue, but that's a bit hacky. Not sure >> of another way. >> > Hmm. You are correct for the 'abort' command; that typically needs to be > submitted to a specific hwq. > So now I see that we have blk_mq_alloc_request_hctx(..., hctx_idx) already, so that should do the job :) Thanks, John
On 16/11/2020 09:03, Hannes Reinecke wrote: > On 11/16/20 9:56 AM, John Garry wrote: >> On 03/07/2020 14:01, Hannes Reinecke wrote: >>> Add helper functions to allow LLDDs to allocate and free >>> internal commands. >> >> Hi Hannes, >> >> Is there any way to ensure that the request allocated is associated >> with some determined HW queue here? >> >> The reason for this requirement is that sometimes the LLDD must submit >> some internal IO (for which we allocate an "internal command") on a >> specific HW queue. An example of this is internal abort IO commands, >> which should be submitted on the same queue as the IO which we are >> attempting to abort was submitted. >> >> So, for sure, the LLDD does not have to honor the hwq associated with >> the request and submit on the desired queue, but then we lose the >> blk-mq CPU hotplug protection. And maybe other problems. >> >> One way to achieve this is to run scsi_get_internal_cmd() on a CPU >> associated with the desired HW queue, but that's a bit hacky. Not sure >> of another way. >> > Hmm. You are correct for the 'abort' command; that typically needs to be > submitted to a specific hwq. > > Let me think about it... > Hannes, Earlier you mentioned " some drivers not only the command needs a tag, but the sgls also, thereby completely messing up our mq tags logic. So to map those we'd need to allocate _several_ tags for one command ..." Which drivers are these? Can you provide a pointer? Is this the blocker? Thanks, John
On 12/7/20 11:15 AM, John Garry wrote: > On 16/11/2020 09:03, Hannes Reinecke wrote: >> On 11/16/20 9:56 AM, John Garry wrote: >>> On 03/07/2020 14:01, Hannes Reinecke wrote: >>>> Add helper functions to allow LLDDs to allocate and free >>>> internal commands. >>> >>> Hi Hannes, >>> >>> Is there any way to ensure that the request allocated is associated >>> with some determined HW queue here? >>> >>> The reason for this requirement is that sometimes the LLDD must >>> submit some internal IO (for which we allocate an "internal command") >>> on a specific HW queue. An example of this is internal abort IO >>> commands, which should be submitted on the same queue as the IO which >>> we are attempting to abort was submitted. >>> >>> So, for sure, the LLDD does not have to honor the hwq associated with >>> the request and submit on the desired queue, but then we lose the >>> blk-mq CPU hotplug protection. And maybe other problems. >>> >>> One way to achieve this is to run scsi_get_internal_cmd() on a CPU >>> associated with the desired HW queue, but that's a bit hacky. Not >>> sure of another way. >>> >> Hmm. You are correct for the 'abort' command; that typically needs to >> be submitted to a specific hwq. >> >> Let me think about it... >> > > > Hannes, > > Earlier you mentioned " some drivers not only the command needs a tag, > but the sgls also, thereby completely messing up our mq tags logic. > So to map those we'd need to allocate _several_ tags for one command ..." > > Which drivers are these? Can you provide a pointer? Is this the blocker? > Not actually a blocker, more something which I had been thinking about. Point is, that the current blk-mq design assumes that each command requires a tag, and each tag maps to a single entity placed on the hardware submission queue. While this is okay for some drivers, it's not a straightforward match for other drivers: - FC uses 'Exchange IDs' to map commands; however, the lifetime for these XIDs is slightly different from the command lifetime. In case of an error the XID must not be re-used (on the same queue) until error recovery has taken place. Which means we cannot use the block-layer tags for XIDs but have to implement an internal facility for that (cf drivers/scsi/libfc/fc_exch.c to get some idea about that). - Some drivers like smartpqi or mpt3sas require the command _and the sgls_ to be placed on the submission queue, and every entity requires an unique tag. So again, we cannot leverage the block-layer tags to control the submission queue but have to implement a driver-specific thing. But I'll be rebasing my patches. Cheers, Hannes
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c index 0ba7a65e7c8d..1d5c1b9a1203 100644 --- a/drivers/scsi/scsi_lib.c +++ b/drivers/scsi/scsi_lib.c @@ -1903,6 +1903,51 @@ void scsi_mq_destroy_tags(struct Scsi_Host *shost) blk_mq_free_tag_set(&shost->tag_set); } +/** + * scsi_get_internal_cmd - allocate an internal SCSI command + * @sdev: SCSI device from which to allocate the command + * @data_direction: Data direction for the allocated command + * @op_flags: request allocation flags + * + * Allocates a SCSI command for internal LLDD use. + */ +struct scsi_cmnd *scsi_get_internal_cmd(struct scsi_device *sdev, + enum dma_data_direction data_direction, int op_flags) +{ + struct request *rq; + struct scsi_cmnd *scmd; + blk_mq_req_flags_t flags = 0; + unsigned int op = REQ_INTERNAL | op_flags; + + op |= (data_direction == DMA_TO_DEVICE) ? + REQ_OP_SCSI_OUT : REQ_OP_SCSI_IN; + rq = blk_mq_alloc_request(sdev->request_queue, op, flags); + if (IS_ERR(rq)) + return NULL; + scmd = blk_mq_rq_to_pdu(rq); + scmd->request = rq; + scmd->device = sdev; + return scmd; +} +EXPORT_SYMBOL_GPL(scsi_get_internal_cmd); + +/** + * scsi_put_internal_cmd - free an internal SCSI command + * @scmd: SCSI command to be freed + * + * Check if @scmd is an internal command, and call + * blk_mq_free_request() if true. + */ +void scsi_put_internal_cmd(struct scsi_cmnd *scmd) +{ + struct request *rq = blk_mq_rq_from_pdu(scmd); + + if (WARN_ON(!blk_rq_is_internal(rq))) + return; + blk_mq_free_request(rq); +} +EXPORT_SYMBOL_GPL(scsi_put_internal_cmd); + /** * scsi_device_from_queue - return sdev associated with a request_queue * @q: The request queue to return the sdev from diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h index bc5909033d13..2759a538adae 100644 --- a/include/scsi/scsi_device.h +++ b/include/scsi/scsi_device.h @@ -8,6 +8,7 @@ #include <linux/blkdev.h> #include <scsi/scsi.h> #include <linux/atomic.h> +#include <linux/dma-direction.h> struct device; struct request_queue; @@ -460,6 +461,9 @@ static inline int scsi_execute_req(struct scsi_device *sdev, return scsi_execute(sdev, cmd, data_direction, buffer, bufflen, NULL, sshdr, timeout, retries, 0, 0, resid); } +struct scsi_cmnd *scsi_get_internal_cmd(struct scsi_device *sdev, + enum dma_data_direction data_direction, int op_flags); +void scsi_put_internal_cmd(struct scsi_cmnd *scmd); extern void sdev_disable_disk_events(struct scsi_device *sdev); extern void sdev_enable_disk_events(struct scsi_device *sdev); extern int scsi_vpd_lun_id(struct scsi_device *, char *, size_t);
Add helper functions to allow LLDDs to allocate and free internal commands. Signed-off-by: Hannes Reinecke <hare@suse.de> --- drivers/scsi/scsi_lib.c | 45 +++++++++++++++++++++++++++++++++++++++++++++ include/scsi/scsi_device.h | 4 ++++ 2 files changed, 49 insertions(+)