From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-pl1-f174.google.com (mail-pl1-f174.google.com [209.85.214.174]) (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 D5075BE5E for ; Wed, 31 Dec 2025 23:43:51 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.214.174 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1767224633; cv=none; b=POrbRLy7axy/JTXMO791UCKIRq0qs/JcS8dOWTcOBjh9H3CC5UoNB73cOQYOJZ+2bK2B0BlSG/XfdDkctSkLGCrZMuOHIbBqYF0c5ZhO+wKJ8yWPFhJGM0+Op2ipN8RlpzWWZhz5wtnJX4w1y0HUPvOoqgIjU34waW4BkYHMPPQ= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1767224633; c=relaxed/simple; bh=PQ4x58UMyK7IF/E6PXh/NlTG9oWRHOPp43EMRO8gXZw=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=UOZhgCjQbaSCAjFvCHN/MMg9Cw6hs3bQk45t9Wnqxv1yrEIgIgGQSj4aCPi/GIXuir1MVCeSaE+IqOyongl98sU2n4rDyjRmX6fnp2USS0y+sBCF+RugieOe5UNlJBZApnOqNHVfMX56+3o/Tlhba2a5vR89RnV3UAq7n06AT58= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=reject dis=none) header.from=purestorage.com; spf=fail smtp.mailfrom=purestorage.com; dkim=pass (2048-bit key) header.d=purestorage.com header.i=@purestorage.com header.b=IAiIe4Q9; arc=none smtp.client-ip=209.85.214.174 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=reject dis=none) header.from=purestorage.com Authentication-Results: smtp.subspace.kernel.org; spf=fail smtp.mailfrom=purestorage.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=purestorage.com header.i=@purestorage.com header.b="IAiIe4Q9" Received: by mail-pl1-f174.google.com with SMTP id d9443c01a7336-2a12ebe4b74so198447995ad.0 for ; Wed, 31 Dec 2025 15:43:51 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=purestorage.com; s=google2022; t=1767224631; x=1767829431; darn=vger.kernel.org; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:from:to:cc:subject:date:message-id:reply-to; bh=iWzNss0H+aMgX+DgryBnOVYj1lJbOiTm3n0MKqsby9c=; b=IAiIe4Q940lQezKPQk3MeUXeaOXx5SmvId+DUE6JdP0XjvsaYx4UWBA/9QrOoutQps 23s+Dl4BbBFmrmFZgnTd2dQu2kEezpk9txUTUhAv03iSnwWETBXZd9U5rGHIVEfYSE4r 873NWR3K0zUHlOV6LansGgI2RMUtVTHYhC6aXAIUkzjx99C3wzXQWSo95gzXLFbEiB7p V+f1eHR8I0wVpAMkdb3JNA2TAbLsRyDArGhMnas1rpzE2HQDT3Z0TBQCKMgUHdEXzs7t t5i7mwywaFKXBQv3YrNF+814wmHQXqjHtl73C/l8t6EEz1ZruTuGkFD5JTnr7wyECcX8 mpTA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1767224631; x=1767829431; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:x-gm-gg:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=iWzNss0H+aMgX+DgryBnOVYj1lJbOiTm3n0MKqsby9c=; b=viUyTGOjfFMxzEvLXYDWQKY7WbbCDB9k7MXLZIUJP6Vq3Wy8EwxoIbUisyK7yho9do G1HEPTb1HgixUnw+rALxcTc7miav4xdQ68TbSnfYGhkJ+2AxnR629A71vsMDzGAYdhTZ H02rf9HospAl4tm1oDQUcomLNt75GT8VlDRXzBd0YDh65PfV2EL2SVXy9VRk4uqWEVzf H9GuXdej8F00BYXgxowTnVyOLpnzrv3+blDhUfAsKDwY12BPf+FlbQNvUVlAknGExkFG PtPPslOuEpGQuhwDffEja1mqHQnQmFksImuhmWP0wjyUovdJax6wKE9p73bg/IeECgSt F/1A== X-Forwarded-Encrypted: i=1; AJvYcCWKY83EWDKqvySRk9djoRKefeTSPLqfwUCDDhfyOZoNRTg5zEKSkbBubDBbTdp+vOKPRoUFDovgPkKfSkE=@vger.kernel.org X-Gm-Message-State: AOJu0YzYl3nh+WAvSpREsDRC0H9Iz3Lbol+umMcXzGhAF8zbQEv7kfhL aqlPT1gfADBcwk3/6lvn95pDSQAHny0S6p03NBqUbJHp1aQ9xfWDaZt21Eo/LlgPMqM= X-Gm-Gg: AY/fxX4KFah5m1p82oxU+TJVVGOKcswuyPBlE4gGFkfvKayMBi25FwhK/Lhwu83sUUq cuj/rAXkGaumuX+LR121Cj+fON6OM0AWLLO6/BIbbTkqXFBHzyiqRq966TMZ1Q9c0SS4Ob9knH9 Uue84FWIRpOl4dShf4YVV8G0yrFtfacQx3Rr2UPV5CudqE1R56SgjWvP+S4ijU+cOaRy7BG0Mm6 gncMMsbea+jiBMd8jmz/Ul+5moaj21DUaPiMqKSPKpno8l6qKgKAmNwzpvEwgARBRplWk7+SL1t OBXzvwAESWJnKsnWVefPxh0dGIkVH+gmeN1UDwJyBMEyFbWeY3xQ3qdamxXV4XausVvp0E25XJT 7pC2sUVWx7aEseLIzTgy782wdZMcPjyquAF4qa30ZyOsfEUtY9CzATs2zC7CwwxYfKsfp+Bvc/L aHKnPiF2pt7EWb8/RhAAOflZCw4HCEAqSAYgvyThskkQ== X-Google-Smtp-Source: AGHT+IEhDAb/Ue0RyM7UYkZ3r6QYW7/vqd5OJlQV+JoxkoBTN3YmJ4WA+8naLYzyFUyH/yBS1aFXlg== X-Received: by 2002:a05:7022:68a3:b0:119:e56b:c75f with SMTP id a92af1059eb24-121722f7fd4mr41595425c88.36.1767224630797; Wed, 31 Dec 2025 15:43:50 -0800 (PST) Received: from medusa.lab.kspace.sh ([208.88.152.253]) by smtp.googlemail.com with UTF8SMTPSA id a92af1059eb24-121724cfdd0sm141683911c88.4.2025.12.31.15.43.50 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 31 Dec 2025 15:43:50 -0800 (PST) Date: Wed, 31 Dec 2025 15:43:49 -0800 From: Mohamed Khalfella To: Sagi Grimberg 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 Subject: Re: [RFC PATCH 08/14] nvme: Implement cross-controller reset recovery Message-ID: <20251231234349.GP3864520-mkhalfella@purestorage.com> References: <20251126021250.2583630-1-mkhalfella@purestorage.com> <20251126021250.2583630-9-mkhalfella@purestorage.com> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: 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? > > > + 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. 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. > > > + } > > + > > + 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. > > > + > > + 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? > > > + 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?