| Message ID | 20220601202628.5469-2-mwilck@suse.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | christophe varoqui |
| Headers | show |
| Series | multipath-tools: simplify defaults for NVMe | expand |
On Wed, Jun 01, 2022 at 10:26:27PM +0200, mwilck@suse.com wrote: > From: Martin Wilck <mwilck@suse.com> > > So far we were using the general defaults (pgpolicy = FAILOVER, > pgfailback = -FAILBACK_MANUAL). Xosé's late patches were setting this to > either MULTIBUS or ANA, and -FAILBACK_IMMEDIATE, respectively > for most specific arrays. At the same time, some vendors don't like > seeing their NVMe arrays listed specifically in the multipath-tools > hwtable. > > IMO it makes sense to change the general defaults. > > detect_prio is the default, and we probe for ANA support. Thus prio > will be "ana" for arrays that support it, and "const" otherwise. > With "const", GROUP_BY_PRIO degenerates to MULTIBUS, and pgfailback > won't happen anyway. This way, our defaults match most Xosé's new entries. The > only devices for which this patch changes behavior (from FAILOVER to MULTIBUS) > are those generic devices that aren't listed, and don't support ANA. > > I considered changing the default for no_path_retry, too, but decided against > it. The default is "fail", and users who dislike that will need to change it. > no_path_retry is more a policy setting than a hardware property, anyway. I agree that these new defaults are sensible, but this patch will cause some user's arrays to change configuration when they update. I'm not against doing this at all, but this is one of those patches that distributions need to take some care with, so that they can make this change at a sensible time. So, unless there are other objections, I'm O.k. with this patch set, I just wanted to point this out. -Ben > --- > libmultipath/hwtable.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/libmultipath/hwtable.c b/libmultipath/hwtable.c > index 39daadc..e0dce84 100644 > --- a/libmultipath/hwtable.c > +++ b/libmultipath/hwtable.c > @@ -90,7 +90,8 @@ static struct hwentry default_hw[] = { > .product = ".*", > .uid_attribute = DEFAULT_NVME_UID_ATTRIBUTE, > .checker_name = NONE, > - .retain_hwhandler = RETAIN_HWHANDLER_OFF, > + .pgpolicy = GROUP_BY_PRIO, > + .pgfailback = -FAILBACK_IMMEDIATE, > }, > /* > * Apple > -- > 2.36.1 -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
On Tue, 2022-06-07 at 17:31 -0500, Benjamin Marzinski wrote: > On Wed, Jun 01, 2022 at 10:26:27PM +0200, mwilck@suse.com wrote: > > From: Martin Wilck <mwilck@suse.com> > > > > So far we were using the general defaults (pgpolicy = FAILOVER, > > pgfailback = -FAILBACK_MANUAL). Xosé's late patches were setting > > this to > > either MULTIBUS or ANA, and -FAILBACK_IMMEDIATE, respectively > > for most specific arrays. At the same time, some vendors don't like > > seeing their NVMe arrays listed specifically in the multipath-tools > > hwtable. > > > > IMO it makes sense to change the general defaults. > > > > detect_prio is the default, and we probe for ANA support. Thus prio > > will be "ana" for arrays that support it, and "const" otherwise. > > With "const", GROUP_BY_PRIO degenerates to MULTIBUS, and pgfailback > > won't happen anyway. This way, our defaults match most Xosé's new > > entries. The > > only devices for which this patch changes behavior (from FAILOVER > > to MULTIBUS) > > are those generic devices that aren't listed, and don't support > > ANA. > > > > I considered changing the default for no_path_retry, too, but > > decided against > > it. The default is "fail", and users who dislike that will need to > > change it. > > no_path_retry is more a policy setting than a hardware property, > > anyway. > > I agree that these new defaults are sensible, but this patch will > cause > some user's arrays to change configuration when they update. I'm not > against doing this at all, but this is one of those patches that > distributions need to take some care with, so that they can make this > change at a sensible time. > > So, unless there are other objections, I'm O.k. with this patch set, > I > just wanted to point this out. Understood. I believe everyone understands that Linux distributions may, and will, sometimes use device defaults that are different from upstream, especially in cases where backward compatibility matters more than anything else. The main motivation behind this patch was to avoid mentioning certain NVMe devices explicitly in the _upstream_ hwtable. Simplification, and a slight improvement of the actual defaults, is a side effect. If you don't mind, I'd appreciated a Reviewed-by ;-) Martin > > -Ben > > > --- > > libmultipath/hwtable.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/libmultipath/hwtable.c b/libmultipath/hwtable.c > > index 39daadc..e0dce84 100644 > > --- a/libmultipath/hwtable.c > > +++ b/libmultipath/hwtable.c > > @@ -90,7 +90,8 @@ static struct hwentry default_hw[] = { > > .product = ".*", > > .uid_attribute = DEFAULT_NVME_UID_ATTRIBUTE, > > .checker_name = NONE, > > - .retain_hwhandler = RETAIN_HWHANDLER_OFF, > > + .pgpolicy = GROUP_BY_PRIO, > > + .pgfailback = -FAILBACK_IMMEDIATE, > > }, > > /* > > * Apple > > -- > > 2.36.1 > -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
diff --git a/libmultipath/hwtable.c b/libmultipath/hwtable.c index 39daadc..e0dce84 100644 --- a/libmultipath/hwtable.c +++ b/libmultipath/hwtable.c @@ -90,7 +90,8 @@ static struct hwentry default_hw[] = { .product = ".*", .uid_attribute = DEFAULT_NVME_UID_ATTRIBUTE, .checker_name = NONE, - .retain_hwhandler = RETAIN_HWHANDLER_OFF, + .pgpolicy = GROUP_BY_PRIO, + .pgfailback = -FAILBACK_IMMEDIATE, }, /* * Apple
From: Martin Wilck <mwilck@suse.com> So far we were using the general defaults (pgpolicy = FAILOVER, pgfailback = -FAILBACK_MANUAL). Xosé's late patches were setting this to either MULTIBUS or ANA, and -FAILBACK_IMMEDIATE, respectively for most specific arrays. At the same time, some vendors don't like seeing their NVMe arrays listed specifically in the multipath-tools hwtable. IMO it makes sense to change the general defaults. detect_prio is the default, and we probe for ANA support. Thus prio will be "ana" for arrays that support it, and "const" otherwise. With "const", GROUP_BY_PRIO degenerates to MULTIBUS, and pgfailback won't happen anyway. This way, our defaults match most Xosé's new entries. The only devices for which this patch changes behavior (from FAILOVER to MULTIBUS) are those generic devices that aren't listed, and don't support ANA. I considered changing the default for no_path_retry, too, but decided against it. The default is "fail", and users who dislike that will need to change it. no_path_retry is more a policy setting than a hardware property, anyway. --- libmultipath/hwtable.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)