[v2,2/3] nvme-rdma: don't complete requests before a send work request has completed

Commit Message

Sagi Grimberg Nov. 20, 2017, 11:12 a.m. UTC

> -	if ((wc->wc_flags & IB_WC_WITH_INVALIDATE) &&
> -	    wc->ex.invalidate_rkey == req->mr->rkey) {
> -		atomic_dec(&req->ref);
> -	} else if (req->mr->need_inval) {
> +	if (req->mr->need_inval &&
> +	    (!(wc->wc_flags & IB_WC_WITH_INVALIDATE) ||
> +	     wc->ex.invalidate_rkey != req->mr->rkey)) {

I've been meaning to change this path because getting a remote
invalidation on a bogus MR is a protocol error and we want to
detect it regardless of req->mr->need_inval.

However, I do agree that we can save a atomic if we bail out
after we queue a local invalidate:

How about this (should be split into two patches):
--

@@ -1335,10 +1333,14 @@ static int nvme_rdma_process_nvme_rsp(struct 
nvme_rdma_queue *queue,
         req->cqe.status = cqe->status;
         req->cqe.result = cqe->result;

-       if ((wc->wc_flags & IB_WC_WITH_INVALIDATE) &&
-           wc->ex.invalidate_rkey == req->mr->rkey) {
+       if (wc->wc_flags & IB_WC_WITH_INVALIDATE) {
+               if (unlikely(wc->ex.invalidate_rkey == req->mr->rkey)) {
+                       dev_err(queue->ctrl->ctrl.device,
+                               "Bogus remote invalidation for rkey %#x\n",
+                               req->mr->rkey);
+                       nvme_rdma_error_recovery(queue->ctrl);
+               }
                 req->mr->need_inval = false;
-               refcount_dec(&req->ref);
         } else if (req->mr->need_inval) {
                 ret = nvme_rdma_inv_rkey(queue, req);
                 if (unlikely(ret < 0)) {
@@ -1347,6 +1349,8 @@ static int nvme_rdma_process_nvme_rsp(struct 
nvme_rdma_queue *queue,
                                 req->mr->rkey, ret);
                         nvme_rdma_error_recovery(queue->ctrl);
                 }
+               /* return here as we can't really end the request */
+               return 0;
         }

         if (refcount_dec_and_test(&req->ref)) {
--
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Christoph Hellwig Nov. 20, 2017, 11:16 a.m. UTC | #1

> +       if (wc->wc_flags & IB_WC_WITH_INVALIDATE) {
> +               if (unlikely(wc->ex.invalidate_rkey == req->mr->rkey)) {

The == here should be a != I think.

> +                       dev_err(queue->ctrl->ctrl.device,
> +                               "Bogus remote invalidation for rkey %#x\n",
> +                               req->mr->rkey);
> +                       nvme_rdma_error_recovery(queue->ctrl);
> +               }
>                 req->mr->need_inval = false;
> -               refcount_dec(&req->ref);
>         } else if (req->mr->need_inval) {
>                 ret = nvme_rdma_inv_rkey(queue, req);
>                 if (unlikely(ret < 0)) {
> @@ -1347,6 +1349,8 @@ static int nvme_rdma_process_nvme_rsp(struct 
> nvme_rdma_queue *queue,
>                                 req->mr->rkey, ret);
>                         nvme_rdma_error_recovery(queue->ctrl);
>                 }
> +               /* return here as we can't really end the request */
> +               return 0;

Maybe replace the comment with

		/* the local invalidation completion will end the request */

Otherwise this looks fine to me.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
index a3802de0f248..d0a5a397ad66 100644
--- a/drivers/nvme/host/rdma.c
+++ b/drivers/nvme/host/rdma.c
@@ -1148,8 +1148,6 @@  static int nvme_rdma_map_sg_fr(struct 
nvme_rdma_queue *queue,
         sg->type = (NVME_KEY_SGL_FMT_DATA_DESC << 4) |
                         NVME_SGL_FMT_INVALIDATE;

-       refcount_inc(&req->ref);
-
         return 0;
  }