[PATCH v2 00/12] multipath fixes to tableless device handling

[PATCH v2 00/12] multipath fixes to tableless device handling

[Martin Wilck]

This patch set is a re-spin of Ben's previous series by the same name [1].

Patch 1 is my take on fixing the regression with map detection
in multipath-tools 0.10.0. The basic idea is to never fail in dm_get_maps(),
even if libmp_mapinfo() fails on one or more maps. The behavior wrt "empty"
maps, or maps without table, is the same as in 0.9.9 and older, these
maps are simply ignored by multipathd.

Patch 2 improves the semantics for decting maps with multiple targets.

Patch 3-6 are from Ben's original series, with unit test fixes added in
patch 3.

Patch 7 and 8 improve the semantics of MAPINFO_CHECK_UUID for partitions.

Patch 9-11 are minor logging improvements.

Patch 12 is an independent bug fix I discovered recently.

As usual, comments and reviews welcome.

Martin

[1] https://lore.kernel.org/dm-devel/20241031183301.391416-1-bmarzins@redhat.com/

Benjamin Marzinski (4):
  libmultipath: check DM UUID earlier in libmp_mapinfo__
  libmultipath: use MAPINFO_CHECK_UUID in dm_get_multipath
  multipathd: print an error when failing to connect to multipathd
  multipathd.service: restart multipathd on failure

Martin Wilck (8):
  libmultipath: dm_get_maps(): don't bail out for single-map failures
  libmultipath: libmp_mapinfo(): return DMP_NO_MATCH for multi-target
    maps
  libmultipath: make MAPINFO_CHECK_UUID work with partitions
  libmultipath: check map UUID in do_foreach_partmaps
  libmultipath: increase log level for removing partitions
  libmultipath: reduce log level of libmp_mapinfo() messages
  libmultipath: don't log boring state messages at level 3
  libmultipath: don't set dev_loss_tmo to 0 for NO_PATH_RETRY_FAIL

 libmultipath/devmapper.c         | 70 ++++++++++++++++----------------
 libmultipath/devmapper.h         |  8 +++-
 libmultipath/discovery.c         | 13 ++++--
 libmultipath/structs.h           |  1 +
 multipathd/multipathd.service.in |  3 ++
 multipathd/uxclnt.c              |  4 +-
 tests/mapinfo.c                  | 17 +++++---
 7 files changed, 71 insertions(+), 45 deletions(-)

-- 
2.47.0

[PATCH v2 01/12] libmultipath: dm_get_maps(): don't bail out for single-map failures

[Martin Wilck]

dm_get_maps() traverses the entire list of dm maps. We shouldn't
give up just because probing a single map failed.

Based on an earlier patch from Benjamin Marzinski <bmarzins@redhat.com>

Fixes: bf3a4ad ("libmultipath: simplify dm_get_maps()")
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/devmapper.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/libmultipath/devmapper.c b/libmultipath/devmapper.c
index c497c22..52bfe9c 100644
--- a/libmultipath/devmapper.c
+++ b/libmultipath/devmapper.c
@@ -1262,10 +1262,8 @@ int dm_get_maps(vector mp)
 			}
 			vector_set_slot(mp, mpp);
 			break;
-		case DMP_NO_MATCH:
-			break;
 		default:
-			return 1;
+			break;
 		}
 		next = names->next;
 		names = (void *) names + next;
-- 
2.47.0

[PATCH v2 02/12] libmultipath: libmp_mapinfo(): return DMP_NO_MATCH for multi-target maps

[Martin Wilck]

multi-target maps are more like maps with a single non-multipath
target than like maps with no target at all.

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

diff --git a/libmultipath/devmapper.c b/libmultipath/devmapper.c
index 52bfe9c..ab6eefc 100644
--- a/libmultipath/devmapper.c
+++ b/libmultipath/devmapper.c
@@ -719,7 +719,7 @@ static int libmp_mapinfo__(int flags, mapid_t id, mapinfo_t info, const char *ma
 		if (dm_get_next_target(dmt, NULL, &start, &length,
 				       &target_type, &params) != NULL) {
 			condlog(2, "%s: map %s has multiple targets", fname__, map_id);
-			return DMP_NOT_FOUND;
+			return DMP_NO_MATCH;
 		}
 		if (!params) {
 			condlog(2, "%s: map %s has no targets", fname__, map_id);
diff --git a/tests/mapinfo.c b/tests/mapinfo.c
index fca6462..66c81e8 100644
--- a/tests/mapinfo.c
+++ b/tests/mapinfo.c
@@ -870,7 +870,7 @@ static void test_mapinfo_bad_next_target_01(void **state)
 	rc = libmp_mapinfo(DM_MAP_BY_NAME,
 			   (mapid_t) { .str = "foo", },
 			   (mapinfo_t) { .size = &size });
-	assert_int_equal(rc, DMP_NOT_FOUND);
+	assert_int_equal(rc, DMP_NO_MATCH);
 }
 
 static void test_mapinfo_bad_next_target_02(void **state)
-- 
2.47.0

Re: [PATCH v2 02/12] libmultipath: libmp_mapinfo(): return DMP_NO_MATCH for multi-target maps

[Benjamin Marzinski]

On Tue, Nov 12, 2024 at 04:02:05PM +0100, Martin Wilck wrote:
> multi-target maps are more like maps with a single non-multipath
> target than like maps with no target at all.
> 
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  libmultipath/devmapper.c | 2 +-
>  tests/mapinfo.c          | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/libmultipath/devmapper.c b/libmultipath/devmapper.c
> index 52bfe9c..ab6eefc 100644
> --- a/libmultipath/devmapper.c
> +++ b/libmultipath/devmapper.c
> @@ -719,7 +719,7 @@ static int libmp_mapinfo__(int flags, mapid_t id, mapinfo_t info, const char *ma
>  		if (dm_get_next_target(dmt, NULL, &start, &length,
>  				       &target_type, &params) != NULL) {
>  			condlog(2, "%s: map %s has multiple targets", fname__, map_id);
> -			return DMP_NOT_FOUND;
> +			return DMP_NO_MATCH;
>  		}
>  		if (!params) {
>  			condlog(2, "%s: map %s has no targets", fname__, map_id);

I feel like given the choice between DMP_NOT_FOUND and DMP_NO_MATCH, an
empty table should be DMP_NO_MATCH since we did find a device. I
assume you left this case alone because it will get changed with the
addition of a new error code for empty tables (I plan on posting that
shortly). So

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

> diff --git a/tests/mapinfo.c b/tests/mapinfo.c
> index fca6462..66c81e8 100644
> --- a/tests/mapinfo.c
> +++ b/tests/mapinfo.c
> @@ -870,7 +870,7 @@ static void test_mapinfo_bad_next_target_01(void **state)
>  	rc = libmp_mapinfo(DM_MAP_BY_NAME,
>  			   (mapid_t) { .str = "foo", },
>  			   (mapinfo_t) { .size = &size });
> -	assert_int_equal(rc, DMP_NOT_FOUND);
> +	assert_int_equal(rc, DMP_NO_MATCH);
>  }
>  
>  static void test_mapinfo_bad_next_target_02(void **state)
> -- 
> 2.47.0

[PATCH v2 03/12] libmultipath: check DM UUID earlier in libmp_mapinfo__

[Martin Wilck]

From: Benjamin Marzinski <bmarzins@redhat.com>

Before checking the target details, first check that the device has a
"mpath-" dm uuid prefix. If it doesn't then we can just ignore the
device. This keeps multipath from printing error messages for
non-multipath devices with multiple targets for instance.

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
Reviewed-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/devmapper.c | 20 +++++++++++---------
 tests/mapinfo.c          | 15 +++++++++++----
 2 files changed, 22 insertions(+), 13 deletions(-)

diff --git a/libmultipath/devmapper.c b/libmultipath/devmapper.c
index ab6eefc..93fbc4a 100644
--- a/libmultipath/devmapper.c
+++ b/libmultipath/devmapper.c
@@ -715,6 +715,16 @@ static int libmp_mapinfo__(int flags, mapid_t id, mapinfo_t info, const char *ma
 		return DMP_NOT_FOUND;
 	}
 
+	if ((info.name && !(name = dm_task_get_name(dmt)))
+	    || ((info.uuid || flags & MAPINFO_CHECK_UUID)
+		&& !(uuid = dm_task_get_uuid(dmt))))
+		return DMP_ERR;
+
+	if (flags & MAPINFO_CHECK_UUID && !is_mpath_uuid(uuid)) {
+		condlog(3, "%s: UUID mismatch: %s", fname__, uuid);
+		return DMP_NO_MATCH;
+	}
+
 	if (info.target || info.status || info.size || flags & MAPINFO_TGT_TYPE__) {
 		if (dm_get_next_target(dmt, NULL, &start, &length,
 				       &target_type, &params) != NULL) {
@@ -740,18 +750,10 @@ static int libmp_mapinfo__(int flags, mapid_t id, mapinfo_t info, const char *ma
 	 * Check possible error conditions.
 	 * If error is returned, don't touch any output parameters.
 	 */
-	if ((info.name && !(name = dm_task_get_name(dmt)))
-	    || ((info.uuid || flags & MAPINFO_CHECK_UUID)
-		&& !(uuid = dm_task_get_uuid(dmt)))
-	    || (info.status && !(tmp_status = strdup(params)))
+	if ((info.status && !(tmp_status = strdup(params)))
 	    || (info.target && !tmp_target && !(tmp_target = strdup(params))))
 		return DMP_ERR;
 
-	if (flags & MAPINFO_CHECK_UUID && !is_mpath_uuid(uuid)) {
-		condlog(3, "%s: UUID mismatch: %s", fname__, uuid);
-		return DMP_NO_MATCH;
-	}
-
 	if (info.name) {
 		strlcpy(info.name, name, WWID_SIZE);
 		condlog(4, "%s: %s: name: \"%s\"", fname__, map_id, info.name);
diff --git a/tests/mapinfo.c b/tests/mapinfo.c
index 66c81e8..4362cdb 100644
--- a/tests/mapinfo.c
+++ b/tests/mapinfo.c
@@ -43,6 +43,15 @@ static const struct dm_info __attribute__((unused)) MPATH_DMI_01 = {
 	.minor = 123,
 };
 
+static const struct dm_info __attribute__((unused)) MPATH_DMI_02 = {
+	.exists = 1,
+	.live_table = 0,
+	.open_count = 1,
+	.target_count = 1,
+	.major = 254,
+	.minor = 123,
+};
+
 static const char MPATH_NAME_01[] = "mpathx";
 static const char MPATH_UUID_01[] = "mpath-3600a098038302d414b2b4d4453474f62";
 static const char MPATH_TARGET_01[] =
@@ -928,6 +937,8 @@ static void test_mapinfo_bad_target_type_03(void **state)
 	mock_mapinfo_name_1(DM_DEVICE_STATUS, 1, "foo", 1, 1, 0);
 	WRAP_DM_TASK_GET_INFO(1);
 	WRAP_DM_TASK_GET_INFO(&MPATH_DMI_01);
+	will_return(__wrap_dm_task_get_name, MPATH_NAME_01);
+	will_return(__wrap_dm_task_get_uuid, MPATH_UUID_01);
 	mock_dm_get_next_target(12345, TGT_PART, MPATH_STATUS_01, NULL);
 	rc = libmp_mapinfo(DM_MAP_BY_NAME | MAPINFO_MPATH_ONLY,
 			   (mapid_t) { .str = "foo", },
@@ -1090,7 +1101,6 @@ static void test_mapinfo_bad_get_name_01(void **state)
 	mock_mapinfo_name_1(DM_DEVICE_STATUS, 1, "foo", 1, 1, 0);
 	WRAP_DM_TASK_GET_INFO(1);
 	WRAP_DM_TASK_GET_INFO(&MPATH_DMI_01);
-	mock_dm_get_next_target(12345, TGT_MPATH, MPATH_STATUS_01, NULL);
 	will_return(__wrap_dm_task_get_name, NULL);
 	rc = libmp_mapinfo(DM_MAP_BY_NAME,
 			   (mapid_t) { .str = "foo", },
@@ -1112,7 +1122,6 @@ static void test_mapinfo_bad_get_uuid_01(void **state)
 	mock_mapinfo_name_1(DM_DEVICE_STATUS, 1, "foo", 1, 1, 0);
 	WRAP_DM_TASK_GET_INFO(1);
 	WRAP_DM_TASK_GET_INFO(&MPATH_DMI_01);
-	mock_dm_get_next_target(12345, TGT_MPATH, MPATH_STATUS_01, NULL);
 	will_return(__wrap_dm_task_get_name, MPATH_NAME_01);
 	will_return(__wrap_dm_task_get_uuid, NULL);
 	rc = libmp_mapinfo(DM_MAP_BY_NAME,
@@ -1162,7 +1171,6 @@ static void test_mapinfo_bad_get_name_02(void **state)
 	mock_mapinfo_name_1(DM_DEVICE_STATUS, 1, "foo", 1, 1, 0);
 	WRAP_DM_TASK_GET_INFO(1);
 	WRAP_DM_TASK_GET_INFO(&MPATH_DMI_01);
-	mock_dm_get_next_target(12345, TGT_MPATH, MPATH_STATUS_01, NULL);
 	will_return(__wrap_dm_task_get_name, NULL);
 
 	rc = libmp_mapinfo(DM_MAP_BY_NAME,
@@ -1195,7 +1203,6 @@ static void test_mapinfo_bad_get_uuid_02(void **state)
 	mock_mapinfo_name_1(DM_DEVICE_STATUS, 1, "foo", 1, 1, 0);
 	WRAP_DM_TASK_GET_INFO(1);
 	WRAP_DM_TASK_GET_INFO(&MPATH_DMI_01);
-	mock_dm_get_next_target(12345, TGT_MPATH, MPATH_STATUS_01, NULL);
 	will_return(__wrap_dm_task_get_name, MPATH_NAME_01);
 	will_return(__wrap_dm_task_get_uuid, NULL);
 
-- 
2.47.0

[PATCH v2 04/12] libmultipath: use MAPINFO_CHECK_UUID in dm_get_multipath

[Martin Wilck]

From: Benjamin Marzinski <bmarzins@redhat.com>

Instead of seperately calling is_mpath_uuid(), just use
MAPINFO_CHECK_UUID when calling libmp_mapinfo.

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
Reviewed-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/devmapper.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/libmultipath/devmapper.c b/libmultipath/devmapper.c
index 93fbc4a..d193586 100644
--- a/libmultipath/devmapper.c
+++ b/libmultipath/devmapper.c
@@ -1211,7 +1211,8 @@ static int dm_get_multipath(const char *name, struct multipath **pmpp)
 	if (!mpp->alias)
 		return DMP_ERR;
 
-	if ((rc = libmp_mapinfo(DM_MAP_BY_NAME | MAPINFO_MPATH_ONLY,
+	if ((rc = libmp_mapinfo(DM_MAP_BY_NAME | MAPINFO_CHECK_UUID |
+				MAPINFO_MPATH_ONLY,
 			  (mapid_t) { .str = name },
 			  (mapinfo_t) {
 				  .size = &mpp->size,
@@ -1220,9 +1221,6 @@ static int dm_get_multipath(const char *name, struct multipath **pmpp)
 			  })) != DMP_OK)
 		return rc;
 
-	if (!is_mpath_uuid(uuid))
-		return DMP_NO_MATCH;
-
 	strlcpy(mpp->wwid, uuid + UUID_PREFIX_LEN, sizeof(mpp->wwid));
 	*pmpp = steal_ptr(mpp);
 
-- 
2.47.0

[PATCH v2 05/12] multipathd: print an error when failing to connect to multipathd

[Martin Wilck]

From: Benjamin Marzinski <bmarzins@redhat.com>

Issuing multipathd commands when the daemon wasn't running didn't
print anything.

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
Reviewed-by: Martin Wilck <mwilck@suse.com>
---
 multipathd/uxclnt.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/multipathd/uxclnt.c b/multipathd/uxclnt.c
index 16133a8..55d253a 100644
--- a/multipathd/uxclnt.c
+++ b/multipathd/uxclnt.c
@@ -52,8 +52,10 @@ int uxclnt(char * inbuf, unsigned int timeout)
 	if (!inbuf)
 		return 1;
 	fd = mpath_connect();
-	if (fd == -1)
+	if (fd == -1) {
+		fprintf(stderr, "ERROR: failed to connect to multipathd\n");
 		return 1;
+	}
 
 	ret = process_req(fd, inbuf, timeout);
 
-- 
2.47.0

[PATCH v2 06/12] multipathd.service: restart multipathd on failure

[Martin Wilck]

From: Benjamin Marzinski <bmarzins@redhat.com>

systemd will now restart multipathd on failure unless it has already been
started 3 times in the last 30 seconds.

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
Reviewed-by: Martin Wilck <mwilck@suse.com>
---
 multipathd/multipathd.service.in | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/multipathd/multipathd.service.in b/multipathd/multipathd.service.in
index 646001e..b6a25b3 100644
--- a/multipathd/multipathd.service.in
+++ b/multipathd/multipathd.service.in
@@ -12,12 +12,15 @@ Conflicts=initrd-cleanup.service
 ConditionKernelCommandLine=!nompath
 ConditionKernelCommandLine=!multipath=off
 ConditionVirtualization=!container
+StartLimitIntervalSec=30
+StartLimitBurst=3
 
 [Service]
 Type=notify
 NotifyAccess=main
 ExecStart=@BINDIR@/multipathd -d -s
 ExecReload=@BINDIR@/multipathd reconfigure
+Restart=on-failure
 TasksMax=infinity
 LimitRTPRIO=10
 CPUWeight=1000
-- 
2.47.0

[PATCH v2 07/12] libmultipath: make MAPINFO_CHECK_UUID work with partitions

[Martin Wilck]

The semantics of MAPINFO_CHECK_UUID, MAPINFO_MPATH_ONLY and MAPINFO_PART_ONLY
are confusing. Fix that by supporting UUID check for partitions, too.

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

diff --git a/libmultipath/devmapper.c b/libmultipath/devmapper.c
index d193586..03dae16 100644
--- a/libmultipath/devmapper.c
+++ b/libmultipath/devmapper.c
@@ -616,6 +616,18 @@ static bool is_mpath_uuid(const char uuid[DM_UUID_LEN])
 	return !strncmp(uuid, UUID_PREFIX, UUID_PREFIX_LEN);
 }
 
+static bool is_mpath_part_uuid(const char part_uuid[DM_UUID_LEN],
+			       const char map_uuid[DM_UUID_LEN])
+{
+	char c;
+	int np, nc;
+
+	if (2 != sscanf(part_uuid, "part%d-%n" UUID_PREFIX "%c", &np, &nc, &c)
+	    || np <= 0)
+		return false;
+	return map_uuid == NULL || !strcmp(part_uuid + nc, map_uuid);
+}
+
 bool
 has_dm_info(const struct multipath *mpp)
 {
@@ -720,8 +732,10 @@ static int libmp_mapinfo__(int flags, mapid_t id, mapinfo_t info, const char *ma
 		&& !(uuid = dm_task_get_uuid(dmt))))
 		return DMP_ERR;
 
-	if (flags & MAPINFO_CHECK_UUID && !is_mpath_uuid(uuid)) {
-		condlog(3, "%s: UUID mismatch: %s", fname__, uuid);
+	if (flags & MAPINFO_CHECK_UUID &&
+	    ((flags & MAPINFO_PART_ONLY && !is_mpath_part_uuid(uuid, NULL)) ||
+	     !is_mpath_uuid(uuid))) {
+		condlog(4, "%s: UUID mismatch: %s", fname__, uuid);
 		return DMP_NO_MATCH;
 	}
 
@@ -846,18 +860,6 @@ int dm_get_wwid(const char *name, char *uuid, int uuid_len)
 	return DMP_OK;
 }
 
-static bool is_mpath_part_uuid(const char part_uuid[DM_UUID_LEN],
-			       const char map_uuid[DM_UUID_LEN])
-{
-	char c;
-	int np, nc;
-
-	if (2 != sscanf(part_uuid, "part%d-%n" UUID_PREFIX "%c", &np, &nc, &c)
-	    || np <= 0)
-		return false;
-	return !strcmp(part_uuid + nc, map_uuid);
-}
-
 int dm_is_mpath(const char *name)
 {
 	int rc = libmp_mapinfo(DM_MAP_BY_NAME | MAPINFO_MPATH_ONLY | MAPINFO_CHECK_UUID,
diff --git a/libmultipath/devmapper.h b/libmultipath/devmapper.h
index ba05e0a..6b3bbad 100644
--- a/libmultipath/devmapper.h
+++ b/libmultipath/devmapper.h
@@ -58,7 +58,13 @@ enum {
 	/* Fail if target type is not "partition" (linear) */
 	MAPINFO_PART_ONLY   = (1 << 9),
 	MAPINFO_TGT_TYPE__  = (MAPINFO_MPATH_ONLY | MAPINFO_PART_ONLY),
-	/* Fail if the UUID doesn't match the multipath UUID format */
+	/*
+	 * Fail if the UUID doesn't match the expected UUID format
+	 * If combined with MAPINFO_PART_ONLY, checks for partition UUID format
+	 * ("part<N>-mpath-xyz").
+	 * Otherwise (whether or not MAPINFO_MPATH_ONLY is set) checks for
+	 * multipath UUID format ("mpath-xyz").
+	 */
 	MAPINFO_CHECK_UUID  = (1 << 10),
 };
 
-- 
2.47.0

[PATCH v2 08/12] libmultipath: check map UUID in do_foreach_partmaps

[Martin Wilck]

Don't try to remove any non-standard partition mappings.

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

diff --git a/libmultipath/devmapper.c b/libmultipath/devmapper.c
index 03dae16..3ab231e 100644
--- a/libmultipath/devmapper.c
+++ b/libmultipath/devmapper.c
@@ -1349,7 +1349,7 @@ do_foreach_partmaps (const char *mapname,
 		    /*
 		     * if there is only a single "linear" target
 		     */
-		    libmp_mapinfo(DM_MAP_BY_NAME | MAPINFO_PART_ONLY,
+		    libmp_mapinfo(DM_MAP_BY_NAME | MAPINFO_PART_ONLY | MAPINFO_CHECK_UUID,
 				  (mapid_t) { .str = names->name },
 				  (mapinfo_t) {
 					  .uuid = part_uuid,
-- 
2.47.0

[PATCH v2 09/12] libmultipath: increase log level for removing partitions

[Martin Wilck]

Removing a partition map is an important action that should be logged
at verbosity level 3.

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

diff --git a/libmultipath/devmapper.c b/libmultipath/devmapper.c
index 3ab231e..3502d05 100644
--- a/libmultipath/devmapper.c
+++ b/libmultipath/devmapper.c
@@ -1392,7 +1392,7 @@ remove_partmap(const char *name, void *data)
 		condlog(2, "%s: map in use", name);
 		return DM_FLUSH_BUSY;
 	}
-	condlog(4, "partition map %s removed", name);
+	condlog(3, "partition map %s removed", name);
 	dm_device_remove(name, rd->flags);
 	return DM_FLUSH_OK;
 }
-- 
2.47.0

[PATCH v2 10/12] libmultipath: reduce log level of libmp_mapinfo() messages

[Martin Wilck]

Unless  MAPINFO_CHECK_UUID was set and we know that we are
looking at a map with a multipath UUID, encountering non-multipath
maps is not an error. Don't log this at verbosity level 2.

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

diff --git a/libmultipath/devmapper.c b/libmultipath/devmapper.c
index 3502d05..9714270 100644
--- a/libmultipath/devmapper.c
+++ b/libmultipath/devmapper.c
@@ -740,20 +740,22 @@ static int libmp_mapinfo__(int flags, mapid_t id, mapinfo_t info, const char *ma
 	}
 
 	if (info.target || info.status || info.size || flags & MAPINFO_TGT_TYPE__) {
+		int lvl = MAPINFO_CHECK_UUID ? 2 : 4;
+
 		if (dm_get_next_target(dmt, NULL, &start, &length,
 				       &target_type, &params) != NULL) {
-			condlog(2, "%s: map %s has multiple targets", fname__, map_id);
+			condlog(lvl, "%s: map %s has multiple targets", fname__, map_id);
 			return DMP_NO_MATCH;
 		}
 		if (!params) {
-			condlog(2, "%s: map %s has no targets", fname__, map_id);
+			condlog(lvl, "%s: map %s has no targets", fname__, map_id);
 			return DMP_NOT_FOUND;
 		}
 		if (flags & MAPINFO_TGT_TYPE__) {
 			const char *tgt_type = flags & MAPINFO_MPATH_ONLY ? TGT_MPATH : TGT_PART;
 
 			if (strcmp(target_type, tgt_type)) {
-				condlog(3, "%s: target type mismatch: \"%s\" != \"%s\"",
+				condlog(lvl, "%s: target type mismatch: \"%s\" != \"%s\"",
 					fname__, tgt_type, target_type);
 				return DMP_NO_MATCH;
 			}
-- 
2.47.0

[PATCH v2 11/12] libmultipath: don't log boring state messages at level 3

[Martin Wilck]

Even at verbosity level 3, it isn't interesting and actually disturbing
to see every successful state and pending state logged by multipathd.
Suppress these messages at level 3.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/discovery.c | 11 ++++++++---
 libmultipath/structs.h   |  1 +
 2 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
index 1d48c30..ae9fc8f 100644
--- a/libmultipath/discovery.c
+++ b/libmultipath/discovery.c
@@ -2018,15 +2018,20 @@ int
 get_state (struct path * pp)
 {
 	struct checker * c = &pp->checker;
-	int state;
+	int state, lvl;
 
 	state = checker_get_state(c);
-	condlog(3, "%s: %s state = %s", pp->dev,
+
+	lvl = state == pp->oldstate || state == PATH_PENDING ? 4 : 3;
+	condlog(lvl, "%s: %s state = %s", pp->dev,
 		checker_name(c), checker_state_name(state));
 	if (state != PATH_UP && state != PATH_GHOST &&
 	    strlen(checker_message(c)))
-		condlog(3, "%s: %s checker%s",
+		condlog(lvl, "%s: %s checker%s",
 			pp->dev, checker_name(c), checker_message(c));
+	if (state != PATH_PENDING)
+		pp->oldstate = state;
+
 	return state;
 }
 
diff --git a/libmultipath/structs.h b/libmultipath/structs.h
index 1f531d3..4821f19 100644
--- a/libmultipath/structs.h
+++ b/libmultipath/structs.h
@@ -375,6 +375,7 @@ struct path {
 	int state;
 	int dmstate;
 	int chkrstate;
+	int oldstate;
 	int failcount;
 	int priority;
 	int pgindex;
-- 
2.47.0

[PATCH v2 12/12] libmultipath: don't set dev_loss_tmo to 0 for NO_PATH_RETRY_FAIL

[Martin Wilck]

If pp->dev_loss is DEV_LOSS_TMO_UNSET and min_dev_loss is 0 (which is
the case if no_path_retry is NO_PATH_RETRY_FAIL or NO_PATH_RETRY_UNDEF),
we will set pp->dev_loss to 0, which is wrong. Fix it.

Fixes: 058b5f5 ("libmultipath: fix dev_loss_tmo even if not set in configuration")
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/discovery.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
index ae9fc8f..b585156 100644
--- a/libmultipath/discovery.c
+++ b/libmultipath/discovery.c
@@ -942,7 +942,7 @@ sysfs_set_scsi_tmo (struct config *conf, struct multipath *mpp)
 			continue;
 		}
 
-		if (pp->dev_loss == DEV_LOSS_TMO_UNSET)
+		if (pp->dev_loss == DEV_LOSS_TMO_UNSET && min_dev_loss != 0)
 			pp->dev_loss = min_dev_loss;
 		else if (pp->dev_loss < min_dev_loss) {
 			pp->dev_loss = min_dev_loss;
-- 
2.47.0

Re: [PATCH v2 00/12] multipath fixes to tableless device handling

[Benjamin Marzinski]

On Tue, Nov 12, 2024 at 04:02:03PM +0100, Martin Wilck wrote:
> This patch set is a re-spin of Ben's previous series by the same name [1].

For the set:
Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
 
> Patch 1 is my take on fixing the regression with map detection
> in multipath-tools 0.10.0. The basic idea is to never fail in dm_get_maps(),
> even if libmp_mapinfo() fails on one or more maps. The behavior wrt "empty"
> maps, or maps without table, is the same as in 0.9.9 and older, these
> maps are simply ignored by multipathd.
> 
> Patch 2 improves the semantics for decting maps with multiple targets.
> 
> Patch 3-6 are from Ben's original series, with unit test fixes added in
> patch 3.
> 
> Patch 7 and 8 improve the semantics of MAPINFO_CHECK_UUID for partitions.
> 
> Patch 9-11 are minor logging improvements.
> 
> Patch 12 is an independent bug fix I discovered recently.
> 
> As usual, comments and reviews welcome.
> 
> Martin
> 
> [1] https://lore.kernel.org/dm-devel/20241031183301.391416-1-bmarzins@redhat.com/
> 
> Benjamin Marzinski (4):
>   libmultipath: check DM UUID earlier in libmp_mapinfo__
>   libmultipath: use MAPINFO_CHECK_UUID in dm_get_multipath
>   multipathd: print an error when failing to connect to multipathd
>   multipathd.service: restart multipathd on failure
> 
> Martin Wilck (8):
>   libmultipath: dm_get_maps(): don't bail out for single-map failures
>   libmultipath: libmp_mapinfo(): return DMP_NO_MATCH for multi-target
>     maps
>   libmultipath: make MAPINFO_CHECK_UUID work with partitions
>   libmultipath: check map UUID in do_foreach_partmaps
>   libmultipath: increase log level for removing partitions
>   libmultipath: reduce log level of libmp_mapinfo() messages
>   libmultipath: don't log boring state messages at level 3
>   libmultipath: don't set dev_loss_tmo to 0 for NO_PATH_RETRY_FAIL
> 
>  libmultipath/devmapper.c         | 70 ++++++++++++++++----------------
>  libmultipath/devmapper.h         |  8 +++-
>  libmultipath/discovery.c         | 13 ++++--
>  libmultipath/structs.h           |  1 +
>  multipathd/multipathd.service.in |  3 ++
>  multipathd/uxclnt.c              |  4 +-
>  tests/mapinfo.c                  | 17 +++++---
>  7 files changed, 71 insertions(+), 45 deletions(-)
> 
> -- 
> 2.47.0