nvme: Ensure forward progress during Admin passthru

How about changing the reaction based on success?

Quoted 1 line · Keith Busch

How about changing the reaction based on success?

I don't think this will work. The other scenario is the original admin command was fine. We enter nvme_update_formats, grab the semaphore as write, issue the nvme admin identify, that fails, we enter timeout, and attempt to down_read in diasble_dev -> stop queues and get stuck there.

We need a way where we are not holding a down_write on the semaphore while issuing I/O.

static void nvme_update_formats(struct nvme_ctrl *ctrl) {

drivers/nvme/host/core.c +0 -1

 	struct nvme_ns *ns, *next;
-	LIST_HEAD(rm_list);

This small patch is an attempt to fix a rare boundary condition. The only downside of the patch is scan work cannot happen concurrently, but I don't think that's possible outside of someone issuing a sysctl.

If the controller goes down during the admin command, the command will timeout, if the controller is in a poor enough state the reset commands will time out. After the original admin command timeout, we unblock userland which can start to revalidate the namespaces. In order todo that we aquire the controller namespace sempahore as WRITE. We then issue an admin command which times out, triggers a reset where we attempt to stop queues in dev_disable(). Stopping queues attempts to down_read on the semaphore, which is currently being held on a down write, waiting for this I/O to timeout. This I/O cannot timeout until the semaphore is dropped from down_write, and now we're deadlocked.

The patch does what nvme_remove_namespaces does, it takes over the controllers namespace list under the down_write. Then modifies the private list, potentially removing namespaces, and resplicing it once revalidation has occured. The whole goal is to *not* hold a down write while issuing admin commands.

Quoted 16 lines

This small patch is an attempt to fix a rare boundary condition. The only downside of the patch is scan work cannot happen concurrently, but I don't think that's possible outside of someone issuing a sysctl.

If the controller goes down during the admin command, the command will timeout, if the controller is in a poor enough state the reset commands will time out. After the original admin command timeout, we unblock userland which can start to revalidate the namespaces. In order todo that we aquire the controller namespace sempahore as WRITE. We then issue an admin command which times out, triggers a reset where we attempt to stop queues in dev_disable(). Stopping queues attempts to down_read on the semaphore, which is currently being held on a down write, waiting for this I/O to timeout. This I/O cannot timeout until the semaphore is dropped from down_write, and now we're deadlocked.

Quoted 4 lines

The patch does what nvme_remove_namespaces does, it takes over the controllers namespace list under the down_write. Then modifies the private list, potentially removing namespaces, and resplicing it once revalidation has occured. The whole goal is to *not* hold a down write while issuing admin commands.

Keith, can you explain why we need to disable the device from the timeout handler instead of simply schedule a reset and return BLK_EH_RESET_TIMER?

The only place where I *think* can't schedule a reset and need to return BLK_EH_DONE is when the controller is already resetting, in which case we'd simply need to cancel the timed out command.

What am I missing?

If the controller supports effects and goes down during the passthru admin command we will deadlock during namespace revalidation.

[  363.488275] INFO: task kworker/u16:5:231 blocked for more than 120 seconds.
[  363.488290]       Not tainted 4.17.0+ #2
[  363.488296] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
[  363.488303] kworker/u16:5   D    0   231      2 0x80000000
[  363.488331] Workqueue: nvme-reset-wq nvme_reset_work [nvme]
[  363.488338] Call Trace:
[  363.488385]  schedule+0x75/0x190
[  363.488396]  rwsem_down_read_failed+0x1c3/0x2f0
[  363.488481]  call_rwsem_down_read_failed+0x14/0x30
[  363.488504]  down_read+0x1d/0x80
[  363.488523]  nvme_stop_queues+0x1e/0xa0 [nvme_core]
[  363.488536]  nvme_dev_disable+0xae4/0x1620 [nvme]
[  363.488614]  nvme_reset_work+0xd1e/0x49d9 [nvme]
[  363.488911]  process_one_work+0x81a/0x1400
[  363.488934]  worker_thread+0x87/0xe80
[  363.488955]  kthread+0x2db/0x390
[  363.488977]  ret_from_fork+0x35/0x40

Fixes 84fef62d135b6 ("nvme: check admin passthru command effects")

Signed-off-by Scott Bauer <scott.bauer at intel.com>

---
 drivers/nvme/host/core.c | 32 ++++++++++++++++++--------------
 1 file changed, 18 insertions(+), 14 deletions(-)

drivers/nvme/host/core.c +0 -0

index 46df030b2c3f..1ad19f0782db 100644
@@ -100,6 +100,15 @@ static struct class *nvme_subsys_class;
 static void nvme_ns_remove(struct nvme_ns *ns);
 static int nvme_revalidate_disk(struct gendisk *disk);

Review

Quoted 10 lines · Scott Bauer

static void nvme_put_subsystem(struct nvme_subsystem *subsys);

(quoted diff) +9 -0

+static void nvme_remove_invalid_namespaces(struct nvme_ctrl *ctrl,
+					   unsigned nsid);
+
+static void nvme_set_queue_dying(struct nvme_ns *ns)
+{
+	blk_set_queue_dying(ns->queue);
+	/* Forcibly unquiesce queues to avoid blocking dispatch */
+	blk_mq_unquiesce_queue(ns->queue);
+}

I think we should actually do everything that the dead namespace does, including revalidating the capacity to 0, just in case a buffered writer is preventing the removal from completing. Here's an update on top of your patch.

{

Quoted 19 lines · Keith Busch

Review

Quoted 10 lines · Scott Bauer

static void nvme_put_subsystem(struct nvme_subsystem *subsys);

(quoted diff) +9 -0

+static void nvme_remove_invalid_namespaces(struct nvme_ctrl *ctrl,
+					   unsigned nsid);
+
+static void nvme_set_queue_dying(struct nvme_ns *ns)
+{
+	blk_set_queue_dying(ns->queue);
+	/* Forcibly unquiesce queues to avoid blocking dispatch */
+	blk_mq_unquiesce_queue(ns->queue);
+}

I think we should actually do everything that the dead namespace does, including revalidating the capacity to 0, just in case a buffered writer is preventing the removal from completing. Here's an update on top of your patch.

{

drivers/nvme/host/core.c static void nvme_remove_invalid_namespaces(struct nvme_ctrl *ctrl, · line 116

Quoted 7 lines

drivers/nvme/host/core.c +7 -0

+	/*
+	 * Revalidating a dead namespace sets capacity to 0. This will end
+	 * buffered writers dirtying pages that can't be synced.
+	 */
+	if (!ns->disk || test_and_set_bit(NVME_NS_DEAD, &ns->flags))
+		continue;
+	revalidate_disk(ns->disk);

I was under the impression we could not do this because the queues are still frozen, that's why we didnt just issue a scan_work?

revalidate_disk can potentially try and issue more I/O while the queues are frozen from passthru start.

If the controller supports effects and goes down during the passthru admin command we will deadlock during namespace revalidation.

[  363.488275] INFO: task kworker/u16:5:231 blocked for more than 120 seconds.
[  363.488290]       Not tainted 4.17.0+ #2
[  363.488296] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
[  363.488303] kworker/u16:5   D    0   231      2 0x80000000
[  363.488331] Workqueue: nvme-reset-wq nvme_reset_work [nvme]
[  363.488338] Call Trace:
[  363.488385]  schedule+0x75/0x190
[  363.488396]  rwsem_down_read_failed+0x1c3/0x2f0
[  363.488481]  call_rwsem_down_read_failed+0x14/0x30
[  363.488504]  down_read+0x1d/0x80
[  363.488523]  nvme_stop_queues+0x1e/0xa0 [nvme_core]
[  363.488536]  nvme_dev_disable+0xae4/0x1620 [nvme]
[  363.488614]  nvme_reset_work+0xd1e/0x49d9 [nvme]
[  363.488911]  process_one_work+0x81a/0x1400
[  363.488934]  worker_thread+0x87/0xe80
[  363.488955]  kthread+0x2db/0x390
[  363.488977]  ret_from_fork+0x35/0x40

Fixes 84fef62d135b6 ("nvme: check admin passthru command effects")

Signed-off-by Scott Bauer <scott.bauer at intel.com>

Quoted 22 lines · Scott Bauer

If the controller supports effects and goes down during the passthru admin command we will deadlock during namespace revalidation.

[  363.488275] INFO: task kworker/u16:5:231 blocked for more than 120 seconds.
[  363.488290]       Not tainted 4.17.0+ #2
[  363.488296] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
[  363.488303] kworker/u16:5   D    0   231      2 0x80000000
[  363.488331] Workqueue: nvme-reset-wq nvme_reset_work [nvme]
[  363.488338] Call Trace:
[  363.488385]  schedule+0x75/0x190
[  363.488396]  rwsem_down_read_failed+0x1c3/0x2f0
[  363.488481]  call_rwsem_down_read_failed+0x14/0x30
[  363.488504]  down_read+0x1d/0x80
[  363.488523]  nvme_stop_queues+0x1e/0xa0 [nvme_core]
[  363.488536]  nvme_dev_disable+0xae4/0x1620 [nvme]
[  363.488614]  nvme_reset_work+0xd1e/0x49d9 [nvme]
[  363.488911]  process_one_work+0x81a/0x1400
[  363.488934]  worker_thread+0x87/0xe80
[  363.488955]  kthread+0x2db/0x390
[  363.488977]  ret_from_fork+0x35/0x40

Quoted 3 lines

Fixes 84fef62d135b6 ("nvme: check admin passthru command effects")

Signed-off-by Scott Bauer <scott.bauer at intel.com>

Looks good to me.

Reviewed-by Keith Busch <keith.busch at linux.intel.com>

Quoted 25 lines · Keith Busch

Quoted 24 lines · Scott Bauer

If the controller supports effects and goes down during the passthru admin command we will deadlock during namespace revalidation.

[  363.488275] INFO: task kworker/u16:5:231 blocked for more than 120 seconds.
[  363.488290]       Not tainted 4.17.0+ #2
[  363.488296] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
[  363.488303] kworker/u16:5   D    0   231      2 0x80000000
[  363.488331] Workqueue: nvme-reset-wq nvme_reset_work [nvme]
[  363.488338] Call Trace:
[  363.488385]  schedule+0x75/0x190
[  363.488396]  rwsem_down_read_failed+0x1c3/0x2f0
[  363.488481]  call_rwsem_down_read_failed+0x14/0x30
[  363.488504]  down_read+0x1d/0x80
[  363.488523]  nvme_stop_queues+0x1e/0xa0 [nvme_core]
[  363.488536]  nvme_dev_disable+0xae4/0x1620 [nvme]
[  363.488614]  nvme_reset_work+0xd1e/0x49d9 [nvme]
[  363.488911]  process_one_work+0x81a/0x1400
[  363.488934]  worker_thread+0x87/0xe80
[  363.488955]  kthread+0x2db/0x390
[  363.488977]  ret_from_fork+0x35/0x40

Fixes 84fef62d135b6 ("nvme: check admin passthru command effects")

Quoted 5 lines

Quoted 1 line

Signed-off-by Scott Bauer <scott.bauer at intel.com>

Looks good to me.

Reviewed-by Keith Busch <keith.busch at linux.intel.com>

Christoph,
Could we get this one queued up for next?

Quoted 3 lines

Christoph,
Could we get this one queued up for next?

Will look at it ASAP.