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

[Benjamin Marzinski <bmarzins@redhat.com>]

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@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?

> 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:

1: udev hasn't gotten an event for a device
2: udev got an event but timed out or failed while processing it
3: udev successfully processed the event for the device, but multipath
   isn't seeing the attributes it was expecting.

We could react more sensibly.

-Ben

> IMO we need a clear definition of the INIT_xyz states and their
> transitions. We won't get along with intuitive logic any more.
> 
> Cheers,
> Martin
> 
> (*) my test procedure is simply to delete the paths' files from
> /run/udev/data and to restart multipathd.

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