On 04/10/2018 10:58 AM, Michal Privoznik wrote:

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@redhat.com> --- 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 @@ </disk> <disk type='block' device='lun'> <driver name='qemu' type='raw'/> - <source dev='/dev/sda'/> + <source dev='/dev/sda'> + <reservations enabled='yes' managed='no'> + <source type='unix' path='/path/to/qemu-pr-helper' mode='client'/> + </reservations> <target dev='sda' bus='scsi'/> <address type='drive' controller='0' bus='0' target='3' unit='0'/> </disk> @@ -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

Now at least 4.3

+ <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

s/enabled/enables

+ 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

<unrelatedRant>I wish we were consistent about using <code> around 'yes' and 'no' - some places use "yes" and/or "no".</unrelatedRant>

+ 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

s/any/a/ ? s/(should one be needed)// IOW: What really decides if one is needed?

+ libvirt is not enabled spawning helper process (i.e. hypervisor

Starting from "However, if.. " This reads strange - why not just indicate "If enabled is yes, then libvirt will ...; otherwise, libvirt will .... Taken from patch 4: + /* 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. + */ When the PR is unmanaged, then the path to its socket...

+ 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.

mode only Parse's and Format's "client"... Perhaps the best thing to indicate here is that since libvirt is then a client of the user provided pr-helper process and will use a unix socket to connect to the process, the type must be unix and the mode must be client with the path being the path to the socket which libvirt will attempt to connect to.

+ </dd> </dl>

<p>

[...]

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"));

allowed or required ;-) s/disks/devices/

+ 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

[...]

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);

Or just have virStoragePRDefFormat return if passed src->pr == NULL, IDC either way.

+ return 0; }

[...] With at least the formatdomain and lun device error message adjustment, Reviewed-by: John Ferlan <jferlan@redhat.com> John

On 04/10/2018 10:58 AM, Michal Privoznik wrote:

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@redhat.com> --- 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/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; +} +

two blank lines [...] Reviewed-by: John Ferlan <jferlan@redhat.com> John

On 04/10/2018 10:58 AM, Michal Privoznik wrote:

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

Well if it's only one, then it had better not conflict (IOW: after the and is unnecessary because there is check).

avoid any conflicts let's generate alias that's based on something unique - like disk target.

Make sure this commit message follows whatever happens in this patch...

Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- 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(-)

This patch does two things - one is just the pure alias/path for the pr-helper for the active domain. The second is copy that same information to the private storage source, which I'm not sure (yet) why it's necessary since both can be regenerated as needed based on fairly static data as opposed to secinfo and encinfo which get filled in with information only available during Prepare (the interaction with the secret driver and the need for the @conn pointer) that wasn't available during launch/command line building. Although some of that has changed with more recent changes to be able to get a secret conn "on the fly". Anyway, I digress. The point being - please note why you're also saving something in the private storage source area... The 'path' is already present in the domain XML (both active and inactive) and the 'alias' could be saved in the active output if you tweaked virStoragePRDefFormat to match what virDomainDeviceInfoFormat does.

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; +}

I don't really see a purpose for this function. If it were to survive, then it could take a parameter "const char *srcalias" and create/return the alias from that based on what's in qemuDomainPrepareDiskPRD. Eventually when the qemuProcessKillPRDaemon is created, it doesn't necessarily distinguish between managed and unmanaged... Still having it fail because it couldn't strdup what amounts to be a static string doesn't make much sense. 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; + }

Not only local, but local and LUN ... Still this check probably should have gone into the Validate code if anything.

+ + 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()))

just make it a straight VIR_STRDUP("pr-helper0") or some sort of #define for VIRT_PR_MANAGED_ALIAS that's usable by qemuProcessKillPRDaemon... You could go really obscure by using "pr-helper" as the defined value and then using that in all your various printf's. John

+ 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,

On 04/16/2018 10:56 AM, Michal Privoznik wrote:

On 04/13/2018 10:51 PM, John Ferlan wrote:

...

On 04/10/2018 10:58 AM, Michal Privoznik wrote:

...

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

Well if it's only one, then it had better not conflict (IOW: after the and is unnecessary because there is check).

...

avoid any conflicts let's generate alias that's based on something unique - like disk target.

Make sure this commit message follows whatever happens in this patch...

...

Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- 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(-)

