[dm-devel] [PATCH 1/2] libmultipath: merge update_multipath_table() and update_multipath_status()

[dm-devel] [PATCH 1/2] libmultipath: merge update_multipath_table() and update_multipath_status()

[mwilck]

From: Martin Wilck <mwilck@suse.com>

Since 378cb66 ("multipath: use update_pathvec_from_dm()"),
we remove paths and even pathgroups from multipathd's data structures
in update_multipath_table() if these paths are found to be non-existent.
But update_multipath_status() is called afterwards, and it
uses the kernel's mapping of pathgroups and paths, which won't match
any more if any members had been removed. disassemble_status() returns
an error if the number of path groups doesn't match, causing the
entire structure setup to fail. And because disassemble_status()
doesn't check the dev_t against the corresponding values in multipathd's
data structures, it may assign wrong DM state to paths.

Fix this by calling disassemble_status() before making any changes to
the data structure in update_pathvec_from_dm(). This can be easily
done, because every call to update_multipath_status() is preceded
by a call to update_multipath_table() anyway, and vice versa. So
we simply merge the two functions into one. This actually simplifies
the code for all callers.

As we remove a symbol, the major library version must be bumped.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmpathpersist/mpath_persist.c   |  1 -
 libmultipath/libmultipath.version | 30 ++++++++-----------------
 libmultipath/structs_vec.c        | 37 ++++++-------------------------
 multipath/main.c                  |  6 ++---
 multipathd/main.c                 |  5 +----
 5 files changed, 19 insertions(+), 60 deletions(-)

diff --git a/libmpathpersist/mpath_persist.c b/libmpathpersist/mpath_persist.c
index 5c95af2..190e970 100644
--- a/libmpathpersist/mpath_persist.c
+++ b/libmpathpersist/mpath_persist.c
@@ -408,7 +408,6 @@ get_mpvec (vector curmp, vector pathvec, char * refwwid)
 			continue;
 
 		if (update_multipath_table(mpp, pathvec, DI_CHECKER) != DMP_OK ||
-		    update_multipath_status(mpp) != DMP_OK ||
 		    update_mpp_paths(mpp, pathvec)) {
 			condlog(1, "error parsing map %s", mpp->wwid);
 			remove_map(mpp, pathvec, curmp, PURGE_VEC);
diff --git a/libmultipath/libmultipath.version b/libmultipath/libmultipath.version
index e9b4608..0cff311 100644
--- a/libmultipath/libmultipath.version
+++ b/libmultipath/libmultipath.version
@@ -31,7 +31,7 @@
  *   The new version inherits the previous ones.
  */
 
-LIBMULTIPATH_4.0.0 {
+LIBMULTIPATH_5.0.0 {
 global:
 	/* symbols referenced by multipath and multipathd */
 	add_foreign;
@@ -198,7 +198,6 @@ global:
 	uevent_is_mpath;
 	uevent_listen;
 	update_mpp_paths;
-	update_multipath_status;
 	update_multipath_strings;
 	update_multipath_table;
 	update_pathvec_from_dm;
@@ -256,33 +255,22 @@ global:
 	libmultipath_init;
 	libmultipath_exit;
 
-local:
-	*;
-};
-
-LIBMULTIPATH_4.1.0 {
-global:
+	/* added in 4.1.0 */
 	libmp_verbosity;
-} LIBMULTIPATH_4.0.0;
 
-LIBMULTIPATH_4.2.0 {
-global:
+	/* added in 4.2.0 */
 	dm_prereq;
 	skip_libmp_dm_init;
-} LIBMULTIPATH_4.1.0;
 
-LIBMULTIPATH_4.3.0 {
-global:
+	/* added in 4.3.0 */
 	start_checker_thread;
-} LIBMULTIPATH_4.2.0;
 
-LIBMULTIPATH_4.4.0 {
-global:
+	/* added in 4.4.0 */
 	get_next_string;
-} LIBMULTIPATH_4.3.0;
 
-LIBMULITIPATH_4.5.0 {
-global:
+	/* added in 4.5.0 */
 	get_vpd_sgio;
 	trigger_partitions_udev_change;
-} LIBMULTIPATH_4.4.0;
+local:
+	*;
+};
diff --git a/libmultipath/structs_vec.c b/libmultipath/structs_vec.c
index 57cd88a..33c522f 100644
--- a/libmultipath/structs_vec.c
+++ b/libmultipath/structs_vec.c
@@ -432,31 +432,14 @@ update_multipath_table (struct multipath *mpp, vector pathvec, int flags)
 		return DMP_ERR;
 	}
 
