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
