Re: [PATCH 03/13] multipathd: allow map removal in do_sync_mpp()

[Benjamin Marzinski <bmarzins@redhat.com>]

On Thu, Dec 12, 2024 at 06:18:40PM +0100, Martin Wilck wrote:
> On Thu, 2024-12-12 at 12:12 -0500, Benjamin Marzinski wrote:
> > On Wed, Dec 11, 2024 at 09:33:40PM +0100, Martin Wilck wrote:
> > > On Wed, 2024-12-11 at 21:20 +0100, Martin Wilck wrote:
> > > > On Wed, 2024-12-11 at 12:09 -0500, Benjamin Marzinski wrote:
> > > > 
> > > > 
> > > > > I'm not actually worried about the kernel so much as
> > > > > libdevmapper.
> > > > > It
> > > > > is
> > > > > not designed for multi-threaded processes, and that has bitten
> > > > > us
> > > > > in
> > > > > the
> > > > > past. For intance, it's why we don't delete devices in
> > > > > dmevent_loop()
> > > > > on
> > > > > libdevmapper errors. dm_get_events() just waits and retries if
> > > > > getting
> > > > > the device list fails, and for each device, it calls
> > > > > dm_is_mpath
> > > > > and
> > > > > will only delete a device on DM_IS_MPATH_NO, which is what I
> > > > > suggested
> > > > > for the cleanup function.
> > > > > 
> > > > > I'm pretty sure we've handled all of the known issues here,
> > > > > with
> > > > > fixes
> > > > > like:
> > > > > 02d4bf07 ("libmultipath: protect racy libdevmapper calls with a
> > > > > mutex")
> > > > > 34e01d2f ("multipath-tools: don't call dm_lib_release() any
> > > > > more")
> > > > > 
> > > > > I'd rather not risk having missed some issue that could cause a
> > > > > temporary error in a function that we call every couple of
> > > > > seconds
> > > > > (almost always unnecessarily).
> > > > 
> > > > Ok, getting it. I thought that an error in DM_TABLE_STATUS must
> > > > almost
> > > > neccessarily mean -ENXIO (from the kernel pov), which would mean
> > > > that
> > > > some external entity removed the device, and that we should act
> > > > as if
> > > > someone had used the "remove map" CLI command. But I didn't think
> > > > about
> > > > libdevmapper.
> > > 
> > > But will libdevmapper return ENXIO if it's somehow interally
> > > confused?
> > > I don't think so. I believe that if we see this error code,
> > > removing
> > > the map is the right thing to do.
> > 
> > I don't think that shouldn't ever happen.
> > 
> > https://github.com/lvmteam/lvm2/blob/928b8e9c6eaf871b3405b91c64eac5ea854f2572/device_mapper/ioctl/libdm-iface.c#L2100
> > 
> > If libdevmapper gets an ENXIO from the kernel, it ends up setting
> > dmi.exists to 0 instead of returning the error.
> 
> I meant the kernel ioctl return code. Sorry for being unclear.
> 
> >From the the point of view of libmultipath, it doesn't matter.
> libmp_mapinfo tests for ENXIO and dmi.exists, and returns DMP_NOT_FOUND
> in both cases.

Sure, that makes sense then. If libdevmapper told us the device doesn't
exist, we can safely remove it.

-Ben
 
> Martin