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 <jfer...@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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
