Re: [PATCH v9 04/29] libmultipath: Add path selection support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] libmultipath: inconsistent path optimization check between
  is_optimized() and access_state
--

commit 5d198416c184ad80e962a53ac50fbd829ed2c691
Author: John Garry <john.garry@linux.dev>

libmultipath: Add path selection support

This commit adds multipath path selection support for NUMA, round-robin,
and queue-depth I/O policies along with path access state abstractions.

> diff --git a/lib/multipath.c b/lib/multipath.c
> index 93c0ca8b390b1..b8ea8478037ef 100644
> --- a/lib/multipath.c
> +++ b/lib/multipath.c
> @@ -8,8 +8,245 @@
>  #include <linux/multipath.h>
>  #include <linux/wait_bit.h>
>  
> +static struct mpath_device *mpath_find_path(struct mpath_head *mpath_head);
> +
[ ... ]
> +static struct mpath_device *__mpath_find_path(struct mpath_head *mpath_head,
> +					int node)
> +{
> +	int found_distance = INT_MAX, fallback_distance = INT_MAX, distance;
> +	struct mpath_device *found = NULL, *fallback = NULL, *mpath_device;
> +
> +	list_for_each_entry_srcu(mpath_device, &mpath_head->dev_list, siblings,
> +		srcu_read_lock_held(&mpath_head->srcu)) {
> +		if (mpath_path_is_disabled(mpath_head, mpath_device))
> +			continue;
> +
> +		if (mpath_device->numa_node != NUMA_NO_NODE &&
> +		    (mpath_read_iopolicy(mpath_head) ==
> +			MPATH_IOPOLICY_NUMA))
> +			distance = node_distance(node,
> +					mpath_device->numa_node);
> +		else
> +			distance = LOCAL_DISTANCE;
> +
> +		switch(mpath_device->access_state) {
> +		case MPATH_STATE_OPTIMIZED:
> +		    if (distance < found_distance) {
> +			    found_distance = distance;
> +			    found = mpath_device;
> +		    }
> +		    break;
> +		case MPATH_STATE_NONOPTIMIZED:
> +		    if (distance < fallback_distance) {
> +			    fallback_distance = distance;
> +			    fallback = mpath_device;
> +		    }
> +		    break;
> +		default:
> +		    break;
> +		}
> +	}
[ ... ]
> +static inline bool mpath_path_is_optimized(struct mpath_head *mpath_head,
> +				struct mpath_device *mpath_device)
> +{
> +	return mpath_head->mpdt->is_optimized(mpath_device);
> +}
> +
> +static struct mpath_device *mpath_numa_path(struct mpath_head *mpath_head)
> +{
> +	int node = numa_node_id();
> +	struct mpath_device *mpath_device;
> +
> +	mpath_device = srcu_dereference(mpath_head->current_path[node],
> +					&mpath_head->srcu);
> +	if (unlikely(!mpath_device))
> +		return __mpath_find_path(mpath_head, node);
> +	if (unlikely(mpath_path_is_disabled(mpath_head, mpath_device)))
> +		return __mpath_find_path(mpath_head, node);
> +	if (unlikely(!mpath_path_is_optimized(mpath_head, mpath_device)))
> +		return __mpath_find_path(mpath_head, node);
> +	return mpath_device;
> +}

[Severity: Medium]
Should __mpath_find_path(), mpath_round_robin_path(), and
mpath_queue_depth_path() check mpath_path_is_optimized() (or a callback
returning enum mpath_access_state) instead of reading
mpath_device->access_state directly?

mpath_numa_path() and mpath_device_set_live() call
mpath_path_is_optimized(), which invokes mpath_head->mpdt->is_optimized(),
whereas __mpath_find_path(), mpath_round_robin_path(), and
mpath_queue_depth_path() inspect mpath_device->access_state. If a driver
reports path optimization dynamically through .is_optimized() without
updating mpath_device->access_state (or vice versa), the two checks
disagree: when current_path[node] is non-optimized according to
.is_optimized() but still has access_state set to MPATH_STATE_OPTIMIZED,
mpath_numa_path() falls back to __mpath_find_path() on every I/O, and
__mpath_find_path() treats that non-optimized path as optimized and can
re-select it over a genuinely optimized path at a greater NUMA distance.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261108204733.557995-1-john.garry@linux.dev?part=4