This is a definition that holds information on SCSI persistent reservation settings. The XML part looks like this: <reservations enabled='yes' managed='no'> <source type='unix' path='/path/to/qemu-pr-helper.sock' mode='client'/> </reservations> If @managed is set to 'yes' then the <source/> is not parsed. This design was agreed on here: https://www.redhat.com/archives/libvir-list/2017-November/msg01005.html Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- docs/formatdomain.html.in | 25 +++- docs/schemas/domaincommon.rng | 34 +----- docs/schemas/storagecommon.rng | 50 ++++++++ src/conf/domain_conf.c | 38 ++++++ src/libvirt_private.syms | 3 + src/util/virstoragefile.c | 131 +++++++++++++++++++++ src/util/virstoragefile.h | 14 +++ .../disk-virtio-scsi-reservations.xml | 49 ++++++++ .../disk-virtio-scsi-reservations.xml | 1 + tests/qemuxml2xmltest.c | 2 + 10 files changed, 316 insertions(+), 31 deletions(-) create mode 100644 tests/qemuxml2argvdata/disk-virtio-scsi-reservations.xml create mode 120000 tests/qemuxml2xmloutdata/disk-virtio-scsi-reservations.xml diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 5e99884dc5..c20e98b4ef 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -2563,7 +2563,10 @@ &lt;/disk&gt; &lt;disk type='block' device='lun'&gt; &lt;driver name='qemu' type='raw'/&gt; - &lt;source dev='/dev/sda'/&gt; + &lt;source dev='/dev/sda'&gt; + &lt;reservations enabled='yes' managed='no'&gt; + &lt;source type='unix' path='/path/to/qemu-pr-helper' mode='client'/&gt; + &lt;/reservations&gt; &lt;target dev='sda' bus='scsi'/&gt; &lt;address type='drive' controller='0' bus='0' target='3' unit='0'/&gt; &lt;/disk&gt; @@ -2926,6 +2929,26 @@ <a href="formatstorageencryption.html">Storage Encryption</a> page for more information. </dd> + <dt><code>reservations</code></dt> + <dd><span class="since">Since libvirt 4.1.0</span>, the + <code>reservations</code> can be a sub-element of the + <code>source</code> element for storage sources (QEMU driver only). + If present (and enabled) it enabled persistent reservations for + SCSI based disks. The element has one mandatory attribute + <code>enabled</code> with accepted values <code>yes</code> and + <code>no</code>. If the feature is enabled, then there's another + mandatory attribute <code>managed</code> (accepted values are the + same as for <code>enabled</code>) that enables or disables libvirt + spawning any helper process (should one be needed). However, if + libvirt is not enabled spawning helper process (i.e. hypervisor + should just use one which is already running), path to its socket + must be provided in child element <code>source</code>, which + currently accepts only the following attributes: <code>type</code> + with one value <code>unix</code>, <code>path</code> with path the + socket, and finally <code>mode</code> which accepts either + <code>server</code> or <code>client</code> and specifies the role + of hypervisor. + </dd> </dl> <p> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 4cab55f05d..93084887fb 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -1530,6 +1530,9 @@ <optional> <ref name="encryption"/> </optional> + <optional> + <ref name="reservations"/> + </optional> <zeroOrMore> <ref name='devSeclabel'/> </zeroOrMore> @@ -2434,18 +2437,6 @@ </attribute> </optional> </define> - <define name="reconnect"> - <element name="reconnect"> - <attribute name="enabled"> - <ref name="virYesNo"/> - </attribute> - <optional> - <attribute name="timeout"> - <ref name="unsignedInt"/> - </attribute> - </optional> - </element> - </define> <!-- An interface description can either be of type bridge in which case @@ -2494,24 +2485,7 @@ <value>vhostuser</value> </attribute> <interleave> - <element name="source"> - <attribute name="type"> - <value>unix</value> - </attribute> - <attribute name="path"> - <ref name="absFilePath"/> - </attribute> - <attribute name="mode"> - <choice> - <value>server</value> - <value>client</value> - </choice> - </attribute> - <optional> - <ref name="reconnect"/> - </optional> - <empty/> - </element> + <ref name="unixSocketSource"/> <ref name="interface-options"/> </interleave> </group> diff --git a/docs/schemas/storagecommon.rng b/docs/schemas/storagecommon.rng index edee1b0845..eed0b33347 100644 --- a/docs/schemas/storagecommon.rng +++ b/docs/schemas/storagecommon.rng @@ -39,6 +39,56 @@ </element> </define> + <define name="reconnect"> + <element name="reconnect"> + <attribute name="enabled"> + <ref name="virYesNo"/> + </attribute> + <optional> + <attribute name="timeout"> + <ref name="unsignedInt"/> + </attribute> + </optional> + </element> + </define> + + <define name='unixSocketSource'> + <element name="source"> + <attribute name="type"> + <value>unix</value> + </attribute> + <attribute name="path"> + <ref name="absFilePath"/> + </attribute> + <attribute name="mode"> + <choice> + <value>server</value> + <value>client</value> + </choice> + </attribute> + <optional> + <ref name="reconnect"/> + </optional> + <empty/> + </element> + </define> + + <define name='reservations'> + <element name='reservations'> + <attribute name='enabled'> + <ref name='virYesNo'/> + </attribute> + <optional> + <attribute name='managed'> + <ref name='virYesNo'/> + </attribute> + </optional> + <optional> + <ref name='unixSocketSource'/> + </optional> + </element> + </define> + <define name='secret'> <element name='secret'> <attribute name='type'> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index d23182f18a..5943d05e0e 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -5210,6 +5210,13 @@ virDomainDiskDefValidate(const virDomainDiskDef *disk) } } + if (disk->src->pr && + disk->device != VIR_DOMAIN_DISK_DEVICE_LUN) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("<reservations/> allowed only for lun disks")); + return -1; + } + /* Reject disks with a bus type that is not compatible with the * given address type. The function considers only buses that are * handled in common code. For other bus types it's not possible @@ -8602,6 +8609,31 @@ virDomainDiskSourcePrivateDataParse(xmlNodePtr node, } +static int +virDomainDiskSourcePRParse(xmlNodePtr node, + xmlXPathContextPtr ctxt, + virStoragePRDefPtr *pr) +{ + xmlNodePtr saveNode = ctxt->node; + int ret = -1; + + ctxt->node = node; + + if (!(ctxt->node = virXPathNode("./reservations", ctxt))) { + ret = 0; + goto cleanup; + } + + if (!(*pr = virStoragePRDefParseXML(ctxt))) + goto cleanup; + + ret = 0; + cleanup: + ctxt->node = saveNode; + return ret; +} + + static int virDomainStorageSourceParse(xmlNodePtr node, xmlXPathContextPtr ctxt, @@ -8648,6 +8680,9 @@ virDomainStorageSourceParse(xmlNodePtr node, !(src->encryption = virStorageEncryptionParseNode(tmp, ctxt))) goto cleanup; + if (virDomainDiskSourcePRParse(node, ctxt, &src->pr) < 0) + goto cleanup; + if (virSecurityDeviceLabelDefParseXML(&src->seclabels, &src->nseclabels, ctxt, flags) < 0) goto cleanup; @@ -22927,6 +22962,9 @@ virDomainStorageSourceFormat(virBufferPtr attrBuf, virStorageEncryptionFormat(childBuf, src->encryption) < 0) return -1; + if (src->pr) + virStoragePRDefFormat(childBuf, src->pr); + return 0; } diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index cab324c4d7..b39b694c60 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2797,6 +2797,9 @@ virStorageNetHostDefFree; virStorageNetHostTransportTypeFromString; virStorageNetHostTransportTypeToString; virStorageNetProtocolTypeToString; +virStoragePRDefFormat; +virStoragePRDefFree; +virStoragePRDefParseXML; virStorageSourceBackingStoreClear; virStorageSourceClear; virStorageSourceCopy; diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 67b9ec71ac..9917837513 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -1892,6 +1892,136 @@ virStorageAuthDefFormat(virBufferPtr buf, } +void +virStoragePRDefFree(virStoragePRDefPtr prd) +{ + if (!prd) + return; + + VIR_FREE(prd->path); + VIR_FREE(prd); +} + + +virStoragePRDefPtr +virStoragePRDefParseXML(xmlXPathContextPtr ctxt) +{ + virStoragePRDefPtr prd, ret = NULL; + char *enabled = NULL; + char *managed = NULL; + char *type = NULL; + char *path = NULL; + char *mode = NULL; + + if (VIR_ALLOC(prd) < 0) + return NULL; + + if (!(enabled = virXPathString("string(./@enabled)", ctxt))) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("missing @enabled attribute for <reservations/>")); + goto cleanup; + } + + if ((prd->enabled = virTristateBoolTypeFromString(enabled)) <= 0) { + virReportError(VIR_ERR_XML_ERROR, + _("invalid value for 'enabled': %s"), enabled); + goto cleanup; + } + + if (prd->enabled == VIR_TRISTATE_BOOL_YES) { + if (!(managed = virXPathString("string(./@managed)", ctxt))) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("missing @managed attribute for <reservations/>")); + goto cleanup; + } + + if ((prd->managed = virTristateBoolTypeFromString(managed)) <= 0) { + virReportError(VIR_ERR_XML_ERROR, + _("invalid value for 'managed': %s"), managed); + goto cleanup; + } + + if (prd->managed == VIR_TRISTATE_BOOL_NO) { + type = virXPathString("string(./source[1]/@type)", ctxt); + path = virXPathString("string(./source[1]/@path)", ctxt); + mode = virXPathString("string(./source[1]/@mode)", ctxt); + + if (!type) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("missing connection type for <reservations/>")); + goto cleanup; + } + + if (!path) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("missing path for <reservations/>")); + goto cleanup; + } + + if (!mode) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("missing connection mode for <reservations/>")); + goto cleanup; + } + + if (STRNEQ(type, "unix")) { + virReportError(VIR_ERR_XML_ERROR, + _("unsupported connection type for <reservations/>: %s"), + type); + goto cleanup; + } + + if (STRNEQ(mode, "client")) { + virReportError(VIR_ERR_XML_ERROR, + _("unsupported connection mode for <reservations/>: %s"), + mode); + goto cleanup; + } + + VIR_STEAL_PTR(prd->path, path); + } + } + + ret = prd; + prd = NULL; + + cleanup: + VIR_FREE(mode); + VIR_FREE(path); + VIR_FREE(type); + VIR_FREE(managed); + VIR_FREE(enabled); + virStoragePRDefFree(prd); + return ret; +} + + +void +virStoragePRDefFormat(virBufferPtr buf, + virStoragePRDefPtr prd) +{ + virBufferAsprintf(buf, "<reservations enabled='%s'", + virTristateBoolTypeToString(prd->enabled)); + if (prd->enabled == VIR_TRISTATE_BOOL_YES) { + virBufferAsprintf(buf, " managed='%s'", + virTristateBoolTypeToString(prd->managed)); + if (prd->managed == VIR_TRISTATE_BOOL_NO) { + virBufferAddLit(buf, ">\n"); + virBufferAdjustIndent(buf, 2); + virBufferAddLit(buf, "<source type='unix'"); + virBufferEscapeString(buf, " path='%s'", prd->path); + virBufferAddLit(buf, " mode='client'/>\n"); + virBufferAdjustIndent(buf, -2); + virBufferAddLit(buf, "</reservations>\n"); + } else { + virBufferAddLit(buf, "/>\n"); + } + } else { + virBufferAddLit(buf, "/>\n"); + } +} + + virSecurityDeviceLabelDefPtr virStorageSourceGetSecurityLabelDef(virStorageSourcePtr src, const char *model) @@ -2270,6 +2400,7 @@ virStorageSourceClear(virStorageSourcePtr def) virBitmapFree(def->features); VIR_FREE(def->compat); virStorageEncryptionFree(def->encryption); + virStoragePRDefFree(def->pr); virStorageSourceSeclabelsClear(def); virStoragePermsFree(def->perms); VIR_FREE(def->timestamps); diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index 596746ccb7..b705ab1f92 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -216,6 +216,14 @@ struct _virStorageAuthDef { virSecretLookupTypeDef seclookupdef; }; +typedef struct _virStoragePRDef virStoragePRDef; +typedef virStoragePRDef *virStoragePRDefPtr; +struct _virStoragePRDef { + int enabled; /* enum virTristateBool */ + int managed; /* enum virTristateBool */ + char *path; +}; + typedef struct _virStorageDriverData virStorageDriverData; typedef virStorageDriverData *virStorageDriverDataPtr; @@ -243,6 +251,7 @@ struct _virStorageSource { bool authInherited; virStorageEncryptionPtr encryption; bool encryptionInherited; + virStoragePRDefPtr pr; virObjectPtr privateData; @@ -370,6 +379,11 @@ virStorageAuthDefPtr virStorageAuthDefParse(xmlNodePtr node, xmlXPathContextPtr ctxt); void virStorageAuthDefFormat(virBufferPtr buf, virStorageAuthDefPtr authdef); +void virStoragePRDefFree(virStoragePRDefPtr prd); +virStoragePRDefPtr virStoragePRDefParseXML(xmlXPathContextPtr ctxt); +void virStoragePRDefFormat(virBufferPtr buf, + virStoragePRDefPtr prd); + virSecurityDeviceLabelDefPtr virStorageSourceGetSecurityLabelDef(virStorageSourcePtr src, const char *model); diff --git a/tests/qemuxml2argvdata/disk-virtio-scsi-reservations.xml b/tests/qemuxml2argvdata/disk-virtio-scsi-reservations.xml new file mode 100644 index 0000000000..036c6e3c25 --- /dev/null +++ b/tests/qemuxml2argvdata/disk-virtio-scsi-reservations.xml @@ -0,0 +1,49 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219136</memory> + <currentMemory unit='KiB'>219136</currentMemory> + <vcpu placement='static'>8</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu-system-i686</emulator> + <disk type='block' device='lun'> + <driver name='qemu' type='raw'/> + <source dev='/dev/HostVG/QEMUGuest1'> + <reservations enabled='yes' managed='yes'/> + </source> + <target dev='sda' bus='scsi'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> + <disk type='block' device='lun'> + <driver name='qemu' type='raw'/> + <source dev='/dev/HostVG/QEMUGuest2'> + <reservations enabled='yes' managed='no'> + <source type='unix' path='/path/to/qemu-pr-helper.sock' mode='client'/> + </reservations> + </source> + <target dev='sdb' bus='scsi'/> + <address type='drive' controller='0' bus='0' target='0' unit='1'/> + </disk> + <controller type='usb' index='0'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/> + </controller> + <controller type='scsi' index='0' model='virtio-scsi'> + <driver queues='8'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/> + </controller> + <controller type='pci' index='0' model='pci-root'/> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <memballoon model='virtio'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0x0'/> + </memballoon> + </devices> +</domain> diff --git a/tests/qemuxml2xmloutdata/disk-virtio-scsi-reservations.xml b/tests/qemuxml2xmloutdata/disk-virtio-scsi-reservations.xml new file mode 120000 index 0000000000..dde52fd1db --- /dev/null +++ b/tests/qemuxml2xmloutdata/disk-virtio-scsi-reservations.xml @@ -0,0 +1 @@ +../qemuxml2argvdata/disk-virtio-scsi-reservations.xml \ No newline at end of file diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index d123180e79..4c069497aa 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -387,6 +387,8 @@ mymain(void) QEMU_CAPS_NODEFCONFIG, QEMU_CAPS_VIRTIO_SCSI); DO_TEST("disk-virtio-scsi-num_queues", QEMU_CAPS_NODEFCONFIG, QEMU_CAPS_VIRTIO_SCSI); + DO_TEST("disk-virtio-scsi-reservations", + QEMU_CAPS_NODEFCONFIG, QEMU_CAPS_VIRTIO_SCSI); DO_TEST("disk-virtio-scsi-cmd_per_lun", QEMU_CAPS_NODEFCONFIG, QEMU_CAPS_VIRTIO_SCSI); DO_TEST("disk-virtio-scsi-max_sectors", -- 2.16.1

