[libvirt] [PATCH] snapshot: affect persistent xml after disk snapshot
For external snapshots to be useful on persistent domains, we must alter the persistent definition alongside the running definition. Thanks to the possibility of disk hotplug as well as of edits that only affect the persistent xml, we can't assume that vm->def and vm->newDef have the same disk at the same index, so we can only update the persistent copy if the device destination matches up. * src/qemu/qemu_driver.c (qemuDomainSnapshotCreateDiskActive) (qemuDomainSnapshotCreateSingleDiskActive): Also affect newDef, if present. --- This is worth including in 0.9.5 - without it, the new feature of disk snapshots on a persistent domain are lost the moment the domain stops running, which is likely to cause data corruption for guests. I'm still trying to reproduce another reported snapshot failure: https://bugzilla.redhat.com/show_bug.cgi?id=738676 although I don't think this patch will help that issue. src/qemu/qemu_driver.c | 38 ++++++++++++++++++++++++++++++++------ 1 files changed, 32 insertions(+), 6 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 32f376ec..cbe28d8 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -9107,12 +9107,15 @@ static int qemuDomainSnapshotCreateSingleDiskActive(struct qemud_driver *driver, virDomainObjPtr vm, virDomainSnapshotDiskDefPtr snap, - virDomainDiskDefPtr disk) + virDomainDiskDefPtr disk, + virDomainDiskDefPtr persistDisk) { qemuDomainObjPrivatePtr priv = vm->privateData; char *device = NULL; char *source = NULL; char *driverType = NULL; + char *persistSource = NULL; + char *persistDriverType = NULL; int ret = -1; int fd = -1; char *origsrc = NULL; @@ -9128,7 +9131,11 @@ qemuDomainSnapshotCreateSingleDiskActive(struct qemud_driver *driver, if (virAsprintf(&device, "drive-%s", disk->info.alias) < 0 || !(source = strdup(snap->file)) || (STRNEQ_NULLABLE(disk->driverType, "qcow2") && - !(driverType = strdup("qcow2")))) { + !(driverType = strdup("qcow2"))) || + (persistDisk && + (!(persistSource = strdup(source)) || + (STRNEQ_NULLABLE(persistDisk->driverType, "qcow2") && + !(persistDriverType = strdup("qcow2")))))) { virReportOOMError(); goto cleanup; } @@ -9176,9 +9183,16 @@ qemuDomainSnapshotCreateSingleDiskActive(struct qemud_driver *driver, disk->driverType = driverType; driverType = NULL; } - - /* XXX Do we also need to update vm->newDef if there are pending - * configuration changes awaiting the next boot? */ + if (persistDisk) { + VIR_FREE(persistDisk->src); + persistDisk->src = persistSource; + persistSource = NULL; + if (persistDriverType) { + VIR_FREE(persistDisk->driverType); + persistDisk->driverType = persistDriverType; + persistDriverType = NULL; + } + } cleanup: if (origsrc) { @@ -9190,6 +9204,8 @@ cleanup: VIR_FREE(device); VIR_FREE(source); VIR_FREE(driverType); + VIR_FREE(persistSource); + VIR_FREE(persistDriverType); return ret; } @@ -9235,12 +9251,22 @@ qemuDomainSnapshotCreateDiskActive(virConnectPtr conn, * SNAPSHOT_EXTERNAL with a valid file name and qcow2 format. */ qemuDomainObjEnterMonitorWithDriver(driver, vm); for (i = 0; i < snap->def->ndisks; i++) { + virDomainDiskDefPtr persistDisk = NULL; + if (snap->def->disks[i].snapshot == VIR_DOMAIN_DISK_SNAPSHOT_NO) continue; + if (vm->newDef) { + int indx = virDomainDiskIndexByName(vm->newDef, + vm->def->disks[i]->dst, + false); + if (indx >= 0) + persistDisk = vm->newDef->disks[indx]; + } ret = qemuDomainSnapshotCreateSingleDiskActive(driver, vm, &snap->def->disks[i], - vm->def->disks[i]); + vm->def->disks[i], + persistDisk); if (ret < 0) break; } -- 1.7.4.4
On 09/16/2011 10:11 PM, Eric Blake wrote:
For external snapshots to be useful on persistent domains, we must alter the persistent definition alongside the running definition. Thanks to the possibility of disk hotplug as well as of edits that only affect the persistent xml, we can't assume that vm->def and vm->newDef have the same disk at the same index, so we can only update the persistent copy if the device destination matches up.
* src/qemu/qemu_driver.c (qemuDomainSnapshotCreateDiskActive) (qemuDomainSnapshotCreateSingleDiskActive): Also affect newDef, if present. ---
This is worth including in 0.9.5 - without it, the new feature of disk snapshots on a persistent domain are lost the moment the domain stops running, which is likely to cause data corruption for guests.
I'm squashing this in, so that the persistent changes are preserved over libvirtd restarts. diff --git i/src/qemu/qemu_driver.c w/src/qemu/qemu_driver.c index cbe28d8..2a1e5ea 100644 --- i/src/qemu/qemu_driver.c +++ w/src/qemu/qemu_driver.c @@ -9221,6 +9221,7 @@ qemuDomainSnapshotCreateDiskActive(virConnectPtr conn, bool resume = false; int ret = -1; int i; + bool persist = false; if (qemuDomainObjBeginJobWithDriver(driver, vm, QEMU_JOB_MODIFY) < 0) return -1; @@ -9259,8 +9260,10 @@ qemuDomainSnapshotCreateDiskActive(virConnectPtr conn, int indx = virDomainDiskIndexByName(vm->newDef, vm->def->disks[i]->dst, false); - if (indx >= 0) + if (indx >= 0) { persistDisk = vm->newDef->disks[indx]; + persist = true; + } } ret = qemuDomainSnapshotCreateSingleDiskActive(driver, vm, @@ -9301,7 +9304,9 @@ cleanup: } if (vm) { - if (virDomainSaveStatus(driver->caps, driver->stateDir, vm) < 0) + if (virDomainSaveStatus(driver->caps, driver->stateDir, vm) < 0 || + (persist && + virDomainSaveConfig(driver->configDir, vm->newDef) < 0)) ret = -1; if (qemuDomainObjEndJob(driver, vm) == 0) { /* Only possible if a transient vm quit while our locks were down, -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On Sat, Sep 17, 2011 at 05:56:22AM -0600, Eric Blake wrote:
On 09/16/2011 10:11 PM, Eric Blake wrote:
For external snapshots to be useful on persistent domains, we must alter the persistent definition alongside the running definition. Thanks to the possibility of disk hotplug as well as of edits that only affect the persistent xml, we can't assume that vm->def and vm->newDef have the same disk at the same index, so we can only update the persistent copy if the device destination matches up.
* src/qemu/qemu_driver.c (qemuDomainSnapshotCreateDiskActive) (qemuDomainSnapshotCreateSingleDiskActive): Also affect newDef, if present. ---
This is worth including in 0.9.5 - without it, the new feature of disk snapshots on a persistent domain are lost the moment the domain stops running, which is likely to cause data corruption for guests.
I'm squashing this in, so that the persistent changes are preserved over libvirtd restarts.
diff --git i/src/qemu/qemu_driver.c w/src/qemu/qemu_driver.c index cbe28d8..2a1e5ea 100644 --- i/src/qemu/qemu_driver.c +++ w/src/qemu/qemu_driver.c @@ -9221,6 +9221,7 @@ qemuDomainSnapshotCreateDiskActive(virConnectPtr conn, bool resume = false; int ret = -1; int i; + bool persist = false;
if (qemuDomainObjBeginJobWithDriver(driver, vm, QEMU_JOB_MODIFY) < 0) return -1; @@ -9259,8 +9260,10 @@ qemuDomainSnapshotCreateDiskActive(virConnectPtr conn, int indx = virDomainDiskIndexByName(vm->newDef, vm->def->disks[i]->dst, false); - if (indx >= 0) + if (indx >= 0) { persistDisk = vm->newDef->disks[indx]; + persist = true; + } }
ret = qemuDomainSnapshotCreateSingleDiskActive(driver, vm, @@ -9301,7 +9304,9 @@ cleanup: }
if (vm) { - if (virDomainSaveStatus(driver->caps, driver->stateDir, vm) < 0) + if (virDomainSaveStatus(driver->caps, driver->stateDir, vm) < 0 || + (persist && + virDomainSaveConfig(driver->configDir, vm->newDef) < 0)) ret = -1; if (qemuDomainObjEndJob(driver, vm) == 0) { /* Only possible if a transient vm quit while our locks were down,
ACK, though I don't fully undestand the index issue, isn't there other ways to find matching devices ? Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
On 09/17/2011 08:48 AM, Daniel Veillard wrote:
On Sat, Sep 17, 2011 at 05:56:22AM -0600, Eric Blake wrote:
On 09/16/2011 10:11 PM, Eric Blake wrote:
For external snapshots to be useful on persistent domains, we must alter the persistent definition alongside the running definition. Thanks to the possibility of disk hotplug as well as of edits that only affect the persistent xml, we can't assume that vm->def and vm->newDef have the same disk at the same index, so we can only update the persistent copy if the device destination matches up.
* src/qemu/qemu_driver.c (qemuDomainSnapshotCreateDiskActive) (qemuDomainSnapshotCreateSingleDiskActive): Also affect newDef, if present. ---
ACK,
Pushed.
though I don't fully undestand the index issue, isn't there other ways to find matching devices ?
if (snap->def->disks[i].snapshot == VIR_DOMAIN_DISK_SNAPSHOT_NO) continue; + if (vm->newDef) { + int indx = virDomainDiskIndexByName(vm->newDef, + vm->def->disks[i]->dst, + false); + if (indx >= 0) + persistDisk = vm->newDef->disks[indx]; + }
The bit about finding the index is for situations like: vm->def->disks[0] is vda, disks[1] is vdb but we have pending changes to the persistent config, so that: vm->newDef->disks[0] is vdb, disks[1] is vdc We are guaranteed that disks[n]->dst is a valid string, so the index lookup says find the disk (if any) in vm->newDef with the same dst as what we are about to modify in vm->def; if both definitions have a disk by the same dst, then we edit both. In the above example, that would be def->disks[0] gives indx -1, so no edit to vm->newDef; while def->disks[1] gives indx 0, and 'vdb' is edited in both definitions. If we had just blindly updated disks[1] in both def and newDef, we would have been updating vdc instead of vdb in newDef. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
participants (2)
-
Daniel Veillard -
Eric Blake