[PATCH 0/7] Some nvme-rdma host patches for 4.15

[PATCH 0/7] Some nvme-rdma host patches for 4.15

[Sagi Grimberg]

A couple of minor fixes. Patch 6 is improving the reconnect
flow to not teardown the admin/io queues in every reconnect
(which doesn't really happen as we check a state on the queues).

Sagi Grimberg (7):
  nvme-rdma: pass tagset to directly nvme_rdma_free_tagset
  nvme-rdma: fix wrong logging message
  nvme-rdma: move assignment to declaration
  nvme-rdma: Check that reinit_request got a proper mr
  nvme-rdma: Don't local invalidate if the queue is not live
  nvme-rdma: teardown admin/io queues once on error recovery
  nvme-rdma: change queue flag semantics DELETING -> ALLOCATED

 drivers/nvme/host/rdma.c | 61 +++++++++++++++++++++++-------------------------
 1 file changed, 29 insertions(+), 32 deletions(-)

-- 
2.7.4

[PATCH 1/7] nvme-rdma: pass tagset to directly nvme_rdma_free_tagset

[Sagi Grimberg]

Instead of flagging admin/io.

Signed-off-by: Sagi Grimberg <sagi at grimberg.me>
---
 drivers/nvme/host/rdma.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
index cf2ac5d893d5..9e040bbc08c9 100644
--- a/drivers/nvme/host/rdma.c
+++ b/drivers/nvme/host/rdma.c
@@ -670,11 +670,10 @@ static int nvme_rdma_alloc_io_queues(struct nvme_rdma_ctrl *ctrl)
 	return ret;
 }
 
