| Message ID | 20190529132901.27645-11-hare@suse.de (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | scsi: enable reserved commands for LLDDs | expand |
On 29/05/2019 14:28, Hannes Reinecke wrote: > From: Hannes Reinecke <hare@suse.com> > > Allocate a separate 'reserved_cmd_q' for sending reserved commands. > > Signed-off-by: Hannes Reinecke <hare@suse.com> > --- > drivers/scsi/scsi_lib.c | 15 ++++++++++++++- > include/scsi/scsi_host.h | 4 ++++ > 2 files changed, 18 insertions(+), 1 deletion(-) > > diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c > index e17153a9ce7c..076459853622 100644 > --- a/drivers/scsi/scsi_lib.c > +++ b/drivers/scsi/scsi_lib.c > @@ -1831,6 +1831,7 @@ struct request_queue *scsi_mq_alloc_queue(struct scsi_device *sdev) > int scsi_mq_setup_tags(struct Scsi_Host *shost) > { > unsigned int cmd_size, sgl_size; > + int ret; > > sgl_size = scsi_mq_inline_sgl_size(shost); > cmd_size = sizeof(struct scsi_cmnd) + shost->hostt->cmd_size + sgl_size; > @@ -1850,11 +1851,23 @@ int scsi_mq_setup_tags(struct Scsi_Host *shost) > BLK_ALLOC_POLICY_TO_MQ_FLAG(shost->hostt->tag_alloc_policy); > shost->tag_set.driver_data = shost; > > - return blk_mq_alloc_tag_set(&shost->tag_set); > + ret = blk_mq_alloc_tag_set(&shost->tag_set); > + if (ret) > + return ret; > + > + if (shost->nr_reserved_cmds && shost->use_reserved_cmd_q) { > + shost->reserved_cmd_q = blk_mq_init_queue(&shost->tag_set); > + if (IS_ERR(shost->reserved_cmd_q)) { > + blk_mq_free_tag_set(&shost->tag_set); > + ret = PTR_ERR(shost->reserved_cmd_q); > + } > + } > + return ret; > } > > void scsi_mq_destroy_tags(struct Scsi_Host *shost) > { > + blk_cleanup_queue(shost->reserved_cmd_q); > blk_mq_free_tag_set(&shost->tag_set); > } > > diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h > index 89998b6bee04..a2bab5f07eff 100644 > --- a/include/scsi/scsi_host.h > +++ b/include/scsi/scsi_host.h > @@ -600,6 +600,7 @@ struct Scsi_Host { > * Number of reserved commands, if any. > */ > unsigned nr_reserved_cmds; > + struct request_queue *reserved_cmd_q; > > unsigned active_mode:2; > unsigned unchecked_isa_dma:1; > @@ -637,6 +638,9 @@ struct Scsi_Host { > /* The transport requires the LUN bits NOT to be stored in CDB[1] */ > unsigned no_scsi2_lun_in_cdb:1; > > + /* Host requires a separate reserved_cmd_q */ > + unsigned use_reserved_cmd_q:1; Is this really required? I would think that a non-zero value for shost->nr_reserved_cmds means the same thing in practice. Thanks, John > + > /* > * Optional work queue to be utilized by the transport > */ >
On 5/30/19 5:28 PM, John Garry wrote: > On 29/05/2019 14:28, Hannes Reinecke wrote: >> From: Hannes Reinecke <hare@suse.com> >> >> Allocate a separate 'reserved_cmd_q' for sending reserved commands. >> >> Signed-off-by: Hannes Reinecke <hare@suse.com> >> --- >> drivers/scsi/scsi_lib.c | 15 ++++++++++++++- >> include/scsi/scsi_host.h | 4 ++++ >> 2 files changed, 18 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c >> index e17153a9ce7c..076459853622 100644 >> --- a/drivers/scsi/scsi_lib.c >> +++ b/drivers/scsi/scsi_lib.c >> @@ -1831,6 +1831,7 @@ struct request_queue *scsi_mq_alloc_queue(struct >> scsi_device *sdev) >> int scsi_mq_setup_tags(struct Scsi_Host *shost) >> { >> unsigned int cmd_size, sgl_size; >> + int ret; >> >> sgl_size = scsi_mq_inline_sgl_size(shost); >> cmd_size = sizeof(struct scsi_cmnd) + shost->hostt->cmd_size + >> sgl_size; >> @@ -1850,11 +1851,23 @@ int scsi_mq_setup_tags(struct Scsi_Host *shost) >> BLK_ALLOC_POLICY_TO_MQ_FLAG(shost->hostt->tag_alloc_policy); >> shost->tag_set.driver_data = shost; >> >> - return blk_mq_alloc_tag_set(&shost->tag_set); >> + ret = blk_mq_alloc_tag_set(&shost->tag_set); >> + if (ret) >> + return ret; >> + >> + if (shost->nr_reserved_cmds && shost->use_reserved_cmd_q) { >> + shost->reserved_cmd_q = blk_mq_init_queue(&shost->tag_set); >> + if (IS_ERR(shost->reserved_cmd_q)) { >> + blk_mq_free_tag_set(&shost->tag_set); >> + ret = PTR_ERR(shost->reserved_cmd_q); >> + } >> + } >> + return ret; >> } >> >> void scsi_mq_destroy_tags(struct Scsi_Host *shost) >> { >> + blk_cleanup_queue(shost->reserved_cmd_q); >> blk_mq_free_tag_set(&shost->tag_set); >> } >> >> diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h >> index 89998b6bee04..a2bab5f07eff 100644 >> --- a/include/scsi/scsi_host.h >> +++ b/include/scsi/scsi_host.h >> @@ -600,6 +600,7 @@ struct Scsi_Host { >> * Number of reserved commands, if any. >> */ >> unsigned nr_reserved_cmds; >> + struct request_queue *reserved_cmd_q; >> >> unsigned active_mode:2; >> unsigned unchecked_isa_dma:1; >> @@ -637,6 +638,9 @@ struct Scsi_Host { >> /* The transport requires the LUN bits NOT to be stored in CDB[1] */ >> unsigned no_scsi2_lun_in_cdb:1; >> >> + /* Host requires a separate reserved_cmd_q */ >> + unsigned use_reserved_cmd_q:1; > > Is this really required? I would think that a non-zero value for > shost->nr_reserved_cmds means the same thing in practice. > ; My implementation actually allows for per-device reserved tags (eg for virtio). But some drivers require to use internal commands prior to any device setup, so they have to use a separate reserved command queue just to be able to allocate tags. So yes, they are required. Cheers, Hannes
On Thu, May 30, 2019 at 07:14:14PM +0200, Hannes Reinecke wrote: >> Is this really required? I would think that a non-zero value for >> shost->nr_reserved_cmds means the same thing in practice. >> ; > My implementation actually allows for per-device reserved tags (eg for > virtio). But some drivers require to use internal commands prior to any > device setup, so they have to use a separate reserved command queue just to > be able to allocate tags. Why would virtio-scsi need per-device reserved commands? It currently uses a global mempool to allocate the reset commands.
On 6/4/19 10:23 AM, Christoph Hellwig wrote: > On Thu, May 30, 2019 at 07:14:14PM +0200, Hannes Reinecke wrote: >>> Is this really required? I would think that a non-zero value for >>> shost->nr_reserved_cmds means the same thing in practice. >>> ; >> My implementation actually allows for per-device reserved tags (eg for >> virtio). But some drivers require to use internal commands prior to any >> device setup, so they have to use a separate reserved command queue just to >> be able to allocate tags. > > Why would virtio-scsi need per-device reserved commands? It currently uses > a global mempool to allocate the reset commands. > Oh, I'm perfectly fine with dropping the per-device reserved commands, and use the host-wide queue in general. It turns out most of the drivers use it that way already. Will be doing so for the next iteration. Cheers, Hannes
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c index e17153a9ce7c..076459853622 100644 --- a/drivers/scsi/scsi_lib.c +++ b/drivers/scsi/scsi_lib.c @@ -1831,6 +1831,7 @@ struct request_queue *scsi_mq_alloc_queue(struct scsi_device *sdev) int scsi_mq_setup_tags(struct Scsi_Host *shost) { unsigned int cmd_size, sgl_size; + int ret; sgl_size = scsi_mq_inline_sgl_size(shost); cmd_size = sizeof(struct scsi_cmnd) + shost->hostt->cmd_size + sgl_size; @@ -1850,11 +1851,23 @@ int scsi_mq_setup_tags(struct Scsi_Host *shost) BLK_ALLOC_POLICY_TO_MQ_FLAG(shost->hostt->tag_alloc_policy); shost->tag_set.driver_data = shost; - return blk_mq_alloc_tag_set(&shost->tag_set); + ret = blk_mq_alloc_tag_set(&shost->tag_set); + if (ret) + return ret; + + if (shost->nr_reserved_cmds && shost->use_reserved_cmd_q) { + shost->reserved_cmd_q = blk_mq_init_queue(&shost->tag_set); + if (IS_ERR(shost->reserved_cmd_q)) { + blk_mq_free_tag_set(&shost->tag_set); + ret = PTR_ERR(shost->reserved_cmd_q); + } + } + return ret; } void scsi_mq_destroy_tags(struct Scsi_Host *shost) { + blk_cleanup_queue(shost->reserved_cmd_q); blk_mq_free_tag_set(&shost->tag_set); } diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h index 89998b6bee04..a2bab5f07eff 100644 --- a/include/scsi/scsi_host.h +++ b/include/scsi/scsi_host.h @@ -600,6 +600,7 @@ struct Scsi_Host { * Number of reserved commands, if any. */ unsigned nr_reserved_cmds; + struct request_queue *reserved_cmd_q; unsigned active_mode:2; unsigned unchecked_isa_dma:1; @@ -637,6 +638,9 @@ struct Scsi_Host { /* The transport requires the LUN bits NOT to be stored in CDB[1] */ unsigned no_scsi2_lun_in_cdb:1; + /* Host requires a separate reserved_cmd_q */ + unsigned use_reserved_cmd_q:1; + /* * Optional work queue to be utilized by the transport */