[libvirt] [PATCH] Recheck disk backing chains after snapshot
When a snapshot operation finishes we have to recheck the backing chain of all disks involved in the snapshot. And we need to do that even if the operation failed because some of the disks might have changed if QEMU did not support transactions. Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- Notes: - BlockRebase and BlockCommit already recheck the backing chain when we get an event about a completed block job (in qemuProcessHandleBlockJob) src/qemu/qemu_driver.c | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 67ba487..492fcc1 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -12942,6 +12942,7 @@ qemuDomainSnapshotCreateDiskActive(virQEMUDriverPtr driver, bool persist = false; bool reuse = (flags & VIR_DOMAIN_SNAPSHOT_CREATE_REUSE_EXT) != 0; virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); + virErrorPtr orig_err = NULL; if (!virDomainObjIsActive(vm)) { virReportError(VIR_ERR_OPERATION_INVALID, @@ -13033,6 +13034,17 @@ qemuDomainSnapshotCreateDiskActive(virQEMUDriverPtr driver, } cleanup: + /* recheck backing chains of all disks involved in the snapshot */ + orig_err = virSaveLastError(); + for (i = 0; i < snap->def->ndisks; i++) { + if (snap->def->disks[i].snapshot == VIR_DOMAIN_SNAPSHOT_LOCATION_NONE) + continue; + qemuDomainDetermineDiskChain(driver, vm, vm->def->disks[i], true); + } + if (orig_err) { + virSetError(orig_err); + virFreeError(orig_err); + } if (ret == 0 || !virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_TRANSACTION)) { if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm) < 0 || -- 1.9.2
On 04/25/2014 07:13 AM, Jiri Denemark wrote:
When a snapshot operation finishes we have to recheck the backing chain of all disks involved in the snapshot. And we need to do that even if the operation failed because some of the disks might have changed if QEMU did not support transactions.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com> ---
Notes: - BlockRebase and BlockCommit already recheck the backing chain when we get an event about a completed block job (in qemuProcessHandleBlockJob)
src/qemu/qemu_driver.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)
cleanup: + /* recheck backing chains of all disks involved in the snapshot */ + orig_err = virSaveLastError(); + for (i = 0; i < snap->def->ndisks; i++) { + if (snap->def->disks[i].snapshot == VIR_DOMAIN_SNAPSHOT_LOCATION_NONE) + continue;
Do we really need to reprobe internal snapshots? That is, could this be: if (snap->def->disks[i].snapshot != VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL) But this makes sense as an improvement to the current code, so: ACK. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On Fri, Apr 25, 2014 at 10:45:36 -0600, Eric Blake wrote:
On 04/25/2014 07:13 AM, Jiri Denemark wrote:
When a snapshot operation finishes we have to recheck the backing chain of all disks involved in the snapshot. And we need to do that even if the operation failed because some of the disks might have changed if QEMU did not support transactions.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com> ---
Notes: - BlockRebase and BlockCommit already recheck the backing chain when we get an event about a completed block job (in qemuProcessHandleBlockJob)
src/qemu/qemu_driver.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)
cleanup: + /* recheck backing chains of all disks involved in the snapshot */ + orig_err = virSaveLastError(); + for (i = 0; i < snap->def->ndisks; i++) { + if (snap->def->disks[i].snapshot == VIR_DOMAIN_SNAPSHOT_LOCATION_NONE) + continue;
Do we really need to reprobe internal snapshots? That is, could this be: if (snap->def->disks[i].snapshot != VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL)
Hmm, right you are. I changed it and pushed the patch. Thanks. Jirka
On 04/25/2014 09:13 AM, Jiri Denemark wrote:
When a snapshot operation finishes we have to recheck the backing chain of all disks involved in the snapshot. And we need to do that even if the operation failed because some of the disks might have changed if QEMU did not support transactions.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com> ---
Notes: - BlockRebase and BlockCommit already recheck the backing chain when we get an event about a completed block job (in qemuProcessHandleBlockJob)
src/qemu/qemu_driver.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)
FYI: This change fixes an issue that virt-test found over my weekend run in the 'blockpull' test which was caused by 4/4 of the addressing backing stores by index: http://www.redhat.com/archives/libvir-list/2014-April/msg00823.html Essentially the test sets things up via: /usr/bin/virsh snapshot-create-as virt-tests-vm1 snapshot1 snap1-desc --disk-only --atomic --no-metadata vda,snapshot=external,file=/home/virt-test/tmp/jeos-20-64.snap1 /usr/bin/virsh snapshot-create-as virt-tests-vm1 snapshot2 snap2-desc --disk-only --atomic --no-metadata vda,snapshot=external,file=/home/virt-test/tmp/jeos-20-64.snap2 /usr/bin/virsh snapshot-create-as virt-tests-vm1 snapshot3 snap3-desc --disk-only --atomic --no-metadata vda,snapshot=external,file=/home/virt-test/tmp/jeos-20-64.snap3 Then does (in different passes): /usr/bin/virsh blockpull virt-tests-vm1 vda --wait --verbose /usr/bin/virsh blockpull virt-tests-vm1 vda --wait --verbose --base /home/virt-test/tmp/jeos-20-64.snap2 /usr/bin/virsh blockpull virt-tests-vm1 vda --wait --verbose --base /home/virt-test/tmp/jeos-20-64.snap1 /usr/bin/virsh blockpull virt-tests-vm1 vda --wait --verbose --timeout 1 /usr/bin/virsh blockpull virt-tests-vm1 vda --wait --verbose --timeout 1 --base /home/virt-test/tmp/jeos-20-64.snap2 /usr/bin/virsh blockpull virt-tests-vm1 vda --wait --verbose --timeout 1 --base /home/virt-test/tmp/jeos-20-64.snap1 Without this patch applied, the blockpull's w/ --base supplied fail with either: could not find image '/home/virt-test/tmp/jeos-20-64.snap2' in chain for '/home/virt-test/tmp/jeos-20-64.snap3' or could not find image '/home/virt-test/tmp/jeos-20-64.snap1' in chain for '/home/virt-test/tmp/jeos-20-64.snap3' John FWIW: The weekend run I had w/ virt-test against recent upstream changes was quite successful. There's still an issue or two to resolve regarding how capacity is checked, but the recent storage changes haven't caused regressions.
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 67ba487..492fcc1 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -12942,6 +12942,7 @@ qemuDomainSnapshotCreateDiskActive(virQEMUDriverPtr driver, bool persist = false; bool reuse = (flags & VIR_DOMAIN_SNAPSHOT_CREATE_REUSE_EXT) != 0; virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); + virErrorPtr orig_err = NULL;
if (!virDomainObjIsActive(vm)) { virReportError(VIR_ERR_OPERATION_INVALID, @@ -13033,6 +13034,17 @@ qemuDomainSnapshotCreateDiskActive(virQEMUDriverPtr driver, }
cleanup: + /* recheck backing chains of all disks involved in the snapshot */ + orig_err = virSaveLastError(); + for (i = 0; i < snap->def->ndisks; i++) { + if (snap->def->disks[i].snapshot == VIR_DOMAIN_SNAPSHOT_LOCATION_NONE) + continue; + qemuDomainDetermineDiskChain(driver, vm, vm->def->disks[i], true); + } + if (orig_err) { + virSetError(orig_err); + virFreeError(orig_err); + }
if (ret == 0 || !virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_TRANSACTION)) { if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm) < 0 ||
participants (3)
-
Eric Blake -
Jiri Denemark -
John Ferlan