[dm-devel] [PATCH 0/4] Fixups to my wwid recheck patch

[dm-devel] [PATCH 0/4] Fixups to my wwid recheck patch

[Benjamin Marzinski]

This patchset has minor fixups that were either suggested by Martin, or
came up through the disussions about my "Handle remapped LUNs better"
patchset.

Benjamin Marzinski (4):
  libmultipath: avoid infinite loop with bad vpd page 83 identifier
  libmultipath: fix priorities in parse_vpd_pg83
  multipathd: improve getting parent udevice in rescan_path
  multipathd: don't trigger uevent for partitions on wwid change

 libmultipath/discovery.c | 16 ++++++++--------
 multipathd/main.c        | 15 +++++----------
 2 files changed, 13 insertions(+), 18 deletions(-)

-- 
2.17.2

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

[dm-devel] [PATCH 1/4] libmultipath: avoid infinite loop with bad vpd page 83 identifier

[Benjamin Marzinski]

If a device with a scsi name identifier has an unknown prefix,
parse_vpd_pg83() needs to advance to the next identifier, instead of
simply trying the same one again in an infinite loop.

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmultipath/discovery.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
index f216a724..5727f7a6 100644
--- a/libmultipath/discovery.c
+++ b/libmultipath/discovery.c
@@ -1157,7 +1157,7 @@ parse_vpd_pg83(const unsigned char *in, size_t in_len,
 			if (memcmp(d + 4, "eui.", 4) &&
 			    memcmp(d + 4, "naa.", 4) &&
 			    memcmp(d + 4, "iqn.", 4))
-				continue;
+				break;
 			if (prio < 4) {
 				prio = 4;
 				vpd = d;
-- 
2.17.2

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

[dm-devel] [PATCH 2/4] libmultipath: fix priorities in parse_vpd_pg83

[Benjamin Marzinski]

The priorities for the EUI-64 (0x02) and NAME (0x08) scsi identifiers in
parse_vpd_pg83() don't match their priorities in 55-scsi-sg3_id.rules.
Switch them so that they match.

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmultipath/discovery.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
index 5727f7a6..f8044141 100644
--- a/libmultipath/discovery.c
+++ b/libmultipath/discovery.c
@@ -1152,19 +1152,19 @@ parse_vpd_pg83(const unsigned char *in, size_t in_len,
 				vpd = d;
 			}
 			break;
-		case 0x8:
-			/* SCSI Name: Prio 4 */
-			if (memcmp(d + 4, "eui.", 4) &&
-			    memcmp(d + 4, "naa.", 4) &&
-			    memcmp(d + 4, "iqn.", 4))
-				break;
+		case 0x2:
+			/* EUI-64: Prio 4 */
 			if (prio < 4) {
 				prio = 4;
 				vpd = d;
 			}
 			break;
-		case 0x2:
-			/* EUI-64: Prio 3 */
+		case 0x8:
+			/* SCSI Name: Prio 3 */
+			if (memcmp(d + 4, "eui.", 4) &&
+			    memcmp(d + 4, "naa.", 4) &&
+			    memcmp(d + 4, "iqn.", 4))
+				break;
 			if (prio < 3) {
 				prio = 3;
 				vpd = d;
-- 
2.17.2

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

[dm-devel] [PATCH 3/4] multipathd: improve getting parent udevice in rescan_path

[Benjamin Marzinski]

Instead of looping through parents and checking, just call
udev_device_get_parent_with_subsystem_devtype() to get the
right one.

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 multipathd/main.c | 14 +++++---------
 1 file changed, 5 insertions(+), 9 deletions(-)

diff --git a/multipathd/main.c b/multipathd/main.c
index 1df69096..bc747d0e 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -822,16 +822,12 @@ ev_remove_map (char * devname, char * alias, int minor, struct vectors * vecs)
 }
 
 static void
-rescan_path(struct udev_device *parent)
+rescan_path(struct udev_device *ud)
 {
-	while(parent) {
-		const char *subsys = udev_device_get_subsystem(parent);
-		if (subsys && !strncmp(subsys, "scsi", 4))
-			break;
-		parent = udev_device_get_parent(parent);
-	}
-	if (parent)
-		sysfs_attr_set_value(parent, "rescan", "1", strlen("1"));
+	ud = udev_device_get_parent_with_subsystem_devtype(ud, "scsi",
+							   "scsi_device");
+	if (ud)
+		sysfs_attr_set_value(ud, "rescan", "1", strlen("1"));
 }
 
 void
-- 
2.17.2

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

[dm-devel] [PATCH 4/4] multipathd: don't trigger uevent for partitions on wwid change

[Benjamin Marzinski]

If the wwid changed, the device is no longer the same, so sending add
events to the devices partitions doesn't make any sense.

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 multipathd/main.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/multipathd/main.c b/multipathd/main.c
index bc747d0e..3579bad7 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -845,7 +845,6 @@ handle_path_wwid_change(struct path *pp, struct vectors *vecs)
 	}
 	rescan_path(udd);
 	sysfs_attr_set_value(udd, "uevent", "add", strlen("add"));
-	trigger_partitions_udev_change(udd, "add", strlen("add"));
 	udev_device_unref(udd);
 }
 
