[PATCH v2 0/2] Deal with kernels without DM support
v2 of: https://www.redhat.com/archives/libvir-list/2020-August/msg00489.html diff to v1: - After discussion to v1 I've decided to not cache DM major number and thus the patches looks a bit different. Michal Prívozník (2): virdevmapper: Don't cache device-mapper major virdevmapper: Handle kernel without device-mapper support src/util/virdevmapper.c | 35 ++++++++++++++++++++++------------- 1 file changed, 22 insertions(+), 13 deletions(-) -- 2.26.2
The device mapper major is needed in virIsDevMapperDevice() which determines whether given device is managed by device-mapper. This number is obtained by parsing /proc/devices and then stored in a global variable so that the file doesn't have to be parsed again. However, as it turns out this logic is flawed - the major number is not static and can change as it can be specified as a parameter when loading the dm-mod module. Unfortunately, I was not able to come up with a good solution and thus the /proc/devices file is being parsed every time we need the device mapper major. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/virdevmapper.c | 17 +++++------------ 1 file changed, 5 insertions(+), 12 deletions(-) diff --git a/src/util/virdevmapper.c b/src/util/virdevmapper.c index fe7f611496..b24d9377f8 100644 --- a/src/util/virdevmapper.c +++ b/src/util/virdevmapper.c @@ -46,11 +46,9 @@ G_STATIC_ASSERT(BUF_SIZE > sizeof(struct dm_ioctl)); -static unsigned int virDMMajor; - static int -virDevMapperOnceInit(void) +virDevMapperGetMajor(unsigned int *major) { g_autofree char *buf = NULL; VIR_AUTOSTRINGLIST lines = NULL; @@ -69,7 +67,7 @@ virDevMapperOnceInit(void) if (sscanf(lines[i], "%u %ms\n", &maj, &dev) == 2 && STREQ(dev, DM_NAME)) { - virDMMajor = maj; + *major = maj; break; } } @@ -85,9 +83,6 @@ virDevMapperOnceInit(void) } -VIR_ONCE_GLOBAL_INIT(virDevMapper); - - static void * virDMIoctl(int controlFD, int cmd, struct dm_ioctl *dm, char **buf) { @@ -304,9 +299,6 @@ virDevMapperGetTargets(const char *path, * consist of devices or yet another targets. If that's the * case, we have to stop recursion somewhere. */ - if (virDevMapperInitialize() < 0) - return -1; - if ((controlFD = virDMOpen()) < 0) return -1; @@ -318,13 +310,14 @@ bool virIsDevMapperDevice(const char *dev_name) { struct stat buf; + unsigned int major; - if (virDevMapperInitialize() < 0) + if (virDevMapperGetMajor(&major) < 0) return false; if (!stat(dev_name, &buf) && S_ISBLK(buf.st_mode) && - major(buf.st_rdev) == virDMMajor) + major(buf.st_rdev) == major) return true; return false; -- 2.26.2
On Tue, Aug 18, 2020 at 11:30:24 +0200, Michal Privoznik wrote:
The device mapper major is needed in virIsDevMapperDevice() which determines whether given device is managed by device-mapper. This number is obtained by parsing /proc/devices and then stored in a global variable so that the file doesn't have to be parsed again. However, as it turns out this logic is flawed - the major number is not static and can change as it can be specified as a parameter when loading the dm-mod module.
Unfortunately, I was not able to come up with a good solution and thus the /proc/devices file is being parsed every time we need the device mapper major.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/virdevmapper.c | 17 +++++------------ 1 file changed, 5 insertions(+), 12 deletions(-)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
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@redhat.com> Reported-by: Christian Ehrhardt <christian.ehrhardt@canonical.com> Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/virdevmapper.c | 20 ++++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-) diff --git a/src/util/virdevmapper.c b/src/util/virdevmapper.c index b24d9377f8..3cc399f382 100644 --- a/src/util/virdevmapper.c +++ b/src/util/virdevmapper.c @@ -54,6 +54,9 @@ virDevMapperGetMajor(unsigned int *major) VIR_AUTOSTRINGLIST lines = NULL; size_t i; + if (!virFileExists(CONTROL_PATH)) + return -2; + if (virFileReadAll(PROC_DEVICES, BUF_SIZE, &buf) < 0) return -1; @@ -126,8 +129,13 @@ virDMOpen(void) memset(&dm, 0, sizeof(dm)); - if ((controlFD = open(CONTROL_PATH, O_RDWR)) < 0) + if ((controlFD = open(CONTROL_PATH, O_RDWR)) < 0) { + if (errno == ENOENT) + return -2; + + virReportSystemError(errno, _("Unable to open %s"), CONTROL_PATH); return -1; + } if (!virDMIoctl(controlFD, DM_VERSION, &dm, &tmp)) { virReportSystemError(errno, "%s", @@ -299,8 +307,16 @@ virDevMapperGetTargets(const char *path, * consist of devices or yet another targets. If that's the * case, we have to stop recursion somewhere. */ - if ((controlFD = virDMOpen()) < 0) + if ((controlFD = virDMOpen()) < 0) { + if (controlFD == -2) { + /* The CONTROL_PATH doesn't exist. Probably the + * module isn't loaded, yet. Don't error out, just + * exit. */ + return 0; + } + return -1; + } return virDevMapperGetTargetsImpl(controlFD, path, devPaths, ttl); } -- 2.26.2
On Tue, Aug 18, 2020 at 11:30:25 +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.
I'd probably mention the fact that all callers just configure some aspects of the DM devices thus it's guaranteed that there's nothing to do in this case and it's safe to ignore the error. Just I can't seem to come up with a nice formulation.
Fixes: 22494556542c676d1b9e7f1c1f2ea13ac17e1e3e Reported-by: Andrea Bolognani <abologna@redhat.com> Reported-by: Christian Ehrhardt <christian.ehrhardt@canonical.com> Signed-off-by: Michal Privoznik <mprivozn@redhat.com> ---
Regardless of the above: Reviewed-by: Peter Krempa <pkrempa@redhat.com>
src/util/virdevmapper.c | 20 ++++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-)
On Tue, Aug 18, 2020 at 11:36 AM Michal Privoznik <mprivozn@redhat.com> wrote:
v2 of:
https://www.redhat.com/archives/libvir-list/2020-August/msg00489.html
diff to v1: - After discussion to v1 I've decided to not cache DM major number and thus the patches looks a bit different.
Michal Prívozník (2): virdevmapper: Don't cache device-mapper major virdevmapper: Handle kernel without device-mapper support
Reviewed-by: Christian Ehrhardt <christian.ehrhardt@canonical.com> Builds have started to re-test it as well ...
src/util/virdevmapper.c | 35 ++++++++++++++++++++++------------- 1 file changed, 22 insertions(+), 13 deletions(-)
-- 2.26.2
-- Christian Ehrhardt Staff Engineer, Ubuntu Server Canonical Ltd
On Tue, Aug 18, 2020 at 12:11 PM Christian Ehrhardt <christian.ehrhardt@canonical.com> wrote:
On Tue, Aug 18, 2020 at 11:36 AM Michal Privoznik <mprivozn@redhat.com> wrote:
v2 of:
https://www.redhat.com/archives/libvir-list/2020-August/msg00489.html
diff to v1: - After discussion to v1 I've decided to not cache DM major number and thus the patches looks a bit different.
Michal Prívozník (2): virdevmapper: Don't cache device-mapper major virdevmapper: Handle kernel without device-mapper support
Reviewed-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
Builds have started to re-test it as well ...
Done and working Tested-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
src/util/virdevmapper.c | 35 ++++++++++++++++++++++------------- 1 file changed, 22 insertions(+), 13 deletions(-)
-- 2.26.2
-- Christian Ehrhardt Staff Engineer, Ubuntu Server Canonical Ltd
-- Christian Ehrhardt Staff Engineer, Ubuntu Server Canonical Ltd
On Tue, Aug 18, 2020 at 12:47 PM Christian Ehrhardt <christian.ehrhardt@canonical.com> wrote:
On Tue, Aug 18, 2020 at 12:11 PM Christian Ehrhardt <christian.ehrhardt@canonical.com> wrote:
On Tue, Aug 18, 2020 at 11:36 AM Michal Privoznik <mprivozn@redhat.com> wrote:
v2 of:
https://www.redhat.com/archives/libvir-list/2020-August/msg00489.html
diff to v1: - After discussion to v1 I've decided to not cache DM major number and thus the patches looks a bit different.
Michal Prívozník (2): virdevmapper: Don't cache device-mapper major virdevmapper: Handle kernel without device-mapper support
I put this through further testing and while working on containers in general now, I found it breaks if I expose (an unusable) /dev/mapper/control to the guest. The guest isn't able to access it properly, but it exists and that breaks libvirt 6.6 even with the recent virdevmapper fixes applied. Conditions: It is available: # grep mapper /proc/devices 253 device-mapper # ll /dev/mapper/control crw------- 1 root root 10, 236 Aug 19 08:36 /dev/mapper/control But not usable: # dmsetup table /dev/mapper/control: open failed: Operation not permitted Failure to communicate with kernel device-mapper driver. Check that device-mapper is available in the kernel. Incompatible libdevmapper 1.02.167 (2019-11-30) and kernel driver (unknown version). Command failed. I could further unconfine it to eventually work, but what I'd want for now is that as discusse don IRC we might consider more bad RCs as "devmapper not in use, don't remap paths" For reference `dmsetup table` also gets: 0.000082 stat("/dev/mapper/control", {st_mode=S_IFCHR|0600, st_rdev=makedev(0xa, 0xec), ...}) = 0 <0.000021> 0.000094 openat(AT_FDCWD, "/dev/mapper/control", O_RDWR) = -1 EPERM (Operation not permitted) <0.000021> Starting the guest fails similar like it did in former issues around this: # virsh start kvm-testguest-groovy-1 error: Failed to start domain kvm-testguest-groovy-1 error: internal error: Process exited prior to exec: libvirt: QEMU Driver error : Unable to get devmapper targets for /var/lib/uvtool/libvirt/images/kvm-testguest-groovy-1.qcow: Operation not permitted The strace of libvirt itself looks like: # grep -C 3 -e 'mapper\/control' libvirt.strace.* libvirt.strace.5332- 0.000066 lstat("/dev/ptmx", {st_mode=S_IFCHR|0666, st_rdev=makedev(0x5, 0x2), ...}) = 0 <0.000014> libvirt.strace.5332- 0.000079 lstat("/dev/kvm", {st_mode=S_IFCHR|0660, st_rdev=makedev(0xa, 0xe8), ...}) = 0 <0.000014> libvirt.strace.5332- 0.000072 lstat("/var/lib/uvtool/libvirt/images/kvm-testguest-groovy-1.qcow", {st_mode=S_IFREG|0600, st_size=229638144, ...}) = 0 <0.000018> libvirt.strace.5332: 0.000067 openat(AT_FDCWD, "/dev/mapper/control", O_RDWR) = -1 EPERM (Operation not permitted) <0.000017> libvirt.strace.5332- 0.000142 umount2("/run/libvirt/qemu/5-kvm-testguest-groovy.dev", 0) = 0 <0.000195> libvirt.strace.5332- 0.000254 stat("/run/libvirt/qemu/5-kvm-testguest-groovy.dev", {st_mode=S_IFDIR|0775, st_size=40, ...}) = 0 <0.000016> libvirt.strace.5332- 0.000068 access("/run/libvirt/qemu/5-kvm-testguest-groovy.dev", F_OK) = 0 <0.000015> And according to our IRC discussion: [10:33] <mprivozn> on the other hand, it might not be that dirty, if we can't open /dev/mapper/control for whatever reason then we can safely assume that devmapper is not available/able to answer our queries and thus no mpath devices can be on the system anyway [10:33] <cpaelzer_> true [10:33] <danpb> mprivozn: the only problem is that EPERM just indicates libvirtd can't access it [10:33] <mprivozn> cpaelzer_: either will show what the problem is; my guess is that open(/dev/mapper/control) in virDMOpen() fails [10:34] <danpb> mprivozn: doesn't mean it can't be in use [10:36] <mprivozn> danpb: agreed; but if we can't get deps for mpath (or determine that its mpath) then I'd say move on with domain startup and hope it wasn't mpath (which is a very narrow use case anyway) [10:36] <danpb> yeah probably best on balance So EPERM it is, would you add a change ignoring that RC please?
participants (3)
-
Christian Ehrhardt -
Michal Privoznik -
Peter Krempa