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 :|
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list