All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/4] block: fix concurrent quiesce
@ 2021-11-03  3:43 Ming Lei
  2021-11-03  3:43 ` [PATCH 1/4] blk-mq: add one API for waiting until quiesce is done Ming Lei
                   ` (4 more replies)
  0 siblings, 5 replies; 12+ messages in thread
From: Ming Lei @ 2021-11-03  3:43 UTC (permalink / raw)
  To: Jens Axboe
  Cc: Yi Zhang, linux-block, linux-nvme, Martin K . Petersen,
	linux-scsi, James Bottomley, Ming Lei

Hi Jens,

Convert SCSI into balanced quiesce and unquiesce by using atomic
variable as suggested by James, meantime fix previous nvme conversion by
adding one new API because we have to wait until the started quiesce is
done.


Ming Lei (4):
  blk-mq: add one API for waiting until quiesce is done
  scsi: avoid to quiesce sdev->request_queue two times
  scsi: make sure that request queue queiesce and unquiesce balanced
  nvme: wait until quiesce is done

 block/blk-mq.c             | 28 +++++++++++++------
 drivers/nvme/host/core.c   |  4 +++
 drivers/scsi/scsi_lib.c    | 55 +++++++++++++++++++++++---------------
 include/linux/blk-mq.h     |  1 +
 include/scsi/scsi_device.h |  1 +
 5 files changed, 59 insertions(+), 30 deletions(-)

-- 
2.31.1


^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH 1/4] blk-mq: add one API for waiting until quiesce is done
  2021-11-03  3:43 [PATCH 0/4] block: fix concurrent quiesce Ming Lei
@ 2021-11-03  3:43 ` Ming Lei
  2021-11-03  3:43 ` [PATCH 2/4] scsi: avoid to quiesce sdev->request_queue two times Ming Lei
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 12+ messages in thread
From: Ming Lei @ 2021-11-03  3:43 UTC (permalink / raw)
  To: Jens Axboe
  Cc: Yi Zhang, linux-block, linux-nvme, Martin K . Petersen,
	linux-scsi, James Bottomley, Ming Lei

Some drivers(NVMe, SCSI) need to call quiesce and unquiesce in pair, but it
is hard to switch to this style, so these drivers need one atomic flag for
helping to balance quiesce and unquiesce.

When quiesce is in-progress, the driver still needs to wait until
the quiesce is done, so add API of blk_mq_wait_quiesce_done() for
these drivers.

Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 block/blk-mq.c         | 28 ++++++++++++++++++++--------
 include/linux/blk-mq.h |  1 +
 2 files changed, 21 insertions(+), 8 deletions(-)

diff --git a/block/blk-mq.c b/block/blk-mq.c
index 2a2c57c98bbd..9fe0677f03a2 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -251,22 +251,18 @@ void blk_mq_quiesce_queue_nowait(struct request_queue *q)
 EXPORT_SYMBOL_GPL(blk_mq_quiesce_queue_nowait);
 
 /**
- * blk_mq_quiesce_queue() - wait until all ongoing dispatches have finished
+ * blk_mq_wait_quiesce_done() - wait until in-progress quiesce is done
  * @q: request queue.
  *
- * Note: this function does not prevent that the struct request end_io()
- * callback function is invoked. Once this function is returned, we make
- * sure no dispatch can happen until the queue is unquiesced via
- * blk_mq_unquiesce_queue().
+ * Note: it is driver's responsibility for making sure that quiesce has
+ * been started.
  */
