Add support for list issue

(quoted diff) +2 -0

@@ -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

Quoted 13 lines

(quoted diff) +9 -0

@@ -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);

block/blk-mq.c void blk_mq_flush_plug_list(struct blk_plug *plug, bool from_schedule) · line 2264

Quoted 3 lines

block/blk-mq.c +3 -0

+		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?

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 <[email protected]>

---
 block/blk-mq.c         | 17 +++++++++++++++++
 include/linux/blk-mq.h |  8 ++++++++
 2 files changed, 25 insertions(+)

block/blk-mq.c +3 -0

index 9b4e79e2ac1e..005715206b16 100644
@@ -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

Quoted 18 lines · 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 <[email protected]>

---
 block/blk-mq.c         | 17 +++++++++++++++++
 include/linux/blk-mq.h |  8 ++++++++
 2 files changed, 25 insertions(+)

block/blk-mq.c +0 -0

index 9b4e79e2ac1e..005715206b16 100644
@@ -2208,6 +2208,19 @@ static void blk_mq_plug_issue_direct(struct blk_plug *plug, bool from_schedule)
 	int queued = 0;
 	int errors = 0;
 

block/blk-mq.c static void blk_mq_plug_issue_direct(struct blk_plug *plug, bool from_schedule) · line 2223

Quoted 13 lines

block/blk-mq.c +13 -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.

Quoted 19 lines · Ming Lei

Quoted 18 lines · 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 <[email protected]>

---
 block/blk-mq.c         | 17 +++++++++++++++++
 include/linux/blk-mq.h |  8 ++++++++
 2 files changed, 25 insertions(+)

block/blk-mq.c +0 -0

index 9b4e79e2ac1e..005715206b16 100644
@@ -2208,6 +2208,19 @@ static void blk_mq_plug_issue_direct(struct blk_plug *plug, bool from_schedule)
 	int queued = 0;
 	int errors = 0;
 

Quoted 16 lines

block/blk-mq.c static void blk_mq_plug_issue_direct(struct blk_plug *plug, bool from_schedule) · line 2223

Quoted 13 lines

block/blk-mq.c +13 -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?

Quoted 37 lines · Jens Axboe

Quoted 35 lines · Ming Lei

Quoted 21 lines · 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 <[email protected]>

---
 block/blk-mq.c         | 17 +++++++++++++++++
 include/linux/blk-mq.h |  8 ++++++++
 2 files changed, 25 insertions(+)

block/blk-mq.c +3 -0

index 9b4e79e2ac1e..005715206b16 100644
@@ -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

block/blk-mq.c static void blk_mq_plug_issue_direct(struct blk_plug *plug, bool from_schedule) · line 2223

Quoted 10 lines

block/blk-mq.c +10 -0

+	 * 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.

Quoted 3 lines

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().

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.

Quoted 3 lines · 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.

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:

---

block/blk-mq.c +32 -1

index 688ebf6a7a7b..447d0b77375d 100644
@@ -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 &&

include/linux/blkdev.h +0 -0

index bd4370baccca..ae7591dc9cbb 100644

Quoted 6 lines · Keith Busch

Quoted 3 lines · 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.

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.

Quoted 6 lines · Ming Lei

Quoted 5 lines · Keith Busch

Quoted 3 lines · 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.

Quoted 4 lines

Quoted 1 line

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.

