[PATCH 0/3] New approach at handling changed WWIDs

[PATCH 0/3] New approach at handling changed WWIDs

[Martin Wilck]

Hi Ben, hi Christophe,

after reviewing Ben's last patch set, I've been pondering over the
handling of changed WWIDs, which seems to have become a bit too clever
and complex for my taste, and I came up with this new approach
instead.

TL;DR: instead of treating paths with changed WWIDs as faulty, act
as if the path was removed, and another path was then added and
obtained the device ID of the removed path.

Long version:

What would cause the WWID of an in-use path to change on the fly? Other than
a) gross malfunction of the storage or kernel (which would be almost certain
to corrupt data anyway, whether or not multipathd is taking precautions),
I can think of two scenarios: b) multipathd might have missed both a
"remove" and a subsequent "add" uevent for the device in question; c)
the admin may have played with the udev rules, and solicited change
uevents manually.

In case b), doing what my patch does is obviously the right thing. In case
c), the right thing would be throwing slimy stinking things at the admin,
but as we can't do that, removing and re-adding seems still more reasonable
than pretending the path was faulty. Even in case a), removing the path
from the current map is no worse than failing it.

What remains to be considered is what Ben was dealing with in his latest set,
a permanent or temporary failure to retrieve the WWID, resulting in a 0-length
WWID to be returned. IMO it's actually the best thing about this new approach
that this doesn't need to be special-cased. The path initialization logic
that we already have would take care of it using the INIT_MISSING_UDEV logic.
Either the WWID would be successfully retrieved eventually, in which case the
path would be re-added to the previous map, or (depending on configuration)
added to a new map or left alone. Or the failure is permanent, in which case
multipathd would eventually give up and orphan the path. AFAICS this would
be the "right thing" to do in all these different cases, without any
additional logic.

Note also that if a "reconfigure" was carried out in the presence of
paths with changed WWID, the final outcome would likely be the same that
my patch now achieves without "reconfigure".

I case I've come to the wrong conclusions because I overlooked something
essential, please tell me.

Going one step futher, I've actually come to think differently about the
"fallback logic" for the case that no WWID can be obtained from udev. I
believe now that such fallback logic should _not_ be used. The point is not
to derive _some_ WWID, but _the right one_, and that's udev's job. But udev
can be customized in complex ways that multipathd has no idea about.
In the worst case, we'd receive some WWIDs from udev and some from our
own logic, and combine paths into a map which wouldn't acutally belong
together. Therefore I vote for ripping out the fallback logic altogether
and depend on udev exclusively for WWID generation. I haven't included this
in the current patch set in order not to make it too controversial.

The only purpose for the fallback logic that I could see is to provide
a configuration option to force multipathd to _always_ determine the
WWID by itself, ignoring udev device properties.

Regards
Martin

Martin Wilck (3):
  multipathd: handle changed wwids by removal and addition
  multipathd: remove "wwid_changed" path attribute
  multipathd: ignore "disable_changed_wwids"

 libmultipath/config.c      |  1 -
 libmultipath/config.h      |  1 -
 libmultipath/dict.c        | 18 +++++++--
 libmultipath/structs.h     |  1 -
 multipath/multipath.conf.5 |  8 +---
 multipathd/main.c          | 77 +++++++++++++++++++-------------------
 6 files changed, 55 insertions(+), 51 deletions(-)

-- 
2.21.0

[PATCH 1/3] multipathd: handle changed wwids by removal and addition

[Martin Wilck]

If a path's WWID changes, it's not necessarily failed. But it certainly
has to be removed from an existing map, otherwise data corruption is
imminent. Instead of keeping the path in the map, failing it, and
remembering the "changed WWID" state, this patch simply removes and
re-adds the path.

If the WWID is truly different now, the path will be readded as if
it was a new path. If the WWID is zero because of a (possibly temporary)
getuid() failure, and getuid() fails in uev_add_path() again, it
will be added with INIT_MISSING_UDEV status and thus handled appropriately
by the "missing udev" logic.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 multipathd/main.c | 71 +++++++++++++++++++++++++----------------------
 1 file changed, 38 insertions(+), 33 deletions(-)

diff --git a/multipathd/main.c b/multipathd/main.c
index fb520b64..bd26209d 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -1184,13 +1184,22 @@ fail:
 	return 1;
 }
 
