Re: [PATCH] qemu: fix nested job with driver lock held

[Laine Stump <laine@xxxxxxxxx>]

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);

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list