savevm-async: avoid segfault when aborting snapshot

[pve-qemu.git] / debian / patches / pve / 0042-PVE-Use-coroutine-QMP-for-backup-cancel_backup.patch

From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
From: Stefan Reiter <s.reiter@proxmox.com>
Date: Tue, 26 Jan 2021 15:45:30 +0100
Subject: [PATCH] PVE: Use coroutine QMP for backup/cancel_backup

Finally turn backup QMP calls into coroutines, now that it's possible.
This has the benefit that calls are asynchronous to the main loop, i.e.
long running operations like connecting to a PBS server will no longer
hang the VM.

Additionally, it allows us to get rid of block_on_coroutine_fn, which
was always a hacky workaround.

While we're already spring cleaning, also remove the QmpBackupTask
struct, since we can now put the 'prepare' function directly into
qmp_backup and thus no longer need those giant walls of text.

(Note that for our patches to work with 5.2.0 this change is actually
required, otherwise monitor_get_fd() fails as we're not in a QMP
coroutine, but one we start ourselves - we could of course set the
monitor for that coroutine ourselves, but let's just fix it the right
way instead)

Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
---
 block/monitor/block-hmp-cmds.c |   4 +-
 hmp-commands.hx                |   2 +
 proxmox-backup-client.c        |  31 -----
 pve-backup.c                   | 232 ++++++++++-----------------------
 qapi/block-core.json           |   4 +-
 5 files changed, 77 insertions(+), 196 deletions(-)

diff --git a/block/monitor/block-hmp-cmds.c b/block/monitor/block-hmp-cmds.c
index ea7b665aa2..ef45552e3b 100644
--- a/block/monitor/block-hmp-cmds.c
+++ b/block/monitor/block-hmp-cmds.c
@@ -1016,7 +1016,7 @@ void hmp_info_snapshots(Monitor *mon, const QDict *qdict)
     g_free(global_snapshots);
 }
 
-void hmp_backup_cancel(Monitor *mon, const QDict *qdict)
+void coroutine_fn hmp_backup_cancel(Monitor *mon, const QDict *qdict)
 {
     Error *error = NULL;
 
@@ -1025,7 +1025,7 @@ void hmp_backup_cancel(Monitor *mon, const QDict *qdict)
     hmp_handle_error(mon, error);
 }
 
-void hmp_backup(Monitor *mon, const QDict *qdict)
+void coroutine_fn hmp_backup(Monitor *mon, const QDict *qdict)
 {
     Error *error = NULL;
 
diff --git a/hmp-commands.hx b/hmp-commands.hx
index 994445f301..aaea875970 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -109,6 +109,7 @@ ERST
 		    "\n\t\t\t Use -d to dump data into a directory instead"
 		    "\n\t\t\t of using VMA format.",
         .cmd = hmp_backup,
+        .coroutine  = true,
     },
 
 SRST
@@ -122,6 +123,7 @@ ERST
         .params     = "",
         .help       = "cancel the current VM backup",
         .cmd        = hmp_backup_cancel,
+        .coroutine  = true,
     },
 
 SRST
diff --git a/proxmox-backup-client.c b/proxmox-backup-client.c
index 4ce7bc0b5e..0923037dec 100644
--- a/proxmox-backup-client.c
+++ b/proxmox-backup-client.c
@@ -5,37 +5,6 @@
 
 /* Proxmox Backup Server client bindings using coroutines */
 