-- 
2.17.2

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] [PATCH 1/4] libmultipath: avoid infinite loop with bad vpd page 83 identifier

[Martin Wilck]

On Thu, 2021-03-25 at 19:52 -0500, Benjamin Marzinski wrote:
> If a device with a scsi name identifier has an unknown prefix,
> parse_vpd_pg83() needs to advance to the next identifier, instead of
> simply trying the same one again in an infinite loop.
> 
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>

Ouch.
Reviewed-by: Martin Wilck <mwilck@suse.com>

-- 
Dr. Martin Wilck <mwilck@suse.com>, Tel. +49 (0)911 74053 2107
SUSE Software Solutions Germany GmbH
HRB 36809, AG Nürnberg GF: Felix Imendörffer



--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] [PATCH 2/4] libmultipath: fix priorities in parse_vpd_pg83

[Martin Wilck]

On Thu, 2021-03-25 at 19:52 -0500, Benjamin Marzinski wrote:
> The priorities for the EUI-64 (0x02) and NAME (0x08) scsi identifiers
> in
> parse_vpd_pg83() don't match their priorities in 55-scsi-sg3_id.rules.
> Switch them so that they match.

I think we should rather change the udev rules file, to be consistent
with what the kernel does:

https://elixir.bootlin.com/linux/latest/A/ident/designator_prio

Regards
Martin



> 
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> ---
>  libmultipath/discovery.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
> index 5727f7a6..f8044141 100644
> --- a/libmultipath/discovery.c
> +++ b/libmultipath/discovery.c
> @@ -1152,19 +1152,19 @@ parse_vpd_pg83(const unsigned char *in, size_t
> in_len,
>                                 vpd = d;
>                         }
>                         break;
> -               case 0x8:
> -                       /* SCSI Name: Prio 4 */
> -                       if (memcmp(d + 4, "eui.", 4) &&
> -                           memcmp(d + 4, "naa.", 4) &&
> -                           memcmp(d + 4, "iqn.", 4))
> -                               break;
> +               case 0x2:
> +                       /* EUI-64: Prio 4 */
>                         if (prio < 4) {
>                                 prio = 4;
>                                 vpd = d;
>                         }
>                         break;
> -               case 0x2:
> -                       /* EUI-64: Prio 3 */
> +               case 0x8:
> +                       /* SCSI Name: Prio 3 */
> +                       if (memcmp(d + 4, "eui.", 4) &&
> +                           memcmp(d + 4, "naa.", 4) &&
> +                           memcmp(d + 4, "iqn.", 4))
> +                               break;
>                         if (prio < 3) {
>                                 prio = 3;
>                                 vpd = d;

-- 
Dr. Martin Wilck <mwilck@suse.com>, Tel. +49 (0)911 74053 2107
SUSE Software Solutions Germany GmbH
HRB 36809, AG Nürnberg GF: Felix Imendörffer



--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] [PATCH 3/4] multipathd: improve getting parent udevice in rescan_path

