[PATCH v7 00/17] TP8028 Rapid Path Failure Recovery

[PATCH v7 00/17] TP8028 Rapid Path Failure Recovery

[Mohamed Khalfella]

This patchset adds support for TP8028 Rapid Path Failure Recovery for
both the nvme target and initiator. Rapid Path Failure Recovery brings
Cross-Controller Reset (CCR) functionality to nvme. This allows an nvme
host to send an nvme command to a source nvme controller to reset the
impacted nvme controller, provided that both source and impacted
controllers are in the same nvme subsystem.

The main use of CCR is when one path to the nvme subsystem fails.
Inflight IOs on the impacted nvme controller need to be terminated
first before they can be retried on another path. Otherwise, data
corruption may happen. CCR provides a quick way to terminate these IOs
on the unreachable nvme controller, allowing recovery to move quickly
and avoid unnecessary delays. In case CCR is not possible, inflight
requests are held for a duration defined by TP4129 KATO Corrections
and Clarifications before they are allowed to be retried.

On the target side:

* New struct members have been added to support CCR. struct
  nvme_id_ctrl has been updated with CIU (Controller Instance
  Uniquifier), CIRN (Controller Instance Random Number), and CQT
  (Command Quiesce Time). The combination of CIU, CNTLID, and CIRN is
  used to identify the impacted controller in the CCR command.

* The CCR nvme command implemented on the target causes the impacted
  controller to fail and drop its connections to the host.

* The CCR log page contains the status of pending CCR requests. An
  entry is added to the log page after a CCR request is validated.
  Completed CCR requests are removed from the log page when the
  controller becomes ready or when requested in the Get Log Page
  command.

* An AEN is sent when a CCR completes to let the host know that it is
  safe to retry inflight requests.

* CQT is a subsystem attribute in configfs, attr_cqt, and defaults
  to 0. Changing it disconnects the existing controllers of the
  subsystem so that hosts read the new value.

On the host side:

* CIU, CIRN, and CQT have been added to struct nvme_ctrl and exposed
  in sysfs. CIU and CIRN can be used to construct and manually send
  admin-passthru CCR commands.

* New controller states FENCING and FENCED have been added to make
  sure that inflight requests do not get canceled if they time out
  during the fencing process. FENCED exists so that the controller
  state machine does not have a transition from FENCING to RESETTING.
  Instead, FENCING -> FENCED -> RESETTING. This prevents a controller
  being fenced from getting reset. Only after fencing finishes is the
  impacted controller reset.

* Controller recovery in nvme_fence_ctrl() is invoked when a LIVE
  controller hits an error or when a request times out. CCR is
  attempted first to reset the impacted controller. If it fails,
  inflight requests are held until it is safe to retry them.

* Updated the nvme fabric transports nvme-tcp, nvme-rdma, and nvme-fc
  to use CCR recovery.

* A controller can be deleted while it is fencing. The transport's
  ->stop_ctrl flushes the fencing works, so the delete path waits for
  the fence without holding any transport lock.

Ideally, all inflight requests should be held during controller
recovery and only retried after recovery is done. However, there are
known situations where that is not the case in this implementation.
These gaps will be addressed in future patches:

* A manual controller reset from sysfs of a LIVE controller will
  result in the controller going to the RESETTING state and all
  inflight requests being canceled immediately, and they may be
  retried on another path. A reset issued during a fencing window is
  rejected by the state machine.

* A manual controller delete from sysfs of a LIVE controller will also
  result in all inflight requests being canceled immediately, and they
  may be retried on another path. A delete issued during a fencing
  window waits for the CCR attempt to finish, but ends time-based
  recovery early: inflight requests are released when the delete
  starts and may be retried on another path before CQT has expired.

* In nvme-fc, the nvme controller will be deleted if the remote port
  disappears with no timeout specified. For a LIVE controller this
  still results in immediate cancellation of requests that may be
  retried on another path. A controller that is already fencing is
  handled like the manual delete above.

* In nvme-rdma, if the HCA is removed, all nvme controllers will be
  deleted. Deleting LIVE controllers still cancels inflight IOs, and
  they may be retried on another path. Controllers in a fencing window
  are handled like the manual delete above.

* Resets started by the nvme core rather than the transport, on an
  ANATT timeout, a persistent AER error, or a keep-alive allocation
  failure, move a LIVE controller to RESETTING without fencing it.

Changes from v6:

- nvmet: Rapid Path Failure Recovery set controller identify fields
  - Never generate a CIRN of 0
  - Commit message fixes

- nvmet/debugfs: Export controller CIU and CIRN via debugfs
  - Explain in the commit message why the values are exported

- nvmet: Implement CCR nvme command
  - Document why nvmet_ctrl_find_get_ccr() takes ctrl->lock

- nvmet: Implement CCR logpage / nvmet: Send an AEN on CCR completion
  - Move clearing the CCR AEN bit from the log page patch to the AEN
    patch, and do it under ctrl->lock
  - Assert ctrl->lock in nvmet_add_async_event_locked()

- nvme: Rapid Path Failure Recovery read controller identify fields
  - Document the ciu and cirn sysfs attributes in
    Documentation/ABI/stable/sysfs-nvme
  - Move nvme_fence_timeout_ms() to "nvme: Implement cross-controller
    reset recovery", where it is first used
  - Commit message rewritten

- nvme: Introduce FENCING and FENCED controller states
  - Allow DELETING and DELETING_NOIO from FENCING and FENCED. This
    replaces v6 patch 18, see below
  - Document the new states in Documentation/ABI/stable/sysfs-nvme
  - Commit message rewritten
  - Dropped the Reviewed-by tags due to the above changes

- nvme: Implement cross-controller reset recovery
  - Describe nvme_fence_ctrl() in the commit message

- nvme: Implement cross-controller reset completion
  - Cancel ccr_work in nvme_uninit_ctrl() so it cannot run after the
    controller is freed

- nvme-tcp / nvme-rdma: Use CCR to recover controller that hits an error
  - Update the timeout handler comments for FENCING
  - Describe the timeout handling and controller deletion during
    fencing in the commit message

- nvme-fc: start error recovery instead of aborting timed out IOs
  - Count every abort in ctrl->iocnt, not only those issued during the
    association teardown, and do not reset the count when the teardown
    starts
  - Outside LIVE, abort the timed out command from the timeout handler
    and return BLK_EH_DONE instead of queueing ioerr_work. This replaces
    queueing ioerr_work in DELETING/DELETING_NOIO, which could queue
    the work after nvme_fc_delete_ctrl() cancelled it for the last time
  - nvme_fc_start_ioerr_recovery() queues ioerr_work only after a
    successful transition to RESETTING
  - Drop the nvme_fc_reset_ctrl_work() reordering, which is no longer
    needed now that timed out commands are aborted outside LIVE
  - Document in the commit message that recovery only runs once the
    next patch is applied
  - Dropped Hannes's Reviewed-by due to the above changes

- nvme-fc: perform error recovery directly from ioerr_work
  - Do the recovery in ioerr_work itself and remove
    nvme_fc_error_recovery(). The work no longer re-checks the
    controller state, since it is only queued after a successful
    transition to RESETTING

- nvme-fc: Use CCR to recover controller that hits an error
  - Rearm the timer for a request that times out while FENCING
  - Dropped Hannes's Reviewed-by due to the above change

- nvme-fc: Hold inflight requests while in FENCING state
  - Replace synchronize_rcu() with an explicit FCPOP_STATE_HELD op
    state. nvme_fc_fcpio_done() holds a request with
    cmpxchg(COMPLETE -> HELD) and the flush cancels it with
    cmpxchg(COMPLETE -> IDLE), so exactly one of them completes it
  - __nvme_fc_abort_op() uses cmpxchg(ACTIVE -> ABORTED)
  - Count an aborted op down only after it has been completed or held,
    and make ctrl->iocnt an atomic_t
  - Start recovery before deciding whether to hold a request, and hold
    it only if the controller is then FENCING. Holding while LIVE
    could leave a request held forever when a delete won the race for
    the controller state
  - Rewrote the commit message
  - Dropped Hannes's Reviewed-by due to the above changes

- nvmet: Add support for CQT to nvme target
  - Mention in the commit message that changing attr_cqt disconnects
    existing controllers

- nvme: Add support for CQT to nvme host
  - Document the cqt sysfs attribute in
    Documentation/ABI/stable/sysfs-nvme
  - Dropped the Reviewed-by tags. The v6 cover letter announced this,
    but the tags were left in the v6 patch by mistake

- Dropped "nvme: let controller deletion wait out a fencing window"
  - nvme_delete_ctrl_wait() is gone, and nvme_delete_ctrl_sync() no
    longer waits for a fence. Deleting a controller in FENCING or
    FENCED now works directly through the state machine, see
    "nvme: Introduce FENCING and FENCED controller states". The tcp
    and rdma module exit paths and rdma device removal use
    nvme_delete_ctrl() again

v6: https://lore.kernel.org/all/20260920182936.2317916-1-mkhalfella@purestorage.com/
v5: https://lore.kernel.org/all/20260712022437.3743117-1-mkhalfella@purestorage.com/

Mohamed Khalfella (17):
  nvmet: Rapid Path Failure Recovery set controller identify fields
  nvmet/debugfs: Export controller CIU and CIRN via debugfs
  nvmet: Implement CCR nvme command
  nvmet: Implement CCR logpage
  nvmet: Send an AEN on CCR completion
  nvme: Rapid Path Failure Recovery read controller identify fields
  nvme: Introduce FENCING and FENCED controller states
  nvme: Implement cross-controller reset recovery
  nvme: Implement cross-controller reset completion
  nvme-tcp: Use CCR to recover controller that hits an error
  nvme-rdma: Use CCR to recover controller that hits an error
  nvme-fc: start error recovery instead of aborting timed out IOs
  nvme-fc: perform error recovery directly from ioerr_work
  nvme-fc: Use CCR to recover controller that hits an error
  nvme-fc: Hold inflight requests while in FENCING state
  nvmet: Add support for CQT to nvme target
  nvme: Add support for CQT to nvme host

 Documentation/ABI/stable/sysfs-nvme |  26 +-
 drivers/nvme/host/constants.c       |   1 +
 drivers/nvme/host/core.c            | 237 +++++++++++++++++-
 drivers/nvme/host/fc.c              | 360 +++++++++++++++++++++-------
 drivers/nvme/host/multipath.c       |   2 +
 drivers/nvme/host/nvme.h            |  26 ++
 drivers/nvme/host/rdma.c            |  67 +++++-
 drivers/nvme/host/sysfs.c           |  27 +++
 drivers/nvme/host/tcp.c             |  67 +++++-
 drivers/nvme/target/admin-cmd.c     | 126 ++++++++++
 drivers/nvme/target/configfs.c      |  36 +++
 drivers/nvme/target/core.c          | 122 +++++++++-
 drivers/nvme/target/debugfs.c       |  22 ++
 drivers/nvme/target/nvmet.h         |  20 +-
 include/linux/nvme.h                |  70 +++++-
 15 files changed, 1106 insertions(+), 103 deletions(-)


base-commit: f33c8bc785401669b29229bcf3d0086ba9f5b87c
-- 
2.56.0

[PATCH v7 01/17] nvmet: Rapid Path Failure Recovery set controller identify fields

[Mohamed Khalfella]

TP8028 Rapid Path Failure Recovery defined new fields in controller
identify response. The newly defined fields are:

- CIU (Controller Instance Uniquifier): is an 8bit non-zero value that
  is assigned a random value when controller is first created. The value
  will be incremented when RDY bit in CSTS register is asserted.
- CIRN (Controller Instance Random Number): is 64bit random value that
  gets generated when controller is created. CIRN is regenerated every
  time RDY bit in CSTS register is asserted.
- CCRL (Cross-Controller Reset Limit): is an 8bit value that defines the
  maximum number of in-progress controller reset operations. CCRL is
  hardcoded to 4 because TP8028 recommended a value no less than 4.

These fields are set for non-discovery subsystems only.

Signed-off-by: Mohamed Khalfella <mkhalfella@purestorage.com>
Reviewed-by: Hannes Reinecke <hare@suse.de>
Reviewed-by: Sagi Grimberg <sagi@grimberg.me>
Reviewed-by: Daniel Wagner <dwagner@suse.de>
---
 drivers/nvme/target/admin-cmd.c |  5 +++++
 drivers/nvme/target/core.c      |  9 +++++++++
 drivers/nvme/target/nvmet.h     |  2 ++
 include/linux/nvme.h            | 10 ++++++++--
 4 files changed, 24 insertions(+), 2 deletions(-)

diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
index f12d83098644..5710ae53eb83 100644
--- a/drivers/nvme/target/admin-cmd.c
+++ b/drivers/nvme/target/admin-cmd.c
@@ -702,6 +702,11 @@ static void nvmet_execute_identify_ctrl(struct nvmet_req *req)
 	id->mdts = nvmet_ctrl_mdts(req);
 	id->cntlid = cpu_to_le16(ctrl->cntlid);
 	id->ver = cpu_to_le32(ctrl->subsys->ver);
+	if (!nvmet_is_disc_subsys(ctrl->subsys)) {
+		id->ciu = ctrl->ciu;
+		id->cirn = cpu_to_le64(ctrl->cirn);
+		id->ccrl = NVMF_CCR_LIMIT;
+	}
 
 	/* XXX: figure out what to do about RTD3R/RTD3 */
 	id->oaes = cpu_to_le32(NVMET_AEN_CFG_OPTIONAL);
diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
index cc1955bc8c84..9c9ad4d9b292 100644
--- a/drivers/nvme/target/core.c
+++ b/drivers/nvme/target/core.c
@@ -1429,6 +1429,10 @@ static void nvmet_start_ctrl(struct nvmet_ctrl *ctrl)
 		return;
 	}
 
+	if (!nvmet_is_disc_subsys(ctrl->subsys)) {
+		ctrl->ciu = ((u8)(ctrl->ciu + 1)) ? : 1;
+		ctrl->cirn = get_random_u64() ? : 1;
+	}
 	ctrl->csts = NVME_CSTS_RDY;
 
 	/*
@@ -1729,6 +1733,11 @@ struct nvmet_ctrl *nvmet_alloc_ctrl(struct nvmet_alloc_ctrl_args *args)
 	}
 	ctrl->cntlid = ret;
 
+	if (!nvmet_is_disc_subsys(ctrl->subsys)) {
+		ctrl->ciu = get_random_u8() ? : 1;
+		ctrl->cirn = get_random_u64() ? : 1;
+	}
+
 	ret = nvmet_ctrl_init_pr(ctrl);
 	if (ret)
 		goto init_pr_fail;
diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
index 2c2460ef5e2b..fba4d320f907 100644
--- a/drivers/nvme/target/nvmet.h
+++ b/drivers/nvme/target/nvmet.h
@@ -274,7 +274,9 @@ struct nvmet_ctrl {
 	uuid_t			hostid;
 	u16			cntlid;
 	u16			max_qid;
+	u8			ciu;
 	u32			kato;
+	u64			cirn;
 	bool			tbkas;
 
 	struct nvmet_port	*port;
diff --git a/include/linux/nvme.h b/include/linux/nvme.h
index 6248711784b3..a3c1b1713b4f 100644
--- a/include/linux/nvme.h
+++ b/include/linux/nvme.h
@@ -21,6 +21,8 @@
 #define NVMF_TRADDR_SIZE	256
 #define NVMF_TSAS_SIZE		256
 
+#define NVMF_CCR_LIMIT		4
+
 #define NVME_DISC_SUBSYS_NAME	"nqn.2014-08.org.nvmexpress.discovery"
 
 #define NVME_NSID_ALL		0xffffffff
@@ -328,7 +330,10 @@ struct nvme_id_ctrl {
 	__le16			crdt1;
 	__le16			crdt2;
 	__le16			crdt3;
-	__u8			rsvd134[122];
+	__u8			rsvd134[1];
+	__u8			ciu;
+	__le64			cirn;
+	__u8			rsvd144[112];
 	__le16			oacs;
 	__u8			acl;
 	__u8			aerl;
@@ -389,7 +394,8 @@ struct nvme_id_ctrl {
 	__u8			msdbd;
 	__u8			rsvd1804[2];
 	__u8			dctype;
-	__u8			rsvd1807[241];
+	__u8			ccrl;
+	__u8			rsvd1808[240];
 	struct nvme_id_power_state	psd[32];
 	__u8			vs[1024];
 };
-- 
2.56.0

[PATCH v7 02/17] nvmet/debugfs: Export controller CIU and CIRN via debugfs

[Mohamed Khalfella]

Export ctrl->ciu and ctrl->cirn as debugfs files under controller
debugfs directory. The values are useful in matching host controller
with the corresponding target controller.

While tools like nvme-cli can parse and dump these values during normal
operation via identify commands, having these attributes exported in
debugfs is essential when a controller is stuck in a state where it
cannot process standard admin commands. Exposing these values directly
ensures administrators and diagnostic scripts can always retrieve CIU
and CIRN for manual Cross-Controller Reset (CCR) operations or path
correlations.

Signed-off-by: Mohamed Khalfella <mkhalfella@purestorage.com>
Reviewed-by: Hannes Reinecke <hare@suse.de>
Reviewed-by: Sagi Grimberg <sagi@grimberg.me>
Reviewed-by: Daniel Wagner <dwagner@suse.de>
---
 drivers/nvme/target/debugfs.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/drivers/nvme/target/debugfs.c b/drivers/nvme/target/debugfs.c
index e85fe1d4c9f8..221fa81e124f 100644
--- a/drivers/nvme/target/debugfs.c
+++ b/drivers/nvme/target/debugfs.c
@@ -153,6 +153,24 @@ static int nvmet_ctrl_tls_concat_show(struct seq_file *m, void *p)
 NVMET_DEBUGFS_ATTR(nvmet_ctrl_tls_concat);
 #endif
 
+static int nvmet_ctrl_instance_ciu_show(struct seq_file *m, void *p)
+{
+	struct nvmet_ctrl *ctrl = m->private;
+
+	seq_printf(m, "%02x\n", ctrl->ciu);
+	return 0;
+}
+NVMET_DEBUGFS_ATTR(nvmet_ctrl_instance_ciu);
+
+static int nvmet_ctrl_instance_cirn_show(struct seq_file *m, void *p)
+{
+	struct nvmet_ctrl *ctrl = m->private;
+
+	seq_printf(m, "%016llx\n", ctrl->cirn);
+	return 0;
+}
+NVMET_DEBUGFS_ATTR(nvmet_ctrl_instance_cirn);
+
 static const char *const nvmet_pr_type_names[] = {
 	[NVME_PR_WRITE_EXCLUSIVE]		= "write_exclusive",
 	[NVME_PR_EXCLUSIVE_ACCESS]		= "exclusive_access",
@@ -287,6 +305,10 @@ int nvmet_debugfs_ctrl_setup(struct nvmet_ctrl *ctrl)
 	debugfs_create_file("tls_key", S_IRUSR, ctrl->debugfs_dir, ctrl,
 			    &nvmet_ctrl_tls_key_fops);
 #endif
+	debugfs_create_file("ciu", S_IRUSR, ctrl->debugfs_dir, ctrl,
+			    &nvmet_ctrl_instance_ciu_fops);
+	debugfs_create_file("cirn", S_IRUSR, ctrl->debugfs_dir, ctrl,
+			    &nvmet_ctrl_instance_cirn_fops);
 	return 0;
 }
 
-- 
2.56.0

[PATCH v7 03/17] nvmet: Implement CCR nvme command

[Mohamed Khalfella]

Defined by TP8028 Rapid Path Failure Recovery, CCR (Cross-Controller
Reset) command is an nvme command issued to source controller by
initiator to reset impacted controller. Implement CCR command for linux
nvme target.

Signed-off-by: Mohamed Khalfella <mkhalfella@purestorage.com>
Reviewed-by: Hannes Reinecke <hare@suse.de>
Reviewed-by: Sagi Grimberg <sagi@grimberg.me>
---
 drivers/nvme/target/admin-cmd.c | 76 ++++++++++++++++++++++++++++++
 drivers/nvme/target/core.c      | 83 +++++++++++++++++++++++++++++++++
 drivers/nvme/target/nvmet.h     | 13 ++++++
 include/linux/nvme.h            | 23 +++++++++
 4 files changed, 195 insertions(+)

diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
index 5710ae53eb83..3df9db454db9 100644
--- a/drivers/nvme/target/admin-cmd.c
+++ b/drivers/nvme/target/admin-cmd.c
@@ -378,6 +378,7 @@ static void nvmet_get_cmd_effects_admin(struct nvmet_ctrl *ctrl,
 	log->acs[nvme_admin_get_features] =
 	log->acs[nvme_admin_async_event] =
 	log->acs[nvme_admin_keep_alive] =
+	log->acs[nvme_admin_cross_ctrl_reset] =
 		cpu_to_le32(NVME_CMD_EFFECTS_CSUPP);
 }
 
@@ -1629,6 +1630,78 @@ void nvmet_execute_keep_alive(struct nvmet_req *req)
 	nvmet_req_complete(req, status);
 }
 
+void nvmet_execute_cross_ctrl_reset(struct nvmet_req *req)
+{
+	struct nvmet_ctrl *ictrl, *sctrl = req->sq->ctrl;
+	struct nvme_command *cmd = req->cmd;
+	struct nvmet_ccr *ccr, *new_ccr;
+	int ccr_active, ccr_total;
+	u16 cntlid, status = NVME_SC_SUCCESS;
+
+	if (!nvmet_check_transfer_len(req, 0))
+		return;
+
+	cntlid = le16_to_cpu(cmd->ccr.icid);
+	if (sctrl->cntlid == cntlid) {
+		req->error_loc =
+			offsetof(struct nvme_cross_ctrl_reset_cmd, icid);
+		status = NVME_SC_INVALID_FIELD | NVME_STATUS_DNR;
+		goto out;
+	}
+
+	/* Find and get impacted controller */
+	ictrl = nvmet_ctrl_find_get_ccr(sctrl->subsys, sctrl->hostnqn,
+					cmd->ccr.ciu, cntlid,
+					le64_to_cpu(cmd->ccr.cirn));
+	if (!ictrl) {
+		/* Immediate Reset Successful */
+		nvmet_set_result(req, 1);
+		status = NVME_SC_SUCCESS;
+		goto out;
+	}
+
+	ccr_total = ccr_active = 0;
+	mutex_lock(&sctrl->lock);
+	list_for_each_entry(ccr, &sctrl->ccr_list, entry) {
+		if (ccr->ctrl == ictrl) {
+			status = NVME_SC_CCR_IN_PROGRESS | NVME_STATUS_DNR;
+			goto out_unlock;
+		}
+
+		ccr_total++;
+		if (ccr->ctrl)
+			ccr_active++;
+	}
+
+	if (ccr_active >= NVMF_CCR_LIMIT) {
+		status = NVME_SC_CCR_LIMIT_EXCEEDED;
+		goto out_unlock;
+	}
+	if (ccr_total >= NVMF_CCR_PER_PAGE) {
+		status = NVME_SC_CCR_LOGPAGE_FULL;
+		goto out_unlock;
+	}
+
+	new_ccr = kmalloc_obj(*new_ccr, GFP_KERNEL);
+	if (!new_ccr) {
+		status = NVME_SC_INTERNAL;
+		goto out_unlock;
+	}
+
+	new_ccr->ciu = cmd->ccr.ciu;
+	new_ccr->icid = cntlid;
+	new_ccr->ctrl = ictrl;
+	list_add_tail(&new_ccr->entry, &sctrl->ccr_list);
+
+out_unlock:
+	mutex_unlock(&sctrl->lock);
+	if (status == NVME_SC_SUCCESS)
+		nvmet_ctrl_fatal_error(ictrl);
+	nvmet_ctrl_put(ictrl);
+out:
+	nvmet_req_complete(req, status);
+}
+
 u32 nvmet_admin_cmd_data_len(struct nvmet_req *req)
 {
 	struct nvme_command *cmd = req->cmd;
@@ -1706,6 +1779,9 @@ u16 nvmet_parse_admin_cmd(struct nvmet_req *req)
 	case nvme_admin_keep_alive:
 		req->execute = nvmet_execute_keep_alive;
 		return 0;
+	case nvme_admin_cross_ctrl_reset:
+		req->execute = nvmet_execute_cross_ctrl_reset;
+		return 0;
 	default:
 		return nvmet_report_invalid_opcode(req);
 	}
diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
index 9c9ad4d9b292..86018e0fb158 100644
--- a/drivers/nvme/target/core.c
+++ b/drivers/nvme/target/core.c
@@ -117,6 +117,20 @@ u16 nvmet_zero_sgl(struct nvmet_req *req, off_t off, size_t len)
 	return 0;
 }
 
