| Message ID | 20240717181106.2173527-18-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 Wed, 2024-07-17 at 14:11 -0400, Benjamin Marzinski wrote: > multipathd's path checking can be very bursty, with all the paths > running their path checkers at the same time, and then all doing > nothing > while waiting for the next check time. Alternatively, the paths in a > multipath device might all run their path checkers at a different > time, > which can keep the multipath device from having a coherent view of > the > state of all of its paths. > > This patch makes all the paths of a multipath device converge to > running > their checkers at the same time, and spreads out when this time is > for > the different multipath devices. > > To do this, the checking time is divided into adjustment intervals > (conf->adjust_int), so that the checkers run at some index within > this > interval. conf->adjust_int is chosen so that it is a multiple of all > possible pp->checkint values. This means that regardless of > pp->checkint, the path should always be checked on the same indexes, > each adjustement interval. > > Each multipath device has a goal index. These are evenly spread out > between 0 and conf->max_checkint. Every conf->adjust_int seconds, > each > multipath device should try to check all of its paths on its goal > index. > If the multipath device's check times are not aligned with the goal > index, then pp->tick for the next check will be decremented by one, > to > align it over time. > > In order for the path checkers to run every pp->checkint seconds, > multipathd needs to track how long a path check has been pending for, > and subtract that time from the number of ticks till the checker is > run > again. If the checker has been pending for more that pp->checkint, > the path will be rechecked on the next tick after the checker > returns. > > Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com> > --- > libmultipath/config.c | 12 ++++++ > libmultipath/config.h | 1 + > libmultipath/structs.c | 1 + > libmultipath/structs.h | 1 + > multipathd/main.c | 91 ++++++++++++++++++++++++++++++++++------ > -- > 5 files changed, 89 insertions(+), 17 deletions(-) > > diff --git a/libmultipath/config.c b/libmultipath/config.c > index 83fa7369..a59533b5 100644 > --- a/libmultipath/config.c > +++ b/libmultipath/config.c > @@ -982,6 +982,18 @@ int _init_config (const char *file, struct > config *conf) > conf->checkint = conf->max_checkint; > condlog(3, "polling interval: %d, max: %d", > conf->checkint, conf->max_checkint); > + /* > + * make sure that that adjust_int is a multiple of all > possible values > + * of pp->checkint. > + */ > + if (conf->max_checkint % conf->checkint == 0) { > + conf->adjust_int = conf->max_checkint; > + } else { > + conf->adjust_int = conf->checkint; > + while (2 * conf->adjust_int < conf->max_checkint) > + conf->adjust_int *= 2; > + conf->adjust_int *= conf->max_checkint; > + } > > if (conf->blist_devnode == NULL) { > conf->blist_devnode = vector_alloc(); > diff --git a/libmultipath/config.h b/libmultipath/config.h > index 384193ab..800c0ca9 100644 > --- a/libmultipath/config.h > +++ b/libmultipath/config.h > @@ -147,6 +147,7 @@ struct config { > int minio_rq; > unsigned int checkint; > unsigned int max_checkint; > + unsigned int adjust_int; > bool use_watchdog; > int pgfailback; > int rr_weight; > diff --git a/libmultipath/structs.c b/libmultipath/structs.c > index 0a26096a..232b4230 100644 > --- a/libmultipath/structs.c > +++ b/libmultipath/structs.c > @@ -149,6 +149,7 @@ uninitialize_path(struct path *pp) > pp->state = PATH_UNCHECKED; > pp->uid_attribute = NULL; > pp->checker_timeout = 0; > + pp->pending_ticks = 0; > > if (checker_selected(&pp->checker)) > checker_put(&pp->checker); > diff --git a/libmultipath/structs.h b/libmultipath/structs.h > index 002eeae1..457d7836 100644 > --- a/libmultipath/structs.h > +++ b/libmultipath/structs.h > @@ -360,6 +360,7 @@ struct path { > unsigned long long size; > unsigned int checkint; > unsigned int tick; > + unsigned int pending_ticks; > int bus; > int offline; > int state; > diff --git a/multipathd/main.c b/multipathd/main.c > index 84450906..87ddb101 100644 > --- a/multipathd/main.c > +++ b/multipathd/main.c > @@ -2388,7 +2388,7 @@ enum check_path_return { > }; > > static int > -check_path (struct vectors * vecs, struct path * pp, unsigned int > ticks) > +do_check_path (struct vectors * vecs, struct path * pp) > { > int newstate; > int new_path_up = 0; > @@ -2400,14 +2400,6 @@ check_path (struct vectors * vecs, struct path > * pp, unsigned int ticks) > int marginal_pathgroups, marginal_changed = 0; > bool need_reload; > > - if (pp->initialized == INIT_REMOVED) > - return CHECK_PATH_SKIPPED; > - > - if (pp->tick) > - pp->tick -= (pp->tick > ticks) ? ticks : pp->tick; > - if (pp->tick) > - return CHECK_PATH_SKIPPED; > - > conf = get_multipath_config(); > checkint = conf->checkint; > max_checkint = conf->max_checkint; > @@ -2419,12 +2411,6 @@ check_path (struct vectors * vecs, struct path > * pp, unsigned int ticks) > pp->checkint = checkint; > }; > > - /* > - * provision a next check soonest, > - * in case we exit abnormally from here > - */ > - pp->tick = checkint; > - > newstate = check_path_state(pp); > if (newstate == PATH_WILD || newstate == PATH_UNCHECKED) > return CHECK_PATH_SKIPPED; > @@ -2587,7 +2573,6 @@ check_path (struct vectors * vecs, struct path > * pp, unsigned int ticks) > condlog(4, "%s: delay next check > %is", > pp->dev_t, pp->checkint); > } > - pp->tick = pp->checkint; > } > } > else if (newstate != PATH_UP && newstate != PATH_GHOST) { > @@ -2640,6 +2625,77 @@ check_path (struct vectors * vecs, struct path > * pp, unsigned int ticks) > return CHECK_PATH_CHECKED; > } > > +static int > +check_path (struct vectors * vecs, struct path * pp, unsigned int > ticks, > + time_t start_secs) > +{ > + int r; > + unsigned int adjust_int, max_checkint; > + struct config *conf; > + time_t next_idx, goal_idx; > + > + if (pp->initialized == INIT_REMOVED) > + return CHECK_PATH_SKIPPED; > + > + if (pp->tick) > + pp->tick -= (pp->tick > ticks) ? ticks : pp->tick; > + if (pp->tick) > + return CHECK_PATH_SKIPPED; > + > + conf = get_multipath_config(); > + max_checkint = conf->max_checkint; > + adjust_int = conf->adjust_int; > + put_multipath_config(conf); > + > + r = do_check_path(vecs, pp); > + > + /* > + * do_check_path() removed or orphaned the path. > + */ > + if (r < 0 || !pp->mpp) > + return r; do_check_path() returns an enum now, so this should be r == CHECK_PATH_REMOVED.
On Thu, Jul 18, 2024 at 10:24:13PM +0200, Martin Wilck wrote: > On Wed, 2024-07-17 at 14:11 -0400, Benjamin Marzinski wrote: > > multipathd's path checking can be very bursty, with all the paths > > running their path checkers at the same time, and then all doing > > nothing > > while waiting for the next check time. Alternatively, the paths in a > > multipath device might all run their path checkers at a different > > time, > > which can keep the multipath device from having a coherent view of > > the > > state of all of its paths. > > > > This patch makes all the paths of a multipath device converge to > > running > > their checkers at the same time, and spreads out when this time is > > for > > the different multipath devices. > > > > To do this, the checking time is divided into adjustment intervals > > (conf->adjust_int), so that the checkers run at some index within > > this > > interval. conf->adjust_int is chosen so that it is a multiple of all > > possible pp->checkint values. This means that regardless of > > pp->checkint, the path should always be checked on the same indexes, > > each adjustement interval. > > > > Each multipath device has a goal index. These are evenly spread out > > between 0 and conf->max_checkint. Every conf->adjust_int seconds, > > each > > multipath device should try to check all of its paths on its goal > > index. > > If the multipath device's check times are not aligned with the goal > > index, then pp->tick for the next check will be decremented by one, > > to > > align it over time. > > > > In order for the path checkers to run every pp->checkint seconds, > > multipathd needs to track how long a path check has been pending for, > > and subtract that time from the number of ticks till the checker is > > run > > again. If the checker has been pending for more that pp->checkint, > > the path will be rechecked on the next tick after the checker > > returns. > > > > Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com> > > --- > > libmultipath/config.c | 12 ++++++ > > libmultipath/config.h | 1 + > > libmultipath/structs.c | 1 + > > libmultipath/structs.h | 1 + > > multipathd/main.c | 91 ++++++++++++++++++++++++++++++++++------ > > -- > > 5 files changed, 89 insertions(+), 17 deletions(-) > > > > diff --git a/libmultipath/config.c b/libmultipath/config.c > > index 83fa7369..a59533b5 100644 > > --- a/libmultipath/config.c > > +++ b/libmultipath/config.c > > @@ -982,6 +982,18 @@ int _init_config (const char *file, struct > > config *conf) > > conf->checkint = conf->max_checkint; > > condlog(3, "polling interval: %d, max: %d", > > conf->checkint, conf->max_checkint); > > + /* > > + * make sure that that adjust_int is a multiple of all > > possible values > > + * of pp->checkint. > > + */ > > + if (conf->max_checkint % conf->checkint == 0) { > > + conf->adjust_int = conf->max_checkint; > > + } else { > > + conf->adjust_int = conf->checkint; > > + while (2 * conf->adjust_int < conf->max_checkint) > > + conf->adjust_int *= 2; > > + conf->adjust_int *= conf->max_checkint; > > + } > > > > if (conf->blist_devnode == NULL) { > > conf->blist_devnode = vector_alloc(); > > diff --git a/libmultipath/config.h b/libmultipath/config.h > > index 384193ab..800c0ca9 100644 > > --- a/libmultipath/config.h > > +++ b/libmultipath/config.h > > @@ -147,6 +147,7 @@ struct config { > > int minio_rq; > > unsigned int checkint; > > unsigned int max_checkint; > > + unsigned int adjust_int; > > bool use_watchdog; > > int pgfailback; > > int rr_weight; > > diff --git a/libmultipath/structs.c b/libmultipath/structs.c > > index 0a26096a..232b4230 100644 > > --- a/libmultipath/structs.c > > +++ b/libmultipath/structs.c > > @@ -149,6 +149,7 @@ uninitialize_path(struct path *pp) > > pp->state = PATH_UNCHECKED; > > pp->uid_attribute = NULL; > > pp->checker_timeout = 0; > > + pp->pending_ticks = 0; > > > > if (checker_selected(&pp->checker)) > > checker_put(&pp->checker); > > diff --git a/libmultipath/structs.h b/libmultipath/structs.h > > index 002eeae1..457d7836 100644 > > --- a/libmultipath/structs.h > > +++ b/libmultipath/structs.h > > @@ -360,6 +360,7 @@ struct path { > > unsigned long long size; > > unsigned int checkint; > > unsigned int tick; > > + unsigned int pending_ticks; > > int bus; > > int offline; > > int state; > > diff --git a/multipathd/main.c b/multipathd/main.c > > index 84450906..87ddb101 100644 > > --- a/multipathd/main.c > > +++ b/multipathd/main.c > > @@ -2388,7 +2388,7 @@ enum check_path_return { > > }; > > > > static int > > -check_path (struct vectors * vecs, struct path * pp, unsigned int > > ticks) > > +do_check_path (struct vectors * vecs, struct path * pp) > > { > > int newstate; > > int new_path_up = 0; > > @@ -2400,14 +2400,6 @@ check_path (struct vectors * vecs, struct path > > * pp, unsigned int ticks) > > int marginal_pathgroups, marginal_changed = 0; > > bool need_reload; > > > > - if (pp->initialized == INIT_REMOVED) > > - return CHECK_PATH_SKIPPED; > > - > > - if (pp->tick) > > - pp->tick -= (pp->tick > ticks) ? ticks : pp->tick; > > - if (pp->tick) > > - return CHECK_PATH_SKIPPED; > > - > > conf = get_multipath_config(); > > checkint = conf->checkint; > > max_checkint = conf->max_checkint; > > @@ -2419,12 +2411,6 @@ check_path (struct vectors * vecs, struct path > > * pp, unsigned int ticks) > > pp->checkint = checkint; > > }; > > > > - /* > > - * provision a next check soonest, > > - * in case we exit abnormally from here > > - */ > > - pp->tick = checkint; > > - > > newstate = check_path_state(pp); > > if (newstate == PATH_WILD || newstate == PATH_UNCHECKED) > > return CHECK_PATH_SKIPPED; > > @@ -2587,7 +2573,6 @@ check_path (struct vectors * vecs, struct path > > * pp, unsigned int ticks) > > condlog(4, "%s: delay next check > > %is", > > pp->dev_t, pp->checkint); > > } > > - pp->tick = pp->checkint; > > } > > } > > else if (newstate != PATH_UP && newstate != PATH_GHOST) { > > @@ -2640,6 +2625,77 @@ check_path (struct vectors * vecs, struct path > > * pp, unsigned int ticks) > > return CHECK_PATH_CHECKED; > > } > > > > +static int > > +check_path (struct vectors * vecs, struct path * pp, unsigned int > > ticks, > > + time_t start_secs) > > +{ > > + int r; > > + unsigned int adjust_int, max_checkint; > > + struct config *conf; > > + time_t next_idx, goal_idx; > > + > > + if (pp->initialized == INIT_REMOVED) > > + return CHECK_PATH_SKIPPED; > > + > > + if (pp->tick) > > + pp->tick -= (pp->tick > ticks) ? ticks : pp->tick; > > + if (pp->tick) > > + return CHECK_PATH_SKIPPED; > > + > > + conf = get_multipath_config(); > > + max_checkint = conf->max_checkint; > > + adjust_int = conf->adjust_int; > > + put_multipath_config(conf); > > + > > + r = do_check_path(vecs, pp); > > + > > + /* > > + * do_check_path() removed or orphaned the path. > > + */ > > + if (r < 0 || !pp->mpp) > > + return r; > > do_check_path() returns an enum now, so this should be r == > CHECK_PATH_REMOVED. Oops. Good catch. -Ben >
diff --git a/libmultipath/config.c b/libmultipath/config.c index 83fa7369..a59533b5 100644 --- a/libmultipath/config.c +++ b/libmultipath/config.c @@ -982,6 +982,18 @@ int _init_config (const char *file, struct config *conf) conf->checkint = conf->max_checkint; condlog(3, "polling interval: %d, max: %d", conf->checkint, conf->max_checkint); + /* + * make sure that that adjust_int is a multiple of all possible values + * of pp->checkint. + */ + if (conf->max_checkint % conf->checkint == 0) { + conf->adjust_int = conf->max_checkint; + } else { + conf->adjust_int = conf->checkint; + while (2 * conf->adjust_int < conf->max_checkint) + conf->adjust_int *= 2; + conf->adjust_int *= conf->max_checkint; + } if (conf->blist_devnode == NULL) { conf->blist_devnode = vector_alloc(); diff --git a/libmultipath/config.h b/libmultipath/config.h index 384193ab..800c0ca9 100644 --- a/libmultipath/config.h +++ b/libmultipath/config.h @@ -147,6 +147,7 @@ struct config { int minio_rq; unsigned int checkint; unsigned int max_checkint; + unsigned int adjust_int; bool use_watchdog; int pgfailback; int rr_weight; diff --git a/libmultipath/structs.c b/libmultipath/structs.c index 0a26096a..232b4230 100644 --- a/libmultipath/structs.c +++ b/libmultipath/structs.c @@ -149,6 +149,7 @@ uninitialize_path(struct path *pp) pp->state = PATH_UNCHECKED; pp->uid_attribute = NULL; pp->checker_timeout = 0; + pp->pending_ticks = 0; if (checker_selected(&pp->checker)) checker_put(&pp->checker); diff --git a/libmultipath/structs.h b/libmultipath/structs.h index 002eeae1..457d7836 100644 --- a/libmultipath/structs.h +++ b/libmultipath/structs.h @@ -360,6 +360,7 @@ struct path { unsigned long long size; unsigned int checkint; unsigned int tick; + unsigned int pending_ticks; int bus; int offline; int state; diff --git a/multipathd/main.c b/multipathd/main.c index 84450906..87ddb101 100644 --- a/multipathd/main.c +++ b/multipathd/main.c @@ -2388,7 +2388,7 @@ enum check_path_return { }; static int -check_path (struct vectors * vecs, struct path * pp, unsigned int ticks) +do_check_path (struct vectors * vecs, struct path * pp) { int newstate; int new_path_up = 0; @@ -2400,14 +2400,6 @@ check_path (struct vectors * vecs, struct path * pp, unsigned int ticks) int marginal_pathgroups, marginal_changed = 0; bool need_reload; - if (pp->initialized == INIT_REMOVED) - return CHECK_PATH_SKIPPED; - - if (pp->tick) - pp->tick -= (pp->tick > ticks) ? ticks : pp->tick; - if (pp->tick) - return CHECK_PATH_SKIPPED; - conf = get_multipath_config(); checkint = conf->checkint; max_checkint = conf->max_checkint; @@ -2419,12 +2411,6 @@ check_path (struct vectors * vecs, struct path * pp, unsigned int ticks) pp->checkint = checkint; }; - /* - * provision a next check soonest, - * in case we exit abnormally from here - */ - pp->tick = checkint; - newstate = check_path_state(pp); if (newstate == PATH_WILD || newstate == PATH_UNCHECKED) return CHECK_PATH_SKIPPED; @@ -2587,7 +2573,6 @@ check_path (struct vectors * vecs, struct path * pp, unsigned int ticks) condlog(4, "%s: delay next check %is", pp->dev_t, pp->checkint); } - pp->tick = pp->checkint; } } else if (newstate != PATH_UP && newstate != PATH_GHOST) { @@ -2640,6 +2625,77 @@ check_path (struct vectors * vecs, struct path * pp, unsigned int ticks) return CHECK_PATH_CHECKED; } +static int +check_path (struct vectors * vecs, struct path * pp, unsigned int ticks, + time_t start_secs) +{ + int r; + unsigned int adjust_int, max_checkint; + struct config *conf; + time_t next_idx, goal_idx; + + if (pp->initialized == INIT_REMOVED) + return CHECK_PATH_SKIPPED; + + if (pp->tick) + pp->tick -= (pp->tick > ticks) ? ticks : pp->tick; + if (pp->tick) + return CHECK_PATH_SKIPPED; + + conf = get_multipath_config(); + max_checkint = conf->max_checkint; + adjust_int = conf->adjust_int; + put_multipath_config(conf); + + r = do_check_path(vecs, pp); + + /* + * do_check_path() removed or orphaned the path. + */ + if (r < 0 || !pp->mpp) + return r; + + if (pp->tick != 0) { + /* the path checker is pending */ + if (pp->state != PATH_DELAYED) + pp->pending_ticks++; + else + pp->pending_ticks = 0; + return r; + } + + /* schedule the next check */ + pp->tick = pp->checkint; + if (pp->pending_ticks >= pp->tick) + pp->tick = 1; + else + pp->tick -= pp->pending_ticks; + pp->pending_ticks = 0; + + if (pp->tick == 1) + return r; + + /* + * every mpp has a goal_idx in the range of + * 0 <= goal_idx < conf->max_checkint + * + * The next check has an index, next_idx, in the range of + * 0 <= next_idx < conf->adjust_int + * + * If the difference between the goal index and the next check index + * is not a multiple of pp->checkint, then the device is not checking + * the paths at its goal index, and pp->tick will be decremented by + * one, to align it over time. + */ + goal_idx = (find_slot(vecs->mpvec, pp->mpp)) * + max_checkint / VECTOR_SIZE(vecs->mpvec); + next_idx = (start_secs + pp->tick) % adjust_int; + if ((goal_idx - next_idx) % pp->checkint != 0) + pp->tick--; + + return r; +} + static int handle_uninitialized_path(struct vectors * vecs, struct path * pp, unsigned int ticks) @@ -2799,7 +2855,8 @@ checkerloop (void *ap) continue; pp->is_checked = true; if (pp->mpp) - rc = check_path(vecs, pp, ticks); + rc = check_path(vecs, pp, ticks, + chk_start_time.tv_sec); else rc = handle_uninitialized_path(vecs, pp, ticks);
multipathd's path checking can be very bursty, with all the paths running their path checkers at the same time, and then all doing nothing while waiting for the next check time. Alternatively, the paths in a multipath device might all run their path checkers at a different time, which can keep the multipath device from having a coherent view of the state of all of its paths. This patch makes all the paths of a multipath device converge to running their checkers at the same time, and spreads out when this time is for the different multipath devices. To do this, the checking time is divided into adjustment intervals (conf->adjust_int), so that the checkers run at some index within this interval. conf->adjust_int is chosen so that it is a multiple of all possible pp->checkint values. This means that regardless of pp->checkint, the path should always be checked on the same indexes, each adjustement interval. Each multipath device has a goal index. These are evenly spread out between 0 and conf->max_checkint. Every conf->adjust_int seconds, each multipath device should try to check all of its paths on its goal index. If the multipath device's check times are not aligned with the goal index, then pp->tick for the next check will be decremented by one, to align it over time. In order for the path checkers to run every pp->checkint seconds, multipathd needs to track how long a path check has been pending for, and subtract that time from the number of ticks till the checker is run again. If the checker has been pending for more that pp->checkint, the path will be rechecked on the next tick after the checker returns. Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com> --- libmultipath/config.c | 12 ++++++ libmultipath/config.h | 1 + libmultipath/structs.c | 1 + libmultipath/structs.h | 1 + multipathd/main.c | 91 ++++++++++++++++++++++++++++++++++-------- 5 files changed, 89 insertions(+), 17 deletions(-)