[Martin Wilck]

On Thu, 2021-03-25 at 19:52 -0500, Benjamin Marzinski wrote:
> Instead of looping through parents and checking, just call
> udev_device_get_parent_with_subsystem_devtype() to get the
> right one.
> 
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>

Reviewed-by: Martin Wilck <mwilck@suse.com>

-- 
Dr. Martin Wilck <mwilck@suse.com>, Tel. +49 (0)911 74053 2107
SUSE Software Solutions Germany GmbH
HRB 36809, AG Nürnberg GF: Felix Imendörffer



--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] [PATCH 4/4] multipathd: don't trigger uevent for partitions on wwid change

[Martin Wilck]

On Thu, 2021-03-25 at 19:52 -0500, Benjamin Marzinski wrote:
> If the wwid changed, the device is no longer the same, so sending add
> events to the devices partitions doesn't make any sense.
> 
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>

Reviewed-by: Martin Wilck <mwilck@suse.com>

> ---
>  multipathd/main.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/multipathd/main.c b/multipathd/main.c
> index bc747d0e..3579bad7 100644
> --- a/multipathd/main.c
> +++ b/multipathd/main.c
> @@ -845,7 +845,6 @@ handle_path_wwid_change(struct path *pp, struct
> vectors *vecs)
>         }
>         rescan_path(udd);
>         sysfs_attr_set_value(udd, "uevent", "add", strlen("add"));
> -       trigger_partitions_udev_change(udd, "add", strlen("add"));
>         udev_device_unref(udd);
>  }
>  


-- 
Dr. Martin Wilck <mwilck@suse.com>, Tel. +49 (0)911 74053 2107
SUSE Software Solutions Germany GmbH
HRB 36809, AG Nürnberg GF: Felix Imendörffer



--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] [PATCH 2/4] libmultipath: fix priorities in parse_vpd_pg83

[Benjamin Marzinski]

On Fri, Mar 26, 2021 at 05:12:36PM +0000, Martin Wilck wrote:
> On Thu, 2021-03-25 at 19:52 -0500, Benjamin Marzinski wrote:
> > The priorities for the EUI-64 (0x02) and NAME (0x08) scsi identifiers
> > in
> > parse_vpd_pg83() don't match their priorities in 55-scsi-sg3_id.rules.
> > Switch them so that they match.
> 
> I think we should rather change the udev rules file, to be consistent
> with what the kernel does:
> 
> https://elixir.bootlin.com/linux/latest/A/ident/designator_prio

If we were going with the kernel as a standard, we should also change
scsi_id, since it doesn't match that the kernel either (or the priority
ordering in 55-scsi-sg3_id.rules for that matter).  The bigger issue
here is that distributions would need to make special plans to take this
change, because user's devices would change uuids, which could cause
problems on existing systems. It will definitely do so on systems using
multipath. Devices will suddenly change wwids, which will rename them.
The whole reason why Red Hat installs 55-scsi-sg3_id.rules as
61-scsi-sg3_id.rules is beause we didn't initally include it, and we
don't want go changing people's device UUIDs, so is has to go after
60-persistent-storage.rules, which sets ID_SERIAL if it's not already
set.

On the other hand, it's obviously safe to make our fallback method of
getting wwids return the same wwids as the primary method does.  Since
recheck_wwids relies on both methods giving the same wwid, I would argue
that it's a bug that they don't. If we don't add this fix, then we
should add back the code that disables recheck_wwids if multipathd
doesn't see that both methods return the same wwid at least once, so we
know that we can rely on parse_vpd_pg83.

Speaking of which, I was planning on doing more work to make our
fallback method of returning wwids work more like 55-scsi-sg3_id.rules
and 60-persistent-storage.rules. For instance, both of those will fall
back to using page 0x80, if setting ID_SERIAL from page 0x83 fails.

-Ben

