[libvirt] [PATCH] qemu: fix nested job with driver lock held
Laine Stump
laine at laine.org
Thu Jul 28 03:58:37 UTC 2011
On 07/27/2011 10:04 PM, Eric Blake wrote:
> qemuMigrationUpdateJobStatus (called in a loop by migration
> and save tasks) uses qemuDomainObjEnterMonitorWithDriver;
> however, that function ended up starting a nested job without
> releasing the driver.
>
> Since no one else is making nested calls, we can inline the
> internal functions to properly track driver_locked.
ACK.
> * src/qemu/qemu_domain.h (qemuDomainObjBeginNestedJob)
> (qemuDomainObjBeginNestedJobWithDriver)
> (qemuDomainObjEndNestedJob): Drop unused prototypes.
> * src/qemu/qemu_domain.c (qemuDomainObjEnterMonitorInternal):
> Reflect driver lock to nested job.
> (qemuDomainObjBeginNestedJob)
> (qemuDomainObjBeginNestedJobWithDriver)
> (qemuDomainObjEndNestedJob): Drop unused functions.
> ---
>
> This does not solve the crash in 'virsh managedsave', but hopefully
> will make analysis easier in finding where we are freeing the monitor
> too early when probing to see if outgoing migration is completed.
>
> src/qemu/qemu_domain.c | 59 +++++++++++------------------------------------
> src/qemu/qemu_domain.h | 9 -------
> 2 files changed, 14 insertions(+), 54 deletions(-)
>
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index fe88ce3..2eaaf3a 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -832,31 +832,6 @@ int qemuDomainObjBeginAsyncJobWithDriver(struct qemud_driver *driver,
> }
>
> /*
> - * Use this to protect monitor sections within active async job.
> - *
> - * The caller must call qemuDomainObjBeginAsyncJob{,WithDriver} before it can
> - * use this method. Never use this method if you only own non-async job, use
> - * qemuDomainObjBeginJob{,WithDriver} instead.
> - */
> -int
> -qemuDomainObjBeginNestedJob(struct qemud_driver *driver,
> - virDomainObjPtr obj)
> -{
> - return qemuDomainObjBeginJobInternal(driver, false, obj,
> - QEMU_JOB_ASYNC_NESTED,
> - QEMU_ASYNC_JOB_NONE);
> -}
> -
> -int
> -qemuDomainObjBeginNestedJobWithDriver(struct qemud_driver *driver,
> - virDomainObjPtr obj)
> -{
> - return qemuDomainObjBeginJobInternal(driver, true, obj,
> - QEMU_JOB_ASYNC_NESTED,
> - QEMU_ASYNC_JOB_NONE);
> -}
> -
> -/*
> * obj must be locked before calling, qemud_driver does not matter
> *
> * To be called after completing the work associated with the
> @@ -888,21 +863,6 @@ qemuDomainObjEndAsyncJob(struct qemud_driver *driver, virDomainObjPtr obj)
> return virDomainObjUnref(obj);
> }
>
> -void
> -qemuDomainObjEndNestedJob(struct qemud_driver *driver, virDomainObjPtr obj)
> -{
> - qemuDomainObjPrivatePtr priv = obj->privateData;
> -
> - qemuDomainObjResetJob(priv);
> - qemuDomainObjSaveJob(driver, obj);
> - virCondSignal(&priv->job.cond);
> -
> - /* safe to ignore since the surrounding async job increased the reference
> - * counter as well */
> - ignore_value(virDomainObjUnref(obj));
> -}
> -
> -
> static int ATTRIBUTE_NONNULL(1)
> qemuDomainObjEnterMonitorInternal(struct qemud_driver *driver,
> bool driver_locked,
> @@ -911,7 +871,9 @@ qemuDomainObjEnterMonitorInternal(struct qemud_driver *driver,
> qemuDomainObjPrivatePtr priv = obj->privateData;
>
> if (priv->job.active == QEMU_JOB_NONE&& priv->job.asyncJob) {
> - if (qemuDomainObjBeginNestedJob(driver, obj)< 0)
> + if (qemuDomainObjBeginJobInternal(driver, driver_locked, obj,
> + QEMU_JOB_ASYNC_NESTED,
> + QEMU_ASYNC_JOB_NONE)< 0)
> return -1;
> if (!virDomainObjIsActive(obj)) {
> qemuReportError(VIR_ERR_OPERATION_FAILED, "%s",
> @@ -952,8 +914,15 @@ qemuDomainObjExitMonitorInternal(struct qemud_driver *driver,
> priv->mon = NULL;
> }
>
> - if (priv->job.active == QEMU_JOB_ASYNC_NESTED)
> - qemuDomainObjEndNestedJob(driver, obj);
> + if (priv->job.active == QEMU_JOB_ASYNC_NESTED) {
> + qemuDomainObjResetJob(priv);
> + qemuDomainObjSaveJob(driver, obj);
> + virCondSignal(&priv->job.cond);
> +
> + /* safe to ignore since the surrounding async job increased
> + * the reference counter as well */
> + ignore_value(virDomainObjUnref(obj));
> + }
> }
>
> /*
> @@ -962,7 +931,7 @@ qemuDomainObjExitMonitorInternal(struct qemud_driver *driver,
> * To be called immediately before any QEMU monitor API call
> * Must have already either called qemuDomainObjBeginJob() and checked
> * that the VM is still active or called qemuDomainObjBeginAsyncJob, in which
> - * case this will call qemuDomainObjBeginNestedJob.
> + * case this will start a nested job.
> *
> * To be followed with qemuDomainObjExitMonitor() once complete
> */
> @@ -988,7 +957,7 @@ void qemuDomainObjExitMonitor(struct qemud_driver *driver,
> * To be called immediately before any QEMU monitor API call
> * Must have already either called qemuDomainObjBeginJobWithDriver() and
> * checked that the VM is still active or called qemuDomainObjBeginAsyncJob,
> - * in which case this will call qemuDomainObjBeginNestedJobWithDriver.
> + * in which case this will start a nested job.
> *
> * To be followed with qemuDomainObjExitMonitorWithDriver() once complete
> */
> diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
> index 679259f..8bff8b0 100644
> --- a/src/qemu/qemu_domain.h
> +++ b/src/qemu/qemu_domain.h
> @@ -143,9 +143,6 @@ int qemuDomainObjBeginAsyncJob(struct qemud_driver *driver,
> virDomainObjPtr obj,
> enum qemuDomainAsyncJob asyncJob)
> ATTRIBUTE_RETURN_CHECK;
> -int qemuDomainObjBeginNestedJob(struct qemud_driver *driver,
> - virDomainObjPtr obj)
> - ATTRIBUTE_RETURN_CHECK;
> int qemuDomainObjBeginJobWithDriver(struct qemud_driver *driver,
> virDomainObjPtr obj,
> enum qemuDomainJob job)
> @@ -154,9 +151,6 @@ int qemuDomainObjBeginAsyncJobWithDriver(struct qemud_driver *driver,
> virDomainObjPtr obj,
> enum qemuDomainAsyncJob asyncJob)
> ATTRIBUTE_RETURN_CHECK;
> -int qemuDomainObjBeginNestedJobWithDriver(struct qemud_driver *driver,
> - virDomainObjPtr obj)
> - ATTRIBUTE_RETURN_CHECK;
>
> int qemuDomainObjEndJob(struct qemud_driver *driver,
> virDomainObjPtr obj)
> @@ -164,9 +158,6 @@ int qemuDomainObjEndJob(struct qemud_driver *driver,
> int qemuDomainObjEndAsyncJob(struct qemud_driver *driver,
> virDomainObjPtr obj)
> ATTRIBUTE_RETURN_CHECK;
> -void qemuDomainObjEndNestedJob(struct qemud_driver *driver,
> - virDomainObjPtr obj);
> -
> void qemuDomainObjSetJobPhase(struct qemud_driver *driver,
> virDomainObjPtr obj,
> int phase);
More information about the libvir-list
mailing list