From patchwork Wed Feb 7 18:00:50 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Paolo Valente X-Patchwork-Id: 127205 Delivered-To: patch@linaro.org Received: by 10.46.124.24 with SMTP id x24csp711790ljc; Wed, 7 Feb 2018 10:01:18 -0800 (PST) X-Google-Smtp-Source: AH8x227LWzAtU0q3Xy1jvrEm+SJEkYRKTuhPHJZ9AWB2Q68GTldLVEHD4Z2KF1nQzjltOkYtKQz7 X-Received: by 10.98.149.69 with SMTP id p66mr6726374pfd.183.1518026478234; Wed, 07 Feb 2018 10:01:18 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1518026478; cv=none; d=google.com; s=arc-20160816; b=AIgR2g/hIdetYuGwScG+xLYX+3wY9siEjfaFaFF0sdVLs5ZJj0BzVwohOlkULvCWj9 Uy6o9lYGOGKeXbY/8VAAK050UWs1eyQaFHvGNiwyZZovDLKeednd9eenrh/YEcS6YdBl In11gTt0jxeyS635TegOskgleYMhAqkgquVjA9y3i8O4T8nNT+6AmRtiV5EEdvFGnHMO cLvD9WWbg6jW2AvCYfvOLUwp0IiqGtg+WOL9ryIGOuUYlIgfQDfHr/uAtucOXECXnUQC BtVTVRRR/eB2C/mTOpSO85Ky5sWCYzb8BGpVEQJmN0IvIYBY/gBLUnX/G6fVand0EOXo A6vQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:references:in-reply-to:message-id:date :subject:cc:to:from:dkim-signature:arc-authentication-results; bh=e/w5xop08AXK/QFABXyb0/dkitF+a5sbLjf1oR1j35I=; b=eUoJlbeS6s+PaIGgJ+gN6tRDkDzNZd4nWo+QYCQg6KKEGEx8delBbZqkKod8aQHZKH FckNphiG7rdjKnlJvXpi6HilrC1zRsthKK2VUe41Np3FLUZwX8a5LViutpILOz0twp/j N17ifHflpUhIs9TKXcY7bx5d40ZNUI3JE0j3oxUPX8E8y2UUWB+bgHFPE/cFBlumVn2u laXz3IArK+pvzSS9k8Qp+57VWJvmktt7gX1iGZsrJWZOXhSWTirShEeRFG31p2e/BdSc oSHTJqEwqPMxgVFi5JogkSa6AHH/7l4sRQsFzOPv7IMT//xyYApCMbdR20neGFkz/Lxl VtRA== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@linaro.org header.s=google header.b=Azf4KbP2; 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 h3-v6si1404387plh.694.2018.02.07.10.01.15; Wed, 07 Feb 2018 10:01:18 -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=Azf4KbP2; 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 S1754295AbeBGSBN (ORCPT + 18 others); Wed, 7 Feb 2018 13:01:13 -0500 Received: from mail-wr0-f196.google.com ([209.85.128.196]:41653 "EHLO mail-wr0-f196.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754152AbeBGSBJ (ORCPT ); Wed, 7 Feb 2018 13:01:09 -0500 Received: by mail-wr0-f196.google.com with SMTP id v15so2016665wrb.8 for ; Wed, 07 Feb 2018 10:01:08 -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:in-reply-to:references; bh=e/w5xop08AXK/QFABXyb0/dkitF+a5sbLjf1oR1j35I=; b=Azf4KbP2guZEfWWnvf9P4FvYUpStLH2h0cnVW7Q+uLCfEJ5GmIyuUPT6h9axIOhRTZ F2a/KvALNRHECTlqIqyjFgDyd02X34B71HgkyFIwsl6WQyKbOKS/LPAQT9O2Em4OyKGa LugbIfS52DV8JpVck0ELv9FZ0kgqhIVOFXosc= 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:in-reply-to :references; bh=e/w5xop08AXK/QFABXyb0/dkitF+a5sbLjf1oR1j35I=; b=MKty5BfhyQZoi4a2o5DenFIFmruH3f2oRJx71kog9XE23tW6fCDBz39ZQCs1i7ekjZ Es69VCpJX+lXrpHRhCYDYxx3CO0p8FJhKwPw8zIN6ahMu9mgn2jRm4WdS/IUQ2qgzGAz fH4eaSgEtYLJsW9TxHPMzH5Yb/CjxlvJm2Ibq+SlDQ4qV6jUum/bbNHoWW1EddwvkkcU qZh7gao5EYEtRH4ZZslT7u/ehbut7Xtinvi/Bn0sPfnVlgRIdna2rBmCDauiJIDXHzCN 3tFIHjCzdg80v2j2EZdb84Yz2auYhYWz7aEqdY4CB1nsMQVYzDvGdUxgOcSdohnmfUDX zCFA== X-Gm-Message-State: APf1xPCxBVpeOukbJ8GjmKLkNn2+YfMrDwaxOOthdgwt7/bdWT7RK7AU 3WLj9IQhtB3wrDyG8TOlD8yEqA== X-Received: by 10.223.136.206 with SMTP id g14mr5987002wrg.201.1518026466707; Wed, 07 Feb 2018 10:01:06 -0800 (PST) Received: from localhost.localdomain ([5.170.76.88]) by smtp.gmail.com with ESMTPSA id 6sm3301791wrd.57.2018.02.07.10.01.04 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Wed, 07 Feb 2018 10:01:05 -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 V2 1/1] block, bfq: add requeue-request hook Date: Wed, 7 Feb 2018 19:00:50 +0100 Message-Id: <20180207180050.5639-2-paolo.valente@linaro.org> X-Mailer: git-send-email 2.15.1 In-Reply-To: <20180207180050.5639-1-paolo.valente@linaro.org> References: <20180207180050.5639-1-paolo.valente@linaro.org> 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 --- block/bfq-iosched.c | 109 ++++++++++++++++++++++++++++++++++++++++------------ 1 file changed, 84 insertions(+), 25 deletions(-) -- 2.15.1 diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c index 47e6ec7427c4..21e6b9e45638 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,20 @@ static void bfq_insert_request(struct blk_mq_hw_ctx *hctx, struct request *rq, else list_add_tail(&rq->queuelist, &bfqd->dispatch); } else { + if (!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. + */ + pr_warn("Regular request associated with no queue"); + WARN_ON(1); + 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 +4715,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 +4767,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 +4789,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 +5484,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,