-typedef struct BlockOnCoroutineWrapper {
-    AioContext *ctx;
-    CoroutineEntry *entry;
-    void *entry_arg;
-    bool finished;
-} BlockOnCoroutineWrapper;
-
-static void coroutine_fn block_on_coroutine_wrapper(void *opaque)
-{
-    BlockOnCoroutineWrapper *wrapper = opaque;
-    wrapper->entry(wrapper->entry_arg);
-    wrapper->finished = true;
-    aio_wait_kick();
-}
-
-void block_on_coroutine_fn(CoroutineEntry *entry, void *entry_arg)
-{
-    assert(!qemu_in_coroutine());
-
-    AioContext *ctx = qemu_get_current_aio_context();
-    BlockOnCoroutineWrapper wrapper = {
-        .finished = false,
-        .entry = entry,
-        .entry_arg = entry_arg,
-        .ctx = ctx,
-    };
-    Coroutine *wrapper_co = qemu_coroutine_create(block_on_coroutine_wrapper, &wrapper);
-    aio_co_enter(ctx, wrapper_co);
-    AIO_WAIT_WHILE(ctx, !wrapper.finished);
-}
-
 // This is called from another thread, so we use aio_co_schedule()
 static void proxmox_backup_schedule_wake(void *data) {
     CoCtxData *waker = (CoCtxData *)data;
diff --git a/pve-backup.c b/pve-backup.c
index 5fa3cc1352..323014744c 100644
--- a/pve-backup.c
+++ b/pve-backup.c
@@ -357,7 +357,7 @@ static void job_cancel_bh(void *opaque) {
     aio_co_enter(data->ctx, data->co);
 }
 
-static void coroutine_fn pvebackup_co_cancel(void *opaque)
+void coroutine_fn qmp_backup_cancel(Error **errp)
 {
     Error *cancel_err = NULL;
     error_setg(&cancel_err, "backup canceled");
@@ -394,11 +394,6 @@ static void coroutine_fn pvebackup_co_cancel(void *opaque)
     qemu_co_mutex_unlock(&backup_state.backup_mutex);
 }
 
-void qmp_backup_cancel(Error **errp)
-{
-    block_on_coroutine_fn(pvebackup_co_cancel, NULL);
-}
-
 // assumes the caller holds backup_mutex
 static int coroutine_fn pvebackup_co_add_config(
     const char *file,
@@ -533,50 +528,27 @@ static void create_backup_jobs_bh(void *opaque) {
     aio_co_enter(data->ctx, data->co);
 }
 
-typedef struct QmpBackupTask {
-    const char *backup_file;
-    bool has_password;
-    const char *password;
-    bool has_keyfile;
-    const char *keyfile;
-    bool has_key_password;
-    const char *key_password;
-    bool has_backup_id;
-    const char *backup_id;
-    bool has_backup_time;
-    const char *fingerprint;
-    bool has_fingerprint;
-    int64_t backup_time;
-    bool has_use_dirty_bitmap;
-    bool use_dirty_bitmap;
-    bool has_format;
-    BackupFormat format;
-    bool has_config_file;
-    const char *config_file;
-    bool has_firewall_file;
-    const char *firewall_file;
-    bool has_devlist;
-    const char *devlist;
-    bool has_compress;
-    bool compress;
-    bool has_encrypt;
-    bool encrypt;
-    bool has_speed;
-    int64_t speed;
-    Error **errp;
-    UuidInfo *result;
-} QmpBackupTask;
-
-static void coroutine_fn pvebackup_co_prepare(void *opaque)
+UuidInfo coroutine_fn *qmp_backup(
+    const char *backup_file,
+    bool has_password, const char *password,
+    bool has_keyfile, const char *keyfile,
+    bool has_key_password, const char *key_password,
+    bool has_fingerprint, const char *fingerprint,
+    bool has_backup_id, const char *backup_id,
+    bool has_backup_time, int64_t backup_time,
+    bool has_use_dirty_bitmap, bool use_dirty_bitmap,
+    bool has_compress, bool compress,
+    bool has_encrypt, bool encrypt,
+    bool has_format, BackupFormat format,
+    bool has_config_file, const char *config_file,
+    bool has_firewall_file, const char *firewall_file,
+    bool has_devlist, const char *devlist,
+    bool has_speed, int64_t speed, Error **errp)
 {
     assert(qemu_in_coroutine());
 
     qemu_co_mutex_lock(&backup_state.backup_mutex);
 
-    QmpBackupTask *task = opaque;
-
-    task->result = NULL; // just to be sure
-
     BlockBackend *blk;
     BlockDriverState *bs = NULL;
     const char *backup_dir = NULL;
@@ -593,17 +565,17 @@ static void coroutine_fn pvebackup_co_prepare(void *opaque)
     const char *firewall_name = "qemu-server.fw";
 
     if (backup_state.di_list) {
-        error_set(task->errp, ERROR_CLASS_GENERIC_ERROR,
+        error_set(errp, ERROR_CLASS_GENERIC_ERROR,
                   "previous backup not finished");
         qemu_co_mutex_unlock(&backup_state.backup_mutex);
-        return;
+        return NULL;
     }
 
     /* Todo: try to auto-detect format based on file name */
-    BackupFormat format = task->has_format ? task->format : BACKUP_FORMAT_VMA;
+    format = has_format ? format : BACKUP_FORMAT_VMA;
 
-    if (task->has_devlist) {
-        devs = g_strsplit_set(task->devlist, ",;:", -1);
+    if (has_devlist) {
+        devs = g_strsplit_set(devlist, ",;:", -1);
 
         gchar **d = devs;
         while (d && *d) {
@@ -611,14 +583,14 @@ static void coroutine_fn pvebackup_co_prepare(void *opaque)
             if (blk) {
                 bs = blk_bs(blk);
                 if (!bdrv_is_inserted(bs)) {
-                    error_setg(task->errp, QERR_DEVICE_HAS_NO_MEDIUM, *d);
+                    error_setg(errp, QERR_DEVICE_HAS_NO_MEDIUM, *d);
                     goto err;
                 }
                 PVEBackupDevInfo *di = g_new0(PVEBackupDevInfo, 1);
                 di->bs = bs;
                 di_list = g_list_append(di_list, di);
             } else {
-                error_set(task->errp, ERROR_CLASS_DEVICE_NOT_FOUND,
+                error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND,
                           "Device '%s' not found", *d);
                 goto err;
             }
@@ -641,7 +613,7 @@ static void coroutine_fn pvebackup_co_prepare(void *opaque)
     }
 
     if (!di_list) {
-        error_set(task->errp, ERROR_CLASS_GENERIC_ERROR, "empty device list");
+        error_set(errp, ERROR_CLASS_GENERIC_ERROR, "empty device list");
         goto err;
     }
 
@@ -651,13 +623,13 @@ static void coroutine_fn pvebackup_co_prepare(void *opaque)
     while (l) {
         PVEBackupDevInfo *di = (PVEBackupDevInfo *)l->data;
         l = g_list_next(l);
-        if (bdrv_op_is_blocked(di->bs, BLOCK_OP_TYPE_BACKUP_SOURCE, task->errp)) {
+        if (bdrv_op_is_blocked(di->bs, BLOCK_OP_TYPE_BACKUP_SOURCE, errp)) {
             goto err;
         }
 
         ssize_t size = bdrv_getlength(di->bs);
         if (size < 0) {
-            error_setg_errno(task->errp, -di->size, "bdrv_getlength failed");
+            error_setg_errno(errp, -di->size, "bdrv_getlength failed");
             goto err;
         }
         di->size = size;
@@ -684,47 +656,44 @@ static void coroutine_fn pvebackup_co_prepare(void *opaque)
     }
 
     if (format == BACKUP_FORMAT_PBS) {
-        if (!task->has_password) {
-            error_set(task->errp, ERROR_CLASS_GENERIC_ERROR, "missing parameter 'password'");
+        if (!has_password) {
+            error_set(errp, ERROR_CLASS_GENERIC_ERROR, "missing parameter 'password'");
             goto err_mutex;
         }
-        if (!task->has_backup_id) {
-            error_set(task->errp, ERROR_CLASS_GENERIC_ERROR, "missing parameter 'backup-id'");
+        if (!has_backup_id) {
+            error_set(errp, ERROR_CLASS_GENERIC_ERROR, "missing parameter 'backup-id'");
             goto err_mutex;
         }
-        if (!task->has_backup_time) {
-            error_set(task->errp, ERROR_CLASS_GENERIC_ERROR, "missing parameter 'backup-time'");
+        if (!has_backup_time) {
+            error_set(errp, ERROR_CLASS_GENERIC_ERROR, "missing parameter 'backup-time'");
             goto err_mutex;
         }
 
         int dump_cb_block_size = PROXMOX_BACKUP_DEFAULT_CHUNK_SIZE; // Hardcoded (4M)
         firewall_name = "fw.conf";
 
-        bool use_dirty_bitmap = task->has_use_dirty_bitmap && task->use_dirty_bitmap;
-
-
         char *pbs_err = NULL;
         pbs = proxmox_backup_new(
-            task->backup_file,
-            task->backup_id,
-            task->backup_time,
+            backup_file,
+            backup_id,
+            backup_time,
             dump_cb_block_size,
-            task->has_password ? task->password : NULL,
-            task->has_keyfile ? task->keyfile : NULL,
-            task->has_key_password ? task->key_password : NULL,
-            task->has_compress ? task->compress : true,
-            task->has_encrypt ? task->encrypt : task->has_keyfile,
-            task->has_fingerprint ? task->fingerprint : NULL,
+            has_password ? password : NULL,
+            has_keyfile ? keyfile : NULL,
+            has_key_password ? key_password : NULL,
+            has_compress ? compress : true,
+            has_encrypt ? encrypt : has_keyfile,
+            has_fingerprint ? fingerprint : NULL,
              &pbs_err);
 
         if (!pbs) {
-            error_set(task->errp, ERROR_CLASS_GENERIC_ERROR,
+            error_set(errp, ERROR_CLASS_GENERIC_ERROR,
                       "proxmox_backup_new failed: %s", pbs_err);
             proxmox_backup_free_error(pbs_err);
             goto err_mutex;
         }
 
-        int connect_result = proxmox_backup_co_connect(pbs, task->errp);
+        int connect_result = proxmox_backup_co_connect(pbs, errp);
         if (connect_result < 0)
             goto err_mutex;
 
@@ -743,9 +712,9 @@ static void coroutine_fn pvebackup_co_prepare(void *opaque)
             BdrvDirtyBitmap *bitmap = bdrv_find_dirty_bitmap(di->bs, PBS_BITMAP_NAME);
             bool expect_only_dirty = false;
 
-            if (use_dirty_bitmap) {
+            if (has_use_dirty_bitmap && use_dirty_bitmap) {
                 if (bitmap == NULL) {
-                    bitmap = bdrv_create_dirty_bitmap(di->bs, dump_cb_block_size, PBS_BITMAP_NAME, task->errp);
+                    bitmap = bdrv_create_dirty_bitmap(di->bs, dump_cb_block_size, PBS_BITMAP_NAME, errp);
                     if (!bitmap) {
                         goto err_mutex;
                     }
@@ -775,12 +744,12 @@ static void coroutine_fn pvebackup_co_prepare(void *opaque)
                 }
             }
 
-            int dev_id = proxmox_backup_co_register_image(pbs, devname, di->size, expect_only_dirty, task->errp);
+            int dev_id = proxmox_backup_co_register_image(pbs, devname, di->size, expect_only_dirty, errp);
             if (dev_id < 0) {
                 goto err_mutex;
             }
 
-            if (!(di->target = bdrv_backup_dump_create(dump_cb_block_size, di->size, pvebackup_co_dump_pbs_cb, di, task->errp))) {
+            if (!(di->target = bdrv_backup_dump_create(dump_cb_block_size, di->size, pvebackup_co_dump_pbs_cb, di, errp))) {
                 goto err_mutex;
             }
 
@@ -794,10 +763,10 @@ static void coroutine_fn pvebackup_co_prepare(void *opaque)
             backup_state.stat.bitmap_list = g_list_append(backup_state.stat.bitmap_list, info);
         }
     } else if (format == BACKUP_FORMAT_VMA) {
-        vmaw = vma_writer_create(task->backup_file, uuid, &local_err);
+        vmaw = vma_writer_create(backup_file, uuid, &local_err);
         if (!vmaw) {
             if (local_err) {
-                error_propagate(task->errp, local_err);
+                error_propagate(errp, local_err);
             }
             goto err_mutex;
         }
@@ -808,25 +777,25 @@ static void coroutine_fn pvebackup_co_prepare(void *opaque)
             PVEBackupDevInfo *di = (PVEBackupDevInfo *)l->data;
             l = g_list_next(l);
 
-            if (!(di->target = bdrv_backup_dump_create(VMA_CLUSTER_SIZE, di->size, pvebackup_co_dump_vma_cb, di, task->errp))) {
+            if (!(di->target = bdrv_backup_dump_create(VMA_CLUSTER_SIZE, di->size, pvebackup_co_dump_vma_cb, di, errp))) {
                 goto err_mutex;
             }
 
             const char *devname = bdrv_get_device_name(di->bs);
             di->dev_id = vma_writer_register_stream(vmaw, devname, di->size);
             if (di->dev_id <= 0) {
-                error_set(task->errp, ERROR_CLASS_GENERIC_ERROR,
+                error_set(errp, ERROR_CLASS_GENERIC_ERROR,
                           "register_stream failed");
                 goto err_mutex;
             }
         }
     } else if (format == BACKUP_FORMAT_DIR) {
-        if (mkdir(task->backup_file, 0640) != 0) {
-            error_setg_errno(task->errp, errno, "can't create directory '%s'\n",
-                             task->backup_file);
+        if (mkdir(backup_file, 0640) != 0) {
+            error_setg_errno(errp, errno, "can't create directory '%s'\n",
+                             backup_file);
             goto err_mutex;
         }
-        backup_dir = task->backup_file;
+        backup_dir = backup_file;
 
         l = di_list;
         while (l) {
@@ -840,34 +809,34 @@ static void coroutine_fn pvebackup_co_prepare(void *opaque)
             bdrv_img_create(di->targetfile, "raw", NULL, NULL, NULL,
                             di->size, flags, false, &local_err);
             if (local_err) {
-                error_propagate(task->errp, local_err);
+                error_propagate(errp, local_err);
                 goto err_mutex;
             }
 
             di->target = bdrv_open(di->targetfile, NULL, NULL, flags, &local_err);
             if (!di->target) {
-                error_propagate(task->errp, local_err);
+                error_propagate(errp, local_err);
                 goto err_mutex;
             }
         }
     } else {
-        error_set(task->errp, ERROR_CLASS_GENERIC_ERROR, "unknown backup format");
+        error_set(errp, ERROR_CLASS_GENERIC_ERROR, "unknown backup format");
         goto err_mutex;
     }
 
 
     /* add configuration file to archive */
-    if (task->has_config_file) {
-        if (pvebackup_co_add_config(task->config_file, config_name, format, backup_dir,
-                                    vmaw, pbs, task->errp) != 0) {
+    if (has_config_file) {
+        if (pvebackup_co_add_config(config_file, config_name, format, backup_dir,
+                                    vmaw, pbs, errp) != 0) {
             goto err_mutex;
         }
     }
 
     /* add firewall file to archive */
-    if (task->has_firewall_file) {
-        if (pvebackup_co_add_config(task->firewall_file, firewall_name, format, backup_dir,
-                                    vmaw, pbs, task->errp) != 0) {
+    if (has_firewall_file) {
+        if (pvebackup_co_add_config(firewall_file, firewall_name, format, backup_dir,
+                                    vmaw, pbs, errp) != 0) {
             goto err_mutex;
         }
     }
@@ -885,7 +854,7 @@ static void coroutine_fn pvebackup_co_prepare(void *opaque)
     if (backup_state.stat.backup_file) {
         g_free(backup_state.stat.backup_file);
     }
-    backup_state.stat.backup_file = g_strdup(task->backup_file);
+    backup_state.stat.backup_file = g_strdup(backup_file);
 
     uuid_copy(backup_state.stat.uuid, uuid);
     uuid_unparse_lower(uuid, backup_state.stat.uuid_str);
@@ -900,7 +869,7 @@ static void coroutine_fn pvebackup_co_prepare(void *opaque)
 
     qemu_mutex_unlock(&backup_state.stat.lock);
 
-    backup_state.speed = (task->has_speed && task->speed > 0) ? task->speed : 0;
+    backup_state.speed = (has_speed && speed > 0) ? speed : 0;
 
     backup_state.vmaw = vmaw;
     backup_state.pbs = pbs;
@@ -910,8 +879,6 @@ static void coroutine_fn pvebackup_co_prepare(void *opaque)
     uuid_info = g_malloc0(sizeof(*uuid_info));
     uuid_info->UUID = uuid_str;
 
-    task->result = uuid_info;
-
     /* Run create_backup_jobs_bh outside of coroutine (in BH) but keep
     * backup_mutex locked. This is fine, a CoMutex can be held across yield
     * points, and we'll release it as soon as the BH reschedules us.
@@ -925,7 +892,7 @@ static void coroutine_fn pvebackup_co_prepare(void *opaque)
     qemu_coroutine_yield();
 
     if (local_err) {
-        error_propagate(task->errp, local_err);
+        error_propagate(errp, local_err);
         goto err;
     }
 
@@ -938,7 +905,7 @@ static void coroutine_fn pvebackup_co_prepare(void *opaque)
     /* start the first job in the transaction */
     job_txn_start_seq(backup_state.txn);
 
-    return;
+    return uuid_info;
 
 err_mutex:
     qemu_mutex_unlock(&backup_state.stat.lock);
@@ -969,7 +936,7 @@ err:
     if (vmaw) {
         Error *err = NULL;
         vma_writer_close(vmaw, &err);
-        unlink(task->backup_file);
+        unlink(backup_file);
     }
 
     if (pbs) {
@@ -980,65 +947,8 @@ err:
         rmdir(backup_dir);
     }
 
-    task->result = NULL;
-
     qemu_co_mutex_unlock(&backup_state.backup_mutex);
-    return;
-}
-
-UuidInfo *qmp_backup(
-    const char *backup_file,
-    bool has_password, const char *password,
-    bool has_keyfile, const char *keyfile,
-    bool has_key_password, const char *key_password,
-    bool has_fingerprint, const char *fingerprint,
-    bool has_backup_id, const char *backup_id,
-    bool has_backup_time, int64_t backup_time,
-    bool has_use_dirty_bitmap, bool use_dirty_bitmap,
-    bool has_compress, bool compress,
-    bool has_encrypt, bool encrypt,
-    bool has_format, BackupFormat format,
-    bool has_config_file, const char *config_file,
-    bool has_firewall_file, const char *firewall_file,
-    bool has_devlist, const char *devlist,
-    bool has_speed, int64_t speed, Error **errp)
-{
-    QmpBackupTask task = {
-        .backup_file = backup_file,
-        .has_password = has_password,
-        .password = password,
-        .has_keyfile = has_keyfile,
-        .keyfile = keyfile,
-        .has_key_password = has_key_password,
-        .key_password = key_password,
-        .has_fingerprint = has_fingerprint,
-        .fingerprint = fingerprint,
-        .has_backup_id = has_backup_id,
-        .backup_id = backup_id,
-        .has_backup_time = has_backup_time,
-        .backup_time = backup_time,
-        .has_use_dirty_bitmap = has_use_dirty_bitmap,
-        .use_dirty_bitmap = use_dirty_bitmap,
-        .has_compress = has_compress,
-        .compress = compress,
-        .has_encrypt = has_encrypt,
-        .encrypt = encrypt,
-        .has_format = has_format,
-        .format = format,
-        .has_config_file = has_config_file,
-        .config_file = config_file,
-        .has_firewall_file = has_firewall_file,
-        .firewall_file = firewall_file,
-        .has_devlist = has_devlist,
-        .devlist = devlist,
-        .has_speed = has_speed,
-        .speed = speed,
-        .errp = errp,
-    };
-
-    block_on_coroutine_fn(pvebackup_co_prepare, &task);
-
-    return task.result;
+    return NULL;
 }
 
 BackupStatus *qmp_query_backup(Error **errp)
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 05382a1069..f0399bf249 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -846,7 +846,7 @@
                                     '*config-file': 'str',
                                     '*firewall-file': 'str',
                                     '*devlist': 'str', '*speed': 'int' },
-  'returns': 'UuidInfo' }
+  'returns': 'UuidInfo', 'coroutine': true }
 
 ##
 # @query-backup:
@@ -868,7 +868,7 @@
 # Notes: This command succeeds even if there is no backup process running.
 #
 ##
-{ 'command': 'backup-cancel' }
+{ 'command': 'backup-cancel', 'coroutine': true }
 
 ##
 # @ProxmoxSupportStatus: