From patchwork Mon Feb 13 23:10:30 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jens Axboe X-Patchwork-Id: 9570861 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id 6FD3C6045D for ; Mon, 13 Feb 2017 23:10:36 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 5F5F12833B for ; Mon, 13 Feb 2017 23:10:36 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 52FE828355; Mon, 13 Feb 2017 23:10:36 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.4 required=2.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID, RCVD_IN_DNSWL_HI, RCVD_IN_SORBS_SPAM autolearn=ham version=3.3.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id C843F2833B for ; Mon, 13 Feb 2017 23:10:35 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751352AbdBMXKe (ORCPT ); Mon, 13 Feb 2017 18:10:34 -0500 Received: from mail-it0-f52.google.com ([209.85.214.52]:38628 "EHLO mail-it0-f52.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751288AbdBMXKe (ORCPT ); Mon, 13 Feb 2017 18:10:34 -0500 Received: by mail-it0-f52.google.com with SMTP id c7so7948666itd.1 for ; Mon, 13 Feb 2017 15:10:33 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=kernel-dk.20150623.gappssmtp.com; s=20150623; h=subject:to:references:cc:from:message-id:date:user-agent :mime-version:in-reply-to:content-transfer-encoding; bh=9LV++jNYdYUC0JkYt8eZxu/vqcYunzZtD3P1gFEUTFQ=; b=k2sPGVId9pP5FsJ3g4cN1bYw1THfxpOo3eWc6iGO/3/T8yLy8lpwC2XdckRqbzJRIN Sdeem/WMmc6dRTDgATBeB7360mJFk9T8WVoRVJwUVwYrV2H2tET3ZwutDrBlZhunf7fJ 1pZeFYd1AySXyufH2iPcxNgeDhNXciufneCVA6y8IRhmzVNBBrFrOMeh+QP/XGbkR8sm C3lMGb4sa95oiyIu72gnXE6phLnaTMsfTGewdd7lax9Ze2jSqP6nlb4jxeMuo4uUwZUv NIaAN6cKaGGQLn57uuKTNf3km/0zAuY/oIRrOZu7jYK5TfvERuladaC67qgU40XhUQ1B oM3w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:references:cc:from:message-id:date :user-agent:mime-version:in-reply-to:content-transfer-encoding; bh=9LV++jNYdYUC0JkYt8eZxu/vqcYunzZtD3P1gFEUTFQ=; b=FekzGETsfjo1b9BRluHZeHLjfVpqRwftt1miHeq0qljnP0tulkvX6eKHVxNI9J/Veu fzTg3dQ1Ty/CYCS6Wbb5bIvH5x1lwDY9ziU2H8EhxZTMvzmB1w4hym7kgfk4sKGfAYJv 1X8JU9eXDJeis1q0oqZfKvcqx+Tk9JAJjYQQ2+zEEc+R2A8k8oBrgbzAzGfSsRV7qFfW +ltRbbg/nz8LonOTsApk0VTqMDbcQxgMEa5TH2xQyHdYxAOe3p5OKImURNwNxtkyO15I zpPgXevu/W5XMpGA+iUaHgwoZVAPDr7JZecB9C9UuxW19+rUbyZohgiNASXQCTjuDo8T ww9A== X-Gm-Message-State: AMke39kYIsbpStjoFBc8vO0Rd4KhAiQwltPkoodMO2gPM/qrPtxEK4Znrl9XyA+gAes8Gg== X-Received: by 10.36.48.208 with SMTP id q199mr911910itq.28.1487027433149; Mon, 13 Feb 2017 15:10:33 -0800 (PST) Received: from [192.168.1.155] ([216.160.245.98]) by smtp.gmail.com with ESMTPSA id 202sm450419ity.8.2017.02.13.15.10.31 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Mon, 13 Feb 2017 15:10:31 -0800 (PST) Subject: Re: [PATCH BUGFIX] block: make elevator_get robust against cross blk/blk-mq choice To: Omar Sandoval , Paolo Valente References: <20170213210107.4848-1-paolo.valente@linaro.org> <20170213210107.4848-2-paolo.valente@linaro.org> <20170213220900.GA11052@vader.DHCP.thefacebook.com> <89b98d59-fcae-6b13-a6a1-6fe62967929d@kernel.dk> Cc: Tejun Heo , linux-block@vger.kernel.org, linux-kernel@vger.kernel.org, ulf.hansson@linaro.org, linus.walleij@linaro.org, broonie@kernel.org From: Jens Axboe Message-ID: Date: Mon, 13 Feb 2017 16:10:30 -0700 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.7.0 MIME-Version: 1.0 In-Reply-To: <89b98d59-fcae-6b13-a6a1-6fe62967929d@kernel.dk> Sender: linux-block-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-block@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP On 02/13/2017 03:28 PM, Jens Axboe wrote: > On 02/13/2017 03:09 PM, Omar Sandoval wrote: >> On Mon, Feb 13, 2017 at 10:01:07PM +0100, Paolo Valente wrote: >>> If, at boot, a legacy I/O scheduler is chosen for a device using blk-mq, >>> or, viceversa, a blk-mq scheduler is chosen for a device using blk, then >>> that scheduler is set and initialized without any check, driving the >>> system into an inconsistent state. This commit addresses this issue by >>> letting elevator_get fail for these wrong cross choices. >>> >>> Signed-off-by: Paolo Valente >>> --- >>> block/elevator.c | 26 ++++++++++++++++++-------- >>> 1 file changed, 18 insertions(+), 8 deletions(-) >> >> Hey, Paolo, >> >> How exactly are you triggering this? In __elevator_change(), we do check >> for mq or not mq: >> >> if (!e->uses_mq && q->mq_ops) { >> elevator_put(e); >> return -EINVAL; >> } >> if (e->uses_mq && !q->mq_ops) { >> elevator_put(e); >> return -EINVAL; >> } >> >> We don't ever appear to call elevator_init() with a specific scheduler >> name, and for the default we switch off of q->mq_ops and use the >> defaults from Kconfig: >> >> if (q->mq_ops && q->nr_hw_queues == 1) >> e = elevator_get(CONFIG_DEFAULT_SQ_IOSCHED, false); >> else if (q->mq_ops) >> e = elevator_get(CONFIG_DEFAULT_MQ_IOSCHED, false); >> else >> e = elevator_get(CONFIG_DEFAULT_IOSCHED, false); >> >> if (!e) { >> printk(KERN_ERR >> "Default I/O scheduler not found. " \ >> "Using noop/none.\n"); >> e = elevator_get("noop", false); >> } >> >> So I guess this could happen if someone manually changed those Kconfig >> options, but I don't see what other case would make this happen, could >> you please explain? > > Was wondering the same - is it using the 'elevator=' boot parameter? > Didn't look at that path just now, but that's the only one I could > think of. If it is, I'd much prefer only using 'chosen_elevator' for > the non-mq stuff, and the fix should be just that instead. > > So instead of: > > if (!e && *chosen_elevator) { > > do > > if (!e && !q->mq_ops && && *chosen_elevator) { Confirmed, that's what it seems to be, and here's a real diff of the above example that works for me: diff --git a/block/elevator.c b/block/elevator.c index 27ff1ed5a6fa..699d10f71a2c 100644 --- a/block/elevator.c +++ b/block/elevator.c @@ -207,11 +207,12 @@ int elevator_init(struct request_queue *q, char *name) } /* - * Use the default elevator specified by config boot param or - * config option. Don't try to load modules as we could be running - * off async and request_module() isn't allowed from async. + * Use the default elevator specified by config boot param for + * non-mq devices, or by config option. Don't try to load modules + * as we could be running off async and request_module() isn't + * allowed from async. */ - if (!e && *chosen_elevator) { + if (!e && !q->mq_ops && *chosen_elevator) { e = elevator_get(chosen_elevator, false); if (!e) printk(KERN_ERR "I/O scheduler %s not found\n",