Re: [PATCH 2/3] libmutipath: don't close fd on dm_lib_release

[Martin Wilck <Martin.Wilck@suse.com>]

On Thu, 2020-07-02 at 15:06 -0500, Benjamin Marzinski wrote:
> On Thu, Jul 02, 2020 at 11:52:21AM +0000, Martin Wilck wrote:
> > On Wed, 2020-03-25 at 17:00 -0500, Benjamin Marzinski wrote:
> > > On Wed, Mar 25, 2020 at 03:52:55PM -0500, Benjamin Marzinski
> > > wrote:
> > > > On Wed, Mar 25, 2020 at 03:16:50PM +0000, Martin Wilck wrote:
> > > > > On Tue, 2020-03-24 at 16:03 -0500, Benjamin Marzinski wrote:
> > > > > 
> > > > > AFAICS, this function has been in libdm since 1.02.111. We
> > > > > support
> > > > > 1.02.89 (if all features enabled, otherwise even older).
> > > > > Perhaps
> > > > > we
> > > > > should make this function call conditional on the libdm
> > > > > verson?
> > > > > 
> > > > > But perhaps more importantly, why do we still need to call
> > > > > dm_lib_release()? AFAICS it's only needed for systems that
> > > > > have
> > > > > no udev
> > > > > support for creating device nodes (to call update_devs() via
> > > > > dm_lib_release()), and we don't support that anymore anyway,
> > > > > do
> > > > > we? 
> > > > > 
> > > > > Since 26c4bb0, we're always setting the
> > > > > DM_UDEV_DISABLE_LIBRARY_FALLBACK flag, and the cookie, too
> > > > > (we aren't setting it for DM_DEVICE_RELOAD, but it isn't
> > > > > needed
> > > > > for
> > > > > that, either, since no device nodes need to be created or
> > > > > removed); so
> > > > > dm_lib_release() should really have no effect.
> > > > > 
> > > > > Regards
> > > > > Martin
> > > > 
> > > > Good call. I'll redo this patch.
> > > 
> > > Actually, I've changed my mind. Calling dm_lib_release() lets us
> > > release
> > > the memory that device-mapper uses to store all the node ops that
> > > it
> > > was saving up.  Without calling dm_lib_release(), AFAICS, that
> > > memory
> > > keeps growing until the daemon exits.
> > 
> > Sorry for coming back to this so late. I've just stared at the
> > libdm
> > code again. 
> > 
> > We always set DM_UDEV_DISABLE_LIBRARY_FALLBACK. In the standard
> > CREATE
> > and REMOVE cases, libdm doesn't stack any operations if this flag
> > is
> > set. The only exceptions are 
> > 
> >  a) RESUME operations with DM_ADD_NODE_ON_RESUME set. This happens
> > implicity when we create new maps
> >  b) RENAME operations
> > 
> > In both cases, we call dm_udev_wait() after the libdm operation,
> > which
> > calls update_devs() and should thus have the same effect as
> > dm_lib_release(). IOW, I still believe we don't need to call
> > dm_lib_release() any more.
> 
> Sure. But can we leave this patch as is, and remove those calls in a
> different patch?

Of course. It's not important, either. I just wanted to make sure
we agree on the technical side.

Regards
Martin

-- 
Dr. Martin Wilck <mwilck@suse.com>, Tel. +49 (0)911 74053 2107
SUSE  Software Solutions Germany GmbH
HRB 36809, AG Nürnberg GF: Felix
Imendörffer