Skip to contentSkip to conversation

NVMe hot plug stuff

PATCH New Keith Busch 4 participants 18 messages Feb 17, 2016 – Feb 21, 2016 NVMe

lore
Share & tools
lore

Keith Busch

author
#0 lore raw

cover NVMe hot plug stuff

This is based of the current linux-block/for-linus branch.

Only 4/5 is the same as from v2, and have retained the reviews. The rest are new or different.

v2->v3:

Used a different approach to the namespace removal deadlock by reducing the scope of the namespace mutex so removal doesn't hold it during IO. This has a dependency on the first patch to ensure a scan doesn't somehow trigger (however unlikely) during removal.

Squashed v2's follow up fix for checking namespace state in nvme's queue_rq.

The final patch doesn't unmap controller registers until removal. This makes it safe to run startup and shutdown concurrently. We could possibly later micro optimize the IO path since the queue doorbell is always mapped. The patch was tested with suspend/resume using rtcwake.

Keith Busch (5):
NVMe: Add controller removing state
NVMe: Fix namespace removal deadlock
NVMe: Simplify device reset failure
NVMe: Move error handling to failed reset handler
NVMe: Don't unmap controller registers on reset

 drivers/nvme/host/core.c |  62 +++++++++++++++--------
 drivers/nvme/host/nvme.h |   6 +++
 drivers/nvme/host/pci.c  | 125 ++++++++++++++++++++++++++---------------------
 3 files changed, 118 insertions(+), 75 deletions(-)
Signature · 1 line
2.6.2.307.g37023ba
author
#1 lore raw

PATCH 1/5 NVMe: Add controller removing state

The patch is preparing for reducing the scope of the namespace mutex. It adds a removing state the namespace scan work uses to know if it is safe to proceed. The scan work is flushed after setting the device state so the removal path can safely walk the namespace list without locking it.

Signed-off-by Keith Busch <keith.busch at intel.com>
---
 drivers/nvme/host/pci.c | 4 ++++
 1 file changed, 4 insertions(+)
