| Message ID | 1467409569-16088-1-git-send-email-bmarzins@redhat.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | christophe varoqui |
| Headers | show |
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 > > -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
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. -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
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. -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
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); }
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(-)