Re: [PATCH] libmultipath: remove buggy reinstate_paths function

Martin Wilck
2025-03-04T22:30:32Z

On Mon, 2025-03-03 at 14:56 -0500, Benjamin Marzinski wrote:
> The purpose of reinstate_paths() is to reinstate all the paths on a
> multipath device that multipathd thinks are usable, similar to
> sync_map_state(). However, reinstate_paths() doesn't work correctly.
> For instance, it will always reinstate every path in enabled (as
> opposed
> to active or disabled) pathgroups. This is clearly wrong. It might be
> badly written code to avoid enabling paths in PATH_GHOST in active
> pathgroups, but that's just a guess and isn't necessary at any rate.
>
> It's called in two places. The first is when multipath is run with
> CMD_CREATE. The second is when domap() is run with a mpp->action of
> ACT_SWITCHPG or ACT_SWITCHPG_RENAME. This case just exists to run
> extra
> reinstates for paths that are not in PATH_UP, on pathgroups that are
> now
> in the enabled state, instead of the active state. This is old code.
> I'm
> not sure if it ever made sense to do this, but it certainly doesn't
> now.
>
> Multipathd already will make sure that its path states are synced
> with
> the kernel states whenever either the paths get checked or a dm event
> occurs. It makes sense to additionally sync with the kernel state
> when a
> multipath device is reloaded, like sync_map_state() currently does,
> since the path's kernel state will start out of sync with
> multipathd's
> state.
>
> However, if multipathd isn't running, I can see the benefit of being
> able to reinstate paths by running "multipath". So if multipath is
> run to create or reload multipath devices (CMD_CREATE), it will now
> call
> sync_map_state() with a flag to make it behave like reinstate_paths()
> did (it will only reinstate paths, but never preemptively fail them).
> I
> thought about only doing this if check_daemon() said that multipathd
> wasn't running, but perhaps people are relying on running multipath
> to reinstate paths before the next scheduled checker run.
>
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>

Reviewed-by: Martin Wilck <mwilck@suse.com>

PS: this doesn't look like "stable" material to me. Right?