1 From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
2 From: Stefan Reiter <s.reiter@proxmox.com>
3 Date: Thu, 20 Aug 2020 14:25:00 +0200
4 Subject: [PATCH] PVE-Backup: Use a transaction to synchronize job states
6 By using a JobTxn, we can sync dirty bitmaps only when *all* jobs were
7 successful - meaning we don't need to remove them when the backup fails,
8 since QEMU's BITMAP_SYNC_MODE_ON_SUCCESS will now handle that for us.
10 To keep the rate-limiting and IO impact from before, we use a sequential
11 transaction, so drives will still be backed up one after the other.
13 Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
14 Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
15 [add new force parameter to job_cancel_sync calls]
16 Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
18 pve-backup.c | 167 +++++++++++++++------------------------------------
19 1 file changed, 49 insertions(+), 118 deletions(-)
21 diff --git a/pve-backup.c b/pve-backup.c
22 index f90abaa50a..63c686463f 100644
25 @@ -52,6 +52,7 @@ static struct PVEBackupState {
27 ProxmoxBackupHandle *pbs;
30 QemuMutex backup_mutex;
31 CoMutex dump_callback_mutex;
33 @@ -71,32 +72,12 @@ typedef struct PVEBackupDevInfo {
38 char targetfile[PATH_MAX];
39 BdrvDirtyBitmap *bitmap;
40 BlockDriverState *target;
44 -static void pvebackup_run_next_job(void);
47 -lookup_active_block_job(PVEBackupDevInfo *di)
49 - if (!di->completed && di->bs) {
50 - for (BlockJob *job = block_job_next(NULL); job; job = block_job_next(job)) {
51 - if (job->job.driver->job_type != JOB_TYPE_BACKUP) {
55 - BackupBlockJob *bjob = container_of(job, BackupBlockJob, common);
56 - if (bjob && bjob->source_bs == di->bs) {
64 static void pvebackup_propagate_error(Error *err)
66 qemu_mutex_lock(&backup_state.stat.lock);
67 @@ -272,18 +253,6 @@ static void coroutine_fn pvebackup_co_cleanup(void *unused)
68 if (local_err != NULL) {
69 pvebackup_propagate_error(local_err);
72 - // on error or cancel we cannot ensure synchronization of dirty
73 - // bitmaps with backup server, so remove all and do full backup next
74 - GList *l = backup_state.di_list;
76 - PVEBackupDevInfo *di = (PVEBackupDevInfo *)l->data;
80 - bdrv_release_dirty_bitmap(di->bitmap);
85 proxmox_backup_disconnect(backup_state.pbs);
86 @@ -322,8 +291,6 @@ static void pvebackup_complete_cb(void *opaque, int ret)
88 qemu_mutex_lock(&backup_state.backup_mutex);
90 - di->completed = true;
93 Error *local_err = NULL;
94 error_setg(&local_err, "job failed with err %d - %s", ret, strerror(-ret));
95 @@ -336,20 +303,17 @@ static void pvebackup_complete_cb(void *opaque, int ret)
97 block_on_coroutine_fn(pvebackup_complete_stream, di);
99 - // remove self from job queue
100 + // remove self from job list
101 backup_state.di_list = g_list_remove(backup_state.di_list, di);
103 - if (di->bitmap && ret < 0) {
104 - // on error or cancel we cannot ensure synchronization of dirty
105 - // bitmaps with backup server, so remove all and do full backup next
106 - bdrv_release_dirty_bitmap(di->bitmap);
111 - qemu_mutex_unlock(&backup_state.backup_mutex);
112 + /* call cleanup if we're the last job */
113 + if (!g_list_first(backup_state.di_list)) {
114 + block_on_coroutine_fn(pvebackup_co_cleanup, NULL);
117 - pvebackup_run_next_job();
118 + qemu_mutex_unlock(&backup_state.backup_mutex);
121 static void pvebackup_cancel(void)
122 @@ -371,36 +335,28 @@ static void pvebackup_cancel(void)
123 proxmox_backup_abort(backup_state.pbs, "backup canceled");
126 - qemu_mutex_unlock(&backup_state.backup_mutex);
130 - BlockJob *next_job = NULL;
132 - qemu_mutex_lock(&backup_state.backup_mutex);
134 - GList *l = backup_state.di_list;
136 - PVEBackupDevInfo *di = (PVEBackupDevInfo *)l->data;
137 - l = g_list_next(l);
138 + /* it's enough to cancel one job in the transaction, the rest will follow
140 + GList *bdi = g_list_first(backup_state.di_list);
141 + BlockJob *cancel_job = bdi && bdi->data ?
142 + ((PVEBackupDevInfo *)bdi->data)->job :
145 - BlockJob *job = lookup_active_block_job(di);
151 + /* ref the job before releasing the mutex, just to be safe */
153 + job_ref(&cancel_job->job);
156 - qemu_mutex_unlock(&backup_state.backup_mutex);
157 + /* job_cancel_sync may enter the job, so we need to release the
158 + * backup_mutex to avoid deadlock */
159 + qemu_mutex_unlock(&backup_state.backup_mutex);
162 - AioContext *aio_context = next_job->job.aio_context;
163 - aio_context_acquire(aio_context);
164 - job_cancel_sync(&next_job->job, true);
165 - aio_context_release(aio_context);
170 + AioContext *aio_context = cancel_job->job.aio_context;
171 + aio_context_acquire(aio_context);
172 + job_cancel_sync(&cancel_job->job, true);
173 + job_unref(&cancel_job->job);
174 + aio_context_release(aio_context);
178 @@ -459,51 +415,19 @@ static int coroutine_fn pvebackup_co_add_config(
182 -bool job_should_pause(Job *job);
184 -static void pvebackup_run_next_job(void)
186 - assert(!qemu_in_coroutine());
188 - qemu_mutex_lock(&backup_state.backup_mutex);
190 - GList *l = backup_state.di_list;
192 - PVEBackupDevInfo *di = (PVEBackupDevInfo *)l->data;
193 - l = g_list_next(l);
195 - BlockJob *job = lookup_active_block_job(di);
198 - qemu_mutex_unlock(&backup_state.backup_mutex);
200 - AioContext *aio_context = job->job.aio_context;
201 - aio_context_acquire(aio_context);
203 - if (job_should_pause(&job->job)) {
204 - bool error_or_canceled = pvebackup_error_or_canceled();
205 - if (error_or_canceled) {
206 - job_cancel_sync(&job->job, true);
208 - job_resume(&job->job);
211 - aio_context_release(aio_context);
216 - block_on_coroutine_fn(pvebackup_co_cleanup, NULL); // no more jobs, run cleanup
218 - qemu_mutex_unlock(&backup_state.backup_mutex);
221 static bool create_backup_jobs(void) {
223 assert(!qemu_in_coroutine());
225 Error *local_err = NULL;
227 + /* create job transaction to synchronize bitmap commit and cancel all
228 + * jobs in case one errors */
229 + if (backup_state.txn) {
230 + job_txn_unref(backup_state.txn);
232 + backup_state.txn = job_txn_new_seq();
234 BackupPerf perf = { .max_workers = 16 };
236 /* create and start all jobs (paused state) */
237 @@ -526,7 +450,7 @@ static bool create_backup_jobs(void) {
238 BlockJob *job = backup_job_create(
239 NULL, di->bs, di->target, backup_state.speed, sync_mode, di->bitmap,
240 bitmap_mode, false, NULL, &perf, BLOCKDEV_ON_ERROR_REPORT, BLOCKDEV_ON_ERROR_REPORT,
241 - JOB_DEFAULT, pvebackup_complete_cb, di, NULL, &local_err);
242 + JOB_DEFAULT, pvebackup_complete_cb, di, backup_state.txn, &local_err);
244 aio_context_release(aio_context);
246 @@ -538,7 +462,8 @@ static bool create_backup_jobs(void) {
247 pvebackup_propagate_error(create_job_err);
250 - job_start(&job->job);
254 bdrv_unref(di->target);
256 @@ -556,6 +481,10 @@ static bool create_backup_jobs(void) {
257 bdrv_unref(di->target);
262 + job_unref(&di->job->job);
267 @@ -946,10 +875,6 @@ err:
268 PVEBackupDevInfo *di = (PVEBackupDevInfo *)l->data;
272 - bdrv_release_dirty_bitmap(di->bitmap);
276 bdrv_unref(di->target);
278 @@ -1038,9 +963,15 @@ UuidInfo *qmp_backup(
279 block_on_coroutine_fn(pvebackup_co_prepare, &task);
282 - create_backup_jobs();
283 + bool errors = create_backup_jobs();
284 qemu_mutex_unlock(&backup_state.backup_mutex);
285 - pvebackup_run_next_job();
288 + /* start the first job in the transaction
289 + * note: this might directly enter the job, so we need to do this
290 + * after unlocking the backup_mutex */
291 + job_txn_start_seq(backup_state.txn);
294 qemu_mutex_unlock(&backup_state.backup_mutex);