Re: [PATCH] libmultipath: remove buggy reinstate_paths function

[Benjamin Marzinski <bmarzins@redhat.com>]

On Tue, Mar 04, 2025 at 11:30:29PM +0100, Martin Wilck wrote:
> 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?

I don't think so. It's not fixing a bug. I would think code cleanups
don't need to be in the stable branch unless there are stable-worthy
bugfixes that apply on top of them, and its easier to bring back the
cleanup rather than rewrite the bugfix.

-Ben