On Sun, 2017-05-28 at 18:44 +0800, Ming Lei wrote:
> On Sat, May 27, 2017 at 09:46:45PM +0000, Bart Van Assche wrote:
> > On Sat, 2017-05-27 at 22:21 +0800, Ming Lei wrote:
> > > bool blk_mq_can_queue(struct blk_mq_hw_ctx *hctx)
> > > @@ -1108,13 +1119,15 @@ static void __blk_mq_run_hw_queue(struct
> > > blk_mq_hw_ctx *hctx)
> > >
> > > if (!(hctx->flags & BLK_MQ_F_BLOCKING)) {
> > > rcu_read_lock();
> > > - blk_mq_sched_dispatch_requests(hctx);
> > > + if (!blk_queue_quiesced(hctx->queue))
> > > + blk_mq_sched_dispatch_requests(hctx);
> > > rcu_read_unlock();
> > > } else {
> > > might_sleep();
> > >
> > > srcu_idx = srcu_read_lock(&hctx->queue_rq_srcu);
> > > - blk_mq_sched_dispatch_requests(hctx);
> > > + if (!blk_queue_quiesced(hctx->queue))
> > > + blk_mq_sched_dispatch_requests(hctx);
> > > srcu_read_unlock(&hctx->queue_rq_srcu, srcu_idx);
> > > }
> > > }
> >
> > Sorry but I don't like these changes. Why have the blk_queue_quiesced()
> > calls be added at other code locations than the blk_mq_hctx_stopped() calls?
> > This will make the block layer unnecessary hard to maintain. Please consider
> > to change the blk_mq_hctx_stopped(hctx) calls in
> > blk_mq_sched_dispatch_requests()
> > and *blk_mq_*run_hw_queue*() into blk_mq_hctx_stopped(hctx) ||
> > blk_queue_quiesced(q).
>
> One benefit is that we make it explicit that the flag has to be checked
> inside the RCU read-side critical sections. If you put it somewhere,
> someone may put it out of read-side critical sections in future.
Hello Ming,
I really would like to see the blk_queue_quiesced() tests as close as possible
to
the blk_mq_hctx_stopped() tests. But I agree that we need a way to document
and/or
verify that these tests occur with an RCU read-side lock held. Have you
considered
to use rcu_read_lock_held() to document this?
Thanks,
Bart.
