[pve-devel] [PATCH INFO v2 qemu 3/5] PVE: Use coroutine QMP for backup/cancel_backup

[Stefan Reiter <s.reiter@proxmox.com>]

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>
---

Included in first patch.
No change from v1.

 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 46c63b1cf9..11c84d5508 100644
--- a/block/monitor/block-hmp-cmds.c
+++ b/block/monitor/block-hmp-cmds.c
@@ -1013,7 +1013,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;
 
@@ -1022,7 +1022,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 70257ef7ca..b7639d189d 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -108,6 +108,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
@@ -121,6 +122,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 6b25292ba1..f7597ae55c 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,
@@ -531,50 +526,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;
@@ -591,17 +563,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) {
@@ -609,14 +581,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;
             }
@@ -639,7 +611,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;
     }
 
@@ -649,13 +621,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;
@@ -682,47 +654,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;
 
@@ -741,9 +710,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;
                     }
@@ -773,12 +742,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;
             }
 
@@ -792,10 +761,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;
         }
@@ -806,25 +775,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) {
@@ -838,34 +807,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;
         }
     }
@@ -883,7 +852,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);
@@ -898,7 +867,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;
@@ -908,8 +877,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.
@@ -923,7 +890,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;
     }
 
@@ -936,7 +903,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);
@@ -967,7 +934,7 @@ err:
     if (vmaw) {
         Error *err = NULL;
         vma_writer_close(vmaw, &err);
-        unlink(task->backup_file);
+        unlink(backup_file);
     }
 
     if (pbs) {
@@ -978,65 +945,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 dee3d87efe..82133e2bee 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -847,7 +847,7 @@
                                     '*config-file': 'str',
                                     '*firewall-file': 'str',
                                     '*devlist': 'str', '*speed': 'int' },
-  'returns': 'UuidInfo' }
+  'returns': 'UuidInfo', 'coroutine': true }
 
 ##
 # @query-backup:
@@ -869,7 +869,7 @@
 # Notes: This command succeeds even if there is no backup process running.
 #
 ##
-{ 'command': 'backup-cancel' }
+{ 'command': 'backup-cancel', 'coroutine': true }
 
 ##
 # @ProxmoxSupportStatus:
-- 
2.20.1