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