[libvirt] [PATCH 0/3] Drop pool lock during volume allocation
The following three patches rearrange storage volume creation so that we can drop the pool lock during the long allocation process. This also has the nice side effect of allowing storage allocation progress reporting, if the API user polls the volume in a separate thread. This is currently only implemented for FS volumes: all other volume creation will maintain existing behavior. Thanks, Cole
Add an 'asyncjobs' counter to the storage pool definition. The counter tracks how many nonblocking jobs the pool is currently running, and prevents the operations destroy, refresh, undefine, and delete. Will be used for non-blocking volume creation. - Cole
On Wed, Apr 15, 2009 at 03:24:57PM -0400, Cole Robinson wrote:
Add an 'asyncjobs' counter to the storage pool definition. The counter tracks how many nonblocking jobs the pool is currently running, and prevents the operations destroy, refresh, undefine, and delete.
Will be used for non-blocking volume creation.
Patch looks clean, ACK, Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
On Wed, Apr 15, 2009 at 03:24:57PM -0400, Cole Robinson wrote:
Add an 'asyncjobs' counter to the storage pool definition. The counter tracks how many nonblocking jobs the pool is currently running, and prevents the operations destroy, refresh, undefine, and delete.
Will be used for non-blocking volume creation.
ACK 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 :|
Add 'building' boolean field to storage volume definition. If marked, the volume cannot be deleted. To be used for non-blocking volume allocation. - Cole
On Wed, Apr 15, 2009 at 03:26:02PM -0400, Cole Robinson wrote:
Add 'building' boolean field to storage volume definition.
If marked, the volume cannot be deleted. To be used for non-blocking volume allocation.
That one looks simple and clean too, ACK Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
On Wed, Apr 15, 2009 at 03:26:02PM -0400, Cole Robinson wrote:
Add 'building' boolean field to storage volume definition.
If marked, the volume cannot be deleted. To be used for non-blocking volume allocation.
ACK 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 :|
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. - Cole
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 :-) [...]
+ /* 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. I think the scheme works, but one thing we need to be careful about if that if adding new operations they should also look at that ->building condition. Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
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 :|
On 04/16/2009 12:24 PM, Daniel P. Berrange wrote:
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
Okay, attached patch sets the flag on the original volume definition, and makes sure we have the pool lock when we do so. This should address the above (and DV's) comments. Thanks, Cole
On Fri, Apr 17, 2009 at 12:33:30PM -0400, Cole Robinson wrote:
On 04/16/2009 12:24 PM, Daniel P. Berrange wrote:
On Thu, Apr 16, 2009 at 03:57:52PM +0200, Daniel Veillard wrote:
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
Okay, attached patch sets the flag on the original volume definition, and makes sure we have the pool lock when we do so. This should address the above (and DV's) comments.
This patch looks fine to me now, so ACK for the set of 3 patches, Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
On Wed, Apr 15, 2009 at 03:23:37PM -0400, Cole Robinson wrote:
The following three patches rearrange storage volume creation so that we can drop the pool lock during the long allocation process. This also has the nice side effect of allowing storage allocation progress reporting, if the API user polls the volume in a separate thread. This is currently only implemented for FS volumes: all other volume creation will maintain existing behavior.
Okay, all commited to CVS, thanks ! Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
participants (3)
-
Cole Robinson -
Daniel P. Berrange -
Daniel Veillard