Patchwork [04/12] libmultipath: cleanup features handling code

login
register
mail settings
Submitter Benjamin Marzinski
Date Dec. 7, 2017, 6:48 p.m.
Message ID <1512672546-12785-5-git-send-email-bmarzins@redhat.com>
Download mbox | patch
Permalink /patch/10100849/
State Not Applicable, archived
Delegated to: christophe varoqui
Headers show

Comments

Benjamin Marzinski - Dec. 7, 2017, 6:48 p.m.
Right now multipath does some extra work to set the values for
no_path_retry and retain_hwhandler on existing maps it reads in from the
kernel.  This is so that select_action() can use these values to see if
it needs to reload the devices. However, the way it works, the
queue_if_no_path feature isn't always set correctly, and multipath has
to go back afterwards and reset the value anyways.

It's simpler for select_action to just look at the values in the
features line it read in from the kernel and the features line it would
like the new map to have.  By comparing these, it also avoids the
problem where the no_path_retry values match, so it doesn't reload, but
the actual queue_if_no_path feature value is incorrect, so it has to go
back and reset it. It can also skip calling setup_feature() entirely.

To do this, assemble_map() needs to update mp->features. This would
otherwise partially happen when it had to reset the queue_if_no_path
value.  retain_attached_hw_handler was never getting updated before, so
the output when you created a map was incorrect.

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmultipath/configure.c | 21 ++++-----------------
 libmultipath/dmparser.c  | 35 ++++++-----------------------------
 libmultipath/structs.c   | 17 -----------------
 libmultipath/structs.h   |  1 -
 4 files changed, 10 insertions(+), 64 deletions(-)
Martin Wilck - Dec. 7, 2017, 10:10 p.m.
On Thu, 2017-12-07 at 12:48 -0600, Benjamin Marzinski wrote:
> Right now multipath does some extra work to set the values for
> no_path_retry and retain_hwhandler on existing maps it reads in from
> the
> kernel.  This is so that select_action() can use these values to see
> if
> it needs to reload the devices. However, the way it works, the
> queue_if_no_path feature isn't always set correctly, and multipath
> has
> to go back afterwards and reset the value anyways.
> 
> It's simpler for select_action to just look at the values in the
> features line it read in from the kernel and the features line it
> would
> like the new map to have.  By comparing these, it also avoids the
> problem where the no_path_retry values match, so it doesn't reload,
> but
> the actual queue_if_no_path feature value is incorrect, so it has to
> go
> back and reset it. It can also skip calling setup_feature() entirely.
> 
> To do this, assemble_map() needs to update mp->features. This would
> otherwise partially happen when it had to reset the queue_if_no_path
> value.  retain_attached_hw_handler was never getting updated before,
> so
> the output when you created a map was incorrect.
> 
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>

I had something similar in mind, but this is actually cleaner.
Reviewed-by: Martin Wilck <mwilck@suse.com>
Martin Wilck - Dec. 8, 2017, 3:24 p.m.
On Thu, 2017-12-07 at 12:48 -0600, Benjamin Marzinski wrote:

> retain_attached_hw_handler was never getting updated before, so
> the output when you created a map was incorrect.

While I've already ACKed your patch, I don't understand what you mean
here. Before your patch, "retain_attached_hw_handler" was set from
config options and correctly copied to the features string in
assemble_map, no?

