Re: [libvirt] [PATCH 4/4] storage: Complete implementation volume by hash object

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(a)redhat.com&gt; >> --- >> 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.

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).