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

[John Ferlan]

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

> 
>> +}
>> +
>> +
> 
> 
>> +
>> +static int
>> +virStoragePoolObjNumOfVolumesCb(void *payload,
>> +                                const void *name ATTRIBUTE_UNUSED,
>> +                                void *opaque)
>> +{
>> +    virStorageVolObjPtr volobj = payload;
>> +    struct _virStorageVolObjCountData *data = opaque;
>> +
>> +    virObjectLock(volobj);
>> +
>> +    if (data->filter && !data->filter(data->conn, data->pooldef,
>> +                                      volobj->voldef))
> 
> If you'd break the line after logical and it would look nicer ;-)
> 

Sure, np.  Same for GetNamesCb and ListExportCb too

TKs -

John

>> +        goto cleanup;
>> +
>> +    data->count++;
>> +
>> + cleanup:
>> +    virObjectUnlock(volobj);
>> +    return 0;
>> +}
> 
> Michal
>