Couple of reasons for that: a) there's no monitor command to change path where the pr-helper connects to, or b) there's no monitor command to introduce a new pr-helper for a disk that already exists. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/libvirt_private.syms | 1 + src/qemu/qemu_domain.c | 8 ++++++++ src/util/virstoragefile.c | 18 ++++++++++++++++++ src/util/virstoragefile.h | 2 ++ 4 files changed, 29 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index b39b694c60..a376e3bb0d 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2799,6 +2799,7 @@ virStorageNetHostTransportTypeToString; virStorageNetProtocolTypeToString; virStoragePRDefFormat; virStoragePRDefFree; +virStoragePRDefIsEqual; virStoragePRDefParseXML; virStorageSourceBackingStoreClear; virStorageSourceClear; diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 100304fd05..5a7b5f8417 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -7820,6 +7820,14 @@ qemuDomainDiskChangeSupported(virDomainDiskDefPtr disk, CHECK_EQ(src->readonly, "readonly", true); CHECK_EQ(src->shared, "shared", true); + if (!virStoragePRDefIsEqual(disk->src->pr, + orig_disk->src->pr)) { + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, + _("cannot modify field '%s' of the disk"), + "reservations"); + return false; + } + #undef CHECK_EQ return true; diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 9917837513..b017024b2f 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -2022,6 +2022,24 @@ virStoragePRDefFormat(virBufferPtr buf, } +bool +virStoragePRDefIsEqual(virStoragePRDefPtr a, + virStoragePRDefPtr b) +{ + if (!a && !b) + return true; + + if (!a || !b) + return false; + + if (a->enabled != b->enabled || + a->managed != b->managed || + STRNEQ_NULLABLE(a->path, b->path)) + return false; + + return true; +} + virSecurityDeviceLabelDefPtr virStorageSourceGetSecurityLabelDef(virStorageSourcePtr src, const char *model) diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index b705ab1f92..77853eefe8 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -383,6 +383,8 @@ void virStoragePRDefFree(virStoragePRDefPtr prd); virStoragePRDefPtr virStoragePRDefParseXML(xmlXPathContextPtr ctxt); void virStoragePRDefFormat(virBufferPtr buf, virStoragePRDefPtr prd); +bool virStoragePRDefIsEqual(virStoragePRDefPtr a, + virStoragePRDefPtr b); virSecurityDeviceLabelDefPtr virStorageSourceGetSecurityLabelDef(virStorageSourcePtr src, -- 2.16.1

