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@xxxxxxxxxx> > > > > --- > > > > 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@xxxxxxxx>, Tel. +49 (0)911 74053 2107 > > > SUSE Software Solutions Germany GmbH > > > HRB 36809, AG Nürnberg GF: Felix Imendörffer > > > > > > > -- > Dr. Martin Wilck <mwilck@xxxxxxxx>, 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@xxxxxxxxxx https://listman.redhat.com/mailman/listinfo/dm-devel
