Re: [PATCH 4/5] nvme: paring quiesce/unquiesce

On Wed, Sep 29, 2021 at 02:49:39PM +0300, Sagi Grimberg wrote:

On 9/29/21 7:15 AM, Ming Lei wrote:

quoted

The current blk_mq_quiesce_queue() and blk_mq_unquiesce_queue() always
stops and starts the queue unconditionally. And there can be concurrent
quiesce/unquiesce coming from different unrelated code paths, so
unquiesce may come unexpectedly and start queue too early.

Prepare for supporting nested / concurrent quiesce/unquiesce, so that we
can address the above issue.

NVMe has very complicated quiesce/unquiesce use pattern, add one mutex
and queue stopped state in nvme_ctrl, so that we can make sure that
quiece/unquiesce is called in pair.

Signed-off-by: Ming Lei <redacted>
---
  drivers/nvme/host/core.c | 51 ++++++++++++++++++++++++++++++++++++----
  drivers/nvme/host/nvme.h |  4 ++++
  2 files changed, 50 insertions(+), 5 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 23fb746a8970..5d0b2eb38e43 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c

@@ -4375,6 +4375,7 @@ int nvme_init_ctrl(struct nvme_ctrl *ctrl, struct device *dev,
  	clear_bit(NVME_CTRL_FAILFAST_EXPIRED, &ctrl->flags);
  	spin_lock_init(&ctrl->lock);
  	mutex_init(&ctrl->scan_lock);
+	mutex_init(&ctrl->queues_stop_lock);
  	INIT_LIST_HEAD(&ctrl->namespaces);
  	xa_init(&ctrl->cels);
  	init_rwsem(&ctrl->namespaces_rwsem);

@@ -4450,14 +4451,44 @@ int nvme_init_ctrl(struct nvme_ctrl *ctrl, struct device *dev,
  }
  EXPORT_SYMBOL_GPL(nvme_init_ctrl);
+static void __nvme_stop_admin_queue(struct nvme_ctrl *ctrl)
+{
+	lockdep_assert_held(&ctrl->queues_stop_lock);
+
+	if (!ctrl->admin_queue_stopped) {
+		blk_mq_quiesce_queue(ctrl->admin_q);
+		ctrl->admin_queue_stopped = true;
+	}
+}
+
+static void __nvme_start_admin_queue(struct nvme_ctrl *ctrl)
+{
+	lockdep_assert_held(&ctrl->queues_stop_lock);
+
+	if (ctrl->admin_queue_stopped) {
+		blk_mq_unquiesce_queue(ctrl->admin_q);
+		ctrl->admin_queue_stopped = false;
+	}
+}

I'd make this a bit we can flip atomically.

quoted

+
  static void nvme_start_ns_queue(struct nvme_ns *ns)
  {
-	blk_mq_unquiesce_queue(ns->queue);
+	lockdep_assert_held(&ns->ctrl->queues_stop_lock);
+
+	if (test_bit(NVME_NS_STOPPED, &ns->flags)) {
+		blk_mq_unquiesce_queue(ns->queue);
+		clear_bit(NVME_NS_STOPPED, &ns->flags);
+	}
  }
  static void nvme_stop_ns_queue(struct nvme_ns *ns)
  {
-	blk_mq_quiesce_queue(ns->queue);
+	lockdep_assert_held(&ns->ctrl->queues_stop_lock);
+
+	if (!test_bit(NVME_NS_STOPPED, &ns->flags)) {
+		blk_mq_quiesce_queue(ns->queue);
+		set_bit(NVME_NS_STOPPED, &ns->flags);
+	}
  }

Why not use test_and_set_bit/test_and_clear_bit for serialization?

quoted

  /*

@@ -4490,16 +4521,18 @@ void nvme_kill_queues(struct nvme_ctrl *ctrl)
  {
  	struct nvme_ns *ns;
+	mutex_lock(&ctrl->queues_stop_lock);
  	down_read(&ctrl->namespaces_rwsem);
  	/* Forcibly unquiesce queues to avoid blocking dispatch */
  	if (ctrl->admin_q && !blk_queue_dying(ctrl->admin_q))
-		nvme_start_admin_queue(ctrl);
+		__nvme_start_admin_queue(ctrl);
  	list_for_each_entry(ns, &ctrl->namespaces, list)
  		nvme_set_queue_dying(ns);
  	up_read(&ctrl->namespaces_rwsem);
+	mutex_unlock(&ctrl->queues_stop_lock);

This extra lock wrapping the namespaces_rwsem is scary. The
ordering rules are not clear to me.

The rule is clear: queues_stop_lock has to be acquired before locking
->namespaces_rwsem.

Using test_and_set_bit/test_and_clear_bit could be enough for pairing
quiesce and unquiesce, I will try to remove the lock of
->queues_stop_lock in next version.


Thanks,
Ming