The capability tracks if qemu has pr-manager-helper object. At this time don't actually detect if qemu has the capability. Not just yet. Only after the code is written the feature will be enabled. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/qemu/qemu_capabilities.c | 1 + src/qemu/qemu_capabilities.h | 1 + 2 files changed, 2 insertions(+) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 91b7aa31ec..efd468692a 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -468,6 +468,7 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST, "virtio-tablet-ccw", "qcow2-luks", "pcie-pci-bridge", + "pr-manager-helper", ); diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index bec28cae92..f0948e3016 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -452,6 +452,7 @@ typedef enum { QEMU_CAPS_DEVICE_VIRTIO_TABLET_CCW, /* -device virtio-tablet-ccw */ QEMU_CAPS_QCOW2_LUKS, /* qcow2 format support LUKS encryption */ QEMU_CAPS_DEVICE_PCIE_PCI_BRIDGE, /* -device pcie-pci-bridge */ + QEMU_CAPS_PR_MANAGER_HELPER, /* -object pr-manager-helper */ QEMU_CAPS_LAST /* this must always be the last item */ } virQEMUCapsFlags; -- 2.16.1

While we're not generating the command line just yet (look for the next commits), we can generate the alias for pr-manager. A domain can have up to one managed pr-manager (in which case socket path is decided by libvirt and pr-helper is spawned by libvirt too), but up to ndisks of unmanaged ones (one per disk basically). In case of the former we can generate a simple alias and be sure it'll not conflict. But in case of the latter to avoid any conflicts let's generate alias that's based on something unique - like disk target. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/libvirt_private.syms | 2 ++ src/qemu/qemu_alias.c | 11 ++++++ src/qemu/qemu_alias.h | 2 ++ src/qemu/qemu_domain.c | 87 +++++++++++++++++++++++++++++++++++++++++++++-- src/qemu/qemu_domain.h | 10 ++++++ src/util/virstoragefile.c | 15 ++++++++ src/util/virstoragefile.h | 2 ++ 7 files changed, 126 insertions(+), 3 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index a376e3bb0d..5b7b5514a2 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2799,7 +2799,9 @@ virStorageNetHostTransportTypeToString; virStorageNetProtocolTypeToString; virStoragePRDefFormat; virStoragePRDefFree; +virStoragePRDefIsEnabled; virStoragePRDefIsEqual; +virStoragePRDefIsManaged; virStoragePRDefParseXML; virStorageSourceBackingStoreClear; virStorageSourceClear; diff --git a/src/qemu/qemu_alias.c b/src/qemu/qemu_alias.c index 95d1e0370a..6db3da27a8 100644 --- a/src/qemu/qemu_alias.c +++ b/src/qemu/qemu_alias.c @@ -773,3 +773,14 @@ qemuAliasChardevFromDevAlias(const char *devAlias) return ret; } + + +char * +qemuDomainGetManagedPRAlias(void) +{ + char *alias; + + ignore_value(VIR_STRDUP(alias, "pr-helper0")); + + return alias; +} diff --git a/src/qemu/qemu_alias.h b/src/qemu/qemu_alias.h index 8c744138ce..91e0a9c893 100644 --- a/src/qemu/qemu_alias.h +++ b/src/qemu/qemu_alias.h @@ -92,4 +92,6 @@ char *qemuAliasTLSObjFromSrcAlias(const char *srcAlias) char *qemuAliasChardevFromDevAlias(const char *devAlias) ATTRIBUTE_NONNULL(1); +char * qemuDomainGetManagedPRAlias(void); + #endif /* __QEMU_ALIAS_H__*/ diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 5a7b5f8417..361a80be84 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -982,6 +982,18 @@ qemuDomainSecretInfoFree(qemuDomainSecretInfoPtr *secinfo) } +static void +qemuDomainDiskPRDFree(qemuDomainDiskPRDPtr prd) +{ + if (!prd) + return; + + VIR_FREE(prd->alias); + VIR_FREE(prd->path); + VIR_FREE(prd); +} + + static virClassPtr qemuDomainDiskPrivateClass; static void qemuDomainDiskPrivateDispose(void *obj); @@ -1062,6 +1074,7 @@ qemuDomainStorageSourcePrivateDispose(void *obj) qemuDomainSecretInfoFree(&priv->secinfo); qemuDomainSecretInfoFree(&priv->encinfo); + qemuDomainDiskPRDFree(priv->prd); } @@ -1473,9 +1486,6 @@ qemuDomainSecretStorageSourcePrepare(qemuDomainObjPrivatePtr priv, if (!hasAuth && !hasEnc) return 0; - if (!(src->privateData = qemuDomainStorageSourcePrivateNew())) - return -1; - srcPriv = QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(src); if (hasAuth) { @@ -11925,11 +11935,79 @@ qemuDomainPrepareDiskCachemode(virDomainDiskDefPtr disk) } +static int +qemuDomainPrepareDiskPRD(qemuDomainObjPrivatePtr priv, + virDomainDiskDefPtr disk) +{ + qemuDomainStorageSourcePrivatePtr srcPriv; + virStoragePRDefPtr prd = disk->src->pr; + char *prAlias = NULL; + char *prPath = NULL; + int ret = -1; + + if (!virStoragePRDefIsEnabled(prd)) + return 0; + + if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_PR_MANAGER_HELPER)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("reservations not supported with this QEMU binary")); + return ret; + } + + if (!virStorageSourceIsLocalStorage(disk->src)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("reservations supported only for local storage")); + return ret; + } + + srcPriv = QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(disk->src); + + /* Managed PR means there's one pr-manager object per domain + * and the pr-helper process is spawned and managed by + * libvirt. + * If PR is unmanaged there's one pr-manager object per disk + * (in general each disk can have its own pr-manager) and + * it's user's responsibility to start the pr-helper process. + */ + if (virStoragePRDefIsManaged(prd)) { + /* Generate PR helper socket path & alias that are same + * for each disk in the domain. */ + + if (!(prAlias = qemuDomainGetManagedPRAlias())) + return ret; + + if (virAsprintf(&prPath, "%s/pr-helper0.sock", priv->libDir) < 0) + goto cleanup; + + } else { + if (virAsprintf(&prAlias, "pr-helper-%s", disk->info.alias) < 0) + return ret; + + if (VIR_STRDUP(prPath, prd->path) < 0) + goto cleanup; + } + + if (VIR_ALLOC(srcPriv->prd) < 0) + goto cleanup; + VIR_STEAL_PTR(srcPriv->prd->alias, prAlias); + VIR_STEAL_PTR(srcPriv->prd->path, prPath); + + ret = 0; + cleanup: + VIR_FREE(prPath); + VIR_FREE(prAlias); + return ret; +} + + int qemuDomainPrepareDiskSource(virDomainDiskDefPtr disk, qemuDomainObjPrivatePtr priv, virQEMUDriverConfigPtr cfg) { + if (!(disk->src->privateData = qemuDomainStorageSourcePrivateNew())) + return -1; + qemuDomainPrepareDiskCachemode(disk); if (qemuDomainPrepareDiskSourceTLS(disk->src, cfg) < 0) @@ -11938,6 +12016,9 @@ qemuDomainPrepareDiskSource(virDomainDiskDefPtr disk, if (qemuDomainSecretDiskPrepare(priv, disk) < 0) return -1; + if (qemuDomainPrepareDiskPRD(priv, disk) < 0) + return -1; + if (qemuDomainPrepareDiskSourceChain(disk, NULL, cfg, priv->qemuCaps) < 0) return -1; diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 21e12f6594..d283f128ef 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -373,6 +373,13 @@ struct _qemuDomainDiskPrivate { bool removable; /* device media can be removed/changed */ }; +typedef struct _qemuDomainDiskPRD qemuDomainDiskPRD; +typedef qemuDomainDiskPRD *qemuDomainDiskPRDPtr; +struct _qemuDomainDiskPRD { + char *alias; + char *path; /* socket path. */ +}; + # define QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(src) \ ((qemuDomainStorageSourcePrivatePtr) (src)->privateData) @@ -386,6 +393,9 @@ struct _qemuDomainStorageSourcePrivate { /* data required for decryption of encrypted storage source */ qemuDomainSecretInfoPtr encinfo; + + /* data required for persistent reservations */ + qemuDomainDiskPRDPtr prd; }; virObjectPtr qemuDomainStorageSourcePrivateNew(void); diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index b017024b2f..877d8ba8e4 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -2040,6 +2040,21 @@ virStoragePRDefIsEqual(virStoragePRDefPtr a, return true; } + +bool +virStoragePRDefIsEnabled(virStoragePRDefPtr prd) +{ + return prd && prd->enabled == VIR_TRISTATE_BOOL_YES; +} + + +bool +virStoragePRDefIsManaged(virStoragePRDefPtr prd) +{ + return prd && prd->managed == VIR_TRISTATE_BOOL_YES; +} + + virSecurityDeviceLabelDefPtr virStorageSourceGetSecurityLabelDef(virStorageSourcePtr src, const char *model) diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index 77853eefe8..2127509ea3 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -385,6 +385,8 @@ void virStoragePRDefFormat(virBufferPtr buf, virStoragePRDefPtr prd); bool virStoragePRDefIsEqual(virStoragePRDefPtr a, virStoragePRDefPtr b); +bool virStoragePRDefIsEnabled(virStoragePRDefPtr prd); +bool virStoragePRDefIsManaged(virStoragePRDefPtr prd); virSecurityDeviceLabelDefPtr virStorageSourceGetSecurityLabelDef(virStorageSourcePtr src, -- 2.16.1

