multipath-tools: RH-patches for upstream ???

multipath-tools: RH-patches for upstream ???

[Xose Vazquez Perez]

Hi Benjamin,


Is there any relevant RH-patch for upstream in fedora repo:
https://src.fedoraproject.org/rpms/device-mapper-multipath/ ???

Maybe:

- https://src.fedoraproject.org/rpms/device-mapper-multipath/blob/master/f/0022-RH-Remove-the-property-blacklist-exception-builtin.patch

    Subject: [PATCH] RH: Remove the property blacklist exception builtin

     Multipath set the default property blacklist exceptions to	
     (ID_SCSI_VPD|ID_WWN).  This has the effect of blacklisting some internal
     devices.  These devices may never have multiple paths, but it is nice
     to be able to set multipath up on them all the same.  This patch simply
     removes the default, and makes it so that if no property
     blacklist_exception is given, then devices aren't failed for not matching
     it.


- https://src.fedoraproject.org/rpms/device-mapper-multipath/blob/master/f/0026-RH-add-wwids-from-kernel-cmdline-mpath.wwids-with-A.patch

    Subject: [PATCH] RH: add wwids from kernel cmdline mpath.wwids with -A

     This patch adds another option to multipath, "-A", which reads
     /proc/cmdline for mpath.wwid=<WWID> options, and adds any wwids it finds
     to /etc/multipath/wwids.  While this isn't usually important during
     normal operation, since these wwids should already be added, it can be
     helpful during installation, to make sure that multipath can claim
     devices as its own, before LVM or something else makes use of them.  The
     patch also execs "/sbin/multipath -A" before running multipathd in
     multipathd.service


- https://src.fedoraproject.org/rpms/device-mapper-multipath/blob/master/f/0027-RH-warn-on-invalid-regex-instead-of-failing.patch

    Subject: [PATCH] RH: warn on invalid regex instead of failing

     multipath.conf used to allow "*" as a match everything regular expression,
     instead of requiring ".*". Instead of erroring when the old style
     regular expressions are used, it should print a warning and convert
     them.


- https://src.fedoraproject.org/rpms/device-mapper-multipath/blob/master/f/0028-RH-reset-default-find_mutipaths-value-to-off.patch

    Subject: [PATCH] RH: reset default find_mutipaths value to off

     Upstream has changed to default find_multipaths to "strict". For now
     Redhat will retain the previous default of "off".


- https://src.fedoraproject.org/rpms/device-mapper-multipath/blob/master/f/0029-RH-Fix-nvme-compilation-warning.patch

    Subject: [PATCH] RH: Fix nvme compilation warning


- https://src.fedoraproject.org/rpms/device-mapper-multipath/blob/master/f/0030-RH-attempt-to-get-ANA-info-via-sysfs-first.patch

    Subject: [PATCH] RH: attempt to get ANA info via sysfs first

     When the ANA prioritizer is run, first see if the "ana_state" sysfs file
     exists, and if it does, try to read the state from there. If that fails,
     fallback to using an ioctl.


Thank you.

Re: multipath-tools: RH-patches for upstream ???

[Benjamin Marzinski]

On Thu, Oct 03, 2019 at 08:28:06PM +0200, Xose Vazquez Perez wrote:
> Hi Benjamin,
> 
> 
> Is there any relevant RH-patch for upstream in fedora repo:
> https://src.fedoraproject.org/rpms/device-mapper-multipath/ ???
> 
> Maybe:
> 
> - https://src.fedoraproject.org/rpms/device-mapper-multipath/blob/master/f/0022-RH-Remove-the-property-blacklist-exception-builtin.patch
> 
>    Subject: [PATCH] RH: Remove the property blacklist exception builtin
> 
>     Multipath set the default property blacklist exceptions to	
>     (ID_SCSI_VPD|ID_WWN).  This has the effect of blacklisting some internal
>     devices.  These devices may never have multiple paths, but it is nice
>     to be able to set multipath up on them all the same.  This patch simply
>     removes the default, and makes it so that if no property
>     blacklist_exception is given, then devices aren't failed for not matching
>     it.

Redhat doesn't include the udev rules file that sets ID_SCSI_VPD, so
there are some rare cases where this property blacklists valid devices.
Thus, it's easier for us to simply include this property line in the
default multipath.conf, and let users remove it if necessary. I would be
fine with this being included upstream, but I suspect it would mess with
other ditsros which are currently assuming that it is there.

> 
> 
> - https://src.fedoraproject.org/rpms/device-mapper-multipath/blob/master/f/0026-RH-add-wwids-from-kernel-cmdline-mpath.wwids-with-A.patch
> 
>    Subject: [PATCH] RH: add wwids from kernel cmdline mpath.wwids with -A
> 
>     This patch adds another option to multipath, "-A", which reads
>     /proc/cmdline for mpath.wwid=<WWID> options, and adds any wwids it finds
>     to /etc/multipath/wwids.  While this isn't usually important during
>     normal operation, since these wwids should already be added, it can be
>     helpful during installation, to make sure that multipath can claim
>     devices as its own, before LVM or something else makes use of them.  The
>     patch also execs "/sbin/multipath -A" before running multipathd in
>     multipathd.service
> 

