[RFC PATCH v1 0/7] nvme-tcp: Implement TP4129 (KATO Corrections and Clarifications)

[RFC PATCH v1 0/7] nvme-tcp: Implement TP4129 (KATO Corrections and Clarifications)

[Mohamed Khalfella]

Hello,

RFC patchset implementing TP4129 (KATO Corrections and Clarifications)
for nvme-tcp. nvme-tcp was choosen as an example to demonstrate the
approach taken in the patchset. Other fabric transports, nvme-rdma and
nvme-fc, will be added including the feedback received in this RFC.

TP4129 requires nvme controller to not immediately cancel inflight
requests when connection is lost between initiator and target.
Instead, inflight requests need to be held for a duration that is long
enough to allow target to learn about connection loss and quiesce
pending commands. Only then pending requests on the initiator side can
be canceled and possibly retried safely on another path. The main issue
TP4129 tries to address is ABA corruption that could happen if inflight
requests are tried immediately on another path.

Requests hold timeout has two components:

- KATO timeout is the time sufficient for target to learn about the
  connection loss. It depends on whether command based or traffic based
  keepalive is used. As per TP4129 the timeout is supposed to be 3 x KATO
  for traffic based keepalive and 2 * KATO for command based keepalive.

- CQT is the time needed by target controller to quiesce in flight nvme
  commands after the controller learns about connection loss.

On controller reset or delete cancel inflight requests if controller was
disabled correctly. Otherwise, hold the requests until it is safe to be
released.

Jyoti Rani (1):
  nvme-core: Read CQT wait from identify controller response

Mohamed Khalfella (6):
  nvmef: Add nvmef_req_hold_timeout_ms() to calculate kato request hold
    time
  nvme-tcp: Move freeing tagset out of nvme_tcp_teardown_io_queues()
  nvme-tcp: Move freeing admin tagset out of
    nvme_tcp_teardown_admin_queue()
  nvme-tcp: Split nvme_tcp_teardown_io_queues() into two functions
  nvme-core: Add support for holding inflight requests
  nvme-tcp: Do not immediately cancel inflight requests during recovery

 drivers/nvme/host/core.c    | 62 +++++++++++++++++++++++++++++++++++++
 drivers/nvme/host/fabrics.h |  7 +++++
 drivers/nvme/host/nvme.h    |  5 +++
 drivers/nvme/host/tcp.c     | 56 +++++++++++++++++++++------------
 include/linux/nvme.h        |  4 ++-
 5 files changed, 114 insertions(+), 20 deletions(-)

-- 
2.48.1

[RFC PATCH v1 1/7] nvme-core: Read CQT wait from identify controller response

[Mohamed Khalfella]

From: Jyoti Rani <jrani@purestorage.com>

CQT wait tells us how much time in milliseconds nvme controller needs to
quiesce pending nvme commands after learning loss of connectivity. Read
it from identify controller response and store it in struct nvme_ctrl in
cqt field. ctrl->cqt will be used later on to calculate the total time
inflight requests need to be held before they can be safely retried.

Signed-off-by: Jyoti Rani <jrani@purestorage.com>
---
 drivers/nvme/host/core.c | 1 +
 drivers/nvme/host/nvme.h | 1 +
 include/linux/nvme.h     | 4 +++-
 3 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 8359d0aa0e44..f4b3c6a42e90 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -3364,6 +3364,7 @@ static int nvme_init_identify(struct nvme_ctrl *ctrl)
 	ctrl->oaes = le32_to_cpu(id->oaes);
 	ctrl->wctemp = le16_to_cpu(id->wctemp);
 	ctrl->cctemp = le16_to_cpu(id->cctemp);
