On Wed, May 31, 2023 at 03:44:58PM +0000, Martin Wilck wrote:
> On Fri, 2023-05-19 at 18:02 -0500, Benjamin Marzinski wrote:
> > This allows configuations to use "group_by_tpg" if alua is
> > autodetected
> > and another policy if it isn't, so they can work with detect_prio.
>
> This is a bit confusing. We might have introduced this kind of
> autodetection without group_by_tpg; using group_by_prio for arrays with
> ALUA support would have made quite a bit of sense.
I guess that all depends on what the autodetection is for. If the goal
for ALUA autodetection is to make it possible to write configs that
support arrays which have an optional ALUA mode, then I don't think this
is necessary. All those arrays should be configured with group_by_prio,
regardless of whether or not they are in ALUA mode.
But we've moved more towards adding autodetection to make multipath work
correctly, even without a config for a specific array. In this case,
yes, if we autodetect ALUA, if would be nice if we could
automatically set group_by_prio.
> What this patch really does is to make multipath-tools prefer
> group_by_tpg over group_by_prio if it finds that ALUA is supported.
> Should this be a separate option, perhaps?
>
> - detect_pgpolicy: use an ALUA-based pgpolicy if available
> - detect_pgpolicy_prefer_tpg: prefer group_by_tpg over group_by_prio
> for arrays supporting ALUA.
>
> This way users could benefit from ALUA autodetection without switching
> to the TPG algorithm automatically.
Sure. Lets go with that. I'll respin this.
> Or do we have good arguments that group_by_tpg is always "better" than
> group_by_prio if ALUA is supported? I guess it might be, but it still
> needs to prove its usefulness it practice.
I would also rather it proved itself first. That's why I had it disabled
by default. We can always switch the default later.
> Also, if we add the auto-detection feature, I think it should default
> to ON, at least upstream.
I don't know of any case where you would need FAILOVER, when you have an
ALUA device. I can imagine someone wanting to be able to turn off
load-balancing, but I think it makes sense to enable this by default
upstream.
>
> Regards,
> Martin
>
> >
> > Signed-off-by: Benjamin Marzinski <bmarzins@xxxxxxxxxx>
> > ---
> > libmultipath/config.c | 2 ++
> > libmultipath/config.h | 2 ++
> > libmultipath/configure.c | 1 +
> > libmultipath/defaults.h | 1 +
> > libmultipath/dict.c | 11 +++++++++++
> > libmultipath/hwtable.c | 1 +
> > libmultipath/libmultipath.version | 10 +++-------
> > libmultipath/propsel.c | 23 ++++++++++++++++++++++-
> > libmultipath/propsel.h | 1 +
> > libmultipath/structs.h | 7 +++++++
> > multipath/multipath.conf.5 | 12 ++++++++++++
> > 11 files changed, 63 insertions(+), 8 deletions(-)
> >
> > diff --git a/libmultipath/config.c b/libmultipath/config.c
> > index 5c5c0726..2e742373 100644
> > --- a/libmultipath/config.c
> > +++ b/libmultipath/config.c
> > @@ -452,6 +452,7 @@ merge_hwe (struct hwentry * dst, struct hwentry *
> > src)
> > merge_num(retain_hwhandler);
> > merge_num(detect_prio);
> > merge_num(detect_checker);
> > + merge_num(detect_pgpolicy);
> > merge_num(deferred_remove);
> > merge_num(delay_watch_checks);
> > merge_num(delay_wait_checks);
> > @@ -617,6 +618,7 @@ store_hwe (vector hwtable, struct hwentry * dhwe)
> > hwe->retain_hwhandler = dhwe->retain_hwhandler;
> > hwe->detect_prio = dhwe->detect_prio;
> > hwe->detect_checker = dhwe->detect_checker;
> > + hwe->detect_pgpolicy = dhwe->detect_pgpolicy;
> > hwe->ghost_delay = dhwe->ghost_delay;
> > hwe->vpd_vendor_id = dhwe->vpd_vendor_id;
> >
> > diff --git a/libmultipath/config.h b/libmultipath/config.h
> > index 87947469..014c6849 100644
> > --- a/libmultipath/config.h
> > +++ b/libmultipath/config.h
> > @@ -76,6 +76,7 @@ struct hwentry {
> > int retain_hwhandler;
> > int detect_prio;
> > int detect_checker;
> > + int detect_pgpolicy;
> > int deferred_remove;
> > int delay_watch_checks;
> > int delay_wait_checks;
> > @@ -171,6 +172,7 @@ struct config {
> > int retain_hwhandler;
> > int detect_prio;
> > int detect_checker;
> > + int detect_pgpolicy;
> > int force_sync;
> > int deferred_remove;
> > int processed_main_config;
> > diff --git a/libmultipath/configure.c b/libmultipath/configure.c
> > index 4a1c28bb..366b166f 100644
> > --- a/libmultipath/configure.c
> > +++ b/libmultipath/configure.c
> > @@ -304,6 +304,7 @@ int setup_map(struct multipath *mpp, char
> > **params, struct vectors *vecs)
> > pthread_cleanup_push(put_multipath_config, conf);
> >
> > select_pgfailback(conf, mpp);
> > + select_detect_pgpolicy(conf, mpp);
> > select_pgpolicy(conf, mpp);
> >
> > /*
> > diff --git a/libmultipath/defaults.h b/libmultipath/defaults.h
> > index a5e9ea0c..090baa5c 100644
> > --- a/libmultipath/defaults.h
> > +++ b/libmultipath/defaults.h
> > @@ -29,6 +29,7 @@
> > #define DEFAULT_RETAIN_HWHANDLER RETAIN_HWHANDLER_ON
> > #define DEFAULT_DETECT_PRIO DETECT_PRIO_ON
> > #define DEFAULT_DETECT_CHECKER DETECT_CHECKER_ON
> > +#define DEFAULT_DETECT_PGPOLICY DETECT_PGPOLICY_OFF
> > #define DEFAULT_DEFERRED_REMOVE DEFERRED_REMOVE_OFF
> > #define DEFAULT_DELAY_CHECKS NU_NO
> > #define DEFAULT_ERR_CHECKS NU_NO
> > diff --git a/libmultipath/dict.c b/libmultipath/dict.c
> > index dddd3cd6..edd4923d 100644
> > --- a/libmultipath/dict.c
> > +++ b/libmultipath/dict.c
> > @@ -868,6 +868,14 @@ declare_ovr_snprint(detect_checker,
> > print_yes_no_undef)
> > declare_hw_handler(detect_checker, set_yes_no_undef)
> > declare_hw_snprint(detect_checker, print_yes_no_undef)
> >
> > +declare_def_handler(detect_pgpolicy, set_yes_no_undef)
> > +declare_def_snprint_defint(detect_pgpolicy, print_yes_no_undef,
> > + DEFAULT_DETECT_PGPOLICY)
> > +declare_ovr_handler(detect_pgpolicy, set_yes_no_undef)
> > +declare_ovr_snprint(detect_pgpolicy, print_yes_no_undef)
> > +declare_hw_handler(detect_pgpolicy, set_yes_no_undef)
> > +declare_hw_snprint(detect_pgpolicy, print_yes_no_undef)
> > +
> > declare_def_handler(force_sync, set_yes_no)
> > declare_def_snprint(force_sync, print_yes_no)
> >
> > @@ -2112,6 +2120,7 @@ init_keywords(vector keywords)
> > install_keyword("retain_attached_hw_handler",
> > &def_retain_hwhandler_handler, &snprint_def_retain_hwhandler);
> > install_keyword("detect_prio", &def_detect_prio_handler,
> > &snprint_def_detect_prio);
> > install_keyword("detect_checker",
> > &def_detect_checker_handler, &snprint_def_detect_checker);
> > + install_keyword("detect_pgpolicy",
> > &def_detect_pgpolicy_handler, &snprint_def_detect_pgpolicy);
> > install_keyword("force_sync", &def_force_sync_handler,
> > &snprint_def_force_sync);
> > install_keyword("strict_timing", &def_strict_timing_handler,
> > &snprint_def_strict_timing);
> > install_keyword("deferred_remove",
> > &def_deferred_remove_handler, &snprint_def_deferred_remove);
> > @@ -2202,6 +2211,7 @@ init_keywords(vector keywords)
> > install_keyword("retain_attached_hw_handler",
> > &hw_retain_hwhandler_handler, &snprint_hw_retain_hwhandler);
> > install_keyword("detect_prio", &hw_detect_prio_handler,
> > &snprint_hw_detect_prio);
> > install_keyword("detect_checker", &hw_detect_checker_handler,
> > &snprint_hw_detect_checker);
> > + install_keyword("detect_pgpolicy",
> > &hw_detect_pgpolicy_handler, &snprint_hw_detect_pgpolicy);
> > install_keyword("deferred_remove",
> > &hw_deferred_remove_handler, &snprint_hw_deferred_remove);
> > install_keyword("delay_watch_checks",
> > &hw_delay_watch_checks_handler, &snprint_hw_delay_watch_checks);
> > install_keyword("delay_wait_checks",
> > &hw_delay_wait_checks_handler, &snprint_hw_delay_wait_checks);
> > @@ -2244,6 +2254,7 @@ init_keywords(vector keywords)
> > install_keyword("retain_attached_hw_handler",
> > &ovr_retain_hwhandler_handler, &snprint_ovr_retain_hwhandler);
> > install_keyword("detect_prio", &ovr_detect_prio_handler,
> > &snprint_ovr_detect_prio);
> > install_keyword("detect_checker",
> > &ovr_detect_checker_handler, &snprint_ovr_detect_checker);
> > + install_keyword("detect_pgpolicy",
> > &ovr_detect_pgpolicy_handler, &snprint_ovr_detect_pgpolicy);
> > install_keyword("deferred_remove",
> > &ovr_deferred_remove_handler, &snprint_ovr_deferred_remove);
> > install_keyword("delay_watch_checks",
> > &ovr_delay_watch_checks_handler, &snprint_ovr_delay_watch_checks);
> > install_keyword("delay_wait_checks",
> > &ovr_delay_wait_checks_handler, &snprint_ovr_delay_wait_checks);
> > diff --git a/libmultipath/hwtable.c b/libmultipath/hwtable.c
> > index 65bca744..803230c1 100644
> > --- a/libmultipath/hwtable.c
> > +++ b/libmultipath/hwtable.c
> > @@ -67,6 +67,7 @@
> > .retain_hwhandler = RETAIN_HWHANDLER_ON,
> > .detect_prio = DETECT_PRIO_ON,
> > .detect_checker = DETECT_CHECKER_ON,
> > + .detect_pgpolicy = DETECT_PGPOLICY_OFF,
> > .deferred_remove = DEFERRED_REMOVE_OFF,
> > .delay_watch_checks = DELAY_CHECKS_OFF,
> > .delay_wait_checks = DELAY_CHECKS_OFF,
> > diff --git a/libmultipath/libmultipath.version
> > b/libmultipath/libmultipath.version
> > index aba1a30e..8f72c452 100644
> > --- a/libmultipath/libmultipath.version
> > +++ b/libmultipath/libmultipath.version
> > @@ -43,7 +43,7 @@ LIBMPATHCOMMON_1.0.0 {
> > put_multipath_config;
> > };
> >
> > -LIBMULTIPATH_18.0.0 {
> > +LIBMULTIPATH_19.0.0 {
> > global:
> > /* symbols referenced by multipath and multipathd */
> > add_foreign;
> > @@ -116,6 +116,7 @@ global:
> > get_refwwid;
> > get_state;
> > get_udev_device;
> > + get_uid;
> > get_used_hwes;
> > get_vpd_sgio;
> > group_by_prio;
> > @@ -141,6 +142,7 @@ global:
> > pathcount;
> > path_discovery;
> > path_get_tpgs;
> > + pathinfo;
> > path_offline;
> > print_all_paths;
> > print_foreign_topology;
> > @@ -235,9 +237,3 @@ global:
> > local:
> > *;
> > };
> > -
> > -LIBMULTIPATH_18.1.0 {
> > -global:
> > - get_uid;
> > - pathinfo;
> > -} LIBMULTIPATH_18.0.0;
> > diff --git a/libmultipath/propsel.c b/libmultipath/propsel.c
> > index d214281b..12f4825d 100644
> > --- a/libmultipath/propsel.c
> > +++ b/libmultipath/propsel.c
> > @@ -265,6 +265,21 @@ verify_alua_prio(struct multipath *mp)
> > return true;
> > }
> >
> > +int select_detect_pgpolicy(struct config *conf, struct multipath
> > *mp)
> > +{
> > + const char *origin;
> > +
> > + mp_set_ovr(detect_pgpolicy);
> > + mp_set_hwe(detect_pgpolicy);
> > + mp_set_conf(detect_pgpolicy);
> > + mp_set_default(detect_pgpolicy, DEFAULT_DETECT_PGPOLICY);
> > +out:
> > + condlog(3, "%s: detect_pgpolicy = %s %s", mp->alias,
> > + (mp->detect_pgpolicy == DETECT_PGPOLICY_ON)? "yes" :
> > "no",
> > + origin);
> > + return 0;
> > +}
> > +
> > int select_pgpolicy(struct config *conf, struct multipath * mp)
> > {
> > const char *origin;
> > @@ -275,13 +290,19 @@ int select_pgpolicy(struct config *conf, struct
> > multipath * mp)
> > origin = cmdline_origin;
> > goto out;
> > }
> > + if (mp->detect_pgpolicy == DETECT_PGPOLICY_ON &&
> > verify_alua_prio(mp)) {
> > + mp->pgpolicy = GROUP_BY_TPG;
> > + origin = autodetect_origin;
> > + goto out;
> > + }
> > mp_set_mpe(pgpolicy);
> > mp_set_ovr(pgpolicy);
> > mp_set_hwe(pgpolicy);
> > mp_set_conf(pgpolicy);
> > mp_set_default(pgpolicy, DEFAULT_PGPOLICY);
> > out:
> > - if (mp->pgpolicy == GROUP_BY_TPG && !verify_alua_prio(mp)) {
> > + if (mp->pgpolicy == GROUP_BY_TPG && origin !=
> > autodetect_origin &&
> > + !verify_alua_prio(mp)) {
> > mp->pgpolicy = DEFAULT_PGPOLICY;
> > origin = "(setting: emergency fallback - not all
> > paths use alua prio)";
> > log_prio = 1;
> > diff --git a/libmultipath/propsel.h b/libmultipath/propsel.h
> > index 152ca44c..513ee6ac 100644
> > --- a/libmultipath/propsel.h
> > +++ b/libmultipath/propsel.h
> > @@ -1,5 +1,6 @@
> > int select_rr_weight (struct config *conf, struct multipath * mp);
> > int select_pgfailback (struct config *conf, struct multipath * mp);
> > +int select_detect_pgpolicy (struct config *conf, struct multipath *
> > mp);
> > int select_pgpolicy (struct config *conf, struct multipath * mp);
> > int select_selector (struct config *conf, struct multipath * mp);
> > int select_alias (struct config *conf, struct multipath * mp);
> > diff --git a/libmultipath/structs.h b/libmultipath/structs.h
> > index 0eea19b4..682a7e17 100644
> > --- a/libmultipath/structs.h
> > +++ b/libmultipath/structs.h
> > @@ -143,6 +143,12 @@ enum detect_checker_states {
> > DETECT_CHECKER_ON = YNU_YES,
> > };
> >
> > +enum detect_pgpolicy_states {
> > + DETECT_PGPOLICY_UNDEF = YNU_UNDEF,
> > + DETECT_PGPOLICY_OFF = YNU_NO,
> > + DETECT_PGPOLICY_ON = YNU_YES,
> > +};
> > +
> > enum deferred_remove_states {
> > DEFERRED_REMOVE_UNDEF = YNU_UNDEF,
> > DEFERRED_REMOVE_OFF = YNU_NO,
> > @@ -389,6 +395,7 @@ enum prflag_value {
> > struct multipath {
> > char wwid[WWID_SIZE];
> > char alias_old[WWID_SIZE];
> > + int detect_pgpolicy;
> > int pgpolicy;
> > pgpolicyfn *pgpolicyfn;
> > int nextpg;
> > diff --git a/multipath/multipath.conf.5 b/multipath/multipath.conf.5
> > index b65a543d..9f8be510 100644
> > --- a/multipath/multipath.conf.5
> > +++ b/multipath/multipath.conf.5
> > @@ -242,6 +242,18 @@ The default is: \fBfailover\fR
> > .
> > .
> > .TP
> > +.B detect_pgpolicy
> > +If set to \fIyes\fR and all path devcices are configured with either
> > the
> > +\fIalua\fR or \fIsysfs\fR prioritizer, the multipath device will
> > automatically
> > +use the \fIgroup_by_tpg\fR path_grouping_policy. If set to \fIno\fR,
> > the
> > +path_grouping_policy will be selected as usual.
> > +.RS
> > +.TP
> > +The default is: \fIno\fR
> > +.RE
> > +.
> > +.
> > +.TP
> > .B pg_timeout
> > (Deprecated) This option is not supported any more, and the value is
> > ignored.
> > .
--
dm-devel mailing list
dm-devel@xxxxxxxxxx
https://listman.redhat.com/mailman/listinfo/dm-devel
