nvme-fabrics: sqsize bug fixes

A few sqsize bugs have been discovered during the connect() operation, basically the NVMe-fabrics layer being not-in-sync with the implementation layer (RDMA).

As these bugs actually do break the NVMe-Fabrics spec, I'd like to make the request to apply these fixes to the 4.8 kernel (when approved).

Jay Freyensee (2):
nvme-rdma: tell fabrics layer admin queue depth
nvme-rdma: sqsize/hsqsize/hrqsize is 0-based val

 drivers/nvme/host/fabrics.c |  2 +-
 drivers/nvme/host/nvme.h    |  1 +
 drivers/nvme/host/rdma.c    | 20 +++++++++++++++++---
 3 files changed, 19 insertions(+), 4 deletions(-)

2.7.4

Upon admin queue connect(), the rdma qp was being set based on NVMF_AQ_DEPTH. However, the fabrics layer was using the sqsize field value set for I/O queues for the admin queue, which through the nvme layer and rdma layer off-whack:

root at fedora23-fabrics-host1 nvmf]# dmesg

[ 3507.798642] nvme_fabrics: nvmf_connect_admin_queue():admin sqsize
being sent is: 128
[ 3507.798858] nvme nvme0: creating 16 I/O queues.
[ 3507.896407] nvme nvme0: new ctrl: NQN "nullside-nqn", addr

192.168.1.3:4420

Thus, to have a different admin queue value (which the fabrics spec states the minimum depth for a fabrics admin queue is 32 via the ASQSZ definition), we need also a new variable to hold the sqsize for admin fabrics queue.

---
 drivers/nvme/host/fabrics.c |  2 +-
 drivers/nvme/host/nvme.h    |  1 +
 drivers/nvme/host/rdma.c    | 18 ++++++++++++++++--
 3 files changed, 18 insertions(+), 3 deletions(-)

index dc99676..f81d937 100644
@@ -363,7 +363,7 @@ int nvmf_connect_admin_queue(struct nvme_ctrl *ctrl)
 	cmd.connect.opcode = nvme_fabrics_command;
 	cmd.connect.fctype = nvme_fabrics_type_connect;
 	cmd.connect.qid = 0;
-	cmd.connect.sqsize = cpu_to_le16(ctrl->sqsize);
+	cmd.connect.sqsize = cpu_to_le16(ctrl->admin_sqsize);
 	/*
 	 * Set keep-alive timeout in seconds granularity (ms * 1000)
 	 * and add a grace period for controller kato enforcement

index ab18b78..32577a7 100644
@@ -137,6 +137,7 @@ struct nvme_ctrl {
 	struct delayed_work ka_work;
 
 	/* Fabrics only */
+	u16 admin_sqsize;
 	u16 sqsize;
 	u32 ioccsz;
 	u32 iorcsz;

index 3e3ce2b..ff44167 100644
@@ -1284,8 +1284,13 @@ static int nvme_rdma_route_resolved(struct nvme_rdma_queue *queue)
 
 	priv.recfmt = cpu_to_le16(NVME_RDMA_CM_FMT_1_0);
 	priv.qid = cpu_to_le16(nvme_rdma_queue_idx(queue));
