| Message ID | 20240912214947.783819-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 Thu, 2024-09-12 at 17:49 -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 9319751e..8bfb166d 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); > After this change, we set only checking in this RCU critical section, and checkint is only used in the if clause below. Perhaps move the critical section into the if block? Martin
On Thu, Oct 03, 2024 at 10:23:57PM +0200, Martin Wilck wrote: > On Thu, 2024-09-12 at 17:49 -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 9319751e..8bfb166d 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); > > > > After this change, we set only checking in this RCU critical section, > and checkint is only used in the if clause below. Perhaps move the > critical section into the if block? Makes sense. Sure. -Ben > Martin >
diff --git a/multipathd/main.c b/multipathd/main.c index 9319751e..8bfb166d 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(-)