Re: [PATCH 3/4] scsi: make sure that request queue queiesce and unquiesce balanced

[Ming Lei <ming.lei@redhat.com>]

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:
> > On Wed, 2021-11-03 at 11:43 +0800, Ming Lei wrote:
> > [...]
> > > +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-
> > > >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.
> 
> > 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