+static bool is_wwid_changed(const struct path *pp, const char *old_wwid)
+{
+	bool wwid_changed = (strncmp(old_wwid, pp->wwid, WWID_SIZE) != 0);
+
+	if (wwid_changed)
+		condlog(1, "%s: WWID changed from \"%s\" to \"%s\"",
+			pp->dev, old_wwid, pp->wwid);
+	return wwid_changed;
+}
+
 static int
 uev_update_path (struct uevent *uev, struct vectors * vecs)
 {
 	int ro, retval = 0, rc;
 	struct path * pp;
 	struct config *conf;
-	int disable_changed_wwids;
 	int needs_reinit = 0;
 
 	switch ((rc = change_foreign(uev->udev))) {
@@ -1208,12 +1217,6 @@ uev_update_path (struct uevent *uev, struct vectors * vecs)
 		break;
 	}
 
-	conf = get_multipath_config();
-	disable_changed_wwids = conf->disable_changed_wwids;
-	put_multipath_config(conf);
-
-	ro = uevent_get_disk_ro(uev);
-
 	pthread_cleanup_push(cleanup_lock, &vecs->lock);
 	lock(&vecs->lock);
 	pthread_testcancel();
@@ -1221,7 +1224,7 @@ uev_update_path (struct uevent *uev, struct vectors * vecs)
 	pp = find_path_by_dev(vecs->pathvec, uev->kernel);
 	if (pp) {
 		struct multipath *mpp = pp->mpp;
-		char wwid[WWID_SIZE];
+		bool wwid_changed;
 
 		if (pp->initialized == INIT_REQUESTED_UDEV) {
 			needs_reinit = 1;
@@ -1232,36 +1235,38 @@ uev_update_path (struct uevent *uev, struct vectors * vecs)
 		if (!strlen(pp->wwid))
 			goto out;
 
-		strcpy(wwid, pp->wwid);
-		get_uid(pp, pp->state, uev->udev);
-
-		if (strncmp(wwid, pp->wwid, WWID_SIZE) != 0) {
-			condlog(0, "%s: path wwid changed from '%s' to '%s'. %s",
-				uev->kernel, wwid, pp->wwid,
-				(disable_changed_wwids ? "disallowing" :
-				 "continuing"));
-			strcpy(pp->wwid, wwid);
-			if (disable_changed_wwids) {
-				if (!pp->wwid_changed) {
-					pp->wwid_changed = 1;
-					pp->tick = 1;
-					if (pp->mpp)
-						dm_fail_path(pp->mpp->alias, pp->dev_t);
-				}
-				goto out;
-			}
+		if (pp->mpp) {
+			get_uid(pp, pp->state, uev->udev);
+			wwid_changed = is_wwid_changed(pp, pp->mpp->wwid);
 		} else {
-			pp->wwid_changed = 0;
+			char *old_wwid = strdup(pp->wwid);
+
+			if (!old_wwid)
+				goto out;
+			pthread_cleanup_push(free, (void*)old_wwid);
+			wwid_changed = is_wwid_changed(pp, old_wwid);
+			pthread_cleanup_pop(1);
+		}
+
+		if (wwid_changed) {
+			ev_remove_path(pp, vecs, 1);
+			needs_reinit = 1;
+			goto out;
+		}
+
+		if (pp->udev != uev->udev) {
 			udev_device_unref(pp->udev);
 			pp->udev = udev_device_ref(uev->udev);
-			conf = get_multipath_config();
-			pthread_cleanup_push(put_multipath_config, conf);
-			if (pathinfo(pp, conf, DI_SYSFS|DI_NOIO) != PATHINFO_OK)
-				condlog(1, "%s: pathinfo failed after change uevent",
-					uev->kernel);
-			pthread_cleanup_pop(1);
 		}
 
+		conf = get_multipath_config();
+		pthread_cleanup_push(put_multipath_config, conf);
+		if (pathinfo(pp, conf, DI_SYSFS|DI_NOIO) != PATHINFO_OK)
+			condlog(1, "%s: pathinfo failed after change uevent",
+				uev->kernel);
+		pthread_cleanup_pop(1);
+
+		ro = uevent_get_disk_ro(uev);
 		if (mpp && ro >= 0) {
 			condlog(2, "%s: update path write_protect to '%d' (uevent)", uev->kernel, ro);
 
-- 
2.21.0

[PATCH 2/3] multipathd: remove "wwid_changed" path attribute

[Martin Wilck]

This is now not needed any more.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/structs.h | 1 -
 multipathd/main.c      | 6 ------
 2 files changed, 7 deletions(-)

diff --git a/libmultipath/structs.h b/libmultipath/structs.h
index b794b0dc..7879d763 100644
--- a/libmultipath/structs.h
+++ b/libmultipath/structs.h
@@ -280,7 +280,6 @@ struct path {
 	int fd;
 	int initialized;
 	int retriggers;
-	int wwid_changed;
 	unsigned int path_failures;
 	time_t dis_reinstate_time;
 	int disable_reinstate;
diff --git a/multipathd/main.c b/multipathd/main.c
index bd26209d..7119894c 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -2016,12 +2016,6 @@ check_path (struct vectors * vecs, struct path * pp, int ticks)
 	if (newstate == PATH_REMOVED)
 		newstate = PATH_DOWN;
 
-	if (pp->wwid_changed) {
-		condlog(2, "%s: path wwid has changed. Refusing to use",
-			pp->dev);
-		newstate = PATH_DOWN;
-	}
-
 	if (newstate == PATH_WILD || newstate == PATH_UNCHECKED) {
 		condlog(2, "%s: unusable path (%s) - checker failed",
 			pp->dev, checker_state_name(newstate));
-- 
2.21.0

[PATCH 3/3] multipathd: ignore "disable_changed_wwids"

[Martin Wilck]

This option has no effect any more.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/config.c      |  1 -
 libmultipath/config.h      |  1 -
 libmultipath/dict.c        | 18 +++++++++++++++---
 multipath/multipath.conf.5 |  8 ++------
 4 files changed, 17 insertions(+), 11 deletions(-)

diff --git a/libmultipath/config.c b/libmultipath/config.c
index 24d71aed..141f092b 100644
--- a/libmultipath/config.c
+++ b/libmultipath/config.c
@@ -715,7 +715,6 @@ load_config (char * file)
 	conf->retrigger_tries = DEFAULT_RETRIGGER_TRIES;
 	conf->retrigger_delay = DEFAULT_RETRIGGER_DELAY;
 	conf->uev_wait_timeout = DEFAULT_UEV_WAIT_TIMEOUT;
-	conf->disable_changed_wwids = DEFAULT_DISABLE_CHANGED_WWIDS;
 	conf->remove_retries = 0;
 	conf->ghost_delay = DEFAULT_GHOST_DELAY;
 	conf->all_tg_pt = DEFAULT_ALL_TG_PT;
diff --git a/libmultipath/config.h b/libmultipath/config.h
index b938c26c..f5bf5b1b 100644
--- a/libmultipath/config.h
+++ b/libmultipath/config.h
@@ -182,7 +182,6 @@ struct config {
 	int delayed_reconfig;
 	int uev_wait_timeout;
 	int skip_kpartx;
-	int disable_changed_wwids;
 	int remove_retries;
 	int max_sectors_kb;
 	int ghost_delay;
diff --git a/libmultipath/dict.c b/libmultipath/dict.c
index eaad4f18..96815f8a 100644
--- a/libmultipath/dict.c
+++ b/libmultipath/dict.c
@@ -156,6 +156,12 @@ out:
 	return len;
 }
 
+static int
+print_ignored (char *buff, int len)
+{
+	return snprintf(buff, len, "ignored");
+}
+
 static int
 print_yes_no (char *buff, int len, long v)
 {
@@ -548,9 +554,15 @@ declare_hw_handler(skip_kpartx, set_yes_no_undef)
 declare_hw_snprint(skip_kpartx, print_yes_no_undef)
 declare_mp_handler(skip_kpartx, set_yes_no_undef)
 declare_mp_snprint(skip_kpartx, print_yes_no_undef)
-
-declare_def_handler(disable_changed_wwids, set_yes_no)
-declare_def_snprint(disable_changed_wwids, print_yes_no)
+static int def_disable_changed_wwids_handler(struct config *conf, vector strvec)
+{
+	return 0;
+}
+static int snprint_def_disable_changed_wwids(struct config *conf, char *buff,
+					     int len, const void *data)
+{
+	return print_ignored(buff, len);
+}
 
 declare_def_handler(remove_retries, set_int)
 declare_def_snprint(remove_retries, print_int)
diff --git a/multipath/multipath.conf.5 b/multipath/multipath.conf.5
index 0fe8461d..a2e3e866 100644
--- a/multipath/multipath.conf.5
+++ b/multipath/multipath.conf.5
@@ -1148,12 +1148,8 @@ The default is: \fBno\fR
 .
 .TP
 .B disable_changed_wwids
-If set to \fIyes\fR, multipathd will check the path wwid on change events, and
-if it has changed from the wwid of the multipath device, multipathd will
-disable access to the path until the wwid changes back.
-.RS
-.TP
-The default is: \fBno\fR
+This option is deprecated and ignored. If the WWID of a path suddenly changes,
+multipathd handles it as if it was removed and then added again.
 .RE
 .
 .
-- 
2.21.0

Re: [PATCH 0/3] New approach at handling changed WWIDs

[Benjamin Marzinski]

On Mon, Mar 18, 2019 at 01:12:32PM +0100, Martin Wilck wrote:
> Hi Ben, hi Christophe,
> 
> after reviewing Ben's last patch set, I've been pondering over the
> handling of changed WWIDs, which seems to have become a bit too clever
> and complex for my taste, and I came up with this new approach
> instead.
> 
> TL;DR: instead of treating paths with changed WWIDs as faulty, act
> as if the path was removed, and another path was then added and
> obtained the device ID of the removed path.
> 
> Long version:

Mine is even longer. Sorry :(

> What would cause the WWID of an in-use path to change on the fly? Other than
> a) gross malfunction of the storage or kernel (which would be almost certain
> to corrupt data anyway, whether or not multipathd is taking precautions),
> I can think of two scenarios: b) multipathd might have missed both a
> "remove" and a subsequent "add" uevent for the device in question; c)
> the admin may have played with the udev rules, and solicited change
> uevents manually.

The issue that orginally caused me to check for a changed wwid was a
user remapping in-use storage on the array, so an existing device no
longer mapped to the same underlying disks. That generally fits under c)
"admin did something stupid". 

I don't believe that b) is something we actually need to worry about.
In that case, multipathd will still have the old fd for the path device,
which will remain unusable, so the path will never get re-enabled.
Also, the kernel should never reuse a device name, if things still have
the old device name open.

So, as far as I know, the only way that the wwid could get legitimately
remapped, barring something like a), which there is no point in trying
to fix, is because of user error (or at least some other program's
error). 

While I agree that a path's wwid changing to a different non-null wwid
is always bad, and removing and re-adding the path seems perfectly
sensible for that scenario, I've can think of two other case where the
wwid can change to null.  d) a change event happens while the path is
down.  e) there is a uevent storm and udev times out while handling a
change event.

d) and e) seem to me like by far the most likely scenarios, so doing
something suboptimal for them, in order to optimize the very unlikely
scenarios seems wrong.

