[PATCH v4 0/1] multipathd: recheck WWIDs of re-added paths with the device

[PATCH v4 0/1] multipathd: recheck WWIDs of re-added paths with the device

[Brian Bunker]

During an array controller failover on an iSCSI host, multipathd
briefly put a path to one LUN into the map of another LUN, and I/O
meant for the first LUN landed on the second. See the v1 cover letter
for the full sequence, and my reply to Martin on v3 for the open
questions about it.

In short: a change uevent reported the wrong WWID for one path. The
removal of the path from its map then blocked for 4.4 seconds while
the map was suspended, and the re-add used that same, by then 4.4
second old, uevent to put the path into the wrong map. The next
INQUIRY on the path, 140 ms after the re-add, returned the correct
WWID.

Changes in v4, after Martin's and Ben's review of v3:

- Back to SG_IO, as in v2. The re-add check reads VPD page 0x83 from
  the device with check_path_wwid_change(), the same function
  recheck_wwid already uses, instead of comparing with the cached
  sysfs page. The cached page can be stale, so it isn't a better
  reference than udev's own INQUIRY.

- The check only runs if recheck_wwid is set. It is off by default,
  so nothing changes for users who don't enable it.

- Still only on PATH_ADD_REINIT, i.e. only for the re-add at the end
  of uev_update_path(). Ordinary adds never run it.

- No changes to libmultipath. get_sysfs_wwid() and the ABI version
  bump from v3 are gone.

- The uev_update_path() check from v2 that ignored a changed udev WWID
  stays dropped.

- multipath.conf(5) documents the new case under recheck_wwid.

We've seen the problem once and haven't reproduced it. We have been
running a v1-style patch on our test initiators since then without a
recurrence, which at this rate doesn't prove much either way.


Brian Bunker (1):
  multipathd: recheck WWIDs of re-added paths with the device

 multipath/multipath.conf.5.in |  8 +++++---
 multipathd/cli_handlers.c     |  2 +-
 multipathd/main.c             | 37 ++++++++++++++++++++++++-----------
 multipathd/main.h             | 13 +++++++++++-
 4 files changed, 44 insertions(+), 16 deletions(-)

-- 
2.56.0

[PATCH v4 1/1] multipathd: recheck WWIDs of re-added paths with the device

[Brian Bunker]

multipathd takes a path's WWID from udev. When a change uevent reports
a different WWID, uev_update_path() removes the path from its map and
re-adds it through uev_add_path(), which places it in whatever map owns
the new WWID. The removal can block for seconds while the map is
suspended, so by the time the path is re-added, the WWID in the uevent
may no longer be what the device reports. If it is wrong, the path goes
into the map of a different LUN and I/O for one LUN is silently sent to
another.

If recheck_wwid is set, confirm the WWID with the device over SG_IO
before such a re-add puts the path into a map, as recheck_wwid already
does when a failed path comes back. If the device doesn't confirm it,
leave the path out of the map: orphan it, clear its WWID and mark it
INIT_MISSING_UDEV, so the checker retriggers udev and the path is
re-added from fresh data.

To tell re-adds from ordinary adds, turn the need_do_map argument of
uev_add_path() and ev_add_path() into enum path_add_mode, with a new
PATH_ADD_REINIT value that behaves like PATH_ADD_MAP and is passed only
for the re-add at the end of uev_update_path(). Ordinary adds are
unchanged.

Document the new case under recheck_wwid in multipath.conf(5).

Signed-off-by: Brian Bunker <brian@purestorage.com>
---
 multipath/multipath.conf.5.in |  8 +++++---
 multipathd/cli_handlers.c     |  2 +-
 multipathd/main.c             | 37 ++++++++++++++++++++++++-----------
 multipathd/main.h             | 13 +++++++++++-
 4 files changed, 44 insertions(+), 16 deletions(-)

