[PATCH 0/6] Support per command retry timer

[PATCH 0/6] Support per command retry timer

[Sagi Grimberg]

We have a single retry timer per request queue. This means that different
commands will absorb the latest crd levels returned by the controller for
any command. This is both inacturate and may theoretically create a
starvation when the controller will return different crd for different
commands in specific patterns.

Fix this by adding a per-command retry timer, that is only accessed/modified
when a command RETRY disposition is performed.

Addionally, respect controller crd level also when failing over a request.

Lastly, add some infrastructure to be able to set different crd levels with
the error injection framework in order to be able to exercise these paths.
This includes nvmet configurable crdt[1,2,3] subsystem attributes, and host
crd level error injection.

The first patch is a standalone minor optimization that can be taken outside
of the patchset. The second patch adds a generic helper that allows for
multiple call-sites calculating the completion crd level, patches 3+4
implement per-command crd for both RETRY and FAILOVER dispositions. Patchs
5+6 are added primarily for testing.

Patch 7/6 is an RFC blktests addition to exercise the new code additions.

Sagi Grimberg (6):
  nvme-mpath: No need to protect req->bio with requeue_lock
  nvme: add nvme_crd_msecs helper
  nvme: add per request retry timer
  nvme-mpath: support controller crd when failing over request
  nvmet: Add support for configurable crdt (command retry delay)
    attributes
  nvme/fault-injection: Support for error injection with custom crd

 drivers/nvme/host/apple.c        |  1 +
 drivers/nvme/host/core.c         | 32 ++++++++---
 drivers/nvme/host/fault_inject.c |  8 ++-
 drivers/nvme/host/fc.c           |  1 +
 drivers/nvme/host/multipath.c    | 97 ++++++++++++++++++++++++++++++--
 drivers/nvme/host/nvme.h         | 14 +++++
 drivers/nvme/host/pci.c          |  1 +
 drivers/nvme/host/rdma.c         |  1 +
 drivers/nvme/host/tcp.c          |  1 +
 drivers/nvme/target/admin-cmd.c  |  4 ++
 drivers/nvme/target/configfs.c   | 62 ++++++++++++++++++++
 drivers/nvme/target/loop.c       |  1 +
 drivers/nvme/target/nvmet.h      |  1 +
 13 files changed, 209 insertions(+), 15 deletions(-)

-- 
2.43.0

[PATCH 1/6] nvme-mpath: No need to protect req->bio with requeue_lock

[Sagi Grimberg]

requeue_lock protects the retry_list, no need to hold it while
iterating request bios.

Signed-off-by: Sagi Grimberg <sagi@grimberg.me>
---
 drivers/nvme/host/multipath.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/nvme/host/multipath.c b/drivers/nvme/host/multipath.c
index 9b9a657fa330..b5501217303c 100644
--- a/drivers/nvme/host/multipath.c
+++ b/drivers/nvme/host/multipath.c
@@ -164,9 +164,10 @@ void nvme_failover_req(struct request *req)
 		queue_work(nvme_wq, &ns->ctrl->ana_work);
 	}
 
-	spin_lock_irqsave(&ns->head->requeue_lock, flags);
 	for (bio = req->bio; bio; bio = bio->bi_next)
 		bio_set_dev(bio, ns->head->disk->part0);
+
+	spin_lock_irqsave(&ns->head->requeue_lock, flags);
 	blk_steal_bios(&ns->head->requeue_list, req);
 	spin_unlock_irqrestore(&ns->head->requeue_lock, flags);
 
-- 
2.43.0

[PATCH 2/6] nvme: add nvme_crd_msecs helper

[Sagi Grimberg]

Hide the details of extracting the crd value. We'll need it from other
call-sites.

Signed-off-by: Sagi Grimberg <sagi@grimberg.me>
---
 drivers/nvme/host/core.c |  8 +-------
 drivers/nvme/host/nvme.h | 10 ++++++++++
 2 files changed, 11 insertions(+), 7 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 453c1f0b2dd0..a49a86f96dd6 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -321,15 +321,9 @@ static blk_status_t nvme_error_status(u16 status)
 
 static void nvme_retry_req(struct request *req)
 {
-	unsigned long delay = 0;
-	u16 crd;
+	unsigned long delay = nvme_crd_msecs(nvme_req(req));
 	struct nvme_ns *ns = req->q->queuedata;
 
-	/* The mask and shift result must be <= 3 */
-	crd = (nvme_req(req)->status & NVME_STATUS_CRD) >> 11;
-	if (crd)
-		delay = nvme_req(req)->ctrl->crdt[crd - 1] * 100;
-
 	nvme_req(req)->retries++;
 	if (ns)
 		atomic_long_inc(&ns->retries);
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index 824651cc898d..6d8f6c541a61 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -921,6 +921,16 @@ static inline enum req_op nvme_req_op(struct nvme_command *cmd)
 	return nvme_is_write(cmd) ? REQ_OP_DRV_OUT : REQ_OP_DRV_IN;
 }
 
+static inline unsigned long nvme_crd_msecs(struct nvme_request *req)
+{
+	/* The mask and shift result must be <= 3 */
+	u16 crd = (req->status & NVME_STATUS_CRD) >> 11;
+
+	if (!crd)
+		return 0;
+	return req->ctrl->crdt[crd - 1] * 100;
+}
+
 #define NVME_QID_ANY -1
 void nvme_init_request(struct request *req, struct nvme_command *cmd);
 void nvme_cleanup_cmd(struct request *req);
-- 
2.43.0

[PATCH 3/6] nvme: add per request retry timer

[Sagi Grimberg]

The existing command retry mechanism adds a request to retry
to a retry list and modifies a request-queue (controller) wide
dealyed queue timer.

The issue is that the existing requests in this queue may wait
for longer periods of time as more requests are completed with
a retry crd level.

Instead, add a per-request timer that will allow different
requests retry in a way that is independent of other requests.

Signed-off-by: Sagi Grimberg <sagi@grimberg.me>
---
 drivers/nvme/host/apple.c  |  1 +
 drivers/nvme/host/core.c   | 24 ++++++++++++++++++++++--
 drivers/nvme/host/fc.c     |  1 +
 drivers/nvme/host/nvme.h   |  2 ++
 drivers/nvme/host/pci.c    |  1 +
 drivers/nvme/host/rdma.c   |  1 +
 drivers/nvme/host/tcp.c    |  1 +
 drivers/nvme/target/loop.c |  1 +
 8 files changed, 30 insertions(+), 2 deletions(-)

diff --git a/drivers/nvme/host/apple.c b/drivers/nvme/host/apple.c
index be3b91b43ea5..c584b9551f7d 100644
--- a/drivers/nvme/host/apple.c
+++ b/drivers/nvme/host/apple.c
@@ -829,6 +829,7 @@ static int apple_nvme_init_request(struct blk_mq_tag_set *set,
 	iod->q = q;
 	nreq->ctrl = &anv->ctrl;
 	nreq->cmd = &iod->cmd;
+	nvme_req_retry_timer_init(nreq);
 
 	return 0;
 }
diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index a49a86f96dd6..11c24f89f4fd 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -328,8 +328,11 @@ static void nvme_retry_req(struct request *req)
 	if (ns)
 		atomic_long_inc(&ns->retries);
 
-	blk_mq_requeue_request(req, false);
-	blk_mq_delay_kick_requeue_list(req->q, delay);
+	if (delay)
+		mod_timer(&nvme_req(req)->retry_timer,
+			jiffies + msecs_to_jiffies(delay));
+	else
+		blk_mq_requeue_request(req, true);
 }
 
 static void nvme_log_error(struct request *req)
@@ -538,6 +541,7 @@ bool nvme_cancel_request(struct request *req, void *data)
 	if (blk_mq_rq_state(req) != MQ_RQ_IN_FLIGHT)
 		return true;
 
+	timer_delete_sync(&nvme_req(req)->retry_timer);
 	nvme_req(req)->status = NVME_SC_HOST_ABORTED_CMD;
 	nvme_req(req)->flags |= NVME_REQ_CANCELLED;
 	blk_mq_complete_request(req);
@@ -720,6 +724,7 @@ EXPORT_SYMBOL_NS_GPL(nvme_put_ns, "NVME_TARGET_PASSTHRU");
 
 static inline void nvme_clear_nvme_request(struct request *req)
 {
+	WARN_ON_ONCE(timer_pending(&nvme_req(req)->retry_timer));
 	nvme_req(req)->status = 0;
 	nvme_req(req)->retries = 0;
 	nvme_req(req)->flags = 0;
@@ -1074,6 +1079,21 @@ static inline blk_status_t nvme_setup_rw(struct nvme_ns *ns,
 	return 0;
 }
 
+static void nvme_retry_timer_fn(struct timer_list *t)
+{
+	struct nvme_request *nr = timer_container_of(nr, t, retry_timer);
+
+	if (unlikely(nr->flags & NVME_REQ_CANCELLED))
+		return;
+	blk_mq_requeue_request(blk_mq_rq_from_pdu(nr), true);
+}
+
+void nvme_req_retry_timer_init(struct nvme_request *req)
+{
+	timer_setup(&req->retry_timer, nvme_retry_timer_fn, 0);
+}
+EXPORT_SYMBOL_GPL(nvme_req_retry_timer_init);
+
 void nvme_cleanup_cmd(struct request *req)
 {
 	if (req->rq_flags & RQF_SPECIAL_PAYLOAD) {
diff --git a/drivers/nvme/host/fc.c b/drivers/nvme/host/fc.c
index 04363b9c4489..4117cf0f7ac2 100644
--- a/drivers/nvme/host/fc.c
+++ b/drivers/nvme/host/fc.c
@@ -2124,6 +2124,7 @@ nvme_fc_init_request(struct blk_mq_tag_set *set, struct request *rq,
 	op->op.fcp_req.private = &op->priv[0];
 	nvme_req(rq)->ctrl = &ctrl->ctrl;
 	nvme_req(rq)->cmd = &op->op.cmd_iu.sqe;
+	nvme_req_retry_timer_init(nvme_req(rq));
 	return res;
 }
 
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index 6d8f6c541a61..944462e912f6 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -249,6 +249,7 @@ struct nvme_request {
 	unsigned long		start_time;
 #endif
 	struct nvme_ctrl	*ctrl;
+	struct timer_list	retry_timer;
 };
 
 /*
@@ -931,6 +932,7 @@ static inline unsigned long nvme_crd_msecs(struct nvme_request *req)
 	return req->ctrl->crdt[crd - 1] * 100;
 }
 
+void nvme_req_retry_timer_init(struct nvme_request *req);
 #define NVME_QID_ANY -1
 void nvme_init_request(struct request *req, struct nvme_command *cmd);
 void nvme_cleanup_cmd(struct request *req);
diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 8438c904ec49..aaaa72ce7cbf 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -671,6 +671,7 @@ static int nvme_pci_init_request(struct blk_mq_tag_set *set,
 
 	nvme_req(req)->ctrl = set->driver_data;
 	nvme_req(req)->cmd = &iod->cmd;
+	nvme_req_retry_timer_init(nvme_req(req));
 	return 0;
 }
 
diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
index 6909e3542794..434999acc6ea 100644
--- a/drivers/nvme/host/rdma.c
+++ b/drivers/nvme/host/rdma.c
@@ -312,6 +312,7 @@ static int nvme_rdma_init_request(struct blk_mq_tag_set *set,
 
 	req->queue = queue;
 	nvme_req(rq)->cmd = req->sqe.data;
+	nvme_req_retry_timer_init(nvme_req(rq));
 
 	return 0;
 }
diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
index ba5c7b3e2a7c..3503e3ad4d6b 100644
--- a/drivers/nvme/host/tcp.c
+++ b/drivers/nvme/host/tcp.c
@@ -562,6 +562,7 @@ static int nvme_tcp_init_request(struct blk_mq_tag_set *set,
 	nvme_req(rq)->cmd = &pdu->cmd;
 	init_llist_node(&req->lentry);
 	INIT_LIST_HEAD(&req->entry);
+	nvme_req_retry_timer_init(nvme_req(rq));
 
 	return 0;
 }
diff --git a/drivers/nvme/target/loop.c b/drivers/nvme/target/loop.c
index fcb1f8186fdd..86c21631345d 100644
--- a/drivers/nvme/target/loop.c
+++ b/drivers/nvme/target/loop.c
@@ -209,6 +209,7 @@ static int nvme_loop_init_request(struct blk_mq_tag_set *set,
 
 	nvme_req(req)->ctrl = &ctrl->ctrl;
 	nvme_req(req)->cmd = &iod->cmd;
+	nvme_req_retry_timer_init(nvme_req(req));
 	return nvme_loop_init_iod(ctrl, blk_mq_rq_to_pdu(req),
 			(set == &ctrl->tag_set) ? hctx_idx + 1 : 0);
 }
-- 
2.43.0

[PATCH 4/6] nvme-mpath: support controller crd when failing over request

[Sagi Grimberg]

When failing over a request (due to a path based status) we should
repect controller crd returned in the nvme completion as much as
possible. Hence we want to delay the failover command execution by
the controller crdt.

We allocate a new nvme_mpath_failover_timer referencing the request
stolen bios in a staging list, and when the command retry delay expires,
and only then the bios are moved to the mpath head requeue list which is
immediately kicked to re-submit these bios. If we failed to allocate
a fot, we fallback to the existing behavior.

Given that we now have a new staging list for mpath devices, we drain
them when removing the device.

Signed-off-by: Sagi Grimberg <sagi@grimberg.me>
---
 drivers/nvme/host/multipath.c | 96 +++++++++++++++++++++++++++++++++--
 drivers/nvme/host/nvme.h      |  1 +
 2 files changed, 93 insertions(+), 4 deletions(-)

diff --git a/drivers/nvme/host/multipath.c b/drivers/nvme/host/multipath.c
index b5501217303c..959dd1e05a2d 100644
--- a/drivers/nvme/host/multipath.c
+++ b/drivers/nvme/host/multipath.c
@@ -9,6 +9,13 @@
 #include <trace/events/block.h>
 #include "nvme.h"
 
+struct nvme_mpath_failover_timer {
+	struct list_head	entry;
+	struct nvme_ns_head	*head;
+	struct bio_list		bios;
+	struct timer_list	timer;
+};
+
 bool multipath = true;
 static bool multipath_always_on;
 
@@ -144,10 +151,48 @@ void nvme_mpath_start_freeze(struct nvme_subsystem *subsys)
 			blk_freeze_queue_start(h->disk->queue);
 }
 
+static void nvme_mpath_failover_timer_fn(struct timer_list *t)
+{
+	struct nvme_mpath_failover_timer *fot = timer_container_of(fot, t, timer);
+	struct nvme_ns_head *head = fot->head;
+	unsigned long flags;
+
+	spin_lock_irqsave(&head->requeue_lock, flags);
+	if (list_empty(&fot->entry)) {
+		spin_unlock_irqrestore(&head->requeue_lock, flags);
+		return;
+	}
+
+	list_del_init(&fot->entry);
+	if (fot->bios.head)
+		bio_list_merge(&head->requeue_list, &fot->bios);
+	spin_unlock_irqrestore(&head->requeue_lock, flags);
+	kblockd_schedule_work(&head->requeue_work);
+	kfree(fot);
+}
+
+static struct nvme_mpath_failover_timer *
+nvme_mpath_alloc_failover_timer(struct nvme_ns_head *head)
+{
+	struct nvme_mpath_failover_timer *fot;
+
+	fot = kzalloc(sizeof(*fot), GFP_ATOMIC);
+	if (!fot)
+		goto out;
+	fot->head = head;
+	bio_list_init(&fot->bios);
+	INIT_LIST_HEAD(&fot->entry);
+	timer_setup(&fot->timer, nvme_mpath_failover_timer_fn, 0);
+out:
+	return fot;
+}
+
 void nvme_failover_req(struct request *req)
 {
 	struct nvme_ns *ns = req->q->queuedata;
 	u16 status = nvme_req(req)->status & NVME_SCT_SC_MASK;
+	struct nvme_mpath_failover_timer *fot = NULL;
+	unsigned int delay;
 	unsigned long flags;
 	struct bio *bio;
 
@@ -167,13 +212,27 @@ void nvme_failover_req(struct request *req)
 	for (bio = req->bio; bio; bio = bio->bi_next)
 		bio_set_dev(bio, ns->head->disk->part0);
 
-	spin_lock_irqsave(&ns->head->requeue_lock, flags);
-	blk_steal_bios(&ns->head->requeue_list, req);
-	spin_unlock_irqrestore(&ns->head->requeue_lock, flags);
+	delay = nvme_crd_msecs(nvme_req(req));
+	if (delay) {
+		fot = nvme_mpath_alloc_failover_timer(ns->head);
+		if (fot) {
+			blk_steal_bios(&fot->bios, req);
+			spin_lock_irqsave(&ns->head->requeue_lock, flags);
+			list_add_tail(&fot->entry, &ns->head->fots);
+			spin_unlock_irqrestore(&ns->head->requeue_lock, flags);
+			mod_timer(&fot->timer, jiffies + msecs_to_jiffies(delay));
+		}
+	}
+	/* no CRD or timer allocation failed, fallback to immediate failover */
+	if (!fot) {
+		spin_lock_irqsave(&ns->head->requeue_lock, flags);
+		blk_steal_bios(&ns->head->requeue_list, req);
+		spin_unlock_irqrestore(&ns->head->requeue_lock, flags);
+		kblockd_schedule_work(&ns->head->requeue_work);
+	}
 
 	nvme_req(req)->status = 0;
 	nvme_end_req(req);
-	kblockd_schedule_work(&ns->head->requeue_work);
 }
 
 void nvme_mpath_start_request(struct request *rq)
@@ -689,6 +748,32 @@ static void nvme_requeue_work(struct work_struct *work)
 	}
 }
 
