Re: [PATCH 05/13] nvme-multipath: round-robin support for marginal paths

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] Improper round-robin fallback logic causes path thrashing between marginal OPTIMIZED and marginal NONOPTIMIZED paths.
- [High] Path selection logic may incorrectly return paths in `INACCESSIBLE`, `CHANGE`, or `PERSISTENT_LOSS` ANA states.

Pre-existing issues:
- [High] The round-robin path selection can enter an infinite loop inside an SRCU read-side critical section if the current path is removed concurrently.
--

--- Patch [5]: [PATCH 05/13] nvme-multipath: round-robin support for marginal paths ---
commit f4b9e08d0e0306a7a08b9e274efbbe8c287e6dde
Author: Jesse Taube <[email protected]>

nvme-multipath: round-robin support for marginal paths

This commit modifies the NVMe round-robin path selection to exclude
marginal paths by default. It introduces logic to fall back to round-robin
on marginal paths only if no optimized or non-optimized non-marginal paths
are found.

> diff --git a/drivers/nvme/host/multipath.c b/drivers/nvme/host/multipath.c
> index dc34634842be8..11f474704fdd7 100644
> --- a/drivers/nvme/host/multipath.c
> +++ b/drivers/nvme/host/multipath.c

[ ... ]

> @@ -425,22 +426,58 @@ static struct nvme_ns *nvme_round_robin_path(struct nvme_ns_head *head)
>  	for (ns = nvme_next_ns(head, old);
>  	     ns && ns != old;
>  	     ns = nvme_next_ns(head, ns)) {

[Severity: High]
This is a pre-existing issue, but does this loop condition risk an infinite
loop if the current path is concurrently removed?

If old (the current path) is concurrently unlinked via list_del_rcu(),
nvme_next_ns() will continue into the active list, wrap around to the head,
and traverse indefinitely. Because old is no longer in the list, ns != old
remains true forever.

Since this patch makes it easier to fall through without hitting a goto out
(for example, when all paths are marginal), could this cause a hard lockup
in the SRCU read-side critical section?

[ ... ]

>  	/*
>  	 * The loop above skips the current path for round-robin semantics.
>  	 * Fall back to the current path if either:
> -	 *  - no other optimized path found and current is optimized,
> +	 *  - no other non-marginal optimized path found and current is,
> +	 *      optimized and not marginal.
>  	 *  - no other usable path found and current is usable.
>  	 */
> -	if (!nvme_path_is_disabled(old) &&
> -	    (old->ana_state == NVME_ANA_OPTIMIZED ||
> -	     (!found && old->ana_state == NVME_ANA_NONOPTIMIZED)))
> +	/* no other usable path found and current is usable. */
> +	if (!nvme_path_is_disabled(old) && !found)
> +		return old;

[Severity: High]
Could this fallback condition incorrectly return paths that are in
inaccessible, change, or persistent loss ANA states?

When the current path transitions to an inaccessible state and no other
usable paths exist, the loop finds no paths (found is NULL). Because
nvme_path_is_disabled() does not check the ANA state, this fallback will
return the old path.

Will this lead to continuous submission of I/O to inaccessible paths and
unnecessary ANA log reads?

> +	/*
> +	 * no other non-marginal optimized path found and current is,
> +	 *   optimized and not marginal.
> +	 */
> +	if (!nvme_path_is_disabled(old) && !nvme_ctrl_is_marginal(old->ctrl) &&
> +	    (old->ana_state == NVME_ANA_OPTIMIZED || found_is_marginal))
>  		return old;

[Severity: High]
Does this logic cause path thrashing between marginal optimized and
marginal non-optimized paths?

If the current path is marginal and optimized, and the loop finds a
marginal non-optimized path, this fallback unconditionally skips retaining
the current marginal path because of the !nvme_ctrl_is_marginal(old->ctrl)
check.

It then selects the non-optimized path instead. On the next I/O, the
reverse happens, causing a bounce between the two paths. Should marginal
optimized paths be retained over marginal non-optimized ones?

>  
>  	if (!found)

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=5