[PATCH 0/5] Some fabrics fixes

[PATCH 0/5] Some fabrics fixes

[Sagi Grimberg]

A set of fixes to the nvme-rdma host driver. First two patches relate
to bug reports from Steve Wise on dervice removal handling (and go
together with Steve's iwcm patch set).

Third patch fixes the host behavior that it never shutdown the
controller (wrong check on the controller state).

Last two patches remove redundant calls to nvme_remove_namespaces
which is called from nvme_uninit_ctrl.

Sagi Grimberg (5):
  nvme-rdma: Fix device removal handling
  nvme-rdma: Free the I/O tags when we delete the controller
  nvme-rdma: Make sure to shutdown the controller if we can
  nvme-rdma: Remove duplicate call to nvme_remove_namespaces
  nvme-loop: Remove duplicate call to nvme_remove_namespaces

 drivers/nvme/host/rdma.c   | 62 ++++++++++++++++++++++++----------------------
 drivers/nvme/target/loop.c |  4 +--
 2 files changed, 34 insertions(+), 32 deletions(-)

-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 0/5] Some fabrics fixes

[Sagi Grimberg]

A set of fixes to the nvme-rdma host driver. First two patches relate
to bug reports from Steve Wise on dervice removal handling (and go
together with Steve's iwcm patch set).

Third patch fixes the host behavior that it never shutdown the
controller (wrong check on the controller state).

Last two patches remove redundant calls to nvme_remove_namespaces
which is called from nvme_uninit_ctrl.

Sagi Grimberg (5):
  nvme-rdma: Fix device removal handling
  nvme-rdma: Free the I/O tags when we delete the controller
  nvme-rdma: Make sure to shutdown the controller if we can
  nvme-rdma: Remove duplicate call to nvme_remove_namespaces
  nvme-loop: Remove duplicate call to nvme_remove_namespaces

 drivers/nvme/host/rdma.c   | 62 ++++++++++++++++++++++++----------------------
 drivers/nvme/target/loop.c |  4 +--
 2 files changed, 34 insertions(+), 32 deletions(-)

-- 
1.9.1

[PATCH 1/5] nvme-rdma: Fix device removal handling

[Sagi Grimberg]

Device removal sequence may have crashed because the
controller (and admin queue space) was freed before
we destroyed the admin queue resources. Thus we
want to destroy the admin queue and only then queue
controller deletion and wait for it to complete.

More specifically we:
1. own the controller deletion (make sure we are not
   competing with another deletion).
2. get rid of inflight reconnects if exists (which
   also destroy and create queues).
3. destroy the queue.
4. safely queue controller deletion (and wait for it
   to complete).

Reported-by: Steve Wise <swise-7bPotxP6k4+P2YhJcF5u+vpXobYPEAuW@public.gmane.org>
Signed-off-by: Sagi Grimberg <sagi-NQWnxTmZq1alnMjI0IkVqw@public.gmane.org>
---
 drivers/nvme/host/rdma.c | 41 +++++++++++++++++++++--------------------
 1 file changed, 21 insertions(+), 20 deletions(-)

diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
index 21ecbf3f3603..f391d67e4ee0 100644
--- a/drivers/nvme/host/rdma.c
+++ b/drivers/nvme/host/rdma.c
@@ -169,7 +169,6 @@ MODULE_PARM_DESC(register_always,
 static int nvme_rdma_cm_handler(struct rdma_cm_id *cm_id,
 		struct rdma_cm_event *event);
 static void nvme_rdma_recv_done(struct ib_cq *cq, struct ib_wc *wc);
-static int __nvme_rdma_del_ctrl(struct nvme_rdma_ctrl *ctrl);
 
 /* XXX: really should move to a generic header sooner or later.. */
 static inline void put_unaligned_le24(u32 val, u8 *p)
@@ -1318,37 +1317,39 @@ out_destroy_queue_ib:
  * that caught the event. Since we hold the callout until the controller
  * deletion is completed, we'll deadlock if the controller deletion will
  * call rdma_destroy_id on this queue's cm_id. Thus, we claim ownership
- * of destroying this queue before-hand, destroy the queue resources
- * after the controller deletion completed with the exception of destroying
- * the cm_id implicitely by returning a non-zero rc to the callout.
+ * of destroying this queue before-hand, destroy the queue resources,
+ * then queue the controller deletion which won't destroy this queue and
+ * we destroy the cm_id implicitely by returning a non-zero rc to the callout.
  */
 static int nvme_rdma_device_unplug(struct nvme_rdma_queue *queue)
 {
 	struct nvme_rdma_ctrl *ctrl = queue->ctrl;
-	int ret, ctrl_deleted = 0;
+	int ret;
 
-	/* First disable the queue so ctrl delete won't free it */
-	if (!test_and_clear_bit(NVME_RDMA_Q_CONNECTED, &queue->flags))
-		goto out;
+	/* Own the controller deletion */
+	if (!nvme_change_ctrl_state(&ctrl->ctrl, NVME_CTRL_DELETING))
+		return 0;
 
-	/* delete the controller */
-	ret = __nvme_rdma_del_ctrl(ctrl);
-	if (!ret) {
-		dev_warn(ctrl->ctrl.device,
-			"Got rdma device removal event, deleting ctrl\n");
-		flush_work(&ctrl->delete_work);
+	dev_warn(ctrl->ctrl.device,
+		"Got rdma device removal event, deleting ctrl\n");
 
-		/* Return non-zero so the cm_id will destroy implicitly */
-		ctrl_deleted = 1;
+	/* Get rid of reconnect work if its running */
+	cancel_delayed_work_sync(&ctrl->reconnect_work);
 
+	/* Disable the queue so ctrl delete won't free it */
+	if (test_and_clear_bit(NVME_RDMA_Q_CONNECTED, &queue->flags)) {
 		/* Free this queue ourselves */
-		rdma_disconnect(queue->cm_id);
-		ib_drain_qp(queue->qp);
+		nvme_rdma_stop_queue(queue);
 		nvme_rdma_destroy_queue_ib(queue);
+
+		/* Return non-zero so the cm_id will destroy implicitly */
+		ret = 1;
 	}
 
-out:
-	return ctrl_deleted;
+	/* Queue controller deletion */
+	queue_work(nvme_rdma_wq, &ctrl->delete_work);
+	flush_work(&ctrl->delete_work);
+	return ret;
 }
 
 static int nvme_rdma_cm_handler(struct rdma_cm_id *cm_id,
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/5] nvme-rdma: Fix device removal handling

[Sagi Grimberg]

Device removal sequence may have crashed because the
controller (and admin queue space) was freed before
we destroyed the admin queue resources. Thus we
want to destroy the admin queue and only then queue
controller deletion and wait for it to complete.

More specifically we:
1. own the controller deletion (make sure we are not
   competing with another deletion).
2. get rid of inflight reconnects if exists (which
   also destroy and create queues).
3. destroy the queue.
4. safely queue controller deletion (and wait for it
   to complete).

Reported-by: Steve Wise <swise at opengridcomputing.com>
Signed-off-by: Sagi Grimberg <sagi at grimberg.me>
---
 drivers/nvme/host/rdma.c | 41 +++++++++++++++++++++--------------------
 1 file changed, 21 insertions(+), 20 deletions(-)

diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
index 21ecbf3f3603..f391d67e4ee0 100644
--- a/drivers/nvme/host/rdma.c
+++ b/drivers/nvme/host/rdma.c
@@ -169,7 +169,6 @@ MODULE_PARM_DESC(register_always,
 static int nvme_rdma_cm_handler(struct rdma_cm_id *cm_id,
 		struct rdma_cm_event *event);
 static void nvme_rdma_recv_done(struct ib_cq *cq, struct ib_wc *wc);
-static int __nvme_rdma_del_ctrl(struct nvme_rdma_ctrl *ctrl);
 
 /* XXX: really should move to a generic header sooner or later.. */
 static inline void put_unaligned_le24(u32 val, u8 *p)
@@ -1318,37 +1317,39 @@ out_destroy_queue_ib:
  * that caught the event. Since we hold the callout until the controller
  * deletion is completed, we'll deadlock if the controller deletion will
  * call rdma_destroy_id on this queue's cm_id. Thus, we claim ownership
- * of destroying this queue before-hand, destroy the queue resources
- * after the controller deletion completed with the exception of destroying
- * the cm_id implicitely by returning a non-zero rc to the callout.
+ * of destroying this queue before-hand, destroy the queue resources,
+ * then queue the controller deletion which won't destroy this queue and
+ * we destroy the cm_id implicitely by returning a non-zero rc to the callout.
  */
 static int nvme_rdma_device_unplug(struct nvme_rdma_queue *queue)
 {
 	struct nvme_rdma_ctrl *ctrl = queue->ctrl;
-	int ret, ctrl_deleted = 0;
+	int ret;
 
-	/* First disable the queue so ctrl delete won't free it */
-	if (!test_and_clear_bit(NVME_RDMA_Q_CONNECTED, &queue->flags))
-		goto out;
+	/* Own the controller deletion */
+	if (!nvme_change_ctrl_state(&ctrl->ctrl, NVME_CTRL_DELETING))
+		return 0;
 
-	/* delete the controller */
-	ret = __nvme_rdma_del_ctrl(ctrl);
-	if (!ret) {
-		dev_warn(ctrl->ctrl.device,
-			"Got rdma device removal event, deleting ctrl\n");
-		flush_work(&ctrl->delete_work);
+	dev_warn(ctrl->ctrl.device,
+		"Got rdma device removal event, deleting ctrl\n");
 
-		/* Return non-zero so the cm_id will destroy implicitly */
-		ctrl_deleted = 1;
+	/* Get rid of reconnect work if its running */
+	cancel_delayed_work_sync(&ctrl->reconnect_work);
 
+	/* Disable the queue so ctrl delete won't free it */
+	if (test_and_clear_bit(NVME_RDMA_Q_CONNECTED, &queue->flags)) {
 		/* Free this queue ourselves */
-		rdma_disconnect(queue->cm_id);
-		ib_drain_qp(queue->qp);
+		nvme_rdma_stop_queue(queue);
 		nvme_rdma_destroy_queue_ib(queue);
+
+		/* Return non-zero so the cm_id will destroy implicitly */
+		ret = 1;
 	}
 
-out:
-	return ctrl_deleted;
+	/* Queue controller deletion */
+	queue_work(nvme_rdma_wq, &ctrl->delete_work);
+	flush_work(&ctrl->delete_work);
+	return ret;
 }
 
 static int nvme_rdma_cm_handler(struct rdma_cm_id *cm_id,
-- 
1.9.1

[PATCH 2/5] nvme-rdma: Free the I/O tags when we delete the controller

[Sagi Grimberg]

If we wait until we free the controller (free_ctrl) we might
lose our rdma device without any notification while we still
have open resources (tags mrs and dma mappings).

Instead, destroy the tags with their rdma resources once we
delete the device and not when freeing it.

Note that we don't do that in nvme_rdma_shutdown_ctrl because
controller reset uses it as well and we want to give active I/O
a chance to complete successfully.

Reported-by: Steve Wise <swise-7bPotxP6k4+P2YhJcF5u+vpXobYPEAuW@public.gmane.org>
Signed-off-by: Sagi Grimberg <sagi-NQWnxTmZq1alnMjI0IkVqw@public.gmane.org>
---
 drivers/nvme/host/rdma.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
index f391d67e4ee0..a70eb3cbf656 100644
--- a/drivers/nvme/host/rdma.c
+++ b/drivers/nvme/host/rdma.c
@@ -686,11 +686,6 @@ static void nvme_rdma_free_ctrl(struct nvme_ctrl *nctrl)
 	list_del(&ctrl->list);
 	mutex_unlock(&nvme_rdma_ctrl_mutex);
 
-	if (ctrl->ctrl.tagset) {
-		blk_cleanup_queue(ctrl->ctrl.connect_q);
-		blk_mq_free_tag_set(&ctrl->tag_set);
-		nvme_rdma_dev_put(ctrl->device);
-	}
 	kfree(ctrl->queues);
 	nvmf_free_options(nctrl->opts);
 free_ctrl:
@@ -1666,6 +1661,11 @@ static void nvme_rdma_del_ctrl_work(struct work_struct *work)
 	nvme_remove_namespaces(&ctrl->ctrl);
 	nvme_rdma_shutdown_ctrl(ctrl);
 	nvme_uninit_ctrl(&ctrl->ctrl);
+	if (ctrl->ctrl.tagset) {
+		blk_cleanup_queue(ctrl->ctrl.connect_q);
+		blk_mq_free_tag_set(&ctrl->tag_set);
+		nvme_rdma_dev_put(ctrl->device);
+	}
 	nvme_put_ctrl(&ctrl->ctrl);
 }
 
@@ -1701,6 +1701,11 @@ static void nvme_rdma_remove_ctrl_work(struct work_struct *work)
 
 	nvme_remove_namespaces(&ctrl->ctrl);
 	nvme_uninit_ctrl(&ctrl->ctrl);
+	if (ctrl->ctrl.tagset) {
+		blk_cleanup_queue(ctrl->ctrl.connect_q);
+		blk_mq_free_tag_set(&ctrl->tag_set);
+		nvme_rdma_dev_put(ctrl->device);
+	}
 	nvme_put_ctrl(&ctrl->ctrl);
 }
 
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/5] nvme-rdma: Free the I/O tags when we delete the controller

[Sagi Grimberg]

If we wait until we free the controller (free_ctrl) we might
lose our rdma device without any notification while we still
have open resources (tags mrs and dma mappings).

Instead, destroy the tags with their rdma resources once we
delete the device and not when freeing it.

Note that we don't do that in nvme_rdma_shutdown_ctrl because
controller reset uses it as well and we want to give active I/O
a chance to complete successfully.

Reported-by: Steve Wise <swise at opengridcomputing.com>
Signed-off-by: Sagi Grimberg <sagi at grimberg.me>
---
 drivers/nvme/host/rdma.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
index f391d67e4ee0..a70eb3cbf656 100644
--- a/drivers/nvme/host/rdma.c
+++ b/drivers/nvme/host/rdma.c
@@ -686,11 +686,6 @@ static void nvme_rdma_free_ctrl(struct nvme_ctrl *nctrl)
 	list_del(&ctrl->list);
 	mutex_unlock(&nvme_rdma_ctrl_mutex);
 
-	if (ctrl->ctrl.tagset) {
-		blk_cleanup_queue(ctrl->ctrl.connect_q);
-		blk_mq_free_tag_set(&ctrl->tag_set);
-		nvme_rdma_dev_put(ctrl->device);
-	}
 	kfree(ctrl->queues);
 	nvmf_free_options(nctrl->opts);
 free_ctrl:
@@ -1666,6 +1661,11 @@ static void nvme_rdma_del_ctrl_work(struct work_struct *work)
 	nvme_remove_namespaces(&ctrl->ctrl);
 	nvme_rdma_shutdown_ctrl(ctrl);
 	nvme_uninit_ctrl(&ctrl->ctrl);
+	if (ctrl->ctrl.tagset) {
+		blk_cleanup_queue(ctrl->ctrl.connect_q);
+		blk_mq_free_tag_set(&ctrl->tag_set);
+		nvme_rdma_dev_put(ctrl->device);
+	}
 	nvme_put_ctrl(&ctrl->ctrl);
 }
 
@@ -1701,6 +1701,11 @@ static void nvme_rdma_remove_ctrl_work(struct work_struct *work)
 
 	nvme_remove_namespaces(&ctrl->ctrl);
 	nvme_uninit_ctrl(&ctrl->ctrl);
+	if (ctrl->ctrl.tagset) {
+		blk_cleanup_queue(ctrl->ctrl.connect_q);
+		blk_mq_free_tag_set(&ctrl->tag_set);
+		nvme_rdma_dev_put(ctrl->device);
+	}
 	nvme_put_ctrl(&ctrl->ctrl);
 }
 
-- 
1.9.1

[PATCH 3/5] nvme-rdma: Make sure to shutdown the controller if we can

[Sagi Grimberg]

Relying on ctrl state in nvme_rdma_shutdown_ctrl is wrong because
it will never be NVME_CTRL_LIVE (delete_ctrl or reset_ctrl invoked it).

Instead, check that the admin queue is connected. Note that it is safe
because we can never see a copmeting thread trying to destroy the admin
queue (reset or delete controller).

Signed-off-by: Sagi Grimberg <sagi-NQWnxTmZq1alnMjI0IkVqw@public.gmane.org>
---
 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 a70eb3cbf656..641ab7f91899 100644
--- a/drivers/nvme/host/rdma.c
+++ b/drivers/nvme/host/rdma.c
@@ -1644,7 +1644,7 @@ static void nvme_rdma_shutdown_ctrl(struct nvme_rdma_ctrl *ctrl)
 		nvme_rdma_free_io_queues(ctrl);
 	}
 
