| Message ID | 20200430131904.5847-4-hare@suse.de (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | scsi: enable reserved commands for LLDDs | expand |
On 2020-04-30 06:18, Hannes Reinecke wrote: > void scsi_put_reserved_cmd(struct scsi_cmnd *scmd) > { > - struct request *rq = blk_mq_rq_from_pdu(scmd); > + struct request *rq; > > - blk_mq_free_request(rq); > + if (scmd && scsi_cmd_is_reserved(scmd)) { > + rq = blk_mq_rq_from_pdu(scmd); > + blk_mq_free_request(rq); > + } > } The above looks weird to me. Why to tolerate that a caller passes NULL as argument to this function? Additionally, wouldn't a WARN_ON_ONCE(!scsi_cmd_is_reserved(scmd)) be more appropriate instead of the if (scsi_cmd_is_reserved(scmd)) { ... } ? Thanks, Bart.
On Thu, Apr 30, 2020 at 03:18:26PM +0200, Hannes Reinecke wrote: > Add function to check if a SCSI command originates from the reserved > tag pool and update scsi_put_reserved_cmd() to only free commands if > they originate from the reserved tag pool. The SCSI bits should go into the previous patch. The block layer bits should be a separate prep patch before that. > +/** > + * blk_mq_rq_is_reserved - Check for reserved request > + * > + * @rq: Request to check No empty line before the parameter description, please. > */ > void scsi_put_reserved_cmd(struct scsi_cmnd *scmd) > { > + struct request *rq; > > + if (scmd && scsi_cmd_is_reserved(scmd)) { > + rq = blk_mq_rq_from_pdu(scmd); > + blk_mq_free_request(rq); > + } The check looks weird. Passing a NULL cmnd here seems like an API abuse to start with, and !scsi_cmd_is_reserved should at best be a WARN_ON_ONCE. So I think this should just be something like: void scsi_put_reserved_cmd(struct scsi_cmnd *scmd) { WARN_ON_ONCE(!scsi_cmd_is_reserved(scmd)); blk_mq_free_request(blk_mq_rq_from_pdu(scmd)); } > +/** > + * scsi_cmd_is_reserved - check for reserved scsi command > + * @scmd: command to check > + * > + * Returns true if @scmd originates from the reserved tag pool. > + */ > +static inline bool scsi_cmd_is_reserved(struct scsi_cmnd *scmd) > +{ > + struct request *rq = blk_mq_rq_from_pdu(scmd); > + > + return blk_mq_rq_is_reserved(rq); Can be shortened to: return blk_mq_rq_is_reserved(blk_mq_rq_from_pdu(scmd));
On 5/1/20 7:43 PM, Christoph Hellwig wrote: > On Thu, Apr 30, 2020 at 03:18:26PM +0200, Hannes Reinecke wrote: >> Add function to check if a SCSI command originates from the reserved >> tag pool and update scsi_put_reserved_cmd() to only free commands if >> they originate from the reserved tag pool. > > The SCSI bits should go into the previous patch. The block layer > bits should be a separate prep patch before that. > Okay. >> +/** >> + * blk_mq_rq_is_reserved - Check for reserved request >> + * >> + * @rq: Request to check > > No empty line before the parameter description, please. > Okay >> */ >> void scsi_put_reserved_cmd(struct scsi_cmnd *scmd) >> { >> + struct request *rq; >> >> + if (scmd && scsi_cmd_is_reserved(scmd)) { >> + rq = blk_mq_rq_from_pdu(scmd); >> + blk_mq_free_request(rq); >> + } > > The check looks weird. Passing a NULL cmnd here seems like an API > abuse to start with, and !scsi_cmd_is_reserved should at best be > a WARN_ON_ONCE. > > So I think this should just be something like: > > void scsi_put_reserved_cmd(struct scsi_cmnd *scmd) > { > WARN_ON_ONCE(!scsi_cmd_is_reserved(scmd)); > blk_mq_free_request(blk_mq_rq_from_pdu(scmd)); > } > Will do. >> +/** >> + * scsi_cmd_is_reserved - check for reserved scsi command >> + * @scmd: command to check >> + * >> + * Returns true if @scmd originates from the reserved tag pool. >> + */ >> +static inline bool scsi_cmd_is_reserved(struct scsi_cmnd *scmd) >> +{ >> + struct request *rq = blk_mq_rq_from_pdu(scmd); >> + >> + return blk_mq_rq_is_reserved(rq); > > Can be shortened to: > > return blk_mq_rq_is_reserved(blk_mq_rq_from_pdu(scmd)); > Same here. Will be updating the patch. Cheers, Hannes
diff --git a/block/blk-mq.c b/block/blk-mq.c index 8e56884fd2e9..44482aaed11e 100644 --- a/block/blk-mq.c +++ b/block/blk-mq.c @@ -604,6 +604,21 @@ static void __blk_mq_complete_request(struct request *rq) put_cpu(); } +/** + * blk_mq_rq_is_reserved - Check for reserved request + * + * @rq: Request to check + * + * Returns true if the request originates from the reserved tag pool. + */ +bool blk_mq_rq_is_reserved(struct request *rq) +{ + struct blk_mq_hw_ctx *hctx = rq->mq_hctx; + + return blk_mq_tag_is_reserved(hctx->tags, rq->tag); +} +EXPORT_SYMBOL_GPL(blk_mq_rq_is_reserved); + static void hctx_unlock(struct blk_mq_hw_ctx *hctx, int srcu_idx) __releases(hctx->srcu) { diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c index d0972744ea7f..30f9ca9fce22 100644 --- a/drivers/scsi/scsi_lib.c +++ b/drivers/scsi/scsi_lib.c @@ -1930,9 +1930,12 @@ EXPORT_SYMBOL_GPL(scsi_get_reserved_cmd); */ void scsi_put_reserved_cmd(struct scsi_cmnd *scmd) { - struct request *rq = blk_mq_rq_from_pdu(scmd); + struct request *rq; - blk_mq_free_request(rq); + if (scmd && scsi_cmd_is_reserved(scmd)) { + rq = blk_mq_rq_from_pdu(scmd); + blk_mq_free_request(rq); + } } EXPORT_SYMBOL_GPL(scsi_put_reserved_cmd); diff --git a/include/linux/blk-mq.h b/include/linux/blk-mq.h index f389d7c724bd..c186dc25fc1c 100644 --- a/include/linux/blk-mq.h +++ b/include/linux/blk-mq.h @@ -494,6 +494,7 @@ void blk_mq_requeue_request(struct request *rq, bool kick_requeue_list); void blk_mq_kick_requeue_list(struct request_queue *q); void blk_mq_delay_kick_requeue_list(struct request_queue *q, unsigned long msecs); bool blk_mq_complete_request(struct request *rq); +bool blk_mq_rq_is_reserved(struct request *rq); bool blk_mq_bio_list_merge(struct request_queue *q, struct list_head *list, struct bio *bio, unsigned int nr_segs); bool blk_mq_queue_stopped(struct request_queue *q); diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h index 80ac89e47b47..2cd894fbdcfa 100644 --- a/include/scsi/scsi_cmnd.h +++ b/include/scsi/scsi_cmnd.h @@ -159,6 +159,19 @@ static inline struct scsi_driver *scsi_cmd_to_driver(struct scsi_cmnd *cmd) return *(struct scsi_driver **)cmd->request->rq_disk->private_data; } +/** + * scsi_cmd_is_reserved - check for reserved scsi command + * @scmd: command to check + * + * Returns true if @scmd originates from the reserved tag pool. + */ +static inline bool scsi_cmd_is_reserved(struct scsi_cmnd *scmd) +{ + struct request *rq = blk_mq_rq_from_pdu(scmd); + + return blk_mq_rq_is_reserved(rq); +} + extern void scsi_finish_command(struct scsi_cmnd *cmd); extern void *scsi_kmap_atomic_sg(struct scatterlist *sg, int sg_count,
Add function to check if a SCSI command originates from the reserved tag pool and update scsi_put_reserved_cmd() to only free commands if they originate from the reserved tag pool. Signed-off-by: Hannes Reinecke <hare@suse.com> --- block/blk-mq.c | 15 +++++++++++++++ drivers/scsi/scsi_lib.c | 7 +++++-- include/linux/blk-mq.h | 1 + include/scsi/scsi_cmnd.h | 13 +++++++++++++ 4 files changed, 34 insertions(+), 2 deletions(-)