drivers/nvme/host/pci.c +4 -0
index a128672..f0e97a3 100644
@@ -120,6 +120,7 @@ struct nvme_dev {
 	unsigned long flags;
 
 #define NVME_CTRL_RESETTING    0
+#define NVME_CTRL_REMOVING     1
 
 	struct nvme_ctrl ctrl;
 	struct completion ioq_wait;

@@ -1583,6 +1584,8 @@ static void nvme_dev_scan(struct work_struct *work)
 {
 	struct nvme_dev *dev = container_of(work, struct nvme_dev, scan_work);
 
+	if (test_bit(NVME_CTRL_REMOVING, &dev->flags))
+		return;
 	if (!dev->tagset.tags)
 		return;
 	nvme_scan_namespaces(&dev->ctrl);

@@ -2116,6 +2119,7 @@ static void nvme_remove(struct pci_dev *pdev)
 {
 	struct nvme_dev *dev = pci_get_drvdata(pdev);
 
+	set_bit(NVME_CTRL_REMOVING, &dev->flags);
 	pci_set_drvdata(pdev, NULL);
 	flush_work(&dev->scan_work);
 	nvme_remove_namespaces(&dev->ctrl);
Signature · 1 line
2.6.2.307.g37023ba
#2 lore raw

PATCH 1/5 NVMe: Add controller removing state

Quoted 6 lines · Keith Busch

The patch is preparing for reducing the scope of the namespace mutex. It adds a removing state the namespace scan work uses to know if it is safe to proceed. The scan work is flushed after setting the device state so the removal path can safely walk the namespace list without locking it.

Signed-off-by Keith Busch <keith.busch at intel.com>

Looks fine, although the flushing / list access magic could relaly use a comment describing it in the source code.

#3 lore raw

PATCH 1/5 NVMe: Add controller removing state

In reply to Keith Busch
Quoted 6 lines · Keith Busch

The patch is preparing for reducing the scope of the namespace mutex. It adds a removing state the namespace scan work uses to know if it is safe to proceed. The scan work is flushed after setting the device state so the removal path can safely walk the namespace list without locking it.

Signed-off-by Keith Busch <keith.busch at intel.com>
Reviewed-by Johannes Thumshirn <jthumshirn at suse.de>
Signature · 6 lines
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
author
#4 lore raw

PATCH 2/5 NVMe: Fix namespace removal deadlock

In reply to Keith Busch

This patch makes nvme namespace removal lockless. It is up to the caller to ensure no active namespace scanning is occuring.

The lockless removal allows the driver to cleanup a namespace request_queue if the controller fails during removal. Previously this could deadlock trying to acquire the namespace mutex in order to handle such events.

Signed-off-by Keith Busch <keith.busch at intel.com>
---
 drivers/nvme/host/core.c | 15 +++++++++------
 drivers/nvme/host/nvme.h |  4 ++++
 2 files changed, 13 insertions(+), 6 deletions(-)
drivers/nvme/host/core.c +9 -6
index 3cd921e..1b7387d 100644
@@ -48,6 +48,10 @@ static void nvme_free_ns(struct kref *kref)
 {
 	struct nvme_ns *ns = container_of(kref, struct nvme_ns, kref);
 
+	mutex_lock(&ns->ctrl->namespaces_mutex);
+	list_del_init(&ns->list);
+	mutex_unlock(&ns->ctrl->namespaces_mutex);
+
 	if (ns->type == NVME_NS_LIGHTNVM)
 		nvme_nvm_unregister(ns->queue, ns->disk->disk_name);
 

@@ -1179,11 +1183,13 @@ static void nvme_alloc_ns(struct nvme_ctrl *ctrl, unsigned nsid)
 
 static void nvme_ns_remove(struct nvme_ns *ns)
 {
-	bool kill = nvme_io_incapable(ns->ctrl) &&
-			!blk_queue_dying(ns->queue);
+	bool kill;
 
-	lockdep_assert_held(&ns->ctrl->namespaces_mutex);
+	if (test_and_set_bit(NVME_NS_REMOVING, &ns->flags))
+		return;
 
+	kill = nvme_io_incapable(ns->ctrl) &&
+			!blk_queue_dying(ns->queue);
 	if (kill) {
 		blk_set_queue_dying(ns->queue);
 

@@ -1206,7 +1212,6 @@ static void nvme_ns_remove(struct nvme_ns *ns)
 		blk_mq_abort_requeue_list(ns->queue);
 		blk_cleanup_queue(ns->queue);
 	}
-	list_del_init(&ns->list);
 	nvme_put_ns(ns);
 }
 

@@ -1300,10 +1305,8 @@ void nvme_remove_namespaces(struct nvme_ctrl *ctrl)
 {
 	struct nvme_ns *ns, *next;
 
-	mutex_lock(&ctrl->namespaces_mutex);
 	list_for_each_entry_safe(ns, next, &ctrl->namespaces, list)
 		nvme_ns_remove(ns);
-	mutex_unlock(&ctrl->namespaces_mutex);
 }
 
 static DEFINE_IDA(nvme_instance_ida);
drivers/nvme/host/nvme.h +4 -0
index 9664d07..db413a7 100644
@@ -112,6 +112,10 @@ struct nvme_ns {
 	bool ext;
 	u8 pi_type;
 	int type;
+	unsigned long flags;
+
+#define NVME_NS_REMOVING 0
+
 	u64 mode_select_num_blocks;
 	u32 mode_select_block_len;
 };
Signature · 1 line
2.6.2.307.g37023ba
#5 lore raw

PATCH 2/5 NVMe: Fix namespace removal deadlock

Review void nvme_remove_namespaces(struct nvme_ctrl *ctrl) { · line 1309

Quoted 9 lines
(quoted diff) +0 -2
@@ -1300,10 +1305,8 @@ void nvme_remove_namespaces(struct nvme_ctrl *ctrl)  {
 	struct nvme_ns *ns, *next;
 
-	mutex_lock(&ctrl->namespaces_mutex);
 	list_for_each_entry_safe(ns, next, &ctrl->namespaces, list)
 		nvme_ns_remove(ns);
-	mutex_unlock(&ctrl->namespaces_mutex);
 }

Without this mutex, is it safe to run concurrently with nvme_free_ns which manipulates the list?

-----Original Message-----
From: Keith Busch [mailto:[email protected]]
Sent: Thursday, February 18, 2016 2:37 AM
To: linux-nvme at lists.infradead.org; Jens Axboe; Christoph Hellwig
Cc: Sagi Grimberg; Wenbo Wang; Johannes Thumshirn; Keith Busch
Subject: [PATCH-4.5-v3 2/5] NVMe: Fix namespace removal deadlock

This patch makes nvme namespace removal lockless. It is up to the caller to ensure no active namespace scanning is occuring.

The lockless removal allows the driver to cleanup a namespace request_queue if the controller fails during removal. Previously this could deadlock trying to acquire the namespace mutex in order to handle such events.

Signed-off-by Keith Busch <keith.busch at intel.com>
---
 drivers/nvme/host/core.c | 15 +++++++++------  drivers/nvme/host/nvme.h |  4 ++++
 2 files changed, 13 insertions(+), 6 deletions(-)
drivers/nvme/host/core.c +9 -6
@@ -48,6 +48,10 @@ static void nvme_free_ns(struct kref *kref)  {
 	struct nvme_ns *ns = container_of(kref, struct nvme_ns, kref);
 
+	mutex_lock(&ns->ctrl->namespaces_mutex);
+	list_del_init(&ns->list);
+	mutex_unlock(&ns->ctrl->namespaces_mutex);
+
 	if (ns->type == NVME_NS_LIGHTNVM)
 		nvme_nvm_unregister(ns->queue, ns->disk->disk_name);
 

@@ -1179,11 +1183,13 @@ static void nvme_alloc_ns(struct nvme_ctrl *ctrl, unsigned nsid)
 
 static void nvme_ns_remove(struct nvme_ns *ns)  {
-	bool kill = nvme_io_incapable(ns->ctrl) &&
-			!blk_queue_dying(ns->queue);
+	bool kill;
 
-	lockdep_assert_held(&ns->ctrl->namespaces_mutex);
+	if (test_and_set_bit(NVME_NS_REMOVING, &ns->flags))
+		return;
 
+	kill = nvme_io_incapable(ns->ctrl) &&
+			!blk_queue_dying(ns->queue);
 	if (kill) {
 		blk_set_queue_dying(ns->queue);
 

@@ -1206,7 +1212,6 @@ static void nvme_ns_remove(struct nvme_ns *ns)
 		blk_mq_abort_requeue_list(ns->queue);
 		blk_cleanup_queue(ns->queue);
 	}
-	list_del_init(&ns->list);
 	nvme_put_ns(ns);
 }
 

@@ -1300,10 +1305,8 @@ void nvme_remove_namespaces(struct nvme_ctrl *ctrl)  {
 	struct nvme_ns *ns, *next;
 
-	mutex_lock(&ctrl->namespaces_mutex);
 	list_for_each_entry_safe(ns, next, &ctrl->namespaces, list)
 		nvme_ns_remove(ns);
-	mutex_unlock(&ctrl->namespaces_mutex);
 }
 
 static DEFINE_IDA(nvme_instance_ida);
drivers/nvme/host/nvme.h +4 -0
@@ -112,6 +112,10 @@ struct nvme_ns {
 	bool ext;
 	u8 pi_type;
 	int type;
+	unsigned long flags;
+
+#define NVME_NS_REMOVING 0
+
 	u64 mode_select_num_blocks;
 	u32 mode_select_block_len;
 };

--
2.6.2.307.g37023ba

#6 lore raw

PATCH 2/5 NVMe: Fix namespace removal deadlock

Quoted 1 line · Wenbo Wang

Without this mutex, is it safe to run concurrently with nvme_free_ns which manipulates the list?

see the first patch for an explanation on why this should be safe.

author
#7 lore raw

PATCH 2/5 NVMe: Fix namespace removal deadlock

Quoted 4 lines · Christoph Hellwig
Quoted 1 line · Wenbo Wang

Without this mutex, is it safe to run concurrently with nvme_free_ns which manipulates the list?

see the first patch for an explanation on why this should be safe.

I think the potential problem Wenbo is talking about is if someone is holding an open reference (mounted, for example) after detaching the namespace.

Removing it from the list when all references are released keeps the list consistent with the block layer so that attaching a different namespace with the same NSID won't create a naming conflict. The newly attached namespace isn't be usable in this case, though.

Instead of using NSID, I think the disk name should use an IDA like the controller's instance. Then we can unlink the namespace from the list when detached instead of with the last reference.

The downside is you can't infer the NSID from the name, but we have NVME_IOCTL_ID and /sys/block/nvmeXnY/nsid for that.

#8 lore raw

PATCH 2/5 NVMe: Fix namespace removal deadlock

In reply to Keith Busch
Quoted 9 lines · Keith Busch

This patch makes nvme namespace removal lockless. It is up to the caller to ensure no active namespace scanning is occuring.

The lockless removal allows the driver to cleanup a namespace request_queue if the controller fails during removal. Previously this could deadlock trying to acquire the namespace mutex in order to handle such events.

Signed-off-by Keith Busch <keith.busch at intel.com>
Reviewed-by Johannes Thumshirn <jthumshirn at suse.de>
Signature · 6 lines
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
#9 lore raw

PATCH 2/5 NVMe: Fix namespace removal deadlock

In reply to Keith Busch
Quoted 2 lines · Keith Busch

This patch makes nvme namespace removal lockless. It is up to the caller to ensure no active namespace scanning is occuring.

I wonder if it wouldn't be safer in the long run if we just switch to RCU locking - that way we don't need any locks in the readers, they can just grab a reference under rcu_read_lock(), and we don't need to rely in implicit serialization for the updaters.

author
#10 lore raw

PATCH 3/5 NVMe: Simplify device reset failure

In reply to Keith Busch

A reset failure schedules the device to unbind from the driver through the pci driver's remove. This cleans up all intialization, so there is no need to duplicate the potentially racy cleanup.

To help understand why a reset failed, the status is logged with the existing warning message.

Signed-off-by Keith Busch <keith.busch at intel.com>
Reviewed-by Johannes Thumshirn <jthumshirn at suse.de>
Reviewed-by Christoph Hellwig <hch at lst.de>
---
 drivers/nvme/host/pci.c | 48 +++++++++++++++++++++---------------------------
 1 file changed, 21 insertions(+), 27 deletions(-)
drivers/nvme/host/pci.c +21 -27
index f0e97a3..9e050dd 100644
@@ -86,7 +86,6 @@ struct nvme_queue;
 
 static int nvme_reset(struct nvme_dev *dev);
 static void nvme_process_cq(struct nvme_queue *nvmeq);
-static void nvme_remove_dead_ctrl(struct nvme_dev *dev);
 static void nvme_dev_disable(struct nvme_dev *dev, bool shutdown);
 
 /*

@@ -1902,10 +1901,19 @@ static void nvme_pci_free_ctrl(struct nvme_ctrl *ctrl)
 	kfree(dev);
 }
 
+static void nvme_remove_dead_ctrl(struct nvme_dev *dev, int status)
+{
+	dev_warn(dev->dev, "Removing after probe failure status: %d\n", status);
+
+	kref_get(&dev->ctrl.kref);
+	if (!schedule_work(&dev->remove_work))
+		nvme_put_ctrl(&dev->ctrl);
+}
+
 static void nvme_reset_work(struct work_struct *work)
 {
 	struct nvme_dev *dev = container_of(work, struct nvme_dev, reset_work);
-	int result;
+	int result = -ENODEV;
 
 	if (WARN_ON(test_bit(NVME_CTRL_RESETTING, &dev->flags)))
 		goto out;

@@ -1925,26 +1933,26 @@ static void nvme_reset_work(struct work_struct *work)
 
 	result = nvme_configure_admin_queue(dev);
 	if (result)
-		goto unmap;
+		goto out;
 
 	nvme_init_queue(dev->queues[0], 0);
 	result = nvme_alloc_admin_tags(dev);
 	if (result)
-		goto disable;
+		goto out;
 
 	result = nvme_init_identify(&dev->ctrl);
 	if (result)
-		goto free_tags;
+		goto out;
 
 	result = nvme_setup_io_queues(dev);
 	if (result)
-		goto free_tags;
+		goto out;
 
 	dev->ctrl.event_limit = NVME_NR_AEN_COMMANDS;
 
 	result = nvme_dev_list_add(dev);
 	if (result)
-		goto remove;
+		goto out;
 
 	/*
 	 * Keep the controller around but remove all namespaces if we don't have

@@ -1961,19 +1969,8 @@ static void nvme_reset_work(struct work_struct *work)
 	clear_bit(NVME_CTRL_RESETTING, &dev->flags);
 	return;
 
- remove:
-	nvme_dev_list_remove(dev);
- free_tags:
-	nvme_dev_remove_admin(dev);
-	blk_put_queue(dev->ctrl.admin_q);
-	dev->ctrl.admin_q = NULL;
-	dev->queues[0]->tags = NULL;
- disable:
-	nvme_disable_admin_queue(dev, false);
- unmap:
-	nvme_dev_unmap(dev);
  out:
-	nvme_remove_dead_ctrl(dev);
+	nvme_remove_dead_ctrl(dev, result);
 }
 
 static void nvme_remove_dead_ctrl_work(struct work_struct *work)

@@ -1986,14 +1983,6 @@ static void nvme_remove_dead_ctrl_work(struct work_struct *work)
 	nvme_put_ctrl(&dev->ctrl);
 }
 
-static void nvme_remove_dead_ctrl(struct nvme_dev *dev)
-{
-	dev_warn(dev->dev, "Removing after probe failure\n");
-	kref_get(&dev->ctrl.kref);
-	if (!schedule_work(&dev->remove_work))
-		nvme_put_ctrl(&dev->ctrl);
-}
-
 static int nvme_reset(struct nvme_dev *dev)
 {
 	if (!dev->ctrl.admin_q || blk_queue_dying(dev->ctrl.admin_q))

@@ -2115,6 +2104,11 @@ static void nvme_shutdown(struct pci_dev *pdev)
 	nvme_dev_disable(dev, true);
 }
 
+/*
+ * The driver's remove may be called on a device in a partially initialized
+ * state. This function must not have any dependencies on the device state in
+ * order to proceed.
+ */
 static void nvme_remove(struct pci_dev *pdev)
 {
 	struct nvme_dev *dev = pci_get_drvdata(pdev);
Signature · 1 line
2.6.2.307.g37023ba
author
#11 lore raw

PATCH 4/5 NVMe: Move error handling to failed reset handler

In reply to Keith Busch

This moves failed queue handling out of the namespace removal path and into the reset failure path, fixing a hanging condition if the controller fails or link down during del_gendisk. Previously the driver had to see the controller as degraded prior to calling del_gendisk to setup the queues to fail. If the controller happened to fail after this though, there was no task to end the request_queue.

On failure, all namespace states are set to dead. This has capacity revalidate to 0, and ends all new requests with error status.

Signed-off-by Keith Busch <keith.busch at intel.com>
---
 drivers/nvme/host/core.c | 53 ++++++++++++++++++++++++++++++++----------------
 drivers/nvme/host/nvme.h |  2 ++
 drivers/nvme/host/pci.c  |  7 ++++++-
 3 files changed, 44 insertions(+), 18 deletions(-)
drivers/nvme/host/core.c +36 -17
index 1b7387d..face31f 100644
@@ -560,6 +560,10 @@ static int nvme_revalidate_disk(struct gendisk *disk)
 	u16 old_ms;
 	unsigned short bs;
 
+	if (test_bit(NVME_NS_DEAD, &ns->flags)) {
+		set_capacity(disk, 0);
+		return -ENODEV;
+	}
 	if (nvme_identify_ns(ns->ctrl, ns->ns_id, &id)) {
 		dev_warn(ns->ctrl->dev, "%s: Identify failure nvme%dn%d\n",
 				__func__, ns->ctrl->instance, ns->ns_id);

@@ -1183,32 +1187,15 @@ static void nvme_alloc_ns(struct nvme_ctrl *ctrl, unsigned nsid)
 
 static void nvme_ns_remove(struct nvme_ns *ns)
 {
-	bool kill;
-
 	if (test_and_set_bit(NVME_NS_REMOVING, &ns->flags))
 		return;
 
-	kill = nvme_io_incapable(ns->ctrl) &&
-			!blk_queue_dying(ns->queue);
-	if (kill) {
-		blk_set_queue_dying(ns->queue);
-
-		/*
-		 * The controller was shutdown first if we got here through
-		 * device removal. The shutdown may requeue outstanding
-		 * requests. These need to be aborted immediately so
-		 * del_gendisk doesn't block indefinitely for their completion.
-		 */
-		blk_mq_abort_requeue_list(ns->queue);
-	}
 	if (ns->disk->flags & GENHD_FL_UP) {
 		if (blk_get_integrity(ns->disk))
 			blk_integrity_unregister(ns->disk);
 		sysfs_remove_group(&disk_to_dev(ns->disk)->kobj,
 					&nvme_ns_attr_group);
 		del_gendisk(ns->disk);
-	}
-	if (kill || !blk_queue_dying(ns->queue)) {
 		blk_mq_abort_requeue_list(ns->queue);
 		blk_cleanup_queue(ns->queue);
 	}

@@ -1405,6 +1392,38 @@ out:
 	return ret;
 }
 
+/**
+ * nvme_kill_queues(): Ends all namespace queues
+ * @ctrl: the dead controller that needs to end
+ *
+ * Call this function when the driver determines it is unable to get the
+ * controller in a state capable of servicing IO.
+ */
+void nvme_kill_queues(struct nvme_ctrl *ctrl)
+{
+	struct nvme_ns *ns;
+
+	mutex_lock(&ctrl->namespaces_mutex);
+	list_for_each_entry(ns, &ctrl->namespaces, list) {
+		if (!kref_get_unless_zero(&ns->kref))
+			continue;
+
+		/*
+		 * Revalidating a dead namespace sets capacity to 0. This will
+		 * end buffered writers dirtying pages that can't be synced.
+		 */
+		if (!test_and_set_bit(NVME_NS_DEAD, &ns->flags))
+			revalidate_disk(ns->disk);
+
+		blk_set_queue_dying(ns->queue);
+		blk_mq_abort_requeue_list(ns->queue);
+		blk_mq_start_stopped_hw_queues(ns->queue, true);
+
+		nvme_put_ns(ns);
+	}
+	mutex_unlock(&ctrl->namespaces_mutex);
+}
+
 void nvme_stop_queues(struct nvme_ctrl *ctrl)
 {
 	struct nvme_ns *ns;
drivers/nvme/host/nvme.h +2 -0
index db413a7..dcdc2d7 100644
@@ -115,6 +115,7 @@ struct nvme_ns {
 	unsigned long flags;
 
 #define NVME_NS_REMOVING 0
+#define NVME_NS_DEAD     1
 
 	u64 mode_select_num_blocks;
 	u32 mode_select_block_len;

@@ -244,6 +245,7 @@ void nvme_remove_namespaces(struct nvme_ctrl *ctrl);
 
 void nvme_stop_queues(struct nvme_ctrl *ctrl);
 void nvme_start_queues(struct nvme_ctrl *ctrl);
+void nvme_kill_queues(struct nvme_ctrl *ctrl);
 
 struct request *nvme_alloc_request(struct request_queue *q,
 		struct nvme_command *cmd, unsigned int flags);
drivers/nvme/host/pci.c +6 -1
index 9e050dd..27042c0 100644
@@ -679,7 +679,10 @@ static int nvme_queue_rq(struct blk_mq_hw_ctx *hctx,
 
 	spin_lock_irq(&nvmeq->q_lock);
 	if (unlikely(nvmeq->cq_vector < 0)) {
-		ret = BLK_MQ_RQ_QUEUE_BUSY;
+		if (ns && test_bit(NVME_NS_DEAD, &ns->flags))
+			ret = BLK_MQ_RQ_QUEUE_ERROR;
+		else
+			ret = BLK_MQ_RQ_QUEUE_BUSY;
 		spin_unlock_irq(&nvmeq->q_lock);
 		goto out;
 	}

@@ -1906,6 +1909,7 @@ static void nvme_remove_dead_ctrl(struct nvme_dev *dev, int status)
 	dev_warn(dev->dev, "Removing after probe failure status: %d\n", status);
 
 	kref_get(&dev->ctrl.kref);
+	nvme_dev_disable(dev, false);
 	if (!schedule_work(&dev->remove_work))
 		nvme_put_ctrl(&dev->ctrl);
 }

@@ -1978,6 +1982,7 @@ static void nvme_remove_dead_ctrl_work(struct work_struct *work)
 	struct nvme_dev *dev = container_of(work, struct nvme_dev, remove_work);
 	struct pci_dev *pdev = to_pci_dev(dev->dev);
 
+	nvme_kill_queues(&dev->ctrl);
 	if (pci_get_drvdata(pdev))
 		pci_stop_and_remove_bus_device_locked(pdev);
 	nvme_put_ctrl(&dev->ctrl);
Signature · 1 line
2.6.2.307.g37023ba
#12 lore raw

PATCH 4/5 NVMe: Move error handling to failed reset handler

drivers/nvme/host/core.c out: · line 1402

Quoted 8 lines
drivers/nvme/host/core.c +8 -0
+/**
+ * nvme_kill_queues(): Ends all namespace queues
+ * @ctrl: the dead controller that needs to end
+ *
+ * Call this function when the driver determines it is unable to get the
+ * controller in a state capable of servicing IO.
+ */
+void nvme_kill_queues(struct nvme_ctrl *ctrl)

Is there a good reason to skyp the admin queue here entirely?

Otherwise looks fine to me.

author
#13 lore raw

PATCH 4/5 NVMe: Move error handling to failed reset handler

Quoted 1 line · Christoph Hellwig

Is there a good reason to skyp the admin queue here entirely?

It is not necessary to progress namespace removal, and we don't want to step on the controller removal path in case it actually is making progress (we only think it's dead in this path).

A catch all place to restart a failed admin queue to allow controller removal to complete is nvme_dev_remove_admin().

---

drivers/nvme/host/pci.c +6 -0
index f11daba..4953008 100644
@@ -1253,6 +1253,12 @@ static struct blk_mq_ops nvme_mq_ops = {
 static void nvme_dev_remove_admin(struct nvme_dev *dev)
 {
 	if (dev->ctrl.admin_q && !blk_queue_dying(dev->ctrl.admin_q)) {
+		/*
+		 * If the controller was reset during removal, it's possible
+		 * user requests may be waiting on a stopped queue. Start the
+		 * queue to flush these to completion.
+		 */
+		blk_mq_start_stopped_hw_queues(dev->ctrl.admin_q, true);
 		blk_cleanup_queue(dev->ctrl.admin_q);
 		blk_mq_free_tag_set(&dev->admin_tagset);
 	}

--

BTW, from testing this I notice the logic in nvme_queue_rq was screwed up in this patch when I switched from the ternary '?:' to 'if-else'.

Jens hasn't pushed anything this series yet so I'll resend and fold the above in.

#14 lore raw

PATCH 4/5 NVMe: Move error handling to failed reset handler

In reply to Keith Busch
Quoted 11 lines · Keith Busch

This moves failed queue handling out of the namespace removal path and into the reset failure path, fixing a hanging condition if the controller fails or link down during del_gendisk. Previously the driver had to see the controller as degraded prior to calling del_gendisk to setup the queues to fail. If the controller happened to fail after this though, there was no task to end the request_queue.

On failure, all namespace states are set to dead. This has capacity revalidate to 0, and ends all new requests with error status.

Signed-off-by Keith Busch <keith.busch at intel.com>
Reviewed-by Johannes Thumshirn <jthumshirn at suse.de>
Signature · 6 lines
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
author
#15 lore raw

PATCH 5/5 NVMe: Don't unmap controller registers on reset

In reply to Keith Busch

Unmapping the registers on reset or shutdown is not necessary. Keeping the mapping simplifies reset handling.

Signed-off-by Keith Busch <keith.busch at intel.com>
---
 drivers/nvme/host/pci.c | 71 +++++++++++++++++++++++++++++--------------------
 1 file changed, 42 insertions(+), 29 deletions(-)
drivers/nvme/host/pci.c +42 -29
index 27042c0..f11daba 100644
@@ -1699,10 +1699,10 @@ static int nvme_dev_add(struct nvme_dev *dev)
 	return 0;
 }
 
-static int nvme_dev_map(struct nvme_dev *dev)
+static int nvme_pci_enable(struct nvme_dev *dev)
 {
 	u64 cap;
-	int bars, result = -ENOMEM;
+	int result = -ENOMEM;
 	struct pci_dev *pdev = to_pci_dev(dev->dev);
 
 	if (pci_enable_device_mem(pdev))

@@ -1710,24 +1710,14 @@ static int nvme_dev_map(struct nvme_dev *dev)
 
 	dev->entry[0].vector = pdev->irq;
 	pci_set_master(pdev);
-	bars = pci_select_bars(pdev, IORESOURCE_MEM);
-	if (!bars)
-		goto disable_pci;
-
-	if (pci_request_selected_regions(pdev, bars, "nvme"))
-		goto disable_pci;
 
 	if (dma_set_mask_and_coherent(dev->dev, DMA_BIT_MASK(64)) &&
 	    dma_set_mask_and_coherent(dev->dev, DMA_BIT_MASK(32)))
 		goto disable;
 
-	dev->bar = ioremap(pci_resource_start(pdev, 0), 8192);
-	if (!dev->bar)
-		goto disable;
-
 	if (readl(dev->bar + NVME_REG_CSTS) == -1) {
 		result = -ENODEV;
-		goto unmap;
+		goto disable;
 	}
 
 	/*

@@ -1737,7 +1727,7 @@ static int nvme_dev_map(struct nvme_dev *dev)
 	if (!pdev->irq) {
 		result = pci_enable_msix(pdev, dev->entry, 1);
 		if (result < 0)
-			goto unmap;
+			goto disable;
 	}
 
 	cap = lo_hi_readq(dev->bar + NVME_REG_CAP);

@@ -1764,18 +1754,20 @@ static int nvme_dev_map(struct nvme_dev *dev)
 	pci_save_state(pdev);
 	return 0;
 
- unmap:
-	iounmap(dev->bar);
-	dev->bar = NULL;
  disable:
-	pci_release_regions(pdev);
- disable_pci:
 	pci_disable_device(pdev);
 	return result;
 }
 
 static void nvme_dev_unmap(struct nvme_dev *dev)
 {
+	if (dev->bar)
+		iounmap(dev->bar);
+	pci_release_regions(to_pci_dev(dev->dev));
+}
+
+static void nvme_pci_disable(struct nvme_dev *dev)
+{
 	struct pci_dev *pdev = to_pci_dev(dev->dev);
 
 	if (pdev->msi_enabled)

@@ -1783,12 +1775,6 @@ static void nvme_dev_unmap(struct nvme_dev *dev)
 	else if (pdev->msix_enabled)
 		pci_disable_msix(pdev);
 
-	if (dev->bar) {
-		iounmap(dev->bar);
-		dev->bar = NULL;
-		pci_release_regions(pdev);
-	}
-
 	if (pci_is_enabled(pdev)) {
 		pci_disable_pcie_error_reporting(pdev);
 		pci_disable_device(pdev);

@@ -1847,7 +1833,7 @@ static void nvme_dev_disable(struct nvme_dev *dev, bool shutdown)
 	nvme_dev_list_remove(dev);
 
 	mutex_lock(&dev->shutdown_lock);
-	if (dev->bar) {
+	if (pci_is_enabled(to_pci_dev(dev->dev))) {
 		nvme_stop_queues(&dev->ctrl);
 		csts = readl(dev->bar + NVME_REG_CSTS);
 	}

@@ -1860,7 +1846,7 @@ static void nvme_dev_disable(struct nvme_dev *dev, bool shutdown)
 		nvme_disable_io_queues(dev);
 		nvme_disable_admin_queue(dev, shutdown);
 	}
-	nvme_dev_unmap(dev);
+	nvme_pci_disable(dev);
 
 	for (i = dev->queue_count - 1; i >= 0; i--)
 		nvme_clear_queue(dev->queues[i]);

@@ -1926,12 +1912,12 @@ static void nvme_reset_work(struct work_struct *work)
 	 * If we're called to reset a live controller first shut it down before
 	 * moving on.
 	 */
-	if (dev->bar)
+	if (dev->ctrl.ctrl_config & NVME_CC_ENABLE)
 		nvme_dev_disable(dev, false);
 
 	set_bit(NVME_CTRL_RESETTING, &dev->flags);
 
-	result = nvme_dev_map(dev);
+	result = nvme_pci_enable(dev);
 	if (result)
 		goto out;
 

@@ -2039,6 +2025,27 @@ static const struct nvme_ctrl_ops nvme_pci_ctrl_ops = {
 	.free_ctrl		= nvme_pci_free_ctrl,
 };
 
+static int nvme_dev_map(struct nvme_dev *dev)
+{
+	int bars;
+	struct pci_dev *pdev = to_pci_dev(dev->dev);
+
+	bars = pci_select_bars(pdev, IORESOURCE_MEM);
+	if (!bars)
+		return -ENODEV;
+	if (pci_request_selected_regions(pdev, bars, "nvme"))
+		return -ENODEV;
+
+	dev->bar = ioremap(pci_resource_start(pdev, 0), 8192);
+	if (!dev->bar)
+		goto release;
+
+       return 0;
+  release:
+       pci_release_regions(pdev);
+       return -ENODEV;
+}
+
 static int nvme_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 {
 	int node, result = -ENOMEM;

@@ -2063,6 +2070,10 @@ static int nvme_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	dev->dev = get_device(&pdev->dev);
 	pci_set_drvdata(pdev, dev);
 
+	result = nvme_dev_map(dev);
+	if (result)
+		goto free;
+
 	INIT_LIST_HEAD(&dev->node);
 	INIT_WORK(&dev->scan_work, nvme_dev_scan);
 	INIT_WORK(&dev->reset_work, nvme_reset_work);

@@ -2086,6 +2097,7 @@ static int nvme_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	nvme_release_prp_pools(dev);
  put_pci:
 	put_device(dev->dev);
+	nvme_dev_unmap(dev);
  free:
 	kfree(dev->queues);
 	kfree(dev->entry);

@@ -2129,6 +2141,7 @@ static void nvme_remove(struct pci_dev *pdev)
 	nvme_free_queues(dev, 0);
 	nvme_release_cmb(dev);
 	nvme_release_prp_pools(dev);
+	nvme_dev_unmap(dev);
 	nvme_put_ctrl(&dev->ctrl);
 }
Signature · 1 line
2.6.2.307.g37023ba
#16 lore raw

PATCH 5/5 NVMe: Don't unmap controller registers on reset

Quoted 4 lines · Keith Busch

Unmapping the registers on reset or shutdown is not necessary. Keeping the mapping simplifies reset handling.

Signed-off-by Keith Busch <keith.busch at intel.com>
Reviewed-by Johannes Thumshirn <jthumshirn at suse.de>
Signature · 6 lines
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