[v4,4/6] scsi: don't wait for quiesce in scsi_stop_queue()

Commit Message

Martin Wilck June 12, 2023, 3:03 p.m. UTC

From: Martin Wilck <[e-mail]>

scsi_stop_queue() has just two callers, one with and one without
"nowait". As blk_mq_quiesce_queue() comes down to
blk_mq_quiesce_queue_nowait() followed by blk_mq_wait_quiesce_done(),
we might as well open-code this in scsi_device_block().

Also, add a comment explaining why blk_mq_quiesce_queue_nowait() must
be called with the state_mutex held, see
https://lore.kernel.org/linux-scsi/[e-mail]/.

Signed-off-by: Martin Wilck <[e-mail]>
---
 drivers/scsi/scsi_lib.c | 32 ++++++++++++++++----------------
 1 file changed, 16 insertions(+), 16 deletions(-)

Comments

Bart Van Assche June 12, 2023, 3:40 p.m. UTC | #1

On 6/12/23 08:03, [e-mail] wrote:
> -	 * However, we still need to wait until quiesce is done
> -	 * in case that queue has been stopped.
> +	 * After return, we still need to wait until quiesce is done.

The above comment would be more clear if "After return, we still need" 
would be changed into "The caller needs".

> @@ -2800,9 +2792,17 @@ static void scsi_device_block(struct scsi_device *sdev, void *data)
>   
>   	mutex_lock(&sdev->state_mutex);
>   	err = __scsi_internal_device_block_nowait(sdev);
> -	if (err == 0)
> -		scsi_stop_queue(sdev, false);
> -	mutex_unlock(&sdev->state_mutex);
> +	if (err == 0) {
> +		/*
> +		 * scsi_stop_queue() must be called with the state_mutex
> +		 * held. Otherwise a simultaneous scsi_start_queue() call
> +		 * might unquiesce the queue before we quiesce it.
> +		 */
> +		scsi_stop_queue(sdev);
> +		mutex_unlock(&sdev->state_mutex);
> +		blk_mq_wait_quiesce_done(sdev->request_queue->tag_set);
> +	} else
> +		mutex_unlock(&sdev->state_mutex);

Has it been considered to modify the above code such that there is a 
single mutex_unlock() call instead of two? I wouldn't mind if 
blk_mq_wait_quiesce_done() would be called if err != 0 since performance 
is not that important if this function fails.

Thanks,

Bart.

Patch

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 69fb7a9d8883..b4cc1501dc36 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -2731,24 +2731,16 @@  void scsi_start_queue(struct scsi_device *sdev)
 		blk_mq_unquiesce_queue(sdev->request_queue);
 }
 
-static void scsi_stop_queue(struct scsi_device *sdev, bool nowait)
+static void scsi_stop_queue(struct scsi_device *sdev)
 {
 	/*
 	 * 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.
+	 * After return, we still need to wait until quiesce is done.
 	 */
-	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->tag_set);
-	}
+	if (!cmpxchg(&sdev->queue_stopped, 0, 1))
+		blk_mq_quiesce_queue_nowait(sdev->request_queue);
 }
 
 /**
@@ -2775,7 +2767,7 @@  int scsi_internal_device_block_nowait(struct scsi_device *sdev)
 	 * request queue.
 	 */
 	if (!ret)
-		scsi_stop_queue(sdev, true);
+		scsi_stop_queue(sdev);
 	return ret;
 }
 EXPORT_SYMBOL_GPL(scsi_internal_device_block_nowait);
@@ -2800,9 +2792,17 @@  static void scsi_device_block(struct scsi_device *sdev, void *data)
 
 	mutex_lock(&sdev->state_mutex);
 	err = __scsi_internal_device_block_nowait(sdev);
-	if (err == 0)
-		scsi_stop_queue(sdev, false);
-	mutex_unlock(&sdev->state_mutex);
+	if (err == 0) {
+		/*
+		 * scsi_stop_queue() must be called with the state_mutex
+		 * held. Otherwise a simultaneous scsi_start_queue() call
+		 * might unquiesce the queue before we quiesce it.
+		 */
+		scsi_stop_queue(sdev);
+		mutex_unlock(&sdev->state_mutex);
+		blk_mq_wait_quiesce_done(sdev->request_queue->tag_set);
+	} else
+		mutex_unlock(&sdev->state_mutex);
 
 	WARN_ONCE(err, "__scsi_internal_device_block_nowait(%s) failed: err = %d\n",
 		  dev_name(&sdev->sdev_gendev), err);