On Mon, 07/13 19:14, John Snow wrote:
> > +static void backup_txn_commit(BlockJob *job)
> > +{
> > + BackupBlockJob *s = container_of(job, BackupBlockJob, common);
> > + if (s->sync_bitmap) {
> > + backup_handle_dirty_bitmap(s, 0);
> > + }
> > +}
> > +
> > +static void backup_txn_abort(BlockJob *job)
> > +{
> > + BackupBlockJob *s = container_of(job, BackupBlockJob, common);
> > + if (s->sync_bitmap) {
> > + backup_handle_dirty_bitmap(s, -1);
> > + }
> > +}
> > +
>
> If you're going to check for sync_bitmap out here in these two functions
> instead of inside backup_handle_dirty_bitmap, add an assertion into the
> called function that job->sync_bitmap *will* be present.
The assertion is not really necessary because we'll get a seg fault if it's
NULL.
>
> > static const BlockJobDriver backup_job_driver = {
> > .instance_size = sizeof(BackupBlockJob),
> > .job_type = BLOCK_JOB_TYPE_BACKUP,
> > .set_speed = backup_set_speed,
> > .iostatus_reset = backup_iostatus_reset,
> > + .txn_commit = backup_txn_commit,
> > + .txn_abort = backup_txn_abort,
> > };
> >
> > static BlockErrorAction backup_error_action(BackupBlockJob *job,
> > @@ -444,7 +462,7 @@ static void coroutine_fn backup_run(void *opaque)
> > qemu_co_rwlock_wrlock(&job->flush_rwlock);
> > qemu_co_rwlock_unlock(&job->flush_rwlock);
> >
> > - if (job->sync_bitmap) {
> > + if (!job->common.txn && job->sync_bitmap) {
> > backup_handle_dirty_bitmap(job, ret);
> > }
>
> Can we add a little call to blockjobs, like:
>
> bool block_cleanup_needed(BlockJob *job)
> {
> return job->common.txn == NULL;
> }
>
> To make this status check look less magical?
> Then, if the sync bitmap check is pushed into backup_handle, you can
> just simply say:
>
> if (blockjob_cleanup_needed(job)) {
> backup_handle_dirty_bitmap(job, ret);
> }
>
> which IMO is nicer.
Good idea.
>
> > hbitmap_free(job->bitmap);
> > @@ -463,7 +481,7 @@ void backup_start(BlockDriverState *bs,
> > BlockDriverState *target,
> > BlockdevOnError on_source_error,
> > BlockdevOnError on_target_error,
> > BlockCompletionFunc *cb, void *opaque,
> > - Error **errp)
> > + BlockJobTxn *txn, Error **errp)
> > {
> > int64_t len;
> >
> > @@ -545,6 +563,7 @@ void backup_start(BlockDriverState *bs,
> > BlockDriverState *target,
> > sync_bitmap : NULL;
> > job->common.len = len;
> > job->common.co = qemu_coroutine_create(backup_run);
> > + block_job_txn_add_job(txn, &job->common);
>
> OK, so all backup jobs will participate in the transaction -- but they
> can control this based on whether or not they pass forward the txn
> parameter.
It's a NOP if txn is NULL.
>
> > qemu_coroutine_enter(job->common.co, job);
> > return;
> >
