On Thu, Sep 10, 2020 at 09:53:21AM +0800, Ming Lei wrote:
> On Wed, Sep 09, 2020 at 09:04:09AM -0700, Keith Busch wrote:
> > On Wed, Sep 09, 2020 at 06:41:14PM +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;
> > > + bool blocking = !!(q->tag_set->flags & BLK_MQ_F_BLOCKING);
> > > + bool was_quiesced =__blk_mq_quiesce_queue_nowait(q);
> > >
> > > - __blk_mq_quiesce_queue_nowait(q);
> > > + if (!was_quiesced && blocking)
> > > + percpu_ref_kill(&q->dispatch_counter);
> > >
> > > - 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 (blocking)
> > > + wait_event(q->mq_quiesce_wq,
> > > + percpu_ref_is_zero(&q->dispatch_counter));
> > > + else
> > > synchronize_rcu();
> > > }
> >
> > In the previous version, you had ensured no thread can unquiesce a queue
> > while another is waiting for quiescence. Now that the locking is gone,
> > a thread could unquiesce the queue before percpu_ref reaches zero, so
> > the wait_event() may never complete on the resurrected percpu_ref.
> >
> > I don't think any drivers do such a thing today, but it just looks like
> > the implementation leaves open the possibility.
>
> This driver can cause bigger trouble if it unquiesces its queue which is
> being quiesced and still not done.
>
> However, we can avoid that by the following delta change:
>
> diff --git a/block/blk-mq.c b/block/blk-mq.c
> index 7669fe815cf9..5632727d71fa 100644
> --- a/block/blk-mq.c
> +++ b/block/blk-mq.c
> @@ -225,9 +225,16 @@ static void __blk_mq_quiesce_queue(struct request_queue *q, bool wait)
> if (!wait)
> return;
>
> + /*
> + * In case of F_BLOCKING, if driver unquiesces its queue which is being
> + * quiesced and still not done, it can cause bigger trouble, and we simply
> + * return & warn once for avoiding hang here.
> + */
> if (blocking)
> wait_event(q->mq_quiesce_wq,
> - percpu_ref_is_zero(&q->dispatch_counter));
> + percpu_ref_is_zero(&q->dispatch_counter) ||
> + WARN_ON_ONCE(!percpu_ref_is_dying(
> + &q->dispatch_counter)));
> else
> synchronize_rcu();
> }
Yeah, I'm okay with this. A warning and return should be good to
indicate driver sequence errors. So if you just want to fold the above
into this patch, then I don't think I have any remaining concerns.
The other race condition is if unquiesce resurrects the ref before
quiesce kills it, and there's already a warning there too.
