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

2017-06-19 Thread Martin Wilck 
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 
> > ---
> >  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),
> > +   >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),
> > +   >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(>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 , Tel. +49 (0)911 74053 2107
SUSE Linux GmbH, GF:

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

2017-06-15 Thread Benjamin Marzinski 
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 
> ---
>  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),
> + >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),
> + >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(>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.

-Ben

>   }
> + if (strstr(mp->features, r_a_h_h) &&
> + mp->retain_hwhandler == RETAIN_HWHANDLER_OFF) {
> + condlog(2, "%s: ignoring feature '%s' because %s is set to 
> 'off'",
> + mp->alias, r_a_h_h, r_a_h_h);
> + remove_feature(>features, r_a_h_h);
> + }
> +
> + condlog(3, "%s: features = \"%s\" %s", mp->alias, mp->features, origin);
>   return 0;
>  }
>  
> @@ -469,9

[dm-devel] [PATCH 3/7] libmultipath: clarify option conflicts for "features"

2017-06-13 Thread Martin Wilck 
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 
---
 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),
+   >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),
+   >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(>features, q_i_n_p);
+   };
}
+   if (strstr(mp->features, r_a_h_h) &&
+   mp->retain_hwhandler == RETAIN_HWHANDLER_OFF) {
+   condlog(2, "%s: ignoring feature '%s' because %s is set to 
'off'",
+   mp->alias, r_a_h_h, r_a_h_h);
+   remove_feature(>features, r_a_h_h);
+   }
+
+   condlog(3, "%s: features = \"%s\" %s", mp->alias, mp->features, origin);
return 0;
 }
 
@@ -469,9 +483,6 @@ out:
if (origin)
condlog(3, "%s: no_path_retry = %s %s", mp->alias, buff,
origin);
-   else if (mp->no_path_retry != NO_PATH_RETRY_UNDEF)
-   condlog(3, "%s: no_path_retry = %s (inherited setting)",
-   mp->alias, buff);
else
condlog(3, "%s: no_path_retry = undef (setting: multipath 
internal)",
mp->alias);
-- 
2.13.0

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