diff --git a/multipath/multipath.conf.5.in b/multipath/multipath.conf.5.in
index a577dfc8..8f358808 100644
--- a/multipath/multipath.conf.5.in
+++ b/multipath/multipath.conf.5.in
@@ -1382,9 +1382,11 @@ The default is: \fB\(dqNONE\(dq\fR
 If set to \fIyes\fR, when a failed path is restored, its wwid is rechecked. If
 the wwid has changed, the path is removed from the current multipath device,
 and re-added as a new path. Multipathd will also recheck a path's wwid if it is
-manually re-added. This option only works for SCSI devices that are configured
-to use the default uid_attribute, \fIID_SERIAL\fR, or sysfs for getting their
-wwid.
+manually re-added, and before re-adding a path whose wwid udev reported as
+changed. If the device doesn't confirm the new wwid, the path is left out of
+the multipath device and udev is asked to report its wwid again. This option
+only works for SCSI devices that are configured to use the default
+uid_attribute, \fIID_SERIAL\fR, or sysfs for getting their wwid.
 .RS
 .TP
 The default is: \fBno\fR
diff --git a/multipathd/cli_handlers.c b/multipathd/cli_handlers.c
index f5a13c46..9b4a04c2 100644
--- a/multipathd/cli_handlers.c
+++ b/multipathd/cli_handlers.c
@@ -673,7 +673,7 @@ cli_add_path (void *v, struct strbuf *reply, void *data)
 			return 1;
 		}
 	}
-	return ev_add_path(pp, vecs, 1);
+	return ev_add_path(pp, vecs, PATH_ADD_MAP);
 blacklisted:
 	append_strbuf_str(reply, "blacklisted\n");
 	condlog(2, "%s: path blacklisted", param);
diff --git a/multipathd/main.c b/multipathd/main.c
index 9bce4d6b..fdf5503b 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -1128,7 +1128,7 @@ check_path_wwid_change(struct path *pp)
 static int uev_update_path (struct uevent *uev, struct vectors * vecs);
 
 static int
-uev_add_path (struct uevent *uev, struct vectors * vecs, int need_do_map)
+uev_add_path(struct uevent *uev, struct vectors *vecs, enum path_add_mode mode)
 {
 	struct path *pp;
 	int ret = 0, i;
@@ -1180,7 +1180,7 @@ uev_add_path (struct uevent *uev, struct vectors * vecs, int need_do_map)
 				     DI_ALL | DI_BLACKLIST);
 			pthread_cleanup_pop(1);
 			if (r == PATHINFO_OK && !prev_mpp)
