nvmet: implement valid sqhd values in completions
Subsystems (2)
PATCH v3 1/1 nvmet: implement valid sqhd values in completions
To support sqhd, for initiators that are following the spec and paying attention to sqhd vs their sqtail values:
- add sqhd to struct nvmet_sq
- initialize sqhd to 0 in nvmet_sq_setup
- rather than propagate the 0's-based qsize value from the connect message which requires a +1 in every sqhd update, and as nothing else references it, convert to 1's-based value in nvmt_sq/cq_setup() calls.
- validate connect message sqsize being non-zero per spec.
- updated assign sqhd for every completion that goes back.
---
v2:
v2: assignment corrected for endianness
v3:
removed if test on req->sq that is unneeded
repost fixed the "comment" indicator in the patch header
drivers/nvme/target/core.c | 12 ++++++------ drivers/nvme/target/fabrics-cmd.c | 9 +++++++-- drivers/nvme/target/nvmet.h | 1 + 3 files changed, 14 insertions(+), 8 deletions(-)
drivers/nvme/target/core.c +6 -6
@@ -390,10 +390,9 @@ static void __nvmet_req_complete(struct nvmet_req *req, u16 status)if (status)nvmet_set_status (req, status); -/* XXX: need to fill in something useful for sq_head */ - req->rsp->sq_head =0 ; -if (likely (req->sq))/* may happen during early failure */ - req->rsp->sq_id =cpu_to_le16 (req->sq->qid); + req->sq->sqhd = (req->sq->sqhd +1 ) % req->sq->size; + req->rsp->sq_head =cpu_to_le16 (req->sq->sqhd); + req->rsp->sq_id =cpu_to_le16 (req->sq->qid); req->rsp->command_id = req->cmd->common.command_id;if (req->ns)
@@ -412,7 +411,7 @@ void nvmet_cq_setup(struct nvmet_ctrl *ctrl, struct nvmet_cq *cq, u16 qid, u16 size) { cq->qid = qid; - cq->size = size; + cq->size = size;/* 1's-based number now ! */ ctrl->cqs[qid] = cq; }
@@ -420,8 +419,9 @@ void nvmet_cq_setup(struct nvmet_ctrl *ctrl, struct nvmet_cq *cq,void nvmet_sq_setup (struct nvmet_ctrl *ctrl,struct nvmet_sq *sq,u16 qid,u16 size) { + sq->sqhd =0 ; sq->qid = qid; - sq->size = size; + sq->size = size;/* 1's-based number now ! */ ctrl->sqs[qid] = sq; }
drivers/nvme/target/fabrics-cmd.c +7 -2
@@ -109,9 +109,14 @@ static u16 nvmet_install_queue(struct nvmet_ctrl *ctrl, struct nvmet_req *req)pr_warn ("queue already connected!\n" );return NVME_SC_CONNECT_CTRL_BUSY |NVME_SC_DNR ; } +if (!sqsize) { +pr_warn ("queue size zero!\n" ); +return NVME_SC_CONNECT_INVALID_PARAM |NVME_SC_DNR ; + } -nvmet_cq_setup (ctrl, req->cq, qid, sqsize); -nvmet_sq_setup (ctrl, req->sq, qid, sqsize); +/* note: convert queue size from 0's-based value to 1's-based value */ +nvmet_cq_setup (ctrl, req->cq, qid, sqsize +1 ); +nvmet_sq_setup (ctrl, req->sq, qid, sqsize +1 );return 0 ; }
drivers/nvme/target/nvmet.h +1 -0
@@ -74,6 +74,7 @@ struct nvmet_sq {struct percpu_ref ref;u16 qid;u16 size; +u16 sqhd;struct completion free_done;struct completion confirm_done; };
Signature · 1 line
2.13.1
PATCH v3 1/1 nvmet: implement valid sqhd values in completions
Thanks,
this looks fine to me:
PATCH v3 1/1 nvmet: implement valid sqhd values in completions
In reply to James SmartQuoted 12 lines · James Smart
To support sqhd, for initiators that are following the spec and paying attention to sqhd vs their sqtail values:
- add sqhd to structnvmet_sq
- initialize sqhd to 0 innvmet_sq_setup
- rather than propagate the 0's-based qsize value from the connect message which requires a +1 in every sqhd update, and as nothing else references it, convert to 1's-based value in nvmt_sq/cq_setup() calls.
- validate connect message sqsize being non-zero per spec.
- updated assign sqhd for every completion that goes back.Signed-off-by James Smart <james.smart at broadcom.com>
Quoted 7 lines
---
v2:
v2: assignment corrected for endianness
v3:
removed if test onreq->sqthat is unneededrepost fixed the "comment" indicator in the patch header
Looks good thanks,
Sagi Grimberg
maintainerPATCH v3 1/1 nvmet: implement valid sqhd values in completions
In reply to James SmartQuoted 30 lines · James Smart
To support sqhd, for initiators that are following the spec and paying attention to sqhd vs their sqtail values:
- add sqhd to structnvmet_sq
- initialize sqhd to 0 innvmet_sq_setup
- rather than propagate the 0's-based qsize value from the connect message which requires a +1 in every sqhd update, and as nothing else references it, convert to 1's-based value in nvmt_sq/cq_setup() calls.
- validate connect message sqsize being non-zero per spec.
- updated assign sqhd for every completion that goes back.Signed-off-by James Smart <james.smart at broadcom.com>---
v2:
v2: assignment corrected for endianness
v3:
removed if test onreq->sqthat is unneededrepost fixed the "comment" indicator in the patch header
drivers/nvme/target/core.c | 12 ++++++------ drivers/nvme/target/fabrics-cmd.c | 9 +++++++-- drivers/nvme/target/nvmet.h | 1 + 3 files changed, 14 insertions(+), 8 deletions(-)drivers/nvme/target/core.c +0
-0
drivers/nvme/target/core.c static void __nvmet_req_complete(struct nvmet_req *req, u16 status) · line 396
Quoted 7 lines
drivers/nvme/target/core.c +0
-4@@ -390,10 +390,9 @@ static void __nvmet_req_complete(struct nvmet_req *req, u16 status)if (status)nvmet_set_status (req, status); -/* XXX: need to fill in something useful for sq_head */ - req->rsp->sq_head =0 ; -if (likely (req->sq))/* may happen during early failure */ - req->rsp->sq_id =cpu_to_le16 (req->sq->qid);
It wasn't documented why it is safe to remove the condition statement.
Besides that, looks good to me,
PATCH v3 1/1 nvmet: implement valid sqhd values in completions
Quoted 4 lines · Sagi Grimberg
drivers/nvme/target/core.c static void __nvmet_req_complete(struct nvmet_req *req, u16 status) · line 396
Quoted 2 lines
drivers/nvme/target/core.c +0
-2-if (likely (req->sq))/* may happen during early failure */ - req->rsp->sq_id =cpu_to_le16 (req->sq->qid);It wasn't documented why it is safe to remove the condition statement.
I'll add something to the commit log based on the discussion on the list on the previous iteration.