Re: [PATCH v3 08/13] libmultipath: Add sysfs helpers

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] mpath_iopolicy_store() does not clear cached paths when the I/O policy changes, contradicting the commit message and leaving stale I/O routing state.
- [Low] Unused 'count' parameter in mpath_iopolicy_store().
--

commit e168713423f62d0ecfcf0e0b0c9ed72180e1f19b
Author: John Garry <john.g.garry@oracle.com>

libmultipath: Add sysfs helpers

This commit adds sysfs helpers for the libmultipath driver sysfs code. It
includes helpers to get/set I/O policies, show device paths per NUMA node,
and handle attribute groups.

>     Note that mpath_iopolicy_store() has a update callback to allow same
>     functionality as nvme_subsys_iopolicy_update() be run for clearing paths.

[Severity: High]
Does this description match the implementation? The code doesn't seem to provide
the callback parameter mentioned here.


> diff --git a/lib/multipath.c b/lib/multipath.c
> index 6d2e1186a10f8..4945f2d847fbf 100644
> --- a/lib/multipath.c
> +++ b/lib/multipath.c
[ ... ]
> +static void mpath_iopolicy_update(enum mpath_iopolicy_e *iopolicy,
> +		int new)
> +{
> +	int old = READ_ONCE(*iopolicy);
> +
> +	if (old == new)
> +		return;
> +
> +	WRITE_ONCE(*iopolicy, new);
> +
> +	pr_info("iopolicy changed from %s to %s\n",
> +		mpath_iopolicy_names[old],
> +		mpath_iopolicy_names[new]);
> +}
> +
> +bool mpath_iopolicy_store(enum mpath_iopolicy_e *iopolicy,
> +				const char *buf, size_t count)
                                                 ^^^^^^^^^^^^

[Severity: Low]
This isn't a bug, but is the count parameter needed here? It doesn't
appear to be used in the function body since sysfs_streq() is used for the
string comparison.

> +{
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(mpath_iopolicy_names); i++) {
> +		if (sysfs_streq(buf, mpath_iopolicy_names[i])) {
> +			mpath_iopolicy_update(iopolicy, i);

[Severity: High]
Does changing the I/O policy via sysfs leave stale paths cached under the old
policy rules?

Neither mpath_iopolicy_store() nor mpath_iopolicy_update() accept a callback
function pointer, nor do they clear the paths themselves. As a result, drivers
adopting this API have no mechanism to hook into the change to acquire
subsystem locks or clear active paths.

> +			return true;
> +		}
> +	}
> +
> +	return false;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260703102918.3723667-1-john.g.garry@oracle.com?part=8