-				ret = ev_add_path(pp, vecs, need_do_map);
+				ret = ev_add_path(pp, vecs, mode);
 			else if (r == PATHINFO_OK &&
 				 !strncmp(pp->wwid, prev_mpp->wwid, WWID_SIZE)) {
 				/*
@@ -1255,7 +1255,7 @@ uev_add_path (struct uevent *uev, struct vectors * vecs, int need_do_map)
 		conf = get_multipath_config();
 		pp->checkint = conf->checkint;
 		put_multipath_config(conf);
-		ret = ev_add_path(pp, vecs, need_do_map);
+		ret = ev_add_path(pp, vecs, mode);
 	} else {
 		condlog(0, "%s: failed to store path info, "
 			"dropping event",
@@ -1297,8 +1297,7 @@ sysfs_get_ro (struct path *pp)
  * 0: added
  * 1: error
  */
-int
-ev_add_path (struct path * pp, struct vectors * vecs, int need_do_map)
+int ev_add_path(struct path *pp, struct vectors *vecs, enum path_add_mode mode)
 {
 	struct multipath * mpp;
 	char *params __attribute__((cleanup(cleanup_charp))) = NULL;
@@ -1315,6 +1314,22 @@ ev_add_path (struct path * pp, struct vectors * vecs, int need_do_map)
 		condlog(0, "%s: failed to get path uid", pp->dev);
 		goto fail; /* leave path added to pathvec */
 	}
+	/*
+	 * This path is being re-added because its udev data was missing or
+	 * changed. The uevent may have been queued long before we got here,
+	 * and adding a path to the map of a different LUN corrupts data. If
+	 * recheck_wwid is set, confirm the WWID with the device first. On a
+	 * mismatch, leave the path out and let the checker retrigger udev
+	 * to get a fresh WWID.
+	 */
+	if (mode == PATH_ADD_REINIT && pp->recheck_wwid == RECHECK_WWID_ON &&
+	    check_path_wwid_change(pp)) {
+		condlog(0, "%s: not adding path, wwid '%s' not confirmed by device",
+			pp->dev, pp->wwid);
+		memset(pp->wwid, 0, WWID_SIZE);
+		pp->initialized = INIT_MISSING_UDEV;
+		goto fail;
+	}
 	mpp = find_mp_by_wwid(vecs->mpvec, pp->wwid);
 	if (mpp && pp->size && mpp->size != pp->size) {
 		condlog(0, "%s: failed to add new path %s, device size mismatch", mpp->alias, pp->dev);
@@ -1375,7 +1390,7 @@ rescan:
 	if (ro == 1)
 		mpp->force_readonly = 1;
 
-	if (!need_do_map)
+	if (mode == PATH_ADD_NO_MAP)
 		return 0;
 
 	if (!dm_map_present(mpp->alias)) {
@@ -1789,7 +1804,7 @@ out:
 	}
 	/* pp->initialized must not be INIT_PARTIAL if needs_reinit is set */
 	if (needs_reinit)
-		retval = uev_add_path(uev, vecs, 1);
+		retval = uev_add_path(uev, vecs, PATH_ADD_REINIT);
 	return retval;
 }
 
@@ -1905,13 +1920,13 @@ uev_trigger (struct uevent * uev, void * trigger_data)
 	 */
 	list_for_each_entry_safe(merge_uev, tmp, &uev->merge_node, node) {
 		if (!strncmp(merge_uev->action, "add", 3))
-			r += uev_add_path(merge_uev, vecs, 0);
+			r += uev_add_path(merge_uev, vecs, PATH_ADD_NO_MAP);
 		if (!strncmp(merge_uev->action, "remove", 6))
 			r += uev_remove_path(merge_uev, vecs, 0);
 	}
 
 	if (!strncmp(uev->action, "add", 3))
-		r += uev_add_path(uev, vecs, 1);
+		r += uev_add_path(uev, vecs, PATH_ADD_MAP);
 	if (!strncmp(uev->action, "remove", 6))
 		r += uev_remove_path(uev, vecs, 1);
 	if (!strncmp(uev->action, "change", 6))
@@ -3015,7 +3030,7 @@ update_uninitialized_path(struct vectors * vecs, struct path * pp)
 		pthread_cleanup_pop(1);
 		/* INIT_OK implies ret == PATHINFO_OK */
 		if (pp->initialized == INIT_OK) {
-			ev_add_path(pp, vecs, 1);
+			ev_add_path(pp, vecs, PATH_ADD_MAP);
 			pp->tick = 1;
 		} else if (ret == PATHINFO_SKIPPED) {
 			int i;
@@ -3036,7 +3051,7 @@ update_uninitialized_path(struct vectors * vecs, struct path * pp)
 					CHECK_PATH_REMOVED :
 					CHECK_PATH_SKIPPED;
 		}
-		ev_add_path(pp, vecs, 1);
+		ev_add_path(pp, vecs, PATH_ADD_MAP);
 		pp->tick = 1;
 	}
 	return CHECK_PATH_CHECKED;
diff --git a/multipathd/main.h b/multipathd/main.h
index b66208ec..2eaf5183 100644
--- a/multipathd/main.h
+++ b/multipathd/main.h
@@ -36,7 +36,18 @@ enum daemon_status wait_for_state_change_if(enum daemon_status oldstate,
 					    unsigned long ms);
 void schedule_reconfigure(enum force_reload_types requested_type);
 int need_to_delay_reconfig (struct vectors *);
-int ev_add_path (struct path *, struct vectors *, int);
+/*
+ * PATH_ADD_REINIT is PATH_ADD_MAP for a path that is being re-added
+ * because its udev data was missing or changed. If recheck_wwid is
+ * set, its WWID is confirmed with the device before the path is put
+ * into a map.
+ */
+enum path_add_mode {
+	PATH_ADD_NO_MAP,
+	PATH_ADD_MAP,
+	PATH_ADD_REINIT,
+};
+int ev_add_path(struct path *, struct vectors *, enum path_add_mode);
 int ev_remove_path (struct path *, struct vectors *, int);
 int ev_add_map (char *, const char *, struct vectors *);
 int flush_map(struct multipath *, struct vectors *);
-- 
2.56.0

Re: [PATCH v4 1/1] multipathd: recheck WWIDs of re-added paths with the device

[Martin Wilck]

> multipathd takes a path's WWID from udev. When a change uevent reports
> a different WWID, uev_update_path() removes the path from its map and
> re-adds it through uev_add_path(), which places it in whatever map owns
> the new WWID. The removal can block for seconds while the map is
> suspended, so by the time the path is re-added, the WWID in the uevent
> may no longer be what the device reports. If it is wrong, the path goes
> into the map of a different LUN and I/O for one LUN is silently sent to
> another.
> 
> If recheck_wwid is set, confirm the WWID with the device over SG_IO
> before such a re-add puts the path into a map, as recheck_wwid already
> does when a failed path comes back. If the device doesn't confirm it,
> leave the path out of the map: orphan it, clear its WWID and mark it
> INIT_MISSING_UDEV, so the checker retriggers udev and the path is
> re-added from fresh data.
> 
> To tell re-adds from ordinary adds, turn the need_do_map argument of
> uev_add_path() and ev_add_path() into enum path_add_mode, with a new
> PATH_ADD_REINIT value that behaves like PATH_ADD_MAP and is passed only
> for the re-add at the end of uev_update_path(). Ordinary adds are
> unchanged.
> 
> Document the new case under recheck_wwid in multipath.conf(5).

As Ben pointed out in his previous email:


It may make sense to convert the "recheck_wwid" option from a boolean
to a multi-value option, to make it possible to choose between the old
and new behavior.

@Ben, what do you think?

> 
> Signed-off-by: Brian Bunker <brian@purestorage.com>

I still have some more comments, see below.
(In case you wonder: I used AI to assist with this review).

> diff --git a/multipath/multipath.conf.5.in b/multipath/multipath.conf.5.in
> index a577dfc..8f35880 100644
> --- a/multipath/multipath.conf.5.in
> +++ b/multipath/multipath.conf.5.in
> @@ -1382,9 +1382,11 @@ The default is: \fB\(dqNONE\(dq\fR
>  If set to \fIyes\fR, when a failed path is restored, its wwid is rechecked. If
>  the wwid has changed, the path is removed from the current multipath device,
>  and re-added as a new path. Multipathd will also recheck a path's wwid if it is
> -manually re-added. This option only works for SCSI devices that are configured
> -to use the default uid_attribute, \fIID_SERIAL\fR, or sysfs for getting their
> -wwid.
> +manually re-added, and before re-adding a path whose wwid udev reported as
> +changed. If the device doesn't confirm the new wwid, the path is left out of
> +the multipath device and udev is asked to report its wwid again. This option
> +only works for SCSI devices that are configured to use the default
> +uid_attribute, \fIID_SERIAL\fR, or sysfs for getting their wwid.
>  .RS
>  .TP
>  The default is: \fBno\fR
> diff --git a/multipathd/cli_handlers.c b/multipathd/cli_handlers.c
> index f5a13c4..9b4a04c 100644
> --- a/multipathd/cli_handlers.c
> +++ b/multipathd/cli_handlers.c
> @@ -673,7 +673,7 @@ cli_add_path (void *v, struct strbuf *reply, void *data)
>  			return 1;
>  		}
>  	}
> -	return ev_add_path(pp, vecs, 1);
> +	return ev_add_path(pp, vecs, PATH_ADD_MAP);
>  blacklisted:
>  	append_strbuf_str(reply, "blacklisted\n");
>  	condlog(2, "%s: path blacklisted", param);
> diff --git a/multipathd/main.c b/multipathd/main.c
> index 9bce4d6..fdf5503 100644
> --- a/multipathd/main.c
> +++ b/multipathd/main.c
> @@ -1128,7 +1128,7 @@ check_path_wwid_change(struct path *pp)
>  static int uev_update_path (struct uevent *uev, struct vectors * vecs);
>  
>  static int
> -uev_add_path (struct uevent *uev, struct vectors * vecs, int need_do_map)
> +uev_add_path(struct uevent *uev, struct vectors *vecs, enum path_add_mode mode)
>  {
>  	struct path *pp;
>  	int ret = 0, i;
> @@ -1180,7 +1180,7 @@ uev_add_path (struct uevent *uev, struct vectors * vecs, int need_do_map)
>  				     DI_ALL | DI_BLACKLIST);
>  			pthread_cleanup_pop(1);
>  			if (r == PATHINFO_OK && !prev_mpp)
> -				ret = ev_add_path(pp, vecs, need_do_map);
> +				ret = ev_add_path(pp, vecs, mode);
>  			else if (r == PATHINFO_OK &&
>  				 !strncmp(pp->wwid, prev_mpp->wwid, WWID_SIZE)) {
>  				/*
> @@ -1255,7 +1255,7 @@ uev_add_path (struct uevent *uev, struct vectors * vecs, int need_do_map)
>  		conf = get_multipath_config();
>  		pp->checkint = conf->checkint;
>  		put_multipath_config(conf);
> -		ret = ev_add_path(pp, vecs, need_do_map);
> +		ret = ev_add_path(pp, vecs, mode);
>  	} else {
>  		condlog(0, "%s: failed to store path info, "
>  			"dropping event",
> @@ -1297,8 +1297,7 @@ sysfs_get_ro (struct path *pp)
>   * 0: added
>   * 1: error
>   */
> -int
> -ev_add_path (struct path * pp, struct vectors * vecs, int need_do_map)
> +int ev_add_path(struct path *pp, struct vectors *vecs, enum path_add_mode mode)
>  {
>  	struct multipath * mpp;
>  	char *params __attribute__((cleanup(cleanup_charp))) = NULL;
> @@ -1315,6 +1314,22 @@ ev_add_path (struct path * pp, struct vectors * vecs, int need_do_map)
>  		condlog(0, "%s: failed to get path uid", pp->dev);
>  		goto fail; /* leave path added to pathvec */
>  	}
> +	/*
> +	 * This path is being re-added because its udev data was missing or
> +	 * changed. The uevent may have been queued long before we got here,
> +	 * and adding a path to the map of a different LUN corrupts data. If
> +	 * recheck_wwid is set, confirm the WWID with the device first. On a
> +	 * mismatch, leave the path out and let the checker retrigger udev
> +	 * to get a fresh WWID.
> +	 */
> +	if (mode == PATH_ADD_REINIT && pp->recheck_wwid == RECHECK_WWID_ON &&
> +	    check_path_wwid_change(pp)) {

check_path_wwid_change() returns false when it fails, is it correct to keep the
path in that case? IMHO we should treat this like the strlen(pp->wwid) == 0 case
above.

> +		condlog(0, "%s: not adding path, wwid '%s' not confirmed by device",
> +			pp->dev, pp->wwid);

check_path_wwid_change() already logs at verbosity 0.
2 or even 3 should be enough here.

> +		memset(pp->wwid, 0, WWID_SIZE);
> +		pp->initialized = INIT_MISSING_UDEV;
> +		goto fail;

In pathinfo(), we also set pp->tick = conf->retrigger_delay. Is there a reason we
don't do it here?

But more generally: as this indicates a possible mismatch between kernel and device
state, should we handle this similar to  handle_path_wwid_change() instead,
i.e. also call rescan_path()? I guess that would need to be done in uev_update_path(),
i.e. it would touch the regular "wwid changed in uevent" path.

> +	}
>  	mpp = find_mp_by_wwid(vecs->mpvec, pp->wwid);
>  	if (mpp && pp->size && mpp->size != pp->size) {
>  		condlog(0, "%s: failed to add new path %s, device size mismatch", mpp->alias, pp->dev);
> @@ -1375,7 +1390,7 @@ rescan:
>  	if (ro == 1)
>  		mpp->force_readonly = 1;
>  
> -	if (!need_do_map)
> +	if (mode == PATH_ADD_NO_MAP)
>  		return 0;
>  
>  	if (!dm_map_present(mpp->alias)) {
> @@ -1789,7 +1804,7 @@ out:
>  	}
>  	/* pp->initialized must not be INIT_PARTIAL if needs_reinit is set */
>  	if (needs_reinit)
> -		retval = uev_add_path(uev, vecs, 1);
> +		retval = uev_add_path(uev, vecs, PATH_ADD_REINIT);

Is it correct to take this branch also if needs_reinit is set because
INIT_REQUESTED_UDEV was set? I don't think so.

-- 
Martin Wilck <mwilck@suse.com>

Re: [PATCH v4 1/1] multipathd: recheck WWIDs of re-added paths with the device

[Benjamin Marzinski]

On Thu, Nov 12, 2026 at 11:24:32PM +0100, Martin Wilck wrote:
> > multipathd takes a path's WWID from udev. When a change uevent reports
> > a different WWID, uev_update_path() removes the path from its map and
> > re-adds it through uev_add_path(), which places it in whatever map owns
> > the new WWID. The removal can block for seconds while the map is
> > suspended, so by the time the path is re-added, the WWID in the uevent
> > may no longer be what the device reports. If it is wrong, the path goes
> > into the map of a different LUN and I/O for one LUN is silently sent to
> > another.
> > 
> > If recheck_wwid is set, confirm the WWID with the device over SG_IO
> > before such a re-add puts the path into a map, as recheck_wwid already
> > does when a failed path comes back. If the device doesn't confirm it,
> > leave the path out of the map: orphan it, clear its WWID and mark it
> > INIT_MISSING_UDEV, so the checker retriggers udev and the path is
> > re-added from fresh data.
> > 
> > To tell re-adds from ordinary adds, turn the need_do_map argument of
> > uev_add_path() and ev_add_path() into enum path_add_mode, with a new
> > PATH_ADD_REINIT value that behaves like PATH_ADD_MAP and is passed only
> > for the re-add at the end of uev_update_path(). Ordinary adds are
> > unchanged.
> > 
> > Document the new case under recheck_wwid in multipath.conf(5).
> 
> As Ben pointed out in his previous email:
> 
> 
> It may make sense to convert the "recheck_wwid" option from a boolean
> to a multi-value option, to make it possible to choose between the old
> and new behavior.
> 
> @Ben, what do you think?

Like I mentioned in that other email thread, I think limiting this to
only run on paths that are up will make it no more risky than
recheck_wwid already is. recheck_wwid exists because people are worried
that their device my change wwid unexpectedly. We could have multiple
levels of worry, but I'm not sure we need to give users such fine
grained control, and this check only gets tripped when something already
went wrong.

more comments below.

> 
> > 
> > Signed-off-by: Brian Bunker <brian@purestorage.com>
> 
> I still have some more comments, see below.
> (In case you wonder: I used AI to assist with this review).
> 
> > diff --git a/multipath/multipath.conf.5.in b/multipath/multipath.conf.5.in
> > index a577dfc..8f35880 100644
> > --- a/multipath/multipath.conf.5.in
> > +++ b/multipath/multipath.conf.5.in
> > @@ -1382,9 +1382,11 @@ The default is: \fB\(dqNONE\(dq\fR
> >  If set to \fIyes\fR, when a failed path is restored, its wwid is rechecked. If
> >  the wwid has changed, the path is removed from the current multipath device,
> >  and re-added as a new path. Multipathd will also recheck a path's wwid if it is
> > -manually re-added. This option only works for SCSI devices that are configured
> > -to use the default uid_attribute, \fIID_SERIAL\fR, or sysfs for getting their
> > -wwid.
> > +manually re-added, and before re-adding a path whose wwid udev reported as
> > +changed. If the device doesn't confirm the new wwid, the path is left out of
> > +the multipath device and udev is asked to report its wwid again. This option
> > +only works for SCSI devices that are configured to use the default
> > +uid_attribute, \fIID_SERIAL\fR, or sysfs for getting their wwid.
> >  .RS
> >  .TP
> >  The default is: \fBno\fR
> > diff --git a/multipathd/cli_handlers.c b/multipathd/cli_handlers.c
> > index f5a13c4..9b4a04c 100644
> > --- a/multipathd/cli_handlers.c
> > +++ b/multipathd/cli_handlers.c
> > @@ -673,7 +673,7 @@ cli_add_path (void *v, struct strbuf *reply, void *data)
> >  			return 1;
> >  		}
> >  	}
> > -	return ev_add_path(pp, vecs, 1);
> > +	return ev_add_path(pp, vecs, PATH_ADD_MAP);
> >  blacklisted:
> >  	append_strbuf_str(reply, "blacklisted\n");
> >  	condlog(2, "%s: path blacklisted", param);
> > diff --git a/multipathd/main.c b/multipathd/main.c
> > index 9bce4d6..fdf5503 100644
> > --- a/multipathd/main.c
> > +++ b/multipathd/main.c
> > @@ -1128,7 +1128,7 @@ check_path_wwid_change(struct path *pp)
> >  static int uev_update_path (struct uevent *uev, struct vectors * vecs);
> >  
> >  static int
> > -uev_add_path (struct uevent *uev, struct vectors * vecs, int need_do_map)
> > +uev_add_path(struct uevent *uev, struct vectors *vecs, enum path_add_mode mode)
> >  {
> >  	struct path *pp;
> >  	int ret = 0, i;
> > @@ -1180,7 +1180,7 @@ uev_add_path (struct uevent *uev, struct vectors * vecs, int need_do_map)
> >  				     DI_ALL | DI_BLACKLIST);
> >  			pthread_cleanup_pop(1);
> >  			if (r == PATHINFO_OK && !prev_mpp)
> > -				ret = ev_add_path(pp, vecs, need_do_map);
> > +				ret = ev_add_path(pp, vecs, mode);
> >  			else if (r == PATHINFO_OK &&
> >  				 !strncmp(pp->wwid, prev_mpp->wwid, WWID_SIZE)) {
> >  				/*
> > @@ -1255,7 +1255,7 @@ uev_add_path (struct uevent *uev, struct vectors * vecs, int need_do_map)
> >  		conf = get_multipath_config();
> >  		pp->checkint = conf->checkint;
> >  		put_multipath_config(conf);
> > -		ret = ev_add_path(pp, vecs, need_do_map);
> > +		ret = ev_add_path(pp, vecs, mode);
> >  	} else {
> >  		condlog(0, "%s: failed to store path info, "
> >  			"dropping event",
> > @@ -1297,8 +1297,7 @@ sysfs_get_ro (struct path *pp)
> >   * 0: added
> >   * 1: error
> >   */
> > -int
> > -ev_add_path (struct path * pp, struct vectors * vecs, int need_do_map)
> > +int ev_add_path(struct path *pp, struct vectors *vecs, enum path_add_mode mode)
> >  {
> >  	struct multipath * mpp;
> >  	char *params __attribute__((cleanup(cleanup_charp))) = NULL;
> > @@ -1315,6 +1314,22 @@ ev_add_path (struct path * pp, struct vectors * vecs, int need_do_map)
> >  		condlog(0, "%s: failed to get path uid", pp->dev);
> >  		goto fail; /* leave path added to pathvec */
> >  	}
> > +	/*
> > +	 * This path is being re-added because its udev data was missing or
> > +	 * changed. The uevent may have been queued long before we got here,
> > +	 * and adding a path to the map of a different LUN corrupts data. If
> > +	 * recheck_wwid is set, confirm the WWID with the device first. On a
> > +	 * mismatch, leave the path out and let the checker retrigger udev
> > +	 * to get a fresh WWID.
> > +	 */
> > +	if (mode == PATH_ADD_REINIT && pp->recheck_wwid == RECHECK_WWID_ON &&
> > +	    check_path_wwid_change(pp)) {
> 
> check_path_wwid_change() returns false when it fails, is it correct to keep the
> path in that case? IMHO we should treat this like the strlen(pp->wwid) == 0 case
> above.

Not sure I agree with this. It's a paranoia check. If it fails, I don't
have a problem with just trusting the data that was given to us. I'm
willing to be convinced otherwise, however.

> > +		condlog(0, "%s: not adding path, wwid '%s' not confirmed by device",
> > +			pp->dev, pp->wwid);
> 
> check_path_wwid_change() already logs at verbosity 0.
> 2 or even 3 should be enough here.

makes sense.

> > +		memset(pp->wwid, 0, WWID_SIZE);
> > +		pp->initialized = INIT_MISSING_UDEV;
> > +		goto fail;
> 
> In pathinfo(), we also set pp->tick = conf->retrigger_delay. Is there a reason we
> don't do it here?

makes sense.

> But more generally: as this indicates a possible mismatch between kernel and device
> state, should we handle this similar to  handle_path_wwid_change() instead,
> i.e. also call rescan_path()? I guess that would need to be done in uev_update_path(),
> i.e. it would touch the regular "wwid changed in uevent" path.

Is this not another place were we are opening ourselves up for commands
blocking? If we want to do a rescan and it can block, it makes more
sense to do it only if recheck_wwids is set, and we know that the path
is up. 

> > +	}
> >  	mpp = find_mp_by_wwid(vecs->mpvec, pp->wwid);
> >  	if (mpp && pp->size && mpp->size != pp->size) {
> >  		condlog(0, "%s: failed to add new path %s, device size mismatch", mpp->alias, pp->dev);
> > @@ -1375,7 +1390,7 @@ rescan:
> >  	if (ro == 1)
> >  		mpp->force_readonly = 1;
> >  
> > -	if (!need_do_map)
> > +	if (mode == PATH_ADD_NO_MAP)
> >  		return 0;
> >  
> >  	if (!dm_map_present(mpp->alias)) {
> > @@ -1789,7 +1804,7 @@ out:
> >  	}
> >  	/* pp->initialized must not be INIT_PARTIAL if needs_reinit is set */
> >  	if (needs_reinit)
> > -		retval = uev_add_path(uev, vecs, 1);
> > +		retval = uev_add_path(uev, vecs, PATH_ADD_REINIT);
> 
> Is it correct to take this branch also if needs_reinit is set because
> INIT_REQUESTED_UDEV was set? I don't think so.

This is a little tricky. I agree that we don't need to add the extra
layer of paranoid checking when the path never got a wwid. In that case,
what most likely what happened is that the udev worker thread timed out
on the previous uevent, so we couldn't get the WWID from uevent
variables. However, if we don't recheck on INIT_REQUESTED_UDEV, then we
can get into a situation where:
1. uev_update_path() notices that the wwid has changed
2. ev_add_path() confirms that the wwid we got from udev doesn't match
   the wwid we get directly asking the device
3. We trigger a new uevent
4. We blindly trust udev's wwid.

Perhaps we need more initialization states or a path device flag to
distinguish between the two cases (never having gotten a WWID, or having
our WWID . Or we can decide that we are o.k. with extra checking
whenever something goes wrong with getting the wwid from udev (when you
have recheck_wwid set). Or am I misunderstanding something about why
doing this is wrong? Are we intentionally only double-checking once
here?

-Ben

> -- 
> Martin Wilck <mwilck@suse.com>