Re: [PATCH 1/6] libmultipath: signal device with no table in libmp_mapinfo

[Benjamin Marzinski <bmarzins@redhat.com>]

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