This patch does two things - one is just the pure alias/path for the pr-helper for the active domain. The second is copy that same information to the private storage source, which I'm not sure (yet) why it's necessary since both can be regenerated as needed based on fairly static data as opposed to secinfo and encinfo which get filled in with information only available during Prepare (the interaction with the secret driver and the need for the @conn pointer) that wasn't available during launch/command line building. Although some of that has changed with more recent changes to be able to get a secret conn "on the fly". Anyway, I digress.

The point being - please note why you're also saving something in the private storage source area... The 'path' is already present in the domain XML (both active and inactive) and the 'alias' could be saved in the active output if you tweaked virStoragePRDefFormat to match what virDomainDeviceInfoFormat does.

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.

I don't think this is the same as @channelTargetDir (as you note in the next response). The problem there was a reliance on a name that ended up being too long and yes, it was a mess. I dunno - I think I've tried the future proofing things too and was told it was unnecessary. Still in order to "provide insight" into my why... Here you have a @path that's essentially static outside of the commonly used priv->libDir and an @alias that is static when managed. When unmanaged, the @path is provided by a consumer and the @alias is essentially a static prefix followed by a commonly used source destination alias. So nothing so far that would need to be saved in two places. Since @alias is an "internal" libvirt->qemu construct we wouldn't have the same problem as some "external" construct changing our definition. Unlike perhaps _qemuDomainVcpuPrivate where the alias perhaps changes based on ordering, insert, remove, etc. (I didn't look, but made the assumption). For PR there is one and it's not going to change. Even if it did, we would always know the former format to search on before using a newer format. In the long run, since there would be only one the PR alias belongs in the _qemuDomainObjPrivate further removing the need to have anything private. As noted somewhere during my review the existing need for a private structure for secrets could now be reworked/removed because the code now can generate the @conn on the fly rather than needing the @conn from the original connection in order to lookup the secret. Depends on if we want an error during command line generation or during the prepare phase.

...

...

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; +}

I don't really see a purpose for this function. If it were to survive, then it could take a parameter "const char *srcalias" and create/return the alias from that based on what's in qemuDomainPrepareDiskPRD.

The purpose is to generate alias at only one place instead of several virAsprintf-s scattered all around the code.

ug, sure this is similar to qemuDomainGetMasterKeyAlias or qemuAssignDeviceWatchdogAlias. Since there can only ever be one managed PR per domain, then scattered is I think limited to two places - start and stop.

...

Eventually when the qemuProcessKillPRDaemon is created, it doesn't necessarily distinguish between managed and unmanaged...

Well, unmanaged helper should not have its socket in libvirt private path. Under any circumstances.

The KillPRDaemon doesn't distinguish between managed/unmanaged, but callers do - perhaps wasn't as clear yet when I noted that...

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

Jumping forward 6 patches. Maybe it would have helped to have all the logic that would create a PR process "in place". Then add the disk logic. The two concepts are intertwined and related, but it is possible to separate too.

...

Still having it fail because it couldn't strdup what amounts to be a static string doesn't make much sense.

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,

I don't think I was advocating usage of a static string to be saved in the ->alias field. I was merely commenting on the need to have a helper to perform the strdup of a static string that could be some #define somewhere.... IOW, later on it's: if (VIR_STRDUP(prAlias, VIRT_PR_MANAGED_ALIAS) < 0) instead of if (!(prAlias = qemuDomainGetManagedPRAlias()))

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.

11 here, 11 there, it all adds up. But no, it's not the 11 or probably more like 16, 32, 64, etc. whatever the underlying code would consider it's smallest hunk. Still if you had 10 managed LUNs, that's a lot of replicated data.

...

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; + }

Not only local, but local and LUN ... Still this check probably should have gone into the Validate code if anything.

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.

ah yes, the volume lun conundrum, sigh. John

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.

...

...

+ + 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()))

just make it a straight VIR_STRDUP("pr-helper0")

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.

...

or some sort of #define for VIRT_PR_MANAGED_ALIAS that's usable by qemuProcessKillPRDaemon... You could go really obscure by using "pr-helper" as the defined value and then using that in all your various printf's.

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/10/2018 10:58 AM, Michal Privoznik wrote:

If qemu-pr-helper is compiled with multipath support the first thing it does is opening /dev/mapper/control. Since we're going

s/opening/open

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.

