nvmet-rdma: Invoke fatal error on error completion
Subsystems (3)
Sagi Grimberg
maintainerPATCH 1/1 nvmet-rdma: Invoke fatal error on error completion
In case we got an error completion the rdma queue pair is in error state, teardown the entire controller. Note that in recv or read error completion we might not have a controller yet, so check for the controller exsistence.
--- drivers/nvme/target/rdma.c | 45 +++++++++++++++++++++++++++++++++++++-------- 1 file changed, 37 insertions(+), 8 deletions(-)
drivers/nvme/target/rdma.c +37 -8
@@ -134,6 +134,7 @@ static void nvmet_rdma_send_done(struct ib_cq *cq, struct ib_wc *wc);static void nvmet_rdma_recv_done (struct ib_cq *cq,struct ib_wc *wc);static void nvmet_rdma_read_data_done (struct ib_cq *cq,struct ib_wc *wc);static void nvmet_rdma_qp_event (struct ib_event *event,void *priv); +static void nvmet_rdma_queue_disconnect (struct nvmet_rdma_queue *queue);static struct nvmet_fabrics_ops nvmet_rdma_ops;
@@ -486,12 +487,32 @@ static void nvmet_rdma_release_rsp(struct nvmet_rdma_rsp *rsp)nvmet_rdma_put_rsp (rsp); } +static void nvmet_rdma_error_comp (struct nvmet_rdma_queue *queue) +{ +if (queue->nvme_sq.ctrl) +nvmet_ctrl_fatal_error (queue->nvme_sq.ctrl); +else +/* +* we didn't setup the controller yet in case +* of admin connect error, just disconnect and +* cleanup the queue +*/ +nvmet_rdma_queue_disconnect (queue); +} +static void nvmet_rdma_send_done (struct ib_cq *cq,struct ib_wc *wc) {struct nvmet_rdma_rsp *rsp = container_of(wc->wr_cqe, struct nvmet_rdma_rsp, send_cqe);nvmet_rdma_release_rsp (rsp); + +if (unlikely (wc->status !=IB_WC_SUCCESS && + wc->status !=IB_WC_WR_FLUSH_ERR )) { +pr_err ("SEND for CQE 0x%p failed with status %s (%d).\n" , + wc->wr_cqe,ib_wc_status_msg (wc->status), wc->status); +nvmet_rdma_error_comp (rsp->queue); + } }static void nvmet_rdma_queue_response (struct nvmet_req *req)
@@ -534,11 +555,13 @@ static void nvmet_rdma_read_data_done(struct ib_cq *cq, struct ib_wc *wc) rsp->req.sg_cnt, nvmet_data_dir(&rsp->req)); rsp->n_rdma =0 ; -if (unlikely (wc->status !=IB_WC_SUCCESS && - wc->status !=IB_WC_WR_FLUSH_ERR )) { -pr_info ("RDMA READ for CQE 0x%p failed with status %s (%d).\n" , - wc->wr_cqe,ib_wc_status_msg (wc->status), wc->status); -nvmet_req_complete (&rsp->req,NVME_SC_DATA_XFER_ERROR ); +if (unlikely (wc->status !=IB_WC_SUCCESS )) { +nvmet_rdma_release_rsp (rsp); +if (wc->status !=IB_WC_WR_FLUSH_ERR ) { +pr_info ("RDMA READ for CQE 0x%p failed with status %s (%d).\n" , + wc->wr_cqe,ib_wc_status_msg (wc->status), wc->status); +nvmet_rdma_error_comp (queue); + }return ; }
@@ -705,13 +728,19 @@ static void nvmet_rdma_recv_done(struct ib_cq *cq, struct ib_wc *wc)struct nvmet_rdma_queue *queue = cq->cq_context;struct nvmet_rdma_rsp *rsp; -if (unlikely (wc->status !=IB_WC_SUCCESS )) +if (unlikely (wc->status !=IB_WC_SUCCESS )) { +if (wc->status !=IB_WC_WR_FLUSH_ERR ) { +pr_err ("RECV for CQE 0x%p failed with status %s (%d)\n" , + wc->wr_cqe,ib_wc_status_msg (wc->status), + wc->status); +nvmet_rdma_error_comp (queue); + }return ; + }if (unlikely (wc->byte_len <sizeof (struct nvme_command ))) {pr_err ("Ctrl Fatal Error: capsule size less than 64 bytes\n" ); -if (queue->nvme_sq.ctrl) -nvmet_ctrl_fatal_error (queue->nvme_sq.ctrl); +nvmet_rdma_error_comp (queue);return ; }
Signature · 1 line
1.9.1
PATCH 1/1 nvmet-rdma: Invoke fatal error on error completion
Quoted 6 lines · Sagi Grimberg
In case we got an error completion the rdma queue pair is in error state, teardown the entire controller. Note that in recv or read error completion we might not have a controller yet, so check for the controller exsistence.
Signed-off-by Sagi Grimberg <sagi at grimberg.me>
This looks fine minus a few minor codingstyle nitpicks that I'd be happy to fix up:
drivers/nvme/target/rdma.c static void nvmet_rdma_release_rsp(struct nvmet_rdma_rsp *rsp) · line 501
Quoted 12 lines
drivers/nvme/target/rdma.c +12
-0+static void nvmet_rdma_error_comp (struct nvmet_rdma_queue *queue) +{ +if (queue->nvme_sq.ctrl) +nvmet_ctrl_fatal_error (queue->nvme_sq.ctrl); +else +/* +* we didn't setup the controller yet in case +* of admin connect error, just disconnect and +* cleanup the queue +*/ +nvmet_rdma_queue_disconnect (queue); +}
With such a long comment I'd prefer to have curly braces just to make the else visually more obvious
drivers/nvme/target/rdma.c static void nvmet_rdma_release_rsp(struct nvmet_rdma_rsp *rsp) · line 511
Quoted 3 lines
drivers/nvme/target/rdma.c +3
-0+ +if (unlikely (wc->status !=IB_WC_SUCCESS && + wc->status !=IB_WC_WR_FLUSH_ERR )) {
Indenting the second line of a condition by a single tab is always wrong, either indent it with two tabs, or so that it aligns with first line. The second is probably nicer here:
if (unlikely(wc->status != IB_WC_SUCCESS &&
wc->status != IB_WC_WR_FLUSH_ERR)) {Given how many !success not !flush_err conditionals we have in various drivers I wonder if we should have a helper in the RDMA core, though.
Sagi Grimberg
maintainerPATCH 1/1 nvmet-rdma: Invoke fatal error on error completion
Quoted 16 lines
drivers/nvme/target/rdma.c static void nvmet_rdma_release_rsp(struct nvmet_rdma_rsp *rsp) · line 501
Quoted 13 lines
drivers/nvme/target/rdma.c +12
-0+static void nvmet_rdma_error_comp (struct nvmet_rdma_queue *queue) +{ +if (queue->nvme_sq.ctrl) +nvmet_ctrl_fatal_error (queue->nvme_sq.ctrl); +else +/* +* we didn't setup the controller yet in case +* of admin connect error, just disconnect and +* cleanup the queue +*/ +nvmet_rdma_queue_disconnect (queue); +}With such a long comment I'd prefer to have curly braces just to make the else visually more obvious
Sure.
Quoted 10 lines
drivers/nvme/target/rdma.c static void nvmet_rdma_release_rsp(struct nvmet_rdma_rsp *rsp) · line 511
Quoted 3 lines
drivers/nvme/target/rdma.c +3
-0+ +if (unlikely (wc->status !=IB_WC_SUCCESS && + wc->status !=IB_WC_WR_FLUSH_ERR )) {Indenting the second line of a condition by a single tab is always wrong, either indent it with two tabs, or so that it aligns with first line. The second is probably nicer here:
if (unlikely(wc->status != IB_WC_SUCCESS && wc->status != IB_WC_WR_FLUSH_ERR)) {
OK, I'll do that.
Quoted 2 lines
Given how many !success not !flush_err conditionals we have in various drivers I wonder if we should have a helper in the RDMA core, though.
They don't always come together. But I can take a look at it. Given that it might bring some slight logic shifts, let's do it incrementally.