Skip to contentSkip to conversation

a few useful nvme fixes and cleanups

PATCH New Sagi Grimberg 2 participants 28 messages May 4, 2017 – May 10, 2017

Subsystems (3)
NVMe NVMe target NVMe FC transport
lore
Share & tools
lore

Sagi Grimberg

author maintainer
#0 lore raw

cover a few useful nvme fixes and cleanups

Some patches I've been sitting on for that past two weeks.

Sagi Grimberg (11):
nvme-loop: get rid of unused controller lock
nvme-rdma: get rid of unused ctrl lock
nvme-rdma: Make queue flags bit numbers and not shifts
nvme-rdma: Don't rearm the CQ when polling directly
nvme-rdma: make nvme_rdma_[create|destroy]_queue_ib symmetrical
nvme-rdma: rework rdma connection establishment error path
nvme-rdma: Get rid of CONNECTED state
nvme: Don't allow to reset a reconnecting controller
nvme: Move transports to use nvme-core workqueue
nvme: queue ns scanning and async request from nvme_wq
nvme: move nr_reconnects to nvme_ctrl

 drivers/nvme/host/core.c    |  22 +++++++---
 drivers/nvme/host/fabrics.c |   2 +-
 drivers/nvme/host/fabrics.h |   2 -
 drivers/nvme/host/fc.c      |  34 ++++-----------
 drivers/nvme/host/nvme.h    |   3 ++
 drivers/nvme/host/pci.c     |  18 ++------
 drivers/nvme/host/rdma.c    | 100 ++++++++++++++++----------------------------
 drivers/nvme/target/loop.c  |  11 ++---
 8 files changed, 72 insertions(+), 120 deletions(-)
Signature · 1 line
2.7.4
author maintainer
#1 lore raw

PATCH 1/11 nvme-loop: get rid of unused controller lock

Signed-off-by Sagi Grimberg <sagi at grimberg.me>
---
 drivers/nvme/target/loop.c | 3 ---
 1 file changed, 3 deletions(-)
