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 <[email protected]> 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/[email protected]?part=4
