| Message ID | 20240828221757.4060548-11-bmarzins@redhat.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | Benjamin Marzinski |
| Headers | show |
| Series | Yet Another path checker refactor | expand |
On Wed, 2024-08-28 at 18:17 -0400, Benjamin Marzinski wrote: > Split out the code that updates a path's state and sets up the next > check time into its own function, update_path(). > > Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com> > --- > multipathd/main.c | 31 ++++++++++++++++++++++--------- > 1 file changed, 22 insertions(+), 9 deletions(-) > > diff --git a/multipathd/main.c b/multipathd/main.c > index 94d4e421..300f8247 100644 > --- a/multipathd/main.c > +++ b/multipathd/main.c > @@ -2390,6 +2390,7 @@ sync_mpp(struct vectors * vecs, struct > multipath *mpp, unsigned int ticks) > } > > enum check_path_return { > + CHECK_PATH_STARTED, > CHECK_PATH_CHECKED, > CHECK_PATH_SKIPPED, > CHECK_PATH_REMOVED, > @@ -2629,13 +2630,10 @@ update_path_state (struct vectors * vecs, > struct path * pp) > } > > static int > -check_path (struct vectors * vecs, struct path * pp, unsigned int > ticks, > - time_t start_secs) > +check_path (struct path * pp, unsigned int ticks) check_path() used to be one of our core functions, and you now re- introduce it with quite different semantics. Perhaps choose a new name? Martin
On Wed, Sep 04, 2024 at 05:38:52PM +0200, Martin Wilck wrote: > On Wed, 2024-08-28 at 18:17 -0400, Benjamin Marzinski wrote: > > Split out the code that updates a path's state and sets up the next > > check time into its own function, update_path(). > > > > Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com> > > --- > > multipathd/main.c | 31 ++++++++++++++++++++++--------- > > 1 file changed, 22 insertions(+), 9 deletions(-) > > > > diff --git a/multipathd/main.c b/multipathd/main.c > > index 94d4e421..300f8247 100644 > > --- a/multipathd/main.c > > +++ b/multipathd/main.c > > @@ -2390,6 +2390,7 @@ sync_mpp(struct vectors * vecs, struct > > multipath *mpp, unsigned int ticks) > > } > > > > enum check_path_return { > > + CHECK_PATH_STARTED, > > CHECK_PATH_CHECKED, > > CHECK_PATH_SKIPPED, > > CHECK_PATH_REMOVED, > > @@ -2629,13 +2630,10 @@ update_path_state (struct vectors * vecs, > > struct path * pp) > > } > > > > static int > > -check_path (struct vectors * vecs, struct path * pp, unsigned int > > ticks, > > - time_t start_secs) > > +check_path (struct path * pp, unsigned int ticks) > > check_path() used to be one of our core functions, and you now re- > introduce it with quite different semantics. > > Perhaps choose a new name? Sure. Although the new check_path() is just the beginning part of the old check_path(), where we actually run the checker, so it seems reasonable to me. But your objection is also reasonable. I was just getting sick of coming up with new function names by this point. -Ben > > Martin
On Wed, Sep 04, 2024 at 02:51:19PM -0400, Benjamin Marzinski wrote: > On Wed, Sep 04, 2024 at 05:38:52PM +0200, Martin Wilck wrote: > > On Wed, 2024-08-28 at 18:17 -0400, Benjamin Marzinski wrote: > > > Split out the code that updates a path's state and sets up the next > > > check time into its own function, update_path(). > > > > > > Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com> > > > --- > > > multipathd/main.c | 31 ++++++++++++++++++++++--------- > > > 1 file changed, 22 insertions(+), 9 deletions(-) > > > > > > diff --git a/multipathd/main.c b/multipathd/main.c > > > index 94d4e421..300f8247 100644 > > > --- a/multipathd/main.c > > > +++ b/multipathd/main.c > > > @@ -2390,6 +2390,7 @@ sync_mpp(struct vectors * vecs, struct > > > multipath *mpp, unsigned int ticks) > > > } > > > > > > enum check_path_return { > > > + CHECK_PATH_STARTED, > > > CHECK_PATH_CHECKED, > > > CHECK_PATH_SKIPPED, > > > CHECK_PATH_REMOVED, > > > @@ -2629,13 +2630,10 @@ update_path_state (struct vectors * vecs, > > > struct path * pp) > > > } > > > > > > static int > > > -check_path (struct vectors * vecs, struct path * pp, unsigned int > > > ticks, > > > - time_t start_secs) > > > +check_path (struct path * pp, unsigned int ticks) > > > > check_path() used to be one of our core functions, and you now re- > > introduce it with quite different semantics. > > > > Perhaps choose a new name? > > Sure. Although the new check_path() is just the beginning part of the > old check_path(), where we actually run the checker, so it seems > reasonable to me. But your objection is also reasonable. I was just > getting sick of coming up with new function names by this point. > > -Ben Do you have ideas for the name, because I can't think of anything that makes more sense then check_path() in the code paths. Here's the code paths checkerloop (initialized paths) ----------------------------------------- check_paths update_paths check_path update_path start_path_check update_path_state start_checker check_path_state checker_check get_state checker_get_state checkerloop (uninitialized paths) ----------------------------------------- check_paths update_paths check_uninitialized_path update_uninitialized_path start_path_check check_path_state start_checker get_state checker_check checker_get_state pathinfo ---------------- start_checker get_state checker_check checker_get_state The only function that stands out to me as misnamed is "check_path_state", which I think I'm going to change to "get_updated_state", since that's a better description of what it's doing, and it avoids using "check" in the update code path. Also, like I said before, if you're looking for the function that gets run to see if a path is due for checking, and runs the path checker if it is, that's check_path(), just like it used to be. It just no longer updates the path and mpp state based on the checker result like it used to. That's now done by update_path() and update_mpp_prio(). > > > > > Martin >
On Thu, 2024-09-05 at 15:02 -0400, Benjamin Marzinski wrote: > On Wed, Sep 04, 2024 at 02:51:19PM -0400, Benjamin Marzinski wrote: > > On Wed, Sep 04, 2024 at 05:38:52PM +0200, Martin Wilck wrote: > > > On Wed, 2024-08-28 at 18:17 -0400, Benjamin Marzinski wrote: > > > > Split out the code that updates a path's state and sets up the > > > > next > > > > check time into its own function, update_path(). > > > > > > > > Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com> > > > > --- > > > > multipathd/main.c | 31 ++++++++++++++++++++++--------- > > > > 1 file changed, 22 insertions(+), 9 deletions(-) > > > > > > > > diff --git a/multipathd/main.c b/multipathd/main.c > > > > index 94d4e421..300f8247 100644 > > > > --- a/multipathd/main.c > > > > +++ b/multipathd/main.c > > > > @@ -2390,6 +2390,7 @@ sync_mpp(struct vectors * vecs, struct > > > > multipath *mpp, unsigned int ticks) > > > > } > > > > > > > > enum check_path_return { > > > > + CHECK_PATH_STARTED, > > > > CHECK_PATH_CHECKED, > > > > CHECK_PATH_SKIPPED, > > > > CHECK_PATH_REMOVED, > > > > @@ -2629,13 +2630,10 @@ update_path_state (struct vectors * > > > > vecs, > > > > struct path * pp) > > > > } > > > > > > > > static int > > > > -check_path (struct vectors * vecs, struct path * pp, unsigned > > > > int > > > > ticks, > > > > - time_t start_secs) > > > > +check_path (struct path * pp, unsigned int ticks) > > > > > > check_path() used to be one of our core functions, and you now > > > re- > > > introduce it with quite different semantics. > > > > > > Perhaps choose a new name? > > > > Sure. Although the new check_path() is just the beginning part of > > the > > old check_path(), where we actually run the checker, so it seems > > reasonable to me. But your objection is also reasonable. I was just > > getting sick of coming up with new function names by this point. > > > > -Ben > > Do you have ideas for the name, because I can't think of anything > that > makes more sense then check_path() in the code paths. Here's the code > paths Ok then, keep the name. I was hoping we could find something better because (as you correctly show) there's already quite some confusion because of multiple functions with very similar names. But if we can't, so be it. Martin
diff --git a/multipathd/main.c b/multipathd/main.c index 94d4e421..300f8247 100644 --- a/multipathd/main.c +++ b/multipathd/main.c @@ -2390,6 +2390,7 @@ sync_mpp(struct vectors * vecs, struct multipath *mpp, unsigned int ticks) } enum check_path_return { + CHECK_PATH_STARTED, CHECK_PATH_CHECKED, CHECK_PATH_SKIPPED, CHECK_PATH_REMOVED, @@ -2629,13 +2630,10 @@ update_path_state (struct vectors * vecs, struct path * pp) } static int -check_path (struct vectors * vecs, struct path * pp, unsigned int ticks, - time_t start_secs) +check_path (struct path * pp, unsigned int ticks) { - int r; - unsigned int adjust_int, checkint, max_checkint; + unsigned int checkint; struct config *conf; - time_t next_idx, goal_idx; if (pp->initialized == INIT_REMOVED) return CHECK_PATH_SKIPPED; @@ -2647,8 +2645,6 @@ check_path (struct vectors * vecs, struct path * pp, unsigned int ticks, conf = get_multipath_config(); checkint = conf->checkint; - max_checkint = conf->max_checkint; - adjust_int = conf->adjust_int; put_multipath_config(conf); if (pp->checkint == CHECKINT_UNDEF) { @@ -2657,6 +2653,17 @@ check_path (struct vectors * vecs, struct path * pp, unsigned int ticks, } start_path_check(pp); + return CHECK_PATH_STARTED; +} + +static int +update_path(struct vectors * vecs, struct path * pp, time_t start_secs) +{ + int r; + unsigned int adjust_int, max_checkint; + struct config *conf; + time_t next_idx, goal_idx; + r = update_path_state(vecs, pp); /* @@ -2685,6 +2692,10 @@ check_path (struct vectors * vecs, struct path * pp, unsigned int ticks, if (pp->tick == 1) return r; + conf = get_multipath_config(); + max_checkint = conf->max_checkint; + adjust_int = conf->adjust_int; + put_multipath_config(conf); /* * every mpp has a goal_idx in the range of * 0 <= goal_idx < conf->max_checkint @@ -2818,8 +2829,10 @@ check_paths(struct vectors *vecs, unsigned int ticks, int *num_paths_p) if (!pp->mpp || pp->is_checked) continue; pp->is_checked = true; - rc = check_path(vecs, pp, ticks, - start_time.tv_sec); + rc = check_path(pp, ticks); + if (rc == CHECK_PATH_STARTED) + rc = update_path(vecs, pp, + start_time.tv_sec); if (rc == CHECK_PATH_CHECKED) (*num_paths_p)++; if (++paths_checked % 128 == 0)
Split out the code that updates a path's state and sets up the next check time into its own function, update_path(). Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com> --- multipathd/main.c | 31 ++++++++++++++++++++++--------- 1 file changed, 22 insertions(+), 9 deletions(-)