backup: create jobs in a drained section

With the drive-backup QMP command, upstream QEMU uses a drained
section for the source drive when creating the backup job. Do the same
here to avoid subtle bugs.

There, the drained section extends until after the job is started, but
this cannot be done here for multi-disk backups (could at most start
the first job). The important thing is that the cbw
(copy-before-write) node is in place and the bcs (block-copy-state)
bitmap is initialized, which both happen during job creation (ensured
by the "block/backup: move bcs bitmap initialization to job creation"
PVE patch).

One such bug is one reported in the community forum [0], where using a
drive with iothread can lead to an overlapping block-copy request and
consequently an assertion failure. The block-copy code relies on the
bcs bitmap to determine if a request for a certain range can be
created. Each time a request is created, it resets the bcs bitmap at
that range to indicate that it's being handled.

The duplicate request can happen as follows:
Thread A attaches the cbw node
Thread B creates a request and resets the bitmap at that range
Thread A clears the bitmap and merges it with the PBS bitmap
The merging can lead to the bitmap being set again at the range of
the previous request, so the block-copy code thinks it's fine to
create a request there.
Thread B creates another requests at an overlapping range before the
other request is finished.

The drained section ensures that nothing else can interfere with the
bcs bitmap between attaching the copy-before-write block node and
initialization of the bitmap.

[0]: https://forum.proxmox.com/threads/133149/

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
This commit is contained in:
Fiona Ebner 2023-09-06 10:45:11 +02:00 committed by Thomas Lamprecht
parent f7eed6caa1
commit 0cffb504e7
2 changed files with 12 additions and 7 deletions

View File

@ -80,7 +80,8 @@ Signed-off-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
adapt to QAPI changes adapt to QAPI changes
improve canceling improve canceling
allow passing max-workers setting allow passing max-workers setting
use malloc_trim after backup] use malloc_trim after backup
create jobs in a drained section]
Signed-off-by: Fiona Ebner <f.ebner@proxmox.com> Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
--- ---
block/meson.build | 5 + block/meson.build | 5 +
@ -93,11 +94,11 @@ Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
monitor/hmp-cmds.c | 72 +++ monitor/hmp-cmds.c | 72 +++
proxmox-backup-client.c | 146 +++++ proxmox-backup-client.c | 146 +++++
proxmox-backup-client.h | 60 ++ proxmox-backup-client.h | 60 ++
pve-backup.c | 1109 ++++++++++++++++++++++++++++++++ pve-backup.c | 1113 ++++++++++++++++++++++++++++++++
qapi/block-core.json | 226 +++++++ qapi/block-core.json | 226 +++++++
qapi/common.json | 13 + qapi/common.json | 13 +
qapi/machine.json | 15 +- qapi/machine.json | 15 +-
14 files changed, 1723 insertions(+), 13 deletions(-) 14 files changed, 1727 insertions(+), 13 deletions(-)
create mode 100644 proxmox-backup-client.c create mode 100644 proxmox-backup-client.c
create mode 100644 proxmox-backup-client.h create mode 100644 proxmox-backup-client.h
create mode 100644 pve-backup.c create mode 100644 pve-backup.c
@ -588,10 +589,10 @@ index 0000000000..8cbf645b2c
+#endif /* PROXMOX_BACKUP_CLIENT_H */ +#endif /* PROXMOX_BACKUP_CLIENT_H */
diff --git a/pve-backup.c b/pve-backup.c diff --git a/pve-backup.c b/pve-backup.c
new file mode 100644 new file mode 100644
index 0000000000..10ca8a0b1d index 0000000000..c5454e7acc
--- /dev/null --- /dev/null
+++ b/pve-backup.c +++ b/pve-backup.c
@@ -0,0 +1,1109 @@ @@ -0,0 +1,1113 @@
+#include "proxmox-backup-client.h" +#include "proxmox-backup-client.h"
+#include "vma.h" +#include "vma.h"
+ +
@ -1116,12 +1117,16 @@ index 0000000000..10ca8a0b1d
+ AioContext *aio_context = bdrv_get_aio_context(di->bs); + AioContext *aio_context = bdrv_get_aio_context(di->bs);
+ aio_context_acquire(aio_context); + aio_context_acquire(aio_context);
+ +
+ bdrv_drained_begin(di->bs);
+
+ BlockJob *job = backup_job_create( + BlockJob *job = backup_job_create(
+ NULL, di->bs, di->target, backup_state.speed, sync_mode, di->bitmap, + NULL, di->bs, di->target, backup_state.speed, sync_mode, di->bitmap,
+ bitmap_mode, false, NULL, &backup_state.perf, BLOCKDEV_ON_ERROR_REPORT, + bitmap_mode, false, NULL, &backup_state.perf, BLOCKDEV_ON_ERROR_REPORT,
+ BLOCKDEV_ON_ERROR_REPORT, JOB_DEFAULT, pvebackup_complete_cb, di, backup_state.txn, + BLOCKDEV_ON_ERROR_REPORT, JOB_DEFAULT, pvebackup_complete_cb, di, backup_state.txn,
+ &local_err); + &local_err);
+ +
+ bdrv_drained_end(di->bs);
+
+ aio_context_release(aio_context); + aio_context_release(aio_context);
+ +
+ di->job = job; + di->job = job;

View File

@ -175,10 +175,10 @@ index 0000000000..887e998b9e
+ NULL); + NULL);
+} +}
diff --git a/pve-backup.c b/pve-backup.c diff --git a/pve-backup.c b/pve-backup.c
index 10ca8a0b1d..0a5ce2cab8 100644 index c5454e7acc..30bc6ff9ed 100644
--- a/pve-backup.c --- a/pve-backup.c
+++ b/pve-backup.c +++ b/pve-backup.c
@@ -1102,6 +1102,7 @@ ProxmoxSupportStatus *qmp_query_proxmox_support(Error **errp) @@ -1106,6 +1106,7 @@ ProxmoxSupportStatus *qmp_query_proxmox_support(Error **errp)
ret->pbs_library_version = g_strdup(proxmox_backup_qemu_version()); ret->pbs_library_version = g_strdup(proxmox_backup_qemu_version());
ret->pbs_dirty_bitmap = true; ret->pbs_dirty_bitmap = true;
ret->pbs_dirty_bitmap_savevm = true; ret->pbs_dirty_bitmap_savevm = true;