From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-11.5 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH, MAILING_LIST_MULTI,NICE_REPLY_A,SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS, URIBL_BLOCKED,USER_AGENT_SANE_1 autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 86438C433DF for ; Tue, 11 Aug 2020 06:17:56 +0000 (UTC) Received: from merlin.infradead.org (merlin.infradead.org [205.233.59.134]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 545C4206B5 for ; Tue, 11 Aug 2020 06:17:56 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="MPrWpLy0" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 545C4206B5 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=huawei.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-nvme-bounces+linux-nvme=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=merlin.20170209; h=Sender:Content-Type: Content-Transfer-Encoding:Cc:List-Subscribe:List-Help:List-Post:List-Archive: List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:Date:Message-ID:From: References:To:Subject:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=UjNT5xNGNlvdmIUVAa4BKlS5IP6o3oA8KU4j1C4Sdpw=; b=MPrWpLy01u+iiuJtIn6UdirWY kBVZBXj1QIHbd6rdDod1DMu5kkaahXUA5YCyJKAhC0s7hgAdp7P1DTptjHbwofngL1USxinHivo5Y Ru3u9PzdHXYMfNDxa70AXwFQX4KIfcFOUXwRuTve8/I1ZRHFT7loOIrZZOH2TFawHfGW4L7c+kFRo QMhUfbAipzqbZqzJvnfl3jNqerWJt9TfmmARTChDpvzKfep8+LV4WWTKGHDd2kyOd1KzrJyC/puDn uteZXvjCnQa5UIGVvktR78d64O6pVd+r2cMT3E6iBga4VdC8U44VztI1mtqEHeqMFTBZat9bb8iQj Oc/G8pRUg==; Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1k5NbY-0003NF-LR; Tue, 11 Aug 2020 06:17:48 +0000 Received: from szxga05-in.huawei.com ([45.249.212.191] helo=huawei.com) by merlin.infradead.org with esmtps (Exim 4.92.3 #3 (Red Hat Linux)) id 1k5NbW-0003MW-EZ for linux-nvme@lists.infradead.org; Tue, 11 Aug 2020 06:17:48 +0000 Received: from DGGEMS411-HUB.china.huawei.com (unknown [172.30.72.59]) by Forcepoint Email with ESMTP id 3F9A39EAE82CDD8AF55E; Tue, 11 Aug 2020 14:17:40 +0800 (CST) Received: from [10.169.42.93] (10.169.42.93) by DGGEMS411-HUB.china.huawei.com (10.3.19.211) with Microsoft SMTP Server id 14.3.487.0; Tue, 11 Aug 2020 14:17:35 +0800 Subject: Re: nvme: explicitly use normal NVMe error handling when appropriate To: Mike Snitzer References: <20200806184057.GA27858@redhat.com> <20200806191943.GA27868@redhat.com> <6B826235-C504-4621-B8F7-34475B200979@netapp.com> <20200807000755.GA28957@redhat.com> <510f5aff-0437-b1ce-f7ab-c812edbea880@grimberg.me> <20200807045015.GA29737@redhat.com> <20200810143620.GA19127@redhat.com> <20200810172209.GA19535@redhat.com> <7f99724a-a1eb-6bec-f8ae-f9a4601b0487@huawei.com> <20200811042000.GA22692@redhat.com> From: Chao Leng Message-ID: <07bd0219-b39b-dcec-35f0-edc22df0d2ae@huawei.com> Date: Tue, 11 Aug 2020 14:17:34 +0800 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:68.0) Gecko/20100101 Thunderbird/68.9.0 MIME-Version: 1.0 In-Reply-To: <20200811042000.GA22692@redhat.com> Content-Language: en-US X-Originating-IP: [10.169.42.93] X-CFilter-Loop: Reflected X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20200811_021746_776738_F0964B08 X-CRM114-Status: GOOD ( 21.61 ) X-BeenThere: linux-nvme@lists.infradead.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Sagi Grimberg , Hannes Reinecke , "linux-nvme@lists.infradead.org" , Christoph Hellwig , dm-devel@redhat.com, Ewan Milne , Keith Busch , "Meneghini, John" Content-Transfer-Encoding: 7bit Content-Type: text/plain; charset="us-ascii"; Format="flowed" Sender: "Linux-nvme" Errors-To: linux-nvme-bounces+linux-nvme=archiver.kernel.org@lists.infradead.org On 2020/8/11 12:20, Mike Snitzer wrote: > On Mon, Aug 10 2020 at 11:32pm -0400, > Chao Leng wrote: > >> >> >> On 2020/8/11 1:22, Mike Snitzer wrote: >>> On Mon, Aug 10 2020 at 10:36am -0400, >>> Mike Snitzer wrote: >>> >>>> On Fri, Aug 07 2020 at 7:35pm -0400, >>>> Sagi Grimberg wrote: >>>> >>>>> >>>>>>> Hey Mike, >>> ... >>>>>> I think NVMe can easily fix this by having an earlier stage of checking, >>>>>> e.g. nvme_local_retry_req(), that shortcircuits ever getting to >>>>>> higher-level multipathing consideration (be it native NVMe or DM >>>>>> multipathing) for cases like NVME_SC_CMD_INTERRUPTED. >>>>>> To be clear: the "default" case of nvme_failover_req() that returns >>>>>> false to fallback to NVMe's "local" normal NVMe error handling -- that >>>>>> can stay.. but a more explicit handling of cases like >>>>>> NVME_SC_CMD_INTERRUPTED should be added to a nvme_local_retry_req() >>>>>> check that happens before nvme_failover_req() in nvme_complete_rq(). >>>>> >>>>> I don't necessarily agree with having a dedicated nvme_local_retry_req(). >>>>> a request that isn't failed over, goes to local error handling (retry or >>>>> not). I actually think that just adding the condition to >>>>> nvme_complete_req and having nvme_failover_req reject it would work. >>>>> >>>>> Keith? >>>> >>>> I think that is basically what I'm thinking too. >>> >>> From: Mike Snitzer >>> Subject: nvme: explicitly use normal NVMe error handling when appropriate >>> >>> Commit 764e9332098c0 ("nvme-multipath: do not reset on unknown >>> status"), among other things, fixed NVME_SC_CMD_INTERRUPTED error >>> handling by changing multipathing's nvme_failover_req() to short-circuit >>> path failover and then fallback to NVMe's normal error handling (which >>> takes care of NVME_SC_CMD_INTERRUPTED). >>> >>> This detour through native NVMe multipathing code is unwelcome because >>> it prevents NVMe core from handling NVME_SC_CMD_INTERRUPTED independent >>> of any multipathing concerns. >>> >>> Introduce nvme_status_needs_local_error_handling() to prioritize >>> non-failover retry, when appropriate, in terms of normal NVMe error >>> handling. nvme_status_needs_local_error_handling() will naturely evolve >>> to include handling of any other errors that normal error handling must >>> be used for. >>> >>> nvme_failover_req()'s ability to fallback to normal NVMe error handling >>> has been preserved because it may be useful for future NVME_SC that >>> nvme_status_needs_local_error_handling() hasn't yet been trained for. >>> >>> Signed-off-by: Mike Snitzer >>> --- >>> drivers/nvme/host/core.c | 16 ++++++++++++++-- >>> 1 file changed, 14 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c >>> index 88cff309d8e4..be749b690af7 100644 >>> --- a/drivers/nvme/host/core.c >>> +++ b/drivers/nvme/host/core.c >>> @@ -252,6 +252,16 @@ static inline bool nvme_req_needs_retry(struct request *req) >>> return true; >>> } >>> +static inline bool nvme_status_needs_local_error_handling(u16 status) >>> +{ >>> + switch (status & 0x7ff) { >>> + case NVME_SC_CMD_INTERRUPTED: >>> + return true; >>> + default: >>> + return false; >>> + } >>> +} >>> + >>> static void nvme_retry_req(struct request *req) >>> { >>> struct nvme_ns *ns = req->q->queuedata; >>> @@ -270,7 +280,8 @@ static void nvme_retry_req(struct request *req) >>> void nvme_complete_rq(struct request *req) >>> { >>> - blk_status_t status = nvme_error_status(nvme_req(req)->status); >>> + u16 nvme_status = nvme_req(req)->status; >>> + blk_status_t status = nvme_error_status(nvme_status); >>> trace_nvme_complete_rq(req); >>> @@ -280,7 +291,8 @@ void nvme_complete_rq(struct request *req) >>> nvme_req(req)->ctrl->comp_seen = true; >>> if (unlikely(status != BLK_STS_OK && nvme_req_needs_retry(req))) { >>> - if ((req->cmd_flags & REQ_NVME_MPATH) && nvme_failover_req(req)) >>> + if (!nvme_status_needs_local_error_handling(nvme_status) && >>> + (req->cmd_flags & REQ_NVME_MPATH) && nvme_failover_req(req)) >> >> This looks no affect. if work with nvme multipath, now is already retry local. > > Not if NVMe is built without multipathing configured. If without nvme multipathing configured, now is also retry local, do not need !nvme_status_needs_local_error_handling(nvme_status). > >> If work with dm-multipath, still return error. > > Yes, I'm aware. Use of REQ_FAILFAST_TRANSPORT isn't something that is > needed for NVMe, so why are you proposing hacks in NVMe to deal with it? I just describe the possible scenarios:1.nvme multipathing configured. 2.without any multipath.3. with dm-multipath. > >>> return; >>> if (!blk_queue_dying(req->q)) { >>> >> >> Suggest: >> REQ_FAILFAST_TRANSPORT may be designed for scsi, because scsi protocol >> do not difine the local retry mechanism. SCSI implements a fuzzy local >> retry mechanism, so need the REQ_FAILFAST_TRANSPORT for multipath >> software, multipath software retry according error code is expected. >> nvme is different with scsi about this. It define local retry mechanism >> and path error code, so nvme should not care REQ_FAILFAST_TRANSPORT. > > Exactly. Except by "nvme should not care REQ_FAILFAST_TRANSPORT." your > patch says you mean "nvme shouldn't disallow retry if > REQ_FAILFAST_TRANSPORT is it". I'm saying: don't try to get such > changes into NVMe. no. the patch just mean: if path error, fail over to retry by multipath (nvme multipath or dm-multipath). Other need local retry local, retry after a defined time according to status(CRD) and CRDT. Now nvme multipath is already do like this, the patch make dm-multipath work like nvme multipath. > > In general, aspects of your patch may have merit but overall it is doing > too much. > > Mike > > . > _______________________________________________ Linux-nvme mailing list Linux-nvme@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-nvme