On Thu, Nov 06, 2025 at 09:15:48PM +0100, Martin Wilck wrote: > 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.
Sure. I sort of alude to it, but admittedly, it's not at all obvious that the first call to update_map_pr() in mpath_pr_event_handle(), when nr_keys_wanted is set, is what I'm referring to when I say that "it checks if there are the same number of registered keys as active paths." > > > > > 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. Ah. When update_map_pr() unsets prflag, it leaves nr_keys_needed alone, meaning that you could exit via the shortcut when called with nr_keys_wanted set to 1, instead running the clear code. I didn't think about that. Good catch. The number of things that would have to go just perfectly wrong for this to cause a problem is pretty staggering (everything necessary for the bug I'm trying to fix, plus a preempt coming in, and then the path that libmpathpersist did register the key on failing in multipathd, all between when libmpathpersist starts registering the key, and when multipathd processes the "setprstatus" command). But it is, at least in theory, possible. I'll send an updated patch that adds some explanation to the commit message and sets *nr_keys to zero in update_map_pr() when we return with mpp->prflag unset. That will make us run the code to clear any old registration from the path. -Ben > > 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 */
