From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wr1-f43.google.com (mail-wr1-f43.google.com [209.85.221.43]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 0D9DB231A23 for ; Sun, 4 Jan 2026 21:39:38 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.221.43 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1767562780; cv=none; b=Mb3zPgRLsNfBYP3qBoQhaFs0rN7f1CzJ7n2+WrqEgwKFBHeTSeovHufeNPYMKSDGQ3GRshaF0S84a0LlAqaMPbvlgjt6YTaLWYQNILx5GD3EXd3pwcc/74ZXcoq44LGSVPfhSMJTeJCylkUiWY14Dq8Wg/wa7sTzLwKQRuPJp5M= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1767562780; c=relaxed/simple; bh=XDbIwTCyoC2AdpYGxQkH1enzYJdgRNsI9KobTjXjzco=; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From: In-Reply-To:Content-Type; b=n3huEBOFHQbhNipkrI6aAVaq54Ey0YEg/OxmTemekJ+Za+G0aDJPZezMh/yYvHV4gSCGxuiBfhowf5/H0i5wiIG4wW2mgxAD8scUA8ktlwgMdwQaFW9cAtlfs8KTYrX2ZUWFGIGtnlBkN0RiMglrib78rnLpamUDFoaq2tI5MDY= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=grimberg.me; spf=pass smtp.mailfrom=gmail.com; arc=none smtp.client-ip=209.85.221.43 Authentication-Results: smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=grimberg.me Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=gmail.com Received: by mail-wr1-f43.google.com with SMTP id ffacd0b85a97d-42fb5810d39so8862069f8f.2 for ; Sun, 04 Jan 2026 13:39:38 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1767562777; x=1768167577; h=content-transfer-encoding:in-reply-to:from:content-language :references:cc:to:subject:user-agent:mime-version:date:message-id :x-gm-gg:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=EkCeWHeFxyyRVvjAUpgu22NteCuEXNRTDFw74UkD50k=; b=FXeJ+VgyBdHJC7txS4Jtnst+vEo4ZeVSOHPB9ZFokfChoaqiPk8GKCDtjARM2S4QfT DpCOjlalxCvQ0Y+AQewHrJGg4pVCcvQVb/Xeo+RD4nZwU3NdJcbpe7KFpyhLFjo8fAJM 2eTPDZfIvEEHHRWz4Zo21iZYqvIikVm3XBWBaixV9UErTsWdIT0AtEo81y1XoGx9EuRv t/GyxIy9PkT9H0qbifExMJ4VF4tJgiACHzATz+2aWuZQ0yulhGvQfRGu9eMpHqoQQwU0 tRshfzRoFKjWd7+xmRlZr/mhHVigiTVYUU2P4KA1GdWpqdavjWFXc0Qc/+VzsULRq6LZ qrtA== X-Forwarded-Encrypted: i=1; AJvYcCWmH1HX32zMQSjJO6/zWXqkD5sDY9J6ihfJx1IUxCe42QnOxtTe/Tud1Wmzls80fP2bhc3y1bpV4CjjobE=@vger.kernel.org X-Gm-Message-State: AOJu0YzMZ8U/UEpNSUK2kKkNrM9yN9j1jT1n+hjsD2IMxsqVrLJ3ERmW 0nl2YOnaanDxNJDClLtKeSEs/Ga0ahgNix7rUY8jmpEW+newWMaFvTpi X-Gm-Gg: AY/fxX4MLHeUfaLrpHbiepckoW8iuUTruJsc4UYgj5vjMBiAoNr9Ar5mKfagmS1gEbb sfA+L5HvcSRfV0cWJjEo57wVwHp9MgBOp+4cXPioocJJXY+6ACGZscttc9w/2wJn0JFzhveyQS4 O6b8g6kYzIar4LUH9hZydB+13mj1/PZgRfZEqdBTmL6b/7HjDDliaOwIk0M6gR5tyFqviRHcsw2 P6FssWWrhqRBicqq0fv1yeCdE12wjW1kftk1/7pp9YoINCd6ccP3x7mF3DD8r51MdSFhf8sWUzl 80wsHlpBSm2CU1MqjdQ/B+8Uy0fAKNa8KiVDv7mTXmlCkjiEoQ/HdUAkioBcXC+AnSZADQ4KmQZ k4xckI/PcdMzA5CyvcJMuyKuz12GsfLAjGq8qikqB6+QIBkutNnHTo8a3eT0Ys3uiOq1CAROUnP tEu1GTGDvdOWEWRjWp8rvmVHB9CoRHV6mt99IFpGCUOkU= X-Google-Smtp-Source: AGHT+IGlLsPoPsMRjs/QuLqTPJFCcvkyyx6uiSnJKdFVtpuE0vg98NJZDUPUOIz653OT0/nuVB2u2Q== X-Received: by 2002:a05:6000:26c3:b0:430:f40f:61b9 with SMTP id ffacd0b85a97d-4324e4c68e1mr53280816f8f.4.1767562777311; Sun, 04 Jan 2026 13:39:37 -0800 (PST) Received: from [10.100.102.74] (89-138-76-94.bb.netvision.net.il. [89.138.76.94]) by smtp.gmail.com with ESMTPSA id ffacd0b85a97d-4324ea82fa1sm93181072f8f.23.2026.01.04.13.39.35 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Sun, 04 Jan 2026 13:39:36 -0800 (PST) Message-ID: Date: Sun, 4 Jan 2026 23:39:35 +0200 Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [RFC PATCH 08/14] nvme: Implement cross-controller reset recovery To: Mohamed Khalfella Cc: Chaitanya Kulkarni , Christoph Hellwig , Jens Axboe , Keith Busch , Aaron Dailey , Randy Jennings , John Meneghini , Hannes Reinecke , linux-nvme@lists.infradead.org, linux-kernel@vger.kernel.org References: <20251126021250.2583630-1-mkhalfella@purestorage.com> <20251126021250.2583630-9-mkhalfella@purestorage.com> <20251231234349.GP3864520-mkhalfella@purestorage.com> Content-Language: en-US From: Sagi Grimberg In-Reply-To: <20251231234349.GP3864520-mkhalfella@purestorage.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit On 01/01/2026 1:43, Mohamed Khalfella wrote: > On Sat 2025-12-27 12:14:11 +0200, Sagi Grimberg wrote: >> >> On 26/11/2025 4:11, Mohamed Khalfella wrote: >>> A host that has more than one path connecting to an nvme subsystem >>> typically has an nvme controller associated with every path. This is >>> mostly applicable to nvmeof. If one path goes down, inflight IOs on that >>> path should not be retried immediately on another path because this >>> could lead to data corruption as described in TP4129. TP8028 defines >>> cross-controller reset mechanism that can be used by host to terminate >>> IOs on the failed path using one of the remaining healthy paths. Only >>> after IOs are terminated, or long enough time passes as defined by >>> TP4129, inflight IOs should be retried on another path. Implement core >>> cross-controller reset shared logic to be used by the transports. >>> >>> Signed-off-by: Mohamed Khalfella >>> --- >>> drivers/nvme/host/constants.c | 1 + >>> drivers/nvme/host/core.c | 133 ++++++++++++++++++++++++++++++++++ >>> drivers/nvme/host/nvme.h | 10 +++ >>> 3 files changed, 144 insertions(+) >>> >>> diff --git a/drivers/nvme/host/constants.c b/drivers/nvme/host/constants.c >>> index dc90df9e13a2..f679efd5110e 100644 >>> --- a/drivers/nvme/host/constants.c >>> +++ b/drivers/nvme/host/constants.c >>> @@ -46,6 +46,7 @@ static const char * const nvme_admin_ops[] = { >>> [nvme_admin_virtual_mgmt] = "Virtual Management", >>> [nvme_admin_nvme_mi_send] = "NVMe Send MI", >>> [nvme_admin_nvme_mi_recv] = "NVMe Receive MI", >>> + [nvme_admin_cross_ctrl_reset] = "Cross Controller Reset", >>> [nvme_admin_dbbuf] = "Doorbell Buffer Config", >>> [nvme_admin_format_nvm] = "Format NVM", >>> [nvme_admin_security_send] = "Security Send", >>> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c >>> index f5b84bc327d3..f38b70ca9cee 100644 >>> --- a/drivers/nvme/host/core.c >>> +++ b/drivers/nvme/host/core.c >>> @@ -554,6 +554,138 @@ void nvme_cancel_admin_tagset(struct nvme_ctrl *ctrl) >>> } >>> EXPORT_SYMBOL_GPL(nvme_cancel_admin_tagset); >>> >>> +static struct nvme_ctrl *nvme_find_ccr_ctrl(struct nvme_ctrl *ictrl, >>> + u32 min_cntlid) >>> +{ >>> + struct nvme_subsystem *subsys = ictrl->subsys; >>> + struct nvme_ctrl *sctrl; >>> + unsigned long flags; >>> + >>> + mutex_lock(&nvme_subsystems_lock); >> This looks like the wrong lock to take here? > This is similar to nvme_validate_cntlid()? > What is the correct lock to use? Not really, its only because it is called from nvme_init_subsystem which spans subsystems. > >>> + list_for_each_entry(sctrl, &subsys->ctrls, subsys_entry) { >>> + if (sctrl->cntlid < min_cntlid) >>> + continue; >> The use of min_cntlid is not clear to me. >> >>> + >>> + if (atomic_dec_if_positive(&sctrl->ccr_limit) < 0) >>> + continue; >>> + >>> + spin_lock_irqsave(&sctrl->lock, flags); >>> + if (sctrl->state != NVME_CTRL_LIVE) { >>> + spin_unlock_irqrestore(&sctrl->lock, flags); >>> + atomic_inc(&sctrl->ccr_limit); >>> + continue; >>> + } >>> + >>> + /* >>> + * We got a good candidate source controller that is locked and >>> + * LIVE. However, no guarantee sctrl will not be deleted after >>> + * sctrl->lock is released. Get a ref of both sctrl and admin_q >>> + * so they do not disappear until we are done with them. >>> + */ >>> + WARN_ON_ONCE(!blk_get_queue(sctrl->admin_q)); >>> + nvme_get_ctrl(sctrl); >>> + spin_unlock_irqrestore(&sctrl->lock, flags); >>> + goto found; >>> + } >>> + sctrl = NULL; >>> +found: >>> + mutex_unlock(&nvme_subsystems_lock); >>> + return sctrl; >>> +} >>> + >>> +static int nvme_issue_wait_ccr(struct nvme_ctrl *sctrl, struct nvme_ctrl *ictrl) >>> +{ >>> + unsigned long flags, tmo, remain; >>> + struct nvme_ccr_entry ccr = { }; >>> + union nvme_result res = { 0 }; >>> + struct nvme_command c = { }; >>> + u32 result; >>> + int ret = 0; >>> + >>> + init_completion(&ccr.complete); >>> + ccr.ictrl = ictrl; >>> + >>> + spin_lock_irqsave(&sctrl->lock, flags); >>> + list_add_tail(&ccr.list, &sctrl->ccrs); >>> + spin_unlock_irqrestore(&sctrl->lock, flags); >>> + >>> + c.ccr.opcode = nvme_admin_cross_ctrl_reset; >>> + c.ccr.ciu = ictrl->ciu; >>> + c.ccr.icid = cpu_to_le16(ictrl->cntlid); >>> + c.ccr.cirn = cpu_to_le64(ictrl->cirn); >>> + ret = __nvme_submit_sync_cmd(sctrl->admin_q, &c, &res, >>> + NULL, 0, NVME_QID_ANY, 0); >>> + if (ret) >>> + goto out; >>> + >>> + result = le32_to_cpu(res.u32); >>> + if (result & 0x01) /* Immediate Reset */ >>> + goto out; >>> + >>> + tmo = msecs_to_jiffies(max(ictrl->cqt, ictrl->kato * 1000)); >>> + remain = wait_for_completion_timeout(&ccr.complete, tmo); >>> + if (!remain) >> I think remain is redundant here. > Deleted 'remain'. > >>> + ret = -EAGAIN; >>> +out: >>> + spin_lock_irqsave(&sctrl->lock, flags); >>> + list_del(&ccr.list); >>> + spin_unlock_irqrestore(&sctrl->lock, flags); >>> + return ccr.ccrs == 1 ? 0 : ret; >> Why would you still return 0 and not EAGAIN? you expired on timeout but >> still >> return success if you have ccrs=1? btw you have ccrs in the ccr struct >> and in the controller >> as a list. Lets rename to distinguish the two. > True, we did expire timeout here. However, after we removed the ccr > entry we found that it was marked as completed. We return success in > this case even though we hit timeout. When does this happen? Why is it worth having the code non-intuitive for something that effectively never happens (unless I'm missing something?) > > Renamed ctrl->ccrs to ctrl->ccr_list. > >>> +} >>> + >>> +unsigned long nvme_recover_ctrl(struct nvme_ctrl *ictrl) >>> +{ >> I'd call it nvme_fence_controller() > Okay. I will do that. I will also rename the controller state FENCING. > >>> + unsigned long deadline, now, timeout; >>> + struct nvme_ctrl *sctrl; >>> + u32 min_cntlid = 0; >>> + int ret; >>> + >>> + timeout = nvme_recovery_timeout_ms(ictrl); >>> + dev_info(ictrl->device, "attempting CCR, timeout %lums\n", timeout); >>> + >>> + now = jiffies; >>> + deadline = now + msecs_to_jiffies(timeout); >>> + while (time_before(now, deadline)) { >>> + sctrl = nvme_find_ccr_ctrl(ictrl, min_cntlid); >>> + if (!sctrl) { >>> + /* CCR failed, switch to time-based recovery */ >>> + return deadline - now; >> It is not clear what is the return code semantics of this function. >> How about making it success/failure and have the caller choose what to do? > The function returns 0 on success. On failure it returns the time in > jiffies to hold requests for before they are canceled. On failure the > returned time is essentially the hold time defined in TP4129 minus the > time it took to attempt CCR. I think it would be cleaner to simple have this function return status code and have the caller worry about time spent. > >>> + } >>> + >>> + ret = nvme_issue_wait_ccr(sctrl, ictrl); >>> + atomic_inc(&sctrl->ccr_limit); >> inc after you wait for the ccr? shouldn't this be before? > I think it should be after we wait for CCR. sctrl->ccr_limit is the > number of concurrent CCRs the controller supports. Only after we are > done with CCR on this controller we increment it. Maybe it should be folded into nvme_issue_wait_ccr for symmetry? > >>> + >>> + if (!ret) { >>> + dev_info(ictrl->device, "CCR succeeded using %s\n", >>> + dev_name(sctrl->device)); >>> + blk_put_queue(sctrl->admin_q); >>> + nvme_put_ctrl(sctrl); >>> + return 0; >>> + } >>> + >>> + /* Try another controller */ >>> + min_cntlid = sctrl->cntlid + 1; >> OK, I see why min_cntlid is used. That is very non-intuitive. >> >> I'm wandering if it will be simpler to take one-shot at ccr and >> if it fails fallback to crt. I mean, if the sctrl is alive, and it was >> unable >> to reset the ictrl in time, how would another ctrl do a better job here? > We need to attempt CCR from multiple controllers for reason explained in > another response. As you figured out min_cntlid is needed in order to > not loop controller list forever. Do you have a better idea? No, just know that I don't like it very much :) > >>> + blk_put_queue(sctrl->admin_q); >>> + nvme_put_ctrl(sctrl); >>> + now = jiffies; >>> + } >>> + >>> + dev_info(ictrl->device, "CCR reached timeout, call it done\n"); >>> + return 0; >>> +} >>> +EXPORT_SYMBOL_GPL(nvme_recover_ctrl); >>> + >>> +void nvme_end_ctrl_recovery(struct nvme_ctrl *ctrl) >>> +{ >>> + unsigned long flags; >>> + >>> + spin_lock_irqsave(&ctrl->lock, flags); >>> + WRITE_ONCE(ctrl->state, NVME_CTRL_RESETTING); >> This needs to be a proper state transition. > We do not want to have proper transition from RECOVERING to RESETTING. > The reason is that we do not want the controller to be reset while it is > being recovered/fenced because requests should not be canceled. One way > to keep the transitions in nvme_change_ctrl_state() is to use two > states. Say FENCING and FENCED. > > The allowed transitions are > > - LIVE -> FENCING > - FENCING -> FENCED > - FENCED -> (RESETTING, DELETING) > > This will also git rid of NVME_CTRL_RECOVERED > > Does this sound good? We could do what failfast is doing, in case we get transition FENCING -> RESETTING/DELETING we flush the fence_work...