| Message ID | 20241031183301.391416-1-bmarzins@redhat.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | multipath fixes to tableless device handling | expand |
On Thu, 2024-10-31 at 14:32 -0400, Benjamin Marzinski wrote: > This patchset include a couple of miscellaneous cleanups, but is > mostly > in response to issues brought up in > https://github.com/opensvc/multipath-tools/issues/100 > It adds auto restarting to the multipathd.service unit file. I'm > fairly > conflicted about the correct limits to be placed on these restarts, > so > if anyone has strong opinions and a good argument, I'm more than > willing > to change them. > > The bulk of the changes deal with how multipath handles devices > without > any table. Multipath was supposed to delete these if they were left > behind after a failed device creation, but that code was broken. > However devices aren't typically left behind after failed creates, so > it > didn't matter. > > A bigger issue is that multipathd and multipath could fail if > tableless > devices were present. With this patchset, they will simply ignore > tableless devices that don't have a multipath prefixed DM UUID. The > last > patch is a RFC patch that changes the behavior for tableless devices > that do have a multipath prefixed DM UUID. It makes multipath remove > these devices on the grounds that they are likely failed creates. > However, looking at the libdevmapper code, I'm not sure that we > actually > want to do this. When a DM_DEVICE_CREATE task is run, it will first > create a tableless device, and then immediately load the table and > resume the device. So it's possible any that tableless devices which > multipath sees are actually in the process of getting created. I left > the patch as part of the set, but I'm not sure that removing the > devices > is the right answer here. I haven't ever seen any tableless multipath > devices lying around (and I couldn't force any to get created when I > tried). Unless other people have seen these, then I don't think the > final patch of this set should go in. > Does it do any harm if we just keep these devices around? If there are path devices around with matching UUIDs, the right thing to do for multipathd would be to reload the map's table with an appropriate multipath target. Otherwise, the map will just float around empty and do no harm. This is how multipathd 0.9.9 and earlier behave, and I see no issue with that. It'd be much worse if someone created a table with an mpath-style UUID and name but an incompatible immutable target type. But in that case, again, we should probably just keep our hands off that map. Martin
On Thu, 2024-10-31 at 14:32 -0400, Benjamin Marzinski wrote: > This patchset include a couple of miscellaneous cleanups, but is > mostly > in response to issues brought up in > https://github.com/opensvc/multipath-tools/issues/100 > It adds auto restarting to the multipathd.service unit file. I'm > fairly > conflicted about the correct limits to be placed on these restarts, > so > if anyone has strong opinions and a good argument, I'm more than > willing > to change them. > > The bulk of the changes deal with how multipath handles devices > without > any table. Multipath was supposed to delete these if they were left > behind after a failed device creation, but that code was broken. > However devices aren't typically left behind after failed creates, so > it > didn't matter. > > A bigger issue is that multipathd and multipath could fail if > tableless > devices were present. With this patchset, they will simply ignore > tableless devices that don't have a multipath prefixed DM UUID. The > last > patch is a RFC patch that changes the behavior for tableless devices > that do have a multipath prefixed DM UUID. It makes multipath remove > these devices on the grounds that they are likely failed creates. > However, looking at the libdevmapper code, I'm not sure that we > actually > want to do this. When a DM_DEVICE_CREATE task is run, it will first > create a tableless device, and then immediately load the table and > resume the device. So it's possible any that tableless devices which > multipath sees are actually in the process of getting created. I left > the patch as part of the set, but I'm not sure that removing the > devices > is the right answer here. I haven't ever seen any tableless multipath > devices lying around (and I couldn't force any to get created when I > tried). Unless other people have seen these, then I don't think the > final patch of this set should go in. > > Benjamin Marzinski (8): > multipathd: print an error when failing to connect to multipathd > libmultipath: check DM UUID earlier in libmp_mapinfo__ > libmultipath: Don't fail dm_get_maps for DMP_NOT_FOUND > multipathd.service: restart multipathd on failure > libmultipath: use MAPINFO_CHECK_UUID in dm_get_multipath > libmultipath: signal multipath UUID device with no table > libmultipath: fix removing device after failed creation > libmultipath: remove devices with no table and multipath DM UUID > > libmultipath/devmapper.c | 56 +++++++++++++++++++++++------- > -- > libmultipath/devmapper.h | 6 ++-- > multipathd/multipathd.service.in | 3 ++ > multipathd/uxclnt.c | 4 ++- > 4 files changed, 51 insertions(+), 18 deletions(-) For patch 1-6 of this series: Reviewed-by: Martin Wilck <mwilck@suse.com>
On Wed, 2024-11-06 at 17:52 +0100, Martin Wilck wrote: > > For patch 1-6 of this series: > Reviewed-by: Martin Wilck <mwilck@suse.com> The series breaks some unit tests. I can fix that. Martin
On Wed, 2024-11-06 at 17:52 +0100, Martin Wilck wrote: > On Thu, 2024-10-31 at 14:32 -0400, Benjamin Marzinski wrote: > > This patchset include a couple of miscellaneous cleanups, but is > > mostly > > in response to issues brought up in > > https://github.com/opensvc/multipath-tools/issues/100 > > It adds auto restarting to the multipathd.service unit file. I'm > > fairly > > conflicted about the correct limits to be placed on these restarts, > > so > > if anyone has strong opinions and a good argument, I'm more than > > willing > > to change them. > > > > The bulk of the changes deal with how multipath handles devices > > without > > any table. Multipath was supposed to delete these if they were left > > behind after a failed device creation, but that code was broken. > > However devices aren't typically left behind after failed creates, > > so > > it > > didn't matter. > > > > A bigger issue is that multipathd and multipath could fail if > > tableless > > devices were present. With this patchset, they will simply ignore > > tableless devices that don't have a multipath prefixed DM UUID. The > > last > > patch is a RFC patch that changes the behavior for tableless > > devices > > that do have a multipath prefixed DM UUID. It makes multipath > > remove > > these devices on the grounds that they are likely failed creates. > > However, looking at the libdevmapper code, I'm not sure that we > > actually > > want to do this. When a DM_DEVICE_CREATE task is run, it will > > first > > create a tableless device, and then immediately load the table and > > resume the device. So it's possible any that tableless devices > > which > > multipath sees are actually in the process of getting created. I > > left > > the patch as part of the set, but I'm not sure that removing the > > devices > > is the right answer here. I haven't ever seen any tableless > > multipath > > devices lying around (and I couldn't force any to get created when > > I > > tried). Unless other people have seen these, then I don't think the > > final patch of this set should go in. > > > > Does it do any harm if we just keep these devices around? If there > are > path devices around with matching UUIDs, the right thing to do for > multipathd would be to reload the map's table with an appropriate > multipath target. Otherwise, the map will just float around empty and > do no harm. This is how multipathd 0.9.9 and earlier behave, and > I see no issue with that. 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. Martin
On Thu, Nov 07, 2024 at 01:20:09PM +0100, Martin Wilck wrote: > On Wed, 2024-11-06 at 17:52 +0100, Martin Wilck wrote: > > On Thu, 2024-10-31 at 14:32 -0400, Benjamin Marzinski wrote: > > > This patchset include a couple of miscellaneous cleanups, but is > > > mostly > > > in response to issues brought up in > > > https://github.com/opensvc/multipath-tools/issues/100 > > > It adds auto restarting to the multipathd.service unit file. I'm > > > fairly > > > conflicted about the correct limits to be placed on these restarts, > > > so > > > if anyone has strong opinions and a good argument, I'm more than > > > willing > > > to change them. > > > > > > The bulk of the changes deal with how multipath handles devices > > > without > > > any table. Multipath was supposed to delete these if they were left > > > behind after a failed device creation, but that code was broken. > > > However devices aren't typically left behind after failed creates, > > > so > > > it > > > didn't matter. > > > > > > A bigger issue is that multipathd and multipath could fail if > > > tableless > > > devices were present. With this patchset, they will simply ignore > > > tableless devices that don't have a multipath prefixed DM UUID. The > > > last > > > patch is a RFC patch that changes the behavior for tableless > > > devices > > > that do have a multipath prefixed DM UUID. It makes multipath > > > remove > > > these devices on the grounds that they are likely failed creates. > > > However, looking at the libdevmapper code, I'm not sure that we > > > actually > > > want to do this. When a DM_DEVICE_CREATE task is run, it will > > > first > > > create a tableless device, and then immediately load the table and > > > resume the device. So it's possible any that tableless devices > > > which > > > multipath sees are actually in the process of getting created. I > > > left > > > the patch as part of the set, but I'm not sure that removing the > > > devices > > > is the right answer here. I haven't ever seen any tableless > > > multipath > > > devices lying around (and I couldn't force any to get created when > > > I > > > tried). Unless other people have seen these, then I don't think the > > > final patch of this set should go in. > > > > > > > Does it do any harm if we just keep these devices around? If there > > are > > path devices around with matching UUIDs, the right thing to do for > > multipathd would be to reload the map's table with an appropriate > > multipath target. Otherwise, the map will just float around empty and > > do no harm. This is how multipathd 0.9.9 and earlier behave, and > > I see no issue with that. > > 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. -Ben > Martin
On Wed, Nov 06, 2024 at 11:39:38PM +0100, Martin Wilck wrote: > On Wed, 2024-11-06 at 17:52 +0100, Martin Wilck wrote: > > > > For patch 1-6 of this series: > > Reviewed-by: Martin Wilck <mwilck@suse.com> > > The series breaks some unit tests. I can fix that. Oops. I'm going to do some more work on this patchset, so I can fix it myself. -Ben > > Martin > >
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: multipathd[3198]: 360014053cdf4a9b80de42ab96c74f4ef: set ACT_CREATE (map does not exist) multipathd[3198]: 360014053cdf4a9b80de42ab96c74f4ef: map already present multipathd[3198]: 360014053cdf4a9b80de42ab96c74f4ef: reload [0 41943040 multipath 1 queue_if_no_path 1 alua 2 1 service-time 0 1 1 8:16 1 serv> Note that it first says "map does not exist", and then sees it present (with no table). The reload works without issues though, if the map is empty. If, on the contrary, the map is not empty but contains another incompatible target (I tried this with a trivial linear mapping), the reload operation will fail: multipathd[1347]: 360014053cdf4a9b80de42ab96c74f4ef: set ACT_CREATE (map does not exist) multipathd[1347]: 360014053cdf4a9b80de42ab96c74f4ef: map already present multipathd[1347]: 360014053cdf4a9b80de42ab96c74f4ef: reload [0 41943040 multipath 1 queue_if_no_path 1 alua 2 1 service-time 0 1 1 8:16 1 serv> kernel: device-mapper: ioctl: can't change device type (old=1 vs new=2) after initial table load. multipathd[1347]: libdevmapper: ioctl/libdm-iface.c(1998): device-mapper: reload ioctl on 360014053cdf4a9b80de42ab96c74f4ef failed: Invalid argument multipathd[1347]: dm_addmap: libdm task=1 error: Invalid argument multipathd[1347]: 360014053cdf4a9b80de42ab96c74f4ef: domap (0) failure for create/reload map multipathd[1347]: 360014053cdf4a9b80de42ab96c74f4ef: removing map Regards Martin
On Thu, 2024-11-07 at 12:48 -0500, Benjamin Marzinski wrote: > On Wed, Nov 06, 2024 at 11:39:38PM +0100, Martin Wilck wrote: > > On Wed, 2024-11-06 at 17:52 +0100, Martin Wilck wrote: > > > > > > For patch 1-6 of this series: > > > Reviewed-by: Martin Wilck <mwilck@suse.com> > > > > The series breaks some unit tests. I can fix that. > > Oops. I'm going to do some more work on this patchset, so I can fix > it > myself. Have a look at my "tip" branch, fixes are already applied there. Martin
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. 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 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. -Ben > > multipathd[3198]: 360014053cdf4a9b80de42ab96c74f4ef: set ACT_CREATE (map does not exist) > multipathd[3198]: 360014053cdf4a9b80de42ab96c74f4ef: map already present > multipathd[3198]: 360014053cdf4a9b80de42ab96c74f4ef: reload [0 41943040 multipath 1 queue_if_no_path 1 alua 2 1 service-time 0 1 1 8:16 1 serv> > > Note that it first says "map does not exist", and then sees it present (with no table). > The reload works without issues though, if the map is empty. > > If, on the contrary, the map is not empty but contains another incompatible target > (I tried this with a trivial linear mapping), the reload operation will fail: > > multipathd[1347]: 360014053cdf4a9b80de42ab96c74f4ef: set ACT_CREATE (map does not exist) > multipathd[1347]: 360014053cdf4a9b80de42ab96c74f4ef: map already present > multipathd[1347]: 360014053cdf4a9b80de42ab96c74f4ef: reload [0 41943040 multipath 1 queue_if_no_path 1 alua 2 1 service-time 0 1 1 8:16 1 serv> > kernel: device-mapper: ioctl: can't change device type (old=1 vs new=2) after initial table load. > multipathd[1347]: libdevmapper: ioctl/libdm-iface.c(1998): device-mapper: reload ioctl on 360014053cdf4a9b80de42ab96c74f4ef failed: Invalid argument > multipathd[1347]: dm_addmap: libdm task=1 error: Invalid argument > multipathd[1347]: 360014053cdf4a9b80de42ab96c74f4ef: domap (0) failure for create/reload map > multipathd[1347]: 360014053cdf4a9b80de42ab96c74f4ef: removing map > > Regards > Martin
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. > 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. > 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
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
On Mon, 2024-11-11 at 12:17 -0500, Benjamin Marzinski wrote: > 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: > > > > > > 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. I would like to keep things as simple as possible. The main mistake that I made in bf3a4ad ("libmultipath: simplify dm_get_maps()") was that I'd changed dm_get_maps() such that returned error if anything went wrong for a single map. That was wrong, it should just skip the map in question (IMO that even includes the DMP_ERR case, but that's up to discussion). Just changing the behavior of dm_get_maps() would fix the regressions reported on GitHub (correct me if I'm wrong). Wrt libmp_mapinfo: From my PoV, there's no difference between "has no table" and "has no targets". There _is_ a difference between "no targets" and "multiple targets" though, as the former can be reloaded as a proper multipath table whereas the latter cannot. So these two cases must be treated differently. I think we should actually introduce a new return code. - DMP_NOT_FOUND should actually mean that the given map does not exist at all - DMP_NO_MATCH should mean that it has targets that don't match (either multiple targets, or something non-multipath), or that the UUID doesn't match (if MAPINFO_CHECK_UUID is set). - We should have another rc code for "exists but is empty", but that should be called DMP_EMPTY rather than DMP_BAD_DEV. > 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. I wouldn't bother about this kind of race too much. If multipathd just ignores this kind of maps as it used to, it won't do any harm AFAICT. We shouldn't alter this behavior, IOW multipathd should neither remove these maps nor add them to its internal tables unless they become populated with paths. If anything, we should finally get down to delegating map creation from multipath to multipathd. > 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. I agree, comments above. > > > 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. What I meant is that if there are matching path devices, coalesce_paths() will create an mpp and reload the table. Otherwise, the table will continue lurking around without being tracked, which doesn't do harm even if it may not seem optimal. If a matching path is added, again, uev_add_path() should reload the map (to be tested). I think that behavior is sane enough for a corner case like this. Martin
On Tue, Nov 12, 2024 at 08:46:52AM +0100, Martin Wilck wrote: > On Mon, 2024-11-11 at 12:17 -0500, Benjamin Marzinski wrote: > > 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: > > > > > > > > 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. > > I would like to keep things as simple as possible. > > The main mistake that I made in bf3a4ad ("libmultipath: simplify > dm_get_maps()") was that I'd changed dm_get_maps() such that returned > error if anything went wrong for a single map. That was wrong, it > should just skip the map in question (IMO that even includes the > DMP_ERR case, but that's up to discussion). Just changing the behavior > of dm_get_maps() would fix the regressions reported on GitHub (correct > me if I'm wrong). > > Wrt libmp_mapinfo: > > >From my PoV, there's no difference between "has no table" and "has no > targets". There _is_ a difference between "no targets" and "multiple > targets" though, as the former can be reloaded as a proper multipath > table whereas the latter cannot. So these two cases must be treated > differently. > > I think we should actually introduce a new return code. > > - DMP_NOT_FOUND should actually mean that the given map does not exist > at all > - DMP_NO_MATCH should mean that it has targets that don't match > (either multiple targets, or something non-multipath), or that the UUID > doesn't match (if MAPINFO_CHECK_UUID is set). > - We should have another rc code for "exists but is empty", but that > should be called DMP_EMPTY rather than DMP_BAD_DEV. This sounds reasonable. I can redo my patch for that on top of the set you just posted. > > 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. > > I wouldn't bother about this kind of race too much. If multipathd just > ignores this kind of maps as it used to, it won't do any harm AFAICT. > We shouldn't alter this behavior, IOW multipathd should neither remove > these maps nor add them to its internal tables unless they become > populated with paths. > > If anything, we should finally get down to delegating map creation from > multipath to multipathd. The issue is that if creating a multipath device fails, the current code will only delete any leftover device it finds if that device is already set up correctly (since dm_flush_map_nosync() won't delete the map if it doesn't have a multipath table). This means we only delete devices in the case where we did race and something else managed to set the device up, which is exactly what we don't want to do. So, I'm going to repost a version of my ("libmultipath: fix removing device after failed creation") patch because anything is better than what we do right now, which is just wrong. > > 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. > > I agree, comments above. > > > > > 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. > > What I meant is that if there are matching path devices, > coalesce_paths() will create an mpp and reload the table. Otherwise, > the table will continue lurking around without being tracked, which > doesn't do harm even if it may not seem optimal. If a matching path is > added, again, uev_add_path() should reload the map (to be tested). > > I think that behavior is sane enough for a corner case like this. Even if there are matching path devices, if there is a dm device that has a different name than the device we are trying to create, but the same UUID, and no table (and thus, not in the current list of mpps), multipath will try to create a new device instead of reload it, and it will fail. I'm trying to avoid that. But this issue has been around forever, and AFAIK has never actually caused a problem, so fixing it isn't worth any real added complexity. -Ben > Martin