Not sure this explains whether it's multipath or namespaces that's the focus/cause of this patch. I thought multipath allows multiple ways to address the same LUN and namespace provides a mechanism to "control" device events and protections so that something like udev doesn't change something behind our back. For PR it's a mechanism to allow certain ioctl calls to be able to control ownership and/or write processing to the LUN so that it's not possible to have two writers. So does this patch add /dev/mapper/control to the namespace list because that's what will "control" the ioctl's to the LUN used by PR ???

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.

This hunk probably doesn't belong in a commit message... and well doesn't necessarily provide the confidence level for acceptance ;-)!

Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- 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;

alternatively char *dmPath = NULL;

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)

Alternatively: VIR_STRDUP(dmPath, DEVICE_MAPPER_CONTROL_PATH) < 0) && VIR_APPEND_ELEMENT_COPY(paths, npaths, dmPath)

+ goto cleanup; + if (qemuDomainNamespaceMknodPaths(vm, paths, npaths) < 0) return -1;

BTW: Existing, but this leaks @paths

VIR_FREE(dmPath); John

On 04/14/2018 02:55 PM, John Ferlan wrote:

On 04/10/2018 10:58 AM, Michal Privoznik wrote:

...

Just like in previous commit, qemu-pr-helper might want to open /dev/mapper/control under certain circumstances. Therefore we have to allow it in cgroups.

Perhaps the two patches should be combined then... yes, I know cgroups is different than namespace, so I understand the separation.

Yeah, I'd like to keep them separated. Even though they allow access to the same path they do that in different areas of the code.

...

Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_cgroup.c | 33 ++++++++++++++++++++++++++++++--- src/util/virdevmapper.c | 8 +++++++- 2 files changed, 37 insertions(+), 4 deletions(-)

diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index d88eb7881f..546a4c8e63 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c @@ -114,6 +114,8 @@ qemuSetupImagePathCgroup(virDomainObjPtr vm, }

+#define DEVICE_MAPPER_CONTROL_PATH "/dev/mapper/control" +

Almost too bad we didn't have a common place for this in some existing included .h file.

