| Message ID | 20201015133633.61836-1-kashyap.desai@broadcom.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v1,1/3] add io_uring with IOPOLL support in scsi layer | expand |
On 15/10/2020 14:36, Kashyap Desai wrote: > io_uring with IOPOLL is not currently supported in scsi mid layer. > Outside of that everything else should work and no extra support in the driver is needed. > Currently io_uring with IOPOLL support is only available in block layer. > This patch is to extend support of mq_poll in scsi layer. > > Signed-off-by: Kashyap Desai <kashyap.desai@broadcom.com> > Cc: sumit.saxena@broadcom.com > Cc: chandrakanth.patil@broadcom.com > Cc: linux-block@vger.kernel.org > > --- > drivers/scsi/scsi_lib.c | 16 ++++++++++++++++ > include/scsi/scsi_cmnd.h | 1 + > include/scsi/scsi_host.h | 11 +++++++++++ > 3 files changed, 28 insertions(+) > > diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c > index 72b12102f777..5a3c383a2bb3 100644 > --- a/drivers/scsi/scsi_lib.c > +++ b/drivers/scsi/scsi_lib.c > @@ -1766,6 +1766,19 @@ static void scsi_mq_exit_request(struct blk_mq_tag_set *set, struct request *rq, > cmd->sense_buffer); > } > > + > +static int scsi_mq_poll(struct blk_mq_hw_ctx *hctx) > +{ > + struct request_queue *q = hctx->queue; > + struct scsi_device *sdev = q->queuedata; > + struct Scsi_Host *shost = sdev->host; could we separately set hctx->driver_data = shost or similar for a quicker lookup? I don't see hctx->driver_data set for SCSI currently. Going through the scsi_device looks strange - I know that it is done in scsi_commit_rqs. > + > + if (shost->hostt->mq_poll) to avoid this check, could we reject if .mq_poll is not set and HCTX_TYPE_POLL is? > + return shost->hostt->mq_poll(shost, hctx->queue_num); > + > + return 0; > +} > + > static int scsi_map_queues(struct blk_mq_tag_set *set) > { > struct Scsi_Host *shost = container_of(set, struct Scsi_Host, tag_set); > @@ -1833,6 +1846,7 @@ static const struct blk_mq_ops scsi_mq_ops_no_commit = { > .cleanup_rq = scsi_cleanup_rq, > .busy = scsi_mq_lld_busy, > .map_queues = scsi_map_queues, > + .poll = scsi_mq_poll, > }; > > > @@ -1861,6 +1875,7 @@ static const struct blk_mq_ops scsi_mq_ops = { > .cleanup_rq = scsi_cleanup_rq, > .busy = scsi_mq_lld_busy, > .map_queues = scsi_map_queues, > + .poll = scsi_mq_poll, > }; > > struct request_queue *scsi_mq_alloc_queue(struct scsi_device *sdev) > @@ -1893,6 +1908,7 @@ int scsi_mq_setup_tags(struct Scsi_Host *shost) > else > tag_set->ops = &scsi_mq_ops_no_commit; > tag_set->nr_hw_queues = shost->nr_hw_queues ? : 1; > + tag_set->nr_maps = shost->nr_maps ? : 1; > tag_set->queue_depth = shost->can_queue; > tag_set->cmd_size = cmd_size; > tag_set->numa_node = NUMA_NO_NODE; > diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h > index e76bac4d14c5..5844374a85b1 100644 > --- a/include/scsi/scsi_cmnd.h > +++ b/include/scsi/scsi_cmnd.h > @@ -9,6 +9,7 @@ > #include <linux/types.h> > #include <linux/timer.h> > #include <linux/scatterlist.h> > +#include <scsi/scsi_host.h> can we maintain alphabetic ordering? > #include <scsi/scsi_device.h> > #include <scsi/scsi_request.h> > > diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h > index 701f178b20ae..905ee6b00c55 100644 > --- a/include/scsi/scsi_host.h > +++ b/include/scsi/scsi_host.h > @@ -270,6 +270,16 @@ struct scsi_host_template { > */ > int (* map_queues)(struct Scsi_Host *shost); > > + /* > + * SCSI interface of blk_poll - poll for IO completions. > + * Possible interface only if scsi LLD expose multiple h/w queues. > + * > + * Return values: Number of completed entries found. /s/values/value/ > + * > + * Status: OPTIONAL > + */ > + int (* mq_poll)(struct Scsi_Host *shost, unsigned int queue_num); > + > /* > * Check if scatterlists need to be padded for DMA draining. > * > @@ -610,6 +620,7 @@ struct Scsi_Host { > * the total queue depth is can_queue. > */ > unsigned nr_hw_queues; > + unsigned nr_maps; > unsigned active_mode:2; > unsigned unchecked_isa_dma:1; > >
On 10/15/20 3:36 PM, Kashyap Desai wrote: > io_uring with IOPOLL is not currently supported in scsi mid layer. > Outside of that everything else should work and no extra support in the driver is needed. > Currently io_uring with IOPOLL support is only available in block layer. > This patch is to extend support of mq_poll in scsi layer. > > Signed-off-by: Kashyap Desai <kashyap.desai@broadcom.com> > Cc: sumit.saxena@broadcom.com > Cc: chandrakanth.patil@broadcom.com > Cc: linux-block@vger.kernel.org > > --- > drivers/scsi/scsi_lib.c | 16 ++++++++++++++++ > include/scsi/scsi_cmnd.h | 1 + > include/scsi/scsi_host.h | 11 +++++++++++ > 3 files changed, 28 insertions(+) > Reviewed-by: Hannes Reinecke <hare@suse.de> Cheers, Hannes -- Dr. Hannes Reinecke Kernel Storage Architect hare@suse.de +49 911 74053 688 SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer
> > diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c index > > 72b12102f777..5a3c383a2bb3 100644 > > --- a/drivers/scsi/scsi_lib.c > > +++ b/drivers/scsi/scsi_lib.c > > @@ -1766,6 +1766,19 @@ static void scsi_mq_exit_request(struct > blk_mq_tag_set *set, struct request *rq, > > cmd->sense_buffer); > > } > > > > + > > +static int scsi_mq_poll(struct blk_mq_hw_ctx *hctx) { > > + struct request_queue *q = hctx->queue; > > + struct scsi_device *sdev = q->queuedata; > > + struct Scsi_Host *shost = sdev->host; > > could we separately set hctx->driver_data = shost or similar for a quicker > lookup? I don't see hctx->driver_data set for SCSI currently. > Going through the scsi_device looks strange - I know that it is done in > scsi_commit_rqs. John - I have included your comments. Below is add-on patch which handles all your comment except one. Below is just compiled (not tested patch). Please let me know if you like to handle "scsi_init_hctx" in this patch or shall we do it as a separate patch (out of this patch series.) ? --- a/drivers/scsi/scsi_lib.c +++ b/drivers/scsi/scsi_lib.c @@ -1769,9 +1769,7 @@ static void scsi_mq_exit_request(struct blk_mq_tag_set *set, struct request *rq, static int scsi_mq_poll(struct blk_mq_hw_ctx *hctx) { - struct request_queue *q = hctx->queue; - struct scsi_device *sdev = q->queuedata; - struct Scsi_Host *shost = sdev->host; + struct Scsi_Host *shost = hctx->driver_data; if (shost->hostt->mq_poll) return shost->hostt->mq_poll(shost, hctx->queue_num); @@ -1779,6 +1777,14 @@ static int scsi_mq_poll(struct blk_mq_hw_ctx *hctx) return 0; } +static int scsi_init_hctx(struct blk_mq_hw_ctx *hctx, void *data, + unsigned int hctx_idx) +{ + struct Scsi_Host *shost = data; + hctx->driver_data = shost; + return 0; +} + static int scsi_map_queues(struct blk_mq_tag_set *set) { struct Scsi_Host *shost = container_of(set, struct Scsi_Host, tag_set); @@ -1846,6 +1852,7 @@ static const struct blk_mq_ops scsi_mq_ops_no_commit = { .cleanup_rq = scsi_cleanup_rq, .busy = scsi_mq_lld_busy, .map_queues = scsi_map_queues, + .init_hctx = scsi_init_hctx, .poll = scsi_mq_poll, }; @@ -1875,6 +1882,7 @@ static const struct blk_mq_ops scsi_mq_ops = { .cleanup_rq = scsi_cleanup_rq, .busy = scsi_mq_lld_busy, .map_queues = scsi_map_queues, + .init_hctx = scsi_init_hctx, .poll = scsi_mq_poll, }; diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h index 5844374a85b1..cc30df96f5f7 100644 --- a/include/scsi/scsi_cmnd.h +++ b/include/scsi/scsi_cmnd.h @@ -9,8 +9,8 @@ #include <linux/types.h> #include <linux/timer.h> #include <linux/scatterlist.h> -#include <scsi/scsi_host.h> #include <scsi/scsi_device.h> +#include <scsi/scsi_host.h> #include <scsi/scsi_request.h> struct Scsi_Host; diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h index 905ee6b00c55..a0cda0f66b84 100644 --- a/include/scsi/scsi_host.h +++ b/include/scsi/scsi_host.h @@ -274,7 +274,7 @@ struct scsi_host_template { * SCSI interface of blk_poll - poll for IO completions. * Possible interface only if scsi LLD expose multiple h/w queues. * - * Return values: Number of completed entries found. + * Return value: Number of completed entries found. * * Status: OPTIONAL */ > > > + > > + if (shost->hostt->mq_poll) > > to avoid this check, could we reject if .mq_poll is not set and > HCTX_TYPE_POLL is? Is this urgent or shall we improve later ? I am not able to figure out how you want to manage this ? Can you explain little bit ? Kashyap > > > + return shost->hostt->mq_poll(shost, hctx->queue_num); > > + > > + return 0; > > +}
On 30/11/2020 07:41, Kashyap Desai wrote: >>> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c index >>> 72b12102f777..5a3c383a2bb3 100644 >>> --- a/drivers/scsi/scsi_lib.c >>> +++ b/drivers/scsi/scsi_lib.c >>> @@ -1766,6 +1766,19 @@ static void scsi_mq_exit_request(struct >> blk_mq_tag_set *set, struct request *rq, >>> cmd->sense_buffer); >>> } >>> >>> + >>> +static int scsi_mq_poll(struct blk_mq_hw_ctx *hctx) { >>> + struct request_queue *q = hctx->queue; >>> + struct scsi_device *sdev = q->queuedata; >>> + struct Scsi_Host *shost = sdev->host; >> could we separately set hctx->driver_data = shost or similar for a quicker >> lookup? I don't see hctx->driver_data set for SCSI currently. >> Going through the scsi_device looks strange - I know that it is done in >> scsi_commit_rqs. > John - I have included your comments. Below is add-on patch which handles > all your comment except one. > Below is just compiled (not tested patch). Please let me know if you like to > handle "scsi_init_hctx" in this patch or shall we do it as a separate patch > (out of this patch series.) ? It might be better as a separate patch if you also change scsi_commit_rqs() to use hctx->driver_data, which you are currently not doing (or showing here). > > > --- a/drivers/scsi/scsi_lib.c > +++ b/drivers/scsi/scsi_lib.c > @@ -1769,9 +1769,7 @@ static void scsi_mq_exit_request(struct blk_mq_tag_set > *set, struct request *rq, > > static int scsi_mq_poll(struct blk_mq_hw_ctx *hctx) > { > - struct request_queue *q = hctx->queue; > - struct scsi_device *sdev = q->queuedata; > - struct Scsi_Host *shost = sdev->host; > + struct Scsi_Host *shost = hctx->driver_data; > > if (shost->hostt->mq_poll) > return shost->hostt->mq_poll(shost, hctx->queue_num); > @@ -1779,6 +1777,14 @@ static int scsi_mq_poll(struct blk_mq_hw_ctx *hctx) > return 0; > } > > +static int scsi_init_hctx(struct blk_mq_hw_ctx *hctx, void *data, > + unsigned int hctx_idx) > +{ > + struct Scsi_Host *shost = data; > + hctx->driver_data = shost; > + return 0; > +} > + > static int scsi_map_queues(struct blk_mq_tag_set *set) > { > struct Scsi_Host *shost = container_of(set, struct Scsi_Host, > tag_set); > @@ -1846,6 +1852,7 @@ static const struct blk_mq_ops scsi_mq_ops_no_commit = > { > .cleanup_rq = scsi_cleanup_rq, > .busy = scsi_mq_lld_busy, > .map_queues = scsi_map_queues, > + .init_hctx = scsi_init_hctx, > .poll = scsi_mq_poll, > }; > > @@ -1875,6 +1882,7 @@ static const struct blk_mq_ops scsi_mq_ops = { > .cleanup_rq = scsi_cleanup_rq, > .busy = scsi_mq_lld_busy, > .map_queues = scsi_map_queues, > + .init_hctx = scsi_init_hctx, > .poll = scsi_mq_poll, > }; > > diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h > index 5844374a85b1..cc30df96f5f7 100644 > --- a/include/scsi/scsi_cmnd.h > +++ b/include/scsi/scsi_cmnd.h > @@ -9,8 +9,8 @@ > #include <linux/types.h> > #include <linux/timer.h> > #include <linux/scatterlist.h> > -#include <scsi/scsi_host.h> > #include <scsi/scsi_device.h> > +#include <scsi/scsi_host.h> > #include <scsi/scsi_request.h> > > struct Scsi_Host; > diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h > index 905ee6b00c55..a0cda0f66b84 100644 > --- a/include/scsi/scsi_host.h > +++ b/include/scsi/scsi_host.h > @@ -274,7 +274,7 @@ struct scsi_host_template { > * SCSI interface of blk_poll - poll for IO completions. > * Possible interface only if scsi LLD expose multiple h/w queues. > * > - * Return values: Number of completed entries found. > + * Return value: Number of completed entries found. > * > * Status: OPTIONAL > */ > >>> + >>> + if (shost->hostt->mq_poll) >> to avoid this check, could we reject if .mq_poll is not set and >> HCTX_TYPE_POLL is? > Is this urgent or shall we improve later ? I am not able to figure out how > you want to manage this ? Can you explain little bit ? I don't think that it will make much overhead difference, so ok to omit for now if more trouble than it's worth to implement. Thanks, John
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c index 72b12102f777..5a3c383a2bb3 100644 --- a/drivers/scsi/scsi_lib.c +++ b/drivers/scsi/scsi_lib.c @@ -1766,6 +1766,19 @@ static void scsi_mq_exit_request(struct blk_mq_tag_set *set, struct request *rq, cmd->sense_buffer); } + +static int scsi_mq_poll(struct blk_mq_hw_ctx *hctx) +{ + struct request_queue *q = hctx->queue; + struct scsi_device *sdev = q->queuedata; + struct Scsi_Host *shost = sdev->host; + + if (shost->hostt->mq_poll) + return shost->hostt->mq_poll(shost, hctx->queue_num); + + return 0; +} + static int scsi_map_queues(struct blk_mq_tag_set *set) { struct Scsi_Host *shost = container_of(set, struct Scsi_Host, tag_set); @@ -1833,6 +1846,7 @@ static const struct blk_mq_ops scsi_mq_ops_no_commit = { .cleanup_rq = scsi_cleanup_rq, .busy = scsi_mq_lld_busy, .map_queues = scsi_map_queues, + .poll = scsi_mq_poll, }; @@ -1861,6 +1875,7 @@ static const struct blk_mq_ops scsi_mq_ops = { .cleanup_rq = scsi_cleanup_rq, .busy = scsi_mq_lld_busy, .map_queues = scsi_map_queues, + .poll = scsi_mq_poll, }; struct request_queue *scsi_mq_alloc_queue(struct scsi_device *sdev) @@ -1893,6 +1908,7 @@ int scsi_mq_setup_tags(struct Scsi_Host *shost) else tag_set->ops = &scsi_mq_ops_no_commit; tag_set->nr_hw_queues = shost->nr_hw_queues ? : 1; + tag_set->nr_maps = shost->nr_maps ? : 1; tag_set->queue_depth = shost->can_queue; tag_set->cmd_size = cmd_size; tag_set->numa_node = NUMA_NO_NODE; diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h index e76bac4d14c5..5844374a85b1 100644 --- a/include/scsi/scsi_cmnd.h +++ b/include/scsi/scsi_cmnd.h @@ -9,6 +9,7 @@ #include <linux/types.h> #include <linux/timer.h> #include <linux/scatterlist.h> +#include <scsi/scsi_host.h> #include <scsi/scsi_device.h> #include <scsi/scsi_request.h> diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h index 701f178b20ae..905ee6b00c55 100644 --- a/include/scsi/scsi_host.h +++ b/include/scsi/scsi_host.h @@ -270,6 +270,16 @@ struct scsi_host_template { */ int (* map_queues)(struct Scsi_Host *shost); + /* + * SCSI interface of blk_poll - poll for IO completions. + * Possible interface only if scsi LLD expose multiple h/w queues. + * + * Return values: Number of completed entries found. + * + * Status: OPTIONAL + */ + int (* mq_poll)(struct Scsi_Host *shost, unsigned int queue_num); + /* * Check if scatterlists need to be padded for DMA draining. * @@ -610,6 +620,7 @@ struct Scsi_Host { * the total queue depth is can_queue. */ unsigned nr_hw_queues; + unsigned nr_maps; unsigned active_mode:2; unsigned unchecked_isa_dma:1;
io_uring with IOPOLL is not currently supported in scsi mid layer. Outside of that everything else should work and no extra support in the driver is needed. Currently io_uring with IOPOLL support is only available in block layer. This patch is to extend support of mq_poll in scsi layer. Signed-off-by: Kashyap Desai <kashyap.desai@broadcom.com> Cc: sumit.saxena@broadcom.com Cc: chandrakanth.patil@broadcom.com Cc: linux-block@vger.kernel.org --- drivers/scsi/scsi_lib.c | 16 ++++++++++++++++ include/scsi/scsi_cmnd.h | 1 + include/scsi/scsi_host.h | 11 +++++++++++ 3 files changed, 28 insertions(+)