[dm-devel] [PATCH 0/3] block: keep quiesce & unquiesce balanced for scsi/dm

[dm-devel] [PATCH 0/3] block: keep quiesce & unquiesce balanced for scsi/dm

[Ming Lei]

Hello Jens,

Recently we merge the patch of e70feb8b3e68 ("blk-mq: support concurrent queue
quiesce/unquiesce") for fixing race between driver and block layer wrt.
queue quiesce.

Yi reported that srp/002 is broken with this patch, turns out scsi and
dm don't keep the two balanced actually.

So fix dm and scsi and make srp/002 pass again.


Ming Lei (3):
  scsi: avoid to quiesce sdev->request_queue two times
  scsi: make sure that request queue queiesce and unquiesce balanced
  dm: don't stop request queue after the dm device is suspended

 drivers/md/dm.c            | 10 ------
 drivers/scsi/scsi_lib.c    | 70 ++++++++++++++++++++++++++------------
 include/scsi/scsi_device.h |  1 +
 3 files changed, 49 insertions(+), 32 deletions(-)

-- 
2.31.1

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

[PATCH 0/3] block: keep quiesce & unquiesce balanced for scsi/dm

[Ming Lei]

Hello Jens,

Recently we merge the patch of e70feb8b3e68 ("blk-mq: support concurrent queue
quiesce/unquiesce") for fixing race between driver and block layer wrt.
queue quiesce.

Yi reported that srp/002 is broken with this patch, turns out scsi and
dm don't keep the two balanced actually.

So fix dm and scsi and make srp/002 pass again.


Ming Lei (3):
  scsi: avoid to quiesce sdev->request_queue two times
  scsi: make sure that request queue queiesce and unquiesce balanced
  dm: don't stop request queue after the dm device is suspended

 drivers/md/dm.c            | 10 ------
 drivers/scsi/scsi_lib.c    | 70 ++++++++++++++++++++++++++------------
 include/scsi/scsi_device.h |  1 +
 3 files changed, 49 insertions(+), 32 deletions(-)

-- 
2.31.1

[dm-devel] [PATCH 1/3] scsi: avoid to quiesce sdev->request_queue two times

[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 30f7d0b4eb73..51fcd46be265 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -2630,6 +2630,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
@@ -2646,24 +2654,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);
 
@@ -2684,13 +2684,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

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

[PATCH 1/3] scsi: avoid to quiesce sdev->request_queue two times

[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 30f7d0b4eb73..51fcd46be265 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -2630,6 +2630,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
@@ -2646,24 +2654,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);
 
@@ -2684,13 +2684,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

[dm-devel] [PATCH 2/3] scsi: make sure that request queue queiesce and unquiesce balanced

[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
bit flag & global spinlock, basically zero cost since request queue quiesce
is seldom triggered.

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    | 45 ++++++++++++++++++++++++++++++--------
 include/scsi/scsi_device.h |  1 +
 2 files changed, 37 insertions(+), 9 deletions(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 51fcd46be265..414f4daf8005 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -2638,6 +2638,40 @@ static int __scsi_internal_device_block_nowait(struct scsi_device *sdev)
 	return 0;
 }
 
+static DEFINE_SPINLOCK(sdev_queue_stop_lock);
+
+void scsi_start_queue(struct scsi_device *sdev)
+{
+	bool need_start;
+	unsigned long flags;
+
+	spin_lock_irqsave(&sdev_queue_stop_lock, flags);
+	need_start = sdev->queue_stopped;
+	sdev->queue_stopped = 0;
+	spin_unlock_irqrestore(&sdev_queue_stop_lock, flags);
+
+	if (need_start)
+		blk_mq_unquiesce_queue(sdev->request_queue);
+}
+
+static void scsi_stop_queue(struct scsi_device *sdev, bool nowait)
+{
+	bool need_stop;
+	unsigned long flags;
+
+	spin_lock_irqsave(&sdev_queue_stop_lock, flags);
+	need_stop = !sdev->queue_stopped;
+	sdev->queue_stopped = 1;
+	spin_unlock_irqrestore(&sdev_queue_stop_lock, flags);
+
+	if (need_stop) {
+		if (nowait)
+			blk_mq_quiesce_queue_nowait(sdev->request_queue);
+		else
+			blk_mq_quiesce_queue(sdev->request_queue);
+	}
+}
+
 /**
  * scsi_internal_device_block_nowait - try to transition to the SDEV_BLOCK state
  * @sdev: device to block
@@ -2662,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);
@@ -2689,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..ac74beccffa2 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -206,6 +206,7 @@ struct scsi_device {
 	unsigned rpm_autosuspend:1;	/* Enable runtime autosuspend at device
 					 * creation time */
 	unsigned ignore_media_change:1; /* Ignore MEDIA CHANGE on resume */
+	unsigned queue_stopped:1;	/* request queue is quiesced */
 
 	bool offline_already;		/* Device offline message logged */
 
-- 
2.31.1

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

[PATCH 2/3] scsi: make sure that request queue queiesce and unquiesce balanced

[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
bit flag & global spinlock, basically zero cost since request queue quiesce
is seldom triggered.

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    | 45 ++++++++++++++++++++++++++++++--------
 include/scsi/scsi_device.h |  1 +
 2 files changed, 37 insertions(+), 9 deletions(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 51fcd46be265..414f4daf8005 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -2638,6 +2638,40 @@ static int __scsi_internal_device_block_nowait(struct scsi_device *sdev)
 	return 0;
 }
 
+static DEFINE_SPINLOCK(sdev_queue_stop_lock);
+
+void scsi_start_queue(struct scsi_device *sdev)
+{
+	bool need_start;
+	unsigned long flags;
+
+	spin_lock_irqsave(&sdev_queue_stop_lock, flags);
+	need_start = sdev->queue_stopped;
+	sdev->queue_stopped = 0;
+	spin_unlock_irqrestore(&sdev_queue_stop_lock, flags);
+
+	if (need_start)
+		blk_mq_unquiesce_queue(sdev->request_queue);
+}
+
+static void scsi_stop_queue(struct scsi_device *sdev, bool nowait)
+{
+	bool need_stop;
+	unsigned long flags;
+
+	spin_lock_irqsave(&sdev_queue_stop_lock, flags);
+	need_stop = !sdev->queue_stopped;
+	sdev->queue_stopped = 1;
+	spin_unlock_irqrestore(&sdev_queue_stop_lock, flags);
+
+	if (need_stop) {
+		if (nowait)
+			blk_mq_quiesce_queue_nowait(sdev->request_queue);
+		else
+			blk_mq_quiesce_queue(sdev->request_queue);
+	}
+}
+
 /**
  * scsi_internal_device_block_nowait - try to transition to the SDEV_BLOCK state
  * @sdev: device to block
@@ -2662,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);
@@ -2689,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..ac74beccffa2 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -206,6 +206,7 @@ struct scsi_device {
 	unsigned rpm_autosuspend:1;	/* Enable runtime autosuspend at device
 					 * creation time */
 	unsigned ignore_media_change:1; /* Ignore MEDIA CHANGE on resume */
+	unsigned queue_stopped:1;	/* request queue is quiesced */
 
 	bool offline_already;		/* Device offline message logged */
 
-- 
2.31.1

Re: [dm-devel] [PATCH 2/3] scsi: make sure that request queue queiesce and unquiesce balanced

[James Bottomley]

On Thu, 2021-10-21 at 22:59 +0800, Ming Lei wrote:
> 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
> bit flag & global spinlock, basically zero cost since request queue
> quiesce
> is seldom triggered.
> 
> 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    | 45 ++++++++++++++++++++++++++++++----
> ----
>  include/scsi/scsi_device.h |  1 +
>  2 files changed, 37 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index 51fcd46be265..414f4daf8005 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -2638,6 +2638,40 @@ static int
> __scsi_internal_device_block_nowait(struct scsi_device *sdev)
>  	return 0;
>  }
>  
> +static DEFINE_SPINLOCK(sdev_queue_stop_lock);
> +
> +void scsi_start_queue(struct scsi_device *sdev)
> +{
> +	bool need_start;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&sdev_queue_stop_lock, flags);
> +	need_start = sdev->queue_stopped;
> +	sdev->queue_stopped = 0;
> +	spin_unlock_irqrestore(&sdev_queue_stop_lock, flags);
> +
> +	if (need_start)
> +		blk_mq_unquiesce_queue(sdev->request_queue);

Well, this is a classic atomic pattern:

if (cmpxchg(&sdev->queue_stopped, 1, 0))
	blk_mq_unquiesce_queue(sdev->request_queue);

The reason to do it with atomics rather than spinlocks is

   1. no need to disable interrupts: atomics are locked
   2. faster because a spinlock takes an exclusive line every time but the
      read to check the value can be in shared mode in cmpxchg
   3. it's just shorter and better code.

The only minor downside is queue_stopped now needs to be a u32.

James


--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [PATCH 2/3] scsi: make sure that request queue queiesce and unquiesce balanced

[James Bottomley]

On Thu, 2021-10-21 at 22:59 +0800, Ming Lei wrote:
> 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
> bit flag & global spinlock, basically zero cost since request queue
> quiesce
> is seldom triggered.
> 
> 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    | 45 ++++++++++++++++++++++++++++++----
> ----
>  include/scsi/scsi_device.h |  1 +
>  2 files changed, 37 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index 51fcd46be265..414f4daf8005 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -2638,6 +2638,40 @@ static int
> __scsi_internal_device_block_nowait(struct scsi_device *sdev)
>  	return 0;
>  }
>  
> +static DEFINE_SPINLOCK(sdev_queue_stop_lock);
> +
> +void scsi_start_queue(struct scsi_device *sdev)
> +{
> +	bool need_start;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&sdev_queue_stop_lock, flags);
> +	need_start = sdev->queue_stopped;
> +	sdev->queue_stopped = 0;
> +	spin_unlock_irqrestore(&sdev_queue_stop_lock, flags);
> +
> +	if (need_start)
> +		blk_mq_unquiesce_queue(sdev->request_queue);

Well, this is a classic atomic pattern:

if (cmpxchg(&sdev->queue_stopped, 1, 0))
	blk_mq_unquiesce_queue(sdev->request_queue);

The reason to do it with atomics rather than spinlocks is

   1. no need to disable interrupts: atomics are locked
   2. faster because a spinlock takes an exclusive line every time but the
      read to check the value can be in shared mode in cmpxchg
   3. it's just shorter and better code.

The only minor downside is queue_stopped now needs to be a u32.

James

Re: [dm-devel] [PATCH 2/3] scsi: make sure that request queue queiesce and unquiesce balanced

[Ming Lei]

Hi James,

On Mon, Nov 01, 2021 at 09:43:27PM -0400, James Bottomley wrote:
> On Thu, 2021-10-21 at 22:59 +0800, Ming Lei wrote:
> > 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
> > bit flag & global spinlock, basically zero cost since request queue
> > quiesce
> > is seldom triggered.
> > 
> > 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    | 45 ++++++++++++++++++++++++++++++----
> > ----
> >  include/scsi/scsi_device.h |  1 +
> >  2 files changed, 37 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> > index 51fcd46be265..414f4daf8005 100644
> > --- a/drivers/scsi/scsi_lib.c
> > +++ b/drivers/scsi/scsi_lib.c
> > @@ -2638,6 +2638,40 @@ static int
> > __scsi_internal_device_block_nowait(struct scsi_device *sdev)
> >  	return 0;
> >  }
> >  
> > +static DEFINE_SPINLOCK(sdev_queue_stop_lock);
> > +
> > +void scsi_start_queue(struct scsi_device *sdev)
> > +{
> > +	bool need_start;
> > +	unsigned long flags;
> > +
> > +	spin_lock_irqsave(&sdev_queue_stop_lock, flags);
> > +	need_start = sdev->queue_stopped;
> > +	sdev->queue_stopped = 0;
> > +	spin_unlock_irqrestore(&sdev_queue_stop_lock, flags);
> > +
> > +	if (need_start)
> > +		blk_mq_unquiesce_queue(sdev->request_queue);
> 
> Well, this is a classic atomic pattern:
> 
> if (cmpxchg(&sdev->queue_stopped, 1, 0))
> 	blk_mq_unquiesce_queue(sdev->request_queue);
> 
> The reason to do it with atomics rather than spinlocks is
> 
>    1. no need to disable interrupts: atomics are locked
>    2. faster because a spinlock takes an exclusive line every time but the
>       read to check the value can be in shared mode in cmpxchg
>    3. it's just shorter and better code.

You are right, I agree.

> 
> The only minor downside is queue_stopped now needs to be a u32.

Yeah, that is the reason I don't take this atomic way since it needs to
add one extra u32 into 'struct scsi_device'.


Thanks,
Ming

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [PATCH 2/3] scsi: make sure that request queue queiesce and unquiesce balanced

[Ming Lei]

Hi James,

On Mon, Nov 01, 2021 at 09:43:27PM -0400, James Bottomley wrote:
> On Thu, 2021-10-21 at 22:59 +0800, Ming Lei wrote:
> > 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
> > bit flag & global spinlock, basically zero cost since request queue
> > quiesce
> > is seldom triggered.
> > 
> > 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    | 45 ++++++++++++++++++++++++++++++----
> > ----
> >  include/scsi/scsi_device.h |  1 +
> >  2 files changed, 37 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> > index 51fcd46be265..414f4daf8005 100644
> > --- a/drivers/scsi/scsi_lib.c
> > +++ b/drivers/scsi/scsi_lib.c
> > @@ -2638,6 +2638,40 @@ static int
> > __scsi_internal_device_block_nowait(struct scsi_device *sdev)
> >  	return 0;
> >  }
> >  
> > +static DEFINE_SPINLOCK(sdev_queue_stop_lock);
> > +
> > +void scsi_start_queue(struct scsi_device *sdev)
> > +{
> > +	bool need_start;
> > +	unsigned long flags;
> > +
> > +	spin_lock_irqsave(&sdev_queue_stop_lock, flags);
> > +	need_start = sdev->queue_stopped;
> > +	sdev->queue_stopped = 0;
> > +	spin_unlock_irqrestore(&sdev_queue_stop_lock, flags);
> > +
> > +	if (need_start)
> > +		blk_mq_unquiesce_queue(sdev->request_queue);
> 
> Well, this is a classic atomic pattern:
> 
> if (cmpxchg(&sdev->queue_stopped, 1, 0))
> 	blk_mq_unquiesce_queue(sdev->request_queue);
> 
> The reason to do it with atomics rather than spinlocks is
> 
>    1. no need to disable interrupts: atomics are locked
>    2. faster because a spinlock takes an exclusive line every time but the
>       read to check the value can be in shared mode in cmpxchg
>    3. it's just shorter and better code.

You are right, I agree.

> 
> The only minor downside is queue_stopped now needs to be a u32.

Yeah, that is the reason I don't take this atomic way since it needs to
add one extra u32 into 'struct scsi_device'.


Thanks,
Ming

Re: [dm-devel] [PATCH 2/3] scsi: make sure that request queue queiesce and unquiesce balanced

[Jens Axboe]

On 11/1/21 7:43 PM, James Bottomley wrote:
> On Thu, 2021-10-21 at 22:59 +0800, Ming Lei wrote:
>> 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
>> bit flag & global spinlock, basically zero cost since request queue
>> quiesce
>> is seldom triggered.
>>
>> 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    | 45 ++++++++++++++++++++++++++++++----
>> ----
>>  include/scsi/scsi_device.h |  1 +
>>  2 files changed, 37 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
>> index 51fcd46be265..414f4daf8005 100644
>> --- a/drivers/scsi/scsi_lib.c
>> +++ b/drivers/scsi/scsi_lib.c
>> @@ -2638,6 +2638,40 @@ static int
>> __scsi_internal_device_block_nowait(struct scsi_device *sdev)
>>  	return 0;
>>  }
>>  
>> +static DEFINE_SPINLOCK(sdev_queue_stop_lock);
>> +
>> +void scsi_start_queue(struct scsi_device *sdev)
>> +{
>> +	bool need_start;
>> +	unsigned long flags;
>> +
>> +	spin_lock_irqsave(&sdev_queue_stop_lock, flags);
>> +	need_start = sdev->queue_stopped;
>> +	sdev->queue_stopped = 0;
>> +	spin_unlock_irqrestore(&sdev_queue_stop_lock, flags);
>> +
>> +	if (need_start)
>> +		blk_mq_unquiesce_queue(sdev->request_queue);
> 
> Well, this is a classic atomic pattern:
> 
> if (cmpxchg(&sdev->queue_stopped, 1, 0))
> 	blk_mq_unquiesce_queue(sdev->request_queue);
> 
> The reason to do it with atomics rather than spinlocks is
> 
>    1. no need to disable interrupts: atomics are locked
>    2. faster because a spinlock takes an exclusive line every time but the
>       read to check the value can be in shared mode in cmpxchg
>    3. it's just shorter and better code.
> 
> The only minor downside is queue_stopped now needs to be a u32.

Are you fine with the change as-is, or do you want it redone? I
can drop the SCSI parts and just queue up the dm fix. Personally
I think it'd be better to get it fixed upfront.

-- 
Jens Axboe

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [PATCH 2/3] scsi: make sure that request queue queiesce and unquiesce balanced

[Jens Axboe]

On 11/1/21 7:43 PM, James Bottomley wrote:
> On Thu, 2021-10-21 at 22:59 +0800, Ming Lei wrote:
>> 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
>> bit flag & global spinlock, basically zero cost since request queue
>> quiesce
>> is seldom triggered.
>>
>> 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    | 45 ++++++++++++++++++++++++++++++----
>> ----
>>  include/scsi/scsi_device.h |  1 +
>>  2 files changed, 37 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
>> index 51fcd46be265..414f4daf8005 100644
>> --- a/drivers/scsi/scsi_lib.c
>> +++ b/drivers/scsi/scsi_lib.c
>> @@ -2638,6 +2638,40 @@ static int
>> __scsi_internal_device_block_nowait(struct scsi_device *sdev)
>>  	return 0;
>>  }
>>  
>> +static DEFINE_SPINLOCK(sdev_queue_stop_lock);
>> +
>> +void scsi_start_queue(struct scsi_device *sdev)
>> +{
>> +	bool need_start;
>> +	unsigned long flags;
>> +
>> +	spin_lock_irqsave(&sdev_queue_stop_lock, flags);
>> +	need_start = sdev->queue_stopped;
>> +	sdev->queue_stopped = 0;
>> +	spin_unlock_irqrestore(&sdev_queue_stop_lock, flags);
>> +
>> +	if (need_start)
>> +		blk_mq_unquiesce_queue(sdev->request_queue);
> 
> Well, this is a classic atomic pattern:
> 
> if (cmpxchg(&sdev->queue_stopped, 1, 0))
> 	blk_mq_unquiesce_queue(sdev->request_queue);
> 
> The reason to do it with atomics rather than spinlocks is
> 
>    1. no need to disable interrupts: atomics are locked
>    2. faster because a spinlock takes an exclusive line every time but the
>       read to check the value can be in shared mode in cmpxchg
>    3. it's just shorter and better code.
> 
> The only minor downside is queue_stopped now needs to be a u32.

Are you fine with the change as-is, or do you want it redone? I
can drop the SCSI parts and just queue up the dm fix. Personally
I think it'd be better to get it fixed upfront.

-- 
Jens Axboe

Re: [dm-devel] [PATCH 2/3] scsi: make sure that request queue queiesce and unquiesce balanced

[James Bottomley]

On Tue, 2021-11-02 at 06:59 -0600, Jens Axboe wrote:
> On 11/1/21 7:43 PM, James Bottomley wrote:
> > On Thu, 2021-10-21 at 22:59 +0800, Ming Lei wrote:
> > > 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 bit flag & global spinlock, basically zero
> > > cost since request queue quiesce is seldom triggered.
> > > 
> > > 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    | 45 ++++++++++++++++++++++++++++++
> > > ----
> > > ----
> > >  include/scsi/scsi_device.h |  1 +
> > >  2 files changed, 37 insertions(+), 9 deletions(-)
> > > 
> > > diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> > > index 51fcd46be265..414f4daf8005 100644
> > > --- a/drivers/scsi/scsi_lib.c
> > > +++ b/drivers/scsi/scsi_lib.c
> > > @@ -2638,6 +2638,40 @@ static int
> > > __scsi_internal_device_block_nowait(struct scsi_device *sdev)
> > >  	return 0;
> > >  }
> > >  
> > > +static DEFINE_SPINLOCK(sdev_queue_stop_lock);
> > > +
> > > +void scsi_start_queue(struct scsi_device *sdev)
> > > +{
> > > +	bool need_start;
> > > +	unsigned long flags;
> > > +
> > > +	spin_lock_irqsave(&sdev_queue_stop_lock, flags);
> > > +	need_start = sdev->queue_stopped;
> > > +	sdev->queue_stopped = 0;
> > > +	spin_unlock_irqrestore(&sdev_queue_stop_lock, flags);
> > > +
> > > +	if (need_start)
> > > +		blk_mq_unquiesce_queue(sdev->request_queue);
> > 
> > Well, this is a classic atomic pattern:
> > 
> > if (cmpxchg(&sdev->queue_stopped, 1, 0))
> > 	blk_mq_unquiesce_queue(sdev->request_queue);
> > 
> > The reason to do it with atomics rather than spinlocks is
> > 
> >    1. no need to disable interrupts: atomics are locked
> >    2. faster because a spinlock takes an exclusive line every time
> > but the
> >       read to check the value can be in shared mode in cmpxchg
> >    3. it's just shorter and better code.
> > 
> > The only minor downside is queue_stopped now needs to be a u32.
> 
> Are you fine with the change as-is, or do you want it redone? I
> can drop the SCSI parts and just queue up the dm fix. Personally
> I think it'd be better to get it fixed upfront.

Well, given the path isn't hot, I don't really care.  However, what I
don't want is to have to continually bat back patches from the make
work code churners trying to update this code for being the wrong
pattern.  I think at the very least it needs a comment saying why we
chose a suboptimal pattern to try to forestall this.

James


--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [PATCH 2/3] scsi: make sure that request queue queiesce and unquiesce balanced

[James Bottomley]

On Tue, 2021-11-02 at 06:59 -0600, Jens Axboe wrote:
> On 11/1/21 7:43 PM, James Bottomley wrote:
> > On Thu, 2021-10-21 at 22:59 +0800, Ming Lei wrote:
> > > 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 bit flag & global spinlock, basically zero
> > > cost since request queue quiesce is seldom triggered.
> > > 
> > > 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    | 45 ++++++++++++++++++++++++++++++
> > > ----
> > > ----
> > >  include/scsi/scsi_device.h |  1 +
> > >  2 files changed, 37 insertions(+), 9 deletions(-)
> > > 
> > > diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> > > index 51fcd46be265..414f4daf8005 100644
> > > --- a/drivers/scsi/scsi_lib.c
> > > +++ b/drivers/scsi/scsi_lib.c
> > > @@ -2638,6 +2638,40 @@ static int
> > > __scsi_internal_device_block_nowait(struct scsi_device *sdev)
> > >  	return 0;
> > >  }
> > >  
> > > +static DEFINE_SPINLOCK(sdev_queue_stop_lock);
> > > +
> > > +void scsi_start_queue(struct scsi_device *sdev)
> > > +{
> > > +	bool need_start;
> > > +	unsigned long flags;
> > > +
> > > +	spin_lock_irqsave(&sdev_queue_stop_lock, flags);
> > > +	need_start = sdev->queue_stopped;
> > > +	sdev->queue_stopped = 0;
> > > +	spin_unlock_irqrestore(&sdev_queue_stop_lock, flags);
> > > +
> > > +	if (need_start)
> > > +		blk_mq_unquiesce_queue(sdev->request_queue);
> > 
> > Well, this is a classic atomic pattern:
> > 
> > if (cmpxchg(&sdev->queue_stopped, 1, 0))
> > 	blk_mq_unquiesce_queue(sdev->request_queue);
> > 
> > The reason to do it with atomics rather than spinlocks is
> > 
> >    1. no need to disable interrupts: atomics are locked
> >    2. faster because a spinlock takes an exclusive line every time
> > but the
> >       read to check the value can be in shared mode in cmpxchg
> >    3. it's just shorter and better code.
> > 
> > The only minor downside is queue_stopped now needs to be a u32.
> 
> Are you fine with the change as-is, or do you want it redone? I
> can drop the SCSI parts and just queue up the dm fix. Personally
> I think it'd be better to get it fixed upfront.

Well, given the path isn't hot, I don't really care.  However, what I
don't want is to have to continually bat back patches from the make
work code churners trying to update this code for being the wrong
pattern.  I think at the very least it needs a comment saying why we
chose a suboptimal pattern to try to forestall this.

James

Re: [dm-devel] [PATCH 2/3] scsi: make sure that request queue queiesce and unquiesce balanced

[Jens Axboe]

On 11/2/21 8:33 AM, James Bottomley wrote:
> On Tue, 2021-11-02 at 06:59 -0600, Jens Axboe wrote:
>> On 11/1/21 7:43 PM, James Bottomley wrote:
>>> On Thu, 2021-10-21 at 22:59 +0800, Ming Lei wrote:
>>>> 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 bit flag & global spinlock, basically zero
>>>> cost since request queue quiesce is seldom triggered.
>>>>
>>>> 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    | 45 ++++++++++++++++++++++++++++++
>>>> ----
>>>> ----
>>>>  include/scsi/scsi_device.h |  1 +
>>>>  2 files changed, 37 insertions(+), 9 deletions(-)
>>>>
>>>> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
>>>> index 51fcd46be265..414f4daf8005 100644
>>>> --- a/drivers/scsi/scsi_lib.c
>>>> +++ b/drivers/scsi/scsi_lib.c
>>>> @@ -2638,6 +2638,40 @@ static int
>>>> __scsi_internal_device_block_nowait(struct scsi_device *sdev)
>>>>  	return 0;
>>>>  }
>>>>  
>>>> +static DEFINE_SPINLOCK(sdev_queue_stop_lock);
>>>> +
>>>> +void scsi_start_queue(struct scsi_device *sdev)
>>>> +{
>>>> +	bool need_start;
>>>> +	unsigned long flags;
>>>> +
>>>> +	spin_lock_irqsave(&sdev_queue_stop_lock, flags);
>>>> +	need_start = sdev->queue_stopped;
>>>> +	sdev->queue_stopped = 0;
>>>> +	spin_unlock_irqrestore(&sdev_queue_stop_lock, flags);
>>>> +
>>>> +	if (need_start)
>>>> +		blk_mq_unquiesce_queue(sdev->request_queue);
>>>
>>> Well, this is a classic atomic pattern:
>>>
>>> if (cmpxchg(&sdev->queue_stopped, 1, 0))
>>> 	blk_mq_unquiesce_queue(sdev->request_queue);
>>>
>>> The reason to do it with atomics rather than spinlocks is
>>>
>>>    1. no need to disable interrupts: atomics are locked
>>>    2. faster because a spinlock takes an exclusive line every time
>>> but the
>>>       read to check the value can be in shared mode in cmpxchg
>>>    3. it's just shorter and better code.
>>>
>>> The only minor downside is queue_stopped now needs to be a u32.
>>
>> Are you fine with the change as-is, or do you want it redone? I
>> can drop the SCSI parts and just queue up the dm fix. Personally
>> I think it'd be better to get it fixed upfront.
> 
> Well, given the path isn't hot, I don't really care.  However, what I
> don't want is to have to continually bat back patches from the make
> work code churners trying to update this code for being the wrong
> pattern.  I think at the very least it needs a comment saying why we
> chose a suboptimal pattern to try to forestall this.

Right, with a comment it's probably better. And as you said, since it's
not a hot path, don't think we'd be revisiting it anyway.

I'll amend the patch with a comment.

-- 
Jens Axboe

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [PATCH 2/3] scsi: make sure that request queue queiesce and unquiesce balanced

[Jens Axboe]

On 11/2/21 8:33 AM, James Bottomley wrote:
> On Tue, 2021-11-02 at 06:59 -0600, Jens Axboe wrote:
>> On 11/1/21 7:43 PM, James Bottomley wrote:
>>> On Thu, 2021-10-21 at 22:59 +0800, Ming Lei wrote:
>>>> 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 bit flag & global spinlock, basically zero
>>>> cost since request queue quiesce is seldom triggered.
>>>>
>>>> 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    | 45 ++++++++++++++++++++++++++++++
>>>> ----
>>>> ----
>>>>  include/scsi/scsi_device.h |  1 +
>>>>  2 files changed, 37 insertions(+), 9 deletions(-)
>>>>
>>>> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
>>>> index 51fcd46be265..414f4daf8005 100644
>>>> --- a/drivers/scsi/scsi_lib.c
>>>> +++ b/drivers/scsi/scsi_lib.c
>>>> @@ -2638,6 +2638,40 @@ static int
>>>> __scsi_internal_device_block_nowait(struct scsi_device *sdev)
>>>>  	return 0;
>>>>  }
>>>>  
>>>> +static DEFINE_SPINLOCK(sdev_queue_stop_lock);
>>>> +
>>>> +void scsi_start_queue(struct scsi_device *sdev)
>>>> +{
>>>> +	bool need_start;
>>>> +	unsigned long flags;
>>>> +
>>>> +	spin_lock_irqsave(&sdev_queue_stop_lock, flags);
>>>> +	need_start = sdev->queue_stopped;
>>>> +	sdev->queue_stopped = 0;
>>>> +	spin_unlock_irqrestore(&sdev_queue_stop_lock, flags);
>>>> +
>>>> +	if (need_start)
>>>> +		blk_mq_unquiesce_queue(sdev->request_queue);
>>>
>>> Well, this is a classic atomic pattern:
>>>
>>> if (cmpxchg(&sdev->queue_stopped, 1, 0))
>>> 	blk_mq_unquiesce_queue(sdev->request_queue);
>>>
>>> The reason to do it with atomics rather than spinlocks is
>>>
>>>    1. no need to disable interrupts: atomics are locked
>>>    2. faster because a spinlock takes an exclusive line every time
>>> but the
>>>       read to check the value can be in shared mode in cmpxchg
>>>    3. it's just shorter and better code.
>>>
>>> The only minor downside is queue_stopped now needs to be a u32.
>>
>> Are you fine with the change as-is, or do you want it redone? I
>> can drop the SCSI parts and just queue up the dm fix. Personally
>> I think it'd be better to get it fixed upfront.
> 
> Well, given the path isn't hot, I don't really care.  However, what I
> don't want is to have to continually bat back patches from the make
> work code churners trying to update this code for being the wrong
> pattern.  I think at the very least it needs a comment saying why we
> chose a suboptimal pattern to try to forestall this.

Right, with a comment it's probably better. And as you said, since it's
not a hot path, don't think we'd be revisiting it anyway.

I'll amend the patch with a comment.

-- 
Jens Axboe

Re: [dm-devel] [PATCH 2/3] scsi: make sure that request queue queiesce and unquiesce balanced

[Jens Axboe]

On 11/2/21 8:36 AM, Jens Axboe wrote:
> On 11/2/21 8:33 AM, James Bottomley wrote:
>> On Tue, 2021-11-02 at 06:59 -0600, Jens Axboe wrote:
>>> On 11/1/21 7:43 PM, James Bottomley wrote:
>>>> On Thu, 2021-10-21 at 22:59 +0800, Ming Lei wrote:
>>>>> 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 bit flag & global spinlock, basically zero
>>>>> cost since request queue quiesce is seldom triggered.
>>>>>
>>>>> 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    | 45 ++++++++++++++++++++++++++++++
>>>>> ----
>>>>> ----
>>>>>  include/scsi/scsi_device.h |  1 +
>>>>>  2 files changed, 37 insertions(+), 9 deletions(-)
>>>>>
>>>>> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
>>>>> index 51fcd46be265..414f4daf8005 100644
>>>>> --- a/drivers/scsi/scsi_lib.c
>>>>> +++ b/drivers/scsi/scsi_lib.c
>>>>> @@ -2638,6 +2638,40 @@ static int
>>>>> __scsi_internal_device_block_nowait(struct scsi_device *sdev)
>>>>>  	return 0;
>>>>>  }
>>>>>  
>>>>> +static DEFINE_SPINLOCK(sdev_queue_stop_lock);
>>>>> +
>>>>> +void scsi_start_queue(struct scsi_device *sdev)
>>>>> +{
>>>>> +	bool need_start;
>>>>> +	unsigned long flags;
>>>>> +
>>>>> +	spin_lock_irqsave(&sdev_queue_stop_lock, flags);
>>>>> +	need_start = sdev->queue_stopped;
>>>>> +	sdev->queue_stopped = 0;
>>>>> +	spin_unlock_irqrestore(&sdev_queue_stop_lock, flags);
>>>>> +
>>>>> +	if (need_start)
>>>>> +		blk_mq_unquiesce_queue(sdev->request_queue);
>>>>
>>>> Well, this is a classic atomic pattern:
>>>>
>>>> if (cmpxchg(&sdev->queue_stopped, 1, 0))
>>>> 	blk_mq_unquiesce_queue(sdev->request_queue);
>>>>
>>>> The reason to do it with atomics rather than spinlocks is
>>>>
>>>>    1. no need to disable interrupts: atomics are locked
>>>>    2. faster because a spinlock takes an exclusive line every time
>>>> but the
>>>>       read to check the value can be in shared mode in cmpxchg
>>>>    3. it's just shorter and better code.
>>>>
>>>> The only minor downside is queue_stopped now needs to be a u32.
>>>
>>> Are you fine with the change as-is, or do you want it redone? I
>>> can drop the SCSI parts and just queue up the dm fix. Personally
>>> I think it'd be better to get it fixed upfront.
>>
>> Well, given the path isn't hot, I don't really care.  However, what I
>> don't want is to have to continually bat back patches from the make
>> work code churners trying to update this code for being the wrong
>> pattern.  I think at the very least it needs a comment saying why we
>> chose a suboptimal pattern to try to forestall this.
> 
> Right, with a comment it's probably better. And as you said, since it's
> not a hot path, don't think we'd be revisiting it anyway.
> 
> I'll amend the patch with a comment.

I started adding the comment and took another look at this, and that
made me change my mind. We really should make this a cmpxcgh, it's not
even using a device lock here.

I've dropped the two SCSI patches for now, Ming can you resend? If James
agrees, I really think queue_stopped should just have the type changed
and the patch redone with that using cmpxcgh().

-- 
Jens Axboe

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [PATCH 2/3] scsi: make sure that request queue queiesce and unquiesce balanced

[Jens Axboe]

On 11/2/21 8:36 AM, Jens Axboe wrote:
> On 11/2/21 8:33 AM, James Bottomley wrote:
>> On Tue, 2021-11-02 at 06:59 -0600, Jens Axboe wrote:
>>> On 11/1/21 7:43 PM, James Bottomley wrote:
>>>> On Thu, 2021-10-21 at 22:59 +0800, Ming Lei wrote:
>>>>> 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 bit flag & global spinlock, basically zero
>>>>> cost since request queue quiesce is seldom triggered.
>>>>>
>>>>> 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    | 45 ++++++++++++++++++++++++++++++
>>>>> ----
>>>>> ----
>>>>>  include/scsi/scsi_device.h |  1 +
>>>>>  2 files changed, 37 insertions(+), 9 deletions(-)
>>>>>
>>>>> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
>>>>> index 51fcd46be265..414f4daf8005 100644
>>>>> --- a/drivers/scsi/scsi_lib.c
>>>>> +++ b/drivers/scsi/scsi_lib.c
>>>>> @@ -2638,6 +2638,40 @@ static int
>>>>> __scsi_internal_device_block_nowait(struct scsi_device *sdev)
>>>>>  	return 0;
>>>>>  }
>>>>>  
>>>>> +static DEFINE_SPINLOCK(sdev_queue_stop_lock);
>>>>> +
>>>>> +void scsi_start_queue(struct scsi_device *sdev)
>>>>> +{
>>>>> +	bool need_start;
>>>>> +	unsigned long flags;
>>>>> +
>>>>> +	spin_lock_irqsave(&sdev_queue_stop_lock, flags);
>>>>> +	need_start = sdev->queue_stopped;
>>>>> +	sdev->queue_stopped = 0;
>>>>> +	spin_unlock_irqrestore(&sdev_queue_stop_lock, flags);
>>>>> +
>>>>> +	if (need_start)
>>>>> +		blk_mq_unquiesce_queue(sdev->request_queue);
>>>>
>>>> Well, this is a classic atomic pattern:
>>>>
>>>> if (cmpxchg(&sdev->queue_stopped, 1, 0))
>>>> 	blk_mq_unquiesce_queue(sdev->request_queue);
>>>>
>>>> The reason to do it with atomics rather than spinlocks is
>>>>
>>>>    1. no need to disable interrupts: atomics are locked
>>>>    2. faster because a spinlock takes an exclusive line every time
>>>> but the
>>>>       read to check the value can be in shared mode in cmpxchg
>>>>    3. it's just shorter and better code.
>>>>
>>>> The only minor downside is queue_stopped now needs to be a u32.
>>>
>>> Are you fine with the change as-is, or do you want it redone? I
>>> can drop the SCSI parts and just queue up the dm fix. Personally
>>> I think it'd be better to get it fixed upfront.
>>
>> Well, given the path isn't hot, I don't really care.  However, what I
>> don't want is to have to continually bat back patches from the make
>> work code churners trying to update this code for being the wrong
>> pattern.  I think at the very least it needs a comment saying why we
>> chose a suboptimal pattern to try to forestall this.
> 
> Right, with a comment it's probably better. And as you said, since it's
> not a hot path, don't think we'd be revisiting it anyway.
> 
> I'll amend the patch with a comment.

I started adding the comment and took another look at this, and that
made me change my mind. We really should make this a cmpxcgh, it's not
even using a device lock here.

I've dropped the two SCSI patches for now, Ming can you resend? If James
agrees, I really think queue_stopped should just have the type changed
and the patch redone with that using cmpxcgh().

-- 
Jens Axboe

Re: [dm-devel] [PATCH 2/3] scsi: make sure that request queue queiesce and unquiesce balanced

[James Bottomley]

On Tue, 2021-11-02 at 08:41 -0600, Jens Axboe wrote:
> On 11/2/21 8:36 AM, Jens Axboe wrote:
> > On 11/2/21 8:33 AM, James Bottomley wrote:
> > > On Tue, 2021-11-02 at 06:59 -0600, Jens Axboe wrote:
> > > > On 11/1/21 7:43 PM, James Bottomley wrote:
> > > > > On Thu, 2021-10-21 at 22:59 +0800, Ming Lei wrote:
> > > > > > 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 bit flag & global spinlock,
> > > > > > basically zero cost since request queue quiesce is seldom
> > > > > > triggered.
> > > > > > 
> > > > > > 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    | 45
> > > > > > ++++++++++++++++++++++++++++++
> > > > > > ----
> > > > > > ----
> > > > > >  include/scsi/scsi_device.h |  1 +
> > > > > >  2 files changed, 37 insertions(+), 9 deletions(-)
> > > > > > 
> > > > > > diff --git a/drivers/scsi/scsi_lib.c
> > > > > > b/drivers/scsi/scsi_lib.c
> > > > > > index 51fcd46be265..414f4daf8005 100644
> > > > > > --- a/drivers/scsi/scsi_lib.c
> > > > > > +++ b/drivers/scsi/scsi_lib.c
> > > > > > @@ -2638,6 +2638,40 @@ static int
> > > > > > __scsi_internal_device_block_nowait(struct scsi_device
> > > > > > *sdev)
> > > > > >  	return 0;
> > > > > >  }
> > > > > >  
> > > > > > +static DEFINE_SPINLOCK(sdev_queue_stop_lock);
> > > > > > +
> > > > > > +void scsi_start_queue(struct scsi_device *sdev)
> > > > > > +{
> > > > > > +	bool need_start;
> > > > > > +	unsigned long flags;
> > > > > > +
> > > > > > +	spin_lock_irqsave(&sdev_queue_stop_lock, flags);
> > > > > > +	need_start = sdev->queue_stopped;
> > > > > > +	sdev->queue_stopped = 0;
> > > > > > +	spin_unlock_irqrestore(&sdev_queue_stop_lock, flags);
> > > > > > +
> > > > > > +	if (need_start)
> > > > > > +		blk_mq_unquiesce_queue(sdev->request_queue);
> > > > > 
> > > > > Well, this is a classic atomic pattern:
> > > > > 
> > > > > if (cmpxchg(&sdev->queue_stopped, 1, 0))
> > > > > 	blk_mq_unquiesce_queue(sdev->request_queue);
> > > > > 
> > > > > The reason to do it with atomics rather than spinlocks is
> > > > > 
> > > > >    1. no need to disable interrupts: atomics are locked
> > > > >    2. faster because a spinlock takes an exclusive line every
> > > > > time but the
> > > > >       read to check the value can be in shared mode in
> > > > > cmpxchg
> > > > >    3. it's just shorter and better code.
> > > > > 
> > > > > The only minor downside is queue_stopped now needs to be a
> > > > > u32.
> > > > 
> > > > Are you fine with the change as-is, or do you want it redone? I
> > > > can drop the SCSI parts and just queue up the dm fix.
> > > > Personally I think it'd be better to get it fixed upfront.
> > > 
> > > Well, given the path isn't hot, I don't really care.  However,
> > > what I don't want is to have to continually bat back patches from
> > > the make work code churners trying to update this code for being
> > > the wrong pattern.  I think at the very least it needs a comment
> > > saying why we chose a suboptimal pattern to try to forestall
> > > this.
> > 
> > Right, with a comment it's probably better. And as you said, since
> > it's not a hot path, don't think we'd be revisiting it anyway.
> > 
> > I'll amend the patch with a comment.
> 
> I started adding the comment and took another look at this, and that
> made me change my mind. We really should make this a cmpxcgh, it's
> not even using a device lock here.
> 
> I've dropped the two SCSI patches for now, Ming can you resend? If
> James agrees, I really think queue_stopped should just have the type
> changed and the patch redone with that using cmpxcgh().

Well, that's what I suggested originally, so I agree ... I don't think
31 more bytes is going to be a huge burden to scsi_device.

James


--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [PATCH 2/3] scsi: make sure that request queue queiesce and unquiesce balanced

[James Bottomley]

On Tue, 2021-11-02 at 08:41 -0600, Jens Axboe wrote:
> On 11/2/21 8:36 AM, Jens Axboe wrote:
> > On 11/2/21 8:33 AM, James Bottomley wrote:
> > > On Tue, 2021-11-02 at 06:59 -0600, Jens Axboe wrote:
> > > > On 11/1/21 7:43 PM, James Bottomley wrote:
> > > > > On Thu, 2021-10-21 at 22:59 +0800, Ming Lei wrote:
> > > > > > 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 bit flag & global spinlock,
> > > > > > basically zero cost since request queue quiesce is seldom
> > > > > > triggered.
> > > > > > 
> > > > > > 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    | 45
> > > > > > ++++++++++++++++++++++++++++++
> > > > > > ----
> > > > > > ----
> > > > > >  include/scsi/scsi_device.h |  1 +
> > > > > >  2 files changed, 37 insertions(+), 9 deletions(-)
> > > > > > 
> > > > > > diff --git a/drivers/scsi/scsi_lib.c
> > > > > > b/drivers/scsi/scsi_lib.c
> > > > > > index 51fcd46be265..414f4daf8005 100644
> > > > > > --- a/drivers/scsi/scsi_lib.c
> > > > > > +++ b/drivers/scsi/scsi_lib.c
> > > > > > @@ -2638,6 +2638,40 @@ static int
> > > > > > __scsi_internal_device_block_nowait(struct scsi_device
> > > > > > *sdev)
> > > > > >  	return 0;
> > > > > >  }
> > > > > >  
> > > > > > +static DEFINE_SPINLOCK(sdev_queue_stop_lock);
> > > > > > +
> > > > > > +void scsi_start_queue(struct scsi_device *sdev)
> > > > > > +{
> > > > > > +	bool need_start;
> > > > > > +	unsigned long flags;
> > > > > > +
> > > > > > +	spin_lock_irqsave(&sdev_queue_stop_lock, flags);
> > > > > > +	need_start = sdev->queue_stopped;
> > > > > > +	sdev->queue_stopped = 0;
> > > > > > +	spin_unlock_irqrestore(&sdev_queue_stop_lock, flags);
> > > > > > +
> > > > > > +	if (need_start)
> > > > > > +		blk_mq_unquiesce_queue(sdev->request_queue);
> > > > > 
> > > > > Well, this is a classic atomic pattern:
> > > > > 
> > > > > if (cmpxchg(&sdev->queue_stopped, 1, 0))
> > > > > 	blk_mq_unquiesce_queue(sdev->request_queue);
> > > > > 
> > > > > The reason to do it with atomics rather than spinlocks is
> > > > > 
> > > > >    1. no need to disable interrupts: atomics are locked
> > > > >    2. faster because a spinlock takes an exclusive line every
> > > > > time but the
> > > > >       read to check the value can be in shared mode in
> > > > > cmpxchg
> > > > >    3. it's just shorter and better code.
> > > > > 
> > > > > The only minor downside is queue_stopped now needs to be a
> > > > > u32.
> > > > 
> > > > Are you fine with the change as-is, or do you want it redone? I
> > > > can drop the SCSI parts and just queue up the dm fix.
> > > > Personally I think it'd be better to get it fixed upfront.
> > > 
> > > Well, given the path isn't hot, I don't really care.  However,
> > > what I don't want is to have to continually bat back patches from
> > > the make work code churners trying to update this code for being
> > > the wrong pattern.  I think at the very least it needs a comment
> > > saying why we chose a suboptimal pattern to try to forestall
> > > this.
> > 
> > Right, with a comment it's probably better. And as you said, since
> > it's not a hot path, don't think we'd be revisiting it anyway.
> > 
> > I'll amend the patch with a comment.
> 
> I started adding the comment and took another look at this, and that
> made me change my mind. We really should make this a cmpxcgh, it's
> not even using a device lock here.
> 
> I've dropped the two SCSI patches for now, Ming can you resend? If
> James agrees, I really think queue_stopped should just have the type
> changed and the patch redone with that using cmpxcgh().

Well, that's what I suggested originally, so I agree ... I don't think
31 more bytes is going to be a huge burden to scsi_device.

James

Re: [dm-devel] [PATCH 2/3] scsi: make sure that request queue queiesce and unquiesce balanced

[Jens Axboe]

On 11/2/21 8:47 AM, James Bottomley wrote:
> On Tue, 2021-11-02 at 08:41 -0600, Jens Axboe wrote:
>> On 11/2/21 8:36 AM, Jens Axboe wrote:
>>> On 11/2/21 8:33 AM, James Bottomley wrote:
>>>> On Tue, 2021-11-02 at 06:59 -0600, Jens Axboe wrote:
>>>>> On 11/1/21 7:43 PM, James Bottomley wrote:
>>>>>> On Thu, 2021-10-21 at 22:59 +0800, Ming Lei wrote:
>>>>>>> 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 bit flag & global spinlock,
>>>>>>> basically zero cost since request queue quiesce is seldom
>>>>>>> triggered.
>>>>>>>
>>>>>>> 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    | 45
>>>>>>> ++++++++++++++++++++++++++++++
>>>>>>> ----
>>>>>>> ----
>>>>>>>  include/scsi/scsi_device.h |  1 +
>>>>>>>  2 files changed, 37 insertions(+), 9 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/scsi/scsi_lib.c
>>>>>>> b/drivers/scsi/scsi_lib.c
>>>>>>> index 51fcd46be265..414f4daf8005 100644
>>>>>>> --- a/drivers/scsi/scsi_lib.c
>>>>>>> +++ b/drivers/scsi/scsi_lib.c
>>>>>>> @@ -2638,6 +2638,40 @@ static int
>>>>>>> __scsi_internal_device_block_nowait(struct scsi_device
>>>>>>> *sdev)
>>>>>>>  	return 0;
>>>>>>>  }
>>>>>>>  
>>>>>>> +static DEFINE_SPINLOCK(sdev_queue_stop_lock);
>>>>>>> +
>>>>>>> +void scsi_start_queue(struct scsi_device *sdev)
>>>>>>> +{
>>>>>>> +	bool need_start;
>>>>>>> +	unsigned long flags;
>>>>>>> +
>>>>>>> +	spin_lock_irqsave(&sdev_queue_stop_lock, flags);
>>>>>>> +	need_start = sdev->queue_stopped;
>>>>>>> +	sdev->queue_stopped = 0;
>>>>>>> +	spin_unlock_irqrestore(&sdev_queue_stop_lock, flags);
>>>>>>> +
>>>>>>> +	if (need_start)
>>>>>>> +		blk_mq_unquiesce_queue(sdev->request_queue);
>>>>>>
>>>>>> Well, this is a classic atomic pattern:
>>>>>>
>>>>>> if (cmpxchg(&sdev->queue_stopped, 1, 0))
>>>>>> 	blk_mq_unquiesce_queue(sdev->request_queue);
>>>>>>
>>>>>> The reason to do it with atomics rather than spinlocks is
>>>>>>
>>>>>>    1. no need to disable interrupts: atomics are locked
>>>>>>    2. faster because a spinlock takes an exclusive line every
>>>>>> time but the
>>>>>>       read to check the value can be in shared mode in
>>>>>> cmpxchg
>>>>>>    3. it's just shorter and better code.
>>>>>>
>>>>>> The only minor downside is queue_stopped now needs to be a
>>>>>> u32.
>>>>>
>>>>> Are you fine with the change as-is, or do you want it redone? I
>>>>> can drop the SCSI parts and just queue up the dm fix.
>>>>> Personally I think it'd be better to get it fixed upfront.
>>>>
>>>> Well, given the path isn't hot, I don't really care.  However,
>>>> what I don't want is to have to continually bat back patches from
>>>> the make work code churners trying to update this code for being
>>>> the wrong pattern.  I think at the very least it needs a comment
>>>> saying why we chose a suboptimal pattern to try to forestall
>>>> this.
>>>
>>> Right, with a comment it's probably better. And as you said, since
>>> it's not a hot path, don't think we'd be revisiting it anyway.
>>>
>>> I'll amend the patch with a comment.
>>
>> I started adding the comment and took another look at this, and that
>> made me change my mind. We really should make this a cmpxcgh, it's
>> not even using a device lock here.
>>
>> I've dropped the two SCSI patches for now, Ming can you resend? If
>> James agrees, I really think queue_stopped should just have the type
>> changed and the patch redone with that using cmpxcgh().
> 
> Well, that's what I suggested originally, so I agree ... I don't think
> 31 more bytes is going to be a huge burden to scsi_device.

Yeah I know, just saying I agree with that being the better solution.

-- 
Jens Axboe

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [PATCH 2/3] scsi: make sure that request queue queiesce and unquiesce balanced

[Jens Axboe]

On 11/2/21 8:47 AM, James Bottomley wrote:
> On Tue, 2021-11-02 at 08:41 -0600, Jens Axboe wrote:
>> On 11/2/21 8:36 AM, Jens Axboe wrote:
>>> On 11/2/21 8:33 AM, James Bottomley wrote:
>>>> On Tue, 2021-11-02 at 06:59 -0600, Jens Axboe wrote:
>>>>> On 11/1/21 7:43 PM, James Bottomley wrote:
>>>>>> On Thu, 2021-10-21 at 22:59 +0800, Ming Lei wrote:
>>>>>>> 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 bit flag & global spinlock,
>>>>>>> basically zero cost since request queue quiesce is seldom
>>>>>>> triggered.
>>>>>>>
>>>>>>> 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    | 45
>>>>>>> ++++++++++++++++++++++++++++++
>>>>>>> ----
>>>>>>> ----
>>>>>>>  include/scsi/scsi_device.h |  1 +
>>>>>>>  2 files changed, 37 insertions(+), 9 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/scsi/scsi_lib.c
>>>>>>> b/drivers/scsi/scsi_lib.c
>>>>>>> index 51fcd46be265..414f4daf8005 100644
>>>>>>> --- a/drivers/scsi/scsi_lib.c
>>>>>>> +++ b/drivers/scsi/scsi_lib.c
>>>>>>> @@ -2638,6 +2638,40 @@ static int
>>>>>>> __scsi_internal_device_block_nowait(struct scsi_device
>>>>>>> *sdev)
>>>>>>>  	return 0;
>>>>>>>  }
>>>>>>>  
>>>>>>> +static DEFINE_SPINLOCK(sdev_queue_stop_lock);
>>>>>>> +
>>>>>>> +void scsi_start_queue(struct scsi_device *sdev)
>>>>>>> +{
>>>>>>> +	bool need_start;
>>>>>>> +	unsigned long flags;
>>>>>>> +
>>>>>>> +	spin_lock_irqsave(&sdev_queue_stop_lock, flags);
>>>>>>> +	need_start = sdev->queue_stopped;
>>>>>>> +	sdev->queue_stopped = 0;
>>>>>>> +	spin_unlock_irqrestore(&sdev_queue_stop_lock, flags);
>>>>>>> +
>>>>>>> +	if (need_start)
>>>>>>> +		blk_mq_unquiesce_queue(sdev->request_queue);
>>>>>>
>>>>>> Well, this is a classic atomic pattern:
>>>>>>
>>>>>> if (cmpxchg(&sdev->queue_stopped, 1, 0))
>>>>>> 	blk_mq_unquiesce_queue(sdev->request_queue);
>>>>>>
>>>>>> The reason to do it with atomics rather than spinlocks is
>>>>>>
>>>>>>    1. no need to disable interrupts: atomics are locked
>>>>>>    2. faster because a spinlock takes an exclusive line every
>>>>>> time but the
>>>>>>       read to check the value can be in shared mode in
>>>>>> cmpxchg
>>>>>>    3. it's just shorter and better code.
>>>>>>
>>>>>> The only minor downside is queue_stopped now needs to be a
>>>>>> u32.
>>>>>
>>>>> Are you fine with the change as-is, or do you want it redone? I
>>>>> can drop the SCSI parts and just queue up the dm fix.
>>>>> Personally I think it'd be better to get it fixed upfront.
>>>>
>>>> Well, given the path isn't hot, I don't really care.  However,
>>>> what I don't want is to have to continually bat back patches from
>>>> the make work code churners trying to update this code for being
>>>> the wrong pattern.  I think at the very least it needs a comment
>>>> saying why we chose a suboptimal pattern to try to forestall
>>>> this.
>>>
>>> Right, with a comment it's probably better. And as you said, since
>>> it's not a hot path, don't think we'd be revisiting it anyway.
>>>
>>> I'll amend the patch with a comment.
>>
>> I started adding the comment and took another look at this, and that
>> made me change my mind. We really should make this a cmpxcgh, it's
>> not even using a device lock here.
>>
>> I've dropped the two SCSI patches for now, Ming can you resend? If
>> James agrees, I really think queue_stopped should just have the type
>> changed and the patch redone with that using cmpxcgh().
> 
> Well, that's what I suggested originally, so I agree ... I don't think
> 31 more bytes is going to be a huge burden to scsi_device.

Yeah I know, just saying I agree with that being the better solution.

-- 
Jens Axboe

Re: [dm-devel] [PATCH 2/3] scsi: make sure that request queue queiesce and unquiesce balanced

[Jens Axboe]

On 11/2/21 8:47 AM, James Bottomley wrote:
> On Tue, 2021-11-02 at 08:41 -0600, Jens Axboe wrote:
>> On 11/2/21 8:36 AM, Jens Axboe wrote:
>>> On 11/2/21 8:33 AM, James Bottomley wrote:
>>>> On Tue, 2021-11-02 at 06:59 -0600, Jens Axboe wrote:
>>>>> On 11/1/21 7:43 PM, James Bottomley wrote:
>>>>>> On Thu, 2021-10-21 at 22:59 +0800, Ming Lei wrote:
>>>>>>> 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 bit flag & global spinlock,
>>>>>>> basically zero cost since request queue quiesce is seldom
>>>>>>> triggered.
>>>>>>>
>>>>>>> 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    | 45
>>>>>>> ++++++++++++++++++++++++++++++
>>>>>>> ----
>>>>>>> ----
>>>>>>>  include/scsi/scsi_device.h |  1 +
>>>>>>>  2 files changed, 37 insertions(+), 9 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/scsi/scsi_lib.c
>>>>>>> b/drivers/scsi/scsi_lib.c
>>>>>>> index 51fcd46be265..414f4daf8005 100644
>>>>>>> --- a/drivers/scsi/scsi_lib.c
>>>>>>> +++ b/drivers/scsi/scsi_lib.c
>>>>>>> @@ -2638,6 +2638,40 @@ static int
>>>>>>> __scsi_internal_device_block_nowait(struct scsi_device
>>>>>>> *sdev)
>>>>>>>  	return 0;
>>>>>>>  }
>>>>>>>  
>>>>>>> +static DEFINE_SPINLOCK(sdev_queue_stop_lock);
>>>>>>> +
>>>>>>> +void scsi_start_queue(struct scsi_device *sdev)
>>>>>>> +{
>>>>>>> +	bool need_start;
>>>>>>> +	unsigned long flags;
>>>>>>> +
>>>>>>> +	spin_lock_irqsave(&sdev_queue_stop_lock, flags);
>>>>>>> +	need_start = sdev->queue_stopped;
>>>>>>> +	sdev->queue_stopped = 0;
>>>>>>> +	spin_unlock_irqrestore(&sdev_queue_stop_lock, flags);
>>>>>>> +
>>>>>>> +	if (need_start)
>>>>>>> +		blk_mq_unquiesce_queue(sdev->request_queue);
>>>>>>
>>>>>> Well, this is a classic atomic pattern:
>>>>>>
>>>>>> if (cmpxchg(&sdev->queue_stopped, 1, 0))
>>>>>> 	blk_mq_unquiesce_queue(sdev->request_queue);
>>>>>>
>>>>>> The reason to do it with atomics rather than spinlocks is
>>>>>>
>>>>>>    1. no need to disable interrupts: atomics are locked
>>>>>>    2. faster because a spinlock takes an exclusive line every
>>>>>> time but the
>>>>>>       read to check the value can be in shared mode in
>>>>>> cmpxchg
>>>>>>    3. it's just shorter and better code.
>>>>>>
>>>>>> The only minor downside is queue_stopped now needs to be a
>>>>>> u32.
>>>>>
>>>>> Are you fine with the change as-is, or do you want it redone? I
>>>>> can drop the SCSI parts and just queue up the dm fix.
>>>>> Personally I think it'd be better to get it fixed upfront.
>>>>
>>>> Well, given the path isn't hot, I don't really care.  However,
>>>> what I don't want is to have to continually bat back patches from
>>>> the make work code churners trying to update this code for being
>>>> the wrong pattern.  I think at the very least it needs a comment
>>>> saying why we chose a suboptimal pattern to try to forestall
>>>> this.
>>>
>>> Right, with a comment it's probably better. And as you said, since
>>> it's not a hot path, don't think we'd be revisiting it anyway.
>>>
>>> I'll amend the patch with a comment.
>>
>> I started adding the comment and took another look at this, and that
>> made me change my mind. We really should make this a cmpxcgh, it's
>> not even using a device lock here.
>>
>> I've dropped the two SCSI patches for now, Ming can you resend? If
>> James agrees, I really think queue_stopped should just have the type
>> changed and the patch redone with that using cmpxcgh().
> 
> Well, that's what I suggested originally, so I agree ... I don't think
> 31 more bytes is going to be a huge burden to scsi_device.
          ^^^^

Bits? :-)

-- 
Jens Axboe

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [PATCH 2/3] scsi: make sure that request queue queiesce and unquiesce balanced

[Jens Axboe]

On 11/2/21 8:47 AM, James Bottomley wrote:
> On Tue, 2021-11-02 at 08:41 -0600, Jens Axboe wrote:
>> On 11/2/21 8:36 AM, Jens Axboe wrote:
>>> On 11/2/21 8:33 AM, James Bottomley wrote:
>>>> On Tue, 2021-11-02 at 06:59 -0600, Jens Axboe wrote:
>>>>> On 11/1/21 7:43 PM, James Bottomley wrote:
>>>>>> On Thu, 2021-10-21 at 22:59 +0800, Ming Lei wrote:
>>>>>>> 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 bit flag & global spinlock,
>>>>>>> basically zero cost since request queue quiesce is seldom
>>>>>>> triggered.
>>>>>>>
>>>>>>> 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    | 45
>>>>>>> ++++++++++++++++++++++++++++++
>>>>>>> ----
>>>>>>> ----
>>>>>>>  include/scsi/scsi_device.h |  1 +
>>>>>>>  2 files changed, 37 insertions(+), 9 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/scsi/scsi_lib.c
>>>>>>> b/drivers/scsi/scsi_lib.c
>>>>>>> index 51fcd46be265..414f4daf8005 100644
>>>>>>> --- a/drivers/scsi/scsi_lib.c
>>>>>>> +++ b/drivers/scsi/scsi_lib.c
>>>>>>> @@ -2638,6 +2638,40 @@ static int
>>>>>>> __scsi_internal_device_block_nowait(struct scsi_device
>>>>>>> *sdev)
>>>>>>>  	return 0;
>>>>>>>  }
>>>>>>>  
>>>>>>> +static DEFINE_SPINLOCK(sdev_queue_stop_lock);
>>>>>>> +
>>>>>>> +void scsi_start_queue(struct scsi_device *sdev)
>>>>>>> +{
>>>>>>> +	bool need_start;
>>>>>>> +	unsigned long flags;
>>>>>>> +
>>>>>>> +	spin_lock_irqsave(&sdev_queue_stop_lock, flags);
>>>>>>> +	need_start = sdev->queue_stopped;
>>>>>>> +	sdev->queue_stopped = 0;
>>>>>>> +	spin_unlock_irqrestore(&sdev_queue_stop_lock, flags);
>>>>>>> +
>>>>>>> +	if (need_start)
>>>>>>> +		blk_mq_unquiesce_queue(sdev->request_queue);
>>>>>>
>>>>>> Well, this is a classic atomic pattern:
>>>>>>
>>>>>> if (cmpxchg(&sdev->queue_stopped, 1, 0))
>>>>>> 	blk_mq_unquiesce_queue(sdev->request_queue);
>>>>>>
>>>>>> The reason to do it with atomics rather than spinlocks is
>>>>>>
>>>>>>    1. no need to disable interrupts: atomics are locked
>>>>>>    2. faster because a spinlock takes an exclusive line every
>>>>>> time but the
>>>>>>       read to check the value can be in shared mode in
>>>>>> cmpxchg
>>>>>>    3. it's just shorter and better code.
>>>>>>
>>>>>> The only minor downside is queue_stopped now needs to be a
>>>>>> u32.
>>>>>
>>>>> Are you fine with the change as-is, or do you want it redone? I
>>>>> can drop the SCSI parts and just queue up the dm fix.
>>>>> Personally I think it'd be better to get it fixed upfront.
>>>>
>>>> Well, given the path isn't hot, I don't really care.  However,
>>>> what I don't want is to have to continually bat back patches from
>>>> the make work code churners trying to update this code for being
>>>> the wrong pattern.  I think at the very least it needs a comment
>>>> saying why we chose a suboptimal pattern to try to forestall
>>>> this.
>>>
>>> Right, with a comment it's probably better. And as you said, since
>>> it's not a hot path, don't think we'd be revisiting it anyway.
>>>
>>> I'll amend the patch with a comment.
>>
>> I started adding the comment and took another look at this, and that
>> made me change my mind. We really should make this a cmpxcgh, it's
>> not even using a device lock here.
>>
>> I've dropped the two SCSI patches for now, Ming can you resend? If
>> James agrees, I really think queue_stopped should just have the type
>> changed and the patch redone with that using cmpxcgh().
> 
> Well, that's what I suggested originally, so I agree ... I don't think
> 31 more bytes is going to be a huge burden to scsi_device.
          ^^^^

Bits? :-)

