[PATCH 1/2] virdevmapper: Don't error on kernels without DM support

[Pino Toscano]

On Tuesday, 18 August 2020 09:54:25 CEST Michal Privoznik wrote:
> On 8/17/20 5:58 PM, Peter Krempa wrote:
> > On Mon, Aug 17, 2020 at 17:40:04 +0200, Michal Privoznik wrote:
> >> On 8/17/20 5:16 PM, Peter Krempa wrote:
> >>> On Mon, Aug 17, 2020 at 16:26:54 +0200, Michal Privoznik wrote:
> >>>> In one of my latest patch (v6.6.0~30) I was trying to remove
> >>>> libdevmapper use in favor of our own implementation. However, the
> >>>> code did not take into account that device mapper can be not
> >>>> compiled into the kernel (e.g. be a separate module that's not
> >>>> loaded) in which case /proc/devices won't have the device-mapper
> >>>> major number and thus virDevMapperGetTargets() and/or
> >>>> virIsDevMapperDevice() fails.
> >>>>
> >>>> Fixes: 22494556542c676d1b9e7f1c1f2ea13ac17e1e3e
> >>>> Reported-by: Andrea Bolognani <abologna at redhat.com>
> >>>> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
> >>>> ---
> >>>>    src/util/virdevmapper.c | 41 +++++++++++++++++++++++++++++++----------
> >>>>    1 file changed, 31 insertions(+), 10 deletions(-)
> >>>>
> >>>> diff --git a/src/util/virdevmapper.c b/src/util/virdevmapper.c
> >>>> index fe7f611496..cc33d8211e 100644
> >>>> --- a/src/util/virdevmapper.c
> >>>> +++ b/src/util/virdevmapper.c
> >>>> @@ -47,10 +47,11 @@
> >>>>    G_STATIC_ASSERT(BUF_SIZE > sizeof(struct dm_ioctl));
> >>>>    static unsigned int virDMMajor;
> >>>> +static virMutex virDevMapperInitMutex = VIR_MUTEX_INITIALIZER;
> >>>
> >>> 'virDMMajor' should now be initialized explicitly ...
> >>
> >> Aren't global static variables initialized to zero automatically (something
> >> about .bss section)?
> > 
> > Yes, they are.
> > 
> >>
> >>>
> >>>>    static int
> >>>> -virDevMapperOnceInit(void)
> >>>> +virDevMapperGetMajor(unsigned int *dmmaj)
> >>>>    {
> >>>>        g_autofree char *buf = NULL;
> >>>>        VIR_AUTOSTRINGLIST lines = NULL;
> >>>> @@ -69,23 +70,34 @@ virDevMapperOnceInit(void)
> >>>>            if (sscanf(lines[i], "%u %ms\n", &maj, &dev) == 2 &&
> >>>>                STREQ(dev, DM_NAME)) {
> >>>> -            virDMMajor = maj;
> >>>
> >>> ... since it's not always initialized here.
> >>>
> >>>> +            *dmmaj = maj;
> >>>>                break;
> >>>>            }
> >>>>        }
> >>>>        if (!lines[i]) {
> >>>> -        virReportError(VIR_ERR_INTERNAL_ERROR,
> >>>> -                       _("Unable to find major for %s"),
> >>>> -                       DM_NAME);
> >>>> -        return -1;
> >>>> +        /* Don't error here. Maybe the kernel was built without
> >>>> +         * devmapper. Let the caller decide. */
> >>>> +        return -2;
> >>>>        }
> >>>>        return 0;
> >>>>    }
> >>>> -VIR_ONCE_GLOBAL_INIT(virDevMapper);
> >>>> +static int
> >>>> +virDevMapperInit(void)
> >>>> +{
> >>>> +    int rc = 0;
> >>>> +
> >>>> +    virMutexLock(&virDevMapperInitMutex);
> >>>> +
> >>>> +    if (virDMMajor == 0)
> >>>
> >>> I'm not quite persuaded with this caching here. For cases when the
> >>> device mapper is loaded we cache it, but in the negative case ...
> >>>
> >>>> +        rc = virDevMapperGetMajor(&virDMMajor);
> >>>
> >>> ... we always process /proc/devices. Why is caching necessary then?
> >>
> >> Are you suggesting to parse /proc/devices every time? My concern is it will
> >> burn CPU cycles needlessly. And I'm not sure how to address the negative
> >> case when DM module is not loaded. It's ineffective, I agree, I just don't
> >> see how to make it better.
> > 
> > Well, at first we should establish when the value is determined/can
> > change:
> > 
> > 1) at kernel build time/boot time (any case when the libvirtd process
> >     will need to be restarted for it to change)
> > 
> >    In these scenarios it doesn't actually make sense to check it prior to
> >    trying to open the device mapper control socket first. You can look it
> >    up and cache it after you open the socket and don't ever need to re-do
> >    it. In that case you can also use the existing VIR_ONCE code.
> > 
> >    You then don't have to clear it when the module is ejected, because
> >    afterwards the control socket will not exist. In case the module is
> >    re-injected, given the contstraints above the value will not change so
> >    no need to re-load.
> > 
> > 2) at module insertion time
> > 
> >    In this case it's actually wrong to cache it, because the module can
> >    be unloaded and reloaded while libvirt will not check and update the
> >    cached value. In those scenarios it should also be determined only
> >    after you open the control fd frist.
> > 
> As promised yesterday, I've dived into the code and found out that the 
> major number can be specified as a parameter to the dm module (just 
> tested and it works). So the next thing I tried was to see how could we 
> check whether the module was reloaded. I've tried opening 
> /dev/mapper/control hoping that I will get EOF on module unload (which I 
> could then use to run a callback that would invalidate the cached 
> value). But having the file open prevents unloading the module.
> 
> Loading/unloading a module results in an udev event, BUT we listen for 
> udev events only in nodedev driver and moving the code out to a driver 
> agnostic location and making it driver agnostic is too much code for a 
> little gain.
> 
> Then I looked whether it's possible to get the major number via an 
> ioctl(). But haven't found anything.

What about stat()ing /dev/mapper/control? That should give you the
major/minor of that special character device.

-- 
Pino Toscano
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: This is a digitally signed message part.
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20200818/13d829e5/attachment-0001.sig>