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 09:12:29AM -0700, Keith Busch wrote:
> On Tue, Jul 28, 2020 at 09:49:38PM +0800, Ming Lei 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

We still can handle this case by not dispatch in case that percpu_ref_tryget()
returns false, which will change the usage into the following way:

        if (hctx_lock(hctx)) {
        	blk_mq_sched_dispatch_requests(hctx);
        	hctx_unlock(hctx);
		}

And __blk_mq_try_issue_directly() needs a bit special treatment because
the request has to be inserted to queue after queue becomes quiesced.


Thanks,
Ming