-void blk_mq_quiesce_queue(struct request_queue *q)
+void blk_mq_wait_quiesce_done(struct request_queue *q)
 {
 	struct blk_mq_hw_ctx *hctx;
 	unsigned int i;
 	bool rcu = false;
 
-	blk_mq_quiesce_queue_nowait(q);
-
 	queue_for_each_hw_ctx(q, hctx, i) {
 		if (hctx->flags & BLK_MQ_F_BLOCKING)
 			synchronize_srcu(hctx->srcu);
@@ -276,6 +272,22 @@ void blk_mq_quiesce_queue(struct request_queue *q)
 	if (rcu)
 		synchronize_rcu();
 }
+EXPORT_SYMBOL_GPL(blk_mq_wait_quiesce_done);
+
+/**
+ * blk_mq_quiesce_queue() - wait until all ongoing dispatches have finished
+ * @q: request queue.
+ *
+ * Note: this function does not prevent that the struct request end_io()
+ * callback function is invoked. Once this function is returned, we make
+ * sure no dispatch can happen until the queue is unquiesced via
+ * blk_mq_unquiesce_queue().
+ */
+void blk_mq_quiesce_queue(struct request_queue *q)
+{
+	blk_mq_quiesce_queue_nowait(q);
+	blk_mq_wait_quiesce_done(q);
+}
 EXPORT_SYMBOL_GPL(blk_mq_quiesce_queue);
 
 /*
diff --git a/include/linux/blk-mq.h b/include/linux/blk-mq.h
index 8682663e7368..2949d9ac7484 100644
--- a/include/linux/blk-mq.h
+++ b/include/linux/blk-mq.h
@@ -798,6 +798,7 @@ void blk_mq_start_hw_queues(struct request_queue *q);
 void blk_mq_start_stopped_hw_queue(struct blk_mq_hw_ctx *hctx, bool async);
 void blk_mq_start_stopped_hw_queues(struct request_queue *q, bool async);
 void blk_mq_quiesce_queue(struct request_queue *q);
+void blk_mq_wait_quiesce_done(struct request_queue *q);
 void blk_mq_unquiesce_queue(struct request_queue *q);
 void blk_mq_delay_run_hw_queue(struct blk_mq_hw_ctx *hctx, unsigned long msecs);
 void blk_mq_run_hw_queue(struct blk_mq_hw_ctx *hctx, bool async);
-- 
2.31.1


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH 2/4] scsi: avoid to quiesce sdev->request_queue two times
  2021-11-03  3:43 [PATCH 0/4] block: fix concurrent quiesce Ming Lei
  2021-11-03  3:43 ` [PATCH 1/4] blk-mq: add one API for waiting until quiesce is done Ming Lei
@ 2021-11-03  3:43 ` Ming Lei
  2021-11-03  3:43 ` [PATCH 3/4] scsi: make sure that request queue queiesce and unquiesce balanced Ming Lei
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 12+ messages in thread
From: Ming Lei @ 2021-11-03  3:43 UTC (permalink / raw)
  To: Jens Axboe
  Cc: Yi Zhang, linux-block, linux-nvme, Martin K . Petersen,
	linux-scsi, James Bottomley, Ming Lei

For fixing queue quiesce race between driver and block layer(elevator
switch, update nr_requests, ...), we need to support concurrent quiesce
and unquiesce, which requires the two to be balanced.

blk_mq_quiesce_queue() calls blk_mq_quiesce_queue_nowait() for updating
quiesce depth and marking the flag, then scsi_internal_device_block() calls
blk_mq_quiesce_queue_nowait() two times actually.

Fix the double quiesce and keep quiesce and unquiesce balanced.

Reported-by: Yi Zhang <yi.zhang@redhat.com>
Fixes: e70feb8b3e68 ("blk-mq: support concurrent queue quiesce/unquiesce")
Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 drivers/scsi/scsi_lib.c | 29 ++++++++++++++---------------
 1 file changed, 14 insertions(+), 15 deletions(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 9c2b99e12ce3..1cd3ef9056d5 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -2645,6 +2645,14 @@ scsi_target_resume(struct scsi_target *starget)
 }
 EXPORT_SYMBOL(scsi_target_resume);
 
+static int __scsi_internal_device_block_nowait(struct scsi_device *sdev)
+{
+	if (scsi_device_set_state(sdev, SDEV_BLOCK))
+		return scsi_device_set_state(sdev, SDEV_CREATED_BLOCK);
+
+	return 0;
+}
+
 /**
  * scsi_internal_device_block_nowait - try to transition to the SDEV_BLOCK state
  * @sdev: device to block
@@ -2661,24 +2669,16 @@ EXPORT_SYMBOL(scsi_target_resume);
  */
 int scsi_internal_device_block_nowait(struct scsi_device *sdev)
 {
-	struct request_queue *q = sdev->request_queue;
-	int err = 0;
-
-	err = scsi_device_set_state(sdev, SDEV_BLOCK);
-	if (err) {
-		err = scsi_device_set_state(sdev, SDEV_CREATED_BLOCK);
-
-		if (err)
-			return err;
-	}
+	int ret = __scsi_internal_device_block_nowait(sdev);
 
 	/*
 	 * The device has transitioned to SDEV_BLOCK.  Stop the
 	 * block layer from calling the midlayer with this device's
 	 * request queue.
 	 */
-	blk_mq_quiesce_queue_nowait(q);
-	return 0;
+	if (!ret)
+		blk_mq_quiesce_queue_nowait(sdev->request_queue);
+	return ret;
 }
 EXPORT_SYMBOL_GPL(scsi_internal_device_block_nowait);
 
@@ -2699,13 +2699,12 @@ EXPORT_SYMBOL_GPL(scsi_internal_device_block_nowait);
  */
 static int scsi_internal_device_block(struct scsi_device *sdev)
 {
-	struct request_queue *q = sdev->request_queue;
 	int err;
 
 	mutex_lock(&sdev->state_mutex);
-	err = scsi_internal_device_block_nowait(sdev);
+	err = __scsi_internal_device_block_nowait(sdev);
 	if (err == 0)
-		blk_mq_quiesce_queue(q);
+		blk_mq_quiesce_queue(sdev->request_queue);
 	mutex_unlock(&sdev->state_mutex);
 
 	return err;
-- 
2.31.1


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH 3/4] scsi: make sure that request queue queiesce and unquiesce balanced
  2021-11-03  3:43 [PATCH 0/4] block: fix concurrent quiesce Ming Lei
  2021-11-03  3:43 ` [PATCH 1/4] blk-mq: add one API for waiting until quiesce is done Ming Lei
  2021-11-03  3:43 ` [PATCH 2/4] scsi: avoid to quiesce sdev->request_queue two times Ming Lei
@ 2021-11-03  3:43 ` Ming Lei
  2021-11-08 16:42   ` James Bottomley
  2021-11-03  3:43 ` [PATCH 4/4] nvme: wait until quiesce is done Ming Lei
  2021-11-07 21:20 ` [PATCH 0/4] block: fix concurrent quiesce Jens Axboe
  4 siblings, 1 reply; 12+ messages in thread
From: Ming Lei @ 2021-11-03  3:43 UTC (permalink / raw)
  To: Jens Axboe
  Cc: Yi Zhang, linux-block, linux-nvme, Martin K . Petersen,
	linux-scsi, James Bottomley, Ming Lei

For fixing queue quiesce race between driver and block layer(elevator
switch, update nr_requests, ...), we need to support concurrent quiesce
and unquiesce, which requires the two call balanced.

It isn't easy to audit that in all scsi drivers, especially the two may
be called from different contexts, so do it in scsi core with one
per-device atomic variable to balance quiesce and unquiesce.

Reported-by: Yi Zhang <yi.zhang@redhat.com>
Fixes: e70feb8b3e68 ("blk-mq: support concurrent queue quiesce/unquiesce")
Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 drivers/scsi/scsi_lib.c    | 30 +++++++++++++++++++++---------
 include/scsi/scsi_device.h |  1 +
 2 files changed, 22 insertions(+), 9 deletions(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 1cd3ef9056d5..e8925a35cb3a 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -2653,6 +2653,25 @@ static int __scsi_internal_device_block_nowait(struct scsi_device *sdev)
 	return 0;
 }
 
+void scsi_start_queue(struct scsi_device *sdev)
+{
+	if (cmpxchg(&sdev->queue_stopped, 1, 0))
+		blk_mq_unquiesce_queue(sdev->request_queue);
+}
+
+static void scsi_stop_queue(struct scsi_device *sdev, bool nowait)
+{
+	if (!cmpxchg(&sdev->queue_stopped, 0, 1)) {
+		if (nowait)
+			blk_mq_quiesce_queue_nowait(sdev->request_queue);
+		else
+			blk_mq_quiesce_queue(sdev->request_queue);
+	} else {
+		if (!nowait)
+			blk_mq_wait_quiesce_done(sdev->request_queue);
+	}
+}
+
 /**
  * scsi_internal_device_block_nowait - try to transition to the SDEV_BLOCK state
  * @sdev: device to block
@@ -2677,7 +2696,7 @@ int scsi_internal_device_block_nowait(struct scsi_device *sdev)
 	 * request queue.
 	 */
 	if (!ret)
-		blk_mq_quiesce_queue_nowait(sdev->request_queue);
+		scsi_stop_queue(sdev, true);
 	return ret;
 }
 EXPORT_SYMBOL_GPL(scsi_internal_device_block_nowait);
