Re: [Qemu-devel] [PATCH v3 12/15] block/backup: support block job transactions

[Fam Zheng <famz@redhat.com>]

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;
> >