Re: [PATCH v2 1/3] libmultipath: merge update_multipath_table() and update_multipath_status()

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [DM Crypt]     [Fedora Desktop]     [ATA RAID]     [Fedora Marketing]     [Fedora Packaging]     [Fedora SELinux]     [Yosemite Discussion]     [KDE Users]     [Fedora Docs]

  Powered by Linux