src/qemu/qemu_domain.c | 64 ++++++++++------ src/util/virdevmapper.h | 4 +- src/util/virutil.h | 2 +- tests/qemuhotplugmock.c | 75 +++++++++++++++++++ tests/qemuhotplugtest.c | 13 ++++ .../qemuhotplug-disk-scsi-multipath.xml | 8 ++ ...uhotplug-base-live+disk-scsi-multipath.xml | 62 +++++++++++++++ 7 files changed, 204 insertions(+), 24 deletions(-) create mode 100644 tests/qemuhotplugtestdevices/qemuhotplug-disk-scsi-multipath.xml create mode 100644 tests/qemuhotplugtestdomains/qemuhotplug-base-live+disk-scsi-multipath.xml
If a disk has persistent reservations enabled, qemu-pr-helper
might open not only /dev/mapper/control but also individual
targets of the multipath device. We are already querying for them
in CGroups, but now we have to create them in the namespace too.
This was brought up in [1].
1: https://bugzilla.redhat.com/show_bug.cgi?id=1711045#c61
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
src/qemu/qemu_domain.c | 64 ++++++++++------
src/util/virdevmapper.h | 4 +-
src/util/virutil.h | 2 +-
tests/qemuhotplugmock.c | 75 +++++++++++++++++++
tests/qemuhotplugtest.c | 13 ++++
.../qemuhotplug-disk-scsi-multipath.xml | 8 ++
...uhotplug-base-live+disk-scsi-multipath.xml | 62 +++++++++++++++
7 files changed, 204 insertions(+), 24 deletions(-)
create mode 100644 tests/qemuhotplugtestdevices/qemuhotplug-disk-scsi-multipath.xml
create mode 100644 tests/qemuhotplugtestdomains/qemuhotplug-base-live+disk-scsi-multipath.xml
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 33c2158eb5..c8e6be7fae 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -62,6 +62,7 @@
#include "virdomaincheckpointobjlist.h"
#include "backup_conf.h"
#include "virutil.h"
+#include "virdevmapper.h"
#ifdef __linux__
# include <sys/sysmacros.h>
@@ -14574,6 +14575,9 @@ qemuDomainSetupDisk(virQEMUDriverConfigPtr cfg G_GNUC_UNUSED,
bool hasNVMe = false;
for (next = disk->src; virStorageSourceIsBacking(next); next = next->backingStore) {
+ VIR_AUTOSTRINGLIST targetPaths = NULL;
+ size_t i;
+
if (next->type == VIR_STORAGE_TYPE_NVME) {
g_autofree char *nvmePath = NULL;
@@ -14592,6 +14596,19 @@ qemuDomainSetupDisk(virQEMUDriverConfigPtr cfg G_GNUC_UNUSED,
if (qemuDomainCreateDevice(next->path, data, false) < 0)
return -1;
+
+ if (virDevMapperGetTargets(next->path, &targetPaths) < 0 &&
+ errno != ENOSYS && errno != EBADF) {
+ virReportSystemError(errno,
+ _("Unable to get devmapper targets for %s"),
+ next->path);
+ return -1;
+ }
+
+ for (i = 0; targetPaths && targetPaths[i]; i++) {
+ if (qemuDomainCreateDevice(targetPaths[i], data, false) < 0)
+ return -1;
+ }
}
}
@@ -15607,21 +15624,19 @@ qemuDomainNamespaceSetupDisk(virDomainObjPtr vm,
virStorageSourcePtr src)
{
virStorageSourcePtr next;
- char **paths = NULL;
+ VIR_AUTOSTRINGLIST paths = NULL;
size_t npaths = 0;
bool hasNVMe = false;
- g_autofree char *dmPath = NULL;
- g_autofree char *vfioPath = NULL;
- int ret = -1;
for (next = src; virStorageSourceIsBacking(next); next = next->backingStore) {
+ VIR_AUTOSTRINGLIST targetPaths = NULL;
g_autofree char *tmpPath = NULL;
if (next->type == VIR_STORAGE_TYPE_NVME) {
hasNVMe = true;
if (!(tmpPath = virPCIDeviceAddressGetIOMMUGroupDev(&next->nvme->pciAddr)))
- goto cleanup;
+ return -1;
} else {
if (virStorageSourceIsEmpty(next) ||
!virStorageSourceIsLocalStorage(next)) {
@@ -15632,30 +15647,35 @@ qemuDomainNamespaceSetupDisk(virDomainObjPtr vm,
tmpPath = g_strdup(next->path);
}
- if (VIR_APPEND_ELEMENT(paths, npaths, tmpPath) < 0)
- goto cleanup;
+ if (virStringListAdd(&paths, tmpPath) < 0)
+ return -1;
+
+ if (virDevMapperGetTargets(next->path, &targetPaths) < 0 &&
+ errno != ENOSYS && errno != EBADF) {
+ virReportSystemError(errno,
+ _("Unable to get devmapper targets for %s"),
+ next->path);
+ return -1;
+ }
+
+ if (virStringListMerge(&paths, &targetPaths) < 0)
+ return -1;
}
/* qemu-pr-helper might require access to /dev/mapper/control. */
- if (src->pr) {
- dmPath = g_strdup(QEMU_DEVICE_MAPPER_CONTROL_PATH);
- if (VIR_APPEND_ELEMENT_COPY(paths, npaths, dmPath) < 0)
- goto cleanup;
- }
+ if (src->pr &&
+ virStringListAdd(&paths, QEMU_DEVICE_MAPPER_CONTROL_PATH) < 0)
+ return -1;
- if (hasNVMe) {
- vfioPath = g_strdup(QEMU_DEV_VFIO);
- if (VIR_APPEND_ELEMENT(paths, npaths, vfioPath) < 0)
- goto cleanup;
- }
+ if (hasNVMe &&
+ virStringListAdd(&paths, QEMU_DEV_VFIO) < 0)
+ return -1;
+ npaths = virStringListLength((const char **) paths);
if (qemuDomainNamespaceMknodPaths(vm, (const char **) paths, npaths) < 0)
- goto cleanup;
+ return -1;
- ret = 0;
- cleanup:
- virStringListFreeCount(paths, npaths);
- return ret;
+ return 0;
}
diff --git a/src/util/virdevmapper.h b/src/util/virdevmapper.h
index e576d2bf7e..87bbc63cfd 100644
--- a/src/util/virdevmapper.h
+++ b/src/util/virdevmapper.h
@@ -20,6 +20,8 @@
#pragma once
+#include "internal.h"
+
int
virDevMapperGetTargets(const char *path,
- char ***devPaths);
+ char ***devPaths) G_GNUC_NO_INLINE;
diff --git a/src/util/virutil.h b/src/util/virutil.h
index cc6b82a177..ee23f0c1f4 100644
--- a/src/util/virutil.h
+++ b/src/util/virutil.h
@@ -120,7 +120,7 @@ bool virValidateWWN(const char *wwn);
int virGetDeviceID(const char *path,
int *maj,
- int *min);
+ int *min) G_GNUC_NO_INLINE;
int virSetDeviceUnprivSGIO(const char *path,
const char *sysfs_dir,
int unpriv_sgio);
diff --git a/tests/qemuhotplugmock.c b/tests/qemuhotplugmock.c
index 43a9d79051..8e5b07788d 100644
--- a/tests/qemuhotplugmock.c
+++ b/tests/qemuhotplugmock.c
@@ -19,7 +19,24 @@
#include <config.h>
#include "qemu/qemu_hotplug.h"
+#include "qemu/qemu_process.h"
#include "conf/domain_conf.h"
+#include "virdevmapper.h"
+#include "virutil.h"
+#include "virmock.h"
+
+static int (*real_virGetDeviceID)(const char *path, int *maj, int *min);
+static bool (*real_virFileExists)(const char *path);
+
+static void
+init_syms(void)
+{
+ if (real_virFileExists)
+ return;
+
+ VIR_MOCK_REAL_INIT(virGetDeviceID);
+ VIR_MOCK_REAL_INIT(virFileExists);
+}
unsigned long long
qemuDomainGetUnplugTimeout(virDomainObjPtr vm G_GNUC_UNUSED)
@@ -31,3 +48,61 @@ qemuDomainGetUnplugTimeout(virDomainObjPtr vm G_GNUC_UNUSED)
return 200;
return 100;
}
+
+
+int
+virDevMapperGetTargets(const char *path,
+ char ***devPaths)
+{
+ *devPaths = NULL;
+
+ if (STREQ(path, "/dev/mapper/virt")) {
+ *devPaths = g_new(char *, 4);
+ (*devPaths)[0] = g_strdup("/dev/block/8:0"); /* /dev/sda */
+ (*devPaths)[1] = g_strdup("/dev/block/8:16"); /* /dev/sdb */
+ (*devPaths)[2] = g_strdup("/dev/block/8:32"); /* /dev/sdc */
+ (*devPaths)[3] = NULL;
+ }
+
+ return 0;
+}
+
+
+int
+virGetDeviceID(const char *path, int *maj, int *min)
+{
+ init_syms();
+
+ if (STREQ(path, "/dev/mapper/virt")) {
+ *maj = 254;
+ *min = 0;
+ return 0;
+ }
+
+ return real_virGetDeviceID(path, maj, min);
+}
+
+
+bool
+virFileExists(const char *path)
+{
+ init_syms();
+
+ if (STREQ(path, "/dev/mapper/virt"))
+ return true;
+
+ return real_virFileExists(path);
+}
+
+
+int
+qemuProcessStartManagedPRDaemon(virDomainObjPtr vm G_GNUC_UNUSED)
+{
+ return 0;
+}
+
+
+void
+qemuProcessKillManagedPRDaemon(virDomainObjPtr vm G_GNUC_UNUSED)
+{
+}
diff --git a/tests/qemuhotplugtest.c b/tests/qemuhotplugtest.c
index e008c1bf0d..8b411d63f0 100644
--- a/tests/qemuhotplugtest.c
+++ b/tests/qemuhotplugtest.c
@@ -87,6 +87,8 @@ qemuHotplugCreateObjects(virDomainXMLOptionPtr xmlopt,
virQEMUCapsSet(priv->qemuCaps, QEMU_CAPS_VNC);
virQEMUCapsSet(priv->qemuCaps, QEMU_CAPS_SPICE);
virQEMUCapsSet(priv->qemuCaps, QEMU_CAPS_SPICE_FILE_XFER_DISABLE);
+ virQEMUCapsSet(priv->qemuCaps, QEMU_CAPS_PR_MANAGER_HELPER);
+ virQEMUCapsSet(priv->qemuCaps, QEMU_CAPS_SCSI_BLOCK);
if (qemuTestCapsCacheInsert(driver.qemuCapsCache, priv->qemuCaps) < 0)
return -1;
@@ -750,6 +752,17 @@ mymain(void)
"device_del", QMP_DEVICE_DELETED("scsi3-0-5-6") QMP_OK,
"human-monitor-command", HMP(""));
+ DO_TEST_ATTACH("base-live", "disk-scsi-multipath", false, true,
+ "object-add", QMP_OK,
+ "human-monitor-command", HMP("OK\\r\\n"),
+ "device_add", QMP_OK);
+ DO_TEST_DETACH("base-live", "disk-scsi-multipath", true, true,
+ "device_del", QMP_OK,
+ "human-monitor-command", HMP(""));
+ DO_TEST_DETACH("base-live", "disk-scsi-multipath", false, false,
+ "device_del", QMP_DEVICE_DELETED("scsi0-0-0-0") QMP_OK,
+ "human-monitor-command", HMP(""));
+
DO_TEST_ATTACH("base-live", "qemu-agent", false, true,
"chardev-add", QMP_OK,
"device_add", QMP_OK);
diff --git a/tests/qemuhotplugtestdevices/qemuhotplug-disk-scsi-multipath.xml b/tests/qemuhotplugtestdevices/qemuhotplug-disk-scsi-multipath.xml
new file mode 100644
index 0000000000..5a6f955284
--- /dev/null
+++ b/tests/qemuhotplugtestdevices/qemuhotplug-disk-scsi-multipath.xml
@@ -0,0 +1,8 @@
+<disk type='block' device='lun'>
+ <driver name='qemu' type='raw'/>
+ <source dev='/dev/mapper/virt'>
+ <reservations managed='yes'/>
+ </source>
+ <target dev='sda' bus='scsi'/>
+ <address type='drive' controller='0' bus='0' target='0' unit='0'/>
+</disk>
diff --git a/tests/qemuhotplugtestdomains/qemuhotplug-base-live+disk-scsi-multipath.xml b/tests/qemuhotplugtestdomains/qemuhotplug-base-live+disk-scsi-multipath.xml
new file mode 100644
index 0000000000..40af064d10
--- /dev/null
+++ b/tests/qemuhotplugtestdomains/qemuhotplug-base-live+disk-scsi-multipath.xml
@@ -0,0 +1,62 @@
+<domain type='kvm' id='7'>
+ <name>hotplug</name>
+ <uuid>d091ea82-29e6-2e34-3005-f02617b36e87</uuid>
+ <memory unit='KiB'>4194304</memory>
+ <currentMemory unit='KiB'>4194304</currentMemory>
+ <vcpu placement='static'>4</vcpu>
+ <os>
+ <type arch='x86_64' machine='pc'>hvm</type>
+ <boot dev='hd'/>
+ </os>
+ <features>
+ <acpi/>
+ <apic/>
+ <pae/>
+ </features>
+ <clock offset='utc'/>
+ <on_poweroff>destroy</on_poweroff>
+ <on_reboot>restart</on_reboot>
+ <on_crash>restart</on_crash>
+ <devices>
+ <emulator>/usr/bin/qemu-system-x86_64</emulator>
+ <disk type='block' device='lun'>
+ <driver name='qemu' type='raw'/>
+ <source dev='/dev/mapper/virt'>
+ <reservations managed='yes'>
+ <source type='unix' path='/tmp/lib/domain-7-hotplug/pr-helper0.sock' mode='client'/>
+ </reservations>
+ </source>
+ <backingStore/>
+ <target dev='sda' bus='scsi'/>
+ <alias name='scsi0-0-0-0'/>
+ <address type='drive' controller='0' bus='0' target='0' unit='0'/>
+ </disk>
+ <controller type='usb' index='0'>
+ <alias name='usb'/>
+ <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/>
+ </controller>
+ <controller type='ide' index='0'>
+ <alias name='ide'/>
+ <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x1'/>
+ </controller>
+ <controller type='scsi' index='0' model='virtio-scsi'>
+ <alias name='scsi0'/>
+ <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/>
+ </controller>
+ <controller type='pci' index='0' model='pci-root'>
+ <alias name='pci'/>
+ </controller>
+ <controller type='virtio-serial' index='0'>
+ <alias name='virtio-serial0'/>
+ <address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0x0'/>
+ </controller>
+ <input type='mouse' bus='ps2'>
+ <alias name='input0'/>
+ </input>
+ <input type='keyboard' bus='ps2'>
+ <alias name='input1'/>
+ </input>
+ <memballoon model='none'/>
+ </devices>
+ <seclabel type='none' model='none'/>
+</domain>
--
2.24.1
On 3/5/20 10:11 AM, Michal Privoznik wrote:
> If a disk has persistent reservations enabled, qemu-pr-helper
> might open not only /dev/mapper/control but also individual
> targets of the multipath device. We are already querying for them
> in CGroups, but now we have to create them in the namespace too.
> This was brought up in [1].
>
> 1: https://bugzilla.redhat.com/show_bug.cgi?id=1711045#c61
>
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> ---
> src/qemu/qemu_domain.c | 64 ++++++++++------
> src/util/virdevmapper.h | 4 +-
> src/util/virutil.h | 2 +-
> tests/qemuhotplugmock.c | 75 +++++++++++++++++++
> tests/qemuhotplugtest.c | 13 ++++
> .../qemuhotplug-disk-scsi-multipath.xml | 8 ++
> ...uhotplug-base-live+disk-scsi-multipath.xml | 62 +++++++++++++++
> 7 files changed, 204 insertions(+), 24 deletions(-)
> create mode 100644 tests/qemuhotplugtestdevices/qemuhotplug-disk-scsi-multipath.xml
> create mode 100644 tests/qemuhotplugtestdomains/qemuhotplug-base-live+disk-scsi-multipath.xml
>
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 33c2158eb5..c8e6be7fae 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -62,6 +62,7 @@
> #include "virdomaincheckpointobjlist.h"
> #include "backup_conf.h"
> #include "virutil.h"
> +#include "virdevmapper.h"
>
> #ifdef __linux__
> # include <sys/sysmacros.h>
> @@ -14574,6 +14575,9 @@ qemuDomainSetupDisk(virQEMUDriverConfigPtr cfg G_GNUC_UNUSED,
> bool hasNVMe = false;
>
> for (next = disk->src; virStorageSourceIsBacking(next); next = next->backingStore) {
> + VIR_AUTOSTRINGLIST targetPaths = NULL;
> + size_t i;
> +
> if (next->type == VIR_STORAGE_TYPE_NVME) {
> g_autofree char *nvmePath = NULL;
>
> @@ -14592,6 +14596,19 @@ qemuDomainSetupDisk(virQEMUDriverConfigPtr cfg G_GNUC_UNUSED,
>
> if (qemuDomainCreateDevice(next->path, data, false) < 0)
> return -1;
> +
> + if (virDevMapperGetTargets(next->path, &targetPaths) < 0 &&
> + errno != ENOSYS && errno != EBADF) {
> + virReportSystemError(errno,
> + _("Unable to get devmapper targets for %s"),
> + next->path);
> + return -1;
> + }
> +
> + for (i = 0; targetPaths && targetPaths[i]; i++) {
> + if (qemuDomainCreateDevice(targetPaths[i], data, false) < 0)
> + return -1;
> + }
> }
> }
>
> @@ -15607,21 +15624,19 @@ qemuDomainNamespaceSetupDisk(virDomainObjPtr vm,
> virStorageSourcePtr src)
> {
> virStorageSourcePtr next;
> - char **paths = NULL;
> + VIR_AUTOSTRINGLIST paths = NULL;
> size_t npaths = 0;
> bool hasNVMe = false;
> - g_autofree char *dmPath = NULL;
> - g_autofree char *vfioPath = NULL;
> - int ret = -1;
>
> for (next = src; virStorageSourceIsBacking(next); next = next->backingStore) {
> + VIR_AUTOSTRINGLIST targetPaths = NULL;
> g_autofree char *tmpPath = NULL;
>
> if (next->type == VIR_STORAGE_TYPE_NVME) {
> hasNVMe = true;
>
> if (!(tmpPath = virPCIDeviceAddressGetIOMMUGroupDev(&next->nvme->pciAddr)))
> - goto cleanup;
> + return -1;
> } else {
> if (virStorageSourceIsEmpty(next) ||
> !virStorageSourceIsLocalStorage(next)) {
> @@ -15632,30 +15647,35 @@ qemuDomainNamespaceSetupDisk(virDomainObjPtr vm,
> tmpPath = g_strdup(next->path);
> }
>
> - if (VIR_APPEND_ELEMENT(paths, npaths, tmpPath) < 0)
> - goto cleanup;
> + if (virStringListAdd(&paths, tmpPath) < 0)
> + return -1;
> +
> + if (virDevMapperGetTargets(next->path, &targetPaths) < 0 &&
> + errno != ENOSYS && errno != EBADF) {
> + virReportSystemError(errno,
> + _("Unable to get devmapper targets for %s"),
> + next->path);
> + return -1;
> + }
> +
> + if (virStringListMerge(&paths, &targetPaths) < 0)
> + return -1;
> }
>
> /* qemu-pr-helper might require access to /dev/mapper/control. */
> - if (src->pr) {
> - dmPath = g_strdup(QEMU_DEVICE_MAPPER_CONTROL_PATH);
> - if (VIR_APPEND_ELEMENT_COPY(paths, npaths, dmPath) < 0)
> - goto cleanup;
> - }
> + if (src->pr &&
> + virStringListAdd(&paths, QEMU_DEVICE_MAPPER_CONTROL_PATH) < 0)
> + return -1;
>
> - if (hasNVMe) {
> - vfioPath = g_strdup(QEMU_DEV_VFIO);
> - if (VIR_APPEND_ELEMENT(paths, npaths, vfioPath) < 0)
> - goto cleanup;
> - }
> + if (hasNVMe &&
> + virStringListAdd(&paths, QEMU_DEV_VFIO) < 0)
> + return -1;
>
> + npaths = virStringListLength((const char **) paths);
> if (qemuDomainNamespaceMknodPaths(vm, (const char **) paths, npaths) < 0)
> - goto cleanup;
> + return -1;
>
> - ret = 0;
> - cleanup:
> - virStringListFreeCount(paths, npaths);
> - return ret;
> + return 0;
It looks like a little bit of cleanup made its way into the patch ;-), but it lgtm.
Reviewed-by: Jim Fehlig <jfehlig@suse.com>
Regards,
Jim
> }
>
>
> diff --git a/src/util/virdevmapper.h b/src/util/virdevmapper.h
> index e576d2bf7e..87bbc63cfd 100644
> --- a/src/util/virdevmapper.h
> +++ b/src/util/virdevmapper.h
> @@ -20,6 +20,8 @@
>
> #pragma once
>
> +#include "internal.h"
> +
> int
> virDevMapperGetTargets(const char *path,
> - char ***devPaths);
> + char ***devPaths) G_GNUC_NO_INLINE;
> diff --git a/src/util/virutil.h b/src/util/virutil.h
> index cc6b82a177..ee23f0c1f4 100644
> --- a/src/util/virutil.h
> +++ b/src/util/virutil.h
> @@ -120,7 +120,7 @@ bool virValidateWWN(const char *wwn);
>
> int virGetDeviceID(const char *path,
> int *maj,
> - int *min);
> + int *min) G_GNUC_NO_INLINE;
> int virSetDeviceUnprivSGIO(const char *path,
> const char *sysfs_dir,
> int unpriv_sgio);
> diff --git a/tests/qemuhotplugmock.c b/tests/qemuhotplugmock.c
> index 43a9d79051..8e5b07788d 100644
> --- a/tests/qemuhotplugmock.c
> +++ b/tests/qemuhotplugmock.c
> @@ -19,7 +19,24 @@
> #include <config.h>
>
> #include "qemu/qemu_hotplug.h"
> +#include "qemu/qemu_process.h"
> #include "conf/domain_conf.h"
> +#include "virdevmapper.h"
> +#include "virutil.h"
> +#include "virmock.h"
> +
> +static int (*real_virGetDeviceID)(const char *path, int *maj, int *min);
> +static bool (*real_virFileExists)(const char *path);
> +
> +static void
> +init_syms(void)
> +{
> + if (real_virFileExists)
> + return;
> +
> + VIR_MOCK_REAL_INIT(virGetDeviceID);
> + VIR_MOCK_REAL_INIT(virFileExists);
> +}
>
> unsigned long long
> qemuDomainGetUnplugTimeout(virDomainObjPtr vm G_GNUC_UNUSED)
> @@ -31,3 +48,61 @@ qemuDomainGetUnplugTimeout(virDomainObjPtr vm G_GNUC_UNUSED)
> return 200;
> return 100;
> }
> +
> +
> +int
> +virDevMapperGetTargets(const char *path,
> + char ***devPaths)
> +{
> + *devPaths = NULL;
> +
> + if (STREQ(path, "/dev/mapper/virt")) {
> + *devPaths = g_new(char *, 4);
> + (*devPaths)[0] = g_strdup("/dev/block/8:0"); /* /dev/sda */
> + (*devPaths)[1] = g_strdup("/dev/block/8:16"); /* /dev/sdb */
> + (*devPaths)[2] = g_strdup("/dev/block/8:32"); /* /dev/sdc */
> + (*devPaths)[3] = NULL;
> + }
> +
> + return 0;
> +}
> +
> +
> +int
> +virGetDeviceID(const char *path, int *maj, int *min)
> +{
> + init_syms();
> +
> + if (STREQ(path, "/dev/mapper/virt")) {
> + *maj = 254;
> + *min = 0;
> + return 0;
> + }
> +
> + return real_virGetDeviceID(path, maj, min);
> +}
> +
> +
> +bool
> +virFileExists(const char *path)
> +{
> + init_syms();
> +
> + if (STREQ(path, "/dev/mapper/virt"))
> + return true;
> +
> + return real_virFileExists(path);
> +}
> +
> +
> +int
> +qemuProcessStartManagedPRDaemon(virDomainObjPtr vm G_GNUC_UNUSED)
> +{
> + return 0;
> +}
> +
> +
> +void
> +qemuProcessKillManagedPRDaemon(virDomainObjPtr vm G_GNUC_UNUSED)
> +{
> +}
> diff --git a/tests/qemuhotplugtest.c b/tests/qemuhotplugtest.c
> index e008c1bf0d..8b411d63f0 100644
> --- a/tests/qemuhotplugtest.c
> +++ b/tests/qemuhotplugtest.c
> @@ -87,6 +87,8 @@ qemuHotplugCreateObjects(virDomainXMLOptionPtr xmlopt,
> virQEMUCapsSet(priv->qemuCaps, QEMU_CAPS_VNC);
> virQEMUCapsSet(priv->qemuCaps, QEMU_CAPS_SPICE);
> virQEMUCapsSet(priv->qemuCaps, QEMU_CAPS_SPICE_FILE_XFER_DISABLE);
> + virQEMUCapsSet(priv->qemuCaps, QEMU_CAPS_PR_MANAGER_HELPER);
> + virQEMUCapsSet(priv->qemuCaps, QEMU_CAPS_SCSI_BLOCK);
>
> if (qemuTestCapsCacheInsert(driver.qemuCapsCache, priv->qemuCaps) < 0)
> return -1;
> @@ -750,6 +752,17 @@ mymain(void)
> "device_del", QMP_DEVICE_DELETED("scsi3-0-5-6") QMP_OK,
> "human-monitor-command", HMP(""));
>
> + DO_TEST_ATTACH("base-live", "disk-scsi-multipath", false, true,
> + "object-add", QMP_OK,
> + "human-monitor-command", HMP("OK\\r\\n"),
> + "device_add", QMP_OK);
> + DO_TEST_DETACH("base-live", "disk-scsi-multipath", true, true,
> + "device_del", QMP_OK,
> + "human-monitor-command", HMP(""));
> + DO_TEST_DETACH("base-live", "disk-scsi-multipath", false, false,
> + "device_del", QMP_DEVICE_DELETED("scsi0-0-0-0") QMP_OK,
> + "human-monitor-command", HMP(""));
> +
> DO_TEST_ATTACH("base-live", "qemu-agent", false, true,
> "chardev-add", QMP_OK,
> "device_add", QMP_OK);
> diff --git a/tests/qemuhotplugtestdevices/qemuhotplug-disk-scsi-multipath.xml b/tests/qemuhotplugtestdevices/qemuhotplug-disk-scsi-multipath.xml
> new file mode 100644
> index 0000000000..5a6f955284
> --- /dev/null
> +++ b/tests/qemuhotplugtestdevices/qemuhotplug-disk-scsi-multipath.xml
> @@ -0,0 +1,8 @@
> +<disk type='block' device='lun'>
> + <driver name='qemu' type='raw'/>
> + <source dev='/dev/mapper/virt'>
> + <reservations managed='yes'/>
> + </source>
> + <target dev='sda' bus='scsi'/>
> + <address type='drive' controller='0' bus='0' target='0' unit='0'/>
> +</disk>
> diff --git a/tests/qemuhotplugtestdomains/qemuhotplug-base-live+disk-scsi-multipath.xml b/tests/qemuhotplugtestdomains/qemuhotplug-base-live+disk-scsi-multipath.xml
> new file mode 100644
> index 0000000000..40af064d10
> --- /dev/null
> +++ b/tests/qemuhotplugtestdomains/qemuhotplug-base-live+disk-scsi-multipath.xml
> @@ -0,0 +1,62 @@
> +<domain type='kvm' id='7'>
> + <name>hotplug</name>
> + <uuid>d091ea82-29e6-2e34-3005-f02617b36e87</uuid>
> + <memory unit='KiB'>4194304</memory>
> + <currentMemory unit='KiB'>4194304</currentMemory>
> + <vcpu placement='static'>4</vcpu>
> + <os>
> + <type arch='x86_64' machine='pc'>hvm</type>
> + <boot dev='hd'/>
> + </os>
> + <features>
> + <acpi/>
> + <apic/>
> + <pae/>
> + </features>
> + <clock offset='utc'/>
> + <on_poweroff>destroy</on_poweroff>
> + <on_reboot>restart</on_reboot>
> + <on_crash>restart</on_crash>
> + <devices>
> + <emulator>/usr/bin/qemu-system-x86_64</emulator>
> + <disk type='block' device='lun'>
> + <driver name='qemu' type='raw'/>
> + <source dev='/dev/mapper/virt'>
> + <reservations managed='yes'>
> + <source type='unix' path='/tmp/lib/domain-7-hotplug/pr-helper0.sock' mode='client'/>
> + </reservations>
> + </source>
> + <backingStore/>
> + <target dev='sda' bus='scsi'/>
> + <alias name='scsi0-0-0-0'/>
> + <address type='drive' controller='0' bus='0' target='0' unit='0'/>
> + </disk>
> + <controller type='usb' index='0'>
> + <alias name='usb'/>
> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/>
> + </controller>
> + <controller type='ide' index='0'>
> + <alias name='ide'/>
> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x1'/>
> + </controller>
> + <controller type='scsi' index='0' model='virtio-scsi'>
> + <alias name='scsi0'/>
> + <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/>
> + </controller>
> + <controller type='pci' index='0' model='pci-root'>
> + <alias name='pci'/>
> + </controller>
> + <controller type='virtio-serial' index='0'>
> + <alias name='virtio-serial0'/>
> + <address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0x0'/>
> + </controller>
> + <input type='mouse' bus='ps2'>
> + <alias name='input0'/>
> + </input>
> + <input type='keyboard' bus='ps2'>
> + <alias name='input1'/>
> + </input>
> + <memballoon model='none'/>
> + </devices>
> + <seclabel type='none' model='none'/>
> +</domain>
>
Verified, This patch fixes the namespace issue while PR in guest with lun passed-through multipath device node.
Lin
________________________________
From: libvir-list-bounces@redhat.com <libvir-list-bounces@redhat.com> on behalf of Michal Privoznik <mprivozn@redhat.com>
Sent: Friday, March 6, 2020 1:11 AM
To: libvir-list@redhat.com <libvir-list@redhat.com>
Subject: [PATCH] qemu: Create multipath targets for PRs
If a disk has persistent reservations enabled, qemu-pr-helper
might open not only /dev/mapper/control but also individual
targets of the multipath device. We are already querying for them
in CGroups, but now we have to create them in the namespace too.
This was brought up in [1].
1: https://bugzilla.redhat.com/show_bug.cgi?id=1711045#c61
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
src/qemu/qemu_domain.c | 64 ++++++++++------
src/util/virdevmapper.h | 4 +-
src/util/virutil.h | 2 +-
tests/qemuhotplugmock.c | 75 +++++++++++++++++++
tests/qemuhotplugtest.c | 13 ++++
.../qemuhotplug-disk-scsi-multipath.xml | 8 ++
...uhotplug-base-live+disk-scsi-multipath.xml | 62 +++++++++++++++
7 files changed, 204 insertions(+), 24 deletions(-)
create mode 100644 tests/qemuhotplugtestdevices/qemuhotplug-disk-scsi-multipath.xml
create mode 100644 tests/qemuhotplugtestdomains/qemuhotplug-base-live+disk-scsi-multipath.xml
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 33c2158eb5..c8e6be7fae 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -62,6 +62,7 @@
#include "virdomaincheckpointobjlist.h"
#include "backup_conf.h"
#include "virutil.h"
+#include "virdevmapper.h"
#ifdef __linux__
# include <sys/sysmacros.h>
@@ -14574,6 +14575,9 @@ qemuDomainSetupDisk(virQEMUDriverConfigPtr cfg G_GNUC_UNUSED,
bool hasNVMe = false;
for (next = disk->src; virStorageSourceIsBacking(next); next = next->backingStore) {
+ VIR_AUTOSTRINGLIST targetPaths = NULL;
+ size_t i;
+
if (next->type == VIR_STORAGE_TYPE_NVME) {
g_autofree char *nvmePath = NULL;
@@ -14592,6 +14596,19 @@ qemuDomainSetupDisk(virQEMUDriverConfigPtr cfg G_GNUC_UNUSED,
if (qemuDomainCreateDevice(next->path, data, false) < 0)
return -1;
+
+ if (virDevMapperGetTargets(next->path, &targetPaths) < 0 &&
+ errno != ENOSYS && errno != EBADF) {
+ virReportSystemError(errno,
+ _("Unable to get devmapper targets for %s"),
+ next->path);
+ return -1;
+ }
+
+ for (i = 0; targetPaths && targetPaths[i]; i++) {
+ if (qemuDomainCreateDevice(targetPaths[i], data, false) < 0)
+ return -1;
+ }
}
}
@@ -15607,21 +15624,19 @@ qemuDomainNamespaceSetupDisk(virDomainObjPtr vm,
virStorageSourcePtr src)
{
virStorageSourcePtr next;
- char **paths = NULL;
+ VIR_AUTOSTRINGLIST paths = NULL;
size_t npaths = 0;
bool hasNVMe = false;
- g_autofree char *dmPath = NULL;
- g_autofree char *vfioPath = NULL;
- int ret = -1;
for (next = src; virStorageSourceIsBacking(next); next = next->backingStore) {
+ VIR_AUTOSTRINGLIST targetPaths = NULL;
g_autofree char *tmpPath = NULL;
if (next->type == VIR_STORAGE_TYPE_NVME) {
hasNVMe = true;
if (!(tmpPath = virPCIDeviceAddressGetIOMMUGroupDev(&next->nvme->pciAddr)))
- goto cleanup;
+ return -1;
} else {
if (virStorageSourceIsEmpty(next) ||
!virStorageSourceIsLocalStorage(next)) {
@@ -15632,30 +15647,35 @@ qemuDomainNamespaceSetupDisk(virDomainObjPtr vm,
tmpPath = g_strdup(next->path);
}
- if (VIR_APPEND_ELEMENT(paths, npaths, tmpPath) < 0)
- goto cleanup;
+ if (virStringListAdd(&paths, tmpPath) < 0)
+ return -1;
+
+ if (virDevMapperGetTargets(next->path, &targetPaths) < 0 &&
+ errno != ENOSYS && errno != EBADF) {
+ virReportSystemError(errno,
+ _("Unable to get devmapper targets for %s"),
+ next->path);
+ return -1;
+ }
+
+ if (virStringListMerge(&paths, &targetPaths) < 0)
+ return -1;
}
/* qemu-pr-helper might require access to /dev/mapper/control. */
- if (src->pr) {
- dmPath = g_strdup(QEMU_DEVICE_MAPPER_CONTROL_PATH);
- if (VIR_APPEND_ELEMENT_COPY(paths, npaths, dmPath) < 0)
- goto cleanup;
- }
+ if (src->pr &&
+ virStringListAdd(&paths, QEMU_DEVICE_MAPPER_CONTROL_PATH) < 0)
+ return -1;
- if (hasNVMe) {
- vfioPath = g_strdup(QEMU_DEV_VFIO);
- if (VIR_APPEND_ELEMENT(paths, npaths, vfioPath) < 0)
- goto cleanup;
- }
+ if (hasNVMe &&
+ virStringListAdd(&paths, QEMU_DEV_VFIO) < 0)
+ return -1;
+ npaths = virStringListLength((const char **) paths);
if (qemuDomainNamespaceMknodPaths(vm, (const char **) paths, npaths) < 0)
- goto cleanup;
+ return -1;
- ret = 0;
- cleanup:
- virStringListFreeCount(paths, npaths);
- return ret;
+ return 0;
}
diff --git a/src/util/virdevmapper.h b/src/util/virdevmapper.h
index e576d2bf7e..87bbc63cfd 100644
--- a/src/util/virdevmapper.h
+++ b/src/util/virdevmapper.h
@@ -20,6 +20,8 @@
#pragma once
+#include "internal.h"
+
int
virDevMapperGetTargets(const char *path,
- char ***devPaths);
+ char ***devPaths) G_GNUC_NO_INLINE;
diff --git a/src/util/virutil.h b/src/util/virutil.h
index cc6b82a177..ee23f0c1f4 100644
--- a/src/util/virutil.h
+++ b/src/util/virutil.h
@@ -120,7 +120,7 @@ bool virValidateWWN(const char *wwn);
int virGetDeviceID(const char *path,
int *maj,
- int *min);
+ int *min) G_GNUC_NO_INLINE;
int virSetDeviceUnprivSGIO(const char *path,
const char *sysfs_dir,
int unpriv_sgio);
diff --git a/tests/qemuhotplugmock.c b/tests/qemuhotplugmock.c
index 43a9d79051..8e5b07788d 100644
--- a/tests/qemuhotplugmock.c
+++ b/tests/qemuhotplugmock.c
@@ -19,7 +19,24 @@
#include <config.h>
#include "qemu/qemu_hotplug.h"
+#include "qemu/qemu_process.h"
#include "conf/domain_conf.h"
+#include "virdevmapper.h"
+#include "virutil.h"
+#include "virmock.h"
+
+static int (*real_virGetDeviceID)(const char *path, int *maj, int *min);
+static bool (*real_virFileExists)(const char *path);
+
+static void
+init_syms(void)
+{
+ if (real_virFileExists)
+ return;
+
+ VIR_MOCK_REAL_INIT(virGetDeviceID);
+ VIR_MOCK_REAL_INIT(virFileExists);
+}
unsigned long long
qemuDomainGetUnplugTimeout(virDomainObjPtr vm G_GNUC_UNUSED)
@@ -31,3 +48,61 @@ qemuDomainGetUnplugTimeout(virDomainObjPtr vm G_GNUC_UNUSED)
return 200;
return 100;
}
+
+
+int
+virDevMapperGetTargets(const char *path,
+ char ***devPaths)
+{
+ *devPaths = NULL;
+
+ if (STREQ(path, "/dev/mapper/virt")) {
+ *devPaths = g_new(char *, 4);
+ (*devPaths)[0] = g_strdup("/dev/block/8:0"); /* /dev/sda */
+ (*devPaths)[1] = g_strdup("/dev/block/8:16"); /* /dev/sdb */
+ (*devPaths)[2] = g_strdup("/dev/block/8:32"); /* /dev/sdc */
+ (*devPaths)[3] = NULL;
+ }
+
+ return 0;
+}
+
+
+int
+virGetDeviceID(const char *path, int *maj, int *min)
+{
+ init_syms();
+
+ if (STREQ(path, "/dev/mapper/virt")) {
+ *maj = 254;
+ *min = 0;
+ return 0;
+ }
+
+ return real_virGetDeviceID(path, maj, min);
+}
+
+
+bool
+virFileExists(const char *path)
+{
+ init_syms();
+
+ if (STREQ(path, "/dev/mapper/virt"))
+ return true;
+
+ return real_virFileExists(path);
+}
+
+
+int
+qemuProcessStartManagedPRDaemon(virDomainObjPtr vm G_GNUC_UNUSED)
+{
+ return 0;
+}
+
+
+void
+qemuProcessKillManagedPRDaemon(virDomainObjPtr vm G_GNUC_UNUSED)
+{
+}
diff --git a/tests/qemuhotplugtest.c b/tests/qemuhotplugtest.c
index e008c1bf0d..8b411d63f0 100644
--- a/tests/qemuhotplugtest.c
+++ b/tests/qemuhotplugtest.c
@@ -87,6 +87,8 @@ qemuHotplugCreateObjects(virDomainXMLOptionPtr xmlopt,
virQEMUCapsSet(priv->qemuCaps, QEMU_CAPS_VNC);
virQEMUCapsSet(priv->qemuCaps, QEMU_CAPS_SPICE);
virQEMUCapsSet(priv->qemuCaps, QEMU_CAPS_SPICE_FILE_XFER_DISABLE);
+ virQEMUCapsSet(priv->qemuCaps, QEMU_CAPS_PR_MANAGER_HELPER);
+ virQEMUCapsSet(priv->qemuCaps, QEMU_CAPS_SCSI_BLOCK);
if (qemuTestCapsCacheInsert(driver.qemuCapsCache, priv->qemuCaps) < 0)
return -1;
@@ -750,6 +752,17 @@ mymain(void)
"device_del", QMP_DEVICE_DELETED("scsi3-0-5-6") QMP_OK,
"human-monitor-command", HMP(""));
+ DO_TEST_ATTACH("base-live", "disk-scsi-multipath", false, true,
+ "object-add", QMP_OK,
+ "human-monitor-command", HMP("OK\\r\\n"),
+ "device_add", QMP_OK);
+ DO_TEST_DETACH("base-live", "disk-scsi-multipath", true, true,
+ "device_del", QMP_OK,
+ "human-monitor-command", HMP(""));
+ DO_TEST_DETACH("base-live", "disk-scsi-multipath", false, false,
+ "device_del", QMP_DEVICE_DELETED("scsi0-0-0-0") QMP_OK,
+ "human-monitor-command", HMP(""));
+
DO_TEST_ATTACH("base-live", "qemu-agent", false, true,
"chardev-add", QMP_OK,
"device_add", QMP_OK);
diff --git a/tests/qemuhotplugtestdevices/qemuhotplug-disk-scsi-multipath.xml b/tests/qemuhotplugtestdevices/qemuhotplug-disk-scsi-multipath.xml
new file mode 100644
index 0000000000..5a6f955284
--- /dev/null
+++ b/tests/qemuhotplugtestdevices/qemuhotplug-disk-scsi-multipath.xml
@@ -0,0 +1,8 @@
+<disk type='block' device='lun'>
+ <driver name='qemu' type='raw'/>
+ <source dev='/dev/mapper/virt'>
+ <reservations managed='yes'/>
+ </source>
+ <target dev='sda' bus='scsi'/>
+ <address type='drive' controller='0' bus='0' target='0' unit='0'/>
+</disk>
diff --git a/tests/qemuhotplugtestdomains/qemuhotplug-base-live+disk-scsi-multipath.xml b/tests/qemuhotplugtestdomains/qemuhotplug-base-live+disk-scsi-multipath.xml
new file mode 100644
index 0000000000..40af064d10
--- /dev/null
+++ b/tests/qemuhotplugtestdomains/qemuhotplug-base-live+disk-scsi-multipath.xml
@@ -0,0 +1,62 @@
+<domain type='kvm' id='7'>
+ <name>hotplug</name>
+ <uuid>d091ea82-29e6-2e34-3005-f02617b36e87</uuid>
+ <memory unit='KiB'>4194304</memory>
+ <currentMemory unit='KiB'>4194304</currentMemory>
+ <vcpu placement='static'>4</vcpu>
+ <os>
+ <type arch='x86_64' machine='pc'>hvm</type>
+ <boot dev='hd'/>
+ </os>
+ <features>
+ <acpi/>
+ <apic/>
+ <pae/>
+ </features>
+ <clock offset='utc'/>
+ <on_poweroff>destroy</on_poweroff>
+ <on_reboot>restart</on_reboot>
+ <on_crash>restart</on_crash>
+ <devices>
+ <emulator>/usr/bin/qemu-system-x86_64</emulator>
+ <disk type='block' device='lun'>
+ <driver name='qemu' type='raw'/>
+ <source dev='/dev/mapper/virt'>
+ <reservations managed='yes'>
+ <source type='unix' path='/tmp/lib/domain-7-hotplug/pr-helper0.sock' mode='client'/>
+ </reservations>
+ </source>
+ <backingStore/>
+ <target dev='sda' bus='scsi'/>
+ <alias name='scsi0-0-0-0'/>
+ <address type='drive' controller='0' bus='0' target='0' unit='0'/>
+ </disk>
+ <controller type='usb' index='0'>
+ <alias name='usb'/>
+ <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/>
+ </controller>
+ <controller type='ide' index='0'>
+ <alias name='ide'/>
+ <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x1'/>
+ </controller>
+ <controller type='scsi' index='0' model='virtio-scsi'>
+ <alias name='scsi0'/>
+ <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/>
+ </controller>
+ <controller type='pci' index='0' model='pci-root'>
+ <alias name='pci'/>
+ </controller>
+ <controller type='virtio-serial' index='0'>
+ <alias name='virtio-serial0'/>
+ <address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0x0'/>
+ </controller>
+ <input type='mouse' bus='ps2'>
+ <alias name='input0'/>
+ </input>
+ <input type='keyboard' bus='ps2'>
+ <alias name='input1'/>
+ </input>
+ <memballoon model='none'/>
+ </devices>
+ <seclabel type='none' model='none'/>
+</domain>
--
2.24.1
On 3/11/20 8:29 AM, Lin Ma wrote: > Verified, This patch fixes the namespace issue while PR in guest with > lun passed-through multipath device node. > Awesome, thanks for testing it! If you want, I can add Tested-by: to the commit message to give you credit. Michal
© 2016 - 2024 Red Hat, Inc.