Have you ever seen this happen where it was multipaths fault?  I
have never seen this happen in the real world where I didn't also see
a kernel message like this in the logs:

kernel: sd 10:0:0:0: Warning! Received an indication that the LUN assignments on this target have changed. The Linux SCSI layer does not automatically remap LUN assignments.
 
This is a pretty clear sign that something bad has happened to the
storage. And it as always been that someone or something has
reconfigured in-use storage in a dangerous way.

> In case b), doing what my patch does is obviously the right thing. In case
> c), the right thing would be throwing slimy stinking things at the admin,
> but as we can't do that, removing and re-adding seems still more reasonable
> than pretending the path was faulty. Even in case a), removing the path
> from the current map is no worse than failing it.
> 
> What remains to be considered is what Ben was dealing with in his latest set,
> a permanent or temporary failure to retrieve the WWID, resulting in a 0-length
> WWID to be returned. IMO it's actually the best thing about this new approach
> that this doesn't need to be special-cased. The path initialization logic
> that we already have would take care of it using the INIT_MISSING_UDEV logic.
> Either the WWID would be successfully retrieved eventually, in which case the
> path would be re-added to the previous map, or (depending on configuration)
> added to a new map or left alone. Or the failure is permanent, in which case
> multipathd would eventually give up and orphan the path. AFAICS this would
> be the "right thing" to do in all these different cases, without any
> additional logic.

This is where I don't fully agree. It means that we can be stuck waiting
for a uevent to occur when the device has come back up, instead of just
restoring it.  I would really like to keep as little code as possible
that needs to get run before we restore a working path, because that
could be, for instance, the last path to your root filesystem, and you
can't touch any non-cached files on it, until you restore that path.

Even aside from the extreme case, multipathd is doing a lot of extra
work, and quite possibly adding a lot of confusing log messages, simply
because of an unfortunately timed change event.
 
> Note also that if a "reconfigure" was carried out in the presence of
> paths with changed WWID, the final outcome would likely be the same that
> my patch now achieves without "reconfigure".

Yeah. I just checked, and this is very broken, and something needs to be
done to fix it.  If a device gets a change event while it's down, it
will no longer have the udev properties necessary to not be blacklisted,
so the device gets blacklisted, and ignored.  Even after it comes back
up and gest another change event to restore these values, multipathd
still ignores it, because the device was blacklisted during its add
event.

However, if you just set the wwid of a path to NULL and run reconfigure,
it will stay added to the map, and get it's wwid restored to the
multipath device's wwid, because configure() calls map_discovery(),
which ends up calling disassemble_map(). This will set a NULL path wwid
to the multipath device wwid, since disassemble_map() assumes that NULL
wwids just mean that multipathd couldn't get the wwid temporarily.

> I case I've come to the wrong conclusions because I overlooked something
> essential, please tell me.
> 
> Going one step futher, I've actually come to think differently about the
> "fallback logic" for the case that no WWID can be obtained from udev. I
> believe now that such fallback logic should _not_ be used. The point is not
> to derive _some_ WWID, but _the right one_, and that's udev's job. But udev
> can be customized in complex ways that multipathd has no idea about.
> In the worst case, we'd receive some WWIDs from udev and some from our
> own logic, and combine paths into a map which wouldn't acutally belong
> together. Therefore I vote for ripping out the fallback logic altogether
> and depend on udev exclusively for WWID generation. I haven't included this
> in the current patch set in order not to make it too controversial.
>
> The only purpose for the fallback logic that I could see is to provide
> a configuration option to force multipathd to _always_ determine the
> WWID by itself, ignoring udev device properties.

I understand your concern.  There are currently safeguards (obviously
not foolproof ones). The fallback wwid has to match the existing wwid,
otherwise the path gets disabled. It seems very unlikely that a path
configured to use the default uid_attribute, which switched to the
fallback method and still got the same wwid, should actually have a
different udev wwid than the one it previously had and that the fallback
method still has.  Also, once a path starts using the fallback method,
it clears uid_attribute, so that it keeps using the fallback method.

But to get back to dealing with changed wwids, I agree with removing an
re-adding a device if it's wwid has changed. So, leaving that aside, the
sticky question is "what to do with a path that has a NULL wwid?"

There are a number of options:

1. Ignore the NULL wwid. This is what my patch 07 does, and I'd like to
make the case for this yet again. I feel pretty sure that a NULL wwid on
a change event is almost always going to occur because that change event
happened when the path was down. If that's not the reason, then I
believe it is either your reasons a) or c) from above. a) is not
fixable. c) is not really our problem. It's a service to try saving the
sysadmin from their own mistakes. But there is no way to make it
foolproof. It is always possible for the device to get remapped and
start getting IO before multipathd ever gets the uevent. The best we can
guarantee is that we will eventually detect when this happens, report
it, and stop it from doing more damage.  Given this, I think it's
perfectly reasonable to just let NULL wwids slide, because that's
ususally the right thing to do, and everything else makes the common
case work worse, while still not providing a guarantee that the worst
case is avoided. This is still my preferred solution.

2. Fail the path on a NULL wwid. In the checker loop, when the path is
up again, re-attempt to get the wwid.  However, since the udev
information we have won't contain the new wwid, use the fallback
information. This is my patch 12, and I agree that there could be issues
with the fallback wwid being determined differently, although like I
mentioned above, multipathd tries to deal with them.

