[3/3] scsi: simplify scsi_stop_queue()

Commit Message

Martin Wilck June 2, 2023, 4:38 p.m. UTC

From: Hannes Reinecke <[e-mail]>

scsi_target_block() calls scsi_stop_queue() for each scsi_device
and scsi_stop_queue() calls blk_mq_wait_quiesce_done() for each LUN.
As blk_mq_wait_quiesce_done() comes down to synchronize_rcu() for
SCSI queues, this can cause substantial delay for scsi_target_block()
on a target with a lot of logical units (we measured more than 100s
delay for blocking a FC rport with 2048 LUNs).

Simplify scsi_stop_queue(), which is only called in this code path, to
never wait for the quiescing to finish. Rather call synchronize_rcu()
from scsi_target_block() after iterating over all devices.
Also, move the call to scsi_stop_queue() in scsi_internal_device_block()
out of the code section where the state_mutex is held.

This patch uses the same basic idea as commit f983622ae605 ("scsi: core:
Avoid calling synchronize_rcu() for each device in scsi_host_block()").

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

Comments

Bart Van Assche June 4, 2023, 1:54 p.m. UTC | #1

On 6/2/23 09:38, [e-mail] wrote:
> @@ -2910,6 +2904,13 @@ scsi_target_block(struct device *dev)
>   					device_block);
>   	else
>   		device_for_each_child(dev, NULL, target_block);
> +
> +	/*
> +	 * SCSI never enables blk-mq's BLK_MQ_F_BLOCKING flag,
> +	 * so blk_mq_wait_quiesce_done() comes down to just synchronize_rcu().
> +	 * Just calling it once is enough.
> +	 */
> +	synchronize_rcu();
>   }
>   EXPORT_SYMBOL_GPL(scsi_target_block);

The above comment is wrong. See also commit b125bb99559e ("scsi:
core: Support setting BLK_MQ_F_BLOCKING").

Bart.

Hannes Reinecke June 5, 2023, 7:31 a.m. UTC | #2

On 6/4/23 15:54, Bart Van Assche wrote:
> On 6/2/23 09:38, [e-mail] wrote:
>> @@ -2910,6 +2904,13 @@ scsi_target_block(struct device *dev)
>>                       device_block);
>>       else
>>           device_for_each_child(dev, NULL, target_block);
>> +
>> +    /*
>> +     * SCSI never enables blk-mq's BLK_MQ_F_BLOCKING flag,
>> +     * so blk_mq_wait_quiesce_done() comes down to just 
>> synchronize_rcu().
>> +     * Just calling it once is enough.
>> +     */
>> +    synchronize_rcu();
>>   }
>>   EXPORT_SYMBOL_GPL(scsi_target_block);
> 
> The above comment is wrong. See also commit b125bb99559e ("scsi:
> core: Support setting BLK_MQ_F_BLOCKING").
> 
Well, this patch got written before your patchset has been posted.
But yeah, we'll be updating it.

Cheers,

Hannes

Patch

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index b7c569a42aa4..ed5bc9e1dbed 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -2725,24 +2725,18 @@  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.
+	 * in case that queue has been stopped. This is done in
+	 * scsi_target_block() for all devices of the target.
 	 */
-	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);
 }
 
 /**
@@ -2769,7 +2763,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);
@@ -2795,9 +2789,9 @@  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)
-		scsi_stop_queue(sdev, false);
 	mutex_unlock(&sdev->state_mutex);
+	if (err == 0)
+		scsi_stop_queue(sdev);
 
 	return err;
 }
@@ -2910,6 +2904,13 @@  scsi_target_block(struct device *dev)
 					device_block);
 	else
 		device_for_each_child(dev, NULL, target_block);
+
+	/*
+	 * SCSI never enables blk-mq's BLK_MQ_F_BLOCKING flag,
+	 * so blk_mq_wait_quiesce_done() comes down to just synchronize_rcu().
+	 * Just calling it once is enough.
+	 */
+	synchronize_rcu();
 }
 EXPORT_SYMBOL_GPL(scsi_target_block);