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

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