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
