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@xxxxxxxxxx>
> >>>> Signed-off-by: Michal Privoznik <mprivozn@xxxxxxxxxx>
> >>>> ---
> >>>> 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 ToscanoAttachment:
signature.asc
Description: This is a digitally signed message part.
