On Tue, 2025-03-18 at 19:32 -0400, Benjamin Marzinski wrote:
> On Tue, Mar 18, 2025 at 12:08:32PM +0100, Martin Wilck wrote:
> > On Mon, 2025-03-17 at 16:00 -0400, Benjamin Marzinski wrote:
> > > On Mon, Mar 17, 2025 at 06:33:51PM +0100, Xose Vazquez Perez
> > > wrote:
> > > > Cc: Martin Wilck <mwi...@suse.com>
> > > > Cc: Benjamin Marzinski <bmarz...@redhat.com>
> > > > Cc: Christophe Varoqui <christophe.varo...@opensvc.com>
> > > > Cc: DM-DEVEL ML <dm-devel@lists.linux.dev>
> > > > Signed-off-by: Xose Vazquez Perez <xose.vazq...@gmail.com>
> > > > ---
> > > > libmultipath/hwtable.c | 11 +++++++++++
> > > > 1 file changed, 11 insertions(+)
> > > >
> > > > diff --git a/libmultipath/hwtable.c b/libmultipath/hwtable.c
> > > > index f8cf3fa9..34b1fd2f 100644
> > > > --- a/libmultipath/hwtable.c
> > > > +++ b/libmultipath/hwtable.c
> > > > @@ -79,6 +79,17 @@
> > > > #endif
> > > >
> > >
> > > If we wanted to make these changes, we could just change
> > > DEFAULT_PGPOLICY, DEFAULT_FAILBACK, and DEFAULT_NO_PATH_RETRY.
> > > But I
> > > still think that for completely unknown devices, we should stick
> > > with
> > > our current defaults. They're safe and I haven't heard any
> > > complaints
> > > about them.
> >
> > I agree. The point of the hwtable is to change the defaults for
> > specific devices. Using "vendor = .*" and "product = ".*" is
> > against
> > the spirit of the hwtable.
> >
> > Unless I am mistaken, this change would override user settings in
> > the
> > "defaults" section of multipath.conf. This happens a lot already
> > for
> > those systems that do have a specific hwtable entry, and I am sure
> > that
> > that comes as a surprise for users (it has confused myself a
> > significant couple of times). By adding these catch-all entries,
> > we'd
> > make the "defaults" section ineffective for any setting that's
> > available in the "devices" section, too.
> >
> > Thinking about it, that might actually be a good thing, as it would
> > eliminate the uncertainty whether or not a given "defaults" setting
> > would take effect for a given storage device. Maybe we should
> > officially stop supporting settings in "defaults" that can be
> > overridden by device-spefic settings, just to reduce confusion in
> > this
> > area, and recommend using a catch-all device setting (in
> > multipath.conf, not in the built-in hwtable) instead.
>
> The device configs from /etc/multipath.conf will override values in
> the
> built-in hwtable. So a catch-all device config in /etc/multipath.conf
> is
> the same as putting options in the overrides section, not the
> defaults
> section.
Yes. I'd realized that meanwhile, too. Thanks for clarifying it, and
sorry for the confusion.
The "proble"m that I see with our configuration logic is that some
settings simply don't take effect.
- hwtable settings take precedence over defaults
- detect_prio etc. take precedence over explicit settings
While this logic does make general sense, I am sure that many users
have pulled their hair about it, even though it's documented.
But this is how it has been for a decade. User have got used to it. I
don't think it makes sense to put lots of energy into changing it.
> Now that we changed how hwentry configs work, we could probably
> deprecate the overrides section, but I don't think we can replace the
> defaults section.
No, we can't. And we should keep overrides, too. I find it quite
convenient. Unlike the other sections, you can be certain that what you
set there will take effect :-) Also, we need it for the protocol
subsection.
Martin
>
> right?
>
> -Ben
>
> > Anyway, that would be a possible cause for major breakage, as user
> > settings in the field would suddenly stop being effective. We can't
> > possibly just let it slip in with a minor hwtable patch like this.
> >
> > Regards
> > Martin
> >
> > > -Ben
> > >
> > > > static struct hwentry default_hw[] = {
> > > > + /*
> > > > + * Generic SCSI devices
> > > > + */
> > > > + {
> > > > + /* Generic SCSI */
> > > > + .vendor = ".*",
> > > > + .product = ".*",
> > > > + .pgpolicy = GROUP_BY_PRIO,
> > > > + .pgfailback = -FAILBACK_IMMEDIATE,
> > > > + .no_path_retry = 30,
> > > > + },
> > > > /*
> > > > * Generic NVMe devices
> > > > *
> > > > --
> > > > 2.48.1
> > >
>
