12:16 a.m.
Imagine you have a device mapper target which consists of 1..N disks
(e.g. it's a multipath device, LUKS disk, whatever). Now, libvirt
currently allows only the frontend (/dev/dm-1) in CGroups. However, this
can make qemu fail [1] so we need to allow all the backend devices too.
1: https://bugzilla.redhat.com/show_bug.cgi?id=1557769#c22
Michal Privoznik (5):
virlog: Don't log devmapper
utils: Introduce virFileGetMPathTargets
virfile: Introduce virFileMajMinToName
qemu: Handle multipath devices properly
news: Document multipath fix
docs/news.xml | 10 ++++
libvirt.spec.in | 2 +
src/libvirt_private.syms | 2 +
src/qemu/qemu_cgroup.c | 43 +++++++++++++-
src/qemu/qemu_domain.c | 58 +++++++++++++++++++
src/util/virfile.c | 143 +++++++++++++++++++++++++++++++++++++++++++++++
src/util/virfile.h | 7 +++
src/util/virlog.c | 23 ++++++++
8 files changed, 285 insertions(+), 3 deletions(-)
--
2.16.1
12:16 a.m.
New subject: [libvirt] [PATCH 1/5] virlog: Don't log devmapper
Unless overridden, libdevmapper logs all messages to stderr.
Therefore if something goes wrong in storage_backend_mpath.c or
parthelper.c we don't honour user set logging targets.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/util/virlog.c | 23 +++++++++++++++++++++++
1 file changed, 23 insertions(+)
diff --git a/src/util/virlog.c b/src/util/virlog.c
index dd927f0ba7..51f2d341db 100644
--- a/src/util/virlog.c
+++ b/src/util/virlog.c
@@ -40,6 +40,9 @@
#if HAVE_SYS_UN_H
# include <sys/un.h>
#endif
+#if WITH_DEVMAPPER
+# include <libdevmapper.h>
+#endif
#include "virerror.h"
#include "virlog.h"
@@ -258,6 +261,20 @@ virLogPriorityString(virLogPriority lvl)
}
+#ifdef WITH_DEVMAPPER
+static void
+virLogDM(int level ATTRIBUTE_UNUSED,
+ const char *file ATTRIBUTE_UNUSED,
+ int line ATTRIBUTE_UNUSED,
+ int dm_errno ATTRIBUTE_UNUSED,
+ const char *fmt ATTRIBUTE_UNUSED,
+ ...)
+{
+ return;
+}
+#endif
+
+
static int
virLogOnceInit(void)
{
@@ -289,6 +306,12 @@ virLogOnceInit(void)
NUL_TERMINATE(virLogHostname);
}
+#ifdef WITH_DEVMAPPER
+ /* Ideally, we would not need this. But libdevmapper prints
+ * error messages to stderr by default. Sad but true. */
+ dm_log_with_errno_init(virLogDM);
+#endif
+
virLogUnlock();
return 0;
}
--
2.16.1
2:49 a.m.
New subject: [libvirt] [PATCH 1/5] virlog: Don't log devmapper
On Mon, Mar 26, 2018 at 07:16:42 +0200, Michal Privoznik wrote:
Unless overridden, libdevmapper logs all messages to stderr.
Therefore if something goes wrong in storage_backend_mpath.c or
parthelper.c we don't honour user set logging targets.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/util/virlog.c | 23 +++++++++++++++++++++++
1 file changed, 23 insertions(+)
I think this should be done in the function which registers the mpath
storage backend. For now it should work as is, but given that we are
going to split the daemons this code will not really be necessary in
other places.
And it really does not have to do much with our logging code.
3:16 a.m.
New subject: [libvirt] [PATCH 1/5] virlog: Don't log devmapper
On 03/26/2018 09:49 AM, Peter Krempa wrote:
On Mon, Mar 26, 2018 at 07:16:42 +0200, Michal Privoznik wrote:
> Unless overridden, libdevmapper logs all messages to stderr.
> Therefore if something goes wrong in storage_backend_mpath.c or
> parthelper.c we don't honour user set logging targets.
>
> Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
> ---
> src/util/virlog.c | 23 +++++++++++++++++++++++
> 1 file changed, 23 insertions(+)
I think this should be done in the function which registers the mpath
storage backend. For now it should work as is, but given that we are
going to split the daemons this code will not really be necessary in
other places.
And it really does not have to do much with our logging code.
Well, in the next patch I'm introducing virFileGetMPathTargets() which
uses dm_* a lot. Now, since the function lives in src/util/ it can be
used by many drivers. Do we expect each one of them to provide dummy
callback on their own? I don't think so. I agree this is not a nice
patch, but I don't really see any better place for it.
Michal
3:45 a.m.
New subject: [libvirt] [PATCH 1/5] virlog: Don't log devmapper
On Mon, Mar 26, 2018 at 10:16:54 +0200, Michal Privoznik wrote:
On 03/26/2018 09:49 AM, Peter Krempa wrote:
> On Mon, Mar 26, 2018 at 07:16:42 +0200, Michal Privoznik wrote:
>> Unless overridden, libdevmapper logs all messages to stderr.
>> Therefore if something goes wrong in storage_backend_mpath.c or
>> parthelper.c we don't honour user set logging targets.
>>
>> Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
>> ---
>> src/util/virlog.c | 23 +++++++++++++++++++++++
>> 1 file changed, 23 insertions(+)
>
> I think this should be done in the function which registers the mpath
> storage backend. For now it should work as is, but given that we are
> going to split the daemons this code will not really be necessary in
> other places.
>
> And it really does not have to do much with our logging code.
>
Well, in the next patch I'm introducing virFileGetMPathTargets() which
uses dm_* a lot. Now, since the function lives in src/util/ it can be
used by many drivers. Do we expect each one of them to provide dummy
callback on their own? I don't think so. I agree this is not a nice
patch, but I don't really see any better place for it.
I've written another response to my original reply which explains a bit
more.
Basically, the code in the next patch should be put in a new file along
with a virOnce function to silence the logs if you don't want to move it
into the storage driver directly.
3:44 a.m.
New subject: [libvirt] [PATCH 1/5] virlog: Don't log devmapper
On Mon, Mar 26, 2018 at 09:49:08 +0200, Peter Krempa wrote:
On Mon, Mar 26, 2018 at 07:16:42 +0200, Michal Privoznik wrote:
> Unless overridden, libdevmapper logs all messages to stderr.
> Therefore if something goes wrong in storage_backend_mpath.c or
> parthelper.c we don't honour user set logging targets.
'parthelper.c' does not use our logging code and also does not use any
libdevmapper.h functions. According to my experiment the header file
include can be dropped.
>
> Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
> ---
> src/util/virlog.c | 23 +++++++++++++++++++++++
> 1 file changed, 23 insertions(+)
I think this should be done in the function which registers the mpath
storage backend. For now it should work as is, but given that we are
going to split the daemons this code will not really be necessary in
other places.
So looking at the new code you've added in this series, it still might
be desirable to move all the libdevmapper use into the storage driver
and keep it there, so users wanting to use multipath would need to
install the corresponding storage driver.
Given that moving it there might be impractical, I'll suggest that you
move it to a separate util file and add virOnce infrastructure to set
the error helpers.
3:50 a.m.
New subject: [libvirt] [PATCH 1/5] virlog: Don't log devmapper
On 03/26/2018 10:44 AM, Peter Krempa wrote:
On Mon, Mar 26, 2018 at 09:49:08 +0200, Peter Krempa wrote:
> On Mon, Mar 26, 2018 at 07:16:42 +0200, Michal Privoznik wrote:
>> Unless overridden, libdevmapper logs all messages to stderr.
>> Therefore if something goes wrong in storage_backend_mpath.c or
>> parthelper.c we don't honour user set logging targets.
'parthelper.c' does not use our logging code and also does not use any
libdevmapper.h functions. According to my experiment the header file
include can be dropped.
>>
>> Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
>> ---
>> src/util/virlog.c | 23 +++++++++++++++++++++++
>> 1 file changed, 23 insertions(+)
>
> I think this should be done in the function which registers the mpath
> storage backend. For now it should work as is, but given that we are
> going to split the daemons this code will not really be necessary in
> other places.
So looking at the new code you've added in this series, it still might
be desirable to move all the libdevmapper use into the storage driver
and keep it there, so users wanting to use multipath would need to
install the corresponding storage driver.
Given that moving it there might be impractical, I'll suggest that you
move it to a separate util file and add virOnce infrastructure to set
the error helpers.
Okay, fair enough. I'll wait for review on the rest of the patches
before I post v2.
Thanks,
Michal
12:16 a.m.
New subject: [libvirt] [PATCH 2/5] utils: Introduce virFileGetMPathTargets
This helper fetches targets for multipath devices.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/libvirt_private.syms | 1 +
src/util/virfile.c | 90 ++++++++++++++++++++++++++++++++++++++++++++++++
src/util/virfile.h | 4 +++
3 files changed, 95 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 03fe3b315f..1bbff72263 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1773,6 +1773,7 @@ virFileGetACLs;
virFileGetHugepageSize;
virFileGetMountReverseSubtree;
virFileGetMountSubtree;
+virFileGetMPathTargets;
virFileHasSuffix;
virFileInData;
virFileIsAbsPath;
diff --git a/src/util/virfile.c b/src/util/virfile.c
index 5e9bd2007a..f89e4bd823 100644
--- a/src/util/virfile.c
+++ b/src/util/virfile.c
@@ -64,6 +64,10 @@
# include <sys/ioctl.h>
#endif
+#ifdef WITH_DEVMAPPER
+# include <libdevmapper.h>
+#endif
+
#include "configmake.h"
#include "intprops.h"
#include "viralloc.h"
@@ -4227,3 +4231,89 @@ virFileWaitForExists(const char *path,
return 0;
}
+
+
+#ifdef WITH_DEVMAPPER
+/**
+ * virFileGetMPathTargets:
+ * @path: multipath device
+ * @devs: returned array of st_rdevs of @path targets
+ * @ndevs: size of @devs and @devNames
+ *
+ * For given @path figure out its targets, and store them in
+ * @devs. Items from @devs are meant to be used in
+ * minor() and major() to receive device MAJ:MIN pairs.
+ *
+ * If @path is not a multipath device, @ndevs is set to 0 and
+ * success is returned.
+ *
+ * If we don't have permissions to talk to kernel, -1 is returned
+ * and errno is set to EBADF.
+ *
+ * Returns 0 on success, -1 otherwise.
+ */
+int
+virFileGetMPathTargets(const char *path,
+ unsigned long long **devs,
+ size_t *ndevs)
+{
+ struct dm_deps *deps;
+ struct dm_task *dmt;
+ struct dm_info info;
+ size_t i;
+ int ret = -1;
+
+ *ndevs = 0;
+
+ if (!(dmt = dm_task_create(DM_DEVICE_DEPS)))
+ goto cleanup;
+
+ if (!dm_task_set_name(dmt, path)) {
+ if (errno == ENOENT) {
+ /* It's okay, @path is not managed by devmapper =>
+ * not a multipath device. */
+ ret = 0;
+ }
+ goto cleanup;
+ }
+
+ dm_task_no_open_count(dmt);
+
+ if (!dm_task_run(dmt))
+ goto cleanup;
+
+ if (!dm_task_get_info(dmt, &info))
+ goto cleanup;
+
+ if (!info.exists) {
+ ret = 0;
+ goto cleanup;
+ }
+
+ if (!(deps = dm_task_get_deps(dmt)))
+ goto cleanup;
+
+ if (VIR_ALLOC_N(*devs, deps->count) < 0)
+ goto cleanup;
+ *ndevs = deps->count;
+
+ for (i = 0; i < deps->count; i++)
+ (*devs)[i] = deps->device[i];
+
+ ret = 0;
+ cleanup:
+ dm_task_destroy(dmt);
+ return ret;
+}
+
+#else /* ! WITH_DEVMAPPER */
+
+int
+virFileGetMPathTargets(const char *path ATTRIBUTE_UNUSED,
+ unsigned long long **devs ATTRIBUTE_UNUSED,
+ size_t *ndevs ATTRIBUTE_UNUSED)
+{
+ errno = ENOSYS;
+ return -1;
+}
+#endif /* ! WITH_DEVMAPPER */
diff --git a/src/util/virfile.h b/src/util/virfile.h
index cd2a3867c2..0db8bf0b99 100644
--- a/src/util/virfile.h
+++ b/src/util/virfile.h
@@ -358,4 +358,8 @@ int virFileInData(int fd,
int *inData,
long long *length);
+int virFileGetMPathTargets(const char *path,
+ unsigned long long **devs,
+ size_t *ndevs);
+
#endif /* __VIR_FILE_H */
--
2.16.1
4:25 a.m.
New subject: [libvirt] [PATCH 2/5] utils: Introduce virFileGetMPathTargets
On Mon, Mar 26, 2018 at 07:16:43 +0200, Michal Privoznik wrote:
This helper fetches targets for multipath devices.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/libvirt_private.syms | 1 +
src/util/virfile.c | 90 ++++++++++++++++++++++++++++++++++++++++++++++++
src/util/virfile.h | 4 +++
3 files changed, 95 insertions(+)
As I've pointed in the conversation regarding patch 1/5, this should be
put into a separate file, so that it's obvious what needs to be removed
if this functionality will need to be put into the storage driver.
Additionally it then will be justified to use a virOnce there to set the
logging function for libdevmapper.
diff --git a/src/util/virfile.c b/src/util/virfile.c
index 5e9bd2007a..f89e4bd823 100644
--- a/src/util/virfile.c
+++ b/src/util/virfile.c
@@ -4227,3 +4231,89 @@ virFileWaitForExists(const char *path,
return 0;
}
+
+
+#ifdef WITH_DEVMAPPER
+/**
+ * virFileGetMPathTargets:
+ * @path: multipath device
+ * @devs: returned array of st_rdevs of @path targets
+ * @ndevs: size of @devs and @devNames
There's no 'devNames' parameter.
+ *
+ * For given @path figure out its targets, and store them in
+ * @devs. Items from @devs are meant to be used in
+ * minor() and major() to receive device MAJ:MIN pairs.
+ *
+ * If @path is not a multipath device, @ndevs is set to 0 and
+ * success is returned.
+ *
+ * If we don't have permissions to talk to kernel, -1 is returned
+ * and errno is set to EBADF.
You need to document that this function does not use libvirt errors.
+ *
+ * Returns 0 on success, -1 otherwise.
+ */
+int
+virFileGetMPathTargets(const char *path,
+ unsigned long long **devs,
struct 'dm_deps' uses uint64_t for this.
+ size_t *ndevs)
+{
+ struct dm_deps *deps;
+ struct dm_task *dmt;
+ struct dm_info info;
+ size_t i;
+ int ret = -1;
+
+ *ndevs = 0;
+
+ if (!(dmt = dm_task_create(DM_DEVICE_DEPS)))
+ goto cleanup;
+
+ if (!dm_task_set_name(dmt, path)) {
+ if (errno == ENOENT) {
+ /* It's okay, @path is not managed by devmapper =>
+ * not a multipath device. */
+ ret = 0;
+ }
+ goto cleanup;
+ }
+
+ dm_task_no_open_count(dmt);
+
+ if (!dm_task_run(dmt))
+ goto cleanup;
+
+ if (!dm_task_get_info(dmt, &info))
+ goto cleanup;
+
+ if (!info.exists) {
+ ret = 0;
+ goto cleanup;
+ }
+
+ if (!(deps = dm_task_get_deps(dmt)))
+ goto cleanup;
+
+ if (VIR_ALLOC_N(*devs, deps->count) < 0)
VIR_ALLOC_N_QUIET, since we don't do libvirt errors.
+ goto cleanup;
+ *ndevs = deps->count;
+
+ for (i = 0; i < deps->count; i++)
+ (*devs)[i] = deps->device[i];
+
+ ret = 0;
+ cleanup:
+ dm_task_destroy(dmt);
+ return ret;
+}
Okay this code is stol^W inspired by the 'dmsetup deps' functionality.
You might want to point that fact out, since it took some time to find
it.
The code looks good, but since it requires the virOnce stuff, v2 will be
needed.
12:16 a.m.
New subject: [libvirt] [PATCH 3/5] virfile: Introduce virFileMajMinToName
This function can be used to translate MAJ:MIN device pair into
/dev/blah name.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/libvirt_private.syms | 1 +
src/util/virfile.c | 53 ++++++++++++++++++++++++++++++++++++++++++++++++
src/util/virfile.h | 3 +++
3 files changed, 57 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 1bbff72263..a705bfd5ef 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1787,6 +1787,7 @@ virFileLength;
virFileLinkPointsTo;
virFileLock;
virFileLoopDeviceAssociate;
+virFileMajMinToName;
virFileMakeParentPath;
virFileMakePath;
virFileMakePathWithMode;
diff --git a/src/util/virfile.c b/src/util/virfile.c
index f89e4bd823..62620862a1 100644
--- a/src/util/virfile.c
+++ b/src/util/virfile.c
@@ -68,6 +68,12 @@
# include <libdevmapper.h>
#endif
+#ifdef MAJOR_IN_MKDEV
+# include <sys/mkdev.h>
+#elif MAJOR_IN_SYSMACROS
+# include <sys/sysmacros.h>
+#endif
+
#include "configmake.h"
#include "intprops.h"
#include "viralloc.h"
@@ -4317,3 +4323,50 @@ virFileGetMPathTargets(const char *path ATTRIBUTE_UNUSED,
return -1;
}
#endif /* ! WITH_DEVMAPPER */
+
+
+/**
+ * virFileMajMinToName:
+ * @device: device (rdev) to translate
+ * @name: returned name
+ *
+ * For given MAJ:MIN pair (stored in one integer like in st_rdev)
+ * fetch device name, e.g. 8:0 is translated to "/dev/sda".
+ * Caller is responsible for freeing @name when no longer needed.
+ *
+ * Returns 0 on success, -1 otherwise.
+ */
+int
+virFileMajMinToName(unsigned long long device,
+ char **name)
+{
+ struct stat sb;
+ char *sysfsPath = NULL;
+ char *link = NULL;
+ int ret = -1;
+
+ *name = NULL;
+ if (virAsprintf(&sysfsPath, "/sys/dev/block/%u:%u",
+ major(device), minor(device)) < 0)
+ goto cleanup;
+
+ if (lstat(sysfsPath, &sb) < 0)
+ goto cleanup;
+
+ if (!S_ISLNK(sb.st_mode)) {
+ errno = ENXIO;
+ goto cleanup;
+ }
+
+ if (virFileReadLink(sysfsPath, &link) < 0)
+ goto cleanup;
+
+ if (virAsprintf(name, "/dev/%s", last_component(link)) < 0)
+ goto cleanup;
+
+ ret = 0;
+ cleanup:
+ VIR_FREE(link);
+ VIR_FREE(sysfsPath);
+ return ret;
+}
diff --git a/src/util/virfile.h b/src/util/virfile.h
index 0db8bf0b99..390940dc98 100644
--- a/src/util/virfile.h
+++ b/src/util/virfile.h
@@ -362,4 +362,7 @@ int virFileGetMPathTargets(const char *path,
unsigned long long **devs,
size_t *ndevs);
+int virFileMajMinToName(unsigned long long device,
+ char **name);
+
#endif /* __VIR_FILE_H */
--
2.16.1
4:58 a.m.
New subject: [libvirt] [PATCH 3/5] virfile: Introduce virFileMajMinToName
On Mon, Mar 26, 2018 at 07:16:44 +0200, Michal Privoznik wrote:
This function can be used to translate MAJ:MIN device pair into
/dev/blah name.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/libvirt_private.syms | 1 +
src/util/virfile.c | 53 ++++++++++++++++++++++++++++++++++++++++++++++++
src/util/virfile.h | 3 +++
3 files changed, 57 insertions(+)
diff --git a/src/util/virfile.c b/src/util/virfile.c
index f89e4bd823..62620862a1 100644
--- a/src/util/virfile.c
+++ b/src/util/virfile.c
@@ -68,6 +68,12 @@
# include <libdevmapper.h>
#endif
+#ifdef MAJOR_IN_MKDEV
+# include <sys/mkdev.h>
+#elif MAJOR_IN_SYSMACROS
+# include <sys/sysmacros.h>
+#endif
+
#include "configmake.h"
#include "intprops.h"
#include "viralloc.h"
@@ -4317,3 +4323,50 @@ virFileGetMPathTargets(const char *path ATTRIBUTE_UNUSED,
return -1;
}
#endif /* ! WITH_DEVMAPPER */
+
+
+/**
+ * virFileMajMinToName:
+ * @device: device (rdev) to translate
+ * @name: returned name
I'd prefer if you supply the device identifiers already separated. That
means that the function in the previous patch should preferrably already
split them so that we don't need to do it here.
+ *
+ * For given MAJ:MIN pair (stored in one integer like in st_rdev)
+ * fetch device name, e.g. 8:0 is translated to "/dev/sda".
+ * Caller is responsible for freeing @name when no longer needed.
There is no info on how to treat errors.
+ *
+ * Returns 0 on success, -1 otherwise.
Why isn't the path returned directly?
+ */
+int
+virFileMajMinToName(unsigned long long device,
+ char **name)
+{
+ struct stat sb;
+ char *sysfsPath = NULL;
+ char *link = NULL;
+ int ret = -1;
+
+ *name = NULL;
+ if (virAsprintf(&sysfsPath, "/sys/dev/block/%u:%u",
+ major(device), minor(device)) < 0)
So the libdevmapper code is using a similar path to figure out the sysfs
path, but only for devicemapper devices since it accesses
/sys/dev/block/%u:%u/dm/name.
I've also found that '/dev/block/' has the information you might need.
Is there any particular reason to use sysfs? I'm not really persuaded
that the directory name in sysfs is good enough so if you can provide
where you've inspired yourself it will be beneficial.
Additionally virAsprintf reports libvirt errors but this function does
not.
+ goto cleanup;
+
+ if (lstat(sysfsPath, &sb) < 0)
+ goto cleanup;
+
+ if (!S_ISLNK(sb.st_mode)) {
+ errno = ENXIO;
+ goto cleanup;
+ }
+
+ if (virFileReadLink(sysfsPath, &link) < 0)
+ goto cleanup;
+
+ if (virAsprintf(name, "/dev/%s", last_component(link)) < 0)
+ goto cleanup;
So this is the fishy path. I'd prefer /dev/block since the path already
points to something in the /dev/ filesystem and isn't conjured up from
something that looks the same.
+
+ ret = 0;
+ cleanup:
+ VIR_FREE(link);
+ VIR_FREE(sysfsPath);
+ return ret;
+}
diff --git a/src/util/virfile.h b/src/util/virfile.h
index 0db8bf0b99..390940dc98 100644
--- a/src/util/virfile.h
+++ b/src/util/virfile.h
@@ -362,4 +362,7 @@ int virFileGetMPathTargets(const char *path,
unsigned long long **devs,
size_t *ndevs);
+int virFileMajMinToName(unsigned long long device,
+ char **name);
+
#endif /* __VIR_FILE_H */
--
2.16.1
--
libvir-list mailing list
libvir-list(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
5:13 a.m.
New subject: [libvirt] [PATCH 3/5] virfile: Introduce virFileMajMinToName
On 03/26/2018 11:58 AM, Peter Krempa wrote:
On Mon, Mar 26, 2018 at 07:16:44 +0200, Michal Privoznik wrote:
> This function can be used to translate MAJ:MIN device pair into
> /dev/blah name.
>
> Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
> ---
> src/libvirt_private.syms | 1 +
> src/util/virfile.c | 53 ++++++++++++++++++++++++++++++++++++++++++++++++
> src/util/virfile.h | 3 +++
> 3 files changed, 57 insertions(+)
>
> diff --git a/src/util/virfile.c b/src/util/virfile.c
> index f89e4bd823..62620862a1 100644
> --- a/src/util/virfile.c
> +++ b/src/util/virfile.c
> @@ -68,6 +68,12 @@
> # include <libdevmapper.h>
> #endif
>
> +#ifdef MAJOR_IN_MKDEV
> +# include <sys/mkdev.h>
> +#elif MAJOR_IN_SYSMACROS
> +# include <sys/sysmacros.h>
> +#endif
> +
> #include "configmake.h"
> #include "intprops.h"
> #include "viralloc.h"
> @@ -4317,3 +4323,50 @@ virFileGetMPathTargets(const char *path ATTRIBUTE_UNUSED,
> return -1;
> }
> #endif /* ! WITH_DEVMAPPER */
> +
> +
> +/**
> + * virFileMajMinToName:
> + * @device: device (rdev) to translate
> + * @name: returned name
I'd prefer if you supply the device identifiers already separated. That
means that the function in the previous patch should preferrably already
split them so that we don't need to do it here.
Okay.
> + *
> + * For given MAJ:MIN pair (stored in one integer like in st_rdev)
> + * fetch device name, e.g. 8:0 is translated to "/dev/sda".
> + * Caller is responsible for freeing @name when no longer needed.
There is no info on how to treat errors.
> + *
> + * Returns 0 on success, -1 otherwise.
Why isn't the path returned directly?
No reason. It's just that I'm used to functions returning an integer. I
can change it.
> + */
> +int
> +virFileMajMinToName(unsigned long long device,
> + char **name)
> +{
> + struct stat sb;
> + char *sysfsPath = NULL;
> + char *link = NULL;
> + int ret = -1;
> +
> + *name = NULL;
> + if (virAsprintf(&sysfsPath, "/sys/dev/block/%u:%u",
> + major(device), minor(device)) < 0)
So the libdevmapper code is using a similar path to figure out the sysfs
path, but only for devicemapper devices since it accesses
/sys/dev/block/%u:%u/dm/name.
I've also found that '/dev/block/' has the information you might need.
Is there any particular reason to use sysfs? I'm not really persuaded
that the directory name in sysfs is good enough so if you can provide
where you've inspired yourself it will be beneficial.
Okay, I'll switch to /dev/block/... Looks like I took too much
inspiration from other projects O:-)
Additionally virAsprintf reports libvirt errors but this function does
not.
> + goto cleanup;
> +
> + if (lstat(sysfsPath, &sb) < 0)
> + goto cleanup;
> +
> + if (!S_ISLNK(sb.st_mode)) {
> + errno = ENXIO;
> + goto cleanup;
> + }
> +
> + if (virFileReadLink(sysfsPath, &link) < 0)
> + goto cleanup;
> +
> + if (virAsprintf(name, "/dev/%s", last_component(link)) < 0)
> + goto cleanup;
So this is the fishy path. I'd prefer /dev/block since the path already
points to something in the /dev/ filesystem and isn't conjured up from
something that looks the same.
Ah, good point. So if I do plain:
virAsprintf(&path, "/dev/block/%u:%u", major(), minor())
I immediately get the correct symlink with both namespace and cgroup
codes know how to deal with. Cool, so this patch might be dropped.
Michal
12:16 a.m.
New subject: [libvirt] [PATCH 4/5] qemu: Handle multipath devices properly
https://bugzilla.redhat.com/show_bug.cgi?id=1557769
Problem with multipath devices is that there can be several other
devices 'hidden' behind them. For instance, /dev/dm-1 can
consist of /dev/sda, /dev/sdb and /dev/sdc. Therefore, when
setting up devices CGroup and namespaces we have to take this
into account.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
libvirt.spec.in | 2 ++
src/qemu/qemu_cgroup.c | 43 ++++++++++++++++++++++++++++++++++---
src/qemu/qemu_domain.c | 58 ++++++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 100 insertions(+), 3 deletions(-)
diff --git a/libvirt.spec.in b/libvirt.spec.in
index b55a947ec9..ebfac10866 100644
--- a/libvirt.spec.in
+++ b/libvirt.spec.in
@@ -796,6 +796,8 @@ Requires: gzip
Requires: bzip2
Requires: lzop
Requires: xz
+# For mpath devices
+Requires: device-mapper
%if 0%{?fedora} || 0%{?rhel} > 7
Requires: systemd-container
%endif
diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c
index b604edb31c..a2198c9789 100644
--- a/src/qemu/qemu_cgroup.c
+++ b/src/qemu/qemu_cgroup.c
@@ -60,7 +60,12 @@ qemuSetupImagePathCgroup(virDomainObjPtr vm,
{
qemuDomainObjPrivatePtr priv = vm->privateData;
int perms = VIR_CGROUP_DEVICE_READ;
- int ret;
+ unsigned long long *mpathdevs = NULL;
+ size_t nmpathdevs = 0;
+ size_t i;
+ char *devPath = NULL;
+ int rv;
+ int ret = -1;
if (!virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_DEVICES))
return 0;
@@ -71,12 +76,44 @@ qemuSetupImagePathCgroup(virDomainObjPtr vm,
VIR_DEBUG("Allow path %s, perms: %s",
path, virCgroupGetDevicePermsString(perms));
- ret = virCgroupAllowDevicePath(priv->cgroup, path, perms, true);
+ rv = virCgroupAllowDevicePath(priv->cgroup, path, perms, true);
virDomainAuditCgroupPath(vm, priv->cgroup, "allow", path,
virCgroupGetDevicePermsString(perms),
- ret);
+ rv);
+ if (rv < 0)
+ goto cleanup;
+ if (virFileGetMPathTargets(path, &mpathdevs, &nmpathdevs) < 0 &&
+ errno != ENOSYS && errno != EBADF) {
+ virReportSystemError(errno,
+ _("Unable to get mpath targets for %s"),
+ path);
+ goto cleanup;
+ }
+
+ for (i = 0; i < nmpathdevs; i++) {
+ if (virFileMajMinToName(mpathdevs[i], &devPath) < 0) {
+ virReportSystemError(errno,
+ _("Unable to translate %llx to device path"),
+ mpathdevs[i]);
+ goto cleanup;
+ }
+
+ rv = virCgroupAllowDevicePath(priv->cgroup, devPath, perms, true);
+
+ virDomainAuditCgroupPath(vm, priv->cgroup, "allow", devPath,
+ virCgroupGetDevicePermsString(perms),
+ rv);
+ if (rv < 0)
+ goto cleanup;
+ VIR_FREE(devPath);
+ }
+
+ ret = 0;
+ cleanup:
+ VIR_FREE(devPath);
+ VIR_FREE(mpathdevs);
return ret;
}
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 4aaf617dae..cd39eb5942 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -10105,6 +10105,10 @@ qemuDomainSetupDisk(virQEMUDriverConfigPtr cfg ATTRIBUTE_UNUSED,
{
virStorageSourcePtr next;
char *dst = NULL;
+ unsigned long long *mpathdevs = NULL;
+ size_t nmpathdevs = 0;
+ char *mpathPath = NULL;
+ size_t i;
int ret = -1;
for (next = disk->src; virStorageSourceIsBacking(next); next =
next->backingStore) {
@@ -10115,10 +10119,34 @@ qemuDomainSetupDisk(virQEMUDriverConfigPtr cfg
ATTRIBUTE_UNUSED,
if (qemuDomainCreateDevice(next->path, data, false) < 0)
goto cleanup;
+
+ if (virFileGetMPathTargets(next->path, &mpathdevs, &nmpathdevs) < 0
&&
+ errno != ENOSYS && errno != EBADF) {
+ virReportSystemError(errno,
+ _("Unable to get mpath targets for %s"),
+ next->path);
+ goto cleanup;
+ }
+
+ for (i = 0; i < nmpathdevs; i++) {
+ if (virFileMajMinToName(mpathdevs[i], &mpathPath) < 0) {
+ virReportSystemError(errno,
+ _("Unable to translate %llx to device
path"),
+ mpathdevs[i]);
+ goto cleanup;
+ }
+
+ if (qemuDomainCreateDevice(mpathPath, data, false) < 0)
+ goto cleanup;
+
+ VIR_FREE(mpathPath);
+ }
}
ret = 0;
cleanup:
+ VIR_FREE(mpathPath);
+ VIR_FREE(mpathdevs);
VIR_FREE(dst);
return ret;
}
@@ -11128,6 +11156,10 @@ qemuDomainNamespaceSetupDisk(virDomainObjPtr vm,
virStorageSourcePtr next;
char **paths = NULL;
size_t npaths = 0;
+ unsigned long long *mpathdevs = NULL;
+ char **mpathdevPaths = NULL;
+ size_t nmpathdevs = 0;
+ size_t i;
int ret = -1;
if (!qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT))
@@ -11142,6 +11174,29 @@ qemuDomainNamespaceSetupDisk(virDomainObjPtr vm,
if (VIR_APPEND_ELEMENT_COPY(paths, npaths, next->path) < 0)
goto cleanup;
+
+ if (virFileGetMPathTargets(next->path, &mpathdevs, &nmpathdevs) < 0
&&
+ errno != ENOSYS && errno != EBADF) {
+ virReportSystemError(errno,
+ _("Unable to get mpath targets for %s"),
+ next->path);
+ goto cleanup;
+ }
+
+ if (VIR_ALLOC_N(mpathdevPaths, nmpathdevs) < 0)
+ goto cleanup;
+
+ for (i = 0; i < nmpathdevs; i++) {
+ if (virFileMajMinToName(mpathdevs[i], &mpathdevPaths[i]) < 0) {
+ virReportSystemError(errno,
+ _("Unable to translate %llx to device
path"),
+ mpathdevs[i]);
+ goto cleanup;
+ }
+
+ if (VIR_APPEND_ELEMENT_COPY(paths, npaths, mpathdevPaths[i]) < 0)
+ goto cleanup;
+ }
}
if (qemuDomainNamespaceMknodPaths(vm, (const char **)paths, npaths) < 0)
@@ -11149,6 +11204,9 @@ qemuDomainNamespaceSetupDisk(virDomainObjPtr vm,
ret = 0;
cleanup:
+ for (i = 0; i < nmpathdevs; i++)
+ VIR_FREE(mpathdevPaths[i]);
+ VIR_FREE(mpathdevPaths);
VIR_FREE(paths);
return ret;
}
--
2.16.1
5:43 a.m.
New subject: [libvirt] [PATCH 4/5] qemu: Handle multipath devices properly
On Mon, Mar 26, 2018 at 07:16:45 +0200, Michal Privoznik wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1557769
Problem with multipath devices is that there can be several other
devices 'hidden' behind them. For instance, /dev/dm-1 can
consist of /dev/sda, /dev/sdb and /dev/sdc. Therefore, when
setting up devices CGroup and namespaces we have to take this
into account.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
libvirt.spec.in | 2 ++
src/qemu/qemu_cgroup.c | 43 ++++++++++++++++++++++++++++++++++---
src/qemu/qemu_domain.c | 58 ++++++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 100 insertions(+), 3 deletions(-)
diff --git a/libvirt.spec.in b/libvirt.spec.in
index b55a947ec9..ebfac10866 100644
--- a/libvirt.spec.in
+++ b/libvirt.spec.in
@@ -796,6 +796,8 @@ Requires: gzip
Requires: bzip2
Requires: lzop
Requires: xz
+# For mpath devices
+Requires: device-mapper
%if 0%{?fedora} || 0%{?rhel} > 7
Requires: systemd-container
%endif
diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c
index b604edb31c..a2198c9789 100644
--- a/src/qemu/qemu_cgroup.c
+++ b/src/qemu/qemu_cgroup.c
@@ -60,7 +60,12 @@ qemuSetupImagePathCgroup(virDomainObjPtr vm,
{
qemuDomainObjPrivatePtr priv = vm->privateData;
int perms = VIR_CGROUP_DEVICE_READ;
- int ret;
+ unsigned long long *mpathdevs = NULL;
+ size_t nmpathdevs = 0;
+ size_t i;
+ char *devPath = NULL;
+ int rv;
+ int ret = -1;
if (!virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_DEVICES))
return 0;
@@ -71,12 +76,44 @@ qemuSetupImagePathCgroup(virDomainObjPtr vm,
VIR_DEBUG("Allow path %s, perms: %s",
path, virCgroupGetDevicePermsString(perms));
- ret = virCgroupAllowDevicePath(priv->cgroup, path, perms, true);
+ rv = virCgroupAllowDevicePath(priv->cgroup, path, perms, true);
virDomainAuditCgroupPath(vm, priv->cgroup, "allow", path,
virCgroupGetDevicePermsString(perms),
- ret);
+ rv);
+ if (rv < 0)
+ goto cleanup;
+ if (virFileGetMPathTargets(path, &mpathdevs, &nmpathdevs) < 0 &&
+ errno != ENOSYS && errno != EBADF) {
+ virReportSystemError(errno,
+ _("Unable to get mpath targets for %s"),
+ path);
+ goto cleanup;
+ }
+
+ for (i = 0; i < nmpathdevs; i++) {
+ if (virFileMajMinToName(mpathdevs[i], &devPath) < 0) {
So for cgroups this really isn't necessary. You can use
virCgroupAllowDevice which already takes the major/minor numbers rather
than a path. Given that virCgroupAllowDevicePath would convert the path
to major/minor numbers, this conversion is really wasteful.
+ virReportSystemError(errno,
+ _("Unable to translate %llx to device
path"),
+ mpathdevs[i]);
This error message would be very useless, given that it's not coverted
into major/minor.
+ goto cleanup;
+ }
+
+ rv = virCgroupAllowDevicePath(priv->cgroup, devPath, perms, true);
+
+ virDomainAuditCgroupPath(vm, priv->cgroup, "allow", devPath,
+ virCgroupGetDevicePermsString(perms),
+ rv);
Hmm, right. The message in the audit log when allowing the device would
not contain the path to the device, which would make it slightly harder
to use.
+ if (rv < 0)
+ goto cleanup;
+ VIR_FREE(devPath);
+ }
+
+ ret = 0;
+ cleanup:
+ VIR_FREE(devPath);
+ VIR_FREE(mpathdevs);
return ret;
The only thing is that I'm not really a fan of calling the detection
code twice for every device you are going to add to the VM, once for
cgroups and once for namespaces. I think that the disk image setup code
should be extracted, so that the data needs to be obtained only once and
afterwards can be used to setup all the required images.
I'm not going to insist, but that also means that nobody will ever do
it. Especially since we for some reason hide all cgroup code setup into
qemu_cgroup. This also means that it has to be duplicated for
namespaces.
The code will require some changes if you implement my suggestions, but
it looks reasonable.
12:16 a.m.
New subject: [libvirt] [PATCH 5/5] news: Document multipath fix
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
docs/news.xml | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/docs/news.xml b/docs/news.xml
index 1088895746..a7ac63eddb 100644
--- a/docs/news.xml
+++ b/docs/news.xml
@@ -94,6 +94,16 @@
</change>
</section>
<section title="Bug fixes">
+ <change>
+ <summary>
+ Improve handling of multipath devices
+ </summary>
+ <description>
+ When starting a domain with a disk hidden behind
+ devmapper libvirt needs to allow them both in devices
+ CGroup: the devmapper target and the disk.
+ </description>
+ </change>
</section>
</release>
<release version="v4.1.0" date="2018-03-05">
--
2.16.1
2519
days inactive
2519
days old
14 comments
2 participants
participants (2)
-
Michal Privoznik -
Peter Krempa