-	if (ctrl->ctrl.state == NVME_CTRL_LIVE)
+	if (test_bit(NVME_RDMA_Q_CONNECTED, &ctrl->queues[0].flags))
 		nvme_shutdown_ctrl(&ctrl->ctrl);
 
 	blk_mq_stop_hw_queues(ctrl->ctrl.admin_q);
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 3/5] nvme-rdma: Make sure to shutdown the controller if we can

[Sagi Grimberg]

Relying on ctrl state in nvme_rdma_shutdown_ctrl is wrong because
it will never be NVME_CTRL_LIVE (delete_ctrl or reset_ctrl invoked it).

Instead, check that the admin queue is connected. Note that it is safe
because we can never see a copmeting thread trying to destroy the admin
queue (reset or delete controller).

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 a70eb3cbf656..641ab7f91899 100644
--- a/drivers/nvme/host/rdma.c
+++ b/drivers/nvme/host/rdma.c
@@ -1644,7 +1644,7 @@ static void nvme_rdma_shutdown_ctrl(struct nvme_rdma_ctrl *ctrl)
 		nvme_rdma_free_io_queues(ctrl);
 	}
 
-	if (ctrl->ctrl.state == NVME_CTRL_LIVE)
+	if (test_bit(NVME_RDMA_Q_CONNECTED, &ctrl->queues[0].flags))
 		nvme_shutdown_ctrl(&ctrl->ctrl);
 
 	blk_mq_stop_hw_queues(ctrl->ctrl.admin_q);
