Re: [libvirt] PATCH: 16/28: Thread safety for storage driver
On Wed, Dec 03, 2008 at 10:59:20AM +0100, Daniel Veillard wrote:
On Sun, Nov 30, 2008 at 11:57:47PM +0000, Daniel P. Berrange wrote:
> This patch makes the storage driver thread safe
[...]
> static void
> storageDriverAutostart(virStorageDriverStatePtr driver) {
Hum, there is something I find incherent, in the network
side we don't lock the driver but we lock the individual
objects in the autostart. Here we don't lock anything. In both
case they are started at the end of the Startup method, which
does lock the driver. So it seems in the network case we
don't need to lock the individual objects unless I'm mistaken
That is a bug in this storage patch. We should be locking the
storage pools here, because someone could send us a SIGHUP
to do a reload while someone else is using a storage pool.
> @@ -172,11 +182,13 @@ storageDriverReload(void) {
> if (!driverState)
> return -1;
>
> + storageDriverLock(driverState);
> virStoragePoolLoadAllConfigs(NULL,
> &driverState->pools,
> driverState->configDir,
> driverState->autostartDir);
> storageDriverAutostart(driverState);
> + storageDriverUnlock(driverState);
>
> return 0;
Hum there is something potentially nasty here:
- suppose you call reload
- you lock the main driver
- you don't lock any of the individual objects
- another thread is busy on a long standing operation in one of the
storage objects
- reload still goes in, virStoragePoolDefParse creates a brand new
object and virStoragePoolObjAssignDef frees the object used by the
other thread.
Yes that is precisely the scenario we need the locking in autostart
for. I'll fix this patch....
I didn't find any locking in virStoragePoolObjAssignDef or
virStoragePoolLoadAllConfigs. Maybe on reload operation it's safer to
just lock the driver and all storage objects before reloading all configs
and autostarting.
There's two scenarios in AssignDef
- Updating config of an existing object. In this case
virStoragePoolObjFindByName will already have locked it for
us
- Creating a new object - in this case we call virStoragePoolObjLock
explicitly.
So, I believe AssignDef locking is OK here. LoadAllConfigs is also OK
because its use of objects all comes via AssignDef which returns a locked
object.
[...]
> @@ -393,6 +440,8 @@ storageListDefinedPools(virConnectPtr co
> return -1;
> }
>
> +/* This method is required to be re-entrant / thread safe, so
> + uses no driver lock */
Well virStorageBackendFindPoolSources prototype has no comment so it's
hard to guess...
Okay, there is a couple issues I raise here, I'm not sure if it's
misunderstanding or genuine problems though...
genuine bugs !
Daniel
--
|: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|