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

[Michal Privoznik]

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.

Hence, I think the best solution for us is to not cache anything and 
simply parse /proc/devices every time. As an optimization, I can firstly 
check whether /dev/mapper/control exists at all an if not exit early.

Michal