[PATCH 0/3] Introduce a light-weight queue close feature

[PATCH 0/3] Introduce a light-weight queue close feature

[Jianchao Wang]

Dear all

As we know, queue freeze is used to stop new IO comming in and drain
the request queue. And the draining queue here is necessary, because
queue freeze kills the percpu-ref q_usage_counter and need to drain
the q_usage_counter before switch it back to percpu mode. This could
be a trouble when we just want to prevent new IO.

In nvme-pci, nvme_dev_disable freezes queues to prevent new IO.
nvme_reset_work will unfreeze and wait to drain the queues. However,
if IO timeout at the moment, no body could do recovery as nvme_reset_work
is waiting. We will encounter IO hang.

So introduce a light-weight queue close feature in this patch set
which could prevent new IO and needn't drain the queue.

The 1st patch introduces a queue_gate into request queue and migrate
preempt only from queue flags on it.

The 2nd patch introduces queue close feature.

The 3rd patch apply the queue close in nvme-pci to avoid the IO hang
issue above.

Jianchao Wang (3)
blk-core: migrate preempt-only mode to queue_gate
blk-core: introduce queue close feature
nvme-pci: use queue close instead of queue freeze

 block/blk-core.c         | 82 +++++++++++++++++++++++++++++++++---------------
 block/blk-mq-debugfs.c   |  1 -
 block/blk.h              |  5 +++
 drivers/nvme/host/core.c | 22 +++++++++++++
 drivers/nvme/host/nvme.h |  3 ++
 drivers/nvme/host/pci.c  | 27 ++++++++--------
 drivers/scsi/scsi_lib.c  | 10 ------
 include/linux/blkdev.h   |  7 +++--
 8 files changed, 104 insertions(+), 53 deletions(-)


Thanks
Jianchao

[PATCH 1/3] blk-core: migrate preempt-only mode to queue_gate

[Jianchao Wang]

This patch introduce queue_gate into request_queue which is
dedicated to entering conditions control in blk_queue_enter.
Helper blk_queue_gate_allow is in charge of checking entering
conditions. If not allowed, go to wait on wq_freeze_wq. This is
a preparation for the next light-weight queue close feature.
And also the preempt-only mode is migrated from the queue_flags
to queue_gate in this patch.

Signed-off-by: Jianchao Wang <jianchao.w.wang@oracle.com>
---
 block/blk-core.c        | 65 +++++++++++++++++++++++++++++--------------------
 block/blk-mq-debugfs.c  |  1 -
 block/blk.h             |  4 +++
 drivers/scsi/scsi_lib.c | 10 --------
 include/linux/blkdev.h  |  4 +--
 5 files changed, 44 insertions(+), 40 deletions(-)

diff --git a/block/blk-core.c b/block/blk-core.c
index dee56c2..d1bdded 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -420,22 +420,31 @@ void blk_sync_queue(struct request_queue *q)
 }
 EXPORT_SYMBOL(blk_sync_queue);
 
-/**
- * blk_set_preempt_only - set QUEUE_FLAG_PREEMPT_ONLY
- * @q: request queue pointer
- *
- * Returns the previous value of the PREEMPT_ONLY flag - 0 if the flag was not
- * set and 1 if the flag was already set.
+/*
+ * When blk_set_preempt_only returns:
+ *  - only preempt bio could enter the queue
+ *  - there is no non-preempt bios in the queue
  */
 int blk_set_preempt_only(struct request_queue *q)
 {
-	return blk_queue_flag_test_and_set(QUEUE_FLAG_PREEMPT_ONLY, q);
+	if (test_and_set_bit(BLK_QUEUE_GATE_PREEMPT_ONLY, &q->queue_gate))
+		return 1;
+
+	synchronize_rcu();
+	/*
+	 * After this, the non-preempt bios either get q_usage_counter
+	 * and enter, or go to wait.
+	 * Next, let's drain the entered ones.
+	 */
+	blk_mq_freeze_queue(q);
+	blk_mq_unfreeze_queue(q);
+	return 0;
 }
 EXPORT_SYMBOL_GPL(blk_set_preempt_only);
 
 void blk_clear_preempt_only(struct request_queue *q)
 {
-	blk_queue_flag_clear(QUEUE_FLAG_PREEMPT_ONLY, q);
+	clear_bit(BLK_QUEUE_GATE_PREEMPT_ONLY, &q->queue_gate);
 	wake_up_all(&q->mq_freeze_wq);
 }
 EXPORT_SYMBOL_GPL(blk_clear_preempt_only);
@@ -910,6 +919,19 @@ struct request_queue *blk_alloc_queue(gfp_t gfp_mask)
 }
 EXPORT_SYMBOL(blk_alloc_queue);
 
