From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-dl1-f51.google.com (mail-dl1-f51.google.com [74.125.82.51]) (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 685C13002BB for ; Tue, 3 Feb 2026 21:24:12 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=74.125.82.51 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1770153854; cv=none; b=EwJiLKdROwUmG0P+VTZUGuKKYP5fJ9S2PPm2aLTHfpf5yGTGqQLwg3p5vUZ/QPKTh6xxN5DikrvxiGB3WSCigAgDanAlgUyXDXdgdm22gnRk8qXcsL83YMv9YlPSJ873XQN/5jCYFaoiQfqvkXH5kvdxyNGi1uLAxhRK6Ex/F0M= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1770153854; c=relaxed/simple; bh=4lD/T8sDwZeM7kC94ScyzkK+iKWZNufuBTPPY2QJsDc=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=qTwe+6OFHhN+CwQvK3tXD7yTMKsbyzeCbmHb/hF2aZ8wGPACrirhT/RjWHmrZG6Hu/p7L1II5yFWrRZl6DCuae6z73POR9wDLFX3L/7ga/gZMxf4T1TRiy5rGTfPV1ouTitKKyDVNp1yDhDY5q8G6Hzp4PkK8t7vYKnZG2zw16E= 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=GqsWdCAV; arc=none smtp.client-ip=74.125.82.51 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="GqsWdCAV" Received: by mail-dl1-f51.google.com with SMTP id a92af1059eb24-11f36012fb2so7165659c88.1 for ; Tue, 03 Feb 2026 13:24:12 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=purestorage.com; s=google2022; t=1770153851; x=1770758651; darn=vger.kernel.org; h=in-reply-to:content-transfer-encoding:content-disposition :mime-version:references:message-id:subject:cc:to:from:date:from:to :cc:subject:date:message-id:reply-to; bh=c11UbJd7HvCGv2K8Ygr2BKg3L56UdPQX0DIKZCeGei0=; b=GqsWdCAVYg4QW9Qs/JhQeMgwb9+2O1Z3bXHsFBVICtc1Mjv+4XVQmV89RHMQWbQyUB vfeRNdZ6G1j1ZNJMTz6Jy3TXYFYFPRFQ6+78CCYLi2vsaxo1OPzgqYlFVpEnOte8VS2A abX8nJEZmsfHfTYe0GYIEtwtAqReLfrTEZ9udid363NRNO6G8QdYixULxDfEVwgCVkY/ SghFMgm17ab/072OO9yE7os/Mp9Z29JnzgSTNpUNm+uYIhM9VulN0rxpOJ92tq4hmGSg 1q1JuzCk8gckV9ndyW2LLyvrpJsv3NQZcXDYHsHCnjqtinG0yk+9pAbDD5MSeoZv/TAV bTmg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1770153851; x=1770758651; h=in-reply-to:content-transfer-encoding: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=c11UbJd7HvCGv2K8Ygr2BKg3L56UdPQX0DIKZCeGei0=; b=JPWXugoXWmzb6pJ31ITkGiOoEO52uyOofh6VuQbo5bM9b8ICXSUPfxo861DO5JzAy0 5EeOMSIMqUocGTdP6ku4XOebKKWTTdHMNbshkM1zm7FRhAQGms6FuxCd2qSzZQ+xe2Rw EUqp/SUKSn9WaG7FAkX1xy5pMqlZvFp8LWc/4f7Ee4cloewD1n0XVQCPwVafaBCIRDoh hAOmwV3ZjJyFAE/lV/IWabZ00jsoeWK+gXv+SHy/HtPMlZ64JlYRjqHtWl4C/ZFh7Zwi MZrAehR7K5LVMmdeYkBAQ5rwgZrqfMntLwvxVVszZQj4/ezDQ1mKM4UFlvKZdPqjGS3A NuMg== X-Forwarded-Encrypted: i=1; AJvYcCWcV8LgvJtJtIoQQ61QxYZAnHP8ZbfyXO1/nEe528+4883JGOmrr15JujYSaE2QlmYgA85JcPO0HhFAD90=@vger.kernel.org X-Gm-Message-State: AOJu0Yx4x+iXHYYqFTmMH8kpBZwiJYgNPKS07CoVhqsIkgUPxe1NKd5i qkcrl4nDmCCit+QGWJSznChUOe0F1Sh1JwtIwKAX18Yb3WSWInI9lzYobMTyjcj83r4= X-Gm-Gg: AZuq6aLj82c/jDZyRS6qc6WipBvsRmVYThM58aMIXrb62PbridXscJbC7aC1IoxkEvt mNg1m3+Lbz31b5MuSlPg9AAoRjuln6hPUiEnhEADJkkZ87igHz1RHXKnDAoiH+ZSxDkkrKXzGum yQz3kUl/zpsZpFQhGl7MJnR5hqIjk8UFhPQ43ZH2o7l+dELiayq8t/Pd8CFTsxkzNGVrucCzRUa kXZ80gQI9xLXpiRvYLw2jgUzH+Dc98zgREZxUwTsxjCem1JFaf4cdAUVmQYhNlUNwL8pAptdg5j KfgTcs0TT6891E12oh026wAOFnFe9vSMzqmcR0ovGlcb5asmXHCZs/FHP1QzGQZY4MYnV+c2idT tpIZiH5FPerP9gRlwRi4MBy+GQxIm+6JEldzwQVESDL7n0YR/oe63ANSDVLvl/NZw3FqqWCCbn7 2r0FfXSGaWlJ4aZGmRK8AMHe/tbi+Epsg= X-Received: by 2002:a05:7022:207:b0:11b:c2fd:3960 with SMTP id a92af1059eb24-126f47cbed9mr276671c88.28.1770153851214; Tue, 03 Feb 2026 13:24:11 -0800 (PST) Received: from medusa.lab.kspace.sh ([208.88.152.253]) by smtp.googlemail.com with UTF8SMTPSA id a92af1059eb24-126f503087esm421549c88.12.2026.02.03.13.24.10 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 03 Feb 2026 13:24:10 -0800 (PST) Date: Tue, 3 Feb 2026 13:24:09 -0800 From: Mohamed Khalfella To: Hannes Reinecke Cc: Justin Tee , Naresh Gottumukkala , Paul Ely , Chaitanya Kulkarni , Christoph Hellwig , Jens Axboe , Keith Busch , Sagi Grimberg , Aaron Dailey , Randy Jennings , Dhaval Giani , linux-nvme@lists.infradead.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH v2 10/14] nvme-tcp: Use CCR to recover controller that hits an error Message-ID: <20260203212409.GG3729-mkhalfella@purestorage.com> References: <20260130223531.2478849-1-mkhalfella@purestorage.com> <20260130223531.2478849-11-mkhalfella@purestorage.com> <48a05027-9ca2-4e84-a7ac-946391ed1e26@suse.de> 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=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <48a05027-9ca2-4e84-a7ac-946391ed1e26@suse.de> On Tue 2026-02-03 06:34:51 +0100, Hannes Reinecke wrote: > On 1/30/26 23:34, Mohamed Khalfella wrote: > > An alive nvme controller that hits an error now will move to FENCING > > state instead of RESETTING state. ctrl->fencing_work attempts CCR to > > terminate inflight IOs. If CCR succeeds, switch to FENCED -> RESETTING > > and continue error recovery as usual. If CCR fails, the behavior depends > > on whether the subsystem supports CQT or not. If CQT is not supported > > then reset the controller immediately as if CCR succeeded in order to > > maintain the current behavior. If CQT is supported switch to time-based > > recovery. Schedule ctrl->fenced_work resets the controller when time > > based recovery finishes. > > > > Either ctrl->err_work or ctrl->reset_work can run after a controller is > > fenced. Flush fencing work when either work run. > > > > Signed-off-by: Mohamed Khalfella > > --- > > drivers/nvme/host/tcp.c | 62 ++++++++++++++++++++++++++++++++++++++++- > > 1 file changed, 61 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c > > index 69cb04406b47..af8d3b36a4bb 100644 > > --- a/drivers/nvme/host/tcp.c > > +++ b/drivers/nvme/host/tcp.c > > @@ -193,6 +193,8 @@ struct nvme_tcp_ctrl { > > struct sockaddr_storage src_addr; > > struct nvme_ctrl ctrl; > > > > + struct work_struct fencing_work; > > + struct delayed_work fenced_work; > > struct work_struct err_work; > > struct delayed_work connect_work; > > struct nvme_tcp_request async_req; > > @@ -611,6 +613,12 @@ static void nvme_tcp_init_recv_ctx(struct nvme_tcp_queue *queue) > > > > static void nvme_tcp_error_recovery(struct nvme_ctrl *ctrl) > > { > > + if (nvme_change_ctrl_state(ctrl, NVME_CTRL_FENCING)) { > > + dev_warn(ctrl->device, "starting controller fencing\n"); > > + queue_work(nvme_wq, &to_tcp_ctrl(ctrl)->fencing_work); > > + return; > > + } > > + > > Don't you need to flush any outstanding 'fenced_work' queue items here > before calling 'queue_work()'? I do not think we need to flush ctr->fencing_work. It can not be running at this time. > > > if (!nvme_change_ctrl_state(ctrl, NVME_CTRL_RESETTING)) > > return; > > > > @@ -2470,12 +2478,59 @@ static void nvme_tcp_reconnect_ctrl_work(struct work_struct *work) > > nvme_tcp_reconnect_or_remove(ctrl, ret); > > } > > > > +static void nvme_tcp_fenced_work(struct work_struct *work) > > +{ > > + struct nvme_tcp_ctrl *tcp_ctrl = container_of(to_delayed_work(work), > > + struct nvme_tcp_ctrl, fenced_work); > > + struct nvme_ctrl *ctrl = &tcp_ctrl->ctrl; > > + > > + nvme_change_ctrl_state(ctrl, NVME_CTRL_FENCED); > > + if (nvme_change_ctrl_state(ctrl, NVME_CTRL_RESETTING)) > > + queue_work(nvme_reset_wq, &tcp_ctrl->err_work); > > +} > > + > > +static void nvme_tcp_fencing_work(struct work_struct *work) > > +{ > > + struct nvme_tcp_ctrl *tcp_ctrl = container_of(work, > > + struct nvme_tcp_ctrl, fencing_work); > > + struct nvme_ctrl *ctrl = &tcp_ctrl->ctrl; > > + unsigned long rem; > > + > > + rem = nvme_fence_ctrl(ctrl); > > + if (!rem) > > + goto done; > > + > > + if (!ctrl->cqt) { > > + dev_info(ctrl->device, > > + "CCR failed, CQT not supported, skip time-based recovery\n"); > > + goto done; > > + } > > + > > As mentioned, cqt handling should be part of another patchset. Let us suppose we drop cqt from this patchset - How will we be able to calculate CCR time budget? Currently it is calculated by nvme_fence_timeout_ms() - What should we do if CCR fails? Retry requests immediately? > > + dev_info(ctrl->device, > > + "CCR failed, switch to time-based recovery, timeout = %ums\n", > > + jiffies_to_msecs(rem)); > > + queue_delayed_work(nvme_wq, &tcp_ctrl->fenced_work, rem); > > + return; > > + > > Why do you need the 'fenced' workqueue at all? All it does is queing yet > another workqueue item, which certainly can be done from the 'fencing' > workqueue directly, no? It is possible to drop ctr->fenced_work and requeue ctrl->fencing_work as delayed work to implement request hold time. If we do that then we need to modify nvme_tcp_fencing_work() to tell if it is being called for 'fencing' or 'fenced'. The first version of this patch used a controller flag RECOVERED for that and it has been suggested to use a separate work to simplify the logic and drop the controller flag. > > > +done: > > + nvme_change_ctrl_state(ctrl, NVME_CTRL_FENCED); > > + if (nvme_change_ctrl_state(ctrl, NVME_CTRL_RESETTING)) > > + queue_work(nvme_reset_wq, &tcp_ctrl->err_work); > > +} > > + > > +static void nvme_tcp_flush_fencing_work(struct nvme_ctrl *ctrl) > > +{ > > + flush_work(&to_tcp_ctrl(ctrl)->fencing_work); > > + flush_delayed_work(&to_tcp_ctrl(ctrl)->fenced_work); > > +} > > + > > static void nvme_tcp_error_recovery_work(struct work_struct *work) > > { > > struct nvme_tcp_ctrl *tcp_ctrl = container_of(work, > > struct nvme_tcp_ctrl, err_work); > > struct nvme_ctrl *ctrl = &tcp_ctrl->ctrl; > > > > + nvme_tcp_flush_fencing_work(ctrl); > > Why not 'fenced_work' ? You mean rename nvme_tcp_flush_fencing_work() to nvme_tcp_flush_fenced_work()? If yes, then I can do that if you think it makes more sense. > > > if (nvme_tcp_key_revoke_needed(ctrl)) > > nvme_auth_revoke_tls_key(ctrl); > > nvme_stop_keep_alive(ctrl); > > @@ -2518,6 +2573,7 @@ static void nvme_reset_ctrl_work(struct work_struct *work) > > container_of(work, struct nvme_ctrl, reset_work); > > int ret; > > > > + nvme_tcp_flush_fencing_work(ctrl); > > Same. > > > if (nvme_tcp_key_revoke_needed(ctrl)) > > nvme_auth_revoke_tls_key(ctrl); > > nvme_stop_ctrl(ctrl); > > @@ -2643,13 +2699,15 @@ static enum blk_eh_timer_return nvme_tcp_timeout(struct request *rq) > > struct nvme_tcp_cmd_pdu *pdu = nvme_tcp_req_cmd_pdu(req); > > struct nvme_command *cmd = &pdu->cmd; > > int qid = nvme_tcp_queue_id(req->queue); > > + enum nvme_ctrl_state state; > > > > dev_warn(ctrl->device, > > "I/O tag %d (%04x) type %d opcode %#x (%s) QID %d timeout\n", > > rq->tag, nvme_cid(rq), pdu->hdr.type, cmd->common.opcode, > > nvme_fabrics_opcode_str(qid, cmd), qid); > > > > - if (nvme_ctrl_state(ctrl) != NVME_CTRL_LIVE) { > > + state = nvme_ctrl_state(ctrl); > > + if (state != NVME_CTRL_LIVE && state != NVME_CTRL_FENCING) { > > 'FENCED' too, presumably? I do not think it makes a difference here. FENCED and RESETTING are almost the same states. > > > /* > > * If we are resetting, connecting or deleting we should > > * complete immediately because we may block controller > > @@ -2904,6 +2962,8 @@ static struct nvme_tcp_ctrl *nvme_tcp_alloc_ctrl(struct device *dev, > > > > INIT_DELAYED_WORK(&ctrl->connect_work, > > nvme_tcp_reconnect_ctrl_work); > > + INIT_DELAYED_WORK(&ctrl->fenced_work, nvme_tcp_fenced_work); > > + INIT_WORK(&ctrl->fencing_work, nvme_tcp_fencing_work); > > INIT_WORK(&ctrl->err_work, nvme_tcp_error_recovery_work); > > INIT_WORK(&ctrl->ctrl.reset_work, nvme_reset_ctrl_work); > > > > Here you are calling CCR whenever error recovery is triggered. > This will cause CCR to be send from a command timeout, which is > technically wrong (CCR should be send when the KATO timeout expires, > not when a command timout expires). Both could be vastly different. KATO is driven by the host. What does KTO expires mean? I think KATO expiry is more applicable to target, no? KATO timeout is a signal of an error that target is not reachable or something is wrong with the target? > > So I'd prefer to have CCR send whenever KATO timeout triggers, and > lease to current command timeout mechanism in place. Assuming we used CCR only when KATO request times out. What should we do when we hit other errors? should nvme_tcp_error_recovery() is called from many places to handle errors and it effectively resets the controller. What should this function do if not trigger CCR? > > Cheers, > > Hannes > -- > Dr. Hannes Reinecke Kernel Storage Architect > hare@suse.de +49 911 74053 688 > SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg > HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich