[PATCH RESEND 0/4] multipath-tools: fixes for path wwid detection and path change uevents

[PATCH RESEND 0/4] multipath-tools: fixes for path wwid detection and path change uevents

[Martin Wilck]

Hi Christophe,

this small series fixes some minor glitches I found in the current path
discovery code, and attempts to implement the safe part of the functionality
discussed in the thread "multipathd: update path's udev in uev_update_path"
in January (based on an idea from Wu Chongyun).

Resending the series, unchanged, rebased on 0.7.5.

Regards,
Martin

Martin Wilck (4):
  libmultipath: get_uid: check VPD pages for SCSI only
  libmultipath: get_uid: don't quit prematurely without udev
  libmultipath: uev_update_path: always warn if WWID changed
  libmultipath: uev_update_path: update path properties

 libmultipath/discovery.c | 58 ++++++++++++++++++++++++++++--------------------
 multipathd/main.c        | 29 +++++++++++++++++-------
 2 files changed, 55 insertions(+), 32 deletions(-)

-- 
2.16.1

[PATCH RESEND 1/4] libmultipath: get_uid: check VPD pages for SCSI only

[Martin Wilck]

The VPD code won't work for non-SCSI devices, anyway. For indentation
reasons, I moved the "retrigger_tries" case to a separate function,
which is also called only for SCSI devices.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/discovery.c | 50 +++++++++++++++++++++++++++++++-----------------
 1 file changed, 32 insertions(+), 18 deletions(-)

diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
index 3d38a2550980..53182a85fa10 100644
--- a/libmultipath/discovery.c
+++ b/libmultipath/discovery.c
@@ -1807,9 +1807,38 @@ get_vpd_uid(struct path * pp)
 		parent = udev_device_get_parent(parent);
 	}
 
+	if (!parent)
+		return -EINVAL;
+
 	return get_vpd_sysfs(parent, 0x83, pp->wwid, WWID_SIZE);
 }
 
+static ssize_t scsi_uid_fallback(struct path *pp, int path_state,
+			     const char **origin)
+{
+	ssize_t len = 0;
+	int retrigger;
+	struct config *conf;
+
+	conf = get_multipath_config();
+	retrigger = conf->retrigger_tries;
+	put_multipath_config(conf);
+	if (pp->retriggers >= retrigger &&
+	    !strcmp(pp->uid_attribute, DEFAULT_UID_ATTRIBUTE)) {
+		len = get_vpd_uid(pp);
+		*origin = "sysfs";
+		pp->uid_attribute = NULL;
+		if (len < 0 && path_state == PATH_UP) {
+			condlog(1, "%s: failed to get sysfs uid: %s",
+				pp->dev, strerror(-len));
+			len = get_vpd_sgio(pp->fd, 0x83, pp->wwid,
+					   WWID_SIZE);
+			*origin = "sgio";
+		}
+	}
+	return len;
+}
+
 int
 get_uid (struct path * pp, int path_state, struct udev_device *udev)
 {
@@ -1851,7 +1880,6 @@ get_uid (struct path * pp, int path_state, struct udev_device *udev)
 		len = get_rbd_uid(pp);
 		origin = "sysfs";
 	} else {
-		int retrigger;
 
 		if (pp->uid_attribute) {
 			len = get_udev_uid(pp, pp->uid_attribute, udev);
@@ -1861,26 +1889,12 @@ get_uid (struct path * pp, int path_state, struct udev_device *udev)
 					"%s: failed to get udev uid: %s",
 					pp->dev, strerror(-len));
 
-		} else {
+		} else if (pp->bus == SYSFS_BUS_SCSI) {
 			len = get_vpd_uid(pp);
 			origin = "sysfs";
 		}
