From 751ed3661b4b4b262e432dcaefa8b6d87ba78f22 Mon Sep 17 00:00:00 2001 From: Thomas Lamprecht Date: Fri, 17 Apr 2020 08:17:59 +0200 Subject: [PATCH] avoid using QemuRecMutex inside coroutines, QemuMutex to lock outside Signed-off-by: Thomas Lamprecht --- ...d-use-QemuRecMutex-inside-coroutines.patch | 211 +++++++++ .../0041-PVE-Use-non-recursive-locks.patch | 403 ------------------ ...se-QemuMutex-instead-of-QemuRecMutex.patch | 227 ++++++++++ debian/patches/series | 3 +- 4 files changed, 440 insertions(+), 404 deletions(-) create mode 100644 debian/patches/pve/0041-PVE-Backup-avoid-use-QemuRecMutex-inside-coroutines.patch delete mode 100644 debian/patches/pve/0041-PVE-Use-non-recursive-locks.patch create mode 100644 debian/patches/pve/0042-PVE-Backup-use-QemuMutex-instead-of-QemuRecMutex.patch diff --git a/debian/patches/pve/0041-PVE-Backup-avoid-use-QemuRecMutex-inside-coroutines.patch b/debian/patches/pve/0041-PVE-Backup-avoid-use-QemuRecMutex-inside-coroutines.patch new file mode 100644 index 0000000..00439d9 --- /dev/null +++ b/debian/patches/pve/0041-PVE-Backup-avoid-use-QemuRecMutex-inside-coroutines.patch @@ -0,0 +1,211 @@ +From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001 +From: Dietmar Maurer +Date: Fri, 17 Apr 2020 08:57:47 +0200 +Subject: [PATCH] PVE Backup: avoid use QemuRecMutex inside coroutines + +--- + pve-backup.c | 59 +++++++++++++++++++++++++++++++++------------------- + 1 file changed, 38 insertions(+), 21 deletions(-) + +diff --git a/pve-backup.c b/pve-backup.c +index 169f0c68d0..dddf430399 100644 +--- a/pve-backup.c ++++ b/pve-backup.c +@@ -11,6 +11,23 @@ + + /* PVE backup state and related function */ + ++/* ++ * Note: A resume from a qemu_coroutine_yield can happen in a different thread, ++ * so you may not use normal mutexes within coroutines: ++ * ++ * ---bad-example--- ++ * qemu_rec_mutex_lock(lock) ++ * ... ++ * qemu_coroutine_yield() // wait for something ++ * // we are now inside a different thread ++ * qemu_rec_mutex_unlock(lock) // Crash - wrong thread!! ++ * ---end-bad-example-- ++ * ++ * ==> Always use CoMutext inside coroutines. ++ * ==> Never acquire/release AioContext withing coroutines (because that use QemuRecMutex) ++ * ++ */ ++ + static struct PVEBackupState { + struct { + // Everithing accessed from qmp_backup_query command is protected using lock +@@ -30,12 +47,14 @@ static struct PVEBackupState { + ProxmoxBackupHandle *pbs; + GList *di_list; + QemuRecMutex backup_mutex; ++ CoMutex dump_callback_mutex; + } backup_state; + + static void pvebackup_init(void) + { + qemu_rec_mutex_init(&backup_state.stat.lock); + qemu_rec_mutex_init(&backup_state.backup_mutex); ++ qemu_co_mutex_init(&backup_state.dump_callback_mutex); + } + + // initialize PVEBackupState at startup +@@ -114,16 +133,16 @@ pvebackup_co_dump_pbs_cb( + Error *local_err = NULL; + int pbs_res = -1; + +- qemu_rec_mutex_lock(&backup_state.backup_mutex); ++ qemu_co_mutex_lock(&backup_state.dump_callback_mutex); + + // avoid deadlock if job is cancelled + if (pvebackup_error_or_canceled()) { +- qemu_rec_mutex_unlock(&backup_state.backup_mutex); ++ qemu_co_mutex_unlock(&backup_state.dump_callback_mutex); + return -1; + } + + pbs_res = proxmox_backup_co_write_data(backup_state.pbs, di->dev_id, buf, start, size, &local_err); +- qemu_rec_mutex_unlock(&backup_state.backup_mutex); ++ qemu_co_mutex_unlock(&backup_state.dump_callback_mutex); + + if (pbs_res < 0) { + pvebackup_propagate_error(local_err); +@@ -149,7 +168,6 @@ pvebackup_co_dump_vma_cb( + const unsigned char *buf = pbuf; + PVEBackupDevInfo *di = opaque; + +- + int ret = -1; + + assert(backup_state.vmaw); +@@ -167,16 +185,16 @@ pvebackup_co_dump_vma_cb( + } + + while (remaining > 0) { +- qemu_rec_mutex_lock(&backup_state.backup_mutex); ++ qemu_co_mutex_lock(&backup_state.dump_callback_mutex); + // avoid deadlock if job is cancelled + if (pvebackup_error_or_canceled()) { +- qemu_rec_mutex_unlock(&backup_state.backup_mutex); ++ qemu_co_mutex_unlock(&backup_state.dump_callback_mutex); + return -1; + } + + size_t zero_bytes = 0; + ret = vma_writer_write(backup_state.vmaw, di->dev_id, cluster_num, buf, &zero_bytes); +- qemu_rec_mutex_unlock(&backup_state.backup_mutex); ++ qemu_co_mutex_unlock(&backup_state.dump_callback_mutex); + + ++cluster_num; + if (buf) { +@@ -203,12 +221,11 @@ pvebackup_co_dump_vma_cb( + return size; + } + ++// assumes the caller holds backup_mutex + static void coroutine_fn pvebackup_co_cleanup(void *unused) + { + assert(qemu_in_coroutine()); + +- qemu_rec_mutex_lock(&backup_state.backup_mutex); +- + qemu_rec_mutex_lock(&backup_state.stat.lock); + backup_state.stat.end_time = time(NULL); + qemu_rec_mutex_unlock(&backup_state.stat.lock); +@@ -239,9 +256,9 @@ static void coroutine_fn pvebackup_co_cleanup(void *unused) + + g_list_free(backup_state.di_list); + backup_state.di_list = NULL; +- qemu_rec_mutex_unlock(&backup_state.backup_mutex); + } + ++// assumes the caller holds backup_mutex + static void coroutine_fn pvebackup_complete_stream(void *opaque) + { + PVEBackupDevInfo *di = opaque; +@@ -295,6 +312,8 @@ static void pvebackup_complete_cb(void *opaque, int ret) + + static void pvebackup_cancel(void) + { ++ assert(!qemu_in_coroutine()); ++ + Error *cancel_err = NULL; + error_setg(&cancel_err, "backup canceled"); + pvebackup_propagate_error(cancel_err); +@@ -348,6 +367,7 @@ void qmp_backup_cancel(Error **errp) + pvebackup_cancel(); + } + ++// assumes the caller holds backup_mutex + static int coroutine_fn pvebackup_co_add_config( + const char *file, + const char *name, +@@ -431,9 +451,9 @@ static void pvebackup_run_next_job(void) + } + } + +- qemu_rec_mutex_unlock(&backup_state.backup_mutex); +- + block_on_coroutine_fn(pvebackup_co_cleanup, NULL); // no more jobs, run cleanup ++ ++ qemu_rec_mutex_unlock(&backup_state.backup_mutex); + } + + static bool create_backup_jobs(void) { +@@ -520,6 +540,7 @@ typedef struct QmpBackupTask { + UuidInfo *result; + } QmpBackupTask; + ++// assumes the caller holds backup_mutex + static void coroutine_fn pvebackup_co_prepare(void *opaque) + { + assert(qemu_in_coroutine()); +@@ -543,11 +564,8 @@ static void coroutine_fn pvebackup_co_prepare(void *opaque) + const char *config_name = "qemu-server.conf"; + const char *firewall_name = "qemu-server.fw"; + +- qemu_rec_mutex_lock(&backup_state.backup_mutex); +- + if (backup_state.di_list) { +- qemu_rec_mutex_unlock(&backup_state.backup_mutex); +- error_set(task->errp, ERROR_CLASS_GENERIC_ERROR, ++ error_set(task->errp, ERROR_CLASS_GENERIC_ERROR, + "previous backup not finished"); + return; + } +@@ -792,8 +810,6 @@ static void coroutine_fn pvebackup_co_prepare(void *opaque) + + backup_state.di_list = di_list; + +- qemu_rec_mutex_unlock(&backup_state.backup_mutex); +- + uuid_info = g_malloc0(sizeof(*uuid_info)); + uuid_info->UUID = uuid_str; + +@@ -836,8 +852,6 @@ err: + rmdir(backup_dir); + } + +- qemu_rec_mutex_unlock(&backup_state.backup_mutex); +- + task->result = NULL; + return; + } +@@ -881,13 +895,16 @@ UuidInfo *qmp_backup( + .errp = errp, + }; + ++ qemu_rec_mutex_lock(&backup_state.backup_mutex); ++ + block_on_coroutine_fn(pvebackup_co_prepare, &task); + + if (*errp == NULL) { +- qemu_rec_mutex_lock(&backup_state.backup_mutex); + create_backup_jobs(); + qemu_rec_mutex_unlock(&backup_state.backup_mutex); + pvebackup_run_next_job(); ++ } else { ++ qemu_rec_mutex_unlock(&backup_state.backup_mutex); + } + + return task.result; diff --git a/debian/patches/pve/0041-PVE-Use-non-recursive-locks.patch b/debian/patches/pve/0041-PVE-Use-non-recursive-locks.patch deleted file mode 100644 index 774f4d4..0000000 --- a/debian/patches/pve/0041-PVE-Use-non-recursive-locks.patch +++ /dev/null @@ -1,403 +0,0 @@ -From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001 -From: Stefan Reiter -Date: Thu, 16 Apr 2020 11:40:16 +0200 -Subject: [PATCH] PVE: Use non-recursive locks - -Release the lock on qemu_coroutine_yield, so coroutines don't deadlock. - -Signed-off-by: Stefan Reiter ---- - pve-backup.c | 82 +++++++++++++++++++++++++++------------------------- - vma-writer.c | 28 ++++++++++++++---- - vma.h | 1 + - 3 files changed, 66 insertions(+), 45 deletions(-) - -diff --git a/pve-backup.c b/pve-backup.c -index 169f0c68d0..46a3d6f995 100644 ---- a/pve-backup.c -+++ b/pve-backup.c -@@ -14,7 +14,7 @@ - static struct PVEBackupState { - struct { - // Everithing accessed from qmp_backup_query command is protected using lock -- QemuRecMutex lock; -+ QemuMutex lock; - Error *error; - time_t start_time; - time_t end_time; -@@ -29,13 +29,13 @@ static struct PVEBackupState { - VmaWriter *vmaw; - ProxmoxBackupHandle *pbs; - GList *di_list; -- QemuRecMutex backup_mutex; -+ QemuMutex backup_mutex; - } backup_state; - - static void pvebackup_init(void) - { -- qemu_rec_mutex_init(&backup_state.stat.lock); -- qemu_rec_mutex_init(&backup_state.backup_mutex); -+ qemu_mutex_init(&backup_state.stat.lock); -+ qemu_mutex_init(&backup_state.backup_mutex); - } - - // initialize PVEBackupState at startup -@@ -72,26 +72,26 @@ lookup_active_block_job(PVEBackupDevInfo *di) - - static void pvebackup_propagate_error(Error *err) - { -- qemu_rec_mutex_lock(&backup_state.stat.lock); -+ qemu_mutex_lock(&backup_state.stat.lock); - error_propagate(&backup_state.stat.error, err); -- qemu_rec_mutex_unlock(&backup_state.stat.lock); -+ qemu_mutex_unlock(&backup_state.stat.lock); - } - - static bool pvebackup_error_or_canceled(void) - { -- qemu_rec_mutex_lock(&backup_state.stat.lock); -+ qemu_mutex_lock(&backup_state.stat.lock); - bool error_or_canceled = !!backup_state.stat.error; -- qemu_rec_mutex_unlock(&backup_state.stat.lock); -+ qemu_mutex_unlock(&backup_state.stat.lock); - - return error_or_canceled; - } - - static void pvebackup_add_transfered_bytes(size_t transferred, size_t zero_bytes) - { -- qemu_rec_mutex_lock(&backup_state.stat.lock); -+ qemu_mutex_lock(&backup_state.stat.lock); - backup_state.stat.zero_bytes += zero_bytes; - backup_state.stat.transferred += transferred; -- qemu_rec_mutex_unlock(&backup_state.stat.lock); -+ qemu_mutex_unlock(&backup_state.stat.lock); - } - - // This may get called from multiple coroutines in multiple io-threads -@@ -114,16 +114,16 @@ pvebackup_co_dump_pbs_cb( - Error *local_err = NULL; - int pbs_res = -1; - -- qemu_rec_mutex_lock(&backup_state.backup_mutex); -+ qemu_mutex_lock(&backup_state.backup_mutex); - - // avoid deadlock if job is cancelled - if (pvebackup_error_or_canceled()) { -- qemu_rec_mutex_unlock(&backup_state.backup_mutex); -+ qemu_mutex_unlock(&backup_state.backup_mutex); - return -1; - } - - pbs_res = proxmox_backup_co_write_data(backup_state.pbs, di->dev_id, buf, start, size, &local_err); -- qemu_rec_mutex_unlock(&backup_state.backup_mutex); -+ qemu_mutex_unlock(&backup_state.backup_mutex); - - if (pbs_res < 0) { - pvebackup_propagate_error(local_err); -@@ -167,16 +167,16 @@ pvebackup_co_dump_vma_cb( - } - - while (remaining > 0) { -- qemu_rec_mutex_lock(&backup_state.backup_mutex); -+ qemu_mutex_lock(&backup_state.backup_mutex); - // avoid deadlock if job is cancelled - if (pvebackup_error_or_canceled()) { -- qemu_rec_mutex_unlock(&backup_state.backup_mutex); -+ qemu_mutex_unlock(&backup_state.backup_mutex); - return -1; - } - - size_t zero_bytes = 0; - ret = vma_writer_write(backup_state.vmaw, di->dev_id, cluster_num, buf, &zero_bytes); -- qemu_rec_mutex_unlock(&backup_state.backup_mutex); -+ qemu_mutex_unlock(&backup_state.backup_mutex); - - ++cluster_num; - if (buf) { -@@ -207,11 +207,11 @@ static void coroutine_fn pvebackup_co_cleanup(void *unused) - { - assert(qemu_in_coroutine()); - -- qemu_rec_mutex_lock(&backup_state.backup_mutex); -+ qemu_mutex_lock(&backup_state.backup_mutex); - -- qemu_rec_mutex_lock(&backup_state.stat.lock); -+ qemu_mutex_lock(&backup_state.stat.lock); - backup_state.stat.end_time = time(NULL); -- qemu_rec_mutex_unlock(&backup_state.stat.lock); -+ qemu_mutex_unlock(&backup_state.stat.lock); - - if (backup_state.vmaw) { - Error *local_err = NULL; -@@ -239,7 +239,7 @@ static void coroutine_fn pvebackup_co_cleanup(void *unused) - - g_list_free(backup_state.di_list); - backup_state.di_list = NULL; -- qemu_rec_mutex_unlock(&backup_state.backup_mutex); -+ qemu_mutex_unlock(&backup_state.backup_mutex); - } - - static void coroutine_fn pvebackup_complete_stream(void *opaque) -@@ -267,7 +267,7 @@ static void pvebackup_complete_cb(void *opaque, int ret) - - PVEBackupDevInfo *di = opaque; - -- qemu_rec_mutex_lock(&backup_state.backup_mutex); -+ qemu_mutex_lock(&backup_state.backup_mutex); - - di->completed = true; - -@@ -288,7 +288,7 @@ static void pvebackup_complete_cb(void *opaque, int ret) - - g_free(di); - -- qemu_rec_mutex_unlock(&backup_state.backup_mutex); -+ qemu_mutex_unlock(&backup_state.backup_mutex); - - pvebackup_run_next_job(); - } -@@ -299,7 +299,7 @@ static void pvebackup_cancel(void) - error_setg(&cancel_err, "backup canceled"); - pvebackup_propagate_error(cancel_err); - -- qemu_rec_mutex_lock(&backup_state.backup_mutex); -+ qemu_mutex_lock(&backup_state.backup_mutex); - - if (backup_state.vmaw) { - /* make sure vma writer does not block anymore */ -@@ -310,13 +310,13 @@ static void pvebackup_cancel(void) - proxmox_backup_abort(backup_state.pbs, "backup canceled"); - } - -- qemu_rec_mutex_unlock(&backup_state.backup_mutex); -+ qemu_mutex_unlock(&backup_state.backup_mutex); - - for(;;) { - - BlockJob *next_job = NULL; - -- qemu_rec_mutex_lock(&backup_state.backup_mutex); -+ qemu_mutex_lock(&backup_state.backup_mutex); - - GList *l = backup_state.di_list; - while (l) { -@@ -330,7 +330,7 @@ static void pvebackup_cancel(void) - } - } - -- qemu_rec_mutex_unlock(&backup_state.backup_mutex); -+ qemu_mutex_unlock(&backup_state.backup_mutex); - - if (next_job) { - AioContext *aio_context = next_job->job.aio_context; -@@ -403,7 +403,7 @@ static void pvebackup_run_next_job(void) - { - assert(!qemu_in_coroutine()); - -- qemu_rec_mutex_lock(&backup_state.backup_mutex); -+ qemu_mutex_lock(&backup_state.backup_mutex); - - GList *l = backup_state.di_list; - while (l) { -@@ -413,7 +413,7 @@ static void pvebackup_run_next_job(void) - BlockJob *job = lookup_active_block_job(di); - - if (job) { -- qemu_rec_mutex_unlock(&backup_state.backup_mutex); -+ qemu_mutex_unlock(&backup_state.backup_mutex); - - AioContext *aio_context = job->job.aio_context; - aio_context_acquire(aio_context); -@@ -431,7 +431,7 @@ static void pvebackup_run_next_job(void) - } - } - -- qemu_rec_mutex_unlock(&backup_state.backup_mutex); -+ qemu_mutex_unlock(&backup_state.backup_mutex); - - block_on_coroutine_fn(pvebackup_co_cleanup, NULL); // no more jobs, run cleanup - } -@@ -543,10 +543,10 @@ static void coroutine_fn pvebackup_co_prepare(void *opaque) - const char *config_name = "qemu-server.conf"; - const char *firewall_name = "qemu-server.fw"; - -- qemu_rec_mutex_lock(&backup_state.backup_mutex); -+ qemu_mutex_lock(&backup_state.backup_mutex); - - if (backup_state.di_list) { -- qemu_rec_mutex_unlock(&backup_state.backup_mutex); -+ qemu_mutex_unlock(&backup_state.backup_mutex); - error_set(task->errp, ERROR_CLASS_GENERIC_ERROR, - "previous backup not finished"); - return; -@@ -689,6 +689,8 @@ static void coroutine_fn pvebackup_co_prepare(void *opaque) - goto err; - } - -+ vma_writer_set_external_lock(vmaw, &backup_state.backup_mutex); -+ - /* register all devices for vma writer */ - l = di_list; - while (l) { -@@ -760,7 +762,7 @@ static void coroutine_fn pvebackup_co_prepare(void *opaque) - } - /* initialize global backup_state now */ - -- qemu_rec_mutex_lock(&backup_state.stat.lock); -+ qemu_mutex_lock(&backup_state.stat.lock); - - if (backup_state.stat.error) { - error_free(backup_state.stat.error); -@@ -783,7 +785,7 @@ static void coroutine_fn pvebackup_co_prepare(void *opaque) - backup_state.stat.transferred = 0; - backup_state.stat.zero_bytes = 0; - -- qemu_rec_mutex_unlock(&backup_state.stat.lock); -+ qemu_mutex_unlock(&backup_state.stat.lock); - - backup_state.speed = (task->has_speed && task->speed > 0) ? task->speed : 0; - -@@ -792,7 +794,7 @@ static void coroutine_fn pvebackup_co_prepare(void *opaque) - - backup_state.di_list = di_list; - -- qemu_rec_mutex_unlock(&backup_state.backup_mutex); -+ qemu_mutex_unlock(&backup_state.backup_mutex); - - uuid_info = g_malloc0(sizeof(*uuid_info)); - uuid_info->UUID = uuid_str; -@@ -836,7 +838,7 @@ err: - rmdir(backup_dir); - } - -- qemu_rec_mutex_unlock(&backup_state.backup_mutex); -+ qemu_mutex_unlock(&backup_state.backup_mutex); - - task->result = NULL; - return; -@@ -884,9 +886,9 @@ UuidInfo *qmp_backup( - block_on_coroutine_fn(pvebackup_co_prepare, &task); - - if (*errp == NULL) { -- qemu_rec_mutex_lock(&backup_state.backup_mutex); -+ qemu_mutex_lock(&backup_state.backup_mutex); - create_backup_jobs(); -- qemu_rec_mutex_unlock(&backup_state.backup_mutex); -+ qemu_mutex_unlock(&backup_state.backup_mutex); - pvebackup_run_next_job(); - } - -@@ -897,11 +899,11 @@ BackupStatus *qmp_query_backup(Error **errp) - { - BackupStatus *info = g_malloc0(sizeof(*info)); - -- qemu_rec_mutex_lock(&backup_state.stat.lock); -+ qemu_mutex_lock(&backup_state.stat.lock); - - if (!backup_state.stat.start_time) { - /* not started, return {} */ -- qemu_rec_mutex_unlock(&backup_state.stat.lock); -+ qemu_mutex_unlock(&backup_state.stat.lock); - return info; - } - -@@ -938,7 +940,7 @@ BackupStatus *qmp_query_backup(Error **errp) - info->has_transferred = true; - info->transferred = backup_state.stat.transferred; - -- qemu_rec_mutex_unlock(&backup_state.stat.lock); -+ qemu_mutex_unlock(&backup_state.stat.lock); - - return info; - } -diff --git a/vma-writer.c b/vma-writer.c -index fe86b18a60..f3fa8e3b4c 100644 ---- a/vma-writer.c -+++ b/vma-writer.c -@@ -68,8 +68,14 @@ struct VmaWriter { - uint32_t config_names[VMA_MAX_CONFIGS]; /* offset into blob_buffer table */ - uint32_t config_data[VMA_MAX_CONFIGS]; /* offset into blob_buffer table */ - uint32_t config_count; -+ -+ QemuMutex *external_lock; - }; - -+void vma_writer_set_external_lock(VmaWriter *vmaw, QemuMutex *lock) { -+ vmaw->external_lock = lock; -+} -+ - void vma_writer_set_error(VmaWriter *vmaw, const char *fmt, ...) - { - va_list ap; -@@ -199,13 +205,20 @@ int vma_writer_register_stream(VmaWriter *vmaw, const char *devname, - return n; - } - --static void coroutine_fn yield_until_fd_writable(int fd) -+static void coroutine_fn yield_until_fd_writable(int fd, QemuMutex *external_lock) - { - assert(qemu_in_coroutine()); - AioContext *ctx = qemu_get_current_aio_context(); - aio_set_fd_handler(ctx, fd, false, NULL, (IOHandler *)qemu_coroutine_enter, - NULL, qemu_coroutine_self()); -+ if (external_lock) { -+ /* still protected from re-entering via flush_lock */ -+ qemu_mutex_unlock(external_lock); -+ } - qemu_coroutine_yield(); -+ if (!external_lock) { -+ qemu_mutex_lock(external_lock); -+ } - aio_set_fd_handler(ctx, fd, false, NULL, NULL, NULL, NULL); - } - -@@ -227,11 +240,16 @@ vma_queue_write(VmaWriter *vmaw, const void *buf, size_t bytes) - - while (done < bytes) { - if (vmaw->status < 0) { -- DPRINTF("vma_queue_write detected canceled backup\n"); -+ DPRINTF("vma_queue_write detected cancelled backup\n"); -+ done = -1; -+ break; -+ } -+ yield_until_fd_writable(vmaw->fd, vmaw->external_lock); -+ if (vmaw->closed || vmaw->status < 0) { -+ DPRINTF("vma_queue_write backup cancelled after waiting for fd\n"); - done = -1; - break; - } -- yield_until_fd_writable(vmaw->fd); - ret = write(vmaw->fd, buf + done, bytes - done); - if (ret > 0) { - done += ret; -@@ -501,7 +519,7 @@ vma_writer_flush_output(VmaWriter *vmaw) - int ret = vma_writer_flush(vmaw); - qemu_co_mutex_unlock(&vmaw->flush_lock); - if (ret < 0) { -- vma_writer_set_error(vmaw, "vma_writer_flush_header failed"); -+ vma_writer_set_error(vmaw, "vma_writer_flush_output failed"); - } - return ret; - } -@@ -570,7 +588,7 @@ static int vma_writer_get_buffer(VmaWriter *vmaw) - while (vmaw->outbuf_count >= (VMA_BLOCKS_PER_EXTENT - 1)) { - ret = vma_writer_flush(vmaw); - if (ret < 0) { -- vma_writer_set_error(vmaw, "vma_writer_get_buffer: flush failed"); -+ vma_writer_set_error(vmaw, "vma_writer_get_header: flush failed"); - break; - } - } -diff --git a/vma.h b/vma.h -index c895c97f6d..ec6f09e83e 100644 ---- a/vma.h -+++ b/vma.h -@@ -115,6 +115,7 @@ typedef struct VmaDeviceInfo { - } VmaDeviceInfo; - - VmaWriter *vma_writer_create(const char *filename, uuid_t uuid, Error **errp); -+void vma_writer_set_external_lock(VmaWriter *vmaw, QemuMutex *lock); - int vma_writer_close(VmaWriter *vmaw, Error **errp); - void vma_writer_error_propagate(VmaWriter *vmaw, Error **errp); - void vma_writer_destroy(VmaWriter *vmaw); diff --git a/debian/patches/pve/0042-PVE-Backup-use-QemuMutex-instead-of-QemuRecMutex.patch b/debian/patches/pve/0042-PVE-Backup-use-QemuMutex-instead-of-QemuRecMutex.patch new file mode 100644 index 0000000..b18762d --- /dev/null +++ b/debian/patches/pve/0042-PVE-Backup-use-QemuMutex-instead-of-QemuRecMutex.patch @@ -0,0 +1,227 @@ +From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001 +From: Dietmar Maurer +Date: Fri, 17 Apr 2020 08:57:48 +0200 +Subject: [PATCH] PVE Backup: use QemuMutex instead of QemuRecMutex + +We acquire/release all mutexes outside coroutines now, so we can now +correctly use a normal mutex. +--- + pve-backup.c | 58 ++++++++++++++++++++++++++-------------------------- + 1 file changed, 29 insertions(+), 29 deletions(-) + +diff --git a/pve-backup.c b/pve-backup.c +index dddf430399..bb917ee972 100644 +--- a/pve-backup.c ++++ b/pve-backup.c +@@ -31,7 +31,7 @@ + static struct PVEBackupState { + struct { + // Everithing accessed from qmp_backup_query command is protected using lock +- QemuRecMutex lock; ++ QemuMutex lock; + Error *error; + time_t start_time; + time_t end_time; +@@ -46,14 +46,14 @@ static struct PVEBackupState { + VmaWriter *vmaw; + ProxmoxBackupHandle *pbs; + GList *di_list; +- QemuRecMutex backup_mutex; ++ QemuMutex backup_mutex; + CoMutex dump_callback_mutex; + } backup_state; + + static void pvebackup_init(void) + { +- qemu_rec_mutex_init(&backup_state.stat.lock); +- qemu_rec_mutex_init(&backup_state.backup_mutex); ++ qemu_mutex_init(&backup_state.stat.lock); ++ qemu_mutex_init(&backup_state.backup_mutex); + qemu_co_mutex_init(&backup_state.dump_callback_mutex); + } + +@@ -91,26 +91,26 @@ lookup_active_block_job(PVEBackupDevInfo *di) + + static void pvebackup_propagate_error(Error *err) + { +- qemu_rec_mutex_lock(&backup_state.stat.lock); ++ qemu_mutex_lock(&backup_state.stat.lock); + error_propagate(&backup_state.stat.error, err); +- qemu_rec_mutex_unlock(&backup_state.stat.lock); ++ qemu_mutex_unlock(&backup_state.stat.lock); + } + + static bool pvebackup_error_or_canceled(void) + { +- qemu_rec_mutex_lock(&backup_state.stat.lock); ++ qemu_mutex_lock(&backup_state.stat.lock); + bool error_or_canceled = !!backup_state.stat.error; +- qemu_rec_mutex_unlock(&backup_state.stat.lock); ++ qemu_mutex_unlock(&backup_state.stat.lock); + + return error_or_canceled; + } + + static void pvebackup_add_transfered_bytes(size_t transferred, size_t zero_bytes) + { +- qemu_rec_mutex_lock(&backup_state.stat.lock); ++ qemu_mutex_lock(&backup_state.stat.lock); + backup_state.stat.zero_bytes += zero_bytes; + backup_state.stat.transferred += transferred; +- qemu_rec_mutex_unlock(&backup_state.stat.lock); ++ qemu_mutex_unlock(&backup_state.stat.lock); + } + + // This may get called from multiple coroutines in multiple io-threads +@@ -226,9 +226,9 @@ static void coroutine_fn pvebackup_co_cleanup(void *unused) + { + assert(qemu_in_coroutine()); + +- qemu_rec_mutex_lock(&backup_state.stat.lock); ++ qemu_mutex_lock(&backup_state.stat.lock); + backup_state.stat.end_time = time(NULL); +- qemu_rec_mutex_unlock(&backup_state.stat.lock); ++ qemu_mutex_unlock(&backup_state.stat.lock); + + if (backup_state.vmaw) { + Error *local_err = NULL; +@@ -284,7 +284,7 @@ static void pvebackup_complete_cb(void *opaque, int ret) + + PVEBackupDevInfo *di = opaque; + +- qemu_rec_mutex_lock(&backup_state.backup_mutex); ++ qemu_mutex_lock(&backup_state.backup_mutex); + + di->completed = true; + +@@ -305,7 +305,7 @@ static void pvebackup_complete_cb(void *opaque, int ret) + + g_free(di); + +- qemu_rec_mutex_unlock(&backup_state.backup_mutex); ++ qemu_mutex_unlock(&backup_state.backup_mutex); + + pvebackup_run_next_job(); + } +@@ -318,7 +318,7 @@ static void pvebackup_cancel(void) + error_setg(&cancel_err, "backup canceled"); + pvebackup_propagate_error(cancel_err); + +- qemu_rec_mutex_lock(&backup_state.backup_mutex); ++ qemu_mutex_lock(&backup_state.backup_mutex); + + if (backup_state.vmaw) { + /* make sure vma writer does not block anymore */ +@@ -329,13 +329,13 @@ static void pvebackup_cancel(void) + proxmox_backup_abort(backup_state.pbs, "backup canceled"); + } + +- qemu_rec_mutex_unlock(&backup_state.backup_mutex); ++ qemu_mutex_unlock(&backup_state.backup_mutex); + + for(;;) { + + BlockJob *next_job = NULL; + +- qemu_rec_mutex_lock(&backup_state.backup_mutex); ++ qemu_mutex_lock(&backup_state.backup_mutex); + + GList *l = backup_state.di_list; + while (l) { +@@ -349,7 +349,7 @@ static void pvebackup_cancel(void) + } + } + +- qemu_rec_mutex_unlock(&backup_state.backup_mutex); ++ qemu_mutex_unlock(&backup_state.backup_mutex); + + if (next_job) { + AioContext *aio_context = next_job->job.aio_context; +@@ -423,7 +423,7 @@ static void pvebackup_run_next_job(void) + { + assert(!qemu_in_coroutine()); + +- qemu_rec_mutex_lock(&backup_state.backup_mutex); ++ qemu_mutex_lock(&backup_state.backup_mutex); + + GList *l = backup_state.di_list; + while (l) { +@@ -433,7 +433,7 @@ static void pvebackup_run_next_job(void) + BlockJob *job = lookup_active_block_job(di); + + if (job) { +- qemu_rec_mutex_unlock(&backup_state.backup_mutex); ++ qemu_mutex_unlock(&backup_state.backup_mutex); + + AioContext *aio_context = job->job.aio_context; + aio_context_acquire(aio_context); +@@ -453,7 +453,7 @@ static void pvebackup_run_next_job(void) + + block_on_coroutine_fn(pvebackup_co_cleanup, NULL); // no more jobs, run cleanup + +- qemu_rec_mutex_unlock(&backup_state.backup_mutex); ++ qemu_mutex_unlock(&backup_state.backup_mutex); + } + + static bool create_backup_jobs(void) { +@@ -778,7 +778,7 @@ static void coroutine_fn pvebackup_co_prepare(void *opaque) + } + /* initialize global backup_state now */ + +- qemu_rec_mutex_lock(&backup_state.stat.lock); ++ qemu_mutex_lock(&backup_state.stat.lock); + + if (backup_state.stat.error) { + error_free(backup_state.stat.error); +@@ -801,7 +801,7 @@ static void coroutine_fn pvebackup_co_prepare(void *opaque) + backup_state.stat.transferred = 0; + backup_state.stat.zero_bytes = 0; + +- qemu_rec_mutex_unlock(&backup_state.stat.lock); ++ qemu_mutex_unlock(&backup_state.stat.lock); + + backup_state.speed = (task->has_speed && task->speed > 0) ? task->speed : 0; + +@@ -895,16 +895,16 @@ UuidInfo *qmp_backup( + .errp = errp, + }; + +- qemu_rec_mutex_lock(&backup_state.backup_mutex); ++ qemu_mutex_lock(&backup_state.backup_mutex); + + block_on_coroutine_fn(pvebackup_co_prepare, &task); + + if (*errp == NULL) { + create_backup_jobs(); +- qemu_rec_mutex_unlock(&backup_state.backup_mutex); ++ qemu_mutex_unlock(&backup_state.backup_mutex); + pvebackup_run_next_job(); + } else { +- qemu_rec_mutex_unlock(&backup_state.backup_mutex); ++ qemu_mutex_unlock(&backup_state.backup_mutex); + } + + return task.result; +@@ -914,11 +914,11 @@ BackupStatus *qmp_query_backup(Error **errp) + { + BackupStatus *info = g_malloc0(sizeof(*info)); + +- qemu_rec_mutex_lock(&backup_state.stat.lock); ++ qemu_mutex_lock(&backup_state.stat.lock); + + if (!backup_state.stat.start_time) { + /* not started, return {} */ +- qemu_rec_mutex_unlock(&backup_state.stat.lock); ++ qemu_mutex_unlock(&backup_state.stat.lock); + return info; + } + +@@ -955,7 +955,7 @@ BackupStatus *qmp_query_backup(Error **errp) + info->has_transferred = true; + info->transferred = backup_state.stat.transferred; + +- qemu_rec_mutex_unlock(&backup_state.stat.lock); ++ qemu_mutex_unlock(&backup_state.stat.lock); + + return info; + } diff --git a/debian/patches/series b/debian/patches/series index 054ea0b..130ba53 100644 --- a/debian/patches/series +++ b/debian/patches/series @@ -38,4 +38,5 @@ pve/0037-mirror-switch-to-bdrv_dirty_bitmap_merge_internal.patch pve/0038-iotests-add-test-for-bitmap-mirror.patch pve/0039-mirror-move-some-checks-to-qmp.patch pve/0040-PVE-savevm-async-set-up-migration-state.patch -pve/0041-PVE-Use-non-recursive-locks.patch +pve/0041-PVE-Backup-avoid-use-QemuRecMutex-inside-coroutines.patch +pve/0042-PVE-Backup-use-QemuMutex-instead-of-QemuRecMutex.patch