-- 
1.9.1

[PATCH 4/5] nvme-rdma: Remove duplicate call to nvme_remove_namespaces

[Sagi Grimberg]

nvme_uninit_ctrl already does that for us.

Signed-off-by: Sagi Grimberg <sagi-NQWnxTmZq1alnMjI0IkVqw@public.gmane.org>
---
 drivers/nvme/host/rdma.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
index 641ab7f91899..bd9b416fe21a 100644
--- a/drivers/nvme/host/rdma.c
+++ b/drivers/nvme/host/rdma.c
@@ -1658,9 +1658,8 @@ static void nvme_rdma_del_ctrl_work(struct work_struct *work)
 	struct nvme_rdma_ctrl *ctrl = container_of(work,
 				struct nvme_rdma_ctrl, delete_work);
 
-	nvme_remove_namespaces(&ctrl->ctrl);
-	nvme_rdma_shutdown_ctrl(ctrl);
 	nvme_uninit_ctrl(&ctrl->ctrl);
+	nvme_rdma_shutdown_ctrl(ctrl);
 	if (ctrl->ctrl.tagset) {
 		blk_cleanup_queue(ctrl->ctrl.connect_q);
 		blk_mq_free_tag_set(&ctrl->tag_set);
@@ -1699,7 +1698,6 @@ static void nvme_rdma_remove_ctrl_work(struct work_struct *work)
 	struct nvme_rdma_ctrl *ctrl = container_of(work,
 				struct nvme_rdma_ctrl, delete_work);
 
-	nvme_remove_namespaces(&ctrl->ctrl);
 	nvme_uninit_ctrl(&ctrl->ctrl);
 	if (ctrl->ctrl.tagset) {
 		blk_cleanup_queue(ctrl->ctrl.connect_q);
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 4/5] nvme-rdma: Remove duplicate call to nvme_remove_namespaces

[Sagi Grimberg]

nvme_uninit_ctrl already does that for us.

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

diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
index 641ab7f91899..bd9b416fe21a 100644
--- a/drivers/nvme/host/rdma.c
+++ b/drivers/nvme/host/rdma.c
@@ -1658,9 +1658,8 @@ static void nvme_rdma_del_ctrl_work(struct work_struct *work)
 	struct nvme_rdma_ctrl *ctrl = container_of(work,
 				struct nvme_rdma_ctrl, delete_work);
 
-	nvme_remove_namespaces(&ctrl->ctrl);
-	nvme_rdma_shutdown_ctrl(ctrl);
 	nvme_uninit_ctrl(&ctrl->ctrl);
+	nvme_rdma_shutdown_ctrl(ctrl);
 	if (ctrl->ctrl.tagset) {
 		blk_cleanup_queue(ctrl->ctrl.connect_q);
 		blk_mq_free_tag_set(&ctrl->tag_set);
@@ -1699,7 +1698,6 @@ static void nvme_rdma_remove_ctrl_work(struct work_struct *work)
 	struct nvme_rdma_ctrl *ctrl = container_of(work,
 				struct nvme_rdma_ctrl, delete_work);
 
-	nvme_remove_namespaces(&ctrl->ctrl);
 	nvme_uninit_ctrl(&ctrl->ctrl);
 	if (ctrl->ctrl.tagset) {
 		blk_cleanup_queue(ctrl->ctrl.connect_q);
-- 
1.9.1

[PATCH 5/5] nvme-loop: Remove duplicate call to nvme_remove_namespaces

[Sagi Grimberg]

nvme_uninit_ctrl already does that for us.

Signed-off-by: Sagi Grimberg <sagi-NQWnxTmZq1alnMjI0IkVqw@public.gmane.org>
---
 drivers/nvme/target/loop.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/nvme/target/loop.c b/drivers/nvme/target/loop.c
index 94e782987cc9..7affd40a6b33 100644
--- a/drivers/nvme/target/loop.c
+++ b/drivers/nvme/target/loop.c
@@ -414,9 +414,8 @@ static void nvme_loop_del_ctrl_work(struct work_struct *work)
 	struct nvme_loop_ctrl *ctrl = container_of(work,
 				struct nvme_loop_ctrl, delete_work);
 
-	nvme_remove_namespaces(&ctrl->ctrl);
-	nvme_loop_shutdown_ctrl(ctrl);
 	nvme_uninit_ctrl(&ctrl->ctrl);
+	nvme_loop_shutdown_ctrl(ctrl);
 	nvme_put_ctrl(&ctrl->ctrl);
 }
 
@@ -501,7 +500,6 @@ out_free_queues:
 	nvme_loop_destroy_admin_queue(ctrl);
 out_disable:
 	dev_warn(ctrl->ctrl.device, "Removing after reset failure\n");
-	nvme_remove_namespaces(&ctrl->ctrl);
 	nvme_uninit_ctrl(&ctrl->ctrl);
 	nvme_put_ctrl(&ctrl->ctrl);
 }
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 5/5] nvme-loop: Remove duplicate call to nvme_remove_namespaces