-static void nvme_rdma_free_tagset(struct nvme_ctrl *nctrl, bool admin)
+static void nvme_rdma_free_tagset(struct nvme_ctrl *nctrl,
+		struct blk_mq_tag_set *set)
 {
 	struct nvme_rdma_ctrl *ctrl = to_rdma_ctrl(nctrl);
-	struct blk_mq_tag_set *set = admin ?
-			&ctrl->admin_tag_set : &ctrl->tag_set;
 
 	blk_mq_free_tag_set(set);
 	nvme_rdma_dev_put(ctrl->device);
@@ -744,7 +743,7 @@ static void nvme_rdma_destroy_admin_queue(struct nvme_rdma_ctrl *ctrl,
 	nvme_rdma_stop_queue(&ctrl->queues[0]);
 	if (remove) {
 		blk_cleanup_queue(ctrl->ctrl.admin_q);
-		nvme_rdma_free_tagset(&ctrl->ctrl, true);
+		nvme_rdma_free_tagset(&ctrl->ctrl, ctrl->ctrl.admin_tagset);
 	}
 	nvme_rdma_free_queue(&ctrl->queues[0]);
 }
@@ -818,7 +817,7 @@ static int nvme_rdma_configure_admin_queue(struct nvme_rdma_ctrl *ctrl,
 		blk_cleanup_queue(ctrl->ctrl.admin_q);
 out_free_tagset:
 	if (new)
-		nvme_rdma_free_tagset(&ctrl->ctrl, true);
+		nvme_rdma_free_tagset(&ctrl->ctrl, ctrl->ctrl.admin_tagset);
 out_free_queue:
 	nvme_rdma_free_queue(&ctrl->queues[0]);
 	return error;
@@ -830,7 +829,7 @@ static void nvme_rdma_destroy_io_queues(struct nvme_rdma_ctrl *ctrl,
 	nvme_rdma_stop_io_queues(ctrl);
 	if (remove) {
 		blk_cleanup_queue(ctrl->ctrl.connect_q);
-		nvme_rdma_free_tagset(&ctrl->ctrl, false);
+		nvme_rdma_free_tagset(&ctrl->ctrl, ctrl->ctrl.tagset);
 	}
 	nvme_rdma_free_io_queues(ctrl);
 }
@@ -873,7 +872,7 @@ static int nvme_rdma_configure_io_queues(struct nvme_rdma_ctrl *ctrl, bool new)
 		blk_cleanup_queue(ctrl->ctrl.connect_q);
 out_free_tag_set:
 	if (new)
-		nvme_rdma_free_tagset(&ctrl->ctrl, false);
+		nvme_rdma_free_tagset(&ctrl->ctrl, ctrl->ctrl.tagset);
 out_free_io_queues:
 	nvme_rdma_free_io_queues(ctrl);
 	return ret;
-- 
2.7.4

[PATCH 1/7] nvme-rdma: pass tagset to directly nvme_rdma_free_tagset

[Johannes Thumshirn]

Looks good,
Reviewed-by: Johannes Thumshirn <jthumshirn at suse.de>
-- 
Johannes Thumshirn                                          Storage
jthumshirn at suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N?rnberg
GF: Felix Imend?rffer, Jane Smithard, Graham Norton
HRB 21284 (AG N?rnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

[PATCH 1/7] nvme-rdma: pass tagset to directly nvme_rdma_free_tagset

[Max Gurtovoy]

On 10/11/2017 3:29 PM, Sagi Grimberg wrote:
> Instead of flagging admin/io.
> 
> Signed-off-by: Sagi Grimberg <sagi at grimberg.me>
> ---
>   drivers/nvme/host/rdma.c | 13 ++++++-------
>   1 file changed, 6 insertions(+), 7 deletions(-)
> 

Looks good,
Reviewed-by: Max Gurtovoy <maxg at mellanox.com>

[PATCH 2/7] nvme-rdma: fix wrong logging message

[Sagi Grimberg]

Not necessarily address resolution failed.

Signed-off-by: Sagi Grimberg <sagi at grimberg.me>
---
 drivers/nvme/host/rdma.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
index 9e040bbc08c9..09f774abe5a8 100644
--- a/drivers/nvme/host/rdma.c
+++ b/drivers/nvme/host/rdma.c
@@ -544,7 +544,7 @@ static int nvme_rdma_alloc_queue(struct nvme_rdma_ctrl *ctrl,
 	ret = nvme_rdma_wait_for_cm(queue);
 	if (ret) {
 		dev_info(ctrl->ctrl.device,
-			"rdma_resolve_addr wait failed (%d).\n", ret);
+			"rdma connection establishment failed (%d)\n", ret);
 		goto out_destroy_cm_id;
 	}
 
-- 
2.7.4

[PATCH 2/7] nvme-rdma: fix wrong logging message

[Johannes Thumshirn]

heh yeah that one's puzzled me quite a bit the 1st time I tried nvme-rdma,
Reviewed-by: Johannes Thumshirn <jthumshirn at suse.de>
-- 
Johannes Thumshirn                                          Storage
jthumshirn at suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N?rnberg
GF: Felix Imend?rffer, Jane Smithard, Graham Norton
HRB 21284 (AG N?rnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

[PATCH 2/7] nvme-rdma: fix wrong logging message

[Max Gurtovoy]

On 10/11/2017 3:29 PM, Sagi Grimberg wrote:
> Not necessarily address resolution failed.
> 
> Signed-off-by: Sagi Grimberg <sagi at grimberg.me>
> ---
>   drivers/nvme/host/rdma.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 

Looks good,
Reviewed-by: Max Gurtovoy <maxg at mellanox.com>

[PATCH 3/7] nvme-rdma: move assignment to declaration

[Sagi Grimberg]

No need for the extra line for trivial assignments.

Signed-off-by: Sagi Grimberg <sagi at grimberg.me>
---
 drivers/nvme/host/rdma.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
index 09f774abe5a8..6e62657662fd 100644
--- a/drivers/nvme/host/rdma.c
+++ b/drivers/nvme/host/rdma.c
@@ -434,11 +434,9 @@ nvme_rdma_find_get_device(struct rdma_cm_id *cm_id)
 
 static void nvme_rdma_destroy_queue_ib(struct nvme_rdma_queue *queue)
 {
-	struct nvme_rdma_device *dev;
-	struct ib_device *ibdev;
+	struct nvme_rdma_device *dev = queue->device;
+	struct ib_device *ibdev = dev->dev;
 
-	dev = queue->device;
-	ibdev = dev->dev;
 	rdma_destroy_qp(queue->cm_id);
 	ib_free_cq(queue->ib_cq);
 
-- 
2.7.4

[PATCH 3/7] nvme-rdma: move assignment to declaration

[Johannes Thumshirn]

Fair enough,
Reviewed-by: Johannes Thumshirn <jthumshirn at suse.de>
-- 
Johannes Thumshirn                                          Storage
jthumshirn at suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N?rnberg
GF: Felix Imend?rffer, Jane Smithard, Graham Norton
HRB 21284 (AG N?rnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

[PATCH 3/7] nvme-rdma: move assignment to declaration

[Max Gurtovoy]

On 10/11/2017 3:29 PM, Sagi Grimberg wrote:
> No need for the extra line for trivial assignments.
> 
> Signed-off-by: Sagi Grimberg <sagi at grimberg.me>

Looks fine,
Reviewed-by: Max Gurtovoy <maxg at mellanox.com>

[PATCH 4/7] nvme-rdma: Check that reinit_request got a proper mr

[Sagi Grimberg]

Warn if req->mr is NULL as it should never happen.

Signed-off-by: Sagi Grimberg <sagi at grimberg.me>
---
 drivers/nvme/host/rdma.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
index 6e62657662fd..93f4dd0b6f26 100644
--- a/drivers/nvme/host/rdma.c
+++ b/drivers/nvme/host/rdma.c
@@ -274,6 +274,9 @@ static int nvme_rdma_reinit_request(void *data, struct request *rq)
 	struct nvme_rdma_request *req = blk_mq_rq_to_pdu(rq);
 	int ret = 0;
 
+	if (WARN_ON_ONCE(!req->mr))
+		return 0;
+
 	ib_dereg_mr(req->mr);
 
 	req->mr = ib_alloc_mr(dev->pd, IB_MR_TYPE_MEM_REG,
-- 
2.7.4

[PATCH 4/7] nvme-rdma: Check that reinit_request got a proper mr

[Johannes Thumshirn]

Out of curiosity, did this happen to you as a motivation for the patch?

Anyways,
Reviewed-by: Johannes Thumshirn <jthumshirn at suse.de>
-- 
Johannes Thumshirn                                          Storage
jthumshirn at suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N?rnberg
GF: Felix Imend?rffer, Jane Smithard, Graham Norton
HRB 21284 (AG N?rnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

[PATCH 5/7] nvme-rdma: Don't local invalidate if the queue is not live

[Sagi Grimberg]

No chance for the local invalidate to succeed if the queue-pair
is in error state. Most likely the target will do a remote
invalidation of our mr so not a big loss on the test_bit.

Signed-off-by: Sagi Grimberg <sagi at grimberg.me>
---
 drivers/nvme/host/rdma.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
index 93f4dd0b6f26..e3fe57011fcb 100644
--- a/drivers/nvme/host/rdma.c
+++ b/drivers/nvme/host/rdma.c
@@ -1055,7 +1055,7 @@ static void nvme_rdma_unmap_data(struct nvme_rdma_queue *queue,
 	if (!blk_rq_bytes(rq))
 		return;
 
-	if (req->mr->need_inval) {
+	if (req->mr->need_inval && test_bit(NVME_RDMA_Q_LIVE, &req->queue->flags)) {
 		res = nvme_rdma_inv_rkey(queue, req);
 		if (unlikely(res < 0)) {
 			dev_err(ctrl->ctrl.device,
-- 
2.7.4

[PATCH 6/7] nvme-rdma: teardown admin/io queues once on error recovery

[Sagi Grimberg]

Relying on the queue state while tearing down on every reconnect
attempt is not a good design. We should do it once in err_work
and simply try to establish the queues for each reconnect attempt.

Signed-off-by: Sagi Grimberg <sagi at grimberg.me>
---
 drivers/nvme/host/rdma.c | 27 ++++++++++++---------------
 1 file changed, 12 insertions(+), 15 deletions(-)

diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
index e3fe57011fcb..2a18465bf361 100644
--- a/drivers/nvme/host/rdma.c
+++ b/drivers/nvme/host/rdma.c
@@ -925,10 +925,6 @@ static void nvme_rdma_reconnect_ctrl_work(struct work_struct *work)
 
 	++ctrl->ctrl.nr_reconnects;
 
-	if (ctrl->ctrl.queue_count > 1)
-		nvme_rdma_destroy_io_queues(ctrl, false);
-
-	nvme_rdma_destroy_admin_queue(ctrl, false);
 	ret = nvme_rdma_configure_admin_queue(ctrl, false);
 	if (ret)
 		goto requeue;
@@ -936,7 +932,7 @@ static void nvme_rdma_reconnect_ctrl_work(struct work_struct *work)
 	if (ctrl->ctrl.queue_count > 1) {
 		ret = nvme_rdma_configure_io_queues(ctrl, false);
 		if (ret)
-			goto requeue;
+			goto destroy_admin;
 	}
 
 	changed = nvme_change_ctrl_state(&ctrl->ctrl, NVME_CTRL_LIVE);
@@ -946,14 +942,17 @@ static void nvme_rdma_reconnect_ctrl_work(struct work_struct *work)
 		return;
 	}
 
-	ctrl->ctrl.nr_reconnects = 0;
-
 	nvme_start_ctrl(&ctrl->ctrl);
 
-	dev_info(ctrl->ctrl.device, "Successfully reconnected\n");
+	dev_info(ctrl->ctrl.device, "Successfully reconnected (%d attepmpt)\n",
+			ctrl->ctrl.nr_reconnects);
+
+	ctrl->ctrl.nr_reconnects = 0;
 
 	return;
 
+destroy_admin:
+	nvme_rdma_destroy_admin_queue(ctrl, false);
 requeue:
 	dev_info(ctrl->ctrl.device, "Failed reconnect attempt %d\n",
 			ctrl->ctrl.nr_reconnects);
@@ -969,17 +968,15 @@ static void nvme_rdma_error_recovery_work(struct work_struct *work)
 
 	if (ctrl->ctrl.queue_count > 1) {
 		nvme_stop_queues(&ctrl->ctrl);
-		nvme_rdma_stop_io_queues(ctrl);
-	}
-	blk_mq_quiesce_queue(ctrl->ctrl.admin_q);
-	nvme_rdma_stop_queue(&ctrl->queues[0]);
-
-	/* We must take care of fastfail/requeue all our inflight requests */
-	if (ctrl->ctrl.queue_count > 1)
 		blk_mq_tagset_busy_iter(&ctrl->tag_set,
 					nvme_cancel_request, &ctrl->ctrl);
+		nvme_rdma_destroy_io_queues(ctrl, false);
+	}
+
+	blk_mq_quiesce_queue(ctrl->ctrl.admin_q);
 	blk_mq_tagset_busy_iter(&ctrl->admin_tag_set,
 				nvme_cancel_request, &ctrl->ctrl);
+	nvme_rdma_destroy_admin_queue(ctrl, false);
 
 	/*
 	 * queues are not a live anymore, so restart the queues to fail fast
-- 
2.7.4

[PATCH 6/7] nvme-rdma: teardown admin/io queues once on error recovery

[Nitzan Carmi]

On 11/10/2017 15:29, Sagi Grimberg wrote:
> Relying on the queue state while tearing down on every reconnect
> attempt is not a good design. We should do it once in err_work
> and simply try to establish the queues for each reconnect attempt.
>
> Signed-off-by: Sagi Grimberg <sagi at grimberg.me>
> ---
>   drivers/nvme/host/rdma.c | 27 ++++++++++++---------------
>   1 file changed, 12 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
> index e3fe57011fcb..2a18465bf361 100644
> --- a/drivers/nvme/host/rdma.c
> +++ b/drivers/nvme/host/rdma.c
> @@ -925,10 +925,6 @@ static void nvme_rdma_reconnect_ctrl_work(struct work_struct *work)
>   
>   	++ctrl->ctrl.nr_reconnects;
>   
> -	if (ctrl->ctrl.queue_count > 1)
> -		nvme_rdma_destroy_io_queues(ctrl, false);
> -
> -	nvme_rdma_destroy_admin_queue(ctrl, false);
>   	ret = nvme_rdma_configure_admin_queue(ctrl, false);
>   	if (ret)
>   		goto requeue;
> @@ -936,7 +932,7 @@ static void nvme_rdma_reconnect_ctrl_work(struct work_struct *work)
>   	if (ctrl->ctrl.queue_count > 1) {
>   		ret = nvme_rdma_configure_io_queues(ctrl, false);
>   		if (ret)
> -			goto requeue;
> +			goto destroy_admin;
>   	}
>   
>   	changed = nvme_change_ctrl_state(&ctrl->ctrl, NVME_CTRL_LIVE);
> @@ -946,14 +942,17 @@ static void nvme_rdma_reconnect_ctrl_work(struct work_struct *work)
>   		return;
>   	}
>   
> -	ctrl->ctrl.nr_reconnects = 0;
> -
>   	nvme_start_ctrl(&ctrl->ctrl);
>   
> -	dev_info(ctrl->ctrl.device, "Successfully reconnected\n");
> +	dev_info(ctrl->ctrl.device, "Successfully reconnected (%d attepmpt)\n",
typo :)
> +			ctrl->ctrl.nr_reconnects);
> +
> +	ctrl->ctrl.nr_reconnects = 0;
>   
>   	return;
>   
> +destroy_admin:
> +	nvme_rdma_destroy_admin_queue(ctrl, false);
>   requeue:
>   	dev_info(ctrl->ctrl.device, "Failed reconnect attempt %d\n",
>   			ctrl->ctrl.nr_reconnects);
> @@ -969,17 +968,15 @@ static void nvme_rdma_error_recovery_work(struct work_struct *work)
>   
>   	if (ctrl->ctrl.queue_count > 1) {
>   		nvme_stop_queues(&ctrl->ctrl);
> -		nvme_rdma_stop_io_queues(ctrl);
> -	}
> -	blk_mq_quiesce_queue(ctrl->ctrl.admin_q);
> -	nvme_rdma_stop_queue(&ctrl->queues[0]);
> -
> -	/* We must take care of fastfail/requeue all our inflight requests */
> -	if (ctrl->ctrl.queue_count > 1)
>   		blk_mq_tagset_busy_iter(&ctrl->tag_set,
>   					nvme_cancel_request, &ctrl->ctrl);
> +		nvme_rdma_destroy_io_queues(ctrl, false);
> +	}
> +
> +	blk_mq_quiesce_queue(ctrl->ctrl.admin_q);
>   	blk_mq_tagset_busy_iter(&ctrl->admin_tag_set,
>   				nvme_cancel_request, &ctrl->ctrl);
> +	nvme_rdma_destroy_admin_queue(ctrl, false);
>   
>   	/*
>   	 * queues are not a live anymore, so restart the queues to fail fast

[PATCH 7/7] nvme-rdma: change queue flag semantics DELETING -> ALLOCATED

[Sagi Grimberg]

Instead of marking we are deleting, mark we
are allocated and check that instead. This makes
the logic symmetrical to connected mark check.

Signed-off-by: Sagi Grimberg <sagi at grimberg.me>
---
 drivers/nvme/host/rdma.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
index 2a18465bf361..405895b1dff2 100644
--- a/drivers/nvme/host/rdma.c
+++ b/drivers/nvme/host/rdma.c
@@ -79,8 +79,8 @@ struct nvme_rdma_request {
 };
 
 enum nvme_rdma_queue_flags {
-	NVME_RDMA_Q_LIVE		= 0,
-	NVME_RDMA_Q_DELETING		= 1,
+	NVME_RDMA_Q_ALLOCATED		= 0,
+	NVME_RDMA_Q_LIVE		= 1,
 };
 
 struct nvme_rdma_queue {
@@ -549,7 +549,7 @@ static int nvme_rdma_alloc_queue(struct nvme_rdma_ctrl *ctrl,
 		goto out_destroy_cm_id;
 	}
 
-	clear_bit(NVME_RDMA_Q_DELETING, &queue->flags);
+	set_bit(NVME_RDMA_Q_ALLOCATED, &queue->flags);
 
 	return 0;
 
@@ -569,7 +569,7 @@ static void nvme_rdma_stop_queue(struct nvme_rdma_queue *queue)
 
 static void nvme_rdma_free_queue(struct nvme_rdma_queue *queue)
 {
-	if (test_and_set_bit(NVME_RDMA_Q_DELETING, &queue->flags))
+	if (!test_and_clear_bit(NVME_RDMA_Q_ALLOCATED, &queue->flags))
 		return;
 
 	nvme_rdma_destroy_queue_ib(queue);
-- 
2.7.4

[PATCH 0/7] Some nvme-rdma host patches for 4.15

[Christoph Hellwig]

Applied to nvme-4.15 with the typo in patch 6 fixed up.