On 03/24/2015 06:06 AM, Erik Skultety wrote: > The update was originally part of the storageDriverAutostart function, s/update/checkPool > but the pools have to be checked earlier during initialization phase, so s/have/need > the 'checkPool' block has been moved to storagePoolUpdateAllState. move the checkPool block into storageDriverInitialize. This ensures activating pools earlier allowing domains using volumes from a pool to be able to find the volume during qemuProcessReconnect ... [e.g. picking up the commit message from 1/7 into here...] > Prior to this update virStoragePoolLoadAllConfigs and > virStoragePoolLoadAllState functions gather all existing pool in the > system, so first it is necessary to filter out the ones that are > inactive (only config XML found), then determine storage backends for > the rest of the pools and run checkPool on each one of them to update > their 'active' property. > I think you're trying to indicate prior to this function, the virStoragePoolLoadAllState and virStoragePoolLoadAllConfigs functions have generated a list all the existing configs (active or inactive)... The purpose of this function is to take any that were deemed active and ensure that the checkPool function is called along with the refreshPool function in order to ensure that the volumes within the pool are present when the domain reconnection occurs. > After update, pools have to be refreshed, otherwise the list of volumes s/update/checkPool > stays empty. Once again we need the connection, but all the > storage backends ignore this argument except for RBD, however RBD > doesn't support 'checkPool' callback, therefore it is safe to pass > connection as NULL pointer. > > Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1177733 > --- > src/storage/storage_driver.c | 73 ++++++++++++++++++++++++++++++++++++-------- > 1 file changed, 61 insertions(+), 12 deletions(-) > This one I'm struggling with... For file based pools there's mostly truth to the claim in the commit message; however, iSCSI and SCSI pools are a bit different. Perhaps along the same issues as rbd and even gluster. I guess I just have to research a bit more of any edge conditions and make sure the changes actually "work" for my iSCSI config... My fc_host config is a bit trickier since I'm sharing the host with someone else who I have to coordinate with (so I'll wait for the v2 patches before going down that path). > diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c > index d09acce..2899521 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; > + > + 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; > + } Redundant check with DriverAutostart and since that's the only place that calls us.... I think dropping the VIR_ERROR is fine... In fact I think: if ((backend = virStorageBackendForType(pool->def->type)) == NULL || !backend->checkPool) { virStoragepoolObjUnlock(pool); continue; } Then you can assume that whatever follows has a 'checkPool' function > + > + /* Backends which do not support 'checkPool' are considered > + * inactive by default. > + */ > + if (backend->checkPool && > + backend->checkPool(pool, &active) < 0) { See I understand why patch 1 removes the conn; however, I'm still not convinced we can do that - let's see if anyone else says anything. > + 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; > + } Could add a : if (!active) { virStoragePoolObjUnlock(pool); continue; } then the rest is just inline... > + > + /* 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; (ahem) I assume this only matters is active true... John > + virStoragePoolObjUnlock(pool); > + } > +} > + > +static void > storageDriverAutostart(void) > { > size_t i; > @@ -99,18 +157,7 @@ storageDriverAutostart(void) > 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) { > @@ -207,6 +254,8 @@ storageStateInitialize(bool privileged, > driver->autostartDir) < 0) > goto error; > > + storagePoolUpdateAllState(); > + > storageDriverUnlock(); > > ret = 0; > -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list