[Sagi Grimberg]

nvme_uninit_ctrl already does that for us.

Signed-off-by: Sagi Grimberg <sagi at grimberg.me>
---
 drivers/nvme/target/loop.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/nvme/target/loop.c b/drivers/nvme/target/loop.c
index 94e782987cc9..7affd40a6b33 100644
--- a/drivers/nvme/target/loop.c
+++ b/drivers/nvme/target/loop.c
@@ -414,9 +414,8 @@ static void nvme_loop_del_ctrl_work(struct work_struct *work)
 	struct nvme_loop_ctrl *ctrl = container_of(work,
 				struct nvme_loop_ctrl, delete_work);
 
-	nvme_remove_namespaces(&ctrl->ctrl);
-	nvme_loop_shutdown_ctrl(ctrl);
 	nvme_uninit_ctrl(&ctrl->ctrl);
+	nvme_loop_shutdown_ctrl(ctrl);
 	nvme_put_ctrl(&ctrl->ctrl);
 }
 
@@ -501,7 +500,6 @@ out_free_queues:
 	nvme_loop_destroy_admin_queue(ctrl);
 out_disable:
 	dev_warn(ctrl->ctrl.device, "Removing after reset failure\n");
-	nvme_remove_namespaces(&ctrl->ctrl);
 	nvme_uninit_ctrl(&ctrl->ctrl);
 	nvme_put_ctrl(&ctrl->ctrl);
 }
-- 
1.9.1

Re: [PATCH 1/5] nvme-rdma: Fix device removal handling

[Christoph Hellwig]

On Fri, Jul 29, 2016 at 10:57:18PM +0300, Sagi Grimberg wrote:
> Device removal sequence may have crashed because the
> controller (and admin queue space) was freed before
> we destroyed the admin queue resources. Thus we
> want to destroy the admin queue and only then queue
> controller deletion and wait for it to complete.
> 
> More specifically we:
> 1. own the controller deletion (make sure we are not
>    competing with another deletion).
> 2. get rid of inflight reconnects if exists (which
>    also destroy and create queues).
> 3. destroy the queue.
> 4. safely queue controller deletion (and wait for it
>    to complete).
> 
> Reported-by: Steve Wise <swise-7bPotxP6k4+P2YhJcF5u+vpXobYPEAuW@public.gmane.org>
> Signed-off-by: Sagi Grimberg <sagi-NQWnxTmZq1alnMjI0IkVqw@public.gmane.org>

Looks fine,

Reviewed-by: Christoph Hellwig <hch-jcswGhMUV9g@public.gmane.org>
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/5] nvme-rdma: Fix device removal handling

[Christoph Hellwig]

On Fri, Jul 29, 2016@10:57:18PM +0300, Sagi Grimberg wrote:
> Device removal sequence may have crashed because the
> controller (and admin queue space) was freed before
> we destroyed the admin queue resources. Thus we
> want to destroy the admin queue and only then queue
> controller deletion and wait for it to complete.
> 
> More specifically we:
> 1. own the controller deletion (make sure we are not
>    competing with another deletion).
> 2. get rid of inflight reconnects if exists (which
>    also destroy and create queues).
> 3. destroy the queue.
> 4. safely queue controller deletion (and wait for it
>    to complete).
> 
> Reported-by: Steve Wise <swise at opengridcomputing.com>
> Signed-off-by: Sagi Grimberg <sagi at grimberg.me>

