On 04/02/2015 06:10 AM, Erik Skultety wrote:
The 'checkPool' callback was originally part of the storageDriverAutostart function, but the pools need to be checked earlier during initialization phase, otherwise we can't start a domain which mountsa volume after daemon's
s/mountsa/mounts a/ s/after daemon's/after the libvirtd daemon/
restarted. This is because qemuProcessReconnect is called earlier than storageDriverAutostart. Therefore the 'checkPool' logic has been moved to storagePoolUpdateAllState which is called inside storageDriverInitialize.
We also need a valid 'conn' reference to be able to execute 'refreshPool' during initialization phase. Though it isn't available until storageDriverAutostart all of our storage backends do ignore 'conn' pointer, except for RBD, but RBD doesn't support 'checkPool' callback, so it's safe to pass conn = NULL in this case.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1177733
yes, this does belong in this one Also, as I'm going through my list of backlog bugzillas, I think that https://bugzilla.redhat.com/show_bug.cgi?id=1125805 can be duplicated to this bz.
--- src/storage/storage_driver.c | 74 ++++++++++++++++++++++++++++++++++++-------- 1 file changed, 61 insertions(+), 13 deletions(-)
diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 36c05b3..12e94ad 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -75,6 +75,64 @@ static void storageDriverUnlock(void) }
static void +storagePoolUpdateAllState(void) +{ + size_t i; + bool active = false;
Instead of initializing it here - do it in the code...
+ + for (i = 0; i < driver->pools.count; i++) { + virStoragePoolObjPtr pool = driver->pools.objs[i]; + virStorageBackendPtr backend; + + virStoragePoolObjLock(pool); + if (!virStoragePoolObjIsActive(pool)) { + virStoragePoolObjUnlock(pool); + continue; + } + + if ((backend = virStorageBackendForType(pool->def->type)) == NULL) { + VIR_ERROR(_("Missing backend %d"), pool->def->type); + virStoragePoolObjUnlock(pool); + continue; + } + + /* Backends which do not support 'checkPool' are considered + * inactive by default. + */ active = false;
So here's my reasoning for resetting active each time through the pool... What happens if pool [1] has a check and is determined to be active... then pool [2] doesn't have a check function, but active is still true from [1] - thus the refreshPool will happen. ACK with that adjustment. John
+ if (backend->checkPool && + backend->checkPool(pool, &active) < 0) { + virErrorPtr err = virGetLastError(); + VIR_ERROR(_("Failed to initialize storage pool '%s': %s"), + pool->def->name, err ? err->message : + _("no error message found")); + virStoragePoolObjUnlock(pool); + continue; + } + + /* We can pass NULL as connection, most backends do not use + * it anyway, but if they do and fail, we want to log error and + * continue with other pools. + */ + if (active) { + virStoragePoolObjClearVols(pool); + if (backend->refreshPool(NULL, pool) < 0) { + virErrorPtr err = virGetLastError(); + if (backend->stopPool) + backend->stopPool(NULL, pool); + VIR_ERROR(_("Failed to restart storage pool '%s': %s"), + pool->def->name, err ? err->message : + _("no error message found")); + virStoragePoolObjUnlock(pool); + continue; + } + } + + pool->active = active; + virStoragePoolObjUnlock(pool); + } +} + +static void storageDriverAutostart(void) { size_t i; @@ -95,23 +153,11 @@ storageDriverAutostart(void)
virStoragePoolObjLock(pool); if ((backend = virStorageBackendForType(pool->def->type)) == NULL) { - VIR_ERROR(_("Missing backend %d"), pool->def->type); virStoragePoolObjUnlock(pool); continue; }
- if (backend->checkPool && - backend->checkPool(pool, &started) < 0) { - virErrorPtr err = virGetLastError(); - VIR_ERROR(_("Failed to initialize storage pool '%s': %s"), - pool->def->name, err ? err->message : - _("no error message found")); - virStoragePoolObjUnlock(pool); - continue; - } - - if (!started && - pool->autostart && + if (pool->autostart && !virStoragePoolObjIsActive(pool)) { if (backend->startPool && backend->startPool(conn, pool) < 0) { @@ -215,6 +261,8 @@ storageStateInitialize(bool privileged, driver->autostartDir) < 0) goto error;
+ storagePoolUpdateAllState(); + storageDriverUnlock();
ret = 0;