From patchwork Wed Feb 7 21:19:20 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Paolo Valente X-Patchwork-Id: 127210 Delivered-To: patch@linaro.org Received: by 10.46.124.24 with SMTP id x24csp862006ljc; Wed, 7 Feb 2018 13:19:42 -0800 (PST) X-Google-Smtp-Source: AH8x2269Np0ya5/Hppkf9MRDmPJ244JzmEpYAYdjRhYeIjUnXmAx9HKQF2CdE5uI8Jim4iNopl32 X-Received: by 10.98.137.213 with SMTP id n82mr7236340pfk.175.1518038382040; Wed, 07 Feb 2018 13:19:42 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1518038382; cv=none; d=google.com; s=arc-20160816; b=b0Hr/mNlG0i6FSQCm6vgXYoM3uhmF8JZxTvmcTYVJ+cR2XSw0Xvd4QgKigJ5jXPFT8 MNMazR7M2LkXrwC9tpO8ydfjX4IUzwVr6CQssMCjQu1YRP6def5CA3ONpQKba/XxGxpZ /aFYNbIhjSCmR0Ei6g/1BMPPCivee9/l0134aHtu2zcFgIIb+tLQbgGDTojaS5jmJ865 rZwORjc0V/qjWo5HjH0ICKln+KzKAYdlkYoJjG82p9O+VFmZyf3X8p+7eg6rTfGOWQZW 9+rNVu5iDjMAJP194ZdQHtxDfertBh5+NGznmQxOfnXxnr3bCYE4+vvC5R0wI8Qi5JQg etDw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:message-id:date:subject:cc:to:from :dkim-signature:arc-authentication-results; bh=5Qq/ggdf3xJF953RGJpccjpQ8FYxHnR3dpUr8/POU+c=; b=liTQ8s0pdvlDW5NfN3TOnlccPU0SIwnTpIZ3cwQO4fnVFBS5uaEi+R0ixWXPgUg6T+ U+nbIjoTupznG+zEg0LvEPtv/lfWa41ohaNrI9rbqzWr4kLkArH7iiCn3wfBTlkqFn8e W+UyNH7D6Zw904ay1vlOYDsowsmdHIktQR3DySdh3xYskZDsO/xpQn7DhzV8DOZEqu6U 8owU9i4itcUK/K739+miKRS5JXqWUaHDqyBagtS+bn/s+6ZsTDME2ibEgDiIAaS9dt+C kygCOKZ6xm8CYsRb6j3blDSfiDibeu1Cm/A8KaQEh0rrkXvyFh4V2o8EDboXutQeEnQO SRRg== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@linaro.org header.s=google header.b=K56b119w; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=linaro.org Return-Path: Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id 63-v6si361977pla.526.2018.02.07.13.19.41; Wed, 07 Feb 2018 13:19:42 -0800 (PST) Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67; Authentication-Results: mx.google.com; dkim=pass header.i=@linaro.org header.s=google header.b=K56b119w; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=linaro.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932267AbeBGVTi (ORCPT + 11 others); Wed, 7 Feb 2018 16:19:38 -0500 Received: from mail-wm0-f65.google.com ([74.125.82.65]:34398 "EHLO mail-wm0-f65.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932244AbeBGVTf (ORCPT ); Wed, 7 Feb 2018 16:19:35 -0500 Received: by mail-wm0-f65.google.com with SMTP id j21-v6so24434015wmh.1 for ; Wed, 07 Feb 2018 13:19:35 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=from:to:cc:subject:date:message-id; bh=5Qq/ggdf3xJF953RGJpccjpQ8FYxHnR3dpUr8/POU+c=; b=K56b119whEZvp7mRBmhNkey97Ih2y/PqBOfQB9wHHQavsibfqCSf1fGmJ821GeUMOg 6gSPvohun8X+99Nl8sTlUQsyfopQ8/a4d6ZmopffBhRJ8cTySItvTVjup7lK9wkooZgQ gSJCIBT9Bfcw+XBYVnVTCfA2Gat0GHQv/vCfU= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:date:message-id; bh=5Qq/ggdf3xJF953RGJpccjpQ8FYxHnR3dpUr8/POU+c=; b=dW8wBinXtO6e11jd+CAxRo9Rd2rxNraL7eSqGSR1HJdlNrEjZfP1QekqICRtajCp2T TIvC+2DCFbug5v8Vg6rbNUkzzA/Y1k6IC6+sYl7rEHTblyoLPHyyvqUDwfzfpY6CCUEW FXRnbmyzugGTQk320BHfEFT9PxwcwNsYfgKwYd81mxwzmdCj1x8pUunBgkC4PAIrNs4+ GfxerfA4ipApyo3qz/KyjtMZZsvc1yqYT6XBttZEwUNqQSHLzU4PZfLv4MttmkFccsnM i9DprmBdJ5EZJ/s7iL5pjuFA9ziapzRCiAHSSsPRcZM3ta00FJRPJwp8iKCPrFO4/tBd iubA== X-Gm-Message-State: APf1xPA5GScfDLL7jB/wE12ObCt2gzUSmxJ1f2thnjfF2NtIUSdJzDYi UdxXajazCnSGBowoK3kX0CgHlw== X-Received: by 10.28.15.131 with SMTP id 125mr6122069wmp.24.1518038374192; Wed, 07 Feb 2018 13:19:34 -0800 (PST) Received: from localhost.localdomain (146-241-58-49.dyn.eolo.it. [146.241.58.49]) by smtp.gmail.com with ESMTPSA id 137sm2701713wmm.46.2018.02.07.13.19.32 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Wed, 07 Feb 2018 13:19:33 -0800 (PST) From: Paolo Valente To: Jens Axboe Cc: linux-block@vger.kernel.org, linux-kernel@vger.kernel.org, ulf.hansson@linaro.org, broonie@kernel.org, linus.walleij@linaro.org, bfq-iosched@googlegroups.com, oleksandr@natalenko.name, alban.browaeys@gmail.com, ming.lei@redhat.com, ivan@ludios.org, 169364@studenti.unimore.it, holger@applied-asynchrony.com, efault@gmx.de, Paolo Valente , Serena Ziviani Subject: [PATCH BUGFIX V3] block, bfq: add requeue-request hook Date: Wed, 7 Feb 2018 22:19:20 +0100 Message-Id: <20180207211920.6343-1-paolo.valente@linaro.org> X-Mailer: git-send-email 2.15.1 Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Commit 'a6a252e64914 ("blk-mq-sched: decide how to handle flush rq via RQF_FLUSH_SEQ")' makes all non-flush re-prepared requests for a device be re-inserted into the active I/O scheduler for that device. As a consequence, I/O schedulers may get the same request inserted again, even several times, without a finish_request invoked on that request before each re-insertion. This fact is the cause of the failure reported in [1]. For an I/O scheduler, every re-insertion of the same re-prepared request is equivalent to the insertion of a new request. For schedulers like mq-deadline or kyber, this fact causes no harm. In contrast, it confuses a stateful scheduler like BFQ, which keeps state for an I/O request, until the finish_request hook is invoked on the request. In particular, BFQ may get stuck, waiting forever for the number of request dispatches, of the same request, to be balanced by an equal number of request completions (while there will be one completion for that request). In this state, BFQ may refuse to serve I/O requests from other bfq_queues. The hang reported in [1] then follows. However, the above re-prepared requests undergo a requeue, thus the requeue_request hook of the active elevator is invoked for these requests, if set. This commit then addresses the above issue by properly implementing the hook requeue_request in BFQ. [1] https://marc.info/?l=linux-block&m=151211117608676 Reported-by: Ivan Kozik Reported-by: Alban Browaeys Tested-by: Mike Galbraith Signed-off-by: Paolo Valente Signed-off-by: Serena Ziviani --- V2: contains fix to bug reported in [2] V3: implements the improvement suggested in [3] [2] https://lkml.org/lkml/2018/2/5/599 [3] https://lkml.org/lkml/2018/2/7/532 block/bfq-iosched.c | 107 ++++++++++++++++++++++++++++++++++++++++------------ 1 file changed, 82 insertions(+), 25 deletions(-) -- 2.15.1 Tested-by: Oleksandr Natalenko diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c index 47e6ec7427c4..aeca22d91101 100644 --- a/block/bfq-iosched.c +++ b/block/bfq-iosched.c @@ -3823,24 +3823,26 @@ static struct request *__bfq_dispatch_request(struct blk_mq_hw_ctx *hctx) } /* - * We exploit the bfq_finish_request hook to decrement - * rq_in_driver, but bfq_finish_request will not be - * invoked on this request. So, to avoid unbalance, - * just start this request, without incrementing - * rq_in_driver. As a negative consequence, - * rq_in_driver is deceptively lower than it should be - * while this request is in service. This may cause - * bfq_schedule_dispatch to be invoked uselessly. + * We exploit the bfq_finish_requeue_request hook to + * decrement rq_in_driver, but + * bfq_finish_requeue_request will not be invoked on + * this request. So, to avoid unbalance, just start + * this request, without incrementing rq_in_driver. As + * a negative consequence, rq_in_driver is deceptively + * lower than it should be while this request is in + * service. This may cause bfq_schedule_dispatch to be + * invoked uselessly. * * As for implementing an exact solution, the - * bfq_finish_request hook, if defined, is probably - * invoked also on this request. So, by exploiting - * this hook, we could 1) increment rq_in_driver here, - * and 2) decrement it in bfq_finish_request. Such a - * solution would let the value of the counter be - * always accurate, but it would entail using an extra - * interface function. This cost seems higher than the - * benefit, being the frequency of non-elevator-private + * bfq_finish_requeue_request hook, if defined, is + * probably invoked also on this request. So, by + * exploiting this hook, we could 1) increment + * rq_in_driver here, and 2) decrement it in + * bfq_finish_requeue_request. Such a solution would + * let the value of the counter be always accurate, + * but it would entail using an extra interface + * function. This cost seems higher than the benefit, + * being the frequency of non-elevator-private * requests very low. */ goto start_rq; @@ -4515,6 +4517,8 @@ static inline void bfq_update_insert_stats(struct request_queue *q, unsigned int cmd_flags) {} #endif +static void bfq_prepare_request(struct request *rq, struct bio *bio); + static void bfq_insert_request(struct blk_mq_hw_ctx *hctx, struct request *rq, bool at_head) { @@ -4541,6 +4545,18 @@ static void bfq_insert_request(struct blk_mq_hw_ctx *hctx, struct request *rq, else list_add_tail(&rq->queuelist, &bfqd->dispatch); } else { + if (WARN_ON_ONCE(!bfqq)) { + /* + * This should never happen. Most likely rq is + * a requeued regular request, being + * re-inserted without being first + * re-prepared. Do a prepare, to avoid + * failure. + */ + bfq_prepare_request(rq, rq->bio); + bfqq = RQ_BFQQ(rq); + } + idle_timer_disabled = __bfq_insert_request(bfqd, rq); /* * Update bfqq, because, if a queue merge has occurred @@ -4697,22 +4713,44 @@ static void bfq_completed_request(struct bfq_queue *bfqq, struct bfq_data *bfqd) bfq_schedule_dispatch(bfqd); } -static void bfq_finish_request_body(struct bfq_queue *bfqq) +static void bfq_finish_requeue_request_body(struct bfq_queue *bfqq) { bfqq->allocated--; bfq_put_queue(bfqq); } -static void bfq_finish_request(struct request *rq) +/* + * Handle either a requeue or a finish for rq. The things to do are + * the same in both cases: all references to rq are to be dropped. In + * particular, rq is considered completed from the point of view of + * the scheduler. + */ +static void bfq_finish_requeue_request(struct request *rq) { - struct bfq_queue *bfqq; + struct bfq_queue *bfqq = RQ_BFQQ(rq); struct bfq_data *bfqd; - if (!rq->elv.icq) + /* + * Requeue and finish hooks are invoked in blk-mq without + * checking whether the involved request is actually still + * referenced in the scheduler. To handle this fact, the + * following two checks make this function exit in case of + * spurious invocations, for which there is nothing to do. + * + * First, check whether rq has nothing to do with an elevator. + */ + if (unlikely(!(rq->rq_flags & RQF_ELVPRIV))) + return; + + /* + * rq either is not associated with any icq, or is an already + * requeued request that has not (yet) been re-inserted into + * a bfq_queue. + */ + if (!rq->elv.icq || !bfqq) return; - bfqq = RQ_BFQQ(rq); bfqd = bfqq->bfqd; if (rq->rq_flags & RQF_STARTED) @@ -4727,13 +4765,14 @@ static void bfq_finish_request(struct request *rq) spin_lock_irqsave(&bfqd->lock, flags); bfq_completed_request(bfqq, bfqd); - bfq_finish_request_body(bfqq); + bfq_finish_requeue_request_body(bfqq); spin_unlock_irqrestore(&bfqd->lock, flags); } else { /* * Request rq may be still/already in the scheduler, - * in which case we need to remove it. And we cannot + * in which case we need to remove it (this should + * never happen in case of requeue). And we cannot * defer such a check and removal, to avoid * inconsistencies in the time interval from the end * of this function to the start of the deferred work. @@ -4748,9 +4787,26 @@ static void bfq_finish_request(struct request *rq) bfqg_stats_update_io_remove(bfqq_group(bfqq), rq->cmd_flags); } - bfq_finish_request_body(bfqq); + bfq_finish_requeue_request_body(bfqq); } + /* + * Reset private fields. In case of a requeue, this allows + * this function to correctly do nothing if it is spuriously + * invoked again on this same request (see the check at the + * beginning of the function). Probably, a better general + * design would be to prevent blk-mq from invoking the requeue + * or finish hooks of an elevator, for a request that is not + * referred by that elevator. + * + * Resetting the following fields would break the + * request-insertion logic if rq is re-inserted into a bfq + * internal queue, without a re-preparation. Here we assume + * that re-insertions of requeued requests, without + * re-preparation, can happen only for pass_through or at_head + * requests (which are not re-inserted into bfq internal + * queues). + */ rq->elv.priv[0] = NULL; rq->elv.priv[1] = NULL; } @@ -5426,7 +5482,8 @@ static struct elevator_type iosched_bfq_mq = { .ops.mq = { .limit_depth = bfq_limit_depth, .prepare_request = bfq_prepare_request, - .finish_request = bfq_finish_request, + .requeue_request = bfq_finish_requeue_request, + .finish_request = bfq_finish_requeue_request, .exit_icq = bfq_exit_icq, .insert_requests = bfq_insert_requests, .dispatch_request = bfq_dispatch_request,