From patchwork Thu Feb 1 17:58:23 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jens Axboe X-Patchwork-Id: 10195771 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 30411601A0 for ; Thu, 1 Feb 2018 17:58:30 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 0D8E428C80 for ; Thu, 1 Feb 2018 17:58:30 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 021B428C84; Thu, 1 Feb 2018 17:58:29 +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.9 required=2.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,RCVD_IN_DNSWL_HI 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 C032B28C80 for ; Thu, 1 Feb 2018 17:58:28 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752611AbeBAR62 (ORCPT ); Thu, 1 Feb 2018 12:58:28 -0500 Received: from mail-it0-f43.google.com ([209.85.214.43]:39924 "EHLO mail-it0-f43.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752433AbeBAR61 (ORCPT ); Thu, 1 Feb 2018 12:58:27 -0500 Received: by mail-it0-f43.google.com with SMTP id 68so4807962ite.4 for ; Thu, 01 Feb 2018 09:58:27 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=kernel-dk.20150623.gappssmtp.com; s=20150623; h=subject:from:to:cc:references:message-id:date:user-agent :mime-version:in-reply-to:content-language:content-transfer-encoding; bh=/A0E4GBGA/LnRJgjlrjOCah3PC/oA2vkor5tPTxAJO0=; b=xGHG92NTdWZ5YfXNfdAb7gigJXja7VPZWbCTfh0bJAwCQoDmkbH5I3Pw4AhllKc7bL xWw1LN0BSh/ftqYTjTaMZVDLgWCtfkRFF4jfdQk1vGjT3x8Q/4hjCedga00exIi36TGF k7BDDYIh2BPcOIYmK7Rz6Xy4rhJVsdGBlnw/xIGl8vQ2nEDPnyxoJ5BUNnvNuiS/xYO5 rKxNoSWMMuL0JIw6SmLIwj6qfzShepkAhTBVnVWfwakWZ58uLmtuHnDXlIC7/QsixyGr vpPwF6s0UXAa1Alou7ceamsxVwlWMLo4GHWjjQbbVhNwEpMLIV4O+s0wXFRU9TqcRjPw IVYQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:from:to:cc:references:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=/A0E4GBGA/LnRJgjlrjOCah3PC/oA2vkor5tPTxAJO0=; b=qrP0dj8ya/MKj663fg6c+jtR6Jv3wz4wYlfkThlWhks9rUIhAporZTWnUhk9/hFi9H LKJCfdpGdkH5DG1h9o2XDMYssFOi8/+JSfkhdhkKDujUGy4dHeguGGW3dBzP8oiJGEQi EW6KmAoSUwWQewZUU56nSuKidP9lvCrKOqX/Z4Miwo/03CH9oZvmxysJfqkGbXKFbGlH wSRgNW/ZnqkStIOhCwwv3HzRZGRBMS+Wm3PTj4YWWVDjLN8VOAu/bM8VLByLQMItHhjD uiIBGlo2qQBRDNXskbAdzKoRbLqc+j0vvnz5Z8z/ER3FrdRv5fj+hcmBV/2F11JDTjHp EkjA== X-Gm-Message-State: AKwxyteZ66KfoOKoilAtcrewQk+RhmHxMf0YvicBtNQggpCcT7Yh07tB aDEx8yVxDcY3G3FF4hAN0y3Beg== X-Google-Smtp-Source: AH8x2243QvuJJj72mGcR0i495MGxv8V5O6MjaiOrKFATYUpN5T6fk3Mtg2uCj7zYSThOfwGUxOxd/w== X-Received: by 10.36.9.144 with SMTP id 138mr14316844itm.125.1517507906897; Thu, 01 Feb 2018 09:58:26 -0800 (PST) Received: from [192.168.1.160] ([216.160.245.98]) by smtp.gmail.com with ESMTPSA id z1sm234127itg.37.2018.02.01.09.58.24 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Thu, 01 Feb 2018 09:58:25 -0800 (PST) Subject: Re: WARNING: CPU: 2 PID: 207 at drivers/nvme/host/core.c:527 nvme_setup_cmd+0x3d3 From: Jens Axboe To: Keith Busch Cc: "jianchao.wang" , "linux-block@vger.kernel.org" , Christoph Hellwig , "linux-nvme@lists.infradead.org" , Ming Lei References: <45f93661-da0d-94c5-1740-85242df8776e@kernel.dk> <0872b361-157b-a876-20af-3d7a4ee7ff31@kernel.dk> <8fd916ab-42d7-c654-5a01-8f1eb4be730e@oracle.com> <0b7686b3-f716-49ba-c7c4-929d84905569@kernel.dk> <7459ffed-c63c-38a9-84f5-456c2a5c4fe0@oracle.com> <0f6c248b-eb67-9b02-a4b9-0366d476e70d@kernel.dk> <20180201045645.GF27735@localhost.localdomain> <576ba7dc-78cf-b5d2-d1de-446d1f4b2732@kernel.dk> Message-ID: <7ec361a3-8900-d259-6842-e0b0b14a253a@kernel.dk> Date: Thu, 1 Feb 2018 10:58:23 -0700 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:58.0) Gecko/20100101 Thunderbird/58.0 MIME-Version: 1.0 In-Reply-To: <576ba7dc-78cf-b5d2-d1de-446d1f4b2732@kernel.dk> Content-Language: en-US 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 2/1/18 8:26 AM, Jens Axboe wrote: > On 1/31/18 9:56 PM, Keith Busch wrote: >> On Wed, Jan 31, 2018 at 08:07:41PM -0700, Jens Axboe wrote: >>> if (total_phys_segments > queue_max_segments(q)) >>> - return 0; >>> + return 0; >> >> This perhaps unintended change happens to point out another problem: >> queue_max_segments is the wrong limit for discards, which require >> queue_max_discard_segments. >> >> It might be easier to merge discard requests special, like how merging >> a discard bio is handled (untested). > > Yeah agreed, we should just split it up completely instead instead of > special casing it in the read/write path. > > >> diff --git a/block/blk-merge.c b/block/blk-merge.c >> index 8452fc7164cc..01671e1373ff 100644 >> --- a/block/blk-merge.c >> +++ b/block/blk-merge.c >> @@ -550,6 +550,28 @@ static bool req_no_special_merge(struct request *req) >> return !q->mq_ops && req->special; >> } >> >> +static bool req_attempt_discard_merge(struct request_queue *q, struct request *req, >> + struct request *next) >> +{ >> + unsigned short segments = blk_rq_nr_discard_segments(req); >> + >> + if (segments >= queue_max_discard_segments(q)) >> + goto no_merge; >> + if (blk_rq_sectors(req) + bio_sectors(next->bio) > >> + blk_rq_get_max_sectors(req, blk_rq_pos(req))) >> + goto no_merge; >> + >> + req->biotail->bi_next = next->bio; >> + req->biotail = next->biotail; >> + req->nr_phys_segments = segments + blk_rq_nr_discard_segments(next); >> + next->bio = NULL; >> + return true; >> + >> +no_merge: >> + req_set_nomerge(q, req); >> + return false; >> +} >> + >> static int ll_merge_requests_fn(struct request_queue *q, struct request *req, >> struct request *next) >> { >> @@ -679,6 +701,15 @@ static struct request *attempt_merge(struct request_queue *q, >> if (req->write_hint != next->write_hint) >> return NULL; >> >> + /* >> + * Discards are ... special. >> + */ >> + if (req_op(req) == REQ_OP_DISCARD) { >> + if (req_attempt_discard_merge(q, req, next)) >> + return next; >> + return NULL; >> + } >> + >> /* >> * If we are allowed to merge, then append bio list >> * from next to rq and release next. merge_requests_fn > > This looks fine to me, the bio-to-request merge path already looks correct. > Care to send a properly formatted patch? I was able to reproduce on a test box, pretty trivially in fact: # echo mq-deadline > /sys/block/nvme2n1/queue/scheduler # mkfs.ext4 /dev/nvme2n1 # mount /dev/nvme2n1 /data -o discard # dd if=/dev/zero of=/data/10g bs=1M count=10k # sync # rm /data/10g # sync <- triggered Your patch still doesn't work, but mainly because we init the segments to 0 when setting up a discard. The below works for me, and cleans up the merge path a bit, since your patch was missing various adjustments on both the merged and freed request. diff --git a/block/blk-core.c b/block/blk-core.c index a2005a485335..e4561c95fc23 100644 --- a/block/blk-core.c +++ b/block/blk-core.c @@ -3282,6 +3282,8 @@ void blk_rq_bio_prep(struct request_queue *q, struct request *rq, { if (bio_has_data(bio)) rq->nr_phys_segments = bio_phys_segments(q, bio); + else if (bio_op(bio) == REQ_OP_DISCARD) + rq->nr_phys_segments = 1; rq->__data_len = bio->bi_iter.bi_size; rq->bio = rq->biotail = bio; diff --git a/block/blk-merge.c b/block/blk-merge.c index 8452fc7164cc..782940c65d8a 100644 --- a/block/blk-merge.c +++ b/block/blk-merge.c @@ -550,6 +550,24 @@ static bool req_no_special_merge(struct request *req) return !q->mq_ops && req->special; } +static bool req_attempt_discard_merge(struct request_queue *q, struct request *req, + struct request *next) +{ + unsigned short segments = blk_rq_nr_discard_segments(req); + + if (segments >= queue_max_discard_segments(q)) + goto no_merge; + if (blk_rq_sectors(req) + bio_sectors(next->bio) > + blk_rq_get_max_sectors(req, blk_rq_pos(req))) + goto no_merge; + + req->nr_phys_segments = segments + blk_rq_nr_discard_segments(next); + return true; +no_merge: + req_set_nomerge(q, req); + return false; +} + static int ll_merge_requests_fn(struct request_queue *q, struct request *req, struct request *next) { @@ -683,9 +701,13 @@ static struct request *attempt_merge(struct request_queue *q, * If we are allowed to merge, then append bio list * from next to rq and release next. merge_requests_fn * will have updated segment counts, update sector - * counts here. + * counts here. Handle DISCARDs separately, as they + * have separate settings. */ - if (!ll_merge_requests_fn(q, req, next)) + if (req_op(req) == REQ_OP_DISCARD) { + if (!req_attempt_discard_merge(q, req, next)) + return NULL; + } else if (!ll_merge_requests_fn(q, req, next)) return NULL; /* @@ -715,7 +737,8 @@ static struct request *attempt_merge(struct request_queue *q, req->__data_len += blk_rq_bytes(next); - elv_merge_requests(q, req, next); + if (req_op(req) != REQ_OP_DISCARD) + elv_merge_requests(q, req, next); /* * 'next' is going away, so update stats accordingly