[libvirt] [PATCH 0/6] qemu: track presence of blockjobs in status XML (blockdev-add saga)
Add a flag to the status XML to tell that no blockjobs are running, so that we don't have to reprobe the backing chain on libvirt restart when no jobs were running. Later on when we will be able to recover jobs we can improve that even for running jobs. Peter Krempa (6): qemu: blockjob: Always save status XML after block event qemu: blockjob: Always save config XML when a blockjob is finished qemu: driver: Save status XML when starting a block job tests: qemuxml2xml: Simplify adding to the status XML expect string generator qemu: domain: Mark if no blockjobs are active in the status XML qemu: process: Don't redetect backing chain on reconnect src/qemu/qemu_blockjob.c | 16 +++------- src/qemu/qemu_domain.c | 39 +++++++++++++++++++++++ src/qemu/qemu_domain.h | 3 ++ src/qemu/qemu_driver.c | 32 ++++++++++--------- src/qemu/qemu_process.c | 19 +++++++---- tests/qemuxml2xmltest.c | 83 +++++++++++++++++++++++++++++++++++++----------- 6 files changed, 141 insertions(+), 51 deletions(-) -- 2.14.1
The status XML would be saved only for the copy job (in case of success) or on failure even for other jobs. As the status contains the backing chain data, which change after success we should always save it on block job completion. --- src/qemu/qemu_blockjob.c | 22 ++++++++-------------- 1 file changed, 8 insertions(+), 14 deletions(-) diff --git a/src/qemu/qemu_blockjob.c b/src/qemu/qemu_blockjob.c index 415768ddc..f061bb743 100644 --- a/src/qemu/qemu_blockjob.c +++ b/src/qemu/qemu_blockjob.c @@ -97,7 +97,6 @@ qemuBlockJobEventProcess(virQEMUDriverPtr driver, const char *path; virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); virDomainDiskDefPtr persistDisk = NULL; - bool save = false; qemuDomainDiskPrivatePtr diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk); VIR_DEBUG("disk=%s, mirrorState=%s, type=%d, status=%d", @@ -164,7 +163,6 @@ qemuBlockJobEventProcess(virQEMUDriverPtr driver, * rather than reprobing, but we haven't quite completed * that conversion to use our XML tracking. */ disk->mirror = NULL; - save = disk->mirrorState != VIR_DOMAIN_DISK_MIRROR_STATE_NONE; disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_STATE_NONE; disk->mirrorJob = VIR_DOMAIN_BLOCK_JOB_TYPE_UNKNOWN; ignore_value(qemuDomainDetermineDiskChain(driver, vm, disk, @@ -175,7 +173,6 @@ qemuBlockJobEventProcess(virQEMUDriverPtr driver, case VIR_DOMAIN_BLOCK_JOB_READY: disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_STATE_READY; - save = true; break; case VIR_DOMAIN_BLOCK_JOB_FAILED: @@ -187,7 +184,6 @@ qemuBlockJobEventProcess(virQEMUDriverPtr driver, } disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_STATE_NONE; disk->mirrorJob = VIR_DOMAIN_BLOCK_JOB_TYPE_UNKNOWN; - save = true; diskPriv->blockjob = false; break; @@ -195,16 +191,14 @@ qemuBlockJobEventProcess(virQEMUDriverPtr driver, break; } - if (save) { - if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm, driver->caps) < 0) - VIR_WARN("Unable to save status on vm %s after block job", - vm->def->name); - if (persistDisk && virDomainSaveConfig(cfg->configDir, - driver->caps, - vm->newDef) < 0) - VIR_WARN("Unable to update persistent definition on vm %s " - "after block job", vm->def->name); - } + if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm, driver->caps) < 0) + VIR_WARN("Unable to save status on vm %s after block job", vm->def->name); + + if (persistDisk && virDomainSaveConfig(cfg->configDir, + driver->caps, + vm->newDef) < 0) + VIR_WARN("Unable to update persistent definition on vm %s " + "after block job", vm->def->name); qemuDomainEventQueue(driver, event); qemuDomainEventQueue(driver, event2); -- 2.14.1
On 10/04/2017 07:59 AM, Peter Krempa wrote:
The status XML would be saved only for the copy job (in case of success) or on failure even for other jobs. As the status contains the backing chain data, which change after success we should always save it on block job completion. --- src/qemu/qemu_blockjob.c | 22 ++++++++-------------- 1 file changed, 8 insertions(+), 14 deletions(-)
Reviewed-by: John Ferlan <jferlan@redhat.com> John
For VMs with persistent config the config may change upon successful completion of a job. Save it always if a persistent VM finishes a blockjob. This will simplify further additions. --- src/qemu/qemu_blockjob.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/qemu/qemu_blockjob.c b/src/qemu/qemu_blockjob.c index f061bb743..c1b46f7d0 100644 --- a/src/qemu/qemu_blockjob.c +++ b/src/qemu/qemu_blockjob.c @@ -194,11 +194,11 @@ qemuBlockJobEventProcess(virQEMUDriverPtr driver, if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm, driver->caps) < 0) VIR_WARN("Unable to save status on vm %s after block job", vm->def->name); - if (persistDisk && virDomainSaveConfig(cfg->configDir, - driver->caps, - vm->newDef) < 0) - VIR_WARN("Unable to update persistent definition on vm %s " - "after block job", vm->def->name); + if (status == VIR_DOMAIN_BLOCK_JOB_COMPLETED && vm->newDef) { + if (virDomainSaveConfig(cfg->configDir, driver->caps, vm->newDef) < 0) + VIR_WARN("Unable to update persistent definition on vm %s " + "after block job", vm->def->name); + } qemuDomainEventQueue(driver, event); qemuDomainEventQueue(driver, event2); -- 2.14.1
On 10/04/2017 07:59 AM, Peter Krempa wrote:
For VMs with persistent config the config may change upon successful completion of a job. Save it always if a persistent VM finishes a blockjob. This will simplify further additions. --- src/qemu/qemu_blockjob.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)
Reviewed-by: John Ferlan <jferlan@redhat.com> John
Few jobs (which don't) use the 'mirror' element did not save the status XML. It will be helpful to do so once we start tracking jobs fully. --- src/qemu/qemu_driver.c | 32 +++++++++++++++++--------------- 1 file changed, 17 insertions(+), 15 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 4855c9047..cf60b8d6b 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -16489,6 +16489,7 @@ qemuDomainBlockPullCommon(virQEMUDriverPtr driver, unsigned int flags) { qemuDomainObjPrivatePtr priv = vm->privateData; + virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); char *device = NULL; virDomainDiskDefPtr disk; virStorageSourcePtr baseSource = NULL; @@ -16580,10 +16581,15 @@ qemuDomainBlockPullCommon(virQEMUDriverPtr driver, QEMU_DOMAIN_DISK_PRIVATE(disk)->blockjob = true; + if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm, driver->caps) < 0) + VIR_WARN("Unable to save status on vm %s after state change", + vm->def->name); + endjob: qemuDomainObjEndJob(driver, vm); cleanup: + virObjectUnref(cfg); VIR_FREE(basePath); VIR_FREE(backingPath); VIR_FREE(device); @@ -17311,6 +17317,7 @@ qemuDomainBlockCommit(virDomainPtr dom, unsigned int flags) { virQEMUDriverPtr driver = dom->conn->privateData; + virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); qemuDomainObjPrivatePtr priv; virDomainObjPtr vm = NULL; char *device = NULL; @@ -17503,24 +17510,18 @@ qemuDomainBlockCommit(virDomainPtr dom, goto endjob; } - if (ret == 0) + if (ret == 0) { QEMU_DOMAIN_DISK_PRIVATE(disk)->blockjob = true; - - if (mirror) { - if (ret == 0) { - virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); - - mirror = NULL; - if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm, driver->caps) < 0) - VIR_WARN("Unable to save status on vm %s after block job", - vm->def->name); - virObjectUnref(cfg); - } else { - disk->mirror = NULL; - disk->mirrorJob = VIR_DOMAIN_BLOCK_JOB_TYPE_UNKNOWN; - } + mirror = NULL; + } else { + disk->mirror = NULL; + disk->mirrorJob = VIR_DOMAIN_BLOCK_JOB_TYPE_UNKNOWN; } + if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm, driver->caps) < 0) + VIR_WARN("Unable to save status on vm %s after block job", + vm->def->name); + endjob: if (ret < 0 && clean_access) { virErrorPtr orig_err = virSaveLastError(); @@ -17542,6 +17543,7 @@ qemuDomainBlockCommit(virDomainPtr dom, VIR_FREE(basePath); VIR_FREE(backingPath); VIR_FREE(device); + virObjectUnref(cfg); virDomainObjEndAPI(&vm); return ret; } -- 2.14.1
On 10/04/2017 07:59 AM, Peter Krempa wrote:
Few jobs (which don't) use the 'mirror' element did not save the status XML. It will be helpful to do so once we start tracking jobs fully. --- src/qemu/qemu_driver.c | 32 +++++++++++++++++--------------- 1 file changed, 17 insertions(+), 15 deletions(-)
[...]
@@ -17311,6 +17317,7 @@ qemuDomainBlockCommit(virDomainPtr dom, unsigned int flags) { virQEMUDriverPtr driver = dom->conn->privateData; + virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); qemuDomainObjPrivatePtr priv; virDomainObjPtr vm = NULL; char *device = NULL;
There's a : virCheckFlags(VIR_DOMAIN_BLOCK_COMMIT_SHALLOW |... , -1); that will leak cfg... with obvious adjustment... Reviewed-by: John Ferlan <jferlan@redhat.com> John [...]
Unindent the static XML block and move around the autoindent calls so that further additions don't have to add more of them. Also rename the string holding the static XML section. --- tests/qemuxml2xmltest.c | 37 +++++++++++++++++++------------------ 1 file changed, 19 insertions(+), 18 deletions(-) diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index 2dba3607c..4e21f5825 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -81,23 +81,23 @@ static const char testStatusXMLPrefixHeader[] = " <taint flag='high-privileges'/>\n" " <monitor path='/var/lib/libvirt/qemu/test.monitor' json='1' type='unix'/>\n"; -static const char testStatusXMLPrefixFooter[] = -" <qemuCaps>\n" -" <flag name='vnet-hdr'/>\n" -" <flag name='qxl.vgamem_mb'/>\n" -" <flag name='qxl-vga.vgamem_mb'/>\n" -" <flag name='pc-dimm'/>\n" -" </qemuCaps>\n" -" <devices>\n" -" <device alias='balloon0'/>\n" -" <device alias='video0'/>\n" -" <device alias='serial0'/>\n" -" <device alias='net0'/>\n" -" <device alias='usb'/>\n" -" </devices>\n" -" <numad nodeset='0-2' cpuset='1,3'/>\n" -" <libDir path='/tmp'/>\n" -" <channelTargetDir path='/tmp/channel'/>\n"; +static const char testStatusXMLPrefixBodyStatic[] = +"<qemuCaps>\n" +" <flag name='vnet-hdr'/>\n" +" <flag name='qxl.vgamem_mb'/>\n" +" <flag name='qxl-vga.vgamem_mb'/>\n" +" <flag name='pc-dimm'/>\n" +"</qemuCaps>\n" +"<devices>\n" +" <device alias='balloon0'/>\n" +" <device alias='video0'/>\n" +" <device alias='serial0'/>\n" +" <device alias='net0'/>\n" +" <device alias='usb'/>\n" +"</devices>\n" +"<numad nodeset='0-2' cpuset='1,3'/>\n" +"<libDir path='/tmp'/>\n" +"<channelTargetDir path='/tmp/channel'/>\n"; static const char testStatusXMLSuffix[] = "</domstatus>\n"; @@ -134,8 +134,9 @@ testGetStatusXMLPrefix(const struct testInfo *data) testGetStatuXMLPrefixVcpus(&buf, data); + virBufferAddStr(&buf, testStatusXMLPrefixBodyStatic); + virBufferAdjustIndent(&buf, -2); - virBufferAdd(&buf, testStatusXMLPrefixFooter, -1); return virBufferContentAndReset(&buf); } -- 2.14.1
On 10/04/2017 07:59 AM, Peter Krempa wrote:
Unindent the static XML block and move around the autoindent calls so that further additions don't have to add more of them.
Also rename the string holding the static XML section. --- tests/qemuxml2xmltest.c | 37 +++++++++++++++++++------------------ 1 file changed, 19 insertions(+), 18 deletions(-)
Reviewed-by: John Ferlan <jferlan@redhat.com> John
Note when no blockjobs are running in the status XML so that we know that the backing chain will not change until we reconnect. --- src/qemu/qemu_domain.c | 39 +++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_domain.h | 3 +++ tests/qemuxml2xmltest.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 88 insertions(+) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index b202d02f9..2bc8f38dc 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -1759,6 +1759,8 @@ qemuDomainObjPrivateDataClear(qemuDomainObjPrivatePtr priv) /* clear previously used namespaces */ virBitmapFree(priv->namespaces); priv->namespaces = NULL; + + priv->reconnectNoActiveBlockjobs = false; } @@ -1854,6 +1856,21 @@ qemuDomainObjPrivateXMLFormatAutomaticPlacement(virBufferPtr buf, } +static int +qemuDomainObjPrivateXMLFormatBlockjobs(virBufferPtr buf, + virDomainObjPtr vm) +{ + virBuffer attrBuf = VIR_BUFFER_INITIALIZER; + + if (!qemuDomainHasBlockjob(vm, false)) + virBufferAddLit(&attrBuf, " active='no'"); + else + virBufferAddLit(&attrBuf, " active='yes'"); + + return virXMLFormatElement(buf, "blockjobs", &attrBuf, NULL); +} + + static int qemuDomainObjPrivateXMLFormat(virBufferPtr buf, virDomainObjPtr vm) @@ -1976,6 +1993,9 @@ qemuDomainObjPrivateXMLFormat(virBufferPtr buf, if (priv->chardevStdioLogd) virBufferAddLit(buf, "<chardevStdioLogd/>\n"); + if (qemuDomainObjPrivateXMLFormatBlockjobs(buf, vm) < 0) + return -1; + return 0; } @@ -2067,6 +2087,22 @@ qemuDomainObjPrivateXMLParseAutomaticPlacement(xmlXPathContextPtr ctxt, } +static int +qemuDomainObjPrivateXMLParseBlockjobs(qemuDomainObjPrivatePtr priv, + xmlXPathContextPtr ctxt) +{ + char *active; + + if ((active = virXPathString("string(./blockjobs/@active)", ctxt))) { + if (virTristateBoolTypeFromString(active) == VIR_TRISTATE_BOOL_NO) + priv->reconnectNoActiveBlockjobs = true; + } + + VIR_FREE(active); + return 0; +} + + static int qemuDomainObjPrivateXMLParse(xmlXPathContextPtr ctxt, virDomainObjPtr vm, @@ -2282,6 +2318,9 @@ qemuDomainObjPrivateXMLParse(xmlXPathContextPtr ctxt, priv->chardevStdioLogd = virXPathBoolean("boolean(./chardevStdioLogd)", ctxt) == 1; + if (qemuDomainObjPrivateXMLParseBlockjobs(priv, ctxt) < 0) + goto error; + return 0; error: diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 1a47396ab..82248b56f 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -320,6 +320,9 @@ struct _qemuDomainObjPrivate { /* If true virtlogd is used as stdio handler for character devices. */ bool chardevStdioLogd; + + /* if true there were no active blockjobs at reconnect to the VM */ + bool reconnectNoActiveBlockjobs; }; # define QEMU_DOMAIN_PRIVATE(vm) \ diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index 4e21f5825..5339f673f 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -34,6 +34,7 @@ struct testInfo { char *outInactiveName; virBitmapPtr activeVcpus; + bool blockjobs; virQEMUCapsPtr qemuCaps; }; @@ -43,11 +44,22 @@ qemuXML2XMLActivePreFormatCallback(virDomainDefPtr def, const void *opaque) { struct testInfo *info = (struct testInfo *) opaque; + size_t i; /* store vCPU bitmap so that the status XML can be created faithfully */ if (!info->activeVcpus) info->activeVcpus = virDomainDefGetOnlineVcpumap(def); + info->blockjobs = false; + + /* remember whether we have mirror jobs */ + for (i = 0; i < def->ndisks; i++) { + if (def->disks[i]->mirror) { + info->blockjobs = true; + break; + } + } + return 0; } @@ -124,6 +136,17 @@ testGetStatuXMLPrefixVcpus(virBufferPtr buf, } +static void +testGetStatusXMLAddBlockjobs(virBufferPtr buf, + const struct testInfo *data) +{ + if (data->blockjobs) + virBufferAddLit(buf, "<blockjobs active='yes'/>\n"); + else + virBufferAddLit(buf, "<blockjobs active='no'/>\n"); +} + + static char * testGetStatusXMLPrefix(const struct testInfo *data) { @@ -136,12 +159,31 @@ testGetStatusXMLPrefix(const struct testInfo *data) virBufferAddStr(&buf, testStatusXMLPrefixBodyStatic); + testGetStatusXMLAddBlockjobs(&buf, data); + virBufferAdjustIndent(&buf, -2); return virBufferContentAndReset(&buf); } +static int +testProcessStatusXML(virDomainObjPtr vm) +{ + size_t i; + + /* fix the private 'blockjob' flag for disks */ + for (i = 0; i < vm->def->ndisks; i++) { + virDomainDiskDefPtr disk = vm->def->disks[i]; + qemuDomainDiskPrivatePtr diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk); + + diskPriv->blockjob = !!disk->mirror; + } + + return 0; +} + + static int testCompareStatusXMLToXMLFiles(const void *opaque) { @@ -200,6 +242,10 @@ testCompareStatusXMLToXMLFiles(const void *opaque) goto cleanup; } + /* process the definition if necessary */ + if (testProcessStatusXML(obj) < 0) + goto cleanup; + /* format it back */ if (!(actual = virDomainObjFormat(driver.xmlopt, obj, NULL, VIR_DOMAIN_DEF_FORMAT_SECURE))) { -- 2.14.1
On 10/04/2017 07:59 AM, Peter Krempa wrote:
Note when no blockjobs are running in the status XML so that we know that the backing chain will not change until we reconnect. --- src/qemu/qemu_domain.c | 39 +++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_domain.h | 3 +++ tests/qemuxml2xmltest.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 88 insertions(+)
Was there a special reason to use the double negative of "reconnectNoActiveBlockjobs" instead of "reconnectActiveBlockjobs" or "reconnectDetermineDiskChain". If it's not present in the status XML, then the default is there are active block jobs or more succinctly as the next patch shows - we need to determine the backing chain when either active == true or it's not defined in the status XML (a/k/a VIR_TRISTATE_BOOL_ABSENT). Also since the definition is much lower and making this comment now makes subsequent comments easier to understand, what not make it a virTristateBool since that's how you parse it (essentially)?
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index b202d02f9..2bc8f38dc 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -1759,6 +1759,8 @@ qemuDomainObjPrivateDataClear(qemuDomainObjPrivatePtr priv) /* clear previously used namespaces */ virBitmapFree(priv->namespaces); priv->namespaces = NULL; + + priv->reconnectNoActiveBlockjobs = false;
If this were a tristate, then this would be ABSENT (or 0)...
}
@@ -1854,6 +1856,21 @@ qemuDomainObjPrivateXMLFormatAutomaticPlacement(virBufferPtr buf, }
+static int +qemuDomainObjPrivateXMLFormatBlockjobs(virBufferPtr buf, + virDomainObjPtr vm) +{ + virBuffer attrBuf = VIR_BUFFER_INITIALIZER; + + if (!qemuDomainHasBlockjob(vm, false)) + virBufferAddLit(&attrBuf, " active='no'"); + else + virBufferAddLit(&attrBuf, " active='yes'");
And these would use virTristateSwitchTypeToString of course. Whether you keep the same order to take "active='no'" == ABSENT is your call (that is, no special need to format 'no')
+ + return virXMLFormatElement(buf, "blockjobs", &attrBuf, NULL); +} + + static int qemuDomainObjPrivateXMLFormat(virBufferPtr buf, virDomainObjPtr vm) @@ -1976,6 +1993,9 @@ qemuDomainObjPrivateXMLFormat(virBufferPtr buf, if (priv->chardevStdioLogd) virBufferAddLit(buf, "<chardevStdioLogd/>\n");
+ if (qemuDomainObjPrivateXMLFormatBlockjobs(buf, vm) < 0) + return -1; + return 0; }
@@ -2067,6 +2087,22 @@ qemuDomainObjPrivateXMLParseAutomaticPlacement(xmlXPathContextPtr ctxt, }
+static int +qemuDomainObjPrivateXMLParseBlockjobs(qemuDomainObjPrivatePtr priv, + xmlXPathContextPtr ctxt) +{ + char *active; + + if ((active = virXPathString("string(./blockjobs/@active)", ctxt))) { + if (virTristateBoolTypeFromString(active) == VIR_TRISTATE_BOOL_NO) + priv->reconnectNoActiveBlockjobs = true; + }
Then, just (in the format that mkletzan prefers ;-)): active = virXPathString("string(./blockjobs/@active)", ctxt); if (active) priv->NAME = virTristateBoolTypeFromString(active);
+ + VIR_FREE(active); + return 0; +} + + static int qemuDomainObjPrivateXMLParse(xmlXPathContextPtr ctxt, virDomainObjPtr vm, @@ -2282,6 +2318,9 @@ qemuDomainObjPrivateXMLParse(xmlXPathContextPtr ctxt, priv->chardevStdioLogd = virXPathBoolean("boolean(./chardevStdioLogd)", ctxt) == 1;
+ if (qemuDomainObjPrivateXMLParseBlockjobs(priv, ctxt) < 0) + goto error; + return 0;
error: diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 1a47396ab..82248b56f 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -320,6 +320,9 @@ struct _qemuDomainObjPrivate {
/* If true virtlogd is used as stdio handler for character devices. */ bool chardevStdioLogd; + + /* if true there were no active blockjobs at reconnect to the VM */ + bool reconnectNoActiveBlockjobs;
virTristateBool
};
# define QEMU_DOMAIN_PRIVATE(vm) \ diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index 4e21f5825..5339f673f 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -34,6 +34,7 @@ struct testInfo { char *outInactiveName;
virBitmapPtr activeVcpus; + bool blockjobs;
virQEMUCapsPtr qemuCaps; }; @@ -43,11 +44,22 @@ qemuXML2XMLActivePreFormatCallback(virDomainDefPtr def, const void *opaque) { struct testInfo *info = (struct testInfo *) opaque; + size_t i;
/* store vCPU bitmap so that the status XML can be created faithfully */ if (!info->activeVcpus) info->activeVcpus = virDomainDefGetOnlineVcpumap(def);
+ info->blockjobs = false; + + /* remember whether we have mirror jobs */ + for (i = 0; i < def->ndisks; i++) { + if (def->disks[i]->mirror) {
Slightly different check than qemuDomainHasBlockjob, but this is just a test.
+ info->blockjobs = true; + break; + } + } + return 0; }
@@ -124,6 +136,17 @@ testGetStatuXMLPrefixVcpus(virBufferPtr buf, }
+static void +testGetStatusXMLAddBlockjobs(virBufferPtr buf, + const struct testInfo *data) +{ + if (data->blockjobs) + virBufferAddLit(buf, "<blockjobs active='yes'/>\n"); + else + virBufferAddLit(buf, "<blockjobs active='no'/>\n");
It's just a test, but you could use the same ToString logic. What you have works, but it's confusing to see double negatives. Always have to read them much more closely. John
+} + + static char * testGetStatusXMLPrefix(const struct testInfo *data) { @@ -136,12 +159,31 @@ testGetStatusXMLPrefix(const struct testInfo *data)
virBufferAddStr(&buf, testStatusXMLPrefixBodyStatic);
+ testGetStatusXMLAddBlockjobs(&buf, data); + virBufferAdjustIndent(&buf, -2);
return virBufferContentAndReset(&buf); }
+static int +testProcessStatusXML(virDomainObjPtr vm) +{ + size_t i; + + /* fix the private 'blockjob' flag for disks */ + for (i = 0; i < vm->def->ndisks; i++) { + virDomainDiskDefPtr disk = vm->def->disks[i]; + qemuDomainDiskPrivatePtr diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk); + + diskPriv->blockjob = !!disk->mirror; + } + + return 0; +} + + static int testCompareStatusXMLToXMLFiles(const void *opaque) { @@ -200,6 +242,10 @@ testCompareStatusXMLToXMLFiles(const void *opaque) goto cleanup; }
+ /* process the definition if necessary */ + if (testProcessStatusXML(obj) < 0) + goto cleanup; + /* format it back */ if (!(actual = virDomainObjFormat(driver.xmlopt, obj, NULL, VIR_DOMAIN_DEF_FORMAT_SECURE))) {
On Wed, Oct 04, 2017 at 16:24:52 -0400, John Ferlan wrote:
On 10/04/2017 07:59 AM, Peter Krempa wrote:
Note when no blockjobs are running in the status XML so that we know that the backing chain will not change until we reconnect. --- src/qemu/qemu_domain.c | 39 +++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_domain.h | 3 +++ tests/qemuxml2xmltest.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 88 insertions(+)
Was there a special reason to use the double negative of "reconnectNoActiveBlockjobs" instead of "reconnectActiveBlockjobs" or "reconnectDetermineDiskChain".
If it's not present in the status XML, then the default is there are active block jobs or more succinctly as the next patch shows - we need to determine the backing chain when either active == true or it's not defined in the status XML (a/k/a VIR_TRISTATE_BOOL_ABSENT).
Also since the definition is much lower and making this comment now makes subsequent comments easier to understand, what not make it a virTristateBool since that's how you parse it (essentially)?
Okay, I went with this in v2
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index b202d02f9..2bc8f38dc 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c
[...]
@@ -1854,6 +1856,21 @@ qemuDomainObjPrivateXMLFormatAutomaticPlacement(virBufferPtr buf, }
+static int +qemuDomainObjPrivateXMLFormatBlockjobs(virBufferPtr buf, + virDomainObjPtr vm) +{ + virBuffer attrBuf = VIR_BUFFER_INITIALIZER; + + if (!qemuDomainHasBlockjob(vm, false)) + virBufferAddLit(&attrBuf, " active='no'"); + else + virBufferAddLit(&attrBuf, " active='yes'");
And these would use virTristateSwitchTypeToString of course. Whether you keep the same order to take "active='no'" == ABSENT is your call (that is, no special need to format 'no')
Well. Actually the only interresting value is 'no'. If the value is absent that might denote that libvirt was not recording the state, not that there was no blockjob. [...]
@@ -2067,6 +2087,22 @@ qemuDomainObjPrivateXMLParseAutomaticPlacement(xmlXPathContextPtr ctxt, }
+static int +qemuDomainObjPrivateXMLParseBlockjobs(qemuDomainObjPrivatePtr priv, + xmlXPathContextPtr ctxt) +{ + char *active; + + if ((active = virXPathString("string(./blockjobs/@active)", ctxt))) { + if (virTristateBoolTypeFromString(active) == VIR_TRISTATE_BOOL_NO) + priv->reconnectNoActiveBlockjobs = true; + }
Then, just (in the format that mkletzan prefers ;-)):
active = virXPathString("string(./blockjobs/@active)", ctxt); if (active) priv->NAME = virTristateBoolTypeFromString(active);
Since you suggested to modify the type to virTristateBool, you can't just assign it, also there can't be an inline check due to virTristateBool being unsigned but virTristateBoolTypeFromString returning -1. I went with an intermediate integer and kept the style I've used since I prefer it more.
@@ -43,11 +44,22 @@ qemuXML2XMLActivePreFormatCallback(virDomainDefPtr def, const void *opaque) { struct testInfo *info = (struct testInfo *) opaque; + size_t i;
/* store vCPU bitmap so that the status XML can be created faithfully */ if (!info->activeVcpus) info->activeVcpus = virDomainDefGetOnlineVcpumap(def);
+ info->blockjobs = false; + + /* remember whether we have mirror jobs */ + for (i = 0; i < def->ndisks; i++) { + if (def->disks[i]->mirror) {
Slightly different check than qemuDomainHasBlockjob, but this is just a test.
Obviously. We don't record diskpriv->blockjob in the XML thus that check can't be used. Later when qemu will allow to reload state of blockjobs it can be fixed to fully track blockjobs.
+ info->blockjobs = true; + break; + } + } + return 0; }
Skip purging the backing chain and redetecting it when it was not going to change during the time we were not present. The decision is based on the new flag which records whether there were blockjobs running to the status XML. --- src/qemu/qemu_process.c | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index bde3ba462..80d204599 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -6892,13 +6892,18 @@ qemuProcessReconnect(void *opaque) if (virStorageTranslateDiskSourcePool(conn, obj->def->disks[i]) < 0) goto error; - /* XXX we should be able to restore all data from XML in the future. - * This should be the only place that calls qemuDomainDetermineDiskChain - * with @report_broken == false to guarantee best-effort domain - * reconnect */ - if (qemuDomainDetermineDiskChain(driver, obj, obj->def->disks[i], - true, false) < 0) - goto error; + /* backing chains need to be refreshed only if they could change */ + if (!priv->reconnectNoActiveBlockjobs) { + /* This should be the only place that calls + * qemuDomainDetermineDiskChain with @report_broken == false + * to guarantee best-effort domain reconnect */ + if (qemuDomainDetermineDiskChain(driver, obj, obj->def->disks[i], + true, false) < 0) + goto error; + } else { + VIR_DEBUG("skipping backing chain detection for '%s'", + obj->def->disks[i]->dst); + } dev.type = VIR_DOMAIN_DEVICE_DISK; dev.data.disk = obj->def->disks[i]; -- 2.14.1
On 10/04/2017 07:59 AM, Peter Krempa wrote:
Skip purging the backing chain and redetecting it when it was not going to change during the time we were not present.
The decision is based on the new flag which records whether there were blockjobs running to the status XML. --- src/qemu/qemu_process.c | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-)
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index bde3ba462..80d204599 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -6892,13 +6892,18 @@ qemuProcessReconnect(void *opaque)
BTW: I'd support a virDomainDiskDefPtr disk = obj->def->disks[i]; right about here too ;-)
if (virStorageTranslateDiskSourcePool(conn, obj->def->disks[i]) < 0) goto error;
- /* XXX we should be able to restore all data from XML in the future. - * This should be the only place that calls qemuDomainDetermineDiskChain - * with @report_broken == false to guarantee best-effort domain - * reconnect */ - if (qemuDomainDetermineDiskChain(driver, obj, obj->def->disks[i], - true, false) < 0) - goto error; + /* backing chains need to be refreshed only if they could change */ + if (!priv->reconnectNoActiveBlockjobs) {
...or from my previous patch suggestion if (priv->NAME != VIR_TRISTATE_BOOL_NO) Reviewed-by: John Ferlan <jferlan@redhat.com> John I can give an R-B on patch 5 depending on your thoughts there. Maybe I'm missing something in future attractions...
+ /* This should be the only place that calls + * qemuDomainDetermineDiskChain with @report_broken == false + * to guarantee best-effort domain reconnect */ + if (qemuDomainDetermineDiskChain(driver, obj, obj->def->disks[i], + true, false) < 0) + goto error; + } else { + VIR_DEBUG("skipping backing chain detection for '%s'", + obj->def->disks[i]->dst); + }
dev.type = VIR_DOMAIN_DEVICE_DISK; dev.data.disk = obj->def->disks[i];
participants (2)
-
John Ferlan -
Peter Krempa