[dm-devel] [PATCH v3 16/17] multipathd: Don't use fallback code after getting wwid

[Martin Wilck]

On Sat, 2019-03-30 at 01:06 -0500, Benjamin Marzinski wrote:
> The fallback code is necessary to set up mutipath devices, if
> multipath
> temporarily can't get the information from udev.  However, once the
> devices are set up, udev is the definitive source of this
> information.
> 
> The wwid gotten from the fallback code and the udev code should
> always
> be the same, in which case it doesn't matter where we get the wwid
> from. But if they are different, it's important to try to do the
> right thing.
> 
> Working under the assumption that udev will either never give us this
> information, or that it usually will. multipath should assume that if
> there are multiple paths to a device, either they will all never get
> a wwid from udev, or some of them will likely already have gotten the
> correct wwid from udev.  In this case, we should fix this as soon as
> possible.
> 
> This does mean that devices where udev will never give out the uuid
> will not notice if the wwid changes, but that's a small price to pay
> for doing the right thing most of the time.
> 
> Signed-off-by: Benjamin Marzinski <bmarzins at redhat.com>
> ---
>  libmultipath/discovery.c | 22 +++++++++-------------
>  libmultipath/discovery.h |  3 ++-
>  multipathd/main.c        |  2 +-
>  3 files changed, 12 insertions(+), 15 deletions(-)
> 
> diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
> index 3ec60d6..744cf2c 100644
> --- a/libmultipath/discovery.c
> +++ b/libmultipath/discovery.c
> @@ -1763,7 +1763,6 @@ static ssize_t uid_fallback(struct path *pp,
> int path_state,
>  	    !strcmp(pp->uid_attribute, DEFAULT_UID_ATTRIBUTE)) {
>  		len = get_vpd_uid(pp);
>  		*origin = "sysfs";
> -		pp->uid_attribute = NULL;

Hm, that makes me realize that the previous code was broken, as we
used strcmp(pp->uid_attribute, DEFAULT_UID_ATTRIBUTE) elsewhere without
checking if uid_attribute was NULL ...

@@ -1846,14 +1846,9 @@ get_uid (struct path * pp, int path_state,
> struct udev_device *udev)
>  			len = get_vpd_uid(pp);
>  			origin = "sysfs";

On the premise that "udev rules", maybe we should also remove the above
code, which is nothing but yet another fallback?

But that can be done on top of your set.

Reviewed-by: Martin Wilck <mwilck at suse.com>
-- 
Dr. Martin Wilck <mwilck at suse.com>, Tel. +49 (0)911 74053 2107
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)