| Message ID | 1583857550-12049-2-git-send-email-john.garry@huawei.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | scsi: enable reserved commands for LLDDs | expand |
On Wed, Mar 11, 2020 at 12:25:27AM +0800, John Garry wrote: > From: Hannes Reinecke <hare@suse.com> > > Add a new field 'nr_reserved_cmds' to the SCSI host template to > instruct the block layer to set aside a tag space for reserved > commands. > > Signed-off-by: Hannes Reinecke <hare@suse.com> > --- > drivers/scsi/scsi_lib.c | 1 + > include/scsi/scsi_host.h | 6 ++++++ > 2 files changed, 7 insertions(+) > > diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c > index 610ee41fa54c..2967325df7a0 100644 > --- a/drivers/scsi/scsi_lib.c > +++ b/drivers/scsi/scsi_lib.c > @@ -1896,6 +1896,7 @@ int scsi_mq_setup_tags(struct Scsi_Host *shost) > shost->tag_set.ops = &scsi_mq_ops_no_commit; > shost->tag_set.nr_hw_queues = shost->nr_hw_queues ? : 1; > shost->tag_set.queue_depth = shost->can_queue; > + shost->tag_set.reserved_tags = shost->nr_reserved_cmds; You reserve tags for special usage, meantime the whole queue depth isn't increased, that means the tags for IO request is decreased given reserved tags can't be used for IO, so IO performance may be effected. If not the case, please explain a bit in commit log. Thanks, Ming
On 3/11/20 12:08 AM, Ming Lei wrote: > On Wed, Mar 11, 2020 at 12:25:27AM +0800, John Garry wrote: >> From: Hannes Reinecke <hare@suse.com> >> >> Add a new field 'nr_reserved_cmds' to the SCSI host template to >> instruct the block layer to set aside a tag space for reserved >> commands. >> >> Signed-off-by: Hannes Reinecke <hare@suse.com> >> --- >> drivers/scsi/scsi_lib.c | 1 + >> include/scsi/scsi_host.h | 6 ++++++ >> 2 files changed, 7 insertions(+) >> >> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c >> index 610ee41fa54c..2967325df7a0 100644 >> --- a/drivers/scsi/scsi_lib.c >> +++ b/drivers/scsi/scsi_lib.c >> @@ -1896,6 +1896,7 @@ int scsi_mq_setup_tags(struct Scsi_Host *shost) >> shost->tag_set.ops = &scsi_mq_ops_no_commit; >> shost->tag_set.nr_hw_queues = shost->nr_hw_queues ? : 1; >> shost->tag_set.queue_depth = shost->can_queue; >> + shost->tag_set.reserved_tags = shost->nr_reserved_cmds; > > You reserve tags for special usage, meantime the whole queue depth > isn't increased, that means the tags for IO request is decreased given > reserved tags can't be used for IO, so IO performance may be effected. > > If not the case, please explain a bit in commit log. > The overall idea of this patchset is to fold _existing_ management command handling into using the blk-mq bitmap. Currently, quite a lot of drivers are using management commands internally, which typically use the same hardware tag pool (ie they are being allocated from the same hardware resources) than the 'normal' I/O commands. But as they are using the same tagpool these drivers already decrement the available number of commands; check eg. hpsa: static int hpsa_scsi_host_alloc(struct ctlr_info *h) { struct Scsi_Host *sh; sh = scsi_host_alloc(&hpsa_driver_template, sizeof(h)); if (sh == NULL) { dev_err(&h->pdev->dev, "scsi_host_alloc failed\n"); return -ENOMEM; } sh->io_port = 0; sh->n_io_port = 0; sh->this_id = -1; sh->max_channel = 3; sh->max_cmd_len = MAX_COMMAND_SIZE; sh->max_lun = HPSA_MAX_LUN; sh->max_id = HPSA_MAX_LUN; sh->can_queue = h->nr_cmds - HPSA_NRESERVED_CMDS; sh->cmd_per_lun = sh->can_queue; So the idea of this patchset is to convert existing use-cases; seeing that they already reserve commands internally performance will not be affected. Cheers, Hannes
On Wed, Mar 11, 2020 at 07:55:46AM +0100, Hannes Reinecke wrote: > On 3/11/20 12:08 AM, Ming Lei wrote: > > On Wed, Mar 11, 2020 at 12:25:27AM +0800, John Garry wrote: > >> From: Hannes Reinecke <hare@suse.com> > >> > >> Add a new field 'nr_reserved_cmds' to the SCSI host template to > >> instruct the block layer to set aside a tag space for reserved > >> commands. > >> > >> Signed-off-by: Hannes Reinecke <hare@suse.com> > >> --- > >> drivers/scsi/scsi_lib.c | 1 + > >> include/scsi/scsi_host.h | 6 ++++++ > >> 2 files changed, 7 insertions(+) > >> > >> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c > >> index 610ee41fa54c..2967325df7a0 100644 > >> --- a/drivers/scsi/scsi_lib.c > >> +++ b/drivers/scsi/scsi_lib.c > >> @@ -1896,6 +1896,7 @@ int scsi_mq_setup_tags(struct Scsi_Host *shost) > >> shost->tag_set.ops = &scsi_mq_ops_no_commit; > >> shost->tag_set.nr_hw_queues = shost->nr_hw_queues ? : 1; > >> shost->tag_set.queue_depth = shost->can_queue; > >> + shost->tag_set.reserved_tags = shost->nr_reserved_cmds; > > > > You reserve tags for special usage, meantime the whole queue depth > > isn't increased, that means the tags for IO request is decreased given > > reserved tags can't be used for IO, so IO performance may be effected. > > > > If not the case, please explain a bit in commit log. > > > The overall idea of this patchset is to fold _existing_ management > command handling into using the blk-mq bitmap. > Currently, quite a lot of drivers are using management commands > internally, which typically use the same hardware tag pool (ie they are > being allocated from the same hardware resources) than the 'normal' I/O > commands. > But as they are using the same tagpool these drivers already decrement > the available number of commands; check eg. hpsa: > > static int hpsa_scsi_host_alloc(struct ctlr_info *h) > { > struct Scsi_Host *sh; > > sh = scsi_host_alloc(&hpsa_driver_template, sizeof(h)); > if (sh == NULL) { > dev_err(&h->pdev->dev, "scsi_host_alloc failed\n"); > return -ENOMEM; > } > > sh->io_port = 0; > sh->n_io_port = 0; > sh->this_id = -1; > sh->max_channel = 3; > sh->max_cmd_len = MAX_COMMAND_SIZE; > sh->max_lun = HPSA_MAX_LUN; > sh->max_id = HPSA_MAX_LUN; > sh->can_queue = h->nr_cmds - HPSA_NRESERVED_CMDS; > sh->cmd_per_lun = sh->can_queue; > > So the idea of this patchset is to convert existing use-cases; seeing > that they already reserve commands internally performance will not be > affected. > OK, got it. I'd suggest to add comment about this idea in commit log, cause it is one fundamental thing about this change. Thanks, Ming
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c index 610ee41fa54c..2967325df7a0 100644 --- a/drivers/scsi/scsi_lib.c +++ b/drivers/scsi/scsi_lib.c @@ -1896,6 +1896,7 @@ int scsi_mq_setup_tags(struct Scsi_Host *shost) shost->tag_set.ops = &scsi_mq_ops_no_commit; shost->tag_set.nr_hw_queues = shost->nr_hw_queues ? : 1; shost->tag_set.queue_depth = shost->can_queue; + shost->tag_set.reserved_tags = shost->nr_reserved_cmds; shost->tag_set.cmd_size = cmd_size; shost->tag_set.numa_node = NUMA_NO_NODE; shost->tag_set.flags = BLK_MQ_F_SHOULD_MERGE; diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h index f577647bf5f2..3f860c8ad623 100644 --- a/include/scsi/scsi_host.h +++ b/include/scsi/scsi_host.h @@ -599,6 +599,12 @@ struct Scsi_Host { * is nr_hw_queues * can_queue. */ unsigned nr_hw_queues; + + /* + * Number of reserved commands, if any. + */ + unsigned nr_reserved_cmds; + unsigned active_mode:2; unsigned unchecked_isa_dma:1;