> 
> Regards
> Martin
> 
> 
> 
> > 
> > Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> > ---
> >  libmultipath/discovery.c | 16 ++++++++--------
> >  1 file changed, 8 insertions(+), 8 deletions(-)
> > 
> > diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
> > index 5727f7a6..f8044141 100644
> > --- a/libmultipath/discovery.c
> > +++ b/libmultipath/discovery.c
> > @@ -1152,19 +1152,19 @@ parse_vpd_pg83(const unsigned char *in, size_t
> > in_len,
> >                                 vpd = d;
> >                         }
> >                         break;
> > -               case 0x8:
> > -                       /* SCSI Name: Prio 4 */
> > -                       if (memcmp(d + 4, "eui.", 4) &&
> > -                           memcmp(d + 4, "naa.", 4) &&
> > -                           memcmp(d + 4, "iqn.", 4))
> > -                               break;
> > +               case 0x2:
> > +                       /* EUI-64: Prio 4 */
> >                         if (prio < 4) {
> >                                 prio = 4;
> >                                 vpd = d;
> >                         }
> >                         break;
> > -               case 0x2:
> > -                       /* EUI-64: Prio 3 */
> > +               case 0x8:
> > +                       /* SCSI Name: Prio 3 */
> > +                       if (memcmp(d + 4, "eui.", 4) &&
> > +                           memcmp(d + 4, "naa.", 4) &&
> > +                           memcmp(d + 4, "iqn.", 4))
> > +                               break;
> >                         if (prio < 3) {
> >                                 prio = 3;
> >                                 vpd = d;
> 
> -- 
> Dr. Martin Wilck <mwilck@suse.com>, Tel. +49 (0)911 74053 2107
> SUSE Software Solutions Germany GmbH
> HRB 36809, AG Nürnberg GF: Felix Imendörffer
> 

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] [PATCH 2/4] libmultipath: fix priorities in parse_vpd_pg83

[Martin Wilck]

On Fri, 2021-03-26 at 21:18 -0500, Benjamin Marzinski wrote:
> On Fri, Mar 26, 2021 at 05:12:36PM +0000, Martin Wilck wrote:
> > On Thu, 2021-03-25 at 19:52 -0500, Benjamin Marzinski wrote:
> > > The priorities for the EUI-64 (0x02) and NAME (0x08) scsi
> > > identifiers
> > > in
> > > parse_vpd_pg83() don't match their priorities in 55-scsi-
> > > sg3_id.rules.
> > > Switch them so that they match.
> > 
> > I think we should rather change the udev rules file, to be
> > consistent
> > with what the kernel does:
> > 
> > https://elixir.bootlin.com/linux/latest/A/ident/designator_prio
> 
> If we were going with the kernel as a standard, we should also change
> scsi_id, since it doesn't match that the kernel either (or the
> priority
> ordering in 55-scsi-sg3_id.rules for that matter).  The bigger issue
> here is that distributions would need to make special plans to take
> this
> change, because user's devices would change uuids, which could cause
> problems on existing systems. It will definitely do so on systems
> using
> multipath. Devices will suddenly change wwids, which will rename
> them.

True. (Although this would happen only to devices that provide both
type 8 (SCSI name string) _and_ either type 3 (NAA) or type 2 (EUI-64)
designators. I'm not sure how many of those exist)

I wish we'd been consistent in the first place...

The preference for type 8 in the kernel dates back to Hannes' patch
from 2015: 9983bed3907c ("scsi: Add scsi_vpd_lun_id()"). It makes sense
in general: while the specs don't explicitly mandate preferences, SPC-4
lists the designators in what seems to be a "increasing priority"
order, and the "name string" format is listed last. Also, "name string"
has the same or even stricter formal requirements than the other types,
while allowing iSCSI name strings besides NAA and EUI-64 identifiers.

> The whole reason why Red Hat installs 55-scsi-sg3_id.rules as
> 61-scsi-sg3_id.rules is beause we didn't initally include it, and we
> don't want go changing people's device UUIDs, so is has to go after
> 60-persistent-storage.rules, which sets ID_SERIAL if it's not already
> set.
> 
> On the other hand, it's obviously safe to make our fallback method of
> getting wwids return the same wwids as the primary method does. 

Ack.

> Since
> recheck_wwids relies on both methods giving the same wwid, I would
> argue
> that it's a bug that they don't. If we don't add this fix, then we
> should add back the code that disables recheck_wwids if multipathd
> doesn't see that both methods return the same wwid at least once, so
> we
> know that we can rely on parse_vpd_pg83.

I was hoping that we could eventually settle on the kernel's priority
list. By doing so, we could perhaps some day actually rely on the
kernel's "wwid" attribute rather than logic of the udev rules (IOW:
change the udev rules to simply use "wwid"). I believe being able to do
so would also be in the interest of SID, am I wrong?