3. Fail the path on a NULL wwid. In the checker loop, when the path is
up again, trigger a change uevent to get the new information. This
delays restoring a path once it's up, and increases the number of things
outside of multipathd's control that could go wrong which would keep the
path from getting restored (udev timeouts, inability for udev to
complete, because access to a necessary device is down).

4. Some sort of hybrid of 2 and 3, where devices can either get their
wwid from udev or directly from the device, and depending on how they
get their wwid, by necessity they do different things. This is workable
but a lot of code for something that is just a best-effort attempt to
protect sysadmins from themselves.

4. Completely remove the path, and try to re-add it.  This method will
likely cause at least as much delay as 3, with an even larger chance
that something goes wrong and keeps access from being restored. That's
completely fine if the wwid has changed to something else, but I really
don't like having to do all that work just because a change event
occurred on a device that was down.

Thoughts?

-Ben
 
> Regards
> Martin
> 
> Martin Wilck (3):
>   multipathd: handle changed wwids by removal and addition
>   multipathd: remove "wwid_changed" path attribute
>   multipathd: ignore "disable_changed_wwids"
> 
>  libmultipath/config.c      |  1 -
>  libmultipath/config.h      |  1 -
>  libmultipath/dict.c        | 18 +++++++--
>  libmultipath/structs.h     |  1 -
>  multipath/multipath.conf.5 |  8 +---
>  multipathd/main.c          | 77 +++++++++++++++++++-------------------
>  6 files changed, 55 insertions(+), 51 deletions(-)
> 
> -- 
> 2.21.0

Re: [PATCH 0/3] New approach at handling changed WWIDs

[Martin Wilck]

On Tue, 2019-03-19 at 12:11 -0500, Benjamin Marzinski wrote:
> On Mon, Mar 18, 2019 at 01:12:32PM +0100, Martin Wilck wrote:
> > Hi Ben, hi Christophe,
> > 
> > after reviewing Ben's last patch set, I've been pondering over the
> > handling of changed WWIDs, which seems to have become a bit too
> > clever
> > and complex for my taste, and I came up with this new approach
> > instead.
> > 
> > TL;DR: instead of treating paths with changed WWIDs as faulty, act
> > as if the path was removed, and another path was then added and
> > obtained the device ID of the removed path.
> > 
> > Long version:
> 
> Mine is even longer. Sorry :(

NP, but you'll forgive me for shortening it a a bit for my reply :-)

> While I agree that a path's wwid changing to a different non-null
> wwid
> is always bad, and removing and re-adding the path seems perfectly
> sensible for that scenario, I've can think of two other case where
> the
> wwid can change to null.  d) a change event happens while the path is
> down.  e) there is a uevent storm and udev times out while handling a
> change event.
> 
> d) and e) seem to me like by far the most likely scenarios, so doing
> something suboptimal for them, in order to optimize the very unlikely
> scenarios seems wrong.

You've worked on this far more than I did, so I trust your judgement.

One remark: With the kernel exporting the VPD pages in sysfs
and my late "multipath -u" patch series, udev should be able to process
uevents for path devices without a single actual device IO. (*) 
While there are still possible causes for udev processing to get stuck,
this would make it much less likely.

> > What remains to be considered is what Ben was dealing with in his
> > latest set,
> > a permanent or temporary failure to retrieve the WWID, resulting in
> > a 0-length
> > WWID to be returned. IMO it's actually the best thing about this
> > new approach
> > that this doesn't need to be special-cased. The path initialization
> > logic
> > that we already have would take care of it using the
> > INIT_MISSING_UDEV logic.
> > Either the WWID would be successfully retrieved eventually, in
> > which case the
> > path would be re-added to the previous map, or (depending on
> > configuration)
> > added to a new map or left alone. Or the failure is permanent, in
> > which case
> > multipathd would eventually give up and orphan the path. AFAICS
> > this would
> > be the "right thing" to do in all these different cases, without
> > any
> > additional logic.
> 
> This is where I don't fully agree. It means that we can be stuck
> waiting
> for a uevent to occur when the device has come back up, instead of
> just
> restoring it.  I would really like to keep as little code as possible
> that needs to get run before we restore a working path, because that
> could be, for instance, the last path to your root filesystem, and
> you
> can't touch any non-cached files on it, until you restore that path.
> 
> Even aside from the extreme case, multipathd is doing a lot of extra
> work, and quite possibly adding a lot of confusing log messages,
> simply
> because of an unfortunately timed change event.
>  
> > Note also that if a "reconfigure" was carried out in the presence
> > of
> > paths with changed WWID, the final outcome would likely be the same
> > that
> > my patch now achieves without "reconfigure".
> 
> Yeah. I just checked, and this is very broken, and something needs to
> be
> done to fix it.  If a device gets a change event while it's down, it
> will no longer have the udev properties necessary to not be
> blacklisted,
> so the device gets blacklisted, and ignored.  Even after it comes
> back
> up and gest another change event to restore these values, multipathd
> still ignores it, because the device was blacklisted during its add
> event.

I wasn't aware of that.

> However, if you just set the wwid of a path to NULL and run
> reconfigure,
> it will stay added to the map, and get it's wwid restored to the
> multipath device's wwid, because configure() calls map_discovery(),
> which ends up calling disassemble_map(). This will set a NULL path
> wwid
> to the multipath device wwid, since disassemble_map() assumes that
> NULL
> wwids just mean that multipathd couldn't get the wwid temporarily.

My thinking was that reconfigure calls path_discovery(), which would
call pathinfo() on all available paths again. I understand now - if
there was an error processing the last uevent for a device, the
information in the udev DB will still be incomplete, and
path_discovery() will obtain the same broken information, possibly
blacklisting the device. I thought the WWID might then be restored by
the INIT_MISSING_UDEV logic, but disassemble_map() would almost
certainly come first. I'm not sure I like it that disassemble_map()
tries to fill in path information. It's ok to do this for paths
that haven't been detected at all (pp == NULL case) but in case of
paths that are present in the udev db, I'm uncertain.

This would also affect your treatment of "WWID changed to zero", no?
Any call to setup_multipath() or update_multipath() would call
disassemble_map(), which would possibly mess with the path's wwid...


