[pve-devel] [PATCH qemu 02/23] PVE backup: fixup error handling for fleecing

[Fiona Ebner]

The drained section needs to be terminated before breaking out of the
loop in the error scenarios. Otherwise, guest IO on the drive would
become stuck.

If the job is created successfully, then the job completion callback
will clean up the snapshot access block nodes. In case failure
happened before the job is created, there was no cleanup for the
snapshot access block nodes yet. Add it.

Signed-off-by: Fiona Ebner <f.ebner at proxmox.com>
---
 pve-backup.c | 38 +++++++++++++++++++++++++-------------
 1 file changed, 25 insertions(+), 13 deletions(-)

diff --git a/pve-backup.c b/pve-backup.c
index 4e730aa3da..c4178758b3 100644
--- a/pve-backup.c
+++ b/pve-backup.c
@@ -357,22 +357,23 @@ static void coroutine_fn pvebackup_co_complete_stream(void *opaque)
     qemu_co_mutex_unlock(&backup_state.backup_mutex);
 }
 
+static void cleanup_snapshot_access(PVEBackupDevInfo *di)
+{
+    if (di->fleecing.snapshot_access) {
+        bdrv_unref(di->fleecing.snapshot_access);
+        di->fleecing.snapshot_access = NULL;
+    }
+    if (di->fleecing.cbw) {
+        bdrv_cbw_drop(di->fleecing.cbw);
+        di->fleecing.cbw = NULL;
+    }
+}
+
 static void pvebackup_complete_cb(void *opaque, int ret)
 {
     PVEBackupDevInfo *di = opaque;
     di->completed_ret = ret;
 
-    /*
-     * Handle block-graph specific cleanup (for fleecing) outside of the coroutine, because the work
-     * won't be done as a coroutine anyways:
-     * - For snapshot_access, allows doing bdrv_unref() directly. Doing it via bdrv_co_unref() would
-     *   just spawn a BH calling bdrv_unref().
-     * - For cbw, draining would need to spawn a BH.
-     */
-    if (di->fleecing.snapshot_access) {
-        bdrv_unref(di->fleecing.snapshot_access);
-        di->fleecing.snapshot_access = NULL;
-    }
     if (di->fleecing.cbw) {
         /*
          * With fleecing, failure for cbw does not fail the guest write, but only sets the snapshot
@@ -383,10 +384,17 @@ static void pvebackup_complete_cb(void *opaque, int ret)
         if (di->completed_ret == -EACCES && snapshot_error) {
             di->completed_ret = snapshot_error;
         }
-        bdrv_cbw_drop(di->fleecing.cbw);
-        di->fleecing.cbw = NULL;
     }
 
+    /*
+     * Handle block-graph specific cleanup (for fleecing) outside of the coroutine, because the work
+     * won't be done as a coroutine anyways:
+     * - For snapshot_access, allows doing bdrv_unref() directly. Doing it via bdrv_co_unref() would
+     *   just spawn a BH calling bdrv_unref().
+     * - For cbw, draining would need to spawn a BH.
+     */
+    cleanup_snapshot_access(di);
+
     /*
      * Needs to happen outside of coroutine, because it takes the graph write lock.
      */
@@ -587,6 +595,7 @@ static void create_backup_jobs_bh(void *opaque) {
             if (!di->fleecing.cbw) {
                 error_setg(errp, "appending cbw node for fleecing failed: %s",
                            local_err ? error_get_pretty(local_err) : "unknown error");
+                bdrv_drained_end(di->bs);
                 break;
             }
 
@@ -599,6 +608,8 @@ static void create_backup_jobs_bh(void *opaque) {
             if (!di->fleecing.snapshot_access) {
                 error_setg(errp, "setting up snapshot access for fleecing failed: %s",
                            local_err ? error_get_pretty(local_err) : "unknown error");
+                cleanup_snapshot_access(di);
+                bdrv_drained_end(di->bs);
                 break;
             }
             source_bs = di->fleecing.snapshot_access;
@@ -637,6 +648,7 @@ static void create_backup_jobs_bh(void *opaque) {
         }
 
         if (!job || local_err) {
+            cleanup_snapshot_access(di);
             error_setg(errp, "backup_job_create failed: %s",
                        local_err ? error_get_pretty(local_err) : "null");
             break;
-- 
2.39.2