+static void nvme_mpath_drain_failover_timers(struct nvme_ns_head *head)
+{
+	struct nvme_mpath_failover_timer *fot;
+	unsigned long flags;
+
+	while (true) {
+		spin_lock_irqsave(&head->requeue_lock, flags);
+		fot = list_first_entry_or_null(&head->fots,
+			struct nvme_mpath_failover_timer, entry);
+		if (!fot) {
+			spin_unlock_irqrestore(&head->requeue_lock, flags);
+			return;
+		}
+		list_del_init(&fot->entry);
+		spin_unlock_irqrestore(&head->requeue_lock, flags);
+
+		timer_delete_sync(&fot->timer);
+
+		spin_lock_irqsave(&head->requeue_lock, flags);
+		if (fot->bios.head)
+			bio_list_merge(&head->requeue_list, &fot->bios);
+		spin_unlock_irqrestore(&head->requeue_lock, flags);
+		kfree(fot);
+	}
+}
+
 static void nvme_remove_head(struct nvme_ns_head *head)
 {
 	if (test_and_clear_bit(NVME_NSHEAD_DISK_LIVE, &head->flags)) {
@@ -696,6 +781,7 @@ static void nvme_remove_head(struct nvme_ns_head *head)
 		 * requeue I/O after NVME_NSHEAD_DISK_LIVE has been cleared
 		 * to allow multipath to fail all I/O.
 		 */
+		nvme_mpath_drain_failover_timers(head);
 		kblockd_schedule_work(&head->requeue_work);
 
 		if (test_and_clear_bit(NVME_NSHEAD_CDEV_LIVE, &head->flags))
@@ -731,6 +817,7 @@ int nvme_mpath_alloc_disk(struct nvme_ctrl *ctrl, struct nvme_ns_head *head)
 	mutex_init(&head->lock);
 	bio_list_init(&head->requeue_list);
 	spin_lock_init(&head->requeue_lock);
+	INIT_LIST_HEAD(&head->fots);
 	INIT_WORK(&head->requeue_work, nvme_requeue_work);
 	INIT_WORK(&head->partition_scan_work, nvme_partition_scan_work);
 	INIT_DELAYED_WORK(&head->remove_work, nvme_remove_head_work);
@@ -1424,6 +1511,7 @@ void nvme_mpath_put_disk(struct nvme_ns_head *head)
 	if (!head->disk)
 		return;
 	/* make sure all pending bios are cleaned up */
+	nvme_mpath_drain_failover_timers(head);
 	kblockd_schedule_work(&head->requeue_work);
 	flush_work(&head->requeue_work);
 	flush_work(&head->partition_scan_work);
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index 944462e912f6..e63f21b3110c 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -565,6 +565,7 @@ struct nvme_ns_head {
 	struct bio_list		requeue_list;
 	spinlock_t		requeue_lock;
 	struct work_struct	requeue_work;
+	struct list_head	fots;
 	struct work_struct	partition_scan_work;
 	struct mutex		lock;
 	unsigned long		flags;
-- 
2.43.0

[PATCH 5/6] nvmet: Add support for configurable crdt (command retry delay) attributes

[Sagi Grimberg]

Paired with host side error injection, allows us to exercise host retry delay
behavior.

Signed-off-by: Sagi Grimberg <sagi@grimberg.me>
---
 drivers/nvme/target/admin-cmd.c |  4 +++
 drivers/nvme/target/configfs.c  | 62 +++++++++++++++++++++++++++++++++
 drivers/nvme/target/nvmet.h     |  1 +
 3 files changed, 67 insertions(+)

diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
index 01b799e92ae6..c1a343e9a58a 100644
--- a/drivers/nvme/target/admin-cmd.c
+++ b/drivers/nvme/target/admin-cmd.c
@@ -786,6 +786,10 @@ static void nvmet_execute_identify_ctrl(struct nvmet_req *req)
 
 	id->nwpc = 1 << 0; /* write protect and no write protect */
 
+	id->crdt1 = cpu_to_le16(subsys->crdt[0]);
+	id->crdt2 = cpu_to_le16(subsys->crdt[1]);
+	id->crdt3 = cpu_to_le16(subsys->crdt[2]);
+
 	status = nvmet_copy_to_sgl(req, 0, id, sizeof(*id));
 
 	kfree(id);
diff --git a/drivers/nvme/target/configfs.c b/drivers/nvme/target/configfs.c
index 2b69ffcfc8df..7bd51f176da0 100644
--- a/drivers/nvme/target/configfs.c
+++ b/drivers/nvme/target/configfs.c
@@ -1470,6 +1470,65 @@ static ssize_t nvmet_subsys_attr_subsys_vendor_id_store(struct config_item *item
 }
 CONFIGFS_ATTR(nvmet_subsys_, attr_subsys_vendor_id);
 
+static ssize_t nvmet_subsys_attr_crdt_show(struct config_item *item,
+		char *page, unsigned int idx)
+{
+	return sysfs_emit(page, "%u\n", to_subsys(item)->crdt[idx]);
+}
+
+static ssize_t nvmet_subsys_attr_crdt_store(struct config_item *item,
+		const char *page, size_t count, unsigned int idx)
+{
+	u16 crdt;
+
+	if (kstrtou16(page, 0, &crdt))
+		return -EINVAL;
+
+	down_write(&nvmet_config_sem);
+	to_subsys(item)->crdt[idx] = crdt;
+	up_write(&nvmet_config_sem);
+	return count;
+}
+
+static ssize_t nvmet_subsys_attr_crdt1_show(struct config_item *item,
+		char *page)
+{
+	return nvmet_subsys_attr_crdt_show(item, page, 0);
+}
+
+static ssize_t nvmet_subsys_attr_crdt1_store(struct config_item *item,
+		const char *page, size_t count)
+{
+	return nvmet_subsys_attr_crdt_store(item, page, count, 0);
+}
+CONFIGFS_ATTR(nvmet_subsys_, attr_crdt1);
+
+static ssize_t nvmet_subsys_attr_crdt2_show(struct config_item *item,
+		char *page)
+{
+	return nvmet_subsys_attr_crdt_show(item, page, 1);
+}
+
+static ssize_t nvmet_subsys_attr_crdt2_store(struct config_item *item,
+		const char *page, size_t count)
+{
+	return nvmet_subsys_attr_crdt_store(item, page, count, 1);
+}
+CONFIGFS_ATTR(nvmet_subsys_, attr_crdt2);
+
+static ssize_t nvmet_subsys_attr_crdt3_show(struct config_item *item,
+		char *page)
+{
+	return nvmet_subsys_attr_crdt_show(item, page, 2);
+}
+
+static ssize_t nvmet_subsys_attr_crdt3_store(struct config_item *item,
+		const char *page, size_t count)
+{
+	return nvmet_subsys_attr_crdt_store(item, page, count, 2);
+}
+CONFIGFS_ATTR(nvmet_subsys_, attr_crdt3);
+
 static ssize_t nvmet_subsys_attr_model_show(struct config_item *item,
 					    char *page)
 {
@@ -1700,6 +1759,9 @@ static struct configfs_attribute *nvmet_subsys_attrs[] = {
 	&nvmet_subsys_attr_attr_cntlid_max,
 	&nvmet_subsys_attr_attr_vendor_id,
 	&nvmet_subsys_attr_attr_subsys_vendor_id,
+	&nvmet_subsys_attr_attr_crdt1,
+	&nvmet_subsys_attr_attr_crdt2,
+	&nvmet_subsys_attr_attr_crdt3,
 	&nvmet_subsys_attr_attr_model,
 	&nvmet_subsys_attr_attr_qid_max,
 	&nvmet_subsys_attr_attr_ieee_oui,
diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
index aaba745e3c21..2650c5e101f3 100644
--- a/drivers/nvme/target/nvmet.h
+++ b/drivers/nvme/target/nvmet.h
@@ -347,6 +347,7 @@ struct nvmet_subsys {
 
 	u16			vendor_id;
 	u16			subsys_vendor_id;
+	u16			crdt[3];
 	char			*model_number;
 	u32			ieee_oui;
 	char			*firmware_rev;
-- 
2.43.0

[PATCH 6/6] nvme/fault-injection: Support for error injection with custom crd

[Sagi Grimberg]

This assumes that the controller exposed crdt[1,2,3] that are actually
meaningful in order to have make difference.

Signed-off-by: Sagi Grimberg <sagi@grimberg.me>
---
 drivers/nvme/host/fault_inject.c | 8 ++++++--
 drivers/nvme/host/nvme.h         | 1 +
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/nvme/host/fault_inject.c b/drivers/nvme/host/fault_inject.c
index 105d6cb41c72..98363fc6dde7 100644
--- a/drivers/nvme/host/fault_inject.c
+++ b/drivers/nvme/host/fault_inject.c
@@ -42,11 +42,13 @@ void nvme_fault_inject_init(struct nvme_fault_inject *fault_inj,
 	}
 	fault_inj->parent = parent;
 
-	/* create debugfs for status code and dont_retry */
+	/* create debugfs for status code, dont_retry and crd */
 	fault_inj->status = NVME_SC_INVALID_OPCODE;
 	fault_inj->dont_retry = true;
+	fault_inj->crd = 0;
 	debugfs_create_x16("status", 0600, dir,	&fault_inj->status);
 	debugfs_create_bool("dont_retry", 0600, dir, &fault_inj->dont_retry);
+	debugfs_create_u8("crd", 0600, dir, &fault_inj->crd);
 }
 
 void nvme_fault_inject_fini(struct nvme_fault_inject *fault_inject)
@@ -73,10 +75,12 @@ void nvme_should_fail(struct request *req)
 	}
 
 	if (fault_inject && should_fail(&fault_inject->attr, 1)) {
-		/* inject status code and DNR bit */
+		/* inject status code, optional DNR and CRD */
 		status = fault_inject->status;
 		if (fault_inject->dont_retry)
 			status |= NVME_STATUS_DNR;
+		if (fault_inject->crd && fault_inject->crd <= 3)
+			status |= (fault_inject->crd << 11) & NVME_STATUS_CRD;
 		nvme_req(req)->status =	status;
 	}
 }
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index e63f21b3110c..9aa0fc5e473a 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -318,6 +318,7 @@ struct nvme_fault_inject {
 	struct fault_attr attr;
 	struct dentry *parent;
 	bool dont_retry;	/* DNR, do not retry */
+	u8 crd;			/* Command Retry Delay (0-3) */
 	u16 status;		/* status code */
 #endif
 };
-- 
2.43.0

[PATCH 7/6 RFC] nvme: test per-command retry delay

[Sagi Grimberg]

Add tests to exercise host command retry delays handling.

070: check that basic command RETRY disposition works and respect ctrl
     crd
071: check that basic command FAILOVER disposition works and respects
     ctrl crd
072: check that different commands completed with different crd levels
     are retried independently, each respecting its paired completion
     crd level
073: check that different commands completed with different crd levels
     are failed-over independently, each respecting its paired completion
     crd level

These tests rely on nvmet support for subsystem crdt attributes
(_require_nvmet_crdt) and nvme host crd error injection support.

In addition we add some common nvme helpers to set nvmet attributes,
inject errors, and leverage nvme diags to count retries/failovers.

Signed-off-by: Sagi Grimberg <sagi@grimberg.me>
---
 common/nvme        | 281 +++++++++++++++++++++++++++++++++++++++++++++
 tests/nvme/070     |  95 +++++++++++++++
 tests/nvme/070.out |   3 +
 tests/nvme/071     | 185 +++++++++++++++++++++++++++++
 tests/nvme/071.out |   3 +
 tests/nvme/072     | 118 +++++++++++++++++++
 tests/nvme/072.out |   8 ++
 tests/nvme/073     | 157 +++++++++++++++++++++++++
 tests/nvme/073.out |   8 ++
 tests/nvme/rc      |  24 +++-
 10 files changed, 881 insertions(+), 1 deletion(-)
 create mode 100755 tests/nvme/070
 create mode 100644 tests/nvme/070.out
 create mode 100755 tests/nvme/071
 create mode 100644 tests/nvme/071.out
 create mode 100755 tests/nvme/072
 create mode 100644 tests/nvme/072.out
 create mode 100755 tests/nvme/073
 create mode 100644 tests/nvme/073.out

diff --git a/common/nvme b/common/nvme
index f3999378db2d..a224dca61840 100644
--- a/common/nvme
+++ b/common/nvme
@@ -1527,3 +1527,284 @@ _nvme_requires() {
 
 	return 0
 }
+
+_require_nvmet_crdt() {
+	local subsysnqn="${1:-$def_subsysnqn}"
+	local crdt_attr="${NVMET_CFS}/subsystems/${subsysnqn}/attr_crdt1"
+	local tmp="blktests-crdt-probe-$$"
+
+	_have_driver nvmet || return $?
+	_have_configfs || return $?
+
+	if [[ -e "${crdt_attr}" ]]; then
+		return 0
+	fi
+
+	if [[ ! -d "${NVMET_CFS}/subsystems" ]]; then
+		SKIP_REASONS+=("nvmet configfs is not available")
+		return 1
+	fi
+
+	mkdir "${NVMET_CFS}/subsystems/${tmp}" || {
+		SKIP_REASONS+=("unable to create nvmet subsystem for CRDT probe")
+		return 1
+	}
+	if [[ ! -e "${NVMET_CFS}/subsystems/${tmp}/attr_crdt1" ]]; then
+		rmdir "${NVMET_CFS}/subsystems/${tmp}"
+		SKIP_REASONS+=("nvmet does not support attr_crdt1/2/3")
+		return 1
+	fi
+	rmdir "${NVMET_CFS}/subsystems/${tmp}"
+	return 0
+}
+
+# Set Identify Controller CRDT values (units of 100ms). Call before connect.
+_nvmet_set_crdt() {
+	local subsysnqn="${def_subsysnqn}"
+	local crdt1=0
+	local crdt2=0
+	local crdt3=0
+	local cfs
+
+	while [[ $# -gt 0 ]]; do
+		case $1 in
+			--subsysnqn)
+				subsysnqn="$2"
+				shift 2
+				;;
+			*)
+				crdt1="$1"
+				crdt2="${2:-0}"
+				crdt3="${3:-0}"
+				shift $#
+				;;
+		esac
+	done
+
+	cfs="${NVMET_CFS}/subsystems/${subsysnqn}"
+	_set_attr "${crdt1}" "${cfs}/attr_crdt1"
+	_set_attr "${crdt2}" "${cfs}/attr_crdt2"
+	_set_attr "${crdt3}" "${cfs}/attr_crdt3"
+}
+
+# List hidden multipath path namespaces sharing uuid with head ns.
+_nvme_path_ns_devs() {
+	local head_ns="$1"
+	local uuid
+	local ns
+	local found=0
+
+	uuid="$(cat "/sys/block/${head_ns}/uuid" 2>/dev/null)" || return 1
+
+	shopt -s nullglob
+	for ns in /sys/block/nvme*c*n*; do
+		[[ -e "${ns}/uuid" ]] || continue
+		if [[ "$(cat "${ns}/uuid")" == "${uuid}" ]]; then
+			basename "${ns}"
+			found=1
+		fi
+	done
+	shopt -u nullglob
+
+	# Fallback: controllers may expose path ns under /sys/class/nvme.
+	if (( found == 0 )); then
+		shopt -s nullglob
+		for ns in /sys/class/nvme/nvme*/nvme*c*n*; do
+			[[ -e "${ns}/uuid" ]] || continue
+			if [[ "$(cat "${ns}/uuid")" == "${uuid}" ]]; then
+				basename "${ns}"
+				found=1
+			fi
+		done
+		shopt -u nullglob
+	fi
+
+	(( found == 1 ))
+}
+
+_nvme_ns_diag_path() {
+	local ns="$1"
+	local attr="$2"
+
+	echo "/sys/block/${ns}/diag/${attr}"
+}
+
+_nvme_get_ns_diag() {
+	local path
+
+	path="$(_nvme_ns_diag_path "$1" "$2")"
+	[[ -e "${path}" ]] || { echo 0; return 1; }
+	cat "${path}"
+}
+
+_nvme_set_ns_diag() {
+	local path
+
+	path="$(_nvme_ns_diag_path "$1" "$2")"
+	[[ -e "${path}" ]] || return 1
+	_set_attr "$3" "${path}"
+}
+
+_nvme_sum_path_diag() {
+	local head_ns="$1"
+	local attr="$2"
+	local path_ns
+	local sum=0
+	local val
+
+	while read -r path_ns; do
+		[[ -z "${path_ns}" ]] && continue
+		val="$(_nvme_get_ns_diag "${path_ns}" "${attr}")"
+		sum=$((sum + val))
+	done < <(_nvme_path_ns_devs "${head_ns}")
+	echo "${sum}"
+}
+
+_nvme_reset_path_diag() {
+	local head_ns="$1"
+	local attr="$2"
+	local path_ns
+
+	while read -r path_ns; do
+		[[ -z "${path_ns}" ]] && continue
+		_nvme_set_ns_diag "${path_ns}" "${attr}" 0 || true
+	done < <(_nvme_path_ns_devs "${head_ns}")
+}
+
+# Fault-inject targets for a namespace: path ns devices if multipath, else the ns.
+_nvme_fault_inject_devs() {
+	local ns="$1"
+	local paths
+
+	paths="$(_nvme_path_ns_devs "${ns}")"
+	if [[ -n "${paths}" ]]; then
+		echo "${paths}"
+	else
+		echo "${ns}"
+	fi
+}
+
+# Resolve the nvme controller sysfs/debugfs name for a namespace device.
+_nvme_ctrl_dev_from_ns() {
+	local ns="$1"
+	local ctrl
+
+	if [[ "${ns}" =~ ^nvme[0-9]+c[0-9]+n[0-9]+$ ]]; then
+		ctrl="$(basename "$(readlink -f "/sys/block/${ns}/device")")"
+		echo "${ctrl}"
+		return 0
+	fi
+	echo "${ns%n*}"
+}
+
+_nvme_now_ms() {
+	echo $(($(date +%s%N) / 1000000))
+}
+
+# Arm host fault inject on a namespace/path device.
+# Args: <dev> <dont_retry> <status> <crd> <times> [probability=100] [verbose=1]
+_nvme_arm_crd_inject() {
+	local dev=$1
+	local dont_retry=$2
+	local status=$3
+	local crd=$4
+	local times=$5
+	local probability=${6:-100}
+	local verbose=${7:-1}
+	local fi="/sys/kernel/debug/${dev}/fault_inject"
+
+	if [[ ! -d "${fi}" ]]; then
+		echo "FAIL: missing ${fi}"
+		return 1
+	fi
+	if [[ ! -e "${fi}/crd" ]]; then
+		echo "FAIL: missing ${fi}/crd"
+		return 1
+	fi
+
+	_set_attr "${verbose}" "${fi}/verbose"
+	_set_attr "${dont_retry}" "${fi}/dont_retry"
+	_set_attr "${status}" "${fi}/status"
+	_set_attr "${crd}" "${fi}/crd"
+	_set_attr "${times}" "${fi}/times"
+	_set_attr "${probability}" "${fi}/probability"
+	return 0
+}
+
+_nvme_disarm_crd_inject() {
+	local fi="/sys/kernel/debug/$1/fault_inject"
+
+	[[ -d "${fi}" ]] || return 0
+	_set_attr 0 "${fi}/probability"
+	_set_attr 0 "${fi}/times"
+	_set_attr 0 "${fi}/crd"
+}
+
+# Timed direct write; prints elapsed milliseconds on stdout.
+_nvme_timed_direct_write() {
+	local dev=$1
+	local start end
+
+	start="$(_nvme_now_ms)"
+	dd if=/dev/zero of="${dev}" bs=4k count=1 oflag=direct status=none \
+		conv=notrunc 2>>"$FULL" || return 1
+	end="$(_nvme_now_ms)"
+	echo $((end - start))
+}
+
+# Background timed direct write. Sets nvme_bg_timed_pid (do NOT call from $()).
+# Writes elapsed ms to <result_file>, or FAIL on I/O error.
+_nvme_bg_timed_direct_write() {
+	local dev=$1
+	local result=$2
+
+	rm -f "${result}"
+	(
+		local start end
+		start="$(_nvme_now_ms)"
+		if dd if=/dev/zero of="${dev}" bs=4k count=1 oflag=direct \
+			status=none conv=notrunc 2>>"$FULL"; then
+			end="$(_nvme_now_ms)"
+			echo $((end - start)) >"${result}"
+		else
+			echo FAIL >"${result}"
+		fi
+	) &
+	nvme_bg_timed_pid=$!
+}
+
+# Wait until fault_inject times reaches 0 (inject consumed).
+_nvme_wait_inject_consumed() {
+	local fi="/sys/kernel/debug/$1/fault_inject/times"
+	local timeout_ms=${2:-2000}
+	local start
+
+	[[ -e "${fi}" ]] || return 1
+	start="$(_nvme_now_ms)"
+	while (( $(cat "${fi}") > 0 )); do
+		if (( $(_nvme_now_ms) - start > timeout_ms )); then
+			return 1
+		fi
+		sleep 0.01
+	done
+	return 0
+}
+
+# Validate elapsed ms against an expected CRD delay.
+# Args: <label> <elapsed_ms> <expected_ms> [max_ms=expected*2]
+_nvme_check_crd_elapsed() {
+	local label=$1
+	local elapsed=$2
+	local expected=$3
+	local max_ms=${4:-$((expected * 2))}
+
+	if (( elapsed < expected / 2 )); then
+		echo "FAIL: ${label} too fast (${elapsed}ms), expected ~${expected}ms"
+		return 1
+	fi
+	if (( elapsed >= max_ms )); then
+		echo "FAIL: ${label} too slow (${elapsed}ms), expected ~${expected}ms (max ${max_ms}ms)"
+		return 1
+	fi
+	return 0
+}
diff --git a/tests/nvme/070 b/tests/nvme/070
new file mode 100755
index 000000000000..14d7160664a0
--- /dev/null
+++ b/tests/nvme/070
@@ -0,0 +1,95 @@
+#!/bin/bash
+# SPDX-License-Identifier: GPL-3.0+
+# Copyright (C) 2026 Sagi Grimberg <sagi@grimberg.me>
+#
+# Test NVMe command retry delay (CRD) with the per-request retry timer.
+# Requires nvmet attr_crdt* and host fault_inject/crd.
+
+. tests/nvme/rc
+
+DESCRIPTION="test NVMe CRD per-request retry under fio"
+QUICK=1
+
+# NVME_SC_INTERNAL
+NVME_SC_INTERNAL=0x6
+
+requires() {
+	_nvme_requires
+	_have_loop
+	_have_fio
+	_have_kernel_options FAULT_INJECTION FAULT_INJECTION_DEBUG_FS
+	_require_nvme_trtype_is_fabrics
+	_require_nvmet_crdt
+}
+
+set_conditions() {
+	_set_nvme_trtype "$@"
+}
+
+inject_crd_retries() {
+	local -a inject_devs
+	local dev
+	local ctrl
+	local i
+
+	mapfile -t inject_devs < <(_nvme_fault_inject_devs "$1")
+	for ((i = 0; i < 5; i++)); do
+		for dev in "${inject_devs[@]}"; do
+			ctrl="$(_nvme_ctrl_dev_from_ns "${dev}")"
+			_nvme_err_inject_setup "${dev}" "${ctrl}"
+			# verbose=1 probability=100 dont_retry=0 status=INTERNAL crd=1 times=8
+			_nvme_enable_crd_err_inject "${dev}" 1 100 0 \
+				"${NVME_SC_INTERNAL}" 1 8
+		done
+		sleep 1
+		for dev in "${inject_devs[@]}"; do
+			ctrl="$(_nvme_ctrl_dev_from_ns "${dev}")"
+			_nvme_disable_err_inject "${dev}"
+			_nvme_err_inject_cleanup "${dev}" "${ctrl}"
+		done
+		sleep 1
+	done
+}
+
+test() {
+	local fio_pid
+	local ns
+	local retries_before
+	local retries_after
+	local inject_dev
+
+	echo "Running ${TEST_NAME}"
+
+	_setup_nvmet
+	_nvmet_target_setup
+	# CRDT1 = 5 * 100ms = 500ms
+	_nvmet_set_crdt 5 0 0
+
+	_nvme_connect_subsys
+	ns=$(_find_nvme_ns "${def_subsys_uuid}")
+
+	inject_dev=$(_nvme_fault_inject_devs "${ns}" | head -1)
+	_nvme_set_ns_diag "${inject_dev}" command_retries_count 0 || true
+	retries_before=$(_nvme_get_ns_diag "${inject_dev}" command_retries_count)
+
+	_run_fio_verify_io --filename="/dev/${ns}" \
+			   --group_reporting --ramp_time=2 \
+			   --time_based --runtime=20 &> "$FULL" &
+	fio_pid=$!
+	sleep 3
+
+	echo "Injecting CRD retries"
+	inject_crd_retries "${ns}"
+
+	wait "${fio_pid}" || echo "FAIL: fio exited with errors (see $FULL)"
+
+	retries_after=$(_nvme_get_ns_diag "${inject_dev}" command_retries_count)
+	if (( retries_after <= retries_before )); then
+		echo "command_retries_count did not increase (${retries_before} -> ${retries_after})"
+	fi
+
+	_nvme_disconnect_subsys
+	_nvmet_target_cleanup
+
+	echo "Test complete"
+}
diff --git a/tests/nvme/070.out b/tests/nvme/070.out
new file mode 100644
index 000000000000..bf52d7b13a5e
--- /dev/null
+++ b/tests/nvme/070.out
@@ -0,0 +1,3 @@
+Running nvme/070
+Injecting CRD retries
+Test complete
diff --git a/tests/nvme/071 b/tests/nvme/071
new file mode 100755
index 000000000000..897a88302b75
--- /dev/null
+++ b/tests/nvme/071
@@ -0,0 +1,185 @@
+#!/bin/bash
+# SPDX-License-Identifier: GPL-3.0+
+# Copyright (C) 2026 Sagi Grimberg <sagi@grimberg.me>
+#
+# Test NVMe multipath failover respects controller CRD. Requires nvmet
+# attr_crdt*, host fault_inject/crd, and nvme_core.multipath=Y.
+
+. tests/nvme/rc
+
+DESCRIPTION="test NVMe CRD multipath failover under fio"
+QUICK=1
+
+# NVME_SC_INTERNAL_PATH_ERROR
+NVME_SC_INTERNAL_PATH_ERROR=0x300
+
+requires() {
+	_nvme_requires
+	_have_loop
+	_have_fio
+	_have_module_param_value nvme_core multipath Y
+	_have_kernel_options FAULT_INJECTION FAULT_INJECTION_DEBUG_FS
+	_require_nvme_trtype_is_fabrics
+	_require_nvmet_crdt
+}
+
+set_conditions() {
+	_set_nvme_trtype "$@"
+}
+
+dump_fault_inject() {
+	local fi="/sys/kernel/debug/$1/fault_inject"
+	local f
+
+	echo "fault_inject $1:" >> "$FULL"
+	if [[ ! -d "${fi}" ]]; then
+		echo "  MISSING ${fi}" >> "$FULL"
+		return
+	fi
+	for f in "${fi}"/*; do
+		echo "  $(basename "$f")=$(cat "$f" 2>/dev/null)" >> "$FULL"
+	done
+}
+
+# Arm path-error+CRD inject on a single path; leave the peer path clean.
+arm_crd_failover_inject() {
+	local dev=$1
+	local fi="/sys/kernel/debug/${dev}/fault_inject"
+	local status_val dnr_val crd_val prob_val
+
+	if [[ ! -d "${fi}" ]]; then
+		echo "FAIL: missing ${fi}"
+		dump_fault_inject "${dev}"
+		return 1
+	fi
+	if [[ ! -e "${fi}/crd" ]]; then
+		echo "FAIL: missing ${fi}/crd (rebuild/install nvme-core with CRD fault inject)"
+		dump_fault_inject "${dev}"
+		return 1
+	fi
+
+	# Avoid ctrl-side setup: only the path ns debugfs matters for data I/O.
+	# status/dont_retry/crd before probability. verbose=1 logs each injection.
+	_set_attr 1 "${fi}/verbose"
+	_set_attr 0 "${fi}/dont_retry"
+	_set_attr "${NVME_SC_INTERNAL_PATH_ERROR}" "${fi}/status"
+	_set_attr 1 "${fi}/crd"
+	_set_attr 100 "${fi}/times"
+	_set_attr 100 "${fi}/probability"
+
+	status_val="$(cat "${fi}/status")"
+	dnr_val="$(cat "${fi}/dont_retry")"
+	crd_val="$(cat "${fi}/crd")"
+	prob_val="$(cat "${fi}/probability")"
+	{
+		echo "armed ${dev}: status=${status_val} dont_retry=${dnr_val} crd=${crd_val} probability=${prob_val} times=$(cat "${fi}/times")"
+	} >> "$FULL"
+
+	# debugfs x16 prints as 0x0300
+	if [[ "${status_val}" != "0x0300" && "${status_val}" != "0x300" &&
+	      "$((status_val))" -ne "$((NVME_SC_INTERNAL_PATH_ERROR))" ]]; then
+		echo "FAIL: status not set (got ${status_val})"
+		dump_fault_inject "${dev}"
+		return 1
+	fi
+	if [[ "${dnr_val}" != "N" && "${dnr_val}" != "0" ]]; then
+		echo "FAIL: dont_retry not cleared (got ${dnr_val})"
+		dump_fault_inject "${dev}"
+		return 1
+	fi
+	if [[ "${crd_val}" != "1" ]]; then
+		echo "FAIL: crd not set (got ${crd_val})"
+		dump_fault_inject "${dev}"
+		return 1
+	fi
+	if [[ "${prob_val}" != "100" ]]; then
+		echo "FAIL: probability not set (got ${prob_val})"
+		dump_fault_inject "${dev}"
+		return 1
+	fi
+	return 0
+}
+
+disarm_crd_failover_inject() {
+	local fi="/sys/kernel/debug/$1/fault_inject"
+
+	_set_attr 0 "${fi}/probability"
+	_set_attr 0 "${fi}/times"
+	_set_attr 0 "${fi}/crd"
+}
+
+test() {
+	local fio_pid
+	local ns
+	local port
+	local -a ports
+	local -a path_devs
+	local inject_dev
+	local peer_dev
+	local inject_before
+	local inject_after
+
+	echo "Running ${TEST_NAME}"
+
+	_setup_nvmet
+	_nvmet_target_setup --ports 2
+	# CRDT1 = 2 * 100ms = 200ms; must be set before connect.
+	_nvmet_set_crdt 2 0 0
+
+	_get_nvmet_ports "${def_subsysnqn}" ports
+	for port in "${ports[@]}"; do
+		_setup_nvmet_port_ana "${port}" 1 "optimized"
+		_nvme_connect_subsys --port "${port}" --no-wait-ns
+	done
+	sleep 1
+
+	ns=$(_find_nvme_ns "${def_subsys_uuid}")
+	mapfile -t path_devs < <(_nvme_path_ns_devs "${ns}")
+	if ((${#path_devs[@]} < 2)); then
+		echo "FAIL: need >=2 path namespaces, found ${#path_devs[@]} (${path_devs[*]})"
+		_nvme_disconnect_subsys
+		_nvmet_target_cleanup
+		return 1
+	fi
+
+	inject_dev=${path_devs[0]}
+	peer_dev=${path_devs[1]}
+	echo "inject=${inject_dev} peer=${peer_dev}" >> "$FULL"
+	dump_fault_inject "${inject_dev}"
+
+	_nvme_set_ns_diag "${inject_dev}" multipath_failover_count 0 || true
+	_nvme_set_ns_diag "${peer_dev}" multipath_failover_count 0 || true
+	inject_before=$(_nvme_get_ns_diag "${inject_dev}" multipath_failover_count)
+
+	# Arm before fio so we know inject is configured; I/O should failover
+	# to the peer path (no EIO if CRD failover works).
+	echo "Injecting CRD failovers"
+	if ! arm_crd_failover_inject "${inject_dev}"; then
+		_nvme_disconnect_subsys
+		_nvmet_target_cleanup
+		return 1
+	fi
+
+	# Append fio output; do not truncate diagnostics already in $FULL.
+	_run_fio --name=crd-failover --filename="/dev/${ns}" \
+		--rw=randwrite --direct=1 --ioengine=libaio --bs=4k \
+		--iodepth=16 --time_based --runtime=10 \
+		--group_reporting >>"$FULL" 2>&1 &
+	fio_pid=$!
+
+	wait "${fio_pid}" || echo "FAIL: fio exited with errors (see $FULL)"
+
+	disarm_crd_failover_inject "${inject_dev}"
+
+	inject_after=$(_nvme_get_ns_diag "${inject_dev}" multipath_failover_count)
+	echo "failovers ${inject_dev}: ${inject_before} -> ${inject_after}" >> "$FULL"
+	if (( inject_after <= inject_before )); then
+		echo "FAIL: multipath_failover_count on ${inject_dev} did not increase (${inject_before} -> ${inject_after})"
+		dump_fault_inject "${inject_dev}"
+	fi
+
+	_nvme_disconnect_subsys
+	_nvmet_target_cleanup
+
+	echo "Test complete"
+}
diff --git a/tests/nvme/071.out b/tests/nvme/071.out
new file mode 100644
index 000000000000..2fca7041e467
--- /dev/null
+++ b/tests/nvme/071.out
@@ -0,0 +1,3 @@
+Running nvme/071
+Injecting CRD failovers
+Test complete
diff --git a/tests/nvme/072 b/tests/nvme/072
new file mode 100755
index 000000000000..2acda72cdd44
--- /dev/null
+++ b/tests/nvme/072
@@ -0,0 +1,118 @@
+#!/bin/bash
+# SPDX-License-Identifier: GPL-3.0+
+# Copyright (C) 2026 Sagi Grimberg <sagi@grimberg.me>
+#
+# Verify per-request CRD retry timers are independent: arm CRD2 then CRD1 on
+# two overlapping commands to the same namespace (fault_inject only holds one
+# crd at a time, but once latched on a request the timers run concurrently).
+# Each command must complete near its own CRDT, not a shared controller delay.
+
+. tests/nvme/rc
+
+DESCRIPTION="test NVMe CRD per-request retry timer independence"
+QUICK=1
+
+NVME_SC_INTERNAL=0x6
+# CRDT units are 100ms: CRD1=1s, CRD2=10s
+CRDT1=10
+CRDT2=100
+CRD1_MS=$((CRDT1 * 100))
+CRD2_MS=$((CRDT2 * 100))
+
+requires() {
+	_nvme_requires
+	_have_loop
+	_have_kernel_options FAULT_INJECTION FAULT_INJECTION_DEBUG_FS
+	_require_nvme_trtype_is_fabrics
+	_require_nvmet_crdt
+}
+
+set_conditions() {
+	_set_nvme_trtype "$@"
+}
+
+test() {
+	local ns
+	local inject_dev
+	local crd2_pid crd1_pid
+	local crd2_result crd1_result
+	local crd2_elapsed crd1_elapsed
+
+	echo "Running ${TEST_NAME}"
+
+	_setup_nvmet
+	_nvmet_target_setup
+	_nvmet_set_crdt "${CRDT1}" "${CRDT2}" 0
+
+	_nvme_connect_subsys
+	ns=$(_find_nvme_ns "${def_subsys_uuid}")
+	inject_dev=$(_nvme_fault_inject_devs "${ns}" | head -1)
+
+	echo "ns=${ns} inject=${inject_dev}" >>"$FULL"
+	echo "CRDT CRD1=${CRD1_MS}ms CRD2=${CRD2_MS}ms"
+
+	if [[ ! -e /sys/kernel/debug/${inject_dev}/fault_inject/crd ]]; then
+		echo "FAIL: missing fault_inject/crd on ${inject_dev}"
+		_nvme_disconnect_subsys
+		_nvmet_target_cleanup
+		return 1
+	fi
+
+	crd2_result="${TMPDIR}/crd2_elapsed"
+	crd1_result="${TMPDIR}/crd1_elapsed"
+
+	echo "Arming CRD2 and starting first write"
+	_nvme_arm_crd_inject "${inject_dev}" 0 "${NVME_SC_INTERNAL}" 2 1 || return 1
+	echo "inject: status=${NVME_SC_INTERNAL} crd=2 times=1" >>"$FULL"
+	_nvme_bg_timed_direct_write "/dev/${ns}" "${crd2_result}"
+	crd2_pid=$nvme_bg_timed_pid
+	echo "CRD2 write pid=${crd2_pid}" >>"$FULL"
+
+	if ! _nvme_wait_inject_consumed "${inject_dev}"; then
+		echo "FAIL: CRD2 inject was not consumed"
+		_nvme_disarm_crd_inject "${inject_dev}"
+		kill "${crd2_pid}" 2>/dev/null || true
+		wait "${crd2_pid}" 2>/dev/null || true
+		_nvme_disconnect_subsys
+		_nvmet_target_cleanup
+		return 1
+	fi
+	echo "CRD2 latched; arming CRD1 while CRD2 retry is pending" >>"$FULL"
+
+	echo "Arming CRD1 and starting second write (CRD2 still pending)"
+	_nvme_arm_crd_inject "${inject_dev}" 0 "${NVME_SC_INTERNAL}" 1 1 || return 1
+	echo "inject: status=${NVME_SC_INTERNAL} crd=1 times=1" >>"$FULL"
+	_nvme_bg_timed_direct_write "/dev/${ns}" "${crd1_result}"
+	crd1_pid=$nvme_bg_timed_pid
+	echo "CRD1 write pid=${crd1_pid}" >>"$FULL"
+
+	echo "Waiting for both writes (expect CRD1~${CRD1_MS}ms then CRD2~${CRD2_MS}ms)"
+	wait "${crd1_pid}" || true
+	wait "${crd2_pid}" || true
+	_nvme_disarm_crd_inject "${inject_dev}"
+	udevadm settle >/dev/null 2>&1 || true
+
+	crd1_elapsed="$(cat "${crd1_result}" 2>/dev/null || echo FAIL)"
+	crd2_elapsed="$(cat "${crd2_result}" 2>/dev/null || echo FAIL)"
+	echo "CRD1 write elapsed ${crd1_elapsed}ms (expected ~${CRD1_MS}ms)" >>"$FULL"
+	echo "CRD2 write elapsed ${crd2_elapsed}ms (expected ~${CRD2_MS}ms)" >>"$FULL"
+
+	if [[ "${crd1_elapsed}" == "FAIL" || -z "${crd1_elapsed}" ]]; then
+		echo "FAIL: CRD1 write did not complete"
+	else
+		echo "CRD1 write elapsed ${crd1_elapsed}ms" | sed -E 's/(elapsed )[0-9]+/\1NUM/'
+		_nvme_check_crd_elapsed "CRD1 write" "${crd1_elapsed}" "${CRD1_MS}" "$((CRD2_MS / 2))"
+	fi
+	if [[ "${crd2_elapsed}" == "FAIL" || -z "${crd2_elapsed}" ]]; then
+		echo "FAIL: CRD2 write did not complete"
+	else
+		echo "CRD2 write elapsed ${crd2_elapsed}ms" | sed -E 's/(elapsed )[0-9]+/\1NUM/'
+		_nvme_check_crd_elapsed "CRD2 write" "${crd2_elapsed}" "${CRD2_MS}"
+	fi
+
+	_nvme_disconnect_subsys
+	_nvmet_target_cleanup
+	udevadm settle >/dev/null 2>&1 || true
+
+	echo "Test complete"
+}
diff --git a/tests/nvme/072.out b/tests/nvme/072.out
new file mode 100644
index 000000000000..d9f4520c8d23
--- /dev/null
+++ b/tests/nvme/072.out
@@ -0,0 +1,8 @@
+Running nvme/072
+CRDT CRD1=1000ms CRD2=10000ms
+Arming CRD2 and starting first write
+Arming CRD1 and starting second write (CRD2 still pending)
+Waiting for both writes (expect CRD1~1000ms then CRD2~10000ms)
+CRD1 write elapsed NUMms
+CRD2 write elapsed NUMms
+Test complete
diff --git a/tests/nvme/073 b/tests/nvme/073
new file mode 100755
index 000000000000..34911392437a
--- /dev/null
+++ b/tests/nvme/073
@@ -0,0 +1,157 @@
+#!/bin/bash
+# SPDX-License-Identifier: GPL-3.0+
+# Copyright (C) 2026 Sagi Grimberg <sagi@grimberg.me>
+#
+# Verify per-failover CRD timers are independent on the same ns head.
+#
+# path0 stays usable after a path-error+CRD (only current_path is cleared), and
+# with NUMA/default selection the next head I/O typically picks path0 again.
+# So both commands are failed on path0 with different CRDs while their failover
+# timers overlap: CRD2 (~10s) then CRD1 (~1s). path1 is left clean so each
+# request can complete once its own CRDT elapses.
+
+. tests/nvme/rc
+
+DESCRIPTION="test NVMe CRD multipath failover timer independence"
+QUICK=1
+
+NVME_SC_INTERNAL_PATH_ERROR=0x300
+# CRDT units are 100ms: CRD1=1s, CRD2=10s
+CRDT1=10
+CRDT2=100
+CRD1_MS=$((CRDT1 * 100))
+CRD2_MS=$((CRDT2 * 100))
+
+requires() {
+	_nvme_requires
+	_have_loop
+	_have_module_param_value nvme_core multipath Y
+	_have_kernel_options FAULT_INJECTION FAULT_INJECTION_DEBUG_FS
+	_require_nvme_trtype_is_fabrics
+	_require_nvmet_crdt
+}
+
+set_conditions() {
+	_set_nvme_trtype "$@"
+}
+
+test() {
+	local ns
+	local port
+	local -a ports
+	local -a path_devs
+	local path0
+	local path1
+	local fo_before fo_after
+	local crd2_pid crd1_pid
+	local crd2_result crd1_result
+	local crd2_elapsed crd1_elapsed
+	local sync_start
+
+	echo "Running ${TEST_NAME}"
+
+	_setup_nvmet
+	_nvmet_target_setup --ports 2
+	_nvmet_set_crdt "${CRDT1}" "${CRDT2}" 0
+
+	_get_nvmet_ports "${def_subsysnqn}" ports
+	for port in "${ports[@]}"; do
+		_setup_nvmet_port_ana "${port}" 1 "optimized"
+		_nvme_connect_subsys --port "${port}" --no-wait-ns
+	done
+	sleep 1
+
+	ns=$(_find_nvme_ns "${def_subsys_uuid}")
+	mapfile -t path_devs < <(_nvme_path_ns_devs "${ns}")
+	if ((${#path_devs[@]} < 2)); then
+		echo "FAIL: need >=2 path namespaces, found ${#path_devs[@]} (${path_devs[*]})"
+		_nvme_disconnect_subsys
+		_nvmet_target_cleanup
+		return 1
+	fi
+
+	path0=${path_devs[0]}
+	path1=${path_devs[1]}
+	echo "ns=${ns} path0=${path0} path1=${path1}" >>"$FULL"
+	echo "CRDT CRD1=${CRD1_MS}ms CRD2=${CRD2_MS}ms"
+
+	if [[ ! -e /sys/kernel/debug/${path0}/fault_inject/crd ]]; then
+		echo "FAIL: missing fault_inject/crd on ${path0}"
+		_nvme_disconnect_subsys
+		_nvmet_target_cleanup
+		return 1
+	fi
+
+	_nvme_set_ns_diag "${path0}" multipath_failover_count 0 || true
+	_nvme_set_ns_diag "${path1}" multipath_failover_count 0 || true
+	fo_before=$(_nvme_get_ns_diag "${path0}" multipath_failover_count)
+
+	crd2_result="${TMPDIR}/crd2_elapsed"
+	crd1_result="${TMPDIR}/crd1_elapsed"
+
+	echo "Arming CRD2 on path0 and starting first write"
+	_nvme_arm_crd_inject "${path0}" 0 "${NVME_SC_INTERNAL_PATH_ERROR}" 2 1 || return 1
+	echo "path0 inject: path_error crd=2 times=1" >>"$FULL"
+	_nvme_bg_timed_direct_write "/dev/${ns}" "${crd2_result}"
+	crd2_pid=$nvme_bg_timed_pid
+	echo "CRD2 write pid=${crd2_pid}" >>"$FULL"
+
+	sync_start="$(_nvme_now_ms)"
+	while (( $(_nvme_get_ns_diag "${path0}" multipath_failover_count) <= fo_before )); do
+		if (( $(_nvme_now_ms) - sync_start > 2000 )); then
+			echo "FAIL: CRD2 failover was not scheduled on ${path0}"
+			_nvme_disarm_crd_inject "${path0}"
+			kill "${crd2_pid}" 2>/dev/null || true
+			wait "${crd2_pid}" 2>/dev/null || true
+			_nvme_disconnect_subsys
+			_nvmet_target_cleanup
+			return 1
+		fi
+		sleep 0.01
+	done
+	echo "CRD2 failover pending; path0 still selectable (NUMA/current)" >>"$FULL"
+
+	# Same path again: latch CRD1 while the CRD2 fot is still pending.
+	echo "Arming CRD1 on path0 and starting second write (CRD2 still pending)"
+	_nvme_arm_crd_inject "${path0}" 0 "${NVME_SC_INTERNAL_PATH_ERROR}" 1 1 || return 1
+	echo "path0 inject: path_error crd=1 times=1" >>"$FULL"
+	_nvme_bg_timed_direct_write "/dev/${ns}" "${crd1_result}"
+	crd1_pid=$nvme_bg_timed_pid
+	echo "CRD1 write pid=${crd1_pid}" >>"$FULL"
+
+	echo "Waiting for both writes (expect CRD1~${CRD1_MS}ms then CRD2~${CRD2_MS}ms)"
+	wait "${crd1_pid}" || true
+	wait "${crd2_pid}" || true
+	_nvme_disarm_crd_inject "${path0}"
+	udevadm settle >/dev/null 2>&1 || true
+
+	crd1_elapsed="$(cat "${crd1_result}" 2>/dev/null || echo FAIL)"
+	crd2_elapsed="$(cat "${crd2_result}" 2>/dev/null || echo FAIL)"
+	fo_after=$(_nvme_get_ns_diag "${path0}" multipath_failover_count)
+	echo "CRD1 failover elapsed ${crd1_elapsed}ms (expected ~${CRD1_MS}ms)" >>"$FULL"
+	echo "CRD2 failover elapsed ${crd2_elapsed}ms (expected ~${CRD2_MS}ms)" >>"$FULL"
+	echo "path0 failover count ${fo_before} -> ${fo_after}" >>"$FULL"
+
+	if (( fo_after < fo_before + 2 )); then
+		echo "FAIL: expected two failovers on path0, got ${fo_before} -> ${fo_after}"
+	fi
+
+	if [[ "${crd1_elapsed}" == "FAIL" || -z "${crd1_elapsed}" ]]; then
+		echo "FAIL: CRD1 failover write did not complete"
+	else
+		echo "CRD1 failover elapsed ${crd1_elapsed}ms" | sed -E 's/(elapsed )[0-9]+/\1NUM/'
+		_nvme_check_crd_elapsed "CRD1 failover" "${crd1_elapsed}" "${CRD1_MS}" "$((CRD2_MS / 2))"
+	fi
+	if [[ "${crd2_elapsed}" == "FAIL" || -z "${crd2_elapsed}" ]]; then
+		echo "FAIL: CRD2 failover write did not complete"
+	else
+		echo "CRD2 failover elapsed ${crd2_elapsed}ms" | sed -E 's/(elapsed )[0-9]+/\1NUM/'
+		_nvme_check_crd_elapsed "CRD2 failover" "${crd2_elapsed}" "${CRD2_MS}"
+	fi
+
+	_nvme_disconnect_subsys
+	_nvmet_target_cleanup
+	udevadm settle >/dev/null 2>&1 || true
+
+	echo "Test complete"
+}
diff --git a/tests/nvme/073.out b/tests/nvme/073.out
new file mode 100644
index 000000000000..e771abb14874
--- /dev/null
+++ b/tests/nvme/073.out
@@ -0,0 +1,8 @@
+Running nvme/073
+CRDT CRD1=1000ms CRD2=10000ms
+Arming CRD2 on path0 and starting first write
+Arming CRD1 on path0 and starting second write (CRD2 still pending)
+Waiting for both writes (expect CRD1~1000ms then CRD2~10000ms)
+CRD1 failover elapsed NUMms
+CRD2 failover elapsed NUMms
+Test complete
diff --git a/tests/nvme/rc b/tests/nvme/rc
index 31a0fc59ff4b..f286d9a95cce 100644
--- a/tests/nvme/rc
+++ b/tests/nvme/rc
@@ -505,17 +505,39 @@ _nvme_err_inject_cleanup()
 
 _nvme_enable_err_inject()
 {
+	# Set status/dont_retry[/crd] before arming probability/times so concurrent
+	# I/O cannot observe the debugfs defaults (INVALID_OPCODE + DNR).
         _set_attr "$2" /sys/kernel/debug/"$1"/fault_inject/verbose
-        _set_attr "$3" /sys/kernel/debug/"$1"/fault_inject/probability
         _set_attr "$4" /sys/kernel/debug/"$1"/fault_inject/dont_retry
         _set_attr "$5" /sys/kernel/debug/"$1"/fault_inject/status
+	if [[ -n "${7:-}" && -e /sys/kernel/debug/"$1"/fault_inject/crd ]]; then
+		_set_attr "$7" /sys/kernel/debug/"$1"/fault_inject/crd
+	fi
         _set_attr "$6" /sys/kernel/debug/"$1"/fault_inject/times
+        _set_attr "$3" /sys/kernel/debug/"$1"/fault_inject/probability
+}
+
+# Enable fault injection with a Command Retry Delay (CRD) level (1-3).
+# Args: <dev> <verbose> <probability> <dont_retry> <status> <crd> <times>
+_nvme_enable_crd_err_inject()
+{
+	local crd_file="/sys/kernel/debug/$1/fault_inject/crd"
+
+	if [[ ! -e "${crd_file}" ]]; then
+		echo "FAIL: fault_inject crd attribute missing on $1"
+		return 1
+	fi
+	# Map to _nvme_enable_err_inject args: times then crd.
+	_nvme_enable_err_inject "$1" "$2" "$3" "$4" "$5" "$7" "$6"
 }
 
 _nvme_disable_err_inject()
 {
         _set_attr 0 /sys/kernel/debug/"$1"/fault_inject/probability
         _set_attr 0 /sys/kernel/debug/"$1"/fault_inject/times
+	if [[ -e /sys/kernel/debug/"$1"/fault_inject/crd ]]; then
+		_set_attr 0 /sys/kernel/debug/"$1"/fault_inject/crd
+	fi
 }
 
 _nvme_enable_passthru_admin_error_logging()
-- 
2.43.0

Re: [PATCH 1/6] nvme-mpath: No need to protect req->bio with requeue_lock

[Hannes Reinecke]

On 8/23/26 10:48 AM, Sagi Grimberg wrote:
> requeue_lock protects the retry_list, no need to hold it while
> iterating request bios.
> 
> Signed-off-by: Sagi Grimberg <sagi@grimberg.me>
> ---
>   drivers/nvme/host/multipath.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/nvme/host/multipath.c b/drivers/nvme/host/multipath.c
> index 9b9a657fa330..b5501217303c 100644
> --- a/drivers/nvme/host/multipath.c
> +++ b/drivers/nvme/host/multipath.c
> @@ -164,9 +164,10 @@ void nvme_failover_req(struct request *req)
>   		queue_work(nvme_wq, &ns->ctrl->ana_work);
>   	}
>   
> -	spin_lock_irqsave(&ns->head->requeue_lock, flags);
>   	for (bio = req->bio; bio; bio = bio->bi_next)
>   		bio_set_dev(bio, ns->head->disk->part0);
> +
> +	spin_lock_irqsave(&ns->head->requeue_lock, flags);
>   	blk_steal_bios(&ns->head->requeue_list, req);
>   	spin_unlock_irqrestore(&ns->head->requeue_lock, flags);
>   
Reviewed-by: Hannes Reinecke <hare@kernel.org>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                  Kernel Storage Architect
hare@suse.de                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich

Re: [PATCH 2/6] nvme: add nvme_crd_msecs helper

[Hannes Reinecke]

On 8/23/26 10:48 AM, Sagi Grimberg wrote:
> Hide the details of extracting the crd value. We'll need it from other
> call-sites.
> 
> Signed-off-by: Sagi Grimberg <sagi@grimberg.me>
> ---
>   drivers/nvme/host/core.c |  8 +-------
>   drivers/nvme/host/nvme.h | 10 ++++++++++
>   2 files changed, 11 insertions(+), 7 deletions(-)
> 
Reviewed-by: Hannes Reinecke <hare@kernel.org>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                  Kernel Storage Architect
hare@suse.de                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich

Re: [PATCH 3/6] nvme: add per request retry timer

[Hannes Reinecke]

On 8/23/26 10:48 AM, Sagi Grimberg wrote:
> The existing command retry mechanism adds a request to retry
> to a retry list and modifies a request-queue (controller) wide
> dealyed queue timer.
> 
> The issue is that the existing requests in this queue may wait
> for longer periods of time as more requests are completed with
> a retry crd level.
> 
> Instead, add a per-request timer that will allow different
> requests retry in a way that is independent of other requests.
> 
> Signed-off-by: Sagi Grimberg <sagi@grimberg.me>
> ---
>   drivers/nvme/host/apple.c  |  1 +
>   drivers/nvme/host/core.c   | 24 ++++++++++++++++++++++--
>   drivers/nvme/host/fc.c     |  1 +
>   drivers/nvme/host/nvme.h   |  2 ++
>   drivers/nvme/host/pci.c    |  1 +
>   drivers/nvme/host/rdma.c   |  1 +
>   drivers/nvme/host/tcp.c    |  1 +
>   drivers/nvme/target/loop.c |  1 +
>   8 files changed, 30 insertions(+), 2 deletions(-)
> 
I'm slightly worried about the size in create in 'struct nvme_request',
but I've always found 'blk_mq_delay_kick_requeue_list()' a bit
optimistic here.

Reviewed-by: Hannes Reinecke <hare@kernel.org>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                  Kernel Storage Architect
hare@suse.de                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich

Re: [PATCH 4/6] nvme-mpath: support controller crd when failing over request

[Hannes Reinecke]

On 8/23/26 10:49 AM, Sagi Grimberg wrote:
> When failing over a request (due to a path based status) we should
> repect controller crd returned in the nvme completion as much as
> possible. Hence we want to delay the failover command execution by
> the controller crdt.
> 
> We allocate a new nvme_mpath_failover_timer referencing the request
> stolen bios in a staging list, and when the command retry delay expires,
> and only then the bios are moved to the mpath head requeue list which is
> immediately kicked to re-submit these bios. If we failed to allocate
> a fot, we fallback to the existing behavior.
> 
> Given that we now have a new staging list for mpath devices, we drain
> them when removing the device.
> 
> Signed-off-by: Sagi Grimberg <sagi@grimberg.me>
> ---
>   drivers/nvme/host/multipath.c | 96 +++++++++++++++++++++++++++++++++--
>   drivers/nvme/host/nvme.h      |  1 +
>   2 files changed, 93 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/nvme/host/multipath.c b/drivers/nvme/host/multipath.c
> index b5501217303c..959dd1e05a2d 100644
> --- a/drivers/nvme/host/multipath.c
> +++ b/drivers/nvme/host/multipath.c
> @@ -9,6 +9,13 @@
>   #include <trace/events/block.h>
>   #include "nvme.h"
>   
> +struct nvme_mpath_failover_timer {
> +	struct list_head	entry;
> +	struct nvme_ns_head	*head;
> +	struct bio_list		bios;
> +	struct timer_list	timer;
> +};
> +
>   bool multipath = true;
>   static bool multipath_always_on;
>   
> @@ -144,10 +151,48 @@ void nvme_mpath_start_freeze(struct nvme_subsystem *subsys)
>   			blk_freeze_queue_start(h->disk->queue);
>   }
>   
> +static void nvme_mpath_failover_timer_fn(struct timer_list *t)
> +{
> +	struct nvme_mpath_failover_timer *fot = timer_container_of(fot, t, timer);
> +	struct nvme_ns_head *head = fot->head;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&head->requeue_lock, flags);
> +	if (list_empty(&fot->entry)) {
> +		spin_unlock_irqrestore(&head->requeue_lock, flags);
> +		return;
> +	}
> +
> +	list_del_init(&fot->entry);
> +	if (fot->bios.head)
> +		bio_list_merge(&head->requeue_list, &fot->bios);
> +	spin_unlock_irqrestore(&head->requeue_lock, flags);
> +	kblockd_schedule_work(&head->requeue_work);
> +	kfree(fot);
> +}
> +
> +static struct nvme_mpath_failover_timer *
> +nvme_mpath_alloc_failover_timer(struct nvme_ns_head *head)
> +{
> +	struct nvme_mpath_failover_timer *fot;
> +
> +	fot = kzalloc(sizeof(*fot), GFP_ATOMIC);
> +	if (!fot)
> +		goto out;
> +	fot->head = head;
> +	bio_list_init(&fot->bios);
> +	INIT_LIST_HEAD(&fot->entry);
> +	timer_setup(&fot->timer, nvme_mpath_failover_timer_fn, 0);
> +out:
> +	return fot;
> +}
> +
>   void nvme_failover_req(struct request *req)
>   {
>   	struct nvme_ns *ns = req->q->queuedata;
>   	u16 status = nvme_req(req)->status & NVME_SCT_SC_MASK;
> +	struct nvme_mpath_failover_timer *fot = NULL;
> +	unsigned int delay;
>   	unsigned long flags;
>   	struct bio *bio;
>   
> @@ -167,13 +212,27 @@ void nvme_failover_req(struct request *req)
>   	for (bio = req->bio; bio; bio = bio->bi_next)
>   		bio_set_dev(bio, ns->head->disk->part0);
>   
> -	spin_lock_irqsave(&ns->head->requeue_lock, flags);
> -	blk_steal_bios(&ns->head->requeue_list, req);
> -	spin_unlock_irqrestore(&ns->head->requeue_lock, flags);
> +	delay = nvme_crd_msecs(nvme_req(req));
> +	if (delay) {
> +		fot = nvme_mpath_alloc_failover_timer(ns->head);
> +		if (fot) {
> +			blk_steal_bios(&fot->bios, req);
> +			spin_lock_irqsave(&ns->head->requeue_lock, flags);
> +			list_add_tail(&fot->entry, &ns->head->fots);
> +			spin_unlock_irqrestore(&ns->head->requeue_lock, flags);
> +			mod_timer(&fot->timer, jiffies + msecs_to_jiffies(delay));
> +		}
> +	}
> +	/* no CRD or timer allocation failed, fallback to immediate failover */
> +	if (!fot) {
> +		spin_lock_irqsave(&ns->head->requeue_lock, flags);
> +		blk_steal_bios(&ns->head->requeue_list, req);
> +		spin_unlock_irqrestore(&ns->head->requeue_lock, flags);
> +		kblockd_schedule_work(&ns->head->requeue_work);
> +	}

Yikes. Allocation during failover is not going to make you friends.

And this whole mechanism looks pretty similar what we did over at 
implementing CCR. Can you use the mechanism from there?

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                  Kernel Storage Architect
hare@suse.de                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich

Re: [PATCH 4/6] nvme-mpath: support controller crd when failing over request

[Sagi Grimberg]

> > +	/* no CRD or timer allocation failed, fallback to immediate failover */
> > +	if (!fot) {
> > +		spin_lock_irqsave(&ns->head->requeue_lock, flags);
> > +		blk_steal_bios(&ns->head->requeue_list, req);
> > +		spin_unlock_irqrestore(&ns->head->requeue_lock, flags);
> > +		kblockd_schedule_work(&ns->head->requeue_work);
> > +	}
> 
> Yikes. Allocation during failover is not going to make you friends.

Well, it is a fairly rare event, and the fallback is exactly what we
do today...

> 
> And this whole mechanism looks pretty similar what we did over at
> implementing CCR. Can you use the mechanism from there?

How is this similar? It is effectively creating a container for
bio_list and timer to requeue them when the timer expires...
 

Re: [PATCH 1/6] nvme-mpath: No need to protect req->bio with requeue_lock

[John Garry]

> ---
>    drivers/nvme/host/multipath.c | 3 ++-
>    1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/nvme/host/multipath.c b/drivers/nvme/host/multipath.c
> index 9b9a657fa330..b5501217303c 100644
> --- a/drivers/nvme/host/multipath.c
> +++ b/drivers/nvme/host/multipath.c
> @@ -164,9 +164,10 @@ void nvme_failover_req(struct request *req)
>    		queue_work(nvme_wq, &ns->ctrl->ana_work);
>    	}
>    
> -	spin_lock_irqsave(&ns->head->requeue_lock, flags);
>    	for (bio = req->bio; bio; bio = bio->bi_next)
>    		bio_set_dev(bio, ns->head->disk->part0);
> +
> +	spin_lock_irqsave(&ns->head->requeue_lock, flags);
>    	blk_steal_bios(&ns->head->requeue_list, req);

I think that the first half of blk_steal_bios() doesn't not require the 
lock either and does the same bio loop as for calling bio_set_dev(), 
above. Maybe another helper could be added for all of that.

But this still looks ok, so:

Reviewed-by: John Garry <john.g.garry@oracle.com>

>    	spin_unlock_irqrestore(&ns->head->requeue_lock, flags);
>    
> -- 
> 2.43.0
> 
> 

Re: [PATCH 7/6 RFC] nvme: test per-command retry delay

[Shin'ichiro Kawasaki]

On Aug 23, 2026 / 11:49, Sagi Grimberg wrote:
> Add tests to exercise host command retry delays handling.
> 
> 070: check that basic command RETRY disposition works and respect ctrl
>      crd
> 071: check that basic command FAILOVER disposition works and respects
>      ctrl crd
> 072: check that different commands completed with different crd levels
>      are retried independently, each respecting its paired completion
>      crd level
> 073: check that different commands completed with different crd levels
>      are failed-over independently, each respecting its paired completion
>      crd level
> 
> These tests rely on nvmet support for subsystem crdt attributes
> (_require_nvmet_crdt) and nvme host crd error injection support.
> 
> In addition we add some common nvme helpers to set nvmet attributes,
> inject errors, and leverage nvme diags to count retries/failovers.
> 
> Signed-off-by: Sagi Grimberg <sagi@grimberg.me>

Thank you for the patch. I ran the added four test cases using the kernel with
the kernel patches, and observed the all four test cases passed. Good.

I walked through the new test cases. Overall, they look good. One point to
improve is the global variable used to return a value. I will comment it in-
line.

I found the new test cases measure some numbers like retry count, failover
count, or elapsed times. Those numbers are used as pass/fail criteria. The
numbers are logged in the FULL file, but it might be useful to print the
numbers in the test run console like this:

nvme/070 (tr=loop) (test NVMe CRD per-request retry under fio) [passed]
    retries after   41       ...  42
    retries before  0        ...  0
    runtime         24.808s  ...  24.844s
nvme/071 (tr=loop) (test NVMe CRD multipath failover under fio) [passed]
    failovers after   100      ...  100
    failovers before  0        ...  0
    runtime           13.764s  ...  13.789s
nvme/072 (tr=loop) (test NVMe CRD per-request retry timer independence) [passed]
    CRD1 write  1090ms   ...  1079ms
    CRD2 write  10563ms  ...  10416ms
    runtime     12.363s  ...  12.166s
nvme/073 (tr=loop) (test NVMe CRD multipath failover timer independence) [passed]
    CRD1 failover  1068ms   ...  1065ms
    CRD2 failover  10091ms  ...  10349ms
    runtime        13.153s  ...  13.432s

FYI, I attached the script changes to print the numbers [*].It uses TEST_RUN[*]
feature of blktests.

Also, please find my in-line comments below:

> diff --git a/common/nvme b/common/nvme
> index f3999378db2d..a224dca61840 100644
> --- a/common/nvme
> +++ b/common/nvme

[...]

> +_nvme_now_ms() {
> +	echo $(($(date +%s%N) / 1000000))
> +}

Just comment: this funcion might worth moving to common/rc: tests/thtrol/*
scripts do almost same thing.

[...]

> +# Background timed direct write. Sets nvme_bg_timed_pid (do NOT call from $()).
> +# Writes elapsed ms to <result_file>, or FAIL on I/O error.
> +_nvme_bg_timed_direct_write() {
> +	local dev=$1
> +	local result=$2
> +
> +	rm -f "${result}"
> +	(
> +		local start end
> +		start="$(_nvme_now_ms)"
> +		if dd if=/dev/zero of="${dev}" bs=4k count=1 oflag=direct \
> +			status=none conv=notrunc 2>>"$FULL"; then
> +			end="$(_nvme_now_ms)"
> +			echo $((end - start)) >"${result}"
> +		else
> +			echo FAIL >"${result}"
> +		fi
> +	) &
> +	nvme_bg_timed_pid=$!
> +}

_nvme_bg_timed_direct_write() uses the global variable nvme_bg_timed_pid to
return the pid to the caller. It is not the best to use global variables for
that purpose. Also, shellcheck warns this:

   common/nvme:1773:2: warning: nvme_bg_timed_pid appears unused. Verify use (or export if used externally). [SC2034]

In general, bash functions return value with the "echo back" method. But I
understand this method won't work here, since sub-shell $() is required to pass
the echoed value to the caller. With this, the background task is no longer a
child of the caller, then the wait command for the received pid fails with the
error "wait: pid x is not a child of this shell".

As the solution for such scenarios, bash provides "nameref" feature (local -n),
which is like the pointer of the C language. The hunk below will add the third
argument to return the pid to the caller. I will comment how the caller sides
will change later.

diff --git a/common/nvme b/common/nvme
index f323c6c..273fa66 100644
--- a/common/nvme
+++ b/common/nvme
@@ -1752,14 +1752,15 @@ _nvme_timed_direct_write() {
 	echo $((end - start))
 }
 
-# Background timed direct write. Sets nvme_bg_timed_pid (do NOT call from $()).
-# Writes elapsed ms to <result_file>, or FAIL on I/O error.
+# Background timed direct write. Writes elapsed ms to <result_file>, or FAIL on
+# I/O error. Return the pid of the background process with bash nameref feature.
 _nvme_bg_timed_direct_write() {
 	local dev=$1
 	local result=$2
+	local -n pid=$3
 
 	rm -f "${result}"
-	(
+	{
 		local start end
 		start="$(_nvme_now_ms)"
 		if dd if=/dev/zero of="${dev}" bs=4k count=1 oflag=direct \
@@ -1769,8 +1770,8 @@ _nvme_bg_timed_direct_write() {
 		else
 			echo FAIL >"${result}"
 		fi
-	) &
-	nvme_bg_timed_pid=$!
+	} &
+	pid=$!
 }
 

> diff --git a/tests/nvme/070 b/tests/nvme/070
> new file mode 100755
> index 000000000000..14d7160664a0
> --- /dev/null
> +++ b/tests/nvme/070
> @@ -0,0 +1,95 @@
> +#!/bin/bash
> +# SPDX-License-Identifier: GPL-3.0+
> +# Copyright (C) 2026 Sagi Grimberg <sagi@grimberg.me>
> +#
> +# Test NVMe command retry delay (CRD) with the per-request retry timer.
> +# Requires nvmet attr_crdt* and host fault_inject/crd.
> +
> +. tests/nvme/rc
> +
> +DESCRIPTION="test NVMe CRD per-request retry under fio"
> +QUICK=1
> +
> +# NVME_SC_INTERNAL

Nit: the line above does not look meaningful when I see the line below.

> +NVME_SC_INTERNAL=0x6
> +

[...]

> +test() {
> +	local fio_pid
> +	local ns
> +	local retries_before
> +	local retries_after
> +	local inject_dev
> +
> +	echo "Running ${TEST_NAME}"
> +
> +	_setup_nvmet
> +	_nvmet_target_setup
> +	# CRDT1 = 5 * 100ms = 500ms
> +	_nvmet_set_crdt 5 0 0
> +
> +	_nvme_connect_subsys
> +	ns=$(_find_nvme_ns "${def_subsys_uuid}")
> +
> +	inject_dev=$(_nvme_fault_inject_devs "${ns}" | head -1)
> +	_nvme_set_ns_diag "${inject_dev}" command_retries_count 0 || true
> +	retries_before=$(_nvme_get_ns_diag "${inject_dev}" command_retries_count)
> +
> +	_run_fio_verify_io --filename="/dev/${ns}" \
> +			   --group_reporting --ramp_time=2 \
> +			   --time_based --runtime=20 &> "$FULL" &

Nit: It is a bit safer to use "&>>" instaed of "&>" in case prep helper
functions leave logs in the FULL file.

[...]

> diff --git a/tests/nvme/072 b/tests/nvme/072
> new file mode 100755
> index 000000000000..2acda72cdd44
> --- /dev/null
> +++ b/tests/nvme/072

[...]

> +test() {
> +	local ns
> +	local inject_dev
> +	local crd2_pid crd1_pid
> +	local crd2_result crd1_result
> +	local crd2_elapsed crd1_elapsed
> +
> +	echo "Running ${TEST_NAME}"
> +
> +	_setup_nvmet
> +	_nvmet_target_setup
> +	_nvmet_set_crdt "${CRDT1}" "${CRDT2}" 0
> +
> +	_nvme_connect_subsys
> +	ns=$(_find_nvme_ns "${def_subsys_uuid}")
> +	inject_dev=$(_nvme_fault_inject_devs "${ns}" | head -1)
> +
> +	echo "ns=${ns} inject=${inject_dev}" >>"$FULL"
> +	echo "CRDT CRD1=${CRD1_MS}ms CRD2=${CRD2_MS}ms"
> +
> +	if [[ ! -e /sys/kernel/debug/${inject_dev}/fault_inject/crd ]]; then
> +		echo "FAIL: missing fault_inject/crd on ${inject_dev}"
> +		_nvme_disconnect_subsys
> +		_nvmet_target_cleanup
> +		return 1
> +	fi
> +
> +	crd2_result="${TMPDIR}/crd2_elapsed"
> +	crd1_result="${TMPDIR}/crd1_elapsed"
> +
> +	echo "Arming CRD2 and starting first write"
> +	_nvme_arm_crd_inject "${inject_dev}" 0 "${NVME_SC_INTERNAL}" 2 1 || return 1
> +	echo "inject: status=${NVME_SC_INTERNAL} crd=2 times=1" >>"$FULL"
> +	_nvme_bg_timed_direct_write "/dev/${ns}" "${crd2_result}"
> +	crd2_pid=$nvme_bg_timed_pid

With the nameref, the two lines above are to be modified as follows:

	_nvme_bg_timed_direct_write "/dev/${ns}" "${crd2_result}" crd2_pid

> +	echo "CRD2 write pid=${crd2_pid}" >>"$FULL"
> +
> +	if ! _nvme_wait_inject_consumed "${inject_dev}"; then
> +		echo "FAIL: CRD2 inject was not consumed"
> +		_nvme_disarm_crd_inject "${inject_dev}"
> +		kill "${crd2_pid}" 2>/dev/null || true
> +		wait "${crd2_pid}" 2>/dev/null || true
> +		_nvme_disconnect_subsys
> +		_nvmet_target_cleanup
> +		return 1
> +	fi
> +	echo "CRD2 latched; arming CRD1 while CRD2 retry is pending" >>"$FULL"
> +
> +	echo "Arming CRD1 and starting second write (CRD2 still pending)"
> +	_nvme_arm_crd_inject "${inject_dev}" 0 "${NVME_SC_INTERNAL}" 1 1 || return 1
> +	echo "inject: status=${NVME_SC_INTERNAL} crd=1 times=1" >>"$FULL"
> +	_nvme_bg_timed_direct_write "/dev/${ns}" "${crd1_result}"
> +	crd1_pid=$nvme_bg_timed_pid

Same here:

	_nvme_bg_timed_direct_write "/dev/${ns}" "${crd1_result}" crd1_pid

> +	echo "CRD1 write pid=${crd1_pid}" >>"$FULL"
> +
> +	echo "Waiting for both writes (expect CRD1~${CRD1_MS}ms then CRD2~${CRD2_MS}ms)"
> +	wait "${crd1_pid}" || true
> +	wait "${crd2_pid}" || true
> +	_nvme_disarm_crd_inject "${inject_dev}"
> +	udevadm settle >/dev/null 2>&1 || true
> +
> +	crd1_elapsed="$(cat "${crd1_result}" 2>/dev/null || echo FAIL)"
> +	crd2_elapsed="$(cat "${crd2_result}" 2>/dev/null || echo FAIL)"
> +	echo "CRD1 write elapsed ${crd1_elapsed}ms (expected ~${CRD1_MS}ms)" >>"$FULL"
> +	echo "CRD2 write elapsed ${crd2_elapsed}ms (expected ~${CRD2_MS}ms)" >>"$FULL"
> +
> +	if [[ "${crd1_elapsed}" == "FAIL" || -z "${crd1_elapsed}" ]]; then
> +		echo "FAIL: CRD1 write did not complete"
> +	else
> +		echo "CRD1 write elapsed ${crd1_elapsed}ms" | sed -E 's/(elapsed )[0-9]+/\1NUM/'
> +		_nvme_check_crd_elapsed "CRD1 write" "${crd1_elapsed}" "${CRD1_MS}" "$((CRD2_MS / 2))"
> +	fi
> +	if [[ "${crd2_elapsed}" == "FAIL" || -z "${crd2_elapsed}" ]]; then
> +		echo "FAIL: CRD2 write did not complete"
> +	else
> +		echo "CRD2 write elapsed ${crd2_elapsed}ms" | sed -E 's/(elapsed )[0-9]+/\1NUM/'
> +		_nvme_check_crd_elapsed "CRD2 write" "${crd2_elapsed}" "${CRD2_MS}"
> +	fi
> +
> +	_nvme_disconnect_subsys
> +	_nvmet_target_cleanup
> +	udevadm settle >/dev/null 2>&1 || true
> +
> +	echo "Test complete"
> +}
> diff --git a/tests/nvme/072.out b/tests/nvme/072.out
> new file mode 100644
> index 000000000000..d9f4520c8d23
> --- /dev/null
> +++ b/tests/nvme/072.out
> @@ -0,0 +1,8 @@
> +Running nvme/072
> +CRDT CRD1=1000ms CRD2=10000ms
> +Arming CRD2 and starting first write
> +Arming CRD1 and starting second write (CRD2 still pending)
> +Waiting for both writes (expect CRD1~1000ms then CRD2~10000ms)
> +CRD1 write elapsed NUMms
> +CRD2 write elapsed NUMms
> +Test complete
> diff --git a/tests/nvme/073 b/tests/nvme/073
> new file mode 100755
> index 000000000000..34911392437a
> --- /dev/null
> +++ b/tests/nvme/073

[...]

> +test() {
> +	local ns
> +	local port
> +	local -a ports
> +	local -a path_devs
> +	local path0
> +	local path1
> +	local fo_before fo_after
> +	local crd2_pid crd1_pid
> +	local crd2_result crd1_result
> +	local crd2_elapsed crd1_elapsed
> +	local sync_start
> +
> +	echo "Running ${TEST_NAME}"
> +
> +	_setup_nvmet
> +	_nvmet_target_setup --ports 2
> +	_nvmet_set_crdt "${CRDT1}" "${CRDT2}" 0
> +
> +	_get_nvmet_ports "${def_subsysnqn}" ports
> +	for port in "${ports[@]}"; do
> +		_setup_nvmet_port_ana "${port}" 1 "optimized"
> +		_nvme_connect_subsys --port "${port}" --no-wait-ns
> +	done
> +	sleep 1
> +
> +	ns=$(_find_nvme_ns "${def_subsys_uuid}")
> +	mapfile -t path_devs < <(_nvme_path_ns_devs "${ns}")
> +	if ((${#path_devs[@]} < 2)); then
> +		echo "FAIL: need >=2 path namespaces, found ${#path_devs[@]} (${path_devs[*]})"
> +		_nvme_disconnect_subsys
> +		_nvmet_target_cleanup
> +		return 1
> +	fi
> +
> +	path0=${path_devs[0]}
> +	path1=${path_devs[1]}
> +	echo "ns=${ns} path0=${path0} path1=${path1}" >>"$FULL"
> +	echo "CRDT CRD1=${CRD1_MS}ms CRD2=${CRD2_MS}ms"
> +
> +	if [[ ! -e /sys/kernel/debug/${path0}/fault_inject/crd ]]; then
> +		echo "FAIL: missing fault_inject/crd on ${path0}"
> +		_nvme_disconnect_subsys
> +		_nvmet_target_cleanup
> +		return 1
> +	fi
> +
> +	_nvme_set_ns_diag "${path0}" multipath_failover_count 0 || true
> +	_nvme_set_ns_diag "${path1}" multipath_failover_count 0 || true
> +	fo_before=$(_nvme_get_ns_diag "${path0}" multipath_failover_count)
> +
> +	crd2_result="${TMPDIR}/crd2_elapsed"
> +	crd1_result="${TMPDIR}/crd1_elapsed"
> +
> +	echo "Arming CRD2 on path0 and starting first write"
> +	_nvme_arm_crd_inject "${path0}" 0 "${NVME_SC_INTERNAL_PATH_ERROR}" 2 1 || return 1
> +	echo "path0 inject: path_error crd=2 times=1" >>"$FULL"
> +	_nvme_bg_timed_direct_write "/dev/${ns}" "${crd2_result}"
> +	crd2_pid=$nvme_bg_timed_pid

Same here:

	_nvme_bg_timed_direct_write "/dev/${ns}" "${crd2_result}" crd2_pid

> +	echo "CRD2 write pid=${crd2_pid}" >>"$FULL"
> +
> +	sync_start="$(_nvme_now_ms)"
> +	while (( $(_nvme_get_ns_diag "${path0}" multipath_failover_count) <= fo_before )); do
> +		if (( $(_nvme_now_ms) - sync_start > 2000 )); then
> +			echo "FAIL: CRD2 failover was not scheduled on ${path0}"
> +			_nvme_disarm_crd_inject "${path0}"
> +			kill "${crd2_pid}" 2>/dev/null || true
> +			wait "${crd2_pid}" 2>/dev/null || true
> +			_nvme_disconnect_subsys
> +			_nvmet_target_cleanup
> +			return 1
> +		fi
> +		sleep 0.01
> +	done
> +	echo "CRD2 failover pending; path0 still selectable (NUMA/current)" >>"$FULL"
> +
> +	# Same path again: latch CRD1 while the CRD2 fot is still pending.
> +	echo "Arming CRD1 on path0 and starting second write (CRD2 still pending)"
> +	_nvme_arm_crd_inject "${path0}" 0 "${NVME_SC_INTERNAL_PATH_ERROR}" 1 1 || return 1
> +	echo "path0 inject: path_error crd=1 times=1" >>"$FULL"
> +	_nvme_bg_timed_direct_write "/dev/${ns}" "${crd1_result}"
> +	crd1_pid=$nvme_bg_timed_pid

Same here:

	_nvme_bg_timed_direct_write "/dev/${ns}" "${crd1_result}" crd1_pid

> +	echo "CRD1 write pid=${crd1_pid}" >>"$FULL"
> +
> +	echo "Waiting for both writes (expect CRD1~${CRD1_MS}ms then CRD2~${CRD2_MS}ms)"
> +	wait "${crd1_pid}" || true
> +	wait "${crd2_pid}" || true
> +	_nvme_disarm_crd_inject "${path0}"
> +	udevadm settle >/dev/null 2>&1 || true
> +
> +	crd1_elapsed="$(cat "${crd1_result}" 2>/dev/null || echo FAIL)"
> +	crd2_elapsed="$(cat "${crd2_result}" 2>/dev/null || echo FAIL)"
> +	fo_after=$(_nvme_get_ns_diag "${path0}" multipath_failover_count)
> +	echo "CRD1 failover elapsed ${crd1_elapsed}ms (expected ~${CRD1_MS}ms)" >>"$FULL"
> +	echo "CRD2 failover elapsed ${crd2_elapsed}ms (expected ~${CRD2_MS}ms)" >>"$FULL"
> +	echo "path0 failover count ${fo_before} -> ${fo_after}" >>"$FULL"

The lines above causes a shellcheck warn:

  tests/nvme/073:131:2: note: Consider using { cmd1; cmd2; } >> file instead of individual redirects. [SC2129]

I suggest to modify the lines as follows:

	{
		echo "CRD1 failover elapsed ${crd1_elapsed}ms (expected ~${CRD1_MS}ms)"
		echo "CRD2 failover elapsed ${crd2_elapsed}ms (expected ~${CRD2_MS}ms)"
		echo "path0 failover count ${fo_before} -> ${fo_after}"
	} >>"$FULL"

> +
> +	if (( fo_after < fo_before + 2 )); then
> +		echo "FAIL: expected two failovers on path0, got ${fo_before} -> ${fo_after}"
> +	fi
> +
> +	if [[ "${crd1_elapsed}" == "FAIL" || -z "${crd1_elapsed}" ]]; then
> +		echo "FAIL: CRD1 failover write did not complete"
> +	else
> +		echo "CRD1 failover elapsed ${crd1_elapsed}ms" | sed -E 's/(elapsed )[0-9]+/\1NUM/'
> +		_nvme_check_crd_elapsed "CRD1 failover" "${crd1_elapsed}" "${CRD1_MS}" "$((CRD2_MS / 2))"
> +	fi
> +	if [[ "${crd2_elapsed}" == "FAIL" || -z "${crd2_elapsed}" ]]; then
> +		echo "FAIL: CRD2 failover write did not complete"
> +	else
> +		echo "CRD2 failover elapsed ${crd2_elapsed}ms" | sed -E 's/(elapsed )[0-9]+/\1NUM/'
> +		_nvme_check_crd_elapsed "CRD2 failover" "${crd2_elapsed}" "${CRD2_MS}"
> +	fi
> +
> +	_nvme_disconnect_subsys
> +	_nvmet_target_cleanup
> +	udevadm settle >/dev/null 2>&1 || true
> +
> +	echo "Test complete"
> +}

[...]

> diff --git a/tests/nvme/rc b/tests/nvme/rc
> index 31a0fc59ff4b..f286d9a95cce 100644
> --- a/tests/nvme/rc
> +++ b/tests/nvme/rc
> @@ -505,17 +505,39 @@ _nvme_err_inject_cleanup()
>  
>  _nvme_enable_err_inject()
>  {
> +	# Set status/dont_retry[/crd] before arming probability/times so concurrent
> +	# I/O cannot observe the debugfs defaults (INVALID_OPCODE + DNR).
>          _set_attr "$2" /sys/kernel/debug/"$1"/fault_inject/verbose
> -        _set_attr "$3" /sys/kernel/debug/"$1"/fault_inject/probability
>          _set_attr "$4" /sys/kernel/debug/"$1"/fault_inject/dont_retry
>          _set_attr "$5" /sys/kernel/debug/"$1"/fault_inject/status
> +	if [[ -n "${7:-}" && -e /sys/kernel/debug/"$1"/fault_inject/crd ]]; then
> +		_set_attr "$7" /sys/kernel/debug/"$1"/fault_inject/crd
> +	fi
>          _set_attr "$6" /sys/kernel/debug/"$1"/fault_inject/times
> +        _set_attr "$3" /sys/kernel/debug/"$1"/fault_inject/probability
> +}

Just comment: this function uses spaces for indent regardless of the patch. The
hunk above also uses spaces for indent, but I think it's fine to keep the
consistency. It is ideal to replace the spaces for indent in the function later.
I found three other injection related function in tests/nvme/rc uses spaces for
indent.



[*] Changes to report pass/fail criteria numbers in the test run console

diff --git a/tests/nvme/070 b/tests/nvme/070
index 14d7160..8fbbd77 100755
--- a/tests/nvme/070
+++ b/tests/nvme/070
@@ -84,6 +84,8 @@ test() {
 	wait "${fio_pid}" || echo "FAIL: fio exited with errors (see $FULL)"
 
 	retries_after=$(_nvme_get_ns_diag "${inject_dev}" command_retries_count)
+	TEST_RUN["retries before"]=$retries_before
+	TEST_RUN["retries after"]=$retries_after
 	if (( retries_after <= retries_before )); then
 		echo "command_retries_count did not increase (${retries_before} -> ${retries_after})"
 	fi
diff --git a/tests/nvme/071 b/tests/nvme/071
index 897a883..644921a 100755
--- a/tests/nvme/071
+++ b/tests/nvme/071
@@ -173,6 +173,8 @@ test() {
 
 	inject_after=$(_nvme_get_ns_diag "${inject_dev}" multipath_failover_count)
 	echo "failovers ${inject_dev}: ${inject_before} -> ${inject_after}" >> "$FULL"
+	TEST_RUN["failovers before"]=$inject_before
+	TEST_RUN["failovers after"]=$inject_after
 	if (( inject_after <= inject_before )); then
 		echo "FAIL: multipath_failover_count on ${inject_dev} did not increase (${inject_before} -> ${inject_after})"
 		dump_fault_inject "${inject_dev}"
diff --git a/tests/nvme/072 b/tests/nvme/072
index 2acda72..4515285 100755
--- a/tests/nvme/072
+++ b/tests/nvme/072
@@ -101,12 +99,14 @@ test() {
 		echo "FAIL: CRD1 write did not complete"
 	else
 		echo "CRD1 write elapsed ${crd1_elapsed}ms" | sed -E 's/(elapsed )[0-9]+/\1NUM/'
+		TEST_RUN["CRD1 write"]="${crd1_elapsed}"ms
 		_nvme_check_crd_elapsed "CRD1 write" "${crd1_elapsed}" "${CRD1_MS}" "$((CRD2_MS / 2))"
 	fi
 	if [[ "${crd2_elapsed}" == "FAIL" || -z "${crd2_elapsed}" ]]; then
 		echo "FAIL: CRD2 write did not complete"
 	else
 		echo "CRD2 write elapsed ${crd2_elapsed}ms" | sed -E 's/(elapsed )[0-9]+/\1NUM/'
+		TEST_RUN["CRD2 write"]="${crd2_elapsed}"ms
 		_nvme_check_crd_elapsed "CRD2 write" "${crd2_elapsed}" "${CRD2_MS}"
 	fi
 
diff --git a/tests/nvme/073 b/tests/nvme/073
index 3491139..27b682d 100755
--- a/tests/nvme/073
+++ b/tests/nvme/073
@@ -140,12 +140,14 @@ test() {
 		echo "FAIL: CRD1 failover write did not complete"
 	else
 		echo "CRD1 failover elapsed ${crd1_elapsed}ms" | sed -E 's/(elapsed )[0-9]+/\1NUM/'
+		TEST_RUN["CRD1 failover"]="${crd1_elapsed}"ms
 		_nvme_check_crd_elapsed "CRD1 failover" "${crd1_elapsed}" "${CRD1_MS}" "$((CRD2_MS / 2))"
 	fi
 	if [[ "${crd2_elapsed}" == "FAIL" || -z "${crd2_elapsed}" ]]; then
 		echo "FAIL: CRD2 failover write did not complete"
 	else
 		echo "CRD2 failover elapsed ${crd2_elapsed}ms" | sed -E 's/(elapsed )[0-9]+/\1NUM/'
+		TEST_RUN["CRD2 failover"]="${crd2_elapsed}"ms
 		_nvme_check_crd_elapsed "CRD2 failover" "${crd2_elapsed}" "${CRD2_MS}"
 	fi
 

Re: [PATCH 7/6 RFC] nvme: test per-command retry delay

[Sagi Grimberg]

On 30/08/2026 11:23, Shin'ichiro Kawasaki wrote:
> On Aug 23, 2026 / 11:49, Sagi Grimberg wrote:
>> Add tests to exercise host command retry delays handling.
>>
>> 070: check that basic command RETRY disposition works and respect ctrl
>>       crd
>> 071: check that basic command FAILOVER disposition works and respects
>>       ctrl crd
>> 072: check that different commands completed with different crd levels
>>       are retried independently, each respecting its paired completion
>>       crd level
>> 073: check that different commands completed with different crd levels
>>       are failed-over independently, each respecting its paired completion
>>       crd level
>>
>> These tests rely on nvmet support for subsystem crdt attributes
>> (_require_nvmet_crdt) and nvme host crd error injection support.
>>
>> In addition we add some common nvme helpers to set nvmet attributes,
>> inject errors, and leverage nvme diags to count retries/failovers.
>>
>> Signed-off-by: Sagi Grimberg <sagi@grimberg.me>
> Thank you for the patch. I ran the added four test cases using the kernel with
> the kernel patches, and observed the all four test cases passed. Good.
>
> I walked through the new test cases. Overall, they look good. One point to
> improve is the global variable used to return a value. I will comment it in-
> line.
>
> I found the new test cases measure some numbers like retry count, failover
> count, or elapsed times. Those numbers are used as pass/fail criteria. The
> numbers are logged in the FULL file, but it might be useful to print the
> numbers in the test run console like this:

Thanks - will wait for some more feedback before a respin.

Re: [PATCH 1/6] nvme-mpath: No need to protect req->bio with requeue_lock

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 2/6] nvme: add nvme_crd_msecs helper

[Christoph Hellwig]

On Sun, Aug 23, 2026 at 11:48:58AM +0300, Sagi Grimberg wrote:
> Hide the details of extracting the crd value. We'll need it from other
> call-sites.

Even without users splitting it out really helps readability..

> +	/* The mask and shift result must be <= 3 */

While this is copied from the previous version I have no idea what it is
supposed to mean.

Re: [PATCH 3/6] nvme: add per request retry timer

[Christoph Hellwig]

On Sun, Aug 23, 2026 at 11:48:59AM +0300, Sagi Grimberg wrote:
> The existing command retry mechanism adds a request to retry
> to a retry list and modifies a request-queue (controller) wide
> dealyed queue timer.
> 
> The issue is that the existing requests in this queue may wait
> for longer periods of time as more requests are completed with
> a retry crd level.
> 
> Instead, add a per-request timer that will allow different
> requests retry in a way that is independent of other requests.

This blows every nvme_request.  I think we'd only need a per-CRD
level retry list and timer, can't we do that instead?

Re: [PATCH 4/6] nvme-mpath: support controller crd when failing over request

[Christoph Hellwig]

On Sun, Aug 23, 2026 at 11:49:00AM +0300, Sagi Grimberg wrote:
> When failing over a request (due to a path based status) we should
> repect controller crd returned in the nvme completion as much as
> possible. Hence we want to delay the failover command execution by
> the controller crdt.
> 
> We allocate a new nvme_mpath_failover_timer referencing the request
> stolen bios in a staging list, and when the command retry delay expires,
> and only then the bios are moved to the mpath head requeue list which is
> immediately kicked to re-submit these bios. If we failed to allocate
> a fot, we fallback to the existing behavior.
> 
> Given that we now have a new staging list for mpath devices, we drain
> them when removing the device.

Given that we only have a few potential CRD levels, what about having
the "container" for the requing statically preallocated for each?

Re: [PATCH 5/6] nvmet: Add support for configurable crdt (command retry delay) attributes

[Christoph Hellwig]

On Sun, Aug 23, 2026 at 11:49:01AM +0300, Sagi Grimberg wrote:
> Paired with host side error injection, allows us to exercise host retry delay

Overly long line.

> behavior.

... and overall an exremely sparse commit message.  І don't really
think we should add code just for injecting errors into the host,
and it seems like that is what this is doing?

Re: [PATCH 2/6] nvme: add nvme_crd_msecs helper

[Sagi Grimberg]

On 02/09/2026 16:40, Christoph Hellwig wrote:
> On Sun, Aug 23, 2026 at 11:48:58AM +0300, Sagi Grimberg wrote:
>> Hide the details of extracting the crd value. We'll need it from other
>> call-sites.
> Even without users splitting it out really helps readability..
>
>> +	/* The mask and shift result must be <= 3 */
> While this is copied from the previous version I have no idea what it is
> supposed to mean.

I can remove it

Re: [PATCH 3/6] nvme: add per request retry timer

[Sagi Grimberg]

On 02/09/2026 16:41, Christoph Hellwig wrote:
> On Sun, Aug 23, 2026 at 11:48:59AM +0300, Sagi Grimberg wrote:
>> The existing command retry mechanism adds a request to retry
>> to a retry list and modifies a request-queue (controller) wide
>> dealyed queue timer.
>>
>> The issue is that the existing requests in this queue may wait
>> for longer periods of time as more requests are completed with
>> a retry crd level.
>>
>> Instead, add a per-request timer that will allow different
>> requests retry in a way that is independent of other requests.
> This blows every nvme_request.  I think we'd only need a per-CRD
> level retry list and timer, can't we do that instead?

I can't see how, because we'd be modifying the timer potentially 
indefinitely while
requests are pending....

I understand that this bloats the nvme_request, however this is not 
accessed in the hot path.

Re: [PATCH 4/6] nvme-mpath: support controller crd when failing over request

[Sagi Grimberg]

On 02/09/2026 16:43, Christoph Hellwig wrote:
> On Sun, Aug 23, 2026 at 11:49:00AM +0300, Sagi Grimberg wrote:
>> When failing over a request (due to a path based status) we should
>> repect controller crd returned in the nvme completion as much as
>> possible. Hence we want to delay the failover command execution by
>> the controller crdt.
>>
>> We allocate a new nvme_mpath_failover_timer referencing the request
>> stolen bios in a staging list, and when the command retry delay expires,
>> and only then the bios are moved to the mpath head requeue list which is
>> immediately kicked to re-submit these bios. If we failed to allocate
>> a fot, we fallback to the existing behavior.
>>
>> Given that we now have a new staging list for mpath devices, we drain
>> them when removing the device.
> Given that we only have a few potential CRD levels, what about having
> the "container" for the requing statically preallocated for each?

I am not sure I understand how we can do that. While there are 3 crd levels,
requests can fail at any point in time with a given crd level, I don't 
see how we can
reliably requeue after the crd level has elapsed without getting into 
the situation
before (if any command failed with crd level X, it would extend the 
timer while other
commands are pending on it).

The allocation in the failover path is not ideal, however the fallback 
gets us to the normal
failover which we have today. And failovers are fairly rare, so I don't 
know if it is worth optimizing for...

Re: [PATCH 5/6] nvmet: Add support for configurable crdt (command retry delay) attributes

[Sagi Grimberg]

On 02/09/2026 16:44, Christoph Hellwig wrote:
> On Sun, Aug 23, 2026 at 11:49:01AM +0300, Sagi Grimberg wrote:
>> Paired with host side error injection, allows us to exercise host retry delay
> Overly long line.
>
>> behavior.
> ... and overall an exremely sparse commit message.  І don't really
> think we should add code just for injecting errors into the host,
> and it seems like that is what this is doing?

Yes, this is what this patch is designed for. However, it could be 
useful to expose via
configfs if someone has a backing device that supports different crd 
levels and uses passthru?
My intention was to use it so we can add blktests, I'm not hard set on 
it...

Re: [PATCH 3/6] nvme: add per request retry timer

[Sagi Grimberg]

On 06/09/2026 3:09, Sagi Grimberg wrote:
>
>
> On 02/09/2026 16:41, Christoph Hellwig wrote:
>> On Sun, Aug 23, 2026 at 11:48:59AM +0300, Sagi Grimberg wrote:
>>> The existing command retry mechanism adds a request to retry
>>> to a retry list and modifies a request-queue (controller) wide
>>> dealyed queue timer.
>>>
>>> The issue is that the existing requests in this queue may wait
>>> for longer periods of time as more requests are completed with
>>> a retry crd level.
>>>
>>> Instead, add a per-request timer that will allow different
>>> requests retry in a way that is independent of other requests.
>> This blows every nvme_request.  I think we'd only need a per-CRD
>> level retry list and timer, can't we do that instead?
>
> I can't see how, because we'd be modifying the timer potentially 
> indefinitely while
> requests are pending....
>
> I understand that this bloats the nvme_request, however this is not 
> accessed in the hot path.

Christoph, would it be preferable to have req->retry_timer dynamically 
allocated? It would reduce
the size of nvme_request.