On Wed, Mar 17, 2021 at 06:27:26PM +0100, mwilck@xxxxxxxx wrote:
> From: Martin Wilck <mwilck@xxxxxxxx>
>
> 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@xxxxxxxx>
> ---
> 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@xxxxxxxxxx
https://listman.redhat.com/mailman/listinfo/dm-devel