For now, I suppose you're right - it's more important to retain
backward compatibility. So, I'm going to ACK your patch. But we should
try to figure out an exit strategy for this, if possible without adding
yet another configuration option for device detection in multipath-
tools.

As a first step, we could change the udev rules to set a device
property based on the kernel's "wwid" sysfs attribute. For now, that
property would just be yet one more ID attribute. We could make the
rest of the udev rule logic depend on a conditional which distributions
could control. I.e. "modern" setups would _only_ use the "wwid"
attribute and not set ID_SERIAL any more. Distros with major interest
in backward compatibilty can keep ID_SERIAL as long as they see fit.

multipathd could figure out the system configuration from the (non)
availability of certain properties, and use an appropriate fallback
logic for either case.

> 
> Speaking of which, I was planning on doing more work to make our
> fallback method of returning wwids work more like 55-scsi-
> sg3_id.rules
> and 60-persistent-storage.rules. For instance, both of those will
> fall
> back to using page 0x80, if setting ID_SERIAL from page 0x83 fails.

Yes, that'd be a good thing.

I think I've made my point that I'd like to achieve consistent behavior
between kernel, udev, and multipathd some day. We should have a plan,
and should make sure the kernel people are on the same boat as us.
I think I'll write to linux-scsi to discuss how we can best proceed.

Best regards,
Martin

> 
> -Ben
> 
> > 
> > Regards
> > Martin
> > 
> > 
> > 
> > > 
> > > Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> > > ---
> > >  libmultipath/discovery.c | 16 ++++++++--------
> > >  1 file changed, 8 insertions(+), 8 deletions(-)
> > > 
> > > diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
> > > index 5727f7a6..f8044141 100644
> > > --- a/libmultipath/discovery.c
> > > +++ b/libmultipath/discovery.c
> > > @@ -1152,19 +1152,19 @@ parse_vpd_pg83(const unsigned char *in,
> > > size_t
> > > in_len,
> > >                                 vpd = d;
> > >                         }
> > >                         break;
> > > -               case 0x8:
> > > -                       /* SCSI Name: Prio 4 */
> > > -                       if (memcmp(d + 4, "eui.", 4) &&
> > > -                           memcmp(d + 4, "naa.", 4) &&
> > > -                           memcmp(d + 4, "iqn.", 4))
> > > -                               break;
> > > +               case 0x2:
> > > +                       /* EUI-64: Prio 4 */
> > >                         if (prio < 4) {
> > >                                 prio = 4;
> > >                                 vpd = d;
> > >                         }
> > >                         break;
> > > -               case 0x2:
> > > -                       /* EUI-64: Prio 3 */
> > > +               case 0x8:
> > > +                       /* SCSI Name: Prio 3 */
> > > +                       if (memcmp(d + 4, "eui.", 4) &&
> > > +                           memcmp(d + 4, "naa.", 4) &&
> > > +                           memcmp(d + 4, "iqn.", 4))
> > > +                               break;
> > >                         if (prio < 3) {
> > >                                 prio = 3;
> > >                                 vpd = d;
> > 
> > -- 
> > Dr. Martin Wilck <mwilck@suse.com>, Tel. +49 (0)911 74053 2107
> > SUSE Software Solutions Germany GmbH
> > HRB 36809, AG Nürnberg GF: Felix Imendörffer
> > 
> 

-- 
Dr. Martin Wilck <mwilck@suse.com>, Tel. +49 (0)911 74053 2107
SUSE Software Solutions Germany GmbH
HRB 36809, AG Nürnberg GF: Felix Imendörffer



--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] [PATCH 2/4] libmultipath: fix priorities in parse_vpd_pg83

