[PATCH 00/31] multipath/kpartx udev rules cleanup, and fixes

[PATCH 00/31] multipath/kpartx udev rules cleanup, and fixes

[Martin Wilck]

The main part of this series is a cleanup of the udev rules for
multipath and kpartx. More about that further down.

 - patch 01 is an obvious bug fix
 - patch 02-08 are fixes for kpartx problems that I encountered
   during testing, with related tests. The interesting one here is
   04, which adds the ability to rename partition mappings if the
   current name doesn't match the specified scheme. The purpose
   is to provide consistent naming of partition mappings across
   the OS, even in the presence of other tools such as parted
   that may follow different conventions.
 - 09-15 are changes to the core code I found necessary for the
   udev rules cleanup, most importantly a helper to figure out
   quickly whether a given multipath has usable paths
   (multipath -U). This replaces the former SUSE-specific tests
   for DM_DEPS and DM_TABLE_STATE.
 - 16ff are the actual changes to the udev rules. This was motivated
   by Ben Marzinski who asked Hannes and me a while ago to remove
   the SUSE-isms in the rules files (patch 23), and cleanup mainly
   kpartx.rules.

More about the udev rules changes:

Multipath maps are special because they can exist without any
usable members/paths, and users expect them to be at least partly
functional in such situations. Care has to be taken to avoid I/O
on maps that may not be able to process it, and not to loose any
symlinks that might be used by systemd to access the partition.
Partitions on multipath maps have similar semantics, with the added
difficulty that no first-hand information about the parent device
is available. A lot of the existing code was trying to fix these
corner case situations, but sometimes incorrectly or not cleanly.
I have pointed out the issues in the individual commit messages.
One important point is that checking DM_NR_VALID_PATHS>0 is not
in sufficient to affirm that I/O will succeed. That was the reason
for writing the "multipath -U" helper.

Also, multipath maps receive frequent udev events that don't
matter to upper layers at all. In such cases, further scanning
should be avoided. This is implemented much more cleanly now,
I hope. 

Following the example of the dm core, I split the kpartx rules
up in "early" rules for setting properties and symlinks, and
"late" rules for taking actions such as running kpartx. I believe
that this makes the code better readable.
CAUTION: The new rules files matter for other packages such as dracut.
Distribution package builders have to be careful here. If this patch set
is accepted, respective patches will be send to the dracut maintainers.

Following Ben's suggestions, a new rules file is added that
is responsible for deleting partition device nodes for multipath
member devices.

I didn't expect this series to grow so large, but after having
worked and tested for considerable time, this is the first
version that I find ready for serious review. I smoke-tested the
interaction of rules files, multipathd, udev, systemd, and
kpartx quite a bit with failover and 0-paths scenarios, successfully.

The changes are less drastic than the stats below suggest.
A considerable part just moves functionality out of existing code
into separate functions in order to use it elsewhere.

Regards,
Martin

Martin Wilck (31):
  libmultipath: fix partition_delimiter config option
  kpartx: helper functions for name and uuid generation
  kpartx: search partitions by UUID, and rename
  test-kpartx: add tests for renaming functionality
  kpartx: fix a corner case when renaming partitions
  kpartx: fix part deletion without partition table
  test-kpartx: test deletion with empty part table
  kpartx: only recognize dasd part table on DASD
  libmultipath: support MPATH_UDEV_NO_PATHS_FLAG on map creation
  libmultipath: add get_udev_device
  libmultipath: get_refwwid: use get_udev_device
  libmultipath: use const char* in some dm helpers
  libmultipath: add DI_NOIO flag for pathinfo
  libmultipath: add dm_get_multipath
  multipath: implement "check usable paths" (-C/-U)
  11-dm-mpath.rules: multipath -U for READY check
  11-dm-mpath.rules: import more ID_FS_xxx vars from db
  11-dm-mpath.rules: no need to test before IMPORT
  11-dm-mpath.rules: handle new maps with READY==0
  11-dm-mpath.rules: don't set READY->ACTIVATION
  11-dm-mpath.rules: Remember DM_ACTIVATION
  multipath.rules: set ID_FS_TYPE to "mpath_member"
  kpartx.rules: don't rely on DM_DEPS and DM_TABLE_STATE
  kpartx.rules: respect DM_UDEV_LOW_PRIORITY_FLAG
  kpartx.rules: improved logic for by-uuid and by-label links
  kpartx.rules: create by-partuuid and by-partlabel symlinks
  kpartx.rules: generate type-name links only for multipath devices
  kpartx.rules: fix logic for adding partitions
  multipath/kpartx rules: avoid superfluous scanning
  kpartx/del-part-nodes.rules: new udev file
  kpartx.rules: move symlink code to other files

 kpartx/Makefile             |   2 +
 kpartx/dasd.c               |   3 +
 kpartx/del-part-nodes.rules |  32 ++++++++
 kpartx/devmapper.c          | 194 ++++++++++++++++++++++++++++++++++++++++++--
 kpartx/devmapper.h          |  18 +++-
 kpartx/dm-parts.rules       |  39 +++++++++
 kpartx/kpartx.c             | 105 +++++++-----------------
 kpartx/kpartx.rules         |  58 ++++++-------
 kpartx/test-kpartx          |  50 ++++++++++++
 libmultipath/config.h       |   1 +
 libmultipath/configure.c    |  58 ++++++++++---
 libmultipath/configure.h    |   1 +
 libmultipath/devmapper.c    |  87 ++++++++++++--------
 libmultipath/devmapper.h    |   5 +-
 libmultipath/discovery.c    |   8 ++
 libmultipath/discovery.h    |   2 +
 multipath/11-dm-mpath.rules |  76 +++++++++++++----
 multipath/main.c            |  90 +++++++++++++++++++-
 multipath/multipath.8       |  13 ++-
 multipath/multipath.rules   |   2 +-
 20 files changed, 659 insertions(+), 185 deletions(-)
 create mode 100644 kpartx/del-part-nodes.rules
 create mode 100644 kpartx/dm-parts.rules

-- 
2.14.0

[PATCH 01/31] libmultipath: fix partition_delimiter config option

[Martin Wilck]

The partition_delimiter setting was effecitvely ignored. Fix it.

Fixes: 95bf339bb9d7 "correctly set partition delimiter on rename"
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/devmapper.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/libmultipath/devmapper.c b/libmultipath/devmapper.c
index 3b41a483..e701e806 100644
--- a/libmultipath/devmapper.c
+++ b/libmultipath/devmapper.c
@@ -1351,10 +1351,12 @@ dm_rename_partmaps (const char * old, char * new, char *delim)
 
 	if (delim)
 		rd.delim = delim;
-	if (isdigit(new[strlen(new)-1]))
-		rd.delim = "p";
-	else
-		rd.delim = "";
+	else {
+		if (isdigit(new[strlen(new)-1]))
+			rd.delim = "p";
+		else
+			rd.delim = "";
+	}
 	return do_foreach_partmaps(old, rename_partmap, &rd);
 }
 
-- 
2.14.0

[PATCH 02/31] kpartx: helper functions for name and uuid generation

[Martin Wilck]

strip_slash() is copied from kpartx.c, and will be removed there
in a follow-up patch. The others are new helpers.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 kpartx/devmapper.c | 44 ++++++++++++++++++++++++++++++++++++++------
 1 file changed, 38 insertions(+), 6 deletions(-)

diff --git a/kpartx/devmapper.c b/kpartx/devmapper.c
index 4ab58ce9..3d382285 100644
--- a/kpartx/devmapper.c
+++ b/kpartx/devmapper.c
@@ -92,6 +92,42 @@ out:
 	return r;
 }
 
+static void
+strip_slash (char * device)
+{
+	char * p = device;
+
+	while (*(p++) != 0x0) {
+
+		if (*p == '/')
+			*p = '!';
+	}
+}
+
+static int format_partname(char *buf, size_t bufsiz,
+			   const char *mapname, const char *delim, int part)
+{
+	if (snprintf(buf, bufsiz, "%s%s%d", mapname, delim, part) >= bufsiz)
+		return 0;
+	strip_slash(buf);
+	return 1;
+}
+
+static char *make_prefixed_uuid(int part, const char *uuid)
+{
+	char *prefixed_uuid;
+	int len = MAX_PREFIX_LEN + strlen(uuid) + 1;
+
+	prefixed_uuid = malloc(len);
+	if (!prefixed_uuid) {
+		fprintf(stderr, "cannot create prefixed uuid : %s\n",
+			strerror(errno));
+		return NULL;
+	}
+	snprintf(prefixed_uuid, len, UUID_PREFIX "%s", part, uuid);
+	return prefixed_uuid;
+}
+
 int dm_addmap(int task, const char *name, const char *target,
 	      const char *params, uint64_t size, int ro, const char *uuid,
 	      int part, mode_t mode, uid_t uid, gid_t gid)
@@ -117,13 +153,9 @@ int dm_addmap(int task, const char *name, const char *target,
 			goto addout;
 
 	if (task == DM_DEVICE_CREATE && uuid) {
-		prefixed_uuid = malloc(MAX_PREFIX_LEN + strlen(uuid) + 1);
-		if (!prefixed_uuid) {
-			fprintf(stderr, "cannot create prefixed uuid : %s\n",
-				strerror(errno));
+		prefixed_uuid = make_prefixed_uuid(part, uuid);
+		if (prefixed_uuid == NULL)
 			goto addout;
-		}
-		sprintf(prefixed_uuid, UUID_PREFIX "%s", part, uuid);
 		if (!dm_task_set_uuid(dmt, prefixed_uuid))
 			goto addout;
 	}
-- 
2.14.0

[PATCH 03/31] kpartx: search partitions by UUID, and rename

[Martin Wilck]

Partition mappings may have been created by other tools such as parted,
multipathd (during map renaming), or kpartx itself with different
delimiter (-p) option. In such cases kpartx fails, because the partition
mappings have a different name. However, we have a convention for UUID
mappings which seems to be more universal. So, when the search for the
named partition fails, search for the UUID instead and try to rename the
map.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 kpartx/devmapper.c | 108 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
 kpartx/devmapper.h |   5 ++-
 kpartx/kpartx.c    |  49 ++++++------------------
 3 files changed, 120 insertions(+), 42 deletions(-)

diff --git a/kpartx/devmapper.c b/kpartx/devmapper.c
index 3d382285..4aca0e51 100644
--- a/kpartx/devmapper.c
+++ b/kpartx/devmapper.c
@@ -188,7 +188,7 @@ addout:
 	return r;
 }
 
-int dm_map_present(char * str, char **uuid)
+static int dm_map_present(char *str, char **uuid)
 {
 	int r = 0;
 	struct dm_task *dmt;
@@ -226,6 +226,51 @@ out:
 	return r;
 }
 
+static int dm_rename (const char *old, const char *new)
+{
+	int r = 0;
+	struct dm_task *dmt;
+	uint16_t udev_flags = DM_UDEV_DISABLE_LIBRARY_FALLBACK;
+	uint32_t cookie = 0;
+
+	dmt = dm_task_create(DM_DEVICE_RENAME);
+	if (!dmt)
+		return r;
+
+	if (!dm_task_set_name(dmt, old) ||
+	    !dm_task_set_newname(dmt, new) ||
+	    !dm_task_no_open_count(dmt) ||
+	    !dm_task_set_cookie(dmt, &cookie, udev_flags))
+		goto out;
+
+	r = dm_task_run(dmt);
+	dm_udev_wait(cookie);
+
+out:
+	dm_task_destroy(dmt);
+	return r;
+}
+
+static const char *dm_find_uuid(const char *uuid)
+{
+	struct dm_task *dmt;
+	const char *name = NULL, *tmp;
+
+	if ((dmt = dm_task_create(DM_DEVICE_INFO)) == NULL)
+		return NULL;
+
+	if (!dm_task_set_uuid(dmt, uuid) ||
+	    !dm_task_run(dmt))
+		goto out;
+
+	tmp = dm_task_get_name(dmt);
+	if (tmp != NULL && *tmp != '\0')
+		name = strdup(tmp);
+
+out:
+	dm_task_destroy(dmt);
+	return name;
+}
 
 char *
 dm_mapname(int major, int minor)
@@ -340,7 +385,7 @@ out:
 }
 
 static int
-dm_get_map(char *mapname, char * outparams)
+dm_get_map(const char *mapname, char * outparams)
 {
 	int r = 1;
 	struct dm_task *dmt;
@@ -589,3 +634,62 @@ dm_remove_partmaps (char * mapname, char *uuid, dev_t devt, int verbose)
 	struct remove_data rd = { verbose };
 	return do_foreach_partmaps(mapname, uuid, devt, remove_partmap, &rd);
 }
+
+int dm_find_part(const char *parent, const char *delim, int part,
+		 const char *parent_uuid,
+		 char *name, size_t namesiz, char **part_uuid, int verbose)
+{
+	int r;
+	char params[PARAMS_SIZE];
+	const char *tmp;
+	char *uuid;
+	int major, minor;
+	char dev_t[32];
+
+	if (!format_partname(name, namesiz, parent, delim, part)) {
+		if (verbose)
+			fprintf(stderr, "partname too small\n");
+		return 0;
+	}
+
+	r = dm_map_present(name, part_uuid);
+	if (r == 1 || parent_uuid == NULL || *parent_uuid == '\0')
+		return r;
+
+	uuid = make_prefixed_uuid(part, parent_uuid);
+	if (!uuid)
+		return 0;
+
+	tmp = dm_find_uuid(uuid);
+	if (tmp == NULL)
+		return r;
+
+	/* Sanity check on partition, see dm_foreach_partmaps */
+	if (dm_type(tmp, "linear") != 1)
+		goto out;
+
+	if (dm_devn(parent, &major, &minor))
+		goto out;
+	snprintf(dev_t, sizeof(dev_t), "%d:%d", major, minor);
+
+	if (dm_get_map(tmp, params))
+		goto out;
+
+	if (!strstr(params, dev_t))
+		goto out;
+
+	if (verbose)
+		fprintf(stderr, "found map %s for uuid %s, renaming to %s\n",
+		       tmp, uuid, name);
+
+	r = dm_rename(tmp, name);
+	if (r == 0) {
+		free(uuid);
+		if (verbose)
+			fprintf(stderr, "renaming %s->%s failed\n", tmp, name);
+	} else
+		*part_uuid = uuid;
+out:
+	free((void*)tmp);
+	return r;
+}
diff --git a/kpartx/devmapper.h b/kpartx/devmapper.h
index 2e28c780..0ce4b0d5 100644
--- a/kpartx/devmapper.h
+++ b/kpartx/devmapper.h
@@ -13,12 +13,13 @@ int dm_prereq (char *, int, int, int);
 int dm_simplecmd (int, const char *, int, uint16_t);
 int dm_addmap (int, const char *, const char *, const char *, uint64_t,
 	       int, const char *, int, mode_t, uid_t, gid_t);
-int dm_map_present (char *, char **);
 char * dm_mapname(int major, int minor);
 dev_t dm_get_first_dep(char *devname);
 char * dm_mapuuid(const char *mapname);
 int dm_devn (const char * mapname, int *major, int *minor);
 int dm_remove_partmaps (char * mapname, char *uuid, dev_t devt, int verbose);
-int dm_no_partitions(char * mapname);
+int dm_find_part(const char *parent, const char *delim, int part,
+		 const char *parent_uuid,
+		 char *name, size_t namesiz, char **part_uuid, int verbose);
 
 #endif /* _KPARTX_DEVMAPPER_H */
diff --git a/kpartx/kpartx.c b/kpartx/kpartx.c
index 9ba78d01..ad486afd 100644
--- a/kpartx/kpartx.c
+++ b/kpartx/kpartx.c
@@ -150,18 +150,6 @@ set_delimiter (char * device, char * delimiter)
 		*delimiter = 'p';
 }
 