I posted this upstream and Hannes NAKed it a while back. We still find
it useful, since the default multipath.conf file for Redhat sets
find_multipaths to yes. You can currently avoid the race that this is
fixing by setting find_multipaths to smart, but there were objections to
using that as a default in Redhat. However, I never really understood
the objection to this patch, and I'd be fine with re-posting it.

> - https://src.fedoraproject.org/rpms/device-mapper-multipath/blob/master/f/0027-RH-warn-on-invalid-regex-instead-of-failing.patch
> 
>    Subject: [PATCH] RH: warn on invalid regex instead of failing
> 
>     multipath.conf used to allow "*" as a match everything regular expression,
>     instead of requiring ".*". Instead of erroring when the old style
>     regular expressions are used, it should print a warning and convert
>     them.

When multipath used its old regex code, "*" worked to match everything.
This patch just exists to make sure that customers didn't need to change
their configs when the regex code changed. Since it's been there posting
warning messages for a while, I plan to eventually drop it entirely, and
let anyone who has been ignoring the warning messages for years finally
have their config error. I see no reason to add it back to the upstream
code now.
 
> 
> - https://src.fedoraproject.org/rpms/device-mapper-multipath/blob/master/f/0028-RH-reset-default-find_mutipaths-value-to-off.patch
> 
>    Subject: [PATCH] RH: reset default find_mutipaths value to off
> 
>     Upstream has changed to default find_multipaths to "strict". For now
>     Redhat will retain the previous default of "off".

This is simply to retain the previous default behavior, for much the
same reason as the above patch. I see no reason to change this upstream.

> 
> - https://src.fedoraproject.org/rpms/device-mapper-multipath/blob/master/f/0029-RH-Fix-nvme-compilation-warning.patch
> 
>    Subject: [PATCH] RH: Fix nvme compilation warning
>

I assume that other people aren't seeing these compilation warnings, and
this it due to different options that redhat uses when compiling.  I
really should push this patch upstream, but that upstream isn't
multipath-tools, its nvme-cli, where we copied this file from. Once it's
changed there, we can pull the updated files back to multipath-tools.
 
>
> - https://src.fedoraproject.org/rpms/device-mapper-multipath/blob/master/f/0030-RH-attempt-to-get-ANA-info-via-sysfs-first.patch
> 
>    Subject: [PATCH] RH: attempt to get ANA info via sysfs first
> 
>     When the ANA prioritizer is run, first see if the "ana_state" sysfs file
>     exists, and if it does, try to read the state from there. If that fails,
>     fallback to using an ioctl.
> 

This won't do anything upstream. This requires a redhat specific kernel
patch that wasn't accepted in the upstream nvme kernel code.  It really
doesn't make much of a difference. It just makes multipath try to grab
ANA state info from sysfs, before failing back to the same ioctl that
upstream uses.

- Ben

> Thank you.

Re: multipath-tools: RH-patches for upstream ???

[Martin Wilck]

Hi Xose, hi Ben,

On Thu, 2019-10-03 at 16:44 -0500, Benjamin Marzinski wrote:
> On Thu, Oct 03, 2019 at 08:28:06PM +0200, Xose Vazquez Perez wrote:
> > Hi Benjamin,
> > 
> > 
> > Is there any relevant RH-patch for upstream in fedora repo:
> > https://src.fedoraproject.org/rpms/device-mapper-multipath/ ???
> > 
> > Maybe:
> > 
> > - 
> > https://src.fedoraproject.org/rpms/device-mapper-multipath/blob/master/f/0022-RH-Remove-the-property-blacklist-exception-builtin.patch
> > 
> >    Subject: [PATCH] RH: Remove the property blacklist exception
> > builtin
> > 
> >     Multipath set the default property blacklist exceptions to	
> >     (ID_SCSI_VPD|ID_WWN).  This has the effect of blacklisting some
> > internal
> >     devices.  These devices may never have multiple paths, but it
> > is nice
> >     to be able to set multipath up on them all the same.  This
> > patch simply
> >     removes the default, and makes it so that if no property
> >     blacklist_exception is given, then devices aren't failed for
> > not matching
> >     it.
> 
> Redhat doesn't include the udev rules file that sets ID_SCSI_VPD, so
> there are some rare cases where this property blacklists valid
> devices.
> Thus, it's easier for us to simply include this property line in the
> default multipath.conf, and let users remove it if necessary. I would
> be
> fine with this being included upstream, but I suspect it would mess
> with
> other ditsros which are currently assuming that it is there.

