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

[Michal Privoznik]

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 at 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