-	/* FIXME: we should deal with the return value here */
-	update_pathvec_from_dm(pathvec, mpp, flags);
-
-	return DMP_OK;
-}
-
-int
-update_multipath_status (struct multipath *mpp)
-{
-	int r = DMP_ERR;
-	char status[PARAMS_SIZE] = {0};
-
-	if (!mpp)
-		return r;
-
-	r = dm_get_status(mpp->alias, status);
-	if (r != DMP_OK) {
+	*params = '\0';
+	if (dm_get_status(mpp->alias, params) != DMP_OK)
 		condlog(3, "%s: %s", mpp->alias, (r == DMP_ERR)? "error getting status" : "map not present");
-		return r;
-	}
-
-	if (disassemble_status(status, mpp)) {
+	else if (disassemble_status(params, mpp))
 		condlog(3, "%s: cannot disassemble status", mpp->alias);
-		return DMP_ERR;
-	}
+
+	/* FIXME: we should deal with the return value here */
+	update_pathvec_from_dm(pathvec, mpp, flags);
 
 	return DMP_OK;
 }
@@ -536,19 +519,13 @@ update_multipath_strings(struct multipath *mpp, vector pathvec)
 	mpp->pg = NULL;
 
 	r = update_multipath_table(mpp, pathvec, 0);
-	if (r != DMP_OK)
-		return r;
 	sync_paths(mpp, pathvec);
 
-	r = update_multipath_status(mpp);
-	if (r != DMP_OK)
-		return r;
-
 	vector_foreach_slot(mpp->pg, pgp, i)
 		if (pgp->paths)
 			path_group_prio_update(pgp);
 
-	return DMP_OK;
+	return r;
 }
 
 static void enter_recovery_mode(struct multipath *mpp)
diff --git a/multipath/main.c b/multipath/main.c
index 3f97582..ef89c7c 100644
--- a/multipath/main.c
+++ b/multipath/main.c
@@ -196,8 +196,7 @@ get_dm_mpvec (enum mpath_cmds cmd, vector curmp, vector pathvec, char * refwwid)
 			continue;
 		}
 