+	ctrl->cqt = le16_to_cpu(id->cqt);
 
 	atomic_set(&ctrl->abort_limit, id->acl + 1);
 	ctrl->vwc = id->vwc;
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index 7be92d07430e..7563332b5b7b 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -344,6 +344,7 @@ struct nvme_ctrl {
 	u32 oaes;
 	u32 aen_result;
 	u32 ctratt;
+	u16 cqt;
 	unsigned int shutdown_timeout;
 	unsigned int kato;
 	bool subsystem;
diff --git a/include/linux/nvme.h b/include/linux/nvme.h
index 2dc05b1c3283..7a94c01f0f06 100644
--- a/include/linux/nvme.h
+++ b/include/linux/nvme.h
@@ -361,7 +361,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.48.1

[RFC PATCH v1 2/7] nvmef: Add nvmef_req_hold_timeout_ms() to calculate kato request hold time

[Mohamed Khalfella]

nvme request hold timeout is the time sufficient for target controller
to learn about loss of connectivity to initiator and quiesce inflight
nvme commands.

The timeout has two components:

- KATO timeout is the time sufficient for target to learn about the
  connection loss to the target. It depends on whether command based or
  traffic based keepalive is used. As per TP4129 the timeout is supposed
  to be 3 x KATO for traffic based keepalive and 2 * KATO for command
  based keepalive.

- CQT is the time needed by target controller to quiesce in flight nvme
  commands after the controller learns about connection loss.

Signed-off-by: Mohamed Khalfella <mkhalfella@purestorage.com>
---
 drivers/nvme/host/fabrics.h | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/nvme/host/fabrics.h b/drivers/nvme/host/fabrics.h
index 21d75dc4a3a0..483823d430d0 100644
--- a/drivers/nvme/host/fabrics.h
+++ b/drivers/nvme/host/fabrics.h
@@ -214,6 +214,13 @@ static inline unsigned int nvmf_nr_io_queues(struct nvmf_ctrl_options *opts)
 		min(opts->nr_poll_queues, num_online_cpus());
 }
 
+static inline unsigned int nvmef_req_hold_timeout_ms(struct nvme_ctrl *ctrl)
+{
+	if (ctrl->ctratt & NVME_CTRL_ATTR_TBKAS)
+		return 3 * ctrl->kato * 1000 + ctrl->cqt;
+	return 2 * ctrl->kato * 1000 + ctrl->cqt;
+}
+
 int nvmf_reg_read32(struct nvme_ctrl *ctrl, u32 off, u32 *val);
 int nvmf_reg_read64(struct nvme_ctrl *ctrl, u32 off, u64 *val);
 int nvmf_reg_write32(struct nvme_ctrl *ctrl, u32 off, u32 val);
-- 
2.48.1

[RFC PATCH v1 3/7] nvme-tcp: Move freeing tagset out of nvme_tcp_teardown_io_queues()

[Mohamed Khalfella]

Update nvme_tcp_teardown_io_queues() to only tear down IO queues.
Let the caller take care of freeing the tagset if needed. This change
prepares the code to inject the delay described in TP4129 after IO
queues are tron down and before IO tagset is freed.

Signed-off-by: Mohamed Khalfella <mkhalfella@purestorage.com>
---
 drivers/nvme/host/tcp.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
index 327f3f2f5399..d12b51ed1a9e 100644
--- a/drivers/nvme/host/tcp.c
+++ b/drivers/nvme/host/tcp.c
@@ -2271,8 +2271,7 @@ static void nvme_tcp_teardown_admin_queue(struct nvme_ctrl *ctrl,
 	}
 }
 
