2:54 a.m.
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(a)redhat.com>
>>> Signed-off-by: Michal Privoznik <mprivozn(a)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