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

Peter Krempa pkrempa at redhat.com
Tue Mar 31 12:04:15 UTC 2015


On Mon, Mar 30, 2015 at 14:51:08 -0600, Eric Blake wrote:
> 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.

No it is not. The code currently does not change the return value
depending on the success state of the operation. I wanted not to change
semantics in this case.

I'm going to follow up to this series with a series that refactors
qemuDomainBlockJobImpl by splitting it into seprate functions instead of
that spaghetti monster of function it is now.

In that change I'll then try to fix the semantics.

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

Thanks.

Peter
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20150331/5a17f23a/attachment-0001.sig>


More information about the libvir-list mailing list