Re: [PATCH v7 9/9] block, scsi: Make SCSI quiesce and resume work reliably

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

On Tue, 2017-10-10 at 18:56 +0800, Ming Lei wrote:
> On Mon, Oct 09, 2017 at 04:14:00PM -0700, Bart Van Assche wrote:
> > [ ... ]
> >  int
> >  scsi_device_quiesce(struct scsi_device *sdev)
> >  {
> > +	struct request_queue *q = sdev->request_queue;
> >  	int err;
> >  
> > +	/* If the SCSI device already has been quiesced, do nothing. */
> > +	if (blk_set_preempt_only(q))
> > +		return 0;
> 
> This way isn't safe:
> 
> 1) suppose path1 sets the flag, and blk_mq_freeze_queue() isn't
> finished, or even not started;
> 
> 2) path2 sees the flag set at the exact time, and returns immediately,
> and unfortunately SCSI QUIESCE isn't respected in this context.

That comment applies to concurrent invocations of scsi_device_quiesce() only.
I think concurrent calls of scsi_device_quiesce() can only occur when power
management suspends or
hibernates a system that is equipped with a parallel
port and on which SCSI parallel domain validation occurs. I think that's a
very unlikely combination. And if we have to
address this, I propose to
disable power management during SCSI parallel domain validation, e.g. by
locking pm_mutex before SPI DV starts and by unlocking that mutex after SPI
DV
has finished. I think that will result in code that is easier to review
than the approach you proposed.

> > +	blk_mq_freeze_queue(q);
> > +	blk_mq_unfreeze_queue(q);
> 
> This way isn't safe too, because queue may has been frozen before
> scsi_device_quiesce() is run, then there isn't synchronize_rcu()
> implicated in blk_mq_freeze_queue().

It's very unlikely that this will cause trouble in practice. Anyway, the
previous version of this code did not show this race so I will change this
code fragment back to what I had in the previous version.

Bart.