[pve-devel] [RFC] PVE-Backup: create jobs in a drained section

[Fiona Ebner <f.ebner@proxmox.com>]

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 potential subtle bugs in the future if upstream starts
relying on that behavior.

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

Inserting the copy-before-write node is already protected with a
drained section, which is why there should be no actual issue right
now. While that drained section does not extend until the bcs bitmap
initialization, it should also not be an issue currently, because the
job is not created from a coroutine (and even if, there would need to
be a yield point in between).

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---

@Thomas: The stuff with making sure source and target use the same
AioContext I talked about off-list doesn't seem to be required, or
to be more precise, it already happens: when copy-before-write opens
the target, it is opened in the same AioContext. There even is an
assertion in block-copy for source and target to use the same
AioContext, so we couldn't mess it up without noticing.

 pve-backup.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/pve-backup.c b/pve-backup.c
index 3ca4f74cb8..75791c2313 100644
--- a/pve-backup.c
+++ b/pve-backup.c
@@ -508,12 +508,16 @@ static void create_backup_jobs_bh(void *opaque) {
         AioContext *aio_context = bdrv_get_aio_context(di->bs);
         aio_context_acquire(aio_context);
 
+        bdrv_drained_begin(di->bs);
+
         BlockJob *job = backup_job_create(
             NULL, di->bs, di->target, backup_state.speed, sync_mode, di->bitmap,
             bitmap_mode, false, NULL, &backup_state.perf, BLOCKDEV_ON_ERROR_REPORT,
             BLOCKDEV_ON_ERROR_REPORT, JOB_DEFAULT, pvebackup_complete_cb, di, backup_state.txn,
             &local_err);
 
+        bdrv_drained_end(di->bs);
+
         aio_context_release(aio_context);
 
         di->job = job;
-- 
2.30.2