Hi Bart

On 09/25/2018 10:20 AM, Bart Van Assche wrote:

On 9/24/18 7:11 PM, jianchao.wang wrote:

quoted

Hi Keith

On 09/25/2018 05:09 AM, Keith Busch wrote:

quoted

-    /* A deadlock might occur if a request is stuck requiring a
-     * timeout at the same time a queue freeze is waiting
-     * completion, since the timeout code would not be able to
-     * acquire the queue reference here.
-     *
-     * That's why we don't use blk_queue_enter here; instead, we use
-     * percpu_ref_tryget directly, because we need to be able to
-     * obtain a reference even in the short window between the queue
-     * starting to freeze, by dropping the first reference in
-     * blk_freeze_queue_start, and the moment the last request is
-     * consumed, marked by the instant q_usage_counter reaches
-     * zero.
-     */
-    if (!percpu_ref_tryget(&q->q_usage_counter))
+    if (!blk_mq_queue_tag_busy_iter(q, blk_mq_check_expired, &next))
          return;

We cannot discard the percpu_ref_tryget here.

There following code in blk_mq_timeout_work still need it:

    if (next != 0) {
        mod_timer(&q->timeout, next);
    } else {
        queue_for_each_hw_ctx(q, hctx, i) {
            /* the hctx may be unmapped, so check it here */
            if (blk_mq_hw_queue_mapped(hctx))
                blk_mq_tag_idle(hctx);
        }
    }

Hi Jianchao,

Had you noticed that the percpu_ref_tryget() call has been moved into
blk_mq_queue_tag_busy_iter()?

Yes.

But the issue is the left part of blk_mq_timeout_work is moved out of protection of q refcount.

Thanks
Jianchao