[libvirt] storage lock issues
John Ferlan
jferlan at redhat.com
Thu Nov 2 21:59:56 UTC 2017
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
>
More information about the libvir-list
mailing list