Hm. ID_SCSI_VPD is nowhere referenced in the upstream code. The default
is "(SCSI_IDENT_|ID_WWN)", where SCSI_IDENT_ could be regarded as a
SUSE-ism because the SCSI_IDENT_* properties are set by sg_inq / sg_vpd
calls, and I'm not sure if other distros consequently use sg_inq rather
than scs_id like we do. We (SUSE) have been working on replacing
scsi_id by sg3_utils calls in upstream systemd, but so far that hasn't
been merged, systemd maintainers are very cautious about touching these
udev rules.

I'd be interested in seeing an example for a device that is erroneously
excluded by the default blacklist rule.

In general, it's a shortcoming of multipath-tools that the built-in
blacklists can only be appended to, not replaced, by user
configuration.

> > 
> > - https://src.fedoraproject.org/rpms/device-mapper-
> > multipath/blob/master/f/0026-RH-add-wwids-from-kernel-cmdline-
> > mpath.wwids-with-A.patch
> > 
> >    Subject: [PATCH] RH: add wwids from kernel cmdline mpath.wwids
> > with -A
> > 
> >     This patch adds another option to multipath, "-A", which reads
> >     /proc/cmdline for mpath.wwid=<WWID> options, and adds any wwids
> > it finds
> >     to /etc/multipath/wwids.  While this isn't usually important
> > during
> >     normal operation, since these wwids should already be added, it
> > can be
> >     helpful during installation, to make sure that multipath can
> > claim
> >     devices as its own, before LVM or something else makes use of
> > them.  The
> >     patch also execs "/sbin/multipath -A" before running multipathd
> > in
> >     multipathd.service
> > 
> 
> I posted this upstream and Hannes NAKed it a while back. We still
> find
> it useful, since the default multipath.conf file for Redhat sets
> find_multipaths to yes. You can currently avoid the race that this is
> fixing by setting find_multipaths to smart, but there were objections
> to
> using that as a default in Redhat. However, I never really understood
> the objection to this patch, and I'd be fine with re-posting it.

https://www.redhat.com/archives/dm-devel/2014-July/msg00011.html

I'm with Hannes. Doing this in dracut, udev rules, or systemd service
files seems cleaner to me than yet another daemon that tries to
interpret the kernel command line.

See e.g. how we implemented multipath=off (cd3184e).

> > - 
> > https://src.fedoraproject.org/rpms/device-mapper-multipath/blob/master/f/0027-RH-warn-on-invalid-regex-instead-of-failing.patch
> > 
> >    Subject: [PATCH] RH: warn on invalid regex instead of failing
> > 
> >     multipath.conf used to allow "*" as a match everything regular
> > expression,
> >     instead of requiring ".*". Instead of erroring when the old
> > style
> >     regular expressions are used, it should print a warning and
> > convert
> >     them.
> c45d2a0e
> When multipath used its old regex code, "*" worked to match
> everything.
> This patch just exists to make sure that customers didn't need to
> change
> their configs when the regex code changed. Since it's been there
> posting
> warning messages for a while, I plan to eventually drop it entirely,
> and
> let anyone who has been ignoring the warning messages for years
> finally
> have their config error. I see no reason to add it back to the
> upstream
> code now.

Ack. Upstream has removed support for "*" in 2014 (b9d11f3), and did
it deliberately.

>  
> > - 
> > https://src.fedoraproject.org/rpms/device-mapper-multipath/blob/master/f/0028-RH-reset-default-find_mutipaths-value-to-off.patch
> > 
> >    Subject: [PATCH] RH: reset default find_mutipaths value to off
> > 
> >     Upstream has changed to default find_multipaths to "strict".
> > For now
> >     Redhat will retain the previous default of "off".
> 
> This is simply to retain the previous default behavior, for much the
> same reason as the above patch. I see no reason to change this
> upstream.

Ack. Btw SUSE changes the default, too (to "greedy", though).

> 
> > - 
> > https://src.fedoraproject.org/rpms/device-mapper-multipath/blob/master/f/0029-RH-Fix-nvme-compilation-warning.patch
> > 
> >    Subject: [PATCH] RH: Fix nvme compilation warning
> > 
> 
> I assume that other people aren't seeing these compilation warnings,
> and
> this it due to different options that redhat uses when compiling.  I
> really should push this patch upstream, but that upstream isn't
> multipath-tools, its nvme-cli, where we copied this file from. Once
> it's
> changed there, we can pull the updated files back to multipath-tools.

Ack. Which compiler option triggers this warning?

