[dm-devel] [PATCH 2/4] libmultipath: fix priorities in parse_vpd_pg83
Martin Wilck
martin.wilck at suse.com
Mon Mar 29 08:44:46 UTC 2021
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 at 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 at 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 at suse.com>, Tel. +49 (0)911 74053 2107
SUSE Software Solutions Germany GmbH
HRB 36809, AG Nürnberg GF: Felix Imendörffer
More information about the dm-devel
mailing list