[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] seg fault when running snapshot-create



On 04/22/2010 09:19 AM, Stephen Shaw wrote:
> On Thu, Apr 22, 2010 at 07:09, Daniel P. Berrange <berrange redhat com> wrote:
>> On Thu, Apr 22, 2010 at 08:54:30AM -0400, Chris Lalancette wrote:
>>> On 04/22/2010 08:34 AM, Daniel P. Berrange wrote:
>>>> On Wed, Apr 21, 2010 at 05:16:21PM -0400, Chris Lalancette wrote:
>>>>> On 04/21/2010 04:34 PM, Stephen Shaw wrote:
>>>>>> I'm getting a seg fault when running virsh snapshot-create 1, but only
>>>>>> when virt-manager is open and connected.
>>>>>>
>>>>>> Here is some of the debug info I was able to come up with -
>>>>>> http://fpaste.org/9GO6/ (bt)
>>>>>> http://fpaste.org/7gkH/ ('thread apply all bt)
>>>>>>
>>>>>> * After the crash
>>>>>> (gdb) p mon->msg
>>>>>> $1 = (qemuMonitorMessagePtr) 0x0
>>>>>>
>>>>>>
>>>>>> nibbler:~ # libvirtd --version
>>>>>> libvirtd (libvirt) 0.8.0
>>>>>>
>>>>>>
>>>>>> Please let me know if there is any other information you need.
>>>>>> Stephen
>>>>>
>>>>> Thanks for the report.  To be perfectly honest, I can't see how what
>>>>> happened could happen :).  But I'll take a closer look at it and see
>>>>> if I can reproduce and see what is going on with it.
>>>>
>>>> I see thread locking problems in the code
>>>>
>>>>  - qemuDomainSnapshotCreateXML() is calling monitor commands, but has
>>>>    not run  qemuDomainObjBeginJobWithDriver() to ensure exclusive
>>>>    access to the monitor
>>>>
>>>>  - qemuDomainSnapshotDiscard has same problem
>>>
>>> Yep, just fixing those now.  I didn't quite understand the ObjBeginJob
>>> before, but I think I'm understanding it now.  This is probably the source of
>>> the problems.
>>
>> There's some notes about the rules in src/qemu/THREADS.txt.
>>
>> You must acquire locks on objects in the following order, not missing
>> any steps.
>>
>>   qemud_driver             (qemudDriverLock)
>>   virDomainObjPtr          (implicit via virDomainObjFindByXXX)
>>   qemuMonitorPrivatePtr    (qemuDomainObjBeginJob)
>>   qemuMonitorPtr           (implicit via qemuDomainObjEnterMonitor)
>>
>>
>> Note that qemuDomainObjEnterMonitor() will release the locks on the
>> qemud_driver & virDomainObjPtr objects, once it has acquired the lock
>> on qemuMonitorPtr. The qemuMonitorPrivatePtr object has a condition
>> variable that ensures continued mutual exclusion, even though the
>> qemud_driver, virDomainObjPtr object are now unlocked.
>>
>> So by missing qemuDomainObjBeginJob(), the condition variable was not
>> acquired, and mutual exclusion was not assured
>>
>> Regards,
>> Daniel
> 
> Thanks for taking care of this.  As soon as the fix is checked in I'll
> rebuild libvirt and continue testing it.

I still have to go over the rules more completely, but the attached patch
seems to do more or less the right thing for me.  If you could give it a
quick test, that would be great.

Thanks,
-- 
Chris Lalancette
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 5f4adfd..4ec4bd6 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -10754,14 +10754,20 @@ static virDomainSnapshotPtr qemuDomainSnapshotCreateXML(virDomainPtr domain,
         }
     }
     else {
+        if (qemuDomainObjBeginJobWithDriver(driver, vm) < 0)
+            goto cleanup;
+
         priv = vm->privateData;
         qemuDomainObjEnterMonitorWithDriver(driver, vm);
         if (qemuMonitorCreateSnapshot(priv->mon, def->name) < 0) {
             qemuDomainObjExitMonitorWithDriver(driver, vm);
+            if (qemuDomainObjEndJob(vm) == 0)
+                vm = NULL;
             goto cleanup;
         }
         qemuDomainObjExitMonitorWithDriver(driver, vm);
-
+        if (qemuDomainObjEndJob(vm) == 0)
+            vm = NULL;
     }
 
     snap->def->state = vm->state;
@@ -11034,18 +11040,18 @@ static int qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
             rc = qemuMonitorLoadSnapshot(priv->mon, snap->def->name);
             qemuDomainObjExitMonitorWithDriver(driver, vm);
             if (rc < 0)
-                goto cleanup;
+                goto endjob;
         }
         else {
             if (qemuDomainSnapshotSetActive(vm, driver->snapshotDir) < 0)
-                goto cleanup;
+                goto endjob;
 
             rc = qemudStartVMDaemon(snapshot->domain->conn, driver, vm, NULL,
                                     -1);
             if (qemuDomainSnapshotSetInactive(vm, driver->snapshotDir) < 0)
-                goto cleanup;
+                goto endjob;
             if (rc < 0)
-                goto cleanup;
+                goto endjob;
         }
 
         if (snap->def->state == VIR_DOMAIN_PAUSED) {
@@ -11057,7 +11063,7 @@ static int qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
             rc = qemuMonitorStopCPUs(priv->mon);
             qemuDomainObjExitMonitorWithDriver(driver, vm);
             if (rc < 0)
-                goto cleanup;
+                goto endjob;
         }
 
         event = virDomainEventNewFromObj(vm,
@@ -11083,17 +11089,18 @@ static int qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
         }
 
         if (qemuDomainSnapshotSetActive(vm, driver->snapshotDir) < 0)
-            goto cleanup;
+            goto endjob;
     }
 
     vm->state = snap->def->state;
 
     ret = 0;
 
-cleanup:
-    if (vm && qemuDomainObjEndJob(vm) == 0)
+endjob:
+    if (qemuDomainObjEndJob(vm) == 0)
         vm = NULL;
 
+cleanup:
     if (event)
         qemuDomainEventQueue(driver, event);
     if (vm)
@@ -11247,6 +11254,9 @@ static int qemuDomainSnapshotDelete(virDomainSnapshotPtr snapshot,
         goto cleanup;
     }
 
+    if (qemuDomainObjBeginJobWithDriver(driver, vm) < 0)
+        goto cleanup;
+
     if (flags & VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN) {
         rem.driver = driver;
         rem.vm = vm;
@@ -11255,11 +11265,15 @@ static int qemuDomainSnapshotDelete(virDomainSnapshotPtr snapshot,
         virHashForEach(vm->snapshots.objs, qemuDomainSnapshotDiscardChildren,
                        &rem);
         if (rem.err < 0)
-            goto cleanup;
+            goto endjob;
     }
 
     ret = qemuDomainSnapshotDiscard(driver, vm, snap);
 
+endjob:
+    if (qemuDomainObjEndJob(vm) == 0)
+        vm = NULL;
+
 cleanup:
     if (vm)
         virDomainObjUnlock(vm);

[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]