[PATCH v3 0/2] nvme-multipath: requeue work fixes

[PATCH v3 0/2] nvme-multipath: requeue work fixes

[Qiliang Yuan]

Patch 1 applies a new delayed_removal_secs to a head removal that is
already pending, so that clearing it fails the I/O queued without a
path right away instead of when the old delay expires.

Patch 2 stops nvme_mpath_set_live() from waiting for an SRCU grace
period per namespace, which makes an ANA change with many namespaces
take tens to hundreds of milliseconds.

Signed-off-by: Qiliang Yuan <qiliang.yuan@linux.dev>
---
V2 -> V3:
- Requeue a pending head removal with the new delayed_removal_secs
  instead of only kicking the requeue work when it is cleared, as Nilay
  suggested
- Rebase onto v7.4-rc2-470-g7e1392a42ea2 and update the numbers from a
  new run
- Don't carry Hannes's Reviewed-by on the v2 patch 1, which is rewritten

V1 -> V2:
- Add patch 1
- Take the SRCU cookie under head->lock in patch 2 (Sashiko)
- Rerun the tests

v2: https://lore.kernel.org/r/20261111-bug-nvme-mpath-set-live-srcu-v2-0-fc772914d293@linux.dev
v1: https://lore.kernel.org/r/20261110-bug-nvme-mpath-set-live-srcu-v1-1-1f58b405b0f8@linux.dev

---
Qiliang Yuan (2):
      nvme-multipath: apply delayed_removal_secs changes to a pending removal
      nvme-multipath: don't wait for a grace period in nvme_mpath_set_live()

 drivers/nvme/host/multipath.c | 18 +++++++++++++++++-
 drivers/nvme/host/nvme.h      |  1 +
 2 files changed, 18 insertions(+), 1 deletion(-)
---
base-commit: 7e1392a42ea288d70a266f6553fe06f57f8647ea
change-id: 20261110-bug-nvme-mpath-set-live-srcu-cbd8965f1457

Best regards,
-- 
Qiliang Yuan <qiliang.yuan@linux.dev>

[PATCH v3 1/2] nvme-multipath: apply delayed_removal_secs changes to a pending removal

[Qiliang Yuan]

With delayed_removal_secs set, removing the last path of a namespace
head doesn't delete the head right away. nvme_mpath_remove_disk()
queues remove_work with the configured delay, and bios submitted in the
meantime wait on head->requeue_list in case a path comes back.

A write to delayed_removal_secs only takes effect for removals that
start later. A removal that is already pending keeps the old delay, so
clearing the attribute leaves the queued bios waiting until that delay
expires instead of failing them, and lowering or raising it doesn't
move the pending removal either.

Requeue a pending remove_work with the new delay when the attribute is
written. Cancel it first so that the module reference taken for it
carries over, as nvme_init_ns_head() does when a path comes back.

With delayed_removal_secs set and no path left, the attribute is
changed two seconds later and the head is removed after (mean of two
runs, min-max in parentheses):

  delay (s)  before (ms)            after (ms)             change
  30 -> 0    26510 (26053-26967)        8 (8-8)            -99.97%
  30 -> 5    27994 (27457-28531)     6194 (5589-6798)      -78%
  10 -> 30    8131 (8086-8176)      29802 (29522-30082)    +267%

A direct read queued before the 30 -> 0 write fails after:

  before (ms)            after (ms)   change
  26466 (26017-26915)    4 (4-4)      -99.98%

Fixes: 62188639ec160 ("nvme-multipath: introduce delayed removal of the multipath head node")
Suggested-by: Nilay Shroff <nilay@linux.ibm.com>
Signed-off-by: Qiliang Yuan <qiliang.yuan@linux.dev>
---
 drivers/nvme/host/multipath.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/nvme/host/multipath.c b/drivers/nvme/host/multipath.c
