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

[James Bottomley <James.Bottomley@HansenPartnership.com>]

On Tue, 2021-11-09 at 11:18 +0800, Ming Lei wrote:
> 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);

Yes, that looks fine ... it will at least act as a caution for
maintainers who come after us.

James