Yeah :(

...

static int qemuSetupImageCgroupInternal(virDomainObjPtr vm, virStorageSourcePtr src, @@ -125,6 +127,10 @@ qemuSetupImageCgroupInternal(virDomainObjPtr vm, return 0; }

+ if (virStoragePRDefIsManaged(src->pr) && + qemuSetupImagePathCgroup(vm, DEVICE_MAPPER_CONTROL_PATH, false) < 0) + return -1; +

Could the above be done potentially many times? Could be expensive, no? Considering what virDevMapperGetTargets[Impl] does...

It shouldn't be expensive. At the very first call of virDevMapperGetTargetsImpl() we get ENXIO (this the change below) and thus there only very small time penalty added.

...

return qemuSetupImagePathCgroup(vm, src->path, src->readonly || forceReadonly); }

@@ -142,9 +148,8 @@ qemuTeardownImageCgroup(virDomainObjPtr vm, virStorageSourcePtr src) { qemuDomainObjPrivatePtr priv = vm->privateData; - int perms = VIR_CGROUP_DEVICE_READ | - VIR_CGROUP_DEVICE_WRITE | - VIR_CGROUP_DEVICE_MKNOD; + int perms = VIR_CGROUP_DEVICE_RWM; + size_t i; int ret;

if (!virCgroupHasController(priv->cgroup, @@ -157,6 +162,28 @@ qemuTeardownImageCgroup(virDomainObjPtr vm, return 0; }

+ for (i = 0; i < vm->def->ndisks; i++) { + virStorageSourcePtr diskSrc = vm->def->disks[i]->src; + + if (src == diskSrc) + continue; + + if (virStoragePRDefIsManaged(diskSrc->pr)) + break; + } + + if (i == vm->def->ndisks) {

If there was only one that's managed and it's @src, then we don't get here...

How come? Say there are 3 disks for a domain and the first one is managed: disks = {A, B, C} (where A.managed = yes}. So the loop goes like this: qemuTeardownImageCgroup(A): i = 0; if (A.src == disks[0].src) //true continue; i = 1; if (A.src == disks[1].src) //false continue; if (isManaged(disks[1]) //false break; i = 2; if (A.src == disks[2].src) //false continue; if (isManaged(disks[2]) //false break; // Here, i == ndisks == 3; Or am I missing something?

...

+ VIR_DEBUG("Disabling device mapper control"); + ret = virCgroupDenyDevicePath(priv->cgroup, + DEVICE_MAPPER_CONTROL_PATH, perms, true);

You go through great lengths to ensure this is only done once converse to the qemuSetupImageCgroupInternal change...

Yes, because we can't deny helper the access if there's still another disk that might be using it (note that only managed disks use helper which runs in qemu's namespace/cgroup context). But we can allow it multiple times.

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?

Well, I view qemuSetupImageCgroup() and qemuTeardownImageCgroup() as entry points to CGroup mgmt. Therefore it's less lines of code to do decision there. Doing it anywhere else would be impractical IMO.

...

+ virDomainAuditCgroupPath(vm, priv->cgroup, "deny", + DEVICE_MAPPER_CONTROL_PATH, + virCgroupGetDevicePermsString(perms), ret); + if (ret < 0) + return ret; + } + + VIR_DEBUG("Deny path %s", src->path);

ret = virCgroupDenyDevicePath(priv->cgroup, src->path, perms, true); diff --git a/src/util/virdevmapper.c b/src/util/virdevmapper.c index d2c25af003..ef4b1e480a 100644 --- a/src/util/virdevmapper.c +++ b/src/util/virdevmapper.c @@ -101,8 +101,14 @@ virDevMapperGetTargetsImpl(const char *path,

dm_task_no_open_count(dmt);

- if (!dm_task_run(dmt)) + if (!dm_task_run(dmt)) { + if (errno == ENXIO) { + /* In some cases devmapper realizes this late device + * is not managed by it. */ + ret = 0; + }

Should this be separated? Or is it related to /dev/mapper/control?

No, this is exactly at the right spot in the correct patch. If virDevMapperGetTargetsImpl() sees /dev/mapper/control the dm_task_run() returns ENXIO. Speaking of which, this is not documented anywhere. I had to dig through devmapper sources to find out this errno is in fact returned by kernel. Michal

On 04/17/2018 02:25 PM, John Ferlan wrote:

On 04/16/2018 10:56 AM, Michal Privoznik wrote:

...

On 04/14/2018 02:55 PM, John Ferlan wrote:

...

On 04/10/2018 10:58 AM, Michal Privoznik wrote:

...

Just like in previous commit, qemu-pr-helper might want to open /dev/mapper/control under certain circumstances. Therefore we have to allow it in cgroups.

Perhaps the two patches should be combined then... yes, I know cgroups is different than namespace, so I understand the separation.

Yeah, I'd like to keep them separated. Even though they allow access to the same path they do that in different areas of the code.

Separate is fine...

...

...

...

Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_cgroup.c | 33 ++++++++++++++++++++++++++++++--- src/util/virdevmapper.c | 8 +++++++- 2 files changed, 37 insertions(+), 4 deletions(-)

diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index d88eb7881f..546a4c8e63 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c @@ -114,6 +114,8 @@ qemuSetupImagePathCgroup(virDomainObjPtr vm, }

+#define DEVICE_MAPPER_CONTROL_PATH "/dev/mapper/control" +

Almost too bad we didn't have a common place for this in some existing included .h file.

Yeah :(

...

...

static int qemuSetupImageCgroupInternal(virDomainObjPtr vm, virStorageSourcePtr src, @@ -125,6 +127,10 @@ qemuSetupImageCgroupInternal(virDomainObjPtr vm, return 0; }

+ if (virStoragePRDefIsManaged(src->pr) && + qemuSetupImagePathCgroup(vm, DEVICE_MAPPER_CONTROL_PATH, false) < 0) + return -1; +

Could the above be done potentially many times? Could be expensive, no? Considering what virDevMapperGetTargets[Impl] does...

It shouldn't be expensive. At the very first call of virDevMapperGetTargetsImpl() we get ENXIO (this the change below) and thus there only very small time penalty added.

Suffice to say it wasn't obvious to me WTF virDevMapperGetTargets[Impl] causes the ENXIO. That logic is not entirely self documenting.

...

...

...

return qemuSetupImagePathCgroup(vm, src->path, src->readonly || forceReadonly); }

@@ -142,9 +148,8 @@ qemuTeardownImageCgroup(virDomainObjPtr vm, virStorageSourcePtr src) { qemuDomainObjPrivatePtr priv = vm->privateData; - int perms = VIR_CGROUP_DEVICE_READ | - VIR_CGROUP_DEVICE_WRITE | - VIR_CGROUP_DEVICE_MKNOD; + int perms = VIR_CGROUP_DEVICE_RWM; + size_t i; int ret;

if (!virCgroupHasController(priv->cgroup, @@ -157,6 +162,28 @@ qemuTeardownImageCgroup(virDomainObjPtr vm, return 0; }

+ for (i = 0; i < vm->def->ndisks; i++) { + virStorageSourcePtr diskSrc = vm->def->disks[i]->src; + + if (src == diskSrc) + continue; + + if (virStoragePRDefIsManaged(diskSrc->pr)) + break; + } + + if (i == vm->def->ndisks) {

If there was only one that's managed and it's @src, then we don't get here...

How come? Say there are 3 disks for a domain and the first one is managed: disks = {A, B, C} (where A.managed = yes}.

So it made sense when I wrote the comment... let's see.

...

So the loop goes like this:

qemuTeardownImageCgroup(A): i = 0; if (A.src == disks[0].src) //true continue;

i = 1; if (A.src == disks[1].src) //false continue;

if (isManaged(disks[1]) //false break;

i = 2; if (A.src == disks[2].src) //false continue;

if (isManaged(disks[2]) //false break;

// Here, i == ndisks == 3;

Or am I missing something?

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.

Well, that would possibly allocate much more than 11 bytes ;-) What we can do, however, when qemu introduces the even support is to track if pr-helper is running (say, we'd have a bool under vm->privateData) and doing the following: if (vm->privateData->prRunning == false && virStoragePRDefIsManaged()) startPRHelper(); Hm.. in fact we can do something like that now even though it will not reflect reality 100%. But it's not going to be any worse that what we have now. Nor better though. If I will have to send yet another version, I might rework it. But as I say, it doesn't make any difference now. Michal

On 04/10/2018 10:58 AM, Michal Privoznik wrote:

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@redhat.com> --- 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

This patch fails qemuxml2argv for me because of commits 'cc32731a' and 'a32539de'... 1430) QEMU XML-2-ARGV disk-virtio-scsi-reservations ... In '/home/jferlan/git/libvirt.work/tests/qemuxml2argvdata/disk-virtio-scsi-reservations.args': Offset 405 Expect [defaults -chardev socket,id=charmonitor,path=/tmp/lib/domain--1-QEMUGuest1/monitor.sock,server,nowait -mon chardev=charmonitor,id=monitor,mode=readline] Actual [-user-config -nodefaults -chardev socket,id=charmonitor,path=/tmp/lib/domain--1-QEMUGuest1/monitor.sock,server,nowait -mon chardev=charmonitor,id=monitor,mode=control] ... FAILED Simple fix... regenerating the output gets: diff --git a/tests/qemuxml2argvdata/disk-virtio-scsi-reservations.args b/tests/qemuxml2argvdata/disk-virtio-scsi-reservations.args index 195e83a048..0bdd35a18a 100644 --- a/tests/qemuxml2argvdata/disk-virtio-scsi-reservations.args +++ b/tests/qemuxml2argvdata/disk-virtio-scsi-reservations.args @@ -16,10 +16,11 @@ path=/path/to/qemu-pr-helper.sock \ -smp 8,sockets=8,cores=1,threads=1 \ -uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \ -nographic \ +-no-user-config \ -nodefaults \ -chardev socket,id=charmonitor,path=/tmp/lib/domain--1-QEMUGuest1/monitor.sock,\ server,nowait \ --mon chardev=charmonitor,id=monitor,mode=readline \ +-mon chardev=charmonitor,id=monitor,mode=control \ -no-acpi \ -boot c \ -device virtio-scsi-pci,id=scsi0,num_queues=8,bus=pci.0,addr=0x3 \

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;

As soon as we find one that needs managing we should break from the loop and then only add pr-manager-helper if we have one to manage. No sense going through the rest of the list of disks. Move @disk into the top level and then use it to decide whether or not to add the object. Then in some way signify that the object was added so that future hotplugs wouldn't need to somehow guess - a simple check that it was added already would seem to suffice.

+ }

The next hunk would seem to be useful for the hotplug path, no? Including perhaps setting some sort of qemu_domain level boolean that the object was added already so that subsequent or consecutive hotplugs wouldn't try to add it again. John

+ + 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;

On 04/16/2018 10:56 AM, Michal Privoznik wrote:

On 04/14/2018 03:04 PM, John Ferlan wrote:

...

On 04/10/2018 10:58 AM, Michal Privoznik wrote:

...

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@redhat.com> --- 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

This patch fails qemuxml2argv for me because of commits 'cc32731a' and 'a32539de'...

Oh yeah. There's always chance that between me posting patches and review somebody will push something that invalidates my patches.

True, especially with the amount/rate of change in the capabilities area! In some ways it's, point it out, let you know I ACKnowledge that you'll have some merge work, but that work isn't something that'd be a problem w/r/t the overall patch.

...

...

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;

As soon as we find one that needs managing we should break from the loop and then only add pr-manager-helper if we have one to manage. No sense going through the rest of the list of disks. Move @disk into the top level and then use it to decide whether or not to add the object. Then in some way signify that the object was added so that future hotplugs wouldn't need to somehow guess - a simple check that it was added already would seem to suffice.

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.

oh, right... "pr-manager-helper" is the object name, <sigh>

...

...

+ }

The next hunk would seem to be useful for the hotplug path, no? Including perhaps setting some sort of qemu_domain level boolean that the object was added already so that subsequent or consecutive hotplugs wouldn't try to add it again.

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.

OK - I had scanned that review and had that comment in mind, but didn't correlate the two when going through this (nor go back and check here once I looked at hotplug later). John

On 04/14/2018 03:35 PM, John Ferlan wrote:

On 04/10/2018 10:58 AM, Michal Privoznik wrote:

...

Just like we allow users overriding path to bridge-helper detected at compile time we can allow them to override path to qemu-pr-helper.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- m4/virt-driver-qemu.m4 | 5 +++++ src/qemu/libvirtd_qemu.aug | 1 + src/qemu/qemu.conf | 4 ++++ src/qemu/qemu_conf.c | 7 ++++++- src/qemu/qemu_conf.h | 1 + src/qemu/test_libvirtd_qemu.aug.in | 1 + 6 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/m4/virt-driver-qemu.m4 b/m4/virt-driver-qemu.m4 index b9bafdab90..80e1d3ad46 100644 --- a/m4/virt-driver-qemu.m4 +++ b/m4/virt-driver-qemu.m4 @@ -57,6 +57,11 @@ AC_DEFUN([LIBVIRT_DRIVER_CHECK_QEMU], [ [/usr/libexec:/usr/lib/qemu:/usr/lib]) AC_DEFINE_UNQUOTED([QEMU_BRIDGE_HELPER], ["$QEMU_BRIDGE_HELPER"], [QEMU bridge helper]) + AC_PATH_PROG([QEMU_PR_HELPER], [qemu-pr-helper], + [/usr/bin/qemu-pr-helper], + [/usr/bin:/usr/libexec])

So the default install location of qemu-pr-helper is /usr/bin unlike bridge-helper which is /usr/libexec?

Yes. I guess this has something to do with aforementioned systemd socket activation.

...

+ AC_DEFINE_UNQUOTED([QEMU_PR_HELPER], ["$QEMU_PR_HELPER"], + [QEMU PR helper]) ])

AC_DEFUN([LIBVIRT_DRIVER_RESULT_QEMU], [ diff --git a/src/qemu/libvirtd_qemu.aug b/src/qemu/libvirtd_qemu.aug index c19bf3a43a..2dc16e91fd 100644 --- a/src/qemu/libvirtd_qemu.aug +++ b/src/qemu/libvirtd_qemu.aug @@ -86,6 +86,7 @@ module Libvirtd_qemu = let process_entry = str_entry "hugetlbfs_mount" | bool_entry "clear_emulator_capabilities" | str_entry "bridge_helper" + | str_entry "pr_helper" | bool_entry "set_process_name" | int_entry "max_processes" | int_entry "max_files" diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf index 07eab7efff..30fdd54e2c 100644 --- a/src/qemu/qemu.conf +++ b/src/qemu/qemu.conf @@ -775,3 +775,7 @@ # This directory is used for memoryBacking source if configured as file. # NOTE: big files will be stored here #memory_backing_dir = "/var/lib/libvirt/qemu/ram" + +# Path to the SCSI persistent reservations helper. This helper is +# used whenever <reservations/> are enabled for SCSI disks.

s/disks/LUN devices/

...

+#pr_helper = "/usr/libexec/qemu-pr-helper"

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?

Yeah, this is a leftover from previous rounds where I had /usr/libexec/. I'm changing this to /usr/bin/.

...

diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index 36cf3a281c..8c69dbe75c 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -307,7 +307,8 @@ virQEMUDriverConfigPtr virQEMUDriverConfigNew(bool privileged) goto error; }

- if (VIR_STRDUP(cfg->bridgeHelperName, QEMU_BRIDGE_HELPER) < 0) + if (VIR_STRDUP(cfg->bridgeHelperName, QEMU_BRIDGE_HELPER) < 0 || + VIR_STRDUP(cfg->prHelperName, QEMU_PR_HELPER) < 0) goto error;

cfg->clearEmulatorCapabilities = true; @@ -392,6 +393,7 @@ static void virQEMUDriverConfigDispose(void *obj) } VIR_FREE(cfg->hugetlbfs); VIR_FREE(cfg->bridgeHelperName); + VIR_FREE(cfg->prHelperName);

VIR_FREE(cfg->saveImageFormat); VIR_FREE(cfg->dumpImageFormat); @@ -759,6 +761,9 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg, if (virConfGetValueString(conf, "bridge_helper", &cfg->bridgeHelperName) < 0) goto cleanup;

+ if (virConfGetValueString(conf, "pr_helper", &cfg->prHelperName) < 0) + goto cleanup; + if (virConfGetValueBool(conf, "mac_filter", &cfg->macFilter) < 0) goto cleanup;

diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h index e1ad5463f3..7a63780c48 100644 --- a/src/qemu/qemu_conf.h +++ b/src/qemu/qemu_conf.h @@ -153,6 +153,7 @@ struct _virQEMUDriverConfig { size_t nhugetlbfs;

char *bridgeHelperName; + char *prHelperName;

bool macFilter;

diff --git a/src/qemu/test_libvirtd_qemu.aug.in b/src/qemu/test_libvirtd_qemu.aug.in index 688e5b9fda..c0efae47bd 100644 --- a/src/qemu/test_libvirtd_qemu.aug.in +++ b/src/qemu/test_libvirtd_qemu.aug.in @@ -100,3 +100,4 @@ module Test_libvirtd_qemu = { "1" = "mount" } } { "memory_backing_dir" = "/var/lib/libvirt/qemu/ram" } +{ "pr_helper" = "/usr/libexec/qemu-pr-helper" }

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@redhat.com>

Thanks, Michal

On 04/10/2018 10:58 AM, Michal Privoznik wrote:

Before we exec() qemu we have to spawn pr-helper processes for all managed reservations (well, technically there can only one).

Don't mince words - what have to spawn the qemu-pr-helper process for all managed persistent reservations.

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@redhat.com> --- 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;

I would hope that we'd be able to figure out in some way that we actually started this for the the domain.

+ + if (!(prAlias = qemuDomainGetManagedPRAlias())) { + VIR_WARN("Unable to get default PR manager alias"); + return; + } + + if (!(prPidfile = qemuProcessBuildPRHelperPidfilePath(vm, prAlias))) {

Since we know that we're using the managed one, then we should be able to pass/formulate the "static" prAlias here rather than possibly failing because we don't have enough memory above and then need to free it immediately afterwards.

+ 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);

Coverity lets us know we can get here w/ pidfile == NULL which isn't good for unlink.

+ } + 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; + }

Now that we know we needed to start one, then once we're successful we should be able to store that we did start it... and that could be added to some sort of private structure. This is where I could see a private data being used. John

+ + 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);

On 04/16/2018 10:57 AM, Michal Privoznik wrote:

On 04/14/2018 04:56 PM, John Ferlan wrote:

...

On 04/10/2018 10:58 AM, Michal Privoznik wrote:

...

Before we exec() qemu we have to spawn pr-helper processes for all managed reservations (well, technically there can only one).

Don't mince words - what have to spawn the qemu-pr-helper process for all managed persistent reservations.

...

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@redhat.com> --- 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;

I would hope that we'd be able to figure out in some way that we actually started this for the the domain.

...

+ + if (!(prAlias = qemuDomainGetManagedPRAlias())) { + VIR_WARN("Unable to get default PR manager alias"); + return; + } + + if (!(prPidfile = qemuProcessBuildPRHelperPidfilePath(vm, prAlias))) {

Since we know that we're using the managed one, then we should be able to pass/formulate the "static" prAlias here rather than possibly failing because we don't have enough memory above and then need to free it immediately afterwards.

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().

True, if we're lacking memory not much is going to happen.

...

...

+ 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 +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; + }

Now that we know we needed to start one, then once we're successful we should be able to store that we did start it... and that could be added to some sort of private structure. This is where I could see a private data being used.

Well, we can have that when qemu finally implements events. Then we can keep this internal state in sync with reality.

The managed pr-helper is domain level based on some storage sources needing it... That's more what I was referring to. If the domain private kept track of it being started (no matter how it did so), then as we found/added/hotplugged a storage source that needed it, then we wouldn't have to keep running the entire ndisks and checking whether something may have already added it or not. I dunno, it's I guess it's just implementation details and a different way of thinking about how to relate this bolted on piece of software. John

On 04/10/2018 10:58 AM, Michal Privoznik wrote:

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@redhat.com> --- 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(-)

Personally not a fan of splitting up the hotplug need for PR daemon from need for adding the object to the disk and the separate unplug patch, but I understand why it's done this way.

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; + } +

If we used the fact that the thing was already started for the domain in some sort of domain private data (which could even store the PID-file path), then we wouldn't need the following...

+ 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;

and the above would set things for us (conceivably) John

+ + 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__ */

On 04/10/2018 10:58 AM, Michal Privoznik wrote:

When attaching a disk that requires pr-manager we might need to plug the pr-manager object too.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_hotplug.c | 41 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 41 insertions(+)

My opinion - combine w/ previous patch.

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; + } + }

I don't understand the need for the above hunk - that was done in patch11 wasn't it? This is why I like things combined - going back and forth and changing logic as patches are developed means something may be missed.

+ + 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;

As noted much earlier - the private data isn't needed here as the prd is in the disk storage definition.

}

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; +

This should just call qemuBuildPRManagerInfoProps directly since it already checks if !prd || !prd->alias John

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);

On 04/16/2018 10:57 AM, Michal Privoznik wrote:

On 04/14/2018 05:14 PM, John Ferlan wrote:

...

On 04/10/2018 10:58 AM, Michal Privoznik wrote:

...

When attaching a disk that requires pr-manager we might need to plug the pr-manager object too.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_hotplug.c | 41 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 41 insertions(+)

My opinion - combine w/ previous patch.

...

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; + } + }

