[PATCH] multipathd: Fix race while registering PR key

[PATCH] multipathd: Fix race while registering PR key

[Benjamin Marzinski]

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).

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 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;
 	}
 
 	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 */
-- 
2.50.1

Re: [PATCH] multipathd: Fix race while registering PR key

[Martin Wilck]

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 <bmarzins@redhat.com>
> ---
>  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 */

Re: [PATCH] multipathd: Fix race while registering PR key

[Benjamin Marzinski]

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 <bmarzins@redhat.com>
> > ---
> >  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 */