> 
> > I case I've come to the wrong conclusions because I overlooked
> > something
> > essential, please tell me.
> > 
> > Going one step futher, I've actually come to think differently
> > about the
> > "fallback logic" for the case that no WWID can be obtained from
> > udev. I
> > believe now that such fallback logic should _not_ be used. The
> > point is not
> > to derive _some_ WWID, but _the right one_, and that's udev's job.
> > But udev
> > can be customized in complex ways that multipathd has no idea
> > about.
> > In the worst case, we'd receive some WWIDs from udev and some from
> > our
> > own logic, and combine paths into a map which wouldn't acutally
> > belong
> > together. Therefore I vote for ripping out the fallback logic
> > altogether
> > and depend on udev exclusively for WWID generation. I haven't
> > included this
> > in the current patch set in order not to make it too controversial.
> > 
> > The only purpose for the fallback logic that I could see is to
> > provide
> > a configuration option to force multipathd to _always_ determine
> > the
> > WWID by itself, ignoring udev device properties.
> 
> I understand your concern.  There are currently safeguards (obviously
> not foolproof ones). The fallback wwid has to match the existing
> wwid,
> otherwise the path gets disabled. It seems very unlikely that a path
> configured to use the default uid_attribute, which switched to the
> fallback method and still got the same wwid, should actually have a
> different udev wwid than the one it previously had and that the
> fallback
> method still has.  Also, once a path starts using the fallback
> method,
> it clears uid_attribute, so that it keeps using the fallback method.
> 
> But to get back to dealing with changed wwids, I agree with removing
> an
> re-adding a device if it's wwid has changed. So, leaving that aside,
> the
> sticky question is "what to do with a path that has a NULL wwid?"
> 
> There are a number of options:
> 
> 1. Ignore the NULL wwid. This is what my patch 07 does, and I'd like
> to
> make the case for this yet again. 

You'd reverted this change in your patch 12, which confused me quite a
bit. What is your preference, really?

> I feel pretty sure that a NULL wwid on
> a change event is almost always going to occur because that change
> event
> happened when the path was down. If that's not the reason, then I
> believe it is either your reasons a) or c) from above. a) is not
> fixable. c) is not really our problem. It's a service to try saving
> the
> sysadmin from their own mistakes. But there is no way to make it
> foolproof. It is always possible for the device to get remapped and
> start getting IO before multipathd ever gets the uevent. The best we
> can
> guarantee is that we will eventually detect when this happens, report
> it, and stop it from doing more damage.  Given this, I think it's
> perfectly reasonable to just let NULL wwids slide, because that's
> ususally the right thing to do, and everything else makes the common
> case work worse, while still not providing a guarantee that the worst
> case is avoided. This is still my preferred solution.
> 
> 2. Fail the path on a NULL wwid. In the checker loop, when the path
> is
> up again, re-attempt to get the wwid.  However, since the udev
> information we have won't contain the new wwid, use the fallback
> information. This is my patch 12, and I agree that there could be
> issues
> with the fallback wwid being determined differently, although like I
> mentioned above, multipathd tries to deal with them.
> 
> 3. Fail the path on a NULL wwid. In the checker loop, when the path
> is
> up again, trigger a change uevent to get the new information. This
> delays restoring a path once it's up, and increases the number of
> things
> outside of multipathd's control that could go wrong which would keep
> the
> path from getting restored (udev timeouts, inability for udev to
> complete, because access to a necessary device is down).
> 
> 4. Some sort of hybrid of 2 and 3, where devices can either get their
> wwid from udev or directly from the device, and depending on how they
> get their wwid, by necessity they do different things. This is
> workable
> but a lot of code for something that is just a best-effort attempt to
> protect sysadmins from themselves.

You have argued well that 2. is best for re-acquiring a reasonable WWID
quickly. Anyway, I don't think it's healthy if the udev information is
wrong for a device, and remains wrong. Thus, IMO, another change event
should be triggered anyway, hoping to restore a state where udev
properties correctly identify the device. If the broken ID is really
just the result of bad timing as you suggest, that should likely be
successful. Some care may be needed to make sure that we don't confuse
ourselves with that self-generated uevent.

> 4. Completely remove the path, and try to re-add it.  This method
> will
> likely cause at least as much delay as 3, with an even larger chance
> that something goes wrong and keeps access from being restored.
> That's
> completely fine if the wwid has changed to something else, but I
> really
> don't like having to do all that work just because a change event
> occurred on a device that was down.
> 
> Thoughts?

Let's go for the removal and re-addition for the non-NULL changed WWID
case, and use the quick path for the NULL-WWID case (maybe we could
even try the "fallback" immediately in uev_update_path()?).

You have a more complete understanding of the whole issue than me, so I
trust that you'll come up with a v3 series addressing all these issues.

Regards
Martin

(*) This works only if the tool used to retrieve the VPDs support
reading from sysfs, of course, as sg_inq does (scsi_id does not,
currently).

-- 
Dr. Martin Wilck <mwilck@suse.com>, Tel. +49 (0)911 74053 2107
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)


--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

Re: [PATCH 0/3] New approach at handling changed WWIDs

[Martin Wilck]

On Tue, 2019-03-19 at 19:44 +0100, Martin Wilck wrote:
> On Tue, 2019-03-19 at 12:11 -0500, Benjamin Marzinski wrote:
> > On Mon, Mar 18, 2019 at 01:12:32PM +0100, Martin Wilck wrote:
> > > 
> > > Note also that if a "reconfigure" was carried out in the presence
> > > of
> > > paths with changed WWID, the final outcome would likely be the
> > > same
> > > that
> > > my patch now achieves without "reconfigure".
> > 
> > Yeah. I just checked, and this is very broken, and something needs
> > to
> > be
> > done to fix it.  If a device gets a change event while it's down,
> > it
> > will no longer have the udev properties necessary to not be
> > blacklisted,
> > so the device gets blacklisted, and ignored.  Even after it comes
> > back
> > up and gest another change event to restore these values,
> > multipathd
> > still ignores it, because the device was blacklisted during its add
> > event.
> 
> I wasn't aware of that.

We have a general problem in multipath-tools here. Our method of
blacklisting devices that don't have whitelisted udev properties
doesn't go together with the notion that udev may fail to set the
properties correctly, and the notion that paths shouldn't be removed or
failed (let alone blacklisted) without good reason.

Either we find a way to distinguish "devices that have incomplete udev
information because of temporary failure" and "devices that are missing
required udev properties permanently", or we must say goodbye to the
special treatment of blacklisting by property.

One obvious thing to do before blacklisting a path is to retry when we
encounter devices with missing properties. We can also check the
fallback UID methods - if they are successful and udev fails
repeatedly, the admin likely has messed up the udev rules.

Ben's approach to ignore WWIDs "changed to 0" at least temporarily
makes a lot of sense in this context. Paths that once used to have a
good WWID should be given up only after a reasonable number of retries.
Paths for which we'd never seen a valid WWID are treated by the
INIT_MISSING_UDEV logic.

Whatever we do, we should stop trying to "fix" the path WWID in
disassemble_map(). That's *so* against the separation of concerns
principle. In getuid(), we might check if a path with missing WWID is
already part of an existing multipath map, and then set the path WWID
from the map WWID as sort-of a last emergency fallback. But that, too,
should only be done during startup (assuming that a previous multipath
or multipathd instance had set up the map correctly, and that udev
information had been "lost" since then), and only after retrying as
described above.

Note that since by-property blacklisting was introduced in 2013,
significant progress has been made in other areas. We have blacklisting
by transport now, "find_multipaths", the "failed_wwids" logic that
avoids repeated attempts at setting up maps for busy devices, and the
INIT_MISSING_UDEV logic to deal with incomplete initialization. The
udev rules have been improved as well. So, doing away with "required
udev properties" may not be so dangerous, after all. 

Thoughts?
Martin

