[RFC 0/2] scsi: Add poll_queue mechanism

[RFC 0/2] scsi: Add poll_queue mechanism

[Himanshu Madhani]

Hi James/Martin,

This is RFC to get comment about propose poll_queue interface in scsi layer.

The blk_mq layer allows polling a transport for a specific completion if
the HIPRI request flag is set. This can be accomplished by using
"libengine=pvsync2 –hipri" option in FIO tool. This flag then allows
polling into LLDD to service specific compeletion queue for IOs. It also
removes the interrupt latency and significantly improved performance in NVMeF 
that utilizes it. There is expense of higher CPU utlization due to active polling.
    
Here's sample results for comparision
    
FiO command line with synchronous IO’s results in latency avg=32.73us reported by FIO

./fio/fio  --time_based --ioengine=sync --direct=1 --runtime=30 --readwrite=read \
    --iodepth=1 --blocksize=4k --name=job0 --group_reporting --filename=/dev/sdd
    
FIO command line with active polling IO’s results in latency avg=26.04 reported by FIO

./fio/fio  --time_based --ioengine=pvsync2 --hipri --direct=1 --runtime=30 \
    --readwrite=read --iodepth=1 --blocksize=4k --name=job0 --group_reporting \
    --filename=/dev/sdd
    
FIO version 2.17

Thanks,
Himanshu

Darren Trapp (2):
  scsi: Provide mechanism for SCSI layer to poll for LLDD.
  qla2xxx: Driver changes to use new poll_queue() callback

 drivers/scsi/qla2xxx/qla_os.c | 36 ++++++++++++++++++++++++++++++++++++
 drivers/scsi/scsi_lib.c       | 39 +++++++++++++++++++++++++++++++++++++++
 include/scsi/scsi_host.h      | 12 ++++++++++++
 3 files changed, 87 insertions(+)

-- 
2.12.0

[RFC 1/2] scsi: Provide mechanism for SCSI layer to poll for LLDD.

[Himanshu Madhani]

From: Darren Trapp <darren.trapp@cavium.com>

Signed-off-by: Darren Trapp <darren.trapp@cavium.com>
Signed-off-by: Himanshu Madhani <himanshu.madhani@cavium.com>
---
 drivers/scsi/scsi_lib.c  | 39 +++++++++++++++++++++++++++++++++++++++
 include/scsi/scsi_host.h | 12 ++++++++++++
 2 files changed, 51 insertions(+)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 19125d72f322..eb01be039677 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -2154,6 +2154,43 @@ struct request_queue *scsi_alloc_queue(struct scsi_device *sdev)
 	return q;
 }
 
+static int
+scsi_poll(struct blk_mq_hw_ctx *hctx, unsigned int tag)
+
+{
+        struct Scsi_Host *shost = hctx->driver_data;
+        struct request *req;
+        struct scsi_cmnd *cmd;
+
+        req = blk_mq_tag_to_rq(shost->tag_set.tags[hctx->queue_num], tag);
+        if (!req)
+                return 0;
+
+        cmd = blk_mq_rq_to_pdu(req);
+	if (!cmd)
+		return 0;
+
+	if (shost->hostt->poll_queue)
+		return(shost->hostt->poll_queue(shost, cmd));
+	else return 0;
+}
+
+static inline void
+__scsi_init_hctx(struct blk_mq_hw_ctx *hctx, struct Scsi_Host *shost, unsigned int hctx_idx)
+{
+	hctx->driver_data = shost;
+}
+
+static int
+scsi_init_hctx(struct blk_mq_hw_ctx *hctx, void *data, unsigned int hctx_idx)
+{
+	struct Scsi_Host *shost = data;
+
+	__scsi_init_hctx(hctx, shost, hctx_idx);
+
+	return 0;
+}
+
 static struct blk_mq_ops scsi_mq_ops = {
 	.queue_rq	= scsi_queue_rq,
 	.complete	= scsi_softirq_done,
@@ -2161,6 +2198,8 @@ static struct blk_mq_ops scsi_mq_ops = {
 	.init_request	= scsi_init_request,
 	.exit_request	= scsi_exit_request,
 	.map_queues	= scsi_map_queues,
+	.poll		= scsi_poll,
+	.init_hctx	= scsi_init_hctx,
 };
 
 struct request_queue *scsi_mq_alloc_queue(struct scsi_device *sdev)
diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
index 3cd8c3bec638..e87e8a69a0df 100644
--- a/include/scsi/scsi_host.h
+++ b/include/scsi/scsi_host.h
@@ -127,6 +127,18 @@ struct scsi_host_template {
 	int (* queuecommand)(struct Scsi_Host *, struct scsi_cmnd *);
 
 	/*
+	 * The poll_queue function allows the scsi layer to poll a
+	 * LLDD for the completion of a specific scsi_cmnd (upon request
+	 * from blk_mq).
+	 *
+	 * The LLDD returns 1 to indicate it completed the request or 0
+	 * otherwise.
+	 *
+	 * STATUS: OPTIONAL
+	 */
+	int (* poll_queue)(struct Scsi_Host *, struct scsi_cmnd *);
+
+	/*
 	 * This is an error handling strategy routine.  You don't need to
 	 * define one of these if you don't want to - there is a default
 	 * routine that is present that should work in most cases.  For those
-- 
2.12.0

[RFC 2/2] qla2xxx: Driver changes to use new poll_queue() callback

[Himanshu Madhani]

From: Darren Trapp <darren.trapp@cavium.com>

The blk_mq layer allows polling a transport for a specific
completion if the HIPRI request flag is set. This can be
accomplished by using "libengine=pvsync2 –hipri" option
in FIO tool. This flag then allows polling into LLDD to
service specific compeletion queue for IOs. It also removes
the interrupt latency and significantly improved performance
in NVMeF that utilizes it. There is expense of higher CPU
utlization due to active polling.

Here's sample results for comparision

FIO command line with synchronous IO’s results in latency avg=32.73us
reported by FIO

./fio/fio  --time_based --ioengine=sync --direct=1 --runtime=30 --readwrite=read \
  --iodepth=1 --blocksize=4k --name=job0 --group_reporting --filename=/dev/sdd

FIO command line with active polling IO’s results in latency avg=26.04 reported by FIO

./fio/fio  --time_based --ioengine=pvsync2 --hipri --direct=1 --runtime=30 \
  --readwrite=read --iodepth=1 --blocksize=4k --name=job0 --group_reporting \
  --filename=/dev/sdd

FIO version 2.17

Signed-off-by: Darren Trapp <darren.trapp@cavium.com>
Signed-off-by: Himanshu Madhani <himanshu.madhani@cavium.com>
---
 drivers/scsi/qla2xxx/qla_os.c | 36 ++++++++++++++++++++++++++++++++++++
 1 file changed, 36 insertions(+)

diff --git a/drivers/scsi/qla2xxx/qla_os.c b/drivers/scsi/qla2xxx/qla_os.c
index 41d5b09f7326..1f3a113954fd 100644
--- a/drivers/scsi/qla2xxx/qla_os.c
+++ b/drivers/scsi/qla2xxx/qla_os.c
@@ -253,6 +253,7 @@ static int qla2xxx_scan_finished(struct Scsi_Host *, unsigned long time);
 static void qla2xxx_scan_start(struct Scsi_Host *);
 static void qla2xxx_slave_destroy(struct scsi_device *);
 static int qla2xxx_queuecommand(struct Scsi_Host *h, struct scsi_cmnd *cmd);
+static int qla2xxx_poll_queue(struct Scsi_Host *, struct scsi_cmnd *);
 static int qla2xxx_eh_abort(struct scsi_cmnd *);
 static int qla2xxx_eh_device_reset(struct scsi_cmnd *);
 static int qla2xxx_eh_target_reset(struct scsi_cmnd *);
@@ -268,6 +269,7 @@ struct scsi_host_template qla2xxx_driver_template = {
 	.module			= THIS_MODULE,
 	.name			= QLA2XXX_DRIVER_NAME,
 	.queuecommand		= qla2xxx_queuecommand,
+	.poll_queue		= qla2xxx_poll_queue,
 
 	.eh_timed_out		= fc_eh_timed_out,
 	.eh_abort_handler	= qla2xxx_eh_abort,
@@ -973,6 +975,40 @@ qla2xxx_mqueuecommand(struct Scsi_Host *host, struct scsi_cmnd *cmd,
 }
 
 /*
+ * Poll for command to be completed. Initiated by SCSI layer.
+ */
+static int
+qla2xxx_poll_queue(struct Scsi_Host *host, struct scsi_cmnd *cmd)
+{
+	uint32_t tag;
+	uint16_t hwq;
+	struct qla_qpair *qpair;
+	unsigned long flags;
+	scsi_qla_host_t *vha = shost_priv(host);
+	struct qla_hw_data *ha = vha->hw;
+	int ret = 0;
+
+	tag = blk_mq_unique_tag(cmd->request);
+	hwq = blk_mq_unique_tag_to_hwq(tag);
+	qpair = ha->queue_pair_map[hwq];
+
+	/* Acquire  ring specific lock */
+	spin_lock_irqsave(&qpair->qp_lock, flags);
+	if (!CMD_SP(cmd)) {
+		spin_unlock_irqrestore(&qpair->qp_lock, flags);
+		return 1;
+	}
+
+	qla24xx_process_response_queue(vha, qpair->rsp);
+
+	if (!CMD_SP(cmd))
+		ret = 1;
+
+	spin_unlock_irqrestore(&qpair->qp_lock, flags);
+	return ret;
+}
+
+/*
  * qla2x00_eh_wait_on_command
  *    Waits for the command to be returned by the Firmware for some
  *    max time.
-- 
2.12.0

Re: [RFC 1/2] scsi: Provide mechanism for SCSI layer to poll for LLDD.

[Bart Van Assche]

On Thu, 2017-03-16 at 14:40 -0700, Himanshu Madhani wrote:
> +static int
> +scsi_poll(struct blk_mq_hw_ctx *hctx, unsigned int tag)
> +
> +{
> +        struct Scsi_Host *shost = hctx->driver_data;
> +        struct request *req;
> +        struct scsi_cmnd *cmd;
> +
> +        req = blk_mq_tag_to_rq(shost->tag_set.tags[hctx->queue_num], tag);
> +        if (!req)
> +                return 0;
> +
> +        cmd = blk_mq_rq_to_pdu(req);
> +	if (!cmd)
> +		return 0;
> +
> +	if (shost->hostt->poll_queue)
> +		return(shost->hostt->poll_queue(shost, cmd));
> +	else return 0;
> +}

What will happen if the request with tag 'tag' is completed after the block
layer decided to call this function and before this function had the chance
to call blk_mq_tag_to_rq()? Will this trigger a kernel crash? Also, please
format patches properly. This is what checkpatch reports for this patch:

ERROR: code indent should use tabs where possible
#244: FILE: drivers/scsi/scsi_lib.c:2161:
+        struct Scsi_Host *shost = hctx->driver_data;$

WARNING: please, no spaces at the start of a line
#244: FILE: drivers/scsi/scsi_lib.c:2161:
+        struct Scsi_Host *shost = hctx->driver_data;$

ERROR: code indent should use tabs where possible
#245: FILE: drivers/scsi/scsi_lib.c:2162:
+        struct request *req;$

WARNING: please, no spaces at the start of a line
#245: FILE: drivers/scsi/scsi_lib.c:2162:
+        struct request *req;$

ERROR: code indent should use tabs where possible
#246: FILE: drivers/scsi/scsi_lib.c:2163:
+        struct scsi_cmnd *cmd;$

WARNING: please, no spaces at the start of a line
#246: FILE: drivers/scsi/scsi_lib.c:2163:
+        struct scsi_cmnd *cmd;$

ERROR: code indent should use tabs where possible
#248: FILE: drivers/scsi/scsi_lib.c:2165:
+        req = blk_mq_tag_to_rq(shost->tag_set.tags[hctx->queue_num], tag);$

WARNING: please, no spaces at the start of a line
#248: FILE: drivers/scsi/scsi_lib.c:2165:
+        req = blk_mq_tag_to_rq(shost->tag_set.tags[hctx->queue_num], tag);$

ERROR: code indent should use tabs where possible
#249: FILE: drivers/scsi/scsi_lib.c:2166:
+        if (!req)$

WARNING: please, no spaces at the start of a line
#249: FILE: drivers/scsi/scsi_lib.c:2166:
+        if (!req)$

ERROR: code indent should use tabs where possible
#250: FILE: drivers/scsi/scsi_lib.c:2167:
+                return 0;$

WARNING: please, no spaces at the start of a line
#250: FILE: drivers/scsi/scsi_lib.c:2167:
+                return 0;$

ERROR: code indent should use tabs where possible
#252: FILE: drivers/scsi/scsi_lib.c:2169:
+        cmd = blk_mq_rq_to_pdu(req);$

WARNING: please, no spaces at the start of a line
#252: FILE: drivers/scsi/scsi_lib.c:2169:
+        cmd = blk_mq_rq_to_pdu(req);$

ERROR: return is not a function, parentheses are not required
#257: FILE: drivers/scsi/scsi_lib.c:2174:
+		return(shost->hostt->poll_queue(shost, cmd));

ERROR: trailing statements should be on next line
#258: FILE: drivers/scsi/scsi_lib.c:2175:
+	else return 0;

WARNING: line over 80 characters
#262: FILE: drivers/scsi/scsi_lib.c:2179:
+__scsi_init_hctx(struct blk_mq_hw_ctx *hctx, struct Scsi_Host *shost, unsigned int hctx_idx)

WARNING: Unnecessary space before function pointer name
#306: FILE: include/scsi/scsi_host.h:139:
+	int (* poll_queue)(struct Scsi_Host *, struct scsi_cmnd *);

ERROR: space prohibited after that '*' (ctx:BxW)
#306: FILE: include/scsi/scsi_host.h:139:
+	int (* poll_queue)(struct Scsi_Host *, struct scsi_cmnd *);
 	     ^
Thanks,

Bart.

Re: [RFC 1/2] scsi: Provide mechanism for SCSI layer to poll for LLDD.

[Madhani, Himanshu]

> On Mar 16, 2017, at 3:27 PM, Bart Van Assche <bart.vanassche@sandisk.com> wrote:
> 
> On Thu, 2017-03-16 at 14:40 -0700, Himanshu Madhani wrote:
>> +static int
>> +scsi_poll(struct blk_mq_hw_ctx *hctx, unsigned int tag)
>> +
>> +{
>> +        struct Scsi_Host *shost = hctx->driver_data;
>> +        struct request *req;
>> +        struct scsi_cmnd *cmd;
>> +
>> +        req = blk_mq_tag_to_rq(shost->tag_set.tags[hctx->queue_num], tag);
>> +        if (!req)
>> +                return 0;
>> +
>> +        cmd = blk_mq_rq_to_pdu(req);
>> +	if (!cmd)
>> +		return 0;
>> +
>> +	if (shost->hostt->poll_queue)
>> +		return(shost->hostt->poll_queue(shost, cmd));
>> +	else return 0;
>> +}
> 
> What will happen if the request with tag 'tag' is completed after the block
> layer decided to call this function and before this function had the chance
> to call blk_mq_tag_to_rq()? Will this trigger a kernel crash? Also, please
> format patches properly. This is what checkpatch reports for this patch:
> 
> ERROR: code indent should use tabs where possible
> #244: FILE: drivers/scsi/scsi_lib.c:2161:
> +        struct Scsi_Host *shost = hctx->driver_data;$
> 
> WARNING: please, no spaces at the start of a line
> #244: FILE: drivers/scsi/scsi_lib.c:2161:
> +        struct Scsi_Host *shost = hctx->driver_data;$
> 
> ERROR: code indent should use tabs where possible
> #245: FILE: drivers/scsi/scsi_lib.c:2162:
> +        struct request *req;$
> 
> WARNING: please, no spaces at the start of a line
> #245: FILE: drivers/scsi/scsi_lib.c:2162:
> +        struct request *req;$
> 
> ERROR: code indent should use tabs where possible
> #246: FILE: drivers/scsi/scsi_lib.c:2163:
> +        struct scsi_cmnd *cmd;$
> 
> WARNING: please, no spaces at the start of a line
> #246: FILE: drivers/scsi/scsi_lib.c:2163:
> +        struct scsi_cmnd *cmd;$
> 
> ERROR: code indent should use tabs where possible
> #248: FILE: drivers/scsi/scsi_lib.c:2165:
> +        req = blk_mq_tag_to_rq(shost->tag_set.tags[hctx->queue_num], tag);$
> 
> WARNING: please, no spaces at the start of a line
> #248: FILE: drivers/scsi/scsi_lib.c:2165:
> +        req = blk_mq_tag_to_rq(shost->tag_set.tags[hctx->queue_num], tag);$
> 
> ERROR: code indent should use tabs where possible
> #249: FILE: drivers/scsi/scsi_lib.c:2166:
> +        if (!req)$
> 
> WARNING: please, no spaces at the start of a line
> #249: FILE: drivers/scsi/scsi_lib.c:2166:
> +        if (!req)$
> 
> ERROR: code indent should use tabs where possible
> #250: FILE: drivers/scsi/scsi_lib.c:2167:
> +                return 0;$
> 
> WARNING: please, no spaces at the start of a line
> #250: FILE: drivers/scsi/scsi_lib.c:2167:
> +                return 0;$
> 
> ERROR: code indent should use tabs where possible
> #252: FILE: drivers/scsi/scsi_lib.c:2169:
> +        cmd = blk_mq_rq_to_pdu(req);$
> 
> WARNING: please, no spaces at the start of a line
> #252: FILE: drivers/scsi/scsi_lib.c:2169:
> +        cmd = blk_mq_rq_to_pdu(req);$
> 
> ERROR: return is not a function, parentheses are not required
> #257: FILE: drivers/scsi/scsi_lib.c:2174:
> +		return(shost->hostt->poll_queue(shost, cmd));
> 
> ERROR: trailing statements should be on next line
> #258: FILE: drivers/scsi/scsi_lib.c:2175:
> +	else return 0;
> 
> WARNING: line over 80 characters
> #262: FILE: drivers/scsi/scsi_lib.c:2179:
> +__scsi_init_hctx(struct blk_mq_hw_ctx *hctx, struct Scsi_Host *shost, unsigned int hctx_idx)
> 
> WARNING: Unnecessary space before function pointer name
> #306: FILE: include/scsi/scsi_host.h:139:
> +	int (* poll_queue)(struct Scsi_Host *, struct scsi_cmnd *);
> 
> ERROR: space prohibited after that '*' (ctx:BxW)
> #306: FILE: include/scsi/scsi_host.h:139:
> +	int (* poll_queue)(struct Scsi_Host *, struct scsi_cmnd *);
>  	     ^
> Thanks,
> 
> Bart.

We’ll take care of Check patch issue in updated RFC. 

Thanks,
- Himanshu

Re: [RFC 1/2] scsi: Provide mechanism for SCSI layer to poll for LLDD.

[Madhani, Himanshu]

Hi Bart, 

> On Mar 16, 2017, at 3:27 PM, Bart Van Assche <bart.vanassche@sandisk.com> wrote:
> 
> On Thu, 2017-03-16 at 14:40 -0700, Himanshu Madhani wrote:
>> +static int
>> +scsi_poll(struct blk_mq_hw_ctx *hctx, unsigned int tag)
>> +
>> +{
>> +        struct Scsi_Host *shost = hctx->driver_data;
>> +        struct request *req;
>> +        struct scsi_cmnd *cmd;
>> +
>> +        req = blk_mq_tag_to_rq(shost->tag_set.tags[hctx->queue_num], tag);
>> +        if (!req)
>> +                return 0;
>> +
>> +        cmd = blk_mq_rq_to_pdu(req);
>> +	if (!cmd)
>> +		return 0;
>> +
>> +	if (shost->hostt->poll_queue)
>> +		return(shost->hostt->poll_queue(shost, cmd));
>> +	else return 0;
>> +}
> 
> What will happen if the request with tag 'tag' is completed after the block
> layer decided to call this function and before this function had the chance
> to call blk_mq_tag_to_rq()? Will this trigger a kernel crash? Also, please
> format patches properly. This is what checkpatch reports for this patch:
> 

Tag is passed into here by rq->tag

When tag goes to blk_mq_tag_to_rq  it just indexes to the rqs array using tag:

struct request *blk_mq_tag_to_rq(struct blk_mq_tags *tags, unsigned int tag)
{
       if (tag < tags->nr_tags) {
               prefetch(tags->rqs[tag]);
               return tags->rqs[tag];
       }

       return NULL;

So if tag is invalid(too large), it returns NULL.  Every step it validates it has a valid * before continuing.  

Worse case we poll for a completion that has already completed. 

We don’t think this will result into NULL pointer crash.

> ERROR: code indent should use tabs where possible
> #244: FILE: drivers/scsi/scsi_lib.c:2161:
> +        struct Scsi_Host *shost = hctx->driver_data;$
> 
> WARNING: please, no spaces at the start of a line
> #244: FILE: drivers/scsi/scsi_lib.c:2161:
> +        struct Scsi_Host *shost = hctx->driver_data;$
> 
> ERROR: code indent should use tabs where possible
> #245: FILE: drivers/scsi/scsi_lib.c:2162:
> +        struct request *req;$
> 
> WARNING: please, no spaces at the start of a line
> #245: FILE: drivers/scsi/scsi_lib.c:2162:
> +        struct request *req;$
> 
> ERROR: code indent should use tabs where possible
> #246: FILE: drivers/scsi/scsi_lib.c:2163:
> +        struct scsi_cmnd *cmd;$
> 
> WARNING: please, no spaces at the start of a line
> #246: FILE: drivers/scsi/scsi_lib.c:2163:
> +        struct scsi_cmnd *cmd;$
> 
> ERROR: code indent should use tabs where possible
> #248: FILE: drivers/scsi/scsi_lib.c:2165:
> +        req = blk_mq_tag_to_rq(shost->tag_set.tags[hctx->queue_num], tag);$
> 
> WARNING: please, no spaces at the start of a line
> #248: FILE: drivers/scsi/scsi_lib.c:2165:
> +        req = blk_mq_tag_to_rq(shost->tag_set.tags[hctx->queue_num], tag);$
> 
> ERROR: code indent should use tabs where possible
> #249: FILE: drivers/scsi/scsi_lib.c:2166:
> +        if (!req)$
> 
> WARNING: please, no spaces at the start of a line
> #249: FILE: drivers/scsi/scsi_lib.c:2166:
> +        if (!req)$
> 
> ERROR: code indent should use tabs where possible
> #250: FILE: drivers/scsi/scsi_lib.c:2167:
> +                return 0;$
> 
> WARNING: please, no spaces at the start of a line
> #250: FILE: drivers/scsi/scsi_lib.c:2167:
> +                return 0;$
> 
> ERROR: code indent should use tabs where possible
> #252: FILE: drivers/scsi/scsi_lib.c:2169:
> +        cmd = blk_mq_rq_to_pdu(req);$
> 
> WARNING: please, no spaces at the start of a line
> #252: FILE: drivers/scsi/scsi_lib.c:2169:
> +        cmd = blk_mq_rq_to_pdu(req);$
> 
> ERROR: return is not a function, parentheses are not required
> #257: FILE: drivers/scsi/scsi_lib.c:2174:
> +		return(shost->hostt->poll_queue(shost, cmd));
> 
> ERROR: trailing statements should be on next line
> #258: FILE: drivers/scsi/scsi_lib.c:2175:
> +	else return 0;
> 
> WARNING: line over 80 characters
> #262: FILE: drivers/scsi/scsi_lib.c:2179:
> +__scsi_init_hctx(struct blk_mq_hw_ctx *hctx, struct Scsi_Host *shost, unsigned int hctx_idx)
> 
> WARNING: Unnecessary space before function pointer name
> #306: FILE: include/scsi/scsi_host.h:139:
> +	int (* poll_queue)(struct Scsi_Host *, struct scsi_cmnd *);
> 
> ERROR: space prohibited after that '*' (ctx:BxW)
> #306: FILE: include/scsi/scsi_host.h:139:
> +	int (* poll_queue)(struct Scsi_Host *, struct scsi_cmnd *);
>  	     ^
> Thanks,
> 
> Bart.

Thanks,
- Himanshu

Re: [RFC 1/2] scsi: Provide mechanism for SCSI layer to poll for LLDD.

[Bart Van Assche]

On Fri, 2017-03-17 at 18:55 +0000, Madhani, Himanshu wrote:
> On Mar 16, 2017, at 3:27 PM, Bart Van Assche <bart.vanassche@sandisk.com> wrote:
> > On Thu, 2017-03-16 at 14:40 -0700, Himanshu Madhani wrote:
> > > +static int
> > > +scsi_poll(struct blk_mq_hw_ctx *hctx, unsigned int tag)
> > > +
> > > +{
> > > +        struct Scsi_Host *shost = hctx->driver_data;
> > > +        struct request *req;
> > > +        struct scsi_cmnd *cmd;
> > > +
> > > +        req = blk_mq_tag_to_rq(shost->tag_set.tags[hctx->queue_num], tag);
> > > +        if (!req)
> > > +                return 0;
> > > +
> > > +        cmd = blk_mq_rq_to_pdu(req);
> > > +	if (!cmd)
> > > +		return 0;
> > > +
> > > +	if (shost->hostt->poll_queue)
> > > +		return(shost->hostt->poll_queue(shost, cmd));
> > > +	else return 0;
> > > +}
> > 
> > What will happen if the request with tag 'tag' is completed after the block
> > layer decided to call this function and before this function had the chance
> > to call blk_mq_tag_to_rq()? Will this trigger a kernel crash? Also, please
> > format patches properly. This is what checkpatch reports for this patch:
> > 
> 
> Tag is passed into here by rq->tag
> 
> When tag goes to blk_mq_tag_to_rq  it just indexes to the rqs array using tag:
> 
> struct request *blk_mq_tag_to_rq(struct blk_mq_tags *tags, unsigned int tag)
> {
>        if (tag < tags->nr_tags) {
>                prefetch(tags->rqs[tag]);
>                return tags->rqs[tag];
>        }
> 
>        return NULL;
> 
> So if tag is invalid(too large), it returns NULL.  Every step it validates it has a valid * before continuing.  
> 
> Worse case we poll for a completion that has already completed. 
> 
> We don’t think this will result into NULL pointer crash.

Hello Himanshu,

Have you noticed that the caller of scsi_poll(), namely blk_mq_poll(), does
not use any kind of mechanism to prevent that the 'tag' argument is freed and
reused while blk_mq_poll() is in progress? I think that a SCSI completion
can occur while blk_mq_poll() is in progress. What will happen if a SCSI
completion occurs concurrently with scsi_poll()? Will that trigger a
use-after-free of the scsi_cmnd structure 'cmd' points at? If so, what will
be the consequences?

Because of this I'm not sure we should proceed with adding blk_mq_poll()
support to the SCSI core. But if such support is added it's probably a much
better choice to change the type of the second argument of .poll_queue()
from struct scsi_cmnd * into int (the tag number). It is then the
responsibility of SCSI LLD's to avoid that their .poll_queue() implementation
triggers a use-after-free.

Bart.