Re: [PATCH v3 3/9] blk-mq: use the introduced blk_mq_unquiesce_queue()

[Bart Van Assche <Bart.VanAssche@sandisk.com>]

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_devic=
e *sdev,
> > >  		return -EINVAL;
> > > =20
> > >  	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);
> >=20
> > As I commented on v2, this change is really wrong. All what's needed he=
re 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 impossibl=
e 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".
>=20
> I am not sure if I understand your idea, could you explain a bit why it i=
s wrong?
>=20
> Let's see the function of scsi_internal_device_block():
>=20
> 	if (q->mq_ops) {
>                 if (wait)
>                         blk_mq_quiesce_queue(q);
>                 else
>                         blk_mq_stop_hw_queues(q);
> 	}
>=20
> So the queue may be put into quiesced if 'wait' is true, or it is
> stopped if 'wait' is false.
>=20
> And this patch just makes the two SCSI APIs symmetrical.
>=20
> Since we will not stop queue in blk_mq_quiesce_queue() later,
> I have to unquiese one queue only if it is quiesced.
>=20
> 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_un=
block()
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 wait=
ing
until ongoing .queue_rq() calls have finished. The only driver that trigger=
s
that code path is the mpt3sas driver. I think it's unfortunate that that dr=
iver
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 allowe=
d.
No matter whether the scsi_internal_device_block() call from the mpt3sas dr=
iver
sets the QUEUE_FLAG_QUIESCED or the BLK_MQ_S_STOPPED flag, I don't think th=
at
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 worki=
ng on
about two months ago. See also https://www.spinics.net/lists/linux-scsi/msg=
109103.html.

Bart.=