Re: [PATCH 0/8] multipath fixes to tableless device handling

[Benjamin Marzinski <bmarzins@redhat.com>]

On Sat, Nov 09, 2024 at 12:03:33AM +0100, Martin Wilck wrote:
> On Fri, 2024-11-08 at 17:08 -0500, Benjamin Marzinski wrote:
> > On Thu, Nov 07, 2024 at 07:02:11PM +0100, Martin Wilck wrote:
> > > On Thu, 2024-11-07 at 12:43 -0500, Benjamin Marzinski wrote:
> > > > On Thu, Nov 07, 2024 at 01:20:09PM +0100, Martin Wilck wrote:
> > > > > 
> > > > > Thinking about it, I believe we should actually accept devices
> > > > > that
> > > > > have an mpath UUID and an empty table, and add them to our
> > > > > mpvec.
> > > > > We
> > > > > should treat them like devices that have a multipath target but
> > > > > no
> > > > > path
> > > > > devices. If any matching paths become available, the map will
> > > > > be
> > > > > reloaded and the issue will be fixed. IMO, this is less prone
> > > > > to
> > > > > race
> > > > > conditions than trying to delete and re-add the devices.
> > > > 
> > > > I'm not sure off the top of my head how easy it will be to handle
> > > > devices with no dm table at all, but I'll take a look into this.
> > > 
> > > I tried this on a system with 0.9.9 yesterday. 0.9.9 will not add
> > > the
> > > map into the mpvec. But in coalesce_paths(), it will just reload
> > > the
> > > map:
> > 
> > Given that the code already does the right thing without needing any
> > changes to handle tableless maps makes me feel like just removing the
> > final patch is the best idea.
> 
> If we do this, do we still need the special case for DMP_BAD_DEV? IMO
> we just need to make sure that dm_get_maps() doesn't return an error if
> it encounters an empty map.

Depends. If we fail attempting to create a device, we could have raced
with something else attempting to create it.  Just checking for
dm_map_present() before we remove the device won't distinguish between
these two cases at all.

On the other hand, there is still a window where the program that we
raced with is in the middle of creating the device, but it doesn't have
a live table yet. In this case, even the DMP_BAD_DEV code won't be able
to distinguish between a map that didn't get created correctly and one
that is in the process of getting created. It would reduce the window
where we could accidentally delete a working device, however.

Since it doesn't add much complexity, I lean towards keeping it, but it
is one more state we need to handle on returns from libmp_mapinfo(), so
I'm open to an argument that it's not worth it.
 
> > The only issue with the current code is that if you have a device
> > with
> > no table with the UUID of a multipath device but a different name
> > than
> > that multipath device should have, multipath will try to create a new
> > device instead of reload it, and this fails, since the UUID is
> > already
> > in use.
> 
> That should be handled in coalesce_paths(). It's basically the scenario
> that occurs if we enable or disable user_friendly names.

Except that we have a mpp for the other device if it has a table. If the
device doesn't have a table, then a mpp isn't created, so
coalesce_paths() doesn't track it. We could add code to create mpps for
these tableless devices, but it's another trade off of added complexity
to handle a rare corner case, and I'm not sure this one is worth it.

-Ben
 
> > That should probably be handled by making the multipath code pay more
> > attention to the device UUID and less to the device name, but that's
> > work for a different patchset. 
> 
> Yes, definitely.
> 
> Martin