[PATCH 0/5] qemu: Fix handling of bitmaps for active layer block commit and block copy
See patch 3 for explanation. Peter Krempa (5): qemu: blockjob: Don't base bitmap handling of active-layer block commit on QEMU_CAPS_BLOCKDEV_REOPEN qemu: blockjob: Actually delete temporary bitmap on failed active commit qemu: block: Remove 'active-write' bitmap even if there are no bitmaps to merge qemuDomainBlockPivot: Rename 'actions' to 'bitmapactions' qemuDomainBlockPivot: Ignore failures of creating active layer bitmap src/qemu/qemu_block.c | 3 ++- src/qemu/qemu_blockjob.c | 16 ++++++++++++--- src/qemu/qemu_driver.c | 20 ++++++++----------- .../qemublocktestdata/bitmapblockcommit/empty | 9 +++++++++ .../bitmapblockcopy/empty-deep-out.json | 9 +++++++++ .../bitmapblockcopy/empty-shallow-out.json | 9 +++++++++ 6 files changed, 50 insertions(+), 16 deletions(-) -- 2.26.2
The handler finalizing the active layer block commit doesn't actually reopen the file for active layer block commit, so the comment and check are invalid. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_blockjob.c | 3 ++- src/qemu/qemu_driver.c | 6 +----- 2 files changed, 3 insertions(+), 6 deletions(-) diff --git a/src/qemu/qemu_blockjob.c b/src/qemu/qemu_blockjob.c index 0039bc0e9f..9b78733c53 100644 --- a/src/qemu/qemu_blockjob.c +++ b/src/qemu/qemu_blockjob.c @@ -1101,7 +1101,8 @@ qemuBlockJobProcessEventCompletedCommitBitmaps(virDomainObjPtr vm, g_autoptr(virJSONValue) actions = NULL; bool active = job->type == QEMU_BLOCKJOB_TYPE_ACTIVE_COMMIT; - if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BLOCKDEV_REOPEN)) + if (!active && + !virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BLOCKDEV_REOPEN)) return 0; if (!(blockNamedNodeData = qemuBlockGetNamedNodeData(vm, asyncJob))) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index d185666ed8..f5074cb151 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -17386,11 +17386,7 @@ 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)) { - + if (blockdev) { actions = virJSONValueNewArray(); if (qemuMonitorTransactionBitmapAdd(actions, -- 2.26.2
On 7/16/20 9:20 AM, Peter Krempa wrote:
The handler finalizing the active layer block commit doesn't actually reopen the file for active layer block commit, so the comment and check are invalid.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_blockjob.c | 3 ++- src/qemu/qemu_driver.c | 6 +----- 2 files changed, 3 insertions(+), 6 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
Commit 20a7abc2d2d tried to delete the possibly leftover bitmap but neglected to call the actual monitor to do so. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_blockjob.c | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_blockjob.c b/src/qemu/qemu_blockjob.c index 9b78733c53..c49c98e547 100644 --- a/src/qemu/qemu_blockjob.c +++ b/src/qemu/qemu_blockjob.c @@ -1416,8 +1416,10 @@ qemuBlockJobProcessEventConcludedCopyAbort(virQEMUDriverPtr driver, static void qemuBlockJobProcessEventFailedActiveCommit(virQEMUDriverPtr driver, virDomainObjPtr vm, - qemuBlockJobDataPtr job) + qemuBlockJobDataPtr job, + qemuDomainAsyncJob asyncJob) { + qemuDomainObjPrivatePtr priv = vm->privateData; g_autoptr(virJSONValue) actions = virJSONValueNewArray(); virDomainDiskDefPtr disk = job->disk; @@ -1429,6 +1431,13 @@ qemuBlockJobProcessEventFailedActiveCommit(virQEMUDriverPtr driver, ignore_value(qemuMonitorTransactionBitmapRemove(actions, disk->mirror->nodeformat, "libvirt-tmp-activewrite")); + if (qemuDomainObjEnterMonitorAsync(priv->driver, vm, asyncJob) < 0) + return; + + qemuMonitorTransaction(priv->mon, &actions); + + if (qemuDomainObjExitMonitor(priv->driver, vm) < 0) + return; /* Ideally, we would make the backing chain read only again (yes, SELinux * can do that using different labels). But that is not implemented yet and @@ -1553,7 +1562,7 @@ qemuBlockJobEventProcessConcludedTransition(qemuBlockJobDataPtr job, if (success) { qemuBlockJobProcessEventCompletedActiveCommit(driver, vm, job, asyncJob); } else { - qemuBlockJobProcessEventFailedActiveCommit(driver, vm, job); + qemuBlockJobProcessEventFailedActiveCommit(driver, vm, job, asyncJob); } break; -- 2.26.2
On 7/16/20 9:20 AM, Peter Krempa wrote:
Commit 20a7abc2d2d tried to delete the possibly leftover bitmap but neglected to call the actual monitor to do so.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_blockjob.c | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-)
Yeah, building up the command structure doesn't help if you don't run it ;) 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 'libvirt-tmp-activewrite' bitmap is added during the 'pivot' operation of block copy and active layer block commit operations regardless of whether there are any bitmaps to merge, but was not removed unless a bitmap was merged. This meant that subsequent attempts to merge into the same image would fail. Fix it by checking whether the 'libvirt-tmp-activewrite' would be used by the code and don't skip the code which would delete it. This is a regression introduced when we switched to the new code for block commit in <20a7abc2d2d> and for block copy in <7bfff40fdfe5>. The actual bug originates from <4fa8654ece>. https://bugzilla.redhat.com/show_bug.cgi?id=1857735 Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_block.c | 3 ++- tests/qemublocktestdata/bitmapblockcommit/empty | 9 +++++++++ .../bitmapblockcopy/empty-deep-out.json | 9 +++++++++ .../bitmapblockcopy/empty-shallow-out.json | 9 +++++++++ 4 files changed, 29 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c index a2eabbcd64..c8607e56a8 100644 --- a/src/qemu/qemu_block.c +++ b/src/qemu/qemu_block.c @@ -2962,7 +2962,7 @@ qemuBlockGetBitmapMergeActions(virStorageSourcePtr topsrc, if (!(bitmaps = qemuBlockGetBitmapMergeActionsGetBitmaps(topsrc, bitmapname, blockNamedNodeData))) - return 0; + goto done; for (next = bitmaps; next; next = next->next) { const char *curbitmap = next->data; @@ -3019,6 +3019,7 @@ qemuBlockGetBitmapMergeActions(virStorageSourcePtr topsrc, return -1; } + done: if (writebitmapsrc && qemuMonitorTransactionBitmapRemove(act, writebitmapsrc->nodeformat, "libvirt-tmp-activewrite") < 0) diff --git a/tests/qemublocktestdata/bitmapblockcommit/empty b/tests/qemublocktestdata/bitmapblockcommit/empty index 9260011852..eddef0ddcd 100644 --- a/tests/qemublocktestdata/bitmapblockcommit/empty +++ b/tests/qemublocktestdata/bitmapblockcommit/empty @@ -1 +1,10 @@ merge bitmpas: +[ + { + "type": "block-dirty-bitmap-remove", + "data": { + "node": "libvirt-2-format", + "name": "libvirt-tmp-activewrite" + } + } +] diff --git a/tests/qemublocktestdata/bitmapblockcopy/empty-deep-out.json b/tests/qemublocktestdata/bitmapblockcopy/empty-deep-out.json index e69de29bb2..99f2589ed4 100644 --- a/tests/qemublocktestdata/bitmapblockcopy/empty-deep-out.json +++ b/tests/qemublocktestdata/bitmapblockcopy/empty-deep-out.json @@ -0,0 +1,9 @@ +[ + { + "type": "block-dirty-bitmap-remove", + "data": { + "node": "mirror-format-node", + "name": "libvirt-tmp-activewrite" + } + } +] diff --git a/tests/qemublocktestdata/bitmapblockcopy/empty-shallow-out.json b/tests/qemublocktestdata/bitmapblockcopy/empty-shallow-out.json index e69de29bb2..99f2589ed4 100644 --- a/tests/qemublocktestdata/bitmapblockcopy/empty-shallow-out.json +++ b/tests/qemublocktestdata/bitmapblockcopy/empty-shallow-out.json @@ -0,0 +1,9 @@ +[ + { + "type": "block-dirty-bitmap-remove", + "data": { + "node": "mirror-format-node", + "name": "libvirt-tmp-activewrite" + } + } +] -- 2.26.2
On 7/16/20 9:20 AM, Peter Krempa wrote:
The 'libvirt-tmp-activewrite' bitmap is added during the 'pivot' operation of block copy and active layer block commit operations regardless of whether there are any bitmaps to merge, but was not removed unless a bitmap was merged. This meant that subsequent attempts to merge into the same image would fail.
Fix it by checking whether the 'libvirt-tmp-activewrite' would be used by the code and don't skip the code which would delete it.
This is a regression introduced when we switched to the new code for block commit in <20a7abc2d2d> and for block copy in <7bfff40fdfe5>. The actual bug originates from <4fa8654ece>.
https://bugzilla.redhat.com/show_bug.cgi?id=1857735
Signed-off-by: Peter Krempa <pkrempa@redhat.com> ---
Reviewed-by: Eric Blake <eblake@redhat.com> -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
There are two possible 'transaction' command arguments in the function. Rename 'actions' as they deal with creating bitmaps only. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index f5074cb151..348ef17141 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -17319,7 +17319,7 @@ qemuDomainBlockPivot(virQEMUDriverPtr driver, int ret = -1; qemuDomainObjPrivatePtr priv = vm->privateData; bool blockdev = virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BLOCKDEV); - g_autoptr(virJSONValue) actions = NULL; + g_autoptr(virJSONValue) bitmapactions = NULL; g_autoptr(virJSONValue) reopenactions = NULL; if (job->state != QEMU_BLOCKJOB_STATE_READY) { @@ -17352,9 +17352,9 @@ qemuDomainBlockPivot(virQEMUDriverPtr driver, bool shallow = job->jobflags & VIR_DOMAIN_BLOCK_COPY_SHALLOW; bool reuse = job->jobflags & VIR_DOMAIN_BLOCK_COPY_REUSE_EXT; - actions = virJSONValueNewArray(); + bitmapactions = virJSONValueNewArray(); - if (qemuMonitorTransactionBitmapAdd(actions, + if (qemuMonitorTransactionBitmapAdd(bitmapactions, disk->mirror->nodeformat, "libvirt-tmp-activewrite", false, @@ -17387,9 +17387,9 @@ qemuDomainBlockPivot(virQEMUDriverPtr driver, case QEMU_BLOCKJOB_TYPE_ACTIVE_COMMIT: if (blockdev) { - actions = virJSONValueNewArray(); + bitmapactions = virJSONValueNewArray(); - if (qemuMonitorTransactionBitmapAdd(actions, + if (qemuMonitorTransactionBitmapAdd(bitmapactions, job->data.commit.base->nodeformat, "libvirt-tmp-activewrite", false, @@ -17413,8 +17413,8 @@ qemuDomainBlockPivot(virQEMUDriverPtr driver, } } - if (actions && rc == 0) - rc = qemuMonitorTransaction(priv->mon, &actions); + if (bitmapactions && rc == 0) + rc = qemuMonitorTransaction(priv->mon, &bitmapactions); if (rc == 0) ret = qemuMonitorJobComplete(priv->mon, job->name); -- 2.26.2
On 7/16/20 9:20 AM, Peter Krempa wrote:
There are two possible 'transaction' command arguments in the function. Rename 'actions' as they deal with creating bitmaps only.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 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
Ignore errors from creating "libvirt-tmp-activewrite" bitmap. This prevents failures of finishing blockjobs if the bitmap already exists. Note that if the bitmap exists, the worst case that can happen is that more bits are marked as dirty in the resulting merge. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 348ef17141..64ddc8dce9 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -17414,7 +17414,7 @@ qemuDomainBlockPivot(virQEMUDriverPtr driver, } if (bitmapactions && rc == 0) - rc = qemuMonitorTransaction(priv->mon, &bitmapactions); + ignore_value(qemuMonitorTransaction(priv->mon, &bitmapactions)); if (rc == 0) ret = qemuMonitorJobComplete(priv->mon, job->name); -- 2.26.2
On 7/16/20 9:20 AM, Peter Krempa wrote:
Ignore errors from creating "libvirt-tmp-activewrite" bitmap. This prevents failures of finishing blockjobs if the bitmap already exists.
Note that if the bitmap exists, the worst case that can happen is that more bits are marked as dirty in the resulting merge.
In turn, your incremental backup might be larger than it needs to be, but you have not lost any data. I agree this is safe.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Reviewed-by: Eric Blake <eblake@redhat.com>
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 348ef17141..64ddc8dce9 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -17414,7 +17414,7 @@ qemuDomainBlockPivot(virQEMUDriverPtr driver, }
if (bitmapactions && rc == 0) - rc = qemuMonitorTransaction(priv->mon, &bitmapactions); + ignore_value(qemuMonitorTransaction(priv->mon, &bitmapactions));
if (rc == 0) ret = qemuMonitorJobComplete(priv->mon, job->name);
-- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
participants (2)
-
Eric Blake -
Peter Krempa