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(-) > > -- dm-devel mailing list dm-devel@xxxxxxxxxx https://listman.redhat.com/mailman/listinfo/dm-devel