>  
> > - 
> > https://src.fedoraproject.org/rpms/device-mapper-multipath/blob/master/f/0030-RH-attempt-to-get-ANA-info-via-sysfs-first.patch
> > 
> >    Subject: [PATCH] RH: attempt to get ANA info via sysfs first
> > 
> >     When the ANA prioritizer is run, first see if the "ana_state"
> > sysfs file
> >     exists, and if it does, try to read the state from there. If
> > that fails,
> >     fallback to using an ioctl.
> > 
> 
> This won't do anything upstream. This requires a redhat specific
> kernel
> patch that wasn't accepted in the upstream nvme kernel code.  It
> really
> doesn't make much of a difference. It just makes multipath try to
> grab
> ANA state info from sysfs, before failing back to the same ioctl that
> upstream uses.

Ack.

Martin

Re: multipath-tools: RH-patches for upstream ???

[Xose Vazquez Perez]

On 10/4/19 9:03 AM, Martin Wilck wrote:

>>> https://src.fedoraproject.org/rpms/device-mapper-multipath/blob/master/f/0029-RH-Fix-nvme-compilation-warning.patch
>>>
>>>     Subject: [PATCH] RH: Fix nvme compilation warning
>>>
>>
>> I assume that other people aren't seeing these compilation warnings,
>> and
>> this it due to different options that redhat uses when compiling.  I
>> really should push this patch upstream, but that upstream isn't
>> multipath-tools, its nvme-cli, where we copied this file from. Once
>> it's
>> changed there, we can pull the updated files back to multipath-tools.
> 
> Ack. Which compiler option triggers this warning?

With this patch: https://src.fedoraproject.org/rpms/device-mapper-multipath/blob/master/f/0024-RH-use-rpm-optflags-if-present.patch
and /usr/lib/rpm/redhat/redhat-hardened-cc1 :
*cc1_options:
+ %{!r:%{!fpie:%{!fPIE:%{!fpic:%{!fPIC:%{!fno-pic:-fPIE}}}}}}

Re: multipath-tools: RH-patches for upstream ???

[Benjamin Marzinski]

On Fri, Oct 04, 2019 at 07:03:26AM +0000, Martin Wilck wrote:
> Hi Xose, hi Ben,
> 
> On Thu, 2019-10-03 at 16:44 -0500, Benjamin Marzinski wrote:
> > On Thu, Oct 03, 2019 at 08:28:06PM +0200, Xose Vazquez Perez wrote:
> > 
> > Redhat doesn't include the udev rules file that sets ID_SCSI_VPD, so
> > there are some rare cases where this property blacklists valid
> > devices.
> > Thus, it's easier for us to simply include this property line in the
> > default multipath.conf, and let users remove it if necessary. I would
> > be
> > fine with this being included upstream, but I suspect it would mess
> > with
> > other ditsros which are currently assuming that it is there.
> 
> Hm. ID_SCSI_VPD is nowhere referenced in the upstream code. The default
> is "(SCSI_IDENT_|ID_WWN)", where SCSI_IDENT_ could be regarded as a
> SUSE-ism because the SCSI_IDENT_* properties are set by sg_inq / sg_vpd
> calls, and I'm not sure if other distros consequently use sg_inq rather
> than scs_id like we do. We (SUSE) have been working on replacing
> scsi_id by sg3_utils calls in upstream systemd, but so far that hasn't
> been merged, systemd maintainers are very cautious about touching these
> udev rules.

Sorry, I meant SCSI_IDENT_. The properties blacklist line originally was
(ID_SCSI_VPD|ID_WWN). I never bothered to update the commit message when
I changed this commit to deal with the new line.
 
> 
> I'd be interested in seeing an example for a device that is erroneously
> excluded by the default blacklist rule.

I'm not sure if there are any specific arrays that always fail to set
ID_WWN. I've gotten rare reports about this failing, but after I tell
them to not use that property blacklist line, they go away, and I never
get enough information to figure out what specifically is happening that
means that ID_WWN isn't getting set. It's something that I've been
meaning to look into, but with this patch, there's not much urgency
around it.
 
> > I posted this upstream and Hannes NAKed it a while back. We still
> > find
> > it useful, since the default multipath.conf file for Redhat sets
> > find_multipaths to yes. You can currently avoid the race that this is
> > fixing by setting find_multipaths to smart, but there were objections
> > to
> > using that as a default in Redhat. However, I never really understood
> > the objection to this patch, and I'd be fine with re-posting it.
> 
> https://www.redhat.com/archives/dm-devel/2014-July/msg00011.html
> 
> I'm with Hannes. Doing this in dracut, udev rules, or systemd service
> files seems cleaner to me than yet another daemon that tries to
> interpret the kernel command line.
> 
> See e.g. how we implemented multipath=off (cd3184e).

Yep. But again, this is already solveable now by running multipath with
find_multipaths=smart, so I'm not sure that there's a need for it
upstream at all.  If I rework this patch to something that would be
accpetable upstream, I'll post it, in case anyone is interested in it.  
 
-Ben