* [PATCH 1/2] nvme: Introduce nvme_is_aen_req function @ 2018-12-12 6:55 Nitzan Carmi 2018-12-12 6:55 ` [PATCH 2/2] nvme-rdma: Handle completions if error_recovery fails Nitzan Carmi 2018-12-12 7:46 ` [PATCH 1/2] nvme: Introduce nvme_is_aen_req function Sagi Grimberg 0 siblings, 2 replies; 6+ messages in thread From: Nitzan Carmi @ 2018-12-12 6:55 UTC (permalink / raw) From: Israel Rukshin <israelr@mellanox.com> Signed-off-by: Israel Rukshin <israelr at mellanox.com> Reviewed-by: Max Gurtovoy <maxg at mellanox.com> --- drivers/nvme/host/nvme.h | 5 +++++ drivers/nvme/host/pci.c | 3 +-- drivers/nvme/host/rdma.c | 4 ++-- 3 files changed, 8 insertions(+), 4 deletions(-) diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h index 081cbdcce880..8330f41991ad 100644 --- a/drivers/nvme/host/nvme.h +++ b/drivers/nvme/host/nvme.h @@ -407,6 +407,11 @@ static inline void nvme_put_ctrl(struct nvme_ctrl *ctrl) put_device(ctrl->device); } +static inline bool nvme_is_aen_req(u16 qid, __u16 command_id) +{ + return (!qid && command_id >= NVME_AQ_BLK_MQ_DEPTH); +} + void nvme_complete_rq(struct request *req); void nvme_cancel_request(struct request *req, void *data, bool reserved); bool nvme_change_ctrl_state(struct nvme_ctrl *ctrl, diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c index c33bb201b884..04af49f2643f 100644 --- a/drivers/nvme/host/pci.c +++ b/drivers/nvme/host/pci.c @@ -891,8 +891,7 @@ static inline void nvme_handle_cqe(struct nvme_queue *nvmeq, u16 idx) * aborts. We don't even bother to allocate a struct request * for them but rather special case them here. */ - if (unlikely(nvmeq->qid == 0 && - cqe->command_id >= NVME_AQ_BLK_MQ_DEPTH)) { + if (unlikely(nvme_is_aen_req(nvmeq->qid, cqe->command_id))) { nvme_complete_async_event(&nvmeq->dev->ctrl, cqe->status, &cqe->result); return; diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c index ab6ec7295bf9..a9eed697505e 100644 --- a/drivers/nvme/host/rdma.c +++ b/drivers/nvme/host/rdma.c @@ -1481,8 +1481,8 @@ static int __nvme_rdma_recv_done(struct ib_cq *cq, struct ib_wc *wc, int tag) * aborts. We don't even bother to allocate a struct request * for them but rather special case them here. */ - if (unlikely(nvme_rdma_queue_idx(queue) == 0 && - cqe->command_id >= NVME_AQ_BLK_MQ_DEPTH)) + if (unlikely(nvme_is_aen_req(nvme_rdma_queue_idx(queue), + cqe->command_id))) nvme_complete_async_event(&queue->ctrl->ctrl, cqe->status, &cqe->result); else -- 2.14.3 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 2/2] nvme-rdma: Handle completions if error_recovery fails 2018-12-12 6:55 [PATCH 1/2] nvme: Introduce nvme_is_aen_req function Nitzan Carmi @ 2018-12-12 6:55 ` Nitzan Carmi 2018-12-12 7:56 ` Sagi Grimberg 2018-12-12 7:46 ` [PATCH 1/2] nvme: Introduce nvme_is_aen_req function Sagi Grimberg 1 sibling, 1 reply; 6+ messages in thread From: Nitzan Carmi @ 2018-12-12 6:55 UTC (permalink / raw) Error recovery might not be executed, in case ctrl.state cannot be moved to RESETTING state. This might happen in: - state CONNECTING - during nvmf_connect command failure. - state DELETING - during nvmf_set_features command failure (in nvme_shutdown_ctrl). We need to complete these hanging requests with DNR error. Signed-off-by: Nitzan Carmi <nitzanc at mellanox.com> Signed-off-by: Israel Rukshin <israelr at mellanox.com> Reviewed-by: Max Gurtovoy <maxg at mellanox.com> --- drivers/nvme/host/rdma.c | 101 +++++++++++++++++++++++++++++++++++------------ 1 file changed, 76 insertions(+), 25 deletions(-) diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c index a9eed697505e..430dc5e6c77e 100644 --- a/drivers/nvme/host/rdma.c +++ b/drivers/nvme/host/rdma.c @@ -1055,32 +1055,52 @@ static void nvme_rdma_error_recovery_work(struct work_struct *work) nvme_rdma_reconnect_or_remove(ctrl); } -static void nvme_rdma_error_recovery(struct nvme_rdma_ctrl *ctrl) +static int nvme_rdma_error_recovery(struct nvme_rdma_ctrl *ctrl) { - if (!nvme_change_ctrl_state(&ctrl->ctrl, NVME_CTRL_RESETTING)) - return; + if (!nvme_change_ctrl_state(&ctrl->ctrl, NVME_CTRL_RESETTING)) { + if (ctrl->ctrl.state == NVME_CTRL_RESETTING) + return 0; + else + return -EAGAIN; + } queue_work(nvme_wq, &ctrl->err_work); + return 0; } -static void nvme_rdma_wr_error(struct ib_cq *cq, struct ib_wc *wc, +static int nvme_rdma_wr_error(struct nvme_rdma_ctrl *ctrl, struct ib_wc *wc, const char *op) { - struct nvme_rdma_queue *queue = cq->cq_context; - struct nvme_rdma_ctrl *ctrl = queue->ctrl; - if (ctrl->ctrl.state == NVME_CTRL_LIVE) dev_info(ctrl->ctrl.device, "%s for CQE 0x%p failed with status %s (%d)\n", op, wc->wr_cqe, ib_wc_status_msg(wc->status), wc->status); - nvme_rdma_error_recovery(ctrl); + return nvme_rdma_error_recovery(ctrl); +} + +static void nvme_rdma_req_error(struct nvme_rdma_request *req, struct ib_wc *wc, + const char *op) +{ + struct nvme_rdma_queue *queue = req->queue; + struct nvme_rdma_ctrl *ctrl = queue->ctrl; + + if (nvme_rdma_wr_error(ctrl, wc, op) && + wc->status != IB_WC_WR_FLUSH_ERR) { + req->status = cpu_to_le16((NVME_SC_ABORT_REQ | + NVME_SC_DNR) << 1); + nvme_end_request(blk_mq_rq_from_pdu(req), req->status, + req->result); + } } static void nvme_rdma_memreg_done(struct ib_cq *cq, struct ib_wc *wc) { + struct nvme_rdma_request *req = + container_of(wc->wr_cqe, struct nvme_rdma_request, reg_cqe); + if (unlikely(wc->status != IB_WC_SUCCESS)) - nvme_rdma_wr_error(cq, wc, "MEMREG"); + nvme_rdma_req_error(req, wc, "MEMREG"); } static void nvme_rdma_inv_rkey_done(struct ib_cq *cq, struct ib_wc *wc) @@ -1090,7 +1110,7 @@ static void nvme_rdma_inv_rkey_done(struct ib_cq *cq, struct ib_wc *wc) struct request *rq = blk_mq_rq_from_pdu(req); if (unlikely(wc->status != IB_WC_SUCCESS)) { - nvme_rdma_wr_error(cq, wc, "LOCAL_INV"); + nvme_rdma_req_error(req, wc, "LOCAL_INV"); return; } @@ -1304,7 +1324,7 @@ static void nvme_rdma_send_done(struct ib_cq *cq, struct ib_wc *wc) struct request *rq = blk_mq_rq_from_pdu(req); if (unlikely(wc->status != IB_WC_SUCCESS)) { - nvme_rdma_wr_error(cq, wc, "SEND"); + nvme_rdma_req_error(req, wc, "SEND"); return; } @@ -1380,8 +1400,10 @@ static struct blk_mq_tags *nvme_rdma_tagset(struct nvme_rdma_queue *queue) static void nvme_rdma_async_done(struct ib_cq *cq, struct ib_wc *wc) { + struct nvme_rdma_queue *queue = cq->cq_context; + if (unlikely(wc->status != IB_WC_SUCCESS)) - nvme_rdma_wr_error(cq, wc, "ASYNC"); + nvme_rdma_wr_error(queue->ctrl, wc, "ASYNC"); } static void nvme_rdma_submit_async_event(struct nvme_ctrl *arg) @@ -1411,26 +1433,39 @@ static void nvme_rdma_submit_async_event(struct nvme_ctrl *arg) WARN_ON_ONCE(ret); } -static int nvme_rdma_process_nvme_rsp(struct nvme_rdma_queue *queue, - struct nvme_completion *cqe, struct ib_wc *wc, int tag) +static struct nvme_rdma_request *nvme_rdma_comp_to_req( + struct nvme_rdma_queue *queue, struct nvme_completion *cqe) { struct request *rq; struct nvme_rdma_request *req; - int ret = 0; rq = blk_mq_tag_to_rq(nvme_rdma_tagset(queue), cqe->command_id); if (!rq) { dev_err(queue->ctrl->ctrl.device, "tag 0x%x on QP %#x not found\n", cqe->command_id, queue->qp->qp_num); - nvme_rdma_error_recovery(queue->ctrl); - return ret; + return NULL; } req = blk_mq_rq_to_pdu(rq); - req->status = cqe->status; req->result = cqe->result; + return req; +} + +static int nvme_rdma_process_nvme_rsp(struct nvme_rdma_queue *queue, + struct nvme_completion *cqe, struct ib_wc *wc, int tag) +{ + struct request *rq; + struct nvme_rdma_request *req; + int ret = 0; + + req = nvme_rdma_comp_to_req(queue, cqe); + if (!req) { + nvme_rdma_error_recovery(queue->ctrl); + return ret; + } + if (wc->wc_flags & IB_WC_WITH_INVALIDATE) { if (unlikely(wc->ex.invalidate_rkey != req->mr->rkey)) { dev_err(queue->ctrl->ctrl.device, @@ -1451,6 +1486,7 @@ static int nvme_rdma_process_nvme_rsp(struct nvme_rdma_queue *queue, } if (refcount_dec_and_test(&req->ref)) { + rq = blk_mq_rq_from_pdu(req); if (rq->tag == tag) ret = 1; nvme_end_request(rq, req->status, req->result); @@ -1466,11 +1502,21 @@ static int __nvme_rdma_recv_done(struct ib_cq *cq, struct ib_wc *wc, int tag) struct nvme_rdma_queue *queue = cq->cq_context; struct ib_device *ibdev = queue->device->dev; struct nvme_completion *cqe = qe->data; + struct nvme_rdma_request *req; const size_t len = sizeof(struct nvme_completion); int ret = 0; if (unlikely(wc->status != IB_WC_SUCCESS)) { - nvme_rdma_wr_error(cq, wc, "RECV"); + if (!nvme_is_aen_req(nvme_rdma_queue_idx(queue), + cqe->command_id) && + wc->status != IB_WC_WR_FLUSH_ERR) { + req = nvme_rdma_comp_to_req(queue, cqe); + if (req) { + nvme_rdma_req_error(req, wc, "RECV"); + return 0; + } + } + nvme_rdma_wr_error(queue->ctrl, wc, "RECV"); return 0; } @@ -1679,16 +1725,21 @@ static enum blk_eh_timer_return nvme_rdma_timeout(struct request *rq, bool reserved) { struct nvme_rdma_request *req = blk_mq_rq_to_pdu(rq); + struct nvme_rdma_ctrl *ctrl = req->queue->ctrl; - dev_warn(req->queue->ctrl->ctrl.device, + dev_warn(ctrl->ctrl.device, "I/O %d QID %d timeout, reset controller\n", rq->tag, nvme_rdma_queue_idx(req->queue)); - /* queue error recovery */ - nvme_rdma_error_recovery(req->queue->ctrl); - - /* fail with DNR on cmd timeout */ - nvme_req(rq)->status = NVME_SC_ABORT_REQ | NVME_SC_DNR; + /* + * if error recovery succeed it will end the request, + * otherwise we have to manually end it + */ + if (nvme_rdma_error_recovery(ctrl)) { + req->status = cpu_to_le16((NVME_SC_ABORT_REQ | + NVME_SC_DNR) << 1); + nvme_end_request(rq, req->status, req->result); + } return BLK_EH_DONE; } -- 2.14.3 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 2/2] nvme-rdma: Handle completions if error_recovery fails 2018-12-12 6:55 ` [PATCH 2/2] nvme-rdma: Handle completions if error_recovery fails Nitzan Carmi @ 2018-12-12 7:56 ` Sagi Grimberg 2018-12-12 8:12 ` Christoph Hellwig 0 siblings, 1 reply; 6+ messages in thread From: Sagi Grimberg @ 2018-12-12 7:56 UTC (permalink / raw) > Error recovery might not be executed, in case ctrl.state > cannot be moved to RESETTING state. This might happen in: > - state CONNECTING - during nvmf_connect command failure. > - state DELETING - during nvmf_set_features command failure > (in nvme_shutdown_ctrl). > > We need to complete these hanging requests with DNR error. There is quite a lot going on than what you describe. In fact you don't even describe what you are doing here, just the problem which is only a part of the story.. > -static void nvme_rdma_error_recovery(struct nvme_rdma_ctrl *ctrl) > +static int nvme_rdma_error_recovery(struct nvme_rdma_ctrl *ctrl) > { > - if (!nvme_change_ctrl_state(&ctrl->ctrl, NVME_CTRL_RESETTING)) > - return; > + if (!nvme_change_ctrl_state(&ctrl->ctrl, NVME_CTRL_RESETTING)) { > + if (ctrl->ctrl.state == NVME_CTRL_RESETTING) > + return 0; > + else > + return -EAGAIN; > + } > > queue_work(nvme_wq, &ctrl->err_work); > + return 0; > } > > @@ -1679,16 +1725,21 @@ static enum blk_eh_timer_return > nvme_rdma_timeout(struct request *rq, bool reserved) > { > struct nvme_rdma_request *req = blk_mq_rq_to_pdu(rq); > + struct nvme_rdma_ctrl *ctrl = req->queue->ctrl; > > - dev_warn(req->queue->ctrl->ctrl.device, > + dev_warn(ctrl->ctrl.device, > "I/O %d QID %d timeout, reset controller\n", > rq->tag, nvme_rdma_queue_idx(req->queue)); > > - /* queue error recovery */ > - nvme_rdma_error_recovery(req->queue->ctrl); > - > - /* fail with DNR on cmd timeout */ > - nvme_req(rq)->status = NVME_SC_ABORT_REQ | NVME_SC_DNR; > + /* > + * if error recovery succeed it will end the request, > + * otherwise we have to manually end it > + */ > + if (nvme_rdma_error_recovery(ctrl)) { > + req->status = cpu_to_le16((NVME_SC_ABORT_REQ | > + NVME_SC_DNR) << 1); > + nvme_end_request(rq, req->status, req->result); > + } > > return BLK_EH_DONE; > } > Can't you live with just the two hunks above? and in the timeout handler you need to stop the queue before you complete the I/O to barrier against competing completions. Also, lets get rid of DNR while we're at it. I don't see why you need the others... I need further explanation for that. ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 2/2] nvme-rdma: Handle completions if error_recovery fails 2018-12-12 7:56 ` Sagi Grimberg @ 2018-12-12 8:12 ` Christoph Hellwig 0 siblings, 0 replies; 6+ messages in thread From: Christoph Hellwig @ 2018-12-12 8:12 UTC (permalink / raw) On Tue, Dec 11, 2018@11:56:26PM -0800, Sagi Grimberg wrote: > Can't you live with just the two hunks above? and in the timeout > handler you need to stop the queue before you complete the I/O to > barrier against competing completions. > > Also, lets get rid of DNR while we're at it. > > I don't see why you need the others... I need further explanation for > that. Agreed. Also there is a lot of churn here, some of it needs to be split into well described self-contained prep patches. ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1/2] nvme: Introduce nvme_is_aen_req function 2018-12-12 6:55 [PATCH 1/2] nvme: Introduce nvme_is_aen_req function Nitzan Carmi 2018-12-12 6:55 ` [PATCH 2/2] nvme-rdma: Handle completions if error_recovery fails Nitzan Carmi @ 2018-12-12 7:46 ` Sagi Grimberg 2018-12-12 8:10 ` Christoph Hellwig 1 sibling, 1 reply; 6+ messages in thread From: Sagi Grimberg @ 2018-12-12 7:46 UTC (permalink / raw) Don't really see the point of this wrapper... But if you are going to introduce it, you need to have others use it as well... ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1/2] nvme: Introduce nvme_is_aen_req function 2018-12-12 7:46 ` [PATCH 1/2] nvme: Introduce nvme_is_aen_req function Sagi Grimberg @ 2018-12-12 8:10 ` Christoph Hellwig 0 siblings, 0 replies; 6+ messages in thread From: Christoph Hellwig @ 2018-12-12 8:10 UTC (permalink / raw) On Tue, Dec 11, 2018@11:46:04PM -0800, Sagi Grimberg wrote: > Don't really see the point of this wrapper... But if > you are going to introduce it, you need to have others > use it as well... I have to say I kinda like the helper. But yes, we should convert all the transports over to it. ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-12-12 8:12 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2018-12-12 6:55 [PATCH 1/2] nvme: Introduce nvme_is_aen_req function Nitzan Carmi 2018-12-12 6:55 ` [PATCH 2/2] nvme-rdma: Handle completions if error_recovery fails Nitzan Carmi 2018-12-12 7:56 ` Sagi Grimberg 2018-12-12 8:12 ` Christoph Hellwig 2018-12-12 7:46 ` [PATCH 1/2] nvme: Introduce nvme_is_aen_req function Sagi Grimberg 2018-12-12 8:10 ` Christoph Hellwig
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).