[Qemu-devel] [PATCH] block/backup: install notifier during creation

[John Snow]

Backup jobs may yield prior to installing their handler, because of the
job_co_entry shim which guarantees that a job won't begin work until
we are ready to start an entire transaction.

Unfortunately, this makes proving correctness about transactional
points-in-time for backup hard to reason about. Make it explicitly clear
by moving the handler registration to creation time, and changing the
write notifier to a no-op until the job is started.

Reported-by: Vladimir Sementsov-Ogievskiy <address@hidden>
Signed-off-by: John Snow <address@hidden>
---
 block/backup.c     | 32 +++++++++++++++++++++++---------
 include/qemu/job.h |  5 +++++
 job.c              |  2 +-
 3 files changed, 29 insertions(+), 10 deletions(-)

diff --git a/block/backup.c b/block/backup.c
index 07d751aea4..4df5b95415 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -344,6 +344,13 @@ static int coroutine_fn backup_before_write_notify(
     assert(QEMU_IS_ALIGNED(req->offset, BDRV_SECTOR_SIZE));
     assert(QEMU_IS_ALIGNED(req->bytes, BDRV_SECTOR_SIZE));
 
+    /* The handler is installed at creation time; the actual point-in-time
+     * starts at job_start(). Transactions guarantee those two points are
+     * the same point in time. */
+    if (!job_started(&job->common.job)) {
+        return 0;
+    }
+
     return backup_do_cow(job, req->offset, req->bytes, NULL, true);
 }
 
@@ -398,6 +405,12 @@ static void backup_clean(Job *job)
     BackupBlockJob *s = container_of(job, BackupBlockJob, common.job);
     BlockDriverState *bs = blk_bs(s->common.blk);
 
+    /* cancelled before job_start: remove write_notifier */
+    if (s->before_write.notify) {
+        notifier_with_return_remove(&s->before_write);
+        s->before_write.notify = NULL;
+    }
+
     if (s->copy_bitmap) {
         bdrv_release_dirty_bitmap(bs, s->copy_bitmap);
         s->copy_bitmap = NULL;
@@ -527,17 +540,8 @@ static void backup_init_copy_bitmap(BackupBlockJob *job)
 static int coroutine_fn backup_run(Job *job, Error **errp)
 {
     BackupBlockJob *s = container_of(job, BackupBlockJob, common.job);
-    BlockDriverState *bs = blk_bs(s->common.blk);
     int ret = 0;
 
-    QLIST_INIT(&s->inflight_reqs);
-    qemu_co_rwlock_init(&s->flush_rwlock);
-
-    backup_init_copy_bitmap(s);
-
-    s->before_write.notify = backup_before_write_notify;
-    bdrv_add_before_write_notifier(bs, &s->before_write);
-
     if (s->sync_mode == MIRROR_SYNC_MODE_TOP) {
         int64_t offset = 0;
         int64_t count;
@@ -572,6 +576,7 @@ static int coroutine_fn backup_run(Job *job, Error **errp)
 
  out:
     notifier_with_return_remove(&s->before_write);
+    s->before_write.notify = NULL;
 
     /* wait until pending backup_do_cow() calls have completed */
     qemu_co_rwlock_wrlock(&s->flush_rwlock);
@@ -767,6 +772,15 @@ BlockJob *backup_job_create(const char *job_id, 
BlockDriverState *bs,
                        &error_abort);
     job->len = len;
 
+    /* Finally, install a write notifier that takes effect after job_start() */
+    backup_init_copy_bitmap(job);
+
+    QLIST_INIT(&job->inflight_reqs);
+    qemu_co_rwlock_init(&job->flush_rwlock);
+
+    job->before_write.notify = backup_before_write_notify;
+    bdrv_add_before_write_notifier(bs, &job->before_write);
+
     return &job->common;
 
  error:
diff --git a/include/qemu/job.h b/include/qemu/job.h
index 9e7cd1e4a0..733afb696b 100644
--- a/include/qemu/job.h
+++ b/include/qemu/job.h
@@ -394,6 +394,11 @@ void job_enter_cond(Job *job, bool(*fn)(Job *job));
  */
 void job_start(Job *job);
 
+/**
+ * job_started returns true if the @job has started.
+ */
+bool job_started(Job *job);
+
 /**
  * @job: The job to enter.
  *
diff --git a/job.c b/job.c
index 28dd48f8a5..745af659ff 100644
--- a/job.c
+++ b/job.c
@@ -243,7 +243,7 @@ bool job_is_completed(Job *job)
     return false;
 }
 
-static bool job_started(Job *job)
+bool job_started(Job *job)
 {
     return job->co;
 }
-- 
2.21.0