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

[PATCH] 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.
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 over
nvme-tcp loopback, nvme_read_ana_log() takes (median, min-max in
parentheses):

  namespaces  before (ms)    after (ms)   change
         256   51 (32-72)     7 (2.5-12)    -86%
        1024  157 (133-203)  41 (24-65)     -74%

Signed-off-by: Qiliang Yuan <qiliang.yuan@linux.dev>
---
 drivers/nvme/host/multipath.c | 14 +++++++++++++-
 drivers/nvme/host/nvme.h      |  1 +
 2 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/drivers/nvme/host/multipath.c b/drivers/nvme/host/multipath.c
index ae8059366b57d..4bc94f165c17d 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);
 
@@ -851,7 +857,13 @@ static void nvme_mpath_set_live(struct nvme_ns *ns)
 	}
 	mutex_unlock(&head->lock);
 
-	synchronize_srcu(&head->srcu);
+	/*
+	 * 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.
+	 */
+	WRITE_ONCE(head->requeue_srcu_cookie,
+		   start_poll_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;

---
base-commit: b3dfbde62cf1981c4638939c159dfd087b7a72e9
change-id: 20261110-bug-nvme-mpath-set-live-srcu-cbd8965f1457

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

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] nvme-multipath: unlocked requeue_srcu_cookie update in
  nvme_mpath_set_live()
It also noticed 1 related pre-existing issue this patch does not
introduce:
- [High] nvme-multipath: stranded I/O in delayed_removal_secs_store()
  [1]
--

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

nvme-multipath: don't wait for a grace period in nvme_mpath_set_live()

This commit replaces the synchronous synchronize_srcu() call in
nvme_mpath_set_live() with start_poll_synchronize_srcu() and defers
waiting for the SRCU grace period to nvme_requeue_work().

> diff --git a/drivers/nvme/host/multipath.c b/drivers/nvme/host/multipath.c
> index ae8059366b57d..4bc94f165c17d 100644
> --- a/drivers/nvme/host/multipath.c
> +++ b/drivers/nvme/host/multipath.c

[ ... ]

> @@ -851,7 +857,13 @@ static void nvme_mpath_set_live(struct nvme_ns *ns)
>  	}
>  	mutex_unlock(&head->lock);
>  
> -	synchronize_srcu(&head->srcu);
> +	/*
> +	 * 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.
> +	 */
> +	WRITE_ONCE(head->requeue_srcu_cookie,
> +		   start_poll_synchronize_srcu(&head->srcu));
>  	nvme_mpath_revalidate_zones(head);
>  	kblockd_schedule_work(&head->requeue_work);
>  }

[Severity: Medium]
Can concurrent nvme_mpath_set_live() calls on the same head overwrite a
newer head->requeue_srcu_cookie with an older, already-completed cookie?

CPU0                              CPU1
nvme_mpath_set_live(ns1)
  mutex_unlock(&head->lock)
  c1 = start_poll_synchronize_srcu()
  // grace period c1 completes;
  // ns1 fails over again, and a
  // reader enters srcu_read_lock()
  // seeing no usable path
                                  nvme_mpath_set_live(ns2)
                                    mutex_unlock(&head->lock)
                                    c2 = start_poll_synchronize_srcu()
                                    WRITE_ONCE(requeue_srcu_cookie, c2)
  WRITE_ONCE(requeue_srcu_cookie, c1)
                                    kblockd_schedule_work(&requeue_work)
                                  nvme_requeue_work()
                                    poll_state_synchronize_srcu(..., c1)
                                      // returns true, skips sync
                                    bio_list_get(&head->requeue_list)
                                      <-- misses in-flight reader's bio

When that happens, nvme_requeue_work() drains head->requeue_list before
the in-flight nvme_ns_head_submit_bio() reader finishes queueing its bio,
leaving the bio stranded on head->requeue_list until a later path event.
Updating head->requeue_srcu_cookie before mutex_unlock(&head->lock) would
serialize the cookie generation and store.

[Severity: High]
This problem wasn't introduced by this patch, but in the same file,
delayed_removal_secs_store() [1] can leave parked bios stranded on
head->requeue_list when delayed_removal_secs is set to 0:

drivers/nvme/host/multipath.c:delayed_removal_secs_store() {
	...
	mutex_lock(&head->subsys->lock);
	head->delayed_removal_secs = sec;
	assign_bit(NVME_NSHEAD_QUEUE_IF_NO_PATH, &head->flags, sec);
	mutex_unlock(&head->subsys->lock);
	/*
	 * Ensure that update to NVME_NSHEAD_QUEUE_IF_NO_PATH is seen
	 * by its reader.
	 */
	synchronize_srcu(&head->srcu);

	return count;
}

When writing 0 clears NVME_NSHEAD_QUEUE_IF_NO_PATH while no paths are
usable, any bios already queued on head->requeue_list need to be drained
and failed, but delayed_removal_secs_store() returns without calling
kblockd_schedule_work(&head->requeue_work), leaving tasks waiting on
those bios hung until a separate path event schedules requeue_work.

[1] https://sashiko.dev/#/bug/linux-7d2cc5ea-2752-4473-b874-a955225beb1b

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261110-bug-nvme-mpath-set-live-srcu-v1-1-1f58b405b0f8@linux.dev?part=1