DM-Devel Archive on lore.kernel.org
 help / color / Atom feed
From: Benjamin Marzinski <bmarzins@redhat.com>
To: mwilck@suse.com
Cc: dm-devel@redhat.com
Subject: Re: [PATCH 02/11] libmultipath: copy mpp->hwe from pp->hwe
Date: Thu, 24 Sep 2020 15:12:45 -0500
Message-ID: <20200924201245.GH11108@octiron.msp.redhat.com> (raw)
In-Reply-To: <20200924133644.14034-3-mwilck@suse.com>

On Thu, Sep 24, 2020 at 03:36:35PM +0200, mwilck@suse.com wrote:
> From: Martin Wilck <mwilck@suse.com>
> 
> Since f0462f0 ("libmultipath: use vector for for pp->hwe and mp->hwe"),
> we've been trying to fix issues caused by paths getting freed and mpp->hwe
> dangling. This approach couldn't work because we need mpp->hwe to persist,
> even if all paths are removed from the map. Before f0462f0, a simple
> assignment worked, because the lifetime of the hwe wasn't bound to the
> path. But now, we need to copy the vector. It turns out that we need to set
> mpp->hwe only in two places, add_map_with_path() and setup_map(), and
> that the code is simplified overall.

Unless I'm missing someting, it looks like
__mpath_persistent_reserve_out() will call select_all_tg_pt(), which
uses mpp->hwe, without ever setting it.  Granted, I don't see how this
was supposed to work before your patch either.

-Ben

> 
> Even now, it can happen that a map is added with add_map_without_paths(),
> and has no paths. In that case, calling do_set_from_hwe() with a NULL
> pointer is not a bug, so remove the message.
> 
> Fixes: f0462f0 ("libmultipath: use vector for for pp->hwe and mp->hwe")
> ---
>  libmultipath/configure.c   |  7 +++++
>  libmultipath/propsel.c     |  4 +--
>  libmultipath/structs.c     | 15 +++++++++++
>  libmultipath/structs.h     |  1 +
>  libmultipath/structs_vec.c | 54 ++++++++++++++++----------------------
>  multipathd/main.c          | 10 -------
>  6 files changed, 46 insertions(+), 45 deletions(-)
> 
> diff --git a/libmultipath/configure.c b/libmultipath/configure.c
> index 6fb477f..d7afc91 100644
> --- a/libmultipath/configure.c
> +++ b/libmultipath/configure.c
> @@ -311,6 +311,13 @@ int setup_map(struct multipath *mpp, char *params, int params_size,
>  	if (mpp->disable_queueing && VECTOR_SIZE(mpp->paths) != 0)
>  		mpp->disable_queueing = 0;
>  
> +	/*
> +	 * If this map was created with add_map_without_path(),
> +	 * mpp->hwe might not be set yet.
> +	 */
> +	if (!mpp->hwe)
> +		extract_hwe_from_path(mpp);
> +
>  	/*
>  	 * properties selectors
>  	 *
> diff --git a/libmultipath/propsel.c b/libmultipath/propsel.c
> index 7e6e0d6..4020134 100644
> --- a/libmultipath/propsel.c
> +++ b/libmultipath/propsel.c
> @@ -65,9 +65,7 @@ do {									\
>  	__do_set_from_vec(struct hwentry, var, (src)->hwe, dest)
>  
>  #define do_set_from_hwe(var, src, dest, msg)				\
> -	if (!src->hwe) {						\
> -		condlog(0, "BUG: do_set_from_hwe called with hwe == NULL"); \
> -	} else if (__do_set_from_hwe(var, src, dest)) {			\
> +	if (src->hwe && __do_set_from_hwe(var, src, dest)) {		\
>  		origin = msg;						\
>  		goto out;						\
>  	}
> diff --git a/libmultipath/structs.c b/libmultipath/structs.c
> index 464596f..2efad6f 100644
> --- a/libmultipath/structs.c
> +++ b/libmultipath/structs.c
> @@ -234,6 +234,17 @@ alloc_multipath (void)
>  	return mpp;
>  }
>  
> +void *set_mpp_hwe(struct multipath *mpp, const struct path *pp)
> +{
> +	if (!mpp || !pp || !pp->hwe)
> +		return NULL;
> +	if (mpp->hwe)
> +		return mpp->hwe;
> +	mpp->hwe = vector_convert(NULL, pp->hwe,
> +				  struct hwentry, identity);
> +	return mpp->hwe;
> +}
> +
>  void free_multipath_attributes(struct multipath *mpp)
>  {
>  	if (!mpp)
> @@ -290,6 +301,10 @@ free_multipath (struct multipath * mpp, enum free_path_mode free_paths)
>  
>  	free_pathvec(mpp->paths, free_paths);
>  	free_pgvec(mpp->pg, free_paths);
> +	if (mpp->hwe) {
> +		vector_free(mpp->hwe);
> +		mpp->hwe = NULL;
> +	}
>  	FREE_PTR(mpp->mpcontext);
>  	FREE(mpp);
>  }
> diff --git a/libmultipath/structs.h b/libmultipath/structs.h
> index 4afd3e8..3bd2bbd 100644
> --- a/libmultipath/structs.h
> +++ b/libmultipath/structs.h
> @@ -421,6 +421,7 @@ struct host_group {
>  struct path * alloc_path (void);
>  struct pathgroup * alloc_pathgroup (void);
>  struct multipath * alloc_multipath (void);
> +void *set_mpp_hwe(struct multipath *mpp, const struct path *pp);
>  void uninitialize_path(struct path *pp);
>  void free_path (struct path *);
>  void free_pathvec (vector vec, enum free_path_mode free_paths);
> diff --git a/libmultipath/structs_vec.c b/libmultipath/structs_vec.c
> index 7fd860e..b10d347 100644
> --- a/libmultipath/structs_vec.c
> +++ b/libmultipath/structs_vec.c
> @@ -282,11 +282,6 @@ err:
>  void orphan_path(struct path *pp, const char *reason)
>  {
>  	condlog(3, "%s: orphan path, %s", pp->dev, reason);
> -	if (pp->mpp && pp->hwe && pp->mpp->hwe == pp->hwe) {
> -		condlog(0, "BUG: orphaning path %s that holds hwe of %s",
> -			pp->dev, pp->mpp->alias);
> -		pp->mpp->hwe = NULL;
> -	}
>  	pp->mpp = NULL;
>  	uninitialize_path(pp);
>  }
> @@ -296,8 +291,6 @@ void orphan_paths(vector pathvec, struct multipath *mpp, const char *reason)
>  	int i;
>  	struct path * pp;
>  
> -	/* Avoid BUG message from orphan_path() */
> -	mpp->hwe = NULL;
>  	vector_foreach_slot (pathvec, pp, i) {
>  		if (pp->mpp == mpp) {
>  			if (pp->initialized == INIT_REMOVED) {
> @@ -385,24 +378,26 @@ extract_hwe_from_path(struct multipath * mpp)
>  	if (mpp->hwe || !mpp->paths)
>  		return;
>  
> -	condlog(3, "%s: searching paths for valid hwe", mpp->alias);
> +	condlog(4, "%s: searching paths for valid hwe", mpp->alias);
>  	/* doing this in two passes seems like paranoia to me */
>  	vector_foreach_slot(mpp->paths, pp, i) {
> -		if (pp->state != PATH_UP)
> -			continue;
> -		if (pp->hwe) {
> -			mpp->hwe = pp->hwe;
> -			return;
> -		}
> +		if (pp->state == PATH_UP &&
> +		    pp->initialized != INIT_REMOVED && pp->hwe)
> +			goto done;
>  	}
>  	vector_foreach_slot(mpp->paths, pp, i) {
> -		if (pp->state == PATH_UP)
> -			continue;
> -		if (pp->hwe) {
> -			mpp->hwe = pp->hwe;
> -			return;
> -		}
> +		if (pp->state != PATH_UP &&
> +		    pp->initialized != INIT_REMOVED && pp->hwe)
> +			goto done;
>  	}
> +done:
> +	if (i < VECTOR_SIZE(mpp->paths))
> +		(void)set_mpp_hwe(mpp, pp);
> +
> +	if (mpp->hwe)
> +		condlog(3, "%s: got hwe from path %s", mpp->alias, pp->dev);
> +	else
> +		condlog(2, "%s: no hwe found", mpp->alias);
>  }
>  
>  int
> @@ -502,8 +497,6 @@ void sync_paths(struct multipath *mpp, vector pathvec)
>  		}
>  		if (!found) {
>  			condlog(3, "%s dropped path %s", mpp->alias, pp->dev);
> -			if (mpp->hwe == pp->hwe)
> -				mpp->hwe = NULL;
>  			vector_del_slot(mpp->paths, i--);
>  			orphan_path(pp, "path removed externally");
>  		}
> @@ -512,8 +505,6 @@ void sync_paths(struct multipath *mpp, vector pathvec)
>  	update_mpp_paths(mpp, pathvec);
>  	vector_foreach_slot (mpp->paths, pp, i)
>  		pp->mpp = mpp;
> -	if (mpp->hwe == NULL)
> -		extract_hwe_from_path(mpp);
>  }
>  
>  int
> @@ -689,9 +680,15 @@ struct multipath *add_map_with_path(struct vectors *vecs, struct path *pp,
>  
>  	conf = get_multipath_config();
>  	mpp->mpe = find_mpe(conf->mptable, pp->wwid);
> -	mpp->hwe = pp->hwe;
>  	put_multipath_config(conf);
>  
> +	/*
> +	 * We need to call this before select_alias(),
> +	 * because that accesses hwe properties.
> +	 */
> +	if (pp->hwe && !set_mpp_hwe(mpp, pp))
> +		goto out;
> +
>  	strcpy(mpp->wwid, pp->wwid);
>  	find_existing_alias(mpp, vecs);
>  	if (select_alias(conf, mpp))
> @@ -742,12 +739,6 @@ int verify_paths(struct multipath *mpp)
>  			vector_del_slot(mpp->paths, i);
>  			i--;
>  
> -			/* Make sure mpp->hwe doesn't point to freed memory.
> -			 * We call extract_hwe_from_path() below to restore
> -			 * mpp->hwe
> -			 */
> -			if (mpp->hwe == pp->hwe)
> -				mpp->hwe = NULL;
>  			/*
>  			 * Don't delete path from pathvec yet. We'll do this
>  			 * after the path has been removed from the map, in
> @@ -759,7 +750,6 @@ int verify_paths(struct multipath *mpp)
>  				mpp->alias, pp->dev, pp->dev_t);
>  		}
>  	}
> -	extract_hwe_from_path(mpp);
>  	return count;
>  }
>  
> diff --git a/multipathd/main.c b/multipathd/main.c
> index a4abbb2..eedc6c1 100644
> --- a/multipathd/main.c
> +++ b/multipathd/main.c
> @@ -1153,13 +1153,6 @@ ev_remove_path (struct path *pp, struct vectors * vecs, int need_do_map)
>  		if (i != -1)
>  			vector_del_slot(mpp->paths, i);
>  
> -		/*
> -		 * Make sure mpp->hwe doesn't point to freed memory
> -		 * We call extract_hwe_from_path() below to restore mpp->hwe
> -		 */
> -		if (mpp->hwe == pp->hwe)
> -			mpp->hwe = NULL;
> -
>  		/*
>  		 * remove the map IF removing the last path
>  		 */
> @@ -1191,9 +1184,6 @@ ev_remove_path (struct path *pp, struct vectors * vecs, int need_do_map)
>  			 */
>  		}
>  
> -		if (mpp->hwe == NULL)
> -			extract_hwe_from_path(mpp);
> -
>  		if (setup_map(mpp, params, PARAMS_SIZE, vecs)) {
>  			condlog(0, "%s: failed to setup map for"
>  				" removal of path %s", mpp->alias, pp->dev);
> -- 
> 2.28.0

  reply index

Thread overview: 23+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-09-24 13:36 [PATCH 00/11] multipath-tools: add linker version scripts mwilck
2020-09-24 13:36 ` [PATCH 01/11] libmultipath: find_mpe(): don't match with empty WWID mwilck
2020-09-24 13:36 ` [PATCH 02/11] libmultipath: copy mpp->hwe from pp->hwe mwilck
2020-09-24 20:12   ` Benjamin Marzinski [this message]
2020-09-25 16:01     ` Martin Wilck
2020-09-25 16:06       ` Benjamin Marzinski
2020-09-24 13:36 ` [PATCH 03/11] libmultipath: dm_map_present_by_uuid(): fix dm_task_create() call mwilck
2020-09-24 13:36 ` [PATCH 04/11] libdmmp tests: fix compilation mwilck
2020-09-24 13:36 ` [PATCH 05/11] libmultipath: prio: constify some function parameters mwilck
2020-09-24 13:36 ` [PATCH 06/11] libmultipath: checkers/prio: allow non-lazy .so loading mwilck
2020-09-24 20:27   ` Benjamin Marzinski
2020-09-24 13:36 ` [PATCH 07/11] multipath-tools Makefiles: separate rules for .so and man pages mwilck
2020-09-24 13:36 ` [PATCH 08/11] libmultipath: create separate .so for unit tests mwilck
2020-09-24 23:35   ` Benjamin Marzinski
2020-09-24 13:36 ` [PATCH 09/11] libmultipath: add linker version script mwilck
2020-09-24 13:36 ` [PATCH 10/11] libmpathpersist: " mwilck
2020-09-25  4:00   ` Benjamin Marzinski
2020-09-25 19:52     ` Martin Wilck
2020-09-25 22:10       ` Benjamin Marzinski
     [not found]         ` <20200925223207.GC3384@octiron.msp.redhat.com>
     [not found]           ` <c0a1e44fcc819583d972884aa126c486fc784fa9.camel@suse.com>
2020-09-25 23:22             ` Benjamin Marzinski
2020-09-24 13:36 ` [PATCH 11/11] libmpathcmd: " mwilck
2020-09-25  2:09 ` [PATCH 00/11] multipath-tools: add linker version scripts Benjamin Marzinski
2020-09-25 19:53   ` Martin Wilck

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200924201245.GH11108@octiron.msp.redhat.com \
    --to=bmarzins@redhat.com \
    --cc=dm-devel@redhat.com \
    --cc=mwilck@suse.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

DM-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/dm-devel/0 dm-devel/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dm-devel dm-devel/ https://lore.kernel.org/dm-devel \
		dm-devel@redhat.com
	public-inbox-index dm-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/com.redhat.dm-devel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git