10:30 a.m.
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
10:30 a.m.
New subject: [PATCH v2 1/2] virdevmapper: Don't cache device-mapper major
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(a)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
10:51 a.m.
New subject: [PATCH v2 1/2] virdevmapper: Don't cache device-mapper major
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(a)redhat.com>
---
src/util/virdevmapper.c | 17 +++++------------
1 file changed, 5 insertions(+), 12 deletions(-)
Reviewed-by: Peter Krempa <pkrempa(a)redhat.com>
10:30 a.m.
New subject: [PATCH v2 2/2] virdevmapper: Handle kernel without device-mapper support
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>
Reported-by: Christian Ehrhardt <christian.ehrhardt(a)canonical.com>
Signed-off-by: Michal Privoznik <mprivozn(a)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
10:59 a.m.
New subject: [PATCH v2 2/2] virdevmapper: Handle kernel without device-mapper support
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(a)redhat.com>
Reported-by: Christian Ehrhardt <christian.ehrhardt(a)canonical.com>
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
Regardless of the above:
Reviewed-by: Peter Krempa <pkrempa(a)redhat.com>
src/util/virdevmapper.c | 20 ++++++++++++++++++--
1 file changed, 18 insertions(+), 2 deletions(-)
11:11 a.m.
On Tue, Aug 18, 2020 at 11:36 AM Michal Privoznik <mprivozn(a)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(a)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
11:47 a.m.
On Tue, Aug 18, 2020 at 12:11 PM Christian Ehrhardt
<christian.ehrhardt(a)canonical.com> wrote:
On Tue, Aug 18, 2020 at 11:36 AM Michal Privoznik <mprivozn(a)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(a)canonical.com>
Builds have started to re-test it as well ...
Done and working
Tested-by: Christian Ehrhardt <christian.ehrhardt(a)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
9:50 a.m.
On Tue, Aug 18, 2020 at 12:47 PM Christian Ehrhardt
<christian.ehrhardt(a)canonical.com> wrote:
On Tue, Aug 18, 2020 at 12:11 PM Christian Ehrhardt
<christian.ehrhardt(a)canonical.com> wrote:
>
> On Tue, Aug 18, 2020 at 11:36 AM Michal Privoznik <mprivozn(a)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?
1782
days inactive
1783
days old
8 comments
3 participants
participants (3)
-
Christian Ehrhardt -
Michal Privoznik -
Peter Krempa