On Fri, 2025-10-31 at 15:19 -0400, Benjamin Marzinski wrote: > When libmpathpersist registers a key, It first checks which paths are > active, then registers the key on those paths, and then tells > multipathd > that the key has been registered, and it should start tracking it. If > a path comes up after libmpathpersist checks for active paths, but > before > it tells multipathd that the key has been registered, multipathd will > not > register a key no that path (since it hasn't been told to that the > device > has a registered key yet). This can leave the device with a path that > is > missing a key. > > To solve this, when multipathd is told that a key has been > registered, > it checks if there are the same number of registered keys as active > paths. If there aren't, it registers keys on all the paths (at least > until the number of registered keys and active paths are equal).
You may want to mention here that the nr_key_wanted check in mpath_pr_event_handle() and the nr_active check in pr_register_active_paths actually allow us to short-circuit a couple of PR operations. > > Signed-off-by: Benjamin Marzinski <[email protected]> > --- > multipathd/cli_handlers.c | 6 ++++- > multipathd/main.c | 47 ++++++++++++++++++++++++++++--------- > -- > multipathd/main.h | 1 + > 3 files changed, 40 insertions(+), 14 deletions(-) > > diff --git a/multipathd/cli_handlers.c b/multipathd/cli_handlers.c > index 7f572fb4..2812d01e 100644 > --- a/multipathd/cli_handlers.c > +++ b/multipathd/cli_handlers.c > @@ -1291,7 +1291,11 @@ cli_setprstatus(void * v, struct strbuf > *reply, void * data) > > if (mpp->prflag != PR_SET) { > set_pr(mpp); > - condlog(2, "%s: prflag set", param); > + pr_register_active_paths(mpp, true); > + if (mpp->prflag == PR_SET) > + condlog(2, "%s: prflag set", param); > + else > + condlog(0, "%s: Failed to set prflag", > param); > } > memset(&mpp->old_pr_key, 0, 8); > > diff --git a/multipathd/main.c b/multipathd/main.c > index d11a8576..70268d98 100644 > --- a/multipathd/main.c > +++ b/multipathd/main.c > @@ -90,7 +90,8 @@ > #define MSG_SIZE 32 > > static unsigned int > -mpath_pr_event_handle(struct path *pp, unsigned int nr_keys_needed); > +mpath_pr_event_handle(struct path *pp, unsigned int nr_keys_needed, > + unsigned int nr_keys_wanted); > > #define LOG_MSG(lvl, pp) \ > do { \ > @@ -629,19 +630,28 @@ flush_map_nopaths(struct multipath *mpp, struct > vectors *vecs) { > return true; > } > > -static void > -pr_register_active_paths(struct multipath *mpp) > +void pr_register_active_paths(struct multipath *mpp, bool > check_nr_active) > { > unsigned int i, j, nr_keys = 0; > + unsigned int nr_active = 0; > struct path *pp; > struct pathgroup *pgp; > > + if (check_nr_active) { > + nr_active = count_active_paths(mpp); > + if (!nr_active) > + return; > + } > + > vector_foreach_slot (mpp->pg, pgp, i) { > vector_foreach_slot (pgp->paths, pp, j) { > if (mpp->prflag == PR_UNSET) > return; > - if ((pp->state == PATH_UP) || (pp->state == > PATH_GHOST)) > - nr_keys = mpath_pr_event_handle(pp, > nr_keys); > + if (pp->state == PATH_UP || pp->state == > PATH_GHOST) { > + nr_keys = mpath_pr_event_handle(pp, > nr_keys, nr_active); > + if (check_nr_active && nr_keys == > nr_active) > + return; > + } > } > } > } > @@ -729,7 +739,7 @@ fail: > > sync_map_state(mpp, false); > > - pr_register_active_paths(mpp); > + pr_register_active_paths(mpp, false); > > if (VECTOR_SIZE(offline_paths) != 0) > handle_orphaned_offline_paths(offline_paths); > @@ -1319,7 +1329,7 @@ rescan: > verify_paths(mpp); > mpp->action = ACT_RELOAD; > prflag = mpp->prflag; > - mpath_pr_event_handle(pp, 0); > + mpath_pr_event_handle(pp, 0, 0); > } else { > if (!should_multipath(pp, vecs->pathvec, vecs- > >mpvec)) { > orphan_path(pp, "only one path"); > @@ -1403,7 +1413,7 @@ rescan: > > if (retries >= 0) { > if ((mpp->prflag == PR_SET && prflag != PR_SET) || > start_waiter) > - pr_register_active_paths(mpp); > + pr_register_active_paths(mpp, false); > condlog(2, "%s [%s]: path added to devmap %s", > pp->dev, pp->dev_t, mpp->alias); > return 0; > @@ -2646,9 +2656,9 @@ update_path_state (struct vectors * vecs, > struct path * pp) > */ > condlog(2, "%s: checking persistent " > "reservation registration", pp- > >dev); > - mpath_pr_event_handle(pp, 0); > + mpath_pr_event_handle(pp, 0, 0); > if (pp->mpp->prflag == PR_SET && prflag != > PR_SET) > - pr_register_active_paths(pp->mpp); > + pr_register_active_paths(pp->mpp, > false); > } > > /* > @@ -3309,7 +3319,7 @@ configure (struct vectors * vecs, enum > force_reload_types reload_type) > vector_foreach_slot(mpvec, mpp, i){ > if (remember_wwid(mpp->wwid) == 1) > trigger_paths_udev_change(mpp, true); > - pr_register_active_paths(mpp); > + pr_register_active_paths(mpp, false); > } > > /* > @@ -4341,16 +4351,25 @@ static int update_map_pr(struct multipath > *mpp, struct path *pp, unsigned int *n > } > > /* > - * This function is called with the number of registered keys that > should be > + * This function is called with two numbers > + * > + * nr_keys_needed: the number of registered keys that should be > * seen for this device to know that the key has not been preempted > while the > * path was getting registered. If 0 is passed in, update_mpath_pr > is called > * before registering the key to figure out the number, assuming > that at > * least one key must exist. > * > + * nr_keys_wanted: Only used if nr_keys_needed is 0, so we don't > know how > + * many keys we currently have. If nr_keys_wanted in non-zero and > the > + * number of keys found by the initial call to update_map_pr() > matches it, > + * exit early, since we have all the keys we are expecting. > + * > * The function returns the number of keys that are registered or 0 > if > * it's unknown. > */ > -static unsigned int mpath_pr_event_handle(struct path *pp, unsigned > int nr_keys_needed) > +static unsigned int > +mpath_pr_event_handle(struct path *pp, unsigned int nr_keys_needed, > + unsigned int nr_keys_wanted) > { > struct multipath *mpp = pp->mpp; > int ret; > @@ -4366,6 +4385,8 @@ static unsigned int > mpath_pr_event_handle(struct path *pp, unsigned int nr_keys_ > nr_keys_needed = 1; > if (update_map_pr(mpp, pp, &nr_keys_needed) != > MPATH_PR_SUCCESS) > return 0; > + if (nr_keys_wanted && nr_keys_wanted == > nr_keys_needed) > + return nr_keys_needed; I am wondering about the case that prflag gets unset by update_map_pr() while nr_active is 1. In this case we'd return early without clearing the registration. Is that correct? I get it that this must mean that update_map_pr() has found 0 paths with this key registered. But the comment below in the mpp->prflag != PR_SET suggests that maybe the registration should still be cleared. Please explain. Regards Martin > } > > check_prhold(mpp, pp); > diff --git a/multipathd/main.h b/multipathd/main.h > index 29b57e3d..6d60ee81 100644 > --- a/multipathd/main.h > +++ b/multipathd/main.h > @@ -54,4 +54,5 @@ int resize_map(struct multipath *mpp, unsigned long > long size, > struct vectors *vecs); > void set_pr(struct multipath *mpp); > void unset_pr(struct multipath *mpp); > +void pr_register_active_paths(struct multipath *mpp, bool > check_active_nr); > #endif /* MAIN_H_INCLUDED */