On 04/13/2018 10:51 PM, John Ferlan wrote: Okay. I will put it in the commit message. I thought we've discussed this earlier. I rather put and info into status XML even though it might look useless right now than having to write some crazy backcompat code later. The purpose is to generate alias at only one place instead of several virAsprintf-s scattered all around the code. Well, unmanaged helper should not have its socket in libvirt private path. Under any circumstances. And what qemuProcessKillPRDaemon does is: 1) it constructs PID file path 2) and kills any pr-helper that might had been left behind. I find this approach more robust than plain "check if there is a disk with pr-helper and if so kill it" because if libvirt is ever split brained Well, there are couple of reasons for stdup()-ing a static string: a) it simplifies the code, e.g. qemuDomainDiskPRDFree() doesn't have to care if alias is a static string or dynamically allocated one, b) higher memory consumption (by 11 bytes btw) is only for a short period of time (when constructing PID file path). Although, the alias is kept around for entire time domain is running. And if libvirt is unable to allocate 11 bytes then you're in much bigger trouble anyways. I don't think it can go there. For IsLocalStorage() to return valid results virDomainDiskTranslateSourcePool() needs to be called first. And it is done so only when starting domain. Not for Validate() callback. And technically, at this point whether disk is LUN or not is checked in virDomainDiskDefValidate(). So the control would not even get here. But okay, I will change the error message. No. Having alias generation at single location is very advantageous IMO. Libvirt's full of small wrappers over functions. And it this case it even makes sense. Then I'd have to modify qemuDomainDiskPRDFree() so that for instance it does: if (STRNEQ(prd->alias, QEMU_PR_MANAGER_ALIAS)) VIR_FREE(prd->alias); VIR_FREE(prd->path); ... which I don't quite like (to say the least). Michal

On 04/17/2018 01:49 PM, John Ferlan wrote: Depends on problem you were trying to future proof from. Not true. If alias were a static string, then _qemuDomainDiskPRD would need to reflect that. That means, alias must be 'const char *' because storing a static string into 'char *' is a big NO NO. But at the same time, alias is dynamically generated (for unmanaged disks), so it has to be 'char *'. Sure, we can obfuscate it by inventing new struct member, but just look at how ugly it looks: struct qemuDomainDiskPRD { char *alias; const char *constAlias; char *path; }; Also, think of all those special casing in the code that uses the struct. This approach that I implemented here is used widely across our code base: qemuAssignDeviceTPMAlias(), qemuAssignDeviceWatchdogAlias() to name few. Why we even need this struct you ask? Couple of reasons: a) this approach implemented here is specific to QEMU and QEMU only. It isn't hypervisor agnostic, thus it doesn't belong to _virStoragePRDef. b) we should separate runtime and config data. That's why virStorage*() functions live under src/util/ as they work with config and not runtime (as in hypervisor specific). This merely mimics vm->privateData abstraction. Why? We have an option to record alias used so that when libvirtd restarts we can read it back. On the other hand what you are proposing is to try current version, if that fails try all the previous. What for? to save couple of bytes of memory/disk space? I don't see added value there, sorry. Also, the whole point of status XML is so that we don't have to use try-error approach. Look at it from different angle: all devices have their alias generated from info contained in config XML (either based on their position like deviceN for N-th device, or based on their address like scsi-0-0-0-1 for corresponding SCSI address). We can regenerate them when reconnecting to qemu. Even better - we can ask on qemu monitor. So why not do that and waste memory/disk space by storing them in status XML? So? It is still better to have it at one place and called from multiple. I guess it makes perfect sense as it is now. Read my reply above. Obfuscating code just to save couple of bytes of memory is not a good idea in my book. Michal

