On 11/02/2017 11:44 AM, Cedric Bosdonnat wrote: > On Thu, 2017-11-02 at 07:03 -0400, John Ferlan wrote: >> So how to fix - well seems to me the storageDriverLock in VolCreateXML >> may be the way since the refresh thread takes the driver lock first, >> then the pool object second. Perhaps even like the build code where it >> takes it temporarily while getting the pool object. I'd have to think a >> bit more about though. Still might be something to try since the Vol >> Refresh thread takes it while running... > > This is surely a bad hack, but this is fixing the problem I'm seeing. > Wouldn't the VolCreateXML function taking the lock for a too long time > and thus lead to other troubles? > This was my first gut reaction (that is the fix you chose), but I wanted to take the time to make sure paths being called wouldn't have deadlocks as well and of course to check history to ensure someone didn't remove the lock for a specific reason. You are correct it's a long time to hold that storageDriverLock, but the alternative is the crash you found. We hold that lock many other long running storage pool operations. BTW: A different solution could to follow how storageVolCreateXMLFrom does things. It only uses the lock around storage pool obj operations. While I'm thinking about it - Vol Delete perhaps should do some locking too. Again once the Storage/Vol code gets the common object and uses RW Locks - many of the existing DriverLock/Unlock won't be necessary because the storage pool and storage volume lists would be self locking. > ---- %< ---- > diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c > index 18d630319..f5a1e7bc2 100644 > --- a/src/storage/storage_driver.c > +++ b/src/storage/storage_driver.c > @@ -1825,6 +1825,7 @@ storageVolCreateXML(virStoragePoolPtr obj, > if (backend->createVol(obj->conn, pool, voldef) < 0) > goto cleanup; > > + storageDriverLock(); > pool->volumes.objs[pool->volumes.count++] = voldef; > volobj = virGetStorageVol(obj->conn, pool->def->name, voldef->name, > voldef->key, NULL, NULL); > @@ -1858,9 +1859,7 @@ storageVolCreateXML(virStoragePoolPtr obj, > > VIR_FREE(buildvoldef); > > - storageDriverLock(); > virStoragePoolObjLock(pool); > - storageDriverUnlock(); > > voldef->building = false; > pool->asyncjobs--; > @@ -1897,6 +1896,7 @@ storageVolCreateXML(virStoragePoolPtr obj, > voldef = NULL; > > cleanup: > + storageDriverUnlock(); If we took it before the PoolObj lock, then we'd need to Unlock after we unlock the pool obj... John > virObjectUnref(volobj); > virStorageVolDefFree(voldef); > if (pool) > ---- %< ---- > > -- > Cedric > -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