-- 
Jens Axboe

[dm-devel] [PATCH 3/3] dm: don't stop request queue after the dm device is suspended

[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 to be balanced.

__bind() is only called from dm_swap_table() in which dm device has been
suspended already, so not necessary to stop queue again. With this way,
request queue quiesce and unquiesce can be 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/md/dm.c | 10 ----------
 1 file changed, 10 deletions(-)

diff --git a/drivers/md/dm.c b/drivers/md/dm.c
index 7870e6460633..727282d79b26 100644
--- a/drivers/md/dm.c
+++ b/drivers/md/dm.c
@@ -1927,16 +1927,6 @@ static struct dm_table *__bind(struct mapped_device *md, struct dm_table *t,
 
 	dm_table_event_callback(t, event_callback, md);
 
-	/*
-	 * The queue hasn't been stopped yet, if the old table type wasn't
-	 * for request-based during suspension.  So stop it to prevent
-	 * I/O mapping before resume.
-	 * This must be done before setting the queue restrictions,
-	 * because request-based dm may be run just after the setting.
-	 */
-	if (request_based)
-		dm_stop_queue(q);
-
 	if (request_based) {
 		/*
 		 * Leverage the fact that request-based DM targets are
-- 
2.31.1

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

[PATCH 3/3] dm: don't stop request queue after the dm device is suspended

[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 to be balanced.

__bind() is only called from dm_swap_table() in which dm device has been
suspended already, so not necessary to stop queue again. With this way,
request queue quiesce and unquiesce can be 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/md/dm.c | 10 ----------
 1 file changed, 10 deletions(-)

diff --git a/drivers/md/dm.c b/drivers/md/dm.c
index 7870e6460633..727282d79b26 100644
--- a/drivers/md/dm.c
+++ b/drivers/md/dm.c
@@ -1927,16 +1927,6 @@ static struct dm_table *__bind(struct mapped_device *md, struct dm_table *t,
 
 	dm_table_event_callback(t, event_callback, md);
 
-	/*
-	 * The queue hasn't been stopped yet, if the old table type wasn't
-	 * for request-based during suspension.  So stop it to prevent
-	 * I/O mapping before resume.
-	 * This must be done before setting the queue restrictions,
-	 * because request-based dm may be run just after the setting.
-	 */
-	if (request_based)
-		dm_stop_queue(q);
-
 	if (request_based) {
 		/*
 		 * Leverage the fact that request-based DM targets are
-- 
2.31.1

Re: [dm-devel] [PATCH 3/3] dm: don't stop request queue after the dm device is suspended

[Mike Snitzer]

On Thu, Oct 21 2021 at 10:59P -0400,
Ming Lei <ming.lei@redhat.com> wrote:

> 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 to be balanced.
> 
> __bind() is only called from dm_swap_table() in which dm device has been
> suspended already, so not necessary to stop queue again. With this way,
> request queue quiesce and unquiesce can be 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/md/dm.c | 10 ----------
>  1 file changed, 10 deletions(-)
> 
> diff --git a/drivers/md/dm.c b/drivers/md/dm.c
> index 7870e6460633..727282d79b26 100644
> --- a/drivers/md/dm.c
> +++ b/drivers/md/dm.c
> @@ -1927,16 +1927,6 @@ static struct dm_table *__bind(struct mapped_device *md, struct dm_table *t,
>  
>  	dm_table_event_callback(t, event_callback, md);
>  
> -	/*
> -	 * The queue hasn't been stopped yet, if the old table type wasn't
> -	 * for request-based during suspension.  So stop it to prevent
> -	 * I/O mapping before resume.
> -	 * This must be done before setting the queue restrictions,
> -	 * because request-based dm may be run just after the setting.
> -	 */
> -	if (request_based)
> -		dm_stop_queue(q);
> -
>  	if (request_based) {
>  		/*
>  		 * Leverage the fact that request-based DM targets are
> -- 
> 2.31.1
> 

I think this is fine given your previous commit (b4459b11e8f dm rq:
don't queue request to blk-mq during DM suspend).

Acked-by: Mike Snitzer <snitzer@redhat.com>

Jens: given this series fixes a 5.16 regression in srp test, please
pick it up for 5.16-rc

Thanks,
Mike

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [PATCH 3/3] dm: don't stop request queue after the dm device is suspended

[Mike Snitzer]

On Thu, Oct 21 2021 at 10:59P -0400,
Ming Lei <ming.lei@redhat.com> wrote:

> 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 to be balanced.
> 
> __bind() is only called from dm_swap_table() in which dm device has been
> suspended already, so not necessary to stop queue again. With this way,
> request queue quiesce and unquiesce can be 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/md/dm.c | 10 ----------
>  1 file changed, 10 deletions(-)
> 
> diff --git a/drivers/md/dm.c b/drivers/md/dm.c
> index 7870e6460633..727282d79b26 100644
> --- a/drivers/md/dm.c
> +++ b/drivers/md/dm.c
> @@ -1927,16 +1927,6 @@ static struct dm_table *__bind(struct mapped_device *md, struct dm_table *t,
>  
>  	dm_table_event_callback(t, event_callback, md);
>  
> -	/*
> -	 * The queue hasn't been stopped yet, if the old table type wasn't
> -	 * for request-based during suspension.  So stop it to prevent
> -	 * I/O mapping before resume.
> -	 * This must be done before setting the queue restrictions,
> -	 * because request-based dm may be run just after the setting.
> -	 */
> -	if (request_based)
> -		dm_stop_queue(q);
> -
>  	if (request_based) {
>  		/*
>  		 * Leverage the fact that request-based DM targets are
> -- 
> 2.31.1
> 

I think this is fine given your previous commit (b4459b11e8f dm rq:
don't queue request to blk-mq during DM suspend).

Acked-by: Mike Snitzer <snitzer@redhat.com>

Jens: given this series fixes a 5.16 regression in srp test, please
pick it up for 5.16-rc

Thanks,
Mike

Re: [dm-devel] [PATCH 0/3] block: keep quiesce & unquiesce balanced for scsi/dm

[Yi Zhang]

Verified with the blktests srp/, thanks Ming.

Tested-by: Yi Zhang <yi.zhang@redhat.com>

On Thu, Oct 21, 2021 at 11:00 PM Ming Lei <ming.lei@redhat.com> wrote:
>
> Hello Jens,
>
> Recently we merge the patch of e70feb8b3e68 ("blk-mq: support concurrent queue
> quiesce/unquiesce") for fixing race between driver and block layer wrt.
> queue quiesce.
>
> Yi reported that srp/002 is broken with this patch, turns out scsi and
> dm don't keep the two balanced actually.
>
> So fix dm and scsi and make srp/002 pass again.
>
>
> Ming Lei (3):
>   scsi: avoid to quiesce sdev->request_queue two times
>   scsi: make sure that request queue queiesce and unquiesce balanced
>   dm: don't stop request queue after the dm device is suspended
>
>  drivers/md/dm.c            | 10 ------
>  drivers/scsi/scsi_lib.c    | 70 ++++++++++++++++++++++++++------------
>  include/scsi/scsi_device.h |  1 +
>  3 files changed, 49 insertions(+), 32 deletions(-)
>
> --
> 2.31.1
>


-- 
Best Regards,
  Yi Zhang

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [PATCH 0/3] block: keep quiesce & unquiesce balanced for scsi/dm

[Yi Zhang]

Verified with the blktests srp/, thanks Ming.

Tested-by: Yi Zhang <yi.zhang@redhat.com>

On Thu, Oct 21, 2021 at 11:00 PM Ming Lei <ming.lei@redhat.com> wrote:
>
> Hello Jens,
>
> Recently we merge the patch of e70feb8b3e68 ("blk-mq: support concurrent queue
> quiesce/unquiesce") for fixing race between driver and block layer wrt.
> queue quiesce.
>
> Yi reported that srp/002 is broken with this patch, turns out scsi and
> dm don't keep the two balanced actually.
>
> So fix dm and scsi and make srp/002 pass again.
>
>
> Ming Lei (3):
>   scsi: avoid to quiesce sdev->request_queue two times
>   scsi: make sure that request queue queiesce and unquiesce balanced
>   dm: don't stop request queue after the dm device is suspended
>
>  drivers/md/dm.c            | 10 ------
>  drivers/scsi/scsi_lib.c    | 70 ++++++++++++++++++++++++++------------
>  include/scsi/scsi_device.h |  1 +
>  3 files changed, 49 insertions(+), 32 deletions(-)
>
> --
> 2.31.1
>


-- 
Best Regards,
  Yi Zhang

Re: [dm-devel] [PATCH 0/3] block: keep quiesce & unquiesce balanced for scsi/dm

[Jens Axboe]

On Thu, 21 Oct 2021 22:59:15 +0800, Ming Lei wrote:
> Recently we merge the patch of e70feb8b3e68 ("blk-mq: support concurrent queue
> quiesce/unquiesce") for fixing race between driver and block layer wrt.
> queue quiesce.
> 
> Yi reported that srp/002 is broken with this patch, turns out scsi and
> dm don't keep the two balanced actually.
> 
> [...]

Applied, thanks!

[1/3] scsi: avoid to quiesce sdev->request_queue two times
      commit: 256117fb3b4f8832d6b29485d49d37ccc4c314d5
[2/3] scsi: make sure that request queue queiesce and unquiesce balanced
      commit: fba9539fc2109740e70e77c303dec50d1411e11f
[3/3] dm: don't stop request queue after the dm device is suspended
      commit: e719593760c34fbf346fc6e348113e042feb5f63

Best regards,
-- 
Jens Axboe


--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [PATCH 0/3] block: keep quiesce & unquiesce balanced for scsi/dm

[Jens Axboe]

On Thu, 21 Oct 2021 22:59:15 +0800, Ming Lei wrote:
> Recently we merge the patch of e70feb8b3e68 ("blk-mq: support concurrent queue
> quiesce/unquiesce") for fixing race between driver and block layer wrt.
> queue quiesce.
> 
> Yi reported that srp/002 is broken with this patch, turns out scsi and
> dm don't keep the two balanced actually.
> 
> [...]

Applied, thanks!

[1/3] scsi: avoid to quiesce sdev->request_queue two times
      commit: 256117fb3b4f8832d6b29485d49d37ccc4c314d5
[2/3] scsi: make sure that request queue queiesce and unquiesce balanced
      commit: fba9539fc2109740e70e77c303dec50d1411e11f
[3/3] dm: don't stop request queue after the dm device is suspended
      commit: e719593760c34fbf346fc6e348113e042feb5f63

Best regards,
-- 
Jens Axboe