From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id 46AAB6B558 for ; Tue, 26 Jan 2021 17:11:02 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 4454619E5A for ; Tue, 26 Jan 2021 17:11:02 +0100 (CET) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [212.186.127.180]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS id 1286519E4F for ; Tue, 26 Jan 2021 17:11:00 +0100 (CET) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id A6D5A409D5 for ; Tue, 26 Jan 2021 17:10:59 +0100 (CET) From: Stefan Reiter To: pve-devel@lists.proxmox.com Date: Tue, 26 Jan 2021 17:10:27 +0100 Message-Id: <20210126161027.26106-2-s.reiter@proxmox.com> X-Mailer: git-send-email 2.20.1 In-Reply-To: <20210126161027.26106-1-s.reiter@proxmox.com> References: <20210126161027.26106-1-s.reiter@proxmox.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-SPAM-LEVEL: Spam detection results: 0 AWL -0.044 Adjusted score from AWL reputation of From: address KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment RCVD_IN_DNSWL_MED -2.3 Sender listed at https://www.dnswl.org/, medium trust SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record T_FILL_THIS_FORM_SHORT 0.01 Fill in a short form with personal information Subject: [pve-devel] [PATCH qemu 2/2] PVE: Use coroutine QMP for backup/cancel_backup X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 26 Jan 2021 16:11:02 -0000 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 --- This patch is already included in the first "patch of patches", but is entirely new and not just a rebase/meson update, so I've included it for seperate review here as well. Ignore when applying. 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