-static void
-strip_slash (char * device)
-{
-	char * p = device;
-
-	while (*(p++) != 0x0) {
-
-		if (*p == '/')
-			*p = '!';
-	}
-}
-
 static int
 find_devname_offset (char * device)
 {
@@ -527,21 +515,16 @@ main(int argc, char **argv){
 					continue;
 				}
 
-				if (safe_sprintf(partname, "%s%s%d",
-					     mapname, delim, j+1)) {
-					fprintf(stderr, "partname too small\n");
-					exit(1);
-				}
-				strip_slash(partname);
-
 				if (safe_sprintf(params, "%d:%d %" PRIu64 ,
 						 major(buf.st_rdev), minor(buf.st_rdev), slices[j].start)) {
 					fprintf(stderr, "params too small\n");
 					exit(1);
 				}
 
-				op = (dm_map_present(partname, &part_uuid) ?
-					DM_DEVICE_RELOAD : DM_DEVICE_CREATE);
+				op = (dm_find_part(mapname, delim, j + 1, uuid,
+						   partname, sizeof(partname),
+						   &part_uuid, verbose) ?
+				      DM_DEVICE_RELOAD : DM_DEVICE_CREATE);
 
 				if (part_uuid && uuid) {
 					if (check_uuid(uuid, part_uuid, &reason) != 0) {
@@ -603,13 +586,6 @@ main(int argc, char **argv){
 						fprintf(stderr, "Invalid slice %d\n",
 							k);
 
-					if (safe_sprintf(partname, "%s%s%d",
-							 mapname, delim, j+1)) {
-						fprintf(stderr, "partname too small\n");
-						exit(1);
-					}
-					strip_slash(partname);
-
 					if (safe_sprintf(params, "%d:%d %" PRIu64,
 							 major(buf.st_rdev), minor(buf.st_rdev),
 							 slices[j].start)) {
@@ -617,8 +593,10 @@ main(int argc, char **argv){
 						exit(1);
 					}
 
-					op = (dm_map_present(partname,
-							     &part_uuid) ?
+					op = (dm_find_part(mapname, delim, j + 1, uuid,
+							   partname,
+							   sizeof(partname),
+							   &part_uuid, verbose) ?
 					      DM_DEVICE_RELOAD : DM_DEVICE_CREATE);
 
 					if (part_uuid && uuid) {
@@ -660,15 +638,10 @@ main(int argc, char **argv){
 
 			for (j = MAXSLICES-1; j >= 0; j--) {
 				char *part_uuid, *reason;
-				if (safe_sprintf(partname, "%s%s%d",
-					     mapname, delim, j+1)) {
-					fprintf(stderr, "partname too small\n");
-					exit(1);
-				}
-				strip_slash(partname);
-
 				if (slices[j].size ||
-				    !dm_map_present(partname, &part_uuid))
+				    !dm_find_part(mapname, delim, j + 1, uuid,
+						  partname, sizeof(partname),
+						  &part_uuid, verbose))
 					continue;
 
 				if (part_uuid && uuid) {
-- 
2.14.0

[PATCH 04/31] test-kpartx: add tests for renaming functionality

[Martin Wilck]

Add test for the functionality implemented in patch
"kpartx: search partitions by UUID, and rename".

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 kpartx/test-kpartx | 36 ++++++++++++++++++++++++++++++++++++
 1 file changed, 36 insertions(+)

diff --git a/kpartx/test-kpartx b/kpartx/test-kpartx
index 7c45cd14..35b0f064 100755
--- a/kpartx/test-kpartx
+++ b/kpartx/test-kpartx
@@ -207,6 +207,17 @@ usleep $WAIT_US
 [[ -b $SPAN2P1 ]]
 [[ -b $SPAN1P1 ]]
 
+step "rename partitions on DM device to default"
+$KPARTX $KPARTX_OPTS -u /dev/mapper/$SPAN1
+[[ ! -b ${SPAN1P1} ]]
+# This assumes that $SPAN1 ends in a non-digit
+[[ -b ${SPAN1P1//-spam/} ]]
+
+step "rename partitions on DM device back from default"
+$KPARTX $KPARTX_OPTS -u -p -spam /dev/mapper/$SPAN1
+[[ -b ${SPAN1P1} ]]
+[[ ! -b ${SPANP1//-foo/} ]]
+
 step "delete partitions on DM devices"
 $KPARTX $KPARTX_OPTS -d /dev/mapper/$SPAN1 >&2
 usleep $WAIT_US
@@ -223,6 +234,31 @@ usleep $WAIT_US
 [[ -b $LO2P1 ]]
 [[ ! -b $SPAN2P1 ]]
 
+step "rename partitions on loop device"
+$KPARTX $KPARTX_OPTS -u -p -spam $LO2
+[[ ! -b ${LO2P1} ]]
+[[ -b ${LO2P1//-foo/-spam} ]]
+
+step "rename partitions on loop device back"
+$KPARTX $KPARTX_OPTS -u -p -foo $LO2
+[[ -b ${LO2P1} ]]
+[[ ! -b ${LO2P1//-foo/-spam} ]]
+
+step "rename partitions on loop device to default"
+$KPARTX $KPARTX_OPTS -u $LO2
+#read a
+[[ ! -b ${LO2P1} ]]
+# $LO1 ends in a digit
+[[ -b ${LO2P1//-foo/p} ]]
+
+step "rename partitions on loop device back from default"
+$KPARTX $KPARTX_OPTS -u -p -foo $LO2
+[[ -b ${LO2P1} ]]
+[[ ! -b ${LO2P1//-foo/p} ]]
+
+step "rename partitions on loop devices"
+$KPARTX $KPARTX_OPTS -u -p spam $LO2
+
 step "delete partitions on loop devices"
 
 $KPARTX $KPARTX_OPTS -d $LO3
-- 
2.14.0

[PATCH 05/31] kpartx: fix a corner case when renaming partitions

[Martin Wilck]

Fix a pathological issue that I discovered with test-kpartx.
When a dm device is named like a device node (e.g. dm name "loop1"),
kpartx would erroneously pick the dm device if asked to work on
partitions on the other device. Fortunately we can use the UUID
scheme created earlier to see whether or not the device is a dm device.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 kpartx/devmapper.c | 44 +++++++++++++++++++++++++++++++++++++++++++-
 kpartx/devmapper.h | 13 +++++++++++++
 kpartx/kpartx.c    | 25 -------------------------
 3 files changed, 56 insertions(+), 26 deletions(-)

diff --git a/kpartx/devmapper.c b/kpartx/devmapper.c
index 4aca0e51..4469fa84 100644
--- a/kpartx/devmapper.c
+++ b/kpartx/devmapper.c
@@ -668,7 +668,12 @@ int dm_find_part(const char *parent, const char *delim, int part,
 	if (dm_type(tmp, "linear") != 1)
 		goto out;
 
-	if (dm_devn(parent, &major, &minor))
+	/*
+	 * Try nondm uuid first. That way we avoid confusing
+	 * a device name with a device mapper name.
+	 */
+	if (!nondm_parse_uuid(parent_uuid, &major, &minor) &&
+	    dm_devn(parent, &major, &minor))
 		goto out;
 	snprintf(dev_t, sizeof(dev_t), "%d:%d", major, minor);
 
@@ -693,3 +698,40 @@ out:
 	free((void*)tmp);
 	return r;
 }
+
+char *nondm_create_uuid(dev_t devt)
+{
+#define NONDM_UUID_BUFLEN (34 + sizeof(NONDM_UUID_PREFIX) + \
+			   sizeof(NONDM_UUID_SUFFIX))
+	static char uuid_buf[NONDM_UUID_BUFLEN];
+	snprintf(uuid_buf, sizeof(uuid_buf), "%s_%u:%u_%s",
+		 NONDM_UUID_PREFIX, major(devt), minor(devt),
+		 NONDM_UUID_SUFFIX);
+	uuid_buf[NONDM_UUID_BUFLEN-1] = '\0';
+	return uuid_buf;
+}
+
+int nondm_parse_uuid(const char *uuid, int *major, int *minor)
+{
+	const char *p;
+	char *e;
+	int ma, mi;
+
+	if (strncmp(uuid, NONDM_UUID_PREFIX "_", sizeof(NONDM_UUID_PREFIX)))
+		return 0;
+	p = uuid + sizeof(NONDM_UUID_PREFIX);
+	ma = strtoul(p, &e, 10);
+	if (e == p || *e != ':')
+		return 0;
+	p = e + 1;
+	mi = strtoul(p, &e, 10);
+	if (e == p || *e != '_')
+		return 0;
+	p = e + 1;
+	if (strcmp(p, NONDM_UUID_SUFFIX))
+		return 0;
+
+	*major = ma;
+	*minor = mi;
+	return 1;
+}
diff --git a/kpartx/devmapper.h b/kpartx/devmapper.h
index 0ce4b0d5..73b80f2f 100644
--- a/kpartx/devmapper.h
+++ b/kpartx/devmapper.h
@@ -22,4 +22,17 @@ int dm_find_part(const char *parent, const char *delim, int part,
 		 const char *parent_uuid,
 		 char *name, size_t namesiz, char **part_uuid, int verbose);
 
+/*
+ * UUID format for partitions created on non-DM devices
+ * ${UUID_PREFIX}devnode_${MAJOR}:${MINOR}_${NONDM_UUID_SUFFIX}"
+ * where ${UUID_PREFIX} is "part${PARTNO}-" (see devmapper.c).
+ *
+ * The suffix should be sufficiently unique to avoid incidental conflicts;
+ * the value below is a base64-encoded random number.
+ * The UUID format shouldn't be changed between kpartx releases.
+ */
+#define NONDM_UUID_PREFIX "devnode"
+#define NONDM_UUID_SUFFIX "Wh5pYvM"
+char *nondm_create_uuid(dev_t devt);
+int nondm_parse_uuid(const char *uuid, int *major, int *minor);
 #endif /* _KPARTX_DEVMAPPER_H */
diff --git a/kpartx/kpartx.c b/kpartx/kpartx.c
index ad486afd..62f54308 100644
--- a/kpartx/kpartx.c
+++ b/kpartx/kpartx.c
@@ -60,31 +60,6 @@ struct pt {
 int ptct = 0;
 int udev_sync = 1;
 
-/*
- * UUID format for partitions created on non-DM devices
- * ${UUID_PREFIX}devnode_${MAJOR}:${MINOR}_${NONDM_UUID_SUFFIX}"
- * where ${UUID_PREFIX} is "part${PARTNO}-" (see devmapper.c).
- *
- * The suffix should be sufficiently unique to avoid incidental conflicts;
- * the value below is a base64-encoded random number.
- * The UUID format shouldn't be changed between kpartx releases.
- */
-#define NONDM_UUID_PREFIX "devnode"
-#define NONDM_UUID_SUFFIX "Wh5pYvM"
-
-static char *
-nondm_create_uuid(dev_t devt)
-{
-#define NONDM_UUID_BUFLEN (34 + sizeof(NONDM_UUID_PREFIX) + \
-			   sizeof(NONDM_UUID_SUFFIX))
-	static char uuid_buf[NONDM_UUID_BUFLEN];
-	snprintf(uuid_buf, sizeof(uuid_buf), "%s_%u:%u_%s",
-		 NONDM_UUID_PREFIX, major(devt), minor(devt),
-		 NONDM_UUID_SUFFIX);
-	uuid_buf[NONDM_UUID_BUFLEN-1] = '\0';
-	return uuid_buf;
-}
-
 static void
 addpts(char *t, ptreader f)
 {
-- 
2.14.0

[PATCH 06/31] kpartx: fix part deletion without partition table

[Martin Wilck]

If we are deleting partition mappings, there's no need to
parse the partition table. We just look for mappings created
by kpartx and destroy them. Without this patch, kpartx fails
to delete partition mappings on devices on which the partition
table has been destroyed.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 kpartx/kpartx.c | 31 +++++++++++++++++--------------
 1 file changed, 17 insertions(+), 14 deletions(-)

diff --git a/kpartx/kpartx.c b/kpartx/kpartx.c
index 62f54308..442b6bd9 100644
--- a/kpartx/kpartx.c
+++ b/kpartx/kpartx.c
@@ -394,6 +394,22 @@ main(int argc, char **argv){
 
 	/* add/remove partitions to the kernel devmapper tables */
 	int r = 0;
+
+	if (what == DELETE) {
+		r = dm_remove_partmaps(mapname, uuid, buf.st_rdev,
+				       verbose);
+		if (loopdev) {
+			if (del_loop(loopdev)) {
+				if (verbose)
+					fprintf(stderr, "can't del loop : %s\n",
+					       loopdev);
+				r = 1;
+			} else
+				fprintf(stderr, "loop deleted : %s\n", loopdev);
+		}
+		goto end;
+	}
+
 	for (i = 0; i < ptct; i++) {
 		ptp = &pts[i];
 
@@ -461,20 +477,6 @@ main(int argc, char **argv){
 
 			break;
 
-		case DELETE:
-			r = dm_remove_partmaps(mapname, uuid, buf.st_rdev,
-					       verbose);
-			if (loopdev) {
-				if (del_loop(loopdev)) {
-					if (verbose)
-						printf("can't del loop : %s\n",
-						       loopdev);
-					exit(1);
-				}
-				printf("loop deleted : %s\n", loopdev);
-			}
-			break;
-
 		case ADD:
 		case UPDATE:
 			/* ADD and UPDATE share the same code that adds new partitions. */
@@ -656,6 +658,7 @@ main(int argc, char **argv){
 		printf("loop deleted : %s\n", device);
 	}
 
+end:
 	dm_lib_release();
 	dm_lib_exit();
 
-- 
2.14.0

[PATCH 07/31] test-kpartx: test deletion with empty part table

[Martin Wilck]

Added a test case for "kpartx: fix part deletion without partition
table".

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 kpartx/test-kpartx | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/kpartx/test-kpartx b/kpartx/test-kpartx
index 35b0f064..60b3eb23 100755
--- a/kpartx/test-kpartx
+++ b/kpartx/test-kpartx
@@ -68,6 +68,10 @@ mk_partitions() {
     parted -s -- $1 mkpart prim ext2 1MiB -1s
 }
 
+wipe_ptable() {
+    dd if=/dev/zero of=$1 bs=1b count=1
+}
+
 step preparation
 
 [[ $UID -eq 0 ]]
@@ -165,8 +169,18 @@ mk_partitions $LO2
 # Test invocation of kpartx with regular file here
 LO2P1=/dev/mapper/$(basename $LO2)-foo1
 $KPARTX $KPARTX_OPTS -a -p -foo $FILE2
+[[ -b $LO2P1 ]]
 push_cleanup 'dmsetup remove -f $(basename $LO2P1)'
 
+step "remove partitions with deleted ptable"
+wipe_ptable $LO2
+$KPARTX $KPARTX_OPTS -d $LO2
+[[ ! -b $LO2P1 ]]
+
+mk_partitions $LO2
+$KPARTX $KPARTX_OPTS -a -p -foo $FILE2
+[[ -b $LO2P1 ]]
+
 LO1P1=/dev/mapper/$(basename $LO1)-eggs1
 $KPARTX $KPARTX_OPTS -a -p -eggs $LO1
 push_cleanup 'dmsetup remove -f $(basename $LO1P1)'
-- 
2.14.0

[PATCH 08/31] kpartx: only recognize dasd part table on DASD

[Martin Wilck]

The code for reading DASD partition tables is so generic
that it will pretend to find a partition even on a totally
zeroed disk. Prevent that by recognizing dasd partition tables
only on DASD disks. Such a check was already present for DM
mappings on DASD, but (strangely) not for DASD itself.

Without this, kpartx will (try to) create a partition mapping
on a loop device with no partition table.

Found this during testing because test-kpartx unexpectedly
succeeded without my "fix part deletion without partition table" fix
when run on a loop device. That was caused by this bug pretending an
existing partition table although there was none.

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

diff --git a/kpartx/dasd.c b/kpartx/dasd.c
index f50c1bdb..e418d5ac 100644
--- a/kpartx/dasd.c
+++ b/kpartx/dasd.c
@@ -133,6 +133,9 @@ read_dasd_pt(int fd, struct slice all, struct slice *sp, int ns)
 			/* Couldn't open the device */
 			return -1;
 		}
+	} else if ((unsigned int)major(sbuf.st_rdev) != 94) {
+			/* Not a DASD */
+			return -1;
 	} else {
 		fd_dasd = fd;
 	}
-- 
2.14.0

[PATCH 09/31] libmultipath: support MPATH_UDEV_NO_PATHS_FLAG on map creation

[Martin Wilck]

Some vendor kernels (e.g. SUSE) have supported loading multipath
maps without valid paths for a long time. Without that feature,
problems can occur in failover scenarios when multipathd tries
to (re)load maps after device failure/removal, because multipathd's
attempts to reload the configuration may fail unnecessarily.
The discussion in the kernel community is ongoing
(see e.g. https://patchwork.kernel.org/patch/4579551/).

One corner case of this is creation of a map with only failed
paths. Such maps can be created if the kernel patch mentioned above
is applied. The current udev rules for dm-multipath can't detect
this situation. This patch fixes that by setting
DM_SUBSYSTEM_UDEV_FLAG2, which is already used for the "map reload"
case with no valid paths. Thus no additional udev rules are required
to detect this situation.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/devmapper.c | 30 ++++++++++++++++++++----------
 1 file changed, 20 insertions(+), 10 deletions(-)

diff --git a/libmultipath/devmapper.c b/libmultipath/devmapper.c
index e701e806..7a3390a7 100644
--- a/libmultipath/devmapper.c
+++ b/libmultipath/devmapper.c
@@ -300,12 +300,14 @@ dm_device_remove (const char *name, int needsync, int deferred_remove) {
 
 static int
 dm_addmap (int task, const char *target, struct multipath *mpp,
-	   char * params, int ro, int skip_kpartx) {
+	   char * params, int ro, uint16_t udev_flags) {
 	int r = 0;
 	struct dm_task *dmt;
 	char *prefixed_uuid = NULL;
 	uint32_t cookie = 0;
-	uint16_t udev_flags = DM_UDEV_DISABLE_LIBRARY_FALLBACK | ((skip_kpartx == SKIP_KPARTX_ON)? MPATH_UDEV_NO_KPARTX_FLAG : 0);
+
+	/* Need to add this here to allow 0 to be passed in udev_flags */
+	udev_flags |= DM_UDEV_DISABLE_LIBRARY_FALLBACK;
 
 	if (!(dmt = libmp_dm_task_create (task)))
 		return 0;
@@ -371,15 +373,27 @@ addout:
 	return r;
 }
 
+static uint16_t build_udev_flags(const struct multipath *mpp, int reload)
+{
+	/* DM_UDEV_DISABLE_LIBRARY_FALLBACK is added in dm_addmap */
+	return	(mpp->skip_kpartx == SKIP_KPARTX_ON ?
+		 MPATH_UDEV_NO_KPARTX_FLAG : 0) |
+		(mpp->nr_active == 0 ?
+		 MPATH_UDEV_NO_PATHS_FLAG : 0) |
+		(reload && !mpp->force_udev_reload ?
+		 MPATH_UDEV_RELOAD_FLAG : 0);
+}
+
 int dm_addmap_create (struct multipath *mpp, char * params)
 {
 	int ro;
+	uint16_t udev_flags = build_udev_flags(mpp, 0);
 
 	for (ro = 0; ro <= 1; ro++) {
 		int err;
 
 		if (dm_addmap(DM_DEVICE_CREATE, TGT_MPATH, mpp, params, ro,
-			      mpp->skip_kpartx))
+			      udev_flags))
 			return 1;
 		/*
 		 * DM_DEVICE_CREATE is actually DM_DEV_CREATE + DM_TABLE_LOAD.
@@ -405,11 +419,7 @@ int dm_addmap_create (struct multipath *mpp, char * params)
 int dm_addmap_reload(struct multipath *mpp, char *params, int flush)
 {
 	int r = 0;
-	uint16_t udev_flags = ((mpp->force_udev_reload)?
-			       0 : MPATH_UDEV_RELOAD_FLAG) |
-			      ((mpp->skip_kpartx == SKIP_KPARTX_ON)?
-			       MPATH_UDEV_NO_KPARTX_FLAG : 0) |
-			      ((mpp->nr_active)? 0 : MPATH_UDEV_NO_PATHS_FLAG);
+	uint16_t udev_flags = build_udev_flags(mpp, 1);
 
 	/*
 	 * DM_DEVICE_RELOAD cannot wait on a cookie, as
@@ -419,12 +429,12 @@ int dm_addmap_reload(struct multipath *mpp, char *params, int flush)
 	 */
 	if (!mpp->force_readonly)
 		r = dm_addmap(DM_DEVICE_RELOAD, TGT_MPATH, mpp, params,
-			      ADDMAP_RW, SKIP_KPARTX_OFF);
+			      ADDMAP_RW, 0);
 	if (!r) {
 		if (!mpp->force_readonly && errno != EROFS)
 			return 0;
 		r = dm_addmap(DM_DEVICE_RELOAD, TGT_MPATH, mpp,
-			      params, ADDMAP_RO, SKIP_KPARTX_OFF);
+			      params, ADDMAP_RO, 0);
 	}
 	if (r)
 		r = dm_simplecmd(DM_DEVICE_RESUME, mpp->alias, !flush,
-- 
2.14.0

[PATCH 10/31] libmultipath: add get_udev_device

[Martin Wilck]

This factors out the functionality of retrieving the
udev_device from different device types from get_refwwid.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/configure.c | 35 +++++++++++++++++++++++++++++++++++
 libmultipath/configure.h |  1 +
 2 files changed, 36 insertions(+)

diff --git a/libmultipath/configure.c b/libmultipath/configure.c
index 74b6f52a..7326132b 100644
--- a/libmultipath/configure.c
+++ b/libmultipath/configure.c
@@ -1106,6 +1106,41 @@ int coalesce_paths (struct vectors * vecs, vector newmp, char * refwwid,
 	return 0;
 }
 
+struct udev_device *get_udev_device(const char *dev, enum devtypes dev_type)
+{
+	struct udev_device *ud = NULL;
+	const char *base;
+
+	if (dev == NULL || *dev == '\0')
+		return NULL;
+
+	switch (dev_type) {
+	case DEV_DEVNODE:
+	case DEV_DEVMAP:
+		/* This should be GNU basename, compiler will warn if not */
+		base = basename(dev);
+		if (*base == '\0')
+			break;
+		ud = udev_device_new_from_subsystem_sysname(udev, "block",
+							    base);
+		break;
+	case DEV_DEVT:
+		ud = udev_device_new_from_devnum(udev, 'b', parse_devt(dev));
+		break;
+	case DEV_UEVENT:
+		ud = udev_device_new_from_environment(udev);
+		break;
+	default:
+		condlog(0, "Internal error: get_udev_device called with invalid type %d\n",
+			dev_type);
+		break;
+	}
+	if (ud == NULL)
+		condlog(2, "get_udev_device: failed to look up %s with type %d",
+			dev, dev_type);
+	return ud;
+}
+
 /*
  * returns:
  * 0 - success
diff --git a/libmultipath/configure.h b/libmultipath/configure.h
index fd7f581d..0ffc28ef 100644
--- a/libmultipath/configure.h
+++ b/libmultipath/configure.h
@@ -36,3 +36,4 @@ int get_refwwid (enum mpath_cmds cmd, char * dev, enum devtypes dev_type,
 		 vector pathvec, char **wwid);
 int reload_map(struct vectors *vecs, struct multipath *mpp, int refresh, int is_daemon);
 int sysfs_get_host_adapter_name(struct path *pp, char *adapter_name);
+struct udev_device *get_udev_device(const char *dev, enum devtypes dev_type);
-- 
2.14.0

[PATCH 11/31] libmultipath: get_refwwid: use get_udev_device

[Martin Wilck]

Call the factored-out function to obtain the udev device.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/configure.c | 23 +++++++++++------------
 1 file changed, 11 insertions(+), 12 deletions(-)

diff --git a/libmultipath/configure.c b/libmultipath/configure.c
index 7326132b..7a3db318 100644
--- a/libmultipath/configure.c
+++ b/libmultipath/configure.c
@@ -1176,12 +1176,12 @@ int get_refwwid(enum mpath_cmds cmd, char *dev, enum devtypes dev_type,
 
 		pp = find_path_by_dev(pathvec, buff);
 		if (!pp) {
-			struct udev_device *udevice = udev_device_new_from_subsystem_sysname(udev, "block", buff);
+			struct udev_device *udevice =
+				get_udev_device(buff, dev_type);
 
-			if (!udevice) {
-				condlog(2, "%s: can't get udev device", buff);
+			if (!udevice)
 				return 1;
-			}
+
 			conf = get_multipath_config();
 			ret = store_pathinfo(pathvec, conf, udevice,
 					     flags, &pp);
@@ -1214,12 +1214,12 @@ int get_refwwid(enum mpath_cmds cmd, char *dev, enum devtypes dev_type,
 		}
 		pp = find_path_by_dev(pathvec, buff);
 		if (!pp) {
-			struct udev_device *udevice = udev_device_new_from_devnum(udev, 'b', parse_devt(dev));
+			struct udev_device *udevice =
+				get_udev_device(dev, dev_type);
 
-			if (!udevice) {
-				condlog(2, "%s: can't get udev device", dev);
+			if (!udevice)
 				return 1;
-			}
+
 			conf = get_multipath_config();
 			ret = store_pathinfo(pathvec, conf, udevice,
 					     flags, &pp);
@@ -1244,12 +1244,11 @@ int get_refwwid(enum mpath_cmds cmd, char *dev, enum devtypes dev_type,
 	}
 
 	if (dev_type == DEV_UEVENT) {
-		struct udev_device *udevice = udev_device_new_from_environment(udev);
+		struct udev_device *udevice = get_udev_device(dev, dev_type);
 
-		if (!udevice) {
-			condlog(2, "%s: can't get udev device", dev);
+		if (!udevice)
 			return 1;
-		}
+
 		conf = get_multipath_config();
 		ret = store_pathinfo(pathvec, conf, udevice,
 				     flags, &pp);
-- 
2.14.0

[PATCH 12/31] libmultipath: use const char* in some dm helpers

[Martin Wilck]

Changing dm_get_info and dm_get_uuid for now.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/devmapper.c | 4 ++--
 libmultipath/devmapper.h | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/libmultipath/devmapper.c b/libmultipath/devmapper.c
index 7a3390a7..6e3abe3e 100644
--- a/libmultipath/devmapper.c
+++ b/libmultipath/devmapper.c
@@ -553,7 +553,7 @@ uuidout:
 	return r;
 }
 
-int dm_get_uuid(char *name, char *uuid)
+int dm_get_uuid(const char *name, char *uuid)
 {
 	if (dm_get_prefixed_uuid(name, uuid))
 		return 1;
@@ -1308,7 +1308,7 @@ alloc_dminfo (void)
 }
 
 int
-dm_get_info (char * mapname, struct dm_info ** dmi)
+dm_get_info (const char * mapname, struct dm_info ** dmi)
 {
 	if (!mapname)
 		return 1;
diff --git a/libmultipath/devmapper.h b/libmultipath/devmapper.h
index 99a554b5..5c824a21 100644
--- a/libmultipath/devmapper.h
+++ b/libmultipath/devmapper.h
@@ -59,8 +59,8 @@ int dm_get_major_minor (const char *name, int *major, int *minor);
 char * dm_mapname(int major, int minor);
 int dm_remove_partmaps (const char * mapname, int need_sync,
 			int deferred_remove);
-int dm_get_uuid(char *name, char *uuid);
-int dm_get_info (char * mapname, struct dm_info ** dmi);
+int dm_get_uuid(const char *name, char *uuid);
+int dm_get_info (const char * mapname, struct dm_info ** dmi);
 int dm_rename (const char * old, char * new, char * delim, int skip_kpartx);
 int dm_reassign(const char * mapname);
 int dm_reassign_table(const char *name, char *old, char *new);
-- 
2.14.0

[PATCH 13/31] libmultipath: add DI_NOIO flag for pathinfo

[Martin Wilck]

This flag can used to avoid any IO on the device itself. Useful
for getting the path state without the risk of hanging or running
into IO errors.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/discovery.c | 8 ++++++++
 libmultipath/discovery.h | 2 ++
 2 files changed, 10 insertions(+)

diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
index 3a912d76..0ff2faa7 100644
--- a/libmultipath/discovery.c
+++ b/libmultipath/discovery.c
@@ -1926,6 +1926,14 @@ int pathinfo(struct path *pp, struct config *conf, int mask)
 	path_state = path_offline(pp);
 	if (path_state == PATH_REMOVED)
 		goto blank;
+	else if (mask & DI_NOIO) {
+		/*
+		 * Avoid any IO on the device itself.
+		 * Behave like DI_CHECKER in the "path unavailable" case.
+		 */
+		pp->chkrstate = pp->state = path_state;
+		return PATHINFO_OK;
+	}
 
 	/*
 	 * fetch info not available through sysfs
diff --git a/libmultipath/discovery.h b/libmultipath/discovery.h
index 51c23d6f..65352881 100644
--- a/libmultipath/discovery.h
+++ b/libmultipath/discovery.h
@@ -62,6 +62,7 @@ enum discovery_mode {
 	__DI_PRIO,
 	__DI_WWID,
 	__DI_BLACKLIST,
+	__DI_NOIO,
 };
 
 #define DI_SYSFS	(1 << __DI_SYSFS)
@@ -70,6 +71,7 @@ enum discovery_mode {
 #define DI_PRIO		(1 << __DI_PRIO)
 #define DI_WWID		(1 << __DI_WWID)
 #define DI_BLACKLIST	(1 << __DI_BLACKLIST)
+#define DI_NOIO		(1 << __DI_NOIO) /* Avoid IO on the device */
 
 #define DI_ALL		(DI_SYSFS  | DI_SERIAL | DI_CHECKER | DI_PRIO | \
 			 DI_WWID)
-- 
2.14.0

[PATCH 14/31] libmultipath: add dm_get_multipath

[Martin Wilck]

This function is simply factored out from dm_get_maps.
No functional difference.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/devmapper.c | 43 +++++++++++++++++++++++++++----------------
 libmultipath/devmapper.h |  1 +
 2 files changed, 28 insertions(+), 16 deletions(-)

diff --git a/libmultipath/devmapper.c b/libmultipath/devmapper.c
index 6e3abe3e..fcac6bc7 100644
--- a/libmultipath/devmapper.c
+++ b/libmultipath/devmapper.c
@@ -1004,6 +1004,31 @@ dm_disablegroup(char * mapname, int index)
 	return dm_groupmsg("disable", mapname, index);
 }
 
+struct multipath *dm_get_multipath(const char *name)
+{
+	struct multipath *mpp = NULL;
+
+	mpp = alloc_multipath();
+	if (!mpp)
+		return NULL;
+
+	mpp->alias = STRDUP(name);
+
+	if (!mpp->alias)
+		goto out;
+
+	if (dm_get_map(name, &mpp->size, NULL))
+		goto out;
+
+	dm_get_uuid(name, mpp->wwid);
+	dm_get_info(name, &mpp->dmi);
+
+	return mpp;
+out:
+	free_multipath(mpp, KEEP_PATHS);
+	return NULL;
+}
+
 int
 dm_get_maps (vector mp)
 {
@@ -1036,24 +1061,12 @@ dm_get_maps (vector mp)
 		if (!dm_is_mpath(names->name))
 			goto next;
 
-		mpp = alloc_multipath();
-
+		mpp = dm_get_multipath(names->name);
 		if (!mpp)
 			goto out;
 
-		mpp->alias = STRDUP(names->name);
-
-		if (!mpp->alias)
-			goto out1;
-
-		if (dm_get_map(names->name, &mpp->size, NULL))
-			goto out1;
-
-		dm_get_uuid(names->name, mpp->wwid);
-		dm_get_info(names->name, &mpp->dmi);
-
 		if (!vector_alloc_slot(mp))
-			goto out1;
+			goto out;
 
 		vector_set_slot(mp, mpp);
 		mpp = NULL;
@@ -1064,8 +1077,6 @@ next:
 
 	r = 0;
 	goto out;
-out1:
-	free_multipath(mpp, KEEP_PATHS);
 out:
 	dm_task_destroy (dmt);
 	return r;
diff --git a/libmultipath/devmapper.h b/libmultipath/devmapper.h
index 5c824a21..62e14d1c 100644
--- a/libmultipath/devmapper.h
+++ b/libmultipath/devmapper.h
@@ -65,6 +65,7 @@ int dm_rename (const char * old, char * new, char * delim, int skip_kpartx);
 int dm_reassign(const char * mapname);
 int dm_reassign_table(const char *name, char *old, char *new);
 int dm_setgeometry(struct multipath *mpp);
+struct multipath *dm_get_multipath(const char *name);
 
 #define VERSION_GE(v, minv) ( \
 	(v[0] > minv[0]) || \
-- 
2.14.0

[PATCH 15/31] multipath: implement "check usable paths" (-C/-U)

[Martin Wilck]

When we process udev rules, it's crucial to know whether I/O on a given
device will succeed. Unfortunately DM_NR_VALID_PATHS is not reliable,
because the kernel path events aren't necessarily received in order, and
even if they are, the number of usable paths may have changed during
udev processing, in particular when there's a lot of load on udev
because many paths are failing or reinstating at the same time.
The latter problem can't be completely avoided, but the closer the
test before the actual "blkid" call, the better.

This patch adds the -C/-U options to multipath to check if a given
map has usable paths. Obviously this command must avoid doing any I/O
on the multipath map itself, thus no checkers are called; only status
from sysfs and dm is collected.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/config.h |  1 +
 multipath/main.c      | 90 +++++++++++++++++++++++++++++++++++++++++++++++++--
 multipath/multipath.8 | 13 +++++++-
 3 files changed, 101 insertions(+), 3 deletions(-)

diff --git a/libmultipath/config.h b/libmultipath/config.h
index ffc69b5f..4aa53da8 100644
--- a/libmultipath/config.h
+++ b/libmultipath/config.h
@@ -34,6 +34,7 @@ enum mpath_cmds {
 	CMD_REMOVE_WWID,
 	CMD_RESET_WWIDS,
 	CMD_ADD_WWID,
+	CMD_USABLE_PATHS,
 };
 
 enum force_reload_types {
diff --git a/multipath/main.c b/multipath/main.c
index dede017e..704c491f 100644
--- a/multipath/main.c
+++ b/multipath/main.c
@@ -117,6 +117,7 @@ usage (char * progname)
 		"  -F      flush all multipath device maps\n"
 		"  -a      add a device wwid to the wwids file\n"
 		"  -c      check if a device should be a path in a multipath device\n"
+		"  -C      check if a multipath device has usable paths\n"
 		"  -q      allow queue_if_no_path when multipathd is not running\n"
 		"  -d      dry run, do not create or update devmaps\n"
 		"  -t      dump internal hardware table\n"
@@ -258,6 +259,81 @@ get_dm_mpvec (enum mpath_cmds cmd, vector curmp, vector pathvec, char * refwwid)
 	return 0;
 }
 
+static int check_usable_paths(struct config *conf,
+			      const char *devpath, enum devtypes dev_type)
+{
+	struct udev_device *ud = NULL;
+	struct multipath *mpp = NULL;
+	struct pathgroup *pg;
+	struct path *pp;
+	char *mapname;
+	vector pathvec = NULL;
+	char params[PARAMS_SIZE], status[PARAMS_SIZE];
+	dev_t devt;
+	int r = 1, i, j;
+
+	ud = get_udev_device(devpath, dev_type);
+	if (ud == NULL)
+		return r;
+
+	devt = udev_device_get_devnum(ud);
+	if (!dm_is_dm_major(major(devt))) {
+		condlog(1, "%s is not a dm device", devpath);
+		goto out;
+	}
+
+	mapname = dm_mapname(major(devt), minor(devt));
+	if (mapname == NULL) {
+		condlog(1, "dm device not found: %s", devpath);
+		goto out;
+	}
+
+	if (!dm_is_mpath(mapname)) {
+		condlog(1, "%s is not a multipath map", devpath);
+		goto free;
+	}
+
+	/* pathvec is needed for disassemble_map */
+	pathvec = vector_alloc();
+	if (pathvec == NULL)
+		goto free;
+
+	mpp = dm_get_multipath(mapname);
+	if (mpp == NULL)
+		goto free;
+
+	dm_get_map(mpp->alias, &mpp->size, params);
+	dm_get_status(mpp->alias, status);
+	disassemble_map(pathvec, params, mpp, 0);
+	disassemble_status(status, mpp);
+
+	vector_foreach_slot (mpp->pg, pg, i) {
+		vector_foreach_slot (pg->paths, pp, j) {
+			pp->udev = get_udev_device(pp->dev_t, DEV_DEVT);
+			if (pp->udev == NULL)
+				continue;
+			if (pathinfo(pp, conf, DI_SYSFS|DI_NOIO) != PATHINFO_OK)
+				continue;
+
+			if (pp->state == PATH_UP &&
+			    pp->dmstate == PSTATE_ACTIVE) {
+				condlog(3, "%s: path %s is usable",
+					devpath, pp->dev);
+				r = 0;
+				goto found;
+			}
+		}
+	}
+found:
+	condlog(2, "%s:%s usable paths found", devpath, r == 0 ? "" : " no");
+free:
+	FREE(mapname);
+	free_multipath(mpp, FREE_PATHS);
+	vector_free(pathvec);
+out:
+	udev_device_unref(ud);
+	return r;
+}
 
 /*
  * Return value:
@@ -522,7 +598,7 @@ main (int argc, char *argv[])
 		exit(1);
 	multipath_conf = conf;
 	conf->retrigger_tries = 0;
-	while ((arg = getopt(argc, argv, ":adchl::FfM:v:p:b:BrR:itquwW")) != EOF ) {
+	while ((arg = getopt(argc, argv, ":adcChl::FfM:v:p:b:BrR:itquUwW")) != EOF ) {
 		switch(arg) {
 		case 1: printf("optarg : %s\n",optarg);
 			break;
@@ -547,6 +623,9 @@ main (int argc, char *argv[])
 		case 'c':
 			cmd = CMD_VALID_PATH;
 			break;
+		case 'C':
+			cmd = CMD_USABLE_PATHS;
+			break;
 		case 'd':
 			if (cmd == CMD_CREATE)
 				cmd = CMD_DRY_RUN;
@@ -593,6 +672,10 @@ main (int argc, char *argv[])
 			cmd = CMD_VALID_PATH;
 			dev_type = DEV_UEVENT;
 			break;
+		case 'U':
+			cmd = CMD_USABLE_PATHS;
+			dev_type = DEV_UEVENT;
+			break;
 		case 'w':
 			cmd = CMD_REMOVE_WWID;
 			break;
@@ -674,7 +757,10 @@ main (int argc, char *argv[])
 		condlog(0, "failed to initialize prioritizers");
 		goto out;
 	}
-
+	if (cmd == CMD_USABLE_PATHS) {
+		r = check_usable_paths(conf, dev, dev_type);
+		goto out;
+	}
 	if (cmd == CMD_VALID_PATH &&
 	    (!dev || dev_type == DEV_DEVMAP)) {
 		condlog(0, "the -c option requires a path to check");
diff --git a/multipath/multipath.8 b/multipath/multipath.8
index b9436e52..a47a027d 100644
--- a/multipath/multipath.8
+++ b/multipath/multipath.8
@@ -25,7 +25,7 @@ multipath \- Device mapper target autoconfig.
 .RB [\| \-b\ \c
 .IR bindings_file \|]
 .RB [\| \-d \|]
-.RB [\| \-h | \-l | \-ll | \-f | \-t | \-F | \-B | \-c | \-q | \|-r | \|-i | \-a | \|-u | \-w | \-W \|]
+.RB [\| \-h | \-l | \-ll | \-f | \-t | \-F | \-B | \-c | \-C | \-q | \-r | \-i | \-a | \-u | \-U | \-w | \-W \|]
 .RB [\| \-p\ \c
 .IR failover | multibus | group_by_serial | group_by_prio | group_by_node_name \|]
 .RB [\| \-R\ \c
@@ -110,6 +110,12 @@ Set user_friendly_names bindings file location.  The default is
 Check if a block device should be a path in a multipath device.
 .
 .TP
+.B \-C
+Check if a multipath device has usable paths. This can be used to
+test whether or not I/O on this device is likely to succeed. The command
+itself doesn't attempt to do I/O on the device.
+.
+.TP
 .B \-q
 Allow device tables with \fIqueue_if_no_path\fR when multipathd is not running.
 .
@@ -123,6 +129,11 @@ Check if the device specified in the program environment should be
 a path in a multipath device.
 .
 .TP
+.B \-U
+Check if the device specified in the program environment is a multipath device
+with usable paths. See \fB-C\fB.
+.
+.TP
 .B \-w
 Remove the WWID for the specified device from the WWIDs file.
 .
-- 
2.14.0

[PATCH 16/31] 11-dm-mpath.rules: multipath -U for READY check

[Martin Wilck]

For kernel-generated path events, DM_NR_VALID_PATHS indicates
whether usable paths are available. But this information isn't
reliable as events may be received out of order.

Use multipath -U to determine whether a multipath map can
handle I/O.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 multipath/11-dm-mpath.rules | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/multipath/11-dm-mpath.rules b/multipath/11-dm-mpath.rules
index b131a103..d1ef85e5 100644
--- a/multipath/11-dm-mpath.rules
+++ b/multipath/11-dm-mpath.rules
@@ -18,8 +18,19 @@ ENV{DM_SUBSYSTEM_UDEV_FLAG2}=="1", ENV{MPATH_DEVICE_READY}="0",\
 	GOTO="mpath_action"
 
 # If the last path has failed mark the device not ready
-ENV{DM_ACTION}=="PATH_FAILED", ENV{DM_NR_VALID_PATHS}=="0",\
-	ENV{MPATH_DEVICE_READY}="0", GOTO="mpath_action"
+# Note that DM_NR_VALID_PATHS is only set for PATH_FAILED|PATH_REINSTATED
+# events.
+# This may not be reliable, as events aren't necessarily received in order.
+ENV{DM_NR_VALID_PATHS}=="0", ENV{MPATH_DEVICE_READY}="0", GOTO="mpath_action"
+
+ENV{MPATH_SBIN_PATH}="/sbin"
+TEST!="$env{MPATH_SBIN_PATH}/multipath", ENV{MPATH_SBIN_PATH}="/usr/sbin"
+
+# Check the map state directly with multipath -U.
+# This doesn't attempt I/O on the device.
+PROGRAM=="$env{MPATH_SBIN_PATH}/multipath -U %k", GOTO="paths_ok"
+ENV{MPATH_DEVICE_READY}="0", GOTO="mpath_action"
+LABEL="paths_ok"
 
 # Don't mark a device ready on a PATH_FAILED event. even if
 # DM_NR_VALID_PATHS is greater than 0. Just keep the existing
-- 
2.14.0

[PATCH 17/31] 11-dm-mpath.rules: import more ID_FS_xxx vars from db

[Martin Wilck]

We have code for importing blkid variables from db but this code
is missing the _ENC variants needed for creating symlinks, and
ID_FS_LABEL.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 multipath/11-dm-mpath.rules | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/multipath/11-dm-mpath.rules b/multipath/11-dm-mpath.rules
index d1ef85e5..2e42a1bd 100644
--- a/multipath/11-dm-mpath.rules
+++ b/multipath/11-dm-mpath.rules
@@ -72,7 +72,9 @@ ENV{DM_NOSCAN}!="1", GOTO="mpath_end"
 ENV{ID_FS_TYPE}!="?*", IMPORT{db}="ID_FS_TYPE"
 ENV{ID_FS_USAGE}!="?*", IMPORT{db}="ID_FS_USAGE"
 ENV{ID_FS_UUID}!="?*", IMPORT{db}="ID_FS_UUID"
-ENV{ID_FS_ENC}!="?*", IMPORT{db}="ID_FS_UUID_ENC"
+ENV{ID_FS_UUID_ENC}!="?*", IMPORT{db}="ID_FS_UUID_ENC"
+ENV{ID_FS_LABEL}!="?*", IMPORT{db}="ID_FS_LABEL"
+ENV{ID_FS_LABEL_ENC}!="?*", IMPORT{db}="ID_FS_LABEL_ENC"
 ENV{ID_FS_VERSION}!="?*", IMPORT{db}="ID_FS_VERSION"
 
 LABEL="mpath_end"
-- 
2.14.0

[PATCH 18/31] 11-dm-mpath.rules: no need to test before IMPORT

[Martin Wilck]

This code is run before 13-dm-disk.rules which calls "blkid",
therefore the code can be simplified because the condition checked
always holds.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 multipath/11-dm-mpath.rules | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/multipath/11-dm-mpath.rules b/multipath/11-dm-mpath.rules
index 2e42a1bd..abf7987b 100644
--- a/multipath/11-dm-mpath.rules
+++ b/multipath/11-dm-mpath.rules
@@ -69,12 +69,12 @@ ENV{MPATH_DEVICE_READY}!="0", ENV{.MPATH_DEVICE_READY_OLD}=="0",\
 
 LABEL="scan_import"
 ENV{DM_NOSCAN}!="1", GOTO="mpath_end"
-ENV{ID_FS_TYPE}!="?*", IMPORT{db}="ID_FS_TYPE"
-ENV{ID_FS_USAGE}!="?*", IMPORT{db}="ID_FS_USAGE"
-ENV{ID_FS_UUID}!="?*", IMPORT{db}="ID_FS_UUID"
-ENV{ID_FS_UUID_ENC}!="?*", IMPORT{db}="ID_FS_UUID_ENC"
-ENV{ID_FS_LABEL}!="?*", IMPORT{db}="ID_FS_LABEL"
-ENV{ID_FS_LABEL_ENC}!="?*", IMPORT{db}="ID_FS_LABEL_ENC"
-ENV{ID_FS_VERSION}!="?*", IMPORT{db}="ID_FS_VERSION"
+IMPORT{db}="ID_FS_TYPE"
+IMPORT{db}="ID_FS_USAGE"
+IMPORT{db}="ID_FS_UUID"
+IMPORT{db}="ID_FS_UUID_ENC"
+IMPORT{db}="ID_FS_LABEL"
+IMPORT{db}="ID_FS_LABEL_ENC"
+IMPORT{db}="ID_FS_VERSION"
 
 LABEL="mpath_end"
-- 
2.14.0

[PATCH 19/31] 11-dm-mpath.rules: handle new maps with READY==0

[Martin Wilck]

We need to distinguish the case where a device came up with
ENV{MPATH_DEVICE_READY}=="0" in the first place from the case
where it changed from "ready" to "not ready".

Otherwise, we may save a wrong state in DM_DISABLE_OTHER_RULES_FLAG_OLD.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 multipath/11-dm-mpath.rules | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/multipath/11-dm-mpath.rules b/multipath/11-dm-mpath.rules
index abf7987b..0be22ae4 100644
--- a/multipath/11-dm-mpath.rules
+++ b/multipath/11-dm-mpath.rules
@@ -39,7 +39,7 @@ ENV{DM_ACTION}=="PATH_FAILED", GOTO="mpath_action"
 
 # This event is either a PATH_REINSTATED or a table reload where
 # there are active paths. Mark the device ready
-ENV{MPATH_DEVICE_READY}=""
+ENV{MPATH_DEVICE_READY}="1"
 
 LABEL="mpath_action"
 # DM_SUBSYSTEM_UDEV_FLAG0 is the "RELOAD" flag for multipath subsystem.
@@ -58,10 +58,10 @@ ENV{MPATH_DEVICE_READY}=="0", ENV{DM_NOSCAN}="1"
 # Also skip all foreign rules if no path is available.
 # Remember the original value of DM_DISABLE_OTHER_RULES_FLAG
 # and restore it back once we have at least one path available.
-ENV{MPATH_DEVICE_READY}=="0", ENV{.MPATH_DEVICE_READY_OLD}!="0",\
+ENV{MPATH_DEVICE_READY}=="0", ENV{.MPATH_DEVICE_READY_OLD}=="1",\
 	ENV{DM_DISABLE_OTHER_RULES_FLAG_OLD}=="",\
-	ENV{DM_DISABLE_OTHER_RULES_FLAG_OLD}="$env{DM_UDEV_DISABLE_OTHER_RULES_FLAG}",\
-	ENV{DM_UDEV_DISABLE_OTHER_RULES_FLAG}="1"
+	ENV{DM_DISABLE_OTHER_RULES_FLAG_OLD}="$env{DM_UDEV_DISABLE_OTHER_RULES_FLAG}"
+ENV{MPATH_DEVICE_READY}=="0", ENV{DM_UDEV_DISABLE_OTHER_RULES_FLAG}="1"
 ENV{MPATH_DEVICE_READY}!="0", ENV{.MPATH_DEVICE_READY_OLD}=="0",\
 	ENV{DM_UDEV_DISABLE_OTHER_RULES_FLAG}="$env{DM_DISABLE_OTHER_RULES_FLAG_OLD}",\
 	ENV{DM_DISABLE_OTHER_RULES_FLAG_OLD}="",\
-- 
2.14.0

[PATCH 20/31] 11-dm-mpath.rules: don't set READY->ACTIVATION

[Martin Wilck]

The fact alone that a map changes from not ready to ready does
not imply that it is activating.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 multipath/11-dm-mpath.rules | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/multipath/11-dm-mpath.rules b/multipath/11-dm-mpath.rules
index 0be22ae4..3f47744f 100644
--- a/multipath/11-dm-mpath.rules
+++ b/multipath/11-dm-mpath.rules
@@ -64,8 +64,7 @@ ENV{MPATH_DEVICE_READY}=="0", ENV{.MPATH_DEVICE_READY_OLD}=="1",\
 ENV{MPATH_DEVICE_READY}=="0", ENV{DM_UDEV_DISABLE_OTHER_RULES_FLAG}="1"
 ENV{MPATH_DEVICE_READY}!="0", ENV{.MPATH_DEVICE_READY_OLD}=="0",\
 	ENV{DM_UDEV_DISABLE_OTHER_RULES_FLAG}="$env{DM_DISABLE_OTHER_RULES_FLAG_OLD}",\
-	ENV{DM_DISABLE_OTHER_RULES_FLAG_OLD}="",\
-	ENV{DM_ACTIVATION}="1"
+	ENV{DM_DISABLE_OTHER_RULES_FLAG_OLD}=""
 
 LABEL="scan_import"
 ENV{DM_NOSCAN}!="1", GOTO="mpath_end"
-- 
2.14.0

[PATCH 21/31] 11-dm-mpath.rules: Remember DM_ACTIVATION

[Martin Wilck]

If DM_ACTIVATION is set by the general dm rules, we need to
bring up this device. But if the mpath device is not ready,
that would be dangerous; it could hang or produce lots of IO
errors. So remember this state, and try to activate when the
map becomes usable later.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 multipath/11-dm-mpath.rules | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/multipath/11-dm-mpath.rules b/multipath/11-dm-mpath.rules
index 3f47744f..9bfd75f8 100644
--- a/multipath/11-dm-mpath.rules
+++ b/multipath/11-dm-mpath.rules
@@ -4,6 +4,7 @@ ENV{DM_UUID}!="mpath-?*", GOTO="mpath_end"
 
 IMPORT{db}="DM_DISABLE_OTHER_RULES_FLAG_OLD"
 IMPORT{db}="MPATH_DEVICE_READY"
+IMPORT{db}="MPATH_NEEDS_ACTIVATION"
 
 # If this uevent didn't come from dm, don't try to update the
 # device state
@@ -55,6 +56,13 @@ ENV{DM_SUBSYSTEM_UDEV_FLAG0}=="1", ENV{DM_ACTIVATION}="0"
 # We'd like to avoid this, especially within udev processing.
 ENV{MPATH_DEVICE_READY}=="0", ENV{DM_NOSCAN}="1"
 
+# If DM_ACTIVATION is set, but can't be satisfied, remember it
+# in MPATH_NEEDS_ACTIVATION, and activate at the next opportunity.
+ENV{MPATH_DEVICE_READY}=="0", ENV{DM_ACTIVATION}=="1", \
+	ENV{MPATH_NEEDS_ACTIVATION}="1", ENV{DM_ACTIVATION}="0"
+ENV{MPATH_DEVICE_READY}!="0", ENV{MPATH_NEEDS_ACTIVATION}=="1", \
+	ENV{DM_ACTIVATION}="1", ENV{MPATH_NEEDS_ACTIVATION}=""
+
 # Also skip all foreign rules if no path is available.
 # Remember the original value of DM_DISABLE_OTHER_RULES_FLAG
 # and restore it back once we have at least one path available.
-- 
2.14.0

[PATCH 22/31] multipath.rules: set ID_FS_TYPE to "mpath_member"

[Martin Wilck]

... for devices that have been identified as such.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 multipath/multipath.rules | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/multipath/multipath.rules b/multipath/multipath.rules
index 86defc0c..bc1a8529 100644
--- a/multipath/multipath.rules
+++ b/multipath/multipath.rules
@@ -21,7 +21,7 @@ TEST!="$env{MPATH_SBIN_PATH}/multipath", ENV{MPATH_SBIN_PATH}="/usr/sbin"
 
 ENV{DM_MULTIPATH_DEVICE_PATH}!="1", \
 	PROGRAM=="$env{MPATH_SBIN_PATH}/multipath -u %k", \
-	ENV{DM_MULTIPATH_DEVICE_PATH}="1", ENV{ID_FS_TYPE}="none", \
+	ENV{DM_MULTIPATH_DEVICE_PATH}="1", ENV{ID_FS_TYPE}="mpath_member", \
 	ENV{SYSTEMD_READY}="0"
 
 LABEL="end_mpath"
-- 
2.14.0

[PATCH 23/31] kpartx.rules: don't rely on DM_DEPS and DM_TABLE_STATE

[Martin Wilck]

These tests (which were SUSE-specific anyway) are not needed
here. The kpartx invocation further down is guarded separately.
SYMLINK actions must be run in any case for consistency, and
there's no need to access the udev db here because kpartx_id
also works on otherwise broken devices.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 kpartx/kpartx.rules | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/kpartx/kpartx.rules b/kpartx/kpartx.rules
index 64d550de..ea34e449 100644
--- a/kpartx/kpartx.rules
+++ b/kpartx/kpartx.rules
@@ -5,12 +5,12 @@
 #
 
 KERNEL!="dm-*", GOTO="kpartx_end"
-ACTION=="remove", GOTO="kpartx_end"
+ACTION!="add|change", GOTO="kpartx_end"
+ENV{DM_UUID}!="?*", GOTO="kpartx_end"
 
-ENV{DM_TABLE_STATE}!="LIVE", GOTO="kpartx_end"
-ENV{DM_DEPS}=="0", GOTO="kpartx_end"
-
-ENV{DM_UUID}=="?*", IMPORT{program}=="kpartx_id %M %m $env{DM_UUID}"
+# kpartx_id is very robust, it works for suspended maps and maps
+# with 0 dependencies
+IMPORT{program}=="kpartx_id %M %m $env{DM_UUID}"
 
 OPTIONS="link_priority=50"
 
-- 
2.14.0

[PATCH 24/31] kpartx.rules: respect DM_UDEV_LOW_PRIORITY_FLAG

[Martin Wilck]

kpartx.rules increased link priority unconditionally, but for
explicitly marked low prio devices that shouldn't be done.
Fix that. Also, use "+=" for OPTIONS, as most other rules do.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 kpartx/kpartx.rules | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kpartx/kpartx.rules b/kpartx/kpartx.rules
index ea34e449..efd21a29 100644
--- a/kpartx/kpartx.rules
+++ b/kpartx/kpartx.rules
@@ -12,7 +12,7 @@ ENV{DM_UUID}!="?*", GOTO="kpartx_end"
 # with 0 dependencies
 IMPORT{program}=="kpartx_id %M %m $env{DM_UUID}"
 
-OPTIONS="link_priority=50"
+ENV{DM_UDEV_LOW_PRIORITY_FLAG}!="1", OPTIONS+="link_priority=50"
 
 ENV{DM_UUID}=="?*", ENV{DM_TYPE}=="?*" \
 	SYMLINK+="disk/by-id/$env{DM_TYPE}-$env{DM_NAME}"
-- 
2.14.0

[PATCH 25/31] kpartx.rules: improved logic for by-uuid and by-label links

[Martin Wilck]

If blkid can be run, these links are already set up by 13-dm-disk.rules.
In the DM_NOSCAN=1 case, they are imported from db in 11-dm-mpath.rules
for multipath maps, but not for partitions (there is currently no flag
for kpartx-generated partitions that indicates whether the device will
be able to do IO).

Moreover, the previous logic for running IMPORT{db} was wrong (condition
used '=="?*"' rather than '!="?*"').

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 kpartx/kpartx.rules | 24 ++++++++++++++++++------
 1 file changed, 18 insertions(+), 6 deletions(-)

diff --git a/kpartx/kpartx.rules b/kpartx/kpartx.rules
index efd21a29..4e5a6d07 100644
--- a/kpartx/kpartx.rules
+++ b/kpartx/kpartx.rules
@@ -25,14 +25,26 @@ ENV{DM_WWN}=="?*", ENV{DM_PART}!="?*", \
 ENV{DM_WWN}=="?*", ENV{DM_PART}=="?*", \
 	SYMLINK+="disk/by-id/wwn-$env{DM_WWN}-part$env{DM_PART}"
 
-# Create persistent by-label/by-uuid links
-ENV{ID_FS_USAGE}=="?*", IMPORT{db}="ID_FS_USAGE"
-ENV{ID_FS_UUID_ENC}=="?*", IMPORT{db}="ID_FS_UUID_ENC"
+# Create persistent by-label/by-uuid links.
+# multipath maps with DM_NOSCAN!=1 are handled in 13-dm-disk.rules.
+DM_UUID=="mpath-*", ENV{DM_NOSCAN}!="1", GOTO="symlink_end"
+
+# For partitions, we don't have DM_NOSCAN.
+# Simply load variables from db if they aren't set.
+# 11-dm-mpath.rules does this for mpath maps.
+# Fixme: we have currently no way to avoid calling blkid on
+# partitions of broken mpath maps.
+ENV{DM_UUID}!="part*-*-*", GOTO="import_end"
+ENV{ID_FS_USAGE}!="?*", IMPORT{db}="ID_FS_USAGE"
+ENV{ID_FS_UUID_ENC}!="?*", IMPORT{db}="ID_FS_UUID_ENC"
+ENV{ID_FS_LABEL_ENC}!="?*", IMPORT{db}="ID_FS_LABEL_ENC"
+LABEL="import_end"
+
 ENV{ID_FS_USAGE}=="filesystem|other|crypto", ENV{ID_FS_UUID_ENC}=="?*", \
-	SYMLINK+="disk/by-uuid/$env{ID_FS_UUID_ENC}"
-ENV{ID_FS_LABEL_ENC}=="?*", IMPORT{db}="ID_FS_LABEL_ENC"
+       SYMLINK+="disk/by-uuid/$env{ID_FS_UUID_ENC}"
 ENV{ID_FS_USAGE}=="filesystem|other", ENV{ID_FS_LABEL_ENC}=="?*", \
-	SYMLINK+="disk/by-label/$env{ID_FS_LABEL_ENC}"
+       SYMLINK+="disk/by-label/$env{ID_FS_LABEL_ENC}"
+LABEL="symlink_end"
 
 # Create dm tables for partitions
 ENV{DM_ACTION}=="PATH_FAILED|PATH_REINSTATED", GOTO="kpartx_end"
-- 
2.14.0

[PATCH 26/31] kpartx.rules: create by-partuuid and by-partlabel symlinks

[Martin Wilck]

The LVM rules do this since 2.02.173 (c48149cf80 "udev: also
create /dev/disk/by-part{label,uuid} and gpt-auto-root symlinks").
We have to do it here for partitions on unaccessible mpath maps.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 kpartx/kpartx.rules | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/kpartx/kpartx.rules b/kpartx/kpartx.rules
index 4e5a6d07..b05181a8 100644
--- a/kpartx/kpartx.rules
+++ b/kpartx/kpartx.rules
@@ -38,12 +38,18 @@ ENV{DM_UUID}!="part*-*-*", GOTO="import_end"
 ENV{ID_FS_USAGE}!="?*", IMPORT{db}="ID_FS_USAGE"
 ENV{ID_FS_UUID_ENC}!="?*", IMPORT{db}="ID_FS_UUID_ENC"
 ENV{ID_FS_LABEL_ENC}!="?*", IMPORT{db}="ID_FS_LABEL_ENC"
+ENV{ID_PART_ENTRY_NAME}!="?*", IMPORT{db}="ID_PART_ENTRY_NAME"
+ENV{ID_PART_ENTRY_UUID}!="?*", IMPORT{db}="ID_PART_ENTRY_UUID"
+ENV{ID_PART_ENTRY_SCHEME}!="?*", IMPORT{db}="ID_PART_ENTRY_SCHEME"
 LABEL="import_end"
 
 ENV{ID_FS_USAGE}=="filesystem|other|crypto", ENV{ID_FS_UUID_ENC}=="?*", \
        SYMLINK+="disk/by-uuid/$env{ID_FS_UUID_ENC}"
 ENV{ID_FS_USAGE}=="filesystem|other", ENV{ID_FS_LABEL_ENC}=="?*", \
        SYMLINK+="disk/by-label/$env{ID_FS_LABEL_ENC}"
+ENV{ID_PART_ENTRY_UUID}=="?*", SYMLINK+="disk/by-partuuid/$env{ID_PART_ENTRY_UUID}"
+ENV{ID_PART_ENTRY_SCHEME}=="gpt", ENV{ID_PART_ENTRY_NAME}=="?*", \
+       SYMLINK+="disk/by-partlabel/$env{ID_PART_ENTRY_NAME}"
 LABEL="symlink_end"
 
 # Create dm tables for partitions
-- 
2.14.0

[PATCH 27/31] kpartx.rules: generate type-name links only for multipath devices

[Martin Wilck]

... and their partitions. This rule generates irritiating symlinks
like /dev/disk/by-id/raid-${VG}-${LV} for LVM LVs otherwise.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 kpartx/kpartx.rules | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kpartx/kpartx.rules b/kpartx/kpartx.rules
index b05181a8..6c8f4d53 100644
--- a/kpartx/kpartx.rules
+++ b/kpartx/kpartx.rules
@@ -14,7 +14,7 @@ IMPORT{program}=="kpartx_id %M %m $env{DM_UUID}"
 
 ENV{DM_UDEV_LOW_PRIORITY_FLAG}!="1", OPTIONS+="link_priority=50"
 
-ENV{DM_UUID}=="?*", ENV{DM_TYPE}=="?*" \
+ENV{DM_UUID}=="*mpath-*", ENV{DM_TYPE}=="?*" \
 	SYMLINK+="disk/by-id/$env{DM_TYPE}-$env{DM_NAME}"
 
 # Create persistent links for multipath tables
-- 
2.14.0

[PATCH 28/31] kpartx.rules: fix logic for adding partitions

[Martin Wilck]

Based on code by Ben Marzinski, this patch updates kpartx.rules.

The main change is that the flags that determine whether scanning is now
possible are DM_NOSCAN and DM_SUSPENDED. This assumes that said flags
are set correctly by 11-dm-mpath.rules.

Note that kpartx can't just be run if DM_ACTIVATION=1; doing so we
would miss events caused e.g. by partition table editing. It's not
necessary to scan for certain events. This will be handled in another
patch.

Currently this works for multipath only, but code for other targets
could be added if desired.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 kpartx/kpartx.rules | 27 +++++++++++++++++++++------
 1 file changed, 21 insertions(+), 6 deletions(-)

diff --git a/kpartx/kpartx.rules b/kpartx/kpartx.rules
index 6c8f4d53..b8141141 100644
--- a/kpartx/kpartx.rules
+++ b/kpartx/kpartx.rules
@@ -52,12 +52,27 @@ ENV{ID_PART_ENTRY_SCHEME}=="gpt", ENV{ID_PART_ENTRY_NAME}=="?*", \
        SYMLINK+="disk/by-partlabel/$env{ID_PART_ENTRY_NAME}"
 LABEL="symlink_end"
 
-# Create dm tables for partitions
-ENV{DM_ACTION}=="PATH_FAILED|PATH_REINSTATED", GOTO="kpartx_end"
-ENV{DM_NR_VALID_PATHS}=="0", GOTO="kpartx_end"
+# Create dm tables for partitions on multipath devices.
+ENV{DM_UUID}!="mpath-?*", GOTO="mpath_kpartx_end"
+
+# DM_SUBSYSTEM_UDEV_FLAG1 is the "skip_kpartx" flag.
+# For events not generated by libdevmapper, we need to fetch it from db.
 ENV{DM_UDEV_PRIMARY_SOURCE_FLAG}!="1", IMPORT{db}="DM_SUBSYSTEM_UDEV_FLAG1"
-ENV{DM_SUBSYSTEM_UDEV_FLAG1}=="1", GOTO="kpartx_end"
-ENV{DM_STATE}!="SUSPENDED", ENV{DM_UUID}=="mpath-*", \
-	RUN+="/sbin/kpartx -un -p -part /dev/$name"
+ENV{DM_SUBSYSTEM_UDEV_FLAG1}=="1", GOTO="mpath_kpartx_end"
+
+# Don't run kpartx now if we know it will fail or hang.
+ENV{DM_SUSPENDED}=="1", GOTO="mpath_kpartx_end"
+ENV{DM_NOSCAN}=="1", GOTO="mpath_kpartx_end"
+
+# Run kpartx
+GOTO="run_kpartx"
+LABEL="mpath_kpartx_end"
+
+## Code for other subsystems (non-multipath) could be placed here ##
+
+GOTO="kpartx_end"
+
+LABEL="run_kpartx"
+RUN+="/sbin/kpartx -un -p -part /dev/$name"
 
 LABEL="kpartx_end"
-- 
2.14.0

[PATCH 29/31] multipath/kpartx rules: avoid superfluous scanning

[Martin Wilck]

Multipath maps receive certain udev events that only indicate
changes in the path list. Use MPATH_UNCHANGED to tell this to
upper layers, so that they can ignore the event.

multipathd-generated events with DM_SUBSYSTEM_UDEV_FLAG0 are one
case, and kernel-generated PATH_FAILED and PATH_REINSTATED events
are another.

The LVM rules rely on DM_ACTIVATION, but other rules such as
kpartx can't do that, because they'd miss e.g. partition table
changes.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 kpartx/kpartx.rules         | 3 +++
 multipath/11-dm-mpath.rules | 8 +++++++-
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/kpartx/kpartx.rules b/kpartx/kpartx.rules
index b8141141..1cbe9429 100644
--- a/kpartx/kpartx.rules
+++ b/kpartx/kpartx.rules
@@ -60,6 +60,9 @@ ENV{DM_UUID}!="mpath-?*", GOTO="mpath_kpartx_end"
 ENV{DM_UDEV_PRIMARY_SOURCE_FLAG}!="1", IMPORT{db}="DM_SUBSYSTEM_UDEV_FLAG1"
 ENV{DM_SUBSYSTEM_UDEV_FLAG1}=="1", GOTO="mpath_kpartx_end"
 
+# 11-dm-mpath.rules sets MPATH_UNCHANGED for events that can be ignored.
+ENV{MPATH_UNCHANGED}=="1", GOTO="mpath_kpartx_end"
+
 # Don't run kpartx now if we know it will fail or hang.
 ENV{DM_SUSPENDED}=="1", GOTO="mpath_kpartx_end"
 ENV{DM_NOSCAN}=="1", GOTO="mpath_kpartx_end"
diff --git a/multipath/11-dm-mpath.rules b/multipath/11-dm-mpath.rules
index 9bfd75f8..4a3f646d 100644
--- a/multipath/11-dm-mpath.rules
+++ b/multipath/11-dm-mpath.rules
@@ -49,7 +49,13 @@ LABEL="mpath_action"
 # something that should be reacted upon since it would be useless extra work.
 # It's exactly mpath's job to provide *seamless* device access to any of the
 # paths that are available underneath.
-ENV{DM_SUBSYSTEM_UDEV_FLAG0}=="1", ENV{DM_ACTIVATION}="0"
+ENV{DM_SUBSYSTEM_UDEV_FLAG0}=="1", \
+	ENV{DM_ACTIVATION}="0", ENV{MPATH_UNCHANGED}="1"
+
+# For path failed or reinstated events, unset DM_ACTIVATION.
+# This is similar to the DM_SUBSYSTEM_UDEV_FLAG0 case above.
+ENV{DM_ACTION}=="PATH_FAILED|PATH_REINSTATED", \
+	ENV{DM_ACTIVATION}="0", ENV{MPATH_UNCHANGED}="1"
 
 # Do not initiate scanning if no path is available,
 # otherwise there would be a hang or IO error on access.
-- 
2.14.0

[PATCH 30/31] kpartx/del-part-nodes.rules: new udev file

[Martin Wilck]

A new udev rules file "68-del-part-nodes.rules" is introduced,
based on code from Ben Marzinki. This code deletes partitions on
multipath member devices. The purpose of this is to avoid users
accidentally accessing partitions of member devices rather than of the
multipath devcice. The deletion is done only once for every disk
device. If the user wants to get the partitions back, he can run "partx
-a" or "partprobe" on the disk device.

This code could be extended to wipe partitions on member devices
of non-multipath dm targets if desired.

Means to deactivate this behavior are provided via kernel parameter
"dont_del_part_nodes", or a custom udev rules file setting the
"DONT_DEL_PART_NODES" environment variable.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 kpartx/Makefile             |  1 +
 kpartx/del-part-nodes.rules | 32 ++++++++++++++++++++++++++++++++
 2 files changed, 33 insertions(+)
 create mode 100644 kpartx/del-part-nodes.rules

diff --git a/kpartx/Makefile b/kpartx/Makefile
index 7b75032e..7f5c1708 100644
--- a/kpartx/Makefile
+++ b/kpartx/Makefile
@@ -30,6 +30,7 @@ install: $(EXEC) $(EXEC).8
 	$(INSTALL_PROGRAM) -m 755 kpartx_id $(DESTDIR)$(libudevdir)
 	$(INSTALL_PROGRAM) -d $(DESTDIR)$(libudevdir)/rules.d
 	$(INSTALL_PROGRAM) -m 644 kpartx.rules $(DESTDIR)$(libudevdir)/rules.d/66-kpartx.rules
+	$(INSTALL_PROGRAM) -m 644 del-part-nodes.rules $(DESTDIR)$(libudevdir)/rules.d/68-del-part-nodes.rules
 	$(INSTALL_PROGRAM) -d $(DESTDIR)$(man8dir)
 	$(INSTALL_PROGRAM) -m 644 $(EXEC).8.gz $(DESTDIR)$(man8dir)
 
diff --git a/kpartx/del-part-nodes.rules b/kpartx/del-part-nodes.rules
new file mode 100644
index 00000000..cee945d9
--- /dev/null
+++ b/kpartx/del-part-nodes.rules
@@ -0,0 +1,32 @@
+# These rules can delete partitions devnodes for slave devices
+# for certain aggregate devices such as multipath.
+# This is desirable to avoid confusion and keep the number
+# of device nodes and symlinks within limits.
+#
+# This is done only once on the first "add" or "change" event for
+# any given device.
+#
+# To suppress this, use the kernel parameter "dont_del_part_nodes",
+# or create an udev rule file that sets ENV{DONT_DEL_PART_NODES}="1".
+
+SUBSYSTEM!="block", GOTO="end_del_part_nodes"
+KERNEL!="sd*|dasd*|rbd*", GOTO="end_del_part_nodes"
+ACTION!="add|change", GOTO="end_del_part_nodes"
+
+IMPORT{cmdline}="dont_del_part_nodes"
+ENV{dont_del_part_nodes}=="1", GOTO="end_del_part_nodes"
+ENV{DONT_DEL_PART_NODES}=="1", GOTO="end_del_part_nodes"
+
+# dm-multipath
+ENV{DM_MULTIPATH_DEVICE_PATH}=="1", GOTO="del_part_nodes"
+
+# Other aggregate device types can be added here.
+
+GOTO="end_del_part_nodes"
+
+LABEL="del_part_nodes"
+IMPORT{db}="DM_DEL_PART_NODES"
+ENV{DM_DEL_PART_NODES}!="1", ENV{DM_DEL_PART_NODES}="1", \
+	RUN+="/usr/sbin/partx -d --nr 1-1024 $env{DEVNAME}"
+
+LABEL="end_del_part_nodes"
-- 
2.14.0

[PATCH 31/31] kpartx.rules: move symlink code to other files

[Martin Wilck]

Current kpartx.rules combines two purposes: setting properties and
creating symlinks for dm partition devices, and creating such
partition devices on top of other devices. This is contrary to
common conventions for udev rules files.

This patch moves the code for properties and symlinks into other
files. The code that generates symlinks for multipath maps is moved
to 11-dm-mpath.rules, and for partitions we introduce a new file
11-dm-parts.rules. Necessarily this results in minor code duplication.
OTOH quite some code is removed because the properties are now set
before 13-dm-disk.rules runs, so we can rely on the latter to create
the symlinks.

The reason I put this last in the series is that it will possibly
require changes in other packages, notably dracut, in order to make
sure partitions mappings are cleanly set up during boot.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 kpartx/Makefile             |  1 +
 kpartx/dm-parts.rules       | 39 +++++++++++++++++++++++++++++++++++++++
 kpartx/kpartx.rules         | 44 --------------------------------------------
 multipath/11-dm-mpath.rules | 22 +++++++++++++++++++++-
 4 files changed, 61 insertions(+), 45 deletions(-)
 create mode 100644 kpartx/dm-parts.rules

diff --git a/kpartx/Makefile b/kpartx/Makefile
index 7f5c1708..8b759b73 100644
--- a/kpartx/Makefile
+++ b/kpartx/Makefile
@@ -29,6 +29,7 @@ install: $(EXEC) $(EXEC).8
 	$(INSTALL_PROGRAM) -d $(DESTDIR)$(libudevdir)
 	$(INSTALL_PROGRAM) -m 755 kpartx_id $(DESTDIR)$(libudevdir)
 	$(INSTALL_PROGRAM) -d $(DESTDIR)$(libudevdir)/rules.d
+	$(INSTALL_PROGRAM) -m 644 dm-parts.rules $(DESTDIR)$(libudevdir)/rules.d/11-dm-parts.rules
 	$(INSTALL_PROGRAM) -m 644 kpartx.rules $(DESTDIR)$(libudevdir)/rules.d/66-kpartx.rules
 	$(INSTALL_PROGRAM) -m 644 del-part-nodes.rules $(DESTDIR)$(libudevdir)/rules.d/68-del-part-nodes.rules
 	$(INSTALL_PROGRAM) -d $(DESTDIR)$(man8dir)
diff --git a/kpartx/dm-parts.rules b/kpartx/dm-parts.rules
new file mode 100644
index 00000000..235642fd
--- /dev/null
+++ b/kpartx/dm-parts.rules
@@ -0,0 +1,39 @@
+# Rules for partitions created by kpartx
+
+KERNEL!="dm-*", GOTO="dm_parts_end"
+ACTION!="add|change", GOTO="dm_parts_end"
+ENV{DM_UUID}!="part[0-9]*", GOTO="dm_parts_end"
+
+# We must take care that symlinks don't get lost,
+# even if blkid fails in 13-dm-disk.rules later.
+#
+# Fixme: we have currently no way to avoid calling blkid on
+# partitions of broken mpath maps such as DM_NOSCAN.
+# But when partition devices appear, kpartx has likely read
+# the partition table shortly before, so odds are not bad
+# that blkid will also succeed.
+
+IMPORT{db}="ID_FS_USAGE"
+IMPORT{db}="ID_FS_UUID_ENC"
+IMPORT{db}="ID_FS_LABEL_ENC"
+IMPORT{db}="ID_PART_ENTRY_NAME"
+IMPORT{db}="ID_PART_ENTRY_UUID"
+IMPORT{db}="ID_PART_ENTRY_SCHEME"
+
+# Maps should take precedence over their members.
+ENV{DM_UDEV_LOW_PRIORITY_FLAG}!="1", OPTIONS+="link_priority=50"
+
+# Set some additional symlinks that typically exist for mpath
+# path members, too, and should be overridden.
+#
+# kpartx_id is very robust, it works for suspended maps and maps
+# with 0 dependencies. It sets DM_TYPE, DM_PART, DM_WWN
+IMPORT{program}=="kpartx_id %M %m $env{DM_UUID}"
+
+# DM_TYPE only has a reasonable value for partitions on multipath.
+ENV{DM_UUID}=="*-mpath-*", ENV{DM_TYPE}=="?*" \
+	SYMLINK+="disk/by-id/$env{DM_TYPE}-$env{DM_NAME}"
+ENV{DM_WWN}=="?*", ENV{DM_PART}=="?*", \
+	SYMLINK+="disk/by-id/wwn-$env{DM_WWN}-part$env{DM_PART}"
+
+LABEL="dm_parts_end"
diff --git a/kpartx/kpartx.rules b/kpartx/kpartx.rules
index 1cbe9429..8f990494 100644
--- a/kpartx/kpartx.rules
+++ b/kpartx/kpartx.rules
@@ -8,50 +8,6 @@ KERNEL!="dm-*", GOTO="kpartx_end"
 ACTION!="add|change", GOTO="kpartx_end"
 ENV{DM_UUID}!="?*", GOTO="kpartx_end"
 
-# kpartx_id is very robust, it works for suspended maps and maps
-# with 0 dependencies
-IMPORT{program}=="kpartx_id %M %m $env{DM_UUID}"
-
-ENV{DM_UDEV_LOW_PRIORITY_FLAG}!="1", OPTIONS+="link_priority=50"
-
-ENV{DM_UUID}=="*mpath-*", ENV{DM_TYPE}=="?*" \
-	SYMLINK+="disk/by-id/$env{DM_TYPE}-$env{DM_NAME}"
-
-# Create persistent links for multipath tables
-ENV{DM_WWN}=="?*", ENV{DM_PART}!="?*", \
-	SYMLINK+="disk/by-id/wwn-$env{DM_WWN}"
-
-# Create persistent links for partitions
-ENV{DM_WWN}=="?*", ENV{DM_PART}=="?*", \
-	SYMLINK+="disk/by-id/wwn-$env{DM_WWN}-part$env{DM_PART}"
-
-# Create persistent by-label/by-uuid links.
-# multipath maps with DM_NOSCAN!=1 are handled in 13-dm-disk.rules.
-DM_UUID=="mpath-*", ENV{DM_NOSCAN}!="1", GOTO="symlink_end"
-
-# For partitions, we don't have DM_NOSCAN.
-# Simply load variables from db if they aren't set.
-# 11-dm-mpath.rules does this for mpath maps.
-# Fixme: we have currently no way to avoid calling blkid on
-# partitions of broken mpath maps.
-ENV{DM_UUID}!="part*-*-*", GOTO="import_end"
-ENV{ID_FS_USAGE}!="?*", IMPORT{db}="ID_FS_USAGE"
-ENV{ID_FS_UUID_ENC}!="?*", IMPORT{db}="ID_FS_UUID_ENC"
-ENV{ID_FS_LABEL_ENC}!="?*", IMPORT{db}="ID_FS_LABEL_ENC"
-ENV{ID_PART_ENTRY_NAME}!="?*", IMPORT{db}="ID_PART_ENTRY_NAME"
-ENV{ID_PART_ENTRY_UUID}!="?*", IMPORT{db}="ID_PART_ENTRY_UUID"
-ENV{ID_PART_ENTRY_SCHEME}!="?*", IMPORT{db}="ID_PART_ENTRY_SCHEME"
-LABEL="import_end"
-
-ENV{ID_FS_USAGE}=="filesystem|other|crypto", ENV{ID_FS_UUID_ENC}=="?*", \
-       SYMLINK+="disk/by-uuid/$env{ID_FS_UUID_ENC}"
-ENV{ID_FS_USAGE}=="filesystem|other", ENV{ID_FS_LABEL_ENC}=="?*", \
-       SYMLINK+="disk/by-label/$env{ID_FS_LABEL_ENC}"
-ENV{ID_PART_ENTRY_UUID}=="?*", SYMLINK+="disk/by-partuuid/$env{ID_PART_ENTRY_UUID}"
-ENV{ID_PART_ENTRY_SCHEME}=="gpt", ENV{ID_PART_ENTRY_NAME}=="?*", \
-       SYMLINK+="disk/by-partlabel/$env{ID_PART_ENTRY_NAME}"
-LABEL="symlink_end"
-
 # Create dm tables for partitions on multipath devices.
 ENV{DM_UUID}!="mpath-?*", GOTO="mpath_kpartx_end"
 
diff --git a/multipath/11-dm-mpath.rules b/multipath/11-dm-mpath.rules
index 4a3f646d..132a9234 100644
--- a/multipath/11-dm-mpath.rules
+++ b/multipath/11-dm-mpath.rules
@@ -80,8 +80,12 @@ ENV{MPATH_DEVICE_READY}!="0", ENV{.MPATH_DEVICE_READY_OLD}=="0",\
 	ENV{DM_UDEV_DISABLE_OTHER_RULES_FLAG}="$env{DM_DISABLE_OTHER_RULES_FLAG_OLD}",\
 	ENV{DM_DISABLE_OTHER_RULES_FLAG_OLD}=""
 
+# The code to check multipath state ends here. We need to set
+# properties and symlinks regardless whether the map is usable or
+# not. If symlinks get lost, systemd may auto-unmount file systems.
+
 LABEL="scan_import"
-ENV{DM_NOSCAN}!="1", GOTO="mpath_end"
+ENV{DM_NOSCAN}!="1", GOTO="import_end"
 IMPORT{db}="ID_FS_TYPE"
 IMPORT{db}="ID_FS_USAGE"
 IMPORT{db}="ID_FS_UUID"
@@ -90,4 +94,20 @@ IMPORT{db}="ID_FS_LABEL"
 IMPORT{db}="ID_FS_LABEL_ENC"
 IMPORT{db}="ID_FS_VERSION"
 
+LABEL="import_end"
+
+# Multipath maps should take precedence over their members.
+ENV{DM_UDEV_LOW_PRIORITY_FLAG}!="1", OPTIONS+="link_priority=50"
+
+# Set some additional symlinks that typically exist for mpath
+# path members, too, and should be overridden.
+
+# kpartx_id is very robust, it works for suspended maps and maps
+# with 0 dependencies. It sets DM_TYPE, DM_PART, DM_WWN
+TEST=="/usr/lib/udev/kpartx_id", \
+	IMPORT{program}=="kpartx_id %M %m $env{DM_UUID}"
+
+ENV{DM_TYPE}=="?*", SYMLINK+="disk/by-id/$env{DM_TYPE}-$env{DM_NAME}"
+ENV{DM_WWN}=="?*", SYMLINK+="disk/by-id/wwn-$env{DM_WWN}"
+
 LABEL="mpath_end"
-- 
2.14.0

Re: [PATCH 09/31] libmultipath: support MPATH_UDEV_NO_PATHS_FLAG on map creation

[Benjamin Marzinski]

On Sun, Sep 03, 2017 at 12:38:38AM +0200, Martin Wilck wrote:
> Some vendor kernels (e.g. SUSE) have supported loading multipath
> maps without valid paths for a long time. Without that feature,
> problems can occur in failover scenarios when multipathd tries
> to (re)load maps after device failure/removal, because multipathd's
> attempts to reload the configuration may fail unnecessarily.
> The discussion in the kernel community is ongoing
> (see e.g. https://patchwork.kernel.org/patch/4579551/).

I don't object to the patch itself, but I'm pretty sure that this
solution to multipath's reloading issues is not currently under
consideration upstream.  It got Naked, and I wrote and alternative
method, which got accepted

https://www.redhat.com/archives/dm-devel/2014-September/msg00094.html

Do you still need this patch? The solution that's currently upstream
doesn't allow you to load new devices with failed paths, but it avoids
the issues that caused the SUSE patch to get Nak'ed. The multipath
user-space code already won't allow you to create a multipath device if
it can't open the path device, so I don't see why you need the ability
for the kernel to allow creation of devices with only failed paths. I
admit, there is a small window where multipath could open the path
device, and then the path device could fail before the load is sent to
the kernel.  In this case, with your patch, you could still create the
device (I believe).  But the much more likely case, where the path has
failed before multipath tries to open it, is still there. I don't see
the benefit of adding code to fix the corner case, while the common case
still doesn't work. Is there some other case where your patch is helpful
that I'm missing?

At any rate. Even if that kernel patch doesn't go upstream, I have no
objection to changing the code so the udev rules are robust enough to
handle this situation.

ACK

-Ben

> 
> One corner case of this is creation of a map with only failed
> paths. Such maps can be created if the kernel patch mentioned above
> is applied. The current udev rules for dm-multipath can't detect
> this situation. This patch fixes that by setting
> DM_SUBSYSTEM_UDEV_FLAG2, which is already used for the "map reload"
> case with no valid paths. Thus no additional udev rules are required
> to detect this situation.
> 
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  libmultipath/devmapper.c | 30 ++++++++++++++++++++----------
>  1 file changed, 20 insertions(+), 10 deletions(-)
> 
> diff --git a/libmultipath/devmapper.c b/libmultipath/devmapper.c
> index e701e806..7a3390a7 100644
> --- a/libmultipath/devmapper.c
> +++ b/libmultipath/devmapper.c
> @@ -300,12 +300,14 @@ dm_device_remove (const char *name, int needsync, int deferred_remove) {
>  
>  static int
>  dm_addmap (int task, const char *target, struct multipath *mpp,
> -	   char * params, int ro, int skip_kpartx) {
> +	   char * params, int ro, uint16_t udev_flags) {
>  	int r = 0;
>  	struct dm_task *dmt;
>  	char *prefixed_uuid = NULL;
>  	uint32_t cookie = 0;
> -	uint16_t udev_flags = DM_UDEV_DISABLE_LIBRARY_FALLBACK | ((skip_kpartx == SKIP_KPARTX_ON)? MPATH_UDEV_NO_KPARTX_FLAG : 0);
> +
> +	/* Need to add this here to allow 0 to be passed in udev_flags */
> +	udev_flags |= DM_UDEV_DISABLE_LIBRARY_FALLBACK;
>  
>  	if (!(dmt = libmp_dm_task_create (task)))
>  		return 0;
> @@ -371,15 +373,27 @@ addout:
>  	return r;
>  }
>  
> +static uint16_t build_udev_flags(const struct multipath *mpp, int reload)
> +{
> +	/* DM_UDEV_DISABLE_LIBRARY_FALLBACK is added in dm_addmap */
> +	return	(mpp->skip_kpartx == SKIP_KPARTX_ON ?
> +		 MPATH_UDEV_NO_KPARTX_FLAG : 0) |
> +		(mpp->nr_active == 0 ?
> +		 MPATH_UDEV_NO_PATHS_FLAG : 0) |
> +		(reload && !mpp->force_udev_reload ?
> +		 MPATH_UDEV_RELOAD_FLAG : 0);
> +}
> +
>  int dm_addmap_create (struct multipath *mpp, char * params)
>  {
>  	int ro;
> +	uint16_t udev_flags = build_udev_flags(mpp, 0);
>  
>  	for (ro = 0; ro <= 1; ro++) {
>  		int err;
>  
>  		if (dm_addmap(DM_DEVICE_CREATE, TGT_MPATH, mpp, params, ro,
> -			      mpp->skip_kpartx))
> +			      udev_flags))
>  			return 1;
>  		/*
>  		 * DM_DEVICE_CREATE is actually DM_DEV_CREATE + DM_TABLE_LOAD.
> @@ -405,11 +419,7 @@ int dm_addmap_create (struct multipath *mpp, char * params)
>  int dm_addmap_reload(struct multipath *mpp, char *params, int flush)
>  {
>  	int r = 0;
> -	uint16_t udev_flags = ((mpp->force_udev_reload)?
> -			       0 : MPATH_UDEV_RELOAD_FLAG) |
> -			      ((mpp->skip_kpartx == SKIP_KPARTX_ON)?
> -			       MPATH_UDEV_NO_KPARTX_FLAG : 0) |
> -			      ((mpp->nr_active)? 0 : MPATH_UDEV_NO_PATHS_FLAG);
> +	uint16_t udev_flags = build_udev_flags(mpp, 1);
>  
>  	/*
>  	 * DM_DEVICE_RELOAD cannot wait on a cookie, as
> @@ -419,12 +429,12 @@ int dm_addmap_reload(struct multipath *mpp, char *params, int flush)
>  	 */
>  	if (!mpp->force_readonly)
>  		r = dm_addmap(DM_DEVICE_RELOAD, TGT_MPATH, mpp, params,
> -			      ADDMAP_RW, SKIP_KPARTX_OFF);
> +			      ADDMAP_RW, 0);
>  	if (!r) {
>  		if (!mpp->force_readonly && errno != EROFS)
>  			return 0;
>  		r = dm_addmap(DM_DEVICE_RELOAD, TGT_MPATH, mpp,
> -			      params, ADDMAP_RO, SKIP_KPARTX_OFF);
> +			      params, ADDMAP_RO, 0);
>  	}
>  	if (r)
>  		r = dm_simplecmd(DM_DEVICE_RESUME, mpp->alias, !flush,
> -- 
> 2.14.0

Re: [PATCH 15/31] multipath: implement "check usable paths" (-C/-U)

[Benjamin Marzinski]

On Sun, Sep 03, 2017 at 12:38:44AM +0200, Martin Wilck wrote:
> When we process udev rules, it's crucial to know whether I/O on a given
> device will succeed. Unfortunately DM_NR_VALID_PATHS is not reliable,
> because the kernel path events aren't necessarily received in order, and
> even if they are, the number of usable paths may have changed during
> udev processing, in particular when there's a lot of load on udev
> because many paths are failing or reinstating at the same time.
> The latter problem can't be completely avoided, but the closer the
> test before the actual "blkid" call, the better.
> 
> This patch adds the -C/-U options to multipath to check if a given
> map has usable paths. Obviously this command must avoid doing any I/O
> on the multipath map itself, thus no checkers are called; only status
> from sysfs and dm is collected.

I'm a little worried about the overhead of adding yet more multipath
commands to udev.  The multipath command takes a while to exec, and
already udev hits issues where in event storms, udev can time out
because it's trying to do too much with too short a timeout.

Do out-of-order uevents really happen? Delayed ones certainly do, but if
we really can see out-of-order events, then all that event coalescing
code that got in should get another pass over it, because I'm pretty
sure it relied on events not being reordered.

If all we're are worried about is delayed events, then it might be o.k.
to just always disable scanning on PATH_FAILED events, because we don't
know if there are any more of them. When we reload a device, we already
pass the DM_SUBSYSTEM_UDEV_FLAG2 to deal with not having
DM_NR_VALID_PATHS on reloads. However, I do realize that a path could
fail immediately after the reload, and your patch does a better job
keeping that window smaller.

Also, when you have reinstates and failures at the same time, you won't
run into problems unless the path you just reinstated immediately fails
(otherwise there will be at least one available path, the one you just
reinstated).  This certainly can happen.  Unfortunately, in my
experience, it usually happens because sysfs says that the path is o.k.
but when the kernel tries to do IO to it, it's flaky. The -C/-U callout
isn't going to catch those cases, because it doesn't do IO.

Now, I agree that you are making the window where things can go wrong
smaller, but there is a cost that is being incurred on processing a
large number of uevents to make that window smaller, and I don't know
exactly how that trade-off works. I've been thinking about making a
library interface that multipath would use to do the commands which are
also called from udev. That would let udev directly call these commands
if they wanted, which would save on the exec time, and cut out any
unnecessary cruft that doesn't need to be done for udev to get its
information.  That might be a solution, in case we do start seeing more
timed-out uevents because of this.

Any thoughts?

-Ben

> 
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  libmultipath/config.h |  1 +
>  multipath/main.c      | 90 +++++++++++++++++++++++++++++++++++++++++++++++++--
>  multipath/multipath.8 | 13 +++++++-
>  3 files changed, 101 insertions(+), 3 deletions(-)
> 
> diff --git a/libmultipath/config.h b/libmultipath/config.h
> index ffc69b5f..4aa53da8 100644
> --- a/libmultipath/config.h
> +++ b/libmultipath/config.h
> @@ -34,6 +34,7 @@ enum mpath_cmds {
>  	CMD_REMOVE_WWID,
>  	CMD_RESET_WWIDS,
>  	CMD_ADD_WWID,
> +	CMD_USABLE_PATHS,
>  };
>  
>  enum force_reload_types {
> diff --git a/multipath/main.c b/multipath/main.c
> index dede017e..704c491f 100644
> --- a/multipath/main.c
> +++ b/multipath/main.c
> @@ -117,6 +117,7 @@ usage (char * progname)
>  		"  -F      flush all multipath device maps\n"
>  		"  -a      add a device wwid to the wwids file\n"
>  		"  -c      check if a device should be a path in a multipath device\n"
> +		"  -C      check if a multipath device has usable paths\n"
>  		"  -q      allow queue_if_no_path when multipathd is not running\n"
>  		"  -d      dry run, do not create or update devmaps\n"
>  		"  -t      dump internal hardware table\n"
> @@ -258,6 +259,81 @@ get_dm_mpvec (enum mpath_cmds cmd, vector curmp, vector pathvec, char * refwwid)
>  	return 0;
>  }
>  
> +static int check_usable_paths(struct config *conf,
> +			      const char *devpath, enum devtypes dev_type)
> +{
> +	struct udev_device *ud = NULL;
> +	struct multipath *mpp = NULL;
> +	struct pathgroup *pg;
> +	struct path *pp;
> +	char *mapname;
> +	vector pathvec = NULL;
> +	char params[PARAMS_SIZE], status[PARAMS_SIZE];
> +	dev_t devt;
> +	int r = 1, i, j;
> +
> +	ud = get_udev_device(devpath, dev_type);
> +	if (ud == NULL)
> +		return r;
> +
> +	devt = udev_device_get_devnum(ud);
> +	if (!dm_is_dm_major(major(devt))) {
> +		condlog(1, "%s is not a dm device", devpath);
> +		goto out;
> +	}
> +
> +	mapname = dm_mapname(major(devt), minor(devt));
> +	if (mapname == NULL) {
> +		condlog(1, "dm device not found: %s", devpath);
> +		goto out;
> +	}
> +
> +	if (!dm_is_mpath(mapname)) {
> +		condlog(1, "%s is not a multipath map", devpath);
> +		goto free;
> +	}
> +
> +	/* pathvec is needed for disassemble_map */
> +	pathvec = vector_alloc();
> +	if (pathvec == NULL)
> +		goto free;
> +
> +	mpp = dm_get_multipath(mapname);
> +	if (mpp == NULL)
> +		goto free;
> +
> +	dm_get_map(mpp->alias, &mpp->size, params);
> +	dm_get_status(mpp->alias, status);
> +	disassemble_map(pathvec, params, mpp, 0);
> +	disassemble_status(status, mpp);
> +
> +	vector_foreach_slot (mpp->pg, pg, i) {
> +		vector_foreach_slot (pg->paths, pp, j) {
> +			pp->udev = get_udev_device(pp->dev_t, DEV_DEVT);
> +			if (pp->udev == NULL)
> +				continue;
> +			if (pathinfo(pp, conf, DI_SYSFS|DI_NOIO) != PATHINFO_OK)
> +				continue;
> +
> +			if (pp->state == PATH_UP &&
> +			    pp->dmstate == PSTATE_ACTIVE) {
> +				condlog(3, "%s: path %s is usable",
> +					devpath, pp->dev);
> +				r = 0;
> +				goto found;
> +			}
> +		}
> +	}
> +found:
> +	condlog(2, "%s:%s usable paths found", devpath, r == 0 ? "" : " no");
> +free:
> +	FREE(mapname);
> +	free_multipath(mpp, FREE_PATHS);
> +	vector_free(pathvec);
> +out:
> +	udev_device_unref(ud);
> +	return r;
> +}
>  
>  /*
>   * Return value:
> @@ -522,7 +598,7 @@ main (int argc, char *argv[])
>  		exit(1);
>  	multipath_conf = conf;
>  	conf->retrigger_tries = 0;
> -	while ((arg = getopt(argc, argv, ":adchl::FfM:v:p:b:BrR:itquwW")) != EOF ) {
> +	while ((arg = getopt(argc, argv, ":adcChl::FfM:v:p:b:BrR:itquUwW")) != EOF ) {
>  		switch(arg) {
>  		case 1: printf("optarg : %s\n",optarg);
>  			break;
> @@ -547,6 +623,9 @@ main (int argc, char *argv[])
>  		case 'c':
>  			cmd = CMD_VALID_PATH;
>  			break;
> +		case 'C':
> +			cmd = CMD_USABLE_PATHS;
> +			break;
>  		case 'd':
>  			if (cmd == CMD_CREATE)
>  				cmd = CMD_DRY_RUN;
> @@ -593,6 +672,10 @@ main (int argc, char *argv[])
>  			cmd = CMD_VALID_PATH;
>  			dev_type = DEV_UEVENT;
>  			break;
> +		case 'U':
> +			cmd = CMD_USABLE_PATHS;
> +			dev_type = DEV_UEVENT;
> +			break;
>  		case 'w':
>  			cmd = CMD_REMOVE_WWID;
>  			break;
> @@ -674,7 +757,10 @@ main (int argc, char *argv[])
>  		condlog(0, "failed to initialize prioritizers");
>  		goto out;
>  	}
> -
> +	if (cmd == CMD_USABLE_PATHS) {
> +		r = check_usable_paths(conf, dev, dev_type);
> +		goto out;
> +	}
>  	if (cmd == CMD_VALID_PATH &&
>  	    (!dev || dev_type == DEV_DEVMAP)) {
>  		condlog(0, "the -c option requires a path to check");
> diff --git a/multipath/multipath.8 b/multipath/multipath.8
> index b9436e52..a47a027d 100644
> --- a/multipath/multipath.8
> +++ b/multipath/multipath.8
> @@ -25,7 +25,7 @@ multipath \- Device mapper target autoconfig.
>  .RB [\| \-b\ \c
>  .IR bindings_file \|]
>  .RB [\| \-d \|]
> -.RB [\| \-h | \-l | \-ll | \-f | \-t | \-F | \-B | \-c | \-q | \|-r | \|-i | \-a | \|-u | \-w | \-W \|]
> +.RB [\| \-h | \-l | \-ll | \-f | \-t | \-F | \-B | \-c | \-C | \-q | \-r | \-i | \-a | \-u | \-U | \-w | \-W \|]
>  .RB [\| \-p\ \c
>  .IR failover | multibus | group_by_serial | group_by_prio | group_by_node_name \|]
>  .RB [\| \-R\ \c
> @@ -110,6 +110,12 @@ Set user_friendly_names bindings file location.  The default is
>  Check if a block device should be a path in a multipath device.
>  .
>  .TP
> +.B \-C
> +Check if a multipath device has usable paths. This can be used to
> +test whether or not I/O on this device is likely to succeed. The command
> +itself doesn't attempt to do I/O on the device.
> +.
> +.TP
>  .B \-q
>  Allow device tables with \fIqueue_if_no_path\fR when multipathd is not running.
>  .
> @@ -123,6 +129,11 @@ Check if the device specified in the program environment should be
>  a path in a multipath device.
>  .
>  .TP
> +.B \-U
> +Check if the device specified in the program environment is a multipath device
> +with usable paths. See \fB-C\fB.
> +.
> +.TP
>  .B \-w
>  Remove the WWID for the specified device from the WWIDs file.
>  .
> -- 
> 2.14.0

Re: [PATCH 20/31] 11-dm-mpath.rules: don't set READY->ACTIVATION

[Benjamin Marzinski]

On Sun, Sep 03, 2017 at 12:38:49AM +0200, Martin Wilck wrote:
> The fact alone that a map changes from not ready to ready does
> not imply that it is activating.

NAK on this one and
[PATCH 21/31] 11-dm-mpath.rules: Remember DM_ACTIVATION

This breaks things. It's often the case that there are devices stacked
on top of multipath.  When multipath loses all of its paths, if it
doesn't have queue_if_no_path set, it can fail IO up to these higher
devices, forcing them to react to the failure.  There is no way that I
know of to be able to check in udev to see if this has occured. The only
safe course of action is to assume the worst and notify lvm that things
are better now.

A simple reproducer of the problem is something like:

- create 2 multipath devices (call them mpatha and mpathb)
- set up raid1 on top of them

# pvcreate /dev/mapper/mpatha
# pvcreate /dev/mapper/mpathc
# vgcreate test /dev/mapper/mpathb /dev/mapper/mpatha
# lvcreate --type raid1 -m 1 -L 1G -n raid test

- verify that everything is o.k.

# dd if=/dev/zero of=/dev/test/raid bs=1M count=100
# multipath -ll
# pvs
# vgs
# lvs

- remove all of the path devices from mpatha
- verify that the raid device is in partial mode

# dd if=/dev/zero of=/dev/test/raid bs=1M count=100 
# multipath -ll
# pvs
# vgs
# lvs

- add the path devices back to the system
- check if the raid device has recovered

# multipath -ll
# pvs
# vgs
# lvs

With these patches, the raid device remains in partial mode, and the
mpatha pv is still listed as unknown. If I revert these two patches,
everything works fine again.

-Ben

> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  multipath/11-dm-mpath.rules | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/multipath/11-dm-mpath.rules b/multipath/11-dm-mpath.rules
> index 0be22ae4..3f47744f 100644
> --- a/multipath/11-dm-mpath.rules
> +++ b/multipath/11-dm-mpath.rules
> @@ -64,8 +64,7 @@ ENV{MPATH_DEVICE_READY}=="0", ENV{.MPATH_DEVICE_READY_OLD}=="1",\
>  ENV{MPATH_DEVICE_READY}=="0", ENV{DM_UDEV_DISABLE_OTHER_RULES_FLAG}="1"
>  ENV{MPATH_DEVICE_READY}!="0", ENV{.MPATH_DEVICE_READY_OLD}=="0",\
>  	ENV{DM_UDEV_DISABLE_OTHER_RULES_FLAG}="$env{DM_DISABLE_OTHER_RULES_FLAG_OLD}",\
> -	ENV{DM_DISABLE_OTHER_RULES_FLAG_OLD}="",\
> -	ENV{DM_ACTIVATION}="1"
> +	ENV{DM_DISABLE_OTHER_RULES_FLAG_OLD}=""
>  
>  LABEL="scan_import"
>  ENV{DM_NOSCAN}!="1", GOTO="mpath_end"
> -- 
> 2.14.0

Re: [PATCH 21/31] 11-dm-mpath.rules: Remember DM_ACTIVATION

[Benjamin Marzinski]

On Sun, Sep 03, 2017 at 12:38:50AM +0200, Martin Wilck wrote:
> If DM_ACTIVATION is set by the general dm rules, we need to
> bring up this device. But if the mpath device is not ready,
> that would be dangerous; it could hang or produce lots of IO
> errors. So remember this state, and try to activate when the
> map becomes usable later.

NAK. See reasons in

[PATCH 20/31] 11-dm-mpath.rules: don't set READY->ACTIVATION

-Ben

> 
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  multipath/11-dm-mpath.rules | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/multipath/11-dm-mpath.rules b/multipath/11-dm-mpath.rules
> index 3f47744f..9bfd75f8 100644
> --- a/multipath/11-dm-mpath.rules
> +++ b/multipath/11-dm-mpath.rules
> @@ -4,6 +4,7 @@ ENV{DM_UUID}!="mpath-?*", GOTO="mpath_end"
>  
>  IMPORT{db}="DM_DISABLE_OTHER_RULES_FLAG_OLD"
>  IMPORT{db}="MPATH_DEVICE_READY"
> +IMPORT{db}="MPATH_NEEDS_ACTIVATION"
>  
>  # If this uevent didn't come from dm, don't try to update the
>  # device state
> @@ -55,6 +56,13 @@ ENV{DM_SUBSYSTEM_UDEV_FLAG0}=="1", ENV{DM_ACTIVATION}="0"
>  # We'd like to avoid this, especially within udev processing.
>  ENV{MPATH_DEVICE_READY}=="0", ENV{DM_NOSCAN}="1"
>  
> +# If DM_ACTIVATION is set, but can't be satisfied, remember it
> +# in MPATH_NEEDS_ACTIVATION, and activate at the next opportunity.
> +ENV{MPATH_DEVICE_READY}=="0", ENV{DM_ACTIVATION}=="1", \
> +	ENV{MPATH_NEEDS_ACTIVATION}="1", ENV{DM_ACTIVATION}="0"
> +ENV{MPATH_DEVICE_READY}!="0", ENV{MPATH_NEEDS_ACTIVATION}=="1", \
> +	ENV{DM_ACTIVATION}="1", ENV{MPATH_NEEDS_ACTIVATION}=""
> +
>  # Also skip all foreign rules if no path is available.
>  # Remember the original value of DM_DISABLE_OTHER_RULES_FLAG
>  # and restore it back once we have at least one path available.
> -- 
> 2.14.0

Re: [PATCH 30/31] kpartx/del-part-nodes.rules: new udev file

[Benjamin Marzinski]

On Sun, Sep 03, 2017 at 12:38:59AM +0200, Martin Wilck wrote:
> A new udev rules file "68-del-part-nodes.rules" is introduced,
> based on code from Ben Marzinki. This code deletes partitions on
> multipath member devices. The purpose of this is to avoid users
> accidentally accessing partitions of member devices rather than of the
> multipath devcice. The deletion is done only once for every disk
> device. If the user wants to get the partitions back, he can run "partx
> -a" or "partprobe" on the disk device.
> 
> This code could be extended to wipe partitions on member devices
> of non-multipath dm targets if desired.
> 
> Means to deactivate this behavior are provided via kernel parameter
> "dont_del_part_nodes", or a custom udev rules file setting the
> "DONT_DEL_PART_NODES" environment variable.

You need to uninstall 68-del-part-nodes.rules. Otherwise, this looks
fine.

-Ben

> 
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  kpartx/Makefile             |  1 +
>  kpartx/del-part-nodes.rules | 32 ++++++++++++++++++++++++++++++++
>  2 files changed, 33 insertions(+)
>  create mode 100644 kpartx/del-part-nodes.rules
> 
> diff --git a/kpartx/Makefile b/kpartx/Makefile
> index 7b75032e..7f5c1708 100644
> --- a/kpartx/Makefile
> +++ b/kpartx/Makefile
> @@ -30,6 +30,7 @@ install: $(EXEC) $(EXEC).8
>  	$(INSTALL_PROGRAM) -m 755 kpartx_id $(DESTDIR)$(libudevdir)
>  	$(INSTALL_PROGRAM) -d $(DESTDIR)$(libudevdir)/rules.d
>  	$(INSTALL_PROGRAM) -m 644 kpartx.rules $(DESTDIR)$(libudevdir)/rules.d/66-kpartx.rules
> +	$(INSTALL_PROGRAM) -m 644 del-part-nodes.rules $(DESTDIR)$(libudevdir)/rules.d/68-del-part-nodes.rules
>  	$(INSTALL_PROGRAM) -d $(DESTDIR)$(man8dir)
>  	$(INSTALL_PROGRAM) -m 644 $(EXEC).8.gz $(DESTDIR)$(man8dir)
>  
> diff --git a/kpartx/del-part-nodes.rules b/kpartx/del-part-nodes.rules
> new file mode 100644
> index 00000000..cee945d9
> --- /dev/null
> +++ b/kpartx/del-part-nodes.rules
> @@ -0,0 +1,32 @@
> +# These rules can delete partitions devnodes for slave devices
> +# for certain aggregate devices such as multipath.
> +# This is desirable to avoid confusion and keep the number
> +# of device nodes and symlinks within limits.
> +#
> +# This is done only once on the first "add" or "change" event for
> +# any given device.
> +#
> +# To suppress this, use the kernel parameter "dont_del_part_nodes",
> +# or create an udev rule file that sets ENV{DONT_DEL_PART_NODES}="1".
> +
> +SUBSYSTEM!="block", GOTO="end_del_part_nodes"
> +KERNEL!="sd*|dasd*|rbd*", GOTO="end_del_part_nodes"
> +ACTION!="add|change", GOTO="end_del_part_nodes"
> +
> +IMPORT{cmdline}="dont_del_part_nodes"
> +ENV{dont_del_part_nodes}=="1", GOTO="end_del_part_nodes"
> +ENV{DONT_DEL_PART_NODES}=="1", GOTO="end_del_part_nodes"
> +
> +# dm-multipath
> +ENV{DM_MULTIPATH_DEVICE_PATH}=="1", GOTO="del_part_nodes"
> +
> +# Other aggregate device types can be added here.
> +
> +GOTO="end_del_part_nodes"
> +
> +LABEL="del_part_nodes"
> +IMPORT{db}="DM_DEL_PART_NODES"
> +ENV{DM_DEL_PART_NODES}!="1", ENV{DM_DEL_PART_NODES}="1", \
> +	RUN+="/usr/sbin/partx -d --nr 1-1024 $env{DEVNAME}"
> +
> +LABEL="end_del_part_nodes"
> -- 
> 2.14.0

Re: [PATCH 31/31] kpartx.rules: move symlink code to other files

[Benjamin Marzinski]

On Sun, Sep 03, 2017 at 12:39:00AM +0200, Martin Wilck wrote:
> Current kpartx.rules combines two purposes: setting properties and
> creating symlinks for dm partition devices, and creating such
> partition devices on top of other devices. This is contrary to
> common conventions for udev rules files.
> 
> This patch moves the code for properties and symlinks into other
> files. The code that generates symlinks for multipath maps is moved
> to 11-dm-mpath.rules, and for partitions we introduce a new file
> 11-dm-parts.rules. Necessarily this results in minor code duplication.
> OTOH quite some code is removed because the properties are now set
> before 13-dm-disk.rules runs, so we can rely on the latter to create
> the symlinks.
> 
> The reason I put this last in the series is that it will possibly
> require changes in other packages, notably dracut, in order to make
> sure partitions mappings are cleanly set up during boot.

You need to uninstall 11-dm-parts.rules. Otherwise, I'm O.k. with this.

-Ben

> 
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  kpartx/Makefile             |  1 +
>  kpartx/dm-parts.rules       | 39 +++++++++++++++++++++++++++++++++++++++
>  kpartx/kpartx.rules         | 44 --------------------------------------------
>  multipath/11-dm-mpath.rules | 22 +++++++++++++++++++++-
>  4 files changed, 61 insertions(+), 45 deletions(-)
>  create mode 100644 kpartx/dm-parts.rules
> 
> diff --git a/kpartx/Makefile b/kpartx/Makefile
> index 7f5c1708..8b759b73 100644
> --- a/kpartx/Makefile
> +++ b/kpartx/Makefile
> @@ -29,6 +29,7 @@ install: $(EXEC) $(EXEC).8
>  	$(INSTALL_PROGRAM) -d $(DESTDIR)$(libudevdir)
>  	$(INSTALL_PROGRAM) -m 755 kpartx_id $(DESTDIR)$(libudevdir)
>  	$(INSTALL_PROGRAM) -d $(DESTDIR)$(libudevdir)/rules.d
> +	$(INSTALL_PROGRAM) -m 644 dm-parts.rules $(DESTDIR)$(libudevdir)/rules.d/11-dm-parts.rules
>  	$(INSTALL_PROGRAM) -m 644 kpartx.rules $(DESTDIR)$(libudevdir)/rules.d/66-kpartx.rules
>  	$(INSTALL_PROGRAM) -m 644 del-part-nodes.rules $(DESTDIR)$(libudevdir)/rules.d/68-del-part-nodes.rules
>  	$(INSTALL_PROGRAM) -d $(DESTDIR)$(man8dir)
> diff --git a/kpartx/dm-parts.rules b/kpartx/dm-parts.rules
> new file mode 100644
> index 00000000..235642fd
> --- /dev/null
> +++ b/kpartx/dm-parts.rules
> @@ -0,0 +1,39 @@
> +# Rules for partitions created by kpartx
> +
> +KERNEL!="dm-*", GOTO="dm_parts_end"
> +ACTION!="add|change", GOTO="dm_parts_end"
> +ENV{DM_UUID}!="part[0-9]*", GOTO="dm_parts_end"
> +
> +# We must take care that symlinks don't get lost,
> +# even if blkid fails in 13-dm-disk.rules later.
> +#
> +# Fixme: we have currently no way to avoid calling blkid on
> +# partitions of broken mpath maps such as DM_NOSCAN.
> +# But when partition devices appear, kpartx has likely read
> +# the partition table shortly before, so odds are not bad
> +# that blkid will also succeed.
> +
> +IMPORT{db}="ID_FS_USAGE"
> +IMPORT{db}="ID_FS_UUID_ENC"
> +IMPORT{db}="ID_FS_LABEL_ENC"
> +IMPORT{db}="ID_PART_ENTRY_NAME"
> +IMPORT{db}="ID_PART_ENTRY_UUID"
> +IMPORT{db}="ID_PART_ENTRY_SCHEME"
> +
> +# Maps should take precedence over their members.
> +ENV{DM_UDEV_LOW_PRIORITY_FLAG}!="1", OPTIONS+="link_priority=50"
> +
> +# Set some additional symlinks that typically exist for mpath
> +# path members, too, and should be overridden.
> +#
> +# kpartx_id is very robust, it works for suspended maps and maps
> +# with 0 dependencies. It sets DM_TYPE, DM_PART, DM_WWN
> +IMPORT{program}=="kpartx_id %M %m $env{DM_UUID}"
> +
> +# DM_TYPE only has a reasonable value for partitions on multipath.
> +ENV{DM_UUID}=="*-mpath-*", ENV{DM_TYPE}=="?*" \
> +	SYMLINK+="disk/by-id/$env{DM_TYPE}-$env{DM_NAME}"
> +ENV{DM_WWN}=="?*", ENV{DM_PART}=="?*", \
> +	SYMLINK+="disk/by-id/wwn-$env{DM_WWN}-part$env{DM_PART}"
> +
> +LABEL="dm_parts_end"
> diff --git a/kpartx/kpartx.rules b/kpartx/kpartx.rules
> index 1cbe9429..8f990494 100644
> --- a/kpartx/kpartx.rules
> +++ b/kpartx/kpartx.rules
> @@ -8,50 +8,6 @@ KERNEL!="dm-*", GOTO="kpartx_end"
>  ACTION!="add|change", GOTO="kpartx_end"
>  ENV{DM_UUID}!="?*", GOTO="kpartx_end"
>  
> -# kpartx_id is very robust, it works for suspended maps and maps
> -# with 0 dependencies
> -IMPORT{program}=="kpartx_id %M %m $env{DM_UUID}"
> -
> -ENV{DM_UDEV_LOW_PRIORITY_FLAG}!="1", OPTIONS+="link_priority=50"
> -
> -ENV{DM_UUID}=="*mpath-*", ENV{DM_TYPE}=="?*" \
> -	SYMLINK+="disk/by-id/$env{DM_TYPE}-$env{DM_NAME}"
> -
> -# Create persistent links for multipath tables
> -ENV{DM_WWN}=="?*", ENV{DM_PART}!="?*", \
> -	SYMLINK+="disk/by-id/wwn-$env{DM_WWN}"
> -
> -# Create persistent links for partitions
> -ENV{DM_WWN}=="?*", ENV{DM_PART}=="?*", \
> -	SYMLINK+="disk/by-id/wwn-$env{DM_WWN}-part$env{DM_PART}"
> -
> -# Create persistent by-label/by-uuid links.
> -# multipath maps with DM_NOSCAN!=1 are handled in 13-dm-disk.rules.
> -DM_UUID=="mpath-*", ENV{DM_NOSCAN}!="1", GOTO="symlink_end"
> -
> -# For partitions, we don't have DM_NOSCAN.
> -# Simply load variables from db if they aren't set.
> -# 11-dm-mpath.rules does this for mpath maps.
> -# Fixme: we have currently no way to avoid calling blkid on
> -# partitions of broken mpath maps.
> -ENV{DM_UUID}!="part*-*-*", GOTO="import_end"
> -ENV{ID_FS_USAGE}!="?*", IMPORT{db}="ID_FS_USAGE"
> -ENV{ID_FS_UUID_ENC}!="?*", IMPORT{db}="ID_FS_UUID_ENC"
> -ENV{ID_FS_LABEL_ENC}!="?*", IMPORT{db}="ID_FS_LABEL_ENC"
> -ENV{ID_PART_ENTRY_NAME}!="?*", IMPORT{db}="ID_PART_ENTRY_NAME"
> -ENV{ID_PART_ENTRY_UUID}!="?*", IMPORT{db}="ID_PART_ENTRY_UUID"
> -ENV{ID_PART_ENTRY_SCHEME}!="?*", IMPORT{db}="ID_PART_ENTRY_SCHEME"
> -LABEL="import_end"
> -
> -ENV{ID_FS_USAGE}=="filesystem|other|crypto", ENV{ID_FS_UUID_ENC}=="?*", \
> -       SYMLINK+="disk/by-uuid/$env{ID_FS_UUID_ENC}"
> -ENV{ID_FS_USAGE}=="filesystem|other", ENV{ID_FS_LABEL_ENC}=="?*", \
> -       SYMLINK+="disk/by-label/$env{ID_FS_LABEL_ENC}"
> -ENV{ID_PART_ENTRY_UUID}=="?*", SYMLINK+="disk/by-partuuid/$env{ID_PART_ENTRY_UUID}"
> -ENV{ID_PART_ENTRY_SCHEME}=="gpt", ENV{ID_PART_ENTRY_NAME}=="?*", \
> -       SYMLINK+="disk/by-partlabel/$env{ID_PART_ENTRY_NAME}"
> -LABEL="symlink_end"
> -
>  # Create dm tables for partitions on multipath devices.
>  ENV{DM_UUID}!="mpath-?*", GOTO="mpath_kpartx_end"
>  
> diff --git a/multipath/11-dm-mpath.rules b/multipath/11-dm-mpath.rules
> index 4a3f646d..132a9234 100644
> --- a/multipath/11-dm-mpath.rules
> +++ b/multipath/11-dm-mpath.rules
> @@ -80,8 +80,12 @@ ENV{MPATH_DEVICE_READY}!="0", ENV{.MPATH_DEVICE_READY_OLD}=="0",\
>  	ENV{DM_UDEV_DISABLE_OTHER_RULES_FLAG}="$env{DM_DISABLE_OTHER_RULES_FLAG_OLD}",\
>  	ENV{DM_DISABLE_OTHER_RULES_FLAG_OLD}=""
>  
> +# The code to check multipath state ends here. We need to set
> +# properties and symlinks regardless whether the map is usable or
> +# not. If symlinks get lost, systemd may auto-unmount file systems.
> +
>  LABEL="scan_import"
> -ENV{DM_NOSCAN}!="1", GOTO="mpath_end"
> +ENV{DM_NOSCAN}!="1", GOTO="import_end"
>  IMPORT{db}="ID_FS_TYPE"
>  IMPORT{db}="ID_FS_USAGE"
>  IMPORT{db}="ID_FS_UUID"
> @@ -90,4 +94,20 @@ IMPORT{db}="ID_FS_LABEL"
>  IMPORT{db}="ID_FS_LABEL_ENC"
>  IMPORT{db}="ID_FS_VERSION"
>  
> +LABEL="import_end"
> +
> +# Multipath maps should take precedence over their members.
> +ENV{DM_UDEV_LOW_PRIORITY_FLAG}!="1", OPTIONS+="link_priority=50"
> +
> +# Set some additional symlinks that typically exist for mpath
> +# path members, too, and should be overridden.
> +
> +# kpartx_id is very robust, it works for suspended maps and maps
> +# with 0 dependencies. It sets DM_TYPE, DM_PART, DM_WWN
> +TEST=="/usr/lib/udev/kpartx_id", \
> +	IMPORT{program}=="kpartx_id %M %m $env{DM_UUID}"
> +
> +ENV{DM_TYPE}=="?*", SYMLINK+="disk/by-id/$env{DM_TYPE}-$env{DM_NAME}"
> +ENV{DM_WWN}=="?*", SYMLINK+="disk/by-id/wwn-$env{DM_WWN}"
> +
>  LABEL="mpath_end"
> -- 
> 2.14.0

Re: [PATCH 00/31] multipath/kpartx udev rules cleanup, and fixes

[Benjamin Marzinski]

On Sun, Sep 03, 2017 at 12:38:29AM +0200, Martin Wilck wrote:
> The main part of this series is a cleanup of the udev rules for
> multipath and kpartx. More about that further down.
> 
>  - patch 01 is an obvious bug fix
>  - patch 02-08 are fixes for kpartx problems that I encountered
>    during testing, with related tests. The interesting one here is
>    04, which adds the ability to rename partition mappings if the
>    current name doesn't match the specified scheme. The purpose
>    is to provide consistent naming of partition mappings across
>    the OS, even in the presence of other tools such as parted
>    that may follow different conventions.
>  - 09-15 are changes to the core code I found necessary for the
>    udev rules cleanup, most importantly a helper to figure out
>    quickly whether a given multipath has usable paths
>    (multipath -U). This replaces the former SUSE-specific tests
>    for DM_DEPS and DM_TABLE_STATE.
>  - 16ff are the actual changes to the udev rules. This was motivated
>    by Ben Marzinski who asked Hannes and me a while ago to remove
>    the SUSE-isms in the rules files (patch 23), and cleanup mainly
>    kpartx.rules.
> 

ACK for everything but:

[PATCH 16/31] 11-dm-mpath.rules: multipath -U for READY check
[PATCH 20/31] 11-dm-mpath.rules: don't set READY->ACTIVATION
[PATCH 21/31] 11-dm-mpath.rules: Remember DM_ACTIVATION
[PATCH 30/31] kpartx/del-part-nodes.rules: new udev file
[PATCH 31/31] kpartx.rules: move symlink code to other files

-Ben

> More about the udev rules changes:
> 
> Multipath maps are special because they can exist without any
> usable members/paths, and users expect them to be at least partly
> functional in such situations. Care has to be taken to avoid I/O
> on maps that may not be able to process it, and not to loose any
> symlinks that might be used by systemd to access the partition.
> Partitions on multipath maps have similar semantics, with the added
> difficulty that no first-hand information about the parent device
> is available. A lot of the existing code was trying to fix these
> corner case situations, but sometimes incorrectly or not cleanly.
> I have pointed out the issues in the individual commit messages.
> One important point is that checking DM_NR_VALID_PATHS>0 is not
> in sufficient to affirm that I/O will succeed. That was the reason
> for writing the "multipath -U" helper.
> 
> Also, multipath maps receive frequent udev events that don't
> matter to upper layers at all. In such cases, further scanning
> should be avoided. This is implemented much more cleanly now,
> I hope. 
> 
> Following the example of the dm core, I split the kpartx rules
> up in "early" rules for setting properties and symlinks, and
> "late" rules for taking actions such as running kpartx. I believe
> that this makes the code better readable.
> CAUTION: The new rules files matter for other packages such as dracut.
> Distribution package builders have to be careful here. If this patch set
> is accepted, respective patches will be send to the dracut maintainers.
> 
> Following Ben's suggestions, a new rules file is added that
> is responsible for deleting partition device nodes for multipath
> member devices.
> 
> I didn't expect this series to grow so large, but after having
> worked and tested for considerable time, this is the first
> version that I find ready for serious review. I smoke-tested the
> interaction of rules files, multipathd, udev, systemd, and
> kpartx quite a bit with failover and 0-paths scenarios, successfully.
> 
> The changes are less drastic than the stats below suggest.
> A considerable part just moves functionality out of existing code
> into separate functions in order to use it elsewhere.
> 
> Regards,
> Martin
> 
> Martin Wilck (31):
>   libmultipath: fix partition_delimiter config option
>   kpartx: helper functions for name and uuid generation
>   kpartx: search partitions by UUID, and rename
>   test-kpartx: add tests for renaming functionality
>   kpartx: fix a corner case when renaming partitions
>   kpartx: fix part deletion without partition table
>   test-kpartx: test deletion with empty part table
>   kpartx: only recognize dasd part table on DASD
>   libmultipath: support MPATH_UDEV_NO_PATHS_FLAG on map creation
>   libmultipath: add get_udev_device
>   libmultipath: get_refwwid: use get_udev_device
>   libmultipath: use const char* in some dm helpers
>   libmultipath: add DI_NOIO flag for pathinfo
>   libmultipath: add dm_get_multipath
>   multipath: implement "check usable paths" (-C/-U)
>   11-dm-mpath.rules: multipath -U for READY check
>   11-dm-mpath.rules: import more ID_FS_xxx vars from db
>   11-dm-mpath.rules: no need to test before IMPORT
>   11-dm-mpath.rules: handle new maps with READY==0
>   11-dm-mpath.rules: don't set READY->ACTIVATION
>   11-dm-mpath.rules: Remember DM_ACTIVATION
>   multipath.rules: set ID_FS_TYPE to "mpath_member"
>   kpartx.rules: don't rely on DM_DEPS and DM_TABLE_STATE
>   kpartx.rules: respect DM_UDEV_LOW_PRIORITY_FLAG
>   kpartx.rules: improved logic for by-uuid and by-label links
>   kpartx.rules: create by-partuuid and by-partlabel symlinks
>   kpartx.rules: generate type-name links only for multipath devices
>   kpartx.rules: fix logic for adding partitions
>   multipath/kpartx rules: avoid superfluous scanning
>   kpartx/del-part-nodes.rules: new udev file
>   kpartx.rules: move symlink code to other files
> 
>  kpartx/Makefile             |   2 +
>  kpartx/dasd.c               |   3 +
>  kpartx/del-part-nodes.rules |  32 ++++++++
>  kpartx/devmapper.c          | 194 ++++++++++++++++++++++++++++++++++++++++++--
>  kpartx/devmapper.h          |  18 +++-
>  kpartx/dm-parts.rules       |  39 +++++++++
>  kpartx/kpartx.c             | 105 +++++++-----------------
>  kpartx/kpartx.rules         |  58 ++++++-------
>  kpartx/test-kpartx          |  50 ++++++++++++
>  libmultipath/config.h       |   1 +
>  libmultipath/configure.c    |  58 ++++++++++---
>  libmultipath/configure.h    |   1 +
>  libmultipath/devmapper.c    |  87 ++++++++++++--------
>  libmultipath/devmapper.h    |   5 +-
>  libmultipath/discovery.c    |   8 ++
>  libmultipath/discovery.h    |   2 +
>  multipath/11-dm-mpath.rules |  76 +++++++++++++----
>  multipath/main.c            |  90 +++++++++++++++++++-
>  multipath/multipath.8       |  13 ++-
>  multipath/multipath.rules   |   2 +-
>  20 files changed, 659 insertions(+), 185 deletions(-)
>  create mode 100644 kpartx/del-part-nodes.rules
>  create mode 100644 kpartx/dm-parts.rules
> 
> -- 
> 2.14.0

Re: [PATCH 20/31] 11-dm-mpath.rules: don't set READY->ACTIVATION

[Martin Wilck]

On Wed, 2017-09-13 at 16:19 -0500, Benjamin Marzinski wrote:
> On Sun, Sep 03, 2017 at 12:38:49AM +0200, Martin Wilck wrote:
> > The fact alone that a map changes from not ready to ready does
> > not imply that it is activating.
> 
> NAK on this one and
> [PATCH 21/31] 11-dm-mpath.rules: Remember DM_ACTIVATION
> 
> This breaks things. It's often the case that there are devices
> stacked
> on top of multipath.  When multipath loses all of its paths, if it
> doesn't have queue_if_no_path set, it can fail IO up to these higher
> devices, forcing them to react to the failure.  There is no way that
> I
> know of to be able to check in udev to see if this has occured. The
> only
> safe course of action is to assume the worst and notify lvm that
> things
> are better now.

You are right, thanks. I didn't consider the case without queueing.
I see no problem with just skipping these two.

Best,
Martin

> 
> A simple reproducer of the problem is something like:
> 
> - create 2 multipath devices (call them mpatha and mpathb)
> - set up raid1 on top of them
> 
> # pvcreate /dev/mapper/mpatha
> # pvcreate /dev/mapper/mpathc
> # vgcreate test /dev/mapper/mpathb /dev/mapper/mpatha
> # lvcreate --type raid1 -m 1 -L 1G -n raid test
> 
> - verify that everything is o.k.
> 
> # dd if=/dev/zero of=/dev/test/raid bs=1M count=100
> # multipath -ll
> # pvs
> # vgs
> # lvs
> 
> - remove all of the path devices from mpatha
> - verify that the raid device is in partial mode
> 
> # dd if=/dev/zero of=/dev/test/raid bs=1M count=100 
> # multipath -ll
> # pvs
> # vgs
> # lvs
> 
> - add the path devices back to the system
> - check if the raid device has recovered
> 
> # multipath -ll
> # pvs
> # vgs
> # lvs
> 
> With these patches, the raid device remains in partial mode, and the
> mpatha pv is still listed as unknown. If I revert these two patches,
> everything works fine again.
> 
> -Ben
> 
> > Signed-off-by: Martin Wilck <mwilck@suse.com>
> > ---
> >  multipath/11-dm-mpath.rules | 3 +--
> >  1 file changed, 1 insertion(+), 2 deletions(-)
> > 
> > diff --git a/multipath/11-dm-mpath.rules b/multipath/11-dm-
> > mpath.rules
> > index 0be22ae4..3f47744f 100644
> > --- a/multipath/11-dm-mpath.rules
> > +++ b/multipath/11-dm-mpath.rules
> > @@ -64,8 +64,7 @@ ENV{MPATH_DEVICE_READY}=="0",
> > ENV{.MPATH_DEVICE_READY_OLD}=="1",\
> >  ENV{MPATH_DEVICE_READY}=="0",
> > ENV{DM_UDEV_DISABLE_OTHER_RULES_FLAG}="1"
> >  ENV{MPATH_DEVICE_READY}!="0", ENV{.MPATH_DEVICE_READY_OLD}=="0",\
> >  	ENV{DM_UDEV_DISABLE_OTHER_RULES_FLAG}="$env{DM_DISABLE_OTH
> > ER_RULES_FLAG_OLD}",\
> > -	ENV{DM_DISABLE_OTHER_RULES_FLAG_OLD}="",\
> > -	ENV{DM_ACTIVATION}="1"
> > +	ENV{DM_DISABLE_OTHER_RULES_FLAG_OLD}=""
> >  
> >  LABEL="scan_import"
> >  ENV{DM_NOSCAN}!="1", GOTO="mpath_end"
> > -- 
> > 2.14.0
> 
> 

-- 
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 15/31] multipath: implement "check usable paths" (-C/-U)

[Martin Wilck]

On Wed, 2017-09-13 at 15:53 -0500, Benjamin Marzinski wrote:
> On Sun, Sep 03, 2017 at 12:38:44AM +0200, Martin Wilck wrote:
> > When we process udev rules, it's crucial to know whether I/O on a
> > given
> > device will succeed. Unfortunately DM_NR_VALID_PATHS is not
> > reliable,
> > because the kernel path events aren't necessarily received in
> > order, and
> > even if they are, the number of usable paths may have changed
> > during
> > udev processing, in particular when there's a lot of load on udev
> > because many paths are failing or reinstating at the same time.
> > The latter problem can't be completely avoided, but the closer the
> > test before the actual "blkid" call, the better.
> > 
> > This patch adds the -C/-U options to multipath to check if a given
> > map has usable paths. Obviously this command must avoid doing any
> > I/O
> > on the multipath map itself, thus no checkers are called; only
> > status
> > from sysfs and dm is collected.
> 
> I'm a little worried about the overhead of adding yet more multipath
> commands to udev.  The multipath command takes a while to exec, and
> already udev hits issues where in event storms, udev can time out
> because it's trying to do too much with too short a timeout.

I was aware of that and tried to make this as lean as possible. On my
system here it takes about 8ms or 500 sytsem calls, which is roughly
the same number as "multipath -c" or "kpartx_id", at least in the case
where there are paths available. AFAICS, most of the time is spent in
libudev collecting device properties. I haven't studied that in depth
though. "blkid" calls are much more expensive AFAICT.

> Do out-of-order uevents really happen? 

For dm-mpath "path events", yes, I'm positive about that. 
See an example at http://paste.opensuse.org/28641254. 
It was taken with an openSUSE Tumbleweed 4.11.8 kernel. It was tkane
from udev monitor data. 
See http://paste.opensuse.org/63686952 for the full log.

You can see that the time stamps and seqnums increase, but
DM_NR_VALID_PATHS does not decrease monitonically as you'd expect (my
script removes all paths of map in order, re-adds them again).
So far I haven't had the time to analyze this on the kernel side. But
even if it could be fixed in the kernel, multipathd and the udev rules
should be able to deal with it.

So, reinforcing my argument from the log message, I truly believe that
DM_NR_VALID_PATHS is not something that we should rely upon too much.

> Delayed ones certainly do, but if
> we really can see out-of-order events, then all that event coalescing
> code that got in should get another pass over it, because I'm pretty
> sure it relied on events not being reordered.

That would need further examination. I thought that the coalescing
logic worked mostly on uevents for path devices, not the
PATH_FAILED/PATH_REINSTATED events for the map devices at which the
udev rules are looking.

> If all we're are worried about is delayed events, then it might be
> o.k.
> to just always disable scanning on PATH_FAILED events, because we
> don't
> know if there are any more of them. When we reload a device, we
> already
> pass the DM_SUBSYSTEM_UDEV_FLAG2 to deal with not having
> DM_NR_VALID_PATHS on reloads. However, I do realize that a path could
> fail immediately after the reload, and your patch does a better job
> keeping that window smaller.
> 
> Also, when you have reinstates and failures at the same time, you
> won't
> run into problems unless the path you just reinstated immediately
> fails
> (otherwise there will be at least one available path, the one you
> just
> reinstated).  This certainly can happen. 

Maybe we could skip calling "multipath -U" for PATH_REINSTATED events.
You're right, the scenario you just describe is really not that likely.

> > Unfortunately, in my
> experience, it usually happens because sysfs says that the path is
> o.k.
> but when the kernel tries to do IO to it, it's flaky. The -C/-U
> callout
> isn't going to catch those cases, because it doesn't do IO.

True, but the whole purpose of this patch is to avoid doing IO in the
first place. We can't do anything about this; both the kernel's and
multipathd's internal representation can only be approximations of the
real device state.

> Now, I agree that you are making the window where things can go wrong
> smaller, but there is a cost that is being incurred on processing a
> large number of uevents to make that window smaller, and I don't know
> exactly how that trade-off works. I've been thinking about making a
> library interface that multipath would use to do the commands which
> are
> also called from udev. That would let udev directly call these
> commands
> if they wanted, which would save on the exec time, and cut out any
> unnecessary cruft that doesn't need to be done for udev to get its
> information.  That might be a solution, in case we do start seeing
> more
> timed-out uevents because of this.

Sure. I've had a similar thought. My tests with "multipath -U" makes me
think that most of the time is spent in collecting properties from
sysfs in libudev. If the code was run in the context of the udev worker
which might have these properties already cached, performance could be
much better. I'm not sure what exactly is cached in the udev workers
though.

Anyway, back to your NAK on this patch, please consider again. 
IMO we're a lot safer with this additional check, in particular in view
of possible out-of-order events.

I introduced this as a replacement for the original "DM_DEPS" check we
had at SUSE. We'd found that to be helpful in avoiding problems during
udev processing in the past. It's always hard to tell if such past
fixes are still required, but at least for SLES we'd risk to cause
customer regressions if we simply dropped it, so we prefer to play safe
here. We can keep this as a SUSE-only patch, if you or others insist
that "multipath -U" is a bad thing.

DM_DEPS just checks if there are any paths (valid or not), and comes
down to a "dmsetup deps" invocation, which takes about 4ms. "multipath
-U" is slower because it needs to look at the paths, but those
additional cycles may pay off if we can avoid a blkid call on a device
with no paths. My first approach to the question "is this map really
ready for IO?" was indeed just a tiny "dmsetup deps" wrapper. But then
I realized the ordering problems for uevents shown above, and I
concluded that a more robust test would be desirable.

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

Re: [PATCH 09/31] libmultipath: support MPATH_UDEV_NO_PATHS_FLAG on map creation

[Martin Wilck]

On Wed, 2017-09-13 at 15:33 -0500, Benjamin Marzinski wrote:
> On Sun, Sep 03, 2017 at 12:38:38AM +0200, Martin Wilck wrote:
> > Some vendor kernels (e.g. SUSE) have supported loading multipath
> > maps without valid paths for a long time. Without that feature,
> > problems can occur in failover scenarios when multipathd tries
> > to (re)load maps after device failure/removal, because multipathd's
> > attempts to reload the configuration may fail unnecessarily.
> > The discussion in the kernel community is ongoing
> > (see e.g. https://patchwork.kernel.org/patch/4579551/).
> 
> I don't object to the patch itself, but I'm pretty sure that this
> solution to multipath's reloading issues is not currently under
> consideration upstream.  It got Naked, and I wrote and alternative
> method, which got accepted
> 
> https://www.redhat.com/archives/dm-devel/2014-September/msg00094.html
> 
> Do you still need this patch? 

This is a hard question to answer. We've been using Hannes' approach in
SLES for years with no problems, and it's hard to predict whether or
not dropping it would cause regressions. Hannes is more qualified than
myself to comment on the kernel side details.


> The solution that's currently upstream
> doesn't allow you to load new devices with failed paths, but it
> avoids
> the issues that caused the SUSE patch to get Nak'ed. The multipath
> user-space code already won't allow you to create a multipath device
> if
> it can't open the path device, so I don't see why you need the
> ability
> for the kernel to allow creation of devices with only failed paths. I
> admit, there is a small window where multipath could open the path
> device, and then the path device could fail before the load is sent
> to
> the kernel.  In this case, with your patch, you could still create
> the
> device (I believe).  But the much more likely case, where the path
> has
> failed before multipath tries to open it, is still there. I don't see
> the benefit of adding code to fix the corner case, while the common
> case
> still doesn't work. Is there some other case where your patch is
> helpful
> that I'm missing?
> 
> At any rate. Even if that kernel patch doesn't go upstream, I have no
> objection to changing the code so the udev rules are robust enough to
> handle this situation.
> 
> ACK

Thanks a lot.

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 00/31] multipath/kpartx udev rules cleanup, and fixes

[Martin Wilck]

On Wed, 2017-09-13 at 16:28 -0500, Benjamin Marzinski wrote:
> 
> ACK for everything but:
> 
> [PATCH 16/31] 11-dm-mpath.rules: multipath -U for READY check

I'd like to ask you to reconsider, see my lengthy other mail.

> [PATCH 20/31] 11-dm-mpath.rules: don't set READY->ACTIVATION
> [PATCH 21/31] 11-dm-mpath.rules: Remember DM_ACTIVATION

OK, these were broken.

> [PATCH 30/31] kpartx/del-part-nodes.rules: new udev file
> [PATCH 31/31] kpartx.rules: move symlink code to other files

Uninstall code was missing in the Makefiles.

Unless someone objects, I'll send just updates for 30/31 and 31/31 in
order to avoid spamming the list with the complete set again.

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 20/31] 11-dm-mpath.rules: don't set READY->ACTIVATION

[Martin Wilck]

On Wed, 2017-09-13 at 16:19 -0500, Benjamin Marzinski wrote:
> On Sun, Sep 03, 2017 at 12:38:49AM +0200, Martin Wilck wrote:
> > The fact alone that a map changes from not ready to ready does
> > not imply that it is activating.
> 
> NAK on this one and
> [PATCH 21/31] 11-dm-mpath.rules: Remember DM_ACTIVATION
> 
> This breaks things. It's often the case that there are devices
> stacked
> on top of multipath.  When multipath loses all of its paths, if it
> doesn't have queue_if_no_path set, it can fail IO up to these higher
> devices, forcing them to react to the failure.

Thinking about this again, most of the stuff we do in the multipath-
related udev rules is for the queueing case. Without
"queue_if_no_path", all that checking whether or not the map will
sustain IO is useless; it would be better to simply pass the device on
to upper layers, which will try to probe it and fail, which is intended
in the non-queueing setup. Perhaps we should check "queue_if_no_path"
early on and skip most of the stuff we  do in the queueing case?

If we did this, maybe even my patches 20/21 might be worth
reconsideration for the "queue_if_no_path" case only?

Cheers,
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 21/31] 11-dm-mpath.rules: Remember DM_ACTIVATION

[Martin Wilck]

Hi Ben,

On Wed, 2017-09-13 at 16:19 -0500, Benjamin Marzinski wrote:
> On Sun, Sep 03, 2017 at 12:38:50AM +0200, Martin Wilck wrote:
> > If DM_ACTIVATION is set by the general dm rules, we need to
> > bring up this device. But if the mpath device is not ready,
> > that would be dangerous; it could hang or produce lots of IO
> > errors. So remember this state, and try to activate when the
> > map becomes usable later.
> 
> NAK. See reasons in
> 
> [PATCH 20/31] 11-dm-mpath.rules: don't set READY->ACTIVATION

Sorry for being slow. Re-reading this, I don't understand any more why
your valid argument against 20/31 invalidates this one as well. 21/31
affects only the case MPATH_DEVICE_READY!=0, in which case it will add
another case where DM_ACTIVATION is set.

Regards
Martin

> 
> -Ben
> 
> > 
> > Signed-off-by: Martin Wilck <mwilck@suse.com>
> > ---
> >  multipath/11-dm-mpath.rules | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> > 
> > diff --git a/multipath/11-dm-mpath.rules b/multipath/11-dm-
> > mpath.rules
> > index 3f47744f..9bfd75f8 100644
> > --- a/multipath/11-dm-mpath.rules
> > +++ b/multipath/11-dm-mpath.rules
> > @@ -4,6 +4,7 @@ ENV{DM_UUID}!="mpath-?*", GOTO="mpath_end"
> >  
> >  IMPORT{db}="DM_DISABLE_OTHER_RULES_FLAG_OLD"
> >  IMPORT{db}="MPATH_DEVICE_READY"
> > +IMPORT{db}="MPATH_NEEDS_ACTIVATION"
> >  
> >  # If this uevent didn't come from dm, don't try to update the
> >  # device state
> > @@ -55,6 +56,13 @@ ENV{DM_SUBSYSTEM_UDEV_FLAG0}=="1",
> > ENV{DM_ACTIVATION}="0"
> >  # We'd like to avoid this, especially within udev processing.
> >  ENV{MPATH_DEVICE_READY}=="0", ENV{DM_NOSCAN}="1"
> >  
> > +# If DM_ACTIVATION is set, but can't be satisfied, remember it
> > +# in MPATH_NEEDS_ACTIVATION, and activate at the next opportunity.
> > +ENV{MPATH_DEVICE_READY}=="0", ENV{DM_ACTIVATION}=="1", \
> > +	ENV{MPATH_NEEDS_ACTIVATION}="1", ENV{DM_ACTIVATION}="0"
> > +ENV{MPATH_DEVICE_READY}!="0", ENV{MPATH_NEEDS_ACTIVATION}=="1", \
> > +	ENV{DM_ACTIVATION}="1", ENV{MPATH_NEEDS_ACTIVATION}=""
> > +
> >  # Also skip all foreign rules if no path is available.
> >  # Remember the original value of DM_DISABLE_OTHER_RULES_FLAG
> >  # and restore it back once we have at least one path available.
> > -- 
> > 2.14.0
> 
> 

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

[PATCH v2 30/31] kpartx/del-part-nodes.rules: new udev file

[Martin Wilck]

A new udev rules file "68-del-part-nodes.rules" is introduced,
based on code from Ben Marzinki. This code deletes partitions on
multipath member devices. The purpose of this is to avoid users
accidentally accessing partitions of member devices rather than of the
multipath devcice. The deletion is done only once for every disk
device. If the user wants to get the partitions back, he can run "partx
-a" or "partprobe" on the disk device.

This code could be extended to wipe partitions on member devices
of non-multipath dm targets if desired.

Means to deactivate this behavior are provided via kernel parameter
"dont_del_part_nodes", or a custom udev rules file setting the
"DONT_DEL_PART_NODES" environment variable.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 kpartx/Makefile             |  2 ++
 kpartx/del-part-nodes.rules | 32 ++++++++++++++++++++++++++++++++
 2 files changed, 34 insertions(+)
 create mode 100644 kpartx/del-part-nodes.rules

diff --git a/kpartx/Makefile b/kpartx/Makefile
index 7b75032ef1ab..da1e1fbddce5 100644
--- a/kpartx/Makefile
+++ b/kpartx/Makefile
@@ -30,6 +30,7 @@ install: $(EXEC) $(EXEC).8
 	$(INSTALL_PROGRAM) -m 755 kpartx_id $(DESTDIR)$(libudevdir)
 	$(INSTALL_PROGRAM) -d $(DESTDIR)$(libudevdir)/rules.d
 	$(INSTALL_PROGRAM) -m 644 kpartx.rules $(DESTDIR)$(libudevdir)/rules.d/66-kpartx.rules
+	$(INSTALL_PROGRAM) -m 644 del-part-nodes.rules $(DESTDIR)$(libudevdir)/rules.d/68-del-part-nodes.rules
 	$(INSTALL_PROGRAM) -d $(DESTDIR)$(man8dir)
 	$(INSTALL_PROGRAM) -m 644 $(EXEC).8.gz $(DESTDIR)$(man8dir)
 
@@ -39,6 +40,7 @@ uninstall:
 	$(RM) $(DESTDIR)$(libudevdir)/kpartx_id
 	$(RM) $(DESTDIR)$(libudevdir)/rules.d/66-kpartx.rules
 	$(RM) $(DESTDIR)$(libudevdir)/rules.d/67-kpartx-compat.rules
+	$(RM) $(DESTDIR)$(libudevdir)/rules.d/68-del-part-nodes.rules
 
 clean:
 	$(RM) core *.o $(EXEC) *.gz
diff --git a/kpartx/del-part-nodes.rules b/kpartx/del-part-nodes.rules
new file mode 100644
index 000000000000..cee945d93a62
--- /dev/null
+++ b/kpartx/del-part-nodes.rules
@@ -0,0 +1,32 @@
+# These rules can delete partitions devnodes for slave devices
+# for certain aggregate devices such as multipath.
+# This is desirable to avoid confusion and keep the number
+# of device nodes and symlinks within limits.
+#
+# This is done only once on the first "add" or "change" event for
+# any given device.
+#
+# To suppress this, use the kernel parameter "dont_del_part_nodes",
+# or create an udev rule file that sets ENV{DONT_DEL_PART_NODES}="1".
+
+SUBSYSTEM!="block", GOTO="end_del_part_nodes"
+KERNEL!="sd*|dasd*|rbd*", GOTO="end_del_part_nodes"
+ACTION!="add|change", GOTO="end_del_part_nodes"
+
+IMPORT{cmdline}="dont_del_part_nodes"
+ENV{dont_del_part_nodes}=="1", GOTO="end_del_part_nodes"
+ENV{DONT_DEL_PART_NODES}=="1", GOTO="end_del_part_nodes"
+
+# dm-multipath
+ENV{DM_MULTIPATH_DEVICE_PATH}=="1", GOTO="del_part_nodes"
+
+# Other aggregate device types can be added here.
+
+GOTO="end_del_part_nodes"
+
+LABEL="del_part_nodes"
+IMPORT{db}="DM_DEL_PART_NODES"
+ENV{DM_DEL_PART_NODES}!="1", ENV{DM_DEL_PART_NODES}="1", \
+	RUN+="/usr/sbin/partx -d --nr 1-1024 $env{DEVNAME}"
+
+LABEL="end_del_part_nodes"
-- 
2.14.0

[PATCH v2 31/31] kpartx.rules: move symlink code to other files

[Martin Wilck]

Current kpartx.rules combines two purposes: setting properties and
creating symlinks for dm partition devices, and creating such
partition devices on top of other devices. This is contrary to
common conventions for udev rules files.

This patch moves the code for properties and symlinks into other
files. The code that generates symlinks for multipath maps is moved
to 11-dm-mpath.rules, and for partitions we introduce a new file
11-dm-parts.rules. Necessarily this results in minor code duplication.
OTOH quite some code is removed because the properties are now set
before 13-dm-disk.rules runs, so we can rely on the latter to create
the symlinks.

The reason I put this last in the series is that it will possibly
require changes in other packages, notably dracut, in order to make
sure partitions mappings are cleanly set up during boot.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 kpartx/Makefile             |  2 ++
 kpartx/dm-parts.rules       | 39 +++++++++++++++++++++++++++++++++++++++
 kpartx/kpartx.rules         | 44 --------------------------------------------
 multipath/11-dm-mpath.rules | 22 +++++++++++++++++++++-
 4 files changed, 62 insertions(+), 45 deletions(-)
 create mode 100644 kpartx/dm-parts.rules

diff --git a/kpartx/Makefile b/kpartx/Makefile
index da1e1fbddce5..bf7362d95efd 100644
--- a/kpartx/Makefile
+++ b/kpartx/Makefile
@@ -29,6 +29,7 @@ install: $(EXEC) $(EXEC).8
 	$(INSTALL_PROGRAM) -d $(DESTDIR)$(libudevdir)
 	$(INSTALL_PROGRAM) -m 755 kpartx_id $(DESTDIR)$(libudevdir)
 	$(INSTALL_PROGRAM) -d $(DESTDIR)$(libudevdir)/rules.d
+	$(INSTALL_PROGRAM) -m 644 dm-parts.rules $(DESTDIR)$(libudevdir)/rules.d/11-dm-parts.rules
 	$(INSTALL_PROGRAM) -m 644 kpartx.rules $(DESTDIR)$(libudevdir)/rules.d/66-kpartx.rules
 	$(INSTALL_PROGRAM) -m 644 del-part-nodes.rules $(DESTDIR)$(libudevdir)/rules.d/68-del-part-nodes.rules
 	$(INSTALL_PROGRAM) -d $(DESTDIR)$(man8dir)
@@ -38,6 +39,7 @@ uninstall:
 	$(RM) $(DESTDIR)$(bindir)/$(EXEC)
 	$(RM) $(DESTDIR)$(man8dir)/$(EXEC).8.gz
 	$(RM) $(DESTDIR)$(libudevdir)/kpartx_id
+	$(RM) $(DESTDIR)$(libudevdir)/rules.d/11-dm-parts.rules
 	$(RM) $(DESTDIR)$(libudevdir)/rules.d/66-kpartx.rules
 	$(RM) $(DESTDIR)$(libudevdir)/rules.d/67-kpartx-compat.rules
 	$(RM) $(DESTDIR)$(libudevdir)/rules.d/68-del-part-nodes.rules
diff --git a/kpartx/dm-parts.rules b/kpartx/dm-parts.rules
new file mode 100644
index 000000000000..235642fd9ae3
--- /dev/null
+++ b/kpartx/dm-parts.rules
@@ -0,0 +1,39 @@
+# Rules for partitions created by kpartx
+
+KERNEL!="dm-*", GOTO="dm_parts_end"
+ACTION!="add|change", GOTO="dm_parts_end"
+ENV{DM_UUID}!="part[0-9]*", GOTO="dm_parts_end"
+
+# We must take care that symlinks don't get lost,
+# even if blkid fails in 13-dm-disk.rules later.
+#
+# Fixme: we have currently no way to avoid calling blkid on
+# partitions of broken mpath maps such as DM_NOSCAN.
+# But when partition devices appear, kpartx has likely read
+# the partition table shortly before, so odds are not bad
+# that blkid will also succeed.
+
+IMPORT{db}="ID_FS_USAGE"
+IMPORT{db}="ID_FS_UUID_ENC"
+IMPORT{db}="ID_FS_LABEL_ENC"
+IMPORT{db}="ID_PART_ENTRY_NAME"
+IMPORT{db}="ID_PART_ENTRY_UUID"
+IMPORT{db}="ID_PART_ENTRY_SCHEME"
+
+# Maps should take precedence over their members.
+ENV{DM_UDEV_LOW_PRIORITY_FLAG}!="1", OPTIONS+="link_priority=50"
+
+# Set some additional symlinks that typically exist for mpath
+# path members, too, and should be overridden.
+#
+# kpartx_id is very robust, it works for suspended maps and maps
+# with 0 dependencies. It sets DM_TYPE, DM_PART, DM_WWN
+IMPORT{program}=="kpartx_id %M %m $env{DM_UUID}"
+
+# DM_TYPE only has a reasonable value for partitions on multipath.
+ENV{DM_UUID}=="*-mpath-*", ENV{DM_TYPE}=="?*" \
+	SYMLINK+="disk/by-id/$env{DM_TYPE}-$env{DM_NAME}"
+ENV{DM_WWN}=="?*", ENV{DM_PART}=="?*", \
+	SYMLINK+="disk/by-id/wwn-$env{DM_WWN}-part$env{DM_PART}"
+
+LABEL="dm_parts_end"
diff --git a/kpartx/kpartx.rules b/kpartx/kpartx.rules
index 1cbe94298081..8f9904945f6a 100644
--- a/kpartx/kpartx.rules
+++ b/kpartx/kpartx.rules
@@ -8,50 +8,6 @@ KERNEL!="dm-*", GOTO="kpartx_end"
 ACTION!="add|change", GOTO="kpartx_end"
 ENV{DM_UUID}!="?*", GOTO="kpartx_end"
 
-# kpartx_id is very robust, it works for suspended maps and maps
-# with 0 dependencies
-IMPORT{program}=="kpartx_id %M %m $env{DM_UUID}"
-
-ENV{DM_UDEV_LOW_PRIORITY_FLAG}!="1", OPTIONS+="link_priority=50"
-
-ENV{DM_UUID}=="*mpath-*", ENV{DM_TYPE}=="?*" \
-	SYMLINK+="disk/by-id/$env{DM_TYPE}-$env{DM_NAME}"
-
-# Create persistent links for multipath tables
-ENV{DM_WWN}=="?*", ENV{DM_PART}!="?*", \
-	SYMLINK+="disk/by-id/wwn-$env{DM_WWN}"
-
-# Create persistent links for partitions
-ENV{DM_WWN}=="?*", ENV{DM_PART}=="?*", \
-	SYMLINK+="disk/by-id/wwn-$env{DM_WWN}-part$env{DM_PART}"
-
-# Create persistent by-label/by-uuid links.
-# multipath maps with DM_NOSCAN!=1 are handled in 13-dm-disk.rules.
-DM_UUID=="mpath-*", ENV{DM_NOSCAN}!="1", GOTO="symlink_end"
-
-# For partitions, we don't have DM_NOSCAN.
-# Simply load variables from db if they aren't set.
-# 11-dm-mpath.rules does this for mpath maps.
-# Fixme: we have currently no way to avoid calling blkid on
-# partitions of broken mpath maps.
-ENV{DM_UUID}!="part*-*-*", GOTO="import_end"
-ENV{ID_FS_USAGE}!="?*", IMPORT{db}="ID_FS_USAGE"
-ENV{ID_FS_UUID_ENC}!="?*", IMPORT{db}="ID_FS_UUID_ENC"
-ENV{ID_FS_LABEL_ENC}!="?*", IMPORT{db}="ID_FS_LABEL_ENC"
-ENV{ID_PART_ENTRY_NAME}!="?*", IMPORT{db}="ID_PART_ENTRY_NAME"
-ENV{ID_PART_ENTRY_UUID}!="?*", IMPORT{db}="ID_PART_ENTRY_UUID"
-ENV{ID_PART_ENTRY_SCHEME}!="?*", IMPORT{db}="ID_PART_ENTRY_SCHEME"
-LABEL="import_end"
-
-ENV{ID_FS_USAGE}=="filesystem|other|crypto", ENV{ID_FS_UUID_ENC}=="?*", \
-       SYMLINK+="disk/by-uuid/$env{ID_FS_UUID_ENC}"
-ENV{ID_FS_USAGE}=="filesystem|other", ENV{ID_FS_LABEL_ENC}=="?*", \
-       SYMLINK+="disk/by-label/$env{ID_FS_LABEL_ENC}"
-ENV{ID_PART_ENTRY_UUID}=="?*", SYMLINK+="disk/by-partuuid/$env{ID_PART_ENTRY_UUID}"
-ENV{ID_PART_ENTRY_SCHEME}=="gpt", ENV{ID_PART_ENTRY_NAME}=="?*", \
-       SYMLINK+="disk/by-partlabel/$env{ID_PART_ENTRY_NAME}"
-LABEL="symlink_end"
-
 # Create dm tables for partitions on multipath devices.
 ENV{DM_UUID}!="mpath-?*", GOTO="mpath_kpartx_end"
 
diff --git a/multipath/11-dm-mpath.rules b/multipath/11-dm-mpath.rules
index 4a3f646d264d..132a92345cdc 100644
--- a/multipath/11-dm-mpath.rules
+++ b/multipath/11-dm-mpath.rules
@@ -80,8 +80,12 @@ ENV{MPATH_DEVICE_READY}!="0", ENV{.MPATH_DEVICE_READY_OLD}=="0",\
 	ENV{DM_UDEV_DISABLE_OTHER_RULES_FLAG}="$env{DM_DISABLE_OTHER_RULES_FLAG_OLD}",\
 	ENV{DM_DISABLE_OTHER_RULES_FLAG_OLD}=""
 
+# The code to check multipath state ends here. We need to set
+# properties and symlinks regardless whether the map is usable or
+# not. If symlinks get lost, systemd may auto-unmount file systems.
+
 LABEL="scan_import"
-ENV{DM_NOSCAN}!="1", GOTO="mpath_end"
+ENV{DM_NOSCAN}!="1", GOTO="import_end"
 IMPORT{db}="ID_FS_TYPE"
 IMPORT{db}="ID_FS_USAGE"
 IMPORT{db}="ID_FS_UUID"
@@ -90,4 +94,20 @@ IMPORT{db}="ID_FS_LABEL"
 IMPORT{db}="ID_FS_LABEL_ENC"
 IMPORT{db}="ID_FS_VERSION"
 
+LABEL="import_end"
+
+# Multipath maps should take precedence over their members.
+ENV{DM_UDEV_LOW_PRIORITY_FLAG}!="1", OPTIONS+="link_priority=50"
+
+# Set some additional symlinks that typically exist for mpath
+# path members, too, and should be overridden.
+
+# kpartx_id is very robust, it works for suspended maps and maps
+# with 0 dependencies. It sets DM_TYPE, DM_PART, DM_WWN
+TEST=="/usr/lib/udev/kpartx_id", \
+	IMPORT{program}=="kpartx_id %M %m $env{DM_UUID}"
+
+ENV{DM_TYPE}=="?*", SYMLINK+="disk/by-id/$env{DM_TYPE}-$env{DM_NAME}"
+ENV{DM_WWN}=="?*", SYMLINK+="disk/by-id/wwn-$env{DM_WWN}"
+
 LABEL="mpath_end"
-- 
2.14.0

Re: [PATCH 20/31] 11-dm-mpath.rules: don't set READY->ACTIVATION

[Benjamin Marzinski]

On Thu, Sep 14, 2017 at 02:48:19PM +0200, Martin Wilck wrote:
> On Wed, 2017-09-13 at 16:19 -0500, Benjamin Marzinski wrote:
> > On Sun, Sep 03, 2017 at 12:38:49AM +0200, Martin Wilck wrote:
> > > The fact alone that a map changes from not ready to ready does
> > > not imply that it is activating.
> > 
> > NAK on this one and
> > [PATCH 21/31] 11-dm-mpath.rules: Remember DM_ACTIVATION
> > 
> > This breaks things. It's often the case that there are devices
> > stacked
> > on top of multipath.  When multipath loses all of its paths, if it
> > doesn't have queue_if_no_path set, it can fail IO up to these higher
> > devices, forcing them to react to the failure.
> 
> Thinking about this again, most of the stuff we do in the multipath-
> related udev rules is for the queueing case. Without
> "queue_if_no_path", all that checking whether or not the map will
> sustain IO is useless; it would be better to simply pass the device on
> to upper layers, which will try to probe it and fail, which is intended
> in the non-queueing setup. Perhaps we should check "queue_if_no_path"
> early on and skip most of the stuff we  do in the queueing case?

I don't really see the benefit in scanning when we're just going to get
back errors, and lose the stored info in the udev database. If IO
happens to the device, the upper layers will find out that it's broken.
If we can lose all our paths, and then have some come back without any
IO going down, why not hide this from the upper layers?

-Ben
 
> If we did this, maybe even my patches 20/21 might be worth
> reconsideration for the "queue_if_no_path" case only?
> 
> Cheers,
> 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 21/31] 11-dm-mpath.rules: Remember DM_ACTIVATION

[Benjamin Marzinski]

On Thu, Sep 14, 2017 at 03:06:43PM +0200, Martin Wilck wrote:
> Hi Ben,
> 
> On Wed, 2017-09-13 at 16:19 -0500, Benjamin Marzinski wrote:
> > On Sun, Sep 03, 2017 at 12:38:50AM +0200, Martin Wilck wrote:
> > > If DM_ACTIVATION is set by the general dm rules, we need to
> > > bring up this device. But if the mpath device is not ready,
> > > that would be dangerous; it could hang or produce lots of IO
> > > errors. So remember this state, and try to activate when the
> > > map becomes usable later.
> > 
> > NAK. See reasons in
> > 
> > [PATCH 20/31] 11-dm-mpath.rules: don't set READY->ACTIVATION
> 
> Sorry for being slow. Re-reading this, I don't understand any more why
> your valid argument against 20/31 invalidates this one as well. 21/31
> affects only the case MPATH_DEVICE_READY!=0, in which case it will add
> another case where DM_ACTIVATION is set.

If we are always setting DM_ACTIVATION when MPATH_DEVICE_READY changes
from zero to non-zero, we don't need to remember that we had to disable
DM_ACTIVATION when the device wasn't ready, and make sure to set it
now, because we are always setting it when we change to a device ready
state... right?

-Ben

> 
> Regards
> Martin
> 
> > 
> > -Ben
> > 
> > > 
> > > Signed-off-by: Martin Wilck <mwilck@suse.com>
> > > ---
> > >  multipath/11-dm-mpath.rules | 8 ++++++++
> > >  1 file changed, 8 insertions(+)
> > > 
> > > diff --git a/multipath/11-dm-mpath.rules b/multipath/11-dm-
> > > mpath.rules
> > > index 3f47744f..9bfd75f8 100644
> > > --- a/multipath/11-dm-mpath.rules
> > > +++ b/multipath/11-dm-mpath.rules
> > > @@ -4,6 +4,7 @@ ENV{DM_UUID}!="mpath-?*", GOTO="mpath_end"
> > >  
> > >  IMPORT{db}="DM_DISABLE_OTHER_RULES_FLAG_OLD"
> > >  IMPORT{db}="MPATH_DEVICE_READY"
> > > +IMPORT{db}="MPATH_NEEDS_ACTIVATION"
> > >  
> > >  # If this uevent didn't come from dm, don't try to update the
> > >  # device state
> > > @@ -55,6 +56,13 @@ ENV{DM_SUBSYSTEM_UDEV_FLAG0}=="1",
> > > ENV{DM_ACTIVATION}="0"
> > >  # We'd like to avoid this, especially within udev processing.
> > >  ENV{MPATH_DEVICE_READY}=="0", ENV{DM_NOSCAN}="1"
> > >  
> > > +# If DM_ACTIVATION is set, but can't be satisfied, remember it
> > > +# in MPATH_NEEDS_ACTIVATION, and activate at the next opportunity.
> > > +ENV{MPATH_DEVICE_READY}=="0", ENV{DM_ACTIVATION}=="1", \
> > > +	ENV{MPATH_NEEDS_ACTIVATION}="1", ENV{DM_ACTIVATION}="0"
> > > +ENV{MPATH_DEVICE_READY}!="0", ENV{MPATH_NEEDS_ACTIVATION}=="1", \
> > > +	ENV{DM_ACTIVATION}="1", ENV{MPATH_NEEDS_ACTIVATION}=""
> > > +
> > >  # Also skip all foreign rules if no path is available.
> > >  # Remember the original value of DM_DISABLE_OTHER_RULES_FLAG
> > >  # and restore it back once we have at least one path available.
> > > -- 
> > > 2.14.0
> > 
> > 
> 
> -- 
> 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 15/31] multipath: implement "check usable paths" (-C/-U)

[Benjamin Marzinski]

On Thu, Sep 14, 2017 at 01:47:31PM +0200, Martin Wilck wrote:
> On Wed, 2017-09-13 at 15:53 -0500, Benjamin Marzinski wrote:
> > On Sun, Sep 03, 2017 at 12:38:44AM +0200, Martin Wilck wrote:
> > > When we process udev rules, it's crucial to know whether I/O on a
> > > given
> > > device will succeed. Unfortunately DM_NR_VALID_PATHS is not
> > > reliable,
> > > because the kernel path events aren't necessarily received in
> > > order, and
> > > even if they are, the number of usable paths may have changed
> > > during
> > > udev processing, in particular when there's a lot of load on udev
> > > because many paths are failing or reinstating at the same time.
> > > The latter problem can't be completely avoided, but the closer the
> > > test before the actual "blkid" call, the better.
> > > 
> > > This patch adds the -C/-U options to multipath to check if a given
> > > map has usable paths. Obviously this command must avoid doing any
> > > I/O
> > > on the multipath map itself, thus no checkers are called; only
> > > status
> > > from sysfs and dm is collected.
> > 
> > I'm a little worried about the overhead of adding yet more multipath
> > commands to udev.  The multipath command takes a while to exec, and
> > already udev hits issues where in event storms, udev can time out
> > because it's trying to do too much with too short a timeout.
> 
> I was aware of that and tried to make this as lean as possible. On my
> system here it takes about 8ms or 500 sytsem calls, which is roughly
> the same number as "multipath -c" or "kpartx_id", at least in the case
> where there are paths available. AFAICS, most of the time is spent in
> libudev collecting device properties. I haven't studied that in depth
> though. "blkid" calls are much more expensive AFAICT.
> 
> > Do out-of-order uevents really happen? 
> 
> For dm-mpath "path events", yes, I'm positive about that. 
> See an example at http://paste.opensuse.org/28641254. 
> It was taken with an openSUSE Tumbleweed 4.11.8 kernel. It was tkane
> from udev monitor data. 
> See http://paste.opensuse.org/63686952 for the full log.

Ick. That's kind of scary. I haven't been thinking about that
possibility when I've been writing or reviewing things... 
 
> You can see that the time stamps and seqnums increase, but
> DM_NR_VALID_PATHS does not decrease monitonically as you'd expect (my
> script removes all paths of map in order, re-adds them again).
> So far I haven't had the time to analyze this on the kernel side. But
> even if it could be fixed in the kernel, multipathd and the udev rules
> should be able to deal with it.
> 
> So, reinforcing my argument from the log message, I truly believe that
> DM_NR_VALID_PATHS is not something that we should rely upon too much.
> 
> > Delayed ones certainly do, but if
> > we really can see out-of-order events, then all that event coalescing
> > code that got in should get another pass over it, because I'm pretty
> > sure it relied on events not being reordered.
> 
> That would need further examination. I thought that the coalescing
> logic worked mostly on uevents for path devices, not the
> PATH_FAILED/PATH_REINSTATED events for the map devices at which the
> udev rules are looking.

Yeah, it does. I guess the real question is whether the reordering is
happening in udev or in the kernel. If it's in the kernel, it may just
be localized to those uevents.

> > If all we're are worried about is delayed events, then it might be
> > o.k.
> > to just always disable scanning on PATH_FAILED events, because we
> > don't
> > know if there are any more of them. When we reload a device, we
> > already
> > pass the DM_SUBSYSTEM_UDEV_FLAG2 to deal with not having
> > DM_NR_VALID_PATHS on reloads. However, I do realize that a path could
> > fail immediately after the reload, and your patch does a better job
> > keeping that window smaller.
> > 
> > Also, when you have reinstates and failures at the same time, you
> > won't
> > run into problems unless the path you just reinstated immediately
> > fails
> > (otherwise there will be at least one available path, the one you
> > just
> > reinstated).  This certainly can happen. 
> 
> Maybe we could skip calling "multipath -U" for PATH_REINSTATED events.
> You're right, the scenario you just describe is really not that likely.
> 
> > > Unfortunately, in my
> > experience, it usually happens because sysfs says that the path is
> > o.k.
> > but when the kernel tries to do IO to it, it's flaky. The -C/-U
> > callout
> > isn't going to catch those cases, because it doesn't do IO.
> 
> True, but the whole purpose of this patch is to avoid doing IO in the
> first place. We can't do anything about this; both the kernel's and
> multipathd's internal representation can only be approximations of the
> real device state.
> 
> > Now, I agree that you are making the window where things can go wrong
> > smaller, but there is a cost that is being incurred on processing a
> > large number of uevents to make that window smaller, and I don't know
> > exactly how that trade-off works. I've been thinking about making a
> > library interface that multipath would use to do the commands which
> > are
> > also called from udev. That would let udev directly call these
> > commands
> > if they wanted, which would save on the exec time, and cut out any
> > unnecessary cruft that doesn't need to be done for udev to get its
> > information.  That might be a solution, in case we do start seeing
> > more
> > timed-out uevents because of this.
> 
> Sure. I've had a similar thought. My tests with "multipath -U" makes me
> think that most of the time is spent in collecting properties from
> sysfs in libudev. If the code was run in the context of the udev worker
> which might have these properties already cached, performance could be
> much better. I'm not sure what exactly is cached in the udev workers
> though.
> 
> Anyway, back to your NAK on this patch, please consider again. 
> IMO we're a lot safer with this additional check, in particular in view
> of possible out-of-order events.
> 
> I introduced this as a replacement for the original "DM_DEPS" check we
> had at SUSE. We'd found that to be helpful in avoiding problems during
> udev processing in the past. It's always hard to tell if such past
> fixes are still required, but at least for SLES we'd risk to cause
> customer regressions if we simply dropped it, so we prefer to play safe
> here. We can keep this as a SUSE-only patch, if you or others insist
> that "multipath -U" is a bad thing.
> 
> DM_DEPS just checks if there are any paths (valid or not), and comes
> down to a "dmsetup deps" invocation, which takes about 4ms. "multipath
> -U" is slower because it needs to look at the paths, but those
> additional cycles may pay off if we can avoid a blkid call on a device
> with no paths. My first approach to the question "is this map really
> ready for IO?" was indeed just a tiny "dmsetup deps" wrapper. But then
> I realized the ordering problems for uevents shown above, and I
> concluded that a more robust test would be desirable.

I'm not NAKing this patch. I agree that this patch is closing a real
window for errors. I just wanted to preemptively bring up some worries I
have about udev timeouts, and I'm glad to know that you thought about
them as well. If multipath still isn't a major time-sink, then my
worries may well be unfounded, and I'm certainly haven't done any
testing which proves that this patch causes any problems. My goal is
just to make sure that the multipath udev rules are as quick as they can
be, since they already are slower than most of the rules.

If we do start seeing an increase in udev timeouts with this callout,
then we need to consider what to do (possibly by making in a library
function or scrapping in and accepting the increased window for IO to
non-ready devices). But, since you have looked into this, and it doesn't
appear to be an issue, ACK.

-Ben

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

Re: [PATCH 21/31] 11-dm-mpath.rules: Remember DM_ACTIVATION

[Martin Wilck]

On Fri, 2017-09-15 at 15:40 -0500, Benjamin Marzinski wrote:
> On Thu, Sep 14, 2017 at 03:06:43PM +0200, Martin Wilck wrote:
> > Hi Ben,
> > 
> > On Wed, 2017-09-13 at 16:19 -0500, Benjamin Marzinski wrote:
> > > On Sun, Sep 03, 2017 at 12:38:50AM +0200, Martin Wilck wrote:
> > > > If DM_ACTIVATION is set by the general dm rules, we need to
> > > > bring up this device. But if the mpath device is not ready,
> > > > that would be dangerous; it could hang or produce lots of IO
> > > > errors. So remember this state, and try to activate when the
> > > > map becomes usable later.
> > > 
> > > NAK. See reasons in
> > > 
> > > [PATCH 20/31] 11-dm-mpath.rules: don't set READY->ACTIVATION
> > 
> > Sorry for being slow. Re-reading this, I don't understand any more
> > why
> > your valid argument against 20/31 invalidates this one as well.
> > 21/31
> > affects only the case MPATH_DEVICE_READY!=0, in which case it will
> > add
> > another case where DM_ACTIVATION is set.
> 
> If we are always setting DM_ACTIVATION when MPATH_DEVICE_READY
> changes
> from zero to non-zero, we don't need to remember that we had to
> disable
> DM_ACTIVATION when the device wasn't ready, and make sure to set it
> now, because we are always setting it when we change to a device
> ready
> state... right?

Err... yes. I guess I shouldn't post on udev rule logic after midnight.
Sorry.

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