[PATCH RFC] multipath-tools: add Generic SCSI in hwtable

[PATCH RFC] multipath-tools: add Generic SCSI in hwtable

[Xose Vazquez Perez]

Cc: Martin Wilck <mwilck@suse.com>
Cc: Benjamin Marzinski <bmarzins@redhat.com>
Cc: Christophe Varoqui <christophe.varoqui@opensvc.com>
Cc: DM-DEVEL ML <dm-devel@lists.linux.dev>
Signed-off-by: Xose Vazquez Perez <xose.vazquez@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
 
 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

Re: [PATCH RFC] multipath-tools: add Generic SCSI in hwtable

[Benjamin Marzinski]

On Mon, Mar 17, 2025 at 06:33:51PM +0100, Xose Vazquez Perez wrote:
> Cc: Martin Wilck <mwilck@suse.com>
> Cc: Benjamin Marzinski <bmarzins@redhat.com>
> Cc: Christophe Varoqui <christophe.varoqui@opensvc.com>
> Cc: DM-DEVEL ML <dm-devel@lists.linux.dev>
> Signed-off-by: Xose Vazquez Perez <xose.vazquez@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.

-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

Re: [PATCH RFC] multipath-tools: add Generic SCSI in hwtable

[Martin Wilck]

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 <mwilck@suse.com>
> > Cc: Benjamin Marzinski <bmarzins@redhat.com>
> > Cc: Christophe Varoqui <christophe.varoqui@opensvc.com>
> > Cc: DM-DEVEL ML <dm-devel@lists.linux.dev>
> > Signed-off-by: Xose Vazquez Perez <xose.vazquez@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.

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
> 

Re: [PATCH RFC] multipath-tools: add Generic SCSI in hwtable

[Benjamin Marzinski]

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 <mwilck@suse.com>
> > > Cc: Benjamin Marzinski <bmarzins@redhat.com>
> > > Cc: Christophe Varoqui <christophe.varoqui@opensvc.com>
> > > Cc: DM-DEVEL ML <dm-devel@lists.linux.dev>
> > > Signed-off-by: Xose Vazquez Perez <xose.vazquez@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.

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.

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
> > 

Re: [PATCH RFC] multipath-tools: add Generic SCSI in hwtable

[Martin Wilck]

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 <mwilck@suse.com>
> > > > Cc: Benjamin Marzinski <bmarzins@redhat.com>
> > > > Cc: Christophe Varoqui <christophe.varoqui@opensvc.com>
> > > > Cc: DM-DEVEL ML <dm-devel@lists.linux.dev>
> > > > Signed-off-by: Xose Vazquez Perez <xose.vazquez@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
> > > 
>