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@xxxxxxxx>
> > ---
> > 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@xxxxxxxx>, 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@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/dm-devel
