[libvirt] [PATCH 3/3] qemu: blockjob: Synchronously update backing chain in XML on ABORT/PIVOT

[Eric Blake]

On 03/30/2015 03:26 AM, Peter Krempa wrote:
> When the synchronous pivot option is selected, libvirt would not update
> the backing chain until the job was exitted. Some applications then

s/exitted/exited/

> received invalid data as their job serialized first.
> 
> This patch removes polling to wait for the ABORT/PIVOT job completion
> and replaces it with a condition. If a synchronous operation is
> requested the update of the XML is executed in the job of the caller of
> the synchronous request. Otherwise the monitor event callback uses a
> separate worker to update the backing chain with a new job.
> 
> This is a regression since 1a92c719101e5bfa6fe2b78006ad04c7f075ea28

unreleased, and therefore worth fixing during freeze.

> 
> When the ABORT job is finished synchronously you get the following call
> stack:
>  #0  qemuBlockJobEventProcess
>  #1  qemuDomainBlockJobImpl
>  #2  qemuDomainBlockJobAbort
>  #3  virDomainBlockJobAbort
> 
> While previously or while using the _ASYNC flag you'd get:
>  #0  qemuBlockJobEventProcess
>  #1  processBlockJobEvent
>  #2  qemuProcessEventHandler
>  #3  virThreadPoolWorker
> ---
>  src/conf/domain_conf.c  | 16 +++++++++++++++-
>  src/conf/domain_conf.h  |  6 ++++++
>  src/qemu/qemu_driver.c  | 45 +++++++++++++++++++--------------------------
>  src/qemu/qemu_process.c | 38 +++++++++++++++++++++++++-------------
>  4 files changed, 65 insertions(+), 40 deletions(-)
> 

> @@ -16389,37 +16395,24 @@ qemuDomainBlockJobImpl(virDomainObjPtr vm,
>                                                       status);
>              event2 = virDomainEventBlockJob2NewFromObj(vm, disk->dst, type,
>                                                         status);
> -        } else if (!(flags & VIR_DOMAIN_BLOCK_JOB_ABORT_ASYNC)) {
> +        } else if (disk->blockJobSync) {
>              /* XXX If the event reports failure, we should reflect
>               * that back into the return status of this API call.  */

Isn't this comment stale now?  That is, since you are waiting for the
event to free the condition, and we are listening for both success and
failure events, we can now tell if the event reports failure.

Otherwise, looks correct to me.  I hate that it is so close to the
release deadline, but I hate regressions more (which is why we have
freezes).  I'd feel better if you got a second review, if you can
wrangle one up in time, but here's my:

ACK.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 604 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20150330/8c8d32b9/attachment-0001.sig>