nvme: allow queues the chance to quiesce after freezing them

PATCH New Jon Derrick 3 participants 7 messages Nov 19, 2015

Subsystems (2)

Block layer NVMe

lore

Share & tools

lore

Jon Derrick

author

2015-11-19 19:11 UTC #0 lore raw

PATCH 1/1 nvme: allow queues the chance to quiesce after freezing them

A panic was discovered while doing io and hitting the sysfs reset. Because io was completing successfully, the nvme_dev_shutdown code detected this non-idle state as a stuck state and started to tear down the queues. This resulted in a paging error when nvme_process_cq wrote the doorbell of a deleted queue.

This patch allows some time after starting the queue freeze for queues to quiesce on their own. It also sets a new nvme_queue member, frozen, to prevent writing of the cq doorbell. If the queues successfully quiesce, nvme_process_cq will run upon resuming. If the queues don't quiesce, existing code considers it a dead controller and is torn down.

Signed-off-by Jon Derrick <jonathan.derrick at intel.com>

---
 block/blk-mq.c            |  8 ++++++++
 drivers/block/nvme-core.c | 33 ++++++++++++++++++++++++++++++++-
 include/linux/blk-mq.h    |  1 +
 3 files changed, 41 insertions(+), 1 deletion(-)

block/blk-mq.c +8 -0

index 85f0143..b7fa323 100644
@@ -128,6 +128,14 @@ static void blk_mq_freeze_queue_wait(struct request_queue *q)
 	wait_event(q->mq_freeze_wq, percpu_ref_is_zero(&q->mq_usage_counter));
 }
 
