On Thu, 2019-04-04 at 15:08 +0800, Ming Lei wrote:

On Wed, Apr 03, 2019 at 01:11:26PM -0700, Bart Van Assche wrote:

...

If blk_mq_try_issue_directly() returns BLK_STS*_RESOURCE that means that the request has not been queued and that the caller should retry to submit the request. Both blk_mq_request_bypass_insert() and blk_mq_sched_insert_request() guarantee that a request will be processed. Hence return BLK_STS_OK if one of these functions is called. This patch avoids that blk_mq_dispatch_rq_list() crashes when using dm-mpath.

Cc: Christoph Hellwig hch@infradead.org Cc: Hannes Reinecke hare@suse.com Cc: James Smart james.smart@broadcom.com Cc: Ming Lei ming.lei@redhat.com Cc: Jianchao Wang jianchao.w.wang@oracle.com Cc: Keith Busch keith.busch@intel.com Cc: Dongli Zhang dongli.zhang@oracle.com Cc: Laurence Oberman loberman@redhat.com Tested-by: Laurence Oberman loberman@redhat.com Reviewed-by: Laurence Oberman loberman@redhat.com Reported-by: Laurence Oberman loberman@redhat.com Fixes: 7f556a44e61d ("blk-mq: refactor the code of issue request directly") # v5.0. Cc: stable@vger.kernel.org Signed-off-by: Bart Van Assche bvanassche@acm.org

---

block/blk-mq.c | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/block/blk-mq.c b/block/blk-mq.c index 652d0c6d5945..b2c20dce8a30 100644 --- a/block/blk-mq.c +++ b/block/blk-mq.c @@ -1859,16 +1859,11 @@ blk_status_t blk_mq_try_issue_directly(struct blk_mq_hw_ctx *hctx, case BLK_STS_RESOURCE: if (force) { blk_mq_request_bypass_insert(rq, run_queue);

• 	/*
  

• 	 * We have to return BLK_STS_OK for the DM
  

• 	 * to avoid livelock. Otherwise, we return
  

• 	 * the real result to indicate whether the
  

• 	 * request is direct-issued successfully.
  

• 	 */
  

• 	ret = bypass ? BLK_STS_OK : ret;
  

• 	ret = BLK_STS_OK;
  

  } else if (!bypass) { blk_mq_sched_insert_request(rq, false, run_queue, false);

• 	ret = BLK_STS_OK;
  

  }

This change itself is correct.

However, there is other issue introduced by 7f556a44e61d.

We need blk_insert_cloned_request() to pass back BLK_STS_RESOURCE/BLK_STS_RESOURCE to caller, so that dm-rq driver may see the underlying queue is busy, then tell blk-mq to deal with the busy condition from dm-rq queue, so that IO merge can get improved.

That is exactly what 396eaf21ee17c476e8f6 ("blk-mq: improve DM's blk-mq IO merging via blk_insert_cloned_request feedback") did.

There must be performance regression with 7f556a44e61d by cut the feedback.

So could you fix them all in one patch?

Since commit 7f556a44e61d introduced multiple problems and since fixing these is nontrivial, how about reverting that commit?

Thanks,

Bart.

On Thu, 2019-04-04 at 11:09 -0400, Laurence Oberman wrote:

When I bisected and got to that commit I was unable to revert it to test without it. I showed that in an earlier update, had merge issues.

loberman@lobewsrhel linux_torvalds]$ git revert 7f556a44e61d error: could not revert 7f556a4... blk-mq: refactor the code of issue request directly hint: after resolving the conflicts, mark the corrected paths hint: with 'git add <paths>' or 'git rm <paths>' hint: and commit the result with 'git commit'

Hi Laurence,

On my setup the following commits revert cleanly if I revert them in the following order: * d6a51a97c0b2 ("blk-mq: replace and kill blk_mq_request_issue_directly") # v5.0. * 5b7a6f128aad ("blk-mq: issue directly with bypass 'false' in blk_mq_sched_insert_requests") # v5.0. * 7f556a44e61d ("blk-mq: refactor the code of issue request directly") # v5.0.

The result of these three reverts is the patch below. Test feedback for this patch would be appreciated.

Thanks,

Bart.

Subject: [PATCH] block: Revert recent blk_mq_request_issue_directly() changes

blk_mq_try_issue_directly() can return BLK_STS*_RESOURCE for requests that have been queued. If that happens when blk_mq_try_issue_directly() is called by the dm-mpath driver then the dm-mpath will try to resubmit a request that is already queued and a kernel crash follows. Since it is nontrivial to fix blk_mq_request_issue_directly(), revert the most recent blk_mq_request_issue_directly() changes.

