Re: [PATCH 1/6] blk-mq: Make blk_mq_quiesce_queue() wait for all .queue_rq() calls

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

On Fri, Apr 28, 2017 at 03:35:59PM +0000, Bart Van Assche wrote:
> On Fri, 2017-04-28 at 10:00 +0800, Ming Lei wrote:
> > On Thu, Apr 27, 2017 at 08:54:32AM -0700, Bart Van Assche wrote:
> > >  void blk_mq_stop_hw_queue(struct blk_mq_hw_ctx *hctx)
> > >  {
> > > -	cancel_work(&hctx->run_work);
> > > -	cancel_delayed_work(&hctx->delay_work);
> > > +	cancel_work_sync(&hctx->run_work);
> > > +	cancel_delayed_work_sync(&hctx->delay_work);
> > 
> > Could you explain it a bit why we need the sync version?
> 
> Because the purpose of this patch is to make blk_mq_quiesce_queue() wait for
> all .queue_rq() calls.

If only .queue_rq() from this queue is waited, blk_mq_freeze_queue() is
enough. If you want to wait .queue_rq() from all queues,
blk_mq_quiesce_queue() can't do that too.

Firstly there shouldn't be a 'struct request_queue' parameter passed to this
function, because you want to wait for .queue_rq() from all queues.

Secondaly your current implementation can't guarantee that point:
	- only hw queues of this queue are stopped in this function, and
	.queue_rq() from other queues still can be run once
	synchronize_rcu() returns.

	- for BLOCKING case, the SRCU is per hctx.

Thirdly I am still not sure why we want to wait for all .queue_rq(),
could you explain it a bit?

At least for mq scheduler switch, looks all schedule data is per request
queue, and we shouldn't have needed to wait all .queue_rq().

> 
> > So I suggest to unity both .run_work and .dealyed_run_work
> > into one work, just as what Jens did in the following link:
> > 
> > 	http://marc.info/?t=149183989800010&r=1&w=2
> 
> That should be done after this patch is upstream otherwise this
> patch won't apply to the stable trees.

OK.

Thanks,
Ming