Re: [Qemu-devel] [PATCH 07/10] block/backup: support block job transactions

[Stefan Hajnoczi <stefanha@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3110 bytes --]

On Tue, Jun 30, 2015 at 11:52:54AM -0400, John Snow wrote:
> On 06/30/2015 11:27 AM, Stefan Hajnoczi wrote:
> > On Mon, Jun 29, 2015 at 06:39:08PM -0400, John Snow wrote:
> >> On 06/25/2015 08:12 AM, Stefan Hajnoczi wrote:
> >>> @@ -537,6 +539,9 @@ void backup_start(BlockDriverState *bs, BlockDriverState *target,
> >>>      job->sync_mode = sync_mode;
> >>>      job->sync_bitmap = sync_mode == MIRROR_SYNC_MODE_DIRTY_BITMAP ?
> >>>                         sync_bitmap : NULL;
> >>> +    if (job->sync_bitmap) {
> >>> +        block_job_txn_add_job(txn, &job->common);
> >>> +    }
> >>
> >> Hmm, is this what we want? This will add backup jobs to a transaction
> >> only if they have a bitmap attached to the job.
> >>
> >> However, if we're doing a mixture of full and incremental backups, we
> >> may still want to roll back the incremental backups if the full backups
> >> failed as part of the transaction.
> >>
> >> The (admittedly more complicated) design I submitted will always add a
> >> job to the transactional group, whether it has a bitmap or not. The
> >> membership test was only if it was launched by the backup transaction
> >> action. The bitmap is only checked for purposes of refcounting and
> >> cleanup mechanics.
> >>
> >> Maybe that wasn't what we wanted either, but this is a difference in how
> >> our series will behave.
> > 
> > The 'backup' operation was added to the QMP 'transaction' command in
> > QEMU 1.6.  If we add non-incremental backup commands to the transaction
> > then behavior changes.
> > 
> 
> Ugh, good point...
> 
> > Perhaps DriveBackup and BlockdevBackup QAPI structures should take an
> > optional 'transaction' bool argument.  That way the caller decides which
> > behavior to use.
> > 
> 
> The way my version operated only changed the cleanup behavior -- it
> didn't attempt to cancel other jobs if they failed or not. It naively
> let them finish, then performed cleanup based on the overall completion
> status.
> 
> That let the old behavior continue working like it did, but changed how
> incrementals worked upon completion.
> 
> (1) Perhaps we can change the forced cancellation aspect and just allow
> jobs to finish naturally even in the event of failure. It's wasteful,
> but it does allow us to maintain the existing behavior while getting the
> behavior we want for incremental transactions.
> 
> (2) Or, yes, add some sort of "all or nothing" flag to transactions(?*)
> that users can toggle on/off. I had wondered in the past if it wouldn't
> be advantageous for libvirt to be able to choose this behavior, if
> managing state of partial completions was desirable in some cases to
> avoid re-running operations unnecessarily.
> 
> *As a thought, perhaps cancel-all-on-error as a flag can be a property
> of the QMP transaction command itself. When set, actions that launch
> jobs can add the job to the TXN. An error can be raised if the flag is
> used in conjunction with an action that doesn't currently/won't ever
> support the do-or-die flag.

Good, this is easy to add.

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]