File libvirt-qemu-snapshot-Fix-job-handling-when-creating-snapshots.patch of Package libvirt
From 3b9dd9e5d1fd1803a556c1054ee5e3cb30ff86e7 Mon Sep 17 00:00:00 2001
Message-Id: <3b9dd9e5d1fd1803a556c1054ee5e3cb30ff86e7@dist-git>
From: Peter Krempa <pkrempa@redhat.com>
Date: Thu, 22 Sep 2016 18:16:01 +0200
Subject: [PATCH] qemu: snapshot: Fix job handling when creating snapshots
https://bugzilla.redhat.com/show_bug.cgi?id=1326652 [6.9]
https://bugzilla.redhat.com/show_bug.cgi?id=1290647 [6.8]
Creating snapshots modifies the domain state. Currently we wouldn't
enter the job for certain operations although they would modify the
state. Refactor job handling so that everything is covered by an async
job.
(cherry picked from commit b3d2a42e80aaee1f322fc9beb98b6ed541574ab3)
Conflicts: 6.9 diverged a lot, mostly as upstream dropped the big
qemu driver lock. This patch was manually reimplemented
and tweaked for the differences.
---
src/qemu/qemu_driver.c | 181 ++++++++++++++++++++++---------------------------
1 file changed, 81 insertions(+), 100 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 07c6e74..93e5dfe 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -11675,32 +11675,22 @@ cleanup:
static int
qemuDomainSnapshotCreateActiveInternal(virConnectPtr conn,
struct qemud_driver *driver,
- virDomainObjPtr *vmptr,
+ virDomainObjPtr vm,
virDomainSnapshotObjPtr snap,
unsigned int flags)
{
- virDomainObjPtr vm = *vmptr;
qemuDomainObjPrivatePtr priv = vm->privateData;
virDomainEventPtr event = NULL;
bool resume = false;
int ret = -1;
- if (qemuDomainObjBeginJobWithDriver(driver, vm, QEMU_JOB_MODIFY) < 0)
- return -1;
-
- if (!virDomainObjIsActive(vm)) {
- virReportError(VIR_ERR_OPERATION_INVALID,
- "%s", _("domain is not running"));
- goto endjob;
- }
-
if (virDomainObjGetState(vm, NULL) == VIR_DOMAIN_RUNNING) {
/* savevm monitor command pauses the domain emitting an event which
* confuses libvirt since it's not notified when qemu resumes the
* domain. Thus we stop and start CPUs ourselves.
*/
if (qemuProcessStopCPUs(driver, vm, VIR_DOMAIN_PAUSED_SAVE,
- QEMU_ASYNC_JOB_NONE) < 0)
+ QEMU_ASYNC_JOB_SNAPSHOT) < 0)
goto cleanup;
resume = true;
@@ -11711,7 +11701,12 @@ qemuDomainSnapshotCreateActiveInternal(virConnectPtr conn,
}
}
- qemuDomainObjEnterMonitorWithDriver(driver, vm);
+ if (qemuDomainObjEnterMonitorAsync(driver, vm,
+ QEMU_ASYNC_JOB_SNAPSHOT) < 0) {
+ resume = false;
+ goto cleanup;
+ }
+
ret = qemuMonitorCreateSnapshot(priv->mon, snap->def->name);
qemuDomainObjExitMonitorWithDriver(driver, vm);
if (ret < 0)
@@ -11722,18 +11717,14 @@ qemuDomainSnapshotCreateActiveInternal(virConnectPtr conn,
VIR_DOMAIN_EVENT_STOPPED_FROM_SNAPSHOT);
qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FROM_SNAPSHOT, 0);
virDomainAuditStop(vm, "from-snapshot");
- /* We already filtered the _HALT flag for persistent domains
- * only, so this end job never drops the last reference. */
- ignore_value(qemuDomainObjEndJob(driver, vm));
resume = false;
- vm = NULL;
}
cleanup:
if (resume && virDomainObjIsActive(vm) &&
qemuProcessStartCPUs(driver, vm, conn,
VIR_DOMAIN_RUNNING_UNPAUSED,
- QEMU_ASYNC_JOB_NONE) < 0) {
+ QEMU_ASYNC_JOB_SNAPSHOT) < 0) {
event = virDomainEventNewFromObj(vm,
VIR_DOMAIN_EVENT_SUSPENDED,
VIR_DOMAIN_EVENT_SUSPENDED_API_ERROR);
@@ -11743,14 +11734,6 @@ cleanup:
}
}
-endjob:
- if (vm && qemuDomainObjEndJob(driver, vm) == 0) {
- /* Only possible if a transient vm quit while our locks were down,
- * in which case we don't want to save snapshot metadata. */
- *vmptr = NULL;
- ret = -1;
- }
-
if (event)
qemuDomainEventQueue(driver, event);
@@ -12172,13 +12155,12 @@ cleanup:
static int
qemuDomainSnapshotCreateActiveExternal(virConnectPtr conn,
struct qemud_driver *driver,
- virDomainObjPtr *vmptr,
+ virDomainObjPtr vm,
virDomainSnapshotObjPtr snap,
unsigned int flags)
{
bool resume = false;
int ret = -1;
- virDomainObjPtr vm = *vmptr;
qemuDomainObjPrivatePtr priv = vm->privateData;
char *xml = NULL;
bool memory = snap->def->memory == VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL;
@@ -12187,10 +12169,6 @@ qemuDomainSnapshotCreateActiveExternal(virConnectPtr conn,
bool transaction = qemuCapsGet(priv->caps, QEMU_CAPS_TRANSACTION);
int thaw = 0; /* 1 if freeze succeeded, -1 if freeze failed */
- if (qemuDomainObjBeginAsyncJobWithDriver(driver, vm,
- QEMU_ASYNC_JOB_SNAPSHOT) < 0)
- goto cleanup;
-
/* If quiesce was requested, then issue a freeze command, and a
* counterpart thaw command, no matter what. The command will
* fail if the guest is paused or the guest agent is not
@@ -12199,7 +12177,7 @@ qemuDomainSnapshotCreateActiveExternal(virConnectPtr conn,
if (qemuDomainSnapshotFSFreeze(driver, vm) < 0) {
/* helper reported the error */
thaw = -1;
- goto endjob;
+ goto cleanup;
} else {
thaw = 1;
}
@@ -12224,12 +12202,12 @@ qemuDomainSnapshotCreateActiveExternal(virConnectPtr conn,
(!memory && atomic && !transaction)) {
if (qemuProcessStopCPUs(driver, vm, VIR_DOMAIN_PAUSED_SNAPSHOT,
QEMU_ASYNC_JOB_SNAPSHOT) < 0)
- goto endjob;
+ goto cleanup;
if (!virDomainObjIsActive(vm)) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("guest unexpectedly quit"));
- goto endjob;
+ goto cleanup;
}
}
}
@@ -12238,7 +12216,7 @@ qemuDomainSnapshotCreateActiveExternal(virConnectPtr conn,
if (memory) {
/* check if migration is possible */
if (!qemuMigrationIsAllowed(driver, vm, vm->def, false, false))
- goto endjob;
+ goto cleanup;
/* allow the migration job to be cancelled or the domain to be paused */
qemuDomainObjSetAsyncJobMask(vm, DEFAULT_JOB_MASK |
@@ -12246,13 +12224,13 @@ qemuDomainSnapshotCreateActiveExternal(virConnectPtr conn,
JOB_MASK(QEMU_JOB_MIGRATION_OP));
if (!(xml = qemuDomainDefFormatLive(driver, vm->def, true, true)))
- goto endjob;
+ goto cleanup;
if ((ret = qemuDomainSaveMemory(driver, vm, snap->def->file,
xml, QEMUD_SAVE_FORMAT_RAW,
resume, 0,
QEMU_ASYNC_JOB_SNAPSHOT)) < 0)
- goto endjob;
+ goto cleanup;
/* the memory image was created, remove it on errors */
memory_unlink = true;
@@ -12270,7 +12248,7 @@ qemuDomainSnapshotCreateActiveExternal(virConnectPtr conn,
*/
if ((ret = qemuDomainSnapshotCreateDiskActive(driver, vm, snap, flags,
QEMU_ASYNC_JOB_SNAPSHOT)) < 0)
- goto endjob;
+ goto cleanup;
/* the snapshot is complete now */
if (flags & VIR_DOMAIN_SNAPSHOT_CREATE_HALT) {
@@ -12280,20 +12258,16 @@ qemuDomainSnapshotCreateActiveExternal(virConnectPtr conn,
VIR_DOMAIN_EVENT_STOPPED_FROM_SNAPSHOT);
qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FROM_SNAPSHOT, 0);
virDomainAuditStop(vm, "from-snapshot");
- /* We already filtered the _HALT flag for persistent domains
- * only, so this end job never drops the last reference. */
- ignore_value(qemuDomainObjEndAsyncJob(driver, vm));
resume = false;
thaw = 0;
- vm = NULL;
if (event)
qemuDomainEventQueue(driver, event);
}
ret = 0;
-endjob:
- if (resume && vm && virDomainObjIsActive(vm) &&
+cleanup:
+ if (resume && virDomainObjIsActive(vm) &&
qemuProcessStartCPUs(driver, vm, conn,
VIR_DOMAIN_RUNNING_UNPAUSED,
QEMU_ASYNC_JOB_SNAPSHOT) < 0) {
@@ -12310,21 +12284,14 @@ endjob:
ret = -1;
}
- if (vm && thaw != 0 &&
+
+ if (thaw != 0 &&
qemuDomainSnapshotFSThaw(driver, vm, thaw > 0) < 0) {
/* helper reported the error, if it was needed */
if (thaw > 0)
ret = -1;
}
- if (vm && !qemuDomainObjEndAsyncJob(driver, vm)) {
- /* Only possible if a transient vm quit while our locks were down,
- * in which case we don't want to save snapshot metadata.
- */
- *vmptr = NULL;
- ret = -1;
- }
-cleanup:
VIR_FREE(xml);
if (memory_unlink && ret < 0)
unlink(snap->def->file);
@@ -12438,6 +12405,16 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain,
goto cleanup;
}
+ /* We are going to modify the domain below. Internal snapshots would use
+ * a regular job, so we need to set the job mask to disallow query as
+ * 'savevm' blocks the monitor. External snapshot will then modify the
+ * job mask appropriately. */
+ if (qemuDomainObjBeginAsyncJobWithDriver(driver, vm,
+ QEMU_ASYNC_JOB_SNAPSHOT) < 0)
+ goto cleanup;
+
+ qemuDomainObjSetAsyncJobMask(vm, QEMU_JOB_NONE);
+
if (flags & VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE) {
/* Prevent circular chains */
if (def->parent) {
@@ -12445,21 +12422,21 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain,
virReportError(VIR_ERR_INVALID_ARG,
_("cannot set snapshot %s as its own parent"),
def->name);
- goto cleanup;
+ goto endjob;
}
other = virDomainSnapshotFindByName(vm->snapshots, def->parent);
if (!other) {
virReportError(VIR_ERR_INVALID_ARG,
_("parent %s for snapshot %s not found"),
def->parent, def->name);
- goto cleanup;
+ goto endjob;
}
while (other->def->parent) {
if (STREQ(other->def->parent, def->name)) {
virReportError(VIR_ERR_INVALID_ARG,
_("parent %s would create cycle to %s"),
other->def->name, def->name);
- goto cleanup;
+ goto endjob;
}
other = virDomainSnapshotFindByName(vm->snapshots,
other->def->parent);
@@ -12478,7 +12455,7 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain,
_("disk-only flag for snapshot %s requires "
"disk-snapshot state"),
def->name);
- goto cleanup;
+ goto endjob;
}
@@ -12487,7 +12464,7 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain,
virReportError(VIR_ERR_INVALID_ARG,
_("definition for snapshot %s must use uuid %s"),
def->name, uuidstr);
- goto cleanup;
+ goto endjob;
}
other = virDomainSnapshotFindByName(vm->snapshots, def->name);
@@ -12500,7 +12477,7 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain,
_("cannot change between online and offline "
"snapshot state in snapshot %s"),
def->name);
- goto cleanup;
+ goto endjob;
}
if ((other->def->state == VIR_DOMAIN_DISK_SNAPSHOT) !=
@@ -12509,14 +12486,14 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain,
_("cannot change between disk snapshot and "
"system checkpoint in snapshot %s"),
def->name);
- goto cleanup;
+ goto endjob;
}
if (other->def->dom) {
if (def->dom) {
if (!virDomainDefCheckABIStability(other->def->dom,
def->dom))
- goto cleanup;
+ goto endjob;
} else {
/* Transfer the domain def */
def->dom = other->def->dom;
@@ -12538,7 +12515,7 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain,
other->def->dom = def->dom;
def->dom = NULL;
}
- goto cleanup;
+ goto endjob;
}
}
@@ -12563,7 +12540,7 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain,
}
if (virDomainSnapshotAlignDisks(def, align_location,
align_match) < 0)
- goto cleanup;
+ goto endjob;
}
}
} else {
@@ -12573,7 +12550,7 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain,
!(def->dom = virDomainDefParseString(driver->caps, xml,
QEMU_EXPECTED_VIRT_TYPES,
VIR_DOMAIN_XML_INACTIVE)))
- goto cleanup;
+ goto endjob;
if (flags & VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY) {
align_location = VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL;
@@ -12596,12 +12573,12 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain,
if (virDomainSnapshotAlignDisks(def, align_location,
align_match) < 0 ||
qemuDomainSnapshotPrepare(vm, def, &flags) < 0)
- goto cleanup;
+ goto endjob;
}
if (!snap) {
if (!(snap = virDomainSnapshotAssignDef(vm->snapshots, def)))
- goto cleanup;
+ goto endjob;
def = NULL;
}
@@ -12613,14 +12590,14 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain,
snap->def->parent = strdup(vm->current_snapshot->def->name);
if (snap->def->parent == NULL) {
virReportOOMError();
- goto cleanup;
+ goto endjob;
}
}
if (update_current) {
vm->current_snapshot->def->current = false;
if (qemuDomainSnapshotWriteMetadata(vm, vm->current_snapshot,
driver->snapshotDir) < 0)
- goto cleanup;
+ goto endjob;
vm->current_snapshot = NULL;
}
}
@@ -12635,13 +12612,13 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain,
snap->def->memory == VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL) {
/* external checkpoint or disk snapshot */
if (qemuDomainSnapshotCreateActiveExternal(domain->conn, driver,
- &vm, snap, flags) < 0)
- goto cleanup;
+ vm, snap, flags) < 0)
+ goto endjob;
} else {
/* internal checkpoint */
if (qemuDomainSnapshotCreateActiveInternal(domain->conn, driver,
- &vm, snap, flags) < 0)
- goto cleanup;
+ vm, snap, flags) < 0)
+ goto endjob;
}
} else {
/* inactive; qemuDomainSnapshotPrepare guaranteed that we
@@ -12652,10 +12629,10 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain,
if (qemuDomainSnapshotCreateInactiveExternal(driver, vm, snap,
reuse) < 0)
- goto cleanup;
+ goto endjob;
} else {
if (qemuDomainSnapshotCreateInactiveInternal(driver, vm, snap) < 0)
- goto cleanup;
+ goto endjob;
}
}
@@ -12665,34 +12642,38 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain,
*/
snapshot = virGetDomainSnapshot(domain, snap->def->name);
-cleanup:
- if (vm) {
- if (snapshot && !(flags & VIR_DOMAIN_SNAPSHOT_CREATE_NO_METADATA)) {
- if (qemuDomainSnapshotWriteMetadata(vm, snap,
- driver->snapshotDir) < 0) {
- /* if writing of metadata fails, error out rather than trying
- * to silently carry on without completing the snapshot */
- virDomainSnapshotFree(snapshot);
- snapshot = NULL;
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("unable to save metadata for snapshot %s"),
- snap->def->name);
- virDomainSnapshotObjListRemove(vm->snapshots, snap);
- } else {
- if (update_current)
- vm->current_snapshot = snap;
- other = virDomainSnapshotFindByName(vm->snapshots,
- snap->def->parent);
- snap->parent = other;
- other->nchildren++;
- snap->sibling = other->first_child;
- other->first_child = snap;
- }
- } else if (snap) {
+endjob:
+ if (snapshot && !(flags & VIR_DOMAIN_SNAPSHOT_CREATE_NO_METADATA)) {
+ if (qemuDomainSnapshotWriteMetadata(vm, snap,
+ driver->snapshotDir) < 0) {
+ /* if writing of metadata fails, error out rather than trying
+ * to silently carry on without completing the snapshot */
+ virDomainSnapshotFree(snapshot);
+ snapshot = NULL;
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("unable to save metadata for snapshot %s"),
+ snap->def->name);
virDomainSnapshotObjListRemove(vm->snapshots, snap);
+ } else {
+ if (update_current)
+ vm->current_snapshot = snap;
+ other = virDomainSnapshotFindByName(vm->snapshots,
+ snap->def->parent);
+ snap->parent = other;
+ other->nchildren++;
+ snap->sibling = other->first_child;
+ other->first_child = snap;
}
- virDomainObjUnlock(vm);
+ } else if (snap) {
+ virDomainSnapshotObjListRemove(vm->snapshots, snap);
}
+
+ if (!qemuDomainObjEndAsyncJob(driver, vm))
+ vm = NULL;
+
+cleanup:
+ if (vm)
+ virDomainObjUnlock(vm);
virDomainSnapshotDefFree(def);
VIR_FREE(xml);
qemuDriverUnlock(driver);
--
2.10.1
