From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-dy1-f182.google.com (mail-dy1-f182.google.com [74.125.82.182]) (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 39157425CF9 for ; Thu, 26 Mar 2026 17:40:14 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=74.125.82.182 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1774546820; cv=none; b=kOl5M7skQT1HyhebG9f5juMjPZdlp0ND/0c3mc8DGwctMXRof8W0uebfprttxvOxWKrNIefHqhzwec6IXx5A0h8d61iS0g3DWsg4bTDOMzhcXdWwYvsCjzPPYRiNd3rVa0W5blD9fSl0LgT6lWZ3PB7D5wpm1uZoI79giJ6YRSI= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1774546820; c=relaxed/simple; bh=w7ON45a/nQ0i2V220wTWpSPVItWWSC8lvc4qmUdAS5w=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=To9eZ3H6KCmCXEypPEhP/rc99FcnCYNNvBZRzj6C6WeTNeWx1gyos4EKYza4z/PFtvhks8qTgxlqR7UZwmklzkYRbJcrYe2maEu/NsBku2HtVwzPcS7tPqKa0GNHCFw+MzJadEAWKk0w1tQTXGgxjy0XsHjkhqpfWmpLph/4/LA= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=quarantine 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=AR9wbZBt; arc=none smtp.client-ip=74.125.82.182 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=quarantine 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="AR9wbZBt" Received: by mail-dy1-f182.google.com with SMTP id 5a478bee46e88-2c160308a54so3204568eec.0 for ; Thu, 26 Mar 2026 10:40:14 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=purestorage.com; s=google2022; t=1774546814; x=1775151614; 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=q3F5UJjPKIs+RVyn9q5QF21wAHd9eT+mZHlFuJucRmw=; b=AR9wbZBtUCg0Do7sV39cch7exij3iibqSX+U+9s61ADM8cZgpuTTbTFsGjxyuRGq7W YT9Qwklwvlbwy173YR3dn2kehI4Oc/SH0EZ9rxsgWnNMzd0ML1j6hJ7W2STYqR6cCbEw lO4CG8YGI6AwNQ7ZACebdgIsOyR8ZyBoRxHI3K+g86YfeMRrZeC6tvKGvXzdaSVLdlTR J+N1zwVDBlZgXU9lbURpzyez9TYvYmbv095FOHIU18ZvKJ5xuZxbLG207iAmTM/LHXA+ YUwsqPEk93wZIqrKMNRw4t24hbX4FYqQIAUA+zT+2ErTQn5OlrBubQOZYNIMlwgPJkdt oFpw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20251104; t=1774546814; x=1775151614; 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=q3F5UJjPKIs+RVyn9q5QF21wAHd9eT+mZHlFuJucRmw=; b=J5223HvpWI+rS4iUwTf3Oi9cqmvuoG96uv0jWKUjYHxi60zcFIL/DMQ1ka7u6Uajqw Y3KcH/K5jArtJyV6ncin3y9mLbeRWXTXww97G7GJDICoUoD/u1SuO3XEk0O/15lYpv5/ /lZfGx7EqLLR5eS+zE/VUN/vUKJx6SWy66g2v2cGHi7hZBzvLiE7N9mSOi50kLcqshkA l2wkIWaASJqyJ8v5DoIlNyqzHzyVzkHLAaRCo9QwU8Ui5XtXFlTEt+g01nBYsh5evGdc eSkMPp4rE93/kyg4jVScpygDn1YV4On6JS7bcx9Opbcc/wRuchkEBC4Qr6reMbq13Xk4 Rq6g== X-Forwarded-Encrypted: i=1; AJvYcCVrjVgpfalKcoyxFdLTFhXkCsZ189l6woey6R3MtW8OMAgHpd4VXI8licyhqiqDhBbR8evE6k+y4iGRdoI=@vger.kernel.org X-Gm-Message-State: AOJu0YzIJ2na8mMfPIfHT7ZaLaG9ouTT3t27LuqWmaoZck5yOBwWYAI6 l12EFMHb3XCX8kO+EX9CWM1a8AXq30qO7Dk6oCMgT7YGNqj0yoFWhCbu/2k1uJ8p+YI= X-Gm-Gg: ATEYQzzz0VTPMwW92VMhH0QGpA0U0n9iM+qPFVts/v6l667CruiBkhXrskRm4V+m5kw B5cZRzP+3hEXOmprJ1JuAPi59A+DSkmkmPK2CgDk8ONSwC51OFKZ2N7DxumqkvyjG260lUPrdoU vTu744ygOFxxuI2eTsZVNvAtyzb4zCuzK2S0c001FsHf6BXLBs9b39AMqzC9eF77GedgJXvPqWZ 23lnYMD0+qcBTI3lGUrSNcgqcILcwiXUfOBfpa5JVw6dxAXn6lWeaEaODCJmhann10YvqpsyFGu Abq8pGm9xo+UXoS6/sLFzDgtLtI3vV8m0N98MGuKkhCXIw/aw00T/PgMy4s3QVPfevlod6ON+5h 2KZDSYLFu1s6rJ5cR87kSkgZM8HLmYLkan9arppcJHfz+pSP6nXfdghunNzCbheThFZTbV26cGD bh6ZpPRoqdxnRELXxlhaFbGkforOyqp0458KqIZkjEAbI= X-Received: by 2002:a05:7300:fb87:b0:2c1:102e:5694 with SMTP id 5a478bee46e88-2c15d42acf5mr5211027eec.34.1774546813386; Thu, 26 Mar 2026 10:40:13 -0700 (PDT) Received: from medusa.lab.kspace.sh ([208.88.152.253]) by smtp.googlemail.com with UTF8SMTPSA id 5a478bee46e88-2c16ee01674sm3365692eec.27.2026.03.26.10.40.12 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 26 Mar 2026 10:40:12 -0700 (PDT) Date: Thu, 26 Mar 2026 10:40:11 -0700 From: Mohamed Khalfella To: James Smart Cc: Justin Tee , Naresh Gottumukkala , Paul Ely , Chaitanya Kulkarni , Christoph Hellwig , Jens Axboe , Keith Busch , Sagi Grimberg , Hannes Reinecke , Aaron Dailey , Randy Jennings , Dhaval Giani , linux-nvme@lists.infradead.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH v3 13/21] nvme-fc: Use CCR to recover controller that hits an error Message-ID: <20260326174011.GP3435530-mkhalfella@purestorage.com> References: <20260214042753.4073668-1-mkhalfella@purestorage.com> <20260214042753.4073668-14-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 Fri 2026-02-27 17:03:55 -0800, James Smart wrote: > On 2/13/2026 8:25 PM, 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. Regardless of the success or failure of CCR > > operation the controller is transitioned to RESETTING state to continue > > error recovery process. > > > > Signed-off-by: Mohamed Khalfella > > --- > > drivers/nvme/host/fc.c | 30 ++++++++++++++++++++++++++++++ > > 1 file changed, 30 insertions(+) > > > > diff --git a/drivers/nvme/host/fc.c b/drivers/nvme/host/fc.c > > index e6ffaa19aba4..6ebabfb7e76d 100644 > > --- a/drivers/nvme/host/fc.c > > +++ b/drivers/nvme/host/fc.c > > @@ -166,6 +166,7 @@ struct nvme_fc_ctrl { > > struct blk_mq_tag_set admin_tag_set; > > struct blk_mq_tag_set tag_set; > > > > + struct work_struct fencing_work; > > struct work_struct ioerr_work; > > struct delayed_work connect_work; > > > > @@ -1868,6 +1869,24 @@ __nvme_fc_fcpop_chk_teardowns(struct nvme_fc_ctrl *ctrl, > > } > > } > > > > +static void nvme_fc_fencing_work(struct work_struct *work) > > +{ > > + struct nvme_fc_ctrl *fc_ctrl = > > + container_of(work, struct nvme_fc_ctrl, fencing_work); > > + struct nvme_ctrl *ctrl = &fc_ctrl->ctrl; > > + unsigned long rem; > > + > > + rem = nvme_fence_ctrl(ctrl); > > + if (rem) { > > + dev_info(ctrl->device, > > + "CCR failed, skipping time-based recovery\n"); > > + } > > + > > + nvme_change_ctrl_state(ctrl, NVME_CTRL_FENCED); > > + if (nvme_change_ctrl_state(ctrl, NVME_CTRL_RESETTING)) > > + queue_work(nvme_reset_wq, &fc_ctrl->ioerr_work); > > catch the rework of prior patch I ended up not dropping ctrl->ioerr_work. There are situations we need error recovery work to run on a separate thread. > > > +} > > + > > static void > > nvme_fc_ctrl_ioerr_work(struct work_struct *work) > > { > > @@ -1889,6 +1908,7 @@ nvme_fc_ctrl_ioerr_work(struct work_struct *work) > > return; > > } > > > > + flush_work(&ctrl->fencing_work); > > nvme_fc_error_recovery(ctrl); > > } > > > > @@ -1915,6 +1935,14 @@ static void nvme_fc_start_ioerr_recovery(struct nvme_fc_ctrl *ctrl, > > { > > enum nvme_ctrl_state state; > > > From prior patch - the CONNECTING logic should be here.... Yes, it is here. The check for CONNECTING state is at the top of nvme_fc_start_ioerr_recovery(). > > > + if (nvme_change_ctrl_state(&ctrl->ctrl, NVME_CTRL_FENCING)) { > > + dev_warn(ctrl->ctrl.device, > > + "NVME-FC{%d}: starting controller fencing %s\n", > > + ctrl->cnum, errmsg); > > + queue_work(nvme_wq, &ctrl->fencing_work); > > + return; > > + } > > + > > if (nvme_change_ctrl_state(&ctrl->ctrl, NVME_CTRL_RESETTING)) { > > dev_warn(ctrl->ctrl.device, "NVME-FC{%d}: starting error recovery %s\n", > > ctrl->cnum, errmsg); > > @@ -3322,6 +3350,7 @@ nvme_fc_reset_ctrl_work(struct work_struct *work) > > struct nvme_fc_ctrl *ctrl = > > container_of(work, struct nvme_fc_ctrl, ctrl.reset_work); > > > > + flush_work(&ctrl->fencing_work); > > nvme_stop_ctrl(&ctrl->ctrl); > > > > /* will block will waiting for io to terminate */ > > @@ -3497,6 +3526,7 @@ nvme_fc_alloc_ctrl(struct device *dev, struct nvmf_ctrl_options *opts, > > > > INIT_WORK(&ctrl->ctrl.reset_work, nvme_fc_reset_ctrl_work); > > INIT_DELAYED_WORK(&ctrl->connect_work, nvme_fc_connect_ctrl_work); > > + INIT_WORK(&ctrl->fencing_work, nvme_fc_fencing_work); > > INIT_WORK(&ctrl->ioerr_work, nvme_fc_ctrl_ioerr_work); > > spin_lock_init(&ctrl->lock); > > > > there is a little to be in sync with my comment on the prior patch, but > otherwise what is here is fine. > > What bothers me in this process is - there are certainly conditions > where there is not connectivity loss where FC can send things such as > the ABTS or a Disconnect LS that can inform the controller to start > terminating. Its odd that we skip this step and go directly to the CCR > reset to terminate the controller. We should have been able to continue > to send the things that start to directly tear down the controller which > can be happening in parallel with the CCR. Depending on how the target is implemented ABTS or Disconnect LS do not guarantee inflight IOs are terminated. CCR main point is terminate inflight IOs making it safe to retry failed IOs.