> diff --git a/libmultipath/configure.c b/libmultipath/configure.c
> index 0dfa250..7ca84b8 100644
> --- a/libmultipath/configure.c
> +++ b/libmultipath/configure.c
> @@ -1060,21 +1062,6 @@ int coalesce_paths (struct vectors * vecs,
> vector newmp, char * refwwid,
>  				remove_feature(&mpp->features,
>  					       "queue_if_no_path");
>  		}
> -		else if (mpp->no_path_retry != NO_PATH_RETRY_UNDEF)
> {
> -			if (mpp->no_path_retry ==
> NO_PATH_RETRY_FAIL) {
> -				condlog(3, "%s: unset
> queue_if_no_path feature",
> -					mpp->alias);
> -				if (!dm_queue_if_no_path(mpp->alias, 
> 0))
> -					remove_feature(&mpp-
> >features,
> -						       "queue_if_no_
> path");
> -			} else {
> -				condlog(3, "%s: set queue_if_no_path
> feature",
> -					mpp->alias);
> -				if (!dm_queue_if_no_path(mpp->alias, 
> 1))
> -					add_feature(&mpp->features,
> -						    "queue_if_no_pat
> h");
> -			}
> -		}

AFAICS we could also get rid of the calls to dm_queue_if_no_path() in 
reload_map(), right?

Martin
Benjamin Marzinski - Dec. 8, 2017, 9:12 p.m.
On Fri, Dec 08, 2017 at 04:24:52PM +0100, Martin Wilck wrote:
> On Thu, 2017-12-07 at 12:48 -0600, Benjamin Marzinski wrote:
> 
> > retain_attached_hw_handler was never getting updated before, so
> > the output when you created a map was incorrect.
> 
> While I've already ACKed your patch, I don't understand what you mean
> here. Before your patch, "retain_attached_hw_handler" was set from
> config options and correctly copied to the features string in
> assemble_map, no?
> 

It is correctly copied to the features string you send to the kernel. It
was not copied to the multipath object that the multipath program is
dealing with.  So if you run "multipath" and it creates the device, what
is prints out as the device it created will not include
"retain_attached_hw_handler", even if it did send that feature to the
kernel.  If you run "multipath -l" afterwards, you will see that the
device does have "retain_attached_hw_handler" set.  This doesn't happen
with queue_if_no_path because of the add_feature() call in the code
below.  That goes through and adds "queue_if_no_path" back to the
features string of the multipath object that multipath will using to
print its output on creation.  Of course, none of that matters on newer
kernels anyway, because we will remove "retain_attached_hw_handler" from
the features line, since that behaviour is automatic now.

> > diff --git a/libmultipath/configure.c b/libmultipath/configure.c
> > index 0dfa250..7ca84b8 100644
> > --- a/libmultipath/configure.c
> > +++ b/libmultipath/configure.c
> > @@ -1060,21 +1062,6 @@ int coalesce_paths (struct vectors * vecs,
> > vector newmp, char * refwwid,
> >  				remove_feature(&mpp->features,
> >  					       "queue_if_no_path");
> >  		}
> > -		else if (mpp->no_path_retry != NO_PATH_RETRY_UNDEF)
> > {
> > -			if (mpp->no_path_retry ==
> > NO_PATH_RETRY_FAIL) {
> > -				condlog(3, "%s: unset
> > queue_if_no_path feature",
> > -					mpp->alias);
> > -				if (!dm_queue_if_no_path(mpp->alias, 
> > 0))
> > -					remove_feature(&mpp-
> > >features,
> > -						       "queue_if_no_
> > path");
> > -			} else {
> > -				condlog(3, "%s: set queue_if_no_path
> > feature",
> > -					mpp->alias);
> > -				if (!dm_queue_if_no_path(mpp->alias, 
> > 1))
> > -					add_feature(&mpp->features,
> > -						    "queue_if_no_pat
> > h");
> > -			}
> > -		}
> 
> AFAICS we could also get rid of the calls to dm_queue_if_no_path() in 
> reload_map(), right?

Oops. I meant to remove it there as well. Good catch. I'll send a
follow-up patch.

-Ben

> Martin
> 
> -- 
> Dr. Martin Wilck <mwilck@suse.com>, Tel. +49 (0)911 74053 2107
> SUSE Linux GmbH, GF: Felix Imend├Ârffer, Jane Smithard, Graham Norton
> HRB 21284 (AG N├╝rnberg)

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

Patch

diff --git a/libmultipath/configure.c b/libmultipath/configure.c
index 0dfa250..7ca84b8 100644
--- a/libmultipath/configure.c
+++ b/libmultipath/configure.c
@@ -557,7 +557,8 @@  select_action (struct multipath * mpp, vector curmp, int force_reload)
 	}
 
 	if (mpp->no_path_retry != NO_PATH_RETRY_UNDEF &&
-	    mpp->no_path_retry != cmpp->no_path_retry) {
+	    !!strstr(mpp->features, "queue_if_no_path") !=
+	    !!strstr(cmpp->features, "queue_if_no_path")) {
 		mpp->action =  ACT_RELOAD;
 		condlog(3, "%s: set ACT_RELOAD (no_path_retry change)",
 			mpp->alias);
@@ -575,7 +576,8 @@  select_action (struct multipath * mpp, vector curmp, int force_reload)
 	}
 
 	if (mpp->retain_hwhandler != RETAIN_HWHANDLER_UNDEF &&
-	    mpp->retain_hwhandler != cmpp->retain_hwhandler &&
+	    !!strstr(mpp->features, "retain_attached_hw_handler") !=
+	    !!strstr(cmpp->features, "retain_attached_hw_handler") &&
 	    get_linux_version_code() < KERNEL_VERSION(4, 3, 0)) {
 		mpp->action = ACT_RELOAD;
 		condlog(3, "%s: set ACT_RELOAD (retain_hwhandler change)",
@@ -1060,21 +1062,6 @@  int coalesce_paths (struct vectors * vecs, vector newmp, char * refwwid,
 				remove_feature(&mpp->features,
 					       "queue_if_no_path");
 		}
-		else if (mpp->no_path_retry != NO_PATH_RETRY_UNDEF) {
-			if (mpp->no_path_retry == NO_PATH_RETRY_FAIL) {
-				condlog(3, "%s: unset queue_if_no_path feature",
-					mpp->alias);
-				if (!dm_queue_if_no_path(mpp->alias, 0))
-					remove_feature(&mpp->features,
-						       "queue_if_no_path");
-			} else {
-				condlog(3, "%s: set queue_if_no_path feature",
-					mpp->alias);
-				if (!dm_queue_if_no_path(mpp->alias, 1))
-					add_feature(&mpp->features,
-						    "queue_if_no_path");
-			}
-		}
 
 		if (!is_daemon && mpp->action != ACT_NOTHING) {
 			conf = get_multipath_config();
diff --git a/libmultipath/dmparser.c b/libmultipath/dmparser.c
index b647c25..642f44f 100644
--- a/libmultipath/dmparser.c
+++ b/libmultipath/dmparser.c
@@ -83,23 +83,15 @@  assemble_map (struct multipath * mp, char * params, int len)
 	nr_priority_groups = VECTOR_SIZE(mp->pg);
 	initial_pg_nr = (nr_priority_groups ? mp->bestpg : 0);
 
-	f = STRDUP(mp->features);
-
-	/*
-	 * We have to set 'queue_if_no_path' here even
-	 * to avoid path failures during map reload.
-	 */
-	if (mp->no_path_retry == NO_PATH_RETRY_FAIL) {
-		/* remove queue_if_no_path settings */
-		condlog(3, "%s: remove queue_if_no_path from '%s'",
-			mp->alias, mp->features);
-		remove_feature(&f, no_path_retry);
-	} else if (mp->no_path_retry != NO_PATH_RETRY_UNDEF) {
-		add_feature(&f, no_path_retry);
+	if (mp->no_path_retry != NO_PATH_RETRY_UNDEF  &&
+	    mp->no_path_retry != NO_PATH_RETRY_FAIL) {
+		add_feature(&mp->features, no_path_retry);
 	}
 	if (mp->retain_hwhandler == RETAIN_HWHANDLER_ON &&
 	    get_linux_version_code() < KERNEL_VERSION(4, 3, 0))
-		add_feature(&f, retain_hwhandler);
+		add_feature(&mp->features, retain_hwhandler);
+
+	f = STRDUP(mp->features);
 
 	APPEND(p, end, "%s %s %i %i", f, mp->hwhandler, nr_priority_groups,
 	       initial_pg_nr);
@@ -148,7 +140,6 @@  int disassemble_map(vector pathvec, char *params, struct multipath *mpp,
 	int num_paths = 0;
 	int num_paths_args = 0;
 	int def_minio = 0;
-	int no_path_retry = NO_PATH_RETRY_UNDEF;
 	struct path * pp;
 	struct pathgroup * pgp;
 
@@ -165,8 +156,6 @@  int disassemble_map(vector pathvec, char *params, struct multipath *mpp,
 		return 1;
 
 	num_features = atoi(mpp->features);
-	no_path_retry = mpp->no_path_retry;
-	mpp->no_path_retry = NO_PATH_RETRY_UNDEF;
 
 	for (i = 0; i < num_features; i++) {
 		p += get_word(p, &word);
@@ -178,23 +167,11 @@  int disassemble_map(vector pathvec, char *params, struct multipath *mpp,
 			FREE(word);
 			return 1;
 		}
-		setup_feature(mpp, word);
 
 		FREE(word);
 	}
 
 	/*
-	 * Reset no_path_retry.
-	 * - if not set from features
-	 * - if queue_if_no_path is set from features but
-	 *   no_path_retry > 0 is selected.
-	 */
-	if ((mpp->no_path_retry == NO_PATH_RETRY_UNDEF ||
-	     mpp->no_path_retry == NO_PATH_RETRY_QUEUE) &&
-	    mpp->no_path_retry != no_path_retry)
-		mpp->no_path_retry = no_path_retry;
-
-	/*
 	 * hwhandler
 	 */
 	p += get_word(p, &mpp->hwhandler);
diff --git a/libmultipath/structs.c b/libmultipath/structs.c
index 3e057f5..c42d765 100644
--- a/libmultipath/structs.c
+++ b/libmultipath/structs.c
@@ -497,23 +497,6 @@  first_path (struct multipath * mpp)
 	return pgp?VECTOR_SLOT(pgp->paths, 0):NULL;
 }
 
-void setup_feature(struct multipath *mpp, char *feature)
-{
-	if (!strncmp(feature, "queue_if_no_path", 16)) {
-		if (mpp->no_path_retry <= NO_PATH_RETRY_UNDEF)
-			mpp->no_path_retry = NO_PATH_RETRY_QUEUE;
-		else
-			condlog(1, "%s: ignoring feature queue_if_no_path because no_path_retry = %d",
-				mpp->alias, mpp->no_path_retry);
-	} else if (!strcmp(feature, "retain_attached_hw_handler")) {
-		if (mpp->retain_hwhandler != RETAIN_HWHANDLER_OFF)
-			mpp->retain_hwhandler = RETAIN_HWHANDLER_ON;
-		else
-			condlog(1, "%s: ignoring feature 'retain_attached_hw_handler'",
-				mpp->alias);
-	}
-}
-
 int add_feature(char **f, const char *n)
 {
 	int c = 0, d, l;
diff --git a/libmultipath/structs.h b/libmultipath/structs.h
index 4d738d2..effa52f 100644
--- a/libmultipath/structs.h
+++ b/libmultipath/structs.h
@@ -386,7 +386,6 @@  struct path * first_path (struct multipath * mpp);
 int pathcountgr (struct pathgroup *, int);
 int pathcount (struct multipath *, int);
 int pathcmp (struct pathgroup *, struct pathgroup *);
-void setup_feature(struct multipath *, char *);
 int add_feature (char **, const char *);
 int remove_feature (char **, const char *);