[libvirt] [PATCH 3/3] Implement non-pool-blocking volume allocation for FS volumes

[Daniel P. Berrange]

On Thu, Apr 16, 2009 at 03:57:52PM +0200, Daniel Veillard wrote:
> On Wed, Apr 15, 2009 at 03:26:44PM -0400, Cole Robinson wrote:
> > Implement non-pool-blocking volume allocation for FS volumes.
> > 
> > We setup the volume object, then drop the pool lock, and start the
> > actual storage allocation. Certain pool operations will be blocked
> > (start, stop, destroy, refresh), but the basic things like volume
> > listings will still work.
> > 
> > This also allows storage allocation progress reporting: the API user can
> > watch the volume object in a separate thread, polling it's 'info' for
> > update to date capacity/allocation reporting.
> 
>   Okay, that one is far more complex :-)

It looks sane to me though. This is pretty much spot on how I
imagined it would all work out.

> 
> [...]
> > +        /* Make a shallow copy of the 'defined' volume definition, since the
> > +         * original allocation value will change as the user polls 'info',
> > +         * but we only need the initial requested values
> > +         */
> > +        memcpy(buildvoldef, voldef, sizeof(*voldef));
> > +        voldef = NULL;
> > +
> > +        /* Drop the pool lock during volume allocation */
> > +        pool->asyncjobs++;
> > +        virStoragePoolObjUnlock(pool);
> > +
> > +        buildvoldef->building = 1;
> > +        buildret = backend->buildVol(obj->conn, buildvoldef);
> > +        buildvoldef->building = 0;
> > +
> > +        virStoragePoolObjLock(pool);
> > +        pool->asyncjobs--;
> 
>   since buildvoldef->building is a condition, maybe it's safer to (un)set
> it while the lock is taken.

This bit doesn't entirely make sense to me. 

The 'buildvoldef' object here is a copy of the real volume defintion
that only this thread is using. Anyone else polling on virStorageVolGetInfo
will be accessing th real def and thus not see the fact that you changed
thee 'building' flag.

I'm thinking you instead meant to set  'voldef->building', and setting
that flag should be protected by the pool lock as DV mentions.  Would
also need to remove the 'voldef= NULL' line if we're going to set the
flag on it

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