index ae8059366b57d..35278b9c4de60 100644
--- a/drivers/nvme/host/multipath.c
+++ b/drivers/nvme/host/multipath.c
@@ -1242,6 +1242,9 @@ static ssize_t delayed_removal_secs_store(struct device *dev,
 	mutex_lock(&head->subsys->lock);
 	head->delayed_removal_secs = sec;
 	assign_bit(NVME_NSHEAD_QUEUE_IF_NO_PATH, &head->flags, sec);
+	/* Apply the new delay to a removal that is already pending */
+	if (cancel_delayed_work(&head->remove_work))
+		queue_delayed_work(nvme_wq, &head->remove_work, sec * HZ);
 	mutex_unlock(&head->subsys->lock);
 	/*
 	 * Ensure that update to NVME_NSHEAD_QUEUE_IF_NO_PATH is seen

-- 
2.43.0

[PATCH v3 2/2] nvme-multipath: don't wait for a grace period in nvme_mpath_set_live()

[Qiliang Yuan]

When a path goes live, nvme_mpath_set_live() waits for an SRCU grace
period of the namespace head before kicking the requeue work, so that
bios which found no usable path are on the requeue list by the time it
runs.

The ANA work calls nvme_mpath_set_live() for every namespace that
becomes optimized or non-optimized, one after another, so an ANA change
waits for one grace period per namespace.

Start the grace period with start_poll_synchronize_srcu() instead and
have nvme_requeue_work() wait for it only if it has not completed yet.
Take the cookie under head->lock, so that concurrent callers store
their cookies in order and the requeue work never sees an older one.
The grace periods of all namespaces then run in parallel, and requeue
work kicked for other reasons finds the cookie completed and does not
wait.

Flipping an ANA group from inaccessible to optimized ten times on each
of two boots over nvme-tcp loopback, nvme_read_ana_log() takes (median,
min-max in parentheses):

  namespaces  before (ms)          after (ms)        change
         256  52.0 (35.4-74.7)     8.7 (2.5-19.6)      -83%
        1024  154.3 (123.9-193.8)  43.1 (18.2-58.3)    -72%

Signed-off-by: Qiliang Yuan <qiliang.yuan@linux.dev>
Reviewed-by: Hannes Reinecke <hare@kernel.org>
Reviewed-by: Nilay Shroff <nilay@linux.ibm.com>
---
 drivers/nvme/host/multipath.c | 15 ++++++++++++++-
 drivers/nvme/host/nvme.h      |  1 +
 2 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/drivers/nvme/host/multipath.c b/drivers/nvme/host/multipath.c
index 35278b9c4de60..971bd14e46803 100644
--- a/drivers/nvme/host/multipath.c
+++ b/drivers/nvme/host/multipath.c
@@ -712,6 +712,11 @@ static void nvme_requeue_work(struct work_struct *work)
 		container_of(work, struct nvme_ns_head, requeue_work);
 	struct bio *bio, *next;
 
+	/* Let bios that found no usable path reach the requeue list first. */
+	if (!poll_state_synchronize_srcu(&head->srcu,
+					 READ_ONCE(head->requeue_srcu_cookie)))
+		synchronize_srcu(&head->srcu);
+
 	spin_lock_irq(&head->requeue_lock);
 	next = bio_list_get(&head->requeue_list);
 	spin_unlock_irq(&head->requeue_lock);
@@ -767,6 +772,7 @@ int nvme_mpath_alloc_disk(struct nvme_ctrl *ctrl, struct nvme_ns_head *head)
 	mutex_init(&head->lock);
 	spin_lock_init(&head->requeue_lock);
 	INIT_WORK(&head->requeue_work, nvme_requeue_work);
+	head->requeue_srcu_cookie = get_completed_synchronize_srcu();
 	INIT_WORK(&head->partition_scan_work, nvme_partition_scan_work);
 	INIT_DELAYED_WORK(&head->remove_work, nvme_remove_head_work);
 
@@ -849,9 +855,16 @@ static void nvme_mpath_set_live(struct nvme_ns *ns)
 			__nvme_find_path(head, node);
 		srcu_read_unlock(&head->srcu, srcu_idx);
 	}
+	/*
+	 * Have nvme_requeue_work() wait for the grace period instead of
+	 * blocking here, so that namespaces going live together, e.g. on
+	 * an ANA change, do not wait for one grace period each. Start it
+	 * under head->lock so that the newest cookie is stored last.
+	 */
+	WRITE_ONCE(head->requeue_srcu_cookie,
+		   start_poll_synchronize_srcu(&head->srcu));
 	mutex_unlock(&head->lock);
 
