[PATCH v2 00/54] multipath-tools series part IV: identify paths by dev_t

[PATCH v2 00/54] multipath-tools series part IV: identify paths by dev_t

[mwilck]

From: Martin Wilck <mwilck@suse.com>

Hi Christophe, hi Ben,

This is part IV of a larger patch series for multipath-tools I've been preparing.
It's based on the previously submitted part III.

The full series will also be available here:
https://github.com/mwilck/multipath-tools/tree/ups/submit-2008
This part is tagged "submit-200812-4".

The dominant idea of the patches in this subseries is to move away from
the device node name as the main path identifier towards the dev_t aka
major/minor number. This is motivated by the fact that dev_t is used by the
kernel device mapper for referring to devices, both maps and paths.
If searching a device by dev_t fails, it's basically certain not to exist.
DM calls with major/minor number may even succeed if the respective devnode
has not been created yet. dev_t also works better for devices that aren't
fully initialized yet. Of course we still track the device name, but mostly
for log messages and user-directed output.

In short, using dev_t as primary identifier makes our code more robust.

The subseries also contains some unrelated fixes for functions I worked on
while making these changes.

Changes v1 -> v2, as suggested by Ben Marzinski:

 * 44/54: "libmultipath: adopt_paths(): don't bail out on single path failure"
    - fail if adding a specific path was requested and failed. 
 * 46/54: "libmultipath: path_discover(): always set DI_BLACKLIST"
    - dropped, replaced by "libmultipath: path_discover(): explain pathinfo flags",
      which contains only comments

NOTE: In my v1 submission, I made a mistake when sending part V, so that
*patch number 54 is present twice* in the full series.
I deliberately didn't correct that this time, to preserve numbering.

Martin Wilck (12):
  libmultipath: adopt_paths(): use find_path_by_devt()
  libmultipath: adopt_paths(): don't bail out on single path failure
  libmultipath: path_discover(): use find_path_by_devt()
  libmultipath: path_discover(): explain pathinfo flags
  libmultipath: get_refwwid(): use find_path_by_devt()
  libmultipath: get_refwwid(): call get_multipath_config() only once
  libmultipath: get_refwwid(): get rid of "check" label
  libmultipath: get_refwwid(): use symbolic return values
  libmultipath: get_refwwid(): use switch statement
  libmultipath: constify get_mpe_wwid()
  multipath: call strchop() on command line argument
  libmultipath: get_refwwid(): skip strchop(), and constify dev
    parameter

 libmultipath/config.c      |   2 +-
 libmultipath/config.h      |   2 +-
 libmultipath/configure.c   | 200 +++++++++++++------------------------
 libmultipath/configure.h   |   2 +-
 libmultipath/discovery.c   |  37 ++++---
 libmultipath/structs_vec.c |  17 ++--
 multipath/main.c           |   2 +
 multipathd/main.c          |   3 +-
 8 files changed, 103 insertions(+), 162 deletions(-)

[PATCH v2 44/54] libmultipath: adopt_paths(): don't bail out on single path failure

[mwilck]

From: Martin Wilck <mwilck@suse.com>

If pathinfo fails for one path to be adopted, we currently
fail the entire function. This may cause ev_add_path() for a valid
path to fail because some other path is broken. Fix it by just
skipping paths that don't look healthy.

adopt_paths() may now succeed even if some paths couldn't be
added (e.g. because of pathinfo() failure). If this happens while we are
trying to add a specific path, we need to check after successful call to
adopt_paths() if that specific path had been actually added, and fail in the
caller otherwise.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/structs_vec.c | 17 +++++++++++------
 multipathd/main.c          |  3 ++-
 2 files changed, 13 insertions(+), 7 deletions(-)

diff --git a/libmultipath/structs_vec.c b/libmultipath/structs_vec.c
index b1f83c6..fb225f1 100644
--- a/libmultipath/structs_vec.c
+++ b/libmultipath/structs_vec.c
@@ -75,16 +75,20 @@ int adopt_paths(vector pathvec, struct multipath *mpp)
 			if (!mpp->paths && !(mpp->paths = vector_alloc()))
 				return 1;
 
-			if (!find_path_by_devt(mpp->paths, pp->dev_t) &&
-			    store_path(mpp->paths, pp))
-					return 1;
 			conf = get_multipath_config();
 			pthread_cleanup_push(put_multipath_config, conf);
 			ret = pathinfo(pp, conf,
 				       DI_PRIO | DI_CHECKER);
 			pthread_cleanup_pop(1);
-			if (ret)
-				return 1;
+			if (ret) {
+				condlog(3, "%s: pathinfo failed for %s",
+					__func__, pp->dev);
+				continue;
+			}
+
+			if (!find_path_by_devt(mpp->paths, pp->dev_t) &&
+			    store_path(mpp->paths, pp))
+					return 1;
 		}
 	}
 	return 0;
@@ -456,7 +460,8 @@ struct multipath *add_map_with_path(struct vectors *vecs, struct path *pp,
 		goto out;
 	mpp->size = pp->size;
 
-	if (adopt_paths(vecs->pathvec, mpp))
+	if (adopt_paths(vecs->pathvec, mpp) ||
+	    find_slot(vecs->pathvec, pp) == -1)
 		goto out;
 
 	if (add_vec) {
diff --git a/multipathd/main.c b/multipathd/main.c
index 61929a7..5902e7c 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -956,7 +956,8 @@ rescan:
 	if (mpp) {
 		condlog(4,"%s: adopting all paths for path %s",
 			mpp->alias, pp->dev);
-		if (adopt_paths(vecs->pathvec, mpp))
+		if (adopt_paths(vecs->pathvec, mpp) ||
+		    find_slot(vecs->pathvec, pp) == -1)
 			goto fail; /* leave path added to pathvec */
 
 		verify_paths(mpp, vecs);
-- 
2.28.0

[PATCH v2 46/54] libmultipath: path_discover(): explain pathinfo flags

[mwilck]

From: Martin Wilck <mwilck@suse.com>

Add a comment explaining why we use different flags for "new" and
existing paths.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/discovery.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
index 5f4ebf0..64d3473 100644
--- a/libmultipath/discovery.c
+++ b/libmultipath/discovery.c
@@ -137,6 +137,11 @@ path_discover (vector pathvec, struct config * conf,
 				      udevice, flag | DI_BLACKLIST,
 				      NULL);
 	else
+		/*
+		 * Don't use DI_BLACKLIST on paths already in pathvec. We rely
+		 * on the caller to pre-populate the pathvec with valid paths
+		 * only.
+		 */
 		return pathinfo(pp, conf, flag);
 }
 
-- 
2.28.0

Re: [PATCH v2 44/54] libmultipath: adopt_paths(): don't bail out on single path failure

[Benjamin Marzinski]

On Wed, Aug 12, 2020 at 01:34:28PM +0200, mwilck@suse.com wrote:
> From: Martin Wilck <mwilck@suse.com>
> 
> If pathinfo fails for one path to be adopted, we currently
> fail the entire function. This may cause ev_add_path() for a valid
> path to fail because some other path is broken. Fix it by just
> skipping paths that don't look healthy.
> 
> adopt_paths() may now succeed even if some paths couldn't be
> added (e.g. because of pathinfo() failure). If this happens while we are
> trying to add a specific path, we need to check after successful call to
> adopt_paths() if that specific path had been actually added, and fail in the
> caller otherwise.
> 
The changes the patch makes look fine, but right before the first line of the
patch, isn't there a line with

pp->mpp = mpp;

It seems like we should reset this if we don't actually add the path the
the multipath device. But since that bug was already there we can fix it
in a seperate patch, so

Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  libmultipath/structs_vec.c | 17 +++++++++++------
>  multipathd/main.c          |  3 ++-
>  2 files changed, 13 insertions(+), 7 deletions(-)
> 
> diff --git a/libmultipath/structs_vec.c b/libmultipath/structs_vec.c
> index b1f83c6..fb225f1 100644
> --- a/libmultipath/structs_vec.c
> +++ b/libmultipath/structs_vec.c
> @@ -75,16 +75,20 @@ int adopt_paths(vector pathvec, struct multipath *mpp)
>  			if (!mpp->paths && !(mpp->paths = vector_alloc()))
>  				return 1;
>  
> -			if (!find_path_by_devt(mpp->paths, pp->dev_t) &&
> -			    store_path(mpp->paths, pp))
> -					return 1;
>  			conf = get_multipath_config();
>  			pthread_cleanup_push(put_multipath_config, conf);
>  			ret = pathinfo(pp, conf,
>  				       DI_PRIO | DI_CHECKER);
>  			pthread_cleanup_pop(1);
> -			if (ret)
> -				return 1;
> +			if (ret) {
> +				condlog(3, "%s: pathinfo failed for %s",
> +					__func__, pp->dev);
> +				continue;
> +			}
> +
> +			if (!find_path_by_devt(mpp->paths, pp->dev_t) &&
> +			    store_path(mpp->paths, pp))
> +					return 1;
>  		}
>  	}
>  	return 0;
> @@ -456,7 +460,8 @@ struct multipath *add_map_with_path(struct vectors *vecs, struct path *pp,
>  		goto out;
>  	mpp->size = pp->size;
>  
> -	if (adopt_paths(vecs->pathvec, mpp))
> +	if (adopt_paths(vecs->pathvec, mpp) ||
> +	    find_slot(vecs->pathvec, pp) == -1)
>  		goto out;
>  
>  	if (add_vec) {
> diff --git a/multipathd/main.c b/multipathd/main.c
> index 61929a7..5902e7c 100644
> --- a/multipathd/main.c
> +++ b/multipathd/main.c
> @@ -956,7 +956,8 @@ rescan:
>  	if (mpp) {
>  		condlog(4,"%s: adopting all paths for path %s",
>  			mpp->alias, pp->dev);
> -		if (adopt_paths(vecs->pathvec, mpp))
> +		if (adopt_paths(vecs->pathvec, mpp) ||
> +		    find_slot(vecs->pathvec, pp) == -1)
>  			goto fail; /* leave path added to pathvec */
>  
>  		verify_paths(mpp, vecs);
> -- 
> 2.28.0

Re: [PATCH v2 46/54] libmultipath: path_discover(): explain pathinfo flags

[Benjamin Marzinski]

On Wed, Aug 12, 2020 at 01:34:29PM +0200, mwilck@suse.com wrote:
> From: Martin Wilck <mwilck@suse.com>
> 
> Add a comment explaining why we use different flags for "new" and
> existing paths.
> 
Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  libmultipath/discovery.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
> index 5f4ebf0..64d3473 100644
> --- a/libmultipath/discovery.c
> +++ b/libmultipath/discovery.c
> @@ -137,6 +137,11 @@ path_discover (vector pathvec, struct config * conf,
>  				      udevice, flag | DI_BLACKLIST,
>  				      NULL);
>  	else
> +		/*
> +		 * Don't use DI_BLACKLIST on paths already in pathvec. We rely
> +		 * on the caller to pre-populate the pathvec with valid paths
> +		 * only.
> +		 */
>  		return pathinfo(pp, conf, flag);
>  }
>  
> -- 
> 2.28.0