[dm-devel] [PATCH 0/7] multipathd: remove udev settle dependency

[Benjamin Marzinski]

On Thu, Nov 04, 2021 at 10:00:11PM +0000, Martin Wilck wrote:
> On Wed, 2021-10-20 at 14:15 -0500, Benjamin Marzinski wrote:
> > So, it turns out that commit 4ef67017 "libmultipath: add
> > update_pathvec_from_dm()" already does most of the hard work of making
> > multipath handle the uninitialized paths that exist during boot, after
> > the switch root, but before the all the coldplug uevents have been
> > processed. The only problem is that this code leaves the paths in a
> > partially initialized state, which doesn't completely get corrected
> > until
> > a reconfigure happens. 
> > 
> > This patchset makes multipath track these partially initailized paths,
> > and makes sure that they get fully initialized, or removed, as
> > necessary.
> > 
> > The first patch is a tangentially related bug I found when trying
> > (unsuccessfully) to recreate multipathd's problem with dropping
> > uninitialized paths. Multipathd was not removing completely missing
> > paths from maps that it found while starting up. The solution I chose
> > was just to make sure that a full reload will happen on these maps,
> > even
> > if a weak reconfigure was requested. However, this means that multipath
> > still completely ignores these missing paths. A side effect of this is
> > that "multipath -l" does not show these paths, even though they exist
> > as
> > part of the multipath table. The full reloads are necessary, to take
> > care of issues that update_pathvec_from_dm() can run into, but we might
> > also want to think about marking these missing paths as INIT_REMOVED,
> > instead of not adding them at all. This would make "multipath -l" still
> > show them, until they actually get removed from the table.
> 
> Yes, that might be a good thing. Completely missing paths (not existing
> in sysfs) in a map represent a dangerous condition; it would be good if
> multipath -l was able to tell the user about it.

Sure, but I can do that as a separate patch?
 
> > 
> > Patch 0005 makes sure to fully initialize the paths when their coldplug
> > event occurs, but if the path is already fully initialized in udev, but
> > multipathd finds out about it from update_pathvec_from_dm(), multipath
> > doesn't do anything to finish initializing the path and moving it to
> > INIT_OK, besides waiting for a uevent or a reconfigure. This is
> > probably
> > fine, since the only way I can see for a path to be in this state is
> > for
> > someone to manually remove the path from multipathd monitoring. But if
> > I'm missing some other way that paths could end up in this state, we
> > could try proactively finishing the initalization of INIT_PARTIAL paths
> > that have all their udev information.
> 
> One option would be to try finishing the initialization in the path
> checker.

Yep. Something like that is what I was thinking.  But I'm still trying
to come up with a way that paths could get into this state without
someone running something like:

# multipathd del path sda
# mutipath

And I'm not sure how much code we want to add just to handle something
contrived like this.

> What about the checker, in general?  Should we take some care that
> partially initialized paths aren't mistakenly set as failed? I'm not
> sure if libudev is able to return a proper fd from
> udev_device_get_devnode() if the device isn't yet initialized in the
> udev db. Without a proper fd, the checker is doomed to fail. And other
> failure modes are possible too without proper udev initialization, I
> suppose?
> 

Since this is a case where the device nodes do exist, since they were
already made into multipath devices, I didn't run into any problems with
checking INIT_PARTIAL paths.

-Ben

> > 
> > I'm also kind of on the fence about patch 0006. There is no reason
> > why
> > we have to remove INIT_PARTIAL paths if the multipath device goes
> > away.
> > But if a path is in INIT_PARTIAL for long enough that the multipath
> > device gets removed, then it's likely not a path we want to be
> > monitoring, and if a uevent occurs, we'll add it back, anyway. Also,
> > knowing that INIT_PARTIAL paths are always part of multipath devices
> > does make the code simpler.
> > 
> > I've tested these patches both by rebooting with necessary and
> > unnecessary multipath devices in the initramfs and multipathd.service
> > set to make multipathd start up at various points after the switch
> > root,
> > and by manually sending remove uevents to unintialize some paths, and
> > then starting multipathd to test specific combinations of path
> > states.
> > But more testing is always welcome.
> 
> I'll try to give this code a test shortly.
> 
> Cheers,
> Martin
> 
> 
> > 
> > Benjamin Marzinski (7):
> >   multipathd: remove missing paths on startup
> >   libmultipath: skip unneeded steps to get path name
> >   libmultipath: don't use fallback wwid in update_pathvec_from_dm
> >   libmultipath: always set INIT_REMOVED in set_path_removed
> >   multipathd: fully initialize paths added by update_pathvec_from_dm
> >   multipathd: remove INIT_PARTIAL paths that aren't in a multipath
> >     device
> >   multipathd: Remove dependency on systemd-udev-settle.service
> > 
> >  libmultipath/configure.c      |  2 ++
> >  libmultipath/devmapper.c      |  2 ++
> >  libmultipath/discovery.c      |  7 ++---
> >  libmultipath/discovery.h      |  2 ++
> >  libmultipath/structs.h        |  7 +++++
> >  libmultipath/structs_vec.c    | 40 ++++++++++++++++-------------
> >  multipathd/cli_handlers.c     |  4 +++
> >  multipathd/main.c             | 48 ++++++++++++++++++++++++++++++++-
> > --
> >  multipathd/main.h             |  1 +
> >  multipathd/multipathd.service |  3 +--
> >  10 files changed, 90 insertions(+), 26 deletions(-)
> >