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