-		conf = get_multipath_config();
-		retrigger = conf->retrigger_tries;
-		put_multipath_config(conf);
-		if (len <= 0 && pp->retriggers >= retrigger &&
-		    !strcmp(pp->uid_attribute, DEFAULT_UID_ATTRIBUTE)) {
-			len = get_vpd_uid(pp);
-			origin = "sysfs";
-			pp->uid_attribute = NULL;
-			if (len < 0 && path_state == PATH_UP) {
-				condlog(1, "%s: failed to get sysfs uid: %s",
-					pp->dev, strerror(-len));
-				len = get_vpd_sgio(pp->fd, 0x83, pp->wwid,
-						   WWID_SIZE);
-				origin = "sgio";
-			}
-		}
+		if (len <= 0 && pp->bus == SYSFS_BUS_SCSI)
+			len = scsi_uid_fallback(pp, path_state, &origin);
 	}
 	if ( len < 0 ) {
 		condlog(1, "%s: failed to get %s uid: %s",
-- 
2.16.1

[PATCH RESEND 2/4] libmultipath: get_uid: don't quit prematurely without udev

[Martin Wilck]

Not all the implemented methods to derive the UID rely on udev
information being present. For example getuid callout, rbd,
and the SCSI vpd code work fine without it. It's unlikely that
we don't get udev data, but we want to be as good as possible
at deriving the uid.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/discovery.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
index 53182a85fa10..9f2a9c907914 100644
--- a/libmultipath/discovery.c
+++ b/libmultipath/discovery.c
@@ -1853,11 +1853,6 @@ get_uid (struct path * pp, int path_state, struct udev_device *udev)
 		put_multipath_config(conf);
 	}
 
-	if (!udev) {
-		condlog(1, "%s: no udev information", pp->dev);
-		return 1;
-	}
-
 	memset(pp->wwid, 0, WWID_SIZE);
 	if (pp->getuid) {
 		char buff[CALLOUT_MAX_SIZE];
@@ -1881,7 +1876,7 @@ get_uid (struct path * pp, int path_state, struct udev_device *udev)
 		origin = "sysfs";
 	} else {
 
-		if (pp->uid_attribute) {
+		if (udev && pp->uid_attribute) {
 			len = get_udev_uid(pp, pp->uid_attribute, udev);
 			origin = "udev";
 			if (len <= 0)
@@ -1900,6 +1895,7 @@ get_uid (struct path * pp, int path_state, struct udev_device *udev)
 		condlog(1, "%s: failed to get %s uid: %s",
 			pp->dev, origin, strerror(-len));
 		memset(pp->wwid, 0x0, WWID_SIZE);
+		return 1;
 	} else {
 		/* Strip any trailing blanks */
 		c = strchr(pp->wwid, '\0');
-- 
2.16.1

[PATCH RESEND 3/4] libmultipath: uev_update_path: always warn if WWID changed

[Martin Wilck]

Print the warning about changed WWID not only if disable_changed_wwids
is set, but always. It's actually more dangerous if that option is
not set.

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

diff --git a/multipathd/main.c b/multipathd/main.c
index f85995810950..e9668205ce92 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -940,15 +940,18 @@ 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];
 
-		if (disable_changed_wwids &&
-		    (strlen(pp->wwid) || pp->wwid_changed)) {
-			char wwid[WWID_SIZE];
+		strcpy(wwid, pp->wwid);
+		get_uid(pp, pp->state, uev->udev);
 
-			strcpy(wwid, pp->wwid);
-			get_uid(pp, pp->state, uev->udev);
-			if (strcmp(wwid, pp->wwid) != 0) {
-				condlog(0, "%s: path wwid changed from '%s' to '%s'. disallowing", uev->kernel, wwid, pp->wwid);
+		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"));
+			if (disable_changed_wwids &&
+			    (strlen(wwid) || pp->wwid_changed)) {
 				strcpy(pp->wwid, wwid);
 				if (!pp->wwid_changed) {
 					pp->wwid_changed = 1;
@@ -957,7 +960,9 @@ uev_update_path (struct uevent *uev, struct vectors * vecs)
 						dm_fail_path(pp->mpp->alias, pp->dev_t);
 				}
 				goto out;
-			} else
+			} else if (!disable_changed_wwids)
+				strcpy(pp->wwid, wwid);
+			else
 				pp->wwid_changed = 0;
 		}
 
-- 
2.16.1

[PATCH RESEND 4/4] libmultipath: uev_update_path: update path properties

[Martin Wilck]

Update pp->udev and those path attributes that can be cheaply
updated from sysfs, i.e. without IO to the disk.

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

diff --git a/multipathd/main.c b/multipathd/main.c
index e9668205ce92..6e6c52a52783 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -964,6 +964,14 @@ uev_update_path (struct uevent *uev, struct vectors * vecs)
 				strcpy(pp->wwid, wwid);
 			else
 				pp->wwid_changed = 0;
+		} else {
+			udev_device_unref(pp->udev);
+			pp->udev = udev_device_ref(uev->udev);
+			conf = get_multipath_config();
+			if (pathinfo(pp, conf, DI_SYSFS|DI_NOIO) != PATHINFO_OK)
+				condlog(1, "%s: pathinfo failed after change uevent",
+					uev->kernel);
+			put_multipath_config(conf);
 		}
 
 		if (pp->initialized == INIT_REQUESTED_UDEV)
-- 
2.16.1

Re: [PATCH RESEND 0/4] multipath-tools: fixes for path wwid detection and path change uevents

[Benjamin Marzinski]

On Thu, Mar 08, 2018 at 12:21:48AM +0100, Martin Wilck wrote:
> Hi Christophe,
> 
> this small series fixes some minor glitches I found in the current path
> discovery code, and attempts to implement the safe part of the functionality
> discussed in the thread "multipathd: update path's udev in uev_update_path"
> in January (based on an idea from Wu Chongyun).
> 
> Resending the series, unchanged, rebased on 0.7.5.
> 
> Regards,
> Martin
> 

Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>

for the set.

> Martin Wilck (4):
>   libmultipath: get_uid: check VPD pages for SCSI only
>   libmultipath: get_uid: don't quit prematurely without udev
>   libmultipath: uev_update_path: always warn if WWID changed
>   libmultipath: uev_update_path: update path properties
> 
>  libmultipath/discovery.c | 58 ++++++++++++++++++++++++++++--------------------
>  multipathd/main.c        | 29 +++++++++++++++++-------
>  2 files changed, 55 insertions(+), 32 deletions(-)
> 
> -- 
> 2.16.1

Re: [PATCH RESEND 0/4] multipath-tools: fixes for path wwid detection and path change uevents

[Martin Wilck]

Hi Ben, Christophe,

I found a problem with this old patch series of mine.

On Wed, 2018-03-14 at 12:40 -0500, Benjamin Marzinski wrote:
> On Thu, Mar 08, 2018 at 12:21:48AM +0100, Martin Wilck wrote:
> > Hi Christophe,
> > 
> > this small series fixes some minor glitches I found in the current
> > path
> > discovery code, and attempts to implement the safe part of the
> > functionality
> > discussed in the thread "multipathd: update path's udev in
> > uev_update_path"
> > in January (based on an idea from Wu Chongyun).
> > 
> > Resending the series, unchanged, rebased on 0.7.5.
> > 
> > Regards,
> > Martin
> > 
> 
> Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
> 
> for the set.
> 
> > Martin Wilck (4):
> >   libmultipath: get_uid: check VPD pages for SCSI only
> >   libmultipath: get_uid: don't quit prematurely without udev
> >   libmultipath: uev_update_path: always warn if WWID changed
> >   libmultipath: uev_update_path: update path properties

while I am still pondering about your latest series, it occured to me
that my patch "get_uid: don't quit prematurely without udev" is against
the spirit of your "retrigger uevents to try and get the uid through
udev" patch from 2016 (688aa22b).

"get_uid: don't quit prematurely without udev" has been merged a while
ago (08d42ee6). Should it be reverted??

Pro: the patch is against the "udev first" philosophy and subverts the
retrigger logic.
Con: with the patch, we'll be able to retrieve WWIDs more quickly in
some situations, as we don't have to wait for udev.

Either way (but more likely with my patch), it may happen that we
retrieve a WWID from sysfs or elsewhere first, and from udev later.
These WWIDs may not necessarily match, and a "WWID changed" problem may
occur.

Please tell me what you think.

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 RESEND 0/4] multipath-tools: fixes for path wwid detection and path change uevents

[Benjamin Marzinski]

On Tue, Oct 02, 2018 at 11:02:50PM +0200, Martin Wilck wrote:
> Hi Ben, Christophe,
> 
> I found a problem with this old patch series of mine.
> 
> while I am still pondering about your latest series, it occured to me
> that my patch "get_uid: don't quit prematurely without udev" is against
> the spirit of your "retrigger uevents to try and get the uid through
> udev" patch from 2016 (688aa22b).
> 
> "get_uid: don't quit prematurely without udev" has been merged a while
> ago (08d42ee6). Should it be reverted??
> 
> Pro: the patch is against the "udev first" philosophy and subverts the
> retrigger logic.
> Con: with the patch, we'll be able to retrieve WWIDs more quickly in
> some situations, as we don't have to wait for udev.
> 
> Either way (but more likely with my patch), it may happen that we
> retrieve a WWID from sysfs or elsewhere first, and from udev later.
> These WWIDs may not necessarily match, and a "WWID changed" problem may
> occur.
> 
> Please tell me what you think.

I'm having a hard time seeing how get_uid() could get called on a path
without a udev device attached in multipathd. It can happen because of
update_paths() in multipath, for paths that don't get discovered when
searching for paths, but do belong to an existing multipath device. I
don't think you patch will hurt anything here. So, unless I'm missing
some code path in multipathd, I don't see a problem with this patch.

In fact, we should probably make scsi_uid_fallback() skip (or at least
always pass) the retrigger check if we aren't in the daemon, so we
always try the failback when we run multipath.

-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 RESEND 0/4] multipath-tools: fixes for path wwid detection and path change uevents

[Martin Wilck]

On Fri, 2018-10-05 at 18:07 -0500,  Benjamin Marzinski  wrote:
> On Tue, Oct 02, 2018 at 11:02:50PM +0200, Martin Wilck wrote:
> > Hi Ben, Christophe,
> > 
> > I found a problem with this old patch series of mine.
> > 
> > while I am still pondering about your latest series, it occured to
> > me
> > that my patch "get_uid: don't quit prematurely without udev" is
> > against
> > the spirit of your "retrigger uevents to try and get the uid
> > through
> > udev" patch from 2016 (688aa22b).
> > 
> > "get_uid: don't quit prematurely without udev" has been merged a
> > while
> > ago (08d42ee6). Should it be reverted??
> > 
> > Pro: the patch is against the "udev first" philosophy and subverts
> > the
> > retrigger logic.
> > Con: with the patch, we'll be able to retrieve WWIDs more quickly
> > in
> > some situations, as we don't have to wait for udev.
> > 
> > Either way (but more likely with my patch), it may happen that we
> > retrieve a WWID from sysfs or elsewhere first, and from udev later.
> > These WWIDs may not necessarily match, and a "WWID changed" problem
> > may
> > occur.
> > 
> > Please tell me what you think.
> 
> I'm having a hard time seeing how get_uid() could get called on a
> path
> without a udev device attached in multipathd. It can happen because
> of
> update_paths() in multipath, for paths that don't get discovered when
> searching for paths, but do belong to an existing multipath device. I
> don't think you patch will hurt anything here. So, unless I'm missing
> some code path in multipathd, I don't see a problem with this patch.

OK, fine then. Thanks for clarifying this.

> In fact, we should probably make scsi_uid_fallback() skip (or at
> least
> always pass) the retrigger check if we aren't in the daemon, so we
> always try the failback when we run multipath.

AFAICS that's already the case, because multipath sets 
conf->retrigger_tries = 0 on startup.

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