On 01/10/2018 12:39 PM, John Ferlan wrote:
On 01/10/2018 04:16 AM, Michal Privoznik wrote:
On 01/09/2018 09:05 PM, John Ferlan wrote:
Alter the volume logic to use the hash tables instead of forward linked lists. There are three hash tables to allow for fast lookup by name, target.path, and key.
Modify the virStoragePoolObjAddVol to place the object in all 3 tables if possible using self locking RWLock on the volumes object. Conversely when removing the volume, it's a removal of the object from the various hash tables.
Implement functions to handle remote ForEach and Search Volume type helpers. These are used by the disk backend in order to facilitate adding a primary, extended, or logical partition.
Implement the various VolDefFindBy* helpers as simple (and fast) hash lookups. The NumOfVolumes, GetNames, and ListExport helpers are all implemented using standard for each hash table calls.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virstorageobj.c | 420 +++++++++++++++++++++++++++++++++++------------ 1 file changed, 311 insertions(+), 109 deletions(-)
diff --git a/src/conf/virstorageobj.c b/src/conf/virstorageobj.c index 8a1c6f782..d92b2b2e3 100644 --- a/src/conf/virstorageobj.c +++ b/src/conf/virstorageobj.c @@ -53,11 +53,6 @@ virStorageVolObjListDispose(void *opaque);
size_t virStoragePoolObjGetVolumesCount(virStoragePoolObjPtr obj) { - return obj->volumes.count; + return virHashSize(obj->volumes->objsKey);
How come we don't need a read lock here? ... I think we should grab a read lock from obj->volumes just like you're doing in virStorageVolDefFindByKey() for instance.
This doesn't traverse volumes->objs - it gets the size directly from the hash table stored @objsKey.
Exactly the reason why we need a lock here. What if another thread is already modifying the table? True, this is not that problematic since we don't really care if we get N or N+1 result here (moreover, this function is used only at one place and that is VIR_DEBUG, so not a big problem), but still I think we need a read lock here.
I'm not against adding an RWLock here, but that's probably what the distinguishing factor was (I wrote the code 3 months ago ;-) - it's just been waiting it's turn).
Sure, no problem. Michal