-static void nvme_tcp_teardown_io_queues(struct nvme_ctrl *ctrl,
-		bool remove)
+static void nvme_tcp_teardown_io_queues(struct nvme_ctrl *ctrl)
 {
 	if (ctrl->queue_count <= 1)
 		return;
@@ -2280,10 +2279,6 @@ static void nvme_tcp_teardown_io_queues(struct nvme_ctrl *ctrl,
 	nvme_sync_io_queues(ctrl);
 	nvme_tcp_stop_io_queues(ctrl);
 	nvme_cancel_tagset(ctrl);
-	if (remove) {
-		nvme_unquiesce_io_queues(ctrl);
-		nvme_remove_io_tag_set(ctrl);
-	}
 	nvme_tcp_free_io_queues(ctrl);
 }
 
@@ -2417,7 +2412,7 @@ static void nvme_tcp_error_recovery_work(struct work_struct *work)
 
 	nvme_stop_keep_alive(ctrl);
 	flush_work(&ctrl->async_event_work);
-	nvme_tcp_teardown_io_queues(ctrl, false);
+	nvme_tcp_teardown_io_queues(ctrl);
 	/* unquiesce to fail fast pending requests */
 	nvme_unquiesce_io_queues(ctrl);
 	nvme_tcp_teardown_admin_queue(ctrl, false);
@@ -2438,7 +2433,7 @@ static void nvme_tcp_error_recovery_work(struct work_struct *work)
 
 static void nvme_tcp_teardown_ctrl(struct nvme_ctrl *ctrl, bool shutdown)
 {
-	nvme_tcp_teardown_io_queues(ctrl, shutdown);
+	nvme_tcp_teardown_io_queues(ctrl);
 	nvme_quiesce_admin_queue(ctrl);
 	nvme_disable_ctrl(ctrl, shutdown);
 	nvme_tcp_teardown_admin_queue(ctrl, shutdown);
@@ -2447,6 +2442,10 @@ static void nvme_tcp_teardown_ctrl(struct nvme_ctrl *ctrl, bool shutdown)
 static void nvme_tcp_delete_ctrl(struct nvme_ctrl *ctrl)
 {
 	nvme_tcp_teardown_ctrl(ctrl, true);
+	if (ctrl->queue_count > 1) {
+		nvme_unquiesce_io_queues(ctrl);
+		nvme_remove_io_tag_set(ctrl);
+	}
 }
 
 static void nvme_reset_ctrl_work(struct work_struct *work)
-- 
2.48.1

[RFC PATCH v1 4/7] nvme-tcp: Move freeing admin tagset out of nvme_tcp_teardown_admin_queue()

[Mohamed Khalfella]

Update nvme_tcp_teardown_admin_queue() to only tear down admin queue.
Let the caller take care of freeing the tagset if needed. This change
prepares the code to inject the delay described in TP4129 after admin
queue is tron down and before admin tagset is freed.

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

diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
index d12b51ed1a9e..99533ff1ea20 100644
--- a/drivers/nvme/host/tcp.c
+++ b/drivers/nvme/host/tcp.c
@@ -2252,17 +2252,12 @@ static int nvme_tcp_configure_admin_queue(struct nvme_ctrl *ctrl, bool new)
 	return error;
 }
 
-static void nvme_tcp_teardown_admin_queue(struct nvme_ctrl *ctrl,
-		bool remove)
+static void nvme_tcp_teardown_admin_queue(struct nvme_ctrl *ctrl)
 {
 	nvme_quiesce_admin_queue(ctrl);
 	blk_sync_queue(ctrl->admin_q);
 	nvme_tcp_stop_queue(ctrl, 0);
 	nvme_cancel_admin_tagset(ctrl);
-	if (remove) {
-		nvme_unquiesce_admin_queue(ctrl);
-		nvme_remove_admin_tag_set(ctrl);
-	}
 	nvme_tcp_free_admin_queue(ctrl);
 	if (ctrl->tls_pskid) {
 		dev_dbg(ctrl->device, "Wipe negotiated TLS_PSK %08x\n",
@@ -2374,7 +2369,11 @@ static int nvme_tcp_setup_ctrl(struct nvme_ctrl *ctrl, bool new)
 	}
 destroy_admin:
 	nvme_stop_keep_alive(ctrl);
-	nvme_tcp_teardown_admin_queue(ctrl, new);
+	nvme_tcp_teardown_admin_queue(ctrl);
+	if (new) {
+		nvme_unquiesce_admin_queue(ctrl);
+		nvme_remove_admin_tag_set(ctrl);
+	}
 	return ret;
 }
 
@@ -2415,7 +2414,7 @@ static void nvme_tcp_error_recovery_work(struct work_struct *work)
 	nvme_tcp_teardown_io_queues(ctrl);
 	/* unquiesce to fail fast pending requests */
 	nvme_unquiesce_io_queues(ctrl);
-	nvme_tcp_teardown_admin_queue(ctrl, false);
+	nvme_tcp_teardown_admin_queue(ctrl);
 	nvme_unquiesce_admin_queue(ctrl);
 	nvme_auth_stop(ctrl);
 
@@ -2436,7 +2435,7 @@ static void nvme_tcp_teardown_ctrl(struct nvme_ctrl *ctrl, bool shutdown)
 	nvme_tcp_teardown_io_queues(ctrl);
 	nvme_quiesce_admin_queue(ctrl);
 	nvme_disable_ctrl(ctrl, shutdown);
-	nvme_tcp_teardown_admin_queue(ctrl, shutdown);
+	nvme_tcp_teardown_admin_queue(ctrl);
 }
 
 static void nvme_tcp_delete_ctrl(struct nvme_ctrl *ctrl)
@@ -2446,6 +2445,8 @@ static void nvme_tcp_delete_ctrl(struct nvme_ctrl *ctrl)
 		nvme_unquiesce_io_queues(ctrl);
 		nvme_remove_io_tag_set(ctrl);
 	}