I don't understand the need for the above hunk - that was done in patch11 wasn't it? This is why I like things combined - going back and forth and changing logic as patches are developed means something may be missed.

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.

qemuDomainMaybeStartPRDaemon which is a few lines above here and in the previous patch is what handled potentially starting the PR Daemon. Once I get here, then I'm running through the same list, doing the same check, and then deciding whether to create the object, but that would seem to be the task of the previous patch, but they're intertwined dependencies, so IMO, one patch. Since adding the ",file.pr-manager=%s" is part of qemuBuildDriveDevStr, it's not like anything done in this patch that's doing anything that couldn't be combined with the previous one.

...

...

+ + 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;

As noted much earlier - the private data isn't needed here as the prd is in the disk storage definition.

...

}

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; +

This should just call qemuBuildPRManagerInfoProps directly since it already checks if !prd || !prd->alias

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.

Of course the comment came from the same notion that running through ndisks again isn't really necessary and my continued thought that the 11 and 12 should be combined. John

On 04/14/2018 05:18 PM, John Ferlan wrote:

[...]

...

* @@ -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; + } +

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

Ah, sure. Nice catch. Michal

On 04/10/2018 10:58 AM, Michal Privoznik wrote:

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@redhat.com> --- src/qemu/qemu_hotplug.c | 38 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+)

Again, my opinion this is combined too - patch is larger, but everything is covered at one time.

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; +} + +

Trying to combine two separate things into one? Thing 1 -> Remove the object from the domain Thing 2 -> Kill the PRD helper *iff* this is the last one using it

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)); +

OK - remove the object if we have one...

if (disk->src->haveTLS) ignore_value(qemuMonitorDelObject(priv->mon, disk->src->tlsAlias));

@@ -3977,6 +4012,9 @@ qemuDomainRemoveDiskDevice(virQEMUDriverPtr driver, } }

+ if (prd) + qemuProcessKillPRDaemon(vm); +

But we only want to kill if this if there are no other disk's needing the pr-helper, right? So we need to have a routine that would be run after the disk is really removed. The two phase removal process always gets me ;-) - that is Detach vs. Remove and which one is last *after* the disk is removed from the list of disks *and* when we know the object has been removed from the domain, then we can stop the PR process iff this is the last one using it. John

qemuDomainReleaseDeviceAddress(vm, &disk->info, src);

if (qemuSecurityRestoreDiskLabel(driver, vm, disk) < 0)