This patch reverts the following commits: * d6a51a97c0b2 ("blk-mq: replace and kill blk_mq_request_issue_directly") # v5.0. * 5b7a6f128aad ("blk-mq: issue directly with bypass 'false' in blk_mq_sched_insert_requests") # v5.0. * 7f556a44e61d ("blk-mq: refactor the code of issue request directly") # v5.0.

Cc: Christoph Hellwig hch@infradead.org Cc: Hannes Reinecke hare@suse.com Cc: James Smart james.smart@broadcom.com Cc: Ming Lei ming.lei@redhat.com Cc: Jianchao Wang jianchao.w.wang@oracle.com Cc: Keith Busch keith.busch@intel.com Cc: Dongli Zhang dongli.zhang@oracle.com Cc: Laurence Oberman loberman@redhat.com Cc: stable@vger.kernel.org Reported-by: Laurence Oberman loberman@redhat.com Fixes: 7f556a44e61d ("blk-mq: refactor the code of issue request directly") # v5.0. Signed-off-by: Bart Van Assche bvanassche@acm.org --- block/blk-core.c | 4 +- block/blk-mq-sched.c | 8 +-- block/blk-mq.c | 122 ++++++++++++++++++++++--------------------- block/blk-mq.h | 6 +-- 4 files changed, 71 insertions(+), 69 deletions(-)

