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