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