diff --git a/block/blk-core.c b/block/blk-core.c index 2921af6f8d33..5bca56016575 100644 --- a/block/blk-core.c +++ b/block/blk-core.c @@ -1232,8 +1232,6 @@ static int blk_cloned_rq_check_limits(struct request_queue *q, */ blk_status_t blk_insert_cloned_request(struct request_queue *q, struct request *rq) { - blk_qc_t unused; - if (blk_cloned_rq_check_limits(q, rq)) return BLK_STS_IOERR;

@@ -1249,7 +1247,7 @@ blk_status_t blk_insert_cloned_request(struct request_queue *q, struct request * * bypass a potential scheduler on the bottom device for * insert. */ - return blk_mq_try_issue_directly(rq->mq_hctx, rq, &unused, true, true); + return blk_mq_request_issue_directly(rq, true); } EXPORT_SYMBOL_GPL(blk_insert_cloned_request);

diff --git a/block/blk-mq-sched.c b/block/blk-mq-sched.c index 40905539afed..aa6bc5c02643 100644 --- a/block/blk-mq-sched.c +++ b/block/blk-mq-sched.c @@ -423,10 +423,12 @@ void blk_mq_sched_insert_requests(struct blk_mq_hw_ctx *hctx, * busy in case of 'none' scheduler, and this way may save * us one extra enqueue & dequeue to sw queue. */ - if (!hctx->dispatch_busy && !e && !run_queue_async) + if (!hctx->dispatch_busy && !e && !run_queue_async) { blk_mq_try_issue_list_directly(hctx, list); - else - blk_mq_insert_requests(hctx, ctx, list); + if (list_empty(list)) + return; + } + blk_mq_insert_requests(hctx, ctx, list); }

blk_mq_run_hw_queue(hctx, run_queue_async); diff --git a/block/blk-mq.c b/block/blk-mq.c index 652d0c6d5945..403984a557bb 100644 --- a/block/blk-mq.c +++ b/block/blk-mq.c @@ -1808,74 +1808,76 @@ static blk_status_t __blk_mq_issue_directly(struct blk_mq_hw_ctx *hctx, return ret; }

-blk_status_t blk_mq_try_issue_directly(struct blk_mq_hw_ctx *hctx, +static blk_status_t __blk_mq_try_issue_directly(struct blk_mq_hw_ctx *hctx, struct request *rq, blk_qc_t *cookie, - bool bypass, bool last) + bool bypass_insert, bool last) { struct request_queue *q = rq->q; bool run_queue = true; - blk_status_t ret = BLK_STS_RESOURCE; - int srcu_idx; - bool force = false;

- hctx_lock(hctx, &srcu_idx); /* - * hctx_lock is needed before checking quiesced flag. + * RCU or SRCU read lock is needed before checking quiesced flag. * - * When queue is stopped or quiesced, ignore 'bypass', insert - * and return BLK_STS_OK to caller, and avoid driver to try to - * dispatch again. + * When queue is stopped or quiesced, ignore 'bypass_insert' from + * blk_mq_request_issue_directly(), and return BLK_STS_OK to caller, + * and avoid driver to try to dispatch again. */ - if (unlikely(blk_mq_hctx_stopped(hctx) || blk_queue_quiesced(q))) { + if (blk_mq_hctx_stopped(hctx) || blk_queue_quiesced(q)) { run_queue = false; - bypass = false; - goto out_unlock; + bypass_insert = false; + goto insert; }

- if (unlikely(q->elevator && !bypass)) - goto out_unlock; + if (q->elevator && !bypass_insert) + goto insert;

if (!blk_mq_get_dispatch_budget(hctx)) - goto out_unlock; + goto insert;

if (!blk_mq_get_driver_tag(rq)) { blk_mq_put_dispatch_budget(hctx); - goto out_unlock; + goto insert; }

- /* - * Always add a request that has been through - *.queue_rq() to the hardware dispatch list. - */ - force = true; - ret = __blk_mq_issue_directly(hctx, rq, cookie, last); -out_unlock: + return __blk_mq_issue_directly(hctx, rq, cookie, last); +insert: + if (bypass_insert) + return BLK_STS_RESOURCE; + + blk_mq_request_bypass_insert(rq, run_queue); + return BLK_STS_OK; +} + +static void blk_mq_try_issue_directly(struct blk_mq_hw_ctx *hctx, + struct request *rq, blk_qc_t *cookie) +{ + blk_status_t ret; + int srcu_idx; + + might_sleep_if(hctx->flags & BLK_MQ_F_BLOCKING); + + hctx_lock(hctx, &srcu_idx); + + ret = __blk_mq_try_issue_directly(hctx, rq, cookie, false, true); + if (ret == BLK_STS_RESOURCE || ret == BLK_STS_DEV_RESOURCE) + blk_mq_request_bypass_insert(rq, true); + else if (ret != BLK_STS_OK) + blk_mq_end_request(rq, ret); + + hctx_unlock(hctx, srcu_idx); +} + +blk_status_t blk_mq_request_issue_directly(struct request *rq, bool last) +{ + blk_status_t ret; + int srcu_idx; + blk_qc_t unused_cookie; + struct blk_mq_hw_ctx *hctx = rq->mq_hctx; + + hctx_lock(hctx, &srcu_idx); + ret = __blk_mq_try_issue_directly(hctx, rq, &unused_cookie, true, last); hctx_unlock(hctx, srcu_idx); - switch (ret) { - case BLK_STS_OK: - break; - case BLK_STS_DEV_RESOURCE: - case BLK_STS_RESOURCE: - if (force) { - blk_mq_request_bypass_insert(rq, run_queue); - /* - * We have to return BLK_STS_OK for the DM - * to avoid livelock. Otherwise, we return - * the real result to indicate whether the - * request is direct-issued successfully. - */ - ret = bypass ? BLK_STS_OK : ret; - } else if (!bypass) { - blk_mq_sched_insert_request(rq, false, - run_queue, false); - } - break; - default: - if (!bypass) - blk_mq_end_request(rq, ret); - break; - }

return ret; } @@ -1883,20 +1885,22 @@ blk_status_t blk_mq_try_issue_directly(struct blk_mq_hw_ctx *hctx, void blk_mq_try_issue_list_directly(struct blk_mq_hw_ctx *hctx, struct list_head *list) { - blk_qc_t unused; - blk_status_t ret = BLK_STS_OK; - while (!list_empty(list)) { + blk_status_t ret; struct request *rq = list_first_entry(list, struct request, queuelist);

list_del_init(&rq->queuelist); - if (ret == BLK_STS_OK) - ret = blk_mq_try_issue_directly(hctx, rq, &unused, - false, + ret = blk_mq_request_issue_directly(rq, list_empty(list)); + if (ret != BLK_STS_OK) { + if (ret == BLK_STS_RESOURCE || + ret == BLK_STS_DEV_RESOURCE) { + blk_mq_request_bypass_insert(rq, list_empty(list)); - else - blk_mq_sched_insert_request(rq, false, true, false); + break; + } + blk_mq_end_request(rq, ret); + } }

/* @@ -1904,7 +1908,7 @@ void blk_mq_try_issue_list_directly(struct blk_mq_hw_ctx *hctx, * the driver there was more coming, but that turned out to * be a lie. */ - if (ret != BLK_STS_OK && hctx->queue->mq_ops->commit_rqs) + if (!list_empty(list) && hctx->queue->mq_ops->commit_rqs) hctx->queue->mq_ops->commit_rqs(hctx); }

@@ -2017,13 +2021,13 @@ static blk_qc_t blk_mq_make_request(struct request_queue *q, struct bio *bio) if (same_queue_rq) { data.hctx = same_queue_rq->mq_hctx; blk_mq_try_issue_directly(data.hctx, same_queue_rq, - &cookie, false, true); + &cookie); } } else if ((q->nr_hw_queues > 1 && is_sync) || (!q->elevator && !data.hctx->dispatch_busy)) { blk_mq_put_ctx(data.ctx); blk_mq_bio_to_request(rq, bio); - blk_mq_try_issue_directly(data.hctx, rq, &cookie, false, true); + blk_mq_try_issue_directly(data.hctx, rq, &cookie); } else { blk_mq_put_ctx(data.ctx); blk_mq_bio_to_request(rq, bio); diff --git a/block/blk-mq.h b/block/blk-mq.h index d704fc7766f4..423ea88ab6fb 100644 --- a/block/blk-mq.h +++ b/block/blk-mq.h @@ -70,10 +70,8 @@ void blk_mq_request_bypass_insert(struct request *rq, bool run_queue); void blk_mq_insert_requests(struct blk_mq_hw_ctx *hctx, struct blk_mq_ctx *ctx, struct list_head *list);

-blk_status_t blk_mq_try_issue_directly(struct blk_mq_hw_ctx *hctx, - struct request *rq, - blk_qc_t *cookie, - bool bypass, bool last); +/* Used by blk_insert_cloned_request() to issue request directly */ +blk_status_t blk_mq_request_issue_directly(struct request *rq, bool last); void blk_mq_try_issue_list_directly(struct blk_mq_hw_ctx *hctx, struct list_head *list);

On Thu, 2019-04-04 at 08:28 -0700, Bart Van Assche wrote:

On Thu, 2019-04-04 at 11:09 -0400, Laurence Oberman wrote:

...

When I bisected and got to that commit I was unable to revert it to test without it. I showed that in an earlier update, had merge issues.

loberman@lobewsrhel linux_torvalds]$ git revert 7f556a44e61d error: could not revert 7f556a4... blk-mq: refactor the code of issue request directly hint: after resolving the conflicts, mark the corrected paths hint: with 'git add <paths>' or 'git rm <paths>' hint: and commit the result with 'git commit'

Hi Laurence,

On my setup the following commits revert cleanly if I revert them in the following order:

• d6a51a97c0b2 ("blk-mq: replace and kill

blk_mq_request_issue_directly") # v5.0.

• 5b7a6f128aad ("blk-mq: issue directly with bypass 'false' in

blk_mq_sched_insert_requests") # v5.0.

• 7f556a44e61d ("blk-mq: refactor the code of issue request

directly") # v5.0.

The result of these three reverts is the patch below. Test feedback for this patch would be appreciated.

Thanks,

Bart.

Subject: [PATCH] block: Revert recent blk_mq_request_issue_directly() changes

blk_mq_try_issue_directly() can return BLK_STS*_RESOURCE for requests that have been queued. If that happens when blk_mq_try_issue_directly() is called by the dm-mpath driver then the dm-mpath will try to resubmit a request that is already queued and a kernel crash follows. Since it is nontrivial to fix blk_mq_request_issue_directly(), revert the most recent blk_mq_request_issue_directly() changes.

This patch reverts the following commits:

• d6a51a97c0b2 ("blk-mq: replace and kill

blk_mq_request_issue_directly") # v5.0.

• 5b7a6f128aad ("blk-mq: issue directly with bypass 'false' in

blk_mq_sched_insert_requests") # v5.0.

• 7f556a44e61d ("blk-mq: refactor the code of issue request

directly") # v5.0.

Cc: Christoph Hellwig hch@infradead.org Cc: Hannes Reinecke hare@suse.com Cc: James Smart james.smart@broadcom.com Cc: Ming Lei ming.lei@redhat.com Cc: Jianchao Wang jianchao.w.wang@oracle.com Cc: Keith Busch keith.busch@intel.com Cc: Dongli Zhang dongli.zhang@oracle.com Cc: Laurence Oberman loberman@redhat.com Cc: stable@vger.kernel.org Reported-by: Laurence Oberman loberman@redhat.com Fixes: 7f556a44e61d ("blk-mq: refactor the code of issue request directly") # v5.0. Signed-off-by: Bart Van Assche bvanassche@acm.org

---

block/blk-core.c | 4 +- block/blk-mq-sched.c | 8 +-- block/blk-mq.c | 122 ++++++++++++++++++++++------------------- -- block/blk-mq.h | 6 +-- 4 files changed, 71 insertions(+), 69 deletions(-)

diff --git a/block/blk-core.c b/block/blk-core.c index 2921af6f8d33..5bca56016575 100644 --- a/block/blk-core.c +++ b/block/blk-core.c @@ -1232,8 +1232,6 @@ static int blk_cloned_rq_check_limits(struct request_queue *q, */ blk_status_t blk_insert_cloned_request(struct request_queue *q, struct request *rq) {

• blk_qc_t unused;
• if (blk_cloned_rq_check_limits(q, rq)) return BLK_STS_IOERR;

@@ -1249,7 +1247,7 @@ blk_status_t blk_insert_cloned_request(struct request_queue *q, struct request * * bypass a potential scheduler on the bottom device for * insert. */

• return blk_mq_try_issue_directly(rq->mq_hctx, rq, &unused,

true, true);

• return blk_mq_request_issue_directly(rq, true);

} EXPORT_SYMBOL_GPL(blk_insert_cloned_request); diff --git a/block/blk-mq-sched.c b/block/blk-mq-sched.c index 40905539afed..aa6bc5c02643 100644 --- a/block/blk-mq-sched.c +++ b/block/blk-mq-sched.c @@ -423,10 +423,12 @@ void blk_mq_sched_insert_requests(struct blk_mq_hw_ctx *hctx, * busy in case of 'none' scheduler, and this way may save * us one extra enqueue & dequeue to sw queue. */

• if (!hctx->dispatch_busy && !e && !run_queue_async)
  

• if (!hctx->dispatch_busy && !e && !run_queue_async) {
  blk_mq_try_issue_list_directly(hctx, list);
  

• else
  

• 	blk_mq_insert_requests(hctx, ctx, list);
  

• 	if (list_empty(list))
  

• 		return;
  

• }
  

• blk_mq_insert_requests(hctx, ctx, list);
  

  }

blk_mq_run_hw_queue(hctx, run_queue_async); diff --git a/block/blk-mq.c b/block/blk-mq.c index 652d0c6d5945..403984a557bb 100644 --- a/block/blk-mq.c +++ b/block/blk-mq.c @@ -1808,74 +1808,76 @@ static blk_status_t __blk_mq_issue_directly(struct blk_mq_hw_ctx *hctx, return ret; } -blk_status_t blk_mq_try_issue_directly(struct blk_mq_hw_ctx *hctx, +static blk_status_t __blk_mq_try_issue_directly(struct blk_mq_hw_ctx *hctx, struct request *rq, blk_qc_t *cookie,

• 				bool bypass, bool last)
  

• 				bool bypass_insert,
  

bool last) { struct request_queue *q = rq->q; bool run_queue = true;

• blk_status_t ret = BLK_STS_RESOURCE;
• int srcu_idx;
• bool force = false;

• hctx_lock(hctx, &srcu_idx); /*

• * hctx_lock is needed before checking quiesced flag.
  

• * RCU or SRCU read lock is needed before checking quiesced
  

flag. *

• * When queue is stopped or quiesced, ignore 'bypass', insert
  

• * and return BLK_STS_OK to caller, and avoid driver to try to
  

• * dispatch again.
  

• * When queue is stopped or quiesced, ignore 'bypass_insert'
  

from

• * blk_mq_request_issue_directly(), and return BLK_STS_OK to
  

caller,

• * and avoid driver to try to dispatch again.
  

  */

• if (unlikely(blk_mq_hctx_stopped(hctx) ||

blk_queue_quiesced(q))) {

• if (blk_mq_hctx_stopped(hctx) || blk_queue_quiesced(q)) { run_queue = false;

• bypass = false;
  

• goto out_unlock;
  

• bypass_insert = false;
  

• goto insert;
  

  }

• if (unlikely(q->elevator && !bypass))

• goto out_unlock;
  

• if (q->elevator && !bypass_insert)

• goto insert;
  

if (!blk_mq_get_dispatch_budget(hctx))

• goto out_unlock;
  

• goto insert;
  

if (!blk_mq_get_driver_tag(rq)) { blk_mq_put_dispatch_budget(hctx);

• goto out_unlock;
  

• goto insert;
  

  }

• /*

• * Always add a request that has been through
  

• *.queue_rq() to the hardware dispatch list.
  

• */
  

• force = true;
• ret = __blk_mq_issue_directly(hctx, rq, cookie, last);

-out_unlock:

• return __blk_mq_issue_directly(hctx, rq, cookie, last);

+insert:

• if (bypass_insert)

• return BLK_STS_RESOURCE;
  

• blk_mq_request_bypass_insert(rq, run_queue);
• return BLK_STS_OK;

+}

+static void blk_mq_try_issue_directly(struct blk_mq_hw_ctx *hctx,

• struct request *rq, blk_qc_t *cookie)
  

+{

• blk_status_t ret;
• int srcu_idx;
• might_sleep_if(hctx->flags & BLK_MQ_F_BLOCKING);
• hctx_lock(hctx, &srcu_idx);
• ret = __blk_mq_try_issue_directly(hctx, rq, cookie, false,

true);

• if (ret == BLK_STS_RESOURCE || ret == BLK_STS_DEV_RESOURCE)

• blk_mq_request_bypass_insert(rq, true);
  

• else if (ret != BLK_STS_OK)

• blk_mq_end_request(rq, ret);
  

• hctx_unlock(hctx, srcu_idx);

+}

+blk_status_t blk_mq_request_issue_directly(struct request *rq, bool last) +{

• blk_status_t ret;
• int srcu_idx;
• blk_qc_t unused_cookie;
• struct blk_mq_hw_ctx *hctx = rq->mq_hctx;
• hctx_lock(hctx, &srcu_idx);
• ret = __blk_mq_try_issue_directly(hctx, rq, &unused_cookie,

true, last); hctx_unlock(hctx, srcu_idx);

• switch (ret) {
• case BLK_STS_OK:

• break;
  

• case BLK_STS_DEV_RESOURCE:
• case BLK_STS_RESOURCE:

• if (force) {
  

• 	blk_mq_request_bypass_insert(rq, run_queue);
  

• 	/*
  

• 	 * We have to return BLK_STS_OK for the DM
  

• 	 * to avoid livelock. Otherwise, we return
  

• 	 * the real result to indicate whether the
  

• 	 * request is direct-issued successfully.
  

• 	 */
  

• 	ret = bypass ? BLK_STS_OK : ret;
  

• } else if (!bypass) {
  

• 	blk_mq_sched_insert_request(rq, false,
  

• 				    run_queue, false);
  

• }
  

• break;
  

• default:

• if (!bypass)
  

• 	blk_mq_end_request(rq, ret);
  

• break;
  

• }

return ret; } @@ -1883,20 +1885,22 @@ blk_status_t blk_mq_try_issue_directly(struct blk_mq_hw_ctx *hctx, void blk_mq_try_issue_list_directly(struct blk_mq_hw_ctx *hctx, struct list_head *list) {

• blk_qc_t unused;
• blk_status_t ret = BLK_STS_OK;
• while (!list_empty(list)) {

• blk_status_t ret;
  

  struct request *rq = list_first_entry(list, struct

request, queuelist); list_del_init(&rq->queuelist);

• if (ret == BLK_STS_OK)
  

• 	ret = blk_mq_try_issue_directly(hctx, rq,
  

&unused,

• 					false,
  

• ret = blk_mq_request_issue_directly(rq,
  

list_empty(list));

• if (ret != BLK_STS_OK) {
  

• 	if (ret == BLK_STS_RESOURCE ||
  

• 			ret == BLK_STS_DEV_RESOURCE) {
  

• 		blk_mq_request_bypass_insert(rq,
  				list_empty(list
  

));

• else
  

• 	blk_mq_sched_insert_request(rq, false, true,
  

false);

• 		break;
  

• 	}
  

• 	blk_mq_end_request(rq, ret);
  

• }
  

  }

/* @@ -1904,7 +1908,7 @@ void blk_mq_try_issue_list_directly(struct blk_mq_hw_ctx *hctx, * the driver there was more coming, but that turned out to * be a lie. */

• if (ret != BLK_STS_OK && hctx->queue->mq_ops->commit_rqs)

• if (!list_empty(list) && hctx->queue->mq_ops->commit_rqs) hctx->queue->mq_ops->commit_rqs(hctx);

} @@ -2017,13 +2021,13 @@ static blk_qc_t blk_mq_make_request(struct request_queue *q, struct bio *bio) if (same_queue_rq) { data.hctx = same_queue_rq->mq_hctx; blk_mq_try_issue_directly(data.hctx, same_queue_rq,

• 			&cookie, false, true);
  

• 			&cookie);
  

  } } else if ((q->nr_hw_queues > 1 && is_sync) || (!q->elevator && !data.hctx->dispatch_busy)) { blk_mq_put_ctx(data.ctx); blk_mq_bio_to_request(rq, bio);

• blk_mq_try_issue_directly(data.hctx, rq, &cookie,
  

false, true);

• blk_mq_try_issue_directly(data.hctx, rq, &cookie);
  

  } else { blk_mq_put_ctx(data.ctx); blk_mq_bio_to_request(rq, bio);

diff --git a/block/blk-mq.h b/block/blk-mq.h index d704fc7766f4..423ea88ab6fb 100644 --- a/block/blk-mq.h +++ b/block/blk-mq.h @@ -70,10 +70,8 @@ void blk_mq_request_bypass_insert(struct request *rq, bool run_queue); void blk_mq_insert_requests(struct blk_mq_hw_ctx *hctx, struct blk_mq_ctx *ctx, struct list_head *list); -blk_status_t blk_mq_try_issue_directly(struct blk_mq_hw_ctx *hctx,

• 				struct request *rq,
  

• 				blk_qc_t *cookie,
  

• 				bool bypass, bool
  

last); +/* Used by blk_insert_cloned_request() to issue request directly */ +blk_status_t blk_mq_request_issue_directly(struct request *rq, bool last); void blk_mq_try_issue_list_directly(struct blk_mq_hw_ctx *hctx, struct list_head *list);

Hello Bart

I can conform, reverting those 3 in order also resolves the panic I was seeing. I have 3 reboot tests of the srpt server allowing the client ot remain stable and try reconnect.

For the above patch: Tested-by: Laurence Oberman loberman@redhat.com

Too many changes in code I am not familiar enough to review and comnment why reverting helps.

Thanks Laurence

On 4/8/19 10:36 AM, jianchao.wang wrote:

On 4/8/19 10:07 AM, jianchao.wang wrote:

...

Hi Bart

On 4/4/19 4:11 AM, Bart Van Assche wrote:

...

If blk_mq_try_issue_directly() returns BLK_STS*_RESOURCE that means that the request has not been queued and that the caller should retry to submit the request. Both blk_mq_request_bypass_insert() and blk_mq_sched_insert_request() guarantee that a request will be processed. Hence return BLK_STS_OK if one of these functions is called. This patch avoids that blk_mq_dispatch_rq_list() crashes when using dm-mpath.

Sorry, I seem to miss the original mail list that reported this issue. As your comment, it looks like that the request is handled again when the blk_mq_try_issue_directly return BLK_STS*_RESOURCE, right ?

The usage of this helper interface is, if care about the return value and want to handle the request yourself when return BLK_STS*_RESOURCE, pass 'byass' as true. otherwise, just pass 'bypass' as false, then blk_mq_try_issue_directly would take over all of the work including requeue or complete the request.

if dm-mpath case, the driver should only invoke dm_dispatch_clone_request, the 'bypass' parameter should only be true. as the blk_mq_try_issue_directly, it would return BLK_STS_OK when have to insert the request, otherwise, it would do nothing but return BLK_STS*_RESOURCE.

Would you please show the cause that the dm-mpath driver invoke blk_mq_try_issue_direclty with 'bypass == false' ?

The issue seems to be here,

blk_mq_try_issue_directly

if (unlikely(blk_mq_hctx_stopped(hctx) || blk_queue_quiesced(q))) {
    run_queue = false;
    bypass = false;          //------> HERE !!!
    goto out_unlock;
}


case BLK_STS_RESOURCE:
    if (force) {
        blk_mq_request_bypass_insert(rq, run_queue);
        ret = bypass ? BLK_STS_OK : ret;
    } else if (!bypass) {
        blk_mq_sched_insert_request(rq, false,
                        run_queue, false);
    }
    break;

Then the request will be inserted and blk_mq_try_issue_dreictly returns BLK_STS_RESOURCE.

Could following patch fix the issue ?

Hi Laurence

Would you please test this patch to see whether the issue could be fixed ?

Thanks Jianchao

diff --git a/block/blk-mq.c b/block/blk-mq.c index a9c1816..a3394f2 100644 --- a/block/blk-mq.c +++ b/block/blk-mq.c @@ -1813,7 +1813,7 @@ blk_status_t blk_mq_try_issue_directly(struct blk_mq_hw_ctx *hctx, */ if (unlikely(blk_mq_hctx_stopped(hctx) || blk_queue_quiesced(q))) { run_queue = false;

•           bypass = false;
  

•           force = true;
            goto out_unlock;
    }
  
  

Thanks Jianchao

...

...

Cc: Christoph Hellwig hch@infradead.org Cc: Hannes Reinecke hare@suse.com Cc: James Smart james.smart@broadcom.com Cc: Ming Lei ming.lei@redhat.com Cc: Jianchao Wang jianchao.w.wang@oracle.com Cc: Keith Busch keith.busch@intel.com Cc: Dongli Zhang dongli.zhang@oracle.com Cc: Laurence Oberman loberman@redhat.com Tested-by: Laurence Oberman loberman@redhat.com Reviewed-by: Laurence Oberman loberman@redhat.com Reported-by: Laurence Oberman loberman@redhat.com Fixes: 7f556a44e61d ("blk-mq: refactor the code of issue request directly") # v5.0. Cc: stable@vger.kernel.org Signed-off-by: Bart Van Assche bvanassche@acm.org

---

block/blk-mq.c | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/block/blk-mq.c b/block/blk-mq.c index 652d0c6d5945..b2c20dce8a30 100644 --- a/block/blk-mq.c +++ b/block/blk-mq.c @@ -1859,16 +1859,11 @@ blk_status_t blk_mq_try_issue_directly(struct blk_mq_hw_ctx *hctx, case BLK_STS_RESOURCE: if (force) { blk_mq_request_bypass_insert(rq, run_queue);

• 	/*
  

• 	 * We have to return BLK_STS_OK for the DM
  

• 	 * to avoid livelock. Otherwise, we return
  

• 	 * the real result to indicate whether the
  

• 	 * request is direct-issued successfully.
  

• 	 */
  

• 	ret = bypass ? BLK_STS_OK : ret;
  

• 	ret = BLK_STS_OK;
  

  } else if (!bypass) { blk_mq_sched_insert_request(rq, false, run_queue, false);

• 	ret = BLK_STS_OK;
  

  } break; default:

On 4/9/19 8:28 PM, Laurence Oberman wrote:

On Tue, 2019-04-09 at 09:31 +0800, jianchao.wang wrote:

...

On 4/8/19 10:36 AM, jianchao.wang wrote:

...

On 4/8/19 10:07 AM, jianchao.wang wrote:

...

Hi Bart

On 4/4/19 4:11 AM, Bart Van Assche wrote:

...

If blk_mq_try_issue_directly() returns BLK_STS*_RESOURCE that means that the request has not been queued and that the caller should retry to submit the request. Both blk_mq_request_bypass_insert() and blk_mq_sched_insert_request() guarantee that a request will be processed. Hence return BLK_STS_OK if one of these functions is called. This patch avoids that blk_mq_dispatch_rq_list() crashes when using dm- mpath.

Sorry, I seem to miss the original mail list that reported this issue. As your comment, it looks like that the request is handled again when the blk_mq_try_issue_directly return BLK_STS*_RESOURCE, right ?

The usage of this helper interface is, if care about the return value and want to handle the request yourself when return BLK_STS*_RESOURCE, pass 'byass' as true. otherwise, just pass 'bypass' as false, then blk_mq_try_issue_directly would take over all of the work including requeue or complete the request.

if dm-mpath case, the driver should only invoke dm_dispatch_clone_request, the 'bypass' parameter should only be true. as the blk_mq_try_issue_directly, it would return BLK_STS_OK when have to insert the request, otherwise, it would do nothing but return BLK_STS*_RESOURCE.

Would you please show the cause that the dm-mpath driver invoke blk_mq_try_issue_direclty with 'bypass == false' ?

The issue seems to be here,

blk_mq_try_issue_directly

if (unlikely(blk_mq_hctx_stopped(hctx) ||

blk_queue_quiesced(q))) { run_queue = false; bypass = false; //------> HERE !!! goto out_unlock; }

case BLK_STS_RESOURCE:
    if (force) {
        blk_mq_request_bypass_insert(rq, run_queue);
        ret = bypass ? BLK_STS_OK : ret;
    } else if (!bypass) {
        blk_mq_sched_insert_request(rq, false,
                        run_queue, false);
    }
    break;

Then the request will be inserted and blk_mq_try_issue_dreictly returns BLK_STS_RESOURCE.

Could following patch fix the issue ?

Hi Laurence

Would you please test this patch to see whether the issue could be fixed ?

Thanks Jianchao

...

diff --git a/block/blk-mq.c b/block/blk-mq.c index a9c1816..a3394f2 100644 --- a/block/blk-mq.c +++ b/block/blk-mq.c @@ -1813,7 +1813,7 @@ blk_status_t blk_mq_try_issue_directly(struct blk_mq_hw_ctx *hctx, */ if (unlikely(blk_mq_hctx_stopped(hctx) || blk_queue_quiesced(q))) { run_queue = false;

•           bypass = false;
  

•           force = true;
            goto out_unlock;
    }
  
  

Thanks Jianchao

...

Hello Sir I think Jens already took the revert patch though. I will try this when I gat a chance. Need to wait until I can reboot the targetserver again.

Thanks so much for your help. Please share the test result here. I will get the reverted patches back after that.

Thanks Jianchao