| Message ID | 1654770559-101375-5-git-send-email-john.garry@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | blk-mq/libata/scsi: SCSI driver tagging improvements | expand |
On 6/9/22 19:29, John Garry wrote: > Add a method to queue reserved commands. > > TODO: > - fix timeout handler to call into reserved commands > - We don't clear host_scribble for libata qc, but we should be able to drop > this if we store libata qc in the scsi cmnd priv_data > > Signed-off-by: John Garry <john.garry@huawei.com> > --- > drivers/scsi/hosts.c | 6 ++++++ > drivers/scsi/scsi_lib.c | 32 ++++++++++++++++++++++++++++++++ > include/scsi/scsi_cmnd.h | 5 +++++ > include/scsi/scsi_host.h | 1 + > 4 files changed, 44 insertions(+) > > diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c > index 27296addaf63..5c9b05a8fec8 100644 > --- a/drivers/scsi/hosts.c > +++ b/drivers/scsi/hosts.c > @@ -221,6 +221,12 @@ int scsi_add_host_with_dma(struct Scsi_Host *shost, struct device *dev, > goto fail; > } > > + if (shost->nr_reserved_cmds && !sht->reserved_queuecommand) { > + shost_printk(KERN_ERR, shost, > + "nr_reserved_cmds set but no method to queue\n"); > + goto fail; This would be a driver implementation bug. So what about a WARN() here ? > + } > + > /* Use min_t(int, ...) in case shost->can_queue exceeds SHRT_MAX */ > shost->cmd_per_lun = min_t(int, shost->cmd_per_lun, > shost->can_queue); > diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c > index f6e53c6d913c..8c8b4c6767d9 100644 > --- a/drivers/scsi/scsi_lib.c > +++ b/drivers/scsi/scsi_lib.c > @@ -1422,6 +1422,16 @@ static void scsi_complete(struct request *rq) > struct scsi_cmnd *cmd = blk_mq_rq_to_pdu(rq); > enum scsi_disposition disposition; > > + if (scsi_is_reserved_cmd(cmd)) { > + struct scsi_device *sdev = cmd->device; > + > + scsi_mq_uninit_cmd(cmd); > + scsi_device_unbusy(sdev, cmd); > + __blk_mq_end_request(rq, 0); > + > + return; > + } > + > INIT_LIST_HEAD(&cmd->eh_entry); > > atomic_inc(&cmd->device->iodone_cnt); > @@ -1706,6 +1716,28 @@ static blk_status_t scsi_queue_rq(struct blk_mq_hw_ctx *hctx, > > WARN_ON_ONCE(cmd->budget_token < 0); > > + if (scsi_is_reserved_cmd(cmd)) { > + unsigned char *host_scribble = cmd->host_scribble; > + > + if (!(req->rq_flags & RQF_DONTPREP)) { > + ret = scsi_prepare_cmd(req); > + if (ret != BLK_STS_OK) { > + Stray blank line. > + goto out_dec_host_busy; > + } No need for the curly brackets here. > + > + req->rq_flags |= RQF_DONTPREP; > + } else { > + clear_bit(SCMD_STATE_COMPLETE, &cmd->state); > + } > + > + cmd->host_scribble = host_scribble; > + > + blk_mq_start_request(req); > + > + return shost->hostt->reserved_queuecommand(shost, cmd); > + } > + > /* > * If the device is not in running state we will reject some or all > * commands. > diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h > index 1e80e70dfa92..e47df5836070 100644 > --- a/include/scsi/scsi_cmnd.h > +++ b/include/scsi/scsi_cmnd.h > @@ -152,6 +152,11 @@ static inline void *scsi_cmd_priv(struct scsi_cmnd *cmd) > return cmd + 1; > } > > +static inline bool scsi_is_reserved_cmd(struct scsi_cmnd *cmd) > +{ > + return blk_mq_is_reserved_rq(scsi_cmd_to_rq(cmd)); > +} > + > void scsi_done(struct scsi_cmnd *cmd); > void scsi_done_direct(struct scsi_cmnd *cmd); > > diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h > index 149dcbd4125e..88c8504395c8 100644 > --- a/include/scsi/scsi_host.h > +++ b/include/scsi/scsi_host.h > @@ -73,6 +73,7 @@ struct scsi_host_template { > * STATUS: REQUIRED > */ > int (* queuecommand)(struct Scsi_Host *, struct scsi_cmnd *); > + int (* reserved_queuecommand)(struct Scsi_Host *, struct scsi_cmnd *); > > /* > * The commit_rqs function is used to trigger a hardware
On 13/06/2022 08:03, Damien Le Moal wrote: >> + if (shost->nr_reserved_cmds && !sht->reserved_queuecommand) { >> + shost_printk(KERN_ERR, shost, >> + "nr_reserved_cmds set but no method to queue\n"); >> + goto fail; > This would be a driver implementation bug. It would be a driver bug, but it probably makes the driver utterly useless and there is no point in continuing (to try to add). If the driver supports reserved commands then they are prob essential to make the driver function. > So what about a WARN() here ? Maybe but I really do not see a point in continuing > >> + } >> + >> /* Use min_t(int, ...) in case shost->can_queue exceeds SHRT_MAX */ >> shost->cmd_per_lun = min_t(int, shost->cmd_per_lun, >> shost->can_queue); >> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c >> index f6e53c6d913c..8c8b4c6767d9 100644 >> --- a/drivers/scsi/scsi_lib.c >> +++ b/drivers/scsi/scsi_lib.c >> @@ -1422,6 +1422,16 @@ static void scsi_complete(struct request *rq) >> struct scsi_cmnd *cmd = blk_mq_rq_to_pdu(rq); >> enum scsi_disposition disposition; >> >> + if (scsi_is_reserved_cmd(cmd)) { >> + struct scsi_device *sdev = cmd->device; >> + >> + scsi_mq_uninit_cmd(cmd); >> + scsi_device_unbusy(sdev, cmd); >> + __blk_mq_end_request(rq, 0); >> + >> + return; >> + } >> + >> INIT_LIST_HEAD(&cmd->eh_entry); >> >> atomic_inc(&cmd->device->iodone_cnt); >> @@ -1706,6 +1716,28 @@ static blk_status_t scsi_queue_rq(struct blk_mq_hw_ctx *hctx, >> >> WARN_ON_ONCE(cmd->budget_token < 0); >> >> + if (scsi_is_reserved_cmd(cmd)) { >> + unsigned char *host_scribble = cmd->host_scribble; >> + >> + if (!(req->rq_flags & RQF_DONTPREP)) { >> + ret = scsi_prepare_cmd(req); >> + if (ret != BLK_STS_OK) { >> + > Stray blank line. ok > >> + goto out_dec_host_busy; >> + } > No need for the curly brackets here. ok > >> + >> + req->rq_flags |= RQF_DONTPREP; >> + } else { >> + clear_bit(SCMD_STATE_COMPLETE, &cmd->state); >> + } Thanks, John
On 6/13/22 18:40, John Garry wrote: > On 13/06/2022 08:03, Damien Le Moal wrote: >>> + if (shost->nr_reserved_cmds && !sht->reserved_queuecommand) { >>> + shost_printk(KERN_ERR, shost, >>> + "nr_reserved_cmds set but no method to queue\n"); >>> + goto fail; >> This would be a driver implementation bug. > > It would be a driver bug, but it probably makes the driver utterly > useless and there is no point in continuing (to try to add). If the > driver supports reserved commands then they are prob essential to make > the driver function. > >> So what about a WARN() here ? > > Maybe but I really do not see a point in continuing Completely agree. My suggestion is to replace the regular shost_printk() error message with something "heavier" like WARN(). Since that is not an expected error. Even a BUG_ON() may be acceptable :) > >> >>> + } >>> + >>> /* Use min_t(int, ...) in case shost->can_queue exceeds SHRT_MAX */ >>> shost->cmd_per_lun = min_t(int, shost->cmd_per_lun, >>> shost->can_queue); >>> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c >>> index f6e53c6d913c..8c8b4c6767d9 100644 >>> --- a/drivers/scsi/scsi_lib.c >>> +++ b/drivers/scsi/scsi_lib.c >>> @@ -1422,6 +1422,16 @@ static void scsi_complete(struct request *rq) >>> struct scsi_cmnd *cmd = blk_mq_rq_to_pdu(rq); >>> enum scsi_disposition disposition; >>> >>> + if (scsi_is_reserved_cmd(cmd)) { >>> + struct scsi_device *sdev = cmd->device; >>> + >>> + scsi_mq_uninit_cmd(cmd); >>> + scsi_device_unbusy(sdev, cmd); >>> + __blk_mq_end_request(rq, 0); >>> + >>> + return; >>> + } >>> + >>> INIT_LIST_HEAD(&cmd->eh_entry); >>> >>> atomic_inc(&cmd->device->iodone_cnt); >>> @@ -1706,6 +1716,28 @@ static blk_status_t scsi_queue_rq(struct blk_mq_hw_ctx *hctx, >>> >>> WARN_ON_ONCE(cmd->budget_token < 0); >>> >>> + if (scsi_is_reserved_cmd(cmd)) { >>> + unsigned char *host_scribble = cmd->host_scribble; >>> + >>> + if (!(req->rq_flags & RQF_DONTPREP)) { >>> + ret = scsi_prepare_cmd(req); >>> + if (ret != BLK_STS_OK) { >>> + >> Stray blank line. > > ok > >> >>> + goto out_dec_host_busy; >>> + } >> No need for the curly brackets here. > > ok > >> >>> + >>> + req->rq_flags |= RQF_DONTPREP; >>> + } else { >>> + clear_bit(SCMD_STATE_COMPLETE, &cmd->state); >>> + } > > Thanks, > John
diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c index 27296addaf63..5c9b05a8fec8 100644 --- a/drivers/scsi/hosts.c +++ b/drivers/scsi/hosts.c @@ -221,6 +221,12 @@ int scsi_add_host_with_dma(struct Scsi_Host *shost, struct device *dev, goto fail; } + if (shost->nr_reserved_cmds && !sht->reserved_queuecommand) { + shost_printk(KERN_ERR, shost, + "nr_reserved_cmds set but no method to queue\n"); + goto fail; + } + /* Use min_t(int, ...) in case shost->can_queue exceeds SHRT_MAX */ shost->cmd_per_lun = min_t(int, shost->cmd_per_lun, shost->can_queue); diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c index f6e53c6d913c..8c8b4c6767d9 100644 --- a/drivers/scsi/scsi_lib.c +++ b/drivers/scsi/scsi_lib.c @@ -1422,6 +1422,16 @@ static void scsi_complete(struct request *rq) struct scsi_cmnd *cmd = blk_mq_rq_to_pdu(rq); enum scsi_disposition disposition; + if (scsi_is_reserved_cmd(cmd)) { + struct scsi_device *sdev = cmd->device; + + scsi_mq_uninit_cmd(cmd); + scsi_device_unbusy(sdev, cmd); + __blk_mq_end_request(rq, 0); + + return; + } + INIT_LIST_HEAD(&cmd->eh_entry); atomic_inc(&cmd->device->iodone_cnt); @@ -1706,6 +1716,28 @@ static blk_status_t scsi_queue_rq(struct blk_mq_hw_ctx *hctx, WARN_ON_ONCE(cmd->budget_token < 0); + if (scsi_is_reserved_cmd(cmd)) { + unsigned char *host_scribble = cmd->host_scribble; + + if (!(req->rq_flags & RQF_DONTPREP)) { + ret = scsi_prepare_cmd(req); + if (ret != BLK_STS_OK) { + + goto out_dec_host_busy; + } + + req->rq_flags |= RQF_DONTPREP; + } else { + clear_bit(SCMD_STATE_COMPLETE, &cmd->state); + } + + cmd->host_scribble = host_scribble; + + blk_mq_start_request(req); + + return shost->hostt->reserved_queuecommand(shost, cmd); + } + /* * If the device is not in running state we will reject some or all * commands. diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h index 1e80e70dfa92..e47df5836070 100644 --- a/include/scsi/scsi_cmnd.h +++ b/include/scsi/scsi_cmnd.h @@ -152,6 +152,11 @@ static inline void *scsi_cmd_priv(struct scsi_cmnd *cmd) return cmd + 1; } +static inline bool scsi_is_reserved_cmd(struct scsi_cmnd *cmd) +{ + return blk_mq_is_reserved_rq(scsi_cmd_to_rq(cmd)); +} + void scsi_done(struct scsi_cmnd *cmd); void scsi_done_direct(struct scsi_cmnd *cmd); diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h index 149dcbd4125e..88c8504395c8 100644 --- a/include/scsi/scsi_host.h +++ b/include/scsi/scsi_host.h @@ -73,6 +73,7 @@ struct scsi_host_template { * STATUS: REQUIRED */ int (* queuecommand)(struct Scsi_Host *, struct scsi_cmnd *); + int (* reserved_queuecommand)(struct Scsi_Host *, struct scsi_cmnd *); /* * The commit_rqs function is used to trigger a hardware
Add a method to queue reserved commands. TODO: - fix timeout handler to call into reserved commands - We don't clear host_scribble for libata qc, but we should be able to drop this if we store libata qc in the scsi cmnd priv_data Signed-off-by: John Garry <john.garry@huawei.com> --- drivers/scsi/hosts.c | 6 ++++++ drivers/scsi/scsi_lib.c | 32 ++++++++++++++++++++++++++++++++ include/scsi/scsi_cmnd.h | 5 +++++ include/scsi/scsi_host.h | 1 + 4 files changed, 44 insertions(+)