| Message ID | 20250303195628.2998595-1-bmarzins@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | libmultipath: remove buggy reinstate_paths function | expand |
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?
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
diff --git a/libmultipath/configure.c b/libmultipath/configure.c index 939271fa..baa13573 100644 --- a/libmultipath/configure.c +++ b/libmultipath/configure.c @@ -804,35 +804,6 @@ void select_action (struct multipath *mpp, const struct vector_s *curmp, return; } -int reinstate_paths(struct multipath *mpp) -{ - int i, j; - struct pathgroup * pgp; - struct path * pp; - - if (!mpp->pg) - return 0; - - vector_foreach_slot (mpp->pg, pgp, i) { - if (!pgp->paths) - continue; - - vector_foreach_slot (pgp->paths, pp, j) { - if (pp->state != PATH_UP && - (pgp->status == PGSTATE_DISABLED || - pgp->status == PGSTATE_ACTIVE)) - continue; - - if (pp->dmstate == PSTATE_FAILED) { - if (dm_reinstate_path(mpp->alias, pp->dev_t)) - condlog(0, "%s: error reinstating", - pp->dev); - } - } - } - return 0; -} - static int lock_multipath (struct multipath * mpp, int lock) { @@ -943,12 +914,6 @@ int domap(struct multipath *mpp, char *params, int is_daemon) case ACT_SWITCHPG: case ACT_SWITCHPG_RENAME: dm_switchgroup(mpp->alias, mpp->bestpg); - /* - * we may have avoided reinstating paths because there where in - * active or disabled PG. Now that the topology has changed, - * retry. - */ - reinstate_paths(mpp); return DOMAP_EXIST; case ACT_CREATE: diff --git a/libmultipath/configure.h b/libmultipath/configure.h index f98a3b21..45ac54fd 100644 --- a/libmultipath/configure.h +++ b/libmultipath/configure.h @@ -56,7 +56,6 @@ int setup_map (struct multipath * mpp, char **params, struct vectors *vecs); void select_action (struct multipath *mpp, const struct vector_s *curmp, int force_reload); int domap (struct multipath * mpp, char * params, int is_daemon); -int reinstate_paths (struct multipath *mpp); int coalesce_paths (struct vectors *vecs, vector curmp, char * refwwid, int force_reload, enum mpath_cmds cmd); int get_refwwid (enum mpath_cmds cmd, const char *dev, enum devtypes dev_type, vector pathvec, char **wwid); diff --git a/libmultipath/libmultipath.version b/libmultipath/libmultipath.version index b14ce6be..9fda717c 100644 --- a/libmultipath/libmultipath.version +++ b/libmultipath/libmultipath.version @@ -153,7 +153,6 @@ global: print_all_paths; print_foreign_topology; print_multipath_topology__; - reinstate_paths; remember_wwid; remove_feature; remove_map; diff --git a/libmultipath/structs_vec.c b/libmultipath/structs_vec.c index a7a07b04..f6407e12 100644 --- a/libmultipath/structs_vec.c +++ b/libmultipath/structs_vec.c @@ -730,7 +730,7 @@ void set_no_path_retry(struct multipath *mpp) } void -sync_map_state(struct multipath *mpp) +sync_map_state(struct multipath *mpp, bool reinstate_only) { struct pathgroup *pgp; struct path *pp; @@ -752,7 +752,8 @@ sync_map_state(struct multipath *mpp) pp->dmstate == PSTATE_UNDEF) && (pp->state == PATH_UP || pp->state == PATH_GHOST)) dm_reinstate_path(mpp->alias, pp->dev_t); - else if ((pp->dmstate == PSTATE_ACTIVE || + else if (!reinstate_only && + (pp->dmstate == PSTATE_ACTIVE || pp->dmstate == PSTATE_UNDEF) && (pp->state == PATH_DOWN || pp->state == PATH_SHAKY)) { diff --git a/libmultipath/structs_vec.h b/libmultipath/structs_vec.h index 03bf8ee1..1188adac 100644 --- a/libmultipath/structs_vec.h +++ b/libmultipath/structs_vec.h @@ -27,7 +27,7 @@ void remove_map (struct multipath *mpp, vector pathvec, vector mpvec); void remove_map_by_alias(const char *alias, struct vectors * vecs); void remove_maps (struct vectors * vecs); -void sync_map_state (struct multipath *); +void sync_map_state (struct multipath *mpp, bool reinstate_only); struct multipath * add_map_with_path (struct vectors * vecs, struct path * pp, int add_vec, const struct multipath *current_mpp); diff --git a/multipath/main.c b/multipath/main.c index de9c33e4..9967dc6d 100644 --- a/multipath/main.c +++ b/multipath/main.c @@ -210,7 +210,7 @@ get_dm_mpvec (enum mpath_cmds cmd, vector curmp, vector pathvec, char * refwwid) print_multipath_topology(mpp, libmp_verbosity); if (cmd == CMD_CREATE) - reinstate_paths(mpp); + sync_map_state(mpp, true); } if (cmd == CMD_LIST_SHORT || cmd == CMD_LIST_LONG) diff --git a/multipathd/main.c b/multipathd/main.c index 3468b21f..e63b6aa7 100644 --- a/multipathd/main.c +++ b/multipathd/main.c @@ -688,7 +688,7 @@ fail: if (setup_multipath(vecs, mpp)) return 1; - sync_map_state(mpp); + sync_map_state(mpp, false); if (mpp->prflag != PRFLAG_SET) update_map_pr(mpp); @@ -806,7 +806,7 @@ sync_maps_state(vector mpvec) struct multipath *mpp; vector_foreach_slot (mpvec, mpp, i) - sync_map_state(mpp); + sync_map_state(mpp, false); } int @@ -1344,7 +1344,7 @@ rescan: if (setup_multipath(vecs, mpp)) goto fail; /* if setup_multipath fails, it removes the map */ - sync_map_state(mpp); + sync_map_state(mpp, false); if (retries >= 0) { if (start_waiter) @@ -1464,7 +1464,7 @@ ev_remove_path (struct path *pp, struct vectors * vecs, int need_do_map) strlcpy(devt, pp->dev_t, sizeof(devt)); if (setup_multipath(vecs, mpp)) return REMOVE_PATH_MAP_ERROR; - sync_map_state(mpp); + sync_map_state(mpp, false); if (retval == REMOVE_PATH_SUCCESS) condlog(2, "%s: path removed from map %s", @@ -1558,7 +1558,7 @@ int resize_map(struct multipath *mpp, unsigned long long size, out: if (setup_multipath(vecs, mpp) != 0) return 2; - sync_map_state(mpp); + sync_map_state(mpp, false); return ret; } @@ -2274,7 +2274,7 @@ int reload_and_sync_map(struct multipath *mpp, struct vectors *vecs) ret = 1; if (setup_multipath(vecs, mpp) != 0) return 2; - sync_map_state(mpp); + sync_map_state(mpp, false); return ret; }
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> --- libmultipath/configure.c | 35 ------------------------------- libmultipath/configure.h | 1 - libmultipath/libmultipath.version | 1 - libmultipath/structs_vec.c | 5 +++-- libmultipath/structs_vec.h | 2 +- multipath/main.c | 2 +- multipathd/main.c | 12 +++++------ 7 files changed, 11 insertions(+), 47 deletions(-)