@@ -2704,19 +2723,12 @@ static int scsi_internal_device_block(struct scsi_device *sdev)
 	mutex_lock(&sdev->state_mutex);
 	err = __scsi_internal_device_block_nowait(sdev);
 	if (err == 0)
-		blk_mq_quiesce_queue(sdev->request_queue);
+		scsi_stop_queue(sdev, false);
 	mutex_unlock(&sdev->state_mutex);
 
 	return err;
 }
 
-void scsi_start_queue(struct scsi_device *sdev)
-{
-	struct request_queue *q = sdev->request_queue;
-
-	blk_mq_unquiesce_queue(q);
-}
-
 /**
  * scsi_internal_device_unblock_nowait - resume a device after a block request
  * @sdev:	device to resume
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index 430b73bd02ac..d1c6fc83b1e3 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -207,6 +207,7 @@ struct scsi_device {
 					 * creation time */
 	unsigned ignore_media_change:1; /* Ignore MEDIA CHANGE on resume */
 
+	unsigned int queue_stopped;	/* request queue is quiesced */
 	bool offline_already;		/* Device offline message logged */
 
 	atomic_t disk_events_disable_depth; /* disable depth for disk events */
-- 
2.31.1


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH 4/4] nvme: wait until quiesce is done
  2021-11-03  3:43 [PATCH 0/4] block: fix concurrent quiesce Ming Lei
                   ` (2 preceding siblings ...)
  2021-11-03  3:43 ` [PATCH 3/4] scsi: make sure that request queue queiesce and unquiesce balanced Ming Lei