[Martin Wilck]

On Thu, 2021-03-25 at 19:52 -0500, Benjamin Marzinski wrote:
> The priorities for the EUI-64 (0x02) and NAME (0x08) scsi identifiers
> in
> parse_vpd_pg83() don't match their priorities in 55-scsi-
> sg3_id.rules.
> Switch them so that they match.
> 
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>

After further discussion:

Reviewed-by: Martin Wilck <mwilck@suse.com>

-- 
Dr. Martin Wilck <mwilck@suse.com>, Tel. +49 (0)911 74053 2107
SUSE Software Solutions Germany GmbH
HRB 36809, AG Nürnberg GF: Felix Imendörffer





--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] [PATCH 2/4] libmultipath: fix priorities in parse_vpd_pg83

[Benjamin Marzinski]

On Mon, Mar 29, 2021 at 08:44:46AM +0000, Martin Wilck wrote:
> On Fri, 2021-03-26 at 21:18 -0500, Benjamin Marzinski wrote:
> > On Fri, Mar 26, 2021 at 05:12:36PM +0000, Martin Wilck wrote:
> > > On Thu, 2021-03-25 at 19:52 -0500, Benjamin Marzinski wrote:
> > > > The priorities for the EUI-64 (0x02) and NAME (0x08) scsi
> > > > identifiers
> > > > in
> > > > parse_vpd_pg83() don't match their priorities in 55-scsi-
> > > > sg3_id.rules.
> > > > Switch them so that they match.
> > > 
> > > I think we should rather change the udev rules file, to be
> > > consistent
> > > with what the kernel does:
> > > 
> > > https://elixir.bootlin.com/linux/latest/A/ident/designator_prio
> > 
> > If we were going with the kernel as a standard, we should also change
> > scsi_id, since it doesn't match that the kernel either (or the
> > priority
> > ordering in 55-scsi-sg3_id.rules for that matter).  The bigger issue
> > here is that distributions would need to make special plans to take
> > this
> > change, because user's devices would change uuids, which could cause
> > problems on existing systems. It will definitely do so on systems
> > using
> > multipath. Devices will suddenly change wwids, which will rename
> > them.
> 
> True. (Although this would happen only to devices that provide both
> type 8 (SCSI name string) _and_ either type 3 (NAA) or type 2 (EUI-64)
> designators. I'm not sure how many of those exist)
> 
> I wish we'd been consistent in the first place...
> 
> The preference for type 8 in the kernel dates back to Hannes' patch
> from 2015: 9983bed3907c ("scsi: Add scsi_vpd_lun_id()"). It makes sense
> in general: while the specs don't explicitly mandate preferences, SPC-4
> lists the designators in what seems to be a "increasing priority"
> order, and the "name string" format is listed last. Also, "name string"
> has the same or even stricter formal requirements than the other types,
> while allowing iSCSI name strings besides NAA and EUI-64 identifiers.
> 
> > The whole reason why Red Hat installs 55-scsi-sg3_id.rules as
> > 61-scsi-sg3_id.rules is beause we didn't initally include it, and we
> > don't want go changing people's device UUIDs, so is has to go after
> > 60-persistent-storage.rules, which sets ID_SERIAL if it's not already
> > set.
> > 
> > On the other hand, it's obviously safe to make our fallback method of
> > getting wwids return the same wwids as the primary method does. 
> 
> Ack.
> 
> > Since
> > recheck_wwids relies on both methods giving the same wwid, I would
> > argue
> > that it's a bug that they don't. If we don't add this fix, then we
> > should add back the code that disables recheck_wwids if multipathd
> > doesn't see that both methods return the same wwid at least once, so
> > we
> > know that we can rely on parse_vpd_pg83.
> 
> I was hoping that we could eventually settle on the kernel's priority
> list. By doing so, we could perhaps some day actually rely on the
> kernel's "wwid" attribute rather than logic of the udev rules (IOW:
> change the udev rules to simply use "wwid"). I believe being able to do
> so would also be in the interest of SID, am I wrong?