Looks fine,

Reviewed-by: Christoph Hellwig <hch at lst.de>

Re: [PATCH 2/5] nvme-rdma: Free the I/O tags when we delete the controller

[Christoph Hellwig]

On Fri, Jul 29, 2016 at 10:57:19PM +0300, Sagi Grimberg wrote:
> If we wait until we free the controller (free_ctrl) we might
> lose our rdma device without any notification while we still
> have open resources (tags mrs and dma mappings).
> 
> Instead, destroy the tags with their rdma resources once we
> delete the device and not when freeing it.
> 
> Note that we don't do that in nvme_rdma_shutdown_ctrl because
> controller reset uses it as well and we want to give active I/O
> a chance to complete successfully.
> 
> Reported-by: Steve Wise <swise-7bPotxP6k4+P2YhJcF5u+vpXobYPEAuW@public.gmane.org>
> Signed-off-by: Sagi Grimberg <sagi-NQWnxTmZq1alnMjI0IkVqw@public.gmane.org>

This looks fine to me, but can we place share the code instead of
duplicating it?  E.g.

static void __nvme_rdma_remove_ctrl(struct nvme_rdma_ctrl *ctrl, bool shutdown)
{
 	nvme_remove_namespaces(&ctrl->ctrl);
	if (shutdown)
	  	nvme_rdma_shutdown_ctrl(ctrl);
  	nvme_uninit_ctrl(&ctrl->ctrl);
	if (ctrl->ctrl.tagset) {
		blk_cleanup_queue(ctrl->ctrl.connect_q);
		blk_mq_free_tag_set(&ctrl->tag_set);
		nvme_rdma_dev_put(ctrl->device);
	}
	nvme_put_ctrl(&ctrl->ctrl);
}

or in a second step we should probably always call shutdown_ctrl
but skip the actual shutdown if the ctrl state doesn't require it.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/5] nvme-rdma: Free the I/O tags when we delete the controller

[Christoph Hellwig]

On Fri, Jul 29, 2016@10:57:19PM +0300, Sagi Grimberg wrote:
> If we wait until we free the controller (free_ctrl) we might
> lose our rdma device without any notification while we still
> have open resources (tags mrs and dma mappings).
> 
> Instead, destroy the tags with their rdma resources once we
> delete the device and not when freeing it.
> 
> Note that we don't do that in nvme_rdma_shutdown_ctrl because
> controller reset uses it as well and we want to give active I/O
> a chance to complete successfully.
> 
> Reported-by: Steve Wise <swise at opengridcomputing.com>
> Signed-off-by: Sagi Grimberg <sagi at grimberg.me>

This looks fine to me, but can we place share the code instead of
duplicating it?  E.g.

static void __nvme_rdma_remove_ctrl(struct nvme_rdma_ctrl *ctrl, bool shutdown)
{
 	nvme_remove_namespaces(&ctrl->ctrl);
	if (shutdown)
	  	nvme_rdma_shutdown_ctrl(ctrl);
  	nvme_uninit_ctrl(&ctrl->ctrl);
	if (ctrl->ctrl.tagset) {
		blk_cleanup_queue(ctrl->ctrl.connect_q);
		blk_mq_free_tag_set(&ctrl->tag_set);
		nvme_rdma_dev_put(ctrl->device);
	}
	nvme_put_ctrl(&ctrl->ctrl);
}

or in a second step we should probably always call shutdown_ctrl
but skip the actual shutdown if the ctrl state doesn't require it.

Re: [PATCH 3/5] nvme-rdma: Make sure to shutdown the controller if we can

[Christoph Hellwig]

On Fri, Jul 29, 2016 at 10:57:20PM +0300, Sagi Grimberg wrote:
> Relying on ctrl state in nvme_rdma_shutdown_ctrl is wrong because
> it will never be NVME_CTRL_LIVE (delete_ctrl or reset_ctrl invoked it).
> 
> Instead, check that the admin queue is connected. Note that it is safe
> because we can never see a copmeting thread trying to destroy the admin
> queue (reset or delete controller).
> 
> Signed-off-by: Sagi Grimberg <sagi-NQWnxTmZq1alnMjI0IkVqw@public.gmane.org>
> ---
>  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 a70eb3cbf656..641ab7f91899 100644
> --- a/drivers/nvme/host/rdma.c
> +++ b/drivers/nvme/host/rdma.c
> @@ -1644,7 +1644,7 @@ static void nvme_rdma_shutdown_ctrl(struct nvme_rdma_ctrl *ctrl)
>  		nvme_rdma_free_io_queues(ctrl);
>  	}
>  
> -	if (ctrl->ctrl.state == NVME_CTRL_LIVE)
> +	if (test_bit(NVME_RDMA_Q_CONNECTED, &ctrl->queues[0].flags))
>  		nvme_shutdown_ctrl(&ctrl->ctrl);
>  
>  	blk_mq_stop_hw_queues(ctrl->ctrl.admin_q);

Maybe the right way to handle this is to unconditionally call
nvme_shutdown_ctrl and make sure we return an early error
on the register write?
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 3/5] nvme-rdma: Make sure to shutdown the controller if we can

[Christoph Hellwig]

On Fri, Jul 29, 2016@10:57:20PM +0300, Sagi Grimberg wrote:
> Relying on ctrl state in nvme_rdma_shutdown_ctrl is wrong because
> it will never be NVME_CTRL_LIVE (delete_ctrl or reset_ctrl invoked it).
> 
> Instead, check that the admin queue is connected. Note that it is safe
> because we can never see a copmeting thread trying to destroy the admin
> queue (reset or delete controller).
> 
> 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 a70eb3cbf656..641ab7f91899 100644
> --- a/drivers/nvme/host/rdma.c
> +++ b/drivers/nvme/host/rdma.c
> @@ -1644,7 +1644,7 @@ static void nvme_rdma_shutdown_ctrl(struct nvme_rdma_ctrl *ctrl)
>  		nvme_rdma_free_io_queues(ctrl);
>  	}
>  
> -	if (ctrl->ctrl.state == NVME_CTRL_LIVE)
> +	if (test_bit(NVME_RDMA_Q_CONNECTED, &ctrl->queues[0].flags))
>  		nvme_shutdown_ctrl(&ctrl->ctrl);
>  
>  	blk_mq_stop_hw_queues(ctrl->ctrl.admin_q);

Maybe the right way to handle this is to unconditionally call
nvme_shutdown_ctrl and make sure we return an early error
on the register write?

