[PATCH] qemu: Create multipath targets for PRs
Jim Fehlig
jfehlig at suse.com
Thu Mar 12 03:07:59 UTC 2020
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 at 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 at 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>
>
More information about the libvir-list
mailing list