[pve-devel] [PATCH v2 qemu 6/6] Several fixes for backup abort and error reporting
Stefan Reiter
s.reiter at proxmox.com
Thu Oct 29 14:10:36 CET 2020
Also add my Signed-off-by to some patches where it was missing.
Signed-off-by: Stefan Reiter <s.reiter at proxmox.com>
---
v2: new
Patches 1-5 squashed in or added to the pve-qemu repository.
Confirmed with "objdump" that the binary is the same with squashed changes as
with standalone patches.
...ckup-proxmox-backup-patches-for-qemu.patch | 41 ++--
...irty-bitmap-tracking-for-incremental.patch | 10 +-
...ct-stderr-to-journal-when-daemonized.patch | 2 +
...d-sequential-job-transaction-support.patch | 1 +
...-transaction-to-synchronize-job-stat.patch | 2 +
...ore-coroutines-and-don-t-block-on-fi.patch | 94 +++++----
...n-up-error-handling-for-create_backu.patch | 187 ++++++++++++++++++
debian/patches/series | 1 +
8 files changed, 275 insertions(+), 63 deletions(-)
create mode 100644 debian/patches/pve/0053-PVE-fix-and-clean-up-error-handling-for-create_backu.patch
diff --git a/debian/patches/pve/0028-PVE-Backup-proxmox-backup-patches-for-qemu.patch b/debian/patches/pve/0028-PVE-Backup-proxmox-backup-patches-for-qemu.patch
index d41b675..7896692 100644
--- a/debian/patches/pve/0028-PVE-Backup-proxmox-backup-patches-for-qemu.patch
+++ b/debian/patches/pve/0028-PVE-Backup-proxmox-backup-patches-for-qemu.patch
@@ -14,13 +14,13 @@ Subject: [PATCH] PVE-Backup: proxmox backup patches for qemu
include/block/block_int.h | 2 +-
include/monitor/hmp.h | 3 +
monitor/hmp-cmds.c | 44 ++
- proxmox-backup-client.c | 182 +++++++
- proxmox-backup-client.h | 52 ++
+ proxmox-backup-client.c | 176 ++++++
+ proxmox-backup-client.h | 59 ++
pve-backup.c | 955 +++++++++++++++++++++++++++++++++
qapi/block-core.json | 109 ++++
qapi/common.json | 13 +
qapi/misc.json | 13 -
- 16 files changed, 1439 insertions(+), 15 deletions(-)
+ 16 files changed, 1440 insertions(+), 15 deletions(-)
create mode 100644 proxmox-backup-client.c
create mode 100644 proxmox-backup-client.h
create mode 100644 pve-backup.c
@@ -278,10 +278,10 @@ index 280bb447a6..0e2d166552 100644
switch (addr->type) {
diff --git a/proxmox-backup-client.c b/proxmox-backup-client.c
new file mode 100644
-index 0000000000..b7bc7f2574
+index 0000000000..a8f6653a81
--- /dev/null
+++ b/proxmox-backup-client.c
-@@ -0,0 +1,182 @@
+@@ -0,0 +1,176 @@
+#include "proxmox-backup-client.h"
+#include "qemu/main-loop.h"
+#include "block/aio-wait.h"
@@ -296,12 +296,6 @@ index 0000000000..b7bc7f2574
+ bool finished;
+} BlockOnCoroutineWrapper;
+
-+// Waker implementaion to syncronice with proxmox backup rust code
-+typedef struct ProxmoxBackupWaker {
-+ Coroutine *co;
-+ AioContext *ctx;
-+} ProxmoxBackupWaker;
-+
+static void coroutine_fn block_on_coroutine_wrapper(void *opaque)
+{
+ BlockOnCoroutineWrapper *wrapper = opaque;
@@ -328,7 +322,7 @@ index 0000000000..b7bc7f2574
+
+// This is called from another thread, so we use aio_co_schedule()
+static void proxmox_backup_schedule_wake(void *data) {
-+ ProxmoxBackupWaker *waker = (ProxmoxBackupWaker *)data;
++ CoCtxData *waker = (CoCtxData *)data;
+ aio_co_schedule(waker->ctx, waker->co);
+}
+
@@ -337,7 +331,7 @@ index 0000000000..b7bc7f2574
+{
+ Coroutine *co = qemu_coroutine_self();
+ AioContext *ctx = qemu_get_current_aio_context();
-+ ProxmoxBackupWaker waker = { .co = co, .ctx = ctx };
++ CoCtxData waker = { .co = co, .ctx = ctx };
+ char *pbs_err = NULL;
+ int pbs_res = -1;
+
@@ -360,7 +354,7 @@ index 0000000000..b7bc7f2574
+{
+ Coroutine *co = qemu_coroutine_self();
+ AioContext *ctx = qemu_get_current_aio_context();
-+ ProxmoxBackupWaker waker = { .co = co, .ctx = ctx };
++ CoCtxData waker = { .co = co, .ctx = ctx };
+ char *pbs_err = NULL;
+ int pbs_res = -1;
+
@@ -383,7 +377,7 @@ index 0000000000..b7bc7f2574
+{
+ Coroutine *co = qemu_coroutine_self();
+ AioContext *ctx = qemu_get_current_aio_context();
-+ ProxmoxBackupWaker waker = { .co = co, .ctx = ctx };
++ CoCtxData waker = { .co = co, .ctx = ctx };
+ char *pbs_err = NULL;
+ int pbs_res = -1;
+
@@ -404,7 +398,7 @@ index 0000000000..b7bc7f2574
+{
+ Coroutine *co = qemu_coroutine_self();
+ AioContext *ctx = qemu_get_current_aio_context();
-+ ProxmoxBackupWaker waker = { .co = co, .ctx = ctx };
++ CoCtxData waker = { .co = co, .ctx = ctx };
+ char *pbs_err = NULL;
+ int pbs_res = -1;
+
@@ -426,7 +420,7 @@ index 0000000000..b7bc7f2574
+{
+ Coroutine *co = qemu_coroutine_self();
+ AioContext *ctx = qemu_get_current_aio_context();
-+ ProxmoxBackupWaker waker = { .co = co, .ctx = ctx };
++ CoCtxData waker = { .co = co, .ctx = ctx };
+ char *pbs_err = NULL;
+ int pbs_res = -1;
+
@@ -451,7 +445,7 @@ index 0000000000..b7bc7f2574
+{
+ Coroutine *co = qemu_coroutine_self();
+ AioContext *ctx = qemu_get_current_aio_context();
-+ ProxmoxBackupWaker waker = { .co = co, .ctx = ctx };
++ CoCtxData waker = { .co = co, .ctx = ctx };
+ char *pbs_err = NULL;
+ int pbs_res = -1;
+
@@ -466,10 +460,10 @@ index 0000000000..b7bc7f2574
+}
diff --git a/proxmox-backup-client.h b/proxmox-backup-client.h
new file mode 100644
-index 0000000000..b311bf8de8
+index 0000000000..1dda8b7d8f
--- /dev/null
+++ b/proxmox-backup-client.h
-@@ -0,0 +1,52 @@
+@@ -0,0 +1,59 @@
+#ifndef PROXMOX_BACKUP_CLIENT_H
+#define PROXMOX_BACKUP_CLIENT_H
+
@@ -477,6 +471,13 @@ index 0000000000..b311bf8de8
+#include "qemu/coroutine.h"
+#include "proxmox-backup-qemu.h"
+
++typedef struct CoCtxData {
++ Coroutine *co;
++ AioContext *ctx;
++ void *data;
++} CoCtxData;
++
++// FIXME: Remove once coroutines are supported for QMP
+void block_on_coroutine_fn(CoroutineEntry *entry, void *entry_arg);
+
+int coroutine_fn
diff --git a/debian/patches/pve/0037-PVE-Backup-Add-dirty-bitmap-tracking-for-incremental.patch b/debian/patches/pve/0037-PVE-Backup-Add-dirty-bitmap-tracking-for-incremental.patch
index 1e550f4..c974060 100644
--- a/debian/patches/pve/0037-PVE-Backup-Add-dirty-bitmap-tracking-for-incremental.patch
+++ b/debian/patches/pve/0037-PVE-Backup-Add-dirty-bitmap-tracking-for-incremental.patch
@@ -99,10 +99,10 @@ index 0e2d166552..3ff014d32a 100644
qapi_free_BackupStatus(info);
diff --git a/proxmox-backup-client.c b/proxmox-backup-client.c
-index b7bc7f2574..0e9c584701 100644
+index a8f6653a81..4ce7bc0b5e 100644
--- a/proxmox-backup-client.c
+++ b/proxmox-backup-client.c
-@@ -95,6 +95,7 @@ proxmox_backup_co_register_image(
+@@ -89,6 +89,7 @@ proxmox_backup_co_register_image(
ProxmoxBackupHandle *pbs,
const char *device_name,
uint64_t size,
@@ -110,7 +110,7 @@ index b7bc7f2574..0e9c584701 100644
Error **errp)
{
Coroutine *co = qemu_coroutine_self();
-@@ -104,7 +105,7 @@ proxmox_backup_co_register_image(
+@@ -98,7 +99,7 @@ proxmox_backup_co_register_image(
int pbs_res = -1;
proxmox_backup_register_image_async(
@@ -120,10 +120,10 @@ index b7bc7f2574..0e9c584701 100644
if (pbs_res < 0) {
if (errp) error_setg(errp, "backup register image failed: %s", pbs_err ? pbs_err : "unknown error");
diff --git a/proxmox-backup-client.h b/proxmox-backup-client.h
-index b311bf8de8..20fd6b1719 100644
+index 1dda8b7d8f..8cbf645b2c 100644
--- a/proxmox-backup-client.h
+++ b/proxmox-backup-client.h
-@@ -25,6 +25,7 @@ proxmox_backup_co_register_image(
+@@ -32,6 +32,7 @@ proxmox_backup_co_register_image(
ProxmoxBackupHandle *pbs,
const char *device_name,
uint64_t size,
diff --git a/debian/patches/pve/0049-PVE-redirect-stderr-to-journal-when-daemonized.patch b/debian/patches/pve/0049-PVE-redirect-stderr-to-journal-when-daemonized.patch
index a4ca3c4..1d54282 100644
--- a/debian/patches/pve/0049-PVE-redirect-stderr-to-journal-when-daemonized.patch
+++ b/debian/patches/pve/0049-PVE-redirect-stderr-to-journal-when-daemonized.patch
@@ -5,6 +5,8 @@ Subject: [PATCH] PVE: redirect stderr to journal when daemonized
QEMU uses the logging for error messages usually, so LOG_ERR is most
fitting.
+
+Signed-off-by: Stefan Reiter <s.reiter at proxmox.com>
---
Makefile.objs | 1 +
os-posix.c | 7 +++++--
diff --git a/debian/patches/pve/0050-PVE-Add-sequential-job-transaction-support.patch b/debian/patches/pve/0050-PVE-Add-sequential-job-transaction-support.patch
index 67b053d..20531d3 100644
--- a/debian/patches/pve/0050-PVE-Add-sequential-job-transaction-support.patch
+++ b/debian/patches/pve/0050-PVE-Add-sequential-job-transaction-support.patch
@@ -3,6 +3,7 @@ From: Stefan Reiter <s.reiter at proxmox.com>
Date: Thu, 20 Aug 2020 14:31:59 +0200
Subject: [PATCH] PVE: Add sequential job transaction support
+Signed-off-by: Stefan Reiter <s.reiter at proxmox.com>
---
include/qemu/job.h | 12 ++++++++++++
job.c | 24 ++++++++++++++++++++++++
diff --git a/debian/patches/pve/0051-PVE-Backup-Use-a-transaction-to-synchronize-job-stat.patch b/debian/patches/pve/0051-PVE-Backup-Use-a-transaction-to-synchronize-job-stat.patch
index 0e33331..a9489a8 100644
--- a/debian/patches/pve/0051-PVE-Backup-Use-a-transaction-to-synchronize-job-stat.patch
+++ b/debian/patches/pve/0051-PVE-Backup-Use-a-transaction-to-synchronize-job-stat.patch
@@ -9,6 +9,8 @@ since QEMU's BITMAP_SYNC_MODE_ON_SUCCESS will now handle that for us.
To keep the rate-limiting and IO impact from before, we use a sequential
transaction, so drives will still be backed up one after the other.
+
+Signed-off-by: Stefan Reiter <s.reiter at proxmox.com>
---
pve-backup.c | 167 +++++++++++++++------------------------------------
1 file changed, 49 insertions(+), 118 deletions(-)
diff --git a/debian/patches/pve/0052-PVE-Backup-Use-more-coroutines-and-don-t-block-on-fi.patch b/debian/patches/pve/0052-PVE-Backup-Use-more-coroutines-and-don-t-block-on-fi.patch
index 8ae58fe..695c0fd 100644
--- a/debian/patches/pve/0052-PVE-Backup-Use-more-coroutines-and-don-t-block-on-fi.patch
+++ b/debian/patches/pve/0052-PVE-Backup-Use-more-coroutines-and-don-t-block-on-fi.patch
@@ -20,38 +20,25 @@ Cases of aio_context_acquire/release from within what is now a coroutine
are changed to aio_co_reschedule_self, which works since a running
coroutine always holds the aio lock for the context it is running in.
-job_cancel_sync is changed to regular job_cancel, since job_cancel_sync
-uses AIO_WAIT_WHILE internally, which is forbidden from Coroutines.
+job_cancel_sync is called from a BH since it can't be run from a
+coroutine (uses AIO_WAIT_WHILE internally).
-create_backup_jobs cannot be run from a coroutine, so it is run
-directly. This does however mean that it runs unlocked, as it cannot
-acquire a CoMutex (see it's comment for rational on why that's ok).
+Same thing for create_backup_jobs, which is converted to a BH too.
To communicate the finishing state, a new property is introduced to
query-backup: 'finishing'. A new state is explicitly not used, since
that would break compatibility with older qemu-server versions.
[0] https://lists.gnu.org/archive/html/qemu-devel/2020-09/msg03515.html
----
- proxmox-backup-client.h | 1 +
- pve-backup.c | 124 ++++++++++++++++++++++------------------
- qapi/block-core.json | 5 +-
- 3 files changed, 74 insertions(+), 56 deletions(-)
-diff --git a/proxmox-backup-client.h b/proxmox-backup-client.h
-index 20fd6b1719..a4781c5851 100644
---- a/proxmox-backup-client.h
-+++ b/proxmox-backup-client.h
-@@ -5,6 +5,7 @@
- #include "qemu/coroutine.h"
- #include "proxmox-backup-qemu.h"
-
-+// FIXME: Remove once coroutines are supported for QMP
- void block_on_coroutine_fn(CoroutineEntry *entry, void *entry_arg);
-
- int coroutine_fn
+Signed-off-by: Stefan Reiter <s.reiter at proxmox.com>
+---
+ pve-backup.c | 148 ++++++++++++++++++++++++++-----------------
+ qapi/block-core.json | 5 +-
+ 2 files changed, 95 insertions(+), 58 deletions(-)
+
diff --git a/pve-backup.c b/pve-backup.c
-index 9562e9c98d..53cf23ed5a 100644
+index 9562e9c98d..0466145bec 100644
--- a/pve-backup.c
+++ b/pve-backup.c
@@ -33,7 +33,9 @@ const char *PBS_BITMAP_NAME = "pbs-incremental-dirty-bitmap";
@@ -172,7 +159,7 @@ index 9562e9c98d..53cf23ed5a 100644
// remove self from job list
backup_state.di_list = g_list_remove(backup_state.di_list, di);
-@@ -310,21 +310,36 @@ static void pvebackup_complete_cb(void *opaque, int ret)
+@@ -310,21 +310,49 @@ static void pvebackup_complete_cb(void *opaque, int ret)
/* call cleanup if we're the last job */
if (!g_list_first(backup_state.di_list)) {
@@ -187,19 +174,33 @@ index 9562e9c98d..53cf23ed5a 100644
-static void pvebackup_cancel(void)
+static void pvebackup_complete_cb(void *opaque, int ret)
{
- assert(!qemu_in_coroutine());
-
+- assert(!qemu_in_coroutine());
+ PVEBackupDevInfo *di = opaque;
+ di->completed_ret = ret;
+
+ /*
+ * Schedule stream cleanup in async coroutine. close_image and finish might
-+ * take a while, so we can't block on them here.
++ * take a while, so we can't block on them here. This way it also doesn't
++ * matter if we're already running in a coroutine or not.
+ * Note: di is a pointer to an entry in the global backup_state struct, so
+ * it stays valid.
+ */
+ Coroutine *co = qemu_coroutine_create(pvebackup_co_complete_stream, di);
-+ aio_co_schedule(qemu_get_aio_context(), co);
++ aio_co_enter(qemu_get_aio_context(), co);
++}
+
++/*
++ * job_cancel(_sync) does not like to be called from coroutines, so defer to
++ * main loop processing via a bottom half.
++ */
++static void job_cancel_bh(void *opaque) {
++ CoCtxData *data = (CoCtxData*)opaque;
++ Job *job = (Job*)data->data;
++ AioContext *job_ctx = job->aio_context;
++ aio_context_acquire(job_ctx);
++ job_cancel_sync(job);
++ aio_context_release(job_ctx);
++ aio_co_enter(data->ctx, data->co);
+}
+
+static void coroutine_fn pvebackup_co_cancel(void *opaque)
@@ -213,14 +214,20 @@ index 9562e9c98d..53cf23ed5a 100644
if (backup_state.vmaw) {
/* make sure vma writer does not block anymore */
-@@ -342,27 +357,16 @@ static void pvebackup_cancel(void)
+@@ -342,27 +370,22 @@ static void pvebackup_cancel(void)
((PVEBackupDevInfo *)bdi->data)->job :
NULL;
- /* ref the job before releasing the mutex, just to be safe */
if (cancel_job) {
- job_ref(&cancel_job->job);
-+ job_cancel(&cancel_job->job, false);
++ CoCtxData data = {
++ .ctx = qemu_get_current_aio_context(),
++ .co = qemu_coroutine_self(),
++ .data = &cancel_job->job,
++ };
++ aio_bh_schedule_oneshot(data.ctx, job_cancel_bh, &data);
++ qemu_coroutine_yield();
}
- /* job_cancel_sync may enter the job, so we need to release the
@@ -244,7 +251,7 @@ index 9562e9c98d..53cf23ed5a 100644
}
// assumes the caller holds backup_mutex
-@@ -415,6 +419,14 @@ static int coroutine_fn pvebackup_co_add_config(
+@@ -415,6 +438,14 @@ static int coroutine_fn pvebackup_co_add_config(
goto out;
}
@@ -259,7 +266,7 @@ index 9562e9c98d..53cf23ed5a 100644
static bool create_backup_jobs(void) {
assert(!qemu_in_coroutine());
-@@ -523,11 +535,12 @@ typedef struct QmpBackupTask {
+@@ -523,11 +554,12 @@ typedef struct QmpBackupTask {
UuidInfo *result;
} QmpBackupTask;
@@ -273,7 +280,18 @@ index 9562e9c98d..53cf23ed5a 100644
QmpBackupTask *task = opaque;
task->result = NULL; // just to be sure
-@@ -616,6 +629,8 @@ static void coroutine_fn pvebackup_co_prepare(void *opaque)
+@@ -548,8 +580,9 @@ 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(task->errp, ERROR_CLASS_GENERIC_ERROR,
+ "previous backup not finished");
++ qemu_co_mutex_unlock(&backup_state.backup_mutex);
+ return;
+ }
+
+@@ -616,6 +649,8 @@ static void coroutine_fn pvebackup_co_prepare(void *opaque)
}
di->size = size;
total += size;
@@ -282,7 +300,7 @@ index 9562e9c98d..53cf23ed5a 100644
}
uuid_generate(uuid);
-@@ -847,6 +862,7 @@ static void coroutine_fn pvebackup_co_prepare(void *opaque)
+@@ -847,6 +882,7 @@ static void coroutine_fn pvebackup_co_prepare(void *opaque)
backup_state.stat.dirty = total - backup_state.stat.reused;
backup_state.stat.transferred = 0;
backup_state.stat.zero_bytes = 0;
@@ -290,7 +308,7 @@ index 9562e9c98d..53cf23ed5a 100644
qemu_mutex_unlock(&backup_state.stat.lock);
-@@ -861,6 +877,8 @@ static void coroutine_fn pvebackup_co_prepare(void *opaque)
+@@ -861,6 +897,8 @@ static void coroutine_fn pvebackup_co_prepare(void *opaque)
uuid_info->UUID = uuid_str;
task->result = uuid_info;
@@ -299,7 +317,7 @@ index 9562e9c98d..53cf23ed5a 100644
return;
err_mutex:
-@@ -903,6 +921,8 @@ err:
+@@ -903,6 +941,8 @@ err:
}
task->result = NULL;
@@ -308,7 +326,7 @@ index 9562e9c98d..53cf23ed5a 100644
return;
}
-@@ -956,22 +976,15 @@ UuidInfo *qmp_backup(
+@@ -956,22 +996,15 @@ UuidInfo *qmp_backup(
.errp = errp,
};
@@ -332,7 +350,7 @@ index 9562e9c98d..53cf23ed5a 100644
}
return task.result;
-@@ -1025,6 +1038,7 @@ BackupStatus *qmp_query_backup(Error **errp)
+@@ -1025,6 +1058,7 @@ BackupStatus *qmp_query_backup(Error **errp)
info->transferred = backup_state.stat.transferred;
info->has_reused = true;
info->reused = backup_state.stat.reused;
diff --git a/debian/patches/pve/0053-PVE-fix-and-clean-up-error-handling-for-create_backu.patch b/debian/patches/pve/0053-PVE-fix-and-clean-up-error-handling-for-create_backu.patch
new file mode 100644
index 0000000..45dabc4
--- /dev/null
+++ b/debian/patches/pve/0053-PVE-fix-and-clean-up-error-handling-for-create_backu.patch
@@ -0,0 +1,187 @@
+From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
+From: Stefan Reiter <s.reiter at proxmox.com>
+Date: Thu, 22 Oct 2020 17:01:07 +0200
+Subject: [PATCH] PVE: fix and clean up error handling for create_backup_jobs
+
+No more weird bool returns, just the standard "errp" format used
+everywhere else too. With this, if backup_job_create fails, the error
+message is actually returned over QMP and can be shown to the user.
+
+To facilitate correct cleanup on such an error, we call
+create_backup_jobs as a bottom half directly from pvebackup_co_prepare.
+This additionally allows us to actually hold the backup_mutex during
+operation.
+
+Also add a job_cancel_sync before job_unref, since a job must be in
+STATUS_NULL to be deleted by unref, which could trigger an assert
+before.
+
+Signed-off-by: Stefan Reiter <s.reiter at proxmox.com>
+---
+ pve-backup.c | 79 +++++++++++++++++++++++++++++++++++-----------------
+ 1 file changed, 54 insertions(+), 25 deletions(-)
+
+diff --git a/pve-backup.c b/pve-backup.c
+index 0466145bec..1a2647e7a5 100644
+--- a/pve-backup.c
++++ b/pve-backup.c
+@@ -50,6 +50,7 @@ static struct PVEBackupState {
+ size_t zero_bytes;
+ GList *bitmap_list;
+ bool finishing;
++ bool starting;
+ } stat;
+ int64_t speed;
+ VmaWriter *vmaw;
+@@ -277,6 +278,16 @@ static void coroutine_fn pvebackup_co_complete_stream(void *opaque)
+ PVEBackupDevInfo *di = opaque;
+ int ret = di->completed_ret;
+
++ qemu_mutex_lock(&backup_state.stat.lock);
++ bool starting = backup_state.stat.starting;
++ qemu_mutex_unlock(&backup_state.stat.lock);
++ if (starting) {
++ /* in 'starting' state, no tasks have been run yet, meaning we can (and
++ * must) skip all cleanup, as we don't know what has and hasn't been
++ * initialized yet. */
++ return;
++ }
++
+ qemu_co_mutex_lock(&backup_state.backup_mutex);
+
+ if (ret < 0) {
+@@ -441,15 +452,15 @@ static int coroutine_fn pvebackup_co_add_config(
+ /*
+ * backup_job_create can *not* be run from a coroutine (and requires an
+ * acquired AioContext), so this can't either.
+- * This does imply that this function cannot run with backup_mutex acquired.
+- * That is ok because it is only ever called between setting up the backup_state
+- * struct and starting the jobs, and from within a QMP call. This means that no
+- * other QMP call can interrupt, and no background job is running yet.
++ * The caller is responsible that backup_mutex is held nonetheless.
+ */
+-static bool create_backup_jobs(void) {
++static void create_backup_jobs_bh(void *opaque) {
+
+ assert(!qemu_in_coroutine());
+
++ CoCtxData *data = (CoCtxData*)opaque;
++ Error **errp = (Error**)data->data;
++
+ Error *local_err = NULL;
+
+ /* create job transaction to synchronize bitmap commit and cancel all
+@@ -483,24 +494,19 @@ static bool create_backup_jobs(void) {
+
+ aio_context_release(aio_context);
+
+- if (!job || local_err != NULL) {
+- Error *create_job_err = NULL;
+- error_setg(&create_job_err, "backup_job_create failed: %s",
+- local_err ? error_get_pretty(local_err) : "null");
++ di->job = job;
+
+- pvebackup_propagate_error(create_job_err);
++ if (!job || local_err) {
++ error_setg(errp, "backup_job_create failed: %s",
++ local_err ? error_get_pretty(local_err) : "null");
+ break;
+ }
+
+- di->job = job;
+-
+ bdrv_unref(di->target);
+ di->target = NULL;
+ }
+
+- bool errors = pvebackup_error_or_canceled();
+-
+- if (errors) {
++ if (*errp) {
+ l = backup_state.di_list;
+ while (l) {
+ PVEBackupDevInfo *di = (PVEBackupDevInfo *)l->data;
+@@ -512,12 +518,17 @@ static bool create_backup_jobs(void) {
+ }
+
+ if (di->job) {
++ AioContext *ctx = di->job->job.aio_context;
++ aio_context_acquire(ctx);
++ job_cancel_sync(&di->job->job);
+ job_unref(&di->job->job);
++ aio_context_release(ctx);
+ }
+ }
+ }
+
+- return errors;
++ /* return */
++ aio_co_enter(data->ctx, data->co);
+ }
+
+ typedef struct QmpBackupTask {
+@@ -883,6 +894,7 @@ static void coroutine_fn pvebackup_co_prepare(void *opaque)
+ backup_state.stat.transferred = 0;
+ backup_state.stat.zero_bytes = 0;
+ backup_state.stat.finishing = false;
++ backup_state.stat.starting = true;
+
+ qemu_mutex_unlock(&backup_state.stat.lock);
+
+@@ -898,7 +910,32 @@ static void coroutine_fn pvebackup_co_prepare(void *opaque)
+
+ 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.
++ */
++ CoCtxData waker = {
++ .co = qemu_coroutine_self(),
++ .ctx = qemu_get_current_aio_context(),
++ .data = &local_err,
++ };
++ aio_bh_schedule_oneshot(waker.ctx, create_backup_jobs_bh, &waker);
++ qemu_coroutine_yield();
++
++ if (local_err) {
++ error_propagate(task->errp, local_err);
++ goto err;
++ }
++
+ qemu_co_mutex_unlock(&backup_state.backup_mutex);
++
++ qemu_mutex_lock(&backup_state.stat.lock);
++ backup_state.stat.starting = false;
++ qemu_mutex_unlock(&backup_state.stat.lock);
++
++ /* start the first job in the transaction */
++ job_txn_start_seq(backup_state.txn);
++
+ return;
+
+ err_mutex:
+@@ -921,6 +958,7 @@ err:
+ g_free(di);
+ }
+ g_list_free(di_list);
++ backup_state.di_list = NULL;
+
+ if (devs) {
+ g_strfreev(devs);
+@@ -998,15 +1036,6 @@ UuidInfo *qmp_backup(
+
+ block_on_coroutine_fn(pvebackup_co_prepare, &task);
+
+- if (*errp == NULL) {
+- bool errors = create_backup_jobs();
+-
+- if (!errors) {
+- // start the first job in the transaction
+- job_txn_start_seq(backup_state.txn);
+- }
+- }
+-
+ return task.result;
+ }
+
diff --git a/debian/patches/series b/debian/patches/series
index 021c0d7..9ebe181 100644
--- a/debian/patches/series
+++ b/debian/patches/series
@@ -53,3 +53,4 @@ pve/0049-PVE-redirect-stderr-to-journal-when-daemonized.patch
pve/0050-PVE-Add-sequential-job-transaction-support.patch
pve/0051-PVE-Backup-Use-a-transaction-to-synchronize-job-stat.patch
pve/0052-PVE-Backup-Use-more-coroutines-and-don-t-block-on-fi.patch
+pve/0053-PVE-fix-and-clean-up-error-handling-for-create_backu.patch
--
2.20.1
More information about the pve-devel
mailing list