Re: [PATCH 03/13] nvme-multipath: numa support for marginal paths

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] Double evaluation of volatile controller state creates a TOCTOU race that can select a sub-optimal marginal path over a better non-marginal path.
- [Medium] The round-robin multipath I/O policy (`nvme_round_robin_path`) was not updated to handle marginal paths, bypassing the new `NVME_CTRL_MARGINAL` priority logic.
- [Low] The comment explaining the fallback path selection has inverted logic.
--

--- Patch [3]: [PATCH 03/13] nvme-multipath: numa support for marginal paths ---
commit 820a8648fdbfc8327ddaf233abd513210c265d17
Author: Jesse Taube <[email protected]>

nvme-multipath: numa support for marginal paths

This patch introduces the NVME_CTRL_MARGINAL flag to handle FPIN LI
(link integrity) messages indicating hardware errors. The multipath
scheduler is updated to direct I/O away from affected marginal ports by
skipping them when non-marginal optimized paths are available.

> diff --git a/drivers/nvme/host/multipath.c b/drivers/nvme/host/multipath.c
> index 9b9a657fa330f..02936c580a373 100644
> --- a/drivers/nvme/host/multipath.c
> +++ b/drivers/nvme/host/multipath.c

[ ... ]

> @@ -323,15 +357,19 @@ static struct nvme_ns *__nvme_find_path(struct nvme_ns_head *head, int node)
>  
>  		switch (ns->ana_state) {
>  		case NVME_ANA_OPTIMIZED:
> -			if (distance < found_distance) {
> +			if (is_best_distance(found_is_marginal, nvme_ctrl_is_marginal(ns->ctrl),
> +					     found_distance, distance)) {
>  				found_distance = distance;
>  				found = ns;
> +				found_is_marginal = nvme_ctrl_is_marginal(ns->ctrl);
                                                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

[Severity: Medium]
Could this double evaluation of nvme_ctrl_is_marginal() cause a race
condition?

nvme_ctrl_is_marginal() uses test_bit(), which performs a volatile memory
read. If the controller's marginal state transitions from false to true between
these two reads, a previously found non-marginal path will be incorrectly
replaced by this new path, which is then subsequently recorded as marginal.

Would it be better to read the marginal state into a local variable once per
iteration?

>  			}
>  			break;
>  		case NVME_ANA_NONOPTIMIZED:
> -			if (distance < fallback_distance) {
> +			if (is_best_distance(fallback_is_marginal, nvme_ctrl_is_marginal(ns->ctrl),
> +					     fallback_distance, distance)) {
>  				fallback_distance = distance;
>  				fallback = ns;
> +				fallback_is_marginal = nvme_ctrl_is_marginal(ns->ctrl);
                                                       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

[Severity: Medium]
Does this have the same race condition as the NVME_ANA_OPTIMIZED case above?

Using a local variable for the marginal state might prevent inconsistencies
if the bit changes between the two checks.

>  			}
>  			break;
>  		default:

[ ... ]

> @@ -339,6 +377,14 @@ static struct nvme_ns *__nvme_find_path(struct nvme_ns_head *head, int node)
>  		}
>  	}
>  
> +	/*
> +	 * Use non-optimized path only if it is not marginal
> +	 * and no optimized path is marginal.
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

[Severity: Low]
Is the logic in this comment inverted?

The code correctly falls back to a non-marginal path only when the optimized
path is marginal (found_is_marginal && !fallback_is_marginal). However, the
comment incorrectly states it falls back if "no optimized path is marginal",
which contradicts the implemented boolean logic.

> +	 */
> +	if (found_is_marginal && !fallback_is_marginal)
> +		found = fallback;
> +
> +	/* No optimized path found, use the fallback */
>  	if (!found)
>  		found = fallback;
>  	if (found)

[ ... ]

> @@ -444,7 +490,8 @@ static struct nvme_ns *nvme_queue_depth_path(struct nvme_ns_head *head)
>  static inline bool nvme_path_is_optimized(struct nvme_ns *ns)
>  {
>  	return nvme_ctrl_state(ns->ctrl) == NVME_CTRL_LIVE &&
> -		ns->ana_state == NVME_ANA_OPTIMIZED;
> +		ns->ana_state == NVME_ANA_OPTIMIZED &&
> +		!nvme_ctrl_is_marginal(ns->ctrl);
>  }

[Severity: Medium]
Since marginal paths are now supported and skipped in the NUMA and queue depth
policies, does nvme_round_robin_path() also need to be updated?

If a user configures iopolicy=round-robin, the multipath scheduler seems to
actively route I/O to marginal paths without checking the state:

drivers/nvme/host/multipath.c:nvme_round_robin_path() {
    ...
		if (ns->ana_state == NVME_ANA_OPTIMIZED) {
			found = ns;
			goto out;
		}
    ...
}

Could this risk I/O timeouts and dropped packets on unreliable hardware links
by bypassing the new NVME_CTRL_MARGINAL priority logic?

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