-		if (update_multipath_table(mpp, pathvec, flags) != DMP_OK ||
-		    update_multipath_status(mpp) != DMP_OK) {
+		if (update_multipath_table(mpp, pathvec, flags) != DMP_OK) {
 			condlog(1, "error parsing map %s", mpp->wwid);
 			remove_map(mpp, pathvec, curmp, PURGE_VEC);
 			i--;
@@ -263,8 +262,7 @@ static int check_usable_paths(struct config *conf,
 	if (mpp == NULL)
 		goto free;
 
-	if (update_multipath_table(mpp, pathvec, 0) != DMP_OK ||
-		    update_multipath_status(mpp) != DMP_OK)
+	if (update_multipath_table(mpp, pathvec, 0) != DMP_OK)
 		    goto free;
 
 	vector_foreach_slot (mpp->pg, pg, i) {
diff --git a/multipathd/main.c b/multipathd/main.c
index e0797cc..154a4ee 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -559,8 +559,6 @@ add_map_without_path (struct vectors *vecs, const char *alias)
 
 	if (update_multipath_table(mpp, vecs->pathvec, 0) != DMP_OK)
 		goto out;
-	if (update_multipath_status(mpp) != DMP_OK)
-		goto out;
 
 	if (!vector_alloc_slot(vecs->mpvec))
 		goto out;
@@ -1469,8 +1467,7 @@ map_discovery (struct vectors * vecs)
 		return 1;
 
 	vector_foreach_slot (vecs->mpvec, mpp, i)
-		if (update_multipath_table(mpp, vecs->pathvec, 0) != DMP_OK ||
-		    update_multipath_status(mpp) != DMP_OK) {
+		if (update_multipath_table(mpp, vecs->pathvec, 0) != DMP_OK) {
 			remove_map(mpp, vecs->pathvec, vecs->mpvec, PURGE_VEC);
 			i--;
 		}
-- 
2.30.1


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

[dm-devel] [PATCH 2/2] 11-dm-mpath.rules: run "multipath -U" with -v1

[mwilck]

From: Martin Wilck <mwilck@suse.com>

In cases where some path devices are temporarily unavailable (e.g. failover),
high amounts of error messages such as these are seen:

Feb 27 08:02:03 ictm1608s02h1 multipath[1420]: get_udev_device: failed to look up 65:224 with type 1
Feb 27 08:02:03 ictm1608s02h1 multipath[1420]: 3600a098000aada210000f1625de51ed9: discarding non-existing path 65:224

This is because every invocation of "multipath -U" prints these messages
at the default log level (-v2). In the case of "multipath -U", these
messages aren't important, and in failover situations, "multipath -U" is
run pretty often, spamming the log with many similar messages.

Generally reducing the log level of these messages would be wrong,
because they are important for multipathd's operation, to verify that
multipathd does the right thing when discovering a discrepancy between the dm
state and the devices present in the system. Therefore, just decrease the
verbosity with which we invoke "multipath -U" in the udev rules.

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

diff --git a/multipath/11-dm-mpath.rules b/multipath/11-dm-mpath.rules
index cd522e8..d191ae8 100644
--- a/multipath/11-dm-mpath.rules
+++ b/multipath/11-dm-mpath.rules
@@ -32,7 +32,7 @@ ACTION=="add", ENV{.MPATH_DEVICE_READY_OLD}=="1", GOTO="paths_ok"
 
 # 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"
+PROGRAM=="$env{MPATH_SBIN_PATH}/multipath -U -v1 %k", GOTO="paths_ok"
 ENV{MPATH_DEVICE_READY}="0", GOTO="mpath_action"
 LABEL="paths_ok"
 
-- 
2.30.1


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

Re: [dm-devel] [PATCH 1/2] libmultipath: merge update_multipath_table() and update_multipath_status()

[Benjamin Marzinski]

On Wed, Mar 17, 2021 at 06:27:26PM +0100, mwilck@suse.com wrote:
> From: Martin Wilck <mwilck@suse.com>
> 
> Since 378cb66 ("multipath: use update_pathvec_from_dm()"),
> we remove paths and even pathgroups from multipathd's data structures
> in update_multipath_table() if these paths are found to be non-existent.
> But update_multipath_status() is called afterwards, and it
> uses the kernel's mapping of pathgroups and paths, which won't match
> any more if any members had been removed. disassemble_status() returns
> an error if the number of path groups doesn't match, causing the
> entire structure setup to fail. And because disassemble_status()
> doesn't check the dev_t against the corresponding values in multipathd's
> data structures, it may assign wrong DM state to paths.
> 
> Fix this by calling disassemble_status() before making any changes to
> the data structure in update_pathvec_from_dm(). This can be easily
> done, because every call to update_multipath_status() is preceded
> by a call to update_multipath_table() anyway, and vice versa. So
> we simply merge the two functions into one. This actually simplifies
> the code for all callers.
> 
> As we remove a symbol, the major library version must be bumped.

A few small questions and comments below

> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  libmpathpersist/mpath_persist.c   |  1 -
>  libmultipath/libmultipath.version | 30 ++++++++-----------------
>  libmultipath/structs_vec.c        | 37 ++++++-------------------------
>  multipath/main.c                  |  6 ++---
>  multipathd/main.c                 |  5 +----
>  5 files changed, 19 insertions(+), 60 deletions(-)
> 
> diff --git a/libmpathpersist/mpath_persist.c b/libmpathpersist/mpath_persist.c
> index 5c95af2..190e970 100644
> --- a/libmpathpersist/mpath_persist.c
> +++ b/libmpathpersist/mpath_persist.c
> @@ -408,7 +408,6 @@ get_mpvec (vector curmp, vector pathvec, char * refwwid)
>  			continue;
>  
>  		if (update_multipath_table(mpp, pathvec, DI_CHECKER) != DMP_OK ||
> -		    update_multipath_status(mpp) != DMP_OK ||
>  		    update_mpp_paths(mpp, pathvec)) {
>  			condlog(1, "error parsing map %s", mpp->wwid);
>  			remove_map(mpp, pathvec, curmp, PURGE_VEC);
> diff --git a/libmultipath/libmultipath.version b/libmultipath/libmultipath.version
> index e9b4608..0cff311 100644
> --- a/libmultipath/libmultipath.version
> +++ b/libmultipath/libmultipath.version
> @@ -31,7 +31,7 @@
>   *   The new version inherits the previous ones.
>   */
>  
> -LIBMULTIPATH_4.0.0 {
> +LIBMULTIPATH_5.0.0 {
>  global:
>  	/* symbols referenced by multipath and multipathd */
>  	add_foreign;
> @@ -198,7 +198,6 @@ global:
>  	uevent_is_mpath;
>  	uevent_listen;
>  	update_mpp_paths;
> -	update_multipath_status;
>  	update_multipath_strings;
>  	update_multipath_table;
>  	update_pathvec_from_dm;
> @@ -256,33 +255,22 @@ global:
>  	libmultipath_init;
>  	libmultipath_exit;
>  
> -local:
> -	*;
> -};
> -
> -LIBMULTIPATH_4.1.0 {
> -global:
> +	/* added in 4.1.0 */
>  	libmp_verbosity;
> -} LIBMULTIPATH_4.0.0;
>  
> -LIBMULTIPATH_4.2.0 {
> -global:
> +	/* added in 4.2.0 */
>  	dm_prereq;
>  	skip_libmp_dm_init;
> -} LIBMULTIPATH_4.1.0;
>  
> -LIBMULTIPATH_4.3.0 {
> -global:
> +	/* added in 4.3.0 */
>  	start_checker_thread;
> -} LIBMULTIPATH_4.2.0;
>  
> -LIBMULTIPATH_4.4.0 {
> -global:
> +	/* added in 4.4.0 */
>  	get_next_string;
> -} LIBMULTIPATH_4.3.0;
>  
> -LIBMULITIPATH_4.5.0 {
> -global:
> +	/* added in 4.5.0 */
>  	get_vpd_sgio;
>  	trigger_partitions_udev_change;
> -} LIBMULTIPATH_4.4.0;
> +local:
> +	*;
> +};
> diff --git a/libmultipath/structs_vec.c b/libmultipath/structs_vec.c
> index 57cd88a..33c522f 100644
> --- a/libmultipath/structs_vec.c
> +++ b/libmultipath/structs_vec.c
> @@ -432,31 +432,14 @@ update_multipath_table (struct multipath *mpp, vector pathvec, int flags)
>  		return DMP_ERR;
>  	}
>  
> -	/* FIXME: we should deal with the return value here */
> -	update_pathvec_from_dm(pathvec, mpp, flags);
> -
> -	return DMP_OK;
> -}
> -
> -int
> -update_multipath_status (struct multipath *mpp)
> -{
> -	int r = DMP_ERR;
> -	char status[PARAMS_SIZE] = {0};
> -
> -	if (!mpp)
> -		return r;
> -
> -	r = dm_get_status(mpp->alias, status);
> -	if (r != DMP_OK) {
> +	*params = '\0';
> +	if (dm_get_status(mpp->alias, params) != DMP_OK)
>  		condlog(3, "%s: %s", mpp->alias, (r == DMP_ERR)? "error getting status" : "map not present");
> -		return r;
> -	}
> -
> -	if (disassemble_status(status, mpp)) {
> +	else if (disassemble_status(params, mpp))
>  		condlog(3, "%s: cannot disassemble status", mpp->alias);
> -		return DMP_ERR;
> -	}

Is the idea that updating the path/pathgroup status is not critical, so
even if it fails, update_multipath_table() can still succeed? That makes
some sense, but it seems like failing to update the status is at least
not expected, so we should increase the log level.

> +
> +	/* FIXME: we should deal with the return value here */
> +	update_pathvec_from_dm(pathvec, mpp, flags);
>  
>  	return DMP_OK;
>  }
> @@ -536,19 +519,13 @@ update_multipath_strings(struct multipath *mpp, vector pathvec)
>  	mpp->pg = NULL;
>  
>  	r = update_multipath_table(mpp, pathvec, 0);
> -	if (r != DMP_OK)
> -		return r;

This one makes less sense to me.  If we failed getting the table, why do
we still sync the paths? Admittedly, it's hard to know what the right
thing to do is, when we fail to get the table.

-Ben

>  	sync_paths(mpp, pathvec);
>  
> -	r = update_multipath_status(mpp);
> -	if (r != DMP_OK)
> -		return r;
> -
>  	vector_foreach_slot(mpp->pg, pgp, i)
>  		if (pgp->paths)
>  			path_group_prio_update(pgp);
>  
> -	return DMP_OK;
> +	return r;
>  }
>  
>  static void enter_recovery_mode(struct multipath *mpp)
> diff --git a/multipath/main.c b/multipath/main.c
> index 3f97582..ef89c7c 100644
> --- a/multipath/main.c
> +++ b/multipath/main.c
> @@ -196,8 +196,7 @@ get_dm_mpvec (enum mpath_cmds cmd, vector curmp, vector pathvec, char * refwwid)
>  			continue;
>  		}
>  
> -		if (update_multipath_table(mpp, pathvec, flags) != DMP_OK ||
> -		    update_multipath_status(mpp) != DMP_OK) {
> +		if (update_multipath_table(mpp, pathvec, flags) != DMP_OK) {
>  			condlog(1, "error parsing map %s", mpp->wwid);
>  			remove_map(mpp, pathvec, curmp, PURGE_VEC);
>  			i--;
> @@ -263,8 +262,7 @@ static int check_usable_paths(struct config *conf,
>  	if (mpp == NULL)
>  		goto free;
>  
> -	if (update_multipath_table(mpp, pathvec, 0) != DMP_OK ||
> -		    update_multipath_status(mpp) != DMP_OK)
> +	if (update_multipath_table(mpp, pathvec, 0) != DMP_OK)
>  		    goto free;
>  
>  	vector_foreach_slot (mpp->pg, pg, i) {
> diff --git a/multipathd/main.c b/multipathd/main.c
> index e0797cc..154a4ee 100644
> --- a/multipathd/main.c
> +++ b/multipathd/main.c
> @@ -559,8 +559,6 @@ add_map_without_path (struct vectors *vecs, const char *alias)
>  
>  	if (update_multipath_table(mpp, vecs->pathvec, 0) != DMP_OK)
>  		goto out;
> -	if (update_multipath_status(mpp) != DMP_OK)
> -		goto out;
>  
>  	if (!vector_alloc_slot(vecs->mpvec))
>  		goto out;
> @@ -1469,8 +1467,7 @@ map_discovery (struct vectors * vecs)
>  		return 1;
>  
>  	vector_foreach_slot (vecs->mpvec, mpp, i)
> -		if (update_multipath_table(mpp, vecs->pathvec, 0) != DMP_OK ||
> -		    update_multipath_status(mpp) != DMP_OK) {
> +		if (update_multipath_table(mpp, vecs->pathvec, 0) != DMP_OK) {
>  			remove_map(mpp, vecs->pathvec, vecs->mpvec, PURGE_VEC);
>  			i--;
>  		}
> -- 
> 2.30.1

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

Re: [dm-devel] [PATCH 2/2] 11-dm-mpath.rules: run "multipath -U" with -v1

[Benjamin Marzinski]

On Wed, Mar 17, 2021 at 06:27:27PM +0100, mwilck@suse.com wrote:
> From: Martin Wilck <mwilck@suse.com>
> 
> In cases where some path devices are temporarily unavailable (e.g. failover),
> high amounts of error messages such as these are seen:
> 
> Feb 27 08:02:03 ictm1608s02h1 multipath[1420]: get_udev_device: failed to look up 65:224 with type 1
> Feb 27 08:02:03 ictm1608s02h1 multipath[1420]: 3600a098000aada210000f1625de51ed9: discarding non-existing path 65:224
> 
> This is because every invocation of "multipath -U" prints these messages
> at the default log level (-v2). In the case of "multipath -U", these
> messages aren't important, and in failover situations, "multipath -U" is
> run pretty often, spamming the log with many similar messages.
> 
> Generally reducing the log level of these messages would be wrong,
> because they are important for multipathd's operation, to verify that
> multipathd does the right thing when discovering a discrepancy between the dm
> state and the devices present in the system. Therefore, just decrease the
> verbosity with which we invoke "multipath -U" in the udev rules.
> 
Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  multipath/11-dm-mpath.rules | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/multipath/11-dm-mpath.rules b/multipath/11-dm-mpath.rules
> index cd522e8..d191ae8 100644
> --- a/multipath/11-dm-mpath.rules
> +++ b/multipath/11-dm-mpath.rules
> @@ -32,7 +32,7 @@ ACTION=="add", ENV{.MPATH_DEVICE_READY_OLD}=="1", GOTO="paths_ok"
>  
>  # 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"
> +PROGRAM=="$env{MPATH_SBIN_PATH}/multipath -U -v1 %k", GOTO="paths_ok"
>  ENV{MPATH_DEVICE_READY}="0", GOTO="mpath_action"
>  LABEL="paths_ok"
>  
> -- 
> 2.30.1

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