| Message ID | 20241115232256.627933-2-bmarzins@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | multipath-tools: Handle tableless DM devices | expand |
On Fri, 2024-11-15 at 18:22 -0500, Benjamin Marzinski wrote: > if libmp_mapinfo() is run on a device that has no active table, > it will now return with a new exit code, DMP_EMPTY, to signal this. > Currently all code paths treat this identically to DMP_NOT_FOUND. > > Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com> I just looked through the code again. I think with this change, we need to modify dm_flush_map__() and do_foreach_partmaps(). They should remove / act on empty maps. What do you think? Also, we can skip the call to is_mpath_part_uuid() in do_foreach_partmaps() after my "make MAPINFO_CHECK_UUID work with partitions" patch. I can submit a patch myself, but as you're at already, maybe you want to do that? Martin
On Tue, Nov 19, 2024 at 05:39:26PM +0100, Martin Wilck wrote: > On Fri, 2024-11-15 at 18:22 -0500, Benjamin Marzinski wrote: > > if libmp_mapinfo() is run on a device that has no active table, > > it will now return with a new exit code, DMP_EMPTY, to signal this. > > Currently all code paths treat this identically to DMP_NOT_FOUND. > > > > Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com> > > I just looked through the code again. I think with this change, we need > to modify dm_flush_map__() and do_foreach_partmaps(). They should > remove / act on empty maps. What do you think? I'm not sure that we need to add extra code to handle the possiblity that an empty device could appear at any time, since like I said, this is a corner case that I've never actually seen in the wild. So if a device was previously a valid multipath device, I don't think we need to worry about the possibility that it suddenly became empty. But I can see the value in running something like # multipath -F and having it clean up any empty multipath devices. As for do_foreach_partmaps(), are you thinking about cleaning up empty partition devices or non-empty partition devices on top of empty multipath devices? Non-empty partition devices on top of empty multipath devices would imply that a multipath device was valid at one point, and then became empty, which I don't see an easy way of happening. The problem with empty partition devices is that partition devices are created by kpartx completely asynchronously to us. That empty partition device could be in the process of being created. So I'm not against checking for DMP_EMPTY in dm_flush_map__() and removing the empty device if its opencount is 0. But I'd rather not try messing with partmaps. Do you have a specific case you are worried about? > Also, we can skip the call to is_mpath_part_uuid() in > do_foreach_partmaps() after my "make MAPINFO_CHECK_UUID work with > partitions" patch. I can submit a patch myself, but as you're at > already, maybe you want to do that? Yep. I can do that. -Ben > Martin
On Tue, 2024-11-19 at 15:33 -0500, Benjamin Marzinski wrote: > On Tue, Nov 19, 2024 at 05:39:26PM +0100, Martin Wilck wrote: > > On Fri, 2024-11-15 at 18:22 -0500, Benjamin Marzinski wrote: > > > if libmp_mapinfo() is run on a device that has no active table, > > > it will now return with a new exit code, DMP_EMPTY, to signal > > > this. > > > Currently all code paths treat this identically to DMP_NOT_FOUND. > > > > > > Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com> > > > > I just looked through the code again. I think with this change, we > > need > > to modify dm_flush_map__() and do_foreach_partmaps(). They should > > remove / act on empty maps. What do you think? > > I'm not sure that we need to add extra code to handle the possiblity > that an empty device could appear at any time, since like I said, > this is a corner case that I've never actually seen in the wild. So > if > a device was previously a valid multipath device, I don't think we > need > to worry about the possibility that it suddenly became empty. > > But I can see the value in running something like > > # multipath -F > > and having it clean up any empty multipath devices. As for > do_foreach_partmaps(), are you thinking about cleaning up empty > partition devices or non-empty partition devices on top of empty > multipath devices? > > Non-empty partition devices on top of empty multipath devices would > imply that a multipath device was valid at one point, and then became > empty, which I don't see an easy way of happening. > > The problem with empty partition devices is that partition devices > are > created by kpartx completely asynchronously to us. That empty > partition > device could be in the process of being created. Right, but multipathd is in the process of deleting the map. If there's actually a race and kpartx finishes creating the partition map, multipathd will fail to remove the multipath map. The likely outcome will be a multipath map with just one partition device. If multipathd comes first, kpartx will fail, but there's a good chance that multipathd will succeed in flushing the multipath map, so we'll end up with a consistent state. If kpartx had run a little earlier and had finished setting up the map, multipathd would have removed it, and if kpartx had run a little later, it would have failed because of a missing target. > So I'm not against checking for DMP_EMPTY in dm_flush_map__() and > removing the empty device if its opencount is 0. But I'd rather not > try messing with partmaps. Do you have a specific case you are > worried > about? From the argument above, I'd say that flushing empty partition maps is the right thing to do, for consistency reasons. But now I may be overlooking something. Martin
On Wed, Nov 20, 2024 at 09:49:17AM +0100, Martin Wilck wrote: > On Tue, 2024-11-19 at 15:33 -0500, Benjamin Marzinski wrote: > > On Tue, Nov 19, 2024 at 05:39:26PM +0100, Martin Wilck wrote: > > > On Fri, 2024-11-15 at 18:22 -0500, Benjamin Marzinski wrote: > > > > if libmp_mapinfo() is run on a device that has no active table, > > > > it will now return with a new exit code, DMP_EMPTY, to signal > > > > this. > > > > Currently all code paths treat this identically to DMP_NOT_FOUND. > > > > > > > > Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com> > > > > > > I just looked through the code again. I think with this change, we > > > need > > > to modify dm_flush_map__() and do_foreach_partmaps(). They should > > > remove / act on empty maps. What do you think? > > > > I'm not sure that we need to add extra code to handle the possiblity > > that an empty device could appear at any time, since like I said, > > this is a corner case that I've never actually seen in the wild. So > > if > > a device was previously a valid multipath device, I don't think we > > need > > to worry about the possibility that it suddenly became empty. > > > > But I can see the value in running something like > > > > # multipath -F > > > > and having it clean up any empty multipath devices. As for > > do_foreach_partmaps(), are you thinking about cleaning up empty > > partition devices or non-empty partition devices on top of empty > > multipath devices? > > > > Non-empty partition devices on top of empty multipath devices would > > imply that a multipath device was valid at one point, and then became > > empty, which I don't see an easy way of happening. > > > > The problem with empty partition devices is that partition devices > > are > > created by kpartx completely asynchronously to us. That empty > > partition > > device could be in the process of being created. > > Right, but multipathd is in the process of deleting the map. If there's > actually a race and kpartx finishes creating the partition map, > multipathd will fail to remove the multipath map. The likely outcome > will be a multipath map with just one partition device. If multipathd > comes first, kpartx will fail, but there's a good chance that > multipathd will succeed in flushing the multipath map, so we'll end up > with a consistent state. > > If kpartx had run a little earlier and had finished setting up the map, > multipathd would have removed it, and if kpartx had run a little later, > it would have failed because of a missing target. > If we make do_foreach_partmaps() remove empty partition maps, it has keep the is_mpath_part_uuid() call. Otherwise we would remove any empty partition device, including ones that aren't for this multipath device, which breaks your argument above. > > So I'm not against checking for DMP_EMPTY in dm_flush_map__() and > > removing the empty device if its opencount is 0. But I'd rather not > > try messing with partmaps. Do you have a specific case you are > > worried > > about? > > >From the argument above, I'd say that flushing empty partition maps is > the right thing to do, for consistency reasons. > > But now I may be overlooking something. How would you feel about adding a parameter to do_foreach_partmaps() to say whether or not it should remove empty partmaps (or possibly just checking if the partmap_func is remove_partmaps). Your argument makes sense when you are removing a device. But what about functions like dm_cancel_remove_partmaps() and dm_rename_partmaps()? I'm not sure that these should automatically empty (a possibly being created) partition devices. -Ben > Martin
On Wed, 2024-11-20 at 16:59 -0500, Benjamin Marzinski wrote: > On Wed, Nov 20, 2024 at 09:49:17AM +0100, Martin Wilck wrote: > > On Tue, 2024-11-19 at 15:33 -0500, Benjamin Marzinski wrote: > > > On Tue, Nov 19, 2024 at 05:39:26PM +0100, Martin Wilck wrote: > > > > On Fri, 2024-11-15 at 18:22 -0500, Benjamin Marzinski wrote: > > > > > if libmp_mapinfo() is run on a device that has no active > > > > > table, > > > > > it will now return with a new exit code, DMP_EMPTY, to signal > > > > > this. > > > > > Currently all code paths treat this identically to > > > > > DMP_NOT_FOUND. > > > > > > > > > > Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com> > > > > > > > > I just looked through the code again. I think with this change, > > > > we > > > > need > > > > to modify dm_flush_map__() and do_foreach_partmaps(). They > > > > should > > > > remove / act on empty maps. What do you think? > > > > > > I'm not sure that we need to add extra code to handle the > > > possiblity > > > that an empty device could appear at any time, since like I said, > > > this is a corner case that I've never actually seen in the wild. > > > So > > > if > > > a device was previously a valid multipath device, I don't think > > > we > > > need > > > to worry about the possibility that it suddenly became empty. > > > > > > But I can see the value in running something like > > > > > > # multipath -F > > > > > > and having it clean up any empty multipath devices. As for > > > do_foreach_partmaps(), are you thinking about cleaning up empty > > > partition devices or non-empty partition devices on top of empty > > > multipath devices? > > > > > > Non-empty partition devices on top of empty multipath devices > > > would > > > imply that a multipath device was valid at one point, and then > > > became > > > empty, which I don't see an easy way of happening. > > > > > > The problem with empty partition devices is that partition > > > devices > > > are > > > created by kpartx completely asynchronously to us. That empty > > > partition > > > device could be in the process of being created. > > > > Right, but multipathd is in the process of deleting the map. If > > there's > > actually a race and kpartx finishes creating the partition map, > > multipathd will fail to remove the multipath map. The likely > > outcome > > will be a multipath map with just one partition device. If > > multipathd > > comes first, kpartx will fail, but there's a good chance that > > multipathd will succeed in flushing the multipath map, so we'll end > > up > > with a consistent state. > > > > If kpartx had run a little earlier and had finished setting up the > > map, > > multipathd would have removed it, and if kpartx had run a little > > later, > > it would have failed because of a missing target. > > > > If we make do_foreach_partmaps() remove empty partition maps, it has > keep the is_mpath_part_uuid() call. Otherwise we would remove any > empty > partition device, including ones that aren't for this multipath > device, > which breaks your argument above. Yes, my bad. There's of course a difference between "is this a partition mapping" and "is this a partition mapped to the current mpath device". Thanks for pointing it out. > > > So I'm not against checking for DMP_EMPTY in dm_flush_map__() and > > > removing the empty device if its opencount is 0. But I'd rather > > > not > > > try messing with partmaps. Do you have a specific case you are > > > worried > > > about? > > > > > From the argument above, I'd say that flushing empty partition > > > maps is > > the right thing to do, for consistency reasons. > > > > But now I may be overlooking something. > > How would you feel about adding a parameter to do_foreach_partmaps() > to > say whether or not it should remove empty partmaps (or possibly just > checking if the partmap_func is remove_partmaps). Your argument > makes > sense when you are removing a device. But what about functions like > dm_cancel_remove_partmaps() and dm_rename_partmaps()? I'm not sure > that > these should automatically empty (a possibly being created) partition > devices. We're moving far into corner case land here :-) I think we should keep it as simple as possible. For me, that means that an empty partition map (with UUID matching the current mpath map) should be treated like any other partition map with matching UUID, no matter what the current operation is. There will be some situations where the outcome will be suboptimal. By keeping it simple, we'll at least be able to understand the outcome. Martin
On Thu, Nov 21, 2024 at 09:57:28AM +0100, Martin Wilck wrote: > On Wed, 2024-11-20 at 16:59 -0500, Benjamin Marzinski wrote: > > > > How would you feel about adding a parameter to do_foreach_partmaps() > > to > > say whether or not it should remove empty partmaps (or possibly just > > checking if the partmap_func is remove_partmaps). Your argument > > makes > > sense when you are removing a device. But what about functions like > > dm_cancel_remove_partmaps() and dm_rename_partmaps()? I'm not sure > > that > > these should automatically empty (a possibly being created) partition > > devices. > > We're moving far into corner case land here :-) > > I think we should keep it as simple as possible. For me, that means > that an empty partition map (with UUID matching the current mpath map) > should be treated like any other partition map with matching UUID, no > matter what the current operation is. > > There will be some situations where the outcome will be suboptimal. By > keeping it simple, we'll at least be able to understand the outcome. That's reasonable. I misunderstood your suggestion as asking to always remove empty partition devices in do_foreach_partmaps(). We'd need to make sure all the partmap_func()s work with empty devices. But that shouldn't be too bad. -Ben > Martin
diff --git a/libmultipath/devmapper.c b/libmultipath/devmapper.c index 9714270d..b718dccf 100644 --- a/libmultipath/devmapper.c +++ b/libmultipath/devmapper.c @@ -86,6 +86,7 @@ const char *dmp_errstr(int rc) [DMP_OK] = "success", [DMP_NOT_FOUND] = "not found", [DMP_NO_MATCH] = "target type mismatch", + [DMP_EMPTY] = "no target", [DMP_LAST__] = "**invalid**", }; if (rc < 0 || rc > DMP_LAST__) @@ -747,9 +748,9 @@ static int libmp_mapinfo__(int flags, mapid_t id, mapinfo_t info, const char *ma condlog(lvl, "%s: map %s has multiple targets", fname__, map_id); return DMP_NO_MATCH; } - if (!params) { + if (!params || !target_type) { condlog(lvl, "%s: map %s has no targets", fname__, map_id); - return DMP_NOT_FOUND; + return DMP_EMPTY; } if (flags & MAPINFO_TGT_TYPE__) { const char *tgt_type = flags & MAPINFO_MPATH_ONLY ? TGT_MPATH : TGT_PART; @@ -873,6 +874,7 @@ int dm_is_mpath(const char *name) return DM_IS_MPATH_YES; case DMP_NOT_FOUND: case DMP_NO_MATCH: + case DMP_EMPTY: return DM_IS_MPATH_NO; case DMP_ERR: default: diff --git a/libmultipath/devmapper.h b/libmultipath/devmapper.h index 6b3bbad9..23926e3f 100644 --- a/libmultipath/devmapper.h +++ b/libmultipath/devmapper.h @@ -35,6 +35,7 @@ enum { DMP_OK, DMP_NOT_FOUND, DMP_NO_MATCH, + DMP_EMPTY, DMP_LAST__, }; @@ -105,7 +106,11 @@ typedef struct libmp_map_info { * @returns: * DMP_OK if successful. * DMP_NOT_FOUND if the map wasn't found, or has no or multiple targets. - * DMP_NO_MATCH if the map didn't match @tgt_type (see above). + * DMP_NO_MATCH if the map didn't match @tgt_type (see above) or didn't + * have a multipath uuid prefix. + * DMP_EMPTY if the map has no table. Note. The check for matching uuid + * prefix will happen first, but the check for matching + * tgt_type will happen afterwards. * DMP_ERR if some other error occurred. * * This function obtains the requested information for the device-mapper map
if libmp_mapinfo() is run on a device that has no active table, it will now return with a new exit code, DMP_EMPTY, to signal this. Currently all code paths treat this identically to DMP_NOT_FOUND. Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com> --- libmultipath/devmapper.c | 6 ++++-- libmultipath/devmapper.h | 7 ++++++- 2 files changed, 10 insertions(+), 3 deletions(-)