drivers/nvme/target/loop.c +0 -3
index 304f1c87c160..84ae300d6498 100644
@@ -45,7 +45,6 @@ struct nvme_loop_iod {
 };
 
 struct nvme_loop_ctrl {
-	spinlock_t		lock;
 	struct nvme_loop_queue	*queues;
 	u32			queue_count;
 

@@ -635,8 +634,6 @@ static struct nvme_ctrl *nvme_loop_create_ctrl(struct device *dev,
 	if (ret)
 		goto out_put_ctrl;
 
-	spin_lock_init(&ctrl->lock);
-
 	ret = -ENOMEM;
 
 	ctrl->ctrl.sqsize = opts->queue_size - 1;
Signature · 1 line
2.7.4
maintainer
#2 lore raw

PATCH 1/11 nvme-loop: get rid of unused controller lock

Looks fine,

Reviewed-by Christoph Hellwig <hch at lst.de>
author maintainer
#3 lore raw

PATCH 2/11 nvme-rdma: get rid of unused ctrl lock

In reply to Sagi Grimberg
Signed-off-by Sagi Grimberg <sagi at grimberg.me>
---
 drivers/nvme/host/rdma.c | 4 ----
 1 file changed, 4 deletions(-)
drivers/nvme/host/rdma.c +0 -4
index 29cf88ac3f61..1cd7a21f63f4 100644
@@ -103,9 +103,6 @@ struct nvme_rdma_queue {
 };
 
 struct nvme_rdma_ctrl {
-	/* read and written in the hot path */
-	spinlock_t		lock;
-
 	/* read only in the hot path */
 	struct nvme_rdma_queue	*queues;
 	u32			queue_count;

@@ -1896,7 +1893,6 @@ static struct nvme_ctrl *nvme_rdma_create_ctrl(struct device *dev,
 	INIT_WORK(&ctrl->err_work, nvme_rdma_error_recovery_work);
 	INIT_WORK(&ctrl->delete_work, nvme_rdma_del_ctrl_work);
 	INIT_WORK(&ctrl->reset_work, nvme_rdma_reset_ctrl_work);
-	spin_lock_init(&ctrl->lock);
 
 	ctrl->queue_count = opts->nr_io_queues + 1; /* +1 for admin queue */
 	ctrl->ctrl.sqsize = opts->queue_size - 1;
Signature · 1 line
2.7.4
maintainer
#4 lore raw

PATCH 2/11 nvme-rdma: get rid of unused ctrl lock

Looks fine,

Reviewed-by Christoph Hellwig <hch at lst.de>
author maintainer
#5 lore raw

PATCH 3/11 nvme-rdma: Make queue flags bit numbers and not shifts

In reply to Sagi Grimberg

bitops accept bit numbers.

Reported-by Vijay Immanuel <vijayi at attalasystems.com>
Signed-off-by Sagi Grimberg <sagi at grimberg.me>
---
 drivers/nvme/host/rdma.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
drivers/nvme/host/rdma.c +4 -4
index 1cd7a21f63f4..5a29e1ea1934 100644
@@ -80,10 +80,10 @@ struct nvme_rdma_request {
 };
 
 enum nvme_rdma_queue_flags {
-	NVME_RDMA_Q_CONNECTED = (1 << 0),
-	NVME_RDMA_IB_QUEUE_ALLOCATED = (1 << 1),
-	NVME_RDMA_Q_DELETING = (1 << 2),
-	NVME_RDMA_Q_LIVE = (1 << 3),
+	NVME_RDMA_Q_CONNECTED		= 0,
+	NVME_RDMA_IB_QUEUE_ALLOCATED	= 1,
+	NVME_RDMA_Q_DELETING		= 2,
+	NVME_RDMA_Q_LIVE		= 3,
 };
 
 struct nvme_rdma_queue {
Signature · 1 line
2.7.4
maintainer
#6 lore raw

PATCH 3/11 nvme-rdma: Make queue flags bit numbers and not shifts

Quoted 4 lines · Sagi Grimberg

bitops accept bit numbers.

Reported-by Vijay Immanuel <vijayi at attalasystems.com>
Signed-off-by Sagi Grimberg <sagi at grimberg.me>

Looks fine,

Reviewed-by Christoph Hellwig <hch at lst.de>
author maintainer
#7 lore raw

PATCH 4/11 nvme-rdma: Don't rearm the CQ when polling directly

In reply to Sagi Grimberg

We don't need it as the core polling context will take are of rearming the completion queue.

Signed-off-by Sagi Grimberg <sagi at grimberg.me>
---
 drivers/nvme/host/rdma.c | 1 -
 1 file changed, 1 deletion(-)
drivers/nvme/host/rdma.c +0 -1
index 5a29e1ea1934..d76fcb22b2c4 100644
@@ -1496,7 +1496,6 @@ static int nvme_rdma_poll(struct blk_mq_hw_ctx *hctx, unsigned int tag)
 	struct ib_wc wc;
 	int found = 0;
 
-	ib_req_notify_cq(cq, IB_CQ_NEXT_COMP);
 	while (ib_poll_cq(cq, 1, &wc) > 0) {
 		struct ib_cqe *cqe = wc.wr_cqe;
Signature · 1 line
2.7.4
maintainer
#8 lore raw

PATCH 4/11 nvme-rdma: Don't rearm the CQ when polling directly

Quoted 4 lines · Sagi Grimberg

We don't need it as the core polling context will take are of rearming the completion queue.

Signed-off-by Sagi Grimberg <sagi at grimberg.me>

Looks fine (and I remember we had this discussion and am a bit surprised we didn't merge something like this yet).

Reviewed-by Christoph Hellwig <hch at lst.de>
author maintainer
#9 lore raw

PATCH 5/11 nvme-rdma: make nvme_rdma_[create|destroy]_queue_ib symmetrical

In reply to Sagi Grimberg

We put the reference on the device in the destroy routine so we should lookup and take the reference in the create routine.

Signed-off-by Sagi Grimberg <sagi at grimberg.me>
---
 drivers/nvme/host/rdma.c | 42 ++++++++++++++++++------------------------
 1 file changed, 18 insertions(+), 24 deletions(-)
drivers/nvme/host/rdma.c +18 -24
index d76fcb22b2c4..2dc381b57e4b 100644
@@ -480,17 +480,21 @@ static void nvme_rdma_destroy_queue_ib(struct nvme_rdma_queue *queue)
 	nvme_rdma_dev_put(dev);
 }
 
-static int nvme_rdma_create_queue_ib(struct nvme_rdma_queue *queue,
-		struct nvme_rdma_device *dev)
+static int nvme_rdma_create_queue_ib(struct nvme_rdma_queue *queue)
 {
-	struct ib_device *ibdev = dev->dev;
+	struct ib_device *ibdev;
 	const int send_wr_factor = 3;			/* MR, SEND, INV */
 	const int cq_factor = send_wr_factor + 1;	/* + RECV */
 	int comp_vector, idx = nvme_rdma_queue_idx(queue);
-
 	int ret;
 
-	queue->device = dev;
+	queue->device = nvme_rdma_find_get_device(queue->cm_id);
+	if (!queue->device) {
+		dev_err(queue->cm_id->device->dev.parent,
+			"no client data found!\n");
+		return -ECONNREFUSED;
+	}
+	ibdev = queue->device->dev;
 
 	/*
 	 * The admin queue is barely used once the controller is live, so don't

@@ -503,12 +507,12 @@ static int nvme_rdma_create_queue_ib(struct nvme_rdma_queue *queue,
 
 
 	/* +1 for ib_stop_cq */
-	queue->ib_cq = ib_alloc_cq(dev->dev, queue,
-				cq_factor * queue->queue_size + 1, comp_vector,
-				IB_POLL_SOFTIRQ);
+	queue->ib_cq = ib_alloc_cq(ibdev, queue,
+				cq_factor * queue->queue_size + 1,
+				comp_vector, IB_POLL_SOFTIRQ);
 	if (IS_ERR(queue->ib_cq)) {
 		ret = PTR_ERR(queue->ib_cq);
-		goto out;
+		goto out_put_dev;
 	}
 
 	ret = nvme_rdma_create_qp(queue, send_wr_factor);

@@ -529,7 +533,8 @@ static int nvme_rdma_create_queue_ib(struct nvme_rdma_queue *queue,
 	ib_destroy_qp(queue->qp);
 out_destroy_ib_cq:
 	ib_free_cq(queue->ib_cq);
-out:
+out_put_dev:
+	nvme_rdma_dev_put(queue->device);
 	return ret;
 }
 

@@ -1263,21 +1268,11 @@ static int nvme_rdma_conn_rejected(struct nvme_rdma_queue *queue,
 
 static int nvme_rdma_addr_resolved(struct nvme_rdma_queue *queue)
 {
-	struct nvme_rdma_device *dev;
 	int ret;
 
-	dev = nvme_rdma_find_get_device(queue->cm_id);
-	if (!dev) {
-		dev_err(queue->cm_id->device->dev.parent,
-			"no client data found!\n");
-		return -ECONNREFUSED;
-	}
-
-	ret = nvme_rdma_create_queue_ib(queue, dev);
-	if (ret) {
-		nvme_rdma_dev_put(dev);
-		goto out;
-	}
+	ret = nvme_rdma_create_queue_ib(queue);
+	if (ret)
+		return ret;
 
 	ret = rdma_resolve_route(queue->cm_id, NVME_RDMA_CONNECT_TIMEOUT_MS);
 	if (ret) {

@@ -1291,7 +1286,6 @@ static int nvme_rdma_addr_resolved(struct nvme_rdma_queue *queue)
 
 out_destroy_queue:
 	nvme_rdma_destroy_queue_ib(queue);
-out:
 	return ret;
 }
Signature · 1 line
2.7.4
maintainer
#10 lore raw

PATCH 5/11 nvme-rdma: make nvme_rdma_[create|destroy]_queue_ib symmetrical

Looks fine,

Reviewed-by Christoph Hellwig <hch at lst.de>
author maintainer
#11 lore raw

PATCH 6/11 nvme-rdma: rework rdma connection establishment error path

In reply to Sagi Grimberg

Instead of introducing a flag for if the queue is allocated, simply free the rdma resources when we get the error.

We allocate the queue rdma resources when we have an address resolution, their we allocate (or take a reference on) our device so we should free it when we have error after the address resolution namely:
1. route resolution error
2. connect reject
3. connect error
4. peer unreachable error

Signed-off-by Sagi Grimberg <sagi at grimberg.me>
---
 drivers/nvme/host/rdma.c | 14 +++++---------
 1 file changed, 5 insertions(+), 9 deletions(-)
drivers/nvme/host/rdma.c +5 -9
index 2dc381b57e4b..7df52896f321 100644
@@ -81,9 +81,8 @@ struct nvme_rdma_request {
 
 enum nvme_rdma_queue_flags {
 	NVME_RDMA_Q_CONNECTED		= 0,
-	NVME_RDMA_IB_QUEUE_ALLOCATED	= 1,
-	NVME_RDMA_Q_DELETING		= 2,
-	NVME_RDMA_Q_LIVE		= 3,
+	NVME_RDMA_Q_DELETING		= 1,
+	NVME_RDMA_Q_LIVE		= 2,
 };
 
 struct nvme_rdma_queue {

@@ -466,9 +465,6 @@ static void nvme_rdma_destroy_queue_ib(struct nvme_rdma_queue *queue)
 	struct nvme_rdma_device *dev;
 	struct ib_device *ibdev;
 
-	if (!test_and_clear_bit(NVME_RDMA_IB_QUEUE_ALLOCATED, &queue->flags))
-		return;
-
 	dev = queue->device;
 	ibdev = dev->dev;
 	rdma_destroy_qp(queue->cm_id);

@@ -525,7 +521,6 @@ static int nvme_rdma_create_queue_ib(struct nvme_rdma_queue *queue)
 		ret = -ENOMEM;
 		goto out_destroy_qp;
 	}
-	set_bit(NVME_RDMA_IB_QUEUE_ALLOCATED, &queue->flags);
 
 	return 0;
 

@@ -590,7 +585,6 @@ static int nvme_rdma_init_queue(struct nvme_rdma_ctrl *ctrl,
 	return 0;
 
 out_destroy_cm_id:
-	nvme_rdma_destroy_queue_ib(queue);
 	rdma_destroy_id(queue->cm_id);
 	return ret;
 }

@@ -1362,12 +1356,14 @@ static int nvme_rdma_cm_handler(struct rdma_cm_id *cm_id,
 		complete(&queue->cm_done);
 		return 0;
 	case RDMA_CM_EVENT_REJECTED:
+		nvme_rdma_destroy_queue_ib(queue);
 		cm_error = nvme_rdma_conn_rejected(queue, ev);
 		break;
-	case RDMA_CM_EVENT_ADDR_ERROR:
 	case RDMA_CM_EVENT_ROUTE_ERROR:
 	case RDMA_CM_EVENT_CONNECT_ERROR:
 	case RDMA_CM_EVENT_UNREACHABLE:
+		nvme_rdma_destroy_queue_ib(queue);
+	case RDMA_CM_EVENT_ADDR_ERROR:
 		dev_dbg(queue->ctrl->ctrl.device,
 			"CM error event %d\n", ev->event);
 		cm_error = -ECONNRESET;
Signature · 1 line
2.7.4
maintainer
#12 lore raw

PATCH 6/11 nvme-rdma: rework rdma connection establishment error path

Quoted 62 lines · Sagi Grimberg

Instead of introducing a flag for if the queue is allocated, simply free the rdma resources when we get the error.

We allocate the queue rdma resources when we have an address resolution, their we allocate (or take a reference on) our device so we should free it when we have error after the address resolution namely:
1. route resolution error
2. connect reject
3. connect error
4. peer unreachable error

Signed-off-by Sagi Grimberg <sagi at grimberg.me>
---
 drivers/nvme/host/rdma.c | 14 +++++---------
 1 file changed, 5 insertions(+), 9 deletions(-)
drivers/nvme/host/rdma.c +2 -8
index 2dc381b57e4b..7df52896f321 100644
@@ -81,9 +81,8 @@ struct nvme_rdma_request {
 
 enum nvme_rdma_queue_flags {
 	NVME_RDMA_Q_CONNECTED		= 0,
-	NVME_RDMA_IB_QUEUE_ALLOCATED	= 1,
-	NVME_RDMA_Q_DELETING		= 2,
-	NVME_RDMA_Q_LIVE		= 3,
+	NVME_RDMA_Q_DELETING		= 1,
+	NVME_RDMA_Q_LIVE		= 2,
 };
 
 struct nvme_rdma_queue {

@@ -466,9 +465,6 @@ static void nvme_rdma_destroy_queue_ib(struct nvme_rdma_queue *queue)
 	struct nvme_rdma_device *dev;
 	struct ib_device *ibdev;
 
-	if (!test_and_clear_bit(NVME_RDMA_IB_QUEUE_ALLOCATED, &queue->flags))
-		return;
-
 	dev = queue->device;
 	ibdev = dev->dev;
 	rdma_destroy_qp(queue->cm_id);

@@ -525,7 +521,6 @@ static int nvme_rdma_create_queue_ib(struct nvme_rdma_queue *queue)
 		ret = -ENOMEM;
 		goto out_destroy_qp;
 	}
-	set_bit(NVME_RDMA_IB_QUEUE_ALLOCATED, &queue->flags);
 
 	return 0;
 

@@ -590,7 +585,6 @@ static int nvme_rdma_init_queue(struct nvme_rdma_ctrl *ctrl,
 	return 0;
 
 out_destroy_cm_id:
-	nvme_rdma_destroy_queue_ib(queue);
 	rdma_destroy_id(queue->cm_id);
 	return ret;
 }

@@ -1362,12 +1356,14 @@ static int nvme_rdma_cm_handler(struct rdma_cm_id *cm_id,
 		complete(&queue->cm_done);
 		return 0;

drivers/nvme/host/rdma.c static int nvme_rdma_cm_handler(struct rdma_cm_id *cm_id, · line 1366

Quoted 10 lines
drivers/nvme/host/rdma.c +3 -1
 	case RDMA_CM_EVENT_REJECTED:
+		nvme_rdma_destroy_queue_ib(queue);
 		cm_error = nvme_rdma_conn_rejected(queue, ev);
 		break;
-	case RDMA_CM_EVENT_ADDR_ERROR:
 	case RDMA_CM_EVENT_ROUTE_ERROR:
 	case RDMA_CM_EVENT_CONNECT_ERROR:
 	case RDMA_CM_EVENT_UNREACHABLE:
+		nvme_rdma_destroy_queue_ib(queue);
+	case RDMA_CM_EVENT_ADDR_ERROR:

Please add a /*FALLTHRU*/ annotation here.

Otherwise looks fine:

Reviewed-by Christoph Hellwig <hch at lst.de>
author maintainer
#13 lore raw

PATCH 7/11 nvme-rdma: Get rid of CONNECTED state

In reply to Sagi Grimberg

We only care about if the queue is LIVE for request submission, so no need for CONNECTED.

Signed-off-by Sagi Grimberg <sagi at grimberg.me>
---
 drivers/nvme/host/rdma.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)
drivers/nvme/host/rdma.c +3 -7
index 7df52896f321..9ce56520bb1a 100644
@@ -80,9 +80,8 @@ struct nvme_rdma_request {
 };
 
 enum nvme_rdma_queue_flags {
-	NVME_RDMA_Q_CONNECTED		= 0,
+	NVME_RDMA_Q_LIVE		= 0,
 	NVME_RDMA_Q_DELETING		= 1,
-	NVME_RDMA_Q_LIVE		= 2,
 };
 
 struct nvme_rdma_queue {

@@ -580,7 +579,6 @@ static int nvme_rdma_init_queue(struct nvme_rdma_ctrl *ctrl,
 	}
 
 	clear_bit(NVME_RDMA_Q_DELETING, &queue->flags);
-	set_bit(NVME_RDMA_Q_CONNECTED, &queue->flags);
 
 	return 0;
 

@@ -803,10 +801,8 @@ static void nvme_rdma_error_recovery_work(struct work_struct *work)
 
 	nvme_stop_keep_alive(&ctrl->ctrl);
 
-	for (i = 0; i < ctrl->queue_count; i++) {
-		clear_bit(NVME_RDMA_Q_CONNECTED, &ctrl->queues[i].flags);
+	for (i = 0; i < ctrl->queue_count; i++)
 		clear_bit(NVME_RDMA_Q_LIVE, &ctrl->queues[i].flags);
-	}
 
 	if (ctrl->queue_count > 1)
 		nvme_stop_queues(&ctrl->ctrl);

@@ -1634,7 +1630,7 @@ static void nvme_rdma_shutdown_ctrl(struct nvme_rdma_ctrl *ctrl)
 		nvme_rdma_free_io_queues(ctrl);
 	}
 
-	if (test_bit(NVME_RDMA_Q_CONNECTED, &ctrl->queues[0].flags))
+	if (test_bit(NVME_RDMA_Q_LIVE, &ctrl->queues[0].flags))
 		nvme_shutdown_ctrl(&ctrl->ctrl);
 
 	blk_mq_stop_hw_queues(ctrl->ctrl.admin_q);
Signature · 1 line
2.7.4
maintainer
#14 lore raw

PATCH 7/11 nvme-rdma: Get rid of CONNECTED state

Looks fine,

Reviewed-by Christoph Hellwig <hch at lst.de>
author maintainer
#15 lore raw

PATCH 8/11 nvme: Don't allow to reset a reconnecting controller

In reply to Sagi Grimberg

The reset operation is guaranteed to fail for all scenarios but the esoteric case where in the last reconnect attempt concurrent with the reset we happen to successfully reconnect.

We just deny initiating a reset if we are reconnecting.

Signed-off-by Sagi Grimberg <sagi at grimberg.me>
---
 drivers/nvme/host/core.c | 1 -
 1 file changed, 1 deletion(-)
drivers/nvme/host/core.c +0 -1
index d5e0906262ea..cfc8f8da89c8 100644
@@ -165,7 +165,6 @@ bool nvme_change_ctrl_state(struct nvme_ctrl *ctrl,
 		switch (old_state) {
 		case NVME_CTRL_NEW:
 		case NVME_CTRL_LIVE:
-		case NVME_CTRL_RECONNECTING:
 			changed = true;
 			/* FALLTHRU */
 		default:
Signature · 1 line
2.7.4
maintainer
#16 lore raw

PATCH 8/11 nvme: Don't allow to reset a reconnecting controller

Looks fine,

Reviewed-by Christoph Hellwig <hch at lst.de>
author maintainer
#17 lore raw

PATCH 9/11 nvme: Move transports to use nvme-core workqueue

In reply to Sagi Grimberg

Instead of each transport using it's own workqueue, export a single nvme-core workqueue and use that instead.

In the future, this will help us moving towards some unification if controller setup/teardown flows.

Signed-off-by Sagi Grimberg <sagi at grimberg.me>
---
 drivers/nvme/host/core.c   | 15 +++++++++++++--
 drivers/nvme/host/fc.c     | 34 +++++++++-------------------------
 drivers/nvme/host/nvme.h   |  2 ++
 drivers/nvme/host/pci.c    | 18 +++---------------
 drivers/nvme/host/rdma.c   | 25 ++++++++-----------------
 drivers/nvme/target/loop.c |  8 ++++----
 6 files changed, 39 insertions(+), 63 deletions(-)
drivers/nvme/host/core.c +13 -2
index cfc8f8da89c8..26698e2c7380 100644
@@ -65,6 +65,9 @@ static bool force_apst;
 module_param(force_apst, bool, 0644);
 MODULE_PARM_DESC(force_apst, "allow APST for newly enumerated devices even if quirked off");
 
+struct workqueue_struct *nvme_wq;
+EXPORT_SYMBOL_GPL(nvme_wq);
+
 static LIST_HEAD(nvme_ctrl_list);
 static DEFINE_SPINLOCK(dev_list_lock);
 

@@ -2517,10 +2520,15 @@ int __init nvme_core_init(void)
 {
 	int result;
 
+	nvme_wq = alloc_workqueue("nvme-wq",
+			WQ_UNBOUND | WQ_MEM_RECLAIM | WQ_SYSFS, 0);
+	if (!nvme_wq)
+		return -ENOMEM;
+
 	result = __register_chrdev(nvme_char_major, 0, NVME_MINORS, "nvme",
 							&nvme_dev_fops);
 	if (result < 0)
-		return result;
+		goto destroy_wq;
 	else if (result > 0)
 		nvme_char_major = result;
 

@@ -2532,8 +2540,10 @@ int __init nvme_core_init(void)
 
 	return 0;
 
- unregister_chrdev:
+unregister_chrdev:
 	__unregister_chrdev(nvme_char_major, 0, NVME_MINORS, "nvme");
+destroy_wq:
+	destroy_workqueue(nvme_wq);
 	return result;
 }
 

@@ -2541,6 +2551,7 @@ void nvme_core_exit(void)
 {
 	class_destroy(nvme_class);
 	__unregister_chrdev(nvme_char_major, 0, NVME_MINORS, "nvme");
+	destroy_workqueue(nvme_wq);
 }
 
 MODULE_LICENSE("GPL");
drivers/nvme/host/fc.c +9 -25
index 4976db56e351..ca0cccd51292 100644
@@ -218,7 +218,6 @@ static LIST_HEAD(nvme_fc_lport_list);
 static DEFINE_IDA(nvme_fc_local_port_cnt);
 static DEFINE_IDA(nvme_fc_ctrl_cnt);
 
-static struct workqueue_struct *nvme_fc_wq;
 
 
 

@@ -1763,7 +1762,7 @@ nvme_fc_error_recovery(struct nvme_fc_ctrl *ctrl, char *errmsg)
 		return;
 	}
 
-	if (!queue_work(nvme_fc_wq, &ctrl->reset_work))
+	if (!queue_work(nvme_wq, &ctrl->reset_work))
 		dev_err(ctrl->ctrl.device,
 			"NVME-FC{%d}: error_recovery: Failed to schedule "
 			"reset work\n", ctrl->cnum);

@@ -2550,7 +2549,7 @@ __nvme_fc_del_ctrl(struct nvme_fc_ctrl *ctrl)
 	if (!nvme_change_ctrl_state(&ctrl->ctrl, NVME_CTRL_DELETING))
 		return -EBUSY;
 
-	if (!queue_work(nvme_fc_wq, &ctrl->delete_work))
+	if (!queue_work(nvme_wq, &ctrl->delete_work))
 		return -EBUSY;
 
 	return 0;

@@ -2571,7 +2570,7 @@ nvme_fc_del_nvme_ctrl(struct nvme_ctrl *nctrl)
 	ret = __nvme_fc_del_ctrl(ctrl);
 
 	if (!ret)
-		flush_workqueue(nvme_fc_wq);
+		flush_workqueue(nvme_wq);
 
 	nvme_put_ctrl(&ctrl->ctrl);
 

@@ -2607,14 +2606,14 @@ nvme_fc_reset_ctrl_work(struct work_struct *work)
 				return;
 			}
 
-			WARN_ON(!queue_work(nvme_fc_wq, &ctrl->delete_work));
+			WARN_ON(!queue_work(nvme_wq, &ctrl->delete_work));
 			return;
 		}
 
 		dev_warn(ctrl->ctrl.device,
 			"NVME-FC{%d}: Reconnect attempt in %d seconds.\n",
 			ctrl->cnum, ctrl->reconnect_delay);
-		queue_delayed_work(nvme_fc_wq, &ctrl->connect_work,
+		queue_delayed_work(nvme_wq, &ctrl->connect_work,
 				ctrl->reconnect_delay * HZ);
 	} else
 		dev_info(ctrl->ctrl.device,

@@ -2636,7 +2635,7 @@ nvme_fc_reset_nvme_ctrl(struct nvme_ctrl *nctrl)
 	if (!nvme_change_ctrl_state(&ctrl->ctrl, NVME_CTRL_RESETTING))
 		return -EBUSY;
 
-	if (!queue_work(nvme_fc_wq, &ctrl->reset_work))
+	if (!queue_work(nvme_wq, &ctrl->reset_work))
 		return -EBUSY;
 
 	flush_work(&ctrl->reset_work);

@@ -2687,14 +2686,14 @@ nvme_fc_connect_ctrl_work(struct work_struct *work)
 				return;
 			}
 
-			WARN_ON(!queue_work(nvme_fc_wq, &ctrl->delete_work));
+			WARN_ON(!queue_work(nvme_wq, &ctrl->delete_work));
 			return;
 		}
 
 		dev_warn(ctrl->ctrl.device,
 			"NVME-FC{%d}: Reconnect attempt in %d seconds.\n",
 			ctrl->cnum, ctrl->reconnect_delay);
-		queue_delayed_work(nvme_fc_wq, &ctrl->connect_work,
+		queue_delayed_work(nvme_wq, &ctrl->connect_work,
 				ctrl->reconnect_delay * HZ);
 	} else
 		dev_info(ctrl->ctrl.device,

@@ -2969,20 +2968,7 @@ static struct nvmf_transport_ops nvme_fc_transport = {
 
 static int __init nvme_fc_init_module(void)
 {
-	int ret;
-
-	nvme_fc_wq = create_workqueue("nvme_fc_wq");
-	if (!nvme_fc_wq)
-		return -ENOMEM;
-
-	ret = nvmf_register_transport(&nvme_fc_transport);
-	if (ret)
-		goto err;
-
-	return 0;
-err:
-	destroy_workqueue(nvme_fc_wq);
-	return ret;
+	return nvmf_register_transport(&nvme_fc_transport);
 }
 
 static void __exit nvme_fc_exit_module(void)

@@ -2993,8 +2979,6 @@ static void __exit nvme_fc_exit_module(void)
 
 	nvmf_unregister_transport(&nvme_fc_transport);
 
-	destroy_workqueue(nvme_fc_wq);
-
 	ida_destroy(&nvme_fc_local_port_cnt);
 	ida_destroy(&nvme_fc_ctrl_cnt);
 }
drivers/nvme/host/nvme.h +2 -0
index 29c708ca9621..edf3ee7da553 100644
@@ -33,6 +33,8 @@ extern unsigned char shutdown_timeout;
 #define NVME_DEFAULT_KATO	5
 #define NVME_KATO_GRACE		10
 
+extern struct workqueue_struct *nvme_wq;
+
 enum {
 	NVME_NS_LBA		= 0,
 	NVME_NS_LIGHTNVM	= 1,
drivers/nvme/host/pci.c +3 -15
index c8541c3dcd19..923343982b41 100644
@@ -66,8 +66,6 @@ static bool use_cmb_sqes = true;
 module_param(use_cmb_sqes, bool, 0644);
 MODULE_PARM_DESC(use_cmb_sqes, "use controller's memory buffer for I/O SQes");
 
-static struct workqueue_struct *nvme_workq;
-
 struct nvme_dev;
 struct nvme_queue;
 

@@ -2010,7 +2008,7 @@ static int nvme_reset(struct nvme_dev *dev)
 		return -ENODEV;
 	if (work_busy(&dev->reset_work))
 		return -ENODEV;
-	if (!queue_work(nvme_workq, &dev->reset_work))
+	if (!queue_work(nvme_wq, &dev->reset_work))
 		return -EBUSY;
 	return 0;
 }

@@ -2136,7 +2134,7 @@ static int nvme_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 
 	dev_info(dev->ctrl.device, "pci function %s\n", dev_name(&pdev->dev));
 
-	queue_work(nvme_workq, &dev->reset_work);
+	queue_work(nvme_wq, &dev->reset_work);
 	return 0;
 
  release_pools:

@@ -2321,22 +2319,12 @@ static struct pci_driver nvme_driver = {
 
 static int __init nvme_init(void)
 {
-	int result;
-
-	nvme_workq = alloc_workqueue("nvme", WQ_UNBOUND | WQ_MEM_RECLAIM, 0);
-	if (!nvme_workq)
-		return -ENOMEM;
-
-	result = pci_register_driver(&nvme_driver);
-	if (result)
-		destroy_workqueue(nvme_workq);
-	return result;
+	return pci_register_driver(&nvme_driver);
 }
 
 static void __exit nvme_exit(void)
 {
 	pci_unregister_driver(&nvme_driver);
-	destroy_workqueue(nvme_workq);
 	_nvme_check_size();
 }
drivers/nvme/host/rdma.c +8 -17
index 9ce56520bb1a..ae16d44f2ada 100644
@@ -140,8 +140,6 @@ static DEFINE_MUTEX(device_list_mutex);
 static LIST_HEAD(nvme_rdma_ctrl_list);
 static DEFINE_MUTEX(nvme_rdma_ctrl_mutex);
 
-static struct workqueue_struct *nvme_rdma_wq;
-
 /*
  * Disabling this option makes small I/O goes faster, but is fundamentally
  * unsafe.  With it turned off we will have to register a global rkey that

@@ -712,11 +710,11 @@ static void nvme_rdma_reconnect_or_remove(struct nvme_rdma_ctrl *ctrl)
 	if (nvmf_should_reconnect(&ctrl->ctrl)) {
 		dev_info(ctrl->ctrl.device, "Reconnecting in %d seconds...\n",
 			ctrl->ctrl.opts->reconnect_delay);
-		queue_delayed_work(nvme_rdma_wq, &ctrl->reconnect_work,
+		queue_delayed_work(nvme_wq, &ctrl->reconnect_work,
 				ctrl->ctrl.opts->reconnect_delay * HZ);
 	} else {
 		dev_info(ctrl->ctrl.device, "Removing controller...\n");
-		queue_work(nvme_rdma_wq, &ctrl->delete_work);
+		queue_work(nvme_wq, &ctrl->delete_work);
 	}
 }
 

@@ -823,7 +821,7 @@ static void nvme_rdma_error_recovery(struct nvme_rdma_ctrl *ctrl)
 	if (!nvme_change_ctrl_state(&ctrl->ctrl, NVME_CTRL_RECONNECTING))
 		return;
 
-	queue_work(nvme_rdma_wq, &ctrl->err_work);
+	queue_work(nvme_wq, &ctrl->err_work);
 }
 
 static void nvme_rdma_wr_error(struct ib_cq *cq, struct ib_wc *wc,

@@ -1667,7 +1665,7 @@ static int __nvme_rdma_del_ctrl(struct nvme_rdma_ctrl *ctrl)
 	if (!nvme_change_ctrl_state(&ctrl->ctrl, NVME_CTRL_DELETING))
 		return -EBUSY;
 
-	if (!queue_work(nvme_rdma_wq, &ctrl->delete_work))
+	if (!queue_work(nvme_wq, &ctrl->delete_work))
 		return -EBUSY;
 
 	return 0;

@@ -1743,7 +1741,7 @@ static void nvme_rdma_reset_ctrl_work(struct work_struct *work)
 del_dead_ctrl:
 	/* Deleting this dead controller... */
 	dev_warn(ctrl->ctrl.device, "Removing after reset failure\n");
-	WARN_ON(!queue_work(nvme_rdma_wq, &ctrl->delete_work));
+	WARN_ON(!queue_work(nvme_wq, &ctrl->delete_work));
 }
 
 static int nvme_rdma_reset_ctrl(struct nvme_ctrl *nctrl)

@@ -1753,7 +1751,7 @@ static int nvme_rdma_reset_ctrl(struct nvme_ctrl *nctrl)
 	if (!nvme_change_ctrl_state(&ctrl->ctrl, NVME_CTRL_RESETTING))
 		return -EBUSY;
 
-	if (!queue_work(nvme_rdma_wq, &ctrl->reset_work))
+	if (!queue_work(nvme_wq, &ctrl->reset_work))
 		return -EBUSY;
 
 	flush_work(&ctrl->reset_work);

@@ -1990,7 +1988,7 @@ static void nvme_rdma_remove_one(struct ib_device *ib_device, void *client_data)
 	}
 	mutex_unlock(&nvme_rdma_ctrl_mutex);
 
-	flush_workqueue(nvme_rdma_wq);
+	flush_workqueue(nvme_wq);
 }
 
 static struct ib_client nvme_rdma_ib_client = {

@@ -2003,13 +2001,9 @@ static int __init nvme_rdma_init_module(void)
 {
 	int ret;
 
-	nvme_rdma_wq = create_workqueue("nvme_rdma_wq");
-	if (!nvme_rdma_wq)
-		return -ENOMEM;
-
 	ret = ib_register_client(&nvme_rdma_ib_client);
 	if (ret)
-		goto err_destroy_wq;
+		return ret;
 
 	ret = nvmf_register_transport(&nvme_rdma_transport);
 	if (ret)

@@ -2019,8 +2013,6 @@ static int __init nvme_rdma_init_module(void)
 
 err_unreg_client:
 	ib_unregister_client(&nvme_rdma_ib_client);
-err_destroy_wq:
-	destroy_workqueue(nvme_rdma_wq);
 	return ret;
 }
 

@@ -2028,7 +2020,6 @@ static void __exit nvme_rdma_cleanup_module(void)
 {
 	nvmf_unregister_transport(&nvme_rdma_transport);
 	ib_unregister_client(&nvme_rdma_ib_client);
-	destroy_workqueue(nvme_rdma_wq);
 }
 
 module_init(nvme_rdma_init_module);
drivers/nvme/target/loop.c +4 -4
index 84ae300d6498..6b90ad1216dc 100644
@@ -150,7 +150,7 @@ nvme_loop_timeout(struct request *rq, bool reserved)
 	struct nvme_loop_iod *iod = blk_mq_rq_to_pdu(rq);
 
 	/* queue error recovery */
-	schedule_work(&iod->queue->ctrl->reset_work);
+	queue_work(nvme_wq, &iod->queue->ctrl->reset_work);
 
 	/* fail with DNR on admin cmd timeout */
 	nvme_req(rq)->status = NVME_SC_ABORT_REQ | NVME_SC_DNR;

@@ -465,7 +465,7 @@ static int __nvme_loop_del_ctrl(struct nvme_loop_ctrl *ctrl)
 	if (!nvme_change_ctrl_state(&ctrl->ctrl, NVME_CTRL_DELETING))
 		return -EBUSY;
 
-	if (!schedule_work(&ctrl->delete_work))
+	if (!queue_work(nvme_wq, &ctrl->delete_work))
 		return -EBUSY;
 
 	return 0;

@@ -545,7 +545,7 @@ static int nvme_loop_reset_ctrl(struct nvme_ctrl *nctrl)
 	if (!nvme_change_ctrl_state(&ctrl->ctrl, NVME_CTRL_RESETTING))
 		return -EBUSY;
 
-	if (!schedule_work(&ctrl->reset_work))
+	if (!queue_work(nvme_wq, &ctrl->reset_work))
 		return -EBUSY;
 
 	flush_work(&ctrl->reset_work);

@@ -762,7 +762,7 @@ static void __exit nvme_loop_cleanup_module(void)
 		__nvme_loop_del_ctrl(ctrl);
 	mutex_unlock(&nvme_loop_ctrl_mutex);
 
-	flush_scheduled_work();
+	flush_workqueue(nvme_wq);
 }
 
 module_init(nvme_loop_init_module);
Signature · 1 line
2.7.4
maintainer
#18 lore raw

PATCH 9/11 nvme: Move transports to use nvme-core workqueue

Quoted 5 lines · Sagi Grimberg

Instead of each transport using it's own workqueue, export a single nvme-core workqueue and use that instead.

In the future, this will help us moving towards some unification if controller setup/teardown flows.

Looks fine,

Reviewed-by Christoph Hellwig <hch at lst.de>
author maintainer
#19 lore raw

PATCH 10/11 nvme: queue ns scanning and async request from nvme_wq

In reply to Sagi Grimberg

To suppress the warning triggered by nvme_uninit_ctrl:
kernel: [ 50.350439] nvme nvme0: rescanning
kernel: [ 50.363351] ------------[ cut here]------------
kernel: [ 50.363396] WARNING: CPU: 1 PID: 37 at kernel/workqueue.c:2423 check_flush_dependency+0x11f/0x130
kernel: [ 50.363409] workqueue: WQ_MEM_RECLAIM
nvme-wq:nvme_del_ctrl_work [nvme_core] is flushing !WQ_MEM_RECLAIM events:nvme_scan_work [nvme_core]

This was triggered with nvme-loop, but can happen with rdma/pci as well afaict.

Signed-off-by Sagi Grimberg <sagi at grimberg.me>

---
Note that given that nvme-pci dedicated workq was WQ_MEM_RECLAIM as well so if we have an available WQ_MEM_RECLAIM workqueue in nvme-core we would have fixed it sooner.

 drivers/nvme/host/core.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
drivers/nvme/host/core.c +3 -3
index 26698e2c7380..6f720456b73f 100644
@@ -2219,7 +2219,7 @@ void nvme_queue_scan(struct nvme_ctrl *ctrl)
 	 * removal.
 	 */
 	if (ctrl->state == NVME_CTRL_LIVE)
-		schedule_work(&ctrl->scan_work);
+		queue_work(nvme_wq, &ctrl->scan_work);
 }
 EXPORT_SYMBOL_GPL(nvme_queue_scan);
 

@@ -2274,7 +2274,7 @@ void nvme_complete_async_event(struct nvme_ctrl *ctrl, __le16 status,
 		/*FALLTHRU*/
 	case NVME_SC_ABORT_REQ:
 		++ctrl->event_limit;
-		schedule_work(&ctrl->async_event_work);
+		queue_work(nvme_wq, &ctrl->async_event_work);
 		break;
 	default:
 		break;

@@ -2297,7 +2297,7 @@ EXPORT_SYMBOL_GPL(nvme_complete_async_event);
 void nvme_queue_async_events(struct nvme_ctrl *ctrl)
 {
 	ctrl->event_limit = NVME_NR_AERS;
-	schedule_work(&ctrl->async_event_work);
+	queue_work(nvme_wq, &ctrl->async_event_work);
 }
 EXPORT_SYMBOL_GPL(nvme_queue_async_events);
Signature · 1 line
2.7.4
maintainer
#20 lore raw

PATCH 10/11 nvme: queue ns scanning and async request from nvme_wq

Quoted 8 lines · Sagi Grimberg

To suppress the warning triggered by nvme_uninit_ctrl:
kernel: [ 50.350439] nvme nvme0: rescanning
kernel: [ 50.363351] ------------[ cut here]------------
kernel: [ 50.363396] WARNING: CPU: 1 PID: 37 at kernel/workqueue.c:2423 check_flush_dependency+0x11f/0x130
kernel: [ 50.363409] workqueue: WQ_MEM_RECLAIM
nvme-wq:nvme_del_ctrl_work [nvme_core] is flushing !WQ_MEM_RECLAIM events:nvme_scan_work [nvme_core]

This was triggered with nvme-loop, but can happen with rdma/pci as well afaict.

Did you check that this won't create conflicts with other nvme actions (e.g. reset)? I remember some issues in the past, but I'd have to look up the details.

author maintainer
#21 lore raw

PATCH 10/11 nvme: queue ns scanning and async request from nvme_wq

Quoted 4 lines

Did you check that this won't create conflicts with other nvme actions (e.g. reset)? I remember some issues in the past, but I'd have to look up the details.

I gave it some thought, due to the fact that we use a multi-threaded workqueue I think there shouldn't be any problem.

I would have been taking the safe side if we had something like a system_mem_reclaim_wq...

author maintainer
#22 lore raw

PATCH 10/11 nvme: queue ns scanning and async request from nvme_wq

Quoted 3 lines
Quoted 3 lines

Did you check that this won't create conflicts with other nvme actions (e.g. reset)?

And yes, I did check controller resets and delete.

maintainer
#23 lore raw

PATCH 10/11 nvme: queue ns scanning and async request from nvme_wq

In reply to Sagi Grimberg

Ok, with the additional explanation this looks fine to me:

Reviewed-by Christoph Hellwig <hch at lst.de>
author maintainer
#24 lore raw

PATCH 11/11 nvme: move nr_reconnects to nvme_ctrl

In reply to Sagi Grimberg

It is not a user option but rather a variable controller attribute.

Signed-off-by Sagi Grimberg <sagi at grimberg.me>
---
 drivers/nvme/host/fabrics.c | 2 +-
 drivers/nvme/host/fabrics.h | 2 --
 drivers/nvme/host/nvme.h    | 1 +
 drivers/nvme/host/rdma.c    | 6 +++---
 4 files changed, 5 insertions(+), 6 deletions(-)
drivers/nvme/host/fabrics.c +1 -1
index 990e6fb32a63..466d12b61153 100644
@@ -474,7 +474,7 @@ EXPORT_SYMBOL_GPL(nvmf_connect_io_queue);
 bool nvmf_should_reconnect(struct nvme_ctrl *ctrl)
 {
 	if (ctrl->opts->max_reconnects != -1 &&
-	    ctrl->opts->nr_reconnects < ctrl->opts->max_reconnects)
+	    ctrl->nr_reconnects < ctrl->opts->max_reconnects)
 		return true;
 
 	return false;
drivers/nvme/host/fabrics.h +0 -2
index f5a9c1fb186f..68e8c459bf0c 100644
@@ -80,7 +80,6 @@ enum {
  * @discovery_nqn: indicates if the subsysnqn is the well-known discovery NQN.
  * @kato:	Keep-alive timeout.
  * @host:	Virtual NVMe host, contains the NQN and Host ID.
- * @nr_reconnects: number of reconnect attempted since the last ctrl failure
  * @max_reconnects: maximum number of allowed reconnect attempts before removing
  *              the controller, (-1) means reconnect forever, zero means remove
  *              immediately;

@@ -98,7 +97,6 @@ struct nvmf_ctrl_options {
 	bool			discovery_nqn;
 	unsigned int		kato;
 	struct nvmf_host	*host;
-	int			nr_reconnects;
 	int			max_reconnects;
 };
drivers/nvme/host/nvme.h +1 -0
index edf3ee7da553..1d096754b27f 100644
@@ -174,6 +174,7 @@ struct nvme_ctrl {
 	u32 iorcsz;
 	u16 icdoff;
 	u16 maxcmd;
+	int nr_reconnects;
 	struct nvmf_ctrl_options *opts;
 };
drivers/nvme/host/rdma.c +3 -3
index ae16d44f2ada..b6ef98a3b6f4 100644
@@ -725,7 +725,7 @@ static void nvme_rdma_reconnect_ctrl_work(struct work_struct *work)
 	bool changed;
 	int ret;
 
-	++ctrl->ctrl.opts->nr_reconnects;
+	++ctrl->ctrl.nr_reconnects;
 
 	if (ctrl->queue_count > 1) {
 		nvme_rdma_free_io_queues(ctrl);

@@ -771,7 +771,7 @@ static void nvme_rdma_reconnect_ctrl_work(struct work_struct *work)
 
 	changed = nvme_change_ctrl_state(&ctrl->ctrl, NVME_CTRL_LIVE);
 	WARN_ON_ONCE(!changed);
-	ctrl->ctrl.opts->nr_reconnects = 0;
+	ctrl->ctrl.nr_reconnects = 0;
 
 	if (ctrl->queue_count > 1) {
 		nvme_start_queues(&ctrl->ctrl);

@@ -787,7 +787,7 @@ static void nvme_rdma_reconnect_ctrl_work(struct work_struct *work)
 	blk_mq_stop_hw_queues(ctrl->ctrl.admin_q);
 requeue:
 	dev_info(ctrl->ctrl.device, "Failed reconnect attempt %d\n",
-			ctrl->ctrl.opts->nr_reconnects);
+			ctrl->ctrl.nr_reconnects);
 	nvme_rdma_reconnect_or_remove(ctrl);
 }
Signature · 1 line
2.7.4
maintainer
#25 lore raw

PATCH 11/11 nvme: move nr_reconnects to nvme_ctrl

Looks fine,

Reviewed-by Christoph Hellwig <hch at lst.de>
maintainer
#26 lore raw

cover a few useful nvme fixes and cleanups

In reply to Sagi Grimberg
Quoted 1 line · Sagi Grimberg

Some patches I've been sitting on for that past two weeks.

What's your target for these? A few look like 4.12 material for sure, but some look like material for the next merge window.

author maintainer
#27 lore raw

cover a few useful nvme fixes and cleanups

Quoted 5 lines
Quoted 2 lines

Some patches I've been sitting on for that past two weeks.

What's your target for these? A few look like 4.12 material for sure, but some look like material for the next merge window.

Nothing here is 4.12 material I think.

Only patch 10/11 fixes a real bug if I'm not wrong. Note that I've triggered it only with loop which before patch 9/11 didn't even use a WQ_MEM_RECLAIM workqueue, but once I converted it to use that, it easily triggered the warning which I think can easily happen if the pci driver happens to call nvme_uninit_ctrl when a ns scan is running (system_wq is not WQ_MEM_RECLAIM).