| Message ID | 20240713060506.2015463-6-bmarzins@redhat.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | christophe varoqui |
| Headers | show |
| Series | path checker refactor and misc fixes | expand |
On Sat, 2024-07-13 at 02:04 -0400, Benjamin Marzinski wrote: > Pull the code to handle uninitialized paths out of check_path() and > into > a new function called handle_uninitialized_paths(). This should cause > no functional changes. > > Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com> > --- > multipathd/main.c | 171 ++++++++++++++++++++++++++++---------------- > -- > 1 file changed, 105 insertions(+), 66 deletions(-) > > diff --git a/multipathd/main.c b/multipathd/main.c > index 9b1b5226..9e329e89 100644 > --- a/multipathd/main.c > +++ b/multipathd/main.c > @@ -2343,8 +2343,7 @@ check_path_state(struct path *pp) > } > > /* > - * Returns '1' if the path has been checked, '-1' if it was > blacklisted > - * and '0' otherwise > + * Returns '1' if the path has been checked and '0' otherwise > */ > int > check_path (struct vectors * vecs, struct path * pp, unsigned int > ticks) > @@ -2354,16 +2353,13 @@ check_path (struct vectors * vecs, struct > path * pp, unsigned int ticks) > int chkr_new_path_up = 0; > int disable_reinstate = 0; > int oldchkrstate = pp->chkrstate; > - int retrigger_tries; > unsigned int checkint, max_checkint; > struct config *conf; > int marginal_pathgroups, marginal_changed = 0; > int ret; > bool need_reload; > > - if (((pp->initialized == INIT_OK || pp->initialized == > INIT_PARTIAL || > - pp->initialized == INIT_REQUESTED_UDEV) && !pp->mpp) > || > - pp->initialized == INIT_REMOVED) > + if (pp->initialized == INIT_REMOVED) > return 0; > > if (pp->tick) > @@ -2372,7 +2368,6 @@ check_path (struct vectors * vecs, struct path > * pp, unsigned int ticks) > return 0; /* don't check this path yet */ > > conf = get_multipath_config(); > - retrigger_tries = conf->retrigger_tries; > checkint = conf->checkint; > max_checkint = conf->max_checkint; > marginal_pathgroups = conf->marginal_pathgroups; > @@ -2383,38 +2378,6 @@ check_path (struct vectors * vecs, struct path > * pp, unsigned int ticks) > pp->checkint = checkint; > }; > > - if (!pp->mpp && pp->initialized == INIT_MISSING_UDEV) { > - if (pp->retriggers < retrigger_tries) { > - static const char change[] = "change"; > - ssize_t ret; > - > - condlog(2, "%s: triggering change event to > reinitialize", > - pp->dev); > - pp->initialized = INIT_REQUESTED_UDEV; > - pp->retriggers++; > - ret = sysfs_attr_set_value(pp->udev, > "uevent", change, > - sizeof(change) - > 1); > - if (ret != sizeof(change) - 1) > - log_sysfs_attr_set_value(1, ret, > - "%s: failed > to trigger change event", > - pp->dev); > - return 0; > - } else { > - condlog(1, "%s: not initialized after %d > udev retriggers", > - pp->dev, retrigger_tries); > - /* > - * Make sure that the "add missing path" > code path > - * below may reinstate the path later, if it > ever > - * comes up again. > - * The WWID needs not be cleared; if it was > set, the > - * state hadn't been INIT_MISSING_UDEV in > the first > - * place. > - */ > - pp->initialized = INIT_FAILED; > - return 0; > - } > - } > - > /* > * provision a next check soonest, > * in case we exit abnormally from here > @@ -2435,32 +2398,6 @@ check_path (struct vectors * vecs, struct path > * pp, unsigned int ticks) > pp->checkint = checkint; > return 1; > } > - if (!pp->mpp) { > - if (!strlen(pp->wwid) && > - (pp->initialized == INIT_FAILED || > - pp->initialized == INIT_NEW) && > - (newstate == PATH_UP || newstate == PATH_GHOST)) > { > - condlog(2, "%s: add missing path", pp->dev); > - conf = get_multipath_config(); > - pthread_cleanup_push(put_multipath_config, > conf); > - ret = pathinfo(pp, conf, DI_ALL | > DI_BLACKLIST); > - pthread_cleanup_pop(1); > - /* INIT_OK implies ret == PATHINFO_OK */ > - if (pp->initialized == INIT_OK) { > - ev_add_path(pp, vecs, 1); > - pp->tick = 1; > - } else { > - if (ret == PATHINFO_SKIPPED) > - return -1; > - /* > - * We failed multiple times to > initialize this > - * path properly. Don't re-check too > often. > - */ > - pp->checkint = max_checkint; > - } > - } > - return 0; > - } > /* > * Async IO in flight. Keep the previous path state > * and reschedule as soon as possible > @@ -2679,6 +2616,104 @@ check_path (struct vectors * vecs, struct > path * pp, unsigned int ticks) > return 1; > } > > +/* > + * Returns -1 if the path was blacklisted, and 0 otherwise > + */ > +static int > +handle_uninitialized_path(struct vectors * vecs, struct path * pp, > + unsigned int ticks) > +{ > + int newstate; > + int retrigger_tries; > + unsigned int checkint, max_checkint; > + struct config *conf; > + int ret; > + > + if (((pp->initialized == INIT_OK || pp->initialized == > INIT_PARTIAL || > + pp->initialized == INIT_REQUESTED_UDEV) && !pp->mpp) > || > + pp->initialized == INIT_REMOVED) > + return 0; !pp->mpp always holds if this function is called, so we can skip this part of the test. > + > + if (pp->tick) > + pp->tick -= (pp->tick > ticks) ? ticks : pp->tick; > + if (pp->tick) > + return 0; /* don't check this path yet */ IMO it would make sense to move this code into checkerloop(). Actually we could make sure that for those paths that are initialized but not part of an mpp (see above), pp->tick is always zero. > + > + conf = get_multipath_config(); > + retrigger_tries = conf->retrigger_tries; > + checkint = conf->checkint; > + max_checkint = conf->max_checkint; > + put_multipath_config(conf); > + > + if (!pp->mpp && pp->initialized == INIT_MISSING_UDEV) { > + if (pp->retriggers < retrigger_tries) { > + static const char change[] = "change"; > + ssize_t ret; > + > + condlog(2, "%s: triggering change event to > reinitialize", > + pp->dev); > + pp->initialized = INIT_REQUESTED_UDEV; > + pp->retriggers++; > + ret = sysfs_attr_set_value(pp->udev, > "uevent", change, > + sizeof(change) - > 1); > + if (ret != sizeof(change) - 1) > + log_sysfs_attr_set_value(1, ret, > + "%s: failed > to trigger change event", > + pp->dev); > + return 0; > + } else { > + condlog(1, "%s: not initialized after %d > udev retriggers", > + pp->dev, retrigger_tries); > + /* > + * Make sure that the "add missing path" > code path > + * below may reinstate the path later, if it > ever > + * comes up again. > + * The WWID needs not be cleared; if it was > set, the > + * state hadn't been INIT_MISSING_UDEV in > the first > + * place. > + */ > + pp->initialized = INIT_FAILED; > + return 0; > + } > + } > + > + /* > + * provision a next check soonest, > + * in case we exit abnormally from here > + */ > + pp->tick = checkint; > + > + newstate = check_path_state(pp); > + > + if (!pp->mpp) { Again, this test can be skipped, or am I overlooking something? > + if (!strlen(pp->wwid) && > + (pp->initialized == INIT_FAILED || > + pp->initialized == INIT_NEW) && > + (newstate == PATH_UP || newstate == PATH_GHOST)) > { > + condlog(2, "%s: add missing path", pp->dev); > + conf = get_multipath_config(); > + pthread_cleanup_push(put_multipath_config, > conf); > + ret = pathinfo(pp, conf, DI_ALL | > DI_BLACKLIST); > + pthread_cleanup_pop(1); > + /* INIT_OK implies ret == PATHINFO_OK */ > + if (pp->initialized == INIT_OK) { > + ev_add_path(pp, vecs, 1); > + pp->tick = 1; > + } else { > + if (ret == PATHINFO_SKIPPED) > + return -1; > + /* > + * We failed multiple times to > initialize this > + * path properly. Don't re-check too > often. > + */ > + pp->checkint = max_checkint; > + } > + } > + return 0; > + } > + return 0; > +} > + > enum checker_state { > CHECKER_STARTING, > CHECKER_RUNNING, > @@ -2751,7 +2786,11 @@ checkerloop (void *ap) > if (pp->is_checked) > continue; > pp->is_checked = true; > - rc = check_path(vecs, pp, ticks); > + if (pp->mpp) > + rc = check_path(vecs, pp, > ticks); > + else > + rc = > handle_uninitialized_path(vecs, pp, > + > ticks); > if (rc < 0) { > condlog(1, "%s: check_path() > failed, removing", > pp->dev);
On Mon, 2024-07-15 at 17:34 +0200, Martin Wilck wrote: > On Sat, 2024-07-13 at 02:04 -0400, Benjamin Marzinski wrote: > > Pull the code to handle uninitialized paths out of check_path() and > > into > > a new function called handle_uninitialized_paths(). This should > > cause > > no functional changes. > > > > Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com> > > --- > > multipathd/main.c | 171 ++++++++++++++++++++++++++++-------------- > > -- > > -- > > 1 file changed, 105 insertions(+), 66 deletions(-) > > !pp->mpp always holds if this function is called, so we can skip this > part of the test. > Just realized that you did this in patch 7. LGTM, then, but if you resubmit, I suggest you just squash these two. Martin
On Mon, Jul 15, 2024 at 05:36:54PM +0200, Martin Wilck wrote: > On Mon, 2024-07-15 at 17:34 +0200, Martin Wilck wrote: > > On Sat, 2024-07-13 at 02:04 -0400, Benjamin Marzinski wrote: > > > Pull the code to handle uninitialized paths out of check_path() and > > > into > > > a new function called handle_uninitialized_paths(). This should > > > cause > > > no functional changes. > > > > > > Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com> > > > --- > > > multipathd/main.c | 171 ++++++++++++++++++++++++++++-------------- > > > -- > > > -- > > > 1 file changed, 105 insertions(+), 66 deletions(-) > > > > !pp->mpp always holds if this function is called, so we can skip this > > part of the test. > > > > Just realized that you did this in patch 7. > > LGTM, then, but if you resubmit, I suggest you just squash these two. Sure. > > Martin
diff --git a/multipathd/main.c b/multipathd/main.c index 9b1b5226..9e329e89 100644 --- a/multipathd/main.c +++ b/multipathd/main.c @@ -2343,8 +2343,7 @@ check_path_state(struct path *pp) } /* - * Returns '1' if the path has been checked, '-1' if it was blacklisted - * and '0' otherwise + * Returns '1' if the path has been checked and '0' otherwise */ int check_path (struct vectors * vecs, struct path * pp, unsigned int ticks) @@ -2354,16 +2353,13 @@ check_path (struct vectors * vecs, struct path * pp, unsigned int ticks) int chkr_new_path_up = 0; int disable_reinstate = 0; int oldchkrstate = pp->chkrstate; - int retrigger_tries; unsigned int checkint, max_checkint; struct config *conf; int marginal_pathgroups, marginal_changed = 0; int ret; bool need_reload; - if (((pp->initialized == INIT_OK || pp->initialized == INIT_PARTIAL || - pp->initialized == INIT_REQUESTED_UDEV) && !pp->mpp) || - pp->initialized == INIT_REMOVED) + if (pp->initialized == INIT_REMOVED) return 0; if (pp->tick) @@ -2372,7 +2368,6 @@ check_path (struct vectors * vecs, struct path * pp, unsigned int ticks) return 0; /* don't check this path yet */ conf = get_multipath_config(); - retrigger_tries = conf->retrigger_tries; checkint = conf->checkint; max_checkint = conf->max_checkint; marginal_pathgroups = conf->marginal_pathgroups; @@ -2383,38 +2378,6 @@ check_path (struct vectors * vecs, struct path * pp, unsigned int ticks) pp->checkint = checkint; }; - if (!pp->mpp && pp->initialized == INIT_MISSING_UDEV) { - if (pp->retriggers < retrigger_tries) { - static const char change[] = "change"; - ssize_t ret; - - condlog(2, "%s: triggering change event to reinitialize", - pp->dev); - pp->initialized = INIT_REQUESTED_UDEV; - pp->retriggers++; - ret = sysfs_attr_set_value(pp->udev, "uevent", change, - sizeof(change) - 1); - if (ret != sizeof(change) - 1) - log_sysfs_attr_set_value(1, ret, - "%s: failed to trigger change event", - pp->dev); - return 0; - } else { - condlog(1, "%s: not initialized after %d udev retriggers", - pp->dev, retrigger_tries); - /* - * Make sure that the "add missing path" code path - * below may reinstate the path later, if it ever - * comes up again. - * The WWID needs not be cleared; if it was set, the - * state hadn't been INIT_MISSING_UDEV in the first - * place. - */ - pp->initialized = INIT_FAILED; - return 0; - } - } - /* * provision a next check soonest, * in case we exit abnormally from here @@ -2435,32 +2398,6 @@ check_path (struct vectors * vecs, struct path * pp, unsigned int ticks) pp->checkint = checkint; return 1; } - if (!pp->mpp) { - if (!strlen(pp->wwid) && - (pp->initialized == INIT_FAILED || - pp->initialized == INIT_NEW) && - (newstate == PATH_UP || newstate == PATH_GHOST)) { - condlog(2, "%s: add missing path", pp->dev); - conf = get_multipath_config(); - pthread_cleanup_push(put_multipath_config, conf); - ret = pathinfo(pp, conf, DI_ALL | DI_BLACKLIST); - pthread_cleanup_pop(1); - /* INIT_OK implies ret == PATHINFO_OK */ - if (pp->initialized == INIT_OK) { - ev_add_path(pp, vecs, 1); - pp->tick = 1; - } else { - if (ret == PATHINFO_SKIPPED) - return -1; - /* - * We failed multiple times to initialize this - * path properly. Don't re-check too often. - */ - pp->checkint = max_checkint; - } - } - return 0; - } /* * Async IO in flight. Keep the previous path state * and reschedule as soon as possible @@ -2679,6 +2616,104 @@ check_path (struct vectors * vecs, struct path * pp, unsigned int ticks) return 1; } +/* + * Returns -1 if the path was blacklisted, and 0 otherwise + */ +static int +handle_uninitialized_path(struct vectors * vecs, struct path * pp, + unsigned int ticks) +{ + int newstate; + int retrigger_tries; + unsigned int checkint, max_checkint; + struct config *conf; + int ret; + + if (((pp->initialized == INIT_OK || pp->initialized == INIT_PARTIAL || + pp->initialized == INIT_REQUESTED_UDEV) && !pp->mpp) || + pp->initialized == INIT_REMOVED) + return 0; + + if (pp->tick) + pp->tick -= (pp->tick > ticks) ? ticks : pp->tick; + if (pp->tick) + return 0; /* don't check this path yet */ + + conf = get_multipath_config(); + retrigger_tries = conf->retrigger_tries; + checkint = conf->checkint; + max_checkint = conf->max_checkint; + put_multipath_config(conf); + + if (!pp->mpp && pp->initialized == INIT_MISSING_UDEV) { + if (pp->retriggers < retrigger_tries) { + static const char change[] = "change"; + ssize_t ret; + + condlog(2, "%s: triggering change event to reinitialize", + pp->dev); + pp->initialized = INIT_REQUESTED_UDEV; + pp->retriggers++; + ret = sysfs_attr_set_value(pp->udev, "uevent", change, + sizeof(change) - 1); + if (ret != sizeof(change) - 1) + log_sysfs_attr_set_value(1, ret, + "%s: failed to trigger change event", + pp->dev); + return 0; + } else { + condlog(1, "%s: not initialized after %d udev retriggers", + pp->dev, retrigger_tries); + /* + * Make sure that the "add missing path" code path + * below may reinstate the path later, if it ever + * comes up again. + * The WWID needs not be cleared; if it was set, the + * state hadn't been INIT_MISSING_UDEV in the first + * place. + */ + pp->initialized = INIT_FAILED; + return 0; + } + } + + /* + * provision a next check soonest, + * in case we exit abnormally from here + */ + pp->tick = checkint; + + newstate = check_path_state(pp); + + if (!pp->mpp) { + if (!strlen(pp->wwid) && + (pp->initialized == INIT_FAILED || + pp->initialized == INIT_NEW) && + (newstate == PATH_UP || newstate == PATH_GHOST)) { + condlog(2, "%s: add missing path", pp->dev); + conf = get_multipath_config(); + pthread_cleanup_push(put_multipath_config, conf); + ret = pathinfo(pp, conf, DI_ALL | DI_BLACKLIST); + pthread_cleanup_pop(1); + /* INIT_OK implies ret == PATHINFO_OK */ + if (pp->initialized == INIT_OK) { + ev_add_path(pp, vecs, 1); + pp->tick = 1; + } else { + if (ret == PATHINFO_SKIPPED) + return -1; + /* + * We failed multiple times to initialize this + * path properly. Don't re-check too often. + */ + pp->checkint = max_checkint; + } + } + return 0; + } + return 0; +} + enum checker_state { CHECKER_STARTING, CHECKER_RUNNING, @@ -2751,7 +2786,11 @@ checkerloop (void *ap) if (pp->is_checked) continue; pp->is_checked = true; - rc = check_path(vecs, pp, ticks); + if (pp->mpp) + rc = check_path(vecs, pp, ticks); + else + rc = handle_uninitialized_path(vecs, pp, + ticks); if (rc < 0) { condlog(1, "%s: check_path() failed, removing", pp->dev);
Pull the code to handle uninitialized paths out of check_path() and into a new function called handle_uninitialized_paths(). This should cause no functional changes. Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com> --- multipathd/main.c | 171 ++++++++++++++++++++++++++++------------------ 1 file changed, 105 insertions(+), 66 deletions(-)