[dm-devel] [PATCH 5/7] multipathd: fully initialize paths added by update_pathvec_from_dm

Mon Nov 8 16:29:55 UTC 2021

On Fri, Nov 05, 2021 at 11:20:01PM +0000, Martin Wilck wrote:
> On Fri, 2021-11-05 at 16:49 -0500, Benjamin Marzinski wrote:
> > On Fri, Nov 05, 2021 at 10:55:11AM +0000, Martin Wilck wrote:
> > > Hi Ben,
> > > 
> > > On Thu, 2021-11-04 at 23:10 +0100, Martin Wilck wrote:
> > > > On Wed, 2021-10-20 at 14:15 -0500, Benjamin Marzinski wrote:
> > > > > ...
> > > > 
> > > > > 
> > > > > Multipath now has a new state to deal with these devices,
> > > > > INIT_PARTIAL.
> > > > > Devices in this state are treated mostly like INIT_OK devices,
> > > > > but
> > > > > when "multipathd add path" is called or an add/change uevent
> > > > > happens
> > > > > on these devices, multipathd will finish initializing them, and
> > > > > remove
> > > > > them if necessary.
> > > > > 
> > > > > Signed-off-by: Benjamin Marzinski <bmarzins at redhat.com>
> > > > 
> > > > Nice. Somehow in my mind the issue always look much more complex.
> > > > I like this, but I want to give it some testing before I finally
> > > > ack
> > > > it.
> > > 
> > > I noted that running "multipathd add path $path" for a path in
> > > INIT_PARTIAL state changes this paths's init state to INIT_OK, even
> > > if
> > > the udev still has no information about it (*).
> > 
> > Ah. Didn't think about that.
> >  
> > > The reason is that pp->wwid is set, and thus pathinfo() won't even
> > > try
> > > to retrieve the WWID, although for INIT_PARTIAL paths the origin of
> > > the
> > > WWID is not 100% trustworthy (being just copied from pp->mpp-
> > > > wwid). pathinfo() will return PATHINFO_OK without having retrieved
> > > > the
> > > WWID. 
> > > 
> > > I suppose we could apply a similar logic as in uev_update_path()
> > > here,
> > > clearing pp->wwid before calling pathinfo(). If udev was still
> > > unaware
> > > of the path, this would mean a transition from INIT_PARTIAL to
> > > INIT_MISSING_UDEV. OTOH, we'd now need to be prepared to have to
> > > remove
> > > the path from the map if the WWID doesn't match after the call to
> > > pathinfo(). This means we'd basically need to reimplement the
> > > "changed
> > > WWID" logic from uev_update_path(), and thus we'd need to unify both
> > > code paths.
> > > 
> > > In general, the difference between INIT_MISSING_UDEV and INIT_PARTIAL
> > > is subtle. Correct me if I'm wrong, but INIT_PARTIAL means "udev has
> > > no
> > > information about this device" and INIT_MISSING_UDEV currently means
> > > "we weren't able to figure out a WWID for the path" (meaning that
> > > INIT_MISSING_UDEV is a misnomer - when fallback are allowed,
> > > INIT_MISSING_UDEV is actually independent of the device's state in
> > > the
> > > udev db. We should rename this to INIT_MISSING_WWID, perhaps).
> > 
> > Yeah. makes sense.
> > 
> > > The
> > > semantics of the two states are very different though:
> > > INIT_MISSING_UDEV will trigger an attempt to regenerate an uevent,
> > > whereas INIT_PARTIAL will just stick passively. IMO it'd make sense
> > > to
> > > trigger an uevent in the INIT_PARTIAL case, too, albeit perhaps not
> > > immediately (after the default uevent timeout - 180s?). 
> > 
> > Sure. We do want to wait long enough to be fairly sure that udev isn't
> > just being a little bit slow.
> > 
> > This would also handle the case where multipathd simply wasn't
> > tracking the path for some reason. If the device doesn't exist in the
> > udev database at all, do with send an "add" event instead of a "change"
> > event?
> 
> I don't think it matters. If no database file exists, udev can still
> deliver some information:
> 
> # udevadm info /dev/sde
> P: /devices/pci0000:00/0000:00:01.0/0000:01:00.7/host3/rport-3:0-
> 0/target3:0:0/3:0:0:4/block/sde
> N: sde
> L: 0
> E: DEVPATH=/devices/pci0000:00/0000:00:01.0/0000:01:00.7/host3/rport-
> 3:0-0/target3:0:0/3:0:0:4/block/sde
> E: DEVNAME=/dev/sde
> E: DEVTYPE=disk
> E: MAJOR=8
> E: MINOR=64
> E: SUBSYSTEM=block
> 
> (note USEC_INITIALIZED is not in the set)
> 
> If I run "udevadm trigger -c change /dev/sde" in this situation, I'll
> get the full info, as if I had run "add" before (some rules may differ
> between "add" and "change", but that's quite unusual).
>

udev rules may not change much, but, for instance, multipathd reacts
differently to add and change events.  So there may be other consumers
of uevents that care about the difference between add and change events.

> > 
> > > Also, we currently don't track the udev state of devices cleanly.
> > > We
> > > set INIT_MISSING_UDEV if we can't obtain uid_attribute, which
> > > doesn't
> > > necessarily mean that udev is unaware of the device. I believe we
> > > should e.g. check the USEC_INITIALIZED property - it is non-NULL if
> > > and
> > > only if the device is present in the udev db. If uid_attribute
> > > isn't
> > > set regardless, we could conclude that the udev rules just don't
> > > set
> > > it. It might make sense to retrigger *one* uevent in that case, but
> > > no
> > > more.
> > 
> > IIRC, the initial reason why INIT_MISSING_UDEV was added was because
> > sometimes udev workers timed out, causing them to not run all the
> > rules.
> > Do you know offhand if USEC_INITIALIZED is set in this case? If we
> > could
> > differentiate between the following states:
> 
> udevd sets this property very early, when it first receives an uevent.
> But libudev calls won't return it until the database file is written,
> which happens last, after all rules and RUN statements have been
> processed, _in the worker_. Thus if the worker is killed, the file
> won't be written.
> 
> 
> 0. the device doesn't exist in sysfs 

We simply delete devices that don't exist in sysfs, right? If we get a
non-remove uevent for a device, but it doesn't exist in sysfs, then I
would assume that a remove uevent will be shortly incomming.

> > 1: udev hasn't gotten an event for a device
> 
> I don't think we can detect this without listening for kernel uevents.
> And even if we listen to them, we could have missed some. udevd doesn't
> have an API for it, either, AFAIK.

Isn't this the most common INIT_PARTIAL case, where we are waiting for
the coldplug uevent? If there is no database file when we are processing
the device, we are in this state. Correct? 

> > 2: udev got an event but timed out or failed while processing it
> 
> This would be the USEC_INITIALIZED case, IMO

If udev has, in the past, successfully processed an event for a device,
but fails while processing a later uevent, it doesn't keep th data from
the previous event. So it could lose the uid_attribute. But the
database file should still exist. Correct?

-Ben

> > 3: udev successfully processed the event for the device, but
> > multipath
> >    isn't seeing the attributes it was expecting.
> > 
> > We could react more sensibly.
> 
> Yes.
> 
> Regards
> Martin