Re: [PATCH 4/5] nvme-rdma: Remove duplicate call to nvme_remove_namespaces

[Christoph Hellwig]

> -	nvme_remove_namespaces(&ctrl->ctrl);
> -	nvme_rdma_shutdown_ctrl(ctrl);
>  	nvme_uninit_ctrl(&ctrl->ctrl);
> +	nvme_rdma_shutdown_ctrl(ctrl);

How safe is the reordering here?  The description should explain why
it's fine.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 4/5] nvme-rdma: Remove duplicate call to nvme_remove_namespaces

[Christoph Hellwig]

> -	nvme_remove_namespaces(&ctrl->ctrl);
> -	nvme_rdma_shutdown_ctrl(ctrl);
>  	nvme_uninit_ctrl(&ctrl->ctrl);
> +	nvme_rdma_shutdown_ctrl(ctrl);

How safe is the reordering here?  The description should explain why
it's fine.

Re: [PATCH 2/5] nvme-rdma: Free the I/O tags when we delete the controller

[Sagi Grimberg]

>> If we wait until we free the controller (free_ctrl) we might
>> lose our rdma device without any notification while we still
>> have open resources (tags mrs and dma mappings).
>>
>> Instead, destroy the tags with their rdma resources once we
>> delete the device and not when freeing it.
>>
>> Note that we don't do that in nvme_rdma_shutdown_ctrl because
>> controller reset uses it as well and we want to give active I/O
>> a chance to complete successfully.
>>
>> Reported-by: Steve Wise <swise-7bPotxP6k4+P2YhJcF5u+vpXobYPEAuW@public.gmane.org>
>> Signed-off-by: Sagi Grimberg <sagi-NQWnxTmZq1alnMjI0IkVqw@public.gmane.org>
>
> This looks fine to me, but can we place share the code instead of
> duplicating it?  E.g.
>
> static void __nvme_rdma_remove_ctrl(struct nvme_rdma_ctrl *ctrl, bool shutdown)
> {
>  	nvme_remove_namespaces(&ctrl->ctrl);
> 	if (shutdown)
> 	  	nvme_rdma_shutdown_ctrl(ctrl);
>   	nvme_uninit_ctrl(&ctrl->ctrl);
> 	if (ctrl->ctrl.tagset) {
> 		blk_cleanup_queue(ctrl->ctrl.connect_q);
> 		blk_mq_free_tag_set(&ctrl->tag_set);
> 		nvme_rdma_dev_put(ctrl->device);
> 	}
> 	nvme_put_ctrl(&ctrl->ctrl);
> }

That sounds fine to me.

> or in a second step we should probably always call shutdown_ctrl
> but skip the actual shutdown if the ctrl state doesn't require it.

What do you mean "if the ctrl state doesn't require it"?
Up until today we managed to avoid checking the ctrl state
in queue_rq and I'd like to keep it that way. I'd be much
happier if we don't depend on queue_rq to fail early under
some assumptions, it might be a slippery slope...
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/5] nvme-rdma: Free the I/O tags when we delete the controller

[Sagi Grimberg]

>> If we wait until we free the controller (free_ctrl) we might
>> lose our rdma device without any notification while we still
>> have open resources (tags mrs and dma mappings).
>>
>> Instead, destroy the tags with their rdma resources once we
>> delete the device and not when freeing it.
>>
>> Note that we don't do that in nvme_rdma_shutdown_ctrl because
>> controller reset uses it as well and we want to give active I/O
>> a chance to complete successfully.
>>
>> Reported-by: Steve Wise <swise at opengridcomputing.com>
>> Signed-off-by: Sagi Grimberg <sagi at grimberg.me>
>
> This looks fine to me, but can we place share the code instead of
> duplicating it?  E.g.
>
> static void __nvme_rdma_remove_ctrl(struct nvme_rdma_ctrl *ctrl, bool shutdown)
> {
>  	nvme_remove_namespaces(&ctrl->ctrl);
> 	if (shutdown)
> 	  	nvme_rdma_shutdown_ctrl(ctrl);
>   	nvme_uninit_ctrl(&ctrl->ctrl);
> 	if (ctrl->ctrl.tagset) {
> 		blk_cleanup_queue(ctrl->ctrl.connect_q);
> 		blk_mq_free_tag_set(&ctrl->tag_set);
> 		nvme_rdma_dev_put(ctrl->device);
> 	}
> 	nvme_put_ctrl(&ctrl->ctrl);
> }

That sounds fine to me.

> or in a second step we should probably always call shutdown_ctrl
> but skip the actual shutdown if the ctrl state doesn't require it.

What do you mean "if the ctrl state doesn't require it"?
Up until today we managed to avoid checking the ctrl state
in queue_rq and I'd like to keep it that way. I'd be much
happier if we don't depend on queue_rq to fail early under
some assumptions, it might be a slippery slope...

Re: [PATCH 3/5] nvme-rdma: Make sure to shutdown the controller if we can

[Sagi Grimberg]

>> diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
>> index a70eb3cbf656..641ab7f91899 100644
>> --- a/drivers/nvme/host/rdma.c
>> +++ b/drivers/nvme/host/rdma.c
>> @@ -1644,7 +1644,7 @@ static void nvme_rdma_shutdown_ctrl(struct nvme_rdma_ctrl *ctrl)
>>  		nvme_rdma_free_io_queues(ctrl);
>>  	}
>>
>> -	if (ctrl->ctrl.state == NVME_CTRL_LIVE)
>> +	if (test_bit(NVME_RDMA_Q_CONNECTED, &ctrl->queues[0].flags))
>>  		nvme_shutdown_ctrl(&ctrl->ctrl);
>>
>>  	blk_mq_stop_hw_queues(ctrl->ctrl.admin_q);
>
> Maybe the right way to handle this is to unconditionally call
> nvme_shutdown_ctrl and make sure we return an early error
> on the register write?

As I wrote on patch 2/5 reply, I'd like to avoid depending on
queue_rq to fail early peeking at the ctrl state. This dependency
can grow in the future and I think we should at least try not to
go there...
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 3/5] nvme-rdma: Make sure to shutdown the controller if we can

[Sagi Grimberg]

>> diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
>> index a70eb3cbf656..641ab7f91899 100644
>> --- a/drivers/nvme/host/rdma.c
>> +++ b/drivers/nvme/host/rdma.c
>> @@ -1644,7 +1644,7 @@ static void nvme_rdma_shutdown_ctrl(struct nvme_rdma_ctrl *ctrl)
>>  		nvme_rdma_free_io_queues(ctrl);
>>  	}
>>
>> -	if (ctrl->ctrl.state == NVME_CTRL_LIVE)
>> +	if (test_bit(NVME_RDMA_Q_CONNECTED, &ctrl->queues[0].flags))
>>  		nvme_shutdown_ctrl(&ctrl->ctrl);
>>
>>  	blk_mq_stop_hw_queues(ctrl->ctrl.admin_q);
>
> Maybe the right way to handle this is to unconditionally call
> nvme_shutdown_ctrl and make sure we return an early error
> on the register write?

As I wrote on patch 2/5 reply, I'd like to avoid depending on
queue_rq to fail early peeking at the ctrl state. This dependency
can grow in the future and I think we should at least try not to
go there...

Re: [PATCH 4/5] nvme-rdma: Remove duplicate call to nvme_remove_namespaces

[Sagi Grimberg]

>> -	nvme_remove_namespaces(&ctrl->ctrl);
>> -	nvme_rdma_shutdown_ctrl(ctrl);
>>  	nvme_uninit_ctrl(&ctrl->ctrl);
>> +	nvme_rdma_shutdown_ctrl(ctrl);
>
> How safe is the reordering here?  The description should explain why
> it's fine.

The reordering is safe because we do want everything in uninit_ctrl
(flush aen,scan and ns-remove) to happen before we shutdown the rdma
stuff. I'll add to the change log.

Thanks!
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 4/5] nvme-rdma: Remove duplicate call to nvme_remove_namespaces

[Sagi Grimberg]

>> -	nvme_remove_namespaces(&ctrl->ctrl);
>> -	nvme_rdma_shutdown_ctrl(ctrl);
>>  	nvme_uninit_ctrl(&ctrl->ctrl);
>> +	nvme_rdma_shutdown_ctrl(ctrl);
>
> How safe is the reordering here?  The description should explain why
> it's fine.

The reordering is safe because we do want everything in uninit_ctrl
(flush aen,scan and ns-remove) to happen before we shutdown the rdma
stuff. I'll add to the change log.

Thanks!

Re: [PATCH 2/5] nvme-rdma: Free the I/O tags when we delete the controller

[Christoph Hellwig]

On Mon, Aug 01, 2016 at 02:15:59PM +0300, Sagi Grimberg wrote:
>> or in a second step we should probably always call shutdown_ctrl
>> but skip the actual shutdown if the ctrl state doesn't require it.
>
> What do you mean "if the ctrl state doesn't require it"?
> Up until today we managed to avoid checking the ctrl state
> in queue_rq and I'd like to keep it that way. I'd be much
> happier if we don't depend on queue_rq to fail early under
> some assumptions, it might be a slippery slope...

In this case I actually thought about checking the state in
the new __nvme_rdma_remove_ctrl.  If called from
nvme_rdma_del_ctrl_work the state will be NVME_CTRL_DELETING,
so we can check that to see if we want to do a controlled
shutdown using nvme_rdma_shutdown_ctrl instead of having
to overwrite ->remove_work.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/5] nvme-rdma: Free the I/O tags when we delete the controller

[Christoph Hellwig]

On Mon, Aug 01, 2016@02:15:59PM +0300, Sagi Grimberg wrote:
>> or in a second step we should probably always call shutdown_ctrl
>> but skip the actual shutdown if the ctrl state doesn't require it.
>
> What do you mean "if the ctrl state doesn't require it"?
> Up until today we managed to avoid checking the ctrl state
> in queue_rq and I'd like to keep it that way. I'd be much
> happier if we don't depend on queue_rq to fail early under
> some assumptions, it might be a slippery slope...

In this case I actually thought about checking the state in
the new __nvme_rdma_remove_ctrl.  If called from
nvme_rdma_del_ctrl_work the state will be NVME_CTRL_DELETING,
so we can check that to see if we want to do a controlled
shutdown using nvme_rdma_shutdown_ctrl instead of having
to overwrite ->remove_work.

Re: [PATCH 3/5] nvme-rdma: Make sure to shutdown the controller if we can

[Christoph Hellwig]

On Mon, Aug 01, 2016 at 02:17:37PM +0300, Sagi Grimberg wrote:
>>> -	if (ctrl->ctrl.state == NVME_CTRL_LIVE)
>>> +	if (test_bit(NVME_RDMA_Q_CONNECTED, &ctrl->queues[0].flags))
>>>  		nvme_shutdown_ctrl(&ctrl->ctrl);
>>>
>>>  	blk_mq_stop_hw_queues(ctrl->ctrl.admin_q);
>>
>> Maybe the right way to handle this is to unconditionally call
>> nvme_shutdown_ctrl and make sure we return an early error
>> on the register write?
>
> As I wrote on patch 2/5 reply, I'd like to avoid depending on
> queue_rq to fail early peeking at the ctrl state. This dependency
> can grow in the future and I think we should at least try not to
> go there...

I don't want ->queue_rq to peek at controller state.  What I had
in mind was copying the PCIe behavior of failing early in the
timeout handler if we are in the reset handler, which will
mean blk_execute_rq will return ASAP with an error.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 3/5] nvme-rdma: Make sure to shutdown the controller if we can

[Christoph Hellwig]

On Mon, Aug 01, 2016@02:17:37PM +0300, Sagi Grimberg wrote:
>>> -	if (ctrl->ctrl.state == NVME_CTRL_LIVE)
>>> +	if (test_bit(NVME_RDMA_Q_CONNECTED, &ctrl->queues[0].flags))
>>>  		nvme_shutdown_ctrl(&ctrl->ctrl);
>>>
>>>  	blk_mq_stop_hw_queues(ctrl->ctrl.admin_q);
>>
>> Maybe the right way to handle this is to unconditionally call
>> nvme_shutdown_ctrl and make sure we return an early error
>> on the register write?
>
> As I wrote on patch 2/5 reply, I'd like to avoid depending on
> queue_rq to fail early peeking at the ctrl state. This dependency
> can grow in the future and I think we should at least try not to
> go there...

I don't want ->queue_rq to peek at controller state.  What I had
in mind was copying the PCIe behavior of failing early in the
timeout handler if we are in the reset handler, which will
mean blk_execute_rq will return ASAP with an error.

Re: [PATCH 2/5] nvme-rdma: Free the I/O tags when we delete the controller

[Sagi Grimberg]

On 01/08/16 18:47, Christoph Hellwig wrote:
> On Mon, Aug 01, 2016 at 02:15:59PM +0300, Sagi Grimberg wrote:
>>> or in a second step we should probably always call shutdown_ctrl
>>> but skip the actual shutdown if the ctrl state doesn't require it.
>>
>> What do you mean "if the ctrl state doesn't require it"?
>> Up until today we managed to avoid checking the ctrl state
>> in queue_rq and I'd like to keep it that way. I'd be much
>> happier if we don't depend on queue_rq to fail early under
>> some assumptions, it might be a slippery slope...
>
> In this case I actually thought about checking the state in
> the new __nvme_rdma_remove_ctrl.  If called from
> nvme_rdma_del_ctrl_work the state will be NVME_CTRL_DELETING,
> so we can check that to see if we want to do a controlled
> shutdown using nvme_rdma_shutdown_ctrl instead of having
> to overwrite ->remove_work.
>

