| Message ID | 20250317173352.14387-1-xose.vazquez@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC] multipath-tools: add Generic SCSI in hwtable | expand |
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
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 >
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 > >
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 > > > >
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 *
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(+)