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.
pgpaYnFbw1UsQ.pgp
Description: PGP signature