1 From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
2 From: Stefan Reiter <s.reiter@proxmox.com>
3 Date: Tue, 26 Jan 2021 15:45:30 +0100
4 Subject: [PATCH] PVE: Use coroutine QMP for backup/cancel_backup
6 Finally turn backup QMP calls into coroutines, now that it's possible.
7 This has the benefit that calls are asynchronous to the main loop, i.e.
8 long running operations like connecting to a PBS server will no longer
11 Additionally, it allows us to get rid of block_on_coroutine_fn, which
12 was always a hacky workaround.
14 While we're already spring cleaning, also remove the QmpBackupTask
15 struct, since we can now put the 'prepare' function directly into
16 qmp_backup and thus no longer need those giant walls of text.
18 (Note that for our patches to work with 5.2.0 this change is actually
19 required, otherwise monitor_get_fd() fails as we're not in a QMP
20 coroutine, but one we start ourselves - we could of course set the
21 monitor for that coroutine ourselves, but let's just fix it the right
24 Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
26 block/monitor/block-hmp-cmds.c | 4 +-
28 proxmox-backup-client.c | 31 -----
29 pve-backup.c | 232 ++++++++++-----------------------
30 qapi/block-core.json | 4 +-
31 5 files changed, 77 insertions(+), 196 deletions(-)
33 diff --git a/block/monitor/block-hmp-cmds.c b/block/monitor/block-hmp-cmds.c
34 index 69254396d5..b838586fc0 100644
35 --- a/block/monitor/block-hmp-cmds.c
36 +++ b/block/monitor/block-hmp-cmds.c
37 @@ -1016,7 +1016,7 @@ void hmp_info_snapshots(Monitor *mon, const QDict *qdict)
38 g_free(global_snapshots);
41 -void hmp_backup_cancel(Monitor *mon, const QDict *qdict)
42 +void coroutine_fn hmp_backup_cancel(Monitor *mon, const QDict *qdict)
46 @@ -1025,7 +1025,7 @@ void hmp_backup_cancel(Monitor *mon, const QDict *qdict)
47 hmp_handle_error(mon, error);
50 -void hmp_backup(Monitor *mon, const QDict *qdict)
51 +void coroutine_fn hmp_backup(Monitor *mon, const QDict *qdict)
55 diff --git a/hmp-commands.hx b/hmp-commands.hx
56 index b18063ce19..02c8f83ca3 100644
59 @@ -109,6 +109,7 @@ ERST
60 "\n\t\t\t Use -d to dump data into a directory instead"
61 "\n\t\t\t of using VMA format.",
67 @@ -122,6 +123,7 @@ ERST
69 .help = "cancel the current VM backup",
70 .cmd = hmp_backup_cancel,
75 diff --git a/proxmox-backup-client.c b/proxmox-backup-client.c
76 index 4ce7bc0b5e..0923037dec 100644
77 --- a/proxmox-backup-client.c
78 +++ b/proxmox-backup-client.c
81 /* Proxmox Backup Server client bindings using coroutines */
83 -typedef struct BlockOnCoroutineWrapper {
85 - CoroutineEntry *entry;
88 -} BlockOnCoroutineWrapper;
90 -static void coroutine_fn block_on_coroutine_wrapper(void *opaque)
92 - BlockOnCoroutineWrapper *wrapper = opaque;
93 - wrapper->entry(wrapper->entry_arg);
94 - wrapper->finished = true;
98 -void block_on_coroutine_fn(CoroutineEntry *entry, void *entry_arg)
100 - assert(!qemu_in_coroutine());
102 - AioContext *ctx = qemu_get_current_aio_context();
103 - BlockOnCoroutineWrapper wrapper = {
106 - .entry_arg = entry_arg,
109 - Coroutine *wrapper_co = qemu_coroutine_create(block_on_coroutine_wrapper, &wrapper);
110 - aio_co_enter(ctx, wrapper_co);
111 - AIO_WAIT_WHILE(ctx, !wrapper.finished);
114 // This is called from another thread, so we use aio_co_schedule()
115 static void proxmox_backup_schedule_wake(void *data) {
116 CoCtxData *waker = (CoCtxData *)data;
117 diff --git a/pve-backup.c b/pve-backup.c
118 index 59ccb38ceb..f858003a06 100644
121 @@ -357,7 +357,7 @@ static void job_cancel_bh(void *opaque) {
122 aio_co_enter(data->ctx, data->co);
125 -static void coroutine_fn pvebackup_co_cancel(void *opaque)
126 +void coroutine_fn qmp_backup_cancel(Error **errp)
128 Error *cancel_err = NULL;
129 error_setg(&cancel_err, "backup canceled");
130 @@ -394,11 +394,6 @@ static void coroutine_fn pvebackup_co_cancel(void *opaque)
131 qemu_co_mutex_unlock(&backup_state.backup_mutex);
134 -void qmp_backup_cancel(Error **errp)
136 - block_on_coroutine_fn(pvebackup_co_cancel, NULL);
139 // assumes the caller holds backup_mutex
140 static int coroutine_fn pvebackup_co_add_config(
142 @@ -533,50 +528,27 @@ static void create_backup_jobs_bh(void *opaque) {
143 aio_co_enter(data->ctx, data->co);
146 -typedef struct QmpBackupTask {
147 - const char *backup_file;
149 - const char *password;
151 - const char *keyfile;
152 - bool has_key_password;
153 - const char *key_password;
154 - bool has_backup_id;
155 - const char *backup_id;
156 - bool has_backup_time;
157 - const char *fingerprint;
158 - bool has_fingerprint;
159 - int64_t backup_time;
160 - bool has_use_dirty_bitmap;
161 - bool use_dirty_bitmap;
163 - BackupFormat format;
164 - bool has_config_file;
165 - const char *config_file;
166 - bool has_firewall_file;
167 - const char *firewall_file;
169 - const char *devlist;
180 -static void coroutine_fn pvebackup_co_prepare(void *opaque)
181 +UuidInfo coroutine_fn *qmp_backup(
182 + const char *backup_file,
183 + bool has_password, const char *password,
184 + bool has_keyfile, const char *keyfile,
185 + bool has_key_password, const char *key_password,
186 + bool has_fingerprint, const char *fingerprint,
187 + bool has_backup_id, const char *backup_id,
188 + bool has_backup_time, int64_t backup_time,
189 + bool has_use_dirty_bitmap, bool use_dirty_bitmap,
190 + bool has_compress, bool compress,
191 + bool has_encrypt, bool encrypt,
192 + bool has_format, BackupFormat format,
193 + bool has_config_file, const char *config_file,
194 + bool has_firewall_file, const char *firewall_file,
195 + bool has_devlist, const char *devlist,
196 + bool has_speed, int64_t speed, Error **errp)
198 assert(qemu_in_coroutine());
200 qemu_co_mutex_lock(&backup_state.backup_mutex);
202 - QmpBackupTask *task = opaque;
204 - task->result = NULL; // just to be sure
207 BlockDriverState *bs = NULL;
208 const char *backup_dir = NULL;
209 @@ -593,17 +565,17 @@ static void coroutine_fn pvebackup_co_prepare(void *opaque)
210 const char *firewall_name = "qemu-server.fw";
212 if (backup_state.di_list) {
213 - error_set(task->errp, ERROR_CLASS_GENERIC_ERROR,
214 + error_set(errp, ERROR_CLASS_GENERIC_ERROR,
215 "previous backup not finished");
216 qemu_co_mutex_unlock(&backup_state.backup_mutex);
221 /* Todo: try to auto-detect format based on file name */
222 - BackupFormat format = task->has_format ? task->format : BACKUP_FORMAT_VMA;
223 + format = has_format ? format : BACKUP_FORMAT_VMA;
225 - if (task->has_devlist) {
226 - devs = g_strsplit_set(task->devlist, ",;:", -1);
228 + devs = g_strsplit_set(devlist, ",;:", -1);
232 @@ -611,14 +583,14 @@ static void coroutine_fn pvebackup_co_prepare(void *opaque)
235 if (!bdrv_is_inserted(bs)) {
236 - error_setg(task->errp, QERR_DEVICE_HAS_NO_MEDIUM, *d);
237 + error_setg(errp, QERR_DEVICE_HAS_NO_MEDIUM, *d);
240 PVEBackupDevInfo *di = g_new0(PVEBackupDevInfo, 1);
242 di_list = g_list_append(di_list, di);
244 - error_set(task->errp, ERROR_CLASS_DEVICE_NOT_FOUND,
245 + error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND,
246 "Device '%s' not found", *d);
249 @@ -641,7 +613,7 @@ static void coroutine_fn pvebackup_co_prepare(void *opaque)
253 - error_set(task->errp, ERROR_CLASS_GENERIC_ERROR, "empty device list");
254 + error_set(errp, ERROR_CLASS_GENERIC_ERROR, "empty device list");
258 @@ -651,13 +623,13 @@ static void coroutine_fn pvebackup_co_prepare(void *opaque)
260 PVEBackupDevInfo *di = (PVEBackupDevInfo *)l->data;
262 - if (bdrv_op_is_blocked(di->bs, BLOCK_OP_TYPE_BACKUP_SOURCE, task->errp)) {
263 + if (bdrv_op_is_blocked(di->bs, BLOCK_OP_TYPE_BACKUP_SOURCE, errp)) {
267 ssize_t size = bdrv_getlength(di->bs);
269 - error_setg_errno(task->errp, -di->size, "bdrv_getlength failed");
270 + error_setg_errno(errp, -di->size, "bdrv_getlength failed");
274 @@ -684,47 +656,44 @@ static void coroutine_fn pvebackup_co_prepare(void *opaque)
277 if (format == BACKUP_FORMAT_PBS) {
278 - if (!task->has_password) {
279 - error_set(task->errp, ERROR_CLASS_GENERIC_ERROR, "missing parameter 'password'");
280 + if (!has_password) {
281 + error_set(errp, ERROR_CLASS_GENERIC_ERROR, "missing parameter 'password'");
284 - if (!task->has_backup_id) {
285 - error_set(task->errp, ERROR_CLASS_GENERIC_ERROR, "missing parameter 'backup-id'");
286 + if (!has_backup_id) {
287 + error_set(errp, ERROR_CLASS_GENERIC_ERROR, "missing parameter 'backup-id'");
290 - if (!task->has_backup_time) {
291 - error_set(task->errp, ERROR_CLASS_GENERIC_ERROR, "missing parameter 'backup-time'");
292 + if (!has_backup_time) {
293 + error_set(errp, ERROR_CLASS_GENERIC_ERROR, "missing parameter 'backup-time'");
297 int dump_cb_block_size = PROXMOX_BACKUP_DEFAULT_CHUNK_SIZE; // Hardcoded (4M)
298 firewall_name = "fw.conf";
300 - bool use_dirty_bitmap = task->has_use_dirty_bitmap && task->use_dirty_bitmap;
303 char *pbs_err = NULL;
304 pbs = proxmox_backup_new(
312 - task->has_password ? task->password : NULL,
313 - task->has_keyfile ? task->keyfile : NULL,
314 - task->has_key_password ? task->key_password : NULL,
315 - task->has_compress ? task->compress : true,
316 - task->has_encrypt ? task->encrypt : task->has_keyfile,
317 - task->has_fingerprint ? task->fingerprint : NULL,
318 + has_password ? password : NULL,
319 + has_keyfile ? keyfile : NULL,
320 + has_key_password ? key_password : NULL,
321 + has_compress ? compress : true,
322 + has_encrypt ? encrypt : has_keyfile,
323 + has_fingerprint ? fingerprint : NULL,
327 - error_set(task->errp, ERROR_CLASS_GENERIC_ERROR,
328 + error_set(errp, ERROR_CLASS_GENERIC_ERROR,
329 "proxmox_backup_new failed: %s", pbs_err);
330 proxmox_backup_free_error(pbs_err);
334 - int connect_result = proxmox_backup_co_connect(pbs, task->errp);
335 + int connect_result = proxmox_backup_co_connect(pbs, errp);
336 if (connect_result < 0)
339 @@ -743,9 +712,9 @@ static void coroutine_fn pvebackup_co_prepare(void *opaque)
340 BdrvDirtyBitmap *bitmap = bdrv_find_dirty_bitmap(di->bs, PBS_BITMAP_NAME);
341 bool expect_only_dirty = false;
343 - if (use_dirty_bitmap) {
344 + if (has_use_dirty_bitmap && use_dirty_bitmap) {
345 if (bitmap == NULL) {
346 - bitmap = bdrv_create_dirty_bitmap(di->bs, dump_cb_block_size, PBS_BITMAP_NAME, task->errp);
347 + bitmap = bdrv_create_dirty_bitmap(di->bs, dump_cb_block_size, PBS_BITMAP_NAME, errp);
351 @@ -775,12 +744,12 @@ static void coroutine_fn pvebackup_co_prepare(void *opaque)
355 - int dev_id = proxmox_backup_co_register_image(pbs, devname, di->size, expect_only_dirty, task->errp);
356 + int dev_id = proxmox_backup_co_register_image(pbs, devname, di->size, expect_only_dirty, errp);
361 - if (!(di->target = bdrv_backup_dump_create(dump_cb_block_size, di->size, pvebackup_co_dump_pbs_cb, di, task->errp))) {
362 + if (!(di->target = bdrv_backup_dump_create(dump_cb_block_size, di->size, pvebackup_co_dump_pbs_cb, di, errp))) {
366 @@ -794,10 +763,10 @@ static void coroutine_fn pvebackup_co_prepare(void *opaque)
367 backup_state.stat.bitmap_list = g_list_append(backup_state.stat.bitmap_list, info);
369 } else if (format == BACKUP_FORMAT_VMA) {
370 - vmaw = vma_writer_create(task->backup_file, uuid, &local_err);
371 + vmaw = vma_writer_create(backup_file, uuid, &local_err);
374 - error_propagate(task->errp, local_err);
375 + error_propagate(errp, local_err);
379 @@ -808,25 +777,25 @@ static void coroutine_fn pvebackup_co_prepare(void *opaque)
380 PVEBackupDevInfo *di = (PVEBackupDevInfo *)l->data;
383 - if (!(di->target = bdrv_backup_dump_create(VMA_CLUSTER_SIZE, di->size, pvebackup_co_dump_vma_cb, di, task->errp))) {
384 + if (!(di->target = bdrv_backup_dump_create(VMA_CLUSTER_SIZE, di->size, pvebackup_co_dump_vma_cb, di, errp))) {
388 const char *devname = bdrv_get_device_name(di->bs);
389 di->dev_id = vma_writer_register_stream(vmaw, devname, di->size);
390 if (di->dev_id <= 0) {
391 - error_set(task->errp, ERROR_CLASS_GENERIC_ERROR,
392 + error_set(errp, ERROR_CLASS_GENERIC_ERROR,
393 "register_stream failed");
397 } else if (format == BACKUP_FORMAT_DIR) {
398 - if (mkdir(task->backup_file, 0640) != 0) {
399 - error_setg_errno(task->errp, errno, "can't create directory '%s'\n",
400 - task->backup_file);
401 + if (mkdir(backup_file, 0640) != 0) {
402 + error_setg_errno(errp, errno, "can't create directory '%s'\n",
406 - backup_dir = task->backup_file;
407 + backup_dir = backup_file;
411 @@ -840,34 +809,34 @@ static void coroutine_fn pvebackup_co_prepare(void *opaque)
412 bdrv_img_create(di->targetfile, "raw", NULL, NULL, NULL,
413 di->size, flags, false, &local_err);
415 - error_propagate(task->errp, local_err);
416 + error_propagate(errp, local_err);
420 di->target = bdrv_open(di->targetfile, NULL, NULL, flags, &local_err);
422 - error_propagate(task->errp, local_err);
423 + error_propagate(errp, local_err);
428 - error_set(task->errp, ERROR_CLASS_GENERIC_ERROR, "unknown backup format");
429 + error_set(errp, ERROR_CLASS_GENERIC_ERROR, "unknown backup format");
434 /* add configuration file to archive */
435 - if (task->has_config_file) {
436 - if (pvebackup_co_add_config(task->config_file, config_name, format, backup_dir,
437 - vmaw, pbs, task->errp) != 0) {
438 + if (has_config_file) {
439 + if (pvebackup_co_add_config(config_file, config_name, format, backup_dir,
440 + vmaw, pbs, errp) != 0) {
445 /* add firewall file to archive */
446 - if (task->has_firewall_file) {
447 - if (pvebackup_co_add_config(task->firewall_file, firewall_name, format, backup_dir,
448 - vmaw, pbs, task->errp) != 0) {
449 + if (has_firewall_file) {
450 + if (pvebackup_co_add_config(firewall_file, firewall_name, format, backup_dir,
451 + vmaw, pbs, errp) != 0) {
455 @@ -885,7 +854,7 @@ static void coroutine_fn pvebackup_co_prepare(void *opaque)
456 if (backup_state.stat.backup_file) {
457 g_free(backup_state.stat.backup_file);
459 - backup_state.stat.backup_file = g_strdup(task->backup_file);
460 + backup_state.stat.backup_file = g_strdup(backup_file);
462 uuid_copy(backup_state.stat.uuid, uuid);
463 uuid_unparse_lower(uuid, backup_state.stat.uuid_str);
464 @@ -900,7 +869,7 @@ static void coroutine_fn pvebackup_co_prepare(void *opaque)
466 qemu_mutex_unlock(&backup_state.stat.lock);
468 - backup_state.speed = (task->has_speed && task->speed > 0) ? task->speed : 0;
469 + backup_state.speed = (has_speed && speed > 0) ? speed : 0;
471 backup_state.vmaw = vmaw;
472 backup_state.pbs = pbs;
473 @@ -910,8 +879,6 @@ static void coroutine_fn pvebackup_co_prepare(void *opaque)
474 uuid_info = g_malloc0(sizeof(*uuid_info));
475 uuid_info->UUID = uuid_str;
477 - task->result = uuid_info;
479 /* Run create_backup_jobs_bh outside of coroutine (in BH) but keep
480 * backup_mutex locked. This is fine, a CoMutex can be held across yield
481 * points, and we'll release it as soon as the BH reschedules us.
482 @@ -925,7 +892,7 @@ static void coroutine_fn pvebackup_co_prepare(void *opaque)
483 qemu_coroutine_yield();
486 - error_propagate(task->errp, local_err);
487 + error_propagate(errp, local_err);
491 @@ -938,7 +905,7 @@ static void coroutine_fn pvebackup_co_prepare(void *opaque)
492 /* start the first job in the transaction */
493 job_txn_start_seq(backup_state.txn);
499 qemu_mutex_unlock(&backup_state.stat.lock);
500 @@ -969,7 +936,7 @@ err:
503 vma_writer_close(vmaw, &err);
504 - unlink(task->backup_file);
505 + unlink(backup_file);
509 @@ -980,65 +947,8 @@ err:
513 - task->result = NULL;
515 qemu_co_mutex_unlock(&backup_state.backup_mutex);
519 -UuidInfo *qmp_backup(
520 - const char *backup_file,
521 - bool has_password, const char *password,
522 - bool has_keyfile, const char *keyfile,
523 - bool has_key_password, const char *key_password,
524 - bool has_fingerprint, const char *fingerprint,
525 - bool has_backup_id, const char *backup_id,
526 - bool has_backup_time, int64_t backup_time,
527 - bool has_use_dirty_bitmap, bool use_dirty_bitmap,
528 - bool has_compress, bool compress,
529 - bool has_encrypt, bool encrypt,
530 - bool has_format, BackupFormat format,
531 - bool has_config_file, const char *config_file,
532 - bool has_firewall_file, const char *firewall_file,
533 - bool has_devlist, const char *devlist,
534 - bool has_speed, int64_t speed, Error **errp)
536 - QmpBackupTask task = {
537 - .backup_file = backup_file,
538 - .has_password = has_password,
539 - .password = password,
540 - .has_keyfile = has_keyfile,
541 - .keyfile = keyfile,
542 - .has_key_password = has_key_password,
543 - .key_password = key_password,
544 - .has_fingerprint = has_fingerprint,
545 - .fingerprint = fingerprint,
546 - .has_backup_id = has_backup_id,
547 - .backup_id = backup_id,
548 - .has_backup_time = has_backup_time,
549 - .backup_time = backup_time,
550 - .has_use_dirty_bitmap = has_use_dirty_bitmap,
551 - .use_dirty_bitmap = use_dirty_bitmap,
552 - .has_compress = has_compress,
553 - .compress = compress,
554 - .has_encrypt = has_encrypt,
555 - .encrypt = encrypt,
556 - .has_format = has_format,
558 - .has_config_file = has_config_file,
559 - .config_file = config_file,
560 - .has_firewall_file = has_firewall_file,
561 - .firewall_file = firewall_file,
562 - .has_devlist = has_devlist,
563 - .devlist = devlist,
564 - .has_speed = has_speed,
569 - block_on_coroutine_fn(pvebackup_co_prepare, &task);
571 - return task.result;
575 BackupStatus *qmp_query_backup(Error **errp)
576 diff --git a/qapi/block-core.json b/qapi/block-core.json
577 index e5de769dc1..afa67c28d2 100644
578 --- a/qapi/block-core.json
579 +++ b/qapi/block-core.json
581 '*config-file': 'str',
582 '*firewall-file': 'str',
583 '*devlist': 'str', '*speed': 'int' },
584 - 'returns': 'UuidInfo' }
585 + 'returns': 'UuidInfo', 'coroutine': true }
590 # Notes: This command succeeds even if there is no backup process running.
593 -{ 'command': 'backup-cancel' }
594 +{ 'command': 'backup-cancel', 'coroutine': true }
597 # @ProxmoxSupportStatus: