[PATCHSET v4 0/4] Add support for list issue

[PATCHSET v4 0/4] Add support for list issue

[Jens Axboe]

With the support in 5.16-rc1 for allocating and completing batches of
IO, the one missing piece is passing down a list of requests for issue.
Drivers can take advantage of this by defining an mq_ops->queue_rqs()
hook.

This implements it for NVMe, allowing copy of multiple commands in one
swoop.

This is good for around a 500K IOPS/core improvement in my testing,
which is around a 5-6% improvement in efficiency.

Note to Christoph - I kept the copy helper, since it's used in 3
spots and I _think_ you ended up being fine with that...

Changes since v3:
- Use nvme_sq_copy_cmd() in nvme_submit_cmds()
- Add reviewed-by's

-- 
Jens Axboe

[PATCH 1/4] block: add mq_ops->queue_rqs hook

[Jens Axboe]

If we have a list of requests in our plug list, send it to the driver in
one go, if possible. The driver must set mq_ops->queue_rqs() to support
this, if not the usual one-by-one path is used.

Reviewed-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Jens Axboe <axboe@kernel.dk>
---
 block/blk-mq.c         | 26 +++++++++++++++++++++++---
 include/linux/blk-mq.h |  8 ++++++++
 2 files changed, 31 insertions(+), 3 deletions(-)