On 04/10/2018 10:58 AM, Michal Privoznik wrote: So if we were to save this information (and at this point I don't think we need to), then this is where we should be allocating and filling in the private data (e.g. not in the previous patch). Again as I noted previously, save the alias when printing the domain active information and I think you're good. AFAICT the only thing printed now (@relPath) is something generated via qemu_driver calls (I didn't dig deep); whereas, this data is easily regeneratable from existing domain definition data. John

If qemu-pr-helper is compiled with multipath support the first thing it does is opening /dev/mapper/control. Since we're going to be running it inside qemu namespace we need to create it there. Unfortunately, we don't know if it was compiled with or without multipath so we have to create it anyway. BTW: This might be the ugliest piece of code I've ever written but @devMapperControl really needs to be type of char * otherwise some crazy check in VIR_APPEND_ELEMENT fails. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/qemu/qemu_domain.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 0856f04406..6fe4eb57e1 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -108,6 +108,7 @@ VIR_ENUM_IMPL(qemuDomainNamespace, QEMU_DOMAIN_NS_LAST, #define PROC_MOUNTS "/proc/mounts" #define DEVPREFIX "/dev/" #define DEV_VFIO "/dev/vfio/vfio" +#define DEVICE_MAPPER_CONTROL_PATH "/dev/mapper/control" struct _qemuDomainLogContext { @@ -10269,6 +10270,11 @@ qemuDomainSetupDisk(virQEMUDriverConfigPtr cfg ATTRIBUTE_UNUSED, goto cleanup; } + /* qemu-pr-helper might require access to /dev/mapper/control. */ + if (virStoragePRDefIsEnabled(disk->src->pr) && + qemuDomainCreateDevice(DEVICE_MAPPER_CONTROL_PATH, data, true) < 0) + goto cleanup; + ret = 0; cleanup: VIR_FREE(dst); @@ -11281,6 +11287,9 @@ qemuDomainNamespaceSetupDisk(virDomainObjPtr vm, const char **paths = NULL; size_t npaths = 0; int ret = -1; + /* This is very nasty but we need it to work around some + * stupid checks in VIR_APPEND_ELEMENT macro. */ + char *devMapperControl = (char *) DEVICE_MAPPER_CONTROL_PATH; if (!qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT)) return 0; @@ -11296,6 +11305,11 @@ qemuDomainNamespaceSetupDisk(virDomainObjPtr vm, goto cleanup; } + /* qemu-pr-helper might require access to /dev/mapper/control. */ + if (virStoragePRDefIsEnabled(src->pr) && + VIR_APPEND_ELEMENT_COPY(paths, npaths, devMapperControl) < 0) + goto cleanup; + if (qemuDomainNamespaceMknodPaths(vm, paths, npaths) < 0) return -1; -- 2.16.1

On 04/14/2018 02:15 PM, John Ferlan wrote: No. It's because if compiled with multipath support, qemu-pr-helper uses /dev/mapper/control to query devmapper version (dm_init()): https://git.qemu.org/?p=qemu.git;a=blob;f=scsi/qemu-pr-helper.c;h=d0f8317... /dev/mapper/control is used to talk to devmapper (kernel) - in this particular case to check if given device is a multipath target, not to manipulate individual devices. Because if a device is multipath target then lowlevel stuff looks different (see do_pr_in() and do_pr_out()). Anyway, it shouldn't matter what pr-helper needs /dev/mapper/control for. It does so we need to create it in the namespace and allow the helper to access it. It's not because of allowing /dev/mapper/control but because of typecast from const char * to char *. Okay, I'll do this. It's cleaner. Michal

On 04/10/2018 10:58 AM, Michal Privoznik wrote: Perhaps the two patches should be combined then... yes, I know cgroups is different than namespace, so I understand the separation. Almost too bad we didn't have a common place for this in some existing included .h file. Could the above be done potentially many times? Could be expensive, no? Considering what virDevMapperGetTargets[Impl] does... If there was only one that's managed and it's @src, then we don't get here... You go through great lengths to ensure this is only done once converse to the qemuSetupImageCgroupInternal change... I guess I would think that if we can determine at Prepare time that NS and cgroups would need to add /dev/mapper/control and we only want to do that config once, then we should be able Yes, of course we then we potentially miss adding for hotplug. Honestly it seems more of a "global" to qemu_domain rather than "local" to a particular disk source even though it's needed by the local disk source, is it really something the disk source itself (or it's private data structure) should be managing? Should this be separated? Or is it related to /dev/mapper/control? John

On 04/16/2018 10:56 AM, Michal Privoznik wrote: Separate is fine... Suffice to say it wasn't obvious to me WTF virDevMapperGetTargets[Impl] causes the ENXIO. That logic is not entirely self documenting. So it made sense when I wrote the comment... let's see. OK - with the example I see... /me wonders at this point whether it'd be clearer to keep a list of pr-helper managed LUN's and "Add" and "Remove" where the 1st add and the last remove trigger the PR start/stop rather than looping through ndisks. hmm... my self documenting code comment ;-)... The relationship between ENXIO and having /dev/mapper/control be "managed" in/by the name space code was not that obvious to me. This is stuff that'll drive someone nuts in the future when they wonder WTF is going on and why ENXIO was magically chosen and added to a cgroup related patch. John

