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@xxxxxxxxxx> > > > > > > > > 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
