[PATCH V2] scsi: avoid to run synchronize_rcu for each device in scsi_host_block

[PATCH V2] scsi: avoid to run synchronize_rcu for each device in scsi_host_block

[Ming Lei]

scsi_host_block() calls scsi_internal_device_block() for each
scsi_device, and scsi_internal_device_block() calls
blk_mq_quiesce_queue() for each LUN. However synchronize_rcu is
run from blk_mq_quiesce_queue().

This way may become unnecessary slow in case of lots of LUNs.

Use scsi_internal_device_block_nowait() to implement scsi_host_block(),
so it is enough to run synchronize_rcu() once since scsi never
supports blk-mq's BLK_MQ_F_BLOCKING.

Cc: Steffen Maier <maier@linux.ibm.com>
Cc: Bart Van Assche <bvanassche@acm.org>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Dexuan Cui <decui@microsoft.com>
Cc: Hannes Reinecke <hare@suse.de>
Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
V2:
	- fix commit log as pointed by Steffen
	- add comment on BLK_MQ_F_BLOCKING as suggested by Bart

 drivers/scsi/scsi_lib.c | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 47835c4b4ee0..86007a523145 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -2841,11 +2841,26 @@ scsi_host_block(struct Scsi_Host *shost)
 	struct scsi_device *sdev;
 	int ret = 0;
 
+	/*
+	 * Call scsi_internal_device_block_nowait then we can avoid to
+	 * run synchronize_rcu() for each LUN
+	 */
 	shost_for_each_device(sdev, shost) {
-		ret = scsi_internal_device_block(sdev);
+		mutex_lock(&sdev->state_mutex);
+		ret = scsi_internal_device_block_nowait(sdev);
+		mutex_unlock(&sdev->state_mutex);
 		if (ret)
 			break;
 	}
+
+	/*
+	 * SCSI never enables blk-mq's BLK_MQ_F_BLOCKING, so run
+	 * synchronize_rcu() once is enough
+	 */
+	WARN_ON_ONCE(shost->tag_set.flags & BLK_MQ_F_BLOCKING);
+
+	if (!ret)
+		synchronize_rcu();
 	return ret;
 }
 EXPORT_SYMBOL_GPL(scsi_host_block);
-- 
2.25.2

Re: [PATCH V2] scsi: avoid to run synchronize_rcu for each device in scsi_host_block

[Hannes Reinecke]

On 4/23/20 4:07 AM, Ming Lei wrote:
> scsi_host_block() calls scsi_internal_device_block() for each
> scsi_device, and scsi_internal_device_block() calls
> blk_mq_quiesce_queue() for each LUN. However synchronize_rcu is
> run from blk_mq_quiesce_queue().
> 
> This way may become unnecessary slow in case of lots of LUNs.
> 
> Use scsi_internal_device_block_nowait() to implement scsi_host_block(),
> so it is enough to run synchronize_rcu() once since scsi never
> supports blk-mq's BLK_MQ_F_BLOCKING.
> 
> Cc: Steffen Maier <maier@linux.ibm.com>
> Cc: Bart Van Assche <bvanassche@acm.org>
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Dexuan Cui <decui@microsoft.com>
> Cc: Hannes Reinecke <hare@suse.de>
> Signed-off-by: Ming Lei <ming.lei@redhat.com>
> ---
> V2:
> 	- fix commit log as pointed by Steffen
> 	- add comment on BLK_MQ_F_BLOCKING as suggested by Bart
> 
>   drivers/scsi/scsi_lib.c | 17 ++++++++++++++++-
>   1 file changed, 16 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index 47835c4b4ee0..86007a523145 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -2841,11 +2841,26 @@ scsi_host_block(struct Scsi_Host *shost)
>   	struct scsi_device *sdev;
>   	int ret = 0;
>   
> +	/*
> +	 * Call scsi_internal_device_block_nowait then we can avoid to
> +	 * run synchronize_rcu() for each LUN
> +	 */
>   	shost_for_each_device(sdev, shost) {
> -		ret = scsi_internal_device_block(sdev);
> +		mutex_lock(&sdev->state_mutex);
> +		ret = scsi_internal_device_block_nowait(sdev);
> +		mutex_unlock(&sdev->state_mutex);
>   		if (ret)
>   			break;
>   	}
> +
> +	/*
> +	 * SCSI never enables blk-mq's BLK_MQ_F_BLOCKING, so run
> +	 * synchronize_rcu() once is enough
> +	 */
> +	WARN_ON_ONCE(shost->tag_set.flags & BLK_MQ_F_BLOCKING);
> +
> +	if (!ret)
> +		synchronize_rcu();
>   	return ret;
>   }
>   EXPORT_SYMBOL_GPL(scsi_host_block);
> 
Looks good.

Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke            Teamlead Storage & Networking
hare@suse.de                               +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer

Re: [PATCH V2] scsi: avoid to run synchronize_rcu for each device in scsi_host_block

[Christoph Hellwig]

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH V2] scsi: avoid to run synchronize_rcu for each device in scsi_host_block

[Martin K. Petersen]

Ming,

> scsi_host_block() calls scsi_internal_device_block() for each
> scsi_device, and scsi_internal_device_block() calls
> blk_mq_quiesce_queue() for each LUN. However synchronize_rcu is
> run from blk_mq_quiesce_queue().

Applied to 5.8/scsi-queue, thank you!

-- 
Martin K. Petersen	Oracle Linux Engineering