Re: [RFC PATCH] blk-mq: implement queue quiesce via percpu_ref for BLK_MQ_F_BLOCKING

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

On Wed, Jul 29, 2020 at 03:42:29PM -0700, Sagi Grimberg wrote:
> 
> > > >   void blk_mq_quiesce_queue(struct request_queue *q)
> > > >   {
> > > > -	struct blk_mq_hw_ctx *hctx;
> > > > -	unsigned int i;
> > > > -	bool rcu = false;
> > > > -
> > > >   	blk_mq_quiesce_queue_nowait(q);
> > > > -	queue_for_each_hw_ctx(q, hctx, i) {
> > > > -		if (hctx->flags & BLK_MQ_F_BLOCKING)
> > > > -			synchronize_srcu(hctx->srcu);
> > > > -		else
> > > > -			rcu = true;
> > > > -	}
> > > > -	if (rcu)
> > > > +	if (q->tag_set->flags & BLK_MQ_F_BLOCKING) {
> > > > +		percpu_ref_kill(&q->dispatch_counter);
> > > > +		wait_event(q->mq_quiesce_wq,
> > > > +				percpu_ref_is_zero(&q->dispatch_counter));
> > > > +	} else
> > > >   		synchronize_rcu();
> > > >   }
> > > 
> > > 
> > > 
> > > > +static void hctx_lock(struct blk_mq_hw_ctx *hctx)
> > > >   {
> > > > -	if (!(hctx->flags & BLK_MQ_F_BLOCKING)) {
> > > > -		/* shut up gcc false positive */
> > > > -		*srcu_idx = 0;
> > > > +	if (!(hctx->flags & BLK_MQ_F_BLOCKING))
> > > >   		rcu_read_lock();
> > > > -	} else
> > > > -		*srcu_idx = srcu_read_lock(hctx->srcu);
> > > > +	else
> > > > +		percpu_ref_get(&hctx->queue->dispatch_counter);
> > > >   }
> > > 
> > > percpu_ref_get() will always succeed, even after quiesce kills it.
> > > Isn't it possible that 'percpu_ref_is_zero(&q->dispatch_counter))' may
> > > never reach 0? We only need to ensure that dispatchers will observe
> > > blk_queue_quiesced(). That doesn't require that there are no current
> > > dispatchers.
> > 
> > IMO it shouldn't be one issue in reality, because:
> > 
> > - when dispatch can't make progress, the submission side will finally
> >    stop because we either run queue from submission side or request
> >    completion
> > - submission side stops because we always have very limited requests
> > 
> > - completion side stops because requests queued to device is limited
> > too
> 
> I don't think that any requests should pass after the kill was called,
> otherwise how can we safely quiesce if requests can come in after
> it?

What we guarantee is that no request can be queued to LLD after
blk_mq_quiesce_queue returns.

With percpu_refcount, once percpu_ref_is_zero(&q->dispatch_counter)
returns true, all code path can observe the QUIESCED flag reliably just
like what SRCU does, so no any request can pass to LLD after blk_mq_quiesce_queue
returns.

> 
> > 
> > We still can handle this case by not dispatch in case that percpu_ref_tryget()
> 
> You meant tryget_live right?

Both works, but tryget_live could be better.


Thanks, 
Ming