Re: [PATCH 3/7] libmultipath: clarify option conflicts for "features"

[Martin Wilck <mwilck@suse.com>]

Hi Ben,

On Thu, 2017-06-15 at 14:54 -0500, Benjamin Marzinski wrote:
> On Wed, Jun 14, 2017 at 12:55:50AM +0200, Martin Wilck wrote:
> > The "features" option in multipath.conf can possibly conflict
> > with the "no_path_retry" and "retain_attached_hw_handler" options.
> > 
> > Currently, "no_path_retry" takes precedence, unless it is set to
> > "fail", in which case it's overridden. No precedence rules are
> > defined for "retain_attached_hw_handler".
> > 
> > Make this behavior more consistent by always giving precedence
> > to the explicit config file options, and improve logging.
> > 
> > Signed-off-by: Martin Wilck <mwilck@suse.com>
> > ---
> >  libmultipath/configure.c |  4 ++--
> >  libmultipath/propsel.c   | 37 ++++++++++++++++++++++++----------
> > ---
> >  2 files changed, 26 insertions(+), 15 deletions(-)
> > 
> > diff --git a/libmultipath/configure.c b/libmultipath/configure.c
> > index bd090d9a..fd4721dd 100644
> > --- a/libmultipath/configure.c
> > +++ b/libmultipath/configure.c
> > @@ -280,11 +280,11 @@ int setup_map(struct multipath *mpp, char
> > *params, int params_size)
> >  	select_pgfailback(conf, mpp);
> >  	select_pgpolicy(conf, mpp);
> >  	select_selector(conf, mpp);
> > -	select_features(conf, mpp);
> >  	select_hwhandler(conf, mpp);
> > +	select_no_path_retry(conf, mpp);
> > +	select_features(conf, mpp);
> >  	select_rr_weight(conf, mpp);
> >  	select_minio(conf, mpp);
> > -	select_no_path_retry(conf, mpp);
> >  	select_mode(conf, mpp);
> >  	select_uid(conf, mpp);
> >  	select_gid(conf, mpp);
> > diff --git a/libmultipath/propsel.c b/libmultipath/propsel.c
> > index 99d17e65..4267aa04 100644
> > --- a/libmultipath/propsel.c
> > +++ b/libmultipath/propsel.c
> > @@ -272,6 +272,9 @@ out:
> >  int select_features(struct config *conf, struct multipath *mp)
> >  {
> >  	char *origin;
> > +	char buff[12];
> > +	static const char q_i_n_p[] = "queue_if_no_path";
> > +	static const char r_a_h_h[] =
> > "retain_attached_hw_handler";
> >  
> >  	mp_set_mpe(features);
> >  	mp_set_ovr(features);
> > @@ -280,19 +283,30 @@ int select_features(struct config *conf,
> > struct multipath *mp)
> >  	mp_set_default(features, DEFAULT_FEATURES);
> >  out:
> >  	mp->features = STRDUP(mp->features);
> > -	condlog(3, "%s: features = \"%s\" %s", mp->alias, mp-
> > >features, origin);
> >  
> > -	if (strstr(mp->features, "queue_if_no_path")) {
> > -		if (mp->no_path_retry == NO_PATH_RETRY_UNDEF)
> > +	if (strstr(mp->features, q_i_n_p)) {
> > +		if (mp->no_path_retry == NO_PATH_RETRY_UNDEF) {
> >  			mp->no_path_retry = NO_PATH_RETRY_QUEUE;
> > -		else if (mp->no_path_retry == NO_PATH_RETRY_FAIL)
> > {
> > -			condlog(1, "%s: config error, overriding
> > 'no_path_retry' value",
> > -				mp->alias);
> > -			mp->no_path_retry = NO_PATH_RETRY_QUEUE;
> > -		} else if (mp->no_path_retry !=
> > NO_PATH_RETRY_QUEUE)
> > -			condlog(1, "%s: config error, ignoring
> > 'queue_if_no_path' because no_path_retry=%d",
> > -				mp->alias, mp->no_path_retry);
> > +			print_no_path_retry(buff, sizeof(buff),
> > +					    &mp->no_path_retry);
> > +			condlog(3, "%s: no_path_retry = %s
> > (inherited setting from feature '%s')",
> > +				mp->alias, buff, q_i_n_p);
> > +		} else {
> > +			print_no_path_retry(buff, sizeof(buff),
> > +					    &mp->no_path_retry);
> > +			condlog(2, "%s: ignoring feature '%s'
> > because no_path_retry is set to '%s'",
> > +				mp->alias, q_i_n_p, buff);
> > +			remove_feature(&mp->features, q_i_n_p);
> > +		};
> 
> This is just a nit, and it won't hurt anything by remaining unfixed,
> but
> it is odd that if you go into select_features() with no_path_retry
> set
> to queue (for any length of time) and features includes
> "queue_if_no_path", you will exit with no_path_retry still set to
> queue
> and "queue_if_no_path" removed from features. However if you go into
> select_features with no_path_retry set to undef and features includes
> "queue_if_no_path", you will exit with no_path_retry set to queue and
> "queue_if_no_path" still included in features. It seems odd that in
> the
> second case, you explicitly set these options to the values that you
> started with in the first case (which you later changed). A perhaps
> more
> sensible option would be to only remove "queue_if_no_path" from
> features
> if no_path_retry is set to something other than NO_PATH_RETRY_QUEUE.


You're right. For internal consistency, I prefer to remove
"queue_if_no_path" from the internal feature string always. Modified
patch is in the works.

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