[PATCH v2 00/19] incremental backup: Handle bitmaps during block-commit
This is now based on top of a different implementation of the fix for late opening of backing chain during block-copy: https://www.redhat.com/archives/libvir-list/2020-March/msg00317.html Changes: - fixes suggested by Pavel - changes to capability use - rebased on master This series uses blockdev-reopen which was not yet stabilized in qemu, thus is experimental for now, but since incremental backup is experimental there's no problem with that. You can fetch the full branch along with patches for enabling the bits at: git fetch https://gitlab.com/pipo.sk/libvirt.git backup-commit You also need to use patched qemu which you can fetch from: git fetch https://gitlab.com/pipo.sk/qemu.git reopen-fixes Peter Krempa (19): qemu: capabilities: Add QEMU_CAPS_BLOCKDEV_REOPEN qemu: monitor: Add handler for blockdev-reopen qemu: block: implement helpers for blockdev-reopen qemuCheckpointDiscardBitmaps: Reopen images for bitmap modifications qemuCheckpointDiscardBitmaps: Use correct field for checkpoint bitmap name qemuDomainBlockCommit: Move checks depending on capabilities after liveness check qemu: domain: Extract formatting of 'commit' blockjob data into a function qemu: domain: Extract parsing of 'commit' blockjob data into a function qemu: blockjob: Store list of bitmaps disabled prior to commit qemublocktest: Fix and optimize fake image chain qemu: block: Implement helpers for dealing with bitmaps during block commit qemublocktest: Add tests for handling of bitmaps during block-commit qemublocktest: Add more tests for block-commit bitmap handling with snapshots qemublocktest: Add tests of broken bitmap chain handling during block-commit qemuBlockJobDiskNewCommit: Propagate 'disabledBitmapsBase' qemuDomainBlockCommit: Handle bitmaps on start of commit qemuDomainBlockPivot: Handle merging of bitmaps when pivoting an active block-commit qemu: blockjob: Handle bitmaps after finish of normal block-commit qemu: blockjob: Re-enable bitmaps after failed block-copy src/qemu/qemu_block.c | 320 ++++++++++++++++++ src/qemu/qemu_block.h | 23 ++ src/qemu/qemu_blockjob.c | 97 +++++- src/qemu/qemu_blockjob.h | 3 + src/qemu/qemu_capabilities.c | 1 + src/qemu/qemu_capabilities.h | 1 + src/qemu/qemu_checkpoint.c | 9 +- src/qemu/qemu_domain.c | 110 ++++-- src/qemu/qemu_driver.c | 78 ++++- src/qemu/qemu_monitor.c | 13 + src/qemu/qemu_monitor.h | 3 + src/qemu/qemu_monitor_json.c | 21 ++ src/qemu/qemu_monitor_json.h | 4 + tests/qemublocktest.c | 126 ++++++- .../bitmapblockcommit/basic-1-2 | 119 +++++++ .../bitmapblockcommit/basic-1-3 | 119 +++++++ .../bitmapblockcommit/basic-2-3 | 2 + .../bitmapblockcommit/snapshots-1-2 | 49 +++ .../bitmapblockcommit/snapshots-1-3 | 76 +++++ .../bitmapblockcommit/snapshots-1-4 | 126 +++++++ .../bitmapblockcommit/snapshots-1-5 | 130 +++++++ .../bitmapblockcommit/snapshots-2-3 | 49 +++ .../bitmapblockcommit/snapshots-2-4 | 99 ++++++ .../bitmapblockcommit/snapshots-2-5 | 103 ++++++ .../bitmapblockcommit/snapshots-3-4 | 72 ++++ .../bitmapblockcommit/snapshots-3-5 | 76 +++++ .../bitmapblockcommit/snapshots-4-4 | 11 + .../bitmapblockcommit/snapshots-4-5 | 33 ++ .../snapshots-synthetic-broken-1-2 | 27 ++ .../snapshots-synthetic-broken-1-3 | 66 ++++ .../snapshots-synthetic-broken-1-4 | 73 ++++ .../snapshots-synthetic-broken-1-5 | 73 ++++ .../snapshots-synthetic-broken-2-3 | 43 +++ .../snapshots-synthetic-broken-2-4 | 50 +++ .../snapshots-synthetic-broken-2-5 | 50 +++ .../snapshots-synthetic-broken-3-4 | 27 ++ .../snapshots-synthetic-broken-3-5 | 27 ++ .../snapshots-synthetic-broken-4-5 | 20 ++ .../blockjob-blockdev-in.xml | 4 + 39 files changed, 2290 insertions(+), 43 deletions(-) create mode 100644 tests/qemublocktestdata/bitmapblockcommit/basic-1-2 create mode 100644 tests/qemublocktestdata/bitmapblockcommit/basic-1-3 create mode 100644 tests/qemublocktestdata/bitmapblockcommit/basic-2-3 create mode 100644 tests/qemublocktestdata/bitmapblockcommit/snapshots-1-2 create mode 100644 tests/qemublocktestdata/bitmapblockcommit/snapshots-1-3 create mode 100644 tests/qemublocktestdata/bitmapblockcommit/snapshots-1-4 create mode 100644 tests/qemublocktestdata/bitmapblockcommit/snapshots-1-5 create mode 100644 tests/qemublocktestdata/bitmapblockcommit/snapshots-2-3 create mode 100644 tests/qemublocktestdata/bitmapblockcommit/snapshots-2-4 create mode 100644 tests/qemublocktestdata/bitmapblockcommit/snapshots-2-5 create mode 100644 tests/qemublocktestdata/bitmapblockcommit/snapshots-3-4 create mode 100644 tests/qemublocktestdata/bitmapblockcommit/snapshots-3-5 create mode 100644 tests/qemublocktestdata/bitmapblockcommit/snapshots-4-4 create mode 100644 tests/qemublocktestdata/bitmapblockcommit/snapshots-4-5 create mode 100644 tests/qemublocktestdata/bitmapblockcommit/snapshots-synthetic-broken-1-2 create mode 100644 tests/qemublocktestdata/bitmapblockcommit/snapshots-synthetic-broken-1-3 create mode 100644 tests/qemublocktestdata/bitmapblockcommit/snapshots-synthetic-broken-1-4 create mode 100644 tests/qemublocktestdata/bitmapblockcommit/snapshots-synthetic-broken-1-5 create mode 100644 tests/qemublocktestdata/bitmapblockcommit/snapshots-synthetic-broken-2-3 create mode 100644 tests/qemublocktestdata/bitmapblockcommit/snapshots-synthetic-broken-2-4 create mode 100644 tests/qemublocktestdata/bitmapblockcommit/snapshots-synthetic-broken-2-5 create mode 100644 tests/qemublocktestdata/bitmapblockcommit/snapshots-synthetic-broken-3-4 create mode 100644 tests/qemublocktestdata/bitmapblockcommit/snapshots-synthetic-broken-3-5 create mode 100644 tests/qemublocktestdata/bitmapblockcommit/snapshots-synthetic-broken-4-5 -- 2.24.1
This capability will be asserted once qemu stabilizes 'blockdev-reopen'. For now we just add the capability so that we can introduce some code that will use the reopening call. This will show our willingness to adopt use of reopen and help qemu developers stabilize it. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_capabilities.c | 1 + src/qemu/qemu_capabilities.h | 1 + 2 files changed, 2 insertions(+) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 25a77c24af..c486697d5b 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -566,6 +566,7 @@ VIR_ENUM_IMPL(virQEMUCaps, "vhost-user-fs", "query-named-block-nodes.flat", "blockdev-snapshot.allow-write-only-overlay", + "blockdev-reopen", ); diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index e952fcb6b8..f0961e273c 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -547,6 +547,7 @@ typedef enum { /* virQEMUCapsFlags grouping marker for syntax-check */ QEMU_CAPS_DEVICE_VHOST_USER_FS, /* -device vhost-user-fs */ QEMU_CAPS_QMP_QUERY_NAMED_BLOCK_NODES_FLAT, /* query-named-block-nodes supports the 'flat' option */ QEMU_CAPS_BLOCKDEV_SNAPSHOT_ALLOW_WRITE_ONLY, /* blockdev-snapshot has the 'allow-write-only-overlay' feature */ + QEMU_CAPS_BLOCKDEV_REOPEN, /* 'blockdev-reopen' qmp command is supported */ QEMU_CAPS_LAST /* this must always be the last item */ } virQEMUCapsFlags; -- 2.24.1
On 3/11/20 7:55 AM, Peter Krempa wrote:
This capability will be asserted once qemu stabilizes 'blockdev-reopen'. For now we just add the capability so that we can introduce some code that will use the reopening call. This will show our willingness to adopt use of reopen and help qemu developers stabilize it.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_capabilities.c | 1 + src/qemu/qemu_capabilities.h | 1 + 2 files changed, 2 insertions(+)
Reviewed-by: Eric Blake <eblake@redhat.com> -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Introduce the monitor code for using blockdev-reopen. For now we'll use x-blockdev-reopen so that the interactions between qemu and libvirt can be tested with the existing code. Since the usage will be guarded by the for-now unasserted capability we'll be able to change the called command when the command will be stabilized. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_monitor.c | 13 +++++++++++++ src/qemu/qemu_monitor.h | 3 +++ src/qemu/qemu_monitor_json.c | 21 +++++++++++++++++++++ src/qemu/qemu_monitor_json.h | 4 ++++ 4 files changed, 41 insertions(+) diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index e54d28b6cc..2a285025df 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -4368,6 +4368,19 @@ qemuMonitorBlockdevAdd(qemuMonitorPtr mon, } +int +qemuMonitorBlockdevReopen(qemuMonitorPtr mon, + virJSONValuePtr *props) +{ + VIR_DEBUG("props=%p (node-name=%s)", *props, + NULLSTR(virJSONValueObjectGetString(*props, "node-name"))); + + QEMU_CHECK_MONITOR(mon); + + return qemuMonitorJSONBlockdevReopen(mon, props); +} + + int qemuMonitorBlockdevDel(qemuMonitorPtr mon, const char *nodename) diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index 2319647a35..5c86da80e5 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -1319,6 +1319,9 @@ int qemuMonitorBlockdevCreate(qemuMonitorPtr mon, int qemuMonitorBlockdevAdd(qemuMonitorPtr mon, virJSONValuePtr *props); +int qemuMonitorBlockdevReopen(qemuMonitorPtr mon, + virJSONValuePtr *props); + int qemuMonitorBlockdevDel(qemuMonitorPtr mon, const char *nodename); diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 3eac80c060..88608be49a 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -8810,6 +8810,27 @@ qemuMonitorJSONBlockdevAdd(qemuMonitorPtr mon, } +int +qemuMonitorJSONBlockdevReopen(qemuMonitorPtr mon, + virJSONValuePtr *props) +{ + g_autoptr(virJSONValue) cmd = NULL; + g_autoptr(virJSONValue) reply = NULL; + virJSONValuePtr pr = g_steal_pointer(props); + + if (!(cmd = qemuMonitorJSONMakeCommandInternal("x-blockdev-reopen", pr))) + return -1; + + if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0) + return -1; + + if (qemuMonitorJSONCheckError(cmd, reply) < 0) + return -1; + + return 0; +} + + int qemuMonitorJSONBlockdevDel(qemuMonitorPtr mon, const char *nodename) diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h index ed48600b82..5b3bb295eb 100644 --- a/src/qemu/qemu_monitor_json.h +++ b/src/qemu/qemu_monitor_json.h @@ -602,6 +602,10 @@ int qemuMonitorJSONBlockdevAdd(qemuMonitorPtr mon, virJSONValuePtr *props) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); +int qemuMonitorJSONBlockdevReopen(qemuMonitorPtr mon, + virJSONValuePtr *props) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); + int qemuMonitorJSONBlockdevDel(qemuMonitorPtr mon, const char *nodename) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); -- 2.24.1
On 3/11/20 7:55 AM, Peter Krempa wrote:
Introduce the monitor code for using blockdev-reopen. For now we'll use x-blockdev-reopen so that the interactions between qemu and libvirt can be tested with the existing code.
Since the usage will be guarded by the for-now unasserted capability we'll be able to change the called command when the command will be stabilized.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_monitor.c | 13 +++++++++++++ src/qemu/qemu_monitor.h | 3 +++ src/qemu/qemu_monitor_json.c | 21 +++++++++++++++++++++ src/qemu/qemu_monitor_json.h | 4 ++++ 4 files changed, 41 insertions(+)
Reviewed-by: Eric Blake <eblake@redhat.com> -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Introduce a set of helpers to call blockdev-reopen in certain scenarios Libvirt will use the QMP command to turn certain members of the backing chain read-write for bitmap manipulation and we'll also want to use it to replace/install the backing chain of a qcow2 format node. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_block.c | 101 ++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_block.h | 9 ++++ 2 files changed, 110 insertions(+) diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c index 152c73f1bf..edebbcd0ce 100644 --- a/src/qemu/qemu_block.c +++ b/src/qemu/qemu_block.c @@ -2960,3 +2960,104 @@ qemuBlockBitmapsHandleBlockcopy(virStorageSourcePtr src, return 0; } + + +/** + * qemuBlockReopenFormat: + * @vm: domain object + * @src: storage source to reopen + * @asyncJob: qemu async job type + * + * Invokes the 'blockdev-reopen' command on the format layer of @src. This means + * that @src must be already properly configured for the desired outcome. The + * nodenames of @src are used to identify the specific image in qemu. + */ +static int +qemuBlockReopenFormat(virDomainObjPtr vm, + virStorageSourcePtr src, + qemuDomainAsyncJob asyncJob) +{ + qemuDomainObjPrivatePtr priv = vm->privateData; + virQEMUDriverPtr driver = priv->driver; + g_autoptr(virJSONValue) reopenprops = NULL; + int rc; + + /* If we are lacking the object here, qemu might have opened an image with + * a node name unknown to us */ + if (!src->backingStore) { + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", + _("can't reopen image with unknown presence of backing store")); + return -1; + } + + if (!(reopenprops = qemuBlockStorageSourceGetBlockdevProps(src, src->backingStore))) + return -1; + + if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0) + return -1; + + rc = qemuMonitorBlockdevReopen(priv->mon, &reopenprops); + + if (qemuDomainObjExitMonitor(driver, vm) < 0 || rc < 0) + return -1; + + return 0; +} + + +/** + * qemuBlockReopenReadWrite: + * @vm: domain object + * @src: storage source to reopen + * @asyncJob: qemu async job type + * + * Wrapper that reopens @src read-write. We currently depend on qemu + * reopening the storage with 'auto-read-only' enabled for us. + * After successful reopen @src's 'readonly' flag is modified. Does nothing + * if @src is already read-write. + */ +int +qemuBlockReopenReadWrite(virDomainObjPtr vm, + virStorageSourcePtr src, + qemuDomainAsyncJob asyncJob) +{ + if (!src->readonly) + return 0; + + src->readonly = false; + if (qemuBlockReopenFormat(vm, src, asyncJob) < 0) { + src->readonly = true; + return -1; + } + + return 0; +} + + +/** + * qemuBlockReopenReadOnly: + * @vm: domain object + * @src: storage source to reopen + * @asyncJob: qemu async job type + * + * Wrapper that reopens @src read-only. We currently depend on qemu + * reopening the storage with 'auto-read-only' enabled for us. + * After successful reopen @src's 'readonly' flag is modified. Does nothing + * if @src is already read-only. + */ +int +qemuBlockReopenReadOnly(virDomainObjPtr vm, + virStorageSourcePtr src, + qemuDomainAsyncJob asyncJob) +{ + if (src->readonly) + return 0; + + src->readonly = true; + if (qemuBlockReopenFormat(vm, src, asyncJob) < 0) { + src->readonly = false; + return -1; + } + + return 0; +} diff --git a/src/qemu/qemu_block.h b/src/qemu/qemu_block.h index eab0128d5d..1d8a364bd0 100644 --- a/src/qemu/qemu_block.h +++ b/src/qemu/qemu_block.h @@ -228,3 +228,12 @@ qemuBlockBitmapsHandleBlockcopy(virStorageSourcePtr src, virHashTablePtr blockNamedNodeData, bool shallow, virJSONValuePtr *actions); + +int +qemuBlockReopenReadWrite(virDomainObjPtr vm, + virStorageSourcePtr src, + qemuDomainAsyncJob asyncJob); +int +qemuBlockReopenReadOnly(virDomainObjPtr vm, + virStorageSourcePtr src, + qemuDomainAsyncJob asyncJob); -- 2.24.1
On 3/11/20 7:55 AM, Peter Krempa wrote:
Introduce a set of helpers to call blockdev-reopen in certain scenarios
Libvirt will use the QMP command to turn certain members of the backing chain read-write for bitmap manipulation and we'll also want to use it to replace/install the backing chain of a qcow2 format node.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_block.c | 101 ++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_block.h | 9 ++++ 2 files changed, 110 insertions(+)
Reviewed-by: Eric Blake <eblake@redhat.com> -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Qemu's bitmap APIs don't reopen the appropriate images read-write for modification. It's libvirt's duty to reopen them via blockdev-reopen if we wish to modify the bitmaps. Use the new helpers to reopen the images for bitmap manipulation. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_checkpoint.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/qemu/qemu_checkpoint.c b/src/qemu/qemu_checkpoint.c index ea87b09aa0..5890deb471 100644 --- a/src/qemu/qemu_checkpoint.c +++ b/src/qemu/qemu_checkpoint.c @@ -300,6 +300,10 @@ qemuCheckpointDiscardBitmaps(virDomainObjPtr vm, false, false, false) < 0) goto relabel; + if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BLOCKDEV_REOPEN) && + qemuBlockReopenReadWrite(vm, src, QEMU_ASYNC_JOB_NONE) < 0) + goto relabel; + relabelimages = g_slist_prepend(relabelimages, src); } @@ -312,6 +316,9 @@ qemuCheckpointDiscardBitmaps(virDomainObjPtr vm, for (next = relabelimages; next; next = next->next) { virStorageSourcePtr src = next->data; + if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BLOCKDEV_REOPEN)) + ignore_value(qemuBlockReopenReadOnly(vm, src, QEMU_ASYNC_JOB_NONE)); + ignore_value(qemuDomainStorageSourceAccessAllow(driver, vm, src, true, false, false)); } -- 2.24.1
On 3/11/20 7:55 AM, Peter Krempa wrote:
Qemu's bitmap APIs don't reopen the appropriate images read-write for modification. It's libvirt's duty to reopen them via blockdev-reopen if we wish to modify the bitmaps.
Use the new helpers to reopen the images for bitmap manipulation.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_checkpoint.c | 7 +++++++ 1 file changed, 7 insertions(+)
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 code deleting checkpoints needs the name of the parent checkpoint's disk's bitmap but was using the disk alias instead. This would create wrong bitmaps after deleting some checkpoints. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_checkpoint.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/qemu/qemu_checkpoint.c b/src/qemu/qemu_checkpoint.c index 5890deb471..76f10a701e 100644 --- a/src/qemu/qemu_checkpoint.c +++ b/src/qemu/qemu_checkpoint.c @@ -283,7 +283,7 @@ qemuCheckpointDiscardBitmaps(virDomainObjPtr vm, * ancestor. */ if ((parentchkdisk = qemuCheckpointFindActiveDiskInParent(vm, parent, chkdisk->name))) - parentbitmap = parentchkdisk->name; + parentbitmap = parentchkdisk->bitmap; if (qemuCheckpointDiscardDiskBitmaps(domdisk->src, blockNamedNodeData, chkdisk->bitmap, parentbitmap, -- 2.24.1
On 3/11/20 7:55 AM, Peter Krempa wrote:
The code deleting checkpoints needs the name of the parent checkpoint's disk's bitmap but was using the disk alias instead. This would create wrong bitmaps after deleting some checkpoints.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_checkpoint.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/qemu/qemu_checkpoint.c b/src/qemu/qemu_checkpoint.c index 5890deb471..76f10a701e 100644 --- a/src/qemu/qemu_checkpoint.c +++ b/src/qemu/qemu_checkpoint.c @@ -283,7 +283,7 @@ qemuCheckpointDiscardBitmaps(virDomainObjPtr vm, * ancestor. */ if ((parentchkdisk = qemuCheckpointFindActiveDiskInParent(vm, parent, chkdisk->name))) - parentbitmap = parentchkdisk->name; + parentbitmap = parentchkdisk->bitmap;
Shouldn't this go in now regardless of whether the rest of the series is still waiting on qemu? Reviewed-by: Eric Blake <eblake@redhat.com> -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Since capabilities are not present for inactive VMs we'd report that we don't support '--delete' or commiting while checkpoints exist rather than the proper error. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index d565054436..d3eb2171ef 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -18413,9 +18413,6 @@ qemuDomainBlockCommit(virDomainPtr dom, if (virDomainBlockCommitEnsureACL(dom->conn, vm->def) < 0) goto cleanup; - if (qemuDomainSupportsCheckpointsBlockjobs(vm) < 0) - goto cleanup; - if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) goto cleanup; @@ -18424,12 +18421,6 @@ qemuDomainBlockCommit(virDomainPtr dom, blockdev = virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BLOCKDEV); - if (!blockdev && (flags & VIR_DOMAIN_BLOCK_COMMIT_DELETE)) { - virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", - _("deleting committed images is not supported by this VM")); - goto endjob; - } - /* Convert bandwidth MiB to bytes, if necessary */ if (!(flags & VIR_DOMAIN_BLOCK_COMMIT_BANDWIDTH_BYTES)) { if (speed > LLONG_MAX >> 20) { @@ -18454,6 +18445,15 @@ qemuDomainBlockCommit(virDomainPtr dom, if (qemuDomainDiskBlockJobIsActive(disk)) goto endjob; + if (qemuDomainSupportsCheckpointsBlockjobs(vm) < 0) + goto endjob; + + if (!blockdev && (flags & VIR_DOMAIN_BLOCK_COMMIT_DELETE)) { + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", + _("deleting committed images is not supported by this VM")); + goto endjob; + } + if (!top || STREQ(top, disk->dst)) topSource = disk->src; else if (virStorageFileParseChainIndex(disk->dst, top, &topIndex) < 0 || -- 2.24.1
On 3/11/20 7:55 AM, Peter Krempa wrote:
Since capabilities are not present for inactive VMs we'd report that we don't support '--delete' or commiting while checkpoints exist rather
committing
than the proper error.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 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
I'll be adding more fields to care about so splitting the code out will be better long-term. Signed-off-by: Peter Krempa <pkrempa@redhat.com> Reviewed-by: Pavel Mores <pmores@redhat.com> --- src/qemu/qemu_domain.c | 27 +++++++++++++++++++-------- 1 file changed, 19 insertions(+), 8 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 3d3f796d85..369d9b8446 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -2530,6 +2530,24 @@ qemuDomainObjPrivateXMLFormatBlockjobFormatSource(virBufferPtr buf, } +static void +qemuDomainPrivateBlockJobFormatCommit(qemuBlockJobDataPtr job, + virBufferPtr buf) +{ + if (job->data.commit.base) + virBufferAsprintf(buf, "<base node='%s'/>\n", job->data.commit.base->nodeformat); + + if (job->data.commit.top) + virBufferAsprintf(buf, "<top node='%s'/>\n", job->data.commit.top->nodeformat); + + if (job->data.commit.topparent) + virBufferAsprintf(buf, "<topparent node='%s'/>\n", job->data.commit.topparent->nodeformat); + + if (job->data.commit.deleteCommittedImages) + virBufferAddLit(buf, "<deleteCommittedImages/>\n"); +} + + static int qemuDomainObjPrivateXMLFormatBlockjobIterator(void *payload, const void *name G_GNUC_UNUSED, @@ -2589,14 +2607,7 @@ qemuDomainObjPrivateXMLFormatBlockjobIterator(void *payload, case QEMU_BLOCKJOB_TYPE_COMMIT: case QEMU_BLOCKJOB_TYPE_ACTIVE_COMMIT: - if (job->data.commit.base) - virBufferAsprintf(&childBuf, "<base node='%s'/>\n", job->data.commit.base->nodeformat); - if (job->data.commit.top) - virBufferAsprintf(&childBuf, "<top node='%s'/>\n", job->data.commit.top->nodeformat); - if (job->data.commit.topparent) - virBufferAsprintf(&childBuf, "<topparent node='%s'/>\n", job->data.commit.topparent->nodeformat); - if (job->data.commit.deleteCommittedImages) - virBufferAddLit(&childBuf, "<deleteCommittedImages/>\n"); + qemuDomainPrivateBlockJobFormatCommit(job, &childBuf); break; case QEMU_BLOCKJOB_TYPE_CREATE: -- 2.24.1
On 3/11/20 7:55 AM, Peter Krempa wrote:
I'll be adding more fields to care about so splitting the code out will be better long-term.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> Reviewed-by: Pavel Mores <pmores@redhat.com> --- src/qemu/qemu_domain.c | 27 +++++++++++++++++++-------- 1 file changed, 19 insertions(+), 8 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
I'll be adding more fields to care about so splitting the code out will be better long-term. Signed-off-by: Peter Krempa <pkrempa@redhat.com> Reviewed-by: Pavel Mores <pmores@redhat.com> --- src/qemu/qemu_domain.c | 57 ++++++++++++++++++++++++++---------------- 1 file changed, 36 insertions(+), 21 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 369d9b8446..d8486a5277 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -3153,6 +3153,40 @@ qemuDomainObjPrivateXMLParseBlockjobNodename(qemuBlockJobDataPtr job, } +static int +qemuDomainObjPrivateXMLParseBlockjobDataCommit(qemuBlockJobDataPtr job, + xmlXPathContextPtr ctxt) +{ + if (job->type == QEMU_BLOCKJOB_TYPE_COMMIT) { + qemuDomainObjPrivateXMLParseBlockjobNodename(job, + "string(./topparent/@node)", + &job->data.commit.topparent, + ctxt); + + if (!job->data.commit.topparent) + return -1; + } + + qemuDomainObjPrivateXMLParseBlockjobNodename(job, + "string(./top/@node)", + &job->data.commit.top, + ctxt); + qemuDomainObjPrivateXMLParseBlockjobNodename(job, + "string(./base/@node)", + &job->data.commit.base, + ctxt); + + if (virXPathNode("./deleteCommittedImages", ctxt)) + job->data.commit.deleteCommittedImages = true; + + if (!job->data.commit.top || + !job->data.commit.base) + return -1; + + return 0; +} + + static void qemuDomainObjPrivateXMLParseBlockjobDataSpecific(qemuBlockJobDataPtr job, xmlXPathContextPtr ctxt, @@ -3172,29 +3206,10 @@ qemuDomainObjPrivateXMLParseBlockjobDataSpecific(qemuBlockJobDataPtr job, break; case QEMU_BLOCKJOB_TYPE_COMMIT: - qemuDomainObjPrivateXMLParseBlockjobNodename(job, - "string(./topparent/@node)", - &job->data.commit.topparent, - ctxt); - - if (!job->data.commit.topparent) - goto broken; - - G_GNUC_FALLTHROUGH; case QEMU_BLOCKJOB_TYPE_ACTIVE_COMMIT: - qemuDomainObjPrivateXMLParseBlockjobNodename(job, - "string(./top/@node)", - &job->data.commit.top, - ctxt); - qemuDomainObjPrivateXMLParseBlockjobNodename(job, - "string(./base/@node)", - &job->data.commit.base, - ctxt); - if (virXPathNode("./deleteCommittedImages", ctxt)) - job->data.commit.deleteCommittedImages = true; - if (!job->data.commit.top || - !job->data.commit.base) + if (qemuDomainObjPrivateXMLParseBlockjobDataCommit(job, ctxt) < 0) goto broken; + break; case QEMU_BLOCKJOB_TYPE_CREATE: -- 2.24.1
On 3/11/20 7:55 AM, Peter Krempa wrote:
I'll be adding more fields to care about so splitting the code out will be better long-term.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> Reviewed-by: Pavel Mores <pmores@redhat.com> --- src/qemu/qemu_domain.c | 57 ++++++++++++++++++++++++++---------------- 1 file changed, 36 insertions(+), 21 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
Starting a commit job will require disabling bitmaps in the base image so that they are not dirtied by the commit job. We need to store a list of the bitmaps so that we can later re-enable them. Add a field and status XML handling code as well as a test. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_blockjob.h | 2 ++ src/qemu/qemu_domain.c | 26 +++++++++++++++++++ .../blockjob-blockdev-in.xml | 4 +++ 3 files changed, 32 insertions(+) diff --git a/src/qemu/qemu_blockjob.h b/src/qemu/qemu_blockjob.h index 72c7fa053e..e2e28ca4d3 100644 --- a/src/qemu/qemu_blockjob.h +++ b/src/qemu/qemu_blockjob.h @@ -88,6 +88,8 @@ struct _qemuBlockJobCommitData { virStorageSourcePtr top; virStorageSourcePtr base; bool deleteCommittedImages; + char **disabledBitmapsBase; /* a NULL-terminated list of bitmap names which + were disabled in @base for the commit job */ }; diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index d8486a5277..f3e98d7ad9 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -2534,6 +2534,9 @@ static void qemuDomainPrivateBlockJobFormatCommit(qemuBlockJobDataPtr job, virBufferPtr buf) { + g_auto(virBuffer) disabledBitmapsBuf = VIR_BUFFER_INIT_CHILD(buf); + char **bitmaps = job->data.commit.disabledBitmapsBase; + if (job->data.commit.base) virBufferAsprintf(buf, "<base node='%s'/>\n", job->data.commit.base->nodeformat); @@ -2545,6 +2548,11 @@ qemuDomainPrivateBlockJobFormatCommit(qemuBlockJobDataPtr job, if (job->data.commit.deleteCommittedImages) virBufferAddLit(buf, "<deleteCommittedImages/>\n"); + + while (bitmaps && *bitmaps) + virBufferEscapeString(&disabledBitmapsBuf, "<bitmap name='%s'/>\n", *(bitmaps++)); + + virXMLFormatElement(buf, "disabledBaseBitmaps", NULL, &disabledBitmapsBuf); } @@ -3157,6 +3165,9 @@ static int qemuDomainObjPrivateXMLParseBlockjobDataCommit(qemuBlockJobDataPtr job, xmlXPathContextPtr ctxt) { + g_autofree xmlNodePtr *nodes = NULL; + ssize_t nnodes; + if (job->type == QEMU_BLOCKJOB_TYPE_COMMIT) { qemuDomainObjPrivateXMLParseBlockjobNodename(job, "string(./topparent/@node)", @@ -3183,6 +3194,21 @@ qemuDomainObjPrivateXMLParseBlockjobDataCommit(qemuBlockJobDataPtr job, !job->data.commit.base) return -1; + if ((nnodes = virXPathNodeSet("./disabledBaseBitmaps/bitmap", ctxt, &nodes)) > 0) { + size_t i; + + job->data.commit.disabledBitmapsBase = g_new0(char *, nnodes + 1); + + for (i = 0; i < nnodes; i++) { + char *tmp; + + if (!(tmp = virXMLPropString(nodes[i], "name"))) + return -1; + + job->data.commit.disabledBitmapsBase[i] = g_steal_pointer(&tmp); + } + } + return 0; } diff --git a/tests/qemustatusxml2xmldata/blockjob-blockdev-in.xml b/tests/qemustatusxml2xmldata/blockjob-blockdev-in.xml index ca6d110179..cc17a17ff4 100644 --- a/tests/qemustatusxml2xmldata/blockjob-blockdev-in.xml +++ b/tests/qemustatusxml2xmldata/blockjob-blockdev-in.xml @@ -243,6 +243,10 @@ <base node='libvirt-19-format'/> <top node='libvirt-17-format'/> <deleteCommittedImages/> + <disabledBaseBitmaps> + <bitmap name='test'/> + <bitmap name='test1'/> + </disabledBaseBitmaps> </blockjob> <blockjob name='create-libvirt-1337-storage' type='create' state='running'> <create mode='storage'/> -- 2.24.1
On 3/11/20 7:55 AM, Peter Krempa wrote:
Starting a commit job will require disabling bitmaps in the base image so that they are not dirtied by the commit job. We need to store a list of the bitmaps so that we can later re-enable them.
I think I get why you don't want them further dirtied, but does it really matter? Visually, consider what happens if we create checkpoint1 after writing data A, then checkpoint2 after writing data B (rendering the first checkpoing read-only) before writing data C, then create an external snapshot (possibly simultaneous with checkpoint3) to create a new active image (the new image also has a distinct bitmap tracking all changes since the snapshot, while the snapshot in the now-backing image remains read-write but unchanged because we no longer write to the backing) before writing data D: backing: AABBCC-- bitmap1: --11---- (disabled) bitmap2: ----11-- (enabled) active: ---D-DD- bitmap3: ---1-11- (enabled) In the above, I used different bitmap names between backing and active (as that is less confusing), but I don't know if that's what your code for bitmaps + external snapshots currently does, or if we end up with two bitmaps both named bitmap2 in both images; but that shouldn't really matter (as qemu needs both the node name and a bitmap name). While in this configuration, a differential backup from checkpoint1 uses backing/bitmap1 + backing/bitmap2 + active/bitmap3 == --11111-; a differential from checkpoint2 uses backing/bitmap2 + active/bitmap3 == ---1111-; a differential from the time of the external snapshot creation uses active/bitmap3 == ---1-11-. So, the question is what happens if we now want to commit active back into backing. If we leave bitmap2 enabled while the commit happens, we end up with: backing: AABDCDD- bitmap1: --11---- (disabled) bitmap2: ---1111- (enabled) You can no longer recreate the point in time where the external snapshot was created (the commit lost checkpoint3), but bitmap2 is still viable for all changes needed for an incremental backup of changes made after bitmap1 was disabled and bitmap2 created. Conversely, if we disable bitmap2 prior to the commit, then you have the option of creating a new bitmap3 prior to the commit to end up with: backing: AABDCDD- bitmap1: --11---- (disabled) bitmap2: ----11-- (disabled) bitmap3: ---1-11- (enabled) such that you now have a point-in-time where you can then do a differential backup from all changes after the external snapshot was created (even though it has now been committed). But your resulting backups from any of the checkpoints see the same cumulative bitmaps as before. And what happens if you accidentally leave bitmap2 enabled during the commit? backing: AABDCDD- bitmap1: --11---- (disabled) bitmap2: ---1111- (disabled) bitmap3: ---1-11- (enabled) You have recorded more bits than strictly necessary in bitmap2, but as bitmap2 is unusable on its own (a differential bitmap HAS to combine bitmap2 with bitmap3, whether you are doing the differential from checkpoint1 or checkoint2), and all of those same bits are recorded in bitamp3, the resulting cumulative bitmap is unchanged. Thus, my question is whether this complexity is necessary because it buys us some safety (is there some failure path I'm overlooking, where keeping bitmap2 unchanged even though backing is partially modified before the failure of a half-completed commit is essential?), but I can still go ahead and review the patch on the assumption that you have a reason for not allowing a stranded-enabled bitmap after a snapshot operation to not be modified during commit.
Add a field and status XML handling code as well as a test.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_blockjob.h | 2 ++ src/qemu/qemu_domain.c | 26 +++++++++++++++++++ .../blockjob-blockdev-in.xml | 4 +++ 3 files changed, 32 insertions(+)
At any rate, I'm not seeing any functional problems with the added code, even if I still have a question on the technical need. 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 Wed, Mar 11, 2020 at 15:15:03 -0500, Eric Blake wrote:
On 3/11/20 7:55 AM, Peter Krempa wrote:
Starting a commit job will require disabling bitmaps in the base image so that they are not dirtied by the commit job. We need to store a list of the bitmaps so that we can later re-enable them.
I think I get why you don't want them further dirtied, but does it really matter?
Well it feels to be the right thing to do in terms of the semantics that were established for checkpoints. To refresh, creating a new checkpoint disables previous bitmaps to "optimize" the number of bytes written in the bitmap. On that terms I think we should make sure that the bitmaps are populated correctly. Given qemu's todays features and the fact that bitmaps don't thake that much space on disk I'd probably nowadays go for a checkpoint always creating a new bitmap and not disabling older ones. In addition we could stop populating snapshots with bitmaps altogether as we can get the bitmaps from an allocation map now. With the above a lot of the operations could be greatly simplified. In the end we can rework all of this as the feature is still really experimental, but for the current situation with other parts of the incremental backup code we should do this and since we decided to go the hard way we should not cut corners here.
Set the 'id' field of the backing chain properly so that we can look up images and initialize 6 images instead of 10 as we don't use more currently. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/qemublocktest.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/qemublocktest.c b/tests/qemublocktest.c index 7b7948d4c6..a6b6376c7d 100644 --- a/tests/qemublocktest.c +++ b/tests/qemublocktest.c @@ -570,6 +570,7 @@ testQemuBackupIncrementalBitmapCalculateGetFakeImage(size_t idx) if (!(ret = virStorageSourceNew())) abort(); + ret->id = idx; ret->type = VIR_STORAGE_TYPE_FILE; ret->format = VIR_STORAGE_FILE_QCOW2; ret->path = g_strdup_printf("/image%zu", idx); @@ -589,7 +590,7 @@ testQemuBackupIncrementalBitmapCalculateGetFakeChain(void) n = ret = testQemuBackupIncrementalBitmapCalculateGetFakeImage(1); - for (i = 2; i < 10; i++) { + for (i = 2; i < 6; i++) { n->backingStore = testQemuBackupIncrementalBitmapCalculateGetFakeImage(i); n = n->backingStore; } -- 2.24.1
On 3/11/20 7:55 AM, Peter Krempa wrote:
Set the 'id' field of the backing chain properly so that we can look up images and initialize 6 images instead of 10 as we don't use more
s/images/images,/
currently.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/qemublocktest.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
Reviewed-by: Eric Blake <eblake@redhat.com> -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
qemuBlockBitmapsHandleCommitStart prepares for disabling the bitmaps in the 'base' of the commit job so that the bitmaps are not dirtied by the commit job. This needs to be done prior to start of the commit job. qemuBlockBitmapsHandleCommitFinish then calculates the necessary merges that agregate all the bitmaps between the commited images and write them into the base bitmap. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_block.c | 219 ++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_block.h | 14 +++ 2 files changed, 233 insertions(+) diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c index edebbcd0ce..6853c021ca 100644 --- a/src/qemu/qemu_block.c +++ b/src/qemu/qemu_block.c @@ -2962,6 +2962,225 @@ qemuBlockBitmapsHandleBlockcopy(virStorageSourcePtr src, } +/** + * @topsrc: virStorageSource representing 'top' of the job + * @basesrc: virStorageSource representing 'base' of the job + * @blockNamedNodeData: hash table containing data about bitmaps + * @actions: filled with arguments for a 'transaction' command + * @disabledBitmapsBase: filled with a list of bitmap names which must be disabled + * + * Prepares data for correctly hanlding bitmaps during the start of a commit + * job. The bitmaps in the 'base' image must be disabled, so that the writes + * done by the blockjob don't dirty the enabled bitmaps. + * + * @actions and @disabledBitmapsBase are untouched if no bitmaps need + * to be disabled. + */ +int +qemuBlockBitmapsHandleCommitStart(virStorageSourcePtr topsrc, + virStorageSourcePtr basesrc, + virHashTablePtr blockNamedNodeData, + virJSONValuePtr *actions, + char ***disabledBitmapsBase) +{ + g_autoptr(virJSONValue) act = virJSONValueNewArray(); + VIR_AUTOSTRINGLIST bitmaplist = NULL; + size_t curbitmapstr = 0; + qemuBlockNamedNodeDataPtr entry; + bool disable_bitmaps = false; + size_t i; + + if (!(entry = virHashLookup(blockNamedNodeData, basesrc->nodeformat))) + return 0; + + bitmaplist = g_new0(char *, entry->nbitmaps); + + for (i = 0; i < entry->nbitmaps; i++) { + qemuBlockNamedNodeDataBitmapPtr bitmap = entry->bitmaps[i]; + + if (!bitmap->recording || bitmap->inconsistent || + !qemuBlockBitmapChainIsValid(topsrc, bitmap->name, blockNamedNodeData)) + continue; + + disable_bitmaps = true; + + if (qemuMonitorTransactionBitmapDisable(act, basesrc->nodeformat, + bitmap->name) < 0) + return -1; + + bitmaplist[curbitmapstr++] = g_strdup(bitmap->name); + } + + if (disable_bitmaps) { + *actions = g_steal_pointer(&act); + *disabledBitmapsBase = g_steal_pointer(&bitmaplist); + } + + return 0; +} + + +struct qemuBlockBitmapsHandleCommitData { + bool skip; + bool create; + bool enable; + const char *basenode; + virJSONValuePtr merge; + unsigned long long granularity; + bool persistent; +}; + + +static void +qemuBlockBitmapsHandleCommitDataFree(void *opaque) +{ + struct qemuBlockBitmapsHandleCommitData *data = opaque; + + virJSONValueFree(data->merge); + g_free(data); +} + + +static int +qemuBlockBitmapsHandleCommitFinishIterate(void *payload, + const void *entryname, + void *opaque) +{ + struct qemuBlockBitmapsHandleCommitData *data = payload; + const char *bitmapname = entryname; + virJSONValuePtr actions = opaque; + + if (data->skip) + return 0; + + if (data->create) { + if (qemuMonitorTransactionBitmapAdd(actions, data->basenode, bitmapname, + data->persistent, !data-> enable, + data->granularity) < 0) + return -1; + } else { + if (data->enable && + qemuMonitorTransactionBitmapEnable(actions, data->basenode, bitmapname) < 0) + return -1; + } + + if (data->merge && + qemuMonitorTransactionBitmapMerge(actions, data->basenode, bitmapname, + &data->merge) < 0) + return -1; + + return 0; +} + + +/** + * @topsrc: virStorageSource representing 'top' of the job + * @basesrc: virStorageSource representing 'base' of the job + * @blockNamedNodeData: hash table containing data about bitmaps + * @actions: filled with arguments for a 'transaction' command + * @disabledBitmapsBase: bitmap names which were disabled + * + * Calculates the necessary bitmap merges/additions/enablements to properly + * handle commit of images from 'top' into 'base'. The necessary operations + * in form of argumets of the 'transaction' command are filled into 'actions' + * if there is anything to do. Otherwise NULL is returned. + */ +int +qemuBlockBitmapsHandleCommitFinish(virStorageSourcePtr topsrc, + virStorageSourcePtr basesrc, + virHashTablePtr blockNamedNodeData, + virJSONValuePtr *actions, + char **disabledBitmapsBase) +{ + g_autoptr(virJSONValue) act = virJSONValueNewArray(); + virStorageSourcePtr n; + qemuBlockNamedNodeDataPtr entry; + g_autoptr(virHashTable) commitdata = NULL; + struct qemuBlockBitmapsHandleCommitData *bitmapdata; + size_t i; + + commitdata = virHashNew(qemuBlockBitmapsHandleCommitDataFree); + + for (n = topsrc; n != basesrc; n = n->backingStore) { + if (!(entry = virHashLookup(blockNamedNodeData, n->nodeformat))) + continue; + + for (i = 0; i < entry->nbitmaps; i++) { + qemuBlockNamedNodeDataBitmapPtr bitmap = entry->bitmaps[i]; + + if (!(bitmapdata = virHashLookup(commitdata, bitmap->name))) { + bitmapdata = g_new0(struct qemuBlockBitmapsHandleCommitData, 1); + + /* we must mirror the state of the topmost bitmap and merge + * everything else */ + bitmapdata->create = true; + bitmapdata->enable = bitmap->recording; + bitmapdata->basenode = basesrc->nodeformat; + bitmapdata->merge = virJSONValueNewArray(); + bitmapdata->granularity = bitmap->granularity; + bitmapdata->persistent = bitmap->persistent; + + if (virHashAddEntry(commitdata, bitmap->name, bitmapdata) < 0) { + qemuBlockBitmapsHandleCommitDataFree(bitmapdata); + return -1; + } + } + + if (bitmap->inconsistent || + !qemuBlockBitmapChainIsValid(topsrc, bitmap->name, blockNamedNodeData)) + bitmapdata->skip = true; + + if (qemuMonitorTransactionBitmapMergeSourceAddBitmap(bitmapdata->merge, + n->nodeformat, + bitmap->name) < 0) + return -1; + } + } + + if ((entry = virHashLookup(blockNamedNodeData, basesrc->nodeformat))) { + /* note that all bitmaps in 'base' were disabled when commit was started */ + for (i = 0; i < entry->nbitmaps; i++) { + qemuBlockNamedNodeDataBitmapPtr bitmap = entry->bitmaps[i]; + + if ((bitmapdata = virHashLookup(commitdata, bitmap->name))) { + bitmapdata->create = false; + } else { + if (disabledBitmapsBase) { + char **disabledbitmaps; + + for (disabledbitmaps = disabledBitmapsBase; *disabledbitmaps; disabledbitmaps++) { + if (STREQ(*disabledBitmapsBase, bitmap->name)) { + bitmapdata = g_new0(struct qemuBlockBitmapsHandleCommitData, 1); + + bitmapdata->create = false; + bitmapdata->enable = true; + bitmapdata->basenode = basesrc->nodeformat; + bitmapdata->granularity = bitmap->granularity; + bitmapdata->persistent = bitmap->persistent; + + if (virHashAddEntry(commitdata, bitmap->name, bitmapdata) < 0) { + qemuBlockBitmapsHandleCommitDataFree(bitmapdata); + return -1; + } + + break; + } + } + } + } + } + } + + if (virHashForEach(commitdata, qemuBlockBitmapsHandleCommitFinishIterate, act) < 0) + return -1; + + if (virJSONValueArraySize(act) > 0) + *actions = g_steal_pointer(&act); + + return 0; +} + + /** * qemuBlockReopenFormat: * @vm: domain object diff --git a/src/qemu/qemu_block.h b/src/qemu/qemu_block.h index 1d8a364bd0..ac666ffb3a 100644 --- a/src/qemu/qemu_block.h +++ b/src/qemu/qemu_block.h @@ -229,6 +229,20 @@ qemuBlockBitmapsHandleBlockcopy(virStorageSourcePtr src, bool shallow, virJSONValuePtr *actions); +int +qemuBlockBitmapsHandleCommitStart(virStorageSourcePtr topsrc, + virStorageSourcePtr basesrc, + virHashTablePtr blockNamedNodeData, + virJSONValuePtr *actions, + char ***disabledBitmapsBase); + +int +qemuBlockBitmapsHandleCommitFinish(virStorageSourcePtr topsrc, + virStorageSourcePtr basesrc, + virHashTablePtr blockNamedNodeData, + virJSONValuePtr *actions, + char **disabledBitmapsBase); + int qemuBlockReopenReadWrite(virDomainObjPtr vm, virStorageSourcePtr src, -- 2.24.1
On 3/11/20 7:55 AM, Peter Krempa wrote:
qemuBlockBitmapsHandleCommitStart prepares for disabling the bitmaps in the 'base' of the commit job so that the bitmaps are not dirtied by the commit job. This needs to be done prior to start of the commit job.
qemuBlockBitmapsHandleCommitFinish then calculates the necessary merges that agregate all the bitmaps between the commited images and write them into the base bitmap.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_block.c | 219 ++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_block.h | 14 +++ 2 files changed, 233 insertions(+)
diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c index edebbcd0ce..6853c021ca 100644 --- a/src/qemu/qemu_block.c +++ b/src/qemu/qemu_block.c @@ -2962,6 +2962,225 @@ qemuBlockBitmapsHandleBlockcopy(virStorageSourcePtr src, }
+/** + * @topsrc: virStorageSource representing 'top' of the job + * @basesrc: virStorageSource representing 'base' of the job + * @blockNamedNodeData: hash table containing data about bitmaps + * @actions: filled with arguments for a 'transaction' command + * @disabledBitmapsBase: filled with a list of bitmap names which must be disabled + * + * Prepares data for correctly hanlding bitmaps during the start of a commit
handling
+ * job. The bitmaps in the 'base' image must be disabled, so that the writes + * done by the blockjob don't dirty the enabled bitmaps. + * + * @actions and @disabledBitmapsBase are untouched if no bitmaps need + * to be disabled. + */ +int +qemuBlockBitmapsHandleCommitStart(virStorageSourcePtr topsrc, + virStorageSourcePtr basesrc, + virHashTablePtr blockNamedNodeData, + virJSONValuePtr *actions, + char ***disabledBitmapsBase) +{
+static int +qemuBlockBitmapsHandleCommitFinishIterate(void *payload, + const void *entryname, + void *opaque) +{ + struct qemuBlockBitmapsHandleCommitData *data = payload; + const char *bitmapname = entryname; + virJSONValuePtr actions = opaque; + + if (data->skip) + return 0; + + if (data->create) { + if (qemuMonitorTransactionBitmapAdd(actions, data->basenode, bitmapname, + data->persistent, !data-> enable,
Spurious space.
+/** + * @topsrc: virStorageSource representing 'top' of the job + * @basesrc: virStorageSource representing 'base' of the job + * @blockNamedNodeData: hash table containing data about bitmaps + * @actions: filled with arguments for a 'transaction' command + * @disabledBitmapsBase: bitmap names which were disabled + * + * Calculates the necessary bitmap merges/additions/enablements to properly + * handle commit of images from 'top' into 'base'. The necessary operations + * in form of argumets of the 'transaction' command are filled into 'actions'
typo and grammar: in the form of arguments to
+ * if there is anything to do. Otherwise NULL is returned. + */ +int +qemuBlockBitmapsHandleCommitFinish(virStorageSourcePtr topsrc, + virStorageSourcePtr basesrc, + virHashTablePtr blockNamedNodeData, + virJSONValuePtr *actions, + char **disabledBitmapsBase) +{
+++ b/src/qemu/qemu_block.h @@ -229,6 +229,20 @@ qemuBlockBitmapsHandleBlockcopy(virStorageSourcePtr src, bool shallow, virJSONValuePtr *actions);
+int +qemuBlockBitmapsHandleCommitStart(virStorageSourcePtr topsrc, + virStorageSourcePtr basesrc, + virHashTablePtr blockNamedNodeData, + virJSONValuePtr *actions, + char ***disabledBitmapsBase); + +int +qemuBlockBitmapsHandleCommitFinish(virStorageSourcePtr topsrc, + virStorageSourcePtr basesrc, + virHashTablePtr blockNamedNodeData, + virJSONValuePtr *actions, + char **disabledBitmapsBase); + int qemuBlockReopenReadWrite(virDomainObjPtr vm, virStorageSourcePtr src,
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 code for testing the two necessary steps of handling bitmaps during block commit and excercise the code on the test data which we have for bitmap handling. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/qemublocktest.c | 95 ++++++++++++++ .../bitmapblockcommit/basic-1-2 | 119 ++++++++++++++++++ .../bitmapblockcommit/basic-1-3 | 119 ++++++++++++++++++ .../bitmapblockcommit/basic-2-3 | 2 + 4 files changed, 335 insertions(+) create mode 100644 tests/qemublocktestdata/bitmapblockcommit/basic-1-2 create mode 100644 tests/qemublocktestdata/bitmapblockcommit/basic-1-3 create mode 100644 tests/qemublocktestdata/bitmapblockcommit/basic-2-3 diff --git a/tests/qemublocktest.c b/tests/qemublocktest.c index a6b6376c7d..3662fee42a 100644 --- a/tests/qemublocktest.c +++ b/tests/qemublocktest.c @@ -599,6 +599,21 @@ testQemuBackupIncrementalBitmapCalculateGetFakeChain(void) } +static virStorageSourcePtr +testQemuBitmapGetFakeChainEntry(virStorageSourcePtr src, + size_t idx) +{ + virStorageSourcePtr n; + + for (n = src; n; n = n->backingStore) { + if (n->id == idx) + return n; + } + + return NULL; +} + + typedef virDomainMomentDefPtr testMomentList; static void @@ -853,6 +868,68 @@ testQemuBlockBitmapBlockcopy(const void *opaque) return virTestCompareToFile(actual, expectpath); } +static const char *blockcommitPrefix = "qemublocktestdata/bitmapblockcommit/"; + +struct testQemuBlockBitmapBlockcommitData { + const char *name; + virStorageSourcePtr top; + virStorageSourcePtr base; + virStorageSourcePtr chain; + const char *nodedatafile; +}; + + +static int +testQemuBlockBitmapBlockcommit(const void *opaque) +{ + const struct testQemuBlockBitmapBlockcommitData *data = opaque; + + g_autofree char *actual = NULL; + g_autofree char *expectpath = NULL; + g_autoptr(virJSONValue) actionsDisable = NULL; + g_autoptr(virJSONValue) actionsMerge = NULL; + g_autoptr(virJSONValue) nodedatajson = NULL; + g_autoptr(virHashTable) nodedata = NULL; + g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER; + VIR_AUTOSTRINGLIST bitmapsDisable = NULL; + + expectpath = g_strdup_printf("%s/%s%s", abs_srcdir, + blockcommitPrefix, data->name); + + if (!(nodedatajson = virTestLoadFileJSON(bitmapDetectPrefix, data->nodedatafile, + ".json", NULL))) + return -1; + + if (!(nodedata = qemuMonitorJSONBlockGetNamedNodeDataJSON(nodedatajson))) { + VIR_TEST_VERBOSE("failed to load nodedata JSON\n"); + return -1; + } + + if (qemuBlockBitmapsHandleCommitStart(data->top, data->base, nodedata, + &actionsDisable, &bitmapsDisable) < 0) + return -1; + + virBufferAddLit(&buf, "pre job bitmap disable:\n"); + + if (actionsDisable && + virJSONValueToBuffer(actionsDisable, &buf, true) < 0) + return -1; + + virBufferAddLit(&buf, "merge bitmpas:\n"); + + if (qemuBlockBitmapsHandleCommitFinish(data->top, data->base, nodedata, + &actionsMerge, bitmapsDisable) < 0) + return -1; + + if (actionsMerge && + virJSONValueToBuffer(actionsMerge, &buf, true) < 0) + return -1; + + actual = virBufferContentAndReset(&buf); + + return virTestCompareToFile(actual, expectpath); +} + static int mymain(void) @@ -866,6 +943,7 @@ mymain(void) struct testQemuCheckpointDeleteMergeData checkpointdeletedata; struct testQemuBlockBitmapValidateData blockbitmapvalidatedata; struct testQemuBlockBitmapBlockcopyData blockbitmapblockcopydata; + struct testQemuBlockBitmapBlockcommitData blockbitmapblockcommitdata; char *capslatest_x86_64 = NULL; virQEMUCapsPtr caps_x86_64 = NULL; g_autoptr(virStorageSource) bitmapSourceChain = NULL; @@ -1196,6 +1274,23 @@ mymain(void) TEST_BITMAP_BLOCKCOPY("snapshots-shallow", true, "snapshots"); TEST_BITMAP_BLOCKCOPY("snapshots-deep", false, "snapshots"); + +#define TEST_BITMAP_BLOCKCOMMIT(testname, topimg, baseimg, ndf) \ + do {\ + blockbitmapblockcommitdata.name = testname; \ + blockbitmapblockcommitdata.top = testQemuBitmapGetFakeChainEntry(bitmapSourceChain, topimg); \ + blockbitmapblockcommitdata.base = testQemuBitmapGetFakeChainEntry(bitmapSourceChain, baseimg); \ + blockbitmapblockcommitdata.nodedatafile = ndf; \ + if (virTestRun("bitmap block commit " testname, \ + testQemuBlockBitmapBlockcommit, \ + &blockbitmapblockcommitdata) < 0) \ + ret = -1; \ + } while (0) + + TEST_BITMAP_BLOCKCOMMIT("basic-1-2", 1, 2, "basic"); + TEST_BITMAP_BLOCKCOMMIT("basic-1-3", 1, 3, "basic"); + TEST_BITMAP_BLOCKCOMMIT("basic-2-3", 2, 3, "basic"); + cleanup: virHashFree(diskxmljsondata.schema); qemuTestDriverFree(&driver); diff --git a/tests/qemublocktestdata/bitmapblockcommit/basic-1-2 b/tests/qemublocktestdata/bitmapblockcommit/basic-1-2 new file mode 100644 index 0000000000..8eeb4c3a11 --- /dev/null +++ b/tests/qemublocktestdata/bitmapblockcommit/basic-1-2 @@ -0,0 +1,119 @@ +pre job bitmap disable: +merge bitmpas: +[ + { + "type": "block-dirty-bitmap-add", + "data": { + "node": "libvirt-2-format", + "name": "a", + "persistent": true, + "disabled": true, + "granularity": 65536 + } + }, + { + "type": "block-dirty-bitmap-merge", + "data": { + "node": "libvirt-2-format", + "target": "a", + "bitmaps": [ + { + "node": "libvirt-1-format", + "name": "a" + } + ] + } + }, + { + "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-1-format", + "name": "b" + } + ] + } + }, + { + "type": "block-dirty-bitmap-add", + "data": { + "node": "libvirt-2-format", + "name": "current", + "persistent": true, + "disabled": false, + "granularity": 65536 + } + }, + { + "type": "block-dirty-bitmap-merge", + "data": { + "node": "libvirt-2-format", + "target": "current", + "bitmaps": [ + { + "node": "libvirt-1-format", + "name": "current" + } + ] + } + }, + { + "type": "block-dirty-bitmap-add", + "data": { + "node": "libvirt-2-format", + "name": "c", + "persistent": true, + "disabled": true, + "granularity": 65536 + } + }, + { + "type": "block-dirty-bitmap-merge", + "data": { + "node": "libvirt-2-format", + "target": "c", + "bitmaps": [ + { + "node": "libvirt-1-format", + "name": "c" + } + ] + } + }, + { + "type": "block-dirty-bitmap-add", + "data": { + "node": "libvirt-2-format", + "name": "d", + "persistent": true, + "disabled": true, + "granularity": 65536 + } + }, + { + "type": "block-dirty-bitmap-merge", + "data": { + "node": "libvirt-2-format", + "target": "d", + "bitmaps": [ + { + "node": "libvirt-1-format", + "name": "d" + } + ] + } + } +] diff --git a/tests/qemublocktestdata/bitmapblockcommit/basic-1-3 b/tests/qemublocktestdata/bitmapblockcommit/basic-1-3 new file mode 100644 index 0000000000..71b48e31a5 --- /dev/null +++ b/tests/qemublocktestdata/bitmapblockcommit/basic-1-3 @@ -0,0 +1,119 @@ +pre job bitmap disable: +merge bitmpas: +[ + { + "type": "block-dirty-bitmap-add", + "data": { + "node": "libvirt-3-format", + "name": "a", + "persistent": true, + "disabled": true, + "granularity": 65536 + } + }, + { + "type": "block-dirty-bitmap-merge", + "data": { + "node": "libvirt-3-format", + "target": "a", + "bitmaps": [ + { + "node": "libvirt-1-format", + "name": "a" + } + ] + } + }, + { + "type": "block-dirty-bitmap-add", + "data": { + "node": "libvirt-3-format", + "name": "b", + "persistent": true, + "disabled": true, + "granularity": 65536 + } + }, + { + "type": "block-dirty-bitmap-merge", + "data": { + "node": "libvirt-3-format", + "target": "b", + "bitmaps": [ + { + "node": "libvirt-1-format", + "name": "b" + } + ] + } + }, + { + "type": "block-dirty-bitmap-add", + "data": { + "node": "libvirt-3-format", + "name": "current", + "persistent": true, + "disabled": false, + "granularity": 65536 + } + }, + { + "type": "block-dirty-bitmap-merge", + "data": { + "node": "libvirt-3-format", + "target": "current", + "bitmaps": [ + { + "node": "libvirt-1-format", + "name": "current" + } + ] + } + }, + { + "type": "block-dirty-bitmap-add", + "data": { + "node": "libvirt-3-format", + "name": "c", + "persistent": true, + "disabled": true, + "granularity": 65536 + } + }, + { + "type": "block-dirty-bitmap-merge", + "data": { + "node": "libvirt-3-format", + "target": "c", + "bitmaps": [ + { + "node": "libvirt-1-format", + "name": "c" + } + ] + } + }, + { + "type": "block-dirty-bitmap-add", + "data": { + "node": "libvirt-3-format", + "name": "d", + "persistent": true, + "disabled": true, + "granularity": 65536 + } + }, + { + "type": "block-dirty-bitmap-merge", + "data": { + "node": "libvirt-3-format", + "target": "d", + "bitmaps": [ + { + "node": "libvirt-1-format", + "name": "d" + } + ] + } + } +] diff --git a/tests/qemublocktestdata/bitmapblockcommit/basic-2-3 b/tests/qemublocktestdata/bitmapblockcommit/basic-2-3 new file mode 100644 index 0000000000..bfc58f994e --- /dev/null +++ b/tests/qemublocktestdata/bitmapblockcommit/basic-2-3 @@ -0,0 +1,2 @@ +pre job bitmap disable: +merge bitmpas: -- 2.24.1
On 3/11/20 7:55 AM, Peter Krempa wrote:
Add code for testing the two necessary steps of handling bitmaps during block commit and excercise the code on the test data which we have for
exercise
bitmap handling.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/qemublocktest.c | 95 ++++++++++++++ .../bitmapblockcommit/basic-1-2 | 119 ++++++++++++++++++ .../bitmapblockcommit/basic-1-3 | 119 ++++++++++++++++++ .../bitmapblockcommit/basic-2-3 | 2 + 4 files changed, 335 insertions(+) create mode 100644 tests/qemublocktestdata/bitmapblockcommit/basic-1-2 create mode 100644 tests/qemublocktestdata/bitmapblockcommit/basic-1-3 create mode 100644 tests/qemublocktestdata/bitmapblockcommit/basic-2-3
Impacted if my concerns earlier in the series about even needing to pre-disable bitmaps prior to a commit causes a change. But without a design change, this looks reasonable for covering the code just added. Reviewed-by: Eric Blake <eblake@redhat.com> -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Test handling of more complex cases of merging bitmaps accross snapshots. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/qemublocktest.c | 15 ++ .../bitmapblockcommit/snapshots-1-2 | 49 +++++++ .../bitmapblockcommit/snapshots-1-3 | 76 ++++++++++ .../bitmapblockcommit/snapshots-1-4 | 126 +++++++++++++++++ .../bitmapblockcommit/snapshots-1-5 | 130 ++++++++++++++++++ .../bitmapblockcommit/snapshots-2-3 | 49 +++++++ .../bitmapblockcommit/snapshots-2-4 | 99 +++++++++++++ .../bitmapblockcommit/snapshots-2-5 | 103 ++++++++++++++ .../bitmapblockcommit/snapshots-3-4 | 72 ++++++++++ .../bitmapblockcommit/snapshots-3-5 | 76 ++++++++++ .../bitmapblockcommit/snapshots-4-4 | 11 ++ .../bitmapblockcommit/snapshots-4-5 | 33 +++++ 12 files changed, 839 insertions(+) create mode 100644 tests/qemublocktestdata/bitmapblockcommit/snapshots-1-2 create mode 100644 tests/qemublocktestdata/bitmapblockcommit/snapshots-1-3 create mode 100644 tests/qemublocktestdata/bitmapblockcommit/snapshots-1-4 create mode 100644 tests/qemublocktestdata/bitmapblockcommit/snapshots-1-5 create mode 100644 tests/qemublocktestdata/bitmapblockcommit/snapshots-2-3 create mode 100644 tests/qemublocktestdata/bitmapblockcommit/snapshots-2-4 create mode 100644 tests/qemublocktestdata/bitmapblockcommit/snapshots-2-5 create mode 100644 tests/qemublocktestdata/bitmapblockcommit/snapshots-3-4 create mode 100644 tests/qemublocktestdata/bitmapblockcommit/snapshots-3-5 create mode 100644 tests/qemublocktestdata/bitmapblockcommit/snapshots-4-4 create mode 100644 tests/qemublocktestdata/bitmapblockcommit/snapshots-4-5 diff --git a/tests/qemublocktest.c b/tests/qemublocktest.c index 3662fee42a..5eb38c3981 100644 --- a/tests/qemublocktest.c +++ b/tests/qemublocktest.c @@ -1291,6 +1291,21 @@ mymain(void) TEST_BITMAP_BLOCKCOMMIT("basic-1-3", 1, 3, "basic"); TEST_BITMAP_BLOCKCOMMIT("basic-2-3", 2, 3, "basic"); + TEST_BITMAP_BLOCKCOMMIT("snapshots-1-2", 1, 2, "snapshots"); + TEST_BITMAP_BLOCKCOMMIT("snapshots-1-3", 1, 3, "snapshots"); + TEST_BITMAP_BLOCKCOMMIT("snapshots-1-4", 1, 4, "snapshots"); + TEST_BITMAP_BLOCKCOMMIT("snapshots-1-5", 1, 5, "snapshots"); + + TEST_BITMAP_BLOCKCOMMIT("snapshots-2-3", 2, 3, "snapshots"); + TEST_BITMAP_BLOCKCOMMIT("snapshots-2-4", 2, 4, "snapshots"); + TEST_BITMAP_BLOCKCOMMIT("snapshots-2-5", 2, 5, "snapshots"); + + TEST_BITMAP_BLOCKCOMMIT("snapshots-3-4", 3, 4, "snapshots"); + TEST_BITMAP_BLOCKCOMMIT("snapshots-3-5", 3, 5, "snapshots"); + + TEST_BITMAP_BLOCKCOMMIT("snapshots-4-5", 4, 5, "snapshots"); + + cleanup: virHashFree(diskxmljsondata.schema); qemuTestDriverFree(&driver); diff --git a/tests/qemublocktestdata/bitmapblockcommit/snapshots-1-2 b/tests/qemublocktestdata/bitmapblockcommit/snapshots-1-2 new file mode 100644 index 0000000000..0015b9ceb3 --- /dev/null +++ b/tests/qemublocktestdata/bitmapblockcommit/snapshots-1-2 @@ -0,0 +1,49 @@ +pre job bitmap disable: +[ + { + "type": "block-dirty-bitmap-disable", + "data": { + "node": "libvirt-2-format", + "name": "d" + } + } +] +merge bitmpas: +[ + { + "type": "block-dirty-bitmap-add", + "data": { + "node": "libvirt-2-format", + "name": "current", + "persistent": true, + "disabled": false, + "granularity": 65536 + } + }, + { + "type": "block-dirty-bitmap-merge", + "data": { + "node": "libvirt-2-format", + "target": "current", + "bitmaps": [ + { + "node": "libvirt-1-format", + "name": "current" + } + ] + } + }, + { + "type": "block-dirty-bitmap-merge", + "data": { + "node": "libvirt-2-format", + "target": "d", + "bitmaps": [ + { + "node": "libvirt-1-format", + "name": "d" + } + ] + } + } +] diff --git a/tests/qemublocktestdata/bitmapblockcommit/snapshots-1-3 b/tests/qemublocktestdata/bitmapblockcommit/snapshots-1-3 new file mode 100644 index 0000000000..5691b408aa --- /dev/null +++ b/tests/qemublocktestdata/bitmapblockcommit/snapshots-1-3 @@ -0,0 +1,76 @@ +pre job bitmap disable: +[ + { + "type": "block-dirty-bitmap-disable", + "data": { + "node": "libvirt-3-format", + "name": "c" + } + } +] +merge bitmpas: +[ + { + "type": "block-dirty-bitmap-add", + "data": { + "node": "libvirt-3-format", + "name": "current", + "persistent": true, + "disabled": false, + "granularity": 65536 + } + }, + { + "type": "block-dirty-bitmap-merge", + "data": { + "node": "libvirt-3-format", + "target": "current", + "bitmaps": [ + { + "node": "libvirt-1-format", + "name": "current" + } + ] + } + }, + { + "type": "block-dirty-bitmap-merge", + "data": { + "node": "libvirt-3-format", + "target": "c", + "bitmaps": [ + { + "node": "libvirt-2-format", + "name": "c" + } + ] + } + }, + { + "type": "block-dirty-bitmap-add", + "data": { + "node": "libvirt-3-format", + "name": "d", + "persistent": true, + "disabled": true, + "granularity": 65536 + } + }, + { + "type": "block-dirty-bitmap-merge", + "data": { + "node": "libvirt-3-format", + "target": "d", + "bitmaps": [ + { + "node": "libvirt-1-format", + "name": "d" + }, + { + "node": "libvirt-2-format", + "name": "d" + } + ] + } + } +] diff --git a/tests/qemublocktestdata/bitmapblockcommit/snapshots-1-4 b/tests/qemublocktestdata/bitmapblockcommit/snapshots-1-4 new file mode 100644 index 0000000000..454001531a --- /dev/null +++ b/tests/qemublocktestdata/bitmapblockcommit/snapshots-1-4 @@ -0,0 +1,126 @@ +pre job bitmap disable: +[ + { + "type": "block-dirty-bitmap-disable", + "data": { + "node": "libvirt-4-format", + "name": "a" + } + } +] +merge bitmpas: +[ + { + "type": "block-dirty-bitmap-merge", + "data": { + "node": "libvirt-4-format", + "target": "a", + "bitmaps": [ + { + "node": "libvirt-3-format", + "name": "a" + } + ] + } + }, + { + "type": "block-dirty-bitmap-add", + "data": { + "node": "libvirt-4-format", + "name": "b", + "persistent": true, + "disabled": true, + "granularity": 65536 + } + }, + { + "type": "block-dirty-bitmap-merge", + "data": { + "node": "libvirt-4-format", + "target": "b", + "bitmaps": [ + { + "node": "libvirt-3-format", + "name": "b" + } + ] + } + }, + { + "type": "block-dirty-bitmap-add", + "data": { + "node": "libvirt-4-format", + "name": "current", + "persistent": true, + "disabled": false, + "granularity": 65536 + } + }, + { + "type": "block-dirty-bitmap-merge", + "data": { + "node": "libvirt-4-format", + "target": "current", + "bitmaps": [ + { + "node": "libvirt-1-format", + "name": "current" + } + ] + } + }, + { + "type": "block-dirty-bitmap-add", + "data": { + "node": "libvirt-4-format", + "name": "c", + "persistent": true, + "disabled": true, + "granularity": 65536 + } + }, + { + "type": "block-dirty-bitmap-merge", + "data": { + "node": "libvirt-4-format", + "target": "c", + "bitmaps": [ + { + "node": "libvirt-2-format", + "name": "c" + }, + { + "node": "libvirt-3-format", + "name": "c" + } + ] + } + }, + { + "type": "block-dirty-bitmap-add", + "data": { + "node": "libvirt-4-format", + "name": "d", + "persistent": true, + "disabled": true, + "granularity": 65536 + } + }, + { + "type": "block-dirty-bitmap-merge", + "data": { + "node": "libvirt-4-format", + "target": "d", + "bitmaps": [ + { + "node": "libvirt-1-format", + "name": "d" + }, + { + "node": "libvirt-2-format", + "name": "d" + } + ] + } + } +] diff --git a/tests/qemublocktestdata/bitmapblockcommit/snapshots-1-5 b/tests/qemublocktestdata/bitmapblockcommit/snapshots-1-5 new file mode 100644 index 0000000000..2fd43d7917 --- /dev/null +++ b/tests/qemublocktestdata/bitmapblockcommit/snapshots-1-5 @@ -0,0 +1,130 @@ +pre job bitmap disable: +[ + { + "type": "block-dirty-bitmap-disable", + "data": { + "node": "libvirt-5-format", + "name": "a" + } + } +] +merge bitmpas: +[ + { + "type": "block-dirty-bitmap-merge", + "data": { + "node": "libvirt-5-format", + "target": "a", + "bitmaps": [ + { + "node": "libvirt-3-format", + "name": "a" + }, + { + "node": "libvirt-4-format", + "name": "a" + } + ] + } + }, + { + "type": "block-dirty-bitmap-add", + "data": { + "node": "libvirt-5-format", + "name": "b", + "persistent": true, + "disabled": true, + "granularity": 65536 + } + }, + { + "type": "block-dirty-bitmap-merge", + "data": { + "node": "libvirt-5-format", + "target": "b", + "bitmaps": [ + { + "node": "libvirt-3-format", + "name": "b" + } + ] + } + }, + { + "type": "block-dirty-bitmap-add", + "data": { + "node": "libvirt-5-format", + "name": "current", + "persistent": true, + "disabled": false, + "granularity": 65536 + } + }, + { + "type": "block-dirty-bitmap-merge", + "data": { + "node": "libvirt-5-format", + "target": "current", + "bitmaps": [ + { + "node": "libvirt-1-format", + "name": "current" + } + ] + } + }, + { + "type": "block-dirty-bitmap-add", + "data": { + "node": "libvirt-5-format", + "name": "c", + "persistent": true, + "disabled": true, + "granularity": 65536 + } + }, + { + "type": "block-dirty-bitmap-merge", + "data": { + "node": "libvirt-5-format", + "target": "c", + "bitmaps": [ + { + "node": "libvirt-2-format", + "name": "c" + }, + { + "node": "libvirt-3-format", + "name": "c" + } + ] + } + }, + { + "type": "block-dirty-bitmap-add", + "data": { + "node": "libvirt-5-format", + "name": "d", + "persistent": true, + "disabled": true, + "granularity": 65536 + } + }, + { + "type": "block-dirty-bitmap-merge", + "data": { + "node": "libvirt-5-format", + "target": "d", + "bitmaps": [ + { + "node": "libvirt-1-format", + "name": "d" + }, + { + "node": "libvirt-2-format", + "name": "d" + } + ] + } + } +] diff --git a/tests/qemublocktestdata/bitmapblockcommit/snapshots-2-3 b/tests/qemublocktestdata/bitmapblockcommit/snapshots-2-3 new file mode 100644 index 0000000000..d719a90bd7 --- /dev/null +++ b/tests/qemublocktestdata/bitmapblockcommit/snapshots-2-3 @@ -0,0 +1,49 @@ +pre job bitmap disable: +[ + { + "type": "block-dirty-bitmap-disable", + "data": { + "node": "libvirt-3-format", + "name": "c" + } + } +] +merge bitmpas: +[ + { + "type": "block-dirty-bitmap-merge", + "data": { + "node": "libvirt-3-format", + "target": "c", + "bitmaps": [ + { + "node": "libvirt-2-format", + "name": "c" + } + ] + } + }, + { + "type": "block-dirty-bitmap-add", + "data": { + "node": "libvirt-3-format", + "name": "d", + "persistent": true, + "disabled": false, + "granularity": 65536 + } + }, + { + "type": "block-dirty-bitmap-merge", + "data": { + "node": "libvirt-3-format", + "target": "d", + "bitmaps": [ + { + "node": "libvirt-2-format", + "name": "d" + } + ] + } + } +] diff --git a/tests/qemublocktestdata/bitmapblockcommit/snapshots-2-4 b/tests/qemublocktestdata/bitmapblockcommit/snapshots-2-4 new file mode 100644 index 0000000000..9e37962344 --- /dev/null +++ b/tests/qemublocktestdata/bitmapblockcommit/snapshots-2-4 @@ -0,0 +1,99 @@ +pre job bitmap disable: +[ + { + "type": "block-dirty-bitmap-disable", + "data": { + "node": "libvirt-4-format", + "name": "a" + } + } +] +merge bitmpas: +[ + { + "type": "block-dirty-bitmap-merge", + "data": { + "node": "libvirt-4-format", + "target": "a", + "bitmaps": [ + { + "node": "libvirt-3-format", + "name": "a" + } + ] + } + }, + { + "type": "block-dirty-bitmap-add", + "data": { + "node": "libvirt-4-format", + "name": "b", + "persistent": true, + "disabled": true, + "granularity": 65536 + } + }, + { + "type": "block-dirty-bitmap-merge", + "data": { + "node": "libvirt-4-format", + "target": "b", + "bitmaps": [ + { + "node": "libvirt-3-format", + "name": "b" + } + ] + } + }, + { + "type": "block-dirty-bitmap-add", + "data": { + "node": "libvirt-4-format", + "name": "c", + "persistent": true, + "disabled": true, + "granularity": 65536 + } + }, + { + "type": "block-dirty-bitmap-merge", + "data": { + "node": "libvirt-4-format", + "target": "c", + "bitmaps": [ + { + "node": "libvirt-2-format", + "name": "c" + }, + { + "node": "libvirt-3-format", + "name": "c" + } + ] + } + }, + { + "type": "block-dirty-bitmap-add", + "data": { + "node": "libvirt-4-format", + "name": "d", + "persistent": true, + "disabled": false, + "granularity": 65536 + } + }, + { + "type": "block-dirty-bitmap-merge", + "data": { + "node": "libvirt-4-format", + "target": "d", + "bitmaps": [ + { + "node": "libvirt-2-format", + "name": "d" + } + ] + } + } +] diff --git a/tests/qemublocktestdata/bitmapblockcommit/snapshots-2-5 b/tests/qemublocktestdata/bitmapblockcommit/snapshots-2-5 new file mode 100644 index 0000000000..d6b20a5d05 --- /dev/null +++ b/tests/qemublocktestdata/bitmapblockcommit/snapshots-2-5 @@ -0,0 +1,103 @@ +pre job bitmap disable: +[ + { + "type": "block-dirty-bitmap-disable", + "data": { + "node": "libvirt-5-format", + "name": "a" + } + } +] +merge bitmpas: +[ + { + "type": "block-dirty-bitmap-merge", + "data": { + "node": "libvirt-5-format", + "target": "a", + "bitmaps": [ + { + "node": "libvirt-3-format", + "name": "a" + }, + { + "node": "libvirt-4-format", + "name": "a" + } + ] + } + }, + { + "type": "block-dirty-bitmap-add", + "data": { + "node": "libvirt-5-format", + "name": "b", + "persistent": true, + "disabled": true, + "granularity": 65536 + } + }, + { + "type": "block-dirty-bitmap-merge", + "data": { + "node": "libvirt-5-format", + "target": "b", + "bitmaps": [ + { + "node": "libvirt-3-format", + "name": "b" + } + ] + } + }, + { + "type": "block-dirty-bitmap-add", + "data": { + "node": "libvirt-5-format", + "name": "c", + "persistent": true, + "disabled": true, + "granularity": 65536 + } + }, + { + "type": "block-dirty-bitmap-merge", + "data": { + "node": "libvirt-5-format", + "target": "c", + "bitmaps": [ + { + "node": "libvirt-2-format", + "name": "c" + }, + { + "node": "libvirt-3-format", + "name": "c" + } + ] + } + }, + { + "type": "block-dirty-bitmap-add", + "data": { + "node": "libvirt-5-format", + "name": "d", + "persistent": true, + "disabled": false, + "granularity": 65536 + } + }, + { + "type": "block-dirty-bitmap-merge", + "data": { + "node": "libvirt-5-format", + "target": "d", + "bitmaps": [ + { + "node": "libvirt-2-format", + "name": "d" + } + ] + } + } +] diff --git a/tests/qemublocktestdata/bitmapblockcommit/snapshots-3-4 b/tests/qemublocktestdata/bitmapblockcommit/snapshots-3-4 new file mode 100644 index 0000000000..b96e8910d7 --- /dev/null +++ b/tests/qemublocktestdata/bitmapblockcommit/snapshots-3-4 @@ -0,0 +1,72 @@ +pre job bitmap disable: +[ + { + "type": "block-dirty-bitmap-disable", + "data": { + "node": "libvirt-4-format", + "name": "a" + } + } +] +merge bitmpas: +[ + { + "type": "block-dirty-bitmap-merge", + "data": { + "node": "libvirt-4-format", + "target": "a", + "bitmaps": [ + { + "node": "libvirt-3-format", + "name": "a" + } + ] + } + }, + { + "type": "block-dirty-bitmap-add", + "data": { + "node": "libvirt-4-format", + "name": "b", + "persistent": true, + "disabled": true, + "granularity": 65536 + } + }, + { + "type": "block-dirty-bitmap-merge", + "data": { + "node": "libvirt-4-format", + "target": "b", + "bitmaps": [ + { + "node": "libvirt-3-format", + "name": "b" + } + ] + } + }, + { + "type": "block-dirty-bitmap-add", + "data": { + "node": "libvirt-4-format", + "name": "c", + "persistent": true, + "disabled": false, + "granularity": 65536 + } + }, + { + "type": "block-dirty-bitmap-merge", + "data": { + "node": "libvirt-4-format", + "target": "c", + "bitmaps": [ + { + "node": "libvirt-3-format", + "name": "c" + } + ] + } + } +] diff --git a/tests/qemublocktestdata/bitmapblockcommit/snapshots-3-5 b/tests/qemublocktestdata/bitmapblockcommit/snapshots-3-5 new file mode 100644 index 0000000000..9570c34c40 --- /dev/null +++ b/tests/qemublocktestdata/bitmapblockcommit/snapshots-3-5 @@ -0,0 +1,76 @@ +pre job bitmap disable: +[ + { + "type": "block-dirty-bitmap-disable", + "data": { + "node": "libvirt-5-format", + "name": "a" + } + } +] +merge bitmpas: +[ + { + "type": "block-dirty-bitmap-merge", + "data": { + "node": "libvirt-5-format", + "target": "a", + "bitmaps": [ + { + "node": "libvirt-3-format", + "name": "a" + }, + { + "node": "libvirt-4-format", + "name": "a" + } + ] + } + }, + { + "type": "block-dirty-bitmap-add", + "data": { + "node": "libvirt-5-format", + "name": "b", + "persistent": true, + "disabled": true, + "granularity": 65536 + } + }, + { + "type": "block-dirty-bitmap-merge", + "data": { + "node": "libvirt-5-format", + "target": "b", + "bitmaps": [ + { + "node": "libvirt-3-format", + "name": "b" + } + ] + } + }, + { + "type": "block-dirty-bitmap-add", + "data": { + "node": "libvirt-5-format", + "name": "c", + "persistent": true, + "disabled": false, + "granularity": 65536 + } + }, + { + "type": "block-dirty-bitmap-merge", + "data": { + "node": "libvirt-5-format", + "target": "c", + "bitmaps": [ + { + "node": "libvirt-3-format", + "name": "c" + } + ] + } + } +] diff --git a/tests/qemublocktestdata/bitmapblockcommit/snapshots-4-4 b/tests/qemublocktestdata/bitmapblockcommit/snapshots-4-4 new file mode 100644 index 0000000000..a445fd7c49 --- /dev/null +++ b/tests/qemublocktestdata/bitmapblockcommit/snapshots-4-4 @@ -0,0 +1,11 @@ +pre job bitmap disable: +[ + { + "type": "block-dirty-bitmap-disable", + "data": { + "node": "libvirt-5-format", + "name": "a" + } + } +] +merge bitmpas: diff --git a/tests/qemublocktestdata/bitmapblockcommit/snapshots-4-5 b/tests/qemublocktestdata/bitmapblockcommit/snapshots-4-5 new file mode 100644 index 0000000000..7e1020d96e --- /dev/null +++ b/tests/qemublocktestdata/bitmapblockcommit/snapshots-4-5 @@ -0,0 +1,33 @@ +pre job bitmap disable: +[ + { + "type": "block-dirty-bitmap-disable", + "data": { + "node": "libvirt-5-format", + "name": "a" + } + } +] +merge bitmpas: +[ + { + "type": "block-dirty-bitmap-enable", + "data": { + "node": "libvirt-5-format", + "name": "a" + } + }, + { + "type": "block-dirty-bitmap-merge", + "data": { + "node": "libvirt-5-format", + "target": "a", + "bitmaps": [ + { + "node": "libvirt-4-format", + "name": "a" + } + ] + } + } +] -- 2.24.1
On 3/11/20 7:55 AM, Peter Krempa wrote:
Test handling of more complex cases of merging bitmaps accross
across
snapshots.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/qemublocktest.c | 15 ++ .../bitmapblockcommit/snapshots-1-2 | 49 +++++++ .../bitmapblockcommit/snapshots-1-3 | 76 ++++++++++ .../bitmapblockcommit/snapshots-1-4 | 126 +++++++++++++++++ .../bitmapblockcommit/snapshots-1-5 | 130 ++++++++++++++++++ .../bitmapblockcommit/snapshots-2-3 | 49 +++++++ .../bitmapblockcommit/snapshots-2-4 | 99 +++++++++++++ .../bitmapblockcommit/snapshots-2-5 | 103 ++++++++++++++ .../bitmapblockcommit/snapshots-3-4 | 72 ++++++++++ .../bitmapblockcommit/snapshots-3-5 | 76 ++++++++++ .../bitmapblockcommit/snapshots-4-4 | 11 ++
Is this merging a snapshot to itself?
.../bitmapblockcommit/snapshots-4-5 | 33 +++++
Otherwise this looks like you are covering every pair possible.
+++ b/tests/qemublocktest.c @@ -1291,6 +1291,21 @@ mymain(void) TEST_BITMAP_BLOCKCOMMIT("basic-1-3", 1, 3, "basic"); TEST_BITMAP_BLOCKCOMMIT("basic-2-3", 2, 3, "basic");
+ TEST_BITMAP_BLOCKCOMMIT("snapshots-1-2", 1, 2, "snapshots"); + TEST_BITMAP_BLOCKCOMMIT("snapshots-1-3", 1, 3, "snapshots"); + TEST_BITMAP_BLOCKCOMMIT("snapshots-1-4", 1, 4, "snapshots"); + TEST_BITMAP_BLOCKCOMMIT("snapshots-1-5", 1, 5, "snapshots"); + + TEST_BITMAP_BLOCKCOMMIT("snapshots-2-3", 2, 3, "snapshots"); + TEST_BITMAP_BLOCKCOMMIT("snapshots-2-4", 2, 4, "snapshots"); + TEST_BITMAP_BLOCKCOMMIT("snapshots-2-5", 2, 5, "snapshots"); + + TEST_BITMAP_BLOCKCOMMIT("snapshots-3-4", 3, 4, "snapshots"); + TEST_BITMAP_BLOCKCOMMIT("snapshots-3-5", 3, 5, "snapshots"); + + TEST_BITMAP_BLOCKCOMMIT("snapshots-4-5", 4, 5, "snapshots");
Hmm - you don't even have "snapshots-4-4" as a stem here. Did you add an accidental file?
diff --git a/tests/qemublocktestdata/bitmapblockcommit/snapshots-4-4 b/tests/qemublocktestdata/bitmapblockcommit/snapshots-4-4 new file mode 100644 index 0000000000..a445fd7c49 --- /dev/null +++ b/tests/qemublocktestdata/bitmapblockcommit/snapshots-4-4 @@ -0,0 +1,11 @@ +pre job bitmap disable: +[ + { + "type": "block-dirty-bitmap-disable", + "data": { + "node": "libvirt-5-format", + "name": "a" + } + } +] +merge bitmpas: diff --git a/tests/qemublocktestdata/bitmapblockcommit/snapshots-4-5 b/tests/qemublocktestdata/bitmapblockcommit/snapshots-4-5
Otherwise, makes sense. Reviewed-by: Eric Blake <eblake@redhat.com> -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Use the 'snapshots-synthetic-broken' test data for block-commit. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tests/qemublocktest.c | 13 ++++ .../snapshots-synthetic-broken-1-2 | 27 +++++++ .../snapshots-synthetic-broken-1-3 | 66 +++++++++++++++++ .../snapshots-synthetic-broken-1-4 | 73 +++++++++++++++++++ .../snapshots-synthetic-broken-1-5 | 73 +++++++++++++++++++ .../snapshots-synthetic-broken-2-3 | 43 +++++++++++ .../snapshots-synthetic-broken-2-4 | 50 +++++++++++++ .../snapshots-synthetic-broken-2-5 | 50 +++++++++++++ .../snapshots-synthetic-broken-3-4 | 27 +++++++ .../snapshots-synthetic-broken-3-5 | 27 +++++++ .../snapshots-synthetic-broken-4-5 | 20 +++++ 11 files changed, 469 insertions(+) create mode 100644 tests/qemublocktestdata/bitmapblockcommit/snapshots-synthetic-broken-1-2 create mode 100644 tests/qemublocktestdata/bitmapblockcommit/snapshots-synthetic-broken-1-3 create mode 100644 tests/qemublocktestdata/bitmapblockcommit/snapshots-synthetic-broken-1-4 create mode 100644 tests/qemublocktestdata/bitmapblockcommit/snapshots-synthetic-broken-1-5 create mode 100644 tests/qemublocktestdata/bitmapblockcommit/snapshots-synthetic-broken-2-3 create mode 100644 tests/qemublocktestdata/bitmapblockcommit/snapshots-synthetic-broken-2-4 create mode 100644 tests/qemublocktestdata/bitmapblockcommit/snapshots-synthetic-broken-2-5 create mode 100644 tests/qemublocktestdata/bitmapblockcommit/snapshots-synthetic-broken-3-4 create mode 100644 tests/qemublocktestdata/bitmapblockcommit/snapshots-synthetic-broken-3-5 create mode 100644 tests/qemublocktestdata/bitmapblockcommit/snapshots-synthetic-broken-4-5 diff --git a/tests/qemublocktest.c b/tests/qemublocktest.c index 5eb38c3981..b782e7969d 100644 --- a/tests/qemublocktest.c +++ b/tests/qemublocktest.c @@ -1305,6 +1305,19 @@ mymain(void) TEST_BITMAP_BLOCKCOMMIT("snapshots-4-5", 4, 5, "snapshots"); + TEST_BITMAP_BLOCKCOMMIT("snapshots-synthetic-broken-1-2", 1, 2, "snapshots-synthetic-broken"); + TEST_BITMAP_BLOCKCOMMIT("snapshots-synthetic-broken-1-3", 1, 3, "snapshots-synthetic-broken"); + TEST_BITMAP_BLOCKCOMMIT("snapshots-synthetic-broken-1-4", 1, 4, "snapshots-synthetic-broken"); + TEST_BITMAP_BLOCKCOMMIT("snapshots-synthetic-broken-1-5", 1, 5, "snapshots-synthetic-broken"); + + TEST_BITMAP_BLOCKCOMMIT("snapshots-synthetic-broken-2-3", 2, 3, "snapshots-synthetic-broken"); + TEST_BITMAP_BLOCKCOMMIT("snapshots-synthetic-broken-2-4", 2, 4, "snapshots-synthetic-broken"); + TEST_BITMAP_BLOCKCOMMIT("snapshots-synthetic-broken-2-5", 2, 5, "snapshots-synthetic-broken"); + + TEST_BITMAP_BLOCKCOMMIT("snapshots-synthetic-broken-3-4", 3, 4, "snapshots-synthetic-broken"); + TEST_BITMAP_BLOCKCOMMIT("snapshots-synthetic-broken-3-5", 3, 5, "snapshots-synthetic-broken"); + + TEST_BITMAP_BLOCKCOMMIT("snapshots-synthetic-broken-4-5", 4, 5, "snapshots-synthetic-broken"); cleanup: virHashFree(diskxmljsondata.schema); diff --git a/tests/qemublocktestdata/bitmapblockcommit/snapshots-synthetic-broken-1-2 b/tests/qemublocktestdata/bitmapblockcommit/snapshots-synthetic-broken-1-2 new file mode 100644 index 0000000000..d413fbe723 --- /dev/null +++ b/tests/qemublocktestdata/bitmapblockcommit/snapshots-synthetic-broken-1-2 @@ -0,0 +1,27 @@ +pre job bitmap disable: +merge bitmpas: +[ + { + "type": "block-dirty-bitmap-add", + "data": { + "node": "libvirt-2-format", + "name": "current", + "persistent": true, + "disabled": false, + "granularity": 65536 + } + }, + { + "type": "block-dirty-bitmap-merge", + "data": { + "node": "libvirt-2-format", + "target": "current", + "bitmaps": [ + { + "node": "libvirt-1-format", + "name": "current" + } + ] + } + } +] diff --git a/tests/qemublocktestdata/bitmapblockcommit/snapshots-synthetic-broken-1-3 b/tests/qemublocktestdata/bitmapblockcommit/snapshots-synthetic-broken-1-3 new file mode 100644 index 0000000000..6eb14f927a --- /dev/null +++ b/tests/qemublocktestdata/bitmapblockcommit/snapshots-synthetic-broken-1-3 @@ -0,0 +1,66 @@ +pre job bitmap disable: +[ + { + "type": "block-dirty-bitmap-disable", + "data": { + "node": "libvirt-3-format", + "name": "b" + } + } +] +merge bitmpas: +[ + { + "type": "block-dirty-bitmap-enable", + "data": { + "node": "libvirt-3-format", + "name": "b" + } + }, + { + "type": "block-dirty-bitmap-add", + "data": { + "node": "libvirt-3-format", + "name": "current", + "persistent": true, + "disabled": false, + "granularity": 65536 + } + }, + { + "type": "block-dirty-bitmap-merge", + "data": { + "node": "libvirt-3-format", + "target": "current", + "bitmaps": [ + { + "node": "libvirt-1-format", + "name": "current" + } + ] + } + }, + { + "type": "block-dirty-bitmap-add", + "data": { + "node": "libvirt-3-format", + "name": "c", + "persistent": true, + "disabled": true, + "granularity": 65536 + } + }, + { + "type": "block-dirty-bitmap-merge", + "data": { + "node": "libvirt-3-format", + "target": "c", + "bitmaps": [ + { + "node": "libvirt-2-format", + "name": "c" + } + ] + } + } +] diff --git a/tests/qemublocktestdata/bitmapblockcommit/snapshots-synthetic-broken-1-4 b/tests/qemublocktestdata/bitmapblockcommit/snapshots-synthetic-broken-1-4 new file mode 100644 index 0000000000..f4d9b72576 --- /dev/null +++ b/tests/qemublocktestdata/bitmapblockcommit/snapshots-synthetic-broken-1-4 @@ -0,0 +1,73 @@ +pre job bitmap disable: +merge bitmpas: +[ + { + "type": "block-dirty-bitmap-add", + "data": { + "node": "libvirt-4-format", + "name": "b", + "persistent": true, + "disabled": false, + "granularity": 65536 + } + }, + { + "type": "block-dirty-bitmap-merge", + "data": { + "node": "libvirt-4-format", + "target": "b", + "bitmaps": [ + { + "node": "libvirt-3-format", + "name": "b" + } + ] + } + }, + { + "type": "block-dirty-bitmap-add", + "data": { + "node": "libvirt-4-format", + "name": "current", + "persistent": true, + "disabled": false, + "granularity": 65536 + } + }, + { + "type": "block-dirty-bitmap-merge", + "data": { + "node": "libvirt-4-format", + "target": "current", + "bitmaps": [ + { + "node": "libvirt-1-format", + "name": "current" + } + ] + } + }, + { + "type": "block-dirty-bitmap-add", + "data": { + "node": "libvirt-4-format", + "name": "c", + "persistent": true, + "disabled": true, + "granularity": 65536 + } + }, + { + "type": "block-dirty-bitmap-merge", + "data": { + "node": "libvirt-4-format", + "target": "c", + "bitmaps": [ + { + "node": "libvirt-2-format", + "name": "c" + } + ] + } + } +] diff --git a/tests/qemublocktestdata/bitmapblockcommit/snapshots-synthetic-broken-1-5 b/tests/qemublocktestdata/bitmapblockcommit/snapshots-synthetic-broken-1-5 new file mode 100644 index 0000000000..a8e575c2d9 --- /dev/null +++ b/tests/qemublocktestdata/bitmapblockcommit/snapshots-synthetic-broken-1-5 @@ -0,0 +1,73 @@ +pre job bitmap disable: +merge bitmpas: +[ + { + "type": "block-dirty-bitmap-add", + "data": { + "node": "libvirt-5-format", + "name": "b", + "persistent": true, + "disabled": false, + "granularity": 65536 + } + }, + { + "type": "block-dirty-bitmap-merge", + "data": { + "node": "libvirt-5-format", + "target": "b", + "bitmaps": [ + { + "node": "libvirt-3-format", + "name": "b" + } + ] + } + }, + { + "type": "block-dirty-bitmap-add", + "data": { + "node": "libvirt-5-format", + "name": "current", + "persistent": true, + "disabled": false, + "granularity": 65536 + } + }, + { + "type": "block-dirty-bitmap-merge", + "data": { + "node": "libvirt-5-format", + "target": "current", + "bitmaps": [ + { + "node": "libvirt-1-format", + "name": "current" + } + ] + } + }, + { + "type": "block-dirty-bitmap-add", + "data": { + "node": "libvirt-5-format", + "name": "c", + "persistent": true, + "disabled": true, + "granularity": 65536 + } + }, + { + "type": "block-dirty-bitmap-merge", + "data": { + "node": "libvirt-5-format", + "target": "c", + "bitmaps": [ + { + "node": "libvirt-2-format", + "name": "c" + } + ] + } + } +] diff --git a/tests/qemublocktestdata/bitmapblockcommit/snapshots-synthetic-broken-2-3 b/tests/qemublocktestdata/bitmapblockcommit/snapshots-synthetic-broken-2-3 new file mode 100644 index 0000000000..d468e2b9d8 --- /dev/null +++ b/tests/qemublocktestdata/bitmapblockcommit/snapshots-synthetic-broken-2-3 @@ -0,0 +1,43 @@ +pre job bitmap disable: +[ + { + "type": "block-dirty-bitmap-disable", + "data": { + "node": "libvirt-3-format", + "name": "b" + } + } +] +merge bitmpas: +[ + { + "type": "block-dirty-bitmap-enable", + "data": { + "node": "libvirt-3-format", + "name": "b" + } + }, + { + "type": "block-dirty-bitmap-add", + "data": { + "node": "libvirt-3-format", + "name": "c", + "persistent": true, + "disabled": true, + "granularity": 65536 + } + }, + { + "type": "block-dirty-bitmap-merge", + "data": { + "node": "libvirt-3-format", + "target": "c", + "bitmaps": [ + { + "node": "libvirt-2-format", + "name": "c" + } + ] + } + } +] diff --git a/tests/qemublocktestdata/bitmapblockcommit/snapshots-synthetic-broken-2-4 b/tests/qemublocktestdata/bitmapblockcommit/snapshots-synthetic-broken-2-4 new file mode 100644 index 0000000000..2a9986bac6 --- /dev/null +++ b/tests/qemublocktestdata/bitmapblockcommit/snapshots-synthetic-broken-2-4 @@ -0,0 +1,50 @@ +pre job bitmap disable: +merge bitmpas: +[ + { + "type": "block-dirty-bitmap-add", + "data": { + "node": "libvirt-4-format", + "name": "b", + "persistent": true, + "disabled": false, + "granularity": 65536 + } + }, + { + "type": "block-dirty-bitmap-merge", + "data": { + "node": "libvirt-4-format", + "target": "b", + "bitmaps": [ + { + "node": "libvirt-3-format", + "name": "b" + } + ] + } + }, + { + "type": "block-dirty-bitmap-add", + "data": { + "node": "libvirt-4-format", + "name": "c", + "persistent": true, + "disabled": true, + "granularity": 65536 + } + }, + { + "type": "block-dirty-bitmap-merge", + "data": { + "node": "libvirt-4-format", + "target": "c", + "bitmaps": [ + { + "node": "libvirt-2-format", + "name": "c" + } + ] + } + } +] diff --git a/tests/qemublocktestdata/bitmapblockcommit/snapshots-synthetic-broken-2-5 b/tests/qemublocktestdata/bitmapblockcommit/snapshots-synthetic-broken-2-5 new file mode 100644 index 0000000000..47d9f6e17a --- /dev/null +++ b/tests/qemublocktestdata/bitmapblockcommit/snapshots-synthetic-broken-2-5 @@ -0,0 +1,50 @@ +pre job bitmap disable: +merge bitmpas: +[ + { + "type": "block-dirty-bitmap-add", + "data": { + "node": "libvirt-5-format", + "name": "b", + "persistent": true, + "disabled": false, + "granularity": 65536 + } + }, + { + "type": "block-dirty-bitmap-merge", + "data": { + "node": "libvirt-5-format", + "target": "b", + "bitmaps": [ + { + "node": "libvirt-3-format", + "name": "b" + } + ] + } + }, + { + "type": "block-dirty-bitmap-add", + "data": { + "node": "libvirt-5-format", + "name": "c", + "persistent": true, + "disabled": true, + "granularity": 65536 + } + }, + { + "type": "block-dirty-bitmap-merge", + "data": { + "node": "libvirt-5-format", + "target": "c", + "bitmaps": [ + { + "node": "libvirt-2-format", + "name": "c" + } + ] + } + } +] diff --git a/tests/qemublocktestdata/bitmapblockcommit/snapshots-synthetic-broken-3-4 b/tests/qemublocktestdata/bitmapblockcommit/snapshots-synthetic-broken-3-4 new file mode 100644 index 0000000000..367a930a74 --- /dev/null +++ b/tests/qemublocktestdata/bitmapblockcommit/snapshots-synthetic-broken-3-4 @@ -0,0 +1,27 @@ +pre job bitmap disable: +merge bitmpas: +[ + { + "type": "block-dirty-bitmap-add", + "data": { + "node": "libvirt-4-format", + "name": "b", + "persistent": true, + "disabled": false, + "granularity": 65536 + } + }, + { + "type": "block-dirty-bitmap-merge", + "data": { + "node": "libvirt-4-format", + "target": "b", + "bitmaps": [ + { + "node": "libvirt-3-format", + "name": "b" + } + ] + } + } +] diff --git a/tests/qemublocktestdata/bitmapblockcommit/snapshots-synthetic-broken-3-5 b/tests/qemublocktestdata/bitmapblockcommit/snapshots-synthetic-broken-3-5 new file mode 100644 index 0000000000..0062ec140c --- /dev/null +++ b/tests/qemublocktestdata/bitmapblockcommit/snapshots-synthetic-broken-3-5 @@ -0,0 +1,27 @@ +pre job bitmap disable: +merge bitmpas: +[ + { + "type": "block-dirty-bitmap-add", + "data": { + "node": "libvirt-5-format", + "name": "b", + "persistent": true, + "disabled": false, + "granularity": 65536 + } + }, + { + "type": "block-dirty-bitmap-merge", + "data": { + "node": "libvirt-5-format", + "target": "b", + "bitmaps": [ + { + "node": "libvirt-3-format", + "name": "b" + } + ] + } + } +] diff --git a/tests/qemublocktestdata/bitmapblockcommit/snapshots-synthetic-broken-4-5 b/tests/qemublocktestdata/bitmapblockcommit/snapshots-synthetic-broken-4-5 new file mode 100644 index 0000000000..b1f10a8a24 --- /dev/null +++ b/tests/qemublocktestdata/bitmapblockcommit/snapshots-synthetic-broken-4-5 @@ -0,0 +1,20 @@ +pre job bitmap disable: +[ + { + "type": "block-dirty-bitmap-disable", + "data": { + "node": "libvirt-5-format", + "name": "a" + } + } +] +merge bitmpas: +[ + { + "type": "block-dirty-bitmap-enable", + "data": { + "node": "libvirt-5-format", + "name": "a" + } + } +] -- 2.24.1
On 3/11/20 7:55 AM, Peter Krempa wrote:
Use the 'snapshots-synthetic-broken' test data for block-commit.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> ---
+++ b/tests/qemublocktest.c @@ -1305,6 +1305,19 @@ mymain(void)
TEST_BITMAP_BLOCKCOMMIT("snapshots-4-5", 4, 5, "snapshots");
+ TEST_BITMAP_BLOCKCOMMIT("snapshots-synthetic-broken-1-2", 1, 2, "snapshots-synthetic-broken"); + TEST_BITMAP_BLOCKCOMMIT("snapshots-synthetic-broken-1-3", 1, 3, "snapshots-synthetic-broken"); + TEST_BITMAP_BLOCKCOMMIT("snapshots-synthetic-broken-1-4", 1, 4, "snapshots-synthetic-broken"); + TEST_BITMAP_BLOCKCOMMIT("snapshots-synthetic-broken-1-5", 1, 5, "snapshots-synthetic-broken"); + + TEST_BITMAP_BLOCKCOMMIT("snapshots-synthetic-broken-2-3", 2, 3, "snapshots-synthetic-broken"); + TEST_BITMAP_BLOCKCOMMIT("snapshots-synthetic-broken-2-4", 2, 4, "snapshots-synthetic-broken"); + TEST_BITMAP_BLOCKCOMMIT("snapshots-synthetic-broken-2-5", 2, 5, "snapshots-synthetic-broken"); + + TEST_BITMAP_BLOCKCOMMIT("snapshots-synthetic-broken-3-4", 3, 4, "snapshots-synthetic-broken"); + TEST_BITMAP_BLOCKCOMMIT("snapshots-synthetic-broken-3-5", 3, 5, "snapshots-synthetic-broken"); + + TEST_BITMAP_BLOCKCOMMIT("snapshots-synthetic-broken-4-5", 4, 5, "snapshots-synthetic-broken");
Again, may be impacted if we change the design, but the test looks like good coverage of the design that the rest of this series adds. 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 an argument to qemuBlockJobDiskNewCommit to propagate the list of disabled bitmaps into the job data structure. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_blockjob.c | 2 ++ src/qemu/qemu_blockjob.h | 1 + src/qemu/qemu_driver.c | 2 +- 3 files changed, 4 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_blockjob.c b/src/qemu/qemu_blockjob.c index e894e1634d..63f1cc79c3 100644 --- a/src/qemu/qemu_blockjob.c +++ b/src/qemu/qemu_blockjob.c @@ -285,6 +285,7 @@ qemuBlockJobDiskNewCommit(virDomainObjPtr vm, virStorageSourcePtr topparent, virStorageSourcePtr top, virStorageSourcePtr base, + char ***disabledBitmapsBase, bool delete_imgs, unsigned int jobflags) { @@ -310,6 +311,7 @@ qemuBlockJobDiskNewCommit(virDomainObjPtr vm, job->data.commit.top = top; job->data.commit.base = base; job->data.commit.deleteCommittedImages = delete_imgs; + job->data.commit.disabledBitmapsBase = g_steal_pointer(disabledBitmapsBase); job->jobflags = jobflags; if (qemuBlockJobRegister(job, vm, disk, true) < 0) diff --git a/src/qemu/qemu_blockjob.h b/src/qemu/qemu_blockjob.h index e2e28ca4d3..9264c70217 100644 --- a/src/qemu/qemu_blockjob.h +++ b/src/qemu/qemu_blockjob.h @@ -187,6 +187,7 @@ qemuBlockJobDiskNewCommit(virDomainObjPtr vm, virStorageSourcePtr topparent, virStorageSourcePtr top, virStorageSourcePtr base, + char ***disabledBitmapsBase, bool delete_imgs, unsigned int jobflags); diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index d3eb2171ef..31c0f2dd91 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -18556,7 +18556,7 @@ qemuDomainBlockCommit(virDomainPtr dom, } if (!(job = qemuBlockJobDiskNewCommit(vm, disk, top_parent, topSource, - baseSource, + baseSource, NULL, flags & VIR_DOMAIN_BLOCK_COMMIT_DELETE, flags))) goto endjob; -- 2.24.1
On 3/11/20 7:56 AM, Peter Krempa wrote:
Add an argument to qemuBlockJobDiskNewCommit to propagate the list of disabled bitmaps into the job data structure.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_blockjob.c | 2 ++ src/qemu/qemu_blockjob.h | 1 + src/qemu/qemu_driver.c | 2 +- 3 files changed, 4 insertions(+), 1 deletion(-)
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 start of the commit job, we need to disable any active bitmap in the base. Use qemuBlockBitmapsHandleCommitStart to calculate which and call the appropriate QMP APIs. We use blockdev-reopen to make the 'base' writable to disable the bitmaps. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 43 +++++++++++++++++++++++++++++++++++++++++- 1 file changed, 42 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 31c0f2dd91..628fe9b107 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -18399,6 +18399,8 @@ qemuDomainBlockCommit(virDomainPtr dom, const char *nodebase = NULL; bool persistjob = false; bool blockdev = false; + g_autoptr(virJSONValue) bitmapDisableActions = NULL; + VIR_AUTOSTRINGLIST bitmapDisableList = NULL; virCheckFlags(VIR_DOMAIN_BLOCK_COMMIT_SHALLOW | VIR_DOMAIN_BLOCK_COMMIT_ACTIVE | @@ -18555,8 +18557,29 @@ qemuDomainBlockCommit(virDomainPtr dom, goto endjob; } + if (blockdev && + virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BLOCKDEV_REOPEN)) { + g_autoptr(virHashTable) blockNamedNodeData = NULL; + if (!(blockNamedNodeData = qemuBlockGetNamedNodeData(vm, QEMU_ASYNC_JOB_NONE))) + goto endjob; + + if (qemuBlockBitmapsHandleCommitStart(topSource, baseSource, + blockNamedNodeData, + &bitmapDisableActions, + &bitmapDisableList) < 0) + goto endjob; + + /* if we don't have terminator on 'base' we can't reopen it */ + if (bitmapDisableActions && !baseSource->backingStore) { + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, + _("can't handle bitmaps on unterminated backing image '%s'"), + base); + goto endjob; + } + } + if (!(job = qemuBlockJobDiskNewCommit(vm, disk, top_parent, topSource, - baseSource, NULL, + baseSource, &bitmapDisableList, flags & VIR_DOMAIN_BLOCK_COMMIT_DELETE, flags))) goto endjob; @@ -18578,6 +18601,24 @@ qemuDomainBlockCommit(virDomainPtr dom, if (!backingPath && top_parent && !(backingPath = qemuBlockGetBackingStoreString(baseSource))) goto endjob; + + if (bitmapDisableActions) { + int rc; + + if (qemuBlockReopenReadWrite(vm, baseSource, QEMU_ASYNC_JOB_NONE) < 0) + goto endjob; + + qemuDomainObjEnterMonitor(driver, vm); + rc = qemuMonitorTransaction(priv->mon, &bitmapDisableActions); + if (qemuDomainObjExitMonitor(driver, vm) < 0) + goto endjob; + + if (qemuBlockReopenReadOnly(vm, baseSource, QEMU_ASYNC_JOB_NONE) < 0) + goto endjob; + + if (rc < 0) + goto endjob; + } } else { device = job->name; } -- 2.24.1
On 3/11/20 7:56 AM, Peter Krempa wrote:
On start of the commit job, we need to disable any active bitmap in the base. Use qemuBlockBitmapsHandleCommitStart to calculate which and call the appropriate QMP APIs. We use blockdev-reopen to make the 'base' writable to disable the bitmaps.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 43 +++++++++++++++++++++++++++++++++++++++++- 1 file changed, 42 insertions(+), 1 deletion(-)
Someday, it would be nice if qemu let us do this without requiring us to do the reopen step ourselves, but in the meantime, this looks like it works. Design-wise, I'm still not convinced why we have to disable the bitmaps in the base prior to the commit (as the only thing a bitmap is good for is for cumulative merging in determining how much of an image to expose over a future differential backup, but that differential is the same whether we write bits in one or multiple bitmaps as part of the commit operation). But code wise, this looks accurate. Reviewed-by: Eric Blake <eblake@redhat.com> -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Active layer block commit makes the 'base' image the new top image of the disk after it finishes. This means that all bitmap operations need to be handled prior to this happening as we'd lose writes otherwise. The ideal place is to handle it when pivoting to the new image as only guest-writes would be happening after this point. Use qemuBlockBitmapsHandleCommitFinish to calculate the merging transaction. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 628fe9b107..3afdecda1b 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -17301,6 +17301,23 @@ qemuDomainBlockPivot(virQEMUDriverPtr driver, break; case QEMU_BLOCKJOB_TYPE_ACTIVE_COMMIT: + /* we technically don't need reopen here, but we couldn't prepare + * the bitmaps if it wasn't present thus must skip this */ + if (blockdev && + virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BLOCKDEV_REOPEN)) { + g_autoptr(virHashTable) blockNamedNodeData = NULL; + + if (!(blockNamedNodeData = qemuBlockGetNamedNodeData(vm, QEMU_ASYNC_JOB_NONE))) + return -1; + + if (qemuBlockBitmapsHandleCommitFinish(job->data.commit.top, + job->data.commit.base, + blockNamedNodeData, + &actions, + job->data.commit.disabledBitmapsBase) < 0) + return -1; + } + break; } -- 2.24.1
On 3/11/20 7:56 AM, Peter Krempa wrote:
Active layer block commit makes the 'base' image the new top image of the disk after it finishes. This means that all bitmap operations need to be handled prior to this happening as we'd lose writes otherwise.
The ideal place is to handle it when pivoting to the new image as only guest-writes would be happening after this point.
Use qemuBlockBitmapsHandleCommitFinish to calculate the merging transaction.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 17 +++++++++++++++++ 1 file changed, 17 insertions(+)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 628fe9b107..3afdecda1b 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -17301,6 +17301,23 @@ qemuDomainBlockPivot(virQEMUDriverPtr driver, break;
case QEMU_BLOCKJOB_TYPE_ACTIVE_COMMIT: + /* we technically don't need reopen here, but we couldn't prepare + * the bitmaps if it wasn't present thus must skip this */ + if (blockdev && + virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BLOCKDEV_REOPEN)) { + g_autoptr(virHashTable) blockNamedNodeData = NULL; + + if (!(blockNamedNodeData = qemuBlockGetNamedNodeData(vm, QEMU_ASYNC_JOB_NONE))) + return -1; + + if (qemuBlockBitmapsHandleCommitFinish(job->data.commit.top, + job->data.commit.base, + blockNamedNodeData, + &actions, + job->data.commit.disabledBitmapsBase) < 0) + return -1; + } +
Reviewed-by: Eric Blake <eblake@redhat.com> -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Merge the bitmaps into base of the block commit after the job finishes. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_blockjob.c | 53 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 53 insertions(+) diff --git a/src/qemu/qemu_blockjob.c b/src/qemu/qemu_blockjob.c index 63f1cc79c3..ed7959175a 100644 --- a/src/qemu/qemu_blockjob.c +++ b/src/qemu/qemu_blockjob.c @@ -1049,6 +1049,56 @@ qemuBlockJobDeleteImages(virQEMUDriverPtr driver, } } + +/** + * qemuBlockJobProcessEventCompletedCommitBitmaps: + * + * Handles the bitmap changes after commit. This function shall return -1 on + * monitor failures. + */ +static int +qemuBlockJobProcessEventCompletedCommitBitmaps(virDomainObjPtr vm, + qemuBlockJobDataPtr job, + qemuDomainAsyncJob asyncJob) +{ + qemuDomainObjPrivatePtr priv = vm->privateData; + g_autoptr(virHashTable) blockNamedNodeData = NULL; + g_autoptr(virJSONValue) actions = NULL; + + if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BLOCKDEV_REOPEN)) + return 0; + + if (!(blockNamedNodeData = qemuBlockGetNamedNodeData(vm, asyncJob))) + return -1; + + if (qemuBlockBitmapsHandleCommitFinish(job->data.commit.top, + job->data.commit.base, + blockNamedNodeData, + &actions, + job->data.commit.disabledBitmapsBase) < 0) + return 0; + + if (!actions) + return 0; + + if (qemuBlockReopenReadWrite(vm, job->data.commit.base, asyncJob) < 0) + return -1; + + if (qemuDomainObjEnterMonitorAsync(priv->driver, vm, asyncJob) < 0) + return -1; + + qemuMonitorTransaction(priv->mon, &actions); + + if (qemuDomainObjExitMonitor(priv->driver, vm) < 0) + return -1; + + if (qemuBlockReopenReadOnly(vm, job->data.commit.base, asyncJob) < 0) + return -1; + + return 0; +} + + /** * qemuBlockJobProcessEventCompletedCommit: * @driver: qemu driver object @@ -1106,6 +1156,9 @@ qemuBlockJobProcessEventCompletedCommit(virQEMUDriverPtr driver, if (!n) return; + if (qemuBlockJobProcessEventCompletedCommitBitmaps(vm, job, asyncJob) < 0) + return; + /* revert access to images */ qemuDomainStorageSourceAccessAllow(driver, vm, job->data.commit.base, true, false, false); -- 2.24.1
On 3/11/20 7:56 AM, Peter Krempa wrote:
Merge the bitmaps into base of the block commit after the job finishes.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_blockjob.c | 53 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 53 insertions(+)
diff --git a/src/qemu/qemu_blockjob.c b/src/qemu/qemu_blockjob.c index 63f1cc79c3..ed7959175a 100644 --- a/src/qemu/qemu_blockjob.c +++ b/src/qemu/qemu_blockjob.c @@ -1049,6 +1049,56 @@ qemuBlockJobDeleteImages(virQEMUDriverPtr driver, } }
+ +/** + * qemuBlockJobProcessEventCompletedCommitBitmaps: + * + * Handles the bitmap changes after commit. This function shall return -1 on + * monitor failures.
s/shall return/returns/ sounds less stodgy :)
+ */ +static int +qemuBlockJobProcessEventCompletedCommitBitmaps(virDomainObjPtr vm, + qemuBlockJobDataPtr job, + qemuDomainAsyncJob asyncJob) +{ + qemuDomainObjPrivatePtr priv = vm->privateData; + g_autoptr(virHashTable) blockNamedNodeData = NULL; + g_autoptr(virJSONValue) actions = NULL; + + if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BLOCKDEV_REOPEN)) + return 0; + + if (!(blockNamedNodeData = qemuBlockGetNamedNodeData(vm, asyncJob))) + return -1; + + if (qemuBlockBitmapsHandleCommitFinish(job->data.commit.top, + job->data.commit.base, + blockNamedNodeData, + &actions, + job->data.commit.disabledBitmapsBase) < 0) + return 0; + + if (!actions) + return 0; + + if (qemuBlockReopenReadWrite(vm, job->data.commit.base, asyncJob) < 0) + return -1; + + if (qemuDomainObjEnterMonitorAsync(priv->driver, vm, asyncJob) < 0) + return -1; + + qemuMonitorTransaction(priv->mon, &actions); + + if (qemuDomainObjExitMonitor(priv->driver, vm) < 0) + return -1; + + if (qemuBlockReopenReadOnly(vm, job->data.commit.base, asyncJob) < 0) + return -1;
Earlier in the series, you ignored failure to restore readonly (leaving things read-write isn't ideal, but isn't a show-stopper to correct operation). Any reason why that use was different than this where you treat it as fatal? 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 block-copy fails we should at least re-enable the bitmaps so that the operation can be re-tried. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_blockjob.c | 42 ++++++++++++++++++++++++++++++++++++++-- 1 file changed, 40 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_blockjob.c b/src/qemu/qemu_blockjob.c index ed7959175a..60fe1cedf6 100644 --- a/src/qemu/qemu_blockjob.c +++ b/src/qemu/qemu_blockjob.c @@ -1346,6 +1346,40 @@ qemuBlockJobProcessEventFailedActiveCommit(virQEMUDriverPtr driver, } +static void +qemuBlockJobProcessEventFailedCommitCommon(virDomainObjPtr vm, + qemuBlockJobDataPtr job, + qemuDomainAsyncJob asyncJob) +{ + qemuDomainObjPrivatePtr priv = vm->privateData; + g_autoptr(virJSONValue) actions = virJSONValueNewArray(); + char **disabledBitmaps = job->data.commit.disabledBitmapsBase; + + if (!disabledBitmaps || !*disabledBitmaps) + return; + + for (; *disabledBitmaps; disabledBitmaps++) { + qemuMonitorTransactionBitmapEnable(actions, + job->data.commit.base->nodeformat, + *disabledBitmaps); + } + + if (qemuBlockReopenReadWrite(vm, job->data.commit.base, asyncJob) < 0) + return; + + if (qemuDomainObjEnterMonitorAsync(priv->driver, vm, asyncJob) < 0) + return; + + qemuMonitorTransaction(priv->mon, &actions); + + if (qemuDomainObjExitMonitor(priv->driver, vm) < 0) + return; + + if (qemuBlockReopenReadOnly(vm, job->data.commit.base, asyncJob) < 0) + return; +} + + static void qemuBlockJobProcessEventConcludedCreate(virQEMUDriverPtr driver, virDomainObjPtr vm, @@ -1453,13 +1487,17 @@ qemuBlockJobEventProcessConcludedTransition(qemuBlockJobDataPtr job, case QEMU_BLOCKJOB_TYPE_COMMIT: if (success) qemuBlockJobProcessEventCompletedCommit(driver, vm, job, asyncJob); + else + qemuBlockJobProcessEventFailedCommitCommon(vm, job, asyncJob); break; case QEMU_BLOCKJOB_TYPE_ACTIVE_COMMIT: - if (success) + if (success) { qemuBlockJobProcessEventCompletedActiveCommit(driver, vm, job, asyncJob); - else + } else { qemuBlockJobProcessEventFailedActiveCommit(driver, vm, job); + qemuBlockJobProcessEventFailedCommitCommon(vm, job, asyncJob); + } break; case QEMU_BLOCKJOB_TYPE_CREATE: -- 2.24.1
On 3/11/20 7:56 AM, Peter Krempa wrote:
If a block-copy fails we should at least re-enable the bitmaps so that the operation can be re-tried.
The subject and commit body say block-copy,
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_blockjob.c | 42 ++++++++++++++++++++++++++++++++++++++-- 1 file changed, 40 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_blockjob.c b/src/qemu/qemu_blockjob.c index ed7959175a..60fe1cedf6 100644 --- a/src/qemu/qemu_blockjob.c +++ b/src/qemu/qemu_blockjob.c @@ -1346,6 +1346,40 @@ qemuBlockJobProcessEventFailedActiveCommit(virQEMUDriverPtr driver, }
+static void +qemuBlockJobProcessEventFailedCommitCommon(virDomainObjPtr vm, + qemuBlockJobDataPtr job, + qemuDomainAsyncJob asyncJob) +{
but the function name say commit. Is that intended?
+ qemuDomainObjPrivatePtr priv = vm->privateData; + g_autoptr(virJSONValue) actions = virJSONValueNewArray(); + char **disabledBitmaps = job->data.commit.disabledBitmapsBase; + + if (!disabledBitmaps || !*disabledBitmaps) + return; + + for (; *disabledBitmaps; disabledBitmaps++) { + qemuMonitorTransactionBitmapEnable(actions, + job->data.commit.base->nodeformat, + *disabledBitmaps); + } + + if (qemuBlockReopenReadWrite(vm, job->data.commit.base, asyncJob) < 0) + return; + + if (qemuDomainObjEnterMonitorAsync(priv->driver, vm, asyncJob) < 0) + return; + + qemuMonitorTransaction(priv->mon, &actions); + + if (qemuDomainObjExitMonitor(priv->driver, vm) < 0) + return; + + if (qemuBlockReopenReadOnly(vm, job->data.commit.base, asyncJob) < 0) + return; +}
Does it really matter? The only reason a bitmap is left enabled in a backing image when we create an external snapshot is because it was easier to leave it alone than to temporarily reopen the backing image read-write just to disable the bitmap. But as long as no writes to the backing file happen (until commit), whether it is left enabled or changed to disabled doesn't affect behavior; so we could also argue that if we changed it to disabled prior to attempting commit, then commit fails, it really doesn't matter if we leave it disabled rather than trying to re-enable it (just to have it be re-disabled on the retry attempt). But on the grounds of trying to leave things as close to what they were before failure, I'm okay with this patch, if you can straighten out my confusion on naming. Reviewed-by: Eric Blake <eblake@redhat.com> -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
participants (2)
-
Eric Blake -
Peter Krempa