I don't know if that is sufficient. In case the host is
attempting reconnects periodically we can still delete
the controller, but the admin queue is not connected and
we shouldn't attempt to send a shutdown command...
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/5] nvme-rdma: Free the I/O tags when we delete the controller

[Sagi Grimberg]

On 01/08/16 18:47, Christoph Hellwig wrote:
> On Mon, Aug 01, 2016@02:15:59PM +0300, Sagi Grimberg wrote:
>>> or in a second step we should probably always call shutdown_ctrl
>>> but skip the actual shutdown if the ctrl state doesn't require it.
>>
>> What do you mean "if the ctrl state doesn't require it"?
>> Up until today we managed to avoid checking the ctrl state
>> in queue_rq and I'd like to keep it that way. I'd be much
>> happier if we don't depend on queue_rq to fail early under
>> some assumptions, it might be a slippery slope...
>
> In this case I actually thought about checking the state in
> the new __nvme_rdma_remove_ctrl.  If called from
> nvme_rdma_del_ctrl_work the state will be NVME_CTRL_DELETING,
> so we can check that to see if we want to do a controlled
> shutdown using nvme_rdma_shutdown_ctrl instead of having
> to overwrite ->remove_work.
>

I don't know if that is sufficient. In case the host is
attempting reconnects periodically we can still delete
the controller, but the admin queue is not connected and
we shouldn't attempt to send a shutdown command...

Re: [PATCH 3/5] nvme-rdma: Make sure to shutdown the controller if we can

[Sagi Grimberg]

>>>> -	if (ctrl->ctrl.state == NVME_CTRL_LIVE)
>>>> +	if (test_bit(NVME_RDMA_Q_CONNECTED, &ctrl->queues[0].flags))
>>>>  		nvme_shutdown_ctrl(&ctrl->ctrl);
>>>>
>>>>  	blk_mq_stop_hw_queues(ctrl->ctrl.admin_q);
>>>
>>> Maybe the right way to handle this is to unconditionally call
>>> nvme_shutdown_ctrl and make sure we return an early error
>>> on the register write?
>>
>> As I wrote on patch 2/5 reply, I'd like to avoid depending on
>> queue_rq to fail early peeking at the ctrl state. This dependency
>> can grow in the future and I think we should at least try not to
>> go there...
>
> I don't want ->queue_rq to peek at controller state.  What I had
> in mind was copying the PCIe behavior of failing early in the
> timeout handler if we are in the reset handler, which will
> mean blk_execute_rq will return ASAP with an error.

Do we want the unnecessary ADMIN_TIMEOUT for sending a shutdown
on a queue we know is not connected? I'm worried it will have scale
issues...
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 3/5] nvme-rdma: Make sure to shutdown the controller if we can

[Sagi Grimberg]

>>>> -	if (ctrl->ctrl.state == NVME_CTRL_LIVE)
>>>> +	if (test_bit(NVME_RDMA_Q_CONNECTED, &ctrl->queues[0].flags))
>>>>  		nvme_shutdown_ctrl(&ctrl->ctrl);
>>>>
>>>>  	blk_mq_stop_hw_queues(ctrl->ctrl.admin_q);
>>>
>>> Maybe the right way to handle this is to unconditionally call
>>> nvme_shutdown_ctrl and make sure we return an early error
>>> on the register write?
>>
>> As I wrote on patch 2/5 reply, I'd like to avoid depending on
>> queue_rq to fail early peeking at the ctrl state. This dependency
>> can grow in the future and I think we should at least try not to
>> go there...
>
> I don't want ->queue_rq to peek at controller state.  What I had
> in mind was copying the PCIe behavior of failing early in the
> timeout handler if we are in the reset handler, which will
> mean blk_execute_rq will return ASAP with an error.

Do we want the unnecessary ADMIN_TIMEOUT for sending a shutdown
on a queue we know is not connected? I'm worried it will have scale
issues...

Re: [PATCH 3/5] nvme-rdma: Make sure to shutdown the controller if we can

[Christoph Hellwig]

On Tue, Aug 02, 2016 at 09:19:56AM +0300, Sagi Grimberg wrote:
>> I don't want ->queue_rq to peek at controller state.  What I had
>> in mind was copying the PCIe behavior of failing early in the
>> timeout handler if we are in the reset handler, which will
>> mean blk_execute_rq will return ASAP with an error.
>
> Do we want the unnecessary ADMIN_TIMEOUT for sending a shutdown
> on a queue we know is not connected? I'm worried it will have scale
> issues...

Allright, let's go with your version.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 3/5] nvme-rdma: Make sure to shutdown the controller if we can

[Christoph Hellwig]

On Tue, Aug 02, 2016@09:19:56AM +0300, Sagi Grimberg wrote:
>> I don't want ->queue_rq to peek at controller state.  What I had
>> in mind was copying the PCIe behavior of failing early in the
>> timeout handler if we are in the reset handler, which will
>> mean blk_execute_rq will return ASAP with an error.
>
> Do we want the unnecessary ADMIN_TIMEOUT for sending a shutdown
> on a queue we know is not connected? I'm worried it will have scale
> issues...

Allright, let's go with your version.

Re: [PATCH 3/5] nvme-rdma: Make sure to shutdown the controller if we can

[Sagi Grimberg]

>>> I don't want ->queue_rq to peek at controller state.  What I had
>>> in mind was copying the PCIe behavior of failing early in the
>>> timeout handler if we are in the reset handler, which will
>>> mean blk_execute_rq will return ASAP with an error.
>>
>> Do we want the unnecessary ADMIN_TIMEOUT for sending a shutdown
>> on a queue we know is not connected? I'm worried it will have scale
>> issues...
>
> Allright, let's go with your version.

Is this an implicit reviewed-by?
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 3/5] nvme-rdma: Make sure to shutdown the controller if we can

[Sagi Grimberg]

>>> I don't want ->queue_rq to peek at controller state.  What I had
>>> in mind was copying the PCIe behavior of failing early in the
>>> timeout handler if we are in the reset handler, which will
>>> mean blk_execute_rq will return ASAP with an error.
>>
>> Do we want the unnecessary ADMIN_TIMEOUT for sending a shutdown
>> on a queue we know is not connected? I'm worried it will have scale
>> issues...
>
> Allright, let's go with your version.

Is this an implicit reviewed-by?