+	nvme_unquiesce_admin_queue(ctrl);
+	nvme_remove_admin_tag_set(ctrl);
 }
 
 static void nvme_reset_ctrl_work(struct work_struct *work)
-- 
2.48.1

[RFC PATCH v1 5/7] nvme-tcp: Split nvme_tcp_teardown_io_queues() into two functions

[Mohamed Khalfella]

Move code that quiesces IO queues out of nvme_tcp_teardown_io_queues() in
a new function nvme_tcp_suspend_io_queues(). In a later change this
allows us to disable nvme controller without needing to tear down IO
queues and canceling inflight requests.

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

diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
index 99533ff1ea20..327e37a25281 100644
--- a/drivers/nvme/host/tcp.c
+++ b/drivers/nvme/host/tcp.c
@@ -2266,12 +2266,18 @@ static void nvme_tcp_teardown_admin_queue(struct nvme_ctrl *ctrl)
 	}
 }
 
-static void nvme_tcp_teardown_io_queues(struct nvme_ctrl *ctrl)
+static void nvme_tcp_suspend_io_queues(struct nvme_ctrl *ctrl)
 {
 	if (ctrl->queue_count <= 1)
 		return;
 	nvme_quiesce_io_queues(ctrl);
 	nvme_sync_io_queues(ctrl);
+}
+
+static void nvme_tcp_teardown_io_queues(struct nvme_ctrl *ctrl)
+{
+	if (ctrl->queue_count <= 1)
+		return;
 	nvme_tcp_stop_io_queues(ctrl);
 	nvme_cancel_tagset(ctrl);
 	nvme_tcp_free_io_queues(ctrl);
@@ -2411,6 +2417,7 @@ static void nvme_tcp_error_recovery_work(struct work_struct *work)
 
 	nvme_stop_keep_alive(ctrl);
 	flush_work(&ctrl->async_event_work);
+	nvme_tcp_suspend_io_queues(ctrl);
 	nvme_tcp_teardown_io_queues(ctrl);
 	/* unquiesce to fail fast pending requests */
 	nvme_unquiesce_io_queues(ctrl);
@@ -2432,6 +2439,7 @@ static void nvme_tcp_error_recovery_work(struct work_struct *work)
 
 static void nvme_tcp_teardown_ctrl(struct nvme_ctrl *ctrl, bool shutdown)
 {
+	nvme_tcp_suspend_io_queues(ctrl);
 	nvme_tcp_teardown_io_queues(ctrl);
 	nvme_quiesce_admin_queue(ctrl);
 	nvme_disable_ctrl(ctrl, shutdown);
-- 
2.48.1

[RFC PATCH v1 6/7] nvme-core: Add support for holding inflight requests

[Mohamed Khalfella]

TP4129 requires nvme controller to hold inflight requests in case of
connection loss for a certain duration of time until it is safe for
these requests to be retried. In case there are inflight requests
held_req_work is scheduled with the appropriate delay to cancel the
requests. Add the necessary logic to nvme-core so that all fabric
transports can use it.

Signed-off-by: Mohamed Khalfella <mkhalfella@purestorage.com>
---
 drivers/nvme/host/core.c | 61 ++++++++++++++++++++++++++++++++++++++++
 drivers/nvme/host/nvme.h |  4 +++
 2 files changed, 65 insertions(+)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index f4b3c6a42e90..4170785ecf21 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -551,6 +551,65 @@ void nvme_cancel_admin_tagset(struct nvme_ctrl *ctrl)
 }
 EXPORT_SYMBOL_GPL(nvme_cancel_admin_tagset);
 
+static void nvme_held_req_work(struct work_struct *work)
+{
+	struct nvme_ctrl *ctrl = container_of(to_delayed_work(work),
+			struct nvme_ctrl, held_req_work);
+	dev_info(ctrl->device, "releasing inflight requests\n");
+	nvme_cancel_tagset(ctrl);
+	nvme_cancel_admin_tagset(ctrl);
+	complete(&ctrl->held_req_completion);
+}
+
+static bool nvme_check_inflight_request(struct request *req, void *data)
+{
+	bool *inflight_reqs = data;
+
+	if (blk_mq_rq_state(req) == MQ_RQ_IN_FLIGHT) {
+		*inflight_reqs = true;
+		return false;
+	}
+	return true;
+}
+
+bool nvme_queue_held_requests_work(struct nvme_ctrl *ctrl)
+{
+	unsigned long timeout = nvmef_req_hold_timeout_ms(ctrl);
+	bool inflight_reqs = false;
+
+	if (ctrl->queue_count > 1 && ctrl->tagset)
+		blk_mq_tagset_busy_iter(ctrl->tagset,
+			nvme_check_inflight_request, &inflight_reqs);
+	if (inflight_reqs)
+		goto schedule_work;
+
+	if (ctrl->admin_tagset)
+		blk_mq_tagset_busy_iter(ctrl->admin_tagset,
+					nvme_check_inflight_request, &inflight_reqs);
+	if (!inflight_reqs)
+		return false;
+
+schedule_work:
+	dev_info(ctrl->device, "holding inflight requests for %lums\n", timeout);
+	reinit_completion(&ctrl->held_req_completion);
+	schedule_delayed_work(&ctrl->held_req_work, msecs_to_jiffies(timeout));
+	return true;
+}
+EXPORT_SYMBOL_GPL(nvme_queue_held_requests_work);
+
+void nvme_wait_for_held_requests(struct nvme_ctrl *ctrl)
+{
+	/*
+	 * Inflight requests can be held for a duration of time that is longer
+	 * than hung_task timeout. Avoid hitting hung_task timeout while waiting
+	 * for held requests to be completed.
+	 */
+	while (!wait_for_completion_timeout(&ctrl->held_req_completion,
+					    secs_to_jiffies(1)))
+		;
+}
+EXPORT_SYMBOL_GPL(nvme_wait_for_held_requests);
+
 bool nvme_change_ctrl_state(struct nvme_ctrl *ctrl,
 		enum nvme_ctrl_state new_state)
 {
@@ -4846,6 +4905,8 @@ int nvme_init_ctrl(struct nvme_ctrl *ctrl, struct device *dev,
 
 	INIT_DELAYED_WORK(&ctrl->ka_work, nvme_keep_alive_work);
 	INIT_DELAYED_WORK(&ctrl->failfast_work, nvme_failfast_work);
+	INIT_DELAYED_WORK(&ctrl->held_req_work, nvme_held_req_work);
+	init_completion(&ctrl->held_req_completion);
 	memset(&ctrl->ka_cmd, 0, sizeof(ctrl->ka_cmd));
 	ctrl->ka_cmd.common.opcode = nvme_admin_keep_alive;
 	ctrl->ka_last_check_time = jiffies;
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index 7563332b5b7b..ed8729f93ace 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -356,6 +356,8 @@ struct nvme_ctrl {
 	struct work_struct async_event_work;
 	struct delayed_work ka_work;
 	struct delayed_work failfast_work;
+	struct delayed_work held_req_work;
+	struct completion held_req_completion;
 	struct nvme_command ka_cmd;
 	unsigned long ka_last_check_time;
 	struct work_struct fw_act_work;
@@ -793,6 +795,8 @@ 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);
+bool nvme_queue_held_requests_work(struct nvme_ctrl *ctrl);
+void nvme_wait_for_held_requests(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);
-- 
2.48.1

[RFC PATCH v1 7/7] nvme-tcp: Do not immediately cancel inflight requests during recovery

[Mohamed Khalfella]

In case of controller recovery, reset, or deletion it is possible there
are inflight requests for which no response was received from target
controller. TP4129 requires such inflight requests to not be canceled
immediately. Instead these requests should be held until the target
learns about disconnection and quiesce pending nvme commands. Implement
this for nvme-tcp.

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

diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
index 327e37a25281..822e6329e332 100644
--- a/drivers/nvme/host/tcp.c
+++ b/drivers/nvme/host/tcp.c
@@ -2252,12 +2252,14 @@ static int nvme_tcp_configure_admin_queue(struct nvme_ctrl *ctrl, bool new)
 	return error;
 }
 
-static void nvme_tcp_teardown_admin_queue(struct nvme_ctrl *ctrl)
+static void nvme_tcp_teardown_admin_queue(struct nvme_ctrl *ctrl,
+					  bool hold_reqs)
 {
 	nvme_quiesce_admin_queue(ctrl);
 	blk_sync_queue(ctrl->admin_q);
 	nvme_tcp_stop_queue(ctrl, 0);
-	nvme_cancel_admin_tagset(ctrl);
+	if (!hold_reqs)
+		nvme_cancel_admin_tagset(ctrl);
 	nvme_tcp_free_admin_queue(ctrl);
 	if (ctrl->tls_pskid) {
 		dev_dbg(ctrl->device, "Wipe negotiated TLS_PSK %08x\n",
@@ -2274,12 +2276,14 @@ static void nvme_tcp_suspend_io_queues(struct nvme_ctrl *ctrl)
 	nvme_sync_io_queues(ctrl);
 }
 
-static void nvme_tcp_teardown_io_queues(struct nvme_ctrl *ctrl)
+static void nvme_tcp_teardown_io_queues(struct nvme_ctrl *ctrl,
+					bool hold_reqs)
 {
 	if (ctrl->queue_count <= 1)
 		return;
 	nvme_tcp_stop_io_queues(ctrl);
-	nvme_cancel_tagset(ctrl);
+	if (!hold_reqs)
+		nvme_cancel_tagset(ctrl);
 	nvme_tcp_free_io_queues(ctrl);
 }
 
@@ -2375,7 +2379,7 @@ static int nvme_tcp_setup_ctrl(struct nvme_ctrl *ctrl, bool new)
 	}
 destroy_admin:
 	nvme_stop_keep_alive(ctrl);
-	nvme_tcp_teardown_admin_queue(ctrl);
+	nvme_tcp_teardown_admin_queue(ctrl, false);
 	if (new) {
 		nvme_unquiesce_admin_queue(ctrl);
 		nvme_remove_admin_tag_set(ctrl);
@@ -2418,10 +2422,12 @@ static void nvme_tcp_error_recovery_work(struct work_struct *work)
 	nvme_stop_keep_alive(ctrl);
 	flush_work(&ctrl->async_event_work);
 	nvme_tcp_suspend_io_queues(ctrl);
-	nvme_tcp_teardown_io_queues(ctrl);
+	nvme_tcp_teardown_io_queues(ctrl, true);
+	nvme_tcp_teardown_admin_queue(ctrl, true);
+	if (nvme_queue_held_requests_work(ctrl))
+		nvme_wait_for_held_requests(ctrl);
 	/* unquiesce to fail fast pending requests */
 	nvme_unquiesce_io_queues(ctrl);
-	nvme_tcp_teardown_admin_queue(ctrl);
 	nvme_unquiesce_admin_queue(ctrl);
 	nvme_auth_stop(ctrl);
 
@@ -2439,11 +2445,15 @@ static void nvme_tcp_error_recovery_work(struct work_struct *work)
 
 static void nvme_tcp_teardown_ctrl(struct nvme_ctrl *ctrl, bool shutdown)
 {
+	bool hold_reqs;
+
 	nvme_tcp_suspend_io_queues(ctrl);
-	nvme_tcp_teardown_io_queues(ctrl);
 	nvme_quiesce_admin_queue(ctrl);
-	nvme_disable_ctrl(ctrl, shutdown);
-	nvme_tcp_teardown_admin_queue(ctrl);
+	hold_reqs = nvme_disable_ctrl(ctrl, shutdown);
+	nvme_tcp_teardown_io_queues(ctrl, hold_reqs);
+	nvme_tcp_teardown_admin_queue(ctrl, hold_reqs);
+	if (hold_reqs && nvme_queue_held_requests_work(ctrl))
+		nvme_wait_for_held_requests(ctrl);
 }
 
 static void nvme_tcp_delete_ctrl(struct nvme_ctrl *ctrl)
-- 
2.48.1

Re: [RFC PATCH v1 0/7] nvme-tcp: Implement TP4129 (KATO Corrections and Clarifications)

[Stuart Hayes]

On 2025-03-24 17:48 Mohamed Khalfella mkhalfella@purestorage.com wrote

> Hello,
>
> RFC patchset implementing TP4129 (KATO Corrections and Clarifications)
> for nvme-tcp. nvme-tcp was choosen as an example to demonstrate the
> approach taken in the patchset. Other fabric transports, nvme-rdma and
> nvme-fc, will be added including the feedback received in this RFC.
>
> TP4129 requires nvme controller to not immediately cancel inflight
> requests when connection is lost between initiator and target.
> Instead, inflight requests need to be held for a duration that is long
> enough to allow target to learn about connection loss and quiesce
> pending commands. Only then pending requests on the initiator side can
> be canceled and possibly retried safely on another path. The main issue
> TP4129 tries to address is ABA corruption that could happen if inflight
> requests are tried immediately on another path.
> 
> Requests hold timeout has two components:
> 
> - KATO timeout is the time sufficient for target to learn about the
>   connection loss. It depends on whether command based or traffic based
>   keepalive is used. As per TP4129 the timeout is supposed to be 3 x KATO
>   for traffic based keepalive and 2 * KATO for command based keepalive.
> 
> - CQT is the time needed by target controller to quiesce in flight nvme
>   commands after the controller learns about connection loss.
> 
> On controller reset or delete cancel inflight requests if controller was
> disabled correctly. Otherwise, hold the requests until it is safe to be
> released.
> 
> Jyoti Rani (1):
>   nvme-core: Read CQT wait from identify controller response
> 
> Mohamed Khalfella (6):
>   nvmef: Add nvmef_req_hold_timeout_ms() to calculate kato request hold
>     time
>   nvme-tcp: Move freeing tagset out of nvme_tcp_teardown_io_queues()
>   nvme-tcp: Move freeing admin tagset out of
>     nvme_tcp_teardown_admin_queue()
>   nvme-tcp: Split nvme_tcp_teardown_io_queues() into two functions
>   nvme-core: Add support for holding inflight requests
>   nvme-tcp: Do not immediately cancel inflight requests during recovery
> 
>  drivers/nvme/host/core.c    | 62 +++++++++++++++++++++++++++++++++++++
>  drivers/nvme/host/fabrics.h |  7 +++++
>  drivers/nvme/host/nvme.h    |  5 +++
>  drivers/nvme/host/tcp.c     | 56 +++++++++++++++++++++------------
>  include/linux/nvme.h        |  4 ++-
>  5 files changed, 114 insertions(+), 20 deletions(-)
> 
> -- 
> 2.48.1

Verified the patch fixes the Data integrity issue for which TP4129 was written.
Tested-by: Shai Dagieli (Shai.Dagieli@dell.com)

Summary:
The test included two parts:
1. Part 1: testing with original kernel without the patch to verify the test can 
   consistently reproduce the data integrity issue.
   a. Result: the Data integrity was recreated.
2. Part 2: testing with a kernel version with the patch.
   a. Results: 
      i.  The data integrity issued was not recreated with the patch. 
      ii. The messages seen on the storage log and dmesg are as expected based on 
          the test error injection and the patch expected behavior and messages.
3. Conclusion: the patch fixes the data integrity issue.
4. Additional comments:
   a. TP4129 defines an extended delay before an IO for which the host did not 
      receive a ‘command completion’ can be retried on a different path. Based 
      on the test logs, the extended delay is applied in other cases where the 
      IO is retried on another path, even when the host received command completion. 
      For example, the extended delay before retry was seen when the status returned 
      in the Command Completion was ‘status code type = 3h’ (Path Related Status) &
      ‘Status code = 00h’ (Internal Path Error). Applying the extended delay in cases
      where it is not required increases the potential performance impact of this
      patch and should be fixed. 
      i. Recommendation: Modify the patch to limit the extended delay to the relevant
         cases: IO retry, on a different path, when the host DID NOT receive a 
         Command Completion status.

Execution overview
- Test ran continuously for almost 24 hours with periodic SDT disruptions and host I/O validation.
 
Findings
- No DIs or I/O errors observed in Vdbench error logs (errorlog.html remained clean).
- No host panics or abnormal behavior were reported.
- dmesg shows expected TP4129 recovery messages, for example:
[18:52:28] nvme nvme2: holding inflight requests for 15000ms
[18:52:43] nvme nvme2: releasing inflight requests
[18:52:43] nvme_cancel_request: 123 callbacks suppressed
[18:52:43] nvme nvme2: Cancelling I/O ...