/**
* nvme_submit_cmd() - Copy a command into a queue and ring the doorbell
* @nvmeq: The queue to use

Quoted 9 lines · Jens Axboe

/**
* nvme_submit_cmd() - Copy a command into a queue and ring the doorbell
* @nvmeq: The queue to use

drivers/nvme/host/pci.c +0 -2

@@ -511,10 +520,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));

drivers/nvme/host/pci.c static void nvme_submit_cmd(struct nvme_queue *nvmeq, struct nvme_command *cmd, · line 525

Quoted 5 lines

drivers/nvme/host/pci.c +1 -2

-	if (++nvmeq->sq_tail == nvmeq->q_depth)
-		nvmeq->sq_tail = 0;
+	nvme_copy_cmd(nvmeq, cmd);
 	nvme_write_sq_db(nvmeq, write_sq);
 	spin_unlock(&nvmeq->sq_lock);

Given that nvme_submit_cmd only has two callers, I'd be tempted to just open code in the callers rather than creating a deep callchain here.

External email: Use caution opening links or attachments

We'll need it for batched submit as well.

Signed-off-by Jens Axboe <[email protected]>

---

drivers/nvme/host/pci.c static blk_status_t nvme_queue_rq(struct blk_mq_hw_ctx *hctx, · line 1004

Quoted 6 lines · Jens Axboe

drivers/nvme/host/pci.c +6 -0

+	ret = nvme_prep_rq(dev, ns, req, &iod->cmd);
+	if (ret == BLK_STS_OK) {
+		nvme_submit_cmd(nvmeq, &iod->cmd, bd->last);
+		return BLK_STS_OK;
+	}
+	return ret;

I'd prefer the traditional handle errors outside the straight path order here:

	ret = nvme_prep_rq(dev, ns, req, &iod->cmd);
	if (ret)
		return ret;
	nvme_submit_cmd(nvmeq, &iod->cmd, bd->last);
	return BLK_STS_OK;

drivers/nvme/host/pci.c static blk_status_t nvme_queue_rq(struct blk_mq_hw_ctx *hctx, · line 1004

Quoted 6 lines · Jens Axboe

drivers/nvme/host/pci.c +6 -0

+	ret = nvme_prep_rq(dev, ns, req, &iod->cmd);
+	if (ret == BLK_STS_OK) {
+		nvme_submit_cmd(nvmeq, &iod->cmd, bd->last);
+		return BLK_STS_OK;
+	}
+	return ret;

I'd prefer the traditional handle errors outside the straight path order here:

	ret = nvme_prep_rq(dev, ns, req, &iod->cmd);
	if (ret)
		return ret;
	nvme_submit_cmd(nvmeq, &iod->cmd, bd->last);
	return BLK_STS_OK;

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.

Quoted 5 lines · 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.

Quoted 5 lines

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.

The concept looks sensible, but the loop in nvme_queue_rqs is a complete mess to follow. What about something like this (untested) on top?

drivers/nvme/host/pci.c +0 -4

index 13722cc400c2c..555a7609580c7 100644
@@ -509,21 +509,6 @@ static inline void nvme_copy_cmd(struct nvme_queue *nvmeq,
 		nvmeq->sq_tail = 0;
 }
 
-/**
- * nvme_submit_cmd() - Copy a command into a queue and ring the doorbell
- * @nvmeq: The queue to use
- * @cmd: The command to send

Quoted 9 lines · Christoph Hellwig

Quoted 8 lines · 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.

Quoted 5 lines

Quoted 2 lines

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

The concept looks sensible, but the loop in nvme_queue_rqs is a complete mess to follow. What about something like this (untested) on top?

Let me take a closer look.

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.

Signed-off-by Jens Axboe <[email protected]>

---
 drivers/nvme/host/pci.c | 67 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 67 insertions(+)

drivers/nvme/host/pci.c +40 -0

index d2b654fc3603..2eedd04b1f90 100644
@@ -1004,6 +1004,72 @@ static blk_status_t nvme_queue_rq(struct blk_mq_hw_ctx *hctx,
 	return ret;
 }
 
+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_copy_cmd(nvmeq, absolute_pointer(&iod->cmd));
+	}
+	nvme_write_sq_db(nvmeq, true);
+	spin_unlock(&nvmeq->sq_lock);
+}
+
+static void nvme_queue_rqs(struct request **rqlist)
+{
+	struct request *requeue_list = NULL, *req, *prev = NULL;
+	struct blk_mq_hw_ctx *hctx;
+	struct nvme_queue *nvmeq;
+	struct nvme_ns *ns;
+
+restart:
+	req = rq_list_peek(rqlist);
+	hctx = req->mq_hctx;
+	nvmeq = hctx->driver_data;
+	ns = hctx->queue->queuedata;
+
+	/*
+	 * 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;
+
+	rq_list_for_each(rqlist, req) {
+		struct nvme_iod *iod = blk_mq_rq_to_pdu(req);
+		blk_status_t ret;
+
+		if (unlikely(!nvme_check_ready(&nvmeq->dev->ctrl, req, true)))
+			goto requeue;
+

drivers/nvme/host/pci.c +17 -0

+		ret = nvme_prep_rq(nvmeq->dev, ns, req, &iod->cmd);
+		if (ret == BLK_STS_OK) {
+			prev = req;
+			continue;
+		}
+requeue:
+		/* detach 'req' and add to remainder list */
+		if (prev)
+			prev->rq_next = req->rq_next;
+		rq_list_add(&requeue_list, req);
+	}
+
+	nvme_submit_cmds(nvmeq, rqlist);
+	*rqlist = requeue_list;
+}
+
 static __always_inline void nvme_pci_unmap_rq(struct request *req)
 {
 	struct nvme_iod *iod = blk_mq_rq_to_pdu(req);

@@ -1741,6 +1807,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,