On Thu, Mar 18, 2021 at 10:14:11AM +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.
>
Reviewed-by: Benjamin Marzinski <bmarzins@xxxxxxxxxx>
> Signed-off-by: Martin Wilck <mwilck@xxxxxxxx>
> ---
> Changes v1 -> v2:
> - log errors at -v2 in update_multipath_table() (Ben)
> - return error in update_multipath_strings() if
> update_multipath_table() fails (Ben)
>
> ---
> libmpathpersist/mpath_persist.c | 1 -
> libmultipath/libmultipath.version | 30 ++++++++----------------
> libmultipath/structs_vec.c | 38 ++++++++-----------------------
> multipath/main.c | 6 ++---
> multipathd/main.c | 5 +---
> 5 files changed, 21 insertions(+), 59 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..0b069f4 100644
> --- a/libmultipath/structs_vec.c
> +++ b/libmultipath/structs_vec.c
> @@ -423,44 +423,27 @@ update_multipath_table (struct multipath *mpp, vector pathvec, int flags)
>
> r = dm_get_map(mpp->alias, &mpp->size, params);
> if (r != DMP_OK) {
> - condlog(3, "%s: %s", mpp->alias, (r == DMP_ERR)? "error getting table" : "map not present");
> + condlog(2, "%s: %s", mpp->alias, (r == DMP_ERR)? "error getting table" : "map not present");
> return r;
> }
>
> if (disassemble_map(pathvec, params, mpp)) {
> - condlog(3, "%s: cannot disassemble map", mpp->alias);
> + condlog(2, "%s: cannot disassemble map", mpp->alias);
> return DMP_ERR;
> }
>
> + *params = '\0';
> + if (dm_get_status(mpp->alias, params) != DMP_OK)
> + condlog(2, "%s: %s", mpp->alias, (r == DMP_ERR)? "error getting status" : "map not present");
> + else if (disassemble_status(params, mpp))
> + condlog(2, "%s: cannot disassemble status", mpp->alias);
> +
> /* 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) {
> - condlog(3, "%s: %s", mpp->alias, (r == DMP_ERR)? "error getting status" : "map not present");
> - return r;
> - }
> -
> - if (disassemble_status(status, mpp)) {
> - condlog(3, "%s: cannot disassemble status", mpp->alias);
> - return DMP_ERR;
> - }
> -
> - return DMP_OK;
> -}
> -
> static struct path *find_devt_in_pathgroups(const struct multipath *mpp,
> const char *dev_t)
> {
> @@ -538,11 +521,8 @@ update_multipath_strings(struct multipath *mpp, vector pathvec)
> 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;
> + sync_paths(mpp, pathvec);
>
> vector_foreach_slot(mpp->pg, pgp, i)
> if (pgp->paths)
> 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