-	priv.hrqsize = cpu_to_le16(queue->queue_size);
-	priv.hsqsize = cpu_to_le16(queue->queue_size);
+	if (priv.qid == 0) {
+		priv.hrqsize = cpu_to_le16(queue->ctrl->ctrl.admin_sqsize);
+		priv.hsqsize = cpu_to_le16(queue->ctrl->ctrl.admin_sqsize);
+	} else {
+		priv.hrqsize = cpu_to_le16(queue->queue_size);
+		priv.hsqsize = cpu_to_le16(queue->queue_size);
+	}
 
 	ret = rdma_connect(queue->cm_id, &param);
 	if (ret) {

@@ -1907,6 +1912,15 @@ static struct nvme_ctrl *nvme_rdma_create_ctrl(struct device *dev,
 	spin_lock_init(&ctrl->lock);
 
 	ctrl->queue_count = opts->nr_io_queues + 1; /* +1 for admin queue */
+
+	/* as nvme_rdma_configure_admin_queue() is setting the rdma's
+	 * internal submission queue to a different value other
+	 * than opts->queue_size, we need to make sure the
+	 * fabric layer uses that value upon an
+	 * NVMeoF admin connect() and not default to the more
+	 * common I/O queue size value (sqsize, opts->queue_size).
+	 */
+	ctrl->ctrl.admin_sqsize = NVMF_AQ_DEPTH-1;
 	ctrl->ctrl.sqsize = opts->queue_size;
 	ctrl->ctrl.kato = opts->kato;
 

2.7.4

Agreed.

Ok, but this allows the code to grow into the ASQSZ field (whenever this is implemented or not), which is done on discovery that each new controller discovered could have a different size Admin queue field.

But I'm perfectly happy using this,it makes the code a bit easier.

Per NVMe-over-Fabrics 1.0 spec, sqsize is represented as a 0-based value.

Also per spec, the RDMA binding values shall be set to sqsize, which makes hsqsize and hrqsize 0-based values.

Thus, the sqsize at the NVMe Fabrics level is now:

[root at fedora23-fabrics-host1 for-48]# dmesg

[  318.720645] nvme_fabrics: nvmf_connect_admin_queue(): sqsize for
admin queue: 31
[  318.720884] nvme nvme0: creating 16 I/O queues.
[  318.810114] nvme_fabrics: nvmf_connect_io_queue(): sqsize for i/o

queue: 127

---
 drivers/nvme/host/rdma.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

index ff44167..6300b10 100644
@@ -1288,8 +1288,8 @@ static int nvme_rdma_route_resolved(struct nvme_rdma_queue *queue)
 		priv.hrqsize = cpu_to_le16(queue->ctrl->ctrl.admin_sqsize);
 		priv.hsqsize = cpu_to_le16(queue->ctrl->ctrl.admin_sqsize);
 	} else {
-		priv.hrqsize = cpu_to_le16(queue->queue_size);
-		priv.hsqsize = cpu_to_le16(queue->queue_size);
+		priv.hrqsize = cpu_to_le16(queue->ctrl->ctrl.sqsize);
+		priv.hsqsize = cpu_to_le16(queue->ctrl->ctrl.sqsize);
 	}
 
 	ret = rdma_connect(queue->cm_id, &param);

@@ -1921,7 +1921,7 @@ static struct nvme_ctrl *nvme_rdma_create_ctrl(struct device *dev,
 	 * common I/O queue size value (sqsize, opts->queue_size).
 	 */
 	ctrl->ctrl.admin_sqsize = NVMF_AQ_DEPTH-1;
-	ctrl->ctrl.sqsize = opts->queue_size;
+	ctrl->ctrl.sqsize = opts->queue_size-1;
 	ctrl->ctrl.kato = opts->kato;
 
 	ret = -ENOMEM;

2.7.4

Did we really decide that? The spec actually explicitly says that a 0 sqsize will be failed:

"If the size is 0h or larger than the controller supports, then a status value of Connect Invalid Parameters shall be returned."

And then it says: "This is a 0?s based value"

Confusing....

Yeah, I think we'll need an errate in the TWG. And I think the current Linux behavior is the sensible one. Jay, do you want to drive this in the WG or I should I bring it up?

I would be happy if all of the 0's based values in the spec were removed, but I think it's a little too late for that...

In this particular case, I'm pretty sure this is not a mistake; the relevant text is copied directly from the NVMe base spec Create I/O Submission Queue command's QSIZE parameter.

What does need to be clarified is the RDMA transport definition. ?The Connect private data contains two queue size-related fields:

- HSQSIZE, which is supposed to be the RDMA QP send depth, but also says "shall be set to the Submission Queue Size (SQSIZE)", while not saying whether it is 0's based.
- HRQSIZE, which is the QP receive depth, which has no equivalent like SQSIZE in the Connect command, and also does not mention being 0's based.

These should hopefully both be 0's based or both not 0's based, but right now, I have no idea what the intent of the spec is.

Thanks,
-- Daniel

(replying back to the list)

So with the comment Dave made, this code in the rdma.c host:

-???????????????priv.hrqsize = cpu_to_le16(queue->queue_size); -???????????????priv.hsqsize = cpu_to_le16(queue->queue_size); +???????????????priv.hrqsize = cpu_to_le16(queue->ctrl->ctrl.sqsize);

priv.hrqsize is wrong according to the NVMe fabrics spec. ?At minimum it should be implemented as:

priv.hrqsize = cpu_to_le16(queue->ctrl->ctrl.sqsize + 1);

+???????????????priv.hsqsize = cpu_to_le16(queue->ctrl->ctrl.sqsize);

and this line remains the same because sqsize is 0 based and the spec clearly says hsqsize shall be set to sqsize.

I see the confusion for hrqsize and I think we should try and get it cleared up in errata, but I think sqsize and hsqsize the way it's stated in the spec is pretty clear.

I think 0 is disallowed because that would mean the queue size is 1, and NVMe requires at least a queue size of 2 to be useful at all (one slot has to be empty to avoid confusion between queue full and queue empty cases).

-- Daniel