@ 2021-11-03  3:43 ` Ming Lei
  2021-11-08 16:45   ` Keith Busch
  2021-11-12 15:38   ` Sagi Grimberg
  2021-11-07 21:20 ` [PATCH 0/4] block: fix concurrent quiesce Jens Axboe
  4 siblings, 2 replies; 12+ messages in thread
From: Ming Lei @ 2021-11-03  3:43 UTC (permalink / raw)
  To: Jens Axboe
  Cc: Yi Zhang, linux-block, linux-nvme, Martin K . Petersen,
	linux-scsi, James Bottomley, Ming Lei

NVMe uses one atomic flag to check if quiesce is needed. If quiesce is
started, the helper returns immediately. This way is wrong, since we
have to wait until quiesce is done.

Fixes: e70feb8b3e68 ("blk-mq: support concurrent queue quiesce/unquiesce")
Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 drivers/nvme/host/core.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 838b5e2058be..4b5de8f5435a 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -4518,6 +4518,8 @@ static void nvme_stop_ns_queue(struct nvme_ns *ns)
 {
 	if (!test_and_set_bit(NVME_NS_STOPPED, &ns->flags))
 		blk_mq_quiesce_queue(ns->queue);
+	else
+		blk_mq_wait_quiesce_done(ns->queue);
 }
 
 /*
@@ -4637,6 +4639,8 @@ void nvme_stop_admin_queue(struct nvme_ctrl *ctrl)
 {
 	if (!test_and_set_bit(NVME_CTRL_ADMIN_Q_STOPPED, &ctrl->flags))
 		blk_mq_quiesce_queue(ctrl->admin_q);
+	else
+		blk_mq_wait_quiesce_done(ctrl->admin_q);
 }
 EXPORT_SYMBOL_GPL(nvme_stop_admin_queue);
 
-- 
2.31.1


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH 0/4] block: fix concurrent quiesce
  2021-11-03  3:43 [PATCH 0/4] block: fix concurrent quiesce Ming Lei
                   ` (3 preceding siblings ...)
  2021-11-03  3:43 ` [PATCH 4/4] nvme: wait until quiesce is done Ming Lei
@ 2021-11-07 21:20 ` Jens Axboe
  4 siblings, 0 replies; 12+ messages in thread
From: Jens Axboe @ 2021-11-07 21:20 UTC (permalink / raw)
  To: Ming Lei
  Cc: Yi Zhang, linux-block, linux-nvme, Martin K . Petersen,
	linux-scsi, James Bottomley

On 11/2/21 9:43 PM, Ming Lei wrote:
> Hi Jens,
> 
> Convert SCSI into balanced quiesce and unquiesce by using atomic
> variable as suggested by James, meantime fix previous nvme conversion by
> adding one new API because we have to wait until the started quiesce is
> done.
> 
> 
> Ming Lei (4):
>   blk-mq: add one API for waiting until quiesce is done
>   scsi: avoid to quiesce sdev->request_queue two times
>   scsi: make sure that request queue queiesce and unquiesce balanced
>   nvme: wait until quiesce is done
> 
>  block/blk-mq.c             | 28 +++++++++++++------
>  drivers/nvme/host/core.c   |  4 +++
>  drivers/scsi/scsi_lib.c    | 55 +++++++++++++++++++++++---------------
>  include/linux/blk-mq.h     |  1 +
>  include/scsi/scsi_device.h |  1 +
>  5 files changed, 59 insertions(+), 30 deletions(-)

James/Martin, are you find with the SCSI side? Would make queueing this
up easier...

-- 
Jens Axboe


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 3/4] scsi: make sure that request queue queiesce and unquiesce balanced
  2021-11-03  3:43 ` [PATCH 3/4] scsi: make sure that request queue queiesce and unquiesce balanced Ming Lei
@ 2021-11-08 16:42   ` James Bottomley
  2021-11-09  0:44     ` Ming Lei
  0 siblings, 1 reply; 12+ messages in thread
From: James Bottomley @ 2021-11-08 16:42 UTC (permalink / raw)
  To: Ming Lei, Jens Axboe
  Cc: Yi Zhang, linux-block, linux-nvme, Martin K . Petersen, linux-scsi

On Wed, 2021-11-03 at 11:43 +0800, Ming Lei wrote:
[...]
> +void scsi_start_queue(struct scsi_device *sdev)
> +{
> +	if (cmpxchg(&sdev->queue_stopped, 1, 0))
> +		blk_mq_unquiesce_queue(sdev->request_queue);
> +}
> +
> +static void scsi_stop_queue(struct scsi_device *sdev, bool nowait)
> +{
> +	if (!cmpxchg(&sdev->queue_stopped, 0, 1)) {
> +		if (nowait)
> +			blk_mq_quiesce_queue_nowait(sdev-
> >request_queue);
> +		else
> +			blk_mq_quiesce_queue(sdev->request_queue);
> +	} else {
> +		if (!nowait)
> +			blk_mq_wait_quiesce_done(sdev->request_queue);
> +	}
> +}

This looks counter intuitive.  I assume it's done so that if we call
scsi_stop_queue when the queue has already been stopped, it waits until
the queue is actually quiesced before returning so the behaviour is the
same in the !nowait case?  Some sort of comment explaining that would
be useful.

James




^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 4/4] nvme: wait until quiesce is done
  2021-11-03  3:43 ` [PATCH 4/4] nvme: wait until quiesce is done Ming Lei
@ 2021-11-08 16:45   ` Keith Busch
  2021-11-12 15:38   ` Sagi Grimberg
  1 sibling, 0 replies; 12+ messages in thread
From: Keith Busch @ 2021-11-08 16:45 UTC (permalink / raw)
  To: Ming Lei
  Cc: Jens Axboe, Yi Zhang, linux-block, linux-nvme,
	Martin K . Petersen, linux-scsi, James Bottomley

On Wed, Nov 03, 2021 at 11:43:05AM +0800, Ming Lei wrote:
> NVMe uses one atomic flag to check if quiesce is needed. If quiesce is
> started, the helper returns immediately. This way is wrong, since we
> have to wait until quiesce is done.

Looks good to me.

Reviewed-by: Keith Busch <kbusch@kernel.org>

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 3/4] scsi: make sure that request queue queiesce and unquiesce balanced
  2021-11-08 16:42   ` James Bottomley
@ 2021-11-09  0:44     ` Ming Lei
  2021-11-09  3:18       ` Ming Lei
  0 siblings, 1 reply; 12+ messages in thread
From: Ming Lei @ 2021-11-09  0:44 UTC (permalink / raw)
  To: James Bottomley
  Cc: Jens Axboe, Yi Zhang, linux-block, linux-nvme,
	Martin K . Petersen, linux-scsi, ming.lei

Hello James,

On Mon, Nov 08, 2021 at 11:42:01AM -0500, James Bottomley wrote:
> On Wed, 2021-11-03 at 11:43 +0800, Ming Lei wrote:
> [...]
> > +void scsi_start_queue(struct scsi_device *sdev)
> > +{
> > +	if (cmpxchg(&sdev->queue_stopped, 1, 0))
> > +		blk_mq_unquiesce_queue(sdev->request_queue);
> > +}
> > +
> > +static void scsi_stop_queue(struct scsi_device *sdev, bool nowait)
> > +{
> > +	if (!cmpxchg(&sdev->queue_stopped, 0, 1)) {
> > +		if (nowait)
> > +			blk_mq_quiesce_queue_nowait(sdev-
> > >request_queue);
> > +		else
> > +			blk_mq_quiesce_queue(sdev->request_queue);
> > +	} else {
> > +		if (!nowait)
> > +			blk_mq_wait_quiesce_done(sdev->request_queue);
> > +	}
> > +}
> 
> This looks counter intuitive.  I assume it's done so that if we call
> scsi_stop_queue when the queue has already been stopped, it waits until

The motivation is to balance blk_mq_quiesce_queue_nowait()/blk_mq_quiesce_queue()
and blk_mq_unquiesce_queue().

That needs one extra mutex to cover the quiesce action and update
the flag, but we can't hold the mutex in scsi_internal_device_block_nowait(),
so take this way with the atomic flag.

> the queue is actually quiesced before returning so the behaviour is the
> same in the !nowait case?  Some sort of comment explaining that would
> be useful.

I will add comment on the current usage.


Thanks,
Ming


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 3/4] scsi: make sure that request queue queiesce and unquiesce balanced
  2021-11-09  0:44     ` Ming Lei
@ 2021-11-09  3:18       ` Ming Lei
  2021-11-09  3:22         ` James Bottomley
  0 siblings, 1 reply; 12+ messages in thread
From: Ming Lei @ 2021-11-09  3:18 UTC (permalink / raw)
  To: James Bottomley
  Cc: Jens Axboe, Yi Zhang, linux-block, linux-nvme,
	Martin K . Petersen, linux-scsi

Hello James,

On Tue, Nov 09, 2021 at 08:44:06AM +0800, Ming Lei wrote:
> Hello James,
> 
> On Mon, Nov 08, 2021 at 11:42:01AM -0500, James Bottomley wrote:
> > On Wed, 2021-11-03 at 11:43 +0800, Ming Lei wrote:
> > [...]
> > > +void scsi_start_queue(struct scsi_device *sdev)
> > > +{
> > > +	if (cmpxchg(&sdev->queue_stopped, 1, 0))
> > > +		blk_mq_unquiesce_queue(sdev->request_queue);
> > > +}
> > > +
> > > +static void scsi_stop_queue(struct scsi_device *sdev, bool nowait)
> > > +{
> > > +	if (!cmpxchg(&sdev->queue_stopped, 0, 1)) {
> > > +		if (nowait)
> > > +			blk_mq_quiesce_queue_nowait(sdev-
> > > >request_queue);
> > > +		else
> > > +			blk_mq_quiesce_queue(sdev->request_queue);
> > > +	} else {
> > > +		if (!nowait)
> > > +			blk_mq_wait_quiesce_done(sdev->request_queue);
> > > +	}
> > > +}
> > 
> > This looks counter intuitive.  I assume it's done so that if we call
> > scsi_stop_queue when the queue has already been stopped, it waits until
> 
> The motivation is to balance blk_mq_quiesce_queue_nowait()/blk_mq_quiesce_queue()
> and blk_mq_unquiesce_queue().
> 
> That needs one extra mutex to cover the quiesce action and update
> the flag, but we can't hold the mutex in scsi_internal_device_block_nowait(),
> so take this way with the atomic flag.
> 
> > the queue is actually quiesced before returning so the behaviour is the
> > same in the !nowait case?  Some sort of comment explaining that would
> > be useful.
> 
> I will add comment on the current usage.

Are you fine with the following comment?

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index e8925a35cb3a..9e3bf028f95a 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -2661,6 +2661,13 @@ void scsi_start_queue(struct scsi_device *sdev)
 
 static void scsi_stop_queue(struct scsi_device *sdev, bool nowait)
 {
+	/*
+	 * The atomic variable of ->queue_stopped covers that
+	 * blk_mq_quiesce_queue* is balanced with blk_mq_unquiesce_queue.
+	 *
+	 * However, we still need to wait until quiesce is done
+	 * in case that queue has been stopped.
+	 */
 	if (!cmpxchg(&sdev->queue_stopped, 0, 1)) {
 		if (nowait)
 			blk_mq_quiesce_queue_nowait(sdev->request_queue);


Thanks,
Ming


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH 3/4] scsi: make sure that request queue queiesce and unquiesce balanced
  2021-11-09  3:18       ` Ming Lei
@ 2021-11-09  3:22         ` James Bottomley
  0 siblings, 0 replies; 12+ messages in thread