-	synchronize_srcu(&head->srcu);
 	nvme_mpath_revalidate_zones(head);
 	kblockd_schedule_work(&head->requeue_work);
 }
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index 8833a463f1c84..3fff9b394b1cd 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -587,6 +587,7 @@ struct nvme_ns_head {
 		__guarded_by(&requeue_lock);
 	spinlock_t		requeue_lock;
 	struct work_struct	requeue_work;
+	unsigned long		requeue_srcu_cookie;
 	struct work_struct	partition_scan_work;
 	struct mutex		lock;
 	unsigned long		flags;

-- 
2.43.0

Re: [PATCH v3 1/2] nvme-multipath: apply delayed_removal_secs changes to a pending removal

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] nvme-multipath: race in delayed_removal_secs_store() re-arms
  remove_work
--

commit 92bcf5100d1d5ed726ca5da1431d5f936dbea0e9
Author: Qiliang Yuan <qiliang.yuan@linux.dev>

nvme-multipath: apply delayed_removal_secs changes to a pending removal

This commit cancels and requeues any pending head->remove_work when
delayed_removal_secs is written via sysfs so the updated delay takes
effect immediately.

> diff --git a/drivers/nvme/host/multipath.c b/drivers/nvme/host/multipath.c
> index ae8059366b57d..35278b9c4de60 100644
> --- a/drivers/nvme/host/multipath.c
> +++ b/drivers/nvme/host/multipath.c
> @@ -1242,6 +1242,9 @@ static ssize_t delayed_removal_secs_store(struct device *dev,
>  	mutex_lock(&head->subsys->lock);
>  	head->delayed_removal_secs = sec;
>  	assign_bit(NVME_NSHEAD_QUEUE_IF_NO_PATH, &head->flags, sec);
> +	/* Apply the new delay to a removal that is already pending */
> +	if (cancel_delayed_work(&head->remove_work))
> +		queue_delayed_work(nvme_wq, &head->remove_work, sec * HZ);

[Severity: Medium]
Can this race with nvme_init_ns_head() and re-arm head->remove_work after
a path has reconnected?

drivers/nvme/host/core.c:nvme_init_ns_head() {
	...
	list_add_tail_rcu(&ns->siblings, &head->list);
	ns->head = head;
	mutex_unlock(&ctrl->subsys->lock);

#ifdef CONFIG_NVME_MULTIPATH
	if (cancel_delayed_work(&head->remove_work))
		module_put(THIS_MODULE);
#endif
}

nvme_init_ns_head() adds the new namespace to head->list under
subsys->lock, but cancels head->remove_work after dropping the lock.  If
delayed_removal_secs_store() runs in between, it clears the pending bit on
head->remove_work before nvme_init_ns_head() checks it, and then requeues
the work:

CPU0                              CPU1
nvme_alloc_ns()
  nvme_init_ns_head()
    mutex_lock(&subsys->lock)
    list_add_tail_rcu(..., &head->list)
    mutex_unlock(&subsys->lock)
                                  delayed_removal_secs_store()
                                    mutex_lock(&subsys->lock)
                                    cancel_delayed_work(&head->remove_work)
                                      (returns true)
    cancel_delayed_work(&head->remove_work)
      (returns false)
                                    queue_delayed_work(..., &head->remove_work, ...)
                                    mutex_unlock(&subsys->lock)
  ...
  out_unlink_ns:
    nvme_put_ns_head(ns->head)    <-- frees head with remove_work queued

This leaves head->remove_work pending while a path is on head->list.  If
nvme_alloc_ns() then fails and takes its out_unlink_ns error path, it
drops both references and frees head while head->remove_work is still
queued on nvme_wq, causing a use-after-free when the work runs.

Checking list_empty(&head->list) under head->subsys->lock before
cancelling and requeuing head->remove_work (or moving the cancel in
nvme_init_ns_head() inside subsys->lock) would avoid this race.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261114-bug-nvme-mpath-set-live-srcu-v3-0-d84651de6334@linux.dev?part=1