[libvirt] [PATCH v1 00/11] Couple of storage driver improvements
Patches 02/11, 09/11 and finally 11/11 are actual bug fixes. The rest is a preparation work. More detailed: 02/11 - Fixes a refcounting issue (and thus invalid memory access) 09/11 - Fixes a problem where starting a transitive pool which has a persistent config may lead to it's disappearance 11/11 - Tries to relax locking to allow more concurrent access Michal Prívozník (11): virStoragePoolObjRemove: Don't unlock pool object upon return virStoragePoolObjListForEach: Grab a reference for pool object storagePoolRefreshImpl: Fix variable name in comment virStoragePoolUpdateInactive: Don't call virStoragePoolObjEndAPI virstorageobj: Rename virStoragePoolObjAssignDef virStoragePoolObjListAdd: Turn boolean arg into flags virStoragePoolObjListAdd: Separate out definition assignment virstorageobj: Introduce VIR_STORAGE_POOL_OBJ_LIST_ADD_LIVE flag storagePoolCreateXML: Don't lose persistent storage on failed create storage_driver: Protect pool def during startup and build storage: Drop and reacquire pool obj lock in some backends src/conf/virstorageobj.c | 89 ++++++++++++++++++++------ src/conf/virstorageobj.h | 17 ++++- src/libvirt_private.syms | 4 +- src/storage/storage_backend_disk.c | 11 +++- src/storage/storage_backend_fs.c | 13 +++- src/storage/storage_backend_logical.c | 15 +++-- src/storage/storage_backend_vstorage.c | 9 ++- src/storage/storage_backend_zfs.c | 7 +- src/storage/storage_driver.c | 67 +++++++++++-------- src/test/test_driver.c | 21 ++---- 10 files changed, 181 insertions(+), 72 deletions(-) -- 2.21.0
The fact that we're removing a pool object from the list of pools doesn't mean we want to unlock it. It violates locking policy too as object locking and unlocking is not done on the same level. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/virstorageobj.c | 5 ++--- src/storage/storage_driver.c | 17 +++-------------- src/test/test_driver.c | 14 +++----------- 3 files changed, 8 insertions(+), 28 deletions(-) diff --git a/src/conf/virstorageobj.c b/src/conf/virstorageobj.c index 31b5af8e9e..6af4a1a22d 100644 --- a/src/conf/virstorageobj.c +++ b/src/conf/virstorageobj.c @@ -514,7 +514,6 @@ virStoragePoolObjRemove(virStoragePoolObjListPtr pools, virObjectLock(obj); virHashRemoveEntry(pools->objs, uuidstr); virHashRemoveEntry(pools->objsName, obj->def->name); - virObjectUnlock(obj); virObjectUnref(obj); virObjectRWUnlock(pools); } @@ -1594,13 +1593,13 @@ virStoragePoolObjLoad(virStoragePoolObjListPtr pools, VIR_FREE(obj->configFile); /* for driver reload */ if (VIR_STRDUP(obj->configFile, path) < 0) { virStoragePoolObjRemove(pools, obj); - virObjectUnref(obj); + virStoragePoolObjEndAPI(&obj); return NULL; } VIR_FREE(obj->autostartLink); /* for driver reload */ if (VIR_STRDUP(obj->autostartLink, autostartLink) < 0) { virStoragePoolObjRemove(pools, obj); - virObjectUnref(obj); + virStoragePoolObjEndAPI(&obj); return NULL; } diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 496d51b1e0..c14620765e 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -125,8 +125,7 @@ virStoragePoolUpdateInactive(virStoragePoolObjPtr *objptr) if (!virStoragePoolObjGetConfigFile(obj)) { virStoragePoolObjRemove(driver->pools, obj); - virObjectUnref(obj); - *objptr = NULL; + virStoragePoolObjEndAPI(objptr); } else if (virStoragePoolObjGetNewDef(obj)) { virStoragePoolObjDefUseNewDef(obj); } @@ -760,12 +759,8 @@ storagePoolCreateXML(virConnectPtr conn, if (build_flags || (flags & VIR_STORAGE_POOL_CREATE_WITH_BUILD)) { - if (backend->buildPool(obj, build_flags) < 0) { - virStoragePoolObjRemove(driver->pools, obj); - virObjectUnref(obj); - obj = NULL; - goto cleanup; - } + if (backend->buildPool(obj, build_flags) < 0) + goto error; } } @@ -798,8 +793,6 @@ storagePoolCreateXML(virConnectPtr conn, error: virStoragePoolObjRemove(driver->pools, obj); - virObjectUnref(obj); - obj = NULL; goto cleanup; } @@ -835,8 +828,6 @@ storagePoolDefineXML(virConnectPtr conn, if (virStoragePoolObjSaveDef(driver, obj, newDef ? newDef : def) < 0) { virStoragePoolObjRemove(driver->pools, obj); - virObjectUnref(obj); - obj = NULL; newDef = NULL; goto cleanup; } @@ -903,8 +894,6 @@ storagePoolUndefine(virStoragePoolPtr pool) VIR_INFO("Undefining storage pool '%s'", def->name); virStoragePoolObjRemove(driver->pools, obj); - virObjectUnref(obj); - obj = NULL; ret = 0; cleanup: diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 7fd06fcfa8..a0f19f5c5c 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -4595,16 +4595,12 @@ testStoragePoolCreateXML(virConnectPtr conn, def->source.adapter.data.fchost.wwnn, def->source.adapter.data.fchost.wwpn) < 0) { virStoragePoolObjRemove(privconn->pools, obj); - virObjectUnref(obj); - obj = NULL; goto cleanup; } } if (testStoragePoolObjSetDefaults(obj) == -1) { virStoragePoolObjRemove(privconn->pools, obj); - virObjectUnref(obj); - obj = NULL; goto cleanup; } @@ -4662,8 +4658,6 @@ testStoragePoolDefineXML(virConnectPtr conn, if (testStoragePoolObjSetDefaults(obj) == -1) { virStoragePoolObjRemove(privconn->pools, obj); - virObjectUnref(obj); - obj = NULL; goto cleanup; } @@ -4692,7 +4686,7 @@ testStoragePoolUndefine(virStoragePoolPtr pool) 0); virStoragePoolObjRemove(privconn->pools, obj); - virObjectUnref(obj); + virStoragePoolObjEndAPI(&obj); virObjectEventStateQueue(privconn->eventState, event); return 0; @@ -4784,11 +4778,9 @@ testStoragePoolDestroy(virStoragePoolPtr pool) VIR_STORAGE_POOL_EVENT_STOPPED, 0); - if (!(virStoragePoolObjGetConfigFile(obj))) { + if (!(virStoragePoolObjGetConfigFile(obj))) virStoragePoolObjRemove(privconn->pools, obj); - virObjectUnref(obj); - obj = NULL; - } + ret = 0; cleanup: -- 2.21.0
On Fri, May 24, 2019 at 04:35:37PM +0200, Michal Privoznik wrote:
The fact that we're removing a pool object from the list of pools doesn't mean we want to unlock it. It violates locking policy too as object locking and unlocking is not done on the same level.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/virstorageobj.c | 5 ++--- src/storage/storage_driver.c | 17 +++-------------- src/test/test_driver.c | 14 +++----------- 3 files changed, 8 insertions(+), 28 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Turns out there's one callback that might remove a storage pool during its run: storagePoolUpdateAllState() call storagePoolUpdateStateCallback() which may call virStoragePoolUpdateInactive() which in turn may call virStoragePoolObjRemove(). Problem is that the UpdateStateCallback() sees a storage pool object with just two references: one for each hash table holding the object. If the function ends up calling ObjRemove() then upon removing the object from hash tables those references are gone and thus any subsequent call touching the object is invalid. The solution to this problem is to grab reference for the object we are running iterator with. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/virstorageobj.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/conf/virstorageobj.c b/src/conf/virstorageobj.c index 6af4a1a22d..286f55fb0c 100644 --- a/src/conf/virstorageobj.c +++ b/src/conf/virstorageobj.c @@ -411,9 +411,13 @@ virStoragePoolObjListForEachCb(void *payload, virStoragePoolObjPtr obj = payload; struct _virStoragePoolObjListForEachData *data = opaque; + /* Grab a reference so that we don't rely only on references grabbed by + * hash table earlier. Remember, an iterator can remove object from the + * hash table. */ + virObjectRef(obj); virObjectLock(obj); data->iter(obj, data->opaque); - virObjectUnlock(obj); + virStoragePoolObjEndAPI(&obj); return 0; } -- 2.21.0
On Fri, May 24, 2019 at 04:35:38PM +0200, Michal Privoznik wrote:
Turns out there's one callback that might remove a storage pool during its run: storagePoolUpdateAllState() call storagePoolUpdateStateCallback() which may call virStoragePoolUpdateInactive() which in turn may call virStoragePoolObjRemove(). Problem is that the UpdateStateCallback() sees a storage pool object with just two references: one for each hash table holding the object. If the function ends up calling ObjRemove() then upon removing the object from hash tables those references are gone and thus any subsequent call touching the object is invalid.
The solution to this problem is to grab reference for the object we are running iterator with.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/virstorageobj.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
The function comment mistakenly refers to 'poolptr' when in fact the variable is named 'objptr'. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/storage/storage_driver.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index c14620765e..4d26c94e66 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -112,7 +112,7 @@ storagePoolRefreshImpl(virStorageBackendPtr backend, /** * virStoragePoolUpdateInactive: - * @poolptr: pointer to a variable holding the pool object pointer + * @objptr: pointer to a variable holding the pool object pointer * * This function is supposed to be called after a pool becomes inactive. The * function switches to the new config object for persistent pools. Inactive -- 2.21.0
The commit summary mistakenly refers to 'storagePoolRefreshImpl' when in fact the function is named 'virStoragePoolUpdateInactive'. On Fri, May 24, 2019 at 04:35:39PM +0200, Michal Privoznik wrote:
The function comment mistakenly refers to 'poolptr' when in fact the variable is named 'objptr'.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/storage/storage_driver.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
With that fixed: Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
There is no need for this function to call virStoragePoolObjEndAPI(). The object is perfectly usable after return from this function. In fact, all callers will call virStoragePoolObjEndAPI() eventually. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/storage/storage_driver.c | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 4d26c94e66..5d3ab1b25f 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -112,20 +112,17 @@ storagePoolRefreshImpl(virStorageBackendPtr backend, /** * virStoragePoolUpdateInactive: - * @objptr: pointer to a variable holding the pool object pointer + * @obj: pool object * * This function is supposed to be called after a pool becomes inactive. The * function switches to the new config object for persistent pools. Inactive * pools are removed. */ static void -virStoragePoolUpdateInactive(virStoragePoolObjPtr *objptr) +virStoragePoolUpdateInactive(virStoragePoolObjPtr obj) { - virStoragePoolObjPtr obj = *objptr; - if (!virStoragePoolObjGetConfigFile(obj)) { virStoragePoolObjRemove(driver->pools, obj); - virStoragePoolObjEndAPI(objptr); } else if (virStoragePoolObjGetNewDef(obj)) { virStoragePoolObjDefUseNewDef(obj); } @@ -176,7 +173,7 @@ storagePoolUpdateStateCallback(virStoragePoolObjPtr obj, virStoragePoolObjSetActive(obj, active); if (!virStoragePoolObjIsActive(obj)) - virStoragePoolUpdateInactive(&obj); + virStoragePoolUpdateInactive(obj); return; } @@ -1076,7 +1073,7 @@ storagePoolDestroy(virStoragePoolPtr pool) virStoragePoolObjSetActive(obj, false); - virStoragePoolUpdateInactive(&obj); + virStoragePoolUpdateInactive(obj); ret = 0; @@ -1194,7 +1191,7 @@ storagePoolRefresh(virStoragePoolPtr pool, 0); virStoragePoolObjSetActive(obj, false); - virStoragePoolUpdateInactive(&obj); + virStoragePoolUpdateInactive(obj); goto cleanup; } -- 2.21.0
On Fri, May 24, 2019 at 04:35:40PM +0200, Michal Privoznik wrote:
There is no need for this function to call virStoragePoolObjEndAPI(). The object is perfectly usable after return from this function. In fact, all callers will call virStoragePoolObjEndAPI() eventually.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/storage/storage_driver.c | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
This function is doing much more than plain assigning pool definition to a pool object. Rename it. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/virstorageobj.c | 12 ++++++------ src/conf/virstorageobj.h | 6 +++--- src/libvirt_private.syms | 2 +- src/storage/storage_driver.c | 4 ++-- src/test/test_driver.c | 6 +++--- 5 files changed, 15 insertions(+), 15 deletions(-) diff --git a/src/conf/virstorageobj.c b/src/conf/virstorageobj.c index 286f55fb0c..7f25931d05 100644 --- a/src/conf/virstorageobj.c +++ b/src/conf/virstorageobj.c @@ -1506,7 +1506,7 @@ virStoragePoolObjSourceFindDuplicate(virStoragePoolObjListPtr pools, /** - * virStoragePoolObjAssignDef: + * virStoragePoolObjListAdd: * @pools: Storage Pool object list pointer * @def: Storage pool definition to add or update * @check_active: If true, ensure that pool is not active @@ -1517,9 +1517,9 @@ virStoragePoolObjSourceFindDuplicate(virStoragePoolObjListPtr pools, * Returns locked and reffed object pointer or NULL on error */ virStoragePoolObjPtr -virStoragePoolObjAssignDef(virStoragePoolObjListPtr pools, - virStoragePoolDefPtr def, - bool check_active) +virStoragePoolObjListAdd(virStoragePoolObjListPtr pools, + virStoragePoolDefPtr def, + bool check_active) { virStoragePoolObjPtr obj = NULL; char uuidstr[VIR_UUID_STRING_BUFLEN]; @@ -1590,7 +1590,7 @@ virStoragePoolObjLoad(virStoragePoolObjListPtr pools, return NULL; } - if (!(obj = virStoragePoolObjAssignDef(pools, def, false))) + if (!(obj = virStoragePoolObjListAdd(pools, def, false))) return NULL; def = NULL; @@ -1651,7 +1651,7 @@ virStoragePoolObjLoadState(virStoragePoolObjListPtr pools, } /* create the object */ - if (!(obj = virStoragePoolObjAssignDef(pools, def, true))) + if (!(obj = virStoragePoolObjListAdd(pools, def, true))) goto cleanup; def = NULL; diff --git a/src/conf/virstorageobj.h b/src/conf/virstorageobj.h index c41d4c16ad..090dd6a7e6 100644 --- a/src/conf/virstorageobj.h +++ b/src/conf/virstorageobj.h @@ -193,9 +193,9 @@ virStoragePoolObjVolumeListExport(virConnectPtr conn, virStoragePoolVolumeACLFilter filter); virStoragePoolObjPtr -virStoragePoolObjAssignDef(virStoragePoolObjListPtr pools, - virStoragePoolDefPtr def, - bool check_active); +virStoragePoolObjListAdd(virStoragePoolObjListPtr pools, + virStoragePoolDefPtr def, + bool check_active); int virStoragePoolObjSaveDef(virStorageDriverStatePtr driver, diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 909975750c..8fb366d218 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1178,7 +1178,6 @@ virSecretObjSetValueSize; # conf/virstorageobj.h virStoragePoolObjAddVol; -virStoragePoolObjAssignDef; virStoragePoolObjClearVols; virStoragePoolObjDecrAsyncjobs; virStoragePoolObjDefUseNewDef; @@ -1197,6 +1196,7 @@ virStoragePoolObjGetVolumesCount; virStoragePoolObjIncrAsyncjobs; virStoragePoolObjIsActive; virStoragePoolObjIsAutostart; +virStoragePoolObjListAdd; virStoragePoolObjListExport; virStoragePoolObjListForEach; virStoragePoolObjListNew; diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 5d3ab1b25f..b4a56e54bb 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -743,7 +743,7 @@ storagePoolCreateXML(virConnectPtr conn, if ((backend = virStorageBackendForType(newDef->type)) == NULL) goto cleanup; - if (!(obj = virStoragePoolObjAssignDef(driver->pools, newDef, true))) + if (!(obj = virStoragePoolObjListAdd(driver->pools, newDef, true))) goto cleanup; newDef = NULL; def = virStoragePoolObjGetDef(obj); @@ -818,7 +818,7 @@ storagePoolDefineXML(virConnectPtr conn, if (virStorageBackendForType(newDef->type) == NULL) goto cleanup; - if (!(obj = virStoragePoolObjAssignDef(driver->pools, newDef, false))) + if (!(obj = virStoragePoolObjListAdd(driver->pools, newDef, false))) goto cleanup; newDef = virStoragePoolObjGetNewDef(obj); def = virStoragePoolObjGetDef(obj); diff --git a/src/test/test_driver.c b/src/test/test_driver.c index a0f19f5c5c..832943c1fb 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -1095,7 +1095,7 @@ testParseStorage(testDriverPtr privconn, if (!def) return -1; - if (!(obj = virStoragePoolObjAssignDef(privconn->pools, def, false))) { + if (!(obj = virStoragePoolObjListAdd(privconn->pools, def, false))) { virStoragePoolDefFree(def); return -1; } @@ -4581,7 +4581,7 @@ testStoragePoolCreateXML(virConnectPtr conn, if (!(newDef = virStoragePoolDefParseString(xml))) goto cleanup; - if (!(obj = virStoragePoolObjAssignDef(privconn->pools, newDef, true))) + if (!(obj = virStoragePoolObjListAdd(privconn->pools, newDef, true))) goto cleanup; newDef = NULL; def = virStoragePoolObjGetDef(obj); @@ -4647,7 +4647,7 @@ testStoragePoolDefineXML(virConnectPtr conn, newDef->allocation = defaultPoolAlloc; newDef->available = defaultPoolCap - defaultPoolAlloc; - if (!(obj = virStoragePoolObjAssignDef(privconn->pools, newDef, false))) + if (!(obj = virStoragePoolObjListAdd(privconn->pools, newDef, false))) goto cleanup; newDef = NULL; def = virStoragePoolObjGetDef(obj); -- 2.21.0
On Fri, May 24, 2019 at 04:35:41PM +0200, Michal Privoznik wrote:
This function is doing much more than plain assigning pool definition to a pool object. Rename it.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/virstorageobj.c | 12 ++++++------ src/conf/virstorageobj.h | 6 +++--- src/libvirt_private.syms | 2 +- src/storage/storage_driver.c | 4 ++-- src/test/test_driver.c | 6 +++--- 5 files changed, 15 insertions(+), 15 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
There will be more boolean information that we want to pass to this function. Instead of having them in separate arguments per each one, use @flags. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/virstorageobj.c | 16 +++++++++++----- src/conf/virstorageobj.h | 6 +++++- src/storage/storage_driver.c | 5 +++-- src/test/test_driver.c | 7 ++++--- 4 files changed, 23 insertions(+), 11 deletions(-) diff --git a/src/conf/virstorageobj.c b/src/conf/virstorageobj.c index 7f25931d05..7515b5d107 100644 --- a/src/conf/virstorageobj.c +++ b/src/conf/virstorageobj.c @@ -1509,17 +1509,20 @@ virStoragePoolObjSourceFindDuplicate(virStoragePoolObjListPtr pools, * virStoragePoolObjListAdd: * @pools: Storage Pool object list pointer * @def: Storage pool definition to add or update - * @check_active: If true, ensure that pool is not active + * @flags: bitwise-OR of VIR_STORAGE_POOL_OBJ_LIST_* flags * * Lookup the @def to see if it already exists in the @pools in order * to either update or add if it does not exist. * + * If VIR_STORAGE_POOL_OBJ_LIST_ADD_CHECK_LIVE is set in @flags + * then this will fail if the pool exists and is active. + * * Returns locked and reffed object pointer or NULL on error */ virStoragePoolObjPtr virStoragePoolObjListAdd(virStoragePoolObjListPtr pools, virStoragePoolDefPtr def, - bool check_active) + unsigned int flags) { virStoragePoolObjPtr obj = NULL; char uuidstr[VIR_UUID_STRING_BUFLEN]; @@ -1530,7 +1533,9 @@ virStoragePoolObjListAdd(virStoragePoolObjListPtr pools, if (virStoragePoolObjSourceFindDuplicate(pools, def) < 0) goto error; - rc = virStoragePoolObjIsDuplicate(pools, def, check_active, &obj); + rc = virStoragePoolObjIsDuplicate(pools, def, + !!(flags & VIR_STORAGE_POOL_OBJ_LIST_ADD_CHECK_LIVE), + &obj); if (rc < 0) goto error; @@ -1590,7 +1595,7 @@ virStoragePoolObjLoad(virStoragePoolObjListPtr pools, return NULL; } - if (!(obj = virStoragePoolObjListAdd(pools, def, false))) + if (!(obj = virStoragePoolObjListAdd(pools, def, 0))) return NULL; def = NULL; @@ -1651,7 +1656,8 @@ virStoragePoolObjLoadState(virStoragePoolObjListPtr pools, } /* create the object */ - if (!(obj = virStoragePoolObjListAdd(pools, def, true))) + if (!(obj = virStoragePoolObjListAdd(pools, def, + VIR_STORAGE_POOL_OBJ_LIST_ADD_CHECK_LIVE))) goto cleanup; def = NULL; diff --git a/src/conf/virstorageobj.h b/src/conf/virstorageobj.h index 090dd6a7e6..fe62515b50 100644 --- a/src/conf/virstorageobj.h +++ b/src/conf/virstorageobj.h @@ -192,10 +192,14 @@ virStoragePoolObjVolumeListExport(virConnectPtr conn, virStorageVolPtr **vols, virStoragePoolVolumeACLFilter filter); +enum { + VIR_STORAGE_POOL_OBJ_LIST_ADD_CHECK_LIVE = (1 << 1), +}; + virStoragePoolObjPtr virStoragePoolObjListAdd(virStoragePoolObjListPtr pools, virStoragePoolDefPtr def, - bool check_active); + unsigned int flags); int virStoragePoolObjSaveDef(virStorageDriverStatePtr driver, diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index b4a56e54bb..38b83a77b7 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -743,7 +743,8 @@ storagePoolCreateXML(virConnectPtr conn, if ((backend = virStorageBackendForType(newDef->type)) == NULL) goto cleanup; - if (!(obj = virStoragePoolObjListAdd(driver->pools, newDef, true))) + if (!(obj = virStoragePoolObjListAdd(driver->pools, newDef, + VIR_STORAGE_POOL_OBJ_LIST_ADD_CHECK_LIVE))) goto cleanup; newDef = NULL; def = virStoragePoolObjGetDef(obj); @@ -818,7 +819,7 @@ storagePoolDefineXML(virConnectPtr conn, if (virStorageBackendForType(newDef->type) == NULL) goto cleanup; - if (!(obj = virStoragePoolObjListAdd(driver->pools, newDef, false))) + if (!(obj = virStoragePoolObjListAdd(driver->pools, newDef, 0))) goto cleanup; newDef = virStoragePoolObjGetNewDef(obj); def = virStoragePoolObjGetDef(obj); diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 832943c1fb..39f5557fe1 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -1095,7 +1095,7 @@ testParseStorage(testDriverPtr privconn, if (!def) return -1; - if (!(obj = virStoragePoolObjListAdd(privconn->pools, def, false))) { + if (!(obj = virStoragePoolObjListAdd(privconn->pools, def, 0))) { virStoragePoolDefFree(def); return -1; } @@ -4581,7 +4581,8 @@ testStoragePoolCreateXML(virConnectPtr conn, if (!(newDef = virStoragePoolDefParseString(xml))) goto cleanup; - if (!(obj = virStoragePoolObjListAdd(privconn->pools, newDef, true))) + if (!(obj = virStoragePoolObjListAdd(privconn->pools, newDef, + VIR_STORAGE_POOL_OBJ_LIST_ADD_CHECK_LIVE))) goto cleanup; newDef = NULL; def = virStoragePoolObjGetDef(obj); @@ -4647,7 +4648,7 @@ testStoragePoolDefineXML(virConnectPtr conn, newDef->allocation = defaultPoolAlloc; newDef->available = defaultPoolCap - defaultPoolAlloc; - if (!(obj = virStoragePoolObjListAdd(privconn->pools, newDef, false))) + if (!(obj = virStoragePoolObjListAdd(privconn->pools, newDef, 0))) goto cleanup; newDef = NULL; def = virStoragePoolObjGetDef(obj); -- 2.21.0
On Fri, May 24, 2019 at 04:35:42PM +0200, Michal Privoznik wrote:
There will be more boolean information that we want to pass to this function. Instead of having them in separate arguments per each one, use @flags.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/virstorageobj.c | 16 +++++++++++----- src/conf/virstorageobj.h | 6 +++++- src/storage/storage_driver.c | 5 +++-- src/test/test_driver.c | 7 ++++--- 4 files changed, 23 insertions(+), 11 deletions(-)
diff --git a/src/conf/virstorageobj.h b/src/conf/virstorageobj.h index 090dd6a7e6..fe62515b50 100644 --- a/src/conf/virstorageobj.h +++ b/src/conf/virstorageobj.h @@ -192,10 +192,14 @@ virStoragePoolObjVolumeListExport(virConnectPtr conn, virStorageVolPtr **vols, virStoragePoolVolumeACLFilter filter);
+enum { + VIR_STORAGE_POOL_OBJ_LIST_ADD_CHECK_LIVE = (1 << 1), +}; +
typedef enum { } virStoragePoolObjListFlags; even though it will not be used, it is nice to name it. Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Separate storage pool definition assignment into a function. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/virstorageobj.c | 23 ++++++++++++++++------- 1 file changed, 16 insertions(+), 7 deletions(-) diff --git a/src/conf/virstorageobj.c b/src/conf/virstorageobj.c index 7515b5d107..2b9ad6fc98 100644 --- a/src/conf/virstorageobj.c +++ b/src/conf/virstorageobj.c @@ -1505,6 +1505,21 @@ virStoragePoolObjSourceFindDuplicate(virStoragePoolObjListPtr pools, } +static void +virStoragePoolObjAssignDef(virStoragePoolObjPtr obj, + virStoragePoolDefPtr def, + unsigned int flgs ATTRIBUTE_UNUSED) +{ + if (!virStoragePoolObjIsActive(obj)) { + virStoragePoolDefFree(obj->def); + obj->def = def; + } else { + virStoragePoolDefFree(obj->newDef); + obj->newDef = def; + } +} + + /** * virStoragePoolObjListAdd: * @pools: Storage Pool object list pointer @@ -1540,14 +1555,8 @@ virStoragePoolObjListAdd(virStoragePoolObjListPtr pools, if (rc < 0) goto error; if (rc > 0) { - if (!virStoragePoolObjIsActive(obj)) { - virStoragePoolDefFree(obj->def); - obj->def = def; - } else { - virStoragePoolDefFree(obj->newDef); - obj->newDef = def; - } virObjectRWUnlock(pools); + virStoragePoolObjAssignDef(obj, def, flags); return obj; } -- 2.21.0
On Fri, May 24, 2019 at 04:35:43PM +0200, Michal Privoznik wrote:
Separate storage pool definition assignment into a function.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/virstorageobj.c | 23 ++++++++++++++++------- 1 file changed, 16 insertions(+), 7 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
On 5/24/19 10:35 AM, Michal Privoznik wrote:
Separate storage pool definition assignment into a function.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/virstorageobj.c | 23 ++++++++++++++++------- 1 file changed, 16 insertions(+), 7 deletions(-)
diff --git a/src/conf/virstorageobj.c b/src/conf/virstorageobj.c index 7515b5d107..2b9ad6fc98 100644 --- a/src/conf/virstorageobj.c +++ b/src/conf/virstorageobj.c @@ -1505,6 +1505,21 @@ virStoragePoolObjSourceFindDuplicate(virStoragePoolObjListPtr pools, }
+static void +virStoragePoolObjAssignDef(virStoragePoolObjPtr obj, + virStoragePoolDefPtr def, + unsigned int flgs ATTRIBUTE_UNUSED) +{ + if (!virStoragePoolObjIsActive(obj)) { + virStoragePoolDefFree(obj->def); + obj->def = def; + } else { + virStoragePoolDefFree(obj->newDef); + obj->newDef = def; + } +} + + /** * virStoragePoolObjListAdd: * @pools: Storage Pool object list pointer @@ -1540,14 +1555,8 @@ virStoragePoolObjListAdd(virStoragePoolObjListPtr pools, if (rc < 0) goto error; if (rc > 0) { - if (!virStoragePoolObjIsActive(obj)) { - virStoragePoolDefFree(obj->def); - obj->def = def; - } else { - virStoragePoolDefFree(obj->newDef); - obj->newDef = def; - } virObjectRWUnlock(pools); + virStoragePoolObjAssignDef(obj, def, flags);
Was it intentional to swap the order? John
return obj; }
On 8/19/19 6:08 PM, John Ferlan wrote:
Was it intentional to swap the order?
That's a good question. Honestly, I don't recall. I'll fix it in my local branch. It looks like we should set the new pool definition with the pool list locked so that virStoragePoolObjIsDuplicate() and virStoragePoolObjSourceFindDuplicate() can return expected value if two threads were fighting. Michal
This flag can be used to denote that the definition we're trying to assign to a pool object is live definition and thus the inactive definition should be saved into ->newDef. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/virstorageobj.c | 19 ++++++++++++++----- src/conf/virstorageobj.h | 1 + 2 files changed, 15 insertions(+), 5 deletions(-) diff --git a/src/conf/virstorageobj.c b/src/conf/virstorageobj.c index 2b9ad6fc98..bdb167e9e2 100644 --- a/src/conf/virstorageobj.c +++ b/src/conf/virstorageobj.c @@ -1508,14 +1508,18 @@ virStoragePoolObjSourceFindDuplicate(virStoragePoolObjListPtr pools, static void virStoragePoolObjAssignDef(virStoragePoolObjPtr obj, virStoragePoolDefPtr def, - unsigned int flgs ATTRIBUTE_UNUSED) + unsigned int flags) { - if (!virStoragePoolObjIsActive(obj)) { - virStoragePoolDefFree(obj->def); - obj->def = def; - } else { + if (virStoragePoolObjIsActive(obj)) { virStoragePoolDefFree(obj->newDef); obj->newDef = def; + } else { + if (!obj->newDef && + flags & VIR_STORAGE_POOL_OBJ_LIST_ADD_LIVE) + VIR_STEAL_PTR(obj->newDef, obj->def); + + virStoragePoolDefFree(obj->def); + obj->def = def; } } @@ -1529,6 +1533,11 @@ virStoragePoolObjAssignDef(virStoragePoolObjPtr obj, * Lookup the @def to see if it already exists in the @pools in order * to either update or add if it does not exist. * + * Use VIR_STORAGE_POOL_OBJ_LIST_ADD_LIVE to denote that @def + * refers to an active definition and thus any possible inactive + * definition found should be saved to ->newDef (in case of + * future restore). + * * If VIR_STORAGE_POOL_OBJ_LIST_ADD_CHECK_LIVE is set in @flags * then this will fail if the pool exists and is active. * diff --git a/src/conf/virstorageobj.h b/src/conf/virstorageobj.h index fe62515b50..df699a84c5 100644 --- a/src/conf/virstorageobj.h +++ b/src/conf/virstorageobj.h @@ -193,6 +193,7 @@ virStoragePoolObjVolumeListExport(virConnectPtr conn, virStoragePoolVolumeACLFilter filter); enum { + VIR_STORAGE_POOL_OBJ_LIST_ADD_LIVE= (1 << 0), VIR_STORAGE_POOL_OBJ_LIST_ADD_CHECK_LIVE = (1 << 1), }; -- 2.21.0
On Fri, May 24, 2019 at 04:35:44PM +0200, Michal Privoznik wrote:
This flag can be used to denote that the definition we're trying to assign to a pool object is live definition and thus the inactive definition should be saved into ->newDef.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/virstorageobj.c | 19 ++++++++++++++----- src/conf/virstorageobj.h | 1 + 2 files changed, 15 insertions(+), 5 deletions(-)
diff --git a/src/conf/virstorageobj.c b/src/conf/virstorageobj.c index 2b9ad6fc98..bdb167e9e2 100644 --- a/src/conf/virstorageobj.c +++ b/src/conf/virstorageobj.c @@ -1508,14 +1508,18 @@ virStoragePoolObjSourceFindDuplicate(virStoragePoolObjListPtr pools, static void virStoragePoolObjAssignDef(virStoragePoolObjPtr obj, virStoragePoolDefPtr def, - unsigned int flgs ATTRIBUTE_UNUSED) + unsigned int flags) { - if (!virStoragePoolObjIsActive(obj)) { - virStoragePoolDefFree(obj->def); - obj->def = def; - } else { + if (virStoragePoolObjIsActive(obj)) { virStoragePoolDefFree(obj->newDef); obj->newDef = def; + } else { + if (!obj->newDef && + flags & VIR_STORAGE_POOL_OBJ_LIST_ADD_LIVE) + VIR_STEAL_PTR(obj->newDef, obj->def); + + virStoragePoolDefFree(obj->def); + obj->def = def; } }
@@ -1529,6 +1533,11 @@ virStoragePoolObjAssignDef(virStoragePoolObjPtr obj, * Lookup the @def to see if it already exists in the @pools in order * to either update or add if it does not exist. * + * Use VIR_STORAGE_POOL_OBJ_LIST_ADD_LIVE to denote that @def + * refers to an active definition and thus any possible inactive + * definition found should be saved to ->newDef (in case of + * future restore). + * * If VIR_STORAGE_POOL_OBJ_LIST_ADD_CHECK_LIVE is set in @flags * then this will fail if the pool exists and is active. * diff --git a/src/conf/virstorageobj.h b/src/conf/virstorageobj.h index fe62515b50..df699a84c5 100644 --- a/src/conf/virstorageobj.h +++ b/src/conf/virstorageobj.h @@ -193,6 +193,7 @@ virStoragePoolObjVolumeListExport(virConnectPtr conn, virStoragePoolVolumeACLFilter filter);
enum { + VIR_STORAGE_POOL_OBJ_LIST_ADD_LIVE= (1 << 0),
Missing space before '='.
VIR_STORAGE_POOL_OBJ_LIST_ADD_CHECK_LIVE = (1 << 1),
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
If there's a persistent storage and user tries to start a new one with the same name and UUID (e.g. to test new configuration) it may happen that upon failure we lose the persistent defintion. Fortunately, we don't remove it from the disk only from the internal list of the pools. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/storage/storage_driver.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 38b83a77b7..def4123b82 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -744,6 +744,7 @@ storagePoolCreateXML(virConnectPtr conn, goto cleanup; if (!(obj = virStoragePoolObjListAdd(driver->pools, newDef, + VIR_STORAGE_POOL_OBJ_LIST_ADD_LIVE | VIR_STORAGE_POOL_OBJ_LIST_ADD_CHECK_LIVE))) goto cleanup; newDef = NULL; @@ -790,7 +791,7 @@ storagePoolCreateXML(virConnectPtr conn, return pool; error: - virStoragePoolObjRemove(driver->pools, obj); + virStoragePoolUpdateInactive(obj); goto cleanup; } -- 2.21.0
On Fri, May 24, 2019 at 04:35:45PM +0200, Michal Privoznik wrote:
If there's a persistent storage and user tries to start a new one with the same name and UUID (e.g. to test new configuration) it may happen that upon failure we lose the persistent defintion. Fortunately, we don't remove it from the disk only from the internal list of the pools.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/storage/storage_driver.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
In near future the storage pool object lock will be released during startPool and buildPool callback (in some backends). But this means that another thread may acquire the pool object lock and change its definition rendering the former thread access not only stale definition but also access freed memory (virStoragePoolObjAssignDef() will free old def when setting a new one). One way out of this would be to have the pool appear as active because our code deals with obj->def and obj->newdef just fine. But we can't declare a pool as active if it's not started or still building up. Therefore, have a boolean flag that is very similar and forces virStoragePoolObjAssignDef() to store new definition in obj->newdef even for an inactive pool. In turn, we have to move the definition to correct place when unsetting the flag. But that's as easy as calling virStoragePoolUpdateInactive(). Technically speaking, change made to storageDriverAutostartCallback() is not needed because until storage driver is initialized no storage API can run therefore there can't be anyone wanting to change the pool's definition. But I'm doing the change there for consistency anyways. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/virstorageobj.c | 26 +++++++++++++++++++++++++- src/conf/virstorageobj.h | 6 ++++++ src/libvirt_private.syms | 2 ++ src/storage/storage_driver.c | 31 ++++++++++++++++++++++++++++++- 4 files changed, 63 insertions(+), 2 deletions(-) diff --git a/src/conf/virstorageobj.c b/src/conf/virstorageobj.c index bdb167e9e2..9abcac479e 100644 --- a/src/conf/virstorageobj.c +++ b/src/conf/virstorageobj.c @@ -86,6 +86,7 @@ struct _virStoragePoolObj { char *configFile; char *autostartLink; bool active; + bool starting; bool autostart; unsigned int asyncjobs; @@ -312,6 +313,21 @@ virStoragePoolObjSetActive(virStoragePoolObjPtr obj, } +void +virStoragePoolObjSetStarting(virStoragePoolObjPtr obj, + bool starting) +{ + obj->starting = starting; +} + + +bool +virStoragePoolObjIsStarting(virStoragePoolObjPtr obj) +{ + return obj->starting; +} + + bool virStoragePoolObjIsAutostart(virStoragePoolObjPtr obj) { @@ -1090,6 +1106,13 @@ virStoragePoolObjIsDuplicate(virStoragePoolObjListPtr pools, obj->def->name); goto cleanup; } + + if (virStoragePoolObjIsStarting(obj)) { + virReportError(VIR_ERR_OPERATION_INVALID, + _("pool '%s' is starting up"), + obj->def->name); + goto cleanup; + } } VIR_STEAL_PTR(*objRet, obj); @@ -1510,7 +1533,8 @@ virStoragePoolObjAssignDef(virStoragePoolObjPtr obj, virStoragePoolDefPtr def, unsigned int flags) { - if (virStoragePoolObjIsActive(obj)) { + if (virStoragePoolObjIsActive(obj) || + virStoragePoolObjIsStarting(obj)) { virStoragePoolDefFree(obj->newDef); obj->newDef = def; } else { diff --git a/src/conf/virstorageobj.h b/src/conf/virstorageobj.h index df699a84c5..7dfdf42b26 100644 --- a/src/conf/virstorageobj.h +++ b/src/conf/virstorageobj.h @@ -92,6 +92,12 @@ void virStoragePoolObjSetActive(virStoragePoolObjPtr obj, bool active); +void +virStoragePoolObjSetStarting(virStoragePoolObjPtr obj, + bool starting); +bool +virStoragePoolObjIsStarting(virStoragePoolObjPtr obj); + bool virStoragePoolObjIsAutostart(virStoragePoolObjPtr obj); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 8fb366d218..243e3179cf 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1196,6 +1196,7 @@ virStoragePoolObjGetVolumesCount; virStoragePoolObjIncrAsyncjobs; virStoragePoolObjIsActive; virStoragePoolObjIsAutostart; +virStoragePoolObjIsStarting; virStoragePoolObjListAdd; virStoragePoolObjListExport; virStoragePoolObjListForEach; @@ -1214,6 +1215,7 @@ virStoragePoolObjSetActive; virStoragePoolObjSetAutostart; virStoragePoolObjSetConfigFile; virStoragePoolObjSetDef; +virStoragePoolObjSetStarting; virStoragePoolObjVolumeGetNames; virStoragePoolObjVolumeListExport; diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index def4123b82..60bfa48e25 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -201,12 +201,14 @@ storageDriverAutostartCallback(virStoragePoolObjPtr obj, if (virStoragePoolObjIsAutostart(obj) && !virStoragePoolObjIsActive(obj)) { + + virStoragePoolObjSetStarting(obj, true); if (backend->startPool && backend->startPool(obj) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Failed to autostart storage pool '%s': %s"), def->name, virGetLastErrorMessage()); - return; + goto cleanup; } started = true; } @@ -225,6 +227,13 @@ storageDriverAutostartCallback(virStoragePoolObjPtr obj, virStoragePoolObjSetActive(obj, true); } } + + cleanup: + if (virStoragePoolObjIsStarting(obj)) { + if (!virStoragePoolObjIsActive(obj)) + virStoragePoolUpdateInactive(obj); + virStoragePoolObjSetStarting(obj, false); + } } @@ -750,6 +759,8 @@ storagePoolCreateXML(virConnectPtr conn, newDef = NULL; def = virStoragePoolObjGetDef(obj); + virStoragePoolObjSetStarting(obj, true); + if (backend->buildPool) { if (flags & VIR_STORAGE_POOL_CREATE_WITH_BUILD_OVERWRITE) build_flags |= VIR_STORAGE_POOL_BUILD_OVERWRITE; @@ -786,6 +797,11 @@ storagePoolCreateXML(virConnectPtr conn, pool = virGetStoragePool(conn, def->name, def->uuid, NULL, NULL); cleanup: + if (virStoragePoolObjIsStarting(obj)) { + if (!virStoragePoolObjIsActive(obj)) + virStoragePoolUpdateInactive(obj); + virStoragePoolObjSetStarting(obj, false); + } virObjectEventStateQueue(driver->storageEventState, event); virStoragePoolObjEndAPI(&obj); return pool; @@ -937,6 +953,8 @@ storagePoolCreate(virStoragePoolPtr pool, goto cleanup; } + virStoragePoolObjSetStarting(obj, true); + if (backend->buildPool) { if (flags & VIR_STORAGE_POOL_CREATE_WITH_BUILD_OVERWRITE) build_flags |= VIR_STORAGE_POOL_BUILD_OVERWRITE; @@ -972,6 +990,11 @@ storagePoolCreate(virStoragePoolPtr pool, ret = 0; cleanup: + if (virStoragePoolObjIsStarting(obj)) { + if (!virStoragePoolObjIsActive(obj)) + virStoragePoolUpdateInactive(obj); + virStoragePoolObjSetStarting(obj, false); + } virObjectEventStateQueue(driver->storageEventState, event); virStoragePoolObjEndAPI(&obj); return ret; @@ -1004,6 +1027,8 @@ storagePoolBuild(virStoragePoolPtr pool, goto cleanup; } + virStoragePoolObjSetStarting(obj, true); + if (backend->buildPool && backend->buildPool(obj, flags) < 0) goto cleanup; @@ -1016,6 +1041,10 @@ storagePoolBuild(virStoragePoolPtr pool, ret = 0; cleanup: + if (virStoragePoolObjIsStarting(obj)) { + virStoragePoolUpdateInactive(obj); + virStoragePoolObjSetStarting(obj, false); + } virObjectEventStateQueue(driver->storageEventState, event); virStoragePoolObjEndAPI(&obj); return ret; -- 2.21.0
On Fri, May 24, 2019 at 04:35:46PM +0200, Michal Privoznik wrote:
In near future the storage pool object lock will be released during startPool and buildPool callback (in some backends). But this means that another thread may acquire the pool object lock and change its definition rendering the former thread access not only stale definition but also access freed memory (virStoragePoolObjAssignDef() will free old def when setting a new one).
One way out of this would be to have the pool appear as active because our code deals with obj->def and obj->newdef just fine. But we can't declare a pool as active if it's not started or still building up. Therefore, have a boolean flag that is very similar and forces virStoragePoolObjAssignDef() to store new definition in obj->newdef even for an inactive pool. In turn, we have to move the definition to correct place when unsetting the flag. But that's as easy as calling virStoragePoolUpdateInactive().
Technically speaking, change made to storageDriverAutostartCallback() is not needed because until storage driver is initialized no storage API can run therefore there can't be anyone wanting to change the pool's definition. But I'm doing the change there for consistency anyways.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/virstorageobj.c | 26 +++++++++++++++++++++++++- src/conf/virstorageobj.h | 6 ++++++ src/libvirt_private.syms | 2 ++ src/storage/storage_driver.c | 31 ++++++++++++++++++++++++++++++- 4 files changed, 63 insertions(+), 2 deletions(-)
@@ -1090,6 +1106,13 @@ virStoragePoolObjIsDuplicate(virStoragePoolObjListPtr pools, obj->def->name); goto cleanup; } + + if (virStoragePoolObjIsStarting(obj)) { + virReportError(VIR_ERR_OPERATION_INVALID, + _("pool '%s' is starting up"), + obj->def->name); + goto cleanup; + } }
VIR_STEAL_PTR(*objRet, obj); diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index def4123b82..60bfa48e25 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -750,6 +759,8 @@ storagePoolCreateXML(virConnectPtr conn,
In the context not present in the mail there is:
if (!(obj = virStoragePoolObjListAdd(driver->pools, newDef, VIR_STORAGE_POOL_OBJ_LIST_ADD_LIVE | VIR_STORAGE_POOL_OBJ_LIST_ADD_CHECK_LIVE))) goto cleanup;
which goes through the code path above in virStoragePoolObjIsDuplicate that checks for starting storage pool,
newDef = NULL; def = virStoragePoolObjGetDef(obj);
+ virStoragePoolObjSetStarting(obj, true); + if (backend->buildPool) { if (flags & VIR_STORAGE_POOL_CREATE_WITH_BUILD_OVERWRITE) build_flags |= VIR_STORAGE_POOL_BUILD_OVERWRITE; @@ -786,6 +797,11 @@ storagePoolCreateXML(virConnectPtr conn, pool = virGetStoragePool(conn, def->name, def->uuid, NULL, NULL);
cleanup: + if (virStoragePoolObjIsStarting(obj)) { + if (!virStoragePoolObjIsActive(obj)) + virStoragePoolUpdateInactive(obj); + virStoragePoolObjSetStarting(obj, false); + } virObjectEventStateQueue(driver->storageEventState, event); virStoragePoolObjEndAPI(&obj); return pool; @@ -937,6 +953,8 @@ storagePoolCreate(virStoragePoolPtr pool, goto cleanup; }
however I don't see such a check here,
+ virStoragePoolObjSetStarting(obj, true); + if (backend->buildPool) { if (flags & VIR_STORAGE_POOL_CREATE_WITH_BUILD_OVERWRITE) build_flags |= VIR_STORAGE_POOL_BUILD_OVERWRITE; @@ -972,6 +990,11 @@ storagePoolCreate(virStoragePoolPtr pool, ret = 0;
cleanup: + if (virStoragePoolObjIsStarting(obj)) { + if (!virStoragePoolObjIsActive(obj)) + virStoragePoolUpdateInactive(obj); + virStoragePoolObjSetStarting(obj, false); + } virObjectEventStateQueue(driver->storageEventState, event); virStoragePoolObjEndAPI(&obj); return ret; @@ -1004,6 +1027,8 @@ storagePoolBuild(virStoragePoolPtr pool, goto cleanup; }
nor here.
+ virStoragePoolObjSetStarting(obj, true); + if (backend->buildPool && backend->buildPool(obj, flags) < 0) goto cleanup; @@ -1016,6 +1041,10 @@ storagePoolBuild(virStoragePoolPtr pool, ret = 0;
cleanup: + if (virStoragePoolObjIsStarting(obj)) { + virStoragePoolUpdateInactive(obj); + virStoragePoolObjSetStarting(obj, false); + } virObjectEventStateQueue(driver->storageEventState, event); virStoragePoolObjEndAPI(&obj); return ret;
Also, storagePoolDelete, storagePoolUndefine and storagePoolDestroy should be disallowed for pools that are starting. With those added: Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
On 5/24/19 10:35 AM, Michal Privoznik wrote:
In near future the storage pool object lock will be released during startPool and buildPool callback (in some backends). But this means that another thread may acquire the pool object lock and change its definition rendering the former thread access not only stale definition but also access freed memory (virStoragePoolObjAssignDef() will free old def when setting a new one).
One way out of this would be to have the pool appear as active because our code deals with obj->def and obj->newdef just fine. But we can't declare a pool as active if it's not started or still building up. Therefore, have a boolean flag that is very similar and forces virStoragePoolObjAssignDef() to store new definition in obj->newdef even for an inactive pool. In turn, we have to move the definition to correct place when unsetting the flag. But that's as easy as calling virStoragePoolUpdateInactive().
Technically speaking, change made to storageDriverAutostartCallback() is not needed because until storage driver is initialized no storage API can run therefore there can't be anyone wanting to change the pool's definition. But I'm doing the change there for consistency anyways.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/virstorageobj.c | 26 +++++++++++++++++++++++++- src/conf/virstorageobj.h | 6 ++++++ src/libvirt_private.syms | 2 ++ src/storage/storage_driver.c | 31 ++++++++++++++++++++++++++++++- 4 files changed, 63 insertions(+), 2 deletions(-)
[...]
diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index def4123b82..60bfa48e25 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -201,12 +201,14 @@ storageDriverAutostartCallback(virStoragePoolObjPtr obj,
if (virStoragePoolObjIsAutostart(obj) && !virStoragePoolObjIsActive(obj)) { + + virStoragePoolObjSetStarting(obj, true); if (backend->startPool && backend->startPool(obj) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Failed to autostart storage pool '%s': %s"), def->name, virGetLastErrorMessage()); - return; + goto cleanup; } started = true; } @@ -225,6 +227,13 @@ storageDriverAutostartCallback(virStoragePoolObjPtr obj, virStoragePoolObjSetActive(obj, true); } } + + cleanup: + if (virStoragePoolObjIsStarting(obj)) { + if (!virStoragePoolObjIsActive(obj)) + virStoragePoolUpdateInactive(obj); + virStoragePoolObjSetStarting(obj, false); + } }
@@ -750,6 +759,8 @@ storagePoolCreateXML(virConnectPtr conn, newDef = NULL; def = virStoragePoolObjGetDef(obj);
Coverity let me know this morning that there's quite a few lines above here which goto cleanup; however, cleanup expects @obj != NULL (or at least virStoragePoolObjIsStarting does). Probably need similar logic like you added in storagePool{Create|Build}. John
+ virStoragePoolObjSetStarting(obj, true); + if (backend->buildPool) { if (flags & VIR_STORAGE_POOL_CREATE_WITH_BUILD_OVERWRITE) build_flags |= VIR_STORAGE_POOL_BUILD_OVERWRITE; @@ -786,6 +797,11 @@ storagePoolCreateXML(virConnectPtr conn, pool = virGetStoragePool(conn, def->name, def->uuid, NULL, NULL);
cleanup: + if (virStoragePoolObjIsStarting(obj)) { + if (!virStoragePoolObjIsActive(obj)) + virStoragePoolUpdateInactive(obj); + virStoragePoolObjSetStarting(obj, false); + } virObjectEventStateQueue(driver->storageEventState, event); virStoragePoolObjEndAPI(&obj); return pool;
[...]
https://bugzilla.redhat.com/show_bug.cgi?id=1711789 Starting up or building some types of pools may take a very long time (e.g. a misconfigured NFS). Holding the pool object locked throughout the whole time hurts concurrency, e.g. if there's another thread that is listing all the pools. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/storage/storage_backend_disk.c | 11 ++++++++++- src/storage/storage_backend_fs.c | 13 +++++++++++-- src/storage/storage_backend_logical.c | 15 ++++++++++----- src/storage/storage_backend_vstorage.c | 9 ++++++++- src/storage/storage_backend_zfs.c | 7 ++++++- 5 files changed, 45 insertions(+), 10 deletions(-) diff --git a/src/storage/storage_backend_disk.c b/src/storage/storage_backend_disk.c index 9b94d26cf9..f58b7b294c 100644 --- a/src/storage/storage_backend_disk.c +++ b/src/storage/storage_backend_disk.c @@ -461,7 +461,10 @@ virStorageBackendDiskStartPool(virStoragePoolObjPtr pool) const char *format; const char *path = def->source.devices[0].path; + /* This can take a significant amount of time. */ + virObjectUnlock(pool); virWaitForDevices(); + virObjectLock(pool); if (!virFileExists(path)) { virReportError(VIR_ERR_INVALID_ARG, @@ -490,6 +493,7 @@ virStorageBackendDiskBuildPool(virStoragePoolObjPtr pool, int format = def->source.format; const char *fmt; VIR_AUTOPTR(virCommand) cmd = NULL; + int ret = -1; virCheckFlags(VIR_STORAGE_POOL_BUILD_OVERWRITE | VIR_STORAGE_POOL_BUILD_NO_OVERWRITE, -1); @@ -523,7 +527,12 @@ virStorageBackendDiskBuildPool(virStoragePoolObjPtr pool, "--script", fmt, NULL); - return virCommandRun(cmd, NULL); + + virObjectUnlock(pool); + ret = virCommandRun(cmd, NULL); + virObjectLock(pool); + + return ret; } diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index ae4e9a03a3..1257419760 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -318,7 +318,14 @@ virStorageBackendFileSystemMount(virStoragePoolObjPtr pool) return -1; cmd = virStorageBackendFileSystemMountCmd(MOUNT, def, src); - return virCommandRun(cmd, NULL); + + /* Mounting a shared FS might take a long time. Don't hold + * the pool locked meanwhile. */ + virObjectUnlock(pool); + rc = virCommandRun(cmd, NULL); + virObjectLock(pool); + + return rc; } @@ -457,13 +464,14 @@ virStorageBackendMakeFileSystem(virStoragePoolObjPtr pool, virReportError(VIR_ERR_OPERATION_INVALID, _("No source device specified when formatting pool '%s'"), def->name); - goto error; + return -1; } device = def->source.devices[0].path; format = virStoragePoolFormatFileSystemTypeToString(def->source.format); VIR_DEBUG("source device: '%s' format: '%s'", device, format); + virObjectUnlock(pool); if (!virFileExists(device)) { virReportError(VIR_ERR_OPERATION_INVALID, _("Source device does not exist when formatting pool '%s'"), @@ -482,6 +490,7 @@ virStorageBackendMakeFileSystem(virStoragePoolObjPtr pool, ret = virStorageBackendExecuteMKFS(device, format); error: + virObjectLock(pool); return ret; } diff --git a/src/storage/storage_backend_logical.c b/src/storage/storage_backend_logical.c index 83b5f27151..603a3f9a42 100644 --- a/src/storage/storage_backend_logical.c +++ b/src/storage/storage_backend_logical.c @@ -50,9 +50,15 @@ virStorageBackendLogicalSetActive(virStoragePoolObjPtr pool, { virStoragePoolDefPtr def = virStoragePoolObjGetDef(pool); VIR_AUTOPTR(virCommand) cmd = NULL; + int ret; cmd = virStorageBackendLogicalChangeCmd(VGCHANGE, def, on); - return virCommandRun(cmd, NULL); + + virObjectUnlock(pool); + ret = virCommandRun(cmd, NULL); + virObjectLock(pool); + + return ret; } @@ -723,11 +729,10 @@ virStorageBackendLogicalBuildPool(virStoragePoolObjPtr pool, virCommandAddArg(vgcmd, path); } + virObjectUnlock(pool); /* Now create the volume group itself */ - if (virCommandRun(vgcmd, NULL) < 0) - goto cleanup; - - ret = 0; + ret = virCommandRun(vgcmd, NULL); + virObjectLock(pool); cleanup: /* On any failure, run through the devices that had pvcreate run in diff --git a/src/storage/storage_backend_vstorage.c b/src/storage/storage_backend_vstorage.c index d446aa2726..cec21dccbf 100644 --- a/src/storage/storage_backend_vstorage.c +++ b/src/storage/storage_backend_vstorage.c @@ -42,6 +42,7 @@ virStorageBackendVzPoolStart(virStoragePoolObjPtr pool) VIR_AUTOFREE(char *) usr_name = NULL; VIR_AUTOFREE(char *) mode = NULL; VIR_AUTOPTR(virCommand) cmd = NULL; + int ret; /* Check the permissions */ if (def->target.perms.mode == (mode_t)-1) @@ -69,7 +70,13 @@ virStorageBackendVzPoolStart(virStoragePoolObjPtr pool) "-g", grp_name, "-u", usr_name, NULL); - return virCommandRun(cmd, NULL); + /* Mounting a shared FS might take a long time. Don't hold + * the pool locked meanwhile. */ + virObjectUnlock(pool); + ret = virCommandRun(cmd, NULL); + virObjectLock(pool); + + return ret; } diff --git a/src/storage/storage_backend_zfs.c b/src/storage/storage_backend_zfs.c index 826a95538e..d172a5a165 100644 --- a/src/storage/storage_backend_zfs.c +++ b/src/storage/storage_backend_zfs.c @@ -385,6 +385,7 @@ virStorageBackendZFSBuildPool(virStoragePoolObjPtr pool, virStoragePoolDefPtr def = virStoragePoolObjGetDef(pool); size_t i; VIR_AUTOPTR(virCommand) cmd = NULL; + int ret = -1; virCheckFlags(0, -1); @@ -400,7 +401,11 @@ virStorageBackendZFSBuildPool(virStoragePoolObjPtr pool, for (i = 0; i < def->source.ndevice; i++) virCommandAddArg(cmd, def->source.devices[i].path); - return virCommandRun(cmd, NULL); + virObjectUnlock(pool); + ret = virCommandRun(cmd, NULL); + virObjectLock(pool); + + return ret; } static int -- 2.21.0
On Fri, May 24, 2019 at 04:35:47PM +0200, Michal Privoznik wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1711789
Starting up or building some types of pools may take a very long time (e.g. a misconfigured NFS). Holding the pool object locked throughout the whole time hurts concurrency, e.g. if there's another thread that is listing all the pools.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/storage/storage_backend_disk.c | 11 ++++++++++- src/storage/storage_backend_fs.c | 13 +++++++++++-- src/storage/storage_backend_logical.c | 15 ++++++++++----- src/storage/storage_backend_vstorage.c | 9 ++++++++- src/storage/storage_backend_zfs.c | 7 ++++++- 5 files changed, 45 insertions(+), 10 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
participants (3)
-
John Ferlan -
Ján Tomko -
Michal Privoznik