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

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&gt; >>>> Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; >>>> --- >>>> 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.