diff mbox series

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

Message ID 20230612165049.29440-5-mwilck@suse.com (mailing list archive)
State New, archived
Headers show
Series scsi: fixes for targets with many LUNs, and scsi_target_block rework | expand

Commit Message

Martin Wilck June 12, 2023, 4:50 p.m. UTC 
From: Martin Wilck <mwilck@suse.com>

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/3b8b13bf-a458-827a-b916-07d7eee8ae00@acm.org/.

Signed-off-by: Martin Wilck <mwilck@suse.com>
Reviewed-by: Hannes Reinecke <hare@suse.de>
---
 drivers/scsi/scsi_lib.c | 32 ++++++++++++++++----------------
 1 file changed, 16 insertions(+), 16 deletions(-)

Comments

Bart Van Assche June 12, 2023, 6:02 p.m. UTC | #1 
On 6/12/23 09:50, mwilck@suse.com wrote:
> @@ -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);

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.
Christoph Hellwig June 13, 2023, 4:33 a.m. UTC | #2 
Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>
Martin Wilck June 13, 2023, 10:57 a.m. UTC | #3 
On Mon, 2023-06-12 at 11:02 -0700, Bart Van Assche wrote:
> On 6/12/23 09:50, mwilck@suse.com wrote:
> > @@ -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);
> 
> 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.

This code is just an intermediate stage. The double mutex_unlock() is
converted back to a single one in the subsequent patch. As Christoph
has already ack'd it, unless it's really important to you, I'd like to
keep this patch as-is.

Thanks,
Martin
diff mbox series

Patch

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 69fb7a9d8883..3e12cc61569d 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.
+	 * The caller needs 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);