Re: [PATCH v5 4/7] scsi: don't wait for quiesce in scsi_stop_queue()

[Martin Wilck <mwilck@suse.com>]

On Mon, 2023-06-12 at 11:02 -0700, Bart Van Assche wrote:
> On 6/12/23 09:50, mwilck@suse.com wrote:
> > @@ -2800,9 +2792,17 @@ static void scsi_device_block(struct
> > scsi_device *sdev, void *data)
> >   
> >         mutex_lock(&sdev->state_mutex);
> >         err = __scsi_internal_device_block_nowait(sdev);
> > -       if (err == 0)
> > -               scsi_stop_queue(sdev, false);
> > -       mutex_unlock(&sdev->state_mutex);
> > +       if (err == 0) {
> > +               /*
> > +                * scsi_stop_queue() must be called with the
> > state_mutex
> > +                * held. Otherwise a simultaneous
> > scsi_start_queue() call
> > +                * might unquiesce the queue before we quiesce it.
> > +                */
> > +               scsi_stop_queue(sdev);
> > +               mutex_unlock(&sdev->state_mutex);
> > +               blk_mq_wait_quiesce_done(sdev->request_queue-
> > >tag_set);
> > +       } else
> > +               mutex_unlock(&sdev->state_mutex);
> >   
> >         WARN_ONCE(err, "__scsi_internal_device_block_nowait(%s)
> > failed: err = %d\n",
> >                   dev_name(&sdev->sdev_gendev), err);
> 
> Has it been considered to modify the above code such that there is a
> single mutex_unlock() call instead of two? I wouldn't mind if
> blk_mq_wait_quiesce_done() would be called if err != 0 since
> performance
> is not that important if this function fails.

This code is just an intermediate stage. The double mutex_unlock() is
converted back to a single one in the subsequent patch. As Christoph
has already ack'd it, unless it's really important to you, I'd like to
keep this patch as-is.

Thanks,
Martin