On Thu, 2017-06-01 at 08:54 +0800, Ming Lei wrote:
> On Wed, May 31, 2017 at 03:21:41PM +0000, Bart Van Assche wrote:
> > On Wed, 2017-05-31 at 20:37 +0800, Ming Lei wrote:
> > > diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> > > index 99e16ac479e3..ffcf05765e2b 100644
> > > --- a/drivers/scsi/scsi_lib.c
> > > +++ b/drivers/scsi/scsi_lib.c
> > > @@ -3031,7 +3031,10 @@ scsi_internal_device_unblock(struct scsi_device *sdev,
> > > return -EINVAL;
> > >
> > > if (q->mq_ops) {
> > > - blk_mq_start_stopped_hw_queues(q, false);
> > > + if (blk_queue_quiesced(q))
> > > + blk_mq_unquiesce_queue(q);
> > > + else
> > > + blk_mq_start_stopped_hw_queues(q, false);
> > > } else {
> > > spin_lock_irqsave(q->queue_lock, flags);
> > > blk_start_queue(q);
> >
> > As I commented on v2, this change is really wrong. All what's needed here is
> > a call to blk_mq_unquiesce_queue() and nothing else. Adding a call to
> > blk_mq_start_stopped_hw_queues() is wrong because it makes it impossible to
> > use the STOPPED flag in the SCSI core to make the block layer core stop calling
> > .queue_rq() if a SCSI LLD returns "busy".
>
> I am not sure if I understand your idea, could you explain a bit why it is wrong?
>
> Let's see the function of scsi_internal_device_block():
>
> if (q->mq_ops) {
> if (wait)
> blk_mq_quiesce_queue(q);
> else
> blk_mq_stop_hw_queues(q);
> }
>
> So the queue may be put into quiesced if 'wait' is true, or it is
> stopped if 'wait' is false.
>
> And this patch just makes the two SCSI APIs symmetrical.
>
> Since we will not stop queue in blk_mq_quiesce_queue() later,
> I have to unquiese one queue only if it is quiesced.
>
> So suppose the queue is put into stopped in scsi_internal_device_block(),
> do we expect not to restart it in scsi_internal_device_unblock() via
> blk_mq_unquiesce_queue()?
Hello Ming,
My opinion is that scsi_internal_device_block() and scsi_internal_device_unblock()
should be changed as follows for the scsi-mq code path:
* scsi_internal_device_block() should call blk_mq_quiesce_queue(q) if the "wait"
argument is true and should set the QUEUE_FLAG_QUIESCED flag if the "wait"
argument is false.
* scsi_internal_device_unblock() should call blk_mq_unquiesce_queue().
I am aware it sounds weird to set the QUEUE_FLAG_QUIESCED flag without waiting
until ongoing .queue_rq() calls have finished. The only driver that triggers
that code path is the mpt3sas driver. I think it's unfortunate that that driver
has ever been allowed to call scsi_internal_device_block() because it's the only
driver that calls that function from a context where sleeping is not allowed.
No matter whether the scsi_internal_device_block() call from the mpt3sas driver
sets the QUEUE_FLAG_QUIESCED or the BLK_MQ_S_STOPPED flag, I don't think that
will have the effect the authors of this driver intended. Unfortunately I'm not
familiar enough with the mpt3sas driver to fix that driver myself.
Note: these changes conflict with a patch SCSI patch series I started working on
about two months ago. See also https://www.spinics.net/lists/linux-scsi/msg109103.html.
Bart.