+void nvmet_ctrl_cleanup_ccrs(struct nvmet_ctrl *ctrl, bool all)
+{
+	struct nvmet_ccr *ccr, *tmp;
+
+	lockdep_assert_held(&ctrl->lock);
+
+	list_for_each_entry_safe(ccr, tmp, &ctrl->ccr_list, entry) {
+		if (all || ccr->ctrl == NULL) {
+			list_del(&ccr->entry);
+			kfree(ccr);
+		}
+	}
+}
+
 static u32 nvmet_max_nsid(struct nvmet_subsys *subsys)
 {
 	struct nvmet_ns *cur;
@@ -1432,6 +1446,7 @@ static void nvmet_start_ctrl(struct nvmet_ctrl *ctrl)
 	if (!nvmet_is_disc_subsys(ctrl->subsys)) {
 		ctrl->ciu = ((u8)(ctrl->ciu + 1)) ? : 1;
 		ctrl->cirn = get_random_u64() ? : 1;
+		nvmet_ctrl_cleanup_ccrs(ctrl, false);
 	}
 	ctrl->csts = NVME_CSTS_RDY;
 
@@ -1537,6 +1552,42 @@ struct nvmet_ctrl *nvmet_ctrl_find_get(const char *subsysnqn,
 	return ctrl;
 }
 
+struct nvmet_ctrl *nvmet_ctrl_find_get_ccr(struct nvmet_subsys *subsys,
+					   const char *hostnqn, u8 ciu,
+					   u16 cntlid, u64 cirn)
+{
+	struct nvmet_ctrl *ctrl, *ictrl = NULL;
+	bool found = false;
+
+	mutex_lock(&subsys->lock);
+	list_for_each_entry(ctrl, &subsys->ctrls, subsys_entry) {
+		if (ctrl->cntlid != cntlid)
+			continue;
+		if (strncmp(ctrl->hostnqn, hostnqn, NVMF_NQN_SIZE))
+			continue;
+
+		/*
+		 * Enabling the controller regenerates ciu and cirn under
+		 * ctrl->lock. Hold the lock so that a controller becoming
+		 * ready concurrently is matched against either the old or
+		 * the new instance, never a mix of both.
+		 */
+		mutex_lock(&ctrl->lock);
+		if (ctrl->ciu == ciu && ctrl->cirn == cirn)
+			found = true;
+		mutex_unlock(&ctrl->lock);
+
+		if (found) {
+			if (kref_get_unless_zero(&ctrl->ref))
+				ictrl = ctrl;
+			break;
+		}
+	}
+	mutex_unlock(&subsys->lock);
+
+	return ictrl;
+}
+
 u16 nvmet_check_ctrl_status(struct nvmet_req *req)
 {
 	if (unlikely(!(req->sq->ctrl->cc & NVME_CC_ENABLE))) {
@@ -1676,6 +1727,7 @@ struct nvmet_ctrl *nvmet_alloc_ctrl(struct nvmet_alloc_ctrl_args *args)
 	}
 #endif
 
+	INIT_LIST_HEAD(&ctrl->ccr_list);
 	INIT_WORK(&ctrl->async_event_work, nvmet_async_event_work);
 	INIT_LIST_HEAD(&ctrl->async_events);
 	INIT_RADIX_TREE(&ctrl->p2p_ns_map, GFP_KERNEL);
@@ -1791,12 +1843,43 @@ struct nvmet_ctrl *nvmet_alloc_ctrl(struct nvmet_alloc_ctrl_args *args)
 }
 EXPORT_SYMBOL_GPL(nvmet_alloc_ctrl);
 
+static void nvmet_ctrl_complete_pending_ccr(struct nvmet_ctrl *ctrl)
+{
+	struct nvmet_subsys *subsys = ctrl->subsys;
+	struct nvmet_ctrl *sctrl;
+	struct nvmet_ccr *ccr;
+
+	lockdep_assert_held(&subsys->lock);
+
+	/* Cleanup all CCRs issued by ctrl as source controller */
+	mutex_lock(&ctrl->lock);
+	nvmet_ctrl_cleanup_ccrs(ctrl, true);
+	mutex_unlock(&ctrl->lock);
+
+	/*
+	 * Find all CCRs targeting ctrl as impacted controller and
+	 * set ccr->ctrl to NULL. This tells the source controller
+	 * that CCR completed successfully.
+	 */
+	list_for_each_entry(sctrl, &subsys->ctrls, subsys_entry) {
+		mutex_lock(&sctrl->lock);
+		list_for_each_entry(ccr, &sctrl->ccr_list, entry) {
+			if (ccr->ctrl == ctrl) {
+				ccr->ctrl = NULL;
+				break;
+			}
+		}
+		mutex_unlock(&sctrl->lock);
+	}
+}
+
 static void nvmet_ctrl_free(struct kref *ref)
 {
 	struct nvmet_ctrl *ctrl = container_of(ref, struct nvmet_ctrl, ref);
 	struct nvmet_subsys *subsys = ctrl->subsys;
 
 	mutex_lock(&subsys->lock);
+	nvmet_ctrl_complete_pending_ccr(ctrl);
 	nvmet_ctrl_destroy_pr(ctrl);
 	nvmet_release_p2p_ns_map(ctrl);
 	list_del(&ctrl->subsys_entry);
diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
index fba4d320f907..5b8d87454772 100644
--- a/drivers/nvme/target/nvmet.h
+++ b/drivers/nvme/target/nvmet.h
@@ -279,6 +279,7 @@ struct nvmet_ctrl {
 	u64			cirn;
 	bool			tbkas;
 
+	struct list_head	ccr_list;
 	struct nvmet_port	*port;
 
 	u32			aen_enabled;
@@ -338,6 +339,13 @@ struct nvmet_passthru {
 	unsigned int		clear_ids;
 };
 
+struct nvmet_ccr {
+	struct nvmet_ctrl	*ctrl;
+	struct list_head	entry;
+	u16			icid;
+	u8			ciu;
+};
+
 struct nvmet_subsys {
 	enum nvme_subsys_type	type;
 
@@ -601,6 +609,7 @@ void nvmet_req_free_sgls(struct nvmet_req *req);
 void nvmet_execute_set_features(struct nvmet_req *req);
 void nvmet_execute_get_features(struct nvmet_req *req);
 void nvmet_execute_keep_alive(struct nvmet_req *req);
+void nvmet_execute_cross_ctrl_reset(struct nvmet_req *req);
 
 u16 nvmet_check_cqid(struct nvmet_ctrl *ctrl, u16 cqid, bool create);
 u16 nvmet_check_io_cqid(struct nvmet_ctrl *ctrl, u16 cqid, bool create);
@@ -643,6 +652,10 @@ struct nvmet_ctrl *nvmet_alloc_ctrl(struct nvmet_alloc_ctrl_args *args);
 struct nvmet_ctrl *nvmet_ctrl_find_get(const char *subsysnqn,
 				       const char *hostnqn, u16 cntlid,
 				       struct nvmet_req *req);
+struct nvmet_ctrl *nvmet_ctrl_find_get_ccr(struct nvmet_subsys *subsys,
+					   const char *hostnqn, u8 ciu,
+					   u16 cntlid, u64 cirn);
+void nvmet_ctrl_cleanup_ccrs(struct nvmet_ctrl *ctrl, bool all);
 void nvmet_ctrl_put(struct nvmet_ctrl *ctrl);
 u16 nvmet_check_ctrl_status(struct nvmet_req *req);
 ssize_t nvmet_ctrl_host_traddr(struct nvmet_ctrl *ctrl,
diff --git a/include/linux/nvme.h b/include/linux/nvme.h
index a3c1b1713b4f..d31c9648af43 100644
--- a/include/linux/nvme.h
+++ b/include/linux/nvme.h
@@ -22,6 +22,7 @@
 #define NVMF_TSAS_SIZE		256
 
 #define NVMF_CCR_LIMIT		4
+#define NVMF_CCR_PER_PAGE	511
 
 #define NVME_DISC_SUBSYS_NAME	"nqn.2014-08.org.nvmexpress.discovery"
 
@@ -1233,6 +1234,22 @@ struct nvme_zone_mgmt_recv_cmd {
 	__le32			cdw14[2];
 };
 
+struct nvme_cross_ctrl_reset_cmd {
+	__u8			opcode;
+	__u8			flags;
+	__u16			command_id;
+	__le32			nsid;
+	__le64			rsvd2[2];
+	union nvme_data_ptr	dptr;
+	__le16			icid;
+	__u8			ciu;
+	__u8			rsvd10;
+	__le32			cdw11;
+	__le64			cirn;
+	__le32			cdw14;
+	__le32			cdw15;
+};
+
 struct nvme_io_mgmt_recv_cmd {
 	__u8			opcode;
 	__u8			flags;
@@ -1331,6 +1348,7 @@ enum nvme_admin_opcode {
 	nvme_admin_virtual_mgmt		= 0x1c,
 	nvme_admin_nvme_mi_send		= 0x1d,
 	nvme_admin_nvme_mi_recv		= 0x1e,
+	nvme_admin_cross_ctrl_reset	= 0x38,
 	nvme_admin_dbbuf		= 0x7C,
 	nvme_admin_format_nvm		= 0x80,
 	nvme_admin_security_send	= 0x81,
@@ -1364,6 +1382,7 @@ enum nvme_admin_opcode {
 		nvme_admin_opcode_name(nvme_admin_virtual_mgmt),	\
 		nvme_admin_opcode_name(nvme_admin_nvme_mi_send),	\
 		nvme_admin_opcode_name(nvme_admin_nvme_mi_recv),	\
+		nvme_admin_opcode_name(nvme_admin_cross_ctrl_reset),	\
 		nvme_admin_opcode_name(nvme_admin_dbbuf),		\
 		nvme_admin_opcode_name(nvme_admin_format_nvm),		\
 		nvme_admin_opcode_name(nvme_admin_security_send),	\
@@ -2022,6 +2041,7 @@ struct nvme_command {
 		struct nvme_dbbuf dbbuf;
 		struct nvme_directive_cmd directive;
 		struct nvme_io_mgmt_recv_cmd imr;
+		struct nvme_cross_ctrl_reset_cmd ccr;
 	};
 };
 
@@ -2186,6 +2206,9 @@ enum {
 	NVME_SC_PMR_SAN_PROHIBITED	= 0x123,
 	NVME_SC_ANA_GROUP_ID_INVALID	= 0x124,
 	NVME_SC_ANA_ATTACH_FAILED	= 0x125,
+	NVME_SC_CCR_IN_PROGRESS		= 0x13f,
+	NVME_SC_CCR_LOGPAGE_FULL	= 0x140,
+	NVME_SC_CCR_LIMIT_EXCEEDED	= 0x141,
 
 	/*
 	 * I/O Command Set Specific - NVM commands:
-- 
2.56.0

[PATCH v7 04/17] nvmet: Implement CCR logpage

[Mohamed Khalfella]

Defined by TP8028 Rapid Path Failure Recovery, CCR (Cross-Controller
Reset) log page contains an entry for each CCR request submitted to
source controller. Implement CCR logpage for nvme linux target.

Signed-off-by: Mohamed Khalfella <mkhalfella@purestorage.com>
Reviewed-by: Hannes Reinecke <hare@suse.de>
Reviewed-by: Sagi Grimberg <sagi@grimberg.me>
Reviewed-by: Daniel Wagner <dwagner@suse.de>
---
 drivers/nvme/target/admin-cmd.c | 43 +++++++++++++++++++++++++++++++++
 include/linux/nvme.h            | 29 ++++++++++++++++++++++
 2 files changed, 72 insertions(+)

diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
index 3df9db454db9..25c320752d2a 100644
--- a/drivers/nvme/target/admin-cmd.c
+++ b/drivers/nvme/target/admin-cmd.c
@@ -220,6 +220,7 @@ static void nvmet_execute_get_supported_log_pages(struct nvmet_req *req)
 	logs->lids[NVME_LOG_FEATURES] = cpu_to_le32(NVME_LIDS_LSUPP);
 	logs->lids[NVME_LOG_RMI] = cpu_to_le32(NVME_LIDS_LSUPP);
 	logs->lids[NVME_LOG_RESERVATION] = cpu_to_le32(NVME_LIDS_LSUPP);
+	logs->lids[NVME_LOG_CCR] = cpu_to_le32(NVME_LIDS_LSUPP);
 
 	status = nvmet_copy_to_sgl(req, 0, logs, sizeof(*logs));
 	kfree(logs);
@@ -617,6 +618,46 @@ static void nvmet_execute_get_log_page_features(struct nvmet_req *req)
 	nvmet_req_complete(req, status);
 }
 
+static void nvmet_execute_get_log_page_ccr(struct nvmet_req *req)
+{
+	struct nvmet_ctrl *ctrl = req->sq->ctrl;
+	struct nvmet_ccr *ccr;
+	struct nvme_ccr_log *log;
+	int index = 0;
+	u16 status;
+
+	log = kzalloc_obj(*log);
+	if (!log) {
+		status = NVME_SC_INTERNAL;
+		goto out;
+	}
+
+	mutex_lock(&ctrl->lock);
+	list_for_each_entry(ccr, &ctrl->ccr_list, entry) {
+		u8 flags = NVME_CCR_FLAGS_VALIDATED | NVME_CCR_FLAGS_INITIATED;
+		u8 ccr_status = ccr->ctrl ? NVME_CCR_STATUS_IN_PROGRESS :
+					NVME_CCR_STATUS_SUCCESS;
+
+		log->entries[index].icid = cpu_to_le16(ccr->icid);
+		log->entries[index].ciu = ccr->ciu;
+		log->entries[index].acid = cpu_to_le16(0xffff);
+		log->entries[index].ccrs = ccr_status;
+		log->entries[index].ccrf = flags;
+		index++;
+	}
+
+	/* Cleanup completed CCRs if requested */
+	if (req->cmd->get_log_page.lsp & 0x1)
+		nvmet_ctrl_cleanup_ccrs(ctrl, false);
+	mutex_unlock(&ctrl->lock);
+
+	log->ne = cpu_to_le16(index);
+	status = nvmet_copy_to_sgl(req, 0, log, sizeof(*log));
+	kfree(log);
+out:
+	nvmet_req_complete(req, status);
+}
+
 static void nvmet_execute_get_log_page(struct nvmet_req *req)
 {
 	if (!nvmet_check_transfer_len(req, nvmet_get_log_page_len(req->cmd)))
@@ -650,6 +691,8 @@ static void nvmet_execute_get_log_page(struct nvmet_req *req)
 		return nvmet_execute_get_log_page_rmi(req);
 	case NVME_LOG_RESERVATION:
 		return nvmet_execute_get_log_page_resv(req);
+	case NVME_LOG_CCR:
+		return nvmet_execute_get_log_page_ccr(req);
 	}
 	pr_debug("unhandled lid %d on qid %d\n",
 	       req->cmd->get_log_page.lid, req->sq->qid);
diff --git a/include/linux/nvme.h b/include/linux/nvme.h
index d31c9648af43..98961f80b173 100644
--- a/include/linux/nvme.h
+++ b/include/linux/nvme.h
@@ -1440,6 +1440,7 @@ enum {
 	NVME_LOG_ANA		= 0x0c,
 	NVME_LOG_FEATURES	= 0x12,
 	NVME_LOG_RMI		= 0x16,
+	NVME_LOG_CCR		= 0x1e,
 	NVME_LOG_FDP_CONFIGS	= 0x20,
 	NVME_LOG_DISC		= 0x70,
 	NVME_LOG_RESERVATION	= 0x80,
@@ -1466,6 +1467,34 @@ enum {
 	NVME_FIS_CSCPE	= 1 << 21,
 };
 
+/* NVMe Cross-Controller Reset Status */
+enum {
+	NVME_CCR_STATUS_IN_PROGRESS,
+	NVME_CCR_STATUS_SUCCESS,
+	NVME_CCR_STATUS_FAILED,
+};
+
+/* NVMe Cross-Controller Reset Flags */
+enum {
+	NVME_CCR_FLAGS_VALIDATED	= 0x01,
+	NVME_CCR_FLAGS_INITIATED	= 0x02,
+};
+
+struct nvme_ccr_log_entry {
+	__le16			icid;
+	__u8			ciu;
+	__u8			rsvd3;
+	__le16			acid;
+	__u8			ccrs;
+	__u8			ccrf;
+};
+
+struct nvme_ccr_log {
+	__le16				ne;
+	__u8				rsvd2[6];
+	struct nvme_ccr_log_entry	entries[NVMF_CCR_PER_PAGE];
+};
+
 /* NVMe Namespace Write Protect State */
 enum {
 	NVME_NS_NO_WRITE_PROTECT = 0,
-- 
2.56.0

[PATCH v7 05/17] nvmet: Send an AEN on CCR completion

[Mohamed Khalfella]

When an impacted controller that is the target of pending CCRs is torn
down, send an AEN to each source controller that issued a CCR against
it. The notification points to the CCR log page that the source
controller can read to check which CCR operation completed.

Split nvmet_add_async_event() into a locked helper so the AEN can be
queued while ctrl->lock is already held.

Signed-off-by: Mohamed Khalfella <mkhalfella@purestorage.com>
Reviewed-by: Hannes Reinecke <hare@suse.de>
Reviewed-by: Sagi Grimberg <sagi@grimberg.me>
Reviewed-by: Daniel Wagner <dwagner@suse.de>
Reviewed-by: Randy Jennings <randyj@purestorage.com>
---
 drivers/nvme/target/admin-cmd.c |  1 +
 drivers/nvme/target/core.c      | 27 ++++++++++++++++++++++++---
 drivers/nvme/target/nvmet.h     |  3 ++-
 include/linux/nvme.h            |  3 +++
 4 files changed, 30 insertions(+), 4 deletions(-)

diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
index 25c320752d2a..e13ffd17669b 100644
--- a/drivers/nvme/target/admin-cmd.c
+++ b/drivers/nvme/target/admin-cmd.c
@@ -649,6 +649,7 @@ static void nvmet_execute_get_log_page_ccr(struct nvmet_req *req)
 	/* Cleanup completed CCRs if requested */
 	if (req->cmd->get_log_page.lsp & 0x1)
 		nvmet_ctrl_cleanup_ccrs(ctrl, false);
+	nvmet_clear_aen_bit(req, NVME_AEN_BIT_CCR_COMPLETE);
 	mutex_unlock(&ctrl->lock);
 
 	log->ne = cpu_to_le16(index);
diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
index 86018e0fb158..12ab522ea5a1 100644
--- a/drivers/nvme/target/core.c
+++ b/drivers/nvme/target/core.c
@@ -205,11 +205,13 @@ static void nvmet_async_event_work(struct work_struct *work)
 	nvmet_async_events_process(ctrl);
 }
 
-void nvmet_add_async_event(struct nvmet_ctrl *ctrl, u8 event_type,
+static void nvmet_add_async_event_locked(struct nvmet_ctrl *ctrl, u8 event_type,
 		u8 event_info, u8 log_page)
 {
 	struct nvmet_async_event *aen;
 
+	lockdep_assert_held(&ctrl->lock);
+
 	aen = kmalloc_obj(*aen);
 	if (!aen)
 		return;
@@ -218,13 +220,19 @@ void nvmet_add_async_event(struct nvmet_ctrl *ctrl, u8 event_type,
 	aen->event_info = event_info;
 	aen->log_page = log_page;
 
-	mutex_lock(&ctrl->lock);
 	list_add_tail(&aen->entry, &ctrl->async_events);
-	mutex_unlock(&ctrl->lock);
 
 	queue_work(nvmet_aen_wq, &ctrl->async_event_work);
 }
 
+void nvmet_add_async_event(struct nvmet_ctrl *ctrl, u8 event_type,
+		u8 event_info, u8 log_page)
+{
+	mutex_lock(&ctrl->lock);
+	nvmet_add_async_event_locked(ctrl, event_type, event_info, log_page);
+	mutex_unlock(&ctrl->lock);
+}
+
 static void nvmet_add_to_changed_ns_log(struct nvmet_ctrl *ctrl, __le32 nsid)
 {
 	u32 i;
@@ -1843,6 +1851,18 @@ struct nvmet_ctrl *nvmet_alloc_ctrl(struct nvmet_alloc_ctrl_args *args)
 }
 EXPORT_SYMBOL_GPL(nvmet_alloc_ctrl);
 
+static void nvmet_ctrl_notify_ccr(struct nvmet_ctrl *ctrl)
+{
+	lockdep_assert_held(&ctrl->lock);
+
+	if (nvmet_aen_bit_disabled(ctrl, NVME_AEN_BIT_CCR_COMPLETE))
+		return;
+
+	nvmet_add_async_event_locked(ctrl, NVME_AER_NOTICE,
+				     NVME_AER_NOTICE_CCR_COMPLETED,
+				     NVME_LOG_CCR);
+}
+
 static void nvmet_ctrl_complete_pending_ccr(struct nvmet_ctrl *ctrl)
 {
 	struct nvmet_subsys *subsys = ctrl->subsys;
@@ -1866,6 +1886,7 @@ static void nvmet_ctrl_complete_pending_ccr(struct nvmet_ctrl *ctrl)
 		list_for_each_entry(ccr, &sctrl->ccr_list, entry) {
 			if (ccr->ctrl == ctrl) {
 				ccr->ctrl = NULL;
+				nvmet_ctrl_notify_ccr(sctrl);
 				break;
 			}
 		}
diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
index 5b8d87454772..2cdfa1165292 100644
--- a/drivers/nvme/target/nvmet.h
+++ b/drivers/nvme/target/nvmet.h
@@ -45,7 +45,8 @@
  * Supported optional AENs:
  */
 #define NVMET_AEN_CFG_OPTIONAL \
-	(NVME_AEN_CFG_NS_ATTR | NVME_AEN_CFG_ANA_CHANGE)
+	(NVME_AEN_CFG_NS_ATTR | NVME_AEN_CFG_ANA_CHANGE | \
+	 NVME_AEN_CFG_CCR_COMPLETE)
 #define NVMET_DISC_AEN_CFG_OPTIONAL \
 	(NVME_AEN_CFG_DISC_CHANGE)
 
diff --git a/include/linux/nvme.h b/include/linux/nvme.h
index 98961f80b173..96b71aaad630 100644
--- a/include/linux/nvme.h
+++ b/include/linux/nvme.h
@@ -871,12 +871,14 @@ enum {
 	NVME_AER_NOTICE_FW_ACT_STARTING = 0x01,
 	NVME_AER_NOTICE_ANA		= 0x03,
 	NVME_AER_NOTICE_DISC_CHANGED	= 0xf0,
+	NVME_AER_NOTICE_CCR_COMPLETED	= 0xf4,
 };
 
 enum {
 	NVME_AEN_BIT_NS_ATTR		= 8,
 	NVME_AEN_BIT_FW_ACT		= 9,
 	NVME_AEN_BIT_ANA_CHANGE		= 11,
+	NVME_AEN_BIT_CCR_COMPLETE	= 20,
 	NVME_AEN_BIT_DISC_CHANGE	= 31,
 };
 
@@ -884,6 +886,7 @@ enum {
 	NVME_AEN_CFG_NS_ATTR		= 1 << NVME_AEN_BIT_NS_ATTR,
 	NVME_AEN_CFG_FW_ACT		= 1 << NVME_AEN_BIT_FW_ACT,
 	NVME_AEN_CFG_ANA_CHANGE		= 1 << NVME_AEN_BIT_ANA_CHANGE,
+	NVME_AEN_CFG_CCR_COMPLETE	= 1 << NVME_AEN_BIT_CCR_COMPLETE,
 	NVME_AEN_CFG_DISC_CHANGE	= 1 << NVME_AEN_BIT_DISC_CHANGE,
 };
 
-- 
2.56.0

[PATCH v7 06/17] nvme: Rapid Path Failure Recovery read controller identify fields

[Mohamed Khalfella]

TP8028 Rapid Path Failure Recovery added new fields to controller
identify response. Read CIU (Controller Instance Uniquifier),
CIRN (Controller Instance Random Number), and CCRL (Cross-Controller
Reset Limit) from controller identify response. Expose CIU and CIRN
as sysfs attributes.

Together with CNTLID, CIU and CIRN identify the current instance of the
controller. A controller that is reset gets new values. Exporting them
lets a user tell controller instances apart, correlate an entry in the
CCR log page (which reports ICID and CIU) with a specific controller,
and issue a CCR command by hand when debugging. The values are cached
from the last Identify, so they are also available when the controller
can no longer process admin commands.

Signed-off-by: Mohamed Khalfella <mkhalfella@purestorage.com>
Reviewed-by: Hannes Reinecke <hare@suse.de>
Reviewed-by: Sagi Grimberg <sagi@grimberg.me>
---
 Documentation/ABI/stable/sysfs-nvme | 11 +++++++++++
 drivers/nvme/host/core.c            |  4 ++++
 drivers/nvme/host/nvme.h            |  3 +++
 drivers/nvme/host/sysfs.c           | 23 +++++++++++++++++++++++
 4 files changed, 41 insertions(+)

diff --git a/Documentation/ABI/stable/sysfs-nvme b/Documentation/ABI/stable/sysfs-nvme
index 9291595f8102..e3395e5730bb 100644
--- a/Documentation/ABI/stable/sysfs-nvme
+++ b/Documentation/ABI/stable/sysfs-nvme
@@ -9,6 +9,17 @@ Description:
 		of the NVMe controller, as reported in the Identify
 		Controller data structure.
 
+What:		/sys/class/nvme/nvmeX/cirn
+What:		/sys/class/nvme/nvmeX/ciu
+Date:		October 2026
+KernelVersion:	7.5
+Contact:	Mohamed Khalfella <mkhalfella@purestorage.com>
+Description:
+		Shows CIRN (Controller Instance Random Number) and CIU
+		(Controller Instance Uniquifier) of the current instance
+		of the NVMe controller as reported by Identify Controller
+		data structure.
+
 What:		/sys/class/nvme/nvmeX/cntlid
 Date:		February 2016
 KernelVersion:	4.6
diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 03671bba1754..38b27d8db011 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -3693,6 +3693,10 @@ static int nvme_init_identify(struct nvme_ctrl *ctrl)
 	ctrl->crdt[1] = le16_to_cpu(id->crdt2);
 	ctrl->crdt[2] = le16_to_cpu(id->crdt3);
 
+	ctrl->ciu = id->ciu;
+	ctrl->cirn = le64_to_cpu(id->cirn);
+	ctrl->ccrl = id->ccrl;
+
 	ctrl->oacs = le16_to_cpu(id->oacs);
 	ctrl->oncs = le16_to_cpu(id->oncs);
 	ctrl->mtfa = le16_to_cpu(id->mtfa);
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index 82cdc7d32b22..0c87072c5265 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -401,11 +401,14 @@ struct nvme_ctrl {
 	u16 crdt[3];
 	u16 oncs;
 	u8 dmrl;
+	u8 ciu;
 	u32 dmrsl;
+	u64 cirn;
 	u16 oacs;
 	u16 sqsize;
 	u32 max_namespaces;
 	atomic_t abort_limit;
+	u8 ccrl;
 	u8 vwc;
 	u32 vs;
 	u32 sgls;
diff --git a/drivers/nvme/host/sysfs.c b/drivers/nvme/host/sysfs.c
index 10018c6356b6..5b549a0513c6 100644
--- a/drivers/nvme/host/sysfs.c
+++ b/drivers/nvme/host/sysfs.c
@@ -491,6 +491,27 @@ nvme_show_int_function(queue_count);
 nvme_show_int_function(sqsize);
 nvme_show_int_function(kato);
 
+static ssize_t nvme_sysfs_ciu_show(struct device *dev,
+					  struct device_attribute *attr,
+					  char *buf)
+{
+	struct nvme_ctrl *ctrl = dev_get_drvdata(dev);
+
+	return sysfs_emit(buf, "%02x\n", ctrl->ciu);
+}
+static DEVICE_ATTR(ciu, S_IRUSR, nvme_sysfs_ciu_show, NULL);
+
+static ssize_t nvme_sysfs_cirn_show(struct device *dev,
+					  struct device_attribute *attr,
+					  char *buf)
+{
+	struct nvme_ctrl *ctrl = dev_get_drvdata(dev);
+
+	return sysfs_emit(buf, "%016llx\n", ctrl->cirn);
+}
+static DEVICE_ATTR(cirn, S_IRUSR, nvme_sysfs_cirn_show, NULL);
+
+
 static ssize_t nvme_sysfs_delete(struct device *dev,
 				struct device_attribute *attr, const char *buf,
 				size_t count)
@@ -943,6 +964,8 @@ static struct attribute *nvme_dev_attrs[] = {
 	&dev_attr_numa_node.attr,
 	&dev_attr_queue_count.attr,
 	&dev_attr_sqsize.attr,
+	&dev_attr_ciu.attr,
+	&dev_attr_cirn.attr,
 	&dev_attr_hostnqn.attr,
 	&dev_attr_hostid.attr,
 	&dev_attr_ctrl_loss_tmo.attr,
-- 
2.56.0

[PATCH v7 07/17] nvme: Introduce FENCING and FENCED controller states

[Mohamed Khalfella]

Introduce two new controller states, FENCING and FENCED, and the state
machine transitions needed to support them. Transports will move a LIVE
controller into these states when an error is encountered; this patch
only adds the states themselves.

FENCING is entered from LIVE. While in FENCING the queues remain alive
but new requests are not allowed to be sent, and the controller cannot
be reset, since a reset cancels inflight IOs that must be held until
either CCR succeeds or time-based recovery completes. A new request
issued to a controller in FENCING or FENCED is failed with a host path
error, so a multipath request is retried on another path.

FENCED is a short-lived state entered from FENCING once fencing is
over. It is the only state from which RESETTING is reachable, so a
manual reset cannot take effect while the controller is in FENCING.

Deletion is allowed from both states. Callers of nvme_delete_ctrl()
assume deleting a live controller cannot fail, and they cannot wait for
the fence either. RDMA device removal and the TCP/RDMA module exit
paths hold the transport's controller list mutex, which ->free_ctrl
needs when the fence drops the last reference on another controller.
Instead the transport's ->stop_ctrl flushes the fencing works, so the
delete path waits the fence out without holding any lock.

A controller deleted while fencing does not keep the fence's hold on
its inflight requests. Once the state leaves FENCING they complete as
in any other delete and may be retried on another path before the
fence would have ended. This is a known limitation, to be addressed
separately.

Update nvme_available_path() to treat a controller in FENCING/FENCED
as an available path, both states are non-terminal, add the state
names to the sysfs state attribute and document them in
Documentation/ABI/stable/sysfs-nvme.

Signed-off-by: Mohamed Khalfella <mkhalfella@purestorage.com>
---
 Documentation/ABI/stable/sysfs-nvme |  5 +++--
 drivers/nvme/host/core.c            | 29 +++++++++++++++++++++++++++--
 drivers/nvme/host/multipath.c       |  2 ++
 drivers/nvme/host/nvme.h            |  4 ++++
 drivers/nvme/host/sysfs.c           |  2 ++
 5 files changed, 38 insertions(+), 4 deletions(-)

diff --git a/Documentation/ABI/stable/sysfs-nvme b/Documentation/ABI/stable/sysfs-nvme
index e3395e5730bb..df883d644fb4 100644
--- a/Documentation/ABI/stable/sysfs-nvme
+++ b/Documentation/ABI/stable/sysfs-nvme
@@ -156,8 +156,9 @@ KernelVersion:	4.11
 Contact:	Sagi Grimberg <sagi@grimberg.me>
 Description:
 		Shows the current state of the controller. Possible values:
-		"new", "live", "resetting", "connecting", "deleting",
-		"deleting (no IO)", "dead".
+		"new", "live", "fencing", "fenced", "resetting", "connecting",
+		"deleting", "deleting (no IO)", "dead". "fencing" and "fenced"
+		were added in 7.5.
 
 What:		/sys/class/nvme/nvmeX/numa_node
 Date:		November 2018
diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 38b27d8db011..4be3ce2e72a7 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -596,10 +596,29 @@ bool nvme_change_ctrl_state(struct nvme_ctrl *ctrl,
 			break;
 		}
 		break;
+	case NVME_CTRL_FENCING:
+		switch (old_state) {
+		case NVME_CTRL_LIVE:
+			changed = true;
+			fallthrough;
+		default:
+			break;
+		}
+		break;
+	case NVME_CTRL_FENCED:
+		switch (old_state) {
+		case NVME_CTRL_FENCING:
+			changed = true;
+			fallthrough;
+		default:
+			break;
+		}
+		break;
 	case NVME_CTRL_RESETTING:
 		switch (old_state) {
 		case NVME_CTRL_NEW:
 		case NVME_CTRL_LIVE:
+		case NVME_CTRL_FENCED:
 			changed = true;
 			atomic_long_inc(&ctrl->nr_reset);
 			fallthrough;
@@ -622,6 +641,8 @@ bool nvme_change_ctrl_state(struct nvme_ctrl *ctrl,
 		case NVME_CTRL_LIVE:
 		case NVME_CTRL_RESETTING:
 		case NVME_CTRL_CONNECTING:
+		case NVME_CTRL_FENCING:
+		case NVME_CTRL_FENCED:
 			changed = true;
 			fallthrough;
 		default:
@@ -786,6 +807,8 @@ blk_status_t nvme_fail_nonready_command(struct nvme_ctrl *ctrl,
 
 	if (state != NVME_CTRL_DELETING_NOIO &&
 	    state != NVME_CTRL_DELETING &&
+	    state != NVME_CTRL_FENCING &&
+	    state != NVME_CTRL_FENCED &&
 	    state != NVME_CTRL_DEAD &&
 	    !test_bit(NVME_CTRL_FAILFAST_EXPIRED, &ctrl->flags) &&
 	    !blk_noretry_request(rq) && !(rq->cmd_flags & REQ_NVME_MPATH))
@@ -828,10 +851,12 @@ bool __nvme_check_ready(struct nvme_ctrl *ctrl, struct request *rq,
 			     req->cmd->fabrics.fctype == nvme_fabrics_type_auth_receive))
 				return true;
 			break;
-		default:
-			break;
+		case NVME_CTRL_FENCING:
+		case NVME_CTRL_FENCED:
 		case NVME_CTRL_DEAD:
 			return false;
+		default:
+			break;
 		}
 	}
 
diff --git a/drivers/nvme/host/multipath.c b/drivers/nvme/host/multipath.c
index ae8059366b57..a987e2413acb 100644
--- a/drivers/nvme/host/multipath.c
+++ b/drivers/nvme/host/multipath.c
@@ -538,6 +538,8 @@ static bool nvme_available_path(struct nvme_ns_head *head)
 				continue;
 			return true;
 		case NVME_CTRL_RESETTING:
+		case NVME_CTRL_FENCING:
+		case NVME_CTRL_FENCED:
 			return true;
 		case NVME_CTRL_CONNECTING:
 			if (!fail_on_ctrl_loss)
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index 0c87072c5265..cb9fca419935 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -320,6 +320,8 @@ static inline u16 nvme_req_qid(struct request *req)
 enum nvme_ctrl_state {
 	NVME_CTRL_NEW,
 	NVME_CTRL_LIVE,
+	NVME_CTRL_FENCING,
+	NVME_CTRL_FENCED,
 	NVME_CTRL_RESETTING,
 	NVME_CTRL_CONNECTING,
 	NVME_CTRL_DELETING,
@@ -875,6 +877,8 @@ static inline bool nvme_state_terminal(struct nvme_ctrl *ctrl)
 	switch (nvme_ctrl_state(ctrl)) {
 	case NVME_CTRL_NEW:
 	case NVME_CTRL_LIVE:
+	case NVME_CTRL_FENCING:
+	case NVME_CTRL_FENCED:
 	case NVME_CTRL_RESETTING:
 	case NVME_CTRL_CONNECTING:
 		return false;
diff --git a/drivers/nvme/host/sysfs.c b/drivers/nvme/host/sysfs.c
index 5b549a0513c6..ec4ca1669253 100644
--- a/drivers/nvme/host/sysfs.c
+++ b/drivers/nvme/host/sysfs.c
@@ -546,6 +546,8 @@ static ssize_t nvme_sysfs_show_state(struct device *dev,
 	static const char *const state_name[] = {
 		[NVME_CTRL_NEW]		= "new",
 		[NVME_CTRL_LIVE]	= "live",
+		[NVME_CTRL_FENCING]	= "fencing",
+		[NVME_CTRL_FENCED]	= "fenced",
 		[NVME_CTRL_RESETTING]	= "resetting",
 		[NVME_CTRL_CONNECTING]	= "connecting",
 		[NVME_CTRL_DELETING]	= "deleting",
-- 
2.56.0

[PATCH v7 08/17] nvme: Implement cross-controller reset recovery

[Mohamed Khalfella]

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.

nvme_fence_ctrl() picks a LIVE controller in the same subsystem that
has CCR capacity left, sends CCR for the impacted controller and waits
for the completion AEN. If CCR fails, it tries the next controller
until the fence timeout expires. It returns 0 on success and the time
left otherwise, which the caller can spend on time-based recovery.

Add nvme_fence_timeout_ms(), which bounds the time spent fencing a
controller. It is derived from the keep-alive timeout: 2 * KATO, or
3 * KATO if traffic based keep-alive is enabled.

Signed-off-by: Mohamed Khalfella <mkhalfella@purestorage.com>
Reviewed-by: Sagi Grimberg <sagi@grimberg.me>
Reviewed-by: Hannes Reinecke <hare@kernel.org>
---
 drivers/nvme/host/constants.c |   1 +
 drivers/nvme/host/core.c      | 152 ++++++++++++++++++++++++++++++++++
 drivers/nvme/host/nvme.h      |  17 ++++
 3 files changed, 170 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 4be3ce2e72a7..27c33dc49175 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -576,6 +576,157 @@ void nvme_cancel_admin_tagset(struct nvme_ctrl *ctrl)
 }
 EXPORT_SYMBOL_GPL(nvme_cancel_admin_tagset);
 
+static struct nvme_ctrl *nvme_find_ctrl_ccr(struct nvme_ctrl *ictrl,
+					    u32 min_cntlid)
+{
+	struct nvme_subsystem *subsys = ictrl->subsys;
+	struct nvme_ctrl *ctrl, *sctrl = NULL;
+	unsigned long flags;
+	int ccr_used;
+
+	mutex_lock(&nvme_subsystems_lock);
+	list_for_each_entry(ctrl, &subsys->ctrls, subsys_entry) {
+		if (ctrl->cntlid < min_cntlid)
+			continue;
+
+		spin_lock_irqsave(&ctrl->lock, flags);
+		if (nvme_ctrl_state(ctrl) != NVME_CTRL_LIVE) {
+			spin_unlock_irqrestore(&ctrl->lock, flags);
+			continue;
+		}
+
+		ccr_used = atomic_inc_return(&ctrl->ccr_used);
+		if (ccr_used > ctrl->ccrl) {
+			atomic_dec(&ctrl->ccr_used);
+			spin_unlock_irqrestore(&ctrl->lock, flags);
+			continue;
+		}
+
+		/*
+		 * We got a good candidate source controller that is locked and
+		 * LIVE. However, no guarantee ctrl will not be deleted after
+		 * ctrl->lock is released. Get a ref of both ctrl and admin_q
+		 * so they do not disappear until we are done with them.
+		 */
+		WARN_ON_ONCE(!blk_get_queue(ctrl->admin_q));
+		nvme_get_ctrl(ctrl);
+		spin_unlock_irqrestore(&ctrl->lock, flags);
+		sctrl = ctrl;
+		break;
+	}
+	mutex_unlock(&nvme_subsystems_lock);
+	return sctrl;
+}
+
+static void nvme_put_ctrl_ccr(struct nvme_ctrl *sctrl)
+{
+	atomic_dec(&sctrl->ccr_used);
+	blk_put_queue(sctrl->admin_q);
+	nvme_put_ctrl(sctrl);
+}
+
+static int nvme_issue_wait_ccr(struct nvme_ctrl *sctrl, struct nvme_ctrl *ictrl,
+			       unsigned long deadline)
+{
+	struct nvme_ccr_entry ccr = { };
+	union nvme_result res = { 0 };
+	struct nvme_command c = { };
+	unsigned long flags, now, tmo = 0;
+	bool completed = false;
+	int ret = 0;
+	u32 result;
+
+	init_completion(&ccr.complete);
+	ccr.ictrl = ictrl;
+
+	spin_lock_irqsave(&sctrl->lock, flags);
+	list_add_tail(&ccr.list, &sctrl->ccr_list);
+	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) {
+		ret = -EIO;
+		goto out;
+	}
+
+	result = le32_to_cpu(res.u32);
+	if (result & 0x01) /* Immediate Reset Successful */
+		goto out;
+
+	now = jiffies;
+	if (time_before(now, deadline))
+		tmo = min_t(unsigned long,
+			    secs_to_jiffies(ictrl->kato), deadline - now);
+
+	if (!wait_for_completion_timeout(&ccr.complete, tmo)) {
+		ret = -ETIMEDOUT;
+		goto out;
+	}
+
+	completed = true;
+
+out:
+	spin_lock_irqsave(&sctrl->lock, flags);
+	list_del(&ccr.list);
+	spin_unlock_irqrestore(&sctrl->lock, flags);
+	if (completed) {
+		if (ccr.ccrs == NVME_CCR_STATUS_SUCCESS)
+			return 0;
+		return -EREMOTEIO;
+	}
+	return ret;
+}
+
+unsigned long nvme_fence_ctrl(struct nvme_ctrl *ictrl)
+{
+	unsigned long now, deadline, timeout;
+	struct nvme_ctrl *sctrl;
+	u32 min_cntlid = 0;
+	int ret;
+
+	timeout = nvme_fence_timeout_ms(ictrl);
+	dev_info(ictrl->device, "attempting CCR, timeout %lums\n", timeout);
+
+	now = jiffies;
+	deadline = jiffies + msecs_to_jiffies(timeout);
+	while (time_before(now, deadline)) {
+		sctrl = nvme_find_ctrl_ccr(ictrl, min_cntlid);
+		if (!sctrl) {
+			dev_dbg(ictrl->device,
+				"failed to find source controller\n");
+			return deadline - now;
+		}
+
+		ret = nvme_issue_wait_ccr(sctrl, ictrl, deadline);
+		if (!ret) {
+			dev_info(ictrl->device, "CCR succeeded using %s\n",
+				 dev_name(sctrl->device));
+			nvme_put_ctrl_ccr(sctrl);
+			return 0;
+		}
+
+		/*
+		 * CCR command or CCR operation failed on this path.
+		 * Try another path as long as we have time.
+		 */
+		dev_err(ictrl->device, "CCR failed using %s, ret = %d\n",
+			dev_name(sctrl->device), ret);
+		min_cntlid = sctrl->cntlid + 1;
+		nvme_put_ctrl_ccr(sctrl);
+		now = jiffies;
+	}
+
+	/* Fencing timed out call it done */
+	dev_info(ictrl->device, "fencing timeout\n");
+	return 0;
+}
+EXPORT_SYMBOL_GPL(nvme_fence_ctrl);
+
 bool nvme_change_ctrl_state(struct nvme_ctrl *ctrl,
 		enum nvme_ctrl_state new_state)
 {
@@ -5277,6 +5428,7 @@ int nvme_init_ctrl(struct nvme_ctrl *ctrl, struct device *dev,
 
 	mutex_init(&ctrl->scan_lock);
 	INIT_LIST_HEAD(&ctrl->namespaces);
+	INIT_LIST_HEAD(&ctrl->ccr_list);
 	ctrl->dev = dev;
 	ctrl->ops = ops;
 	ctrl->quirks = quirks;
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index cb9fca419935..261955538efe 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -349,6 +349,13 @@ enum nvme_ctrl_flags {
 	NVME_CTRL_FROZEN		= 6,
 };
 
+struct nvme_ccr_entry {
+	struct list_head list;
+	struct completion complete;
+	struct nvme_ctrl *ictrl;
+	u8 ccrs;
+};
+
 struct nvme_ctrl {
 	bool comp_seen;
 	bool identified;
@@ -366,6 +373,7 @@ struct nvme_ctrl {
 	struct blk_mq_tag_set *tagset;
 	struct blk_mq_tag_set *admin_tagset;
 	struct list_head namespaces;
+	struct list_head ccr_list;
 	struct mutex namespaces_lock;
 	struct srcu_struct srcu;
 	struct device ctrl_device;
@@ -410,6 +418,7 @@ struct nvme_ctrl {
 	u16 sqsize;
 	u32 max_namespaces;
 	atomic_t abort_limit;
+	atomic_t ccr_used;
 	u8 ccrl;
 	u8 vwc;
 	u32 vs;
@@ -912,6 +921,7 @@ blk_status_t nvme_host_path_error(struct request *req);
 bool nvme_cancel_request(struct request *req, void *data);
 void nvme_cancel_tagset(struct nvme_ctrl *ctrl);
 void nvme_cancel_admin_tagset(struct nvme_ctrl *ctrl);
+unsigned long nvme_fence_ctrl(struct nvme_ctrl *ctrl);
 bool nvme_change_ctrl_state(struct nvme_ctrl *ctrl,
 		enum nvme_ctrl_state new_state);
 int nvme_disable_ctrl(struct nvme_ctrl *ctrl, bool shutdown);
@@ -1359,4 +1369,11 @@ static inline bool nvme_multi_css(struct nvme_ctrl *ctrl)
 	return (ctrl->ctrl_config & NVME_CC_CSS_MASK) == NVME_CC_CSS_CSI;
 }
 
+static inline unsigned long nvme_fence_timeout_ms(struct nvme_ctrl *ctrl)
+{
+	if (ctrl->ctratt & NVME_CTRL_ATTR_TBKAS)
+		return 3 * ctrl->kato * 1000;
+	return 2 * ctrl->kato * 1000;
+}
+
 #endif /* _NVME_H */
-- 
2.56.0

[PATCH v7 09/17] nvme: Implement cross-controller reset completion

[Mohamed Khalfella]

An nvme source controller that issues CCR command expects to receive an
NVME_AER_NOTICE_CCR_COMPLETED when pending CCR succeeds or fails. Add
ctrl->ccr_work to read NVME_LOG_CCR logpage and wake up threads waiting
on CCR completion.

Signed-off-by: Mohamed Khalfella <mkhalfella@purestorage.com>
Reviewed-by: Sagi Grimberg <sagi@grimberg.me>
---
 drivers/nvme/host/core.c | 51 +++++++++++++++++++++++++++++++++++++++-
 drivers/nvme/host/nvme.h |  1 +
 2 files changed, 51 insertions(+), 1 deletion(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 27c33dc49175..54b83092a9e8 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -1961,7 +1961,8 @@ EXPORT_SYMBOL_GPL(nvme_set_queue_count);
 
 #define NVME_AEN_SUPPORTED \
 	(NVME_AEN_CFG_NS_ATTR | NVME_AEN_CFG_FW_ACT | \
-	 NVME_AEN_CFG_ANA_CHANGE | NVME_AEN_CFG_DISC_CHANGE)
+	 NVME_AEN_CFG_ANA_CHANGE | NVME_AEN_CFG_CCR_COMPLETE | \
+	 NVME_AEN_CFG_DISC_CHANGE)
 
 static void nvme_enable_aen(struct nvme_ctrl *ctrl)
 {
@@ -5060,6 +5061,48 @@ static void nvme_get_fw_slot_info(struct nvme_ctrl *ctrl)
 	kfree(log);
 }
 
+static void nvme_ccr_work(struct work_struct *work)
+{
+	struct nvme_ctrl *ctrl = container_of(work, struct nvme_ctrl, ccr_work);
+	struct nvme_ccr_entry *ccr;
+	struct nvme_ccr_log_entry *entry;
+	struct nvme_ccr_log *log;
+	int num_entries, ret, i;
+	unsigned long flags;
+
+	log = kmalloc_obj(*log);
+	if (!log)
+		return;
+
+	ret = nvme_get_log(ctrl, 0, NVME_LOG_CCR, 0x01,
+			   0x00, log, sizeof(*log), 0);
+	if (ret)
+		goto out;
+
+	spin_lock_irqsave(&ctrl->lock, flags);
+	num_entries = min(le16_to_cpu(log->ne), NVMF_CCR_PER_PAGE);
+	for (i = 0; i < num_entries; i++) {
+		entry = &log->entries[i];
+		if (entry->ccrs == NVME_CCR_STATUS_IN_PROGRESS)
+			continue;
+
+		list_for_each_entry(ccr, &ctrl->ccr_list, list) {
+			struct nvme_ctrl *ictrl = ccr->ictrl;
+
+			if (ictrl->cntlid != le16_to_cpu(entry->icid) ||
+			    ictrl->ciu != entry->ciu)
+				continue;
+
+			/* Complete matching entry */
+			ccr->ccrs = entry->ccrs;
+			complete(&ccr->complete);
+		}
+	}
+	spin_unlock_irqrestore(&ctrl->lock, flags);
+out:
+	kfree(log);
+}
+
 static void nvme_fw_act_work(struct work_struct *work)
 {
 	struct nvme_ctrl *ctrl = container_of(work,
@@ -5136,6 +5179,9 @@ static bool nvme_handle_aen_notice(struct nvme_ctrl *ctrl, u32 result)
 	case NVME_AER_NOTICE_DISC_CHANGED:
 		ctrl->aen_result = result;
 		break;
+	case NVME_AER_NOTICE_CCR_COMPLETED:
+		queue_work(nvme_wq, &ctrl->ccr_work);
+		break;
 	default:
 		dev_warn(ctrl->device, "async event result %08x\n", result);
 	}
@@ -5324,6 +5370,7 @@ void nvme_stop_ctrl(struct nvme_ctrl *ctrl)
 	nvme_stop_failfast_work(ctrl);
 	flush_work(&ctrl->async_event_work);
 	cancel_work_sync(&ctrl->fw_act_work);
+	cancel_work_sync(&ctrl->ccr_work);
 	if (ctrl->ops->stop_ctrl)
 		ctrl->ops->stop_ctrl(ctrl);
 }
@@ -5362,6 +5409,7 @@ EXPORT_SYMBOL_GPL(nvme_start_ctrl);
 
 void nvme_uninit_ctrl(struct nvme_ctrl *ctrl)
 {
+	cancel_work_sync(&ctrl->ccr_work);
 	nvme_stop_keep_alive(ctrl);
 	nvme_hwmon_exit(ctrl);
 	nvme_fault_inject_fini(&ctrl->fault_inject);
@@ -5434,6 +5482,7 @@ int nvme_init_ctrl(struct nvme_ctrl *ctrl, struct device *dev,
 	ctrl->quirks = quirks;
 	ctrl->numa_node = NUMA_NO_NODE;
 	INIT_WORK(&ctrl->scan_work, nvme_scan_work);
+	INIT_WORK(&ctrl->ccr_work, nvme_ccr_work);
 	INIT_WORK(&ctrl->async_event_work, nvme_async_event_work);
 	INIT_WORK(&ctrl->fw_act_work, nvme_fw_act_work);
 	INIT_WORK(&ctrl->delete_work, nvme_delete_ctrl_work);
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index 261955538efe..884541644240 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -438,6 +438,7 @@ struct nvme_ctrl {
 	struct nvme_id_power_state psd[32];
 	struct nvme_effects_log *effects;
 	struct work_struct scan_work;
+	struct work_struct ccr_work;
 	struct work_struct async_event_work;
 	struct delayed_work ka_work;
 	struct delayed_work failfast_work;
-- 
2.56.0

[PATCH v7 10/17] nvme-tcp: Use CCR to recover controller that hits an error

[Mohamed Khalfella]

A LIVE controller that hits an error now moves to FENCING instead of
RESETTING. ctrl->fencing_work attempts CCR to terminate inflight IOs.
Regardless of the success or failure of the CCR operation, the
controller then moves to RESETTING to continue error recovery.

A request that times out while the controller is FENCING is not
completed. Its timer is rearmed, since inflight requests must not be
retried before the fence is over. nvme_tcp_stop_ctrl() flushes
fencing_work, so deleting a controller that is fencing waits for the
fence to finish.

Signed-off-by: Mohamed Khalfella <mkhalfella@purestorage.com>
Reviewed-by: Hannes Reinecke <hare@kernel.org>
---
 drivers/nvme/host/tcp.c | 39 ++++++++++++++++++++++++++++++++++-----
 1 file changed, 34 insertions(+), 5 deletions(-)

diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
index 7ecc6c007f8d..244c6c50cf99 100644
--- a/drivers/nvme/host/tcp.c
+++ b/drivers/nvme/host/tcp.c
@@ -195,6 +195,7 @@ struct nvme_tcp_ctrl {
 	struct sockaddr_storage src_addr;
 	struct nvme_ctrl	ctrl;
 
+	struct work_struct	fencing_work;
 	struct work_struct	err_work;
 	struct delayed_work	connect_work;
 	struct nvme_tcp_request async_req;
@@ -651,6 +652,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;
+	}
+
 	if (!nvme_change_ctrl_state(ctrl, NVME_CTRL_RESETTING))
 		return;
 
@@ -2767,6 +2774,22 @@ static void nvme_tcp_reconnect_ctrl_work(struct work_struct *work)
 	nvme_tcp_reconnect_or_remove(ctrl, ret);
 }
 
+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)
+		dev_info(ctrl->device, "CCR failed, starting error recovery\n");
+
+	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_error_recovery_work(struct work_struct *work)
 {
 	struct nvme_tcp_ctrl *tcp_ctrl = container_of(work,
@@ -2842,6 +2865,7 @@ static void nvme_reset_ctrl_work(struct work_struct *work)
 
 static void nvme_tcp_stop_ctrl(struct nvme_ctrl *ctrl)
 {
+	flush_work(&to_tcp_ctrl(ctrl)->fencing_work);
 	flush_work(&to_tcp_ctrl(ctrl)->err_work);
 	cancel_delayed_work_sync(&to_tcp_ctrl(ctrl)->connect_work);
 }
@@ -2944,17 +2968,19 @@ 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) {
 		/*
-		 * If we are resetting, connecting or deleting we should
-		 * complete immediately because we may block controller
-		 * teardown or setup sequence
+		 * If fencing has finished or we are resetting, connecting,
+		 * or deleting we should complete immediately because we may
+		 * block controller teardown or setup sequence
 		 * - ctrl disable/shutdown fabrics requests
 		 * - connect requests
 		 * - initialization admin requests
@@ -2970,7 +2996,9 @@ static enum blk_eh_timer_return nvme_tcp_timeout(struct request *rq)
 
 	/*
 	 * LIVE state should trigger the normal error recovery which will
-	 * handle completing this request.
+	 * handle completing this request. In FENCING state the request must
+	 * not be completed before fencing has finished, so just rearm the
+	 * timer and let the error recovery pick it up.
 	 */
 	nvme_tcp_error_recovery(ctrl);
 	return BLK_EH_RESET_TIMER;
@@ -3209,6 +3237,7 @@ static struct nvme_tcp_ctrl *nvme_tcp_alloc_ctrl(struct device *dev,
 
 	INIT_DELAYED_WORK(&ctrl->connect_work,
 			nvme_tcp_reconnect_ctrl_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);
 
-- 
2.56.0

[PATCH v7 11/17] nvme-rdma: Use CCR to recover controller that hits an error

[Mohamed Khalfella]

A LIVE controller that hits an error now moves to FENCING instead of
RESETTING. ctrl->fencing_work attempts CCR to terminate inflight IOs.
Regardless of the success or failure of the CCR operation, the
controller then moves to RESETTING to continue error recovery.

A request that times out while the controller is FENCING is not
completed. Its timer is rearmed, since inflight requests must not be
retried before the fence is over. nvme_rdma_stop_ctrl() flushes
fencing_work, so deleting a controller that is fencing waits for the
fence to finish.

Signed-off-by: Mohamed Khalfella <mkhalfella@purestorage.com>
---
 drivers/nvme/host/rdma.c | 39 ++++++++++++++++++++++++++++++++++-----
 1 file changed, 34 insertions(+), 5 deletions(-)

diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
index 32189fdfc0f5..da1ec2dd6205 100644
--- a/drivers/nvme/host/rdma.c
+++ b/drivers/nvme/host/rdma.c
@@ -119,6 +119,7 @@ struct nvme_rdma_ctrl {
 
 	/* other member variables */
 	struct blk_mq_tag_set	tag_set;
+	struct work_struct	fencing_work;
 	struct work_struct	err_work;
 
 	struct nvme_rdma_qe	async_event_sqe;
@@ -992,6 +993,7 @@ static void nvme_rdma_stop_ctrl(struct nvme_ctrl *nctrl)
 {
 	struct nvme_rdma_ctrl *ctrl = to_rdma_ctrl(nctrl);
 
+	flush_work(&ctrl->fencing_work);
 	flush_work(&ctrl->err_work);
 	cancel_delayed_work_sync(&ctrl->reconnect_work);
 }
@@ -1153,6 +1155,22 @@ static void nvme_rdma_reconnect_ctrl_work(struct work_struct *work)
 	nvme_rdma_reconnect_or_remove(ctrl, ret);
 }
 
+static void nvme_rdma_fencing_work(struct work_struct *work)
+{
+	struct nvme_rdma_ctrl *rdma_ctrl = container_of(work,
+			struct nvme_rdma_ctrl, fencing_work);
+	struct nvme_ctrl *ctrl = &rdma_ctrl->ctrl;
+	unsigned long rem;
+
+	rem = nvme_fence_ctrl(ctrl);
+	if (rem)
+		dev_info(ctrl->device, "CCR failed, starting error recovery\n");
+
+	nvme_change_ctrl_state(ctrl, NVME_CTRL_FENCED);
+	if (nvme_change_ctrl_state(ctrl, NVME_CTRL_RESETTING))
+		queue_work(nvme_reset_wq, &rdma_ctrl->err_work);
+}
+
 static void nvme_rdma_error_recovery_work(struct work_struct *work)
 {
 	struct nvme_rdma_ctrl *ctrl = container_of(work,
@@ -1180,6 +1198,12 @@ static void nvme_rdma_error_recovery_work(struct work_struct *work)
 
 static void nvme_rdma_error_recovery(struct nvme_rdma_ctrl *ctrl)
 {
+	if (nvme_change_ctrl_state(&ctrl->ctrl, NVME_CTRL_FENCING)) {
+		dev_warn(ctrl->ctrl.device, "starting controller fencing\n");
+		queue_work(nvme_wq, &ctrl->fencing_work);
+		return;
+	}
+
 	if (!nvme_change_ctrl_state(&ctrl->ctrl, NVME_CTRL_RESETTING))
 		return;
 
@@ -1991,17 +2015,19 @@ static enum blk_eh_timer_return nvme_rdma_timeout(struct request *rq)
 	struct nvme_rdma_ctrl *ctrl = queue->ctrl;
 	struct nvme_command *cmd = req->req.cmd;
 	int qid = nvme_rdma_queue_idx(queue);
+	enum nvme_ctrl_state state;
 
 	dev_warn(ctrl->ctrl.device,
 		 "I/O tag %d (%04x) opcode %#x (%s) QID %d timeout\n",
 		 rq->tag, nvme_cid(rq), cmd->common.opcode,
 		 nvme_fabrics_opcode_str(qid, cmd), qid);
 
-	if (nvme_ctrl_state(&ctrl->ctrl) != NVME_CTRL_LIVE) {
+	state = nvme_ctrl_state(&ctrl->ctrl);
+	if (state != NVME_CTRL_LIVE && state != NVME_CTRL_FENCING) {
 		/*
-		 * If we are resetting, connecting or deleting we should
-		 * complete immediately because we may block controller
-		 * teardown or setup sequence
+		 * If fencing has finished or we are resetting, connecting,
+		 * or deleting we should complete immediately because we may
+		 * block controller teardown or setup sequence
 		 * - ctrl disable/shutdown fabrics requests
 		 * - connect requests
 		 * - initialization admin requests
@@ -2017,7 +2043,9 @@ static enum blk_eh_timer_return nvme_rdma_timeout(struct request *rq)
 
 	/*
 	 * LIVE state should trigger the normal error recovery which will
-	 * handle completing this request.
+	 * handle completing this request. In FENCING state the request must
+	 * not be completed before fencing has finished, so just rearm the
+	 * timer and let the error recovery pick it up.
 	 */
 	nvme_rdma_error_recovery(ctrl);
 	return BLK_EH_RESET_TIMER;
@@ -2328,6 +2356,7 @@ static struct nvme_rdma_ctrl *nvme_rdma_alloc_ctrl(struct device *dev,
 
 	INIT_DELAYED_WORK(&ctrl->reconnect_work,
 			nvme_rdma_reconnect_ctrl_work);
+	INIT_WORK(&ctrl->fencing_work, nvme_rdma_fencing_work);
 	INIT_WORK(&ctrl->err_work, nvme_rdma_error_recovery_work);
 	INIT_WORK(&ctrl->ctrl.reset_work, nvme_rdma_reset_ctrl_work);
 
-- 
2.56.0

[PATCH v7 12/17] nvme-fc: start error recovery instead of aborting timed out IOs

[Mohamed Khalfella]

Aborts issued from the timeout handler run outside the FCCTRL_TERMIO
window, so they are not counted in ctrl->iocnt and
nvme_fc_delete_association() does not wait for them. The association
can be torn down while the LLDD is still working on the abort.

Instead of aborting the timed out command, reset the controller like
the other fabrics transports do. The association teardown aborts the
outstanding IOs, counts the aborts and waits for them to complete.

Count every abort in ctrl->iocnt, not only the ones issued while
FCCTRL_TERMIO is set, and do not reset the count when the teardown
starts. An abort issued before the teardown is then waited for as well.

Only a LIVE controller starts error recovery. In every other state the
association is being set up, being torn down or already gone, so abort
the timed out command from the timeout handler instead and return
BLK_EH_DONE. The command completes through nvme_fc_fcpio_done() once
the LLDD has finished the abort, and the abort is counted so a teardown
that follows waits for it. This matters for a controller being deleted.
The delete path drains the IOs itself and tears the association down in
nvme_fc_delete_ctrl() once the drain is done, and DELETING cannot move
to RESETTING, so without the abort a timed out IO would hang the drain
against a dead target.

The new nvme_fc_start_ioerr_recovery() is the common entry point for IO
timeouts, transport errors reported by nvme_fc_fcpio_done(), Disconnect
LS and connectivity loss. It moves the controller to RESETTING and
queues ioerr_work only if that succeeds, so nothing is ever queued on a
controller that is CONNECTING or being deleted. A Disconnect LS or a
transport error received while CONNECTING is resolved when the inflight
request fails, or times out and is aborted.

ioerr_work still calls nvme_fc_error_recovery(), which returns without
doing anything for a controller that is not LIVE. With the controller
already in RESETTING when the work runs, no recovery takes place yet
and the controller stays in RESETTING. The next patch moves the
recovery into ioerr_work itself and completes the change.

Link: https://lore.kernel.org/all/20250529214928.2112990-1-mkhalfella@purestorage.com/
Signed-off-by: Mohamed Khalfella <mkhalfella@purestorage.com>
---
 drivers/nvme/host/fc.c | 66 ++++++++++++++++++++++++++++++------------
 1 file changed, 47 insertions(+), 19 deletions(-)

diff --git a/drivers/nvme/host/fc.c b/drivers/nvme/host/fc.c
index 0b9ff50d26ff..c4603be38144 100644
--- a/drivers/nvme/host/fc.c
+++ b/drivers/nvme/host/fc.c
@@ -227,6 +227,8 @@ static DEFINE_IDA(nvme_fc_ctrl_cnt);
 static struct device *fc_udev_device;
 
 static void nvme_fc_complete_rq(struct request *rq);
+static void nvme_fc_start_ioerr_recovery(struct nvme_fc_ctrl *ctrl,
+					 char *errmsg);
 
 /* *********************** FC-NVME Port Management ************************ */
 
@@ -788,7 +790,7 @@ nvme_fc_ctrl_connectivity_loss(struct nvme_fc_ctrl *ctrl)
 		"Reconnect", ctrl->cnum);
 
 	set_bit(ASSOC_FAILED, &ctrl->flags);
-	nvme_reset_ctrl(&ctrl->ctrl);
+	nvme_fc_start_ioerr_recovery(ctrl, "Connectivity Loss");
 }
 
 /**
@@ -1569,7 +1571,8 @@ nvme_fc_ls_disconnect_assoc(struct nvmefc_ls_rcv_op *lsop)
 	 */
 
 	/* fail the association */
-	nvme_fc_error_recovery(ctrl, "Disconnect Association LS received");
+	nvme_fc_start_ioerr_recovery(ctrl,
+				     "Disconnect Association LS received");
 
 	/* release the reference taken by nvme_fc_match_disconn_ls() */
 	nvme_fc_ctrl_put(ctrl);
@@ -1813,11 +1816,18 @@ __nvme_fc_abort_op(struct nvme_fc_ctrl *ctrl, struct nvme_fc_fcp_op *op)
 	unsigned long flags;
 	int opstate;
 
+	/*
+	 * Every abort is counted, not only those issued by the association
+	 * teardown: the teardown waits for iocnt to drop to zero before it
+	 * deletes the hw queues, and an abort issued earlier, by
+	 * nvme_fc_timeout() while deleting or by the CONNECTING recovery
+	 * path, must not still be in progress in the LLDD then.
+	 */
 	spin_lock_irqsave(&ctrl->lock, flags);
 	opstate = atomic_xchg(&op->state, FCPOP_STATE_ABORTED);
-	if (opstate != FCPOP_STATE_ACTIVE)
+	if (opstate != FCPOP_STATE_ACTIVE) {
 		atomic_set(&op->state, opstate);
-	else if (test_bit(FCCTRL_TERMIO, &ctrl->flags)) {
+	} else {
 		op->flags |= FCOP_FLAGS_TERMIO;
 		ctrl->iocnt++;
 	}
@@ -1856,8 +1866,7 @@ __nvme_fc_fcpop_chk_teardowns(struct nvme_fc_ctrl *ctrl,
 
 	if (opstate == FCPOP_STATE_ABORTED) {
 		spin_lock_irqsave(&ctrl->lock, flags);
-		if (test_bit(FCCTRL_TERMIO, &ctrl->flags) &&
-		    op->flags & FCOP_FLAGS_TERMIO) {
+		if (op->flags & FCOP_FLAGS_TERMIO) {
 			if (!--ctrl->iocnt)
 				wake_up(&ctrl->ioabort_wait);
 		}
@@ -1892,6 +1901,19 @@ char *nvme_fc_io_getuuid(struct nvmefc_fcp_req *req)
 }
 EXPORT_SYMBOL_GPL(nvme_fc_io_getuuid);
 
+/*
+ * Start recovery only if this caller can claim it with a state change.
+ */
+static void
+nvme_fc_start_ioerr_recovery(struct nvme_fc_ctrl *ctrl, char *errmsg)
+{
+	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);
+		queue_work(nvme_reset_wq, &ctrl->ioerr_work);
+	}
+}
+
 static void
 nvme_fc_fcpio_done(struct nvmefc_fcp_req *req)
 {
@@ -2049,9 +2071,8 @@ nvme_fc_fcpio_done(struct nvmefc_fcp_req *req)
 		nvme_fc_complete_rq(rq);
 
 check_error:
-	if (terminate_assoc &&
-	    nvme_ctrl_state(&ctrl->ctrl) != NVME_CTRL_RESETTING)
-		queue_work(nvme_reset_wq, &ctrl->ioerr_work);
+	if (terminate_assoc)
+		nvme_fc_start_ioerr_recovery(ctrl, "io error");
 }
 
 static int
@@ -2548,24 +2569,32 @@ static enum blk_eh_timer_return nvme_fc_timeout(struct request *rq)
 	struct nvme_fc_cmd_iu *cmdiu = &op->cmd_iu;
 	struct nvme_command *sqe = &cmdiu->sqe;
 
-	/*
-	 * Attempt to abort the offending command. Command completion
-	 * will detect the aborted io and will fail the connection.
-	 */
 	dev_info(ctrl->ctrl.device,
 		"NVME-FC{%d.%d}: io timeout: opcode %d fctype %d (%s) w10/11: "
 		"x%08x/x%08x\n",
 		ctrl->cnum, qnum, sqe->common.opcode, sqe->fabrics.fctype,
 		nvme_fabrics_opcode_str(qnum, sqe),
 		sqe->common.cdw10, sqe->common.cdw11);
-	if (__nvme_fc_abort_op(ctrl, op))
-		nvme_fc_error_recovery(ctrl, "io timeout abort failed");
+
+	if (nvme_ctrl_state(&ctrl->ctrl) != NVME_CTRL_LIVE) {
+		/*
+		 * The association is being set up, being torn down or is
+		 * already gone, so recovery cannot be started from here.
+		 * Abort the command so the connect or teardown it would
+		 * block can make progress. It completes through
+		 * nvme_fc_fcpio_done() once the LLDD has finished the
+		 * abort, and the abort is counted so a teardown that
+		 * follows waits for it.
+		 */
+		__nvme_fc_abort_op(ctrl, op);
+		return BLK_EH_DONE;
+	}
 
 	/*
-	 * the io abort has been initiated. Have the reset timer
-	 * restarted and the abort completion will complete the io
-	 * shortly. Avoids a synchronous wait while the abort finishes.
+	 * A LIVE controller starts error recovery, which tears the
+	 * association down and completes this request.
 	 */
+	nvme_fc_start_ioerr_recovery(ctrl, "io timeout");
 	return BLK_EH_RESET_TIMER;
 }
 
@@ -3217,7 +3246,6 @@ nvme_fc_delete_association(struct nvme_fc_ctrl *ctrl)
 
 	spin_lock_irqsave(&ctrl->lock, flags);
 	set_bit(FCCTRL_TERMIO, &ctrl->flags);
-	ctrl->iocnt = 0;
 	spin_unlock_irqrestore(&ctrl->lock, flags);
 
 	__nvme_fc_abort_outstanding_ios(ctrl, false);
-- 
2.56.0

[PATCH v7 13/17] nvme-fc: perform error recovery directly from ioerr_work

[Mohamed Khalfella]

Now that nvme_fc_start_ioerr_recovery() moves the controller to
RESETTING before queueing ioerr_work, the nvme_reset_ctrl() call in
nvme_fc_error_recovery() fails with -EBUSY and no recovery runs.

Do the recovery in the work itself instead: stop the keep-alive and
the AEN submission, tear down the association so that any command a
core work item is waiting on is aborted at once, stop the controller,
move to CONNECTING and schedule a reconnect. If the CONNECTING
transition fails the controller is being deleted and the delete path
finishes the job. This is how rdma and tcp structure their error
recovery too.

The work is only ever queued right after a successful transition to
RESETTING, so when it runs it owns the controller: a reset or a
firmware activation holding RESETTING cannot be in progress at the
same time, and the work does not need to re-check the state.

Signed-off-by: Mohamed Khalfella <mkhalfella@purestorage.com>
---
 drivers/nvme/host/fc.c | 57 ++++++++++++++++--------------------------
 1 file changed, 21 insertions(+), 36 deletions(-)

diff --git a/drivers/nvme/host/fc.c b/drivers/nvme/host/fc.c
index c4603be38144..0ba621649afe 100644
--- a/drivers/nvme/host/fc.c
+++ b/drivers/nvme/host/fc.c
@@ -229,6 +229,8 @@ static struct device *fc_udev_device;
 static void nvme_fc_complete_rq(struct request *rq);
 static void nvme_fc_start_ioerr_recovery(struct nvme_fc_ctrl *ctrl,
 					 char *errmsg);
+static void __nvme_fc_abort_outstanding_ios(struct nvme_fc_ctrl *ctrl,
+					    bool start_queues);
 
 /* *********************** FC-NVME Port Management ************************ */
 
@@ -987,7 +989,8 @@ fc_dma_unmap_sg(struct device *dev, struct scatterlist *sg, int nents,
 static void nvme_fc_ctrl_put(struct nvme_fc_ctrl *);
 static int nvme_fc_ctrl_get(struct nvme_fc_ctrl *);
 
-static void nvme_fc_error_recovery(struct nvme_fc_ctrl *ctrl, char *errmsg);
+static void nvme_fc_delete_association(struct nvme_fc_ctrl *ctrl);
+static void nvme_fc_reconnect_or_delete(struct nvme_fc_ctrl *ctrl, int status);
 
 static void
 __nvme_fc_finish_ls_req(struct nvmefc_ls_req_op *lsop)
@@ -1880,7 +1883,23 @@ nvme_fc_ctrl_ioerr_work(struct work_struct *work)
 	struct nvme_fc_ctrl *ctrl =
 			container_of(work, struct nvme_fc_ctrl, ioerr_work);
 
-	nvme_fc_error_recovery(ctrl, "transport detected io error");
+	nvme_stop_keep_alive(&ctrl->ctrl);
+	flush_work(&ctrl->ctrl.async_event_work);
+
+	/* will block while waiting for io to terminate */
+	nvme_fc_delete_association(ctrl);
+	nvme_stop_ctrl(&ctrl->ctrl);
+
+	/* Do not reconnect if controller is being deleted */
+	if (!nvme_change_ctrl_state(&ctrl->ctrl, NVME_CTRL_CONNECTING))
+		return;
+
+	if (ctrl->rport->remoteport.port_state == FC_OBJSTATE_ONLINE) {
+		queue_delayed_work(nvme_wq, &ctrl->connect_work, 0);
+		return;
+	}
+
+	nvme_fc_reconnect_or_delete(ctrl, -ENOTCONN);
 }
 
 /*
@@ -2528,39 +2547,6 @@ __nvme_fc_abort_outstanding_ios(struct nvme_fc_ctrl *ctrl, bool start_queues)
 		nvme_unquiesce_admin_queue(&ctrl->ctrl);
 }
 
-static void
-nvme_fc_error_recovery(struct nvme_fc_ctrl *ctrl, char *errmsg)
-{
-	enum nvme_ctrl_state state = nvme_ctrl_state(&ctrl->ctrl);
-
-	/*
-	 * if an error (io timeout, etc) while (re)connecting, the remote
-	 * port requested terminating of the association (disconnect_ls)
-	 * or an error (timeout or abort) occurred on an io while creating
-	 * the controller.  Abort any ios on the association and let the
-	 * create_association error path resolve things.
-	 */
-	if (state == NVME_CTRL_CONNECTING) {
-		__nvme_fc_abort_outstanding_ios(ctrl, true);
-		dev_warn(ctrl->ctrl.device,
-			"NVME-FC{%d}: transport error during (re)connect\n",
-			ctrl->cnum);
-		return;
-	}
-
-	/* Otherwise, only proceed if in LIVE state - e.g. on first error */
-	if (state != NVME_CTRL_LIVE)
-		return;
-
-	dev_warn(ctrl->ctrl.device,
-		"NVME-FC{%d}: transport association event: %s\n",
-		ctrl->cnum, errmsg);
-	dev_warn(ctrl->ctrl.device,
-		"NVME-FC{%d}: resetting controller\n", ctrl->cnum);
-
-	nvme_reset_ctrl(&ctrl->ctrl);
-}
-
 static enum blk_eh_timer_return nvme_fc_timeout(struct request *rq)
 {
 	struct nvme_fc_fcp_op *op = blk_mq_rq_to_pdu(rq);
@@ -3399,7 +3385,6 @@ nvme_fc_reset_ctrl_work(struct work_struct *work)
 	}
 }
 
-
 static const struct nvme_ctrl_ops nvme_fc_ctrl_ops = {
 	.name			= "fc",
 	.module			= THIS_MODULE,
-- 
2.56.0

[PATCH v7 14/17] nvme-fc: Use CCR to recover controller that hits an error

[Mohamed Khalfella]

A LIVE controller that hits an error now moves to FENCING instead of
RESETTING. ctrl->fencing_work attempts CCR to terminate inflight IOs.
Regardless of the success or failure of the CCR operation, the
controller then moves to RESETTING to continue error recovery.

nvme_fc_fencing_work() tears down the association before fencing so
the LLDD can finish unregistering the remoteport/localport. With this
patch alone that still completes the inflight requests at the start of
the fence, which matches the behavior before this series. The next
patch holds them until fencing has finished.

Signed-off-by: Mohamed Khalfella <mkhalfella@purestorage.com>
---
 drivers/nvme/host/fc.c | 69 ++++++++++++++++++++++++++++++++++++------
 1 file changed, 59 insertions(+), 10 deletions(-)

diff --git a/drivers/nvme/host/fc.c b/drivers/nvme/host/fc.c
index 0ba621649afe..095adc9011cc 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;
 
@@ -1877,6 +1878,33 @@ __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;
+
+	/*
+	 * Tear down the association before fencing starts. Fencing can take
+	 * a long time and the LLDD can't finish unregistering the
+	 * remoteport/localport until all associations on it are gone.
+	 * Inflight requests are held and only completed once fencing has
+	 * finished.
+	 */
+	nvme_stop_keep_alive(ctrl);
+	cancel_work_sync(&ctrl->async_event_work);
+	nvme_fc_delete_association(fc_ctrl);
+
+	rem = nvme_fence_ctrl(ctrl);
+	if (rem)
+		dev_info(ctrl->device, "CCR failed, starting error 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);
+}
+
 static void
 nvme_fc_ctrl_ioerr_work(struct work_struct *work)
 {
@@ -1921,11 +1949,20 @@ char *nvme_fc_io_getuuid(struct nvmefc_fcp_req *req)
 EXPORT_SYMBOL_GPL(nvme_fc_io_getuuid);
 
 /*
- * Start recovery only if this caller can claim it with a state change.
+ * Start recovery only if this caller can claim it with a state change:
+ * FENCING from LIVE, or RESETTING otherwise.
  */
 static void
 nvme_fc_start_ioerr_recovery(struct nvme_fc_ctrl *ctrl, char *errmsg)
 {
+	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);
@@ -2439,6 +2476,12 @@ nvme_fc_ctrl_get(struct nvme_fc_ctrl *ctrl)
 	return kref_get_unless_zero(&ctrl->ref);
 }
 
+static void
+nvme_fc_stop_ctrl(struct nvme_ctrl *nctrl)
+{
+	flush_work(&to_fc_ctrl(nctrl)->fencing_work);
+}
+
 /*
  * All accesses from nvme core layer done - can now free the
  * controller. Called after last nvme_put_ctrl() call
@@ -2554,6 +2597,7 @@ static enum blk_eh_timer_return nvme_fc_timeout(struct request *rq)
 	u16 qnum = op->queue->qnum;
 	struct nvme_fc_cmd_iu *cmdiu = &op->cmd_iu;
 	struct nvme_command *sqe = &cmdiu->sqe;
+	enum nvme_ctrl_state state;
 
 	dev_info(ctrl->ctrl.device,
 		"NVME-FC{%d.%d}: io timeout: opcode %d fctype %d (%s) w10/11: "
@@ -2562,15 +2606,16 @@ static enum blk_eh_timer_return nvme_fc_timeout(struct request *rq)
 		nvme_fabrics_opcode_str(qnum, sqe),
 		sqe->common.cdw10, sqe->common.cdw11);
 
-	if (nvme_ctrl_state(&ctrl->ctrl) != NVME_CTRL_LIVE) {
+	state = nvme_ctrl_state(&ctrl->ctrl);
+	if (state != NVME_CTRL_LIVE && state != NVME_CTRL_FENCING) {
 		/*
-		 * The association is being set up, being torn down or is
-		 * already gone, so recovery cannot be started from here.
-		 * Abort the command so the connect or teardown it would
-		 * block can make progress. It completes through
-		 * nvme_fc_fcpio_done() once the LLDD has finished the
-		 * abort, and the abort is counted so a teardown that
-		 * follows waits for it.
+		 * The fence is over or the association is being set up,
+		 * being torn down or is already gone, so recovery cannot
+		 * be started from here. Abort the command so the connect
+		 * or teardown it would block can make progress. It
+		 * completes through nvme_fc_fcpio_done() once the LLDD has
+		 * finished the abort, and the abort is counted so a
+		 * teardown that follows waits for it.
 		 */
 		__nvme_fc_abort_op(ctrl, op);
 		return BLK_EH_DONE;
@@ -2578,7 +2623,9 @@ static enum blk_eh_timer_return nvme_fc_timeout(struct request *rq)
 
 	/*
 	 * A LIVE controller starts error recovery, which tears the
-	 * association down and completes this request.
+	 * association down and completes this request. While FENCING the
+	 * request must not be completed before the fence is over, so only
+	 * rearm the timer; the fence completes it.
 	 */
 	nvme_fc_start_ioerr_recovery(ctrl, "io timeout");
 	return BLK_EH_RESET_TIMER;
@@ -3397,6 +3444,7 @@ static const struct nvme_ctrl_ops nvme_fc_ctrl_ops = {
 	.submit_async_event	= nvme_fc_submit_async_event,
 	.delete_ctrl		= nvme_fc_delete_ctrl,
 	.get_address		= nvmf_get_address,
+	.stop_ctrl		= nvme_fc_stop_ctrl,
 	.get_virt_boundary	= nvmf_get_virt_boundary,
 };
 
@@ -3516,6 +3564,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);
 
-- 
2.56.0

[PATCH v7 15/17] nvme-fc: Hold inflight requests while in FENCING state

[Mohamed Khalfella]

When a controller enters FENCING, its inflight requests must not be
completed until CCR or CQT guarantees that the target can no longer
execute them. Completing them earlier lets multipath resubmit them on
another path while the original command may still run on this one.

nvme-fc currently cannot hold requests across the fence.
nvme_fc_fencing_work() tears the association down first, so the LLDD
can finish unregistering the remoteport/localport, and
nvme_fc_delete_association() completes all requests as part of the
teardown. nvme_fc_fcpio_done() also completes requests that finish with
an error, which happens to every request aborted by the teardown.

Split nvme_fc_delete_association() in two:

 - __nvme_fc_teardown_association() aborts outstanding ios, waits for
   the aborts to drain, sends Disconnect, deletes the hw queues and
   marks the controller inactive on the rport. It completes no
   requests.

 - nvme_fc_flush_held_requests() completes the held requests and
   unquiesces the queues.

nvme_fc_delete_association() calls both, as before.
nvme_fc_fencing_work() calls the teardown up front and the flush only
after nvme_fence_ctrl() returns, before moving to FENCED. The queues
stay quiesced in between, so nothing new reaches the dead association.

In nvme_fc_fcpio_done(), start recovery first and hold the request
only if the controller is FENCING afterwards, since a held request is
completed only by the flush that fencing_work runs. If the claim fails
because a reset, a delete, or a connect in progress owns the controller,
the request is completed at once and that owner finishes the teardown.
A held request cannot be completed behind the fence's back because
nvme_fc_timeout() returns BLK_EH_RESET_TIMER while FENCING, and
nvme_fc_start_ioerr_recovery() does nothing in FENCING.

Track held requests explicitly in op->state with a new
FCPOP_STATE_HELD, and have the flush complete only those. An op enters
HELD only through cmpxchg(COMPLETE -> HELD) in nvme_fc_fcpio_done().
For each busy request, the flush does cmpxchg(COMPLETE -> IDLE) and
cancels the request only if it saw HELD. This settles the race with an
nvme_fc_fcpio_done() that has already moved its op to COMPLETE but has
not yet decided whether to hold it. Teardown does not wait for such an
op, since it was no longer ACTIVE when the aborts were issued:

 - nvme_fc_fcpio_done() wins: the op is HELD and the flush cancels it.
 - The flush wins: the op is IDLE, the hold cmpxchg fails and
   nvme_fc_fcpio_done() completes the request itself.

The flush must use a single cmpxchg. Reading the state and then acting
on it would let the op become HELD in between and leave the request
with nothing to complete it.

Both cases rely on the flush being the only thing that can move an op
out of COMPLETE behind nvme_fc_fcpio_done(), so make
__nvme_fc_abort_op() use cmpxchg(ACTIVE -> ABORTED). It used
xchg(ABORTED) and then restored the old value if the op was not ACTIVE,
so for a moment such an op read as ABORTED. A hold cmpxchg that ran in
that moment would fail and complete the request in the middle of the
fence.

Count an aborted op down only after nvme_fc_fcpio_done() has completed
or held its request, so that teardown, once it stops waiting, knows
every aborted request has been decided. The count-down now runs outside
ctrl->lock, so make ctrl->iocnt an atomic_t and wait for it with
wait_event().

Signed-off-by: Mohamed Khalfella <mkhalfella@purestorage.com>
---
 drivers/nvme/host/fc.c | 171 +++++++++++++++++++++++++++++++++--------
 1 file changed, 139 insertions(+), 32 deletions(-)

diff --git a/drivers/nvme/host/fc.c b/drivers/nvme/host/fc.c
index 095adc9011cc..5e8977ad51cf 100644
--- a/drivers/nvme/host/fc.c
+++ b/drivers/nvme/host/fc.c
@@ -83,6 +83,7 @@ enum nvme_fcpop_state {
 	FCPOP_STATE_ACTIVE	= 2,
 	FCPOP_STATE_ABORTED	= 3,
 	FCPOP_STATE_COMPLETE	= 4,
+	FCPOP_STATE_HELD	= 5,
 };
 
 struct nvme_fc_fcp_op {
@@ -172,7 +173,7 @@ struct nvme_fc_ctrl {
 
 	struct kref		ref;
 	unsigned long		flags;
-	u32			iocnt;
+	atomic_t		iocnt;
 	wait_queue_head_t	ioabort_wait;
 
 	struct nvme_fc_fcp_op	aen_ops[NVME_NR_AEN_COMMANDS];
@@ -990,6 +991,8 @@ fc_dma_unmap_sg(struct device *dev, struct scatterlist *sg, int nents,
 static void nvme_fc_ctrl_put(struct nvme_fc_ctrl *);
 static int nvme_fc_ctrl_get(struct nvme_fc_ctrl *);
 
+static void __nvme_fc_teardown_association(struct nvme_fc_ctrl *ctrl);
+static void nvme_fc_flush_held_requests(struct nvme_fc_ctrl *ctrl);
 static void nvme_fc_delete_association(struct nvme_fc_ctrl *ctrl);
 static void nvme_fc_reconnect_or_delete(struct nvme_fc_ctrl *ctrl, int status);
 
@@ -1828,12 +1831,11 @@ __nvme_fc_abort_op(struct nvme_fc_ctrl *ctrl, struct nvme_fc_fcp_op *op)
 	 * path, must not still be in progress in the LLDD then.
 	 */
 	spin_lock_irqsave(&ctrl->lock, flags);
-	opstate = atomic_xchg(&op->state, FCPOP_STATE_ABORTED);
-	if (opstate != FCPOP_STATE_ACTIVE) {
-		atomic_set(&op->state, opstate);
-	} else {
+	opstate = atomic_cmpxchg(&op->state, FCPOP_STATE_ACTIVE,
+				 FCPOP_STATE_ABORTED);
+	if (opstate == FCPOP_STATE_ACTIVE) {
 		op->flags |= FCOP_FLAGS_TERMIO;
-		ctrl->iocnt++;
+		atomic_inc(&ctrl->iocnt);
 	}
 	spin_unlock_irqrestore(&ctrl->lock, flags);
 
@@ -1863,19 +1865,27 @@ nvme_fc_abort_aen_ops(struct nvme_fc_ctrl *ctrl)
 }
 
 static inline void
+__nvme_fc_fcpop_count_one_down(struct nvme_fc_ctrl *ctrl)
+{
+	if (atomic_dec_return(&ctrl->iocnt) == 0)
+		wake_up(&ctrl->ioabort_wait);
+}
+
+static inline bool
 __nvme_fc_fcpop_chk_teardowns(struct nvme_fc_ctrl *ctrl,
 		struct nvme_fc_fcp_op *op, int opstate)
 {
 	unsigned long flags;
+	bool ret = false;
 
 	if (opstate == FCPOP_STATE_ABORTED) {
 		spin_lock_irqsave(&ctrl->lock, flags);
-		if (op->flags & FCOP_FLAGS_TERMIO) {
-			if (!--ctrl->iocnt)
-				wake_up(&ctrl->ioabort_wait);
-		}
+		if (op->flags & FCOP_FLAGS_TERMIO)
+			ret = true;
 		spin_unlock_irqrestore(&ctrl->lock, flags);
 	}
+
+	return ret;
 }
 
 static void nvme_fc_fencing_work(struct work_struct *work)
@@ -1894,12 +1904,14 @@ static void nvme_fc_fencing_work(struct work_struct *work)
 	 */
 	nvme_stop_keep_alive(ctrl);
 	cancel_work_sync(&ctrl->async_event_work);
-	nvme_fc_delete_association(fc_ctrl);
+	if (test_and_clear_bit(ASSOC_ACTIVE, &fc_ctrl->flags))
+		__nvme_fc_teardown_association(fc_ctrl);
 
 	rem = nvme_fence_ctrl(ctrl);
 	if (rem)
 		dev_info(ctrl->device, "CCR failed, starting error recovery\n");
 
+	nvme_fc_flush_held_requests(fc_ctrl);
 	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);
@@ -1982,7 +1994,7 @@ nvme_fc_fcpio_done(struct nvmefc_fcp_req *req)
 	struct nvme_command *sqe = &op->cmd_iu.sqe;
 	__le16 status = cpu_to_le16(NVME_SC_SUCCESS << 1);
 	union nvme_result result;
-	bool terminate_assoc = true;
+	bool op_term, hold = false, terminate_assoc = true;
 	int opstate;
 
 	/*
@@ -2113,22 +2125,58 @@ nvme_fc_fcpio_done(struct nvmefc_fcp_req *req)
 	terminate_assoc = false;
 
 done:
+	/*
+	 * Start recovery before completing or holding anything. A held
+	 * request is only completed by nvme_fc_flush_held_requests(), which
+	 * fencing_work runs once the fence is over, so a request may only be
+	 * held once FENCING has been claimed and fencing_work is guaranteed
+	 * to run. If the claim fails because a reset or a delete already owns
+	 * the controller, the request is completed below and that owner
+	 * tears the association down.
+	 */
+	if (terminate_assoc) {
+		nvme_fc_start_ioerr_recovery(ctrl, "io error");
+		hold = nvme_ctrl_state(&ctrl->ctrl) == NVME_CTRL_FENCING;
+	}
+
 	if (op->flags & FCOP_FLAGS_AEN) {
 		nvme_complete_async_event(&queue->ctrl->ctrl, status, &result);
-		__nvme_fc_fcpop_chk_teardowns(ctrl, op, opstate);
+		if (__nvme_fc_fcpop_chk_teardowns(ctrl, op, opstate))
+			__nvme_fc_fcpop_count_one_down(ctrl);
 		atomic_set(&op->state, FCPOP_STATE_IDLE);
 		op->flags = FCOP_FLAGS_AEN;	/* clear other flags */
 		nvme_fc_ctrl_put(ctrl);
-		goto check_error;
+		return;
 	}
 
-	__nvme_fc_fcpop_chk_teardowns(ctrl, op, opstate);
+	/*
+	 * We can not access op after the request is completed because it can
+	 * be reused immediately. At the same time we want to wakeup the thread
+	 * waiting for ongoing IOs _after_ requests are completed. This is
+	 * necessary because that thread will start canceling inflight IOs
+	 * and we want to avoid request completion racing with cancellation.
+	 */
+	op_term = __nvme_fc_fcpop_chk_teardowns(ctrl, op, opstate);
+
+	/*
+	 * If the controller is FENCING, do not complete this request now.
+	 * Move the op to HELD and let nvme_fc_cancel_held_requests() cancel
+	 * it when it is safe to do so. HELD is only reachable from COMPLETE
+	 * through this cmpxchg: if the cancellation has already walked the
+	 * tagset it moved the op out of COMPLETE, the cmpxchg fails and the
+	 * request is completed here instead. Either way exactly one path
+	 * completes it.
+	 */
+	if (hold && atomic_cmpxchg(&op->state, FCPOP_STATE_COMPLETE,
+				   FCPOP_STATE_HELD) == FCPOP_STATE_COMPLETE)
+		goto check_op_term;
+
 	if (!nvme_try_complete_req(rq, status, result))
 		nvme_fc_complete_rq(rq);
 
-check_error:
-	if (terminate_assoc)
-		nvme_fc_start_ioerr_recovery(ctrl, "io error");
+check_op_term:
+	if (op_term)
+		__nvme_fc_fcpop_count_one_down(ctrl);
 }
 
 static int
@@ -2803,7 +2851,8 @@ nvme_fc_start_fcp_op(struct nvme_fc_ctrl *ctrl, struct nvme_fc_queue *queue,
 		 * cmd with the csn was supposed to arrive.
 		 */
 		opstate = atomic_xchg(&op->state, FCPOP_STATE_COMPLETE);
-		__nvme_fc_fcpop_chk_teardowns(ctrl, op, opstate);
+		if (__nvme_fc_fcpop_chk_teardowns(ctrl, op, opstate))
+			__nvme_fc_fcpop_count_one_down(ctrl);
 
 		if (!(op->flags & FCOP_FLAGS_AEN)) {
 			nvme_fc_unmap_data(ctrl, op->rq, op);
@@ -3261,22 +3310,22 @@ nvme_fc_create_association(struct nvme_fc_ctrl *ctrl)
 	return ret;
 }
 
-
 /*
- * This routine stops operation of the controller on the host side.
- * On the host os stack side: Admin and IO queues are stopped,
- *   outstanding ios on them terminated via FC ABTS.
- * On the link side: the association is terminated.
+ * This routine tears down the association on the link side and releases
+ * all LLDD resources: outstanding ios are terminated via FC ABTS, hw
+ * queues are deleted and the controller is marked inactive on the
+ * rport. Requests that nvme_fc_fcpio_done() decided to hold are NOT
+ * completed here, the caller must follow up with
+ * nvme_fc_flush_held_requests() once it is safe to release them.
+ *
+ * The caller must own the ASSOC_ACTIVE bit (test_and_clear_bit()).
  */
 static void
-nvme_fc_delete_association(struct nvme_fc_ctrl *ctrl)
+__nvme_fc_teardown_association(struct nvme_fc_ctrl *ctrl)
 {
 	struct nvmefc_ls_rcv_op *disls = NULL;
 	unsigned long flags;
 
-	if (!test_and_clear_bit(ASSOC_ACTIVE, &ctrl->flags))
-		return;
-
 	spin_lock_irqsave(&ctrl->lock, flags);
 	set_bit(FCCTRL_TERMIO, &ctrl->flags);
 	spin_unlock_irqrestore(&ctrl->lock, flags);
@@ -3287,8 +3336,8 @@ nvme_fc_delete_association(struct nvme_fc_ctrl *ctrl)
 	nvme_fc_abort_aen_ops(ctrl);
 
 	/* wait for all io that had to be aborted */
+	wait_event(ctrl->ioabort_wait, atomic_read(&ctrl->iocnt) == 0);
 	spin_lock_irq(&ctrl->lock);
-	wait_event_lock_irq(ctrl->ioabort_wait, ctrl->iocnt == 0, ctrl->lock);
 	clear_bit(FCCTRL_TERMIO, &ctrl->flags);
 	spin_unlock_irq(&ctrl->lock);
 
@@ -3323,13 +3372,71 @@ nvme_fc_delete_association(struct nvme_fc_ctrl *ctrl)
 	__nvme_fc_delete_hw_queue(ctrl, &ctrl->queues[0], 0);
 	nvme_fc_free_queue(&ctrl->queues[0]);
 
+	nvme_fc_ctlr_inactive_on_rport(ctrl);
+}
+
+static bool
+nvme_fc_cancel_held_request(struct request *req, void *data)
+{
+	struct nvme_fc_fcp_op *op = blk_mq_rq_to_pdu(req);
+	struct nvme_ctrl *ctrl = data;
+
+	/*
+	 * A single cmpxchg decides who completes this request. If the op is
+	 * HELD, nvme_fc_fcpio_done() is done with it and we cancel it. If it
+	 * is still COMPLETE, nvme_fc_fcpio_done() has not decided yet: move
+	 * it to IDLE so its own cmpxchg to HELD fails and it completes the
+	 * request itself (nvme_fc_complete_rq() sets IDLE anyway). Doing this
+	 * as read-then-cmpxchg would let the op become HELD in between and
+	 * leave it with no one to complete it.
+	 */
+	if (atomic_cmpxchg(&op->state, FCPOP_STATE_COMPLETE,
+			   FCPOP_STATE_IDLE) == FCPOP_STATE_HELD)
+		nvme_cancel_request(req, ctrl);
+
+	return true;
+}
+
+static void
+nvme_fc_cancel_held_requests(struct nvme_ctrl *ctrl, struct blk_mq_tag_set *set)
+{
+	if (!set)
+		return;
+
+	blk_mq_tagset_busy_iter(set, nvme_fc_cancel_held_request, ctrl);
+	blk_mq_tagset_wait_completed_request(set);
+}
+
+/* Complete the requests nvme_fc_fcpio_done() held during the fence */
+static void
+nvme_fc_flush_held_requests(struct nvme_fc_ctrl *fc_ctrl)
+{
+	struct nvme_ctrl *ctrl = &fc_ctrl->ctrl;
+
+	nvme_fc_cancel_held_requests(ctrl, ctrl->tagset);
+	nvme_fc_cancel_held_requests(ctrl, ctrl->admin_tagset);
+
 	/* re-enable the admin_q so anything new can fast fail */
-	nvme_unquiesce_admin_queue(&ctrl->ctrl);
+	nvme_unquiesce_admin_queue(ctrl);
 
 	/* resume the io queues so that things will fast fail */
-	nvme_unquiesce_io_queues(&ctrl->ctrl);
+	nvme_unquiesce_io_queues(ctrl);
+}
 
-	nvme_fc_ctlr_inactive_on_rport(ctrl);
+/*
+ * This routine stops operation of the controller on the host side.
+ * On the host os stack side: Admin and IO queues are stopped,
+ *   outstanding ios on them terminated via FC ABTS.
+ * On the link side: the association is terminated.
+ */
+static void
+nvme_fc_delete_association(struct nvme_fc_ctrl *ctrl)
+{
+	if (!test_and_clear_bit(ASSOC_ACTIVE, &ctrl->flags))
+		return;
+
+	__nvme_fc_teardown_association(ctrl);
+	nvme_fc_flush_held_requests(ctrl);
 }
 
 static void
-- 
2.56.0

[PATCH v7 16/17] nvmet: Add support for CQT to nvme target

[Mohamed Khalfella]

TP4129 KATO Corrections and Clarifications defined CQT (Command Quiesce
Time) which is used along with KATO (Keep Alive Timeout) to set an upper
time limit for attempting Cross-Controller Recovery. CQT is added as a
subsystem attribute that defaults to 0 to maintain the current behavior.
Changing attr_cqt disconnects the existing controllers of the subsystem
so that hosts reconnect and read the new value.

Signed-off-by: Mohamed Khalfella <mkhalfella@purestorage.com>
Reviewed-by: Hannes Reinecke <hare@kernel.org>
Reviewed-by: Sagi Grimberg <sagi@grimberg.me>
Reviewed-by: Daniel Wagner <dwagner@suse.de>
---
 drivers/nvme/target/admin-cmd.c |  1 +
 drivers/nvme/target/configfs.c  | 36 +++++++++++++++++++++++++++++++++
 drivers/nvme/target/core.c      |  3 +++
 drivers/nvme/target/nvmet.h     |  2 ++
 include/linux/nvme.h            |  5 ++++-
 5 files changed, 46 insertions(+), 1 deletion(-)

diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
index e13ffd17669b..76d65cca25dc 100644
--- a/drivers/nvme/target/admin-cmd.c
+++ b/drivers/nvme/target/admin-cmd.c
@@ -748,6 +748,7 @@ static void nvmet_execute_identify_ctrl(struct nvmet_req *req)
 	id->cntlid = cpu_to_le16(ctrl->cntlid);
 	id->ver = cpu_to_le32(ctrl->subsys->ver);
 	if (!nvmet_is_disc_subsys(ctrl->subsys)) {
+		id->cqt = cpu_to_le16(ctrl->cqt);
 		id->ciu = ctrl->ciu;
 		id->cirn = cpu_to_le64(ctrl->cirn);
 		id->ccrl = NVMF_CCR_LIMIT;
diff --git a/drivers/nvme/target/configfs.c b/drivers/nvme/target/configfs.c
index bde94656e4b7..babc4c7101f5 100644
--- a/drivers/nvme/target/configfs.c
+++ b/drivers/nvme/target/configfs.c
@@ -1770,6 +1770,41 @@ static ssize_t nvmet_subsys_attr_pi_enable_store(struct config_item *item,
 CONFIGFS_ATTR(nvmet_subsys_, attr_pi_enable);
 #endif
 
+static ssize_t nvmet_subsys_attr_cqt_show(struct config_item *item,
+					  char *page)
+{
+	return snprintf(page, PAGE_SIZE, "%u\n", to_subsys(item)->cqt);
+}
+
+static ssize_t nvmet_subsys_attr_cqt_store(struct config_item *item,
+					   const char *page, size_t cnt)
+{
+	struct nvmet_subsys *subsys = to_subsys(item);
+	struct nvmet_ctrl *ctrl;
+	u16 cqt;
+
+	if (nvmet_is_disc_subsys(subsys))
+		return -EINVAL;
+
+	if (sscanf(page, "%hu\n", &cqt) != 1)
+		return -EINVAL;
+
+	down_write(&nvmet_config_sem);
+	mutex_lock(&subsys->lock);
+	if (subsys->cqt == cqt)
+		goto out;
+
+	subsys->cqt = cqt;
+	/* Force reconnect */
+	list_for_each_entry(ctrl, &subsys->ctrls, subsys_entry)
+		ctrl->ops->delete_ctrl(ctrl);
+out:
+	mutex_unlock(&subsys->lock);
+	up_write(&nvmet_config_sem);
+	return cnt;
+}
+CONFIGFS_ATTR(nvmet_subsys_, attr_cqt);
+
 static ssize_t nvmet_subsys_attr_qid_max_show(struct config_item *item,
 					      char *page)
 {
@@ -1812,6 +1847,7 @@ static struct configfs_attribute *nvmet_subsys_attrs[] = {
 	&nvmet_subsys_attr_attr_vendor_id,
 	&nvmet_subsys_attr_attr_subsys_vendor_id,
 	&nvmet_subsys_attr_attr_model,
+	&nvmet_subsys_attr_attr_cqt,
 	&nvmet_subsys_attr_attr_qid_max,
 	&nvmet_subsys_attr_attr_ieee_oui,
 	&nvmet_subsys_attr_attr_firmware,
diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
index 12ab522ea5a1..8a3e51d573d4 100644
--- a/drivers/nvme/target/core.c
+++ b/drivers/nvme/target/core.c
@@ -1794,6 +1794,7 @@ struct nvmet_ctrl *nvmet_alloc_ctrl(struct nvmet_alloc_ctrl_args *args)
 	ctrl->cntlid = ret;
 
 	if (!nvmet_is_disc_subsys(ctrl->subsys)) {
+		ctrl->cqt = subsys->cqt;
 		ctrl->ciu = get_random_u8() ? : 1;
 		ctrl->cirn = get_random_u64() ? : 1;
 	}
@@ -2022,10 +2023,12 @@ struct nvmet_subsys *nvmet_subsys_alloc(const char *subsysnqn,
 
 	switch (type) {
 	case NVME_NQN_NVME:
+		subsys->cqt = NVMF_CQT_MS;
 		subsys->max_qid = NVMET_NR_QUEUES;
 		break;
 	case NVME_NQN_DISC:
 	case NVME_NQN_CURR:
+		subsys->cqt = 0;
 		subsys->max_qid = 0;
 		break;
 	default:
diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
index 2cdfa1165292..911ad2b997f7 100644
--- a/drivers/nvme/target/nvmet.h
+++ b/drivers/nvme/target/nvmet.h
@@ -275,6 +275,7 @@ struct nvmet_ctrl {
 	uuid_t			hostid;
 	u16			cntlid;
 	u16			max_qid;
+	u16			cqt;
 	u8			ciu;
 	u32			kato;
 	u64			cirn;
@@ -366,6 +367,7 @@ struct nvmet_subsys {
 #ifdef CONFIG_NVME_TARGET_DEBUGFS
 	struct dentry		*debugfs_dir;
 #endif
+	u16			cqt;
 	u16			max_qid;
 
 	u64			ver;
diff --git a/include/linux/nvme.h b/include/linux/nvme.h
index 96b71aaad630..6d413f2ca3d5 100644
--- a/include/linux/nvme.h
+++ b/include/linux/nvme.h
@@ -21,6 +21,7 @@
 #define NVMF_TRADDR_SIZE	256
 #define NVMF_TSAS_SIZE		256
 
+#define NVMF_CQT_MS		0
 #define NVMF_CCR_LIMIT		4
 #define NVMF_CCR_PER_PAGE	511
 
@@ -368,7 +369,9 @@ struct nvme_id_ctrl {
 	__u8			anacap;
 	__le32			anagrpmax;
 	__le32			nanagrpid;
-	__u8			rsvd352[160];
+	__u8			rsvd352[34];
+	__le16			cqt;
+	__u8			rsvd388[124];
 	__u8			sqes;
 	__u8			cqes;
 	__le16			maxcmd;
-- 
2.56.0

[PATCH v7 17/17] nvme: Add support for CQT to nvme host

[Mohamed Khalfella]

TP4129 KATO Corrections and Clarifications defined CQT (Command Quiesce
Time) which is used along with KATO (Keep Alive Timeout) to set an upper
limit for attempting Cross-Controller Recovery. Add ctrl->cqt, read its
value from controller identify response, expose it via sysfs, and have
nvme_fence_timeout_ms() account for it.

Use CQT to drive time-based recovery in fc, rdma, and tcp transports.
Add a fenced_work delayed work to each transport controller. If CCR
fails, fencing_work switches to error recovery immediately if CQT is not
supported. Otherwise it schedules fenced_work after the remaining fence
timeout, which also switches to error recovery when scheduled.

Signed-off-by: Mohamed Khalfella <mkhalfella@purestorage.com>
---
 Documentation/ABI/stable/sysfs-nvme | 10 +++++++++
 drivers/nvme/host/core.c            |  1 +
 drivers/nvme/host/fc.c              | 33 +++++++++++++++++++++++++++--
 drivers/nvme/host/nvme.h            |  5 +++--
 drivers/nvme/host/rdma.c            | 32 ++++++++++++++++++++++++++--
 drivers/nvme/host/sysfs.c           |  2 ++
 drivers/nvme/host/tcp.c             | 32 ++++++++++++++++++++++++++--
 7 files changed, 107 insertions(+), 8 deletions(-)

diff --git a/Documentation/ABI/stable/sysfs-nvme b/Documentation/ABI/stable/sysfs-nvme
index df883d644fb4..75ac22a3fd8a 100644
--- a/Documentation/ABI/stable/sysfs-nvme
+++ b/Documentation/ABI/stable/sysfs-nvme
@@ -120,6 +120,16 @@ Description:
 		Shows the Keep Alive Timeout value in milliseconds for
 		this controller.
 
+What:		/sys/class/nvme/nvmeX/cqt
+Date:		October 2026
+KernelVersion:	7.5
+Contact:	Mohamed Khalfella <mkhalfella@purestorage.com>
+Description:
+		Shows CQT (Command Quiesce Time) in milliseconds as reported
+		by the Identify Controller data structure. CQT is the time the
+		controller needs to quiesce outstanding commands once the
+		host loses communication with it.
+
 What:		/sys/class/nvme/nvmeX/cmb
 Date:		October 2016
 KernelVersion:	4.9
diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 54b83092a9e8..4c5448dbbfbd 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -3873,6 +3873,7 @@ static int nvme_init_identify(struct nvme_ctrl *ctrl)
 	ctrl->ciu = id->ciu;
 	ctrl->cirn = le64_to_cpu(id->cirn);
 	ctrl->ccrl = id->ccrl;
+	ctrl->cqt = le16_to_cpu(id->cqt);
 
 	ctrl->oacs = le16_to_cpu(id->oacs);
 	ctrl->oncs = le16_to_cpu(id->oncs);
diff --git a/drivers/nvme/host/fc.c b/drivers/nvme/host/fc.c
index 5e8977ad51cf..2d5d2ad6082d 100644
--- a/drivers/nvme/host/fc.c
+++ b/drivers/nvme/host/fc.c
@@ -168,6 +168,7 @@ struct nvme_fc_ctrl {
 	struct blk_mq_tag_set	tag_set;
 
 	struct work_struct	fencing_work;
+	struct delayed_work	fenced_work;
 	struct work_struct	ioerr_work;
 	struct delayed_work	connect_work;
 
@@ -1888,6 +1889,19 @@ __nvme_fc_fcpop_chk_teardowns(struct nvme_fc_ctrl *ctrl,
 	return ret;
 }
 
+static void nvme_fc_fenced_work(struct work_struct *work)
+{
+	struct nvme_fc_ctrl *fc_ctrl = container_of(to_delayed_work(work),
+					struct nvme_fc_ctrl, fenced_work);
+	struct nvme_ctrl *ctrl = &fc_ctrl->ctrl;
+
+	dev_info(ctrl->device, "Time-based recovery finished\n");
+	nvme_fc_flush_held_requests(fc_ctrl);
+	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);
+}
+
 static void nvme_fc_fencing_work(struct work_struct *work)
 {
 	struct nvme_fc_ctrl *fc_ctrl =
@@ -1908,9 +1922,22 @@ static void nvme_fc_fencing_work(struct work_struct *work)
 		__nvme_fc_teardown_association(fc_ctrl);
 
 	rem = nvme_fence_ctrl(ctrl);
-	if (rem)
-		dev_info(ctrl->device, "CCR failed, starting error recovery\n");
+	if (!rem)
+		goto done;
 
+	if (!ctrl->cqt) {
+		dev_info(ctrl->device,
+			 "CCR failed, CQT not supported, skip time-based recovery\n");
+		goto done;
+	}
+
+	dev_info(ctrl->device,
+		 "CCR failed, switch to time-based recovery, timeout = %ums\n",
+		 jiffies_to_msecs(rem));
+	queue_delayed_work(nvme_wq, &fc_ctrl->fenced_work, rem);
+	return;
+
+done:
 	nvme_fc_flush_held_requests(fc_ctrl);
 	nvme_change_ctrl_state(ctrl, NVME_CTRL_FENCED);
 	if (nvme_change_ctrl_state(ctrl, NVME_CTRL_RESETTING))
@@ -2528,6 +2555,7 @@ static void
 nvme_fc_stop_ctrl(struct nvme_ctrl *nctrl)
 {
 	flush_work(&to_fc_ctrl(nctrl)->fencing_work);
+	flush_delayed_work(&to_fc_ctrl(nctrl)->fenced_work);
 }
 
 /*
@@ -3672,6 +3700,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_DELAYED_WORK(&ctrl->fenced_work, nvme_fc_fenced_work);
 	INIT_WORK(&ctrl->ioerr_work, nvme_fc_ctrl_ioerr_work);
 	spin_lock_init(&ctrl->lock);
 
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index 884541644240..67cf70e24953 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -409,6 +409,7 @@ struct nvme_ctrl {
 	u32 max_zone_append;
 #endif
 	u16 crdt[3];
+	u16 cqt;
 	u16 oncs;
 	u8 dmrl;
 	u8 ciu;
@@ -1373,8 +1374,8 @@ static inline bool nvme_multi_css(struct nvme_ctrl *ctrl)
 static inline unsigned long nvme_fence_timeout_ms(struct nvme_ctrl *ctrl)
 {
 	if (ctrl->ctratt & NVME_CTRL_ATTR_TBKAS)
-		return 3 * ctrl->kato * 1000;
-	return 2 * ctrl->kato * 1000;
+		return 3 * ctrl->kato * 1000 + ctrl->cqt;
+	return 2 * ctrl->kato * 1000 + ctrl->cqt;
 }
 
 #endif /* _NVME_H */
diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
index da1ec2dd6205..037423d27d8d 100644
--- a/drivers/nvme/host/rdma.c
+++ b/drivers/nvme/host/rdma.c
@@ -120,6 +120,7 @@ struct nvme_rdma_ctrl {
 	/* other member variables */
 	struct blk_mq_tag_set	tag_set;
 	struct work_struct	fencing_work;
+	struct delayed_work	fenced_work;
 	struct work_struct	err_work;
 
 	struct nvme_rdma_qe	async_event_sqe;
@@ -994,6 +995,7 @@ static void nvme_rdma_stop_ctrl(struct nvme_ctrl *nctrl)
 	struct nvme_rdma_ctrl *ctrl = to_rdma_ctrl(nctrl);
 
 	flush_work(&ctrl->fencing_work);
+	flush_delayed_work(&ctrl->fenced_work);
 	flush_work(&ctrl->err_work);
 	cancel_delayed_work_sync(&ctrl->reconnect_work);
 }
@@ -1155,6 +1157,18 @@ static void nvme_rdma_reconnect_ctrl_work(struct work_struct *work)
 	nvme_rdma_reconnect_or_remove(ctrl, ret);
 }
 
+static void nvme_rdma_fenced_work(struct work_struct *work)
+{
+	struct nvme_rdma_ctrl *rdma_ctrl = container_of(to_delayed_work(work),
+					   struct nvme_rdma_ctrl, fenced_work);
+	struct nvme_ctrl *ctrl = &rdma_ctrl->ctrl;
+
+	dev_info(ctrl->device, "Time-based recovery finished\n");
+	nvme_change_ctrl_state(ctrl, NVME_CTRL_FENCED);
+	if (nvme_change_ctrl_state(ctrl, NVME_CTRL_RESETTING))
+		queue_work(nvme_reset_wq, &rdma_ctrl->err_work);
+}
+
 static void nvme_rdma_fencing_work(struct work_struct *work)
 {
 	struct nvme_rdma_ctrl *rdma_ctrl = container_of(work,
@@ -1163,9 +1177,22 @@ static void nvme_rdma_fencing_work(struct work_struct *work)
 	unsigned long rem;
 
 	rem = nvme_fence_ctrl(ctrl);
-	if (rem)
-		dev_info(ctrl->device, "CCR failed, starting error recovery\n");
+	if (!rem)
+		goto done;
+
+	if (!ctrl->cqt) {
+		dev_info(ctrl->device,
+			 "CCR failed, CQT not supported, skip time-based recovery\n");
+		goto done;
+	}
+
+	dev_info(ctrl->device,
+		 "CCR failed, switch to time-based recovery, timeout = %ums\n",
+		 jiffies_to_msecs(rem));
+	queue_delayed_work(nvme_wq, &rdma_ctrl->fenced_work, rem);
+	return;
 
+done:
 	nvme_change_ctrl_state(ctrl, NVME_CTRL_FENCED);
 	if (nvme_change_ctrl_state(ctrl, NVME_CTRL_RESETTING))
 		queue_work(nvme_reset_wq, &rdma_ctrl->err_work);
@@ -2357,6 +2384,7 @@ static struct nvme_rdma_ctrl *nvme_rdma_alloc_ctrl(struct device *dev,
 	INIT_DELAYED_WORK(&ctrl->reconnect_work,
 			nvme_rdma_reconnect_ctrl_work);
 	INIT_WORK(&ctrl->fencing_work, nvme_rdma_fencing_work);
+	INIT_DELAYED_WORK(&ctrl->fenced_work, nvme_rdma_fenced_work);
 	INIT_WORK(&ctrl->err_work, nvme_rdma_error_recovery_work);
 	INIT_WORK(&ctrl->ctrl.reset_work, nvme_rdma_reset_ctrl_work);
 
diff --git a/drivers/nvme/host/sysfs.c b/drivers/nvme/host/sysfs.c
index ec4ca1669253..67bd4ae61484 100644
--- a/drivers/nvme/host/sysfs.c
+++ b/drivers/nvme/host/sysfs.c
@@ -490,6 +490,7 @@ nvme_show_int_function(numa_node);
 nvme_show_int_function(queue_count);
 nvme_show_int_function(sqsize);
 nvme_show_int_function(kato);
+nvme_show_int_function(cqt);
 
 static ssize_t nvme_sysfs_ciu_show(struct device *dev,
 					  struct device_attribute *attr,
@@ -968,6 +969,7 @@ static struct attribute *nvme_dev_attrs[] = {
 	&dev_attr_sqsize.attr,
 	&dev_attr_ciu.attr,
 	&dev_attr_cirn.attr,
+	&dev_attr_cqt.attr,
 	&dev_attr_hostnqn.attr,
 	&dev_attr_hostid.attr,
 	&dev_attr_ctrl_loss_tmo.attr,
diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
index 244c6c50cf99..d079ca6412a5 100644
--- a/drivers/nvme/host/tcp.c
+++ b/drivers/nvme/host/tcp.c
@@ -196,6 +196,7 @@ struct nvme_tcp_ctrl {
 	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;
@@ -2774,6 +2775,18 @@ 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;
+
+	dev_info(ctrl->device, "Time-based recovery finished\n");
+	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,
@@ -2782,9 +2795,22 @@ static void nvme_tcp_fencing_work(struct work_struct *work)
 	unsigned long rem;
 
 	rem = nvme_fence_ctrl(ctrl);
-	if (rem)
-		dev_info(ctrl->device, "CCR failed, starting error recovery\n");
+	if (!rem)
+		goto done;
+
+	if (!ctrl->cqt) {
+		dev_info(ctrl->device,
+			 "CCR failed, CQT not supported, skip time-based recovery\n");
+		goto done;
+	}
 
+	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;
+
+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);
@@ -2866,6 +2892,7 @@ static void nvme_reset_ctrl_work(struct work_struct *work)
 static void nvme_tcp_stop_ctrl(struct nvme_ctrl *ctrl)
 {
 	flush_work(&to_tcp_ctrl(ctrl)->fencing_work);
+	flush_delayed_work(&to_tcp_ctrl(ctrl)->fenced_work);
 	flush_work(&to_tcp_ctrl(ctrl)->err_work);
 	cancel_delayed_work_sync(&to_tcp_ctrl(ctrl)->connect_work);
 }
@@ -3238,6 +3265,7 @@ static struct nvme_tcp_ctrl *nvme_tcp_alloc_ctrl(struct device *dev,
 	INIT_DELAYED_WORK(&ctrl->connect_work,
 			nvme_tcp_reconnect_ctrl_work);
 	INIT_WORK(&ctrl->fencing_work, nvme_tcp_fencing_work);
+	INIT_DELAYED_WORK(&ctrl->fenced_work, nvme_tcp_fenced_work);
 	INIT_WORK(&ctrl->err_work, nvme_tcp_error_recovery_work);
 	INIT_WORK(&ctrl->ctrl.reset_work, nvme_reset_ctrl_work);
 
-- 
2.56.0

Re: [PATCH v7 02/17] nvmet/debugfs: Export controller CIU and CIRN via debugfs

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Low] nvmet: torn 64-bit read of ctrl->cirn in
  nvmet_ctrl_instance_cirn_show()
--

commit 70231338c7dc52cc534009767cd7997ab988025e
Author: Mohamed Khalfella <mkhalfella@purestorage.com>

nvmet/debugfs: Export controller CIU and CIRN via debugfs

This commit exports a target controller's CIU and CIRN fields as debugfs
attributes under the controller's debugfs directory.

> diff --git a/drivers/nvme/target/debugfs.c b/drivers/nvme/target/debugfs.c
> index e85fe1d4c9f8d..221fa81e124fe 100644
> --- a/drivers/nvme/target/debugfs.c
> +++ b/drivers/nvme/target/debugfs.c
> @@ -153,6 +153,24 @@ static int nvmet_ctrl_tls_concat_show(struct seq_file *m, void *p)
>  NVMET_DEBUGFS_ATTR(nvmet_ctrl_tls_concat);
>  #endif
>  
> +static int nvmet_ctrl_instance_ciu_show(struct seq_file *m, void *p)
> +{
> +	struct nvmet_ctrl *ctrl = m->private;
> +
> +	seq_printf(m, "%02x\n", ctrl->ciu);
> +	return 0;
> +}
> +NVMET_DEBUGFS_ATTR(nvmet_ctrl_instance_ciu);
> +
> +static int nvmet_ctrl_instance_cirn_show(struct seq_file *m, void *p)
> +{
> +	struct nvmet_ctrl *ctrl = m->private;
> +
> +	seq_printf(m, "%016llx\n", ctrl->cirn);
> +	return 0;
> +}
> +NVMET_DEBUGFS_ATTR(nvmet_ctrl_instance_cirn);

[Severity: Low]
Can nvmet_ctrl_instance_cirn_show() observe a torn 64-bit read of
ctrl->cirn on 32-bit architectures?

nvmet_alloc_ctrl() calls nvmet_debugfs_ctrl_setup() before the host
enables the controller, and nvmet_update_cc() -> nvmet_start_ctrl() later
writes a new 64-bit value to ctrl->cirn under ctrl->lock whenever the
controller is enabled or re-enabled.  Reading ctrl->cirn without holding
ctrl->lock races with that update and can report a corrupted mix of the
old and new values to userspace.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261114030454.2836543-1-mkhalfella@purestorage.com?part=2

Re: [PATCH v7 01/17] nvmet: Rapid Path Failure Recovery set controller identify fields

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] nvmet: missing TP8028 field overrides in
  nvmet_passthru_override_id_ctrl()
--

commit b07e2fdf2211131fb02142faf21d5cf7328792da
Author: Mohamed Khalfella <mkhalfella@purestorage.com>

nvmet: Rapid Path Failure Recovery set controller identify fields

This commit adds the TP8028 Rapid Path Failure Recovery fields (ciu, cirn,
and ccrl) to struct nvme_id_ctrl, initializes ciu and cirn on controller
allocation and start, and populates them in the Identify Controller
response for non-discovery subsystems.

> diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
> index f12d830986446..5710ae53eb83a 100644
> --- a/drivers/nvme/target/admin-cmd.c
> +++ b/drivers/nvme/target/admin-cmd.c
> @@ -702,6 +702,11 @@ static void nvmet_execute_identify_ctrl(struct nvmet_req *req)
>  	id->mdts = nvmet_ctrl_mdts(req);
>  	id->cntlid = cpu_to_le16(ctrl->cntlid);
>  	id->ver = cpu_to_le32(ctrl->subsys->ver);
> +	if (!nvmet_is_disc_subsys(ctrl->subsys)) {
> +		id->ciu = ctrl->ciu;
> +		id->cirn = cpu_to_le64(ctrl->cirn);
> +		id->ccrl = NVMF_CCR_LIMIT;
> +	}

[Severity: Medium]
Should nvmet_passthru_override_id_ctrl() also override id->ciu, id->cirn,
and id->ccrl?

On a passthrough subsystem, Identify Controller runs through
nvmet_passthru_execute_cmd_work() -> nvmet_passthru_override_id_ctrl(),
which overrides id->cntlid and id->ver but leaves id->ciu, id->cirn, and
id->ccrl with whatever the backend controller returned.  Every fabric
controller connected to the passthrough subsystem will report the backend
device's ciu and cirn instead of ctrl->ciu and ctrl->cirn, and if the
backend controller advertises a non-zero ccrl, hosts will issue
Cross-Controller Reset commands that nvmet_parse_passthru_admin_cmd()
rejects.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261114030454.2836543-1-mkhalfella@purestorage.com?part=1

Re: [PATCH v7 04/17] nvmet: Implement CCR logpage

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [High] nvmet: uninitialized SGL tail leak in
  nvmet_execute_get_log_page_ccr()
- [Medium] nvmet: premature CCR cleanup before SGL copy in
  nvmet_execute_get_log_page_ccr()
--

commit bd4c242dbfa9b91e0bb3eb6e57be8d11c6a19d19
Author: Mohamed Khalfella <mkhalfella@purestorage.com>

nvmet: Implement CCR logpage

This commit implements the Cross-Controller Reset (CCR) log page (LID 0x1e)
for the NVMe target and advertises it in the Supported Log Pages log.

> diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
> index 3df9db454db9c..25c320752d2a7 100644
> --- a/drivers/nvme/target/admin-cmd.c
> +++ b/drivers/nvme/target/admin-cmd.c

[ ... ]

> @@ -617,6 +618,46 @@ static void nvmet_execute_get_log_page_features(struct nvmet_req *req)
>  	nvmet_req_complete(req, status);
>  }
>  
> +static void nvmet_execute_get_log_page_ccr(struct nvmet_req *req)
> +{
> +	struct nvmet_ctrl *ctrl = req->sq->ctrl;
> +	struct nvmet_ccr *ccr;
> +	struct nvme_ccr_log *log;
> +	int index = 0;
> +	u16 status;
> +
> +	log = kzalloc_obj(*log);
> +	if (!log) {
> +		status = NVME_SC_INTERNAL;
> +		goto out;
> +	}
> +
> +	mutex_lock(&ctrl->lock);
> +	list_for_each_entry(ccr, &ctrl->ccr_list, entry) {
> +		u8 flags = NVME_CCR_FLAGS_VALIDATED | NVME_CCR_FLAGS_INITIATED;
> +		u8 ccr_status = ccr->ctrl ? NVME_CCR_STATUS_IN_PROGRESS :
> +					NVME_CCR_STATUS_SUCCESS;
> +
> +		log->entries[index].icid = cpu_to_le16(ccr->icid);
> +		log->entries[index].ciu = ccr->ciu;
> +		log->entries[index].acid = cpu_to_le16(0xffff);
> +		log->entries[index].ccrs = ccr_status;
> +		log->entries[index].ccrf = flags;
> +		index++;
> +	}
> +
> +	/* Cleanup completed CCRs if requested */
> +	if (req->cmd->get_log_page.lsp & 0x1)
> +		nvmet_ctrl_cleanup_ccrs(ctrl, false);
> +	mutex_unlock(&ctrl->lock);

[Severity: Medium]
Can nvmet_ctrl_cleanup_ccrs() discard completed CCR entries when the log
page fails to copy to the host?

nvmet_execute_get_log_page() only checks that req->transfer_len matches the
command's NUMDL/NUMDU length.  If a host requests fewer than sizeof(*log)
bytes (for example, reading the 8-byte header) with LSP bit 0 set,
nvmet_ctrl_cleanup_ccrs() frees all completed entries from ctrl->ccr_list
before nvmet_copy_to_sgl() fails with NVME_SC_SGL_INVALID_DATA, so the host
permanently loses those completion records.

> +
> +	log->ne = cpu_to_le16(index);
> +	status = nvmet_copy_to_sgl(req, 0, log, sizeof(*log));

[Severity: High]
Does this leak uninitialized target kernel memory when the host requests
more than sizeof(*log) bytes?

Transports allocate req->sg for req->transfer_len bytes via sgl_alloc()
with GFP_KERNEL (without __GFP_ZERO).  Because nvmet_execute_get_log_page()
only checks that req->transfer_len matches NUMDL/NUMDU and
nvmet_execute_get_log_page_ccr() neither rejects req->transfer_len !=
sizeof(*log) (like nvmet_execute_get_log_page_rmi() and
nvmet_execute_get_log_page_smart()) nor zeroes trailing SGL bytes,
requesting more than 4096 bytes copies sizeof(*log) at offset 0 and returns
the remaining uninitialized scatterlist pages to the host.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261114030454.2836543-1-mkhalfella@purestorage.com?part=4

Re: [PATCH v7 08/17] nvme: Implement cross-controller reset recovery

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] nvme: nvme_find_ctrl_ccr() skips valid controllers on
  unsorted list
--

commit 5d2164548bbd55a6b77ef7780332ce46368dbeb8
Author: Mohamed Khalfella <mkhalfella@purestorage.com>

nvme: Implement cross-controller reset recovery

This commit adds core cross-controller reset (CCR) recovery logic in
nvme_fence_ctrl() and nvme_fence_timeout_ms() to terminate in-flight I/Os
on a failed controller using a healthy controller in the same subsystem.

> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index 4be3ce2e72a78..27c33dc49175e 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -576,6 +576,157 @@ void nvme_cancel_admin_tagset(struct nvme_ctrl *ctrl)
>  }
>  EXPORT_SYMBOL_GPL(nvme_cancel_admin_tagset);
>  
> +static struct nvme_ctrl *nvme_find_ctrl_ccr(struct nvme_ctrl *ictrl,
> +					    u32 min_cntlid)
> +{
> +	struct nvme_subsystem *subsys = ictrl->subsys;
> +	struct nvme_ctrl *ctrl, *sctrl = NULL;
> +	unsigned long flags;
> +	int ccr_used;
> +
> +	mutex_lock(&nvme_subsystems_lock);
> +	list_for_each_entry(ctrl, &subsys->ctrls, subsys_entry) {
> +		if (ctrl->cntlid < min_cntlid)
> +			continue;
> +
> +		spin_lock_irqsave(&ctrl->lock, flags);
> +		if (nvme_ctrl_state(ctrl) != NVME_CTRL_LIVE) {
> +			spin_unlock_irqrestore(&ctrl->lock, flags);
> +			continue;
> +		}
> +
> +		ccr_used = atomic_inc_return(&ctrl->ccr_used);
> +		if (ccr_used > ctrl->ccrl) {
> +			atomic_dec(&ctrl->ccr_used);
> +			spin_unlock_irqrestore(&ctrl->lock, flags);
> +			continue;
> +		}
> +
> +		/*
> +		 * We got a good candidate source controller that is locked and
> +		 * LIVE. However, no guarantee ctrl will not be deleted after
> +		 * ctrl->lock is released. Get a ref of both ctrl and admin_q
> +		 * so they do not disappear until we are done with them.
> +		 */
> +		WARN_ON_ONCE(!blk_get_queue(ctrl->admin_q));
> +		nvme_get_ctrl(ctrl);
> +		spin_unlock_irqrestore(&ctrl->lock, flags);
> +		sctrl = ctrl;
> +		break;
> +	}
> +	mutex_unlock(&nvme_subsystems_lock);
> +	return sctrl;
> +}

[ ... ]

> +unsigned long nvme_fence_ctrl(struct nvme_ctrl *ictrl)
> +{
> +	unsigned long now, deadline, timeout;
> +	struct nvme_ctrl *sctrl;
> +	u32 min_cntlid = 0;
> +	int ret;
> +
> +	timeout = nvme_fence_timeout_ms(ictrl);
> +	dev_info(ictrl->device, "attempting CCR, timeout %lums\n", timeout);
> +
> +	now = jiffies;
> +	deadline = jiffies + msecs_to_jiffies(timeout);
> +	while (time_before(now, deadline)) {
> +		sctrl = nvme_find_ctrl_ccr(ictrl, min_cntlid);
> +		if (!sctrl) {
> +			dev_dbg(ictrl->device,
> +				"failed to find source controller\n");
> +			return deadline - now;
> +		}
> +
> +		ret = nvme_issue_wait_ccr(sctrl, ictrl, deadline);
> +		if (!ret) {
> +			dev_info(ictrl->device, "CCR succeeded using %s\n",
> +				 dev_name(sctrl->device));
> +			nvme_put_ctrl_ccr(sctrl);
> +			return 0;
> +		}
> +
> +		/*
> +		 * CCR command or CCR operation failed on this path.
> +		 * Try another path as long as we have time.
> +		 */
> +		dev_err(ictrl->device, "CCR failed using %s, ret = %d\n",
> +			dev_name(sctrl->device), ret);
> +		min_cntlid = sctrl->cntlid + 1;
> +		nvme_put_ctrl_ccr(sctrl);
> +		now = jiffies;
> +	}

[Severity: Medium]
Can nvme_find_ctrl_ccr() skip valid candidate controllers in subsys->ctrls
when retrying after a failed CCR attempt?

nvme_init_subsystem() appends controllers to subsys->ctrls with
list_add_tail(), while nvmf_connect_admin_queue() assigns a new dynamic
ctrl->cntlid on every connect or reconnect without reordering the list.
Because nvme_find_ctrl_ccr() stops at the first live controller with
ctrl->cntlid >= min_cntlid rather than picking the one with the smallest
matching cntlid, failing on an earlier list entry with a higher cntlid and
advancing min_cntlid to sctrl->cntlid + 1 skips any later entries in
subsys->ctrls that have smaller cntlid values, causing nvme_fence_ctrl()
to give up on CCR early and fall back to time-based recovery.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261114030454.2836543-1-mkhalfella@purestorage.com?part=8

Re: [PATCH v7 07/17] nvme: Introduce FENCING and FENCED controller states

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [High] nvme: premature non-mpath I/O failure in
  nvme_fail_nonready_command()
- [Medium] nvme: live queues re-enabled in nvme_check_ready() when
  deleting from FENCING
--

commit 78ddca4023a9ef3590c04ed05791d87621f68277
Author: Mohamed Khalfella <mkhalfella@purestorage.com>

nvme: Introduce FENCING and FENCED controller states

This commit adds the NVME_CTRL_FENCING and NVME_CTRL_FENCED controller
states along with their state machine transitions, readiness checks,
multipath path availability handling, and sysfs state names.

> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index 38b27d8db0119..4be3ce2e72a78 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c

[ ... ]

> @@ -622,6 +641,8 @@ bool nvme_change_ctrl_state(struct nvme_ctrl *ctrl,
>  		case NVME_CTRL_LIVE:
>  		case NVME_CTRL_RESETTING:
>  		case NVME_CTRL_CONNECTING:
> +		case NVME_CTRL_FENCING:
> +		case NVME_CTRL_FENCED:
>  			changed = true;
>  			fallthrough;
>  		default:

[Severity: Medium]
When a controller in NVME_CTRL_FENCING or NVME_CTRL_FENCED transitions
to NVME_CTRL_DELETING, can new I/O be dispatched onto its still-live
queues while nvme_stop_ctrl() waits for fencing to finish?

While in NVME_CTRL_FENCING, the transport queues remain live and new
requests are blocked only because __nvme_check_ready() and
nvme_path_get_state() reject NVME_CTRL_FENCING and NVME_CTRL_FENCED.
In nvme_do_delete_ctrl(), ctrl->state stays NVME_CTRL_DELETING across
nvme_stop_ctrl() until nvme_remove_namespaces() sets
NVME_CTRL_DELETING_NOIO, and both nvme_check_ready() and
nvme_path_get_state() treat NVME_CTRL_DELETING on a live queue as ready
and enabled:

drivers/nvme/host/nvme.h:nvme_check_ready() {
	if (likely(state == NVME_CTRL_LIVE))
		return true;
	if (ctrl->ops->flags & NVME_F_FABRICS && state == NVME_CTRL_DELETING)
		return queue_live;
	return __nvme_check_ready(ctrl, rq, queue_live, state);
}

drivers/nvme/host/multipath.c:nvme_path_get_state() {
	if (state != NVME_CTRL_LIVE && state != NVME_CTRL_DELETING)
		return NVME_PATH_DISABLED_CTRL_DOWN;
}

As a result, deleting a fencing controller re-enables multipath
selection and allows new requests to be queued to the broken controller
throughout the fence wait instead of failing over immediately.

> @@ -786,6 +807,8 @@ blk_status_t nvme_fail_nonready_command(struct nvme_ctrl *ctrl,
>  
>  	if (state != NVME_CTRL_DELETING_NOIO &&
>  	    state != NVME_CTRL_DELETING &&
> +	    state != NVME_CTRL_FENCING &&
> +	    state != NVME_CTRL_FENCED &&
>  	    state != NVME_CTRL_DEAD &&
>  	    !test_bit(NVME_CTRL_FAILFAST_EXPIRED, &ctrl->flags) &&
>  	    !blk_noretry_request(rq) && !(rq->cmd_flags & REQ_NVME_MPATH))

[Severity: High]
Does excluding NVME_CTRL_FENCING and NVME_CTRL_FENCED here cause
non-multipath I/O to fail immediately with BLK_STS_TRANSPORT during
fencing?

Multipath requests (REQ_NVME_MPATH or REQ_FAILFAST_TRANSPORT) already
bypass the BLK_STS_RESOURCE return via the trailing checks and call
nvme_host_path_error(rq), so excluding NVME_CTRL_FENCING and
NVME_CTRL_FENCED here only changes the behavior for normal
non-multipath requests.  Because the queues are not quiesced during
NVME_CTRL_FENCING, nvme_host_path_error() -> nvme_complete_rq() ->
nvme_decide_disposition() returns RETRY, and nvme_retry_req() requeues
the request with zero delay until nvme_max_retries is exhausted in a
tight loop, after which __nvme_end_req() logs an uninitialized
nr->cmd->common.opcode in nvme_log_error() (since nvme_check_ready()
failed before nvme_setup_cmd()) and fails the I/O to the caller.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261114030454.2836543-1-mkhalfella@purestorage.com?part=7

Re: [PATCH v7 09/17] nvme: Implement cross-controller reset completion

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [High] nvme: UAF and deadlock in nvme_ccr_work() when controller is
  not LIVE
--

commit ccb451fbdaf89eaa6e4be835d2b43da8b0854559
Author: Mohamed Khalfella <mkhalfella@purestorage.com>

nvme: Implement cross-controller reset completion

This commit adds ctrl->ccr_work and handles NVME_AER_NOTICE_CCR_COMPLETED
events by reading the NVME_LOG_CCR log page and completing matching entries on
ctrl->ccr_list.

> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index 27c33dc49175e..54b83092a9e8f 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
[ ... ]
> @@ -5060,6 +5061,48 @@ static void nvme_get_fw_slot_info(struct nvme_ctrl *ctrl)
>  	kfree(log);
>  }
>  
> +static void nvme_ccr_work(struct work_struct *work)
> +{
> +	struct nvme_ctrl *ctrl = container_of(work, struct nvme_ctrl, ccr_work);
> +	struct nvme_ccr_entry *ccr;
> +	struct nvme_ccr_log_entry *entry;
> +	struct nvme_ccr_log *log;
> +	int num_entries, ret, i;
> +	unsigned long flags;
> +
> +	log = kmalloc_obj(*log);
> +	if (!log)
> +		return;
> +
> +	ret = nvme_get_log(ctrl, 0, NVME_LOG_CCR, 0x01,
> +			   0x00, log, sizeof(*log), 0);

[Severity: High]
Can nvme_ccr_work() submit a Get Log Page command to a destroyed or quiesced
ctrl->admin_q when the controller is no longer live?

During controller deletion, nvme_do_delete_ctrl() calls nvme_stop_ctrl()
before ctrl->ops->delete_ctrl() stops the admin queue, and
nvme_uninit_ctrl() runs only after nvme_remove_admin_tag_set() has already
destroyed ctrl->admin_q and freed ctrl->admin_tagset.  If an in-flight AEN
completes with NVME_AER_NOTICE_CCR_COMPLETED after nvme_stop_ctrl() cancels
ctrl->ccr_work, nvme_ccr_work() is re-queued and can access ctrl->admin_q
while or after it is destroyed:

CPU0                                  CPU1
nvme_do_delete_ctrl()
  nvme_stop_ctrl()
    cancel_work_sync(&ctrl->ccr_work)
                                      nvme_complete_async_event()
                                        nvme_handle_aen_notice()
                                          queue_work(nvme_wq, &ctrl->ccr_work)
  ctrl->ops->delete_ctrl()
    ...
    nvme_remove_admin_tag_set()
      blk_mq_destroy_queue(ctrl->admin_q)
      blk_mq_free_tag_set(ctrl->admin_tagset)
                                      nvme_ccr_work()
                                        nvme_get_log(ctrl, ...)
                                          <-- use-after-free on admin_q
  nvme_uninit_ctrl()
    cancel_work_sync(&ctrl->ccr_work)

Because nvme_ccr_work() does not check nvme_ctrl_state(ctrl) ==
NVME_CTRL_LIVE, it can also run while ctrl->admin_q is quiesced during reset
or transport error recovery; nvme_get_log() then blocks in blk_execute_rq()
waiting for the queue to be unquiesced, and the next nvme_stop_ctrl() ->
cancel_work_sync(&ctrl->ccr_work) deadlocks before teardown can unquiesce
ctrl->admin_q.

Checking nvme_ctrl_state(ctrl) != NVME_CTRL_LIVE at the start of
nvme_ccr_work() (like nvme_ana_work() and nvme_scan_work()) and cancelling
ctrl->ccr_work in nvme_remove_admin_tag_set() before destroying
ctrl->admin_q would avoid both issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261114030454.2836543-1-mkhalfella@purestorage.com?part=9

Re: [PATCH v7 03/17] nvmet: Implement CCR nvme command

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] nvmet: TOCTOU race between nvmet_ctrl_find_get_ccr() and
  nvmet_start_ctrl()
--

commit 8afa887480a9e0f0751311107895be0ad6ed159f
Author: Mohamed Khalfella <mkhalfella@purestorage.com>

nvmet: Implement CCR nvme command

This commit implements the Cross-Controller Reset admin command for the
NVMe target, allowing a source controller to look up and trigger a fatal
error on an impacted controller matching the specified controller ID, ciu,
and cirn.

> diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
> index 5710ae53eb83a..3df9db454db9c 100644
> --- a/drivers/nvme/target/admin-cmd.c
> +++ b/drivers/nvme/target/admin-cmd.c
[ ... ]
> @@ -1629,6 +1630,78 @@ void nvmet_execute_keep_alive(struct nvmet_req *req)
>  	nvmet_req_complete(req, status);
>  }
>  
> +void nvmet_execute_cross_ctrl_reset(struct nvmet_req *req)
> +{
[ ... ]
> +	/* Find and get impacted controller */
> +	ictrl = nvmet_ctrl_find_get_ccr(sctrl->subsys, sctrl->hostnqn,
> +					cmd->ccr.ciu, cntlid,
> +					le64_to_cpu(cmd->ccr.cirn));
[ ... ]
> +out_unlock:
> +	mutex_unlock(&sctrl->lock);
> +	if (status == NVME_SC_SUCCESS)
> +		nvmet_ctrl_fatal_error(ictrl);
> +	nvmet_ctrl_put(ictrl);
> +out:
> +	nvmet_req_complete(req, status);
> +}
[ ... ]
> diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
> index 9c9ad4d9b292d..86018e0fb1586 100644
> --- a/drivers/nvme/target/core.c
> +++ b/drivers/nvme/target/core.c
[ ... ]
> @@ -1432,6 +1446,7 @@ static void nvmet_start_ctrl(struct nvmet_ctrl *ctrl)
>  	if (!nvmet_is_disc_subsys(ctrl->subsys)) {
>  		ctrl->ciu = ((u8)(ctrl->ciu + 1)) ? : 1;
>  		ctrl->cirn = get_random_u64() ? : 1;
> +		nvmet_ctrl_cleanup_ccrs(ctrl, false);
>  	}
>  	ctrl->csts = NVME_CSTS_RDY;
>  
> @@ -1537,6 +1552,42 @@ struct nvmet_ctrl *nvmet_ctrl_find_get(const char *subsysnqn,
>  	return ctrl;
>  }
>  
> +struct nvmet_ctrl *nvmet_ctrl_find_get_ccr(struct nvmet_subsys *subsys,
> +					   const char *hostnqn, u8 ciu,
> +					   u16 cntlid, u64 cirn)
> +{
> +	struct nvmet_ctrl *ctrl, *ictrl = NULL;
> +	bool found = false;
> +
> +	mutex_lock(&subsys->lock);
> +	list_for_each_entry(ctrl, &subsys->ctrls, subsys_entry) {
> +		if (ctrl->cntlid != cntlid)
> +			continue;
> +		if (strncmp(ctrl->hostnqn, hostnqn, NVMF_NQN_SIZE))
> +			continue;
> +
> +		/*
> +		 * Enabling the controller regenerates ciu and cirn under
> +		 * ctrl->lock. Hold the lock so that a controller becoming
> +		 * ready concurrently is matched against either the old or
> +		 * the new instance, never a mix of both.
> +		 */
> +		mutex_lock(&ctrl->lock);
> +		if (ctrl->ciu == ciu && ctrl->cirn == cirn)
> +			found = true;
> +		mutex_unlock(&ctrl->lock);

[Severity: Medium]
Can nvmet_execute_cross_ctrl_reset() race with nvmet_start_ctrl() and
trigger a fatal error on a newly enabled controller instance?

nvmet_ctrl_find_get_ccr() drops ctrl->lock right after matching ctrl->ciu
and ctrl->cirn.  Before nvmet_execute_cross_ctrl_reset() calls
nvmet_ctrl_fatal_error(ictrl), a concurrent nvmet_update_cc() can enable
ictrl and advance ictrl->ciu and ictrl->cirn:

CPU0                              CPU1
nvmet_execute_cross_ctrl_reset()
  nvmet_ctrl_find_get_ccr()
    mutex_lock(&ictrl->lock)
    /* matches ciu and cirn */
    mutex_unlock(&ictrl->lock)
                                  nvmet_update_cc()
                                    mutex_lock(&ictrl->lock)
                                    nvmet_start_ctrl()
                                      ictrl->ciu = ...;
                                      ictrl->cirn = ...;
                                    mutex_unlock(&ictrl->lock)
  nvmet_ctrl_fatal_error(ictrl)   <-- kills the new instance

If the interleaving goes the other way and nvmet_ctrl_fatal_error(ictrl)
runs just before nvmet_start_ctrl(), nvmet_start_ctrl() overwrites
ictrl->csts with the ready state, clearing the fatal status bit while
ictrl->fatal_err_work is already queued.

In addition, when a controller is re-enabled in-place via
nvmet_start_ctrl() rather than freed in nvmet_ctrl_free(),
nvmet_start_ctrl() only cleans up entries on ctrl->ccr_list where ctrl was
the source controller, leaving pending entries on other controllers where
ctrl was the impacted controller active with a non-null ccr->ctrl pointer.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261114030454.2836543-1-mkhalfella@purestorage.com?part=3

Re: [PATCH v7 13/17] nvme-fc: perform error recovery directly from ioerr_work

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [High] nvme-fc: race between nvme_fc_ctrl_ioerr_work() and controller
  deletion
- [Low] nvme-fc: unused forward declaration of
  __nvme_fc_abort_outstanding_ios()
--

commit f47f39335cf25a25520dc165768b29ed4e1e01c4
Author: Mohamed Khalfella <mkhalfella@purestorage.com>

nvme-fc: perform error recovery directly from ioerr_work

This commit replaces nvme_fc_error_recovery() with direct association
teardown, controller stop, and reconnect scheduling inside
nvme_fc_ctrl_ioerr_work().

> diff --git a/drivers/nvme/host/fc.c b/drivers/nvme/host/fc.c
> index c4603be381445..0ba621649afed 100644
> --- a/drivers/nvme/host/fc.c
> +++ b/drivers/nvme/host/fc.c
> @@ -229,6 +229,8 @@ static struct device *fc_udev_device;
>  static void nvme_fc_complete_rq(struct request *rq);
>  static void nvme_fc_start_ioerr_recovery(struct nvme_fc_ctrl *ctrl,
>  					 char *errmsg);
> +static void __nvme_fc_abort_outstanding_ios(struct nvme_fc_ctrl *ctrl,
> +					    bool start_queues);

[Severity: Low]
This isn't a bug, but nothing calls __nvme_fc_abort_outstanding_ios()
before its definition now that nvme_fc_error_recovery() is removed, so this
forward declaration is unused.

[ ... ]

> @@ -1880,7 +1883,23 @@ nvme_fc_ctrl_ioerr_work(struct work_struct *work)
>  	struct nvme_fc_ctrl *ctrl =
>  			container_of(work, struct nvme_fc_ctrl, ioerr_work);
>  
> -	nvme_fc_error_recovery(ctrl, "transport detected io error");
> +	nvme_stop_keep_alive(&ctrl->ctrl);
> +	flush_work(&ctrl->ctrl.async_event_work);
> +
> +	/* will block while waiting for io to terminate */
> +	nvme_fc_delete_association(ctrl);
> +	nvme_stop_ctrl(&ctrl->ctrl);
> +
> +	/* Do not reconnect if controller is being deleted */
> +	if (!nvme_change_ctrl_state(&ctrl->ctrl, NVME_CTRL_CONNECTING))
> +		return;
> +
> +	if (ctrl->rport->remoteport.port_state == FC_OBJSTATE_ONLINE) {
> +		queue_delayed_work(nvme_wq, &ctrl->connect_work, 0);
> +		return;
> +	}
> +
> +	nvme_fc_reconnect_or_delete(ctrl, -ENOTCONN);
>  }

[Severity: High]
Can nvme_fc_ctrl_ioerr_work() race with controller deletion and re-arm
ctrl->connect_work after nvme_fc_delete_ctrl() has already canceled it?

Previously, error recovery called nvme_reset_ctrl(), and
nvme_do_delete_ctrl() flushed ctrl->reset_work before calling
nvme_stop_ctrl(), nvme_remove_namespaces(), and ctrl->ops->delete_ctrl().
Unlike rdma and tcp, nvme_fc_ctrl_ops does not implement .stop_ctrl to
flush ioerr_work, and nvme_fc_delete_ctrl() cancels connect_work before
canceling ioerr_work:

drivers/nvme/host/fc.c:nvme_fc_delete_ctrl() {
	...
	cancel_delayed_work_sync(&ctrl->connect_work);
	...
	nvme_fc_delete_association(ctrl);
	cancel_work_sync(&ctrl->ioerr_work);
	...
}

If controller deletion starts right after nvme_fc_ctrl_ioerr_work() moves
the controller to NVME_CTRL_CONNECTING, connect_work can be queued after
cancel_delayed_work_sync(&ctrl->connect_work) has already completed:

CPU0                                    CPU1
nvme_fc_ctrl_ioerr_work()
  ...
  nvme_change_ctrl_state(...,
    NVME_CTRL_CONNECTING)
                                        nvme_delete_ctrl()
                                          nvme_change_ctrl_state(...,
                                            NVME_CTRL_DELETING)
                                          ...
                                          nvme_do_delete_ctrl()
                                            ...
                                            nvme_fc_delete_ctrl()
                                              cancel_delayed_work_sync(
                                                &ctrl->connect_work)
                                              ...
                                              cancel_work_sync(
                                                &ctrl->ioerr_work)
  queue_delayed_work(nvme_wq,
    &ctrl->connect_work, 0)
                                              nvme_remove_io_tag_set()
                                              nvme_remove_admin_tag_set()
                                            nvme_uninit_ctrl()
                                        nvme_fc_connect_ctrl_work()
                                          nvme_fc_create_association()
                                            <-- use-after-free

This leaves connect_work running nvme_fc_create_association() while the
tag sets are torn down and struct nvme_fc_ctrl is freed.

In addition, because nvme_do_delete_ctrl() does not wait for ioerr_work
before calling nvme_remove_namespaces(), nvme_fc_delete_association() in
ioerr_work can run concurrently with nvme_remove_namespaces(), racing on
quiescing and unquiescing the I/O queues while scan_work is flushed and
hardware queues are deleted.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261114030454.2836543-1-mkhalfella@purestorage.com?part=13

Re: [PATCH v7 14/17] nvme-fc: Use CCR to recover controller that hits an error

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] nvme-fc: race between nvme_fc_start_ioerr_recovery() and
  controller delete
--

commit f16da5f8edc4e397a5fc84e9cd9ede4c82bfa5d8
Author: Mohamed Khalfella <mkhalfella@purestorage.com>

nvme-fc: Use CCR to recover controller that hits an error

This commit transitions LIVE NVMe/FC controllers to FENCING on I/O errors
and runs Cross-Controller Reset (CCR) via fencing_work before proceeding to
RESETTING.

> diff --git a/drivers/nvme/host/fc.c b/drivers/nvme/host/fc.c
> index 0ba621649afed..095adc9011ccd 100644
> --- a/drivers/nvme/host/fc.c
> +++ b/drivers/nvme/host/fc.c
[ ... ]
> @@ -1921,11 +1949,20 @@ char *nvme_fc_io_getuuid(struct nvmefc_fcp_req *req)
>  EXPORT_SYMBOL_GPL(nvme_fc_io_getuuid);
>  
>  /*
> - * Start recovery only if this caller can claim it with a state change.
> + * Start recovery only if this caller can claim it with a state change:
> + * FENCING from LIVE, or RESETTING otherwise.
>   */
>  static void
>  nvme_fc_start_ioerr_recovery(struct nvme_fc_ctrl *ctrl, char *errmsg)
>  {
> +	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);
> @@ -2439,6 +2476,12 @@ nvme_fc_ctrl_get(struct nvme_fc_ctrl *ctrl)
>  	return kref_get_unless_zero(&ctrl->ref);
>  }
>  
> +static void
> +nvme_fc_stop_ctrl(struct nvme_ctrl *nctrl)
> +{
> +	flush_work(&to_fc_ctrl(nctrl)->fencing_work);
> +}

[Severity: Medium]
Can ctrl->fencing_work be queued after nvme_fc_stop_ctrl() has already
flushed it if controller deletion races with nvme_fc_start_ioerr_recovery()?

nvme_change_ctrl_state() allows transitioning from NVME_CTRL_FENCING to
NVME_CTRL_DELETING, and nvme_fc_start_ioerr_recovery() drops ctrl->ctrl.lock
before calling dev_warn() and queue_work():

CPU0                                  CPU1
nvme_fc_start_ioerr_recovery()
  nvme_change_ctrl_state(...,
    NVME_CTRL_FENCING)
                                      nvme_delete_ctrl()
                                        nvme_change_ctrl_state(...,
                                          NVME_CTRL_DELETING)
                                      nvme_do_delete_ctrl()
                                        nvme_stop_ctrl()
                                          nvme_fc_stop_ctrl()
                                            flush_work(&ctrl->fencing_work)
  dev_warn(...)
  queue_work(nvme_wq,
    &ctrl->fencing_work)
                                        ...
                                        nvme_uninit_ctrl()
                                          nvme_put_ctrl() -> frees ctrl
                                      nvme_fc_fencing_work() <-- use-after-free

Because flush_work() returns immediately when fencing_work has not been
queued yet, nothing in the remainder of nvme_do_delete_ctrl() stops
nvme_fc_fencing_work() from running concurrently with nvme_fc_delete_ctrl()
or after struct nvme_fc_ctrl is freed.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261114030454.2836543-1-mkhalfella@purestorage.com?part=14

Re: [PATCH v7 10/17] nvme-tcp: Use CCR to recover controller that hits an error

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 5 potential
issues to consider:
- [High] nvme-tcp: premature completion of in-flight requests in
  nvme_tcp_try_send()
- [High] nvme-tcp: non-multipath I/O retry exhaustion during
  NVME_CTRL_FENCING
- [High] nvme-tcp: race in nvme_tcp_fencing_work() skips err_work on
  reset
- [High] nvme-tcp: race between nvme_tcp_error_recovery() and
  nvme_tcp_stop_ctrl()
- [Medium] nvme-tcp: race in nvme_tcp_error_recovery() bypasses
  controller fencing
--

commit abb8e00ab3d00deda1ad0650e2f1521537edf566
Author: Mohamed Khalfella <mkhalfella@purestorage.com>

nvme-tcp: Use CCR to recover controller that hits an error

This commit transitions a live nvme-tcp controller to NVME_CTRL_FENCING on
error to attempt cross-controller reset via fencing_work before proceeding
to NVME_CTRL_RESETTING, while holding timed-out requests until fencing
completes.

> diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
> index 7ecc6c007f8d4..244c6c50cf990 100644
> --- a/drivers/nvme/host/tcp.c
> +++ b/drivers/nvme/host/tcp.c
[ ... ]
> @@ -651,6 +652,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;
> +	}

[Severity: High]
Does leaving the queues unquiesced while ctrl is in NVME_CTRL_FENCING
cause non-multipath requests to exhaust their retries and fail
immediately?

When a request is dispatched to nvme_tcp_queue_rq() during
NVME_CTRL_FENCING or NVME_CTRL_FENCED, nvme_check_ready() returns false
and nvme_tcp_queue_rq() calls nvme_fail_nonready_command() before
nvme_tcp_setup_cmd_pdu():

drivers/nvme/host/core.c:nvme_fail_nonready_command() {
	...
	if (state != NVME_CTRL_DELETING_NOIO &&
	    state != NVME_CTRL_DELETING &&
	    state != NVME_CTRL_FENCING &&
	    state != NVME_CTRL_FENCED &&
	    state != NVME_CTRL_DEAD &&
	    !test_bit(NVME_CTRL_FAILFAST_EXPIRED, &ctrl->flags) &&
	    !blk_noretry_request(rq) && !(rq->cmd_flags & REQ_NVME_MPATH))
		return BLK_STS_RESOURCE;

	if (!(rq->rq_flags & RQF_DONTPREP))
		nvme_clear_nvme_request(rq);

	return nvme_host_path_error(rq);
}

Because NVME_CTRL_FENCING and NVME_CTRL_FENCED are excluded from returning
BLK_STS_RESOURCE, a non-multipath request (without REQ_NVME_MPATH) sets
RQF_DONTPREP and completes with NVME_SC_HOST_PATH_ERROR.
nvme_decide_disposition() then chooses RETRY and nvme_retry_req() kicks
the requeue list with zero delay on the still-unquiesced queue.  The
request loops through nvme_tcp_queue_rq() five times in a tight loop until
retries reach nvme_max_retries, logs an uninitialized or stale command in
nvme_log_error(), and fails with BLK_STS_TRANSPORT instead of waiting for
controller recovery.

> +
>  	if (!nvme_change_ctrl_state(ctrl, NVME_CTRL_RESETTING))
>  		return;

[Severity: Medium]
Can this fallback nvme_change_ctrl_state(ctrl, NVME_CTRL_RESETTING) call
transition a newly NVME_CTRL_LIVE controller directly to
NVME_CTRL_RESETTING and bypass fencing?

In nvme-tcp, ctrl->state leaves NVME_CTRL_NEW before any queues or sockets
exist, so NVME_CTRL_RESETTING can only be entered from NVME_CTRL_LIVE or
NVME_CTRL_FENCED.  If a socket callback triggers nvme_tcp_error_recovery()
while nvme_tcp_setup_ctrl() is finishing connection setup, the two state
changes can interleave:

CPU0                                  CPU1
nvme_tcp_error_recovery()             nvme_tcp_setup_ctrl()
  nvme_change_ctrl_state(...,
    NVME_CTRL_FENCING) -> false
    (state is NVME_CTRL_CONNECTING)
                                        nvme_change_ctrl_state(...,
                                          NVME_CTRL_LIVE) -> true
                                          nvme_kick_requeue_lists()
  nvme_change_ctrl_state(...,
    NVME_CTRL_RESETTING) -> true
  queue_work(..., &err_work)          <-- skips fencing_work

Requests dispatched as the controller became NVME_CTRL_LIVE are then
canceled by err_work and retried on another path without fencing the
controller first.  Removing the NVME_CTRL_RESETTING fallback from
nvme_tcp_error_recovery() avoids this race, since nvme-tcp only needs to
trigger error recovery from NVME_CTRL_LIVE.

> @@ -2767,6 +2774,22 @@ static void nvme_tcp_reconnect_ctrl_work(struct work_struct *work)
>  	nvme_tcp_reconnect_or_remove(ctrl, ret);
>  }
>  
> +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)
> +		dev_info(ctrl->device, "CCR failed, starting error recovery\n");
> +
> +	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);
> +}

[Severity: High]
Can another caller claim the NVME_CTRL_FENCED to NVME_CTRL_RESETTING
transition between these two nvme_change_ctrl_state() calls and prevent
tcp_ctrl->err_work from running?

nvme_change_ctrl_state(ctrl, NVME_CTRL_FENCED) sets ctrl->state to
NVME_CTRL_FENCED, calls wake_up_all(&ctrl->state_wq), and drops ctrl->lock
before the second nvme_change_ctrl_state(ctrl, NVME_CTRL_RESETTING) call:

CPU0                                  CPU1
nvme_tcp_fencing_work()               nvmf_subsystem_reset()
  nvme_change_ctrl_state(...,           nvme_wait_reset(ctrl)
    NVME_CTRL_FENCED)
    wake_up_all(&ctrl->state_wq)
                                          nvme_change_ctrl_state(...,
                                            NVME_CTRL_RESETTING) -> true
  nvme_change_ctrl_state(...,
    NVME_CTRL_RESETTING) -> false
  (err_work not queued)
                                        reg_write32() -> fails on dead sock
                                        returns error  <-- stuck in RESETTING

When nvmf_subsystem_reset() wins the transition, reg_write32() fails over
the broken transport before nvme_try_sched_reset(), leaving the controller
permanently wedged in NVME_CTRL_RESETTING with neither err_work nor
reset_work scheduled.  A concurrent nvme_reset_ctrl() (such as from
nvme_keep_alive_work(), nvme_anatt_timeout(), or sysfs reset_controller)
can likewise claim NVME_CTRL_RESETTING here and queue reset_work instead
of err_work, which attempts nvme_disable_ctrl() over the dead admin socket
and leaves the queues quiesced if nvme_tcp_setup_ctrl() fails.

[ ... ]

> @@ -2842,6 +2865,7 @@ static void nvme_reset_ctrl_work(struct work_struct *work)
>  
>  static void nvme_tcp_stop_ctrl(struct nvme_ctrl *ctrl)
>  {
> +	flush_work(&to_tcp_ctrl(ctrl)->fencing_work);
>  	flush_work(&to_tcp_ctrl(ctrl)->err_work);
>  	cancel_delayed_work_sync(&to_tcp_ctrl(ctrl)->connect_work);
>  }

[Severity: High]
Can nvme_tcp_stop_ctrl() run flush_work(&to_tcp_ctrl(ctrl)->fencing_work)
before nvme_tcp_error_recovery() calls queue_work(), allowing fencing_work
to run during and after controller deletion?

In nvme_do_delete_ctrl(), nvme_stop_ctrl() runs before
ctrl->ops->delete_ctrl() stops the sockets and queues, and
nvme_change_ctrl_state(ctrl, NVME_CTRL_DELETING) is permitted from
NVME_CTRL_FENCING:

CPU0                                  CPU1
nvme_tcp_error_recovery()             nvme_do_delete_ctrl()
  nvme_change_ctrl_state(...,
    NVME_CTRL_FENCING) -> true
                                        nvme_change_ctrl_state(...,
                                          NVME_CTRL_DELETING) -> true
                                        nvme_stop_ctrl()
                                          nvme_tcp_stop_ctrl()
                                            flush_work(&fencing_work)
  dev_warn(...)
  queue_work(nvme_wq, &fencing_work)
                                        ...
                                        nvme_free_ctrl()
                                          kfree(ctrl)
nvme_tcp_fencing_work()
  nvme_fence_ctrl(ctrl)               <-- use-after-free

Because flush_work() returns immediately when fencing_work has not been
queued yet, fencing_work is never flushed again before the controller is
freed.

[ ... ]

> @@ -2944,17 +2968,19 @@ static enum blk_eh_timer_return nvme_tcp_timeout(struct request *rq)
[ ... ]
>  	/*
>  	 * LIVE state should trigger the normal error recovery which will
> -	 * handle completing this request.
> +	 * handle completing this request. In FENCING state the request must
> +	 * not be completed before fencing has finished, so just rearm the
> +	 * timer and let the error recovery pick it up.
>  	 */
>  	nvme_tcp_error_recovery(ctrl);
>  	return BLK_EH_RESET_TIMER;

[Severity: High]
While nvme_tcp_timeout() holds timed-out requests during NVME_CTRL_FENCING,
does nvme_tcp_try_send() still complete in-flight requests immediately on
a socket send error before fencing finishes?

When a controller enters NVME_CTRL_FENCING, its queues and queue->io_work
remain active until fencing_work finishes and queues err_work.  If an
in-flight write request has already sent its command capsule (or received
an R2T) and sock_sendmsg() fails while sending H2C data,
nvme_tcp_try_send() fails and completes the request right away:

drivers/nvme/host/tcp.c:nvme_tcp_try_send() {
	...
	} else if (ret < 0) {
		dev_err(queue->ctrl->ctrl.device,
			"failed to send request %d\n", ret);
		nvme_tcp_fail_request(queue->request);
		nvme_tcp_done_send_req(queue);
	}
	...
}

nvme_tcp_fail_request() completes the request with
NVME_SC_HOST_PATH_ERROR, which triggers multipath failover while the
controller is still in NVME_CTRL_LIVE or NVME_CTRL_FENCING and before
nvme_fence_ctrl() has terminated the command on the target.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261114030454.2836543-1-mkhalfella@purestorage.com?part=10

Re: [PATCH v7 12/17] nvme-fc: start error recovery instead of aborting timed out IOs

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 4 potential
issues to consider:
- [High] nvme-fc: ignored errors in CONNECTING state allow broken
  association to go LIVE
- [Medium] nvme-fc: nvme_fc_timeout() leaves queue live after non-LIVE
  command abort
- [Medium] nvme-fc: missing abort drain before queue deletion in connect
  error paths
- [Low] nvme-fc: missing Fixes trailer for commit 52793d62a696 in commit
  message
--

commit 0a8f4b079afa7a27de6edf488747423c0c7e82ab
Author: Mohamed Khalfella <mkhalfella@purestorage.com>

> nvme-fc: start error recovery instead of aborting timed out IOs
>
> Aborts issued from the timeout handler run outside the FCCTRL_TERMIO
> window, so they are not counted in ctrl->iocnt and
> nvme_fc_delete_association() does not wait for them. The association
> can be torn down while the LLDD is still working on the abort.
>
> Instead of aborting the timed out command, reset the controller like
> the other fabrics transports do. The association teardown aborts the
> outstanding IOs, counts the aborts and waits for them to complete.
>
> Count every abort in ctrl->iocnt, not only the ones issued while
> FCCTRL_TERMIO is set, and do not reset the count when the teardown
> starts. An abort issued before the teardown is then waited for as well.
>
> Only a LIVE controller starts error recovery. In every other state the
> association is being set up, being torn down or already gone, so abort
> the timed out command from the timeout handler instead and return
> BLK_EH_DONE. The command completes through nvme_fc_fcpio_done() once
> the LLDD has finished the abort, and the abort is counted so a teardown
> that follows waits for it. This matters for a controller being deleted.
> The delete path drains the IOs itself and tears the association down in
> nvme_fc_delete_ctrl() once the drain is done, and DELETING cannot move
> to RESETTING, so without the abort a timed out IO would hang the drain
> against a dead target.
>
> The new nvme_fc_start_ioerr_recovery() is the common entry point for IO
> timeouts, transport errors reported by nvme_fc_fcpio_done(), Disconnect
> LS and connectivity loss. It moves the controller to RESETTING and
> queues ioerr_work only if that succeeds, so nothing is ever queued on a
> controller that is CONNECTING or being deleted. A Disconnect LS or a
> transport error received while CONNECTING is resolved when the inflight
> request fails, or times out and is aborted.
>
> ioerr_work still calls nvme_fc_error_recovery(), which returns without
> doing anything for a controller that is not LIVE. With the controller
> already in RESETTING when the work runs, no recovery takes place yet
> and the controller stays in RESETTING. The next patch moves the
> recovery into ioerr_work itself and completes the change.
>
> Link: https://lore.kernel.org/all/20250529214928.2112990-1-mkhalfella@purestorage.com/
> Signed-off-by: Mohamed Khalfella <mkhalfella@purestorage.com>

[Severity: Low]
Should this bug fix include a Fixes tag for the commit that gated
ctrl->iocnt accounting on the FCCTRL_TERMIO flag:

  Fixes: 52793d62a696 ("nvme-fc: fix io timeout to abort I/O")

> diff --git a/drivers/nvme/host/fc.c b/drivers/nvme/host/fc.c
> index 0b9ff50d26ff3..c4603be381445 100644
> --- a/drivers/nvme/host/fc.c
> +++ b/drivers/nvme/host/fc.c
[ ... ]
> @@ -1569,7 +1571,8 @@ nvme_fc_ls_disconnect_assoc(struct nvmefc_ls_rcv_op *lsop)
>  	 */
>  
>  	/* fail the association */
> -	nvme_fc_error_recovery(ctrl, "Disconnect Association LS received");
> +	nvme_fc_start_ioerr_recovery(ctrl,
> +				     "Disconnect Association LS received");
>  
>  	/* release the reference taken by nvme_fc_match_disconn_ls() */
>  	nvme_fc_ctrl_put(ctrl);
> @@ -1813,11 +1816,18 @@ __nvme_fc_abort_op(struct nvme_fc_ctrl *ctrl, struct nvme_fc_fcp_op *op)
>  	unsigned long flags;
>  	int opstate;
>  
> +	/*
> +	 * Every abort is counted, not only those issued by the association
> +	 * teardown: the teardown waits for iocnt to drop to zero before it
> +	 * deletes the hw queues, and an abort issued earlier, by
> +	 * nvme_fc_timeout() while deleting or by the CONNECTING recovery
> +	 * path, must not still be in progress in the LLDD then.
> +	 */
>  	spin_lock_irqsave(&ctrl->lock, flags);
>  	opstate = atomic_xchg(&op->state, FCPOP_STATE_ABORTED);
> -	if (opstate != FCPOP_STATE_ACTIVE)
> +	if (opstate != FCPOP_STATE_ACTIVE) {
>  		atomic_set(&op->state, opstate);
> -	else if (test_bit(FCCTRL_TERMIO, &ctrl->flags)) {
> +	} else {
>  		op->flags |= FCOP_FLAGS_TERMIO;
>  		ctrl->iocnt++;
>  	}

[Severity: Medium]
Can the connect failure unwind paths in nvme_fc_create_association(),
nvme_fc_create_io_queues(), and nvme_fc_recreate_io_queues() delete
hardware queues while an abort or asynchronous request is still active in
the lower-level driver?

Previously, a transport error or Disconnect Association link service while
connecting ran nvme_fc_error_recovery() ->
__nvme_fc_abort_outstanding_ios(), which quiesced the queues, aborted busy
requests on both tagsets, and waited for completion.  With
nvme_fc_start_ioerr_recovery() becoming a no-op while connecting and
nvme_fc_timeout() incrementing ctrl->iocnt for timed-out commands, an
in-flight keep-alive command (started by nvme_init_ctrl_finish()) or an
abort from nvme_fc_timeout() can still be running when a later connect step
fails.  The error labels in nvme_fc_create_io_queues() and
nvme_fc_create_association() call nvme_fc_delete_hw_io_queues() and
__nvme_fc_delete_hw_queue() and clear the active association flag without
aborting outstanding requests or waiting on ctrl->ioabort_wait for
ctrl->iocnt to reach zero:

drivers/nvme/host/fc.c:nvme_fc_create_association() {
    ...
out_delete_hw_queue:
	__nvme_fc_delete_hw_queue(ctrl, &ctrl->queues[0], 0);
out_free_queue:
	nvme_fc_free_queue(&ctrl->queues[0]);
	clear_bit(ASSOC_ACTIVE, &ctrl->flags);
    ...
}

Because the active association flag is cleared there, even a subsequent
nvme_fc_delete_association() returns immediately without waiting for
ctrl->iocnt to drop to zero.

[ ... ]

> @@ -1892,6 +1901,19 @@ char *nvme_fc_io_getuuid(struct nvmefc_fcp_req *req)
>  }
>  EXPORT_SYMBOL_GPL(nvme_fc_io_getuuid);
>  
> +/*
> + * Start recovery only if this caller can claim it with a state change.
> + */
> +static void
> +nvme_fc_start_ioerr_recovery(struct nvme_fc_ctrl *ctrl, char *errmsg)
> +{
> +	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);
> +		queue_work(nvme_reset_wq, &ctrl->ioerr_work);
> +	}
> +}
> +
>  static void
>  nvme_fc_fcpio_done(struct nvmefc_fcp_req *req)
>  {
> @@ -2049,9 +2071,8 @@ nvme_fc_fcpio_done(struct nvmefc_fcp_req *req)
>  		nvme_fc_complete_rq(rq);
>  
>  check_error:
> -	if (terminate_assoc &&
> -	    nvme_ctrl_state(&ctrl->ctrl) != NVME_CTRL_RESETTING)
> -		queue_work(nvme_reset_wq, &ctrl->ioerr_work);
> +	if (terminate_assoc)
> +		nvme_fc_start_ioerr_recovery(ctrl, "io error");
>  }

[Severity: High]
Can nvme_fc_create_association() transition a broken or disconnected
association to the live state when a timeout, transport error, or
Disconnect Association link service occurs while connecting?

While ctrl->ctrl.state is connecting, nvme_change_ctrl_state() rejects a
transition to resetting, so nvme_fc_start_ioerr_recovery() is a no-op and
neither sets the association failed flag nor calls
__nvme_fc_abort_outstanding_ios() to clear the queue live bit and mark
requests cancelled:

1. When an admin command times out in nvme_fc_timeout() while connecting,
__nvme_fc_abort_op() aborts the command without setting the cancelled
request flag on op->nreq.flags.  nvme_fc_fcpio_done() then completes the
request with positive status NVME_SC_HOST_ABORTED_CMD (0x371) instead of
-EINTR.  If that command is nvme_set_queue_count() in
nvme_fc_create_io_queues(), nvme_set_queue_count() treats positive status
as non-fatal, sets nr_io_queues to 0, and returns 0, allowing
nvme_fc_create_association() to transition the controller to live with zero
I/O queues.

2. Likewise, optional commands in nvme_init_ctrl_finish() (such as
nvme_get_effects_log() or nvme_configure_apst()) and asynchronous
keep-alive commands ignore positive status codes from transport errors or
aborts, and a Disconnect Association request received in
nvme_fc_ls_disconnect_assoc() after ctrl->association_id is set stores
ctrl->rcv_disconn and calls nvme_fc_start_ioerr_recovery() without setting
the association failed flag.  When the remaining setup commands finish,
nvme_fc_create_association() sees no error, moves the controller to live,
and leaves ctrl->rcv_disconn unanswered:

drivers/nvme/host/fc.c:nvme_fc_create_association() {
    ...
	if (!ret && test_bit(ASSOC_FAILED, &ctrl->flags))
		ret = -EIO;
	if (ret)
		goto out_term_aen_ops;

	if (!nvme_change_ctrl_state(&ctrl->ctrl, NVME_CTRL_LIVE)) {
    ...
}

> @@ -2548,24 +2569,32 @@ static enum blk_eh_timer_return nvme_fc_timeout(struct request *rq)
>  	struct nvme_fc_cmd_iu *cmdiu = &op->cmd_iu;
>  	struct nvme_command *sqe = &cmdiu->sqe;
>  
> -	/*
> -	 * Attempt to abort the offending command. Command completion
> -	 * will detect the aborted io and will fail the connection.
> -	 */
>  	dev_info(ctrl->ctrl.device,
>  		"NVME-FC{%d.%d}: io timeout: opcode %d fctype %d (%s) w10/11: "
>  		"x%08x/x%08x\n",
>  		ctrl->cnum, qnum, sqe->common.opcode, sqe->fabrics.fctype,
>  		nvme_fabrics_opcode_str(qnum, sqe),
>  		sqe->common.cdw10, sqe->common.cdw11);
> -	if (__nvme_fc_abort_op(ctrl, op))
> -		nvme_fc_error_recovery(ctrl, "io timeout abort failed");
> +
> +	if (nvme_ctrl_state(&ctrl->ctrl) != NVME_CTRL_LIVE) {
> +		/*
> +		 * The association is being set up, being torn down or is
> +		 * already gone, so recovery cannot be started from here.
> +		 * Abort the command so the connect or teardown it would
> +		 * block can make progress. It completes through
> +		 * nvme_fc_fcpio_done() once the LLDD has finished the
> +		 * abort, and the abort is counted so a teardown that
> +		 * follows waits for it.
> +		 */
> +		__nvme_fc_abort_op(ctrl, op);
> +		return BLK_EH_DONE;
> +	}

[Severity: Medium]
Does aborting a timed-out request here without clearing the queue live flag
or marking the request cancelled allow the request to be retried against a
dead target while deleting?

During controller deletion, nvme_do_delete_ctrl() runs
nvme_remove_namespaces() before nvme_fc_delete_ctrl() ->
nvme_fc_delete_association(), so the queue live flag is still set on
op->queue->flags.  When the abort finishes, nvme_fc_fcpio_done() completes
the request with NVME_SC_HOST_ABORTED_CMD (without do-not-retry), so
non-multipath requests (such as del_gendisk() writeback before the queue is
marked dying) or retryable internal commands are retried by
nvme_decide_disposition().  Because op->queue->flags still has the queue
live bit set, nvme_check_ready() returns true in nvme_fc_queue_rq() and
re-dispatches the request to the dead target for up to nvme_max_retries
additional timeouts instead of fast-failing via
nvme_fail_nonready_command().

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261114030454.2836543-1-mkhalfella@purestorage.com?part=12

Re: [PATCH v7 15/17] nvme-fc: Hold inflight requests while in FENCING state

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [High] nvme-fc: use-after-free on ctrl in nvme_fc_fcpio_done() after
  completing rq
- [High] nvme-fc: missing drain of completing ops in
  nvme_fc_fcpio_done()
--

commit 37b80917265ea84d22dcb27e9731ac42994f67a4
Author: Mohamed Khalfella <mkhalfella@purestorage.com>

nvme-fc: Hold inflight requests while in FENCING state

This commit splits nvme_fc_delete_association() into teardown and request
flushing helpers, tracks held requests with FCPOP_STATE_HELD, and delays
completing in-flight requests until controller fencing finishes.

> diff --git a/drivers/nvme/host/fc.c b/drivers/nvme/host/fc.c
> index 095adc9011ccd..5e8977ad51cf8 100644
> --- a/drivers/nvme/host/fc.c
> +++ b/drivers/nvme/host/fc.c
[ ... ]
> @@ -2113,22 +2125,58 @@ nvme_fc_fcpio_done(struct nvmefc_fcp_req *req)
>  	terminate_assoc = false;
>  
>  done:
> +	/*
> +	 * Start recovery before completing or holding anything. A held
> +	 * request is only completed by nvme_fc_flush_held_requests(), which
> +	 * fencing_work runs once the fence is over, so a request may only be
> +	 * held once FENCING has been claimed and fencing_work is guaranteed
> +	 * to run. If the claim fails because a reset or a delete already owns
> +	 * the controller, the request is completed below and that owner
> +	 * tears the association down.
> +	 */
> +	if (terminate_assoc) {
> +		nvme_fc_start_ioerr_recovery(ctrl, "io error");
> +		hold = nvme_ctrl_state(&ctrl->ctrl) == NVME_CTRL_FENCING;
> +	}
> +
>  	if (op->flags & FCOP_FLAGS_AEN) {
>  		nvme_complete_async_event(&queue->ctrl->ctrl, status, &result);
> -		__nvme_fc_fcpop_chk_teardowns(ctrl, op, opstate);
> +		if (__nvme_fc_fcpop_chk_teardowns(ctrl, op, opstate))
> +			__nvme_fc_fcpop_count_one_down(ctrl);
>  		atomic_set(&op->state, FCPOP_STATE_IDLE);
>  		op->flags = FCOP_FLAGS_AEN;	/* clear other flags */
>  		nvme_fc_ctrl_put(ctrl);
> -		goto check_error;
> +		return;
>  	}
>  
> -	__nvme_fc_fcpop_chk_teardowns(ctrl, op, opstate);
> +	/*
> +	 * We can not access op after the request is completed because it can
> +	 * be reused immediately. At the same time we want to wakeup the thread
> +	 * waiting for ongoing IOs _after_ requests are completed. This is
> +	 * necessary because that thread will start canceling inflight IOs
> +	 * and we want to avoid request completion racing with cancellation.
> +	 */
> +	op_term = __nvme_fc_fcpop_chk_teardowns(ctrl, op, opstate);
> +
> +	/*
> +	 * If the controller is FENCING, do not complete this request now.
> +	 * Move the op to HELD and let nvme_fc_cancel_held_requests() cancel
> +	 * it when it is safe to do so. HELD is only reachable from COMPLETE
> +	 * through this cmpxchg: if the cancellation has already walked the
> +	 * tagset it moved the op out of COMPLETE, the cmpxchg fails and the
> +	 * request is completed here instead. Either way exactly one path
> +	 * completes it.
> +	 */
> +	if (hold && atomic_cmpxchg(&op->state, FCPOP_STATE_COMPLETE,
> +				   FCPOP_STATE_HELD) == FCPOP_STATE_COMPLETE)
> +		goto check_op_term;
> +
>  	if (!nvme_try_complete_req(rq, status, result))
>  		nvme_fc_complete_rq(rq);
>  
> -check_error:
> -	if (terminate_assoc)
> -		nvme_fc_start_ioerr_recovery(ctrl, "io error");
> +check_op_term:
> +	if (op_term)
> +		__nvme_fc_fcpop_count_one_down(ctrl);
>  }

[Severity: High]
Can __nvme_fc_fcpop_count_one_down(ctrl) cause a use-after-free on ctrl
when a non-held aborted request completes?

nvme_fc_complete_rq(rq) drops the FCP op's reference via
nvme_fc_ctrl_put(ctrl) before check_op_term runs.  When
__nvme_fc_fcpop_count_one_down() then decrements ctrl->iocnt to 0, a
concurrent controller deletion can finish teardown and free ctrl before
wake_up(&ctrl->ioabort_wait) runs:

CPU0                                  CPU1
nvme_fc_fcpio_done()                  nvme_do_delete_ctrl()
  nvme_fc_complete_rq(rq)               nvme_fc_delete_ctrl()
    nvme_fc_ctrl_put(ctrl)                ...
  __nvme_fc_fcpop_count_one_down()          wait_event(..., iocnt == 0)
    atomic_dec_return(&ctrl->iocnt)
                                          ...
                                        nvme_uninit_ctrl()
                                        nvme_put_ctrl()
                                          ...
                                            kfree(ctrl)
    wake_up(&ctrl->ioabort_wait)  <-- use-after-free

Acquiring an extra reference on ctrl around
__nvme_fc_fcpop_count_one_down(ctrl), or dropping the op's ctrl reference
only after check_op_term when the request is completed, would keep ctrl
alive across the wakeup.

[ ... ]

> @@ -3323,13 +3372,71 @@ nvme_fc_delete_association(struct nvme_fc_ctrl *ctrl)
>  	__nvme_fc_delete_hw_queue(ctrl, &ctrl->queues[0], 0);
>  	nvme_fc_free_queue(&ctrl->queues[0]);
>  
> +	nvme_fc_ctlr_inactive_on_rport(ctrl);
> +}
> +
> +static bool
> +nvme_fc_cancel_held_request(struct request *req, void *data)
> +{
> +	struct nvme_fc_fcp_op *op = blk_mq_rq_to_pdu(req);
> +	struct nvme_ctrl *ctrl = data;
> +
> +	/*
> +	 * A single cmpxchg decides who completes this request. If the op is
> +	 * HELD, nvme_fc_fcpio_done() is done with it and we cancel it. If it
> +	 * is still COMPLETE, nvme_fc_fcpio_done() has not decided yet: move
> +	 * it to IDLE so its own cmpxchg to HELD fails and it completes the
> +	 * request itself (nvme_fc_complete_rq() sets IDLE anyway). Doing this
> +	 * as read-then-cmpxchg would let the op become HELD in between and
> +	 * leave it with no one to complete it.
> +	 */
> +	if (atomic_cmpxchg(&op->state, FCPOP_STATE_COMPLETE,
> +			   FCPOP_STATE_IDLE) == FCPOP_STATE_HELD)
> +		nvme_cancel_request(req, ctrl);
> +
> +	return true;
> +}
> +
> +static void
> +nvme_fc_cancel_held_requests(struct nvme_ctrl *ctrl, struct blk_mq_tag_set *set)
> +{
> +	if (!set)
> +		return;
> +
> +	blk_mq_tagset_busy_iter(set, nvme_fc_cancel_held_request, ctrl);
> +	blk_mq_tagset_wait_completed_request(set);
> +}

[Severity: High]
Does nvme_fc_cancel_held_requests() return without waiting when it wins
the cmpxchg against an in-progress nvme_fc_fcpio_done()?

When nvme_fc_fcpio_done() exchanges op->state to FCPOP_STATE_COMPLETE
before __nvme_fc_abort_op() runs, ctrl->iocnt is not incremented for that
op.  If nvme_fc_cancel_held_request() then runs before nvme_fc_fcpio_done()
reaches the hold cmpxchg, it moves op->state from FCPOP_STATE_COMPLETE to
FCPOP_STATE_IDLE without calling nvme_cancel_request().  Because
blk_mq_tagset_wait_completed_request() only waits for requests in
MQ_RQ_COMPLETE state, and rq is still MQ_RQ_IN_FLIGHT until
nvme_fc_fcpio_done() calls nvme_try_complete_req(), the flush and fence
can finish while nvme_fc_fcpio_done() is still executing:

CPU0                                  CPU1
nvme_fc_fcpio_done()
  atomic_xchg(&op->state, COMPLETE)
  dev_info(...)
                                      nvme_fc_fencing_work()
                                        __nvme_fc_teardown_association()
                                        nvme_fence_ctrl()
                                        nvme_fc_flush_held_requests()
                                          nvme_fc_cancel_held_request()
                                            cmpxchg(COMPLETE -> IDLE)
                                          blk_mq_tagset_wait_completed_request()
                                        ...
                                        /* reconnects to NVME_CTRL_LIVE */
  nvme_fc_start_ioerr_recovery()
    /* enters NVME_CTRL_FENCING */
  cmpxchg(COMPLETE -> HELD) /* fails */
  nvme_try_complete_req(rq, ...)  <-- completes rq during FENCING

A late nvme_fc_fcpio_done() from the old association can then trigger
nvme_fc_start_ioerr_recovery() on the new association and complete rq
immediately while in NVME_CTRL_FENCING because op->state is already
FCPOP_STATE_IDLE.

In addition, for AEN ops in nvme_fc_fcpio_done(), calling
__nvme_fc_fcpop_count_one_down() before resetting op->state and op->flags
lets __nvme_fc_teardown_association() proceed to nvme_fc_term_aen_ops() ->
__nvme_fc_exit_request() (which unmaps DMA and sets FCPOP_STATE_UNINIT)
while nvme_fc_fcpio_done() is still accessing the AEN op and subsequently
overwrites op->state back to FCPOP_STATE_IDLE.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261114030454.2836543-1-mkhalfella@purestorage.com?part=15