[PATCH] multipathd: restore paths after reconfigure

[PATCH] multipathd: restore paths after reconfigure

[Benjamin Marzinski]

multipathd has code to finish gathering the information of paths that
were not active at the time they were discovered. When the checker loop
goes to check a path, and notices that it wasn't fully initialized, it
is supposed to complete the initialization.  However the code is broken.
This means that if you reconfigure multipathd while paths are down, they
will no longer be usable. This patch makes sure that check_path will
actually rerun pathinfo to finish setting up the path, so that after the
path comes back up, it will be usable again.

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmultipath/discovery.c |  5 +++--
 multipathd/main.c        | 20 ++++++++++++++++----
 2 files changed, 19 insertions(+), 6 deletions(-)

diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
index d5215f9..eb2e926 100644
--- a/libmultipath/discovery.c
+++ b/libmultipath/discovery.c
@@ -1713,8 +1713,9 @@ pathinfo (struct path *pp, struct config *conf, int mask)
 			get_prio(pp);
 		}
 	}
-
-	pp->initialized = INIT_OK;
+	
+	if ((mask & DI_ALL) == DI_ALL)
+		pp->initialized = INIT_OK;
 	return PATHINFO_OK;
 
 blank:
diff --git a/multipathd/main.c b/multipathd/main.c
index 9682b3e..6e3ae69 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -1439,7 +1439,8 @@ int update_path_groups(struct multipath *mpp, struct vectors *vecs, int refresh)
 }
 
 /*
- * Returns '1' if the path has been checked, '0' otherwise
+ * Returns '1' if the path has been checked, '-1' if it was blacklisted
+ * and '0' otherwise
  */
 int
 check_path (struct vectors * vecs, struct path * pp, int ticks)
@@ -1452,6 +1453,7 @@ check_path (struct vectors * vecs, struct path * pp, int ticks)
 	int oldchkrstate = pp->chkrstate;
 	int retrigger_tries, checkint;
 	struct config *conf;
+	int ret;
 
 	if ((pp->initialized == INIT_OK ||
 	     pp->initialized == INIT_REQUESTED_UDEV) && !pp->mpp)
@@ -1511,10 +1513,14 @@ check_path (struct vectors * vecs, struct path * pp, int ticks)
 		    (newstate == PATH_UP || newstate == PATH_GHOST)) {
 			condlog(2, "%s: add missing path", pp->dev);
 			conf = get_multipath_config();
-			if (pathinfo(pp, conf, DI_ALL) == 0) {
+			ret = pathinfo(pp, conf, DI_ALL | DI_BLACKLIST);
+			if (ret == PATHINFO_OK) {
 				ev_add_path(pp, vecs);
 				pp->tick = 1;
-			}
+			} else if (ret == PATHINFO_SKIPPED) {
+				put_multipath_config(conf);
+				return -1;
+			} 
 			put_multipath_config(conf);
 		}
 		return 0;
@@ -1779,7 +1785,13 @@ checkerloop (void *ap)
 			lock(vecs->lock);
 			pthread_testcancel();
 			vector_foreach_slot (vecs->pathvec, pp, i) {
-				num_paths += check_path(vecs, pp, ticks);
+				rc = check_path(vecs, pp, ticks);
+				if (rc < 0) {
+					vector_del_slot(vecs->pathvec, i);
+					free_path(pp);
+					i--;
+				} else;
+					num_paths += rc;
 			}
 			lock_cleanup_pop(vecs->lock);
 		}
-- 
1.8.3.1

Re: [PATCH] multipathd: restore paths after reconfigure

[Christophe Varoqui]