+static inline bool blk_queue_gate_allow(struct request_queue *q,
+		blk_mq_req_flags_t flags)
+{
+	if (!q->queue_gate)
+		return true;
+
+	if (test_bit(BLK_QUEUE_GATE_PREEMPT_ONLY, &q->queue_gate) &&
+		!(flags & BLK_MQ_REQ_PREEMPT))
+		return false;
+
+	return true;
+}
+
 /**
  * blk_queue_enter() - try to increase q->q_usage_counter
  * @q: request queue pointer
@@ -917,29 +939,20 @@ EXPORT_SYMBOL(blk_alloc_queue);
  */
 int blk_queue_enter(struct request_queue *q, blk_mq_req_flags_t flags)
 {
-	const bool preempt = flags & BLK_MQ_REQ_PREEMPT;
-
 	while (true) {
-		bool success = false;
 
 		rcu_read_lock();
-		if (percpu_ref_tryget_live(&q->q_usage_counter)) {
-			/*
-			 * The code that sets the PREEMPT_ONLY flag is
-			 * responsible for ensuring that that flag is globally
-			 * visible before the queue is unfrozen.
-			 */
-			if (preempt || !blk_queue_preempt_only(q)) {
-				success = true;
-			} else {
-				percpu_ref_put(&q->q_usage_counter);
-			}
+		if (unlikely(READ_ONCE(q->queue_gate))) {
+			if (!blk_queue_gate_allow(q, flags))
+				goto wait;
 		}
-		rcu_read_unlock();
 
-		if (success)
+		if (percpu_ref_tryget_live(&q->q_usage_counter)) {
+			rcu_read_unlock();
 			return 0;
-
+		}
+wait:
+		rcu_read_unlock();
 		if (flags & BLK_MQ_REQ_NOWAIT)
 			return -EBUSY;
 
@@ -954,7 +967,7 @@ int blk_queue_enter(struct request_queue *q, blk_mq_req_flags_t flags)
 
 		wait_event(q->mq_freeze_wq,
 			   (atomic_read(&q->mq_freeze_depth) == 0 &&
-			    (preempt || !blk_queue_preempt_only(q))) ||
+			    blk_queue_gate_allow(q, flags)) ||
 			   blk_queue_dying(q));
 		if (blk_queue_dying(q))
 			return -ENODEV;
diff --git a/block/blk-mq-debugfs.c b/block/blk-mq-debugfs.c
index cb1e6cf..4174951 100644
--- a/block/blk-mq-debugfs.c
+++ b/block/blk-mq-debugfs.c
@@ -132,7 +132,6 @@ static const char *const blk_queue_flag_name[] = {
 	QUEUE_FLAG_NAME(REGISTERED),
 	QUEUE_FLAG_NAME(SCSI_PASSTHROUGH),
 	QUEUE_FLAG_NAME(QUIESCED),
-	QUEUE_FLAG_NAME(PREEMPT_ONLY),
 };
 #undef QUEUE_FLAG_NAME
 
diff --git a/block/blk.h b/block/blk.h
index 9db4e38..cdef4c1 100644
--- a/block/blk.h
+++ b/block/blk.h
@@ -19,6 +19,10 @@
 extern struct dentry *blk_debugfs_root;
 #endif
 
+enum blk_queue_gate_flag_t {
+	BLK_QUEUE_GATE_PREEMPT_ONLY,
+};
+
 struct blk_flush_queue {
 	unsigned int		flush_queue_delayed:1;
 	unsigned int		flush_pending_idx:1;
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 0adfb3b..491d8bf 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -3059,16 +3059,6 @@ scsi_device_quiesce(struct scsi_device *sdev)
 
 	blk_set_preempt_only(q);
 
-	blk_mq_freeze_queue(q);
-	/*
-	 * Ensure that the effect of blk_set_preempt_only() will be visible
-	 * for percpu_ref_tryget() callers that occur after the queue
-	 * unfreeze even if the queue was already frozen before this function
-	 * was called. See also https://lwn.net/Articles/573497/.
-	 */
-	synchronize_rcu();
-	blk_mq_unfreeze_queue(q);
-
 	mutex_lock(&sdev->state_mutex);
 	err = scsi_device_set_state(sdev, SDEV_QUIESCE);
 	if (err == 0)
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index d6869e0..4a33814 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -504,6 +504,7 @@ struct request_queue {
 	 * various queue flags, see QUEUE_* below
 	 */
 	unsigned long		queue_flags;
+	unsigned long		queue_gate;
 
 	/*
 	 * ida allocated id for this queue.  Used to index queues from
@@ -698,7 +699,6 @@ struct request_queue {
 #define QUEUE_FLAG_REGISTERED  26	/* queue has been registered to a disk */
 #define QUEUE_FLAG_SCSI_PASSTHROUGH 27	/* queue supports SCSI commands */
 #define QUEUE_FLAG_QUIESCED    28	/* queue has been quiesced */
-#define QUEUE_FLAG_PREEMPT_ONLY	29	/* only process REQ_PREEMPT requests */
 
 #define QUEUE_FLAG_DEFAULT	((1 << QUEUE_FLAG_IO_STAT) |		\
 				 (1 << QUEUE_FLAG_SAME_COMP)	|	\
@@ -736,8 +736,6 @@ bool blk_queue_flag_test_and_clear(unsigned int flag, struct request_queue *q);
 	((rq)->cmd_flags & (REQ_FAILFAST_DEV|REQ_FAILFAST_TRANSPORT| \
 			     REQ_FAILFAST_DRIVER))
 #define blk_queue_quiesced(q)	test_bit(QUEUE_FLAG_QUIESCED, &(q)->queue_flags)
-#define blk_queue_preempt_only(q)				\
-	test_bit(QUEUE_FLAG_PREEMPT_ONLY, &(q)->queue_flags)
 #define blk_queue_fua(q)	test_bit(QUEUE_FLAG_FUA, &(q)->queue_flags)
 
 extern int blk_set_preempt_only(struct request_queue *q);
-- 
2.7.4

[PATCH 2/3] blk-core: introduce queue close feature

[Jianchao Wang]

blk queue freeze is often used to prevent new IO from entering
request queue. However, becuase we kill the percpu-ref
q_usage_counter when freeze queue, we have to drain the request
queue when unfreeze. This is unnecessary for just preventing new
IO. In addition, If there is IO timeout or other issue when unfreeze
the queue, the scenario could be very tricky.

So we introduce BLK_QUEUE_GATE_CLOSED to implement a light-weight
queue close feature base on the queue_gate to prevent new IO from
comming in queue which will not need to drain the queue any more.

Signed-off-by: Jianchao Wang <jianchao.w.wang@oracle.com>
---
 block/blk-core.c       | 17 +++++++++++++++++
 block/blk.h            |  1 +
 include/linux/blkdev.h |  3 +++
 3 files changed, 21 insertions(+)

diff --git a/block/blk-core.c b/block/blk-core.c
index d1bdded..b073c68 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -449,6 +449,23 @@ void blk_clear_preempt_only(struct request_queue *q)
 }
 EXPORT_SYMBOL_GPL(blk_clear_preempt_only);
 
+int blk_set_queue_closed(struct request_queue *q)
+{
+	if (test_and_set_bit(BLK_QUEUE_GATE_CLOSED, &q->queue_gate))
+		return 1;
+
+	synchronize_rcu();
+	return 0;
+}
+EXPORT_SYMBOL_GPL(blk_set_queue_closed);
+
+void blk_clear_queue_closed(struct request_queue *q)
+{
+	clear_bit(BLK_QUEUE_GATE_CLOSED, &q->queue_gate);
+	wake_up_all(&q->mq_freeze_wq);
+}
+EXPORT_SYMBOL_GPL(blk_clear_queue_closed);
+
 /**
  * __blk_run_queue_uncond - run a queue whether or not it has been stopped
  * @q:	The queue to run
diff --git a/block/blk.h b/block/blk.h
index cdef4c1..90ff6bb 100644
--- a/block/blk.h
+++ b/block/blk.h
@@ -21,6 +21,7 @@ extern struct dentry *blk_debugfs_root;
 
 enum blk_queue_gate_flag_t {
 	BLK_QUEUE_GATE_PREEMPT_ONLY,
+	BLK_QUEUE_GATE_CLOSED,
 };
 
 struct blk_flush_queue {
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 4a33814..a7f77da 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -741,6 +741,9 @@ bool blk_queue_flag_test_and_clear(unsigned int flag, struct request_queue *q);
 extern int blk_set_preempt_only(struct request_queue *q);
 extern void blk_clear_preempt_only(struct request_queue *q);
 
+extern int blk_set_queue_closed(struct request_queue *q);
+extern void blk_clear_queue_closed(struct request_queue *q);
+
 static inline int queue_in_flight(struct request_queue *q)
 {
 	return q->in_flight[0] + q->in_flight[1];
-- 
2.7.4

[PATCH 3/3] nvme-pci: use queue close instead of queue freeze

[Jianchao Wang]

nvme_dev_disable freezes queues to prevent new IO. nvme_reset_work
will unfreeze and wait to drain the queues. However, if IO timeout
at the moment, no body could do recovery as nvme_reset_work is
waiting. We will encounter IO hang.

To avoid this scenario, use queue close to prevent new IO which
doesn't need to drain the queues. And just use queue freeze to
try to wait for in-flight IO for shutdown case.

Signed-off-by: Jianchao Wang <jianchao.w.wang@oracle.com>
---
 drivers/nvme/host/core.c | 22 ++++++++++++++++++++++
 drivers/nvme/host/nvme.h |  3 +++
 drivers/nvme/host/pci.c  | 27 ++++++++++++++-------------
 3 files changed, 39 insertions(+), 13 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index dd8ec1d..ce5b35b 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -3602,6 +3602,28 @@ void nvme_kill_queues(struct nvme_ctrl *ctrl)
 }
 EXPORT_SYMBOL_GPL(nvme_kill_queues);
 
+void nvme_close_queues(struct nvme_ctrl *ctrl)
+{
+	struct nvme_ns *ns;
+
+	down_read(&ctrl->namespaces_rwsem);
+	list_for_each_entry(ns, &ctrl->namespaces, list)
+		blk_set_queue_closed(ns->queue);
+	up_read(&ctrl->namespaces_rwsem);
+}
+EXPORT_SYMBOL_GPL(nvme_close_queues);
+
+void nvme_open_queues(struct nvme_ctrl *ctrl)
+{
+	struct nvme_ns *ns;
+
+	down_read(&ctrl->namespaces_rwsem);
+	list_for_each_entry(ns, &ctrl->namespaces, list)
+		blk_clear_queue_closed(ns->queue);
+	up_read(&ctrl->namespaces_rwsem);
+}
+EXPORT_SYMBOL_GPL(nvme_open_queues);
+
 void nvme_unfreeze(struct nvme_ctrl *ctrl)
 {
 	struct nvme_ns *ns;
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index bb4a200..fcd44cb 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -437,6 +437,9 @@ void nvme_wait_freeze(struct nvme_ctrl *ctrl);
 void nvme_wait_freeze_timeout(struct nvme_ctrl *ctrl, long timeout);
 void nvme_start_freeze(struct nvme_ctrl *ctrl);
 
+void nvme_close_queues(struct nvme_ctrl *ctrl);
+void nvme_open_queues(struct nvme_ctrl *ctrl);
+
 #define NVME_QID_ANY -1
 struct request *nvme_alloc_request(struct request_queue *q,
 		struct nvme_command *cmd, blk_mq_req_flags_t flags, int qid);
diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index d668682..c0ccd04 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -2145,23 +2145,25 @@ static void nvme_dev_disable(struct nvme_dev *dev, bool shutdown)
 	struct pci_dev *pdev = to_pci_dev(dev->dev);
 
 	mutex_lock(&dev->shutdown_lock);
+	nvme_close_queues(&dev->ctrl);
 	if (pci_is_enabled(pdev)) {
 		u32 csts = readl(dev->bar + NVME_REG_CSTS);
 
-		if (dev->ctrl.state == NVME_CTRL_LIVE ||
-		    dev->ctrl.state == NVME_CTRL_RESETTING)
-			nvme_start_freeze(&dev->ctrl);
 		dead = !!((csts & NVME_CSTS_CFS) || !(csts & NVME_CSTS_RDY) ||
 			pdev->error_state  != pci_channel_io_normal);
-	}
 
-	/*
-	 * Give the controller a chance to complete all entered requests if
-	 * doing a safe shutdown.
-	 */
-	if (!dead) {
-		if (shutdown)
-			nvme_wait_freeze_timeout(&dev->ctrl, NVME_IO_TIMEOUT);
+		if (dev->ctrl.state == NVME_CTRL_LIVE ||
+		    dev->ctrl.state == NVME_CTRL_RESETTING) {
+			/*
+			 * Give the controller a chance to complete all entered
+			 * requests if doing a safe shutdown.
+			 */
+			if (!dead && shutdown) {
+				nvme_start_freeze(&dev->ctrl);
+				nvme_wait_freeze_timeout(&dev->ctrl, NVME_IO_TIMEOUT);
+				nvme_unfreeze(&dev->ctrl);
+			}
+		}
 	}
 
 	nvme_stop_queues(&dev->ctrl);
@@ -2328,11 +2330,9 @@ static void nvme_reset_work(struct work_struct *work)
 		new_state = NVME_CTRL_ADMIN_ONLY;
 	} else {
 		nvme_start_queues(&dev->ctrl);
-		nvme_wait_freeze(&dev->ctrl);
 		/* hit this only when allocate tagset fails */
 		if (nvme_dev_add(dev))
 			new_state = NVME_CTRL_ADMIN_ONLY;
-		nvme_unfreeze(&dev->ctrl);
 	}
 
 	/*
@@ -2345,6 +2345,7 @@ static void nvme_reset_work(struct work_struct *work)
 		goto out;
 	}
 
+	nvme_open_queues(&dev->ctrl);
 	nvme_start_ctrl(&dev->ctrl);
 	return;
 
-- 
2.7.4

Re: [PATCH 0/3] Introduce a light-weight queue close feature

[Bart Van Assche]

On Wed, 2018-09-05 at 12:09 +0800, Jianchao Wang wrote:
> As we know, queue freeze is used to stop new IO comming in and drain
> the request queue. And the draining queue here is necessary, because
> queue freeze kills the percpu-ref q_usage_counter and need to drain
> the q_usage_counter before switch it back to percpu mode. This could
> be a trouble when we just want to prevent new IO.
> 
> In nvme-pci, nvme_dev_disable freezes queues to prevent new IO.
> nvme_reset_work will unfreeze and wait to drain the queues. However,
> if IO timeout at the moment, no body could do recovery as nvme_reset_work
> is waiting. We will encounter IO hang.
> 
> So introduce a light-weight queue close feature in this patch set
> which could prevent new IO and needn't drain the queue.
> 
> The 1st patch introduces a queue_gate into request queue and migrate
> preempt only from queue flags on it.
> 
> The 2nd patch introduces queue close feature.
> 
> The 3rd patch apply the queue close in nvme-pci to avoid the IO hang
> issue above.

Hello Jianchao,

Is this patch series based on a theoretical concern or rather on something
you ran into? In the latter case, can you explain which scenario makes it
likely on your setup to encounter an NVMe timeout?

Thanks,

Bart.

Re: [PATCH 2/3] blk-core: introduce queue close feature

[Bart Van Assche]

On Wed, 2018-09-05 at 12:09 +0800, Jianchao Wang wrote:
> blk queue freeze is often used to prevent new IO from entering
> request queue. However, becuase we kill the percpu-ref
> q_usage_counter when freeze queue, we have to drain the request
> queue when unfreeze. This is unnecessary for just preventing new
> IO. In addition, If there is IO timeout or other issue when unfreeze
> the queue, the scenario could be very tricky.
> 
> So we introduce BLK_QUEUE_GATE_CLOSED to implement a light-weight
> queue close feature base on the queue_gate to prevent new IO from
> comming in queue which will not need to drain the queue any more.

Does the "queue gate close" feature cause blk_get_request() /
blk_mq_get_request() to block until blk_clear_queue_closed() is called? If
so, I think we need a better name for this feature. How about calling these
two operations suspend and resume?

Thanks,

Bart.

Re: [PATCH 0/3] Introduce a light-weight queue close feature

[Ming Lei]

On Wed, Sep 05, 2018 at 12:09:43PM +0800, Jianchao Wang wrote:
> Dear all
> 
> As we know, queue freeze is used to stop new IO comming in and drain
> the request queue. And the draining queue here is necessary, because
> queue freeze kills the percpu-ref q_usage_counter and need to drain
> the q_usage_counter before switch it back to percpu mode. This could
> be a trouble when we just want to prevent new IO.
> 
> In nvme-pci, nvme_dev_disable freezes queues to prevent new IO.
> nvme_reset_work will unfreeze and wait to drain the queues. However,
> if IO timeout at the moment, no body could do recovery as nvme_reset_work
> is waiting. We will encounter IO hang.

As we discussed this nvme time issue before, I have pointed out that
this is because of blk_mq_unfreeze_queue()'s limit which requires that
unfreeze can only be done when this queue ref counter drops to zero.

For this nvme timeout case, we may relax the limit, for example,
introducing another API of blk_freeze_queue_stop() as counter-pair of
blk_freeze_queue_start(), and simply switch the percpu-ref to percpu mode
from atomic mode inside the new API.

> 
> So introduce a light-weight queue close feature in this patch set
> which could prevent new IO and needn't drain the queue.

Frankly speaking, IMO, it may not be an good idea to mess up the fast path
just for handling the extremely unusual timeout event. The same is true
for doing the preemp only stuff, as you saw I have posted patchset for
killing it.

Thanks,
Ming

Re: [PATCH 3/3] nvme-pci: use queue close instead of queue freeze

[Ming Lei]

On Wed, Sep 05, 2018 at 12:09:46PM +0800, Jianchao Wang wrote:
> nvme_dev_disable freezes queues to prevent new IO. nvme_reset_work
> will unfreeze and wait to drain the queues. However, if IO timeout
> at the moment, no body could do recovery as nvme_reset_work is
> waiting. We will encounter IO hang.
> 
> To avoid this scenario, use queue close to prevent new IO which
> doesn't need to drain the queues. And just use queue freeze to
> try to wait for in-flight IO for shutdown case.
> 
> Signed-off-by: Jianchao Wang <jianchao.w.wang@oracle.com>
> ---
>  drivers/nvme/host/core.c | 22 ++++++++++++++++++++++
>  drivers/nvme/host/nvme.h |  3 +++
>  drivers/nvme/host/pci.c  | 27 ++++++++++++++-------------
>  3 files changed, 39 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index dd8ec1d..ce5b35b 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -3602,6 +3602,28 @@ void nvme_kill_queues(struct nvme_ctrl *ctrl)
>  }
>  EXPORT_SYMBOL_GPL(nvme_kill_queues);
>  
> +void nvme_close_queues(struct nvme_ctrl *ctrl)
> +{
> +	struct nvme_ns *ns;
> +
> +	down_read(&ctrl->namespaces_rwsem);
> +	list_for_each_entry(ns, &ctrl->namespaces, list)
> +		blk_set_queue_closed(ns->queue);
> +	up_read(&ctrl->namespaces_rwsem);
> +}
> +EXPORT_SYMBOL_GPL(nvme_close_queues);
> +
> +void nvme_open_queues(struct nvme_ctrl *ctrl)
> +{
> +	struct nvme_ns *ns;
> +
> +	down_read(&ctrl->namespaces_rwsem);
> +	list_for_each_entry(ns, &ctrl->namespaces, list)
> +		blk_clear_queue_closed(ns->queue);
> +	up_read(&ctrl->namespaces_rwsem);
> +}
> +EXPORT_SYMBOL_GPL(nvme_open_queues);
> +
>  void nvme_unfreeze(struct nvme_ctrl *ctrl)
>  {
>  	struct nvme_ns *ns;
> diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
> index bb4a200..fcd44cb 100644
> --- a/drivers/nvme/host/nvme.h
> +++ b/drivers/nvme/host/nvme.h
> @@ -437,6 +437,9 @@ void nvme_wait_freeze(struct nvme_ctrl *ctrl);
>  void nvme_wait_freeze_timeout(struct nvme_ctrl *ctrl, long timeout);
>  void nvme_start_freeze(struct nvme_ctrl *ctrl);
>  
> +void nvme_close_queues(struct nvme_ctrl *ctrl);
> +void nvme_open_queues(struct nvme_ctrl *ctrl);
> +
>  #define NVME_QID_ANY -1
>  struct request *nvme_alloc_request(struct request_queue *q,
>  		struct nvme_command *cmd, blk_mq_req_flags_t flags, int qid);
> diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
> index d668682..c0ccd04 100644
> --- a/drivers/nvme/host/pci.c
> +++ b/drivers/nvme/host/pci.c
> @@ -2145,23 +2145,25 @@ static void nvme_dev_disable(struct nvme_dev *dev, bool shutdown)
>  	struct pci_dev *pdev = to_pci_dev(dev->dev);
>  
>  	mutex_lock(&dev->shutdown_lock);
> +	nvme_close_queues(&dev->ctrl);
>  	if (pci_is_enabled(pdev)) {
>  		u32 csts = readl(dev->bar + NVME_REG_CSTS);
>  
> -		if (dev->ctrl.state == NVME_CTRL_LIVE ||
> -		    dev->ctrl.state == NVME_CTRL_RESETTING)
> -			nvme_start_freeze(&dev->ctrl);
>  		dead = !!((csts & NVME_CSTS_CFS) || !(csts & NVME_CSTS_RDY) ||
>  			pdev->error_state  != pci_channel_io_normal);
> -	}
>  
> -	/*
> -	 * Give the controller a chance to complete all entered requests if
> -	 * doing a safe shutdown.
> -	 */
> -	if (!dead) {
> -		if (shutdown)
> -			nvme_wait_freeze_timeout(&dev->ctrl, NVME_IO_TIMEOUT);
> +		if (dev->ctrl.state == NVME_CTRL_LIVE ||
> +		    dev->ctrl.state == NVME_CTRL_RESETTING) {
> +			/*
> +			 * Give the controller a chance to complete all entered
> +			 * requests if doing a safe shutdown.
> +			 */
> +			if (!dead && shutdown) {
> +				nvme_start_freeze(&dev->ctrl);
> +				nvme_wait_freeze_timeout(&dev->ctrl, NVME_IO_TIMEOUT);
> +				nvme_unfreeze(&dev->ctrl);
> +			}
> +		}
>  	}
>  
>  	nvme_stop_queues(&dev->ctrl);
> @@ -2328,11 +2330,9 @@ static void nvme_reset_work(struct work_struct *work)
>  		new_state = NVME_CTRL_ADMIN_ONLY;
>  	} else {
>  		nvme_start_queues(&dev->ctrl);
> -		nvme_wait_freeze(&dev->ctrl);
>  		/* hit this only when allocate tagset fails */
>  		if (nvme_dev_add(dev))
>  			new_state = NVME_CTRL_ADMIN_ONLY;
> -		nvme_unfreeze(&dev->ctrl);

nvme_dev_add() still may call freeze & unfreeze queue, so your patch
can't avoid draining queue completely here.

Thanks,
Ming

Re: [PATCH 3/3] nvme-pci: use queue close instead of queue freeze

[jianchao.wang]

Hi Ming

On 09/06/2018 06:09 AM, Ming Lei wrote:
> nvme_dev_add() still may call freeze & unfreeze queue, so your patch
> can't avoid draining queue completely here.

Yes, I know this. We still need to freeze queue when update nr_hw_queues.
But we move forward a step at least. :)
We don't need to drain request queue in normal case of nvme_reset_work.

As for updating nr_hw_queues, we could try some other method on it next.


Thanks
Jianchao

Re: [PATCH 2/3] blk-core: introduce queue close feature

[jianchao.wang]

Hi Bart

On 09/05/2018 11:57 PM, Bart Van Assche wrote:
> On Wed, 2018-09-05 at 12:09 +0800, Jianchao Wang wrote:
>> blk queue freeze is often used to prevent new IO from entering
>> request queue. However, becuase we kill the percpu-ref
>> q_usage_counter when freeze queue, we have to drain the request
>> queue when unfreeze. This is unnecessary for just preventing new
>> IO. In addition, If there is IO timeout or other issue when unfreeze
>> the queue, the scenario could be very tricky.
>>
>> So we introduce BLK_QUEUE_GATE_CLOSED to implement a light-weight
>> queue close feature base on the queue_gate to prevent new IO from
>> comming in queue which will not need to drain the queue any more.
> 
> Does the "queue gate close" feature cause blk_get_request() /
> blk_mq_get_request() to block until blk_clear_queue_closed() is called? 

Yes. It works in blk_queue_enter. So it could cover blk_mq_get_request

If
> so, I think we need a better name for this feature. How about calling these
> two operations suspend and resume?

I'm open for it. :)

> 
> Thanks,
> 
> Bart.
> 

Re: [PATCH 0/3] Introduce a light-weight queue close feature

[jianchao.wang]

Hi Bart

On 09/05/2018 11:48 PM, Bart Van Assche wrote:
>> The 3rd patch apply the queue close in nvme-pci to avoid the IO hang
>> issue above.
> Hello Jianchao,
> 
> Is this patch series based on a theoretical concern or rather on something
> you ran into? In the latter case, can you explain which scenario makes it
> likely on your setup to encounter an NVMe timeout?

It is a theoretical concern. But I think it is very obvious.
It could be very easy to reproduce if simulate IO timeout in software.

Thanks
Jianchao

Re: [PATCH 0/3] Introduce a light-weight queue close feature

[jianchao.wang]

Hi Ming

On 09/06/2018 05:27 AM, Ming Lei wrote:
> On Wed, Sep 05, 2018 at 12:09:43PM +0800, Jianchao Wang wrote:
>> Dear all
>>
>> As we know, queue freeze is used to stop new IO comming in and drain
>> the request queue. And the draining queue here is necessary, because
>> queue freeze kills the percpu-ref q_usage_counter and need to drain
>> the q_usage_counter before switch it back to percpu mode. This could
>> be a trouble when we just want to prevent new IO.
>>
>> In nvme-pci, nvme_dev_disable freezes queues to prevent new IO.
>> nvme_reset_work will unfreeze and wait to drain the queues. However,
>> if IO timeout at the moment, no body could do recovery as nvme_reset_work
>> is waiting. We will encounter IO hang.
> 
> As we discussed this nvme time issue before, I have pointed out that
> this is because of blk_mq_unfreeze_queue()'s limit which requires that
> unfreeze can only be done when this queue ref counter drops to zero.
> 
> For this nvme timeout case, we may relax the limit, for example,
> introducing another API of blk_freeze_queue_stop() as counter-pair of
> blk_freeze_queue_start(), and simply switch the percpu-ref to percpu mode
> from atomic mode inside the new API.

Looks like we cannot switch a percpu-ref to percpu mode directly w/o drain it.
Some references maybe lost.

static void __percpu_ref_switch_to_percpu(struct percpu_ref *ref)
{
	unsigned long __percpu *percpu_count = percpu_count_ptr(ref);
	int cpu;

	BUG_ON(!percpu_count);

	if (!(ref->percpu_count_ptr & __PERCPU_REF_ATOMIC))
		return;

	atomic_long_add(PERCPU_COUNT_BIAS, &ref->count);

	/*
	 * Restore per-cpu operation.  smp_store_release() is paired
	 * with READ_ONCE() in __ref_is_percpu() and guarantees that the
	 * zeroing is visible to all percpu accesses which can see the
	 * following __PERCPU_REF_ATOMIC clearing.
	 */
	for_each_possible_cpu(cpu)
		*per_cpu_ptr(percpu_count, cpu) = 0;

	smp_store_release(&ref->percpu_count_ptr,
			  ref->percpu_count_ptr & ~__PERCPU_REF_ATOMIC);
}

> 
>>
>> So introduce a light-weight queue close feature in this patch set
>> which could prevent new IO and needn't drain the queue.
> 
> Frankly speaking, IMO, it may not be an good idea to mess up the fast path
> just for handling the extremely unusual timeout event. The same is true
> for doing the preemp only stuff, as you saw I have posted patchset for
> killing it.
> 

In normal case, it is just a judgment like 

	if (unlikely(READ_ONCE(q->queue_gate))

It should not be a big deal.

Thanks
Jianchao

> Thanks,
> Ming
> 
> _______________________________________________
> Linux-nvme mailing list
> Linux-nvme@lists.infradead.org
> https://urldefense.proofpoint.com/v2/url?u=http-3A__lists.infradead.org_mailman_listinfo_linux-2Dnvme&d=DwICAg&c=RoP1YumCXCgaWHvlZYR8PZh8Bv7qIrMUB65eapI_JnE&r=7WdAxUBeiTUTCy8v-7zXyr4qk7sx26ATvfo6QSTvZyQ&m=DAEJOGyHAQ8bbVD3QYxBNFE2vn70OyFrmEF5VkwzHRw&s=pmqDbwFqgHROYOJBCE4k9SKOc7PRMk4ESya6gYks_CQ&e=
> 

Re: [PATCH 0/3] Introduce a light-weight queue close feature

[Ming Lei]

On Thu, Sep 06, 2018 at 09:51:43AM +0800, jianchao.wang wrote:
> Hi Ming
> 
> On 09/06/2018 05:27 AM, Ming Lei wrote:
> > On Wed, Sep 05, 2018 at 12:09:43PM +0800, Jianchao Wang wrote:
> >> Dear all
> >>
> >> As we know, queue freeze is used to stop new IO comming in and drain
> >> the request queue. And the draining queue here is necessary, because
> >> queue freeze kills the percpu-ref q_usage_counter and need to drain
> >> the q_usage_counter before switch it back to percpu mode. This could
> >> be a trouble when we just want to prevent new IO.
> >>
> >> In nvme-pci, nvme_dev_disable freezes queues to prevent new IO.
> >> nvme_reset_work will unfreeze and wait to drain the queues. However,
> >> if IO timeout at the moment, no body could do recovery as nvme_reset_work
> >> is waiting. We will encounter IO hang.
> > 
> > As we discussed this nvme time issue before, I have pointed out that
> > this is because of blk_mq_unfreeze_queue()'s limit which requires that
> > unfreeze can only be done when this queue ref counter drops to zero.
> > 
> > For this nvme timeout case, we may relax the limit, for example,
> > introducing another API of blk_freeze_queue_stop() as counter-pair of
> > blk_freeze_queue_start(), and simply switch the percpu-ref to percpu mode
> > from atomic mode inside the new API.
> 
> Looks like we cannot switch a percpu-ref to percpu mode directly w/o drain it.
> Some references maybe lost.
> 
> static void __percpu_ref_switch_to_percpu(struct percpu_ref *ref)
> {
> 	unsigned long __percpu *percpu_count = percpu_count_ptr(ref);
> 	int cpu;
> 
> 	BUG_ON(!percpu_count);
> 
> 	if (!(ref->percpu_count_ptr & __PERCPU_REF_ATOMIC))
> 		return;
> 
> 	atomic_long_add(PERCPU_COUNT_BIAS, &ref->count);
> 
> 	/*
> 	 * Restore per-cpu operation.  smp_store_release() is paired
> 	 * with READ_ONCE() in __ref_is_percpu() and guarantees that the
> 	 * zeroing is visible to all percpu accesses which can see the
> 	 * following __PERCPU_REF_ATOMIC clearing.
> 	 */
> 	for_each_possible_cpu(cpu)
> 		*per_cpu_ptr(percpu_count, cpu) = 0;
> 
> 	smp_store_release(&ref->percpu_count_ptr,
> 			  ref->percpu_count_ptr & ~__PERCPU_REF_ATOMIC);

Before REF_ATOMIC is cleared, all counting is done on the atomic type
of &ref->count, and it is easy to keep the reference counter at
ATOMIC mode. Also the reference counter can only be READ at atomic mode.

So could you explain a bit how the lost may happen? And it is lost at
atomic mode or percpu mode?

> }
> 
> > 
> >>
> >> So introduce a light-weight queue close feature in this patch set
> >> which could prevent new IO and needn't drain the queue.
> > 
> > Frankly speaking, IMO, it may not be an good idea to mess up the fast path
> > just for handling the extremely unusual timeout event. The same is true
> > for doing the preemp only stuff, as you saw I have posted patchset for
> > killing it.
> > 
> 
> In normal case, it is just a judgment like 
> 
> 	if (unlikely(READ_ONCE(q->queue_gate))
> 
> It should not be a big deal.

Adding this stuff in fast path is quite difficult to verify its correctness
because it is really lockless, or even barrier-less.

Not to mention, READ_ONCE() implies one barrier of smp_read_barrier_depends().

Thanks,
Ming

Re: [PATCH 3/3] nvme-pci: use queue close instead of queue freeze

[Ming Lei]

On Thu, Sep 06, 2018 at 09:28:46AM +0800, jianchao.wang wrote:
> Hi Ming
> 
> On 09/06/2018 06:09 AM, Ming Lei wrote:
> > nvme_dev_add() still may call freeze & unfreeze queue, so your patch
> > can't avoid draining queue completely here.
> 
> Yes, I know this. We still need to freeze queue when update nr_hw_queues.
> But we move forward a step at least. :)
> We don't need to drain request queue in normal case of nvme_reset_work.

It is hard to say who is the normal case. In case of CPU hotplug, it is quite
easy to trigger updating nr_hw_queues.

> 
> As for updating nr_hw_queues, we could try some other method on it next.

The thing is that draining queue may be inevitable inside reset work
function because of updating nr_hw_queues, that means the approach in
this patchset is just a partial solution

Or it may not make sense to do that because we may never remove the draining
queue in the code path of updating nr_hw_queues.

Given they belong to same issue, I suggest to solve them all.

Thanks,
Ming

Re: [PATCH 0/3] Introduce a light-weight queue close feature

[jianchao.wang]

On 09/06/2018 08:57 PM, Ming Lei wrote:
> On Thu, Sep 06, 2018 at 09:51:43AM +0800, jianchao.wang wrote:
>> Hi Ming
>>
>> On 09/06/2018 05:27 AM, Ming Lei wrote:
>>> On Wed, Sep 05, 2018 at 12:09:43PM +0800, Jianchao Wang wrote:
>>>> Dear all
>>>>
>>>> As we know, queue freeze is used to stop new IO comming in and drain
>>>> the request queue. And the draining queue here is necessary, because
>>>> queue freeze kills the percpu-ref q_usage_counter and need to drain
>>>> the q_usage_counter before switch it back to percpu mode. This could
>>>> be a trouble when we just want to prevent new IO.
>>>>
>>>> In nvme-pci, nvme_dev_disable freezes queues to prevent new IO.
>>>> nvme_reset_work will unfreeze and wait to drain the queues. However,
>>>> if IO timeout at the moment, no body could do recovery as nvme_reset_work
>>>> is waiting. We will encounter IO hang.
>>>
>>> As we discussed this nvme time issue before, I have pointed out that
>>> this is because of blk_mq_unfreeze_queue()'s limit which requires that
>>> unfreeze can only be done when this queue ref counter drops to zero.
>>>
>>> For this nvme timeout case, we may relax the limit, for example,
>>> introducing another API of blk_freeze_queue_stop() as counter-pair of
>>> blk_freeze_queue_start(), and simply switch the percpu-ref to percpu mode
>>> from atomic mode inside the new API.
>>
>> Looks like we cannot switch a percpu-ref to percpu mode directly w/o drain it.
>> Some references maybe lost.
>>
>> static void __percpu_ref_switch_to_percpu(struct percpu_ref *ref)
>> {
>> 	unsigned long __percpu *percpu_count = percpu_count_ptr(ref);
>> 	int cpu;
>>
>> 	BUG_ON(!percpu_count);
>>
>> 	if (!(ref->percpu_count_ptr & __PERCPU_REF_ATOMIC))
>> 		return;
>>
>> 	atomic_long_add(PERCPU_COUNT_BIAS, &ref->count);
>>
>> 	/*
>> 	 * Restore per-cpu operation.  smp_store_release() is paired
>> 	 * with READ_ONCE() in __ref_is_percpu() and guarantees that the
>> 	 * zeroing is visible to all percpu accesses which can see the
>> 	 * following __PERCPU_REF_ATOMIC clearing.i
>> 	 */
>> 	for_each_possible_cpu(cpu)
>> 		*per_cpu_ptr(percpu_count, cpu) = 0;
>>
>> 	smp_store_release(&ref->percpu_count_ptr,
>> 			  ref->percpu_count_ptr & ~__PERCPU_REF_ATOMIC);
> 
> Before REF_ATOMIC is cleared, all counting is done on the atomic type
> of &ref->count, and it is easy to keep the reference counter at
> ATOMIC mode. Also the reference counter can only be READ at atomic mode.
> 
> So could you explain a bit how the lost may happen? And it is lost at
> atomic mode or percpu mode?

I just mean __percpu_ref_switch_percpu just zeros the percpu_count.
It doesn't give the original values back to the percpu_count from atomic count

> 
>> }
>>
>>>
>>>>
>>>> So introduce a light-weight queue close feature in this patch set
>>>> which could prevent new IO and needn't drain the queue.
>>>
>>> Frankly speaking, IMO, it may not be an good idea to mess up the fast path
>>> just for handling the extremely unusual timeout event. The same is true
>>> for doing the preemp only stuff, as you saw I have posted patchset for
>>> killing it.
>>>
>>
>> In normal case, it is just a judgment like 
>>
>> 	if (unlikely(READ_ONCE(q->queue_gate))
>>
>> It should not be a big deal.> 
> Adding this stuff in fast path is quite difficult to verify its correctness
> because it is really lockless, or even barrier-less.
> 
> Not to mention, READ_ONCE() implies one barrier of smp_read_barrier_depends().

The checking is under rcu lock, the write side could use synchonize_rcu to ensure
the updating is globally visible.t
As for the READ_ONCE, it could be discarded.

Thanks
Jianchao
> 
> Thanks,
> Ming
> 
> _______________________________________________
> Linux-nvme mailing list
> Linux-nvme@lists.infradead.org
> https://urldefense.proofpoint.com/v2/url?u=http-3A__lists.infradead.org_mailman_listinfo_linux-2Dnvme&d=DwICAg&c=RoP1YumCXCgaWHvlZYR8PZh8Bv7qIrMUB65eapI_JnE&r=7WdAxUBeiTUTCy8v-7zXyr4qk7sx26ATvfo6QSTvZyQ&m=BQVCMSSS6lYwogr6CE82oIlpLu5ReP8c4lHGgnvswV4&s=rZLfbqzKKXjlCpY1Sy6ocaQjAKZoJq_A49gvSucohNk&e=
> 

Re: [PATCH 3/3] nvme-pci: use queue close instead of queue freeze

[jianchao.wang]

On 09/06/2018 09:07 PM, Ming Lei wrote:
> On Thu, Sep 06, 2018 at 09:28:46AM +0800, jianchao.wang wrote:
>> Hi Ming
>>
>> On 09/06/2018 06:09 AM, Ming Lei wrote:
>>> nvme_dev_add() still may call freeze & unfreeze queue, so your patch
>>> can't avoid draining queue completely here.
>>
>> Yes, I know this. We still need to freeze queue when update nr_hw_queues.
>> But we move forward a step at least. :)
>> We don't need to drain request queue in normal case of nvme_reset_work.
> 
> It is hard to say who is the normal case. In case of CPU hotplug, it is quite
> easy to trigger updating nr_hw_queues.
> 
>>
>> As for updating nr_hw_queues, we could try some other method on it next.
> 
> The thing is that draining queue may be inevitable inside reset work
> function because of updating nr_hw_queues, that means the approach in
> this patchset is just a partial solution
> 
> Or it may not make sense to do that because we may never remove the draining
> queue in the code path of updating nr_hw_queues.
> 
> Given they belong to same issue, I suggest to solve them all.

I still think if just prevent new IO, freeze queue is over-kill.

With respect to update nr_hw_queues, we could try to take the bios down from the
queued requests as the blk_steal_bios. Then re-issue these bios after updating the
nr_hw_queues.

Thanks
Jianchao

> 
> Thanks,
> Ming
> 
> _______________________________________________
> Linux-nvme mailing list
> Linux-nvme@lists.infradead.org
> https://urldefense.proofpoint.com/v2/url?u=http-3A__lists.infradead.org_mailman_listinfo_linux-2Dnvme&d=DwICAg&c=RoP1YumCXCgaWHvlZYR8PZh8Bv7qIrMUB65eapI_JnE&r=7WdAxUBeiTUTCy8v-7zXyr4qk7sx26ATvfo6QSTvZyQ&m=cybJ83konrbKh9jtodiaZRmNrKAo1YtEI_oyaNIbuvk&s=EBhkv_XTOQb3K3-4esnkBrs_ErKuNzXDqZwrIzEA0ig&e=
> 

Re: [PATCH 0/3] Introduce a light-weight queue close feature

[Ming Lei]

On Thu, Sep 06, 2018 at 09:55:21PM +0800, jianchao.wang wrote:
> 
> 
> On 09/06/2018 08:57 PM, Ming Lei wrote:
> > On Thu, Sep 06, 2018 at 09:51:43AM +0800, jianchao.wang wrote:
> >> Hi Ming
> >>
> >> On 09/06/2018 05:27 AM, Ming Lei wrote:
> >>> On Wed, Sep 05, 2018 at 12:09:43PM +0800, Jianchao Wang wrote:
> >>>> Dear all
> >>>>
> >>>> As we know, queue freeze is used to stop new IO comming in and drain
> >>>> the request queue. And the draining queue here is necessary, because
> >>>> queue freeze kills the percpu-ref q_usage_counter and need to drain
> >>>> the q_usage_counter before switch it back to percpu mode. This could
> >>>> be a trouble when we just want to prevent new IO.
> >>>>
> >>>> In nvme-pci, nvme_dev_disable freezes queues to prevent new IO.
> >>>> nvme_reset_work will unfreeze and wait to drain the queues. However,
> >>>> if IO timeout at the moment, no body could do recovery as nvme_reset_work
> >>>> is waiting. We will encounter IO hang.
> >>>
> >>> As we discussed this nvme time issue before, I have pointed out that
> >>> this is because of blk_mq_unfreeze_queue()'s limit which requires that
> >>> unfreeze can only be done when this queue ref counter drops to zero.
> >>>
> >>> For this nvme timeout case, we may relax the limit, for example,
> >>> introducing another API of blk_freeze_queue_stop() as counter-pair of
> >>> blk_freeze_queue_start(), and simply switch the percpu-ref to percpu mode
> >>> from atomic mode inside the new API.
> >>
> >> Looks like we cannot switch a percpu-ref to percpu mode directly w/o drain it.
> >> Some references maybe lost.
> >>
> >> static void __percpu_ref_switch_to_percpu(struct percpu_ref *ref)
> >> {
> >> 	unsigned long __percpu *percpu_count = percpu_count_ptr(ref);
> >> 	int cpu;
> >>
> >> 	BUG_ON(!percpu_count);
> >>
> >> 	if (!(ref->percpu_count_ptr & __PERCPU_REF_ATOMIC))
> >> 		return;
> >>
> >> 	atomic_long_add(PERCPU_COUNT_BIAS, &ref->count);
> >>
> >> 	/*
> >> 	 * Restore per-cpu operation.  smp_store_release() is paired
> >> 	 * with READ_ONCE() in __ref_is_percpu() and guarantees that the
> >> 	 * zeroing is visible to all percpu accesses which can see the
> >> 	 * following __PERCPU_REF_ATOMIC clearing.i
> >> 	 */
> >> 	for_each_possible_cpu(cpu)
> >> 		*per_cpu_ptr(percpu_count, cpu) = 0;
> >>
> >> 	smp_store_release(&ref->percpu_count_ptr,
> >> 			  ref->percpu_count_ptr & ~__PERCPU_REF_ATOMIC);
> > 
> > Before REF_ATOMIC is cleared, all counting is done on the atomic type
> > of &ref->count, and it is easy to keep the reference counter at
> > ATOMIC mode. Also the reference counter can only be READ at atomic mode.
> > 
> > So could you explain a bit how the lost may happen? And it is lost at
> > atomic mode or percpu mode?
> 
> I just mean __percpu_ref_switch_percpu just zeros the percpu_count.
> It doesn't give the original values back to the percpu_count from atomic count

The original value has been added to the atomic part of the refcount,
please see percpu_ref_switch_to_atomic_rcu(), so zeroing is fine.

What do you think of the following patch? Which will allow to reinit one
percpu_refcount from atomic mode when it doesn't drop to zero.

--
diff --git a/block/blk-mq.c b/block/blk-mq.c
index 85a1c1a59c72..c05d8924b4cb 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -134,19 +134,33 @@ void blk_mq_in_flight_rw(struct request_queue *q, struct hd_struct *part,
 	blk_mq_queue_tag_busy_iter(q, blk_mq_check_inflight_rw, &mi);
 }
 
-void blk_freeze_queue_start(struct request_queue *q)
+static void __blk_freeze_queue_start(struct request_queue *q, bool kill_sync)
 {
 	int freeze_depth;
 
 	freeze_depth = atomic_inc_return(&q->mq_freeze_depth);
 	if (freeze_depth == 1) {
-		percpu_ref_kill(&q->q_usage_counter);
+		if (kill_sync)
+			percpu_ref_kill_sync(&q->q_usage_counter);
+		else
+			percpu_ref_kill(&q->q_usage_counter);
 		if (q->mq_ops)
 			blk_mq_run_hw_queues(q, false);
 	}
 }
+
+void blk_freeze_queue_start(struct request_queue *q)
+{
+	__blk_freeze_queue_start(q, false);
+}
 EXPORT_SYMBOL_GPL(blk_freeze_queue_start);
 
+void blk_freeze_queue_start_sync(struct request_queue *q)
+{
+	__blk_freeze_queue_start(q, true);
+}
+EXPORT_SYMBOL_GPL(blk_freeze_queue_start_sync);
+
 void blk_mq_freeze_queue_wait(struct request_queue *q)
 {
 	wait_event(q->mq_freeze_wq, percpu_ref_is_zero(&q->q_usage_counter));
diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index dd8ec1dd9219..bde5a054597d 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -3644,7 +3644,7 @@ void nvme_start_freeze(struct nvme_ctrl *ctrl)
 
 	down_read(&ctrl->namespaces_rwsem);
 	list_for_each_entry(ns, &ctrl->namespaces, list)
-		blk_freeze_queue_start(ns->queue);
+		blk_freeze_queue_start_sync(ns->queue);
 	up_read(&ctrl->namespaces_rwsem);
 }
 EXPORT_SYMBOL_GPL(nvme_start_freeze);
diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index d668682f91df..7b9ac1f9bce3 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -2328,7 +2328,6 @@ static void nvme_reset_work(struct work_struct *work)
 		new_state = NVME_CTRL_ADMIN_ONLY;
 	} else {
 		nvme_start_queues(&dev->ctrl);
-		nvme_wait_freeze(&dev->ctrl);
 		/* hit this only when allocate tagset fails */
 		if (nvme_dev_add(dev))
 			new_state = NVME_CTRL_ADMIN_ONLY;
diff --git a/include/linux/blk-mq.h b/include/linux/blk-mq.h
index 1da59c16f637..5cf8e3a6335c 100644
--- a/include/linux/blk-mq.h
+++ b/include/linux/blk-mq.h
@@ -280,6 +280,7 @@ void blk_mq_tagset_busy_iter(struct blk_mq_tag_set *tagset,
 void blk_mq_freeze_queue(struct request_queue *q);
 void blk_mq_unfreeze_queue(struct request_queue *q);
 void blk_freeze_queue_start(struct request_queue *q);
+void blk_freeze_queue_start_sync(struct request_queue *q);
 void blk_mq_freeze_queue_wait(struct request_queue *q);
 int blk_mq_freeze_queue_wait_timeout(struct request_queue *q,
 				     unsigned long timeout);
diff --git a/include/linux/percpu-refcount.h b/include/linux/percpu-refcount.h
index 009cdf3d65b6..36f4428b2a6c 100644
--- a/include/linux/percpu-refcount.h
+++ b/include/linux/percpu-refcount.h
@@ -127,6 +127,8 @@ static inline void percpu_ref_kill(struct percpu_ref *ref)
 	percpu_ref_kill_and_confirm(ref, NULL);
 }
 
+extern void percpu_ref_kill_sync(struct percpu_ref *ref);
+
 /*
  * Internal helper.  Don't use outside percpu-refcount proper.  The
  * function doesn't return the pointer and let the caller test it for NULL
diff --git a/lib/percpu-refcount.c b/lib/percpu-refcount.c
index 9f96fa7bc000..4c0ee5eda2d6 100644
--- a/lib/percpu-refcount.c
+++ b/lib/percpu-refcount.c
@@ -130,8 +130,10 @@ static void percpu_ref_switch_to_atomic_rcu(struct rcu_head *rcu)
 	unsigned long count = 0;
 	int cpu;
 
-	for_each_possible_cpu(cpu)
+	for_each_possible_cpu(cpu) {
 		count += *per_cpu_ptr(percpu_count, cpu);
+		*per_cpu_ptr(percpu_count, cpu) = 0;
+	}
 
 	pr_debug("global %ld percpu %ld",
 		 atomic_long_read(&ref->count), (long)count);
@@ -187,7 +189,6 @@ static void __percpu_ref_switch_to_atomic(struct percpu_ref *ref,
 static void __percpu_ref_switch_to_percpu(struct percpu_ref *ref)
 {
 	unsigned long __percpu *percpu_count = percpu_count_ptr(ref);
-	int cpu;
 
 	BUG_ON(!percpu_count);
 
@@ -196,15 +197,6 @@ static void __percpu_ref_switch_to_percpu(struct percpu_ref *ref)
 
 	atomic_long_add(PERCPU_COUNT_BIAS, &ref->count);
 
-	/*
-	 * Restore per-cpu operation.  smp_store_release() is paired
-	 * with READ_ONCE() in __ref_is_percpu() and guarantees that the
-	 * zeroing is visible to all percpu accesses which can see the
-	 * following __PERCPU_REF_ATOMIC clearing.
-	 */
-	for_each_possible_cpu(cpu)
-		*per_cpu_ptr(percpu_count, cpu) = 0;
-
 	smp_store_release(&ref->percpu_count_ptr,
 			  ref->percpu_count_ptr & ~__PERCPU_REF_ATOMIC);
 }
@@ -278,6 +270,23 @@ void percpu_ref_switch_to_atomic_sync(struct percpu_ref *ref)
 EXPORT_SYMBOL_GPL(percpu_ref_switch_to_atomic_sync);
 
 /**
+ * percpu_ref_kill_sync - drop the initial ref and and wait for the
+ * switch to complete.
+ * @ref: percpu_ref to kill
+ *
+ * Must be used to drop the initial ref on a percpu refcount; must be called
+ * precisely once before shutdown.
+ *
+ * Switches @ref into atomic mode before gathering up the percpu counters
+ * and dropping the initial ref.
+ */
+void percpu_ref_kill_sync(struct percpu_ref *ref)
+{
+	percpu_ref_kill(ref);
+	wait_event(percpu_ref_switch_waitq, !ref->confirm_switch);
+}
+
+/**
  * percpu_ref_switch_to_percpu - switch a percpu_ref to percpu mode
  * @ref: percpu_ref to switch to percpu mode
  *
@@ -349,7 +358,7 @@ EXPORT_SYMBOL_GPL(percpu_ref_kill_and_confirm);
  *
  * Re-initialize @ref so that it's in the same state as when it finished
  * percpu_ref_init() ignoring %PERCPU_REF_INIT_DEAD.  @ref must have been
- * initialized successfully and reached 0 but not exited.
+ * initialized successfully and in atomic mode but not exited.
  *
  * Note that percpu_ref_tryget[_live]() are safe to perform on @ref while
  * this function is in progress.
@@ -357,10 +366,11 @@ EXPORT_SYMBOL_GPL(percpu_ref_kill_and_confirm);
 void percpu_ref_reinit(struct percpu_ref *ref)
 {
 	unsigned long flags;
+	unsigned long __percpu *percpu_count;
 
 	spin_lock_irqsave(&percpu_ref_switch_lock, flags);
 
-	WARN_ON_ONCE(!percpu_ref_is_zero(ref));
+	WARN_ON_ONCE(__ref_is_percpu(ref, &percpu_count));
 
 	ref->percpu_count_ptr &= ~__PERCPU_REF_DEAD;
 	percpu_ref_get(ref);

Thanks,
Ming

Re: [PATCH 3/3] nvme-pci: use queue close instead of queue freeze

[Ming Lei]

On Thu, Sep 06, 2018 at 10:19:27PM +0800, jianchao.wang wrote:
> 
> 
> On 09/06/2018 09:07 PM, Ming Lei wrote:
> > On Thu, Sep 06, 2018 at 09:28:46AM +0800, jianchao.wang wrote:
> >> Hi Ming
> >>
> >> On 09/06/2018 06:09 AM, Ming Lei wrote:
> >>> nvme_dev_add() still may call freeze & unfreeze queue, so your patch
> >>> can't avoid draining queue completely here.
> >>
> >> Yes, I know this. We still need to freeze queue when update nr_hw_queues.
> >> But we move forward a step at least. :)
> >> We don't need to drain request queue in normal case of nvme_reset_work.
> > 
> > It is hard to say who is the normal case. In case of CPU hotplug, it is quite
> > easy to trigger updating nr_hw_queues.
> > 
> >>
> >> As for updating nr_hw_queues, we could try some other method on it next.
> > 
> > The thing is that draining queue may be inevitable inside reset work
> > function because of updating nr_hw_queues, that means the approach in
> > this patchset is just a partial solution
> > 
> > Or it may not make sense to do that because we may never remove the draining
> > queue in the code path of updating nr_hw_queues.
> > 
> > Given they belong to same issue, I suggest to solve them all.
> 
> I still think if just prevent new IO, freeze queue is over-kill.
> 
> With respect to update nr_hw_queues, we could try to take the bios down from the
> queued requests as the blk_steal_bios. Then re-issue these bios after updating the
> nr_hw_queues.

It is always easier to say than done, :-)

Please go ahead and post patches, and I am happy to review.


Thanks,
Ming

Re: [PATCH 3/3] nvme-pci: use queue close instead of queue freeze

[jianchao.wang]

On 09/08/2018 12:23 AM, Ming Lei wrote:
> On Thu, Sep 06, 2018 at 10:19:27PM +0800, jianchao.wang wrote:
>>
>>
>> On 09/06/2018 09:07 PM, Ming Lei wrote:
>>> On Thu, Sep 06, 2018 at 09:28:46AM +0800, jianchao.wang wrote:
>>>> Hi Ming
>>>>
>>>> On 09/06/2018 06:09 AM, Ming Lei wrote:
>>>>> nvme_dev_add() still may call freeze & unfreeze queue, so your patch
>>>>> can't avoid draining queue completely here.
>>>>
>>>> Yes, I know this. We still need to freeze queue when update nr_hw_queues.
>>>> But we move forward a step at least. :)
>>>> We don't need to drain request queue in normal case of nvme_reset_work.
>>>
>>> It is hard to say who is the normal case. In case of CPU hotplug, it is quite
>>> easy to trigger updating nr_hw_queues.
>>>
>>>>
>>>> As for updating nr_hw_queues, we could try some other method on it next.
>>>
>>> The thing is that draining queue may be inevitable inside reset work
>>> function because of updating nr_hw_queues, that means the approach in
>>> this patchset is just a partial solution
>>>
>>> Or it may not make sense to do that because we may never remove the draining
>>> queue in the code path of updating nr_hw_queues.
>>>
>>> Given they belong to same issue, I suggest to solve them all.
>>
>> I still think if just prevent new IO, freeze queue is over-kill.
>>
>> With respect to update nr_hw_queues, we could try to take the bios down from the
>> queued requests as the blk_steal_bios. Then re-issue these bios after updating the
>> nr_hw_queues.
> 
> It is always easier to say than done, :-)
> 
> Please go ahead and post patches, and I am happy to review.
> 

Yes, I'm trying it and will post it when complete. :)

Thanks
Jianchao

> 
> Thanks,
> Ming
> 
> _______________________________________________
> Linux-nvme mailing list
> Linux-nvme@lists.infradead.org
> https://urldefense.proofpoint.com/v2/url?u=http-3A__lists.infradead.org_mailman_listinfo_linux-2Dnvme&d=DwICAg&c=RoP1YumCXCgaWHvlZYR8PZh8Bv7qIrMUB65eapI_JnE&r=7WdAxUBeiTUTCy8v-7zXyr4qk7sx26ATvfo6QSTvZyQ&m=8v99qftJpifVTL4l7Od8_RT0UZCUN1aIotRsKWugz28&s=Z56o8mcARcWcRHe1yOr8Hu2a7djUCM6hKtrDTHuA9Qc&e=
>