Re: [PATCH v3 4/8] scsi: call scsi_stop_queue() without state_mutex held

[Martin Wilck <mwilck@suse.com>]

Bart,

On Wed, 2023-06-07 at 21:37 +0200, Martin Wilck wrote:
> On Wed, 2023-06-07 at 12:16 -0700, Bart Van Assche wrote:
> > On 6/7/23 11:22, mwilck@suse.com wrote:
> > > From: Martin Wilck <mwilck@suse.com>
> > > 
> > > sdev->state_mutex protects only sdev->sdev_state. There's no
> > > reason
> > > to keep it held while calling scsi_stop_queue().
> > > 
> > > Signed-off-by: Martin Wilck <mwilck@suse.com>
> > > ---
> > >   drivers/scsi/scsi_lib.c | 2 +-
> > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> > > index ce5788643011..26e7ce25fa05 100644
> > > --- a/drivers/scsi/scsi_lib.c
> > > +++ b/drivers/scsi/scsi_lib.c
> > > @@ -2795,9 +2795,9 @@ static void scsi_device_block(struct
> > > scsi_device *sdev, void *data)
> > >   
> > >         mutex_lock(&sdev->state_mutex);
> > >         err = __scsi_internal_device_block_nowait(sdev);
> > > +       mutex_unlock(&sdev->state_mutex);
> > >         if (err == 0)
> > >                 scsi_stop_queue(sdev, false);
> > > -       mutex_unlock(&sdev->state_mutex);
> > >   
> > >         WARN_ONCE(err, "__scsi_internal_device_block_nowait(%s)
> > > failed: err = %d\n",
> > >                   dev_name(&sdev->sdev_gendev), err);
> > 
> > There is a reason why scsi_stop_queue() is called with the sdev
> > state
> > mutex held: if this mutex is not held, unblocking of a SCSI device
> > can 
> > start before the scsi_stop_queue() call has finished. It is not
> > allowed 
> > to swap the order of the blk_mq_quiesce_queue() and 
> > blk_mq_unquiesce_queue() calls.
> 
> Thanks. This wasn't obvious to me from the current code. I'll add a
> comment in the next version.

The crucial question is now, is it sufficient to call
blk_mq_quiesce_queue_nowait() under the mutex, or does the call to
blk_mq_wait_quiesce_done() have to be under the mutex, too?
The latter would actually kill off our attempt to fix the delay
in fc_remote_port_delete() that was caused by repeated
synchronize_rcu() calls.

But if I understand you correctly, moving the wait out of the mutex
should be ok. I'll update the series accordingly.

Thanks,
Martin