[libvirt] [PATCH 0/4] Be consistent with vir*Obj*Remove* APIs
Using comments from recent changes/reviews w/ libxl and usage of the virDomainObjListRemove which note that it's "a problem" that the Remove API expects a locked @obj on input, but then returns the @obj unlocked. Although adjusting domainobjlist is a "future" task, we can at least modify the other drivers/vir*obj modules to be consistent at least with respect to expecting and returning a locked (and reffed) object. Thus, modify interface, secret, nodedev, and storage pool. The network is already done this way and nwfilter appears to be a lost cause as we cannot come to a concensus over how to use hash objects and thus the code still uses forward linked list object manipulation. Changes for virdomainobjlist require a number of other changes to happen first with respect to consistent usage of Find and Add API's to return locked and ref counted objects, then being able to have the virDomainObjListRemove be consistent as well. John Ferlan (4): secret: Return with locked obj from virSecretObjListRemove interface: Return with locked obj from virInterfaceObjListRemove nodedev: Return with locked obj from virNodeDeviceObjListRemove storagepool: Return with locked obj from virStoragePoolObjRemove src/conf/virinterfaceobj.c | 13 ++++++++++++- src/conf/virnodedeviceobj.c | 13 ++++++++++++- src/conf/virsecretobj.c | 15 ++++++++------- src/conf/virstorageobj.c | 17 ++++++++++++++--- src/node_device/node_device_hal.c | 4 ++-- src/node_device/node_device_udev.c | 2 +- src/secret/secret_driver.c | 4 ---- src/storage/storage_driver.c | 12 +----------- src/test/test_driver.c | 20 +++++--------------- 9 files changed, 55 insertions(+), 45 deletions(-) -- 2.13.6
Rather than unlock the object that was expected to be locked on input, let the caller perform the unlock or more succinctly a virSecretObjEndAPI on the object which will perform the Unref and Unlock and clear the @obj. Also clean up the virSecretObjListRemove function comments. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virsecretobj.c | 15 ++++++++------- src/secret/secret_driver.c | 4 ---- 2 files changed, 8 insertions(+), 11 deletions(-) diff --git a/src/conf/virsecretobj.c b/src/conf/virsecretobj.c index 47e0b28968..49c341484b 100644 --- a/src/conf/virsecretobj.c +++ b/src/conf/virsecretobj.c @@ -284,11 +284,14 @@ virSecretObjListFindByUsage(virSecretObjListPtr secrets, /* * virSecretObjListRemove: * @secrets: list of secret objects - * @secret: a secret object + * @obj: a secret object * - * Remove the object from the hash table. The caller must hold the lock - * on the driver owning @secrets and must have also locked @secret to - * ensure no one else is either waiting for @secret or still using it. + * Remove @obj from the secret obj list hash table. The caller must hold + * the lock on @obj to ensure no one else is either waiting for @obj or + * still using it. + * + * Upon return the @obj remains locked with at least 1 reference and + * the caller is expected to use virSecretObjEndAPI on it. */ void virSecretObjListRemove(virSecretObjListPtr secrets, @@ -308,7 +311,6 @@ virSecretObjListRemove(virSecretObjListPtr secrets, virObjectRWLockWrite(secrets); virObjectLock(obj); virHashRemoveEntry(secrets->objs, uuidstr); - virObjectUnlock(obj); virObjectUnref(obj); virObjectRWUnlock(secrets); } @@ -927,8 +929,7 @@ virSecretLoad(virSecretObjListPtr secrets, if (virSecretLoadValue(obj) < 0) { virSecretObjListRemove(secrets, obj); - virObjectUnref(obj); - obj = NULL; + virSecretObjEndAPI(&obj); /* clears obj */ } cleanup: diff --git a/src/secret/secret_driver.c b/src/secret/secret_driver.c index 23a3c9bdad..7c611253aa 100644 --- a/src/secret/secret_driver.c +++ b/src/secret/secret_driver.c @@ -271,8 +271,6 @@ secretDefineXML(virConnectPtr conn, VIR_STEAL_PTR(def, objDef); } else { virSecretObjListRemove(driver->secrets, obj); - virObjectUnref(obj); - obj = NULL; } cleanup: @@ -413,8 +411,6 @@ secretUndefine(virSecretPtr secret) virSecretObjDeleteData(obj); virSecretObjListRemove(driver->secrets, obj); - virObjectUnref(obj); - obj = NULL; ret = 0; -- 2.13.6
On Wed, Mar 21, 2018 at 11:53:32AM -0400, John Ferlan wrote:
Rather than unlock the object that was expected to be locked on input, let the caller perform the unlock or more succinctly a virSecretObjEndAPI on the object which will perform the Unref and Unlock and clear the @obj.
I understand the incentive for this change and I agree with it, but I do have a question below.
Also clean up the virSecretObjListRemove function comments.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virsecretobj.c | 15 ++++++++------- src/secret/secret_driver.c | 4 ---- 2 files changed, 8 insertions(+), 11 deletions(-)
diff --git a/src/conf/virsecretobj.c b/src/conf/virsecretobj.c index 47e0b28968..49c341484b 100644 --- a/src/conf/virsecretobj.c +++ b/src/conf/virsecretobj.c @@ -284,11 +284,14 @@ virSecretObjListFindByUsage(virSecretObjListPtr secrets, /* * virSecretObjListRemove: * @secrets: list of secret objects - * @secret: a secret object + * @obj: a secret object * - * Remove the object from the hash table. The caller must hold the lock - * on the driver owning @secrets and must have also locked @secret to - * ensure no one else is either waiting for @secret or still using it. + * Remove @obj from the secret obj list hash table. The caller must hold + * the lock on @obj to ensure no one else is either waiting for @obj or + * still using it. + * + * Upon return the @obj remains locked with at least 1 reference and + * the caller is expected to use virSecretObjEndAPI on it. */ void virSecretObjListRemove(virSecretObjListPtr secrets, @@ -308,7 +311,6 @@ virSecretObjListRemove(virSecretObjListPtr secrets, virObjectRWLockWrite(secrets); virObjectLock(obj); virHashRemoveEntry(secrets->objs, uuidstr); - virObjectUnlock(obj); virObjectUnref(obj); virObjectRWUnlock(secrets);
So, why do we require the caller to hold a lock on the object when the very first thing we do is that we ref the @obj, UNLOCK it to avoid a deadlock with someone else already holding a lock on @secrets and waiting on @obj and only when we have the lock on @secrets, we re-lock @obj. Why isn't it sufficient to just ref @obj since that's an atomic op and then only lock it once we acquire a lock on @secrets too. In this case, you don't have to accept and return a locked object, you only lock when absolutely necessary, what am I missing? In my opinion this lock-unlock-lock (with occasional ref) fire dance is quite cumbersome, but I guess I might be missing some important aspect here which I don't see at the first glance.
} @@ -927,8 +929,7 @@ virSecretLoad(virSecretObjListPtr secrets,
if (virSecretLoadValue(obj) < 0) { virSecretObjListRemove(secrets, obj); - virObjectUnref(obj); - obj = NULL; + virSecretObjEndAPI(&obj); /* clears obj */
This can be further simplified by moving the ObjEndAPI to the cleanup section and removing the braces on this 'if' block. Erik
On 03/22/2018 05:44 AM, Erik Skultety wrote:
On Wed, Mar 21, 2018 at 11:53:32AM -0400, John Ferlan wrote:
Rather than unlock the object that was expected to be locked on input, let the caller perform the unlock or more succinctly a virSecretObjEndAPI on the object which will perform the Unref and Unlock and clear the @obj.
I understand the incentive for this change and I agree with it, but I do have a question below.
Also clean up the virSecretObjListRemove function comments.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virsecretobj.c | 15 ++++++++------- src/secret/secret_driver.c | 4 ---- 2 files changed, 8 insertions(+), 11 deletions(-)
diff --git a/src/conf/virsecretobj.c b/src/conf/virsecretobj.c index 47e0b28968..49c341484b 100644 --- a/src/conf/virsecretobj.c +++ b/src/conf/virsecretobj.c @@ -284,11 +284,14 @@ virSecretObjListFindByUsage(virSecretObjListPtr secrets, /* * virSecretObjListRemove: * @secrets: list of secret objects - * @secret: a secret object + * @obj: a secret object * - * Remove the object from the hash table. The caller must hold the lock - * on the driver owning @secrets and must have also locked @secret to - * ensure no one else is either waiting for @secret or still using it. + * Remove @obj from the secret obj list hash table. The caller must hold + * the lock on @obj to ensure no one else is either waiting for @obj or + * still using it. + * + * Upon return the @obj remains locked with at least 1 reference and + * the caller is expected to use virSecretObjEndAPI on it. */ void virSecretObjListRemove(virSecretObjListPtr secrets, @@ -308,7 +311,6 @@ virSecretObjListRemove(virSecretObjListPtr secrets, virObjectRWLockWrite(secrets); virObjectLock(obj); virHashRemoveEntry(secrets->objs, uuidstr); - virObjectUnlock(obj); virObjectUnref(obj); virObjectRWUnlock(secrets);
So, why do we require the caller to hold a lock on the object when the very first thing we do is that we ref the @obj, UNLOCK it to avoid a deadlock with someone else already holding a lock on @secrets and waiting on @obj and only when we have the lock on @secrets, we re-lock @obj. Why isn't it sufficient to just ref @obj since that's an atomic op and then only lock it once we acquire a lock on @secrets too. In this case, you don't have to accept and return a locked object, you only lock when absolutely necessary, what am I missing? In my opinion this lock-unlock-lock (with occasional ref) fire dance is quite cumbersome, but I guess I might be missing some important aspect here which I don't see at the first glance.
I have no compelling argument other than history (it's how it's been done) and a desire to not change too much code, but I can take that path to see what happens. Ironically as I was making these changes I thought - why even do that ref since we enter with the @obj reffed and locked. Adding one more ref doesn't help or hurt when it comes to unlock as we enter (this code) with 2 refs and 1 lock. We RemoveEntry losing 1 ref and then have the caller unref when it's done. The lock fire dance is only because of lock ordering. I think part of the issue is that there's mostly error path callers to ListRemove and one non error path caller (Undefine). For each of those callers, we grab the list lock, fetch the object, and return it locked/reffed - changing that would have the same fire dance problem you note in the current Remove path, but perhaps for larger swaths of code. Another part of the issue is that we don't grab the List lock in the driver, rather we grab it in the vir*obj.c code. That's a design decision propagated from the domain/network change to use hash tables that I just followed when altering secrets, interface, nodedev, storage, nwfilter, etc. So in order to not have the fire dance, the list locking would need to move to the driver and would be held for longer periods which is what IIRC we're trying to avoid originally (think domain code and how many threads can hammer that). Of course with rwlock/read locks that perhaps is now a non issue. NB: consider the possibility that two threads try to Undefine at the same time... One wins the list lock race and does the hash table removal, but since there's still a refcnt from the other thread, the memory isn't freed. When the second thread gets the lock, it'll call the RemoveEntry (and fail, but not error). Eventually it unrefs the last ref and poof the memory is gone.
} @@ -927,8 +929,7 @@ virSecretLoad(virSecretObjListPtr secrets,
if (virSecretLoadValue(obj) < 0) { virSecretObjListRemove(secrets, obj); - virObjectUnref(obj); - obj = NULL; + virSecretObjEndAPI(&obj); /* clears obj */
This can be further simplified by moving the ObjEndAPI to the cleanup section and removing the braces on this 'if' block.
True, that'd be an extra "before" patch I think though. John
Rather than unlock the object that was expected to be locked on input, let the caller perform the unlock or more succinctly a virInterfaceObjEndAPI on the object which will perform the Unref and Unlock and clear the @obj. Also add comments to the virInterfaceObjListRemove. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virinterfaceobj.c | 13 ++++++++++++- src/test/test_driver.c | 2 +- 2 files changed, 13 insertions(+), 2 deletions(-) diff --git a/src/conf/virinterfaceobj.c b/src/conf/virinterfaceobj.c index f90c0bd9c4..28fbbaa08b 100644 --- a/src/conf/virinterfaceobj.c +++ b/src/conf/virinterfaceobj.c @@ -358,6 +358,18 @@ virInterfaceObjListAssignDef(virInterfaceObjListPtr interfaces, } +/* + * virInterfaceObjListRemove: + * @interfaces: list of interface objects + * @obj: an interface object + * + * Remove @obj from the interface obj list hash table. The caller must hold + * the lock on @obj to ensure no one else is either waiting for @obj or + * still using it. + * + * Upon return the @obj remains locked with at least 1 reference and + * the caller is expected to use virInterfaceObjEndAPI on it. + */ void virInterfaceObjListRemove(virInterfaceObjListPtr interfaces, virInterfaceObjPtr obj) @@ -370,7 +382,6 @@ virInterfaceObjListRemove(virInterfaceObjListPtr interfaces, virObjectRWLockWrite(interfaces); virObjectLock(obj); virHashRemoveEntry(interfaces->objsName, obj->def->name); - virObjectUnlock(obj); virObjectUnref(obj); virObjectRWUnlock(interfaces); } diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 4de0cc5333..b0aa350d95 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -4085,7 +4085,7 @@ testInterfaceUndefine(virInterfacePtr iface) return -1; virInterfaceObjListRemove(privconn->ifaces, obj); - virObjectUnref(obj); + virInterfaceObjEndAPI(&obj); return 0; } -- 2.13.6
Rather than unlock the object that was expected to be locked on input, let the caller perform the unlock or more succinctly a virNodeDeviceObjEndAPI on the object which will perform the Unref and Unlock and clear the @obj. Also add comments for virNodeDeviceObjListRemove. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virnodedeviceobj.c | 13 ++++++++++++- src/node_device/node_device_hal.c | 4 ++-- src/node_device/node_device_udev.c | 2 +- src/test/test_driver.c | 4 +--- 4 files changed, 16 insertions(+), 7 deletions(-) diff --git a/src/conf/virnodedeviceobj.c b/src/conf/virnodedeviceobj.c index ad0f27ee47..a5939c5de5 100644 --- a/src/conf/virnodedeviceobj.c +++ b/src/conf/virnodedeviceobj.c @@ -470,6 +470,18 @@ virNodeDeviceObjListAssignDef(virNodeDeviceObjListPtr devs, } +/* + * virNodeDeviceObjListRemove: + * @devs: list of node device objects + * @obj: a node device object + * + * Remove @obj from the node device obj list hash table. The caller must hold + * the lock on @obj to ensure no one else is either waiting for @obj or + * still using it. + * + * Upon return the @obj remains locked with at least 1 reference and + * the caller is expected to use virNodeDeviceObjEndAPI on it. + */ void virNodeDeviceObjListRemove(virNodeDeviceObjListPtr devs, virNodeDeviceObjPtr obj) @@ -485,7 +497,6 @@ virNodeDeviceObjListRemove(virNodeDeviceObjListPtr devs, virObjectRWLockWrite(devs); virObjectLock(obj); virHashRemoveEntry(devs->objs, def->name); - virObjectUnlock(obj); virObjectUnref(obj); virObjectRWUnlock(devs); } diff --git a/src/node_device/node_device_hal.c b/src/node_device/node_device_hal.c index 6ad56f4166..8aac806a64 100644 --- a/src/node_device/node_device_hal.c +++ b/src/node_device/node_device_hal.c @@ -512,7 +512,7 @@ dev_refresh(const char *udi) * to sub-capabilities (like net.80203) is nasty ... so avoid it. */ virNodeDeviceObjListRemove(driver->devs, obj); - virObjectUnref(obj); + virNodeDeviceObjEndAPI(&obj); dev_create(udi); } else { VIR_DEBUG("no device named %s", name); @@ -541,7 +541,7 @@ device_removed(LibHalContext *ctx ATTRIBUTE_UNUSED, virNodeDeviceObjListRemove(driver->devs, obj); else VIR_DEBUG("no device named %s", name); - virObjectUnref(obj); + virNodeDeviceObjEndAPI(&obj); } diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index e87eb32a85..1151b04ecb 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -1298,7 +1298,7 @@ udevRemoveOneDevice(struct udev_device *device) VIR_DEBUG("Removing device '%s' with sysfs path '%s'", def->name, name); virNodeDeviceObjListRemove(driver->devs, obj); - virObjectUnref(obj); + virNodeDeviceObjEndAPI(&obj); if (event) virObjectEventStateQueue(driver->nodeDeviceEventState, event); diff --git a/src/test/test_driver.c b/src/test/test_driver.c index b0aa350d95..a7aaabe09b 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -4716,7 +4716,7 @@ testDestroyVport(testDriverPtr privconn, 0); virNodeDeviceObjListRemove(privconn->devs, obj); - virObjectUnref(obj); + virNodeDeviceObjEndAPI(&obj); testObjectEventQueue(privconn, event); return 0; @@ -5695,8 +5695,6 @@ testNodeDeviceDestroy(virNodeDevicePtr dev) virObjectLock(obj); virNodeDeviceObjListRemove(driver->devs, obj); - virObjectUnref(obj); - obj = NULL; cleanup: virNodeDeviceObjEndAPI(&obj); -- 2.13.6
Rather than unlock the object that was expected to be locked on input, let the caller perform the unlock or more succinctly a virStoragePoolObjEndAPI on the object which will perform the Unref and Unlock and clear the @obj. Also add comments for virStoragePoolObjRemove. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virstorageobj.c | 17 ++++++++++++++--- src/storage/storage_driver.c | 12 +----------- src/test/test_driver.c | 14 +++----------- 3 files changed, 18 insertions(+), 25 deletions(-) diff --git a/src/conf/virstorageobj.c b/src/conf/virstorageobj.c index 799b8c9fa3..c057c6b3ed 100644 --- a/src/conf/virstorageobj.c +++ b/src/conf/virstorageobj.c @@ -517,6 +517,18 @@ virStoragePoolObjListSearch(virStoragePoolObjListPtr pools, } +/* + * virStoragePoolObjRemove: + * @pools: list of storage pool objects + * @obj: a storage pool object + * + * Remove @obj from the storage pool obj list hash tables. The caller must + * hold the lock on @obj to ensure no one else is either waiting for @obj or + * still using it. + * + * Upon return the @obj remains locked with at least 1 reference and + * the caller is expected to use virStoragePoolObjEndAPI on it. + */ void virStoragePoolObjRemove(virStoragePoolObjListPtr pools, virStoragePoolObjPtr obj) @@ -530,7 +542,6 @@ virStoragePoolObjRemove(virStoragePoolObjListPtr pools, virObjectLock(obj); virHashRemoveEntry(pools->objs, uuidstr); virHashRemoveEntry(pools->objsName, obj->def->name); - virObjectUnlock(obj); virObjectUnref(obj); virObjectRWUnlock(pools); } @@ -1138,13 +1149,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 d5e38af5aa..b2946d0509 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -94,7 +94,7 @@ virStoragePoolUpdateInactive(virStoragePoolObjPtr *objptr) if (!virStoragePoolObjGetConfigFile(obj)) { virStoragePoolObjRemove(driver->pools, obj); - virObjectUnref(obj); + virStoragePoolObjEndAPI(&obj); *objptr = NULL; } else if (virStoragePoolObjGetNewDef(obj)) { virStoragePoolObjDefUseNewDef(obj); @@ -746,8 +746,6 @@ storagePoolCreateXML(virConnectPtr conn, (flags & VIR_STORAGE_POOL_CREATE_WITH_BUILD)) { if (backend->buildPool(obj, build_flags) < 0) { virStoragePoolObjRemove(driver->pools, obj); - virObjectUnref(obj); - obj = NULL; goto cleanup; } } @@ -756,8 +754,6 @@ storagePoolCreateXML(virConnectPtr conn, if (backend->startPool && backend->startPool(obj) < 0) { virStoragePoolObjRemove(driver->pools, obj); - virObjectUnref(obj); - obj = NULL; goto cleanup; } @@ -771,8 +767,6 @@ storagePoolCreateXML(virConnectPtr conn, if (backend->stopPool) backend->stopPool(obj); virStoragePoolObjRemove(driver->pools, obj); - virObjectUnref(obj); - obj = NULL; goto cleanup; } @@ -833,8 +827,6 @@ storagePoolDefineXML(virConnectPtr conn, if (virStoragePoolObjSaveDef(driver, obj, def) < 0) { virStoragePoolObjRemove(driver->pools, obj); - virObjectUnref(obj); - obj = NULL; goto cleanup; } @@ -901,8 +893,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 a7aaabe09b..4b7f8c4244 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -4556,16 +4556,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; } @@ -4627,8 +4623,6 @@ testStoragePoolDefineXML(virConnectPtr conn, if (testStoragePoolObjSetDefaults(obj) == -1) { virStoragePoolObjRemove(privconn->pools, obj); - virObjectUnref(obj); - obj = NULL; goto cleanup; } @@ -4658,7 +4652,7 @@ testStoragePoolUndefine(virStoragePoolPtr pool) 0); virStoragePoolObjRemove(privconn->pools, obj); - virObjectUnref(obj); + virStoragePoolObjEndAPI(&obj); testObjectEventQueue(privconn, event); return 0; @@ -4750,11 +4744,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.13.6
participants (2)
-
Erik Skultety -
John Ferlan