[libvirt] PATCH: 16/28: Thread safety for storage driver

[Daniel P. Berrange]

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 :|