[libvirt] [RFC PATCH 00/16] qemu: checkpoint: Add support for deleting checkpoints accross snapshots
Posted as RFC because qemu probably doesn't reopen backing images for bitmap operations resulting in: $ virsh checkpoint-delete vm --checkpointname a error: Failed to delete checkpoint a error: internal error: unable to execute QEMU command 'transaction': Bitmap 'a' is readonly and cannot be modified Unfortunately this can't be done manually because 'blockdev-reopen' is still experimental. Peter Krempa (16): qemu: checkpoint: Store whether deleted checkpoint is current in a variable qemu: checkpoint: split out checkpoint deletion bitmaps qemu: checkpoint: rename disk->chkdisk in qemuCheckpointDiscardBitmaps qemu: checkpoint: rename disk->chkdisk in qemuCheckpointAddActions qemu: checkpoint: Use disk definition directly when creating checkpoint qemu: checkpoint: tolerate missing disks on checkpoint deletion qemu: domain: Remove unused qemuDomainDiskNodeFormatLookup qemu: checkpoint: Introduce helper to find checkpoint disk definition in parents qemu: checkpoint: Extract calculation of bitmap merging for checkpoint deletion tests: qemublock: Add test for checkpoint deletion bitmap merge tests: qemublock: Add few more test cases for checkpoint deletion tests: qemublock: Add synthetic snapshot+checkpoint test data qemu: checkpoint: Introduce support for deleting checkpoints accross snapshots tests: qemublock: Add checkpoint deletion test for deep backing chain tests: qemublock: Add checkpoint deletion tests for some special cases qemu: checkpoint: Track and relabel images for bitmap merging src/qemu/qemu_checkpoint.c | 329 +++++-- src/qemu/qemu_checkpoint.h | 9 + src/qemu/qemu_domain.c | 14 - src/qemu/qemu_domain.h | 3 - tests/qemublocktest.c | 102 +++ .../snapshots-synthetic-checkpoint.json | 827 ++++++++++++++++++ .../bitmap/snapshots-synthetic-checkpoint.out | 13 + .../checkpointdelete/basic-current-out.json | 29 + .../basic-intermediate1-out.json | 22 + .../basic-intermediate2-out.json | 22 + .../basic-intermediate3-out.json | 22 + .../checkpointdelete/basic-noparent-out.json | 9 + .../snapshots-current-out.json | 29 + .../snapshots-intermediate1-out.json | 24 + .../snapshots-intermediate2-out.json | 62 ++ .../snapshots-intermediate3-out.json | 61 ++ .../snapshots-noparent-out.json | 27 + ...hots-synthetic-checkpoint-current-out.json | 29 + ...ynthetic-checkpoint-intermediate1-out.json | 31 + ...ynthetic-checkpoint-intermediate2-out.json | 34 + ...ynthetic-checkpoint-intermediate3-out.json | 61 ++ ...ots-synthetic-checkpoint-noparent-out.json | 27 + 22 files changed, 1680 insertions(+), 106 deletions(-) create mode 100644 tests/qemublocktestdata/bitmap/snapshots-synthetic-checkpoint.json create mode 100644 tests/qemublocktestdata/bitmap/snapshots-synthetic-checkpoint.out create mode 100644 tests/qemublocktestdata/checkpointdelete/basic-current-out.json create mode 100644 tests/qemublocktestdata/checkpointdelete/basic-intermediate1-out.json create mode 100644 tests/qemublocktestdata/checkpointdelete/basic-intermediate2-out.json create mode 100644 tests/qemublocktestdata/checkpointdelete/basic-intermediate3-out.json create mode 100644 tests/qemublocktestdata/checkpointdelete/basic-noparent-out.json create mode 100644 tests/qemublocktestdata/checkpointdelete/snapshots-current-out.json create mode 100644 tests/qemublocktestdata/checkpointdelete/snapshots-intermediate1-out.json create mode 100644 tests/qemublocktestdata/checkpointdelete/snapshots-intermediate2-out.json create mode 100644 tests/qemublocktestdata/checkpointdelete/snapshots-intermediate3-out.json create mode 100644 tests/qemublocktestdata/checkpointdelete/snapshots-noparent-out.json create mode 100644 tests/qemublocktestdata/checkpointdelete/snapshots-synthetic-checkpoint-current-out.json create mode 100644 tests/qemublocktestdata/checkpointdelete/snapshots-synthetic-checkpoint-intermediate1-out.json create mode 100644 tests/qemublocktestdata/checkpointdelete/snapshots-synthetic-checkpoint-intermediate2-out.json create mode 100644 tests/qemublocktestdata/checkpointdelete/snapshots-synthetic-checkpoint-intermediate3-out.json create mode 100644 tests/qemublocktestdata/checkpointdelete/snapshots-synthetic-checkpoint-noparent-out.json -- 2.24.1
Avoid two computations by using a boolean. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_checkpoint.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_checkpoint.c b/src/qemu/qemu_checkpoint.c index 2fa5c1ae00..d13d4c2a37 100644 --- a/src/qemu/qemu_checkpoint.c +++ b/src/qemu/qemu_checkpoint.c @@ -117,6 +117,7 @@ qemuCheckpointDiscard(virQEMUDriverPtr driver, size_t i, j; g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); g_autofree char *chkFile = NULL; + bool chkcurrent = chk == virDomainCheckpointGetCurrent(vm->checkpoints); if (!metadata_only && !virDomainObjIsActive(vm)) { virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", @@ -172,7 +173,7 @@ qemuCheckpointDiscard(virQEMUDriverPtr driver, if (qemuMonitorTransactionBitmapMergeSourceAddBitmap(arr, node, disk->bitmap) < 0) return -1; - if (chk == virDomainCheckpointGetCurrent(vm->checkpoints)) { + if (chkcurrent) { if (qemuMonitorTransactionBitmapEnable(actions, node, disk2->bitmap) < 0) return -1; } @@ -192,7 +193,7 @@ qemuCheckpointDiscard(virQEMUDriverPtr driver, return -1; } - if (chk == virDomainCheckpointGetCurrent(vm->checkpoints)) { + if (chkcurrent) { virDomainCheckpointSetCurrent(vm->checkpoints, NULL); if (update_parent && parent) { virDomainCheckpointSetCurrent(vm->checkpoints, parent); -- 2.24.1
On 1/9/20 12:21 PM, Peter Krempa wrote:
Avoid two computations by using a boolean.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_checkpoint.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_checkpoint.c b/src/qemu/qemu_checkpoint.c
Reviewed-by: Eric Blake <eblake@redhat.com> -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
qemuCheckpointDiscard is a massive function that can be separated into smaller bits. Extract the part that actually modifies the disk from the metadata handling. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_checkpoint.c | 137 ++++++++++++++++++++----------------- 1 file changed, 76 insertions(+), 61 deletions(-) diff --git a/src/qemu/qemu_checkpoint.c b/src/qemu/qemu_checkpoint.c index d13d4c2a37..9ff3129570 100644 --- a/src/qemu/qemu_checkpoint.c +++ b/src/qemu/qemu_checkpoint.c @@ -104,6 +104,81 @@ qemuCheckpointWriteMetadata(virDomainObjPtr vm, } +static int +qemuCheckpointDiscardBitmaps(virDomainObjPtr vm, + virDomainCheckpointDefPtr chkdef, + bool chkcurrent, + virDomainMomentObjPtr parent) +{ + qemuDomainObjPrivatePtr priv = vm->privateData; + virQEMUDriverPtr driver = priv->driver; + virDomainMomentObjPtr moment; + virDomainCheckpointDefPtr parentdef = NULL; + bool search_parents; + int rc; + g_autoptr(virJSONValue) actions = NULL; + size_t i; + size_t j; + + if (!(actions = virJSONValueNewArray())) + return -1; + + for (i = 0; i < chkdef->ndisks; i++) { + virDomainCheckpointDiskDef *disk = &chkdef->disks[i]; + const char *node; + + if (disk->type != VIR_DOMAIN_CHECKPOINT_TYPE_BITMAP) + continue; + + node = qemuDomainDiskNodeFormatLookup(vm, disk->name); + /* If any ancestor checkpoint has a bitmap for the same + * disk, then this bitmap must be merged to the + * ancestor. */ + search_parents = true; + for (moment = parent; + search_parents && moment; + moment = virDomainCheckpointFindByName(vm->checkpoints, + parentdef->parent.parent_name)) { + parentdef = virDomainCheckpointObjGetDef(moment); + for (j = 0; j < parentdef->ndisks; j++) { + virDomainCheckpointDiskDef *disk2; + g_autoptr(virJSONValue) arr = NULL; + + disk2 = &parentdef->disks[j]; + if (STRNEQ(disk->name, disk2->name) || + disk2->type != VIR_DOMAIN_CHECKPOINT_TYPE_BITMAP) + continue; + search_parents = false; + + if (!(arr = virJSONValueNewArray())) + return -1; + + if (qemuMonitorTransactionBitmapMergeSourceAddBitmap(arr, node, disk->bitmap) < 0) + return -1; + + if (chkcurrent) { + if (qemuMonitorTransactionBitmapEnable(actions, node, disk2->bitmap) < 0) + return -1; + } + + if (qemuMonitorTransactionBitmapMerge(actions, node, disk2->bitmap, &arr) < 0) + return -1; + } + } + + if (qemuMonitorTransactionBitmapRemove(actions, node, disk->bitmap) < 0) + return -1; + } + + qemuDomainObjEnterMonitor(driver, vm); + rc = qemuMonitorTransaction(priv->mon, &actions); + if (qemuDomainObjExitMonitor(driver, vm) < 0 || rc < 0) + return -1; + + return 0; +} + + static int qemuCheckpointDiscard(virQEMUDriverPtr driver, virDomainObjPtr vm, @@ -112,9 +187,6 @@ qemuCheckpointDiscard(virQEMUDriverPtr driver, bool metadata_only) { virDomainMomentObjPtr parent = NULL; - virDomainMomentObjPtr moment; - virDomainCheckpointDefPtr parentdef = NULL; - size_t i, j; g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); g_autofree char *chkFile = NULL; bool chkcurrent = chk == virDomainCheckpointGetCurrent(vm->checkpoints); @@ -129,67 +201,10 @@ qemuCheckpointDiscard(virQEMUDriverPtr driver, chk->def->name); if (!metadata_only) { - qemuDomainObjPrivatePtr priv = vm->privateData; - bool search_parents; virDomainCheckpointDefPtr chkdef = virDomainCheckpointObjGetDef(chk); - int rc; - g_autoptr(virJSONValue) actions = NULL; - - if (!(actions = virJSONValueNewArray())) - return -1; - parent = virDomainCheckpointFindByName(vm->checkpoints, chk->def->parent_name); - for (i = 0; i < chkdef->ndisks; i++) { - virDomainCheckpointDiskDef *disk = &chkdef->disks[i]; - const char *node; - - if (disk->type != VIR_DOMAIN_CHECKPOINT_TYPE_BITMAP) - continue; - - node = qemuDomainDiskNodeFormatLookup(vm, disk->name); - /* If any ancestor checkpoint has a bitmap for the same - * disk, then this bitmap must be merged to the - * ancestor. */ - search_parents = true; - for (moment = parent; - search_parents && moment; - moment = virDomainCheckpointFindByName(vm->checkpoints, - parentdef->parent.parent_name)) { - parentdef = virDomainCheckpointObjGetDef(moment); - for (j = 0; j < parentdef->ndisks; j++) { - virDomainCheckpointDiskDef *disk2; - g_autoptr(virJSONValue) arr = NULL; - - disk2 = &parentdef->disks[j]; - if (STRNEQ(disk->name, disk2->name) || - disk2->type != VIR_DOMAIN_CHECKPOINT_TYPE_BITMAP) - continue; - search_parents = false; - - if (!(arr = virJSONValueNewArray())) - return -1; - - if (qemuMonitorTransactionBitmapMergeSourceAddBitmap(arr, node, disk->bitmap) < 0) - return -1; - - if (chkcurrent) { - if (qemuMonitorTransactionBitmapEnable(actions, node, disk2->bitmap) < 0) - return -1; - } - - if (qemuMonitorTransactionBitmapMerge(actions, node, disk2->bitmap, &arr) < 0) - return -1; - } - } - - if (qemuMonitorTransactionBitmapRemove(actions, node, disk->bitmap) < 0) - return -1; - } - - qemuDomainObjEnterMonitor(driver, vm); - rc = qemuMonitorTransaction(priv->mon, &actions); - if (qemuDomainObjExitMonitor(driver, vm) < 0 || rc < 0) + if (qemuCheckpointDiscardBitmaps(vm, chkdef, chkcurrent, parent) < 0) return -1; } -- 2.24.1
On 1/9/20 12:21 PM, Peter Krempa wrote:
qemuCheckpointDiscard is a massive function that can be separated into smaller bits. Extract the part that actually modifies the disk from the metadata handling.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_checkpoint.c | 137 ++++++++++++++++++++----------------- 1 file changed, 76 insertions(+), 61 deletions(-)
diff --git a/src/qemu/qemu_checkpoint.c b/src/qemu/qemu_checkpoint.c index d13d4c2a37..9ff3129570 100644 --- a/src/qemu/qemu_checkpoint.c +++ b/src/qemu/qemu_checkpoint.c @@ -104,6 +104,81 @@ qemuCheckpointWriteMetadata(virDomainObjPtr vm, }
+static int +qemuCheckpointDiscardBitmaps(virDomainObjPtr vm, + virDomainCheckpointDefPtr chkdef, + bool chkcurrent, + virDomainMomentObjPtr parent) +{ + qemuDomainObjPrivatePtr priv = vm->privateData; + virQEMUDriverPtr driver = priv->driver; + virDomainMomentObjPtr moment; + virDomainCheckpointDefPtr parentdef = NULL; + bool search_parents; + int rc; + g_autoptr(virJSONValue) actions = NULL; + size_t i; + size_t j; + + if (!(actions = virJSONValueNewArray())) + return -1;
Can virJSONValueNewArray() still return failure now that we have shifted to using g_new()? But changing it (if needed) should be independent of the code motion shown here. Reviewed-by: Eric Blake <eblake@redhat.com> -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
On Fri, Jan 24, 2020 at 11:13:01 -0600, Eric Blake wrote:
On 1/9/20 12:21 PM, Peter Krempa wrote:
qemuCheckpointDiscard is a massive function that can be separated into smaller bits. Extract the part that actually modifies the disk from the metadata handling.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_checkpoint.c | 137 ++++++++++++++++++++----------------- 1 file changed, 76 insertions(+), 61 deletions(-)
diff --git a/src/qemu/qemu_checkpoint.c b/src/qemu/qemu_checkpoint.c index d13d4c2a37..9ff3129570 100644 --- a/src/qemu/qemu_checkpoint.c +++ b/src/qemu/qemu_checkpoint.c @@ -104,6 +104,81 @@ qemuCheckpointWriteMetadata(virDomainObjPtr vm, }
+static int +qemuCheckpointDiscardBitmaps(virDomainObjPtr vm, + virDomainCheckpointDefPtr chkdef, + bool chkcurrent, + virDomainMomentObjPtr parent) +{ + qemuDomainObjPrivatePtr priv = vm->privateData; + virQEMUDriverPtr driver = priv->driver; + virDomainMomentObjPtr moment; + virDomainCheckpointDefPtr parentdef = NULL; + bool search_parents; + int rc; + g_autoptr(virJSONValue) actions = NULL; + size_t i; + size_t j; + + if (!(actions = virJSONValueNewArray())) + return -1;
Can virJSONValueNewArray() still return failure now that we have shifted to using g_new()?
It can't fail any more, but it internally uses VIR_ALLOC which is declared with ATTRIBUTE_RETURN_CHECK thus we have to check the return value. Also even if we did not, e.g. coverity heuristically assumes that if a big number of callers check the return value and moans if some don't. That said. I can add a ignore_value().
But changing it (if needed) should be independent of the code motion shown here.
Reviewed-by: Eric Blake <eblake@redhat.com>
-- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Upcomming patches will also use the domain diks definition. Rename disk to chkdisk for clarity. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_checkpoint.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/qemu/qemu_checkpoint.c b/src/qemu/qemu_checkpoint.c index 9ff3129570..d347b8fc6c 100644 --- a/src/qemu/qemu_checkpoint.c +++ b/src/qemu/qemu_checkpoint.c @@ -124,13 +124,13 @@ qemuCheckpointDiscardBitmaps(virDomainObjPtr vm, return -1; for (i = 0; i < chkdef->ndisks; i++) { - virDomainCheckpointDiskDef *disk = &chkdef->disks[i]; + virDomainCheckpointDiskDef *chkdisk = &chkdef->disks[i]; const char *node; - if (disk->type != VIR_DOMAIN_CHECKPOINT_TYPE_BITMAP) + if (chkdisk->type != VIR_DOMAIN_CHECKPOINT_TYPE_BITMAP) continue; - node = qemuDomainDiskNodeFormatLookup(vm, disk->name); + node = qemuDomainDiskNodeFormatLookup(vm, chkdisk->name); /* If any ancestor checkpoint has a bitmap for the same * disk, then this bitmap must be merged to the * ancestor. */ @@ -145,7 +145,7 @@ qemuCheckpointDiscardBitmaps(virDomainObjPtr vm, g_autoptr(virJSONValue) arr = NULL; disk2 = &parentdef->disks[j]; - if (STRNEQ(disk->name, disk2->name) || + if (STRNEQ(chkdisk->name, disk2->name) || disk2->type != VIR_DOMAIN_CHECKPOINT_TYPE_BITMAP) continue; search_parents = false; @@ -153,7 +153,7 @@ qemuCheckpointDiscardBitmaps(virDomainObjPtr vm, if (!(arr = virJSONValueNewArray())) return -1; - if (qemuMonitorTransactionBitmapMergeSourceAddBitmap(arr, node, disk->bitmap) < 0) + if (qemuMonitorTransactionBitmapMergeSourceAddBitmap(arr, node, chkdisk->bitmap) < 0) return -1; if (chkcurrent) { @@ -166,7 +166,7 @@ qemuCheckpointDiscardBitmaps(virDomainObjPtr vm, } } - if (qemuMonitorTransactionBitmapRemove(actions, node, disk->bitmap) < 0) + if (qemuMonitorTransactionBitmapRemove(actions, node, chkdisk->bitmap) < 0) return -1; } -- 2.24.1
On 1/9/20 12:21 PM, Peter Krempa wrote:
Upcomming patches will also use the domain diks definition. Rename disk
Upcoming, disck
to chkdisk for clarity.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_checkpoint.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)
Mechanical, other than the commit message typos. Reviewed-by: Eric Blake <eblake@redhat.com> -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
On 1/24/20 11:15 AM, Eric Blake wrote:
On 1/9/20 12:21 PM, Peter Krempa wrote:
Upcomming patches will also use the domain diks definition. Rename disk
Upcoming, disck
Urgh - I typo'd my typo correction. 'disck' is not a word, and 'disc' is unusual these days; so I meant 'disk'.
to chkdisk for clarity.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_checkpoint.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)
Mechanical, other than the commit message typos. Reviewed-by: Eric Blake <eblake@redhat.com>
-- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Upcomming patches will also use the domain diks definition. Rename disk to chkdisk for clarity. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_checkpoint.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/qemu/qemu_checkpoint.c b/src/qemu/qemu_checkpoint.c index d347b8fc6c..0aa854324b 100644 --- a/src/qemu/qemu_checkpoint.c +++ b/src/qemu/qemu_checkpoint.c @@ -319,13 +319,13 @@ qemuCheckpointAddActions(virDomainObjPtr vm, bool search_parents; for (i = 0; i < def->ndisks; i++) { - virDomainCheckpointDiskDef *disk = &def->disks[i]; + virDomainCheckpointDiskDef *chkdisk = &def->disks[i]; const char *node; - if (disk->type != VIR_DOMAIN_CHECKPOINT_TYPE_BITMAP) + if (chkdisk->type != VIR_DOMAIN_CHECKPOINT_TYPE_BITMAP) continue; - node = qemuDomainDiskNodeFormatLookup(vm, disk->name); - if (qemuMonitorTransactionBitmapAdd(actions, node, disk->bitmap, true, false, 0) < 0) + node = qemuDomainDiskNodeFormatLookup(vm, chkdisk->name); + if (qemuMonitorTransactionBitmapAdd(actions, node, chkdisk->bitmap, true, false, 0) < 0) return -1; /* We only want one active bitmap for a disk along the @@ -345,7 +345,7 @@ qemuCheckpointAddActions(virDomainObjPtr vm, virDomainCheckpointDiskDef *disk2; disk2 = &olddef->disks[j]; - if (STRNEQ(disk->name, disk2->name) || + if (STRNEQ(chkdisk->name, disk2->name) || disk2->type != VIR_DOMAIN_CHECKPOINT_TYPE_BITMAP) continue; if (qemuMonitorTransactionBitmapDisable(actions, node, disk2->bitmap) < 0) -- 2.24.1
On 1/9/20 12:21 PM, Peter Krempa wrote:
Upcomming patches will also use the domain diks definition. Rename disk to chkdisk for clarity.
Same typos as in 3/16. You could squash the two patches if desired, since it is so mechanical. Reviewed-by: Eric Blake <eblake@redhat.com> -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Lookup the whole disk definition rather than just the node name. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_checkpoint.c | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/src/qemu/qemu_checkpoint.c b/src/qemu/qemu_checkpoint.c index 0aa854324b..03a8321135 100644 --- a/src/qemu/qemu_checkpoint.c +++ b/src/qemu/qemu_checkpoint.c @@ -320,12 +320,16 @@ qemuCheckpointAddActions(virDomainObjPtr vm, for (i = 0; i < def->ndisks; i++) { virDomainCheckpointDiskDef *chkdisk = &def->disks[i]; - const char *node; + virDomainDiskDefPtr domdisk = virDomainDiskByTarget(vm->def, chkdisk->name); - if (chkdisk->type != VIR_DOMAIN_CHECKPOINT_TYPE_BITMAP) + /* checkpoint definition validator mandates that the corresponding + * domdisk should exist */ + if (!domdisk || + chkdisk->type != VIR_DOMAIN_CHECKPOINT_TYPE_BITMAP) continue; - node = qemuDomainDiskNodeFormatLookup(vm, chkdisk->name); - if (qemuMonitorTransactionBitmapAdd(actions, node, chkdisk->bitmap, true, false, 0) < 0) + + if (qemuMonitorTransactionBitmapAdd(actions, domdisk->src->nodeformat, + chkdisk->bitmap, true, false, 0) < 0) return -1; /* We only want one active bitmap for a disk along the @@ -348,7 +352,9 @@ qemuCheckpointAddActions(virDomainObjPtr vm, if (STRNEQ(chkdisk->name, disk2->name) || disk2->type != VIR_DOMAIN_CHECKPOINT_TYPE_BITMAP) continue; - if (qemuMonitorTransactionBitmapDisable(actions, node, disk2->bitmap) < 0) + if (qemuMonitorTransactionBitmapDisable(actions, + domdisk->src->nodeformat, + disk2->bitmap) < 0) return -1; search_parents = false; break; -- 2.24.1
On 1/9/20 12:21 PM, Peter Krempa wrote:
Lookup the whole disk definition rather than just the node name.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_checkpoint.c | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-)
Reviewed-by: Eric Blake <eblake@redhat.com> -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
If a disk is unplugged and then the user tries to delete a checkpoint the code would try to use NULL node name as it was not checked. Fix this by fetching the whole disk definition object and verifying it was found. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_checkpoint.c | 23 +++++++++++++++++------ 1 file changed, 17 insertions(+), 6 deletions(-) diff --git a/src/qemu/qemu_checkpoint.c b/src/qemu/qemu_checkpoint.c index 03a8321135..326707e098 100644 --- a/src/qemu/qemu_checkpoint.c +++ b/src/qemu/qemu_checkpoint.c @@ -125,12 +125,15 @@ qemuCheckpointDiscardBitmaps(virDomainObjPtr vm, for (i = 0; i < chkdef->ndisks; i++) { virDomainCheckpointDiskDef *chkdisk = &chkdef->disks[i]; - const char *node; + virDomainDiskDefPtr domdisk = virDomainDiskByTarget(vm->def, chkdisk->name); + + /* domdisk can be missing e.g. when it was unplugged */ + if (!domdisk) + continue; if (chkdisk->type != VIR_DOMAIN_CHECKPOINT_TYPE_BITMAP) continue; - node = qemuDomainDiskNodeFormatLookup(vm, chkdisk->name); /* If any ancestor checkpoint has a bitmap for the same * disk, then this bitmap must be merged to the * ancestor. */ @@ -153,20 +156,28 @@ qemuCheckpointDiscardBitmaps(virDomainObjPtr vm, if (!(arr = virJSONValueNewArray())) return -1; - if (qemuMonitorTransactionBitmapMergeSourceAddBitmap(arr, node, chkdisk->bitmap) < 0) + if (qemuMonitorTransactionBitmapMergeSourceAddBitmap(arr, + domdisk->src->nodeformat, + chkdisk->bitmap) < 0) return -1; if (chkcurrent) { - if (qemuMonitorTransactionBitmapEnable(actions, node, disk2->bitmap) < 0) + if (qemuMonitorTransactionBitmapEnable(actions, + domdisk->src->nodeformat, + disk2->bitmap) < 0) return -1; } - if (qemuMonitorTransactionBitmapMerge(actions, node, disk2->bitmap, &arr) < 0) + if (qemuMonitorTransactionBitmapMerge(actions, + domdisk->src->nodeformat, + disk2->bitmap, &arr) < 0) return -1; } } - if (qemuMonitorTransactionBitmapRemove(actions, node, chkdisk->bitmap) < 0) + if (qemuMonitorTransactionBitmapRemove(actions, + domdisk->src->nodeformat, + chkdisk->bitmap) < 0) return -1; } -- 2.24.1
On 1/9/20 12:21 PM, Peter Krempa wrote:
If a disk is unplugged and then the user tries to delete a checkpoint the code would try to use NULL node name as it was not checked.
Fix this by fetching the whole disk definition object and verifying it was found.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_checkpoint.c | 23 +++++++++++++++++------ 1 file changed, 17 insertions(+), 6 deletions(-)
Reviewed-by: Eric Blake <eblake@redhat.com> But it also makes me wonder if the act of hot-unplug should update the definition of existing checkpoints. (Doesn't stop this patch from being useful as-is, but may point to further design work and future patches) -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
On Fri, Jan 24, 2020 at 12:47:41 -0600, Eric Blake wrote:
On 1/9/20 12:21 PM, Peter Krempa wrote:
If a disk is unplugged and then the user tries to delete a checkpoint the code would try to use NULL node name as it was not checked.
Fix this by fetching the whole disk definition object and verifying it was found.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_checkpoint.c | 23 +++++++++++++++++------ 1 file changed, 17 insertions(+), 6 deletions(-)
Reviewed-by: Eric Blake <eblake@redhat.com>
But it also makes me wonder if the act of hot-unplug should update the definition of existing checkpoints. (Doesn't stop this patch from being useful as-is, but may point to further design work and future patches)
It can't since we chose to not version checkpoints on snapshots. Thus if you've taken a checkpoint and then a snapshot and want to detach the disk, reverting to the checkpoint will add the disk back and the checkpoint must stay valid. As the checkpoint is not versioned we wouldn't have the data to add it back.
The function has no users now and there's no need for it as the common pattern is to look up the whole disk object anyways. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_domain.c | 14 -------------- src/qemu/qemu_domain.h | 3 --- 2 files changed, 17 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 24e84a5966..4966f5ef36 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -12048,20 +12048,6 @@ qemuDomainDiskChangeSupported(virDomainDiskDefPtr disk, } -/* Return the format node name for a given disk of an online guest */ -const char * -qemuDomainDiskNodeFormatLookup(virDomainObjPtr vm, - const char *disk) -{ - size_t i; - - for (i = 0; i < vm->def->ndisks; i++) { - if (STREQ(vm->def->disks[i]->dst, disk)) - return vm->def->disks[i]->src->nodeformat; - } - return NULL; -} - bool qemuDomainDiskBlockJobIsActive(virDomainDiskDefPtr disk) { diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index c6afc484f6..4e2ddf6880 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -864,9 +864,6 @@ int qemuDomainDetermineDiskChain(virQEMUDriverPtr driver, bool qemuDomainDiskChangeSupported(virDomainDiskDefPtr disk, virDomainDiskDefPtr orig_disk); -const char *qemuDomainDiskNodeFormatLookup(virDomainObjPtr vm, - const char *disk); - void qemuDomainGetImageIds(virQEMUDriverConfigPtr cfg, virDomainObjPtr vm, virStorageSourcePtr src, -- 2.24.1
On 1/9/20 12:21 PM, Peter Krempa wrote:
The function has no users now and there's no need for it as the common pattern is to look up the whole disk object anyways.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_domain.c | 14 -------------- src/qemu/qemu_domain.h | 3 --- 2 files changed, 17 deletions(-)
Code deletion is easy to review :) Reviewed-by: Eric Blake <eblake@redhat.com> -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
The algorithm is used in two places to find the parent checkpoint object which contains given disk and then uses data from the disk. Additionally the code is written in a very non-obvious way. Factor out the lookup of the disk into a function which also simplifies the callers. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_checkpoint.c | 129 ++++++++++++++++++++----------------- 1 file changed, 71 insertions(+), 58 deletions(-) diff --git a/src/qemu/qemu_checkpoint.c b/src/qemu/qemu_checkpoint.c index 326707e098..1100f6e744 100644 --- a/src/qemu/qemu_checkpoint.c +++ b/src/qemu/qemu_checkpoint.c @@ -104,6 +104,50 @@ qemuCheckpointWriteMetadata(virDomainObjPtr vm, } +/** + * qemuCheckpointFindActiveDiskInParent: + * @vm: domain object + * @from: starting moment object + * @diskname: name (target) of the disk to find + * + * Find the first checkpoint starting from @from continuing through parents + * of the checkpoint which describes disk @diskname. Return the pointer to the + * definition of the disk. + */ +static virDomainCheckpointDiskDef * +qemuCheckpointFindActiveDiskInParent(virDomainObjPtr vm, + virDomainMomentObjPtr from, + const char *diskname) +{ + virDomainMomentObjPtr parent = from; + virDomainCheckpointDefPtr parentdef = NULL; + size_t i; + + while (parent) { + parentdef = virDomainCheckpointObjGetDef(parent); + + for (i = 0; i < parentdef->ndisks; i++) { + virDomainCheckpointDiskDef *chkdisk = &parentdef->disks[i]; + + if (STRNEQ(chkdisk->name, diskname)) + continue; + + /* currently inspected checkpoint doesn't describe the disk, + * continue into parent checkpoint */ + if (chkdisk->type != VIR_DOMAIN_CHECKPOINT_TYPE_BITMAP) + break; + + return chkdisk; + } + + parent = virDomainCheckpointFindByName(vm->checkpoints, + parentdef->parent.parent_name); + } + + return NULL; +} + + static int qemuCheckpointDiscardBitmaps(virDomainObjPtr vm, virDomainCheckpointDefPtr chkdef, @@ -112,13 +156,9 @@ qemuCheckpointDiscardBitmaps(virDomainObjPtr vm, { qemuDomainObjPrivatePtr priv = vm->privateData; virQEMUDriverPtr driver = priv->driver; - virDomainMomentObjPtr moment; - virDomainCheckpointDefPtr parentdef = NULL; - bool search_parents; int rc; g_autoptr(virJSONValue) actions = NULL; size_t i; - size_t j; if (!(actions = virJSONValueNewArray())) return -1; @@ -126,6 +166,7 @@ qemuCheckpointDiscardBitmaps(virDomainObjPtr vm, for (i = 0; i < chkdef->ndisks; i++) { virDomainCheckpointDiskDef *chkdisk = &chkdef->disks[i]; virDomainDiskDefPtr domdisk = virDomainDiskByTarget(vm->def, chkdisk->name); + virDomainCheckpointDiskDef *parentchkdisk = NULL; /* domdisk can be missing e.g. when it was unplugged */ if (!domdisk) @@ -137,42 +178,28 @@ qemuCheckpointDiscardBitmaps(virDomainObjPtr vm, /* If any ancestor checkpoint has a bitmap for the same * disk, then this bitmap must be merged to the * ancestor. */ - search_parents = true; - for (moment = parent; - search_parents && moment; - moment = virDomainCheckpointFindByName(vm->checkpoints, - parentdef->parent.parent_name)) { - parentdef = virDomainCheckpointObjGetDef(moment); - for (j = 0; j < parentdef->ndisks; j++) { - virDomainCheckpointDiskDef *disk2; - g_autoptr(virJSONValue) arr = NULL; - - disk2 = &parentdef->disks[j]; - if (STRNEQ(chkdisk->name, disk2->name) || - disk2->type != VIR_DOMAIN_CHECKPOINT_TYPE_BITMAP) - continue; - search_parents = false; - - if (!(arr = virJSONValueNewArray())) - return -1; + if ((parentchkdisk = qemuCheckpointFindActiveDiskInParent(vm, parent, chkdisk->name))) { + g_autoptr(virJSONValue) arr = NULL; - if (qemuMonitorTransactionBitmapMergeSourceAddBitmap(arr, - domdisk->src->nodeformat, - chkdisk->bitmap) < 0) - return -1; + if (!(arr = virJSONValueNewArray())) + return -1; - if (chkcurrent) { - if (qemuMonitorTransactionBitmapEnable(actions, - domdisk->src->nodeformat, - disk2->bitmap) < 0) - return -1; - } + if (qemuMonitorTransactionBitmapMergeSourceAddBitmap(arr, + domdisk->src->nodeformat, + chkdisk->bitmap) < 0) + return -1; - if (qemuMonitorTransactionBitmapMerge(actions, - domdisk->src->nodeformat, - disk2->bitmap, &arr) < 0) + if (chkcurrent) { + if (qemuMonitorTransactionBitmapEnable(actions, + domdisk->src->nodeformat, + parentchkdisk->bitmap) < 0) return -1; } + + if (qemuMonitorTransactionBitmapMerge(actions, + domdisk->src->nodeformat, + parentchkdisk->bitmap, &arr) < 0) + return -1; } if (qemuMonitorTransactionBitmapRemove(actions, @@ -324,14 +351,12 @@ qemuCheckpointAddActions(virDomainObjPtr vm, virDomainMomentObjPtr old_current, virDomainCheckpointDefPtr def) { - size_t i, j; - virDomainCheckpointDefPtr olddef; - virDomainMomentObjPtr parent; - bool search_parents; + size_t i; for (i = 0; i < def->ndisks; i++) { virDomainCheckpointDiskDef *chkdisk = &def->disks[i]; virDomainDiskDefPtr domdisk = virDomainDiskByTarget(vm->def, chkdisk->name); + virDomainCheckpointDiskDef *parentchkdisk = NULL; /* checkpoint definition validator mandates that the corresponding * domdisk should exist */ @@ -351,25 +376,13 @@ qemuCheckpointAddActions(virDomainObjPtr vm, * iteration; but it is also possible to have to search * further than the immediate parent to find another * checkpoint with a bitmap on the same disk. */ - search_parents = true; - for (parent = old_current; search_parents && parent; - parent = virDomainCheckpointFindByName(vm->checkpoints, - olddef->parent.parent_name)) { - olddef = virDomainCheckpointObjGetDef(parent); - for (j = 0; j < olddef->ndisks; j++) { - virDomainCheckpointDiskDef *disk2; - - disk2 = &olddef->disks[j]; - if (STRNEQ(chkdisk->name, disk2->name) || - disk2->type != VIR_DOMAIN_CHECKPOINT_TYPE_BITMAP) - continue; - if (qemuMonitorTransactionBitmapDisable(actions, - domdisk->src->nodeformat, - disk2->bitmap) < 0) - return -1; - search_parents = false; - break; - } + if ((parentchkdisk = qemuCheckpointFindActiveDiskInParent(vm, old_current, + chkdisk->name))) { + + if (qemuMonitorTransactionBitmapDisable(actions, + domdisk->src->nodeformat, + parentchkdisk->bitmap) < 0) + return -1; } } return 0; -- 2.24.1
On 1/9/20 12:21 PM, Peter Krempa wrote:
The algorithm is used in two places to find the parent checkpoint object which contains given disk and then uses data from the disk. Additionally the code is written in a very non-obvious way. Factor out the lookup of the disk into a function which also simplifies the callers.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_checkpoint.c | 129 ++++++++++++++++++++----------------- 1 file changed, 71 insertions(+), 58 deletions(-)
Thank you. I noticed the repetition when I was preparing my v10 of incremental backup, but did not manage to factor it out cleanly like this.
+/** + * qemuCheckpointFindActiveDiskInParent: + * @vm: domain object + * @from: starting moment object + * @diskname: name (target) of the disk to find + * + * Find the first checkpoint starting from @from continuing through parents + * of the checkpoint which describes disk @diskname. Return the pointer to the + * definition of the disk. + */ +static virDomainCheckpointDiskDef * +qemuCheckpointFindActiveDiskInParent(virDomainObjPtr vm, + virDomainMomentObjPtr from, + const char *diskname) +{
Reviewed-by: Eric Blake <eblake@redhat.com> -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
This will allow some testing before refactoring. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_checkpoint.c | 72 ++++++++++++++++++++++++-------------- src/qemu/qemu_checkpoint.h | 7 ++++ 2 files changed, 53 insertions(+), 26 deletions(-) diff --git a/src/qemu/qemu_checkpoint.c b/src/qemu/qemu_checkpoint.c index 1100f6e744..e75cdd0458 100644 --- a/src/qemu/qemu_checkpoint.c +++ b/src/qemu/qemu_checkpoint.c @@ -148,6 +148,46 @@ qemuCheckpointFindActiveDiskInParent(virDomainObjPtr vm, } +int +qemuCheckpointDiscardDiskBitmaps(virStorageSourcePtr src, + const char *delbitmap, + const char *parentbitmap, + bool chkcurrent, + virJSONValuePtr actions) +{ + if (parentbitmap) { + g_autoptr(virJSONValue) arr = NULL; + + if (!(arr = virJSONValueNewArray())) + return -1; + + if (qemuMonitorTransactionBitmapMergeSourceAddBitmap(arr, + src->nodeformat, + delbitmap) < 0) + return -1; + + if (chkcurrent) { + if (qemuMonitorTransactionBitmapEnable(actions, + src->nodeformat, + parentbitmap) < 0) + return -1; + } + + if (qemuMonitorTransactionBitmapMerge(actions, + src->nodeformat, + parentbitmap, &arr) < 0) + return -1; + } + + if (qemuMonitorTransactionBitmapRemove(actions, + src->nodeformat, + delbitmap) < 0) + return -1; + + return 0; +} + + static int qemuCheckpointDiscardBitmaps(virDomainObjPtr vm, virDomainCheckpointDefPtr chkdef, @@ -167,6 +207,7 @@ qemuCheckpointDiscardBitmaps(virDomainObjPtr vm, virDomainCheckpointDiskDef *chkdisk = &chkdef->disks[i]; virDomainDiskDefPtr domdisk = virDomainDiskByTarget(vm->def, chkdisk->name); virDomainCheckpointDiskDef *parentchkdisk = NULL; + const char *parentbitmap = NULL; /* domdisk can be missing e.g. when it was unplugged */ if (!domdisk) @@ -178,33 +219,12 @@ qemuCheckpointDiscardBitmaps(virDomainObjPtr vm, /* If any ancestor checkpoint has a bitmap for the same * disk, then this bitmap must be merged to the * ancestor. */ - if ((parentchkdisk = qemuCheckpointFindActiveDiskInParent(vm, parent, chkdisk->name))) { - g_autoptr(virJSONValue) arr = NULL; - - if (!(arr = virJSONValueNewArray())) - return -1; - - if (qemuMonitorTransactionBitmapMergeSourceAddBitmap(arr, - domdisk->src->nodeformat, - chkdisk->bitmap) < 0) - return -1; - - if (chkcurrent) { - if (qemuMonitorTransactionBitmapEnable(actions, - domdisk->src->nodeformat, - parentchkdisk->bitmap) < 0) - return -1; - } - - if (qemuMonitorTransactionBitmapMerge(actions, - domdisk->src->nodeformat, - parentchkdisk->bitmap, &arr) < 0) - return -1; - } + if ((parentchkdisk = qemuCheckpointFindActiveDiskInParent(vm, parent, + chkdisk->name))) + parentbitmap = parentchkdisk->name; - if (qemuMonitorTransactionBitmapRemove(actions, - domdisk->src->nodeformat, - chkdisk->bitmap) < 0) + if (qemuCheckpointDiscardDiskBitmaps(domdisk->src, chkdisk->bitmap, + parentbitmap, chkcurrent, actions) < 0) return -1; } diff --git a/src/qemu/qemu_checkpoint.h b/src/qemu/qemu_checkpoint.h index eb85611ea6..85fd453d50 100644 --- a/src/qemu/qemu_checkpoint.h +++ b/src/qemu/qemu_checkpoint.h @@ -71,3 +71,10 @@ qemuCheckpointCreateFinalize(virQEMUDriverPtr driver, void qemuCheckpointRollbackMetadata(virDomainObjPtr vm, virDomainMomentObjPtr chk); + +int +qemuCheckpointDiscardDiskBitmaps(virStorageSourcePtr src, + const char *delbitmap, + const char *parentbitmap, + bool chkcurrent, + virJSONValuePtr actions); -- 2.24.1
On 1/9/20 12:21 PM, Peter Krempa wrote:
This will allow some testing before refactoring.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_checkpoint.c | 72 ++++++++++++++++++++++++-------------- src/qemu/qemu_checkpoint.h | 7 ++++ 2 files changed, 53 insertions(+), 26 deletions(-)
diff --git a/src/qemu/qemu_checkpoint.c b/src/qemu/qemu_checkpoint.c
Reviewed-by: Eric Blake <eblake@redhat.com> -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Add test infrastructure and a basic test for bitmap deletion. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/qemublocktest.c | 59 +++++++++++++++++++ .../checkpointdelete/basic-noparent-out.json | 9 +++ 2 files changed, 68 insertions(+) create mode 100644 tests/qemublocktestdata/checkpointdelete/basic-noparent-out.json diff --git a/tests/qemublocktest.c b/tests/qemublocktest.c index 3e9edb85f0..3ed2486ad2 100644 --- a/tests/qemublocktest.c +++ b/tests/qemublocktest.c @@ -27,6 +27,7 @@ #include "qemu/qemu_qapi.h" #include "qemu/qemu_monitor_json.h" #include "qemu/qemu_backup.h" +#include "qemu/qemu_checkpoint.h" #include "qemu/qemu_command.h" @@ -696,6 +697,50 @@ testQemuBackupIncrementalBitmapCalculate(const void *opaque) } +static const char *checkpointDeletePrefix = "qemublocktestdata/checkpointdelete/"; + +struct testQemuCheckpointDeleteMergeData { + const char *name; + virStorageSourcePtr chain; + const char *deletebitmap; + const char *parentbitmap; +}; + + +static int +testQemuCheckpointDeleteMerge(const void *opaque) +{ + const struct testQemuCheckpointDeleteMergeData *data = opaque; + g_autofree char *actual = NULL; + g_autofree char *expectpath = NULL; + g_autoptr(virJSONValue) actions = NULL; + bool currentcheckpoint; + + expectpath = g_strdup_printf("%s/%s%s-out.json", abs_srcdir, + checkpointDeletePrefix, data->name); + + if (!(actions = virJSONValueNewArray())) + return -1; + + /* hack to get the 'current' state until the function stops accepting it */ + currentcheckpoint = STREQ("current", data->deletebitmap); + + if (qemuCheckpointDiscardDiskBitmaps(data->chain, + data->deletebitmap, + data->parentbitmap, + currentcheckpoint, + actions) < 0) { + VIR_TEST_VERBOSE("failed to generate checkpoint delete transaction\n"); + return -1; + } + + if (!(actual = virJSONValueToString(actions, true))) + return -1; + + return virTestCompareToFile(actual, expectpath); +} + + static int mymain(void) { @@ -705,6 +750,7 @@ mymain(void) struct testQemuDiskXMLToJSONData diskxmljsondata; struct testQemuImageCreateData imagecreatedata; struct testQemuBackupIncrementalBitmapCalculateData backupbitmapcalcdata; + struct testQemuCheckpointDeleteMergeData checkpointdeletedata; char *capslatest_x86_64 = NULL; virQEMUCapsPtr caps_x86_64 = NULL; g_autoptr(virStorageSource) bitmapSourceChain = NULL; @@ -941,6 +987,19 @@ mymain(void) TEST_BACKUP_BITMAP_CALCULATE("snapshot-intermediate", bitmapSourceChain, "d", "snapshots"); TEST_BACKUP_BITMAP_CALCULATE("snapshot-deep", bitmapSourceChain, "a", "snapshots"); +#define TEST_CHECKPOINT_DELETE_MERGE(testname, delbmp, parbmp) \ + do { \ + checkpointdeletedata.name = testname; \ + checkpointdeletedata.chain = bitmapSourceChain; \ + checkpointdeletedata.deletebitmap = delbmp; \ + checkpointdeletedata.parentbitmap = parbmp; \ + if (virTestRun("checkpoint delete " testname, \ + testQemuCheckpointDeleteMerge, &checkpointdeletedata) < 0) \ + ret = -1; \ + } while (0) + + TEST_CHECKPOINT_DELETE_MERGE("basic-noparent", "a", NULL); + cleanup: virHashFree(diskxmljsondata.schema); qemuTestDriverFree(&driver); diff --git a/tests/qemublocktestdata/checkpointdelete/basic-noparent-out.json b/tests/qemublocktestdata/checkpointdelete/basic-noparent-out.json new file mode 100644 index 0000000000..e87382fdb4 --- /dev/null +++ b/tests/qemublocktestdata/checkpointdelete/basic-noparent-out.json @@ -0,0 +1,9 @@ +[ + { + "type": "block-dirty-bitmap-remove", + "data": { + "node": "libvirt-1-format", + "name": "a" + } + } +] -- 2.24.1
On 1/9/20 12:21 PM, Peter Krempa wrote:
Add test infrastructure and a basic test for bitmap deletion.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/qemublocktest.c | 59 +++++++++++++++++++ .../checkpointdelete/basic-noparent-out.json | 9 +++ 2 files changed, 68 insertions(+) create mode 100644 tests/qemublocktestdata/checkpointdelete/basic-noparent-out.json
+static int +testQemuCheckpointDeleteMerge(const void *opaque) +{ + const struct testQemuCheckpointDeleteMergeData *data = opaque; + g_autofree char *actual = NULL; + g_autofree char *expectpath = NULL; + g_autoptr(virJSONValue) actions = NULL; + bool currentcheckpoint; + + expectpath = g_strdup_printf("%s/%s%s-out.json", abs_srcdir, + checkpointDeletePrefix, data->name); + + if (!(actions = virJSONValueNewArray())) + return -1; + + /* hack to get the 'current' state until the function stops accepting it */ + currentcheckpoint = STREQ("current", data->deletebitmap); + + if (qemuCheckpointDiscardDiskBitmaps(data->chain, + data->deletebitmap, + data->parentbitmap, + currentcheckpoint, + actions) < 0) {
Are there patches planned later on to change this? Or is this comment going to be an instance of technical debt for an unknown length of time? At any rate, the test looks sane enough; Reviewed-by: Eric Blake <eblake@redhat.com> -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
On Mon, Jan 27, 2020 at 12:55:02 -0600, Eric Blake wrote:
On 1/9/20 12:21 PM, Peter Krempa wrote:
Add test infrastructure and a basic test for bitmap deletion.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/qemublocktest.c | 59 +++++++++++++++++++ .../checkpointdelete/basic-noparent-out.json | 9 +++ 2 files changed, 68 insertions(+) create mode 100644 tests/qemublocktestdata/checkpointdelete/basic-noparent-out.json
+static int +testQemuCheckpointDeleteMerge(const void *opaque) +{ + const struct testQemuCheckpointDeleteMergeData *data = opaque; + g_autofree char *actual = NULL; + g_autofree char *expectpath = NULL; + g_autoptr(virJSONValue) actions = NULL; + bool currentcheckpoint; + + expectpath = g_strdup_printf("%s/%s%s-out.json", abs_srcdir, + checkpointDeletePrefix, data->name); + + if (!(actions = virJSONValueNewArray())) + return -1; + + /* hack to get the 'current' state until the function stops accepting it */ + currentcheckpoint = STREQ("current", data->deletebitmap); + + if (qemuCheckpointDiscardDiskBitmaps(data->chain, + data->deletebitmap, + data->parentbitmap, + currentcheckpoint, + actions) < 0) {
Are there patches planned later on to change this? Or is this comment going to be an instance of technical debt for an unknown length of time?
Patch 13 modifies qemuCheckpointDiscardDiskBitmaps to work accross the backing chain and after that modification it doesn't require the parameter any more.
Add all intermediate steps and deletion of the current checkpoint on a flat (single-image) disk image. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/qemublocktest.c | 4 +++ .../checkpointdelete/basic-current-out.json | 29 +++++++++++++++++++ .../basic-intermediate1-out.json | 22 ++++++++++++++ .../basic-intermediate2-out.json | 22 ++++++++++++++ .../basic-intermediate3-out.json | 22 ++++++++++++++ 5 files changed, 99 insertions(+) create mode 100644 tests/qemublocktestdata/checkpointdelete/basic-current-out.json create mode 100644 tests/qemublocktestdata/checkpointdelete/basic-intermediate1-out.json create mode 100644 tests/qemublocktestdata/checkpointdelete/basic-intermediate2-out.json create mode 100644 tests/qemublocktestdata/checkpointdelete/basic-intermediate3-out.json diff --git a/tests/qemublocktest.c b/tests/qemublocktest.c index 3ed2486ad2..203e0f70b1 100644 --- a/tests/qemublocktest.c +++ b/tests/qemublocktest.c @@ -999,6 +999,10 @@ mymain(void) } while (0) TEST_CHECKPOINT_DELETE_MERGE("basic-noparent", "a", NULL); + TEST_CHECKPOINT_DELETE_MERGE("basic-intermediate1", "b", "a"); + TEST_CHECKPOINT_DELETE_MERGE("basic-intermediate2", "c", "b"); + TEST_CHECKPOINT_DELETE_MERGE("basic-intermediate3", "d", "c"); + TEST_CHECKPOINT_DELETE_MERGE("basic-current", "current", "d"); cleanup: virHashFree(diskxmljsondata.schema); diff --git a/tests/qemublocktestdata/checkpointdelete/basic-current-out.json b/tests/qemublocktestdata/checkpointdelete/basic-current-out.json new file mode 100644 index 0000000000..1b607567e8 --- /dev/null +++ b/tests/qemublocktestdata/checkpointdelete/basic-current-out.json @@ -0,0 +1,29 @@ +[ + { + "type": "block-dirty-bitmap-enable", + "data": { + "node": "libvirt-1-format", + "name": "d" + } + }, + { + "type": "block-dirty-bitmap-merge", + "data": { + "node": "libvirt-1-format", + "target": "d", + "bitmaps": [ + { + "node": "libvirt-1-format", + "name": "current" + } + ] + } + }, + { + "type": "block-dirty-bitmap-remove", + "data": { + "node": "libvirt-1-format", + "name": "current" + } + } +] diff --git a/tests/qemublocktestdata/checkpointdelete/basic-intermediate1-out.json b/tests/qemublocktestdata/checkpointdelete/basic-intermediate1-out.json new file mode 100644 index 0000000000..eccb7ed15f --- /dev/null +++ b/tests/qemublocktestdata/checkpointdelete/basic-intermediate1-out.json @@ -0,0 +1,22 @@ +[ + { + "type": "block-dirty-bitmap-merge", + "data": { + "node": "libvirt-1-format", + "target": "a", + "bitmaps": [ + { + "node": "libvirt-1-format", + "name": "b" + } + ] + } + }, + { + "type": "block-dirty-bitmap-remove", + "data": { + "node": "libvirt-1-format", + "name": "b" + } + } +] diff --git a/tests/qemublocktestdata/checkpointdelete/basic-intermediate2-out.json b/tests/qemublocktestdata/checkpointdelete/basic-intermediate2-out.json new file mode 100644 index 0000000000..de40e4b5b0 --- /dev/null +++ b/tests/qemublocktestdata/checkpointdelete/basic-intermediate2-out.json @@ -0,0 +1,22 @@ +[ + { + "type": "block-dirty-bitmap-merge", + "data": { + "node": "libvirt-1-format", + "target": "b", + "bitmaps": [ + { + "node": "libvirt-1-format", + "name": "c" + } + ] + } + }, + { + "type": "block-dirty-bitmap-remove", + "data": { + "node": "libvirt-1-format", + "name": "c" + } + } +] diff --git a/tests/qemublocktestdata/checkpointdelete/basic-intermediate3-out.json b/tests/qemublocktestdata/checkpointdelete/basic-intermediate3-out.json new file mode 100644 index 0000000000..b5d85f43f0 --- /dev/null +++ b/tests/qemublocktestdata/checkpointdelete/basic-intermediate3-out.json @@ -0,0 +1,22 @@ +[ + { + "type": "block-dirty-bitmap-merge", + "data": { + "node": "libvirt-1-format", + "target": "c", + "bitmaps": [ + { + "node": "libvirt-1-format", + "name": "d" + } + ] + } + }, + { + "type": "block-dirty-bitmap-remove", + "data": { + "node": "libvirt-1-format", + "name": "d" + } + } +] -- 2.24.1
On 1/9/20 12:21 PM, Peter Krempa wrote:
Add all intermediate steps and deletion of the current checkpoint on a flat (single-image) disk image.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/qemublocktest.c | 4 +++ .../checkpointdelete/basic-current-out.json | 29 +++++++++++++++++++ .../basic-intermediate1-out.json | 22 ++++++++++++++ .../basic-intermediate2-out.json | 22 ++++++++++++++ .../basic-intermediate3-out.json | 22 ++++++++++++++ 5 files changed, 99 insertions(+) create mode 100644 tests/qemublocktestdata/checkpointdelete/basic-current-out.json create mode 100644 tests/qemublocktestdata/checkpointdelete/basic-intermediate1-out.json create mode 100644 tests/qemublocktestdata/checkpointdelete/basic-intermediate2-out.json create mode 100644 tests/qemublocktestdata/checkpointdelete/basic-intermediate3-out.json
+++ b/tests/qemublocktestdata/checkpointdelete/basic-current-out.json @@ -0,0 +1,29 @@ +[ + { + "type": "block-dirty-bitmap-enable", + "data": { + "node": "libvirt-1-format", + "name": "d" + } + }, + { + "type": "block-dirty-bitmap-merge", + "data": { + "node": "libvirt-1-format", + "target": "d", + "bitmaps": [ + { + "node": "libvirt-1-format", + "name": "current" + } + ] + } + }, + { + "type": "block-dirty-bitmap-remove", + "data": { + "node": "libvirt-1-format", + "name": "current" + } + } +]
Creates a rather large transaction (3 items, of which one itself takes an array of bitmaps), but that's par for the course. The test looks good. Reviewed-by: Eric Blake <eblake@redhat.com> -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Add a faked qemu output which would simulate scenario where libvirt would take a snapshot and checkpoint simultaneously. This is visible in libvirt-2-format node where bitmap 'c' appears, but bitmap 'b' which is active in the previous layer is not present. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/qemublocktest.c | 1 + .../snapshots-synthetic-checkpoint.json | 827 ++++++++++++++++++ .../bitmap/snapshots-synthetic-checkpoint.out | 13 + 3 files changed, 841 insertions(+) create mode 100644 tests/qemublocktestdata/bitmap/snapshots-synthetic-checkpoint.json create mode 100644 tests/qemublocktestdata/bitmap/snapshots-synthetic-checkpoint.out diff --git a/tests/qemublocktest.c b/tests/qemublocktest.c index 203e0f70b1..8a3f7da029 100644 --- a/tests/qemublocktest.c +++ b/tests/qemublocktest.c @@ -966,6 +966,7 @@ mymain(void) TEST_BITMAP_DETECT("basic"); TEST_BITMAP_DETECT("synthetic"); TEST_BITMAP_DETECT("snapshots"); + TEST_BITMAP_DETECT("snapshots-synthetic-checkpoint"); #define TEST_BACKUP_BITMAP_CALCULATE(testname, source, incrbackup, named) \ do { \ diff --git a/tests/qemublocktestdata/bitmap/snapshots-synthetic-checkpoint.json b/tests/qemublocktestdata/bitmap/snapshots-synthetic-checkpoint.json new file mode 100644 index 0000000000..25cc150d67 --- /dev/null +++ b/tests/qemublocktestdata/bitmap/snapshots-synthetic-checkpoint.json @@ -0,0 +1,827 @@ +[ + { + "iops_rd": 0, + "detect_zeroes": "off", + "image": { + "backing-image": { + "backing-image": { + "backing-image": { + "backing-image": { + "virtual-size": 10485760, + "filename": "/tmp/pull4.qcow2", + "cluster-size": 65536, + "format": "qcow2", + "actual-size": 208896, + "format-specific": { + "type": "qcow2", + "data": { + "compat": "1.1", + "lazy-refcounts": false, + "bitmaps": [ + { + "flags": [ + "auto" + ], + "name": "a", + "granularity": 65536 + } + ], + "refcount-bits": 16, + "corrupt": false + } + }, + "dirty-flag": false + }, + "backing-filename-format": "qcow2", + "virtual-size": 10485760, + "filename": "/tmp/pull4.1575911522", + "cluster-size": 65536, + "format": "qcow2", + "actual-size": 208896, + "format-specific": { + "type": "qcow2", + "data": { + "compat": "1.1", + "lazy-refcounts": false, + "bitmaps": [ + { + "flags": [ + "auto" + ], + "name": "a", + "granularity": 65536 + } + ], + "refcount-bits": 16, + "corrupt": false + } + }, + "full-backing-filename": "/tmp/pull4.qcow2", + "backing-filename": "/tmp/pull4.qcow2", + "dirty-flag": false + }, + "backing-filename-format": "qcow2", + "virtual-size": 10485760, + "filename": "/tmp/pull4.1575911527", + "cluster-size": 65536, + "format": "qcow2", + "actual-size": 217088, + "format-specific": { + "type": "qcow2", + "data": { + "compat": "1.1", + "lazy-refcounts": false, + "bitmaps": [ + { + "flags": [ + "auto" + ], + "name": "c", + "granularity": 65536 + }, + { + "flags": [ + + ], + "name": "b", + "granularity": 65536 + }, + { + "flags": [ + + ], + "name": "a", + "granularity": 65536 + } + ], + "refcount-bits": 16, + "corrupt": false + } + }, + "full-backing-filename": "/tmp/pull4.1575911522", + "backing-filename": "/tmp/pull4.1575911522", + "dirty-flag": false + }, + "backing-filename-format": "qcow2", + "virtual-size": 10485760, + "filename": "/tmp/pull4.1575911540", + "cluster-size": 65536, + "format": "qcow2", + "actual-size": 212992, + "format-specific": { + "type": "qcow2", + "data": { + "compat": "1.1", + "lazy-refcounts": false, + "bitmaps": [ + { + "flags": [ + "auto" + ], + "name": "d", + "granularity": 65536 + }, + { + "flags": [ + + ], + "name": "c", + "granularity": 65536 + } + ], + "refcount-bits": 16, + "corrupt": false + } + }, + "full-backing-filename": "/tmp/pull4.1575911527", + "backing-filename": "/tmp/pull4.1575911527", + "dirty-flag": false + }, + "backing-filename-format": "qcow2", + "virtual-size": 10485760, + "filename": "/tmp/pull4.1575911550", + "cluster-size": 65536, + "format": "qcow2", + "actual-size": 212992, + "format-specific": { + "type": "qcow2", + "data": { + "compat": "1.1", + "lazy-refcounts": false, + "bitmaps": [ + { + "flags": [ + "in-use", + "auto" + ], + "name": "current", + "granularity": 65536 + }, + { + "flags": [ + "in-use" + ], + "name": "d", + "granularity": 65536 + } + ], + "refcount-bits": 16, + "corrupt": false + } + }, + "full-backing-filename": "/tmp/pull4.1575911540", + "backing-filename": "/tmp/pull4.1575911540", + "dirty-flag": false + }, + "iops_wr": 0, + "ro": false, + "node-name": "libvirt-1-format", + "backing_file_depth": 4, + "drv": "qcow2", + "iops": 0, + "bps_wr": 0, + "write_threshold": 0, + "backing_file": "/tmp/pull4.1575911540", + "dirty-bitmaps": [ + { + "name": "d", + "recording": false, + "persistent": true, + "busy": false, + "status": "disabled", + "granularity": 65536, + "count": 0 + }, + { + "name": "current", + "recording": true, + "persistent": true, + "busy": false, + "status": "active", + "granularity": 65536, + "count": 0 + } + ], + "encrypted": false, + "bps": 0, + "bps_rd": 0, + "cache": { + "no-flush": false, + "direct": false, + "writeback": true + }, + "file": "/tmp/pull4.1575911550", + "encryption_key_missing": false + }, + { + "iops_rd": 0, + "detect_zeroes": "off", + "image": { + "virtual-size": 393728, + "filename": "/tmp/pull4.1575911550", + "format": "file", + "actual-size": 212992, + "dirty-flag": false + }, + "iops_wr": 0, + "ro": false, + "node-name": "libvirt-1-storage", + "backing_file_depth": 0, + "drv": "file", + "iops": 0, + "bps_wr": 0, + "write_threshold": 0, + "encrypted": false, + "bps": 0, + "bps_rd": 0, + "cache": { + "no-flush": false, + "direct": false, + "writeback": true + }, + "file": "/tmp/pull4.1575911550", + "encryption_key_missing": false + }, + { + "iops_rd": 0, + "detect_zeroes": "off", + "image": { + "backing-image": { + "backing-image": { + "backing-image": { + "virtual-size": 10485760, + "filename": "/tmp/pull4.qcow2", + "cluster-size": 65536, + "format": "qcow2", + "actual-size": 208896, + "format-specific": { + "type": "qcow2", + "data": { + "compat": "1.1", + "lazy-refcounts": false, + "bitmaps": [ + { + "flags": [ + "auto" + ], + "name": "a", + "granularity": 65536 + } + ], + "refcount-bits": 16, + "corrupt": false + } + }, + "dirty-flag": false + }, + "backing-filename-format": "qcow2", + "virtual-size": 10485760, + "filename": "/tmp/pull4.1575911522", + "cluster-size": 65536, + "format": "qcow2", + "actual-size": 208896, + "format-specific": { + "type": "qcow2", + "data": { + "compat": "1.1", + "lazy-refcounts": false, + "bitmaps": [ + { + "flags": [ + "auto" + ], + "name": "a", + "granularity": 65536 + } + ], + "refcount-bits": 16, + "corrupt": false + } + }, + "full-backing-filename": "/tmp/pull4.qcow2", + "backing-filename": "/tmp/pull4.qcow2", + "dirty-flag": false + }, + "backing-filename-format": "qcow2", + "virtual-size": 10485760, + "filename": "/tmp/pull4.1575911527", + "cluster-size": 65536, + "format": "qcow2", + "actual-size": 217088, + "format-specific": { + "type": "qcow2", + "data": { + "compat": "1.1", + "lazy-refcounts": false, + "bitmaps": [ + { + "flags": [ + "auto" + ], + "name": "c", + "granularity": 65536 + }, + { + "flags": [ + + ], + "name": "b", + "granularity": 65536 + }, + { + "flags": [ + + ], + "name": "a", + "granularity": 65536 + } + ], + "refcount-bits": 16, + "corrupt": false + } + }, + "full-backing-filename": "/tmp/pull4.1575911522", + "backing-filename": "/tmp/pull4.1575911522", + "dirty-flag": false + }, + "backing-filename-format": "qcow2", + "virtual-size": 10485760, + "filename": "/tmp/pull4.1575911540", + "cluster-size": 65536, + "format": "qcow2", + "actual-size": 212992, + "format-specific": { + "type": "qcow2", + "data": { + "compat": "1.1", + "lazy-refcounts": false, + "bitmaps": [ + { + "flags": [ + "auto" + ], + "name": "d", + "granularity": 65536 + }, + { + "flags": [ + + ], + "name": "c", + "granularity": 65536 + } + ], + "refcount-bits": 16, + "corrupt": false + } + }, + "full-backing-filename": "/tmp/pull4.1575911527", + "backing-filename": "/tmp/pull4.1575911527", + "dirty-flag": false + }, + "iops_wr": 0, + "ro": true, + "node-name": "libvirt-2-format", + "backing_file_depth": 3, + "drv": "qcow2", + "iops": 0, + "bps_wr": 0, + "write_threshold": 0, + "backing_file": "/tmp/pull4.1575911527", + "dirty-bitmaps": [ + { + "name": "c", + "recording": false, + "persistent": true, + "busy": false, + "status": "disabled", + "granularity": 65536, + "count": 0 + }, + { + "name": "d", + "recording": true, + "persistent": true, + "busy": false, + "status": "active", + "granularity": 65536, + "count": 0 + } + ], + "encrypted": false, + "bps": 0, + "bps_rd": 0, + "cache": { + "no-flush": false, + "direct": false, + "writeback": true + }, + "file": "/tmp/pull4.1575911540", + "encryption_key_missing": false + }, + { + "iops_rd": 0, + "detect_zeroes": "off", + "image": { + "virtual-size": 393728, + "filename": "/tmp/pull4.1575911540", + "format": "file", + "actual-size": 212992, + "dirty-flag": false + }, + "iops_wr": 0, + "ro": false, + "node-name": "libvirt-2-storage", + "backing_file_depth": 0, + "drv": "file", + "iops": 0, + "bps_wr": 0, + "write_threshold": 0, + "encrypted": false, + "bps": 0, + "bps_rd": 0, + "cache": { + "no-flush": false, + "direct": false, + "writeback": true + }, + "file": "/tmp/pull4.1575911540", + "encryption_key_missing": false + }, + { + "iops_rd": 0, + "detect_zeroes": "off", + "image": { + "backing-image": { + "backing-image": { + "virtual-size": 10485760, + "filename": "/tmp/pull4.qcow2", + "cluster-size": 65536, + "format": "qcow2", + "actual-size": 208896, + "format-specific": { + "type": "qcow2", + "data": { + "compat": "1.1", + "lazy-refcounts": false, + "bitmaps": [ + { + "flags": [ + "auto" + ], + "name": "a", + "granularity": 65536 + } + ], + "refcount-bits": 16, + "corrupt": false + } + }, + "dirty-flag": false + }, + "backing-filename-format": "qcow2", + "virtual-size": 10485760, + "filename": "/tmp/pull4.1575911522", + "cluster-size": 65536, + "format": "qcow2", + "actual-size": 208896, + "format-specific": { + "type": "qcow2", + "data": { + "compat": "1.1", + "lazy-refcounts": false, + "bitmaps": [ + { + "flags": [ + "auto" + ], + "name": "a", + "granularity": 65536 + } + ], + "refcount-bits": 16, + "corrupt": false + } + }, + "full-backing-filename": "/tmp/pull4.qcow2", + "backing-filename": "/tmp/pull4.qcow2", + "dirty-flag": false + }, + "backing-filename-format": "qcow2", + "virtual-size": 10485760, + "filename": "/tmp/pull4.1575911527", + "cluster-size": 65536, + "format": "qcow2", + "actual-size": 217088, + "format-specific": { + "type": "qcow2", + "data": { + "compat": "1.1", + "lazy-refcounts": false, + "bitmaps": [ + { + "flags": [ + "auto" + ], + "name": "c", + "granularity": 65536 + }, + { + "flags": [ + + ], + "name": "b", + "granularity": 65536 + }, + { + "flags": [ + + ], + "name": "a", + "granularity": 65536 + } + ], + "refcount-bits": 16, + "corrupt": false + } + }, + "full-backing-filename": "/tmp/pull4.1575911522", + "backing-filename": "/tmp/pull4.1575911522", + "dirty-flag": false + }, + "iops_wr": 0, + "ro": true, + "node-name": "libvirt-3-format", + "backing_file_depth": 2, + "drv": "qcow2", + "iops": 0, + "bps_wr": 0, + "write_threshold": 0, + "backing_file": "/tmp/pull4.1575911522", + "dirty-bitmaps": [ + { + "name": "a", + "recording": false, + "persistent": true, + "busy": false, + "status": "disabled", + "granularity": 65536, + "count": 0 + }, + { + "name": "b", + "recording": true, + "persistent": true, + "busy": false, + "status": "disabled", + "granularity": 65536, + "count": 0 + } + ], + "encrypted": false, + "bps": 0, + "bps_rd": 0, + "cache": { + "no-flush": false, + "direct": false, + "writeback": true + }, + "file": "/tmp/pull4.1575911527", + "encryption_key_missing": false + }, + { + "iops_rd": 0, + "detect_zeroes": "off", + "image": { + "virtual-size": 459264, + "filename": "/tmp/pull4.1575911527", + "format": "file", + "actual-size": 217088, + "dirty-flag": false + }, + "iops_wr": 0, + "ro": false, + "node-name": "libvirt-3-storage", + "backing_file_depth": 0, + "drv": "file", + "iops": 0, + "bps_wr": 0, + "write_threshold": 0, + "encrypted": false, + "bps": 0, + "bps_rd": 0, + "cache": { + "no-flush": false, + "direct": false, + "writeback": true + }, + "file": "/tmp/pull4.1575911527", + "encryption_key_missing": false + }, + { + "iops_rd": 0, + "detect_zeroes": "off", + "image": { + "backing-image": { + "virtual-size": 10485760, + "filename": "/tmp/pull4.qcow2", + "cluster-size": 65536, + "format": "qcow2", + "actual-size": 208896, + "format-specific": { + "type": "qcow2", + "data": { + "compat": "1.1", + "lazy-refcounts": false, + "bitmaps": [ + { + "flags": [ + "auto" + ], + "name": "a", + "granularity": 65536 + } + ], + "refcount-bits": 16, + "corrupt": false + } + }, + "dirty-flag": false + }, + "backing-filename-format": "qcow2", + "virtual-size": 10485760, + "filename": "/tmp/pull4.1575911522", + "cluster-size": 65536, + "format": "qcow2", + "actual-size": 208896, + "format-specific": { + "type": "qcow2", + "data": { + "compat": "1.1", + "lazy-refcounts": false, + "bitmaps": [ + { + "flags": [ + "auto" + ], + "name": "a", + "granularity": 65536 + } + ], + "refcount-bits": 16, + "corrupt": false + } + }, + "full-backing-filename": "/tmp/pull4.qcow2", + "backing-filename": "/tmp/pull4.qcow2", + "dirty-flag": false + }, + "iops_wr": 0, + "ro": true, + "node-name": "libvirt-4-format", + "backing_file_depth": 1, + "drv": "qcow2", + "iops": 0, + "bps_wr": 0, + "write_threshold": 0, + "backing_file": "/tmp/pull4.qcow2", + "dirty-bitmaps": [ + { + "name": "a", + "recording": true, + "persistent": true, + "busy": false, + "status": "active", + "granularity": 65536, + "count": 0 + } + ], + "encrypted": false, + "bps": 0, + "bps_rd": 0, + "cache": { + "no-flush": false, + "direct": false, + "writeback": true + }, + "file": "/tmp/pull4.1575911522", + "encryption_key_missing": false + }, + { + "iops_rd": 0, + "detect_zeroes": "off", + "image": { + "virtual-size": 328192, + "filename": "/tmp/pull4.1575911522", + "format": "file", + "actual-size": 208896, + "dirty-flag": false + }, + "iops_wr": 0, + "ro": false, + "node-name": "libvirt-4-storage", + "backing_file_depth": 0, + "drv": "file", + "iops": 0, + "bps_wr": 0, + "write_threshold": 0, + "encrypted": false, + "bps": 0, + "bps_rd": 0, + "cache": { + "no-flush": false, + "direct": false, + "writeback": true + }, + "file": "/tmp/pull4.1575911522", + "encryption_key_missing": false + }, + { + "iops_rd": 0, + "detect_zeroes": "off", + "image": { + "virtual-size": 10485760, + "filename": "/tmp/pull4.qcow2", + "cluster-size": 65536, + "format": "qcow2", + "actual-size": 208896, + "format-specific": { + "type": "qcow2", + "data": { + "compat": "1.1", + "lazy-refcounts": false, + "bitmaps": [ + { + "flags": [ + "auto" + ], + "name": "a", + "granularity": 65536 + } + ], + "refcount-bits": 16, + "corrupt": false + } + }, + "dirty-flag": false + }, + "iops_wr": 0, + "ro": true, + "node-name": "libvirt-5-format", + "backing_file_depth": 0, + "drv": "qcow2", + "iops": 0, + "bps_wr": 0, + "write_threshold": 0, + "dirty-bitmaps": [ + { + "name": "a", + "recording": true, + "persistent": true, + "busy": false, + "status": "active", + "granularity": 65536, + "count": 0 + } + ], + "encrypted": false, + "bps": 0, + "bps_rd": 0, + "cache": { + "no-flush": false, + "direct": false, + "writeback": true + }, + "file": "/tmp/pull4.qcow2", + "encryption_key_missing": false + }, + { + "iops_rd": 0, + "detect_zeroes": "off", + "image": { + "virtual-size": 328192, + "filename": "/tmp/pull4.qcow2", + "format": "file", + "actual-size": 208896, + "dirty-flag": false + }, + "iops_wr": 0, + "ro": false, + "node-name": "libvirt-5-storage", + "backing_file_depth": 0, + "drv": "file", + "iops": 0, + "bps_wr": 0, + "write_threshold": 0, + "encrypted": false, + "bps": 0, + "bps_rd": 0, + "cache": { + "no-flush": false, + "direct": false, + "writeback": true + }, + "file": "/tmp/pull4.qcow2", + "encryption_key_missing": false + } +] diff --git a/tests/qemublocktestdata/bitmap/snapshots-synthetic-checkpoint.out b/tests/qemublocktestdata/bitmap/snapshots-synthetic-checkpoint.out new file mode 100644 index 0000000000..0270657001 --- /dev/null +++ b/tests/qemublocktestdata/bitmap/snapshots-synthetic-checkpoint.out @@ -0,0 +1,13 @@ +libvirt-1-format: + d: record:0 busy:0 persist:1 inconsist:0 gran:65536 dirty:0 + current: record:1 busy:0 persist:1 inconsist:0 gran:65536 dirty:0 +libvirt-2-format: + c: record:0 busy:0 persist:1 inconsist:0 gran:65536 dirty:0 + d: record:1 busy:0 persist:1 inconsist:0 gran:65536 dirty:0 +libvirt-3-format: + a: record:0 busy:0 persist:1 inconsist:0 gran:65536 dirty:0 + b: record:1 busy:0 persist:1 inconsist:0 gran:65536 dirty:0 +libvirt-4-format: + a: record:1 busy:0 persist:1 inconsist:0 gran:65536 dirty:0 +libvirt-5-format: + a: record:1 busy:0 persist:1 inconsist:0 gran:65536 dirty:0 -- 2.24.1
On 1/9/20 12:21 PM, Peter Krempa wrote:
Add a faked qemu output which would simulate scenario where libvirt would take a snapshot and checkpoint simultaneously. This is visible in libvirt-2-format node where bitmap 'c' appears, but bitmap 'b' which is active in the previous layer is not present.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/qemublocktest.c | 1 + .../snapshots-synthetic-checkpoint.json | 827 ++++++++++++++++++
Hmm - proof why we want qemu to add the optional flat parameter to avoid the O(n^2) growth of output as backing chains get longer.
.../bitmap/snapshots-synthetic-checkpoint.out | 13 + 3 files changed, 841 insertions(+) create mode 100644 tests/qemublocktestdata/bitmap/snapshots-synthetic-checkpoint.json create mode 100644 tests/qemublocktestdata/bitmap/snapshots-synthetic-checkpoint.out
The test itself looks sane. Reviewed-by: Eric Blake <eblake@redhat.com> -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Allow deleting of checkpoints when snapshots were created along. The code tracks and modifies the checkpoint list so that backups can still be taken with such a backing chain. This unfortunately requires to rename few bitmaps (by copying and deleting them) in some cases. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_checkpoint.c | 112 ++++++++++++++++++++++++++++--------- src/qemu/qemu_checkpoint.h | 5 +- tests/qemublocktest.c | 34 +++++++---- 3 files changed, 111 insertions(+), 40 deletions(-) diff --git a/src/qemu/qemu_checkpoint.c b/src/qemu/qemu_checkpoint.c index e75cdd0458..087a740cf8 100644 --- a/src/qemu/qemu_checkpoint.c +++ b/src/qemu/qemu_checkpoint.c @@ -24,6 +24,7 @@ #include "qemu_capabilities.h" #include "qemu_monitor.h" #include "qemu_domain.h" +#include "qemu_block.h" #include "virerror.h" #include "virlog.h" @@ -150,39 +151,92 @@ qemuCheckpointFindActiveDiskInParent(virDomainObjPtr vm, int qemuCheckpointDiscardDiskBitmaps(virStorageSourcePtr src, + virHashTablePtr blockNamedNodeData, const char *delbitmap, const char *parentbitmap, - bool chkcurrent, - virJSONValuePtr actions) + virJSONValuePtr actions, + const char *diskdst) { - if (parentbitmap) { - g_autoptr(virJSONValue) arr = NULL; + virStorageSourcePtr n = src; - if (!(arr = virJSONValueNewArray())) - return -1; + /* find the backing chain entry with bitmap named '@bitmap' */ + while (n) { + qemuBlockNamedNodeDataBitmapPtr tmp; - if (qemuMonitorTransactionBitmapMergeSourceAddBitmap(arr, - src->nodeformat, - delbitmap) < 0) - return -1; + if ((tmp = qemuBlockNamedNodeDataGetBitmapByName(blockNamedNodeData, + n, delbitmap))) { + break; + } + + n = n->backingStore; + } + + if (!n) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("bitmap '%s' not found in backing chain of '%s'"), + delbitmap, diskdst); + return -1; + } - if (chkcurrent) { - if (qemuMonitorTransactionBitmapEnable(actions, - src->nodeformat, - parentbitmap) < 0) + while (n) { + qemuBlockNamedNodeDataBitmapPtr srcbitmap; + + if (!(srcbitmap = qemuBlockNamedNodeDataGetBitmapByName(blockNamedNodeData, + n, delbitmap))) + break; + + /* For the actual checkpoint deletion we will merge any bitmap into the + * bitmap of the parent checkpoint (@mergebitmap) or for any image + * where the parent checkpoint bitmap is not present we must rename + * the bitmap of the deleted checkpoint into the bitmap of the parent + * checkpoint as qemu can't currently take the allocation map and turn + * it into a bitmap and thus we wouldn't be able to do a backup. */ + if (parentbitmap) { + qemuBlockNamedNodeDataBitmapPtr dstbitmap; + g_autoptr(virJSONValue) arr = NULL; + + dstbitmap = qemuBlockNamedNodeDataGetBitmapByName(blockNamedNodeData, + n, parentbitmap); + + if (dstbitmap) { + if (srcbitmap->recording && !dstbitmap->recording) { + if (qemuMonitorTransactionBitmapEnable(actions, + n->nodeformat, + dstbitmap->name) < 0) + return -1; + } + + } else { + if (qemuMonitorTransactionBitmapAdd(actions, + n->nodeformat, + parentbitmap, + true, + !srcbitmap->recording, + srcbitmap->granularity) < 0) + return -1; + } + + if (!(arr = virJSONValueNewArray())) + return -1; + + if (qemuMonitorTransactionBitmapMergeSourceAddBitmap(arr, + n->nodeformat, + srcbitmap->name) < 0) + return -1; + + if (qemuMonitorTransactionBitmapMerge(actions, + n->nodeformat, + parentbitmap, &arr) < 0) return -1; } - if (qemuMonitorTransactionBitmapMerge(actions, - src->nodeformat, - parentbitmap, &arr) < 0) + if (qemuMonitorTransactionBitmapRemove(actions, + n->nodeformat, + srcbitmap->name) < 0) return -1; - } - if (qemuMonitorTransactionBitmapRemove(actions, - src->nodeformat, - delbitmap) < 0) - return -1; + n = n->backingStore; + } return 0; } @@ -191,11 +245,11 @@ qemuCheckpointDiscardDiskBitmaps(virStorageSourcePtr src, static int qemuCheckpointDiscardBitmaps(virDomainObjPtr vm, virDomainCheckpointDefPtr chkdef, - bool chkcurrent, virDomainMomentObjPtr parent) { qemuDomainObjPrivatePtr priv = vm->privateData; virQEMUDriverPtr driver = priv->driver; + g_autoptr(virHashTable) blockNamedNodeData = NULL; int rc; g_autoptr(virJSONValue) actions = NULL; size_t i; @@ -203,6 +257,11 @@ qemuCheckpointDiscardBitmaps(virDomainObjPtr vm, if (!(actions = virJSONValueNewArray())) return -1; + qemuDomainObjEnterMonitor(driver, vm); + blockNamedNodeData = qemuMonitorBlockGetNamedNodeData(priv->mon); + if (qemuDomainObjExitMonitor(priv->driver, vm) < 0 || !blockNamedNodeData) + return -1; + for (i = 0; i < chkdef->ndisks; i++) { virDomainCheckpointDiskDef *chkdisk = &chkdef->disks[i]; virDomainDiskDefPtr domdisk = virDomainDiskByTarget(vm->def, chkdisk->name); @@ -223,8 +282,9 @@ qemuCheckpointDiscardBitmaps(virDomainObjPtr vm, chkdisk->name))) parentbitmap = parentchkdisk->name; - if (qemuCheckpointDiscardDiskBitmaps(domdisk->src, chkdisk->bitmap, - parentbitmap, chkcurrent, actions) < 0) + if (qemuCheckpointDiscardDiskBitmaps(domdisk->src, blockNamedNodeData, + chkdisk->bitmap, parentbitmap, + actions, domdisk->dst) < 0) return -1; } @@ -262,7 +322,7 @@ qemuCheckpointDiscard(virQEMUDriverPtr driver, virDomainCheckpointDefPtr chkdef = virDomainCheckpointObjGetDef(chk); parent = virDomainCheckpointFindByName(vm->checkpoints, chk->def->parent_name); - if (qemuCheckpointDiscardBitmaps(vm, chkdef, chkcurrent, parent) < 0) + if (qemuCheckpointDiscardBitmaps(vm, chkdef, parent) < 0) return -1; } diff --git a/src/qemu/qemu_checkpoint.h b/src/qemu/qemu_checkpoint.h index 85fd453d50..976b1eed0f 100644 --- a/src/qemu/qemu_checkpoint.h +++ b/src/qemu/qemu_checkpoint.h @@ -74,7 +74,8 @@ qemuCheckpointRollbackMetadata(virDomainObjPtr vm, int qemuCheckpointDiscardDiskBitmaps(virStorageSourcePtr src, + virHashTablePtr blockNamedNodeData, const char *delbitmap, const char *parentbitmap, - bool chkcurrent, - virJSONValuePtr actions); + virJSONValuePtr actions, + const char *diskdst); diff --git a/tests/qemublocktest.c b/tests/qemublocktest.c index 8a3f7da029..293757b8d4 100644 --- a/tests/qemublocktest.c +++ b/tests/qemublocktest.c @@ -704,6 +704,7 @@ struct testQemuCheckpointDeleteMergeData { virStorageSourcePtr chain; const char *deletebitmap; const char *parentbitmap; + const char *nodedatafile; }; @@ -714,22 +715,30 @@ testQemuCheckpointDeleteMerge(const void *opaque) g_autofree char *actual = NULL; g_autofree char *expectpath = NULL; g_autoptr(virJSONValue) actions = NULL; - bool currentcheckpoint; + g_autoptr(virJSONValue) nodedatajson = NULL; + g_autoptr(virHashTable) nodedata = NULL; expectpath = g_strdup_printf("%s/%s%s-out.json", abs_srcdir, checkpointDeletePrefix, data->name); - if (!(actions = virJSONValueNewArray())) + if (!(nodedatajson = virTestLoadFileJSON(bitmapDetectPrefix, data->nodedatafile, + ".json", NULL))) return -1; - /* hack to get the 'current' state until the function stops accepting it */ - currentcheckpoint = STREQ("current", data->deletebitmap); + if (!(nodedata = qemuMonitorJSONBlockGetNamedNodeDataJSON(nodedatajson))) { + VIR_TEST_VERBOSE("failed to load nodedata JSON\n"); + return -1; + } + + if (!(actions = virJSONValueNewArray())) + return -1; if (qemuCheckpointDiscardDiskBitmaps(data->chain, + nodedata, data->deletebitmap, data->parentbitmap, - currentcheckpoint, - actions) < 0) { + actions, + "testdisk") < 0) { VIR_TEST_VERBOSE("failed to generate checkpoint delete transaction\n"); return -1; } @@ -988,22 +997,23 @@ mymain(void) TEST_BACKUP_BITMAP_CALCULATE("snapshot-intermediate", bitmapSourceChain, "d", "snapshots"); TEST_BACKUP_BITMAP_CALCULATE("snapshot-deep", bitmapSourceChain, "a", "snapshots"); -#define TEST_CHECKPOINT_DELETE_MERGE(testname, delbmp, parbmp) \ +#define TEST_CHECKPOINT_DELETE_MERGE(testname, delbmp, parbmp, named) \ do { \ checkpointdeletedata.name = testname; \ checkpointdeletedata.chain = bitmapSourceChain; \ checkpointdeletedata.deletebitmap = delbmp; \ checkpointdeletedata.parentbitmap = parbmp; \ + checkpointdeletedata.nodedatafile = named; \ if (virTestRun("checkpoint delete " testname, \ testQemuCheckpointDeleteMerge, &checkpointdeletedata) < 0) \ ret = -1; \ } while (0) - TEST_CHECKPOINT_DELETE_MERGE("basic-noparent", "a", NULL); - TEST_CHECKPOINT_DELETE_MERGE("basic-intermediate1", "b", "a"); - TEST_CHECKPOINT_DELETE_MERGE("basic-intermediate2", "c", "b"); - TEST_CHECKPOINT_DELETE_MERGE("basic-intermediate3", "d", "c"); - TEST_CHECKPOINT_DELETE_MERGE("basic-current", "current", "d"); + TEST_CHECKPOINT_DELETE_MERGE("basic-noparent", "a", NULL, "basic"); + TEST_CHECKPOINT_DELETE_MERGE("basic-intermediate1", "b", "a", "basic"); + TEST_CHECKPOINT_DELETE_MERGE("basic-intermediate2", "c", "b", "basic"); + TEST_CHECKPOINT_DELETE_MERGE("basic-intermediate3", "d", "c", "basic"); + TEST_CHECKPOINT_DELETE_MERGE("basic-current", "current", "d", "basic"); cleanup: virHashFree(diskxmljsondata.schema); -- 2.24.1
Add test cases for merging various pairs of bitmaps when snapshots were created together with checkpoints. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/qemublocktest.c | 6 +++ .../snapshots-current-out.json | 29 +++++++++++ .../snapshots-intermediate1-out.json | 22 ++++++++ .../snapshots-intermediate2-out.json | 52 +++++++++++++++++++ .../snapshots-intermediate3-out.json | 52 +++++++++++++++++++ .../snapshots-noparent-out.json | 23 ++++++++ 6 files changed, 184 insertions(+) create mode 100644 tests/qemublocktestdata/checkpointdelete/snapshots-current-out.json create mode 100644 tests/qemublocktestdata/checkpointdelete/snapshots-intermediate1-out.json create mode 100644 tests/qemublocktestdata/checkpointdelete/snapshots-intermediate2-out.json create mode 100644 tests/qemublocktestdata/checkpointdelete/snapshots-intermediate3-out.json create mode 100644 tests/qemublocktestdata/checkpointdelete/snapshots-noparent-out.json diff --git a/tests/qemublocktest.c b/tests/qemublocktest.c index 293757b8d4..b8a052b19b 100644 --- a/tests/qemublocktest.c +++ b/tests/qemublocktest.c @@ -1015,6 +1015,12 @@ mymain(void) TEST_CHECKPOINT_DELETE_MERGE("basic-intermediate3", "d", "c", "basic"); TEST_CHECKPOINT_DELETE_MERGE("basic-current", "current", "d", "basic"); + TEST_CHECKPOINT_DELETE_MERGE("snapshots-noparent", "a", NULL, "snapshots"); + TEST_CHECKPOINT_DELETE_MERGE("snapshots-intermediate1", "b", "a", "snapshots"); + TEST_CHECKPOINT_DELETE_MERGE("snapshots-intermediate2", "c", "b", "snapshots"); + TEST_CHECKPOINT_DELETE_MERGE("snapshots-intermediate3", "d", "c", "snapshots"); + TEST_CHECKPOINT_DELETE_MERGE("snapshots-current", "current", "d", "snapshots"); + cleanup: virHashFree(diskxmljsondata.schema); qemuTestDriverFree(&driver); diff --git a/tests/qemublocktestdata/checkpointdelete/snapshots-current-out.json b/tests/qemublocktestdata/checkpointdelete/snapshots-current-out.json new file mode 100644 index 0000000000..1b607567e8 --- /dev/null +++ b/tests/qemublocktestdata/checkpointdelete/snapshots-current-out.json @@ -0,0 +1,29 @@ +[ + { + "type": "block-dirty-bitmap-enable", + "data": { + "node": "libvirt-1-format", + "name": "d" + } + }, + { + "type": "block-dirty-bitmap-merge", + "data": { + "node": "libvirt-1-format", + "target": "d", + "bitmaps": [ + { + "node": "libvirt-1-format", + "name": "current" + } + ] + } + }, + { + "type": "block-dirty-bitmap-remove", + "data": { + "node": "libvirt-1-format", + "name": "current" + } + } +] diff --git a/tests/qemublocktestdata/checkpointdelete/snapshots-intermediate1-out.json b/tests/qemublocktestdata/checkpointdelete/snapshots-intermediate1-out.json new file mode 100644 index 0000000000..29fefeea63 --- /dev/null +++ b/tests/qemublocktestdata/checkpointdelete/snapshots-intermediate1-out.json @@ -0,0 +1,22 @@ +[ + { + "type": "block-dirty-bitmap-merge", + "data": { + "node": "libvirt-3-format", + "target": "a", + "bitmaps": [ + { + "node": "libvirt-3-format", + "name": "b" + } + ] + } + }, + { + "type": "block-dirty-bitmap-remove", + "data": { + "node": "libvirt-3-format", + "name": "b" + } + } +] diff --git a/tests/qemublocktestdata/checkpointdelete/snapshots-intermediate2-out.json b/tests/qemublocktestdata/checkpointdelete/snapshots-intermediate2-out.json new file mode 100644 index 0000000000..ca5627269c --- /dev/null +++ b/tests/qemublocktestdata/checkpointdelete/snapshots-intermediate2-out.json @@ -0,0 +1,52 @@ +[ + { + "type": "block-dirty-bitmap-add", + "data": { + "node": "libvirt-2-format", + "name": "b", + "persistent": true, + "disabled": true, + "granularity": 65536 + } + }, + { + "type": "block-dirty-bitmap-merge", + "data": { + "node": "libvirt-2-format", + "target": "b", + "bitmaps": [ + { + "node": "libvirt-2-format", + "name": "c" + } + ] + } + }, + { + "type": "block-dirty-bitmap-remove", + "data": { + "node": "libvirt-2-format", + "name": "c" + } + }, + { + "type": "block-dirty-bitmap-merge", + "data": { + "node": "libvirt-3-format", + "target": "b", + "bitmaps": [ + { + "node": "libvirt-3-format", + "name": "c" + } + ] + } + }, + { + "type": "block-dirty-bitmap-remove", + "data": { + "node": "libvirt-3-format", + "name": "c" + } + } +] diff --git a/tests/qemublocktestdata/checkpointdelete/snapshots-intermediate3-out.json b/tests/qemublocktestdata/checkpointdelete/snapshots-intermediate3-out.json new file mode 100644 index 0000000000..ee7ae97fdb --- /dev/null +++ b/tests/qemublocktestdata/checkpointdelete/snapshots-intermediate3-out.json @@ -0,0 +1,52 @@ +[ + { + "type": "block-dirty-bitmap-add", + "data": { + "node": "libvirt-1-format", + "name": "c", + "persistent": true, + "disabled": true, + "granularity": 65536 + } + }, + { + "type": "block-dirty-bitmap-merge", + "data": { + "node": "libvirt-1-format", + "target": "c", + "bitmaps": [ + { + "node": "libvirt-1-format", + "name": "d" + } + ] + } + }, + { + "type": "block-dirty-bitmap-remove", + "data": { + "node": "libvirt-1-format", + "name": "d" + } + }, + { + "type": "block-dirty-bitmap-merge", + "data": { + "node": "libvirt-2-format", + "target": "c", + "bitmaps": [ + { + "node": "libvirt-2-format", + "name": "d" + } + ] + } + }, + { + "type": "block-dirty-bitmap-remove", + "data": { + "node": "libvirt-2-format", + "name": "d" + } + } +] diff --git a/tests/qemublocktestdata/checkpointdelete/snapshots-noparent-out.json b/tests/qemublocktestdata/checkpointdelete/snapshots-noparent-out.json new file mode 100644 index 0000000000..45a84b47c2 --- /dev/null +++ b/tests/qemublocktestdata/checkpointdelete/snapshots-noparent-out.json @@ -0,0 +1,23 @@ +[ + { + "type": "block-dirty-bitmap-remove", + "data": { + "node": "libvirt-3-format", + "name": "a" + } + }, + { + "type": "block-dirty-bitmap-remove", + "data": { + "node": "libvirt-4-format", + "name": "a" + } + }, + { + "type": "block-dirty-bitmap-remove", + "data": { + "node": "libvirt-5-format", + "name": "a" + } + } +] -- 2.24.1
Use the synthetic test data to verify that the algorithm correctly picks bitmaps to merge when the bitmap is changed along with the image itself. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/qemublocktest.c | 7 +++ .../snapshots-intermediate2-out.json | 7 +++ .../snapshots-intermediate3-out.json | 7 +++ ...hots-synthetic-checkpoint-current-out.json | 29 +++++++++ ...ynthetic-checkpoint-intermediate1-out.json | 29 +++++++++ ...ynthetic-checkpoint-intermediate2-out.json | 32 ++++++++++ ...ynthetic-checkpoint-intermediate3-out.json | 59 +++++++++++++++++++ ...ots-synthetic-checkpoint-noparent-out.json | 23 ++++++++ 8 files changed, 193 insertions(+) create mode 100644 tests/qemublocktestdata/checkpointdelete/snapshots-synthetic-checkpoint-current-out.json create mode 100644 tests/qemublocktestdata/checkpointdelete/snapshots-synthetic-checkpoint-intermediate1-out.json create mode 100644 tests/qemublocktestdata/checkpointdelete/snapshots-synthetic-checkpoint-intermediate2-out.json create mode 100644 tests/qemublocktestdata/checkpointdelete/snapshots-synthetic-checkpoint-intermediate3-out.json create mode 100644 tests/qemublocktestdata/checkpointdelete/snapshots-synthetic-checkpoint-noparent-out.json diff --git a/tests/qemublocktest.c b/tests/qemublocktest.c index b8a052b19b..c2c7507e20 100644 --- a/tests/qemublocktest.c +++ b/tests/qemublocktest.c @@ -1021,6 +1021,13 @@ mymain(void) TEST_CHECKPOINT_DELETE_MERGE("snapshots-intermediate3", "d", "c", "snapshots"); TEST_CHECKPOINT_DELETE_MERGE("snapshots-current", "current", "d", "snapshots"); + TEST_CHECKPOINT_DELETE_MERGE("snapshots-synthetic-checkpoint-noparent", "a", NULL, "snapshots-synthetic-checkpoint"); + TEST_CHECKPOINT_DELETE_MERGE("snapshots-synthetic-checkpoint-intermediate1", "b", "a", "snapshots-synthetic-checkpoint"); + TEST_CHECKPOINT_DELETE_MERGE("snapshots-synthetic-checkpoint-intermediate2", "c", "b", "snapshots-synthetic-checkpoint"); + TEST_CHECKPOINT_DELETE_MERGE("snapshots-synthetic-checkpoint-intermediate3", "d", "c", "snapshots-synthetic-checkpoint"); + TEST_CHECKPOINT_DELETE_MERGE("snapshots-synthetic-checkpoint-current", "current", "d", "snapshots-synthetic-checkpoint"); + + cleanup: virHashFree(diskxmljsondata.schema); qemuTestDriverFree(&driver); diff --git a/tests/qemublocktestdata/checkpointdelete/snapshots-intermediate2-out.json b/tests/qemublocktestdata/checkpointdelete/snapshots-intermediate2-out.json index ca5627269c..4da21a9df7 100644 --- a/tests/qemublocktestdata/checkpointdelete/snapshots-intermediate2-out.json +++ b/tests/qemublocktestdata/checkpointdelete/snapshots-intermediate2-out.json @@ -29,6 +29,13 @@ "name": "c" } }, + { + "type": "block-dirty-bitmap-enable", + "data": { + "node": "libvirt-3-format", + "name": "b" + } + }, { "type": "block-dirty-bitmap-merge", "data": { diff --git a/tests/qemublocktestdata/checkpointdelete/snapshots-intermediate3-out.json b/tests/qemublocktestdata/checkpointdelete/snapshots-intermediate3-out.json index ee7ae97fdb..dc87dd60b8 100644 --- a/tests/qemublocktestdata/checkpointdelete/snapshots-intermediate3-out.json +++ b/tests/qemublocktestdata/checkpointdelete/snapshots-intermediate3-out.json @@ -29,6 +29,13 @@ "name": "d" } }, + { + "type": "block-dirty-bitmap-enable", + "data": { + "node": "libvirt-2-format", + "name": "c" + } + }, { "type": "block-dirty-bitmap-merge", "data": { diff --git a/tests/qemublocktestdata/checkpointdelete/snapshots-synthetic-checkpoint-current-out.json b/tests/qemublocktestdata/checkpointdelete/snapshots-synthetic-checkpoint-current-out.json new file mode 100644 index 0000000000..1b607567e8 --- /dev/null +++ b/tests/qemublocktestdata/checkpointdelete/snapshots-synthetic-checkpoint-current-out.json @@ -0,0 +1,29 @@ +[ + { + "type": "block-dirty-bitmap-enable", + "data": { + "node": "libvirt-1-format", + "name": "d" + } + }, + { + "type": "block-dirty-bitmap-merge", + "data": { + "node": "libvirt-1-format", + "target": "d", + "bitmaps": [ + { + "node": "libvirt-1-format", + "name": "current" + } + ] + } + }, + { + "type": "block-dirty-bitmap-remove", + "data": { + "node": "libvirt-1-format", + "name": "current" + } + } +] diff --git a/tests/qemublocktestdata/checkpointdelete/snapshots-synthetic-checkpoint-intermediate1-out.json b/tests/qemublocktestdata/checkpointdelete/snapshots-synthetic-checkpoint-intermediate1-out.json new file mode 100644 index 0000000000..e979691e6f --- /dev/null +++ b/tests/qemublocktestdata/checkpointdelete/snapshots-synthetic-checkpoint-intermediate1-out.json @@ -0,0 +1,29 @@ +[ + { + "type": "block-dirty-bitmap-enable", + "data": { + "node": "libvirt-3-format", + "name": "a" + } + }, + { + "type": "block-dirty-bitmap-merge", + "data": { + "node": "libvirt-3-format", + "target": "a", + "bitmaps": [ + { + "node": "libvirt-3-format", + "name": "b" + } + ] + } + }, + { + "type": "block-dirty-bitmap-remove", + "data": { + "node": "libvirt-3-format", + "name": "b" + } + } +] diff --git a/tests/qemublocktestdata/checkpointdelete/snapshots-synthetic-checkpoint-intermediate2-out.json b/tests/qemublocktestdata/checkpointdelete/snapshots-synthetic-checkpoint-intermediate2-out.json new file mode 100644 index 0000000000..e82098918a --- /dev/null +++ b/tests/qemublocktestdata/checkpointdelete/snapshots-synthetic-checkpoint-intermediate2-out.json @@ -0,0 +1,32 @@ +[ + { + "type": "block-dirty-bitmap-add", + "data": { + "node": "libvirt-2-format", + "name": "b", + "persistent": true, + "disabled": true, + "granularity": 65536 + } + }, + { + "type": "block-dirty-bitmap-merge", + "data": { + "node": "libvirt-2-format", + "target": "b", + "bitmaps": [ + { + "node": "libvirt-2-format", + "name": "c" + } + ] + } + }, + { + "type": "block-dirty-bitmap-remove", + "data": { + "node": "libvirt-2-format", + "name": "c" + } + } +] diff --git a/tests/qemublocktestdata/checkpointdelete/snapshots-synthetic-checkpoint-intermediate3-out.json b/tests/qemublocktestdata/checkpointdelete/snapshots-synthetic-checkpoint-intermediate3-out.json new file mode 100644 index 0000000000..dc87dd60b8 --- /dev/null +++ b/tests/qemublocktestdata/checkpointdelete/snapshots-synthetic-checkpoint-intermediate3-out.json @@ -0,0 +1,59 @@ +[ + { + "type": "block-dirty-bitmap-add", + "data": { + "node": "libvirt-1-format", + "name": "c", + "persistent": true, + "disabled": true, + "granularity": 65536 + } + }, + { + "type": "block-dirty-bitmap-merge", + "data": { + "node": "libvirt-1-format", + "target": "c", + "bitmaps": [ + { + "node": "libvirt-1-format", + "name": "d" + } + ] + } + }, + { + "type": "block-dirty-bitmap-remove", + "data": { + "node": "libvirt-1-format", + "name": "d" + } + }, + { + "type": "block-dirty-bitmap-enable", + "data": { + "node": "libvirt-2-format", + "name": "c" + } + }, + { + "type": "block-dirty-bitmap-merge", + "data": { + "node": "libvirt-2-format", + "target": "c", + "bitmaps": [ + { + "node": "libvirt-2-format", + "name": "d" + } + ] + } + }, + { + "type": "block-dirty-bitmap-remove", + "data": { + "node": "libvirt-2-format", + "name": "d" + } + } +] diff --git a/tests/qemublocktestdata/checkpointdelete/snapshots-synthetic-checkpoint-noparent-out.json b/tests/qemublocktestdata/checkpointdelete/snapshots-synthetic-checkpoint-noparent-out.json new file mode 100644 index 0000000000..45a84b47c2 --- /dev/null +++ b/tests/qemublocktestdata/checkpointdelete/snapshots-synthetic-checkpoint-noparent-out.json @@ -0,0 +1,23 @@ +[ + { + "type": "block-dirty-bitmap-remove", + "data": { + "node": "libvirt-3-format", + "name": "a" + } + }, + { + "type": "block-dirty-bitmap-remove", + "data": { + "node": "libvirt-4-format", + "name": "a" + } + }, + { + "type": "block-dirty-bitmap-remove", + "data": { + "node": "libvirt-5-format", + "name": "a" + } + } +] -- 2.24.1
Allow qemu access to modify backing files in case when we want to delete a checkpoint. This patch adds tracking of which images need to be relabelled when calculating the transaction, the code to relabel them and rollback. To verify that stuff works we also output the list of images to relabel into the test case output files in qemublocktest. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_checkpoint.c | 33 ++++++++++++++++--- src/qemu/qemu_checkpoint.h | 3 +- tests/qemublocktest.c | 19 +++++++++-- .../snapshots-intermediate1-out.json | 2 ++ .../snapshots-intermediate2-out.json | 3 ++ .../snapshots-intermediate3-out.json | 2 ++ .../snapshots-noparent-out.json | 4 +++ ...ynthetic-checkpoint-intermediate1-out.json | 2 ++ ...ynthetic-checkpoint-intermediate2-out.json | 2 ++ ...ynthetic-checkpoint-intermediate3-out.json | 2 ++ ...ots-synthetic-checkpoint-noparent-out.json | 4 +++ 11 files changed, 69 insertions(+), 7 deletions(-) diff --git a/src/qemu/qemu_checkpoint.c b/src/qemu/qemu_checkpoint.c index 087a740cf8..13bd6653b7 100644 --- a/src/qemu/qemu_checkpoint.c +++ b/src/qemu/qemu_checkpoint.c @@ -155,7 +155,8 @@ qemuCheckpointDiscardDiskBitmaps(virStorageSourcePtr src, const char *delbitmap, const char *parentbitmap, virJSONValuePtr actions, - const char *diskdst) + const char *diskdst, + GSList **reopenimages) { virStorageSourcePtr n = src; @@ -235,6 +236,9 @@ qemuCheckpointDiscardDiskBitmaps(virStorageSourcePtr src, srcbitmap->name) < 0) return -1; + if (n != src) + *reopenimages = g_slist_prepend(*reopenimages, n); + n = n->backingStore; } @@ -253,6 +257,9 @@ qemuCheckpointDiscardBitmaps(virDomainObjPtr vm, int rc; g_autoptr(virJSONValue) actions = NULL; size_t i; + g_autoptr(GSList) reopenimages = NULL; + g_autoptr(GSList) relabelimages = NULL; + GSList *next; if (!(actions = virJSONValueNewArray())) return -1; @@ -284,16 +291,34 @@ qemuCheckpointDiscardBitmaps(virDomainObjPtr vm, if (qemuCheckpointDiscardDiskBitmaps(domdisk->src, blockNamedNodeData, chkdisk->bitmap, parentbitmap, - actions, domdisk->dst) < 0) + actions, domdisk->dst, + &reopenimages) < 0) return -1; } + /* label any non-top images for read-write access */ + for (next = reopenimages; next; next = next->next) { + virStorageSourcePtr src = next->data; + + if (qemuDomainStorageSourceAccessAllow(driver, vm, src, false, false) < 0) + goto relabel; + + relabelimages = g_slist_prepend(relabelimages, src); + } + qemuDomainObjEnterMonitor(driver, vm); rc = qemuMonitorTransaction(priv->mon, &actions); - if (qemuDomainObjExitMonitor(driver, vm) < 0 || rc < 0) + if (qemuDomainObjExitMonitor(driver, vm) < 0) return -1; - return 0; + relabel: + for (next = relabelimages; next; next = next->next) { + virStorageSourcePtr src = next->data; + + ignore_value(qemuDomainStorageSourceAccessAllow(driver, vm, src, true, false)); + } + + return rc; } diff --git a/src/qemu/qemu_checkpoint.h b/src/qemu/qemu_checkpoint.h index 976b1eed0f..cf1e9e46cb 100644 --- a/src/qemu/qemu_checkpoint.h +++ b/src/qemu/qemu_checkpoint.h @@ -78,4 +78,5 @@ qemuCheckpointDiscardDiskBitmaps(virStorageSourcePtr src, const char *delbitmap, const char *parentbitmap, virJSONValuePtr actions, - const char *diskdst); + const char *diskdst, + GSList **reopenimages); diff --git a/tests/qemublocktest.c b/tests/qemublocktest.c index c2c7507e20..a368e6a74f 100644 --- a/tests/qemublocktest.c +++ b/tests/qemublocktest.c @@ -717,6 +717,9 @@ testQemuCheckpointDeleteMerge(const void *opaque) g_autoptr(virJSONValue) actions = NULL; g_autoptr(virJSONValue) nodedatajson = NULL; g_autoptr(virHashTable) nodedata = NULL; + g_autoptr(GSList) reopenimages = NULL; + g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER; + GSList *tmp; expectpath = g_strdup_printf("%s/%s%s-out.json", abs_srcdir, checkpointDeletePrefix, data->name); @@ -738,14 +741,26 @@ testQemuCheckpointDeleteMerge(const void *opaque) data->deletebitmap, data->parentbitmap, actions, - "testdisk") < 0) { + "testdisk", + &reopenimages) < 0) { VIR_TEST_VERBOSE("failed to generate checkpoint delete transaction\n"); return -1; } - if (!(actual = virJSONValueToString(actions, true))) + if (virJSONValueToBuffer(actions, &buf, true) < 0) return -1; + if (reopenimages) { + virBufferAddLit(&buf, "reopen nodes:\n"); + + for (tmp = reopenimages; tmp; tmp = tmp->next) { + virStorageSourcePtr src = tmp->data; + virBufferAsprintf(&buf, "%s\n", src->nodeformat); + } + } + + actual = virBufferContentAndReset(&buf); + return virTestCompareToFile(actual, expectpath); } diff --git a/tests/qemublocktestdata/checkpointdelete/snapshots-intermediate1-out.json b/tests/qemublocktestdata/checkpointdelete/snapshots-intermediate1-out.json index 29fefeea63..c9bda3a17a 100644 --- a/tests/qemublocktestdata/checkpointdelete/snapshots-intermediate1-out.json +++ b/tests/qemublocktestdata/checkpointdelete/snapshots-intermediate1-out.json @@ -20,3 +20,5 @@ } } ] +reopen nodes: +libvirt-3-format diff --git a/tests/qemublocktestdata/checkpointdelete/snapshots-intermediate2-out.json b/tests/qemublocktestdata/checkpointdelete/snapshots-intermediate2-out.json index 4da21a9df7..8a0e3f2cff 100644 --- a/tests/qemublocktestdata/checkpointdelete/snapshots-intermediate2-out.json +++ b/tests/qemublocktestdata/checkpointdelete/snapshots-intermediate2-out.json @@ -57,3 +57,6 @@ } } ] +reopen nodes: +libvirt-3-format +libvirt-2-format diff --git a/tests/qemublocktestdata/checkpointdelete/snapshots-intermediate3-out.json b/tests/qemublocktestdata/checkpointdelete/snapshots-intermediate3-out.json index dc87dd60b8..211bc40baf 100644 --- a/tests/qemublocktestdata/checkpointdelete/snapshots-intermediate3-out.json +++ b/tests/qemublocktestdata/checkpointdelete/snapshots-intermediate3-out.json @@ -57,3 +57,5 @@ } } ] +reopen nodes: +libvirt-2-format diff --git a/tests/qemublocktestdata/checkpointdelete/snapshots-noparent-out.json b/tests/qemublocktestdata/checkpointdelete/snapshots-noparent-out.json index 45a84b47c2..f750f44da2 100644 --- a/tests/qemublocktestdata/checkpointdelete/snapshots-noparent-out.json +++ b/tests/qemublocktestdata/checkpointdelete/snapshots-noparent-out.json @@ -21,3 +21,7 @@ } } ] +reopen nodes: +libvirt-5-format +libvirt-4-format +libvirt-3-format diff --git a/tests/qemublocktestdata/checkpointdelete/snapshots-synthetic-checkpoint-intermediate1-out.json b/tests/qemublocktestdata/checkpointdelete/snapshots-synthetic-checkpoint-intermediate1-out.json index e979691e6f..d7e6d18637 100644 --- a/tests/qemublocktestdata/checkpointdelete/snapshots-synthetic-checkpoint-intermediate1-out.json +++ b/tests/qemublocktestdata/checkpointdelete/snapshots-synthetic-checkpoint-intermediate1-out.json @@ -27,3 +27,5 @@ } } ] +reopen nodes: +libvirt-3-format diff --git a/tests/qemublocktestdata/checkpointdelete/snapshots-synthetic-checkpoint-intermediate2-out.json b/tests/qemublocktestdata/checkpointdelete/snapshots-synthetic-checkpoint-intermediate2-out.json index e82098918a..cfbff010c2 100644 --- a/tests/qemublocktestdata/checkpointdelete/snapshots-synthetic-checkpoint-intermediate2-out.json +++ b/tests/qemublocktestdata/checkpointdelete/snapshots-synthetic-checkpoint-intermediate2-out.json @@ -30,3 +30,5 @@ } } ] +reopen nodes: +libvirt-2-format diff --git a/tests/qemublocktestdata/checkpointdelete/snapshots-synthetic-checkpoint-intermediate3-out.json b/tests/qemublocktestdata/checkpointdelete/snapshots-synthetic-checkpoint-intermediate3-out.json index dc87dd60b8..211bc40baf 100644 --- a/tests/qemublocktestdata/checkpointdelete/snapshots-synthetic-checkpoint-intermediate3-out.json +++ b/tests/qemublocktestdata/checkpointdelete/snapshots-synthetic-checkpoint-intermediate3-out.json @@ -57,3 +57,5 @@ } } ] +reopen nodes: +libvirt-2-format diff --git a/tests/qemublocktestdata/checkpointdelete/snapshots-synthetic-checkpoint-noparent-out.json b/tests/qemublocktestdata/checkpointdelete/snapshots-synthetic-checkpoint-noparent-out.json index 45a84b47c2..f750f44da2 100644 --- a/tests/qemublocktestdata/checkpointdelete/snapshots-synthetic-checkpoint-noparent-out.json +++ b/tests/qemublocktestdata/checkpointdelete/snapshots-synthetic-checkpoint-noparent-out.json @@ -21,3 +21,7 @@ } } ] +reopen nodes: +libvirt-5-format +libvirt-4-format +libvirt-3-format -- 2.24.1
On 1/9/20 12:21 PM, Peter Krempa wrote:
Posted as RFC because qemu probably doesn't reopen backing images for bitmap operations resulting in:
$ virsh checkpoint-delete vm --checkpointname a error: Failed to delete checkpoint a error: internal error: unable to execute QEMU command 'transaction': Bitmap 'a' is readonly and cannot be modified
Unfortunately this can't be done manually because 'blockdev-reopen' is still experimental.
In the meantime, with existing qemu, can we still at least teach libvirt to delete its metadata, and ignore unknown bitmaps leftover in qcow2 images that we were unable to delete because of the qemu limitation on read-only backing files? Yes, your qcow2 files will get bloated with dead bitmaps, but that's not going to interfere with guest operations or with future backups that use the bitmaps still tracked by libvirt. And it matches the failure scenario where an attempt at an incremental backup fails due to qemu crashing: the active bitmap is corrupt, so all older bitmaps are no longer usable, and your only recourse is to resort to full backup instead of incremental backup; but full backup is still worthwhile, and ignoring the now-useless bitmaps from the earlier backup points doesn't interfere with that. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
participants (2)
-
Eric Blake -
Peter Krempa