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.