-- 
Dr. Martin Wilck <mwilck@suse.com>, Tel. +49 (0)911 74053 2107
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)


--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

Re: [PATCH 0/3] New approach at handling changed WWIDs

[Benjamin Marzinski]

On Wed, Mar 20, 2019 at 09:37:35AM +0100, Martin Wilck wrote:
> On Tue, 2019-03-19 at 19:44 +0100, Martin Wilck wrote:
> > On Tue, 2019-03-19 at 12:11 -0500, Benjamin Marzinski wrote:
> > > On Mon, Mar 18, 2019 at 01:12:32PM +0100, Martin Wilck wrote:
> > > > 
> > > > Note also that if a "reconfigure" was carried out in the presence
> > > > of
> > > > paths with changed WWID, the final outcome would likely be the
> > > > same
> > > > that
> > > > my patch now achieves without "reconfigure".
> > > 
> > > Yeah. I just checked, and this is very broken, and something needs
> > > to
> > > be
> > > done to fix it.  If a device gets a change event while it's down,
> > > it
> > > will no longer have the udev properties necessary to not be
> > > blacklisted,
> > > so the device gets blacklisted, and ignored.  Even after it comes
> > > back
> > > up and gest another change event to restore these values,
> > > multipathd
> > > still ignores it, because the device was blacklisted during its add
> > > event.
> > 
> > I wasn't aware of that.
> 
> We have a general problem in multipath-tools here. Our method of
> blacklisting devices that don't have whitelisted udev properties
> doesn't go together with the notion that udev may fail to set the
> properties correctly, and the notion that paths shouldn't be removed or
> failed (let alone blacklisted) without good reason.
> 
> Either we find a way to distinguish "devices that have incomplete udev
> information because of temporary failure" and "devices that are missing
> required udev properties permanently", or we must say goodbye to the
> special treatment of blacklisting by property.
> 
> One obvious thing to do before blacklisting a path is to retry when we
> encounter devices with missing properties. We can also check the
> fallback UID methods - if they are successful and udev fails
> repeatedly, the admin likely has messed up the udev rules.
> 
> Ben's approach to ignore WWIDs "changed to 0" at least temporarily
> makes a lot of sense in this context. Paths that once used to have a
> good WWID should be given up only after a reasonable number of retries.
> Paths for which we'd never seen a valid WWID are treated by the
> INIT_MISSING_UDEV logic.

ideally, we would be able to determine whether or not udev was able to
get all the necessary information. It would be nice to be notified if
scsi_id failed or udev timed out.
 
> Whatever we do, we should stop trying to "fix" the path WWID in
> disassemble_map(). That's *so* against the separation of concerns
> principle. In getuid(), we might check if a path with missing WWID is
> already part of an existing multipath map, and then set the path WWID
> from the map WWID as sort-of a last emergency fallback. But that, too,
> should only be done during startup (assuming that a previous multipath
> or multipathd instance had set up the map correctly, and that udev
> information had been "lost" since then), and only after retrying as
> described above.

We don't want to remove paths from multipath devices because multipathd
started up when the path was missing udev information. The udev
properties are trickier, but if we simply have a null WWID, it makes
sense to allow it as a last resort if the device otherwise appears to
have the same paths as it previously did. users can always run

# multipath -f

to remove the device. If it looks like some of the paths are supposed to
change on the device, we should quite possibly not include paths with a
null WWID, because we don't know what has changed.  But we can do this
someplace else than in disassemble_map(). 

> Note that since by-property blacklisting was introduced in 2013,
> significant progress has been made in other areas. We have blacklisting
> by transport now, "find_multipaths", the "failed_wwids" logic that
> avoids repeated attempts at setting up maps for busy devices, and the
> INIT_MISSING_UDEV logic to deal with incomplete initialization. The
> udev rules have been improved as well. So, doing away with "required
> udev properties" may not be so dangerous, after all. 
> 
> Thoughts?

Another option would be to do some extra work in reconfigure.  If we
held on to the old path, and cleaned up everything but the old udev
device and file descriptor, we could be sure that the kernel wouldn't
reuse that device major:minor while we were reconfiguring. If we got
some paths without their udev information, we would have the old udev
information to check against the new config, to see if the device should
be removed. Again, this works best if we could determine if we were
missing udev information.  Although in this case we could probably just
use any path that became blacklisted because of not having the necessary
property information.

-Ben

> Martin
> 
> -- 
> Dr. Martin Wilck <mwilck@suse.com>, Tel. +49 (0)911 74053 2107
> SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton
> HRB 21284 (AG Nürnberg)
> 

Re: [PATCH 0/3] New approach at handling changed WWIDs

[Martin Wilck]

On Thu, 2019-03-21 at 17:31 -0500, Benjamin Marzinski wrote:
> 
> ideally, we would be able to determine whether or not udev was able
> to
> get all the necessary information. It would be nice to be notified if
> scsi_id failed or udev timed out.

The latter (udev timeout) won't work without significant changes to
udev/systemd. Currently udevd simply kills hanging workers with
SIGKILL, and doesn't bother to send an "incomplete uevent" message to
monitor listeners.

What we *might* do in multipathd is listen for kernel uevents _and_
udev uevents, and figuring out possible problems by relating and
comparing them. It would be a non-trivial task; we'd have to deal with
the possibility that kernel events might get lost, or might have
happened before multipathd was started. During startup, this wouldn't
help us. Rather, it would enable us to react on events during runtime
which we currently miss. I see this as a feature enhancement -
possible, but really hard to get right, and not directly related to the
problems we are currently dealing with.

If we look at udev information for a block device (either during device
probing or uevent processing), and the WWID is not set but some other
properties (e.g. ID_PATH) are, we can be pretty sure that scsi_id or
sg_inq have failed during processing of the last uevent for that
device. If, in this case, we used our fallback action to retrieve the
WWID, we'd be able to determine if it was a very short-lived problem or
something more serious.
 
> > Whatever we do, we should stop trying to "fix" the path WWID in
> > disassemble_map(). That's *so* against the separation of concerns
> > principle. In getuid(), we might check if a path with missing WWID
> > is
> > already part of an existing multipath map, and then set the path
> > WWID
> > from the map WWID as sort-of a last emergency fallback. But that,
> > too,
> > should only be done during startup (assuming that a previous
> > multipath
> > or multipathd instance had set up the map correctly, and that udev
> > information had been "lost" since then), and only after retrying as
> > described above.
> 
> We don't want to remove paths from multipath devices because
> multipathd
> started up when the path was missing udev information. The udev
> properties are trickier, but if we simply have a null WWID, it makes
> sense to allow it as a last resort if the device otherwise appears to
> have the same paths as it previously did.

I agree, but I don't agree with filling in pp->wwid from mpp->wwid.

>  users can always run
> 
> # multipath -f
> 
> to remove the device. If it looks like some of the paths are supposed
> to
> change on the device, we should quite possibly not include paths with
> a
> null WWID, because we don't know what has changed.  But we can do
> this
> someplace else than in disassemble_map(). 

How would you determine that something is "supposed to change"?