diff --git a/block/blk-mq.c b/block/blk-mq.c
index 75154cc788db..51991232824a 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -2553,6 +2553,7 @@ void blk_mq_flush_plug_list(struct blk_plug *plug, bool from_schedule)
 {
 	struct blk_mq_hw_ctx *this_hctx;
 	struct blk_mq_ctx *this_ctx;
+	struct request *rq;
 	unsigned int depth;
 	LIST_HEAD(list);
 
@@ -2561,7 +2562,28 @@ void blk_mq_flush_plug_list(struct blk_plug *plug, bool from_schedule)
 	plug->rq_count = 0;
 
 	if (!plug->multiple_queues && !plug->has_elevator && !from_schedule) {
-		struct request_queue *q = rq_list_peek(&plug->mq_list)->q;
+		struct request_queue *q;
+
+		rq = rq_list_peek(&plug->mq_list);
+		q = rq->q;
+
+		/*
+		 * Peek first request and see if we have a ->queue_rqs() hook.
+		 * If we do, we can dispatch the whole plug list in one go. We
+		 * already know at this point that all requests belong to the
+		 * same queue, caller must ensure that's the case.
+		 *
+		 * Since we pass off the full list to the driver at this point,
+		 * we do not increment the active request count for the queue.
+		 * Bypass shared tags for now because of that.
+		 */
+		if (q->mq_ops->queue_rqs &&
+		    !(rq->mq_hctx->flags & BLK_MQ_F_TAG_QUEUE_SHARED)) {
+			blk_mq_run_dispatch_ops(q,
+				q->mq_ops->queue_rqs(&plug->mq_list));
+			if (rq_list_empty(plug->mq_list))
+				return;
+		}
 
 		blk_mq_run_dispatch_ops(q,
 				blk_mq_plug_issue_direct(plug, false));
@@ -2573,8 +2595,6 @@ void blk_mq_flush_plug_list(struct blk_plug *plug, bool from_schedule)
 	this_ctx = NULL;
 	depth = 0;
 	do {
-		struct request *rq;
-
 		rq = rq_list_pop(&plug->mq_list);
 
 		if (!this_hctx) {
diff --git a/include/linux/blk-mq.h b/include/linux/blk-mq.h
index 772f8f921526..550996cf419c 100644
--- a/include/linux/blk-mq.h
+++ b/include/linux/blk-mq.h
@@ -492,6 +492,14 @@ struct blk_mq_ops {
 	 */
 	void (*commit_rqs)(struct blk_mq_hw_ctx *);
 
+	/**
+	 * @queue_rqs: Queue a list of new requests. Driver is guaranteed
+	 * that each request belongs to the same queue. If the driver doesn't
+	 * empty the @rqlist completely, then the rest will be queued
+	 * individually by the block layer upon return.
+	 */
+	void (*queue_rqs)(struct request **rqlist);
+
 	/**
 	 * @get_budget: Reserve budget before queue request, once .queue_rq is
 	 * run, it is driver's responsibility to release the
-- 
2.34.1

[PATCH 2/4] nvme: split command copy into a helper

[Jens Axboe]

We'll need it for batched submit as well.

Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>
Reviewed-by: Hannes Reinecke <hare@suse.de>
Reviewed-by: Max Gurtovoy <mgurtovoy@nvidia.com>
Signed-off-by: Jens Axboe <axboe@kernel.dk>
---
 drivers/nvme/host/pci.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 8637538f3fd5..09ea21f75439 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -500,6 +500,15 @@ static inline void nvme_write_sq_db(struct nvme_queue *nvmeq, bool write_sq)
 	nvmeq->last_sq_tail = nvmeq->sq_tail;
 }
 
+static inline void nvme_sq_copy_cmd(struct nvme_queue *nvmeq,
+				    struct nvme_command *cmd)
+{
+	memcpy(nvmeq->sq_cmds + (nvmeq->sq_tail << nvmeq->sqes), cmd,
+		sizeof(*cmd));
+	if (++nvmeq->sq_tail == nvmeq->q_depth)
+		nvmeq->sq_tail = 0;
+}
+
 /**
  * nvme_submit_cmd() - Copy a command into a queue and ring the doorbell
  * @nvmeq: The queue to use
@@ -510,10 +519,7 @@ static void nvme_submit_cmd(struct nvme_queue *nvmeq, struct nvme_command *cmd,
 			    bool write_sq)
 {
 	spin_lock(&nvmeq->sq_lock);
-	memcpy(nvmeq->sq_cmds + (nvmeq->sq_tail << nvmeq->sqes),
-	       cmd, sizeof(*cmd));
-	if (++nvmeq->sq_tail == nvmeq->q_depth)
-		nvmeq->sq_tail = 0;
+	nvme_sq_copy_cmd(nvmeq, cmd);
 	nvme_write_sq_db(nvmeq, write_sq);
 	spin_unlock(&nvmeq->sq_lock);
 }
-- 
2.34.1

[PATCH 3/4] nvme: separate command prep and issue

[Jens Axboe]

Add a nvme_prep_rq() helper to setup a command, and nvme_queue_rq() is
adapted to use this helper.

Reviewed-by: Hannes Reinecke <hare@suse.de>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Jens Axboe <axboe@kernel.dk>
---
 drivers/nvme/host/pci.c | 57 ++++++++++++++++++++++++-----------------
 1 file changed, 33 insertions(+), 24 deletions(-)

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 09ea21f75439..6be6b1ab4285 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -918,52 +918,32 @@ static blk_status_t nvme_map_metadata(struct nvme_dev *dev, struct request *req,
 	return BLK_STS_OK;
 }
 
-/*
- * NOTE: ns is NULL when called on the admin queue.
- */
-static blk_status_t nvme_queue_rq(struct blk_mq_hw_ctx *hctx,
-			 const struct blk_mq_queue_data *bd)
+static blk_status_t nvme_prep_rq(struct nvme_dev *dev, struct request *req)
 {
-	struct nvme_ns *ns = hctx->queue->queuedata;
-	struct nvme_queue *nvmeq = hctx->driver_data;
-	struct nvme_dev *dev = nvmeq->dev;
-	struct request *req = bd->rq;
 	struct nvme_iod *iod = blk_mq_rq_to_pdu(req);
-	struct nvme_command *cmnd = &iod->cmd;
 	blk_status_t ret;
 
 	iod->aborted = 0;
 	iod->npages = -1;
 	iod->nents = 0;
 
-	/*
-	 * We should not need to do this, but we're still using this to
-	 * ensure we can drain requests on a dying queue.
-	 */
-	if (unlikely(!test_bit(NVMEQ_ENABLED, &nvmeq->flags)))
-		return BLK_STS_IOERR;
-
-	if (!nvme_check_ready(&dev->ctrl, req, true))
-		return nvme_fail_nonready_command(&dev->ctrl, req);
-
-	ret = nvme_setup_cmd(ns, req);
+	ret = nvme_setup_cmd(req->q->queuedata, req);
 	if (ret)
 		return ret;
 
 	if (blk_rq_nr_phys_segments(req)) {
-		ret = nvme_map_data(dev, req, cmnd);
+		ret = nvme_map_data(dev, req, &iod->cmd);
 		if (ret)
 			goto out_free_cmd;
 	}
 
 	if (blk_integrity_rq(req)) {
-		ret = nvme_map_metadata(dev, req, cmnd);
+		ret = nvme_map_metadata(dev, req, &iod->cmd);
 		if (ret)
 			goto out_unmap_data;
 	}
 
 	blk_mq_start_request(req);
-	nvme_submit_cmd(nvmeq, cmnd, bd->last);
 	return BLK_STS_OK;
 out_unmap_data:
 	nvme_unmap_data(dev, req);
@@ -972,6 +952,35 @@ static blk_status_t nvme_queue_rq(struct blk_mq_hw_ctx *hctx,
 	return ret;
 }
 
+/*
+ * NOTE: ns is NULL when called on the admin queue.
+ */
+static blk_status_t nvme_queue_rq(struct blk_mq_hw_ctx *hctx,
+			 const struct blk_mq_queue_data *bd)
+{
+	struct nvme_queue *nvmeq = hctx->driver_data;
+	struct nvme_dev *dev = nvmeq->dev;
+	struct request *req = bd->rq;
+	struct nvme_iod *iod = blk_mq_rq_to_pdu(req);
+	blk_status_t ret;
+
+	/*
+	 * We should not need to do this, but we're still using this to
+	 * ensure we can drain requests on a dying queue.
+	 */
+	if (unlikely(!test_bit(NVMEQ_ENABLED, &nvmeq->flags)))
+		return BLK_STS_IOERR;
+
+	if (unlikely(!nvme_check_ready(&dev->ctrl, req, true)))
+		return nvme_fail_nonready_command(&dev->ctrl, req);
+
+	ret = nvme_prep_rq(dev, req);
+	if (unlikely(ret))
+		return ret;
+	nvme_submit_cmd(nvmeq, &iod->cmd, bd->last);
+	return BLK_STS_OK;
+}
+
 static __always_inline void nvme_pci_unmap_rq(struct request *req)
 {
 	struct nvme_iod *iod = blk_mq_rq_to_pdu(req);
-- 
2.34.1

[PATCH 4/4] nvme: add support for mq_ops->queue_rqs()

[Jens Axboe]

This enables the block layer to send us a full plug list of requests
that need submitting. The block layer guarantees that they all belong
to the same queue, but we do have to check the hardware queue mapping
for each request.

If errors are encountered, leave them in the passed in list. Then the
block layer will handle them individually.

This is good for about a 4% improvement in peak performance, taking us
from 9.6M to 10M IOPS/core.

Reviewed-by: Hannes Reinecke <hare@suse.de>
Reviewed-by: Keith Busch <kbusch@kernel.org>
Signed-off-by: Jens Axboe <axboe@kernel.dk>
---
 drivers/nvme/host/pci.c | 58 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 58 insertions(+)

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 6be6b1ab4285..e34ad67c4c41 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -981,6 +981,63 @@ static blk_status_t nvme_queue_rq(struct blk_mq_hw_ctx *hctx,
 	return BLK_STS_OK;
 }
 
+static void nvme_submit_cmds(struct nvme_queue *nvmeq, struct request **rqlist)
+{
+	spin_lock(&nvmeq->sq_lock);
+	while (!rq_list_empty(*rqlist)) {
+		struct request *req = rq_list_pop(rqlist);
+		struct nvme_iod *iod = blk_mq_rq_to_pdu(req);
+
+		nvme_sq_copy_cmd(nvmeq, &iod->cmd);
+	}
+	nvme_write_sq_db(nvmeq, true);
+	spin_unlock(&nvmeq->sq_lock);
+}
+
+static bool nvme_prep_rq_batch(struct nvme_queue *nvmeq, struct request *req)
+{
+	/*
+	 * We should not need to do this, but we're still using this to
+	 * ensure we can drain requests on a dying queue.
+	 */
+	if (unlikely(!test_bit(NVMEQ_ENABLED, &nvmeq->flags)))
+		return false;
+	if (unlikely(!nvme_check_ready(&nvmeq->dev->ctrl, req, true)))
+		return false;
+
+	req->mq_hctx->tags->rqs[req->tag] = req;
+	return nvme_prep_rq(nvmeq->dev, req) == BLK_STS_OK;
+}
+
+static void nvme_queue_rqs(struct request **rqlist)
+{
+	struct request *req = rq_list_peek(rqlist), *prev = NULL;
+	struct request *requeue_list = NULL;
+
+	do {
+		struct nvme_queue *nvmeq = req->mq_hctx->driver_data;
+
+		if (!nvme_prep_rq_batch(nvmeq, req)) {
+			/* detach 'req' and add to remainder list */
+			if (prev)
+				prev->rq_next = req->rq_next;
+			rq_list_add(&requeue_list, req);
+		} else {
+			prev = req;
+		}
+
+		req = rq_list_next(req);
+		if (!req || (prev && req->mq_hctx != prev->mq_hctx)) {
+			/* detach rest of list, and submit */
+			prev->rq_next = NULL;
+			nvme_submit_cmds(nvmeq, rqlist);
+			*rqlist = req;
+		}
+	} while (req);
+
+	*rqlist = requeue_list;
+}
+
 static __always_inline void nvme_pci_unmap_rq(struct request *req)
 {
 	struct nvme_iod *iod = blk_mq_rq_to_pdu(req);
@@ -1678,6 +1735,7 @@ static const struct blk_mq_ops nvme_mq_admin_ops = {
 
 static const struct blk_mq_ops nvme_mq_ops = {
 	.queue_rq	= nvme_queue_rq,
+	.queue_rqs	= nvme_queue_rqs,
 	.complete	= nvme_pci_complete_rq,
 	.commit_rqs	= nvme_commit_rqs,
 	.init_hctx	= nvme_init_hctx,
-- 
2.34.1

[PATCH v2 4/4] nvme: add support for mq_ops->queue_rqs()

[Jens Axboe]

This enables the block layer to send us a full plug list of requests
that need submitting. The block layer guarantees that they all belong
to the same queue, but we do have to check the hardware queue mapping
for each request.

If errors are encountered, leave them in the passed in list. Then the
block layer will handle them individually.

This is good for about a 4% improvement in peak performance, taking us
from 9.6M to 10M IOPS/core.

Reviewed-by: Hannes Reinecke <hare@suse.de>
Reviewed-by: Keith Busch <kbusch@kernel.org>
Signed-off-by: Jens Axboe <axboe@kernel.dk>

---

Added the prev check suggested by Max.

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 6be6b1ab4285..58d97660374a 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -981,6 +981,64 @@ static blk_status_t nvme_queue_rq(struct blk_mq_hw_ctx *hctx,
 	return BLK_STS_OK;
 }
 
+static void nvme_submit_cmds(struct nvme_queue *nvmeq, struct request **rqlist)
+{
+	spin_lock(&nvmeq->sq_lock);
+	while (!rq_list_empty(*rqlist)) {
+		struct request *req = rq_list_pop(rqlist);
+		struct nvme_iod *iod = blk_mq_rq_to_pdu(req);
+
+		nvme_sq_copy_cmd(nvmeq, &iod->cmd);
+	}
+	nvme_write_sq_db(nvmeq, true);
+	spin_unlock(&nvmeq->sq_lock);
+}
+
+static bool nvme_prep_rq_batch(struct nvme_queue *nvmeq, struct request *req)
+{
+	/*
+	 * We should not need to do this, but we're still using this to
+	 * ensure we can drain requests on a dying queue.
+	 */
+	if (unlikely(!test_bit(NVMEQ_ENABLED, &nvmeq->flags)))
+		return false;
+	if (unlikely(!nvme_check_ready(&nvmeq->dev->ctrl, req, true)))
+		return false;
+
+	req->mq_hctx->tags->rqs[req->tag] = req;
+	return nvme_prep_rq(nvmeq->dev, req) == BLK_STS_OK;
+}
+
+static void nvme_queue_rqs(struct request **rqlist)
+{
+	struct request *req = rq_list_peek(rqlist), *prev = NULL;
+	struct request *requeue_list = NULL;
+
+	do {
+		struct nvme_queue *nvmeq = req->mq_hctx->driver_data;
+
+		if (!nvme_prep_rq_batch(nvmeq, req)) {
+			/* detach 'req' and add to remainder list */
+			if (prev)
+				prev->rq_next = req->rq_next;
+			rq_list_add(&requeue_list, req);
+		} else {
+			prev = req;
+		}
+
+		req = rq_list_next(req);
+		if (!req || (prev && req->mq_hctx != prev->mq_hctx)) {
+			/* detach rest of list, and submit */
+			if (prev)
+				prev->rq_next = NULL;
+			nvme_submit_cmds(nvmeq, rqlist);
+			*rqlist = req;
+		}
+	} while (req);
+
+	*rqlist = requeue_list;
+}
+
 static __always_inline void nvme_pci_unmap_rq(struct request *req)
 {
 	struct nvme_iod *iod = blk_mq_rq_to_pdu(req);
@@ -1678,6 +1736,7 @@ static const struct blk_mq_ops nvme_mq_admin_ops = {
 
 static const struct blk_mq_ops nvme_mq_ops = {
 	.queue_rq	= nvme_queue_rq,
+	.queue_rqs	= nvme_queue_rqs,
 	.complete	= nvme_pci_complete_rq,
 	.commit_rqs	= nvme_commit_rqs,
 	.init_hctx	= nvme_init_hctx,

-- 
Jens Axboe

[PATCH 1/4] block: add mq_ops->queue_rqs hook

[Jens Axboe]

If we have a list of requests in our plug list, send it to the driver in
one go, if possible. The driver must set mq_ops->queue_rqs() to support
this, if not the usual one-by-one path is used.

Reviewed-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Jens Axboe <axboe@kernel.dk>
---
 block/blk-mq.c         | 26 +++++++++++++++++++++++---
 include/linux/blk-mq.h |  8 ++++++++
 2 files changed, 31 insertions(+), 3 deletions(-)

diff --git a/block/blk-mq.c b/block/blk-mq.c
index 75154cc788db..51991232824a 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -2553,6 +2553,7 @@ void blk_mq_flush_plug_list(struct blk_plug *plug, bool from_schedule)
 {
 	struct blk_mq_hw_ctx *this_hctx;
 	struct blk_mq_ctx *this_ctx;
+	struct request *rq;
 	unsigned int depth;
 	LIST_HEAD(list);
 
@@ -2561,7 +2562,28 @@ void blk_mq_flush_plug_list(struct blk_plug *plug, bool from_schedule)
 	plug->rq_count = 0;
 
 	if (!plug->multiple_queues && !plug->has_elevator && !from_schedule) {
-		struct request_queue *q = rq_list_peek(&plug->mq_list)->q;
+		struct request_queue *q;
+
+		rq = rq_list_peek(&plug->mq_list);
+		q = rq->q;
+
+		/*
+		 * Peek first request and see if we have a ->queue_rqs() hook.
+		 * If we do, we can dispatch the whole plug list in one go. We
+		 * already know at this point that all requests belong to the
+		 * same queue, caller must ensure that's the case.
+		 *
+		 * Since we pass off the full list to the driver at this point,
+		 * we do not increment the active request count for the queue.
+		 * Bypass shared tags for now because of that.
+		 */
+		if (q->mq_ops->queue_rqs &&
+		    !(rq->mq_hctx->flags & BLK_MQ_F_TAG_QUEUE_SHARED)) {
+			blk_mq_run_dispatch_ops(q,
+				q->mq_ops->queue_rqs(&plug->mq_list));
+			if (rq_list_empty(plug->mq_list))
+				return;
+		}
 
 		blk_mq_run_dispatch_ops(q,
 				blk_mq_plug_issue_direct(plug, false));
@@ -2573,8 +2595,6 @@ void blk_mq_flush_plug_list(struct blk_plug *plug, bool from_schedule)
 	this_ctx = NULL;
 	depth = 0;
 	do {
-		struct request *rq;
-
 		rq = rq_list_pop(&plug->mq_list);
 
 		if (!this_hctx) {
diff --git a/include/linux/blk-mq.h b/include/linux/blk-mq.h
index 772f8f921526..550996cf419c 100644
--- a/include/linux/blk-mq.h
+++ b/include/linux/blk-mq.h
@@ -492,6 +492,14 @@ struct blk_mq_ops {
 	 */
 	void (*commit_rqs)(struct blk_mq_hw_ctx *);
 
+	/**
+	 * @queue_rqs: Queue a list of new requests. Driver is guaranteed
+	 * that each request belongs to the same queue. If the driver doesn't
+	 * empty the @rqlist completely, then the rest will be queued
+	 * individually by the block layer upon return.
+	 */
+	void (*queue_rqs)(struct request **rqlist);
+
 	/**
 	 * @get_budget: Reserve budget before queue request, once .queue_rq is
 	 * run, it is driver's responsibility to release the
-- 
2.34.1

Re: [PATCH 1/4] block: add mq_ops->queue_rqs hook

[Jens Axboe]

On 12/5/21 2:07 AM, Hannes Reinecke wrote:
> On 12/4/21 9:13 PM, Jens Axboe wrote:
>> On 12/4/21 3:43 AM, Hannes Reinecke wrote:
>>> On 12/3/21 10:45 PM, Jens Axboe wrote:
>>>> If we have a list of requests in our plug list, send it to the driver in
>>>> one go, if possible. The driver must set mq_ops->queue_rqs() to support
>>>> this, if not the usual one-by-one path is used.
>>>>
>>>> Signed-off-by: Jens Axboe <axboe@kernel.dk>
>>>> ---
>>>>    block/blk-mq.c         | 24 +++++++++++++++++++++---
>>>>    include/linux/blk-mq.h |  8 ++++++++
>>>>    2 files changed, 29 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/block/blk-mq.c b/block/blk-mq.c
>>>> index 22ec21aa0c22..9ac9174a2ba4 100644
>>>> --- a/block/blk-mq.c
>>>> +++ b/block/blk-mq.c
>>>> @@ -2513,6 +2513,7 @@ void blk_mq_flush_plug_list(struct blk_plug *plug, bool from_schedule)
>>>>    {
>>>>    	struct blk_mq_hw_ctx *this_hctx;
>>>>    	struct blk_mq_ctx *this_ctx;
>>>> +	struct request *rq;
>>>>    	unsigned int depth;
>>>>    	LIST_HEAD(list);
>>>>    
>>>> @@ -2521,7 +2522,26 @@ void blk_mq_flush_plug_list(struct blk_plug *plug, bool from_schedule)
>>>>    	plug->rq_count = 0;
>>>>    
>>>>    	if (!plug->multiple_queues && !plug->has_elevator && !from_schedule) {
>>>> -		blk_mq_run_dispatch_ops(plug->mq_list->q,
>>>> +		struct request_queue *q;
>>>> +
>>>> +		rq = plug->mq_list;
>>>> +		q = rq->q;
>>>> +
>>>> +		/*
>>>> +		 * Peek first request and see if we have a ->queue_rqs() hook.
>>>> +		 * If we do, we can dispatch the whole plug list in one go. We
>>>> +		 * already know at this point that all requests belong to the
>>>> +		 * same queue, caller must ensure that's the case.
>>>> +		 */
>>>> +		if (q->mq_ops->queue_rqs &&
>>>> +		    !(rq->mq_hctx->flags & BLK_MQ_F_TAG_QUEUE_SHARED)) {
>>>
>>> What is the dependency on shared tags here?
>>>   From what I've seen it's just about submitting requests; the only
>>> difference to shared tags is the way the tags are allocated.
>>> Care to explain?
>>
>> For shared tags, we need to actively increment the use count per
>> request. This path doesn't do that, so it's disabled for now. It could
>> be done, but then it'd have to be in the caller, so I'd rather leave it
>> for a future optimization if anyone cares enough about this for shared
>> tags.
>>
>> I can add a comment about it if that helps.
>>
> Please do.
> It'll act as a reminder what needs to be done if and when one of the 
> drivers requiring shared tags is looking at implementing queue_rqs.

I added to the comment yesterday:

https://git.kernel.dk/cgit/linux-block/commit/?h=perf-wip&id=f9f526700607bf804fa8541c824ea54253f4241a

-- 
Jens Axboe

Re: [PATCH 1/4] block: add mq_ops->queue_rqs hook

[Hannes Reinecke]

On 12/4/21 9:13 PM, Jens Axboe wrote:
> On 12/4/21 3:43 AM, Hannes Reinecke wrote:
>> On 12/3/21 10:45 PM, Jens Axboe wrote:
>>> If we have a list of requests in our plug list, send it to the driver in
>>> one go, if possible. The driver must set mq_ops->queue_rqs() to support
>>> this, if not the usual one-by-one path is used.
>>>
>>> Signed-off-by: Jens Axboe <axboe@kernel.dk>
>>> ---
>>>    block/blk-mq.c         | 24 +++++++++++++++++++++---
>>>    include/linux/blk-mq.h |  8 ++++++++
>>>    2 files changed, 29 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/block/blk-mq.c b/block/blk-mq.c
>>> index 22ec21aa0c22..9ac9174a2ba4 100644
>>> --- a/block/blk-mq.c
>>> +++ b/block/blk-mq.c
>>> @@ -2513,6 +2513,7 @@ void blk_mq_flush_plug_list(struct blk_plug *plug, bool from_schedule)
>>>    {
>>>    	struct blk_mq_hw_ctx *this_hctx;
>>>    	struct blk_mq_ctx *this_ctx;
>>> +	struct request *rq;
>>>    	unsigned int depth;
>>>    	LIST_HEAD(list);
>>>    
>>> @@ -2521,7 +2522,26 @@ void blk_mq_flush_plug_list(struct blk_plug *plug, bool from_schedule)
>>>    	plug->rq_count = 0;
>>>    
>>>    	if (!plug->multiple_queues && !plug->has_elevator && !from_schedule) {
>>> -		blk_mq_run_dispatch_ops(plug->mq_list->q,
>>> +		struct request_queue *q;
>>> +
>>> +		rq = plug->mq_list;
>>> +		q = rq->q;
>>> +
>>> +		/*
>>> +		 * Peek first request and see if we have a ->queue_rqs() hook.
>>> +		 * If we do, we can dispatch the whole plug list in one go. We
>>> +		 * already know at this point that all requests belong to the
>>> +		 * same queue, caller must ensure that's the case.
>>> +		 */
>>> +		if (q->mq_ops->queue_rqs &&
>>> +		    !(rq->mq_hctx->flags & BLK_MQ_F_TAG_QUEUE_SHARED)) {
>>
>> What is the dependency on shared tags here?
>>   From what I've seen it's just about submitting requests; the only
>> difference to shared tags is the way the tags are allocated.
>> Care to explain?
> 
> For shared tags, we need to actively increment the use count per
> request. This path doesn't do that, so it's disabled for now. It could
> be done, but then it'd have to be in the caller, so I'd rather leave it
> for a future optimization if anyone cares enough about this for shared
> tags.
> 
> I can add a comment about it if that helps.
> 
Please do.
It'll act as a reminder what needs to be done if and when one of the 
drivers requiring shared tags is looking at implementing queue_rqs.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer

Re: [PATCH 1/4] block: add mq_ops->queue_rqs hook

[Jens Axboe]

On 12/4/21 3:43 AM, Hannes Reinecke wrote:
> On 12/3/21 10:45 PM, Jens Axboe wrote:
>> If we have a list of requests in our plug list, send it to the driver in
>> one go, if possible. The driver must set mq_ops->queue_rqs() to support
>> this, if not the usual one-by-one path is used.
>>
>> Signed-off-by: Jens Axboe <axboe@kernel.dk>
>> ---
>>   block/blk-mq.c         | 24 +++++++++++++++++++++---
>>   include/linux/blk-mq.h |  8 ++++++++
>>   2 files changed, 29 insertions(+), 3 deletions(-)
>>
>> diff --git a/block/blk-mq.c b/block/blk-mq.c
>> index 22ec21aa0c22..9ac9174a2ba4 100644
>> --- a/block/blk-mq.c
>> +++ b/block/blk-mq.c
>> @@ -2513,6 +2513,7 @@ void blk_mq_flush_plug_list(struct blk_plug *plug, bool from_schedule)
>>   {
>>   	struct blk_mq_hw_ctx *this_hctx;
>>   	struct blk_mq_ctx *this_ctx;
>> +	struct request *rq;
>>   	unsigned int depth;
>>   	LIST_HEAD(list);
>>   
>> @@ -2521,7 +2522,26 @@ void blk_mq_flush_plug_list(struct blk_plug *plug, bool from_schedule)
>>   	plug->rq_count = 0;
>>   
>>   	if (!plug->multiple_queues && !plug->has_elevator && !from_schedule) {
>> -		blk_mq_run_dispatch_ops(plug->mq_list->q,
>> +		struct request_queue *q;
>> +
>> +		rq = plug->mq_list;
>> +		q = rq->q;
>> +
>> +		/*
>> +		 * Peek first request and see if we have a ->queue_rqs() hook.
>> +		 * If we do, we can dispatch the whole plug list in one go. We
>> +		 * already know at this point that all requests belong to the
>> +		 * same queue, caller must ensure that's the case.
>> +		 */
>> +		if (q->mq_ops->queue_rqs &&
>> +		    !(rq->mq_hctx->flags & BLK_MQ_F_TAG_QUEUE_SHARED)) {
> 
> What is the dependency on shared tags here?
>  From what I've seen it's just about submitting requests; the only 
> difference to shared tags is the way the tags are allocated.
> Care to explain?

For shared tags, we need to actively increment the use count per
request. This path doesn't do that, so it's disabled for now. It could
be done, but then it'd have to be in the caller, so I'd rather leave it
for a future optimization if anyone cares enough about this for shared
tags.

I can add a comment about it if that helps.

-- 
Jens Axboe

Re: [PATCH 1/4] block: add mq_ops->queue_rqs hook

[Hannes Reinecke]

On 12/3/21 10:45 PM, Jens Axboe wrote:
> If we have a list of requests in our plug list, send it to the driver in
> one go, if possible. The driver must set mq_ops->queue_rqs() to support
> this, if not the usual one-by-one path is used.
> 
> Signed-off-by: Jens Axboe <axboe@kernel.dk>
> ---
>   block/blk-mq.c         | 24 +++++++++++++++++++++---
>   include/linux/blk-mq.h |  8 ++++++++
>   2 files changed, 29 insertions(+), 3 deletions(-)
> 
> diff --git a/block/blk-mq.c b/block/blk-mq.c
> index 22ec21aa0c22..9ac9174a2ba4 100644
> --- a/block/blk-mq.c
> +++ b/block/blk-mq.c
> @@ -2513,6 +2513,7 @@ void blk_mq_flush_plug_list(struct blk_plug *plug, bool from_schedule)
>   {
>   	struct blk_mq_hw_ctx *this_hctx;
>   	struct blk_mq_ctx *this_ctx;
> +	struct request *rq;
>   	unsigned int depth;
>   	LIST_HEAD(list);
>   
> @@ -2521,7 +2522,26 @@ void blk_mq_flush_plug_list(struct blk_plug *plug, bool from_schedule)
>   	plug->rq_count = 0;
>   
>   	if (!plug->multiple_queues && !plug->has_elevator && !from_schedule) {
> -		blk_mq_run_dispatch_ops(plug->mq_list->q,
> +		struct request_queue *q;
> +
> +		rq = plug->mq_list;
> +		q = rq->q;
> +
> +		/*
> +		 * Peek first request and see if we have a ->queue_rqs() hook.
> +		 * If we do, we can dispatch the whole plug list in one go. We
> +		 * already know at this point that all requests belong to the
> +		 * same queue, caller must ensure that's the case.
> +		 */
> +		if (q->mq_ops->queue_rqs &&
> +		    !(rq->mq_hctx->flags & BLK_MQ_F_TAG_QUEUE_SHARED)) {

What is the dependency on shared tags here?
 From what I've seen it's just about submitting requests; the only 
difference to shared tags is the way the tags are allocated.
Care to explain?

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer

[PATCH 1/4] block: add mq_ops->queue_rqs hook

[Jens Axboe]

If we have a list of requests in our plug list, send it to the driver in
one go, if possible. The driver must set mq_ops->queue_rqs() to support
this, if not the usual one-by-one path is used.

Signed-off-by: Jens Axboe <axboe@kernel.dk>
---
 block/blk-mq.c         | 24 +++++++++++++++++++++---
 include/linux/blk-mq.h |  8 ++++++++
 2 files changed, 29 insertions(+), 3 deletions(-)

diff --git a/block/blk-mq.c b/block/blk-mq.c
index 22ec21aa0c22..9ac9174a2ba4 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -2513,6 +2513,7 @@ void blk_mq_flush_plug_list(struct blk_plug *plug, bool from_schedule)
 {
 	struct blk_mq_hw_ctx *this_hctx;
 	struct blk_mq_ctx *this_ctx;
+	struct request *rq;
 	unsigned int depth;
 	LIST_HEAD(list);
 
@@ -2521,7 +2522,26 @@ void blk_mq_flush_plug_list(struct blk_plug *plug, bool from_schedule)
 	plug->rq_count = 0;
 
 	if (!plug->multiple_queues && !plug->has_elevator && !from_schedule) {
-		blk_mq_run_dispatch_ops(plug->mq_list->q,
+		struct request_queue *q;
+
+		rq = plug->mq_list;
+		q = rq->q;
+
+		/*
+		 * Peek first request and see if we have a ->queue_rqs() hook.
+		 * If we do, we can dispatch the whole plug list in one go. We
+		 * already know at this point that all requests belong to the
+		 * same queue, caller must ensure that's the case.
+		 */
+		if (q->mq_ops->queue_rqs &&
+		    !(rq->mq_hctx->flags & BLK_MQ_F_TAG_QUEUE_SHARED)) {
+			blk_mq_run_dispatch_ops(q,
+				q->mq_ops->queue_rqs(&plug->mq_list));
+			if (rq_list_empty(plug->mq_list))
+				return;
+		}
+
+		blk_mq_run_dispatch_ops(q,
 				blk_mq_plug_issue_direct(plug, false));
 		if (rq_list_empty(plug->mq_list))
 			return;
@@ -2531,8 +2551,6 @@ void blk_mq_flush_plug_list(struct blk_plug *plug, bool from_schedule)
 	this_ctx = NULL;
 	depth = 0;
 	do {
-		struct request *rq;
-
 		rq = rq_list_pop(&plug->mq_list);
 
 		if (!this_hctx) {
diff --git a/include/linux/blk-mq.h b/include/linux/blk-mq.h
index ecdc049b52fa..cdd183757ea0 100644
--- a/include/linux/blk-mq.h
+++ b/include/linux/blk-mq.h
@@ -494,6 +494,14 @@ struct blk_mq_ops {
 	 */
 	void (*commit_rqs)(struct blk_mq_hw_ctx *);
 
+	/**
+	 * @queue_rqs: Queue a list of new requests. Driver is guaranteed
+	 * that each request belongs to the same queue. If the driver doesn't
+	 * empty the @rqlist completely, then the rest will be queued
+	 * individually by the block layer upon return.
+	 */
+	void (*queue_rqs)(struct request **rqlist);
+
 	/**
 	 * @get_budget: Reserve budget before queue request, once .queue_rq is
 	 * run, it is driver's responsibility to release the
-- 
2.34.1

Re: [PATCH 1/4] block: add mq_ops->queue_rqs hook

[Ming Lei]

On Wed, Nov 17, 2021 at 07:02:27PM -0700, Keith Busch wrote:
> On Thu, Nov 18, 2021 at 08:18:29AM +0800, Ming Lei wrote:
> > On Wed, Nov 17, 2021 at 12:41:01PM -0800, Keith Busch wrote:
> > > On Tue, Nov 16, 2021 at 08:38:04PM -0700, Jens Axboe wrote:
> > > > If we have a list of requests in our plug list, send it to the driver in
> > > > one go, if possible. The driver must set mq_ops->queue_rqs() to support
> > > > this, if not the usual one-by-one path is used.
> > > 
> > > It looks like we still need to sync with the request_queue quiesce flag.
> > 
> > Basically it is same with my previous post[1], but the above patch doesn't
> > handle request queue allocation/freeing correctly in case of BLK_MQ_F_BLOCKING.
> 
> Thanks for the pointer. I also thought it may be just as well that blocking
> dispatchers don't get to use the optimized rqlist queueing,

Yeah, that is fine, but nvme-tcp may not benefit from the ->queue_rq() optimization.

> which would simplify quiesce to the normal rcu usage.

Normal dispatch code still need to check quiesced for blocking queue, so
srcu is still needed, not see easier way than srcu yet. Years ago, I
tried to replace the srcu with percpu-refcnt, which is still more
complicated than srcu, but srcu_index can be dropped.


Thanks,
Ming

Re: [PATCH 1/4] block: add mq_ops->queue_rqs hook

[Keith Busch]

On Thu, Nov 18, 2021 at 08:18:29AM +0800, Ming Lei wrote:
> On Wed, Nov 17, 2021 at 12:41:01PM -0800, Keith Busch wrote:
> > On Tue, Nov 16, 2021 at 08:38:04PM -0700, Jens Axboe wrote:
> > > If we have a list of requests in our plug list, send it to the driver in
> > > one go, if possible. The driver must set mq_ops->queue_rqs() to support
> > > this, if not the usual one-by-one path is used.
> > 
> > It looks like we still need to sync with the request_queue quiesce flag.
> 
> Basically it is same with my previous post[1], but the above patch doesn't
> handle request queue allocation/freeing correctly in case of BLK_MQ_F_BLOCKING.

Thanks for the pointer. I also thought it may be just as well that blocking
dispatchers don't get to use the optimized rqlist queueing, which would
simplify quiesce to the normal rcu usage.

Re: [PATCH 1/4] block: add mq_ops->queue_rqs hook

[Ming Lei]

On Wed, Nov 17, 2021 at 12:41:01PM -0800, Keith Busch wrote:
> On Tue, Nov 16, 2021 at 08:38:04PM -0700, Jens Axboe wrote:
> > If we have a list of requests in our plug list, send it to the driver in
> > one go, if possible. The driver must set mq_ops->queue_rqs() to support
> > this, if not the usual one-by-one path is used.
> 
> It looks like we still need to sync with the request_queue quiesce flag.

I think this approach is good.

> Something like the following (untested) on top of this patch should do
> it:
> 
> ---
> diff --git a/block/blk-mq.c b/block/blk-mq.c
> index 688ebf6a7a7b..447d0b77375d 100644
> --- a/block/blk-mq.c
> +++ b/block/blk-mq.c
> @@ -263,6 +263,9 @@ void blk_mq_wait_quiesce_done(struct request_queue *q)
>  	unsigned int i;
>  	bool rcu = false;
>  
> +	if (q->tag_set->flags & BLK_MQ_F_BLOCKING)
> +		synchronize_srcu(q->srcu);
> +
>  	queue_for_each_hw_ctx(q, hctx, i) {
>  		if (hctx->flags & BLK_MQ_F_BLOCKING)
>  			synchronize_srcu(hctx->srcu);
> @@ -2201,6 +2204,25 @@ static void blk_mq_commit_rqs(struct blk_mq_hw_ctx *hctx, int *queued,
>  	*queued = 0;
>  }
>  
> +static void queue_lock(struct request_queue *q, int *srcu_idx)
> +	__acquires(q->srcu)
> +{
> +	if (!(q->tag_set->flags & BLK_MQ_F_BLOCKING)) {
> +		*srcu_idx = 0;
> +		rcu_read_lock();
> +	} else
> +		*srcu_idx = srcu_read_lock(q->srcu);
> +}
> +
> +static void queue_unlock(struct request_queue *q, int srcu_idx)
> +	__releases(q->srcu)
> +{
> +	if (!(q->tag_set->flags & BLK_MQ_F_BLOCKING))
> +		rcu_read_unlock();
> +	else
> +		srcu_read_unlock(q->srcu, srcu_idx);
> +}
> +
>  static void blk_mq_plug_issue_direct(struct blk_plug *plug, bool from_schedule)
>  {
>  	struct blk_mq_hw_ctx *hctx = NULL;
> @@ -2216,7 +2238,14 @@ static void blk_mq_plug_issue_direct(struct blk_plug *plug, bool from_schedule)
>  	 */
>  	rq = rq_list_peek(&plug->mq_list);
>  	if (rq->q->mq_ops->queue_rqs) {
> -		rq->q->mq_ops->queue_rqs(&plug->mq_list);
> +		struct request_queue *q = rq->q;
> +		int srcu_idx;
> +
> +		queue_lock(q, &srcu_idx);
> +		if (!blk_queue_quiesced(q))
> +			q->mq_ops->queue_rqs(&plug->mq_list);
> +		queue_unlock(q, srcu_idx);
> +
>  		if (rq_list_empty(plug->mq_list))
>  			return;
>  	}
> @@ -3727,6 +3756,8 @@ int blk_mq_init_allocated_queue(struct blk_mq_tag_set *set,
>  	blk_queue_rq_timeout(q, set->timeout ? set->timeout : 30 * HZ);
>  
>  	q->tag_set = set;
> +	if (set->flags & BLK_MQ_F_BLOCKING)
> +		init_srcu_struct(q->srcu);
>  
>  	q->queue_flags |= QUEUE_FLAG_MQ_DEFAULT;
>  	if (set->nr_maps > HCTX_TYPE_POLL &&
> diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
> index bd4370baccca..ae7591dc9cbb 100644
> --- a/include/linux/blkdev.h
> +++ b/include/linux/blkdev.h
> @@ -373,6 +373,8 @@ struct request_queue {
>  	 * devices that do not have multiple independent access ranges.
>  	 */
>  	struct blk_independent_access_ranges *ia_ranges;
> +
> +	struct srcu_struct	srcu[];

Basically it is same with my previous post[1], but the above patch doesn't
handle request queue allocation/freeing correctly in case of BLK_MQ_F_BLOCKING.

[1] https://lore.kernel.org/linux-block/20211103160018.3764976-1-ming.lei@redhat.com/


Thanks,
Ming

Re: [PATCH 1/4] block: add mq_ops->queue_rqs hook

[Ming Lei]

On Wed, Nov 17, 2021 at 12:48:14PM -0800, Keith Busch wrote:
> On Wed, Nov 17, 2021 at 08:43:02AM -0700, Jens Axboe wrote:
> > On 11/17/21 1:20 AM, Ming Lei wrote:
> > > On Tue, Nov 16, 2021 at 08:38:04PM -0700, Jens Axboe wrote:
> > >> If we have a list of requests in our plug list, send it to the driver in
> > >> one go, if possible. The driver must set mq_ops->queue_rqs() to support
> > >> this, if not the usual one-by-one path is used.
> > >>
> > >> Signed-off-by: Jens Axboe <axboe@kernel.dk>
> > >> ---
> > >>  block/blk-mq.c         | 17 +++++++++++++++++
> > >>  include/linux/blk-mq.h |  8 ++++++++
> > >>  2 files changed, 25 insertions(+)
> > >>
> > >> diff --git a/block/blk-mq.c b/block/blk-mq.c
> > >> index 9b4e79e2ac1e..005715206b16 100644
> > >> --- a/block/blk-mq.c
> > >> +++ b/block/blk-mq.c
> > >> @@ -2208,6 +2208,19 @@ static void blk_mq_plug_issue_direct(struct blk_plug *plug, bool from_schedule)
> > >>  	int queued = 0;
> > >>  	int errors = 0;
> > >>  
> > >> +	/*
> > >> +	 * Peek first request and see if we have a ->queue_rqs() hook. If we
> > >> +	 * do, we can dispatch the whole plug list in one go. We already know
> > >> +	 * at this point that all requests belong to the same queue, caller
> > >> +	 * must ensure that's the case.
> > >> +	 */
> > >> +	rq = rq_list_peek(&plug->mq_list);
> > >> +	if (rq->q->mq_ops->queue_rqs) {
> > >> +		rq->q->mq_ops->queue_rqs(&plug->mq_list);
> > >> +		if (rq_list_empty(plug->mq_list))
> > >> +			return;
> > >> +	}
> > >> +
> > > 
> > > Then BLK_MQ_F_TAG_QUEUE_SHARED isn't handled as before for multiple NVMe
> > > NS.
> > 
> > Can you expand? If we have multiple namespaces in the plug list, we have
> > multiple queues. There's no direct issue of the list if that's the case.
> > Or maybe I'm missing what you mean here?
> 
> If the plug list only has requests for one namespace, I think Ming is
> referring to the special accounting for BLK_MQ_F_TAG_QUEUE_SHARED in
> __blk_mq_get_driver_tag() that normally gets called before dispatching
> to the driver, but isn't getting called when using .queue_rqs().

Yeah, that is it. This is one normal case, each task runs I/O on
different namespace, but all NSs share/contend the single host tags,
BLK_MQ_F_TAG_QUEUE_SHARED is supposed to provide fair allocation among
all these NSs.


thanks, 
Ming

Re: [PATCH 1/4] block: add mq_ops->queue_rqs hook

[Keith Busch]

On Wed, Nov 17, 2021 at 08:43:02AM -0700, Jens Axboe wrote:
> On 11/17/21 1:20 AM, Ming Lei wrote:
> > On Tue, Nov 16, 2021 at 08:38:04PM -0700, Jens Axboe wrote:
> >> If we have a list of requests in our plug list, send it to the driver in
> >> one go, if possible. The driver must set mq_ops->queue_rqs() to support
> >> this, if not the usual one-by-one path is used.
> >>
> >> Signed-off-by: Jens Axboe <axboe@kernel.dk>
> >> ---
> >>  block/blk-mq.c         | 17 +++++++++++++++++
> >>  include/linux/blk-mq.h |  8 ++++++++
> >>  2 files changed, 25 insertions(+)
> >>
> >> diff --git a/block/blk-mq.c b/block/blk-mq.c
> >> index 9b4e79e2ac1e..005715206b16 100644
> >> --- a/block/blk-mq.c
> >> +++ b/block/blk-mq.c
> >> @@ -2208,6 +2208,19 @@ static void blk_mq_plug_issue_direct(struct blk_plug *plug, bool from_schedule)
> >>  	int queued = 0;
> >>  	int errors = 0;
> >>  
> >> +	/*
> >> +	 * Peek first request and see if we have a ->queue_rqs() hook. If we
> >> +	 * do, we can dispatch the whole plug list in one go. We already know
> >> +	 * at this point that all requests belong to the same queue, caller
> >> +	 * must ensure that's the case.
> >> +	 */
> >> +	rq = rq_list_peek(&plug->mq_list);
> >> +	if (rq->q->mq_ops->queue_rqs) {
> >> +		rq->q->mq_ops->queue_rqs(&plug->mq_list);
> >> +		if (rq_list_empty(plug->mq_list))
> >> +			return;
> >> +	}
> >> +
> > 
> > Then BLK_MQ_F_TAG_QUEUE_SHARED isn't handled as before for multiple NVMe
> > NS.
> 
> Can you expand? If we have multiple namespaces in the plug list, we have
> multiple queues. There's no direct issue of the list if that's the case.
> Or maybe I'm missing what you mean here?

If the plug list only has requests for one namespace, I think Ming is
referring to the special accounting for BLK_MQ_F_TAG_QUEUE_SHARED in
__blk_mq_get_driver_tag() that normally gets called before dispatching
to the driver, but isn't getting called when using .queue_rqs().

Re: [PATCH 1/4] block: add mq_ops->queue_rqs hook

[Keith Busch]

On Tue, Nov 16, 2021 at 08:38:04PM -0700, Jens Axboe wrote:
> If we have a list of requests in our plug list, send it to the driver in
> one go, if possible. The driver must set mq_ops->queue_rqs() to support
> this, if not the usual one-by-one path is used.

It looks like we still need to sync with the request_queue quiesce flag.
Something like the following (untested) on top of this patch should do
it:

---
diff --git a/block/blk-mq.c b/block/blk-mq.c
index 688ebf6a7a7b..447d0b77375d 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -263,6 +263,9 @@ void blk_mq_wait_quiesce_done(struct request_queue *q)
 	unsigned int i;
 	bool rcu = false;
 
+	if (q->tag_set->flags & BLK_MQ_F_BLOCKING)
+		synchronize_srcu(q->srcu);
+
 	queue_for_each_hw_ctx(q, hctx, i) {
 		if (hctx->flags & BLK_MQ_F_BLOCKING)
 			synchronize_srcu(hctx->srcu);
@@ -2201,6 +2204,25 @@ static void blk_mq_commit_rqs(struct blk_mq_hw_ctx *hctx, int *queued,
 	*queued = 0;
 }
 
+static void queue_lock(struct request_queue *q, int *srcu_idx)
+	__acquires(q->srcu)
+{
+	if (!(q->tag_set->flags & BLK_MQ_F_BLOCKING)) {
+		*srcu_idx = 0;
+		rcu_read_lock();
+	} else
+		*srcu_idx = srcu_read_lock(q->srcu);
+}
+
+static void queue_unlock(struct request_queue *q, int srcu_idx)
+	__releases(q->srcu)
+{
+	if (!(q->tag_set->flags & BLK_MQ_F_BLOCKING))
+		rcu_read_unlock();
+	else
+		srcu_read_unlock(q->srcu, srcu_idx);
+}
+
 static void blk_mq_plug_issue_direct(struct blk_plug *plug, bool from_schedule)
 {
 	struct blk_mq_hw_ctx *hctx = NULL;
@@ -2216,7 +2238,14 @@ static void blk_mq_plug_issue_direct(struct blk_plug *plug, bool from_schedule)
 	 */
 	rq = rq_list_peek(&plug->mq_list);
 	if (rq->q->mq_ops->queue_rqs) {
-		rq->q->mq_ops->queue_rqs(&plug->mq_list);
+		struct request_queue *q = rq->q;
+		int srcu_idx;
+
+		queue_lock(q, &srcu_idx);
+		if (!blk_queue_quiesced(q))
+			q->mq_ops->queue_rqs(&plug->mq_list);
+		queue_unlock(q, srcu_idx);
+
 		if (rq_list_empty(plug->mq_list))
 			return;
 	}
@@ -3727,6 +3756,8 @@ int blk_mq_init_allocated_queue(struct blk_mq_tag_set *set,
 	blk_queue_rq_timeout(q, set->timeout ? set->timeout : 30 * HZ);
 
 	q->tag_set = set;
+	if (set->flags & BLK_MQ_F_BLOCKING)
+		init_srcu_struct(q->srcu);
 
 	q->queue_flags |= QUEUE_FLAG_MQ_DEFAULT;
 	if (set->nr_maps > HCTX_TYPE_POLL &&
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index bd4370baccca..ae7591dc9cbb 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -373,6 +373,8 @@ struct request_queue {
 	 * devices that do not have multiple independent access ranges.
 	 */
 	struct blk_independent_access_ranges *ia_ranges;
+
+	struct srcu_struct	srcu[];
 };
 
 /* Keep blk_queue_flag_name[] in sync with the definitions below */
--

Re: [PATCH 1/4] block: add mq_ops->queue_rqs hook

[Jens Axboe]

On 11/17/21 1:20 AM, Ming Lei wrote:
> On Tue, Nov 16, 2021 at 08:38:04PM -0700, Jens Axboe wrote:
>> If we have a list of requests in our plug list, send it to the driver in
>> one go, if possible. The driver must set mq_ops->queue_rqs() to support
>> this, if not the usual one-by-one path is used.
>>
>> Signed-off-by: Jens Axboe <axboe@kernel.dk>
>> ---
>>  block/blk-mq.c         | 17 +++++++++++++++++
>>  include/linux/blk-mq.h |  8 ++++++++
>>  2 files changed, 25 insertions(+)
>>
>> diff --git a/block/blk-mq.c b/block/blk-mq.c
>> index 9b4e79e2ac1e..005715206b16 100644
>> --- a/block/blk-mq.c
>> +++ b/block/blk-mq.c
>> @@ -2208,6 +2208,19 @@ static void blk_mq_plug_issue_direct(struct blk_plug *plug, bool from_schedule)
>>  	int queued = 0;
>>  	int errors = 0;
>>  
>> +	/*
>> +	 * Peek first request and see if we have a ->queue_rqs() hook. If we
>> +	 * do, we can dispatch the whole plug list in one go. We already know
>> +	 * at this point that all requests belong to the same queue, caller
>> +	 * must ensure that's the case.
>> +	 */
>> +	rq = rq_list_peek(&plug->mq_list);
>> +	if (rq->q->mq_ops->queue_rqs) {
>> +		rq->q->mq_ops->queue_rqs(&plug->mq_list);
>> +		if (rq_list_empty(plug->mq_list))
>> +			return;
>> +	}
>> +
> 
> Then BLK_MQ_F_TAG_QUEUE_SHARED isn't handled as before for multiple NVMe
> NS.

Can you expand? If we have multiple namespaces in the plug list, we have
multiple queues. There's no direct issue of the list if that's the case.
Or maybe I'm missing what you mean here?

-- 
Jens Axboe

Re: [PATCH 1/4] block: add mq_ops->queue_rqs hook

[Jens Axboe]

On 11/16/21 11:25 PM, Christoph Hellwig wrote:
>> @@ -2208,6 +2208,19 @@ static void blk_mq_plug_issue_direct(struct blk_plug *plug, bool from_schedule)
>>  	int queued = 0;
>>  	int errors = 0;
>>  
>> +	/*
>> +	 * Peek first request and see if we have a ->queue_rqs() hook. If we
>> +	 * do, we can dispatch the whole plug list in one go. We already know
>> +	 * at this point that all requests belong to the same queue, caller
>> +	 * must ensure that's the case.
>> +	 */
>> +	rq = rq_list_peek(&plug->mq_list);
>> +	if (rq->q->mq_ops->queue_rqs) {
>> +		rq->q->mq_ops->queue_rqs(&plug->mq_list);
>> +		if (rq_list_empty(plug->mq_list))
>> +			return;
>> +	}
> 
> I'd move this straight into the caller to simplify the follow, something
> like the patch below.  Also I wonder if the slow path might want to
> be moved from blk_mq_flush_plug_list into a separate helper to keep
> the fast path as straight as possible?

Yes, I think that's better. I'll fold that in.

-- 
Jens Axboe

Re: [PATCH 1/4] block: add mq_ops->queue_rqs hook

[Ming Lei]

On Tue, Nov 16, 2021 at 08:38:04PM -0700, Jens Axboe wrote:
> If we have a list of requests in our plug list, send it to the driver in
> one go, if possible. The driver must set mq_ops->queue_rqs() to support
> this, if not the usual one-by-one path is used.
> 
> Signed-off-by: Jens Axboe <axboe@kernel.dk>
> ---
>  block/blk-mq.c         | 17 +++++++++++++++++
>  include/linux/blk-mq.h |  8 ++++++++
>  2 files changed, 25 insertions(+)
> 
> diff --git a/block/blk-mq.c b/block/blk-mq.c
> index 9b4e79e2ac1e..005715206b16 100644
> --- a/block/blk-mq.c
> +++ b/block/blk-mq.c
> @@ -2208,6 +2208,19 @@ static void blk_mq_plug_issue_direct(struct blk_plug *plug, bool from_schedule)
>  	int queued = 0;
>  	int errors = 0;
>  
> +	/*
> +	 * Peek first request and see if we have a ->queue_rqs() hook. If we
> +	 * do, we can dispatch the whole plug list in one go. We already know
> +	 * at this point that all requests belong to the same queue, caller
> +	 * must ensure that's the case.
> +	 */
> +	rq = rq_list_peek(&plug->mq_list);
> +	if (rq->q->mq_ops->queue_rqs) {
> +		rq->q->mq_ops->queue_rqs(&plug->mq_list);
> +		if (rq_list_empty(plug->mq_list))
> +			return;
> +	}
> +

Then BLK_MQ_F_TAG_QUEUE_SHARED isn't handled as before for multiple NVMe
NS.


thanks,
Ming

Re: [PATCH 1/4] block: add mq_ops->queue_rqs hook

[Christoph Hellwig]

> @@ -2208,6 +2208,19 @@ static void blk_mq_plug_issue_direct(struct blk_plug *plug, bool from_schedule)
>  	int queued = 0;
>  	int errors = 0;
>  
> +	/*
> +	 * Peek first request and see if we have a ->queue_rqs() hook. If we
> +	 * do, we can dispatch the whole plug list in one go. We already know
> +	 * at this point that all requests belong to the same queue, caller
> +	 * must ensure that's the case.
> +	 */
> +	rq = rq_list_peek(&plug->mq_list);
> +	if (rq->q->mq_ops->queue_rqs) {
> +		rq->q->mq_ops->queue_rqs(&plug->mq_list);
> +		if (rq_list_empty(plug->mq_list))
> +			return;
> +	}

I'd move this straight into the caller to simplify the follow, something
like the patch below.  Also I wonder if the slow path might want to
be moved from blk_mq_flush_plug_list into a separate helper to keep
the fast path as straight as possible?

diff --git a/block/blk-mq.c b/block/blk-mq.c
index 3ab34c4f20daf..370f4b2ab4511 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -2255,6 +2255,14 @@ void blk_mq_flush_plug_list(struct blk_plug *plug, bool from_schedule)
 	plug->rq_count = 0;
 
 	if (!plug->multiple_queues && !plug->has_elevator && !from_schedule) {
+		struct request_queue *q = rq_list_peek(&plug->mq_list)->q;
+
+		if (q->mq_ops->queue_rqs) {
+			q->mq_ops->queue_rqs(&plug->mq_list);
+			if (rq_list_empty(plug->mq_list))
+				return;
+		}
+
 		blk_mq_plug_issue_direct(plug, false);
 		if (rq_list_empty(plug->mq_list))
 			return;

[PATCH 1/4] block: add mq_ops->queue_rqs hook

[Jens Axboe]

If we have a list of requests in our plug list, send it to the driver in
one go, if possible. The driver must set mq_ops->queue_rqs() to support
this, if not the usual one-by-one path is used.

Signed-off-by: Jens Axboe <axboe@kernel.dk>
---
 block/blk-mq.c         | 17 +++++++++++++++++
 include/linux/blk-mq.h |  8 ++++++++
 2 files changed, 25 insertions(+)

diff --git a/block/blk-mq.c b/block/blk-mq.c
index 9b4e79e2ac1e..005715206b16 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -2208,6 +2208,19 @@ static void blk_mq_plug_issue_direct(struct blk_plug *plug, bool from_schedule)
 	int queued = 0;
 	int errors = 0;
 
+	/*
+	 * Peek first request and see if we have a ->queue_rqs() hook. If we
+	 * do, we can dispatch the whole plug list in one go. We already know
+	 * at this point that all requests belong to the same queue, caller
+	 * must ensure that's the case.
+	 */
+	rq = rq_list_peek(&plug->mq_list);
+	if (rq->q->mq_ops->queue_rqs) {
+		rq->q->mq_ops->queue_rqs(&plug->mq_list);
+		if (rq_list_empty(plug->mq_list))
+			return;
+	}
+
 	while ((rq = rq_list_pop(&plug->mq_list))) {
 		bool last = rq_list_empty(plug->mq_list);
 		blk_status_t ret;
@@ -2256,6 +2269,10 @@ void blk_mq_flush_plug_list(struct blk_plug *plug, bool from_schedule)
 
 	if (!plug->multiple_queues && !plug->has_elevator && !from_schedule) {
 		blk_mq_plug_issue_direct(plug, false);
+		/*
+		 * Expected case, all requests got dispatched. If not, fall
+		 * through to individual dispatch of the remainder.
+		 */
 		if (rq_list_empty(plug->mq_list))
 			return;
 	}
diff --git a/include/linux/blk-mq.h b/include/linux/blk-mq.h
index 3ba1e750067b..897cf475e7eb 100644
--- a/include/linux/blk-mq.h
+++ b/include/linux/blk-mq.h
@@ -503,6 +503,14 @@ struct blk_mq_ops {
 	 */
 	void (*commit_rqs)(struct blk_mq_hw_ctx *);
 
+	/**
+	 * @queue_rqs: Queue a list of new requests. Driver is guaranteed
+	 * that each request belongs to the same queue. If the driver doesn't
+	 * empty the @rqlist completely, then the rest will be queued
+	 * individually by the block layer upon return.
+	 */
+	void (*queue_rqs)(struct request **rqlist);
+
 	/**
 	 * @get_budget: Reserve budget before queue request, once .queue_rq is
 	 * run, it is driver's responsibility to release the
-- 
2.33.1