On Fri, Sep 10, 2021 at 01:40:59PM +0200, mwilck@xxxxxxxx wrote:
> From: Martin Wilck <mwilck@xxxxxxxx>
>
> Since e3270f7 ("multipathd: use weaker "force_reload" at startup"),
> (multipath-tools 0.7.0), we only reload those maps that must be
> reloaded during startup. "multipath reconfigure", OTOH, reloads
> every map, which may take a long time on systems with lots of
> storage devices, as every DM_DEVICE_RELOAD ioctl involves a
> suspend/resume cycle.
>
> The logic we use during startup is actually very robust and catches
> all cases in which a reload is necessary. "reconfigure" operations
> are often done because of configuration changes, and usually don't
> require a full reload of every map.
>
> This patch changes the default behavior of "multipath reconfigure"
> to "weak" reload, like we do on startup since e3270f7. The behavior
> can be changed by setting the configuration option
> "force_reconfigure yes" before starting the reconfigure operation.
> "multipath -r" is also affected, but "multipath -D -r" is not.
>
> It would have been nice to have introduced a new cli command
> "reconfigure force" instead, but the way "reconfigure" is
> implemented, that would have required a major rewrite of the code.
This looks o.k. But I don't think it would be that hard to add a new
multipathd command to reconfigure all the devices. My personal
preference would be to leave force_reconfigure off by default, so that
we keep the same behavior, and add a command to force a full reconfig.
I'll try to work up a patch with my idea that can apply on top of this.
But the code itself looks fine, and if we don't agree on my patch, I can
always just change the default for the RHEL version, at least until the
next major release, so
Reviewed-by: Benjamin Marzinski <bmarzins@xxxxxxxxxx>
>
> Signed-off-by: Martin Wilck <mwilck@xxxxxxxx>
> ---
> libmultipath/config.c | 1 +
> libmultipath/config.h | 1 +
> libmultipath/configure.c | 19 ++++++++++++++++++-
> libmultipath/defaults.h | 1 +
> libmultipath/dict.c | 4 ++++
> multipath/multipath.8 | 6 ++++--
> multipath/multipath.conf.5 | 17 +++++++++++++++++
> multipathd/main.c | 18 +++++-------------
> multipathd/multipathd.8 | 6 ++++--
> 9 files changed, 55 insertions(+), 18 deletions(-)
>
> diff --git a/libmultipath/config.c b/libmultipath/config.c
> index 30046a1..a1ef4c3 100644
> --- a/libmultipath/config.c
> +++ b/libmultipath/config.c
> @@ -869,6 +869,7 @@ int _init_config (const char *file, struct config *conf)
> conf->ghost_delay = DEFAULT_GHOST_DELAY;
> conf->all_tg_pt = DEFAULT_ALL_TG_PT;
> conf->recheck_wwid = DEFAULT_RECHECK_WWID;
> + conf->force_reconfigure = DEFAULT_FORCE_RECONFIGURE;
> /*
> * preload default hwtable
> */
> diff --git a/libmultipath/config.h b/libmultipath/config.h
> index 933fe0d..4617177 100644
> --- a/libmultipath/config.h
> +++ b/libmultipath/config.h
> @@ -189,6 +189,7 @@ struct config {
> int skip_delegate;
> unsigned int sequence_nr;
> int recheck_wwid;
> + int force_reconfigure;
>
> char * multipath_dir;
> char * selector;
> diff --git a/libmultipath/configure.c b/libmultipath/configure.c
> index 7edb355..262657e 100644
> --- a/libmultipath/configure.c
> +++ b/libmultipath/configure.c
> @@ -1093,12 +1093,27 @@ out:
> return ret;
> }
>
> +static const char* reconfigure_str(int force)
> +{
> + switch(force) {
> + case FORCE_RELOAD_NONE:
> + return "new";
> + case FORCE_RELOAD_WEAK:
> + return "changed";
> + case FORCE_RELOAD_YES:
> + return "all";
> + default:
> + return "<undefined>";
> + }
> +}
> +
> /*
> * The force_reload parameter determines how coalesce_paths treats existing maps.
> * FORCE_RELOAD_NONE: existing maps aren't touched at all
> * FORCE_RELOAD_YES: all maps are rebuilt from scratch and (re)loaded in DM
> * FORCE_RELOAD_WEAK: existing maps are compared to the current conf and only
> - * reloaded in DM if there's a difference. This is useful during startup.
> + * reloaded in DM if there's a difference. This is normally sufficient.
> + * This is controlled by the "force_reconfigure" config option.
> */
> int coalesce_paths (struct vectors *vecs, vector mpvec, char *refwwid,
> int force_reload, enum mpath_cmds cmd)
> @@ -1117,6 +1132,8 @@ int coalesce_paths (struct vectors *vecs, vector mpvec, char *refwwid,
> int allow_queueing;
> struct bitfield *size_mismatch_seen;
>
> + condlog(2, "%s: reloading %s maps", __func__,
> + reconfigure_str(force_reload));
> /* ignore refwwid if it's empty */
> if (refwwid && !strlen(refwwid))
> refwwid = NULL;
> diff --git a/libmultipath/defaults.h b/libmultipath/defaults.h
> index c27946c..eab08a0 100644
> --- a/libmultipath/defaults.h
> +++ b/libmultipath/defaults.h
> @@ -56,6 +56,7 @@
> #define DEFAULT_RECHECK_WWID RECHECK_WWID_OFF
> /* Enable no foreign libraries by default */
> #define DEFAULT_ENABLE_FOREIGN "NONE"
> +#define DEFAULT_FORCE_RECONFIGURE YN_NO
>
> #define CHECKINT_UNDEF UINT_MAX
> #define DEFAULT_CHECKINT 5
> diff --git a/libmultipath/dict.c b/libmultipath/dict.c
> index 7a72738..fee08cf 100644
> --- a/libmultipath/dict.c
> +++ b/libmultipath/dict.c
> @@ -299,6 +299,9 @@ declare_def_snprint(verbosity, print_int)
> declare_def_handler(reassign_maps, set_yes_no)
> declare_def_snprint(reassign_maps, print_yes_no)
>
> +declare_def_handler(force_reconfigure, set_yes_no)
> +declare_def_snprint(force_reconfigure, print_yes_no)
> +
> declare_def_handler(multipath_dir, set_str)
> declare_def_snprint(multipath_dir, print_str)
>
> @@ -1713,6 +1716,7 @@ init_keywords(vector keywords)
> install_keyword("polling_interval", &checkint_handler, &snprint_def_checkint);
> install_keyword("max_polling_interval", &def_max_checkint_handler, &snprint_def_max_checkint);
> install_keyword("reassign_maps", &def_reassign_maps_handler, &snprint_def_reassign_maps);
> + install_keyword("force_reconfigure", &def_force_reconfigure_handler, &snprint_def_force_reconfigure);
> install_keyword("multipath_dir", &def_multipath_dir_handler, &snprint_def_multipath_dir);
> install_keyword("path_selector", &def_selector_handler, &snprint_def_selector);
> install_keyword("path_grouping_policy", &def_pgpolicy_handler, &snprint_def_pgpolicy);
> diff --git a/multipath/multipath.8 b/multipath/multipath.8
> index 17df59f..b3980c0 100644
> --- a/multipath/multipath.8
> +++ b/multipath/multipath.8
> @@ -264,9 +264,11 @@ Force new maps to use the specified policy, overriding the configuration in
> .
> .TP
> .B \-r
> -Force a reload of all existing multipath maps. This command is delegated to
> +Reload existing multipath maps. This command is delegated to
> the multipathd daemon if it's running. In this case, other command line
> -switches of the \fImultipath\fR command have no effect.
> +switches of the \fImultipath\fR command have no effect, and multipathd
> +executes a \fIreconfigure\fR command. See the \fIforce_reconfigure\fR option
> +in \fBmultipath.conf(5)\fR.
> .
> .TP
> .BI \-R " retries"
> diff --git a/multipath/multipath.conf.5 b/multipath/multipath.conf.5
> index 42a15ff..814de66 100644
> --- a/multipath/multipath.conf.5
> +++ b/multipath/multipath.conf.5
> @@ -34,6 +34,7 @@ or \fBmultipathd show config\fR command.
> .SH SYNTAX
> .\" ----------------------------------------------------------------------------
> .
> +.
> The configuration file contains entries of the form:
> .RS
> .nf
> @@ -165,6 +166,22 @@ The default is: \fB4 * polling_interval\fR
> .
> .
> .TP
> +.B force_reconfigure
> +This controls what happens when \fBmultipathd reconfigure\fR or
> +\fBmultipath -r\fR is executed. If set to \fIyes\fR, all multipath
> +maps will be reloaded, regardless if this is necessary or not, which
> +may take a lot of time on large systems. This used to be the default
> +on previous versions of multipath-tools. If set to \fIno\fR,
> +only those maps will be reloaded for which some parameters
> +have changed that are relevant for the device-mapper configuration of the map.
> +This is the behavior during \fImultipathd\fR startup.
> +.RS
> +.TP
> +The default is: \fBno\fR
> +.RE
> +.
> +.
> +.TP
> .B reassign_maps
> Enable reassigning of device-mapper maps. With this option multipathd
> will remap existing device-mapper maps to always point to multipath
> diff --git a/multipathd/main.c b/multipathd/main.c
> index bda51c9..6d7c8c9 100644
> --- a/multipathd/main.c
> +++ b/multipathd/main.c
> @@ -2594,14 +2594,13 @@ checkerloop (void *ap)
> }
>
> int
> -configure (struct vectors * vecs)
> +configure (struct vectors *vecs, bool force)
> {
> struct multipath * mpp;
> struct path * pp;
> vector mpvec;
> int i, ret;
> struct config *conf;
> - static int force_reload = FORCE_RELOAD_WEAK;
>
> if (!vecs->pathvec && !(vecs->pathvec = vector_alloc())) {
> condlog(0, "couldn't allocate path vec in configure");
> @@ -2649,15 +2648,9 @@ configure (struct vectors * vecs)
> if (should_exit())
> goto fail;
>
> - /*
> - * create new set of maps & push changed ones into dm
> - * In the first call, use FORCE_RELOAD_WEAK to avoid making
> - * superfluous ACT_RELOAD ioctls. Later calls are done
> - * with FORCE_RELOAD_YES.
> - */
> - ret = coalesce_paths(vecs, mpvec, NULL, force_reload, CMD_NONE);
> - if (force_reload == FORCE_RELOAD_WEAK)
> - force_reload = FORCE_RELOAD_YES;
> + ret = coalesce_paths(vecs, mpvec, NULL,
> + force ? FORCE_RELOAD_YES : FORCE_RELOAD_WEAK,
> + CMD_NONE);
> if (ret != CP_OK) {
> condlog(0, "configure failed while coalescing paths");
> goto fail;
> @@ -2769,8 +2762,7 @@ reconfigure (struct vectors * vecs)
> rcu_assign_pointer(multipath_conf, conf);
> call_rcu(&old->rcu, rcu_free_config);
>
> - configure(vecs);
> -
> + configure(vecs, conf->force_reconfigure == YN_YES);
>
> return 0;
> }
> diff --git a/multipathd/multipathd.8 b/multipathd/multipathd.8
> index 048a838..b52a617 100644
> --- a/multipathd/multipathd.8
> +++ b/multipathd/multipathd.8
> @@ -195,8 +195,10 @@ group index, starting with 1.
> .
> .TP
> .B reconfigure
> -Reconfigures the multipaths. This should be triggered automatically after anyi
> -hotplug event.
> +Reconfigure the multipaths. This is only necessary after applying configuration
> +changes, as multipathd sets up new devices automatically. See the
> +\fIforce_reconfigure\fR option in \fBmultipath.conf(5)\fR. Note that multipathd
> +can't process most commands while reconfiguring.
> .
> .TP
> .B suspend map|multipath $map
> --
> 2.33.0
--
dm-devel mailing list
dm-devel@xxxxxxxxxx
https://listman.redhat.com/mailman/listinfo/dm-devel