> 
> > Note that since by-property blacklisting was introduced in 2013,
> > significant progress has been made in other areas. We have
> > blacklisting
> > by transport now, "find_multipaths", the "failed_wwids" logic that
> > avoids repeated attempts at setting up maps for busy devices, and
> > the
> > INIT_MISSING_UDEV logic to deal with incomplete initialization. The
> > udev rules have been improved as well. So, doing away with
> > "required
> > udev properties" may not be so dangerous, after all. 
> > 
> > Thoughts?
> 
> Another option would be to do some extra work in reconfigure.  If we
> held on to the old path, and cleaned up everything but the old udev
> device and file descriptor, we could be sure that the kernel wouldn't
> reuse that device major:minor while we were reconfiguring. If we got
> some paths without their udev information, we would have the old udev
> information to check against the new config, to see if the device
> should
> be removed. Again, this works best if we could determine if we were
> missing udev information.  Although in this case we could probably
> just
> use any path that became blacklisted because of not having the
> necessary
> property information.

IOW, we shouldn't blacklist these paths, which is what I was trying to
say. Your idea to hold the references until reconfigure() is finished
sounds clever to me.

The idea of blacklisting-by-missing-properties is to determine if
ID_SERIAL for a given WWID is _reliable_. It should be used if we get a
non-zero ID_SERIAL and at the same time none of the required
properties, e.g. ID_WWN. IMO, if this is the case, we can be certain
that scsi_id did *not* fail - after all, it was able to obtain
ID_SERIAL. OTOH, if neither ID_SERIAL nor ID_WWN is set, failure to
access the device is likely. Thus the solution here is simple: We
should apply "blacklisting by missing property" *only* if ID_SERIAL is
set, but ID_WWN is not.

Martin

-- 
Dr. Martin Wilck <mwilck@suse.com>, Tel. +49 (0)911 74053 2107
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)


--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

Re: [PATCH 0/3] New approach at handling changed WWIDs

[Benjamin Marzinski]

On Fri, Mar 22, 2019 at 12:28:56PM +0100, Martin Wilck wrote:
> On Thu, 2019-03-21 at 17:31 -0500, Benjamin Marzinski wrote:
> > 
> > ideally, we would be able to determine whether or not udev was able
> > to
> > get all the necessary information. It would be nice to be notified if
> > scsi_id failed or udev timed out.
> 
> The latter (udev timeout) won't work without significant changes to
> udev/systemd. Currently udevd simply kills hanging workers with
> SIGKILL, and doesn't bother to send an "incomplete uevent" message to
> monitor listeners.
> 
> What we *might* do in multipathd is listen for kernel uevents _and_
> udev uevents, and figuring out possible problems by relating and
> comparing them. It would be a non-trivial task; we'd have to deal with
> the possibility that kernel events might get lost, or might have
> happened before multipathd was started. During startup, this wouldn't
> help us. Rather, it would enable us to react on events during runtime
> which we currently miss. I see this as a feature enhancement -
> possible, but really hard to get right, and not directly related to the
> problems we are currently dealing with.
> 
> If we look at udev information for a block device (either during device
> probing or uevent processing), and the WWID is not set but some other
> properties (e.g. ID_PATH) are, we can be pretty sure that scsi_id or
> sg_inq have failed during processing of the last uevent for that
> device. If, in this case, we used our fallback action to retrieve the
> WWID, we'd be able to determine if it was a very short-lived problem or
> something more serious.
>  
> > > Whatever we do, we should stop trying to "fix" the path WWID in
> > > disassemble_map(). That's *so* against the separation of concerns
> > > principle. In getuid(), we might check if a path with missing WWID
> > > is
> > > already part of an existing multipath map, and then set the path
> > > WWID
> > > from the map WWID as sort-of a last emergency fallback. But that,
> > > too,
> > > should only be done during startup (assuming that a previous
> > > multipath
> > > or multipathd instance had set up the map correctly, and that udev
> > > information had been "lost" since then), and only after retrying as
> > > described above.
> > 
> > We don't want to remove paths from multipath devices because
> > multipathd
> > started up when the path was missing udev information. The udev
> > properties are trickier, but if we simply have a null WWID, it makes
> > sense to allow it as a last resort if the device otherwise appears to
> > have the same paths as it previously did.
> 
> I agree, but I don't agree with filling in pp->wwid from mpp->wwid.

I'm fine with that.
 
> >  users can always run
> > 
> > # multipath -f
> > 
> > to remove the device. If it looks like some of the paths are supposed
> > to
> > change on the device, we should quite possibly not include paths with
> > a
> > null WWID, because we don't know what has changed.  But we can do
> > this
> > someplace else than in disassemble_map(). 
> 
> How would you determine that something is "supposed to change"?

Sorry. That was worded badly. For example, if we start up and determine
that mpatha should look just like the existing mpatha, except for a path
that in missing information, it makes sense to assume that path also
belongs. If configure determines that some of the paths that are in the
existing mpatha definitely do not belong there, then it shouldn't assume
the paths that we know nothing about do belong there.

So, if it appears that something has changed in a multipath device,
looking at the paths that we do have information for, we can't make
assumptions about the paths we don't have information for. 

> > 
> > > Note that since by-property blacklisting was introduced in 2013,
> > > significant progress has been made in other areas. We have
> > > blacklisting
> > > by transport now, "find_multipaths", the "failed_wwids" logic that
> > > avoids repeated attempts at setting up maps for busy devices, and
> > > the
> > > INIT_MISSING_UDEV logic to deal with incomplete initialization. The
> > > udev rules have been improved as well. So, doing away with
> > > "required
> > > udev properties" may not be so dangerous, after all. 
> > > 
> > > Thoughts?
> > 
> > Another option would be to do some extra work in reconfigure.  If we
> > held on to the old path, and cleaned up everything but the old udev
> > device and file descriptor, we could be sure that the kernel wouldn't
> > reuse that device major:minor while we were reconfiguring. If we got
> > some paths without their udev information, we would have the old udev
> > information to check against the new config, to see if the device
> > should
> > be removed. Again, this works best if we could determine if we were
> > missing udev information.  Although in this case we could probably
> > just
> > use any path that became blacklisted because of not having the
> > necessary
> > property information.
> 
> IOW, we shouldn't blacklist these paths, which is what I was trying to
> say. Your idea to hold the references until reconfigure() is finished
> sounds clever to me.
> 
> The idea of blacklisting-by-missing-properties is to determine if
> ID_SERIAL for a given WWID is _reliable_. It should be used if we get a
> non-zero ID_SERIAL and at the same time none of the required
> properties, e.g. ID_WWN. IMO, if this is the case, we can be certain
> that scsi_id did *not* fail - after all, it was able to obtain
> ID_SERIAL. OTOH, if neither ID_SERIAL nor ID_WWN is set, failure to
> access the device is likely. Thus the solution here is simple: We
> should apply "blacklisting by missing property" *only* if ID_SERIAL is
> set, but ID_WWN is not.

Sure.

-Ben
 