This is the easier part. All we need to do here is put -object pr-manager-helper,id=$alias,path=$socketPath and then just reference the object in -drive file.pr-manager=$alias. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/qemu/qemu_command.c | 94 ++++++++++++++++++++++ src/qemu/qemu_command.h | 3 + .../disk-virtio-scsi-reservations.args | 35 ++++++++ tests/qemuxml2argvtest.c | 4 + 4 files changed, 136 insertions(+) create mode 100644 tests/qemuxml2argvdata/disk-virtio-scsi-reservations.args diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 514c3ab2ef..81f6025207 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -1514,6 +1514,20 @@ qemuDiskSourceGetProps(virStorageSourcePtr src) } +static void +qemuBuildDriveSourcePR(virBufferPtr buf, + virStorageSourcePtr src) +{ + qemuDomainStorageSourcePrivatePtr srcPriv = QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(src); + qemuDomainDiskPRDPtr prd = srcPriv->prd; + + if (!prd || !prd->alias) + return; + + virBufferAsprintf(buf, ",file.pr-manager=%s", prd->alias); +} + + static int qemuBuildDriveSourceStr(virDomainDiskDefPtr disk, virQEMUCapsPtr qemuCaps, @@ -1591,6 +1605,8 @@ qemuBuildDriveSourceStr(virDomainDiskDefPtr disk, if (disk->src->debug) virBufferAsprintf(buf, ",file.debug=%d", disk->src->debugLevel); + + qemuBuildDriveSourcePR(buf, disk->src); } else { if (!(source = virQEMUBuildDriveCommandlineFromJSON(srcprops))) goto cleanup; @@ -9872,6 +9888,81 @@ qemuBuildPanicCommandLine(virCommandPtr cmd, } +/** + * qemuBuildPRManagerInfoProps: + * @prd: disk PR runtime info + * @propsret: JSON properties to return + * + * Build the JSON properties for the pr-manager object. + * + * Returns: 0 on success (@propsret is NULL if no properties are needed), + * -1 on failure (with error message set). + */ +int +qemuBuildPRManagerInfoProps(qemuDomainDiskPRDPtr prd, + virJSONValuePtr *propsret) +{ + *propsret = NULL; + + if (!prd || !prd->alias) + return 0; + + if (virJSONValueObjectCreate(propsret, + "s:path", prd->path, + NULL) < 0) + return -1; + + return 0; +} + + +static int +qemuBuildMasterPRCommandLine(virCommandPtr cmd, + const virDomainDef *def) +{ + size_t i; + bool managedAdded = false; + virJSONValuePtr props = NULL; + char *tmp = NULL; + int ret = -1; + + for (i = 0; i < def->ndisks; i++) { + const virDomainDiskDef *disk = def->disks[i]; + qemuDomainStorageSourcePrivatePtr srcPriv = QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(disk->src); + qemuDomainDiskPRDPtr prd = srcPriv->prd; + + + if (virStoragePRDefIsManaged(disk->src->pr)) { + if (managedAdded) + continue; + + managedAdded = true; + } + + if (qemuBuildPRManagerInfoProps(prd, &props) < 0) + goto cleanup; + + if (!props) + continue; + + if (!(tmp = virQEMUBuildObjectCommandlineFromJSON("pr-manager-helper", + prd->alias, + props))) + goto cleanup; + virJSONValueFree(props); + props = NULL; + + virCommandAddArgList(cmd, "-object", tmp, NULL); + VIR_FREE(tmp); + } + + ret = 0; + cleanup: + virJSONValueFree(props); + return ret; +} + + /** * qemuBuildCommandLineValidate: * @@ -10024,6 +10115,9 @@ qemuBuildCommandLine(virQEMUDriverPtr driver, if (qemuBuildMasterKeyCommandLine(cmd, priv) < 0) goto error; + if (qemuBuildMasterPRCommandLine(cmd, def) < 0) + goto error; + if (enableFips) virCommandAddArg(cmd, "-enable-fips"); diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h index 31c9da673c..32f3697181 100644 --- a/src/qemu/qemu_command.h +++ b/src/qemu/qemu_command.h @@ -54,6 +54,9 @@ virCommandPtr qemuBuildCommandLine(virQEMUDriverPtr driver, size_t *nnicindexes, int **nicindexes); +/* Generate the object properties for pr-manager */ +int qemuBuildPRManagerInfoProps(qemuDomainDiskPRDPtr prd, + virJSONValuePtr *propsret); /* Generate the object properties for a secret */ int qemuBuildSecretInfoProps(qemuDomainSecretInfoPtr secinfo, diff --git a/tests/qemuxml2argvdata/disk-virtio-scsi-reservations.args b/tests/qemuxml2argvdata/disk-virtio-scsi-reservations.args new file mode 100644 index 0000000000..195e83a048 --- /dev/null +++ b/tests/qemuxml2argvdata/disk-virtio-scsi-reservations.args @@ -0,0 +1,35 @@ +LC_ALL=C \ +PATH=/bin \ +HOME=/home/test \ +USER=test \ +LOGNAME=test \ +QEMU_AUDIO_DRV=none \ +/usr/bin/qemu-system-i686 \ +-name QEMUGuest1 \ +-S \ +-object pr-manager-helper,id=pr-helper0,\ +path=/tmp/lib/domain--1-QEMUGuest1/pr-helper0.sock \ +-object pr-manager-helper,id=pr-helper-scsi0-0-0-1,\ +path=/path/to/qemu-pr-helper.sock \ +-M pc \ +-m 214 \ +-smp 8,sockets=8,cores=1,threads=1 \ +-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \ +-nographic \ +-nodefaults \ +-chardev socket,id=charmonitor,path=/tmp/lib/domain--1-QEMUGuest1/monitor.sock,\ +server,nowait \ +-mon chardev=charmonitor,id=monitor,mode=readline \ +-no-acpi \ +-boot c \ +-device virtio-scsi-pci,id=scsi0,num_queues=8,bus=pci.0,addr=0x3 \ +-usb \ +-drive file=/dev/HostVG/QEMUGuest1,file.pr-manager=pr-helper0,format=raw,\ +if=none,id=drive-scsi0-0-0-0,boot=on \ +-device scsi-block,bus=scsi0.0,channel=0,scsi-id=0,lun=0,\ +drive=drive-scsi0-0-0-0,id=scsi0-0-0-0 \ +-drive file=/dev/HostVG/QEMUGuest2,file.pr-manager=pr-helper-scsi0-0-0-1,\ +format=raw,if=none,id=drive-scsi0-0-0-1 \ +-device scsi-block,bus=scsi0.0,channel=0,scsi-id=0,lun=1,\ +drive=drive-scsi0-0-0-1,id=scsi0-0-0-1 \ +-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x4 diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 165137e93c..fdaeb473d0 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -3055,6 +3055,10 @@ mymain(void) QEMU_CAPS_PIIX_DISABLE_S3, QEMU_CAPS_PIIX_DISABLE_S4, QEMU_CAPS_ICH9_USB_EHCI1, QEMU_CAPS_PCI_MULTIFUNCTION); + DO_TEST("disk-virtio-scsi-reservations", + QEMU_CAPS_DRIVE_BOOT, QEMU_CAPS_VIRTIO_SCSI, + QEMU_CAPS_SCSI_BLOCK, QEMU_CAPS_PR_MANAGER_HELPER); + /* Test disks with format probing enabled for legacy reasons. * New tests should not go in this section. */ driver.config->allowDiskFormatProbing = true; -- 2.16.1

On 04/14/2018 03:04 PM, John Ferlan wrote: Oh yeah. There's always chance that between me posting patches and review somebody will push something that invalidates my patches. Not true. We need to add manager objects even for cases where libvirt is not managing the helper process. For instance, for the following XML: <disk type='block' device='lun'> <source dev='/dev/HostVG/QEMUGuest2'> <reservations enabled='yes' managed='no'> <source type='unix' path='/path/to/qemu-pr-helper.sock' mode='client'/> </reservations> </source> </disk> the following cmd line needs to be generated: -object pr-manager-helper,id=pr-helper-scsi0-0-0-1,\ path=/path/to/qemu-pr-helper.sock \ otherwise qemu would now know where to connect. However, if libvirt is managing the pr-helper process, all the managed disks share the same process => object on the command line => it must be added exactly once. It's reused there yes. As Peter suggested in one of earlier reviews instead of having two functions (one for generating cmd line and the other for JSON) I can write just one and use virQEMUBuildObjectCommandlineFromJSON() to translate JSON into cmd line. Michal

On 04/10/2018 10:58 AM, Michal Privoznik wrote: So the default install location of qemu-pr-helper is /usr/bin unlike bridge-helper which is /usr/libexec? s/disks/LUN devices/ Going with my note above - the default path in the m4 file shows /usr/bin first... So should this match that? Similar to how qemu-bridge-helper matches the /usr/libexec install location? And don't forget this one if something does change, but I'm sure that'd wash out of the check syntax-check I think the questions/issues are minimal and easily fixed by a response, so consider the concept at least Reviewed-by: John Ferlan <jferlan(a)redhat.com&gt; John

Before we exec() qemu we have to spawn pr-helper processes for all managed reservations (well, technically there can only one). The only caveat there is that we should place the process into the same namespace and cgroup as qemu (so that it shares the same view of the system). But we can do that only after we've forked. That means calling the setup function between fork() and exec(). Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/qemu/qemu_process.c | 224 ++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 224 insertions(+) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index f02114c693..982c989a0a 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -2556,6 +2556,223 @@ qemuProcessResctrlCreate(virQEMUDriverPtr driver, ret = 0; cleanup: virObjectUnref(caps); + + return ret; +} + + +static char * +qemuProcessBuildPRHelperPidfilePath(virDomainObjPtr vm, + const char *prdAlias) +{ + qemuDomainObjPrivatePtr priv = vm->privateData; + + return virPidFileBuildPath(priv->libDir, prdAlias); +} + + +static void +qemuProcessKillPRDaemon(virDomainObjPtr vm) +{ + virErrorPtr orig_err; + char *prAlias; + char *prPidfile; + + if (!(prAlias = qemuDomainGetManagedPRAlias())) { + VIR_WARN("Unable to get default PR manager alias"); + return; + } + + if (!(prPidfile = qemuProcessBuildPRHelperPidfilePath(vm, prAlias))) { + VIR_WARN("Unable to construct pr-helper pidfile path"); + VIR_FREE(prAlias); + return; + } + VIR_FREE(prAlias); + + virErrorPreserveLast(&orig_err); + if (virPidFileForceCleanupPath(prPidfile) < 0) { + VIR_WARN("Unable to kill pr-helper process"); + } else if (unlink(prPidfile) < 0 && + errno != ENOENT) { + virReportSystemError(errno, + _("Unable to remove stale pidfile %s"), + prPidfile); + } + virErrorRestore(&orig_err); + + VIR_FREE(prPidfile); +} + + +static int +qemuProcessStartPRDaemonHook(void *opaque) +{ + virDomainObjPtr vm = opaque; + size_t i, nfds = 0; + int *fds = NULL; + int ret = -1; + + if (virProcessGetNamespaces(vm->pid, &nfds, &fds) < 0) + return ret; + + if (nfds > 0 && + virProcessSetNamespaces(nfds, fds) < 0) + goto cleanup; + + ret = 0; + cleanup: + for (i = 0; i < nfds; i++) + VIR_FORCE_CLOSE(fds[i]); + VIR_FREE(fds); + return ret; +} + + +static int +qemuProcessStartPRDaemon(virDomainObjPtr vm, + qemuDomainDiskPRDPtr prd) +{ + qemuDomainObjPrivatePtr priv = vm->privateData; + virQEMUDriverPtr driver = priv->driver; + virQEMUDriverConfigPtr cfg; + int errfd = -1; + char *pidfile = NULL; + int pidfd = -1; + pid_t cpid = -1; + virCommandPtr cmd = NULL; + virTimeBackOffVar timebackoff; + const unsigned long long timeout = 500000; /* ms */ + int ret = -1; + + cfg = virQEMUDriverGetConfig(driver); + + if (!virFileIsExecutable(cfg->prHelperName)) { + virReportSystemError(errno, _("'%s' is not a suitable pr helper"), + cfg->prHelperName); + goto cleanup; + } + + if (!(pidfile = qemuProcessBuildPRHelperPidfilePath(vm, prd->alias))) + goto cleanup; + + /* Just try to acquire. Dummy pid will be replaced later */ + if ((pidfd = virPidFileAcquirePath(pidfile, false, -1)) < 0) + goto cleanup; + + /* Remove stale socket */ + if (unlink(prd->path) < 0 && + errno != ENOENT) { + virReportSystemError(errno, + _("Unable to remove stale socket path: %s"), + prd->path); + goto cleanup; + } + + if (!(cmd = virCommandNewArgList(cfg->prHelperName, + "-k", prd->path, + "-f", pidfile, + NULL))) + goto cleanup; + + virCommandDaemonize(cmd); + /* We want our virCommand to write child PID into the pidfile + * so that we can read it even before exec(). */ + virCommandSetPidFile(cmd, pidfile); + virCommandSetErrorFD(cmd, &errfd); + + /* Place the process into the same namespace and cgroup as + * qemu (so that it shares the same view of the system). */ + virCommandSetPreExecHook(cmd, qemuProcessStartPRDaemonHook, vm); + + if (virCommandRun(cmd, NULL) < 0) + goto cleanup; + + if (virPidFileReadPath(pidfile, &cpid) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("pr helper %s didn't show up"), prd->alias); + goto cleanup; + } + + if (virTimeBackOffStart(&timebackoff, 1, timeout) < 0) + goto cleanup; + while (virTimeBackOffWait(&timebackoff)) { + char errbuf[1024] = { 0 }; + + if (virFileExists(prd->path)) + break; + + if (virProcessKill(cpid, 0) == 0) + continue; + + if (saferead(errfd, errbuf, sizeof(errbuf) - 1) < 0) { + virReportSystemError(errno, + _("pr helper %s died unexpectedly"), prd->alias); + } else { + virReportError(VIR_ERR_OPERATION_FAILED, + _("pr helper died and reported: %s"), errbuf); + } + goto cleanup; + } + + if (!virFileExists(prd->path)) { + virReportError(VIR_ERR_OPERATION_TIMEOUT, "%s", + _("pr helper socked did not show up")); + goto cleanup; + } + + if (priv->cgroup && + virCgroupAddMachineTask(priv->cgroup, cpid) < 0) + goto cleanup; + + if (qemuSecurityDomainSetPathLabel(driver->securityManager, + vm->def, prd->path, true) < 0) + goto cleanup; + + ret = 0; + cleanup: + if (ret < 0) { + virCommandAbort(cmd); + if (cpid >= 0) + virProcessKillPainfully(cpid, true); + unlink(pidfile); + } + virCommandFree(cmd); + VIR_FORCE_CLOSE(pidfd); + VIR_FREE(pidfile); + VIR_FORCE_CLOSE(errfd); + virObjectUnref(cfg); + return ret; +} + + +static int +qemuProcessMaybeStartPRDaemon(virDomainObjPtr vm) +{ + size_t i; + int ret = -1; + qemuDomainDiskPRDPtr prd = NULL; + + for (i = 0; i < vm->def->ndisks; i++) { + const virDomainDiskDef *disk = vm->def->disks[i]; + qemuDomainStorageSourcePrivatePtr srcPriv; + + if (!virStoragePRDefIsManaged(disk->src->pr)) + continue; + + srcPriv = QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(disk->src); + prd = srcPriv->prd; + break; + } + + if (prd && + qemuProcessStartPRDaemon(vm, prd) < 0) + goto cleanup; + + ret = 0; + cleanup: + if (ret < 0) + qemuProcessKillPRDaemon(vm); return ret; } @@ -6073,6 +6290,10 @@ qemuProcessLaunch(virConnectPtr conn, if (qemuProcessResctrlCreate(driver, vm) < 0) goto cleanup; + VIR_DEBUG("Setting up PR daemon"); + if (qemuProcessMaybeStartPRDaemon(vm) < 0) + goto cleanup; + VIR_DEBUG("Setting domain security labels"); if (qemuSecuritySetAllLabel(driver, vm, @@ -6600,6 +6821,9 @@ void qemuProcessStop(virQEMUDriverPtr driver, /* Remove the master key */ qemuDomainMasterKeyRemove(priv); + /* Do this before we delete the tree and remove pidfile. */ + qemuProcessKillPRDaemon(vm); + virFileDeleteTree(priv->libDir); virFileDeleteTree(priv->channelTargetDir); -- 2.16.1

On 04/14/2018 04:56 PM, John Ferlan wrote: Yeah, well if there's not enough memory to allocate 11 bytes there's probably not enough memory to do any stuff done below, i.e. qemuProcessBuildPRHelperPidfilePath(), or any syscall done in virPidFileForceCleanupPath(). Well, we can have that when qemu finally implements events. Then we can keep this internal state in sync with reality. Michal

When plugging a disk into domain we need to start qemu-pr-helper process iff this is the first disk with PR enabled for the domain. Otherwise the helper is already running (or not needed). Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/qemu/qemu_hotplug.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_process.c | 4 ++-- src/qemu/qemu_process.h | 5 +++++ 3 files changed, 58 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index f0d549de38..468153c79c 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -348,6 +348,49 @@ qemuDomainChangeEjectableMedia(virQEMUDriverPtr driver, } +/** + * qemuDomainMaybeStartPRDaemon: + * @vm: domain object + * @disk: disk to hotplug + * + * Checks if it's needed to start qemu-pr-helper and starts it. + * + * Returns: 0 if qemu-pr-helper is not needed + * 1 if it is needed and was started + * -1 otherwise. + */ +static int +qemuDomainMaybeStartPRDaemon(virDomainObjPtr vm, + virDomainDiskDefPtr disk) +{ + size_t i; + qemuDomainStorageSourcePrivatePtr srcPriv; + + if (!virStoragePRDefIsManaged(disk->src->pr)) { + /* @disk itself does not require qemu-pr-helper. */ + return 0; + } + + for (i = 0; i < vm->def->ndisks; i++) { + const virDomainDiskDef *domainDisk = vm->def->disks[i]; + + if (virStoragePRDefIsManaged(domainDisk->src->pr)) { + /* qemu-pr-helper should be already started because + * another disk in domain requires it. */ + return 0; + } + } + + /* @disk requires qemu-pr-helper but none is running. + * Start it now. */ + srcPriv = QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(disk->src); + if (qemuProcessStartPRDaemon(vm, srcPriv->prd) < 0) + return -1; + + return 1; +} + + /** * qemuDomainAttachDiskGeneric: * @@ -368,6 +411,7 @@ qemuDomainAttachDiskGeneric(virQEMUDriverPtr driver, bool driveAdded = false; bool secobjAdded = false; bool encobjAdded = false; + bool prdStarted = false; virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); virJSONValuePtr secobjProps = NULL; virJSONValuePtr encobjProps = NULL; @@ -384,6 +428,11 @@ qemuDomainAttachDiskGeneric(virQEMUDriverPtr driver, if (qemuDomainPrepareDiskSource(disk, priv, cfg) < 0) goto error; + if ((rv = qemuDomainMaybeStartPRDaemon(vm, disk)) < 0) + goto cleanup; + else if (rv > 0) + prdStarted = true; + srcPriv = QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(disk->src); if (srcPriv) { secinfo = srcPriv->secinfo; @@ -481,6 +530,8 @@ qemuDomainAttachDiskGeneric(virQEMUDriverPtr driver, error: qemuDomainDelDiskSrcTLSObject(driver, vm, disk->src); ignore_value(qemuHotplugPrepareDiskAccess(driver, vm, disk, NULL, true)); + if (prdStarted) + qemuProcessKillPRDaemon(vm); goto cleanup; } diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 982c989a0a..11aaeabb38 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -2571,7 +2571,7 @@ qemuProcessBuildPRHelperPidfilePath(virDomainObjPtr vm, } -static void +void qemuProcessKillPRDaemon(virDomainObjPtr vm) { virErrorPtr orig_err; @@ -2629,7 +2629,7 @@ qemuProcessStartPRDaemonHook(void *opaque) } -static int +int qemuProcessStartPRDaemon(virDomainObjPtr vm, qemuDomainDiskPRDPtr prd) { diff --git a/src/qemu/qemu_process.h b/src/qemu/qemu_process.h index 2741115673..8df5832e23 100644 --- a/src/qemu/qemu_process.h +++ b/src/qemu/qemu_process.h @@ -203,4 +203,9 @@ int qemuProcessRefreshDisks(virQEMUDriverPtr driver, virDomainObjPtr vm, qemuDomainAsyncJob asyncJob); +int qemuProcessStartPRDaemon(virDomainObjPtr vm, + qemuDomainDiskPRDPtr prd); + +void qemuProcessKillPRDaemon(virDomainObjPtr vm); + #endif /* __QEMU_PROCESS_H__ */ -- 2.16.1

When attaching a disk that requires pr-manager we might need to plug the pr-manager object too. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/qemu/qemu_hotplug.c | 41 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 41 insertions(+) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 468153c79c..43bb910ed6 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -391,6 +391,29 @@ qemuDomainMaybeStartPRDaemon(virDomainObjPtr vm, } +static int +qemuMaybeBuildPRManagerInfoProps(virDomainObjPtr vm, + qemuDomainDiskPRDPtr prd, + virJSONValuePtr *propsret) +{ + size_t i; + + *propsret = NULL; + + for (i = 0; i < vm->def->ndisks; i++) { + const virDomainDiskDef *domainDisk = vm->def->disks[i]; + + if (virStoragePRDefIsManaged(domainDisk->src->pr)) { + /* qemu-pr-helper should be already started because + * another disk in domain requires it. */ + return 0; + } + } + + return qemuBuildPRManagerInfoProps(prd, propsret); +} + + /** * qemuDomainAttachDiskGeneric: * @@ -411,13 +434,16 @@ qemuDomainAttachDiskGeneric(virQEMUDriverPtr driver, bool driveAdded = false; bool secobjAdded = false; bool encobjAdded = false; + bool prmgrAdded = false; bool prdStarted = false; virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); virJSONValuePtr secobjProps = NULL; virJSONValuePtr encobjProps = NULL; + virJSONValuePtr prmgrProps = NULL; qemuDomainStorageSourcePrivatePtr srcPriv; qemuDomainSecretInfoPtr secinfo = NULL; qemuDomainSecretInfoPtr encinfo = NULL; + qemuDomainDiskPRDPtr prd = NULL; if (qemuHotplugPrepareDiskAccess(driver, vm, disk, NULL, false) < 0) goto cleanup; @@ -437,6 +463,7 @@ qemuDomainAttachDiskGeneric(virQEMUDriverPtr driver, if (srcPriv) { secinfo = srcPriv->secinfo; encinfo = srcPriv->encinfo; + prd = srcPriv->prd; } if (secinfo && secinfo->type == VIR_DOMAIN_SECRET_INFO_TYPE_AES) { @@ -447,6 +474,9 @@ qemuDomainAttachDiskGeneric(virQEMUDriverPtr driver, if (encinfo && qemuBuildSecretInfoProps(encinfo, &encobjProps) < 0) goto error; + if (qemuMaybeBuildPRManagerInfoProps(vm, prd, &prmgrProps) < 0) + goto error; + if (disk->src->haveTLS && qemuDomainAddDiskSrcTLSObject(driver, vm, disk->src, disk->info.alias) < 0) @@ -484,6 +514,15 @@ qemuDomainAttachDiskGeneric(virQEMUDriverPtr driver, encobjAdded = true; } + if (prmgrProps) { + rv = qemuMonitorAddObject(priv->mon, "pr-manager-helper", prd->alias, + prmgrProps); + prmgrProps = NULL; /* qemuMonitorAddObject consumes */ + if (rv < 0) + goto exit_monitor; + prmgrAdded = true; + } + if (qemuMonitorAddDrive(priv->mon, drivestr) < 0) goto exit_monitor; driveAdded = true; @@ -521,6 +560,8 @@ qemuDomainAttachDiskGeneric(virQEMUDriverPtr driver, ignore_value(qemuMonitorDelObject(priv->mon, secinfo->s.aes.alias)); if (encobjAdded) ignore_value(qemuMonitorDelObject(priv->mon, encinfo->s.aes.alias)); + if (prmgrAdded) + ignore_value(qemuMonitorDelObject(priv->mon, prd->alias)); if (qemuDomainObjExitMonitor(driver, vm) < 0) ret = -2; virErrorRestore(&orig_err); -- 2.16.1

On 04/14/2018 05:14 PM, John Ferlan wrote: Okay the comment might be misleading. It should be s/started/added/. Anwyay, when adding a disk that has PR configured we need to: 1) start pr-helper process (if we are the ones managing it), 2) construct JSON for pr-manager object, 3) add pr-mananager object on the monitor, 4) finally, add disk on the monitor. And we need to do it in this order, otherwise qemu might try to connect to not yet running process. Now, obviously, since there can be only one managed pr-helper process per domain, there can be only one pr-manager object. Therefore, when somebody is trying to hotplug a disk with PR enabled & managed, but there already is a disk like that: step 1) turns into no-op (see previous patch), and we also need steps 2) and 3) to turn into no-op because the object is already in qemu. Trying to add it again would result in error. No, because we might end up trying to add pr-manager object that is already there in qemu which would fail and so would whole hotplug opertaion. Michal

