Re: [libvirt] [PATCH 3/3] qemu: blockjob: Synchronously update backing chain in XML on ABORT/PIVOT
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
signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 3/3] qemu: blockjob: Synchronously update backing chain in XML on ABORT/PIVOT
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
signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 3/3] qemu: blockjob: Synchronously update backing chain in XML on ABORT/PIVOT
When the synchronous pivot option is selected, libvirt would not update
the backing chain until the job was exitted. Some applications then
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
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(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 9324de0..cd6ee22 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -1289,9 +1289,22 @@ virDomainDiskDefNew(void)
if (VIR_ALLOC(ret) < 0)
return NULL;
+
if (VIR_ALLOC(ret->src) < 0)
-VIR_FREE(ret);
+goto error;
+
+if (virCondInit(&ret->blockJobSyncCond) < 0) {
+virReportSystemError(errno, "%s", _("Failed to initialize condition"));
+goto error;
+}
+
return ret;
+
+ error:
+virStorageSourceFree(ret->src);
+VIR_FREE(ret);
+
+return NULL;
}
@@ -1310,6 +1323,7 @@ virDomainDiskDefFree(virDomainDiskDefPtr def)
VIR_FREE(def->product);
VIR_FREE(def->domain_name);
virDomainDeviceInfoClear(&def->info);
+virCondDestroy(&def->blockJobSyncCond);
VIR_FREE(def);
}
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 608f61f..84e880a 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -685,6 +685,12 @@ struct _virDomainDiskDef {
int mirrorState; /* enum virDomainDiskMirrorState */
int mirrorJob; /* virDomainBlockJobType */
+/* for some synchronous block jobs, we need to notify the owner */
+virCond blockJobSyncCond;
+int blockJobType; /* type of the block job from the event */
+int blockJobStatus; /* status of the finished block job */
+bool blockJobSync; /* the block job needs synchronized termination */
+
struct {
unsigned int cylinders;
unsigned int heads;
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 257dea8..b37995b 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -16276,6 +16276,12 @@ qemuDomainBlockJobImpl(virDomainObjPtr vm,
goto endjob;
if (mode == BLOCK_JOB_ABORT) {
+if (async && !(flags & VIR_DOMAIN_BLOCK_JOB_ABORT_ASYNC)) {
+/* prepare state for event delivery */
+disk->blockJobStatus = -1;
+disk->blockJobSync = true;
+}
+
if ((flags & VIR_DOMAIN_BLOCK_JOB_ABORT_PIVOT) &&
!(async && disk->mirror)) {
virReportError(VIR_ERR_OPERATION_INVALID,
@@ -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. */
-while (1) {
-/* Poll every 50ms */
-static struct timespec ts = {
-.tv_sec = 0,
-.tv_nsec = 50 * 1000 * 1000ull };
-virDomainBlockJobInfo dummy;
-
-qemuDomainObjEnterMonitor(driver, vm);
-ret = qemuMonitorBlockJobInfo(priv->mon, device, &dummy, NULL);
-if (qemuDomainObjExitMonitor(driver, vm) < 0)
-ret = -1;
-
-if (ret <= 0)
-break;
-
-virObjectUnlock(vm);
-nanosleep(&ts, NULL);
-
-virObjectLock(vm);
-
-if (!virDomainObjIsActive(vm)) {
-virReportError(VIR_ERR_OPERATION_INVALID, "%s",
- _("domain is not running"));
-ret = -1;
-break;
+while (disk->blockJobStatus == -1 && disk->blockJobSync) {
+if (virCondWait(&disk->blockJobSyncCond, &vm->paren