[-- Attachment #1.1: Type: text/plain, Size: 3935 bytes --]

Merged.
Thanks.

On Fri, Jul 1, 2016 at 11:46 PM, Benjamin Marzinski <bmarzins@redhat.com>
wrote:

> multipathd has code to finish gathering the information of paths that
> were not active at the time they were discovered. When the checker loop
> goes to check a path, and notices that it wasn't fully initialized, it
> is supposed to complete the initialization.  However the code is broken.
> This means that if you reconfigure multipathd while paths are down, they
> will no longer be usable. This patch makes sure that check_path will
> actually rerun pathinfo to finish setting up the path, so that after the
> path comes back up, it will be usable again.
>
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> ---
>  libmultipath/discovery.c |  5 +++--
>  multipathd/main.c        | 20 ++++++++++++++++----
>  2 files changed, 19 insertions(+), 6 deletions(-)
>
> diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
> index d5215f9..eb2e926 100644
> --- a/libmultipath/discovery.c
> +++ b/libmultipath/discovery.c
> @@ -1713,8 +1713,9 @@ pathinfo (struct path *pp, struct config *conf, int
> mask)
>                         get_prio(pp);
>                 }
>         }
> -
> -       pp->initialized = INIT_OK;
> +
> +       if ((mask & DI_ALL) == DI_ALL)
> +               pp->initialized = INIT_OK;
>         return PATHINFO_OK;
>
>  blank:
> diff --git a/multipathd/main.c b/multipathd/main.c
> index 9682b3e..6e3ae69 100644
> --- a/multipathd/main.c
> +++ b/multipathd/main.c
> @@ -1439,7 +1439,8 @@ int update_path_groups(struct multipath *mpp, struct
> vectors *vecs, int refresh)
>  }
>
>  /*
> - * Returns '1' if the path has been checked, '0' otherwise
> + * Returns '1' if the path has been checked, '-1' if it was blacklisted
> + * and '0' otherwise
>   */
>  int
>  check_path (struct vectors * vecs, struct path * pp, int ticks)
> @@ -1452,6 +1453,7 @@ check_path (struct vectors * vecs, struct path * pp,
> int ticks)
>         int oldchkrstate = pp->chkrstate;
>         int retrigger_tries, checkint;
>         struct config *conf;
> +       int ret;
>
>         if ((pp->initialized == INIT_OK ||
>              pp->initialized == INIT_REQUESTED_UDEV) && !pp->mpp)
> @@ -1511,10 +1513,14 @@ check_path (struct vectors * vecs, struct path *
> pp, int ticks)
>                     (newstate == PATH_UP || newstate == PATH_GHOST)) {
>                         condlog(2, "%s: add missing path", pp->dev);
>                         conf = get_multipath_config();
> -                       if (pathinfo(pp, conf, DI_ALL) == 0) {
> +                       ret = pathinfo(pp, conf, DI_ALL | DI_BLACKLIST);
> +                       if (ret == PATHINFO_OK) {
>                                 ev_add_path(pp, vecs);
>                                 pp->tick = 1;
> -                       }
> +                       } else if (ret == PATHINFO_SKIPPED) {
> +                               put_multipath_config(conf);
> +                               return -1;
> +                       }
>                         put_multipath_config(conf);
>                 }
>                 return 0;
> @@ -1779,7 +1785,13 @@ checkerloop (void *ap)
>                         lock(vecs->lock);
>                         pthread_testcancel();
>                         vector_foreach_slot (vecs->pathvec, pp, i) {
> -                               num_paths += check_path(vecs, pp, ticks);
> +                               rc = check_path(vecs, pp, ticks);
> +                               if (rc < 0) {
> +                                       vector_del_slot(vecs->pathvec, i);
> +                                       free_path(pp);
> +                                       i--;
> +                               } else;
> +                                       num_paths += rc;
>                         }
>                         lock_cleanup_pop(vecs->lock);
>                 }
> --
> 1.8.3.1
>
>

[-- Attachment #1.2: Type: text/html, Size: 5204 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH] multipathd: restore paths after reconfigure

[Bart Van Assche]

On 07/01/2016 02:46 PM, Benjamin Marzinski wrote:
> multipathd has code to finish gathering the information of paths that
> were not active at the time they were discovered. When the checker loop
> goes to check a path, and notices that it wasn't fully initialized, it
> is supposed to complete the initialization.  However the code is broken.
> This means that if you reconfigure multipathd while paths are down, they
> will no longer be usable. This patch makes sure that check_path will
> actually rerun pathinfo to finish setting up the path, so that after the
> path comes back up, it will be usable again.
> [ ... ]
> @@ -1779,7 +1785,13 @@ checkerloop (void *ap)
>  			lock(vecs->lock);
>  			pthread_testcancel();
>  			vector_foreach_slot (vecs->pathvec, pp, i) {
> -				num_paths += check_path(vecs, pp, ticks);
> +				rc = check_path(vecs, pp, ticks);
> +				if (rc < 0) {
> +					vector_del_slot(vecs->pathvec, i);
> +					free_path(pp);
> +					i--;
> +				} else;
> +					num_paths += rc;
>  			}
>  			lock_cleanup_pop(vecs->lock);
>  		}

Hi Ben,

Was the semicolon after the "else" intended or was it a typo?

Thanks,

Bart.

Re: [PATCH] multipathd: restore paths after reconfigure

[Benjamin Marzinski]

On Thu, Jul 21, 2016 at 09:29:37AM -0700, Bart Van Assche wrote:
> On 07/01/2016 02:46 PM, Benjamin Marzinski wrote:
> >multipathd has code to finish gathering the information of paths that
> >were not active at the time they were discovered. When the checker loop
> >goes to check a path, and notices that it wasn't fully initialized, it
> >is supposed to complete the initialization.  However the code is broken.
> >This means that if you reconfigure multipathd while paths are down, they
> >will no longer be usable. This patch makes sure that check_path will
> >actually rerun pathinfo to finish setting up the path, so that after the
> >path comes back up, it will be usable again.
> >[ ... ]
> >@@ -1779,7 +1785,13 @@ checkerloop (void *ap)
> > 			lock(vecs->lock);
> > 			pthread_testcancel();
> > 			vector_foreach_slot (vecs->pathvec, pp, i) {
> >-				num_paths += check_path(vecs, pp, ticks);
> >+				rc = check_path(vecs, pp, ticks);
> >+				if (rc < 0) {
> >+					vector_del_slot(vecs->pathvec, i);
> >+					free_path(pp);
> >+					i--;
> >+				} else;
> >+					num_paths += rc;
> > 			}
> > 			lock_cleanup_pop(vecs->lock);
> > 		}
> 
> Hi Ben,
> 
> Was the semicolon after the "else" intended or was it a typo?

Oops! That was a typo. I'll send off a quick patch. Thanks for catching
that.

-Ben

> 
> Thanks,
> 
> Bart.