[libvirt] [PATCH] qemu: fix crash when mixing sync and async monitor jobs

[Wen Congyang]

At 07/29/2011 09:34 PM, Eric Blake write:
> On 07/29/2011 02:59 AM, Wen Congyang wrote:
>> At 07/29/2011 07:47 AM, Eric Blake Write:
>>> Currently, we attempt to run sync job and async job at the same time. It
>>> means that the monitor commands for two jobs can be run in any order.
>>>
>>> In the function qemuDomainObjEnterMonitorInternal():
>>> if (priv->job.active == QEMU_JOB_NONE&& priv->job.asyncJob) {
>>> if (qemuDomainObjBeginNestedJob(driver, obj)< 0)
>>> We check whether the caller is an async job by priv->job.active and
>>> priv->job.asynJob. But when an async job is running, and a sync job is
>>> also running at the time of the check, then priv->job.active is not
>>> QEMU_JOB_NONE. So we cannot check whether the caller is an async job
>>> in the function qemuDomainObjEnterMonitorInternal(), and must instead
>>> put the burden on the caller to tell us when an async command wants
>>> to do a nested job.
>>> ---
>>>
>>> My initial smoke testing shows that this fixes 'virsh managedsave',
>>> but I still have more testing to do before I'm convinced I got
>>> everything (for example, I need to test migration still).
>>
>> I test this patch with save by virt-manager, and find that it will cause
>> libvirt to be deadlock.
>
> I'm seeing deadlock as well, in my further testing.
>
>>
>> With this patch, we can ignore the return value of
>> qemuDomainObjEnterMonitor(WithDriver),
>> because these two functions always return 0. But we can not ignore the
>> return value of qemuDomainObjEnterMonitorAsync().
>> If qemuDomainObjEnterMonitorAsync() failed, we do nothing in this
>> function.
>> So it's very dangerous to call qemuDomainObjExitMonitorWithDriver() when
>> qemuDomainObjEnterMonitorAsync() failed.
>>
>> I think this problem already exists before this patch.
>
> First, a meta-question - is the approach of this patch better than the
> approach of your patch (that is, this patch was attempting to make the
> sync job condvar be the only condition used for starting a monitor
> command, and the async job condvar merely ensures that only one async
> job can be run at once and that an async monitor command corresponds to
> the current async job)? Or is this patch beyond hope with its deadlock
> problems, so that we should go with your patch (adding a new condvar in
> the monitor to allow both sync job and async job to request a monitor
> command, with the monitor doing the serialization)?

First, I think your patch is better.

The reason of the deadlock problem is that: we ignore the return value of
qemuDomainObjEnterMonitor(WithDriver). Without your patch, it's very very
dangerous to ignore it if the job is async job.

With your patch, qemuDomainObjEnterMonitor(WithDriver) is changed to 
qemuDomainObjEnterMonitorAsync()
if the job is async job. So with your patch, we can ignore 
qemuDomainObjEnterMonitor(WithDriver)'s
return value safely. But we must not ignore 
qemuDomainObjEnterMonitorAsync()'s return value.

>
>>>
>>> -static int ATTRIBUTE_NONNULL(1)
>>> +static int
>>> qemuDomainObjEnterMonitorInternal(struct qemud_driver *driver,
>>> bool driver_locked,
>>> - virDomainObjPtr obj)
>>> + virDomainObjPtr obj,
>>> + enum qemuDomainAsyncJob asyncJob)
>>> {
>>> qemuDomainObjPrivatePtr priv = obj->privateData;
>>>
>>> - if (priv->job.active == QEMU_JOB_NONE&& priv->job.asyncJob) {
>>> + if (asyncJob != QEMU_ASYNC_JOB_NONE) {
>>> + if (asyncJob != priv->job.asyncJob) {
>>
>> When we recover a async job after livbirtd restart, priv->job.asyncJob
>> is QEMU_ASYNC_JOB_NONE,
>> because we call qemuDomainObjRestoreJob() to reset priv->job in the
>> function qemuProcessReconnect().
>
> Can that be fixed with a tweak to qemuDomainObjRestoreJob()?

Maybe. But I think we can use QEMU_ASYNC_JOB_NONE when we recover or
cancel a job.

>
>>
>>> + qemuReportError(VIR_ERR_INTERNAL_ERROR,
>>> + _("unepxected async job %d"), asyncJob);
>>> + return -1;
>>> + }
>>> 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",
>> _("domain is no longer running"));
>> return -1;
>> }
>>
>> if the domain is not active after calling
>> qemuDomainObjBeginJobInternal(), we set
>> priv->job.active to QEMU_JOB_ASYNC_NESTED, but we do not clear it and
>> notify the other
>> thread which is waiting priv->job.cond.
>
> Good catch.
>
>>> @@ -2424,7 +2430,8 @@ qemuProcessRecoverJob(struct qemud_driver *driver,
>>> reason == VIR_DOMAIN_PAUSED_SAVE) ||
>>> reason == VIR_DOMAIN_PAUSED_UNKNOWN)) {
>>> if (qemuProcessStartCPUs(driver, vm, conn,
>>> - VIR_DOMAIN_RUNNING_UNPAUSED)< 0) {
>>> + VIR_DOMAIN_RUNNING_UNPAUSED,
>>> + job->asyncJob)< 0) {
>>
>> As the above mentioned, we set priv->job.asynJob to
>> QEMU_ASYNC_JOB_NONE, and
>> priv->job.active is QEMU_JOB_MODIFY. I think we can use
>> QEMU_ASYNC_JOB_NONE
>> instead of job->asyncJob safely.
>
> I'll give it a shot.
>