You're right. I would love to get away from the arbitrariness of the
udev rules for picking the WWID, and SID would definitely benefit from
a standard, non-udev way of getting the WWID.
 
> For now, I suppose you're right - it's more important to retain
> backward compatibility. So, I'm going to ACK your patch. But we should
> try to figure out an exit strategy for this, if possible without adding
> yet another configuration option for device detection in multipath-
> tools.
>
> As a first step, we could change the udev rules to set a device
> property based on the kernel's "wwid" sysfs attribute. For now, that
> property would just be yet one more ID attribute. We could make the
> rest of the udev rule logic depend on a conditional which distributions
> could control. I.e. "modern" setups would _only_ use the "wwid"
> attribute and not set ID_SERIAL any more. Distros with major interest
> in backward compatibilty can keep ID_SERIAL as long as they see fit.
> 
> multipathd could figure out the system configuration from the (non)
> availability of certain properties, and use an appropriate fallback
> logic for either case.

That seems like reasonable first step, although one that won't help SID,
since it can't rely on getting the wwid from udev.  This actually brings
up a different point I have. Is your main objection to adding more
config options that it is complicating the code, or confusing the users?

Because multipath wouldn't need to add any configuration options to be
easily usable with SID (the current workaround, setting uid_attribute to
"", is pretty non-obvious to users) if it could just check if sid was
running, and key off that.  However this adds even more code complexity
than simply checking a config option. I don't know how you would feel
about accepting a patch that does this, when SID is production ready.
 
> > 
> > Speaking of which, I was planning on doing more work to make our
> > fallback method of returning wwids work more like 55-scsi-
> > sg3_id.rules
> > and 60-persistent-storage.rules. For instance, both of those will
> > fall
> > back to using page 0x80, if setting ID_SERIAL from page 0x83 fails.
> 
> Yes, that'd be a good thing.
> 
> I think I've made my point that I'd like to achieve consistent behavior
> between kernel, udev, and multipathd some day. We should have a plan,
> and should make sure the kernel people are on the same boat as us.
> I think I'll write to linux-scsi to discuss how we can best proceed.
> 
> Best regards,
> Martin
> 
> > 
> > -Ben
> > 
> > > 
> > > Regards
> > > Martin
> > > 
> > > 
> > > 
> > > > 
> > > > Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> > > > ---
> > > >  libmultipath/discovery.c | 16 ++++++++--------
> > > >  1 file changed, 8 insertions(+), 8 deletions(-)
> > > > 
> > > > diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
> > > > index 5727f7a6..f8044141 100644
> > > > --- a/libmultipath/discovery.c
> > > > +++ b/libmultipath/discovery.c
> > > > @@ -1152,19 +1152,19 @@ parse_vpd_pg83(const unsigned char *in,
> > > > size_t
> > > > in_len,
> > > >                                 vpd = d;
> > > >                         }
> > > >                         break;
> > > > -               case 0x8:
> > > > -                       /* SCSI Name: Prio 4 */
> > > > -                       if (memcmp(d + 4, "eui.", 4) &&
> > > > -                           memcmp(d + 4, "naa.", 4) &&
> > > > -                           memcmp(d + 4, "iqn.", 4))
> > > > -                               break;
> > > > +               case 0x2:
> > > > +                       /* EUI-64: Prio 4 */
> > > >                         if (prio < 4) {
> > > >                                 prio = 4;
> > > >                                 vpd = d;
> > > >                         }
> > > >                         break;
> > > > -               case 0x2:
> > > > -                       /* EUI-64: Prio 3 */
> > > > +               case 0x8:
> > > > +                       /* SCSI Name: Prio 3 */
> > > > +                       if (memcmp(d + 4, "eui.", 4) &&
> > > > +                           memcmp(d + 4, "naa.", 4) &&
> > > > +                           memcmp(d + 4, "iqn.", 4))
> > > > +                               break;
> > > >                         if (prio < 3) {
> > > >                                 prio = 3;
> > > >                                 vpd = d;
> > > 
> > > -- 
> > > Dr. Martin Wilck <mwilck@suse.com>, Tel. +49 (0)911 74053 2107
> > > SUSE Software Solutions Germany GmbH
> > > HRB 36809, AG Nürnberg GF: Felix Imendörffer
> > > 
> > 
> 
> -- 
> Dr. Martin Wilck <mwilck@suse.com>, Tel. +49 (0)911 74053 2107
> SUSE Software Solutions Germany GmbH
> HRB 36809, AG Nürnberg GF: Felix Imendörffer
> 

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] [PATCH 2/4] libmultipath: fix priorities in parse_vpd_pg83