[...] Oh yeah - coverity let me know that @prmgrProps could be leaked if we don't get this far - need to add the virJSONValueFree cleanup [...] John

If we are the last one to use pr-manager object we need to remove it and also kill the qemu-pr-helper process. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/qemu/qemu_hotplug.c | 38 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 43bb910ed6..98e1bf7082 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -3894,6 +3894,34 @@ static bool qemuIsMultiFunctionDevice(virDomainDefPtr def, } +static qemuDomainDiskPRDPtr +qemuDomainDiskNeedRemovePR(virDomainObjPtr vm, + virDomainDiskDefPtr disk) +{ + qemuDomainStorageSourcePrivatePtr srcPriv; + size_t i; + + if (!virStoragePRDefIsManaged(disk->src->pr)) + return NULL; + + for (i = 0; i < vm->def->ndisks; i++) { + const virDomainDiskDef *domainDisk = vm->def->disks[i]; + + if (domainDisk == disk) + continue; + + if (virStoragePRDefIsManaged(domainDisk->src->pr)) + break; + } + + if (i != vm->def->ndisks) + return NULL; + + srcPriv = QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(disk->src); + return srcPriv->prd; +} + + static int qemuDomainRemoveDiskDevice(virQEMUDriverPtr driver, virDomainObjPtr vm, @@ -3907,6 +3935,7 @@ qemuDomainRemoveDiskDevice(virQEMUDriverPtr driver, char *drivestr; char *objAlias = NULL; char *encAlias = NULL; + qemuDomainDiskPRDPtr prd = NULL; VIR_DEBUG("Removing disk %s from domain %p %s", disk->info.alias, vm, vm->def->name); @@ -3944,6 +3973,8 @@ qemuDomainRemoveDiskDevice(virQEMUDriverPtr driver, } } + prd = qemuDomainDiskNeedRemovePR(vm, disk); + qemuDomainObjEnterMonitor(driver, vm); qemuMonitorDriveDel(priv->mon, drivestr); @@ -3959,6 +3990,10 @@ qemuDomainRemoveDiskDevice(virQEMUDriverPtr driver, ignore_value(qemuMonitorDelObject(priv->mon, encAlias)); VIR_FREE(encAlias); + /* If it fails, then so be it - it was a best shot */ + if (prd) + ignore_value(qemuMonitorDelObject(priv->mon, prd->alias)); + if (disk->src->haveTLS) ignore_value(qemuMonitorDelObject(priv->mon, disk->src->tlsAlias)); @@ -3977,6 +4012,9 @@ qemuDomainRemoveDiskDevice(virQEMUDriverPtr driver, } } + if (prd) + qemuProcessKillPRDaemon(vm); + qemuDomainReleaseDeviceAddress(vm, &disk->info, src); if (qemuSecurityRestoreDiskLabel(driver, vm, disk) < 0) -- 2.16.1

On 04/14/2018 05:14 PM, John Ferlan wrote: Detach is called first and then, when guest OS stopped using the device (disk in this case) and so did qemu Remove is called. And we have to do the steps I described in 12/14 but in reverse order: 1) remove the disk (done already, no special handling from this feature POV), 2) remove pr-manager object (if no other disk still uses it), 3) kill the pr-helper process. The check from step 2) is done in qemuDomainDiskNeedRemovePR() and on success (=kill & remove) a non-NULL pointer is returned. Michal