Re: [libvirt] [PATCH] qemu: Allow migration to be cancelled at any phase

On 07.11.2012 23:23, Jiri Denemark wrote: > > AbortJob API was never a synchronous one and I don't think we need to change > it. Not to mention that this code unlocks and locks vm without holding an > extra reference to it. And even if it did so, it cannot guarantee that the job > it's checking in the loop is the same one it tried to cancel. It's quite > unlikely but the original job could have finished and another one could have > been started while this code was sleeping. However, AbortJob is documented as synchronous. If current implementation doesn't follow docs it is a bug. On the other hand, I don't recall anybody screaming about it so far. But that means nothing, right?

>> @@ -1172,6 +1172,12 @@ qemuMigrationUpdateJobStatus(struct qemud_driver *driver, >> } >> priv->job.info.timeElapsed -= priv->job.start; >> >> + if (qemuDomainObjAbortAsyncJobRequested(vm)) { >> + VIR_DEBUG("Migration abort requested. Translating " >> + "status to MIGRATION_STATUS_CANCELLED"); >> + status = QEMU_MONITOR_MIGRATION_STATUS_CANCELLED; >> + } >> + > > This check seems to be to late and that complicates the code later. If we keep > qemuDomainAbortJob calling qemuMonitorMigrateCancel in qemuDomainAbortJob, we > may count on that and check priv.job.asyncAbort before entering the monitor > to tell QEMU to start migrating in qemuMigrationRun. If the abort is not > requested by then, it may only happen after we call qemuMonitorMigrateTo* and > in that case the migration will be properly aborted by > qemuMonitorMigrateCancel. > Not really. The issue I've seen was like this: Thread A Thread B 1. start async migration out job 2. Since job is set, abort it by calling 'migrate_cancel' 3. return to user 4. issue 'migrate' on the monitor And I think your suggestion makes it just harder to hit this race and not really avoid it. However with my code, we are guaranteed that 'migrate_cancel' will have an effect.