+long blk_mq_freeze_queue_wait_timeout(struct request_queue *q, long timeout)
+{
+	return wait_event_timeout(q->mq_freeze_wq,
+			percpu_ref_is_zero(&q->mq_usage_counter),
+			timeout);
+}
+EXPORT_SYMBOL_GPL(blk_mq_freeze_queue_wait_timeout);
+
 /*
  * Guarantee no request is in use, so we can change any data structure of
  * the queue afterward.

drivers/block/nvme-core.c +32 -1

index ccc0c1f..183a868 100644
@@ -122,6 +122,7 @@ struct nvme_queue {
 	u8 cq_phase;
 	u8 cqe_seen;
 	struct async_cmd_info cmdinfo;
+	bool frozen;
 };
 
 /*

@@ -977,7 +978,8 @@ static int nvme_process_cq(struct nvme_queue *nvmeq)
 	if (head == nvmeq->cq_head && phase == nvmeq->cq_phase)
 		return 0;
 
-	writel(head, nvmeq->q_db + nvmeq->dev->db_stride);
+	if (unlikely(!nvmeq->frozen))
+		writel(head, nvmeq->q_db + nvmeq->dev->db_stride);
 	nvmeq->cq_head = head;
 	nvmeq->cq_phase = phase;

@@ -2774,6 +2776,8 @@ static void nvme_dev_list_remove(struct nvme_dev *dev)
 static void nvme_freeze_queues(struct nvme_dev *dev)
 {
 	struct nvme_ns *ns;
+	unsigned long start, timeout;
+	int i;
 
 	list_for_each_entry(ns, &dev->namespaces, list) {
 		blk_mq_freeze_queue_start(ns->queue);

@@ -2785,11 +2789,28 @@ static void nvme_freeze_queues(struct nvme_dev *dev)
 		blk_mq_cancel_requeue_work(ns->queue);
 		blk_mq_stop_hw_queues(ns->queue);
 	}
+
+	for (i = 1; i < dev->queue_count; i++) {
+		struct nvme_queue *nvmeq = dev->queues[i];
+		if (!nvmeq)
+			continue;
+		nvmeq->frozen = true;
+	}
+
+	start = jiffies;
+	list_for_each_entry(ns, &dev->namespaces, list) {
+		timeout = ns->queue->tag_set->timeout;
+		if (time_after_eq(jiffies, start + timeout))
+			timeout = 0;
+		blk_mq_freeze_queue_wait_timeout(ns->queue, timeout);
+	}
 }
 
 static void nvme_unfreeze_queues(struct nvme_dev *dev)
 {
 	struct nvme_ns *ns;
+	unsigned long flags;
+	int i;
 
 	list_for_each_entry(ns, &dev->namespaces, list) {
 		queue_flag_clear_unlocked(QUEUE_FLAG_STOPPED, ns->queue);

@@ -2797,6 +2818,16 @@ static void nvme_unfreeze_queues(struct nvme_dev *dev)
 		blk_mq_start_stopped_hw_queues(ns->queue, true);
 		blk_mq_kick_requeue_list(ns->queue);
 	}
+
+	for (i = 1; i < dev->queue_count; i++) {
+		struct nvme_queue *nvmeq = dev->queues[i];
+		if (!nvmeq)
+			continue;
+		nvmeq->frozen = false;
+		spin_lock_irqsave(&nvmeq->q_lock, flags);
+		nvme_process_cq(nvmeq);
+		spin_unlock_irqrestore(&nvmeq->q_lock, flags);
+	}
 }
 
 static void nvme_dev_shutdown(struct nvme_dev *dev)

include/linux/blk-mq.h +1 -0

index 5e7d43a..3741e59 100644
@@ -228,6 +228,7 @@ void blk_mq_all_tag_busy_iter(struct blk_mq_tags *tags, busy_tag_iter_fn *fn,
 void blk_mq_freeze_queue(struct request_queue *q);
 void blk_mq_unfreeze_queue(struct request_queue *q);
 void blk_mq_freeze_queue_start(struct request_queue *q);
+long blk_mq_freeze_queue_wait_timeout(struct request_queue *q, long timeout);
 
 /*
  * Driver command data is immediately after the request. So subtract request

Signature · 1 line

2.5.0

Jon Derrick

author

2015-11-19 20:53 UTC #1 lore raw

PATCH 1/1 nvme: allow queues the chance to quiesce after freezing them

drivers/block/nvme-core.c static int nvme_process_cq(struct nvme_queue *nvmeq) · line 982

Quoted 3 lines

drivers/block/nvme-core.c +2 -1

-	writel(head, nvmeq->q_db + nvmeq->dev->db_stride);
+	if (unlikely(!nvmeq->frozen))
+		writel(head, nvmeq->q_db + nvmeq->dev->db_stride);

Oops... This last minute addition should have been a likely().

Keith Busch

2015-11-19 21:41 UTC #2 lore raw

PATCH 1/1 nvme: allow queues the chance to quiesce after freezing them

In reply to Jon Derrick

Quoted 11 lines · Jon Derrick

A panic was discovered while doing io and hitting the sysfs reset. Because io was completing successfully, the nvme_dev_shutdown code detected this non-idle state as a stuck state and started to tear down the queues. This resulted in a paging error when nvme_process_cq wrote the doorbell of a deleted queue.
This patch allows some time after starting the queue freeze for queues to quiesce on their own. It also sets a new nvme_queue member, frozen, to prevent writing of the cq doorbell. If the queues successfully quiesce, nvme_process_cq will run upon resuming. If the queues don't quiesce, existing code considers it a dead controller and is torn down.

I think all we really want is skip notifying completions on a "suspended" queue. We can tell by the value of the cq-vector, and it's already lock protected.

It also sounds like we need to poll the cq after the delete completes to catch successful completions before we force cancel the rest.

This appears to work for me. Does it pass your test?
---

drivers/nvme/host/pci.c +6 -1

index 394fd16..930042fa 100644
@@ -968,7 +968,8 @@ static void __nvme_process_cq(struct nvme_queue *nvmeq, unsigned int *tag)
 	if (head == nvmeq->cq_head && phase == nvmeq->cq_phase)
 		return;
 
-	writel(head, nvmeq->q_db + nvmeq->dev->db_stride);
+	if (likely(nvmeq->cq_vector >= 0))
+		writel(head, nvmeq->q_db + nvmeq->dev->db_stride);
 	nvmeq->cq_head = head;
 	nvmeq->cq_phase = phase;

@@ -2787,6 +2788,10 @@ static void nvme_del_queue_end(struct nvme_queue *nvmeq)
 {
 	struct nvme_delq_ctx *dq = nvmeq->cmdinfo.ctx;
 	nvme_put_dq(dq);
+
+	spin_lock_irq(&nvmeq->q_lock);
+	nvme_process_cq(nvmeq);
+	spin_unlock_irq(&nvmeq->q_lock);
 }
 
 static int adapter_async_del_queue(struct nvme_queue *nvmeq, u8 opcode,

Christoph Hellwig

2015-11-19 21:53 UTC #3 lore raw

PATCH 1/1 nvme: allow queues the chance to quiesce after freezing them

Quoted 13 lines · Keith Busch

Quoted 11 lines · Jon Derrick
A panic was discovered while doing io and hitting the sysfs reset. Because io was completing successfully, the nvme_dev_shutdown code detected this non-idle state as a stuck state and started to tear down the queues. This resulted in a paging error when nvme_process_cq wrote the doorbell of a deleted queue.
This patch allows some time after starting the queue freeze for queues to quiesce on their own. It also sets a new nvme_queue member, frozen, to prevent writing of the cq doorbell. If the queues successfully quiesce, nvme_process_cq will run upon resuming. If the queues don't quiesce, existing code considers it a dead controller and is torn down.

Quoted 8 lines

I think all we really want is skip notifying completions on a "suspended" queue. We can tell by the value of the cq-vector, and it's already lock protected.
It also sounds like we need to poll the cq after the delete completes to catch successful completions before we force cancel the rest.
This appears to work for me. Does it pass your test?

This looks reasonable. I ran into stray ->q_db derference a lot during reset testing, but after my abort and reset rewrites ([1] for th latest version) I couldn't reproduce it any more.

[1]
http://git.infradead.org/users/hch/block.git/shortlog/refs/heads/nvme-req.8

Keith Busch

2015-11-19 22:20 UTC #4 lore raw

PATCH 1/1 nvme: allow queues the chance to quiesce after freezing them

Quoted 6 lines · Christoph Hellwig

This looks reasonable. I ran into stray ->q_db derference a lot during reset testing, but after my abort and reset rewrites ([1] for th latest version) I couldn't reproduce it any more.
[1]
http://git.infradead.org/users/hch/block.git/shortlog/refs/heads/nvme-req.8

There's good stuff in this tree. I've been using it most of this week to study the reset handling and still have a concern here.

I mentioned ending IO in the reset handler in a previous review, and I can create memory and data corruption with well timed delays (or badly timed, depending on your perspective). We can't end IO while the controller owns it. Even though the timeout handler often means the controller is not going to return the command anyway, ending it releases memory the controller may still access.

I don't see how we can disable the controller to forcefully end commands with merged probe and reset work, and thinking these need to be kept separate. Timeouts are done in a workqueue now, so can we do controller shutdown inline with the timeout handler?

Christoph Hellwig

2015-11-19 22:24 UTC #5 lore raw

PATCH 1/1 nvme: allow queues the chance to quiesce after freezing them

Quoted 4 lines · Keith Busch

I don't see how we can disable the controller to forcefully end commands with merged probe and reset work, and thinking these need to be kept separate. Timeouts are done in a workqueue now, so can we do controller shutdown inline with the timeout handler?

That's one of the two options I'm exploring. The other is a new request flag that tells the block later not to reuse the tag after a return from the timeout handler.

Jon Derrick

author

2015-11-19 22:12 UTC #6 lore raw

PATCH 1/1 nvme: allow queues the chance to quiesce after freezing them

In reply to Keith Busch

Quoted 4 lines

I think all we really want is skip notifying completions on a "suspended" queue. We can tell by the value of the cq-vector, and it's already lock protected.

Good catch. I wish I had made that relationship :)
(Though I was hoping somebody would figure out a less hacky way of doing it than I did)

Quoted 4 lines

It also sounds like we need to poll the cq after the delete completes to catch successful completions before we force cancel the rest.
This appears to work for me. Does it pass your test?

Passes my tests as well so you can add my tb:
Tested-by: Jon Derrick <jonathan.derrick at intel.com>

Re: Christoph

Quoted 1 line

This looks reasonable. I ran into stray ->q_db derference a lot during reset testing, but after my abort and reset rewrites ([1] for th latest

version) I couldn't reproduce it any more.

Sorry this was against 4.3 (and was reproducible on 4.4-rc1). I will give your repo a test shortly