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@xxxxxxxxxx>
---
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@xxxxxxxxxx>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
