Re: [PATCH v3 5/6] block: Make SCSI device suspend and resume work reliably

[Ming Lei <ming.lei@redhat.com>]

On Mon, Sep 25, 2017 at 04:20:20PM +0000, Bart Van Assche wrote:
> On Mon, 2017-09-25 at 10:26 +0800, Ming Lei wrote:
> > On Fri, Sep 22, 2017 at 03:14:04PM -0700, Bart Van Assche wrote:
> > > +int blk_queue_enter(struct request_queue *q, bool nowait, bool preempt)
> > >  {
> > >  	while (true) {
> > >  		int ret;
> > >  
> > > -		if (percpu_ref_tryget_live(&q->q_usage_counter))
> > > -			return 0;
> > > +		if (percpu_ref_tryget_live(&q->q_usage_counter)) {
> > > +			/*
> > > +			 * Ensure read order of q_usage_counter and the
> > > +			 * PREEMPT_ONLY queue flag.
> > > +			 */
> > > +			smp_rmb();
> > > +			if (preempt || !blk_queue_preempt_only(q))
> > > +				return 0;
> > > +			else
> > > +				percpu_ref_put(&q->q_usage_counter);
> > > +		}
> > 
> > Now you introduce one smp_rmb() and test on preempt flag on
> > blk-mq's fast path, which should have been avoided, so I
> > think this way is worse than my patchset.
> 
> So that means that you have not noticed that it is safe to leave out that
> smp_rmp() call because blk-mq queue freezing and unfreezing waits for a grace
> period and hence waits until all CPUs have executed a full memory barrier?

No, memory barrier has to be pair, this barrier has to be there, another
one before unfreeze/freeze can be removed because it is implied inside
freeze/unfreeze.

Actually it should have been smp_mb() between writing the percpu-refcount
and reading preempt_only flag, otherwise if the two op is reordered,
queue freeze/unfreeze may see the counter is zero, and this normal I/O
still can be observed even after queue is freezed and SCSI is put into
quiesced.

-- 
Ming