From: James Bottomley @ 2021-11-09  3:22 UTC (permalink / raw)
  To: Ming Lei
  Cc: Jens Axboe, Yi Zhang, linux-block, linux-nvme,
	Martin K . Petersen, linux-scsi

On Tue, 2021-11-09 at 11:18 +0800, Ming Lei wrote:
> Hello James,
> 
> On Tue, Nov 09, 2021 at 08:44:06AM +0800, Ming Lei wrote:
> > Hello James,
> > 
> > On Mon, Nov 08, 2021 at 11:42:01AM -0500, James Bottomley wrote:
> > > On Wed, 2021-11-03 at 11:43 +0800, Ming Lei wrote:
> > > [...]
> > > > +void scsi_start_queue(struct scsi_device *sdev)
> > > > +{
> > > > +	if (cmpxchg(&sdev->queue_stopped, 1, 0))
> > > > +		blk_mq_unquiesce_queue(sdev->request_queue);
> > > > +}
> > > > +
> > > > +static void scsi_stop_queue(struct scsi_device *sdev, bool
> > > > nowait)
> > > > +{
> > > > +	if (!cmpxchg(&sdev->queue_stopped, 0, 1)) {
> > > > +		if (nowait)
> > > > +			blk_mq_quiesce_queue_nowait(sdev-
> > > > > request_queue);
> > > > +		else
> > > > +			blk_mq_quiesce_queue(sdev-
> > > > >request_queue);
> > > > +	} else {
> > > > +		if (!nowait)
> > > > +			blk_mq_wait_quiesce_done(sdev-
> > > > >request_queue);
> > > > +	}
> > > > +}
> > > 
> > > This looks counter intuitive.  I assume it's done so that if we
> > > call
> > > scsi_stop_queue when the queue has already been stopped, it waits
> > > until
> > 
> > The motivation is to balance
> > blk_mq_quiesce_queue_nowait()/blk_mq_quiesce_queue()
> > and blk_mq_unquiesce_queue().
> > 
> > That needs one extra mutex to cover the quiesce action and update
> > the flag, but we can't hold the mutex in
> > scsi_internal_device_block_nowait(),
> > so take this way with the atomic flag.
> > 
> > > the queue is actually quiesced before returning so the behaviour
> > > is the
> > > same in the !nowait case?  Some sort of comment explaining that
> > > would
> > > be useful.
> > 
> > I will add comment on the current usage.
> 
> Are you fine with the following comment?
> 
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index e8925a35cb3a..9e3bf028f95a 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -2661,6 +2661,13 @@ void scsi_start_queue(struct scsi_device
> *sdev)
>  
>  static void scsi_stop_queue(struct scsi_device *sdev, bool nowait)
>  {
> +	/*
> +	 * The atomic variable of ->queue_stopped covers that
> +	 * blk_mq_quiesce_queue* is balanced with
> blk_mq_unquiesce_queue.
> +	 *
> +	 * However, we still need to wait until quiesce is done
> +	 * in case that queue has been stopped.
> +	 */
>  	if (!cmpxchg(&sdev->queue_stopped, 0, 1)) {
>  		if (nowait)
>  			blk_mq_quiesce_queue_nowait(sdev-
> >request_queue);

Yes, that looks fine ... it will at least act as a caution for
maintainers who come after us.

James



^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 4/4] nvme: wait until quiesce is done
  2021-11-03  3:43 ` [PATCH 4/4] nvme: wait until quiesce is done Ming Lei
  2021-11-08 16:45   ` Keith Busch
@ 2021-11-12 15:38   ` Sagi Grimberg
  1 sibling, 0 replies; 12+ messages in thread
From: Sagi Grimberg @ 2021-11-12 15:38 UTC (permalink / raw)
  To: Ming Lei, Jens Axboe
  Cc: Yi Zhang, linux-block, linux-nvme, Martin K . Petersen,
	linux-scsi, James Bottomley

Reviewed-by: Sagi Grimberg <sagi@grimberg.me>

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2021-11-12 15:39 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-11-03  3:43 [PATCH 0/4] block: fix concurrent quiesce Ming Lei
2021-11-03  3:43 ` [PATCH 1/4] blk-mq: add one API for waiting until quiesce is done Ming Lei
2021-11-03  3:43 ` [PATCH 2/4] scsi: avoid to quiesce sdev->request_queue two times Ming Lei
2021-11-03  3:43 ` [PATCH 3/4] scsi: make sure that request queue queiesce and unquiesce balanced Ming Lei
2021-11-08 16:42   ` James Bottomley
2021-11-09  0:44     ` Ming Lei
2021-11-09  3:18       ` Ming Lei
2021-11-09  3:22         ` James Bottomley
2021-11-03  3:43 ` [PATCH 4/4] nvme: wait until quiesce is done Ming Lei
2021-11-08 16:45   ` Keith Busch
2021-11-12 15:38   ` Sagi Grimberg
2021-11-07 21:20 ` [PATCH 0/4] block: fix concurrent quiesce Jens Axboe

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.