09.08.2019 23:13, John Snow wrote: > 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 <vsement...@virtuozzo.com> > Signed-off-by: John Snow <js...@redhat.com> > --- > 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; > + }
Hmm, sorry if it is a stupid question, I'm not good in multiprocessing and in Qemu iothreads.. job_started just reads job->co. If bs runs in iothread, and therefore write-notifier is in iothread, when job_start is called from main thread.. Is it guaranteed that write-notifier will see job->co variable change early enough to not miss guest write? Should not job->co be volatile for example or something like this? If not think about this patch looks good for me. > + > 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; > } > -- Best regards, Vladimir