> Martin
> 
> -- 
> Dr. Martin Wilck <mwilck@suse.com>, Tel. +49 (0)911 74053 2107
> SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton
> HRB 21284 (AG Nürnberg)
> 

Re: [PATCH 0/3] New approach at handling changed WWIDs

[Benjamin Marzinski]

On Tue, Mar 19, 2019 at 07:44:09PM +0100, Martin Wilck wrote:
> On Tue, 2019-03-19 at 12:11 -0500, Benjamin Marzinski wrote:
> > On Mon, Mar 18, 2019 at 01:12:32PM +0100, Martin Wilck wrote:
> > 
> > d) and e) seem to me like by far the most likely scenarios, so doing
> > something suboptimal for them, in order to optimize the very unlikely
> > scenarios seems wrong.
> 
> You've worked on this far more than I did, so I trust your judgement.
> 
> One remark: With the kernel exporting the VPD pages in sysfs
> and my late "multipath -u" patch series, udev should be able to process
> uevents for path devices without a single actual device IO. (*) 
> While there are still possible causes for udev processing to get stuck,
> this would make it much less likely.

For reasons I don't understand, not all devices get a vpd_pg83 sysfs
file, even though they clearly have a page 0x83, since multipath and
sg_inq can read from it via ioctl.  I have a device like this. This is
quite possibly a kernel issue that should get fixed, but regardless,
it's been there for a while, and I just cheked and it still exists in
the 5.0.0-rc6 kernel.

> > > Note also that if a "reconfigure" was carried out in the presence
> > > of
> > > paths with changed WWID, the final outcome would likely be the same
> > > that
> > > my patch now achieves without "reconfigure".
> > 
> > Yeah. I just checked, and this is very broken, and something needs to
> > be
> > done to fix it.  If a device gets a change event while it's down, it
> > will no longer have the udev properties necessary to not be
> > blacklisted,
> > so the device gets blacklisted, and ignored.  Even after it comes
> > back
> > up and gest another change event to restore these values, multipathd
> > still ignores it, because the device was blacklisted during its add
> > event.
> 
> I wasn't aware of that.

I just found it out when I was checking to see what really happened on a
reconfigure with a null wwid path.

> > However, if you just set the wwid of a path to NULL and run
> > reconfigure,
> > it will stay added to the map, and get it's wwid restored to the
> > multipath device's wwid, because configure() calls map_discovery(),
> > which ends up calling disassemble_map(). This will set a NULL path
> > wwid
> > to the multipath device wwid, since disassemble_map() assumes that
> > NULL
> > wwids just mean that multipathd couldn't get the wwid temporarily.
> 
> My thinking was that reconfigure calls path_discovery(), which would
> call pathinfo() on all available paths again. I understand now - if
> there was an error processing the last uevent for a device, the
> information in the udev DB will still be incomplete, and
> path_discovery() will obtain the same broken information, possibly
> blacklisting the device. I thought the WWID might then be restored by
> the INIT_MISSING_UDEV logic, but disassemble_map() would almost
> certainly come first. I'm not sure I like it that disassemble_map()
> tries to fill in path information. It's ok to do this for paths
> that haven't been detected at all (pp == NULL case) but in case of
> paths that are present in the udev db, I'm uncertain.
> 
> This would also affect your treatment of "WWID changed to zero", no?
> Any call to setup_multipath() or update_multipath() would call
> disassemble_map(), which would possibly mess with the path's wwid...
> 

No, because my code doesn't change the wwid if moves to NULL, it simply
remembers the old wwid.  This only happens when you do a reconfigure,
because you are getting new paths. But regardless, I agree that we
should avoid setting the path wwid based on the map wwid.

> You'd reverted this change in your patch 12, which confused me quite a
> bit. What is your preference, really?

My preference really is my 07 patch, which is why I didn't simply merge
them to start with.

> Let's go for the removal and re-addition for the non-NULL changed WWID
> case, and use the quick path for the NULL-WWID case (maybe we could
> even try the "fallback" immediately in uev_update_path()?).

That was the whole point of my setting retriggers to the max. So that we
would immediately try the fallback method if we got a NULL wwid, after
we had successfully gotten it in the past.
 
> You have a more complete understanding of the whole issue than me, so I
> trust that you'll come up with a v3 series addressing all these issues.

I'd like to propose a different compromise; that is my option 1 (patch
07) along with triggering a uevent to update udev for NULL wwids. Here
is my biggest objection to option 2 (patch 12). In the scenario where we
have an incomplete uevent because of a uevent storm, multipathd will
suddenly fail a path that is working fine, simply because the machine is
under load.  I just don't relish the idea of explaining to a customer
that we failed their paths (likely making the situation even worse) even
though we didn't have to, simply because we were worried that the
customer might do something stupid, and we would have a better chance
(although not a certaintly) of protecting them from their own stupid
mistake if we unnecessarily failed working paths.

This might be o.k. if we defaulted this behaviour to "off", but since it
is "on" by default, we are running a very real risk of failing perfectly
working paths, because we think our users might be stupid.

-Ben

> Regards
> Martin
> 
> (*) This works only if the tool used to retrieve the VPDs support
> reading from sysfs, of course, as sg_inq does (scsi_id does not,
> currently).

So, even when I run sg_inq on a device that does have the vpd_pg83 sysfs
file, it still does an ioctl. Do you know if that's just because I'm
using version 1.42, instead of 1.44?  As far as I know, sg_inq always
has done an ioctl.

> 
> -- 
> Dr. Martin Wilck <mwilck@suse.com>, Tel. +49 (0)911 74053 2107
> SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton
> HRB 21284 (AG Nürnberg)
> 

Re: [PATCH 0/3] New approach at handling changed WWIDs

[Martin Wilck]

On Mon, 2019-03-25 at 17:27 -0500, Benjamin Marzinski wrote:
> 
> > One remark: With the kernel exporting the VPD pages in sysfs
> > and my late "multipath -u" patch series, udev should be able to
> > process
> > uevents for path devices without a single actual device IO. (*) 
> > While there are still possible causes for udev processing to get
> > stuck,
> > this would make it much less likely.
> 
> For reasons I don't understand, not all devices get a vpd_pg83 sysfs
> file, even though they clearly have a page 0x83, since multipath and
> sg_inq can read from it via ioctl.  I have a device like this. This
> is
> quite possibly a kernel issue that should get fixed, but regardless,
> it's been there for a while, and I just cheked and it still exists in
> the 5.0.0-rc6 kernel.

This is intentional. The kernel applies conservative heuristics to
determine whether these VPDs are supported. See the comment in
scsi_device_supports_vpd(). Sometimes the kernel gets it wrong, usually
because the device claims to be a pre-SCSI_SPC_2 device. Such devices
should be fixed with the BLIST_TRY_VPD_PAGES device flag if they do
support the DI VPD.

OPEN-V devices are a notorious example. But meanwhile the blist flags
are in place for them.

Regards
Martin

-- 
Dr. Martin Wilck <mwilck@suse.com>, Tel. +49 (0)911 74053 2107
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)


--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel