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:

quoted

On Wed, 2021-11-03 at 11:43 +0800, Ming Lei wrote:
[...]

quoted

+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-

quoted

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.

quoted

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