[Martin Wilck]

On Mon, 2021-03-29 at 13:20 -0500, Benjamin Marzinski wrote:
> > 
> > multipathd could figure out the system configuration from the (non)
> > availability of certain properties, and use an appropriate fallback
> > logic for either case.
> 
> That seems like reasonable first step, although one that won't help
> SID,
> since it can't rely on getting the wwid from udev. 

Can you conceive of a different approach that would be better for SID?
I'd like to hear about it.

>  This actually brings
> up a different point I have. Is your main objection to adding more
> config options that it is complicating the code, or confusing the
> users?

Both, with emphasis on the latter. I'm quite positive that we have too
many options already. That doesn't mean I would generally oppose new
options if they make sense. We should rather try to get rid of some new
ones that nobody uses any more.

> Because multipath wouldn't need to add any configuration options to
> be
> easily usable with SID (the current workaround, setting uid_attribute
> to
> "", is pretty non-obvious to users) if it could just check if sid was
> running, and key off that.  However this adds even more code
> complexity
> than simply checking a config option. I don't know how you would feel
> about accepting a patch that does this, when SID is production ready.

I could live well with this autodetection. I think it would be better
than doing the same thing with a configuration options.

Regards,
Martin


-- 
Dr. Martin Wilck <mwilck@suse.com>, Tel. +49 (0)911 74053 2107
SUSE Software Solutions Germany GmbH
HRB 36809, AG Nürnberg GF: Felix Imendörffer



--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] [PATCH 2/4] libmultipath: fix priorities in parse_vpd_pg83

[Benjamin Marzinski]

On Mon, Mar 29, 2021 at 07:08:14PM +0000, Martin Wilck wrote:
> On Mon, 2021-03-29 at 13:20 -0500, Benjamin Marzinski wrote:
> > > 
> > > multipathd could figure out the system configuration from the (non)
> > > availability of certain properties, and use an appropriate fallback
> > > logic for either case.
> > 
> > That seems like reasonable first step, although one that won't help
> > SID,
> > since it can't rely on getting the wwid from udev. 
> 
> Can you conceive of a different approach that would be better for SID?
> I'd like to hear about it.

I assume that we will continue to have a fallback function to get the
wwid, that will get the proper wwid. As long as you are O.k. with
multipath autodetecting if SID is running, it can just continue to use
the fallback method, so this should be fine.

> >  This actually brings
> > up a different point I have. Is your main objection to adding more
> > config options that it is complicating the code, or confusing the
> > users?
> 
> Both, with emphasis on the latter. I'm quite positive that we have too
> many options already. That doesn't mean I would generally oppose new
> options if they make sense. We should rather try to get rid of some new
> ones that nobody uses any more.
> 
> > Because multipath wouldn't need to add any configuration options to
> > be
> > easily usable with SID (the current workaround, setting uid_attribute
> > to
> > "", is pretty non-obvious to users) if it could just check if sid was
> > running, and key off that.  However this adds even more code
> > complexity
> > than simply checking a config option. I don't know how you would feel
> > about accepting a patch that does this, when SID is production ready.
> 
> I could live well with this autodetection. I think it would be better
> than doing the same thing with a configuration options.
> 
> Regards,
> Martin
> 
> 
> -- 
> Dr. Martin Wilck <mwilck@suse.com>, Tel. +49 (0)911 74053 2107
> SUSE Software Solutions Germany GmbH
> HRB 36809, AG Nürnberg GF: Felix Imendörffer
> 

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel