2:54 a.m.
On Tue, Dec 06, 2016 at 22:48:39 +0530, Prasanna Kalever wrote:
On Mon, Dec 5, 2016 at 8:08 PM, Peter Krempa
<pkrempa(a)redhat.com> wrote:
> On Mon, Dec 05, 2016 at 18:55:19 +0530, Prasanna Kumar Kalever wrote:
>> Currently, in case if we have 4 extra attached disks, then for each disk
>> we need to call 'glfs_init' (over network) and friends which could be
costly.
>>
>> Additionally snapshot(external) scenario will further complex the situation.
>>
>> This patch maintain a cache of glfs objects per volume, hence the
>> all disks/snapshots belonging to the volume whose glfs object is cached
>> can avoid calls to 'glfs_init' and friends by simply taking a ref on
>> existing object.
>>
>> The glfs object is shared only if volume name and all the volfile servers match
>> (includes hostname, transport and port number).
>>
>> Thanks to 'Peter Krempa' for all the inputs.
>>
>> Signed-off-by: Prasanna Kumar Kalever <prasanna.kalever(a)redhat.com>
>> ---
>> src/storage/storage_backend_fs.c | 2 +
>> src/storage/storage_backend_gluster.c | 249 +++++++++++++++++++++++++++++++---
>> src/storage/storage_driver.c | 5 +-
>> 3 files changed, 230 insertions(+), 26 deletions(-)
>>
>> diff --git a/src/storage/storage_backend_fs.c
b/src/storage/storage_backend_fs.c
>> index de0e8d5..0e03e06 100644
>> --- a/src/storage/storage_backend_fs.c
>> +++ b/src/storage/storage_backend_fs.c
>> @@ -1488,6 +1488,8 @@ virStorageFileBackendFileDeinit(virStorageSourcePtr src)
>>
>> VIR_FREE(priv->canonpath);
>> VIR_FREE(priv);
>> +
>> + VIR_FREE(src->drv);
>
> See at the end.
>
>> }
>>
>>
>> diff --git a/src/storage/storage_backend_gluster.c
b/src/storage/storage_backend_gluster.c
>> index 0d5b7f6..1f85dfa 100644
>> --- a/src/storage/storage_backend_gluster.c
>> +++ b/src/storage/storage_backend_gluster.c
>> @@ -31,11 +31,26 @@
>> #include "virstoragefile.h"
>> #include "virstring.h"
>> #include "viruri.h"
>> +#include "viratomic.h"
>
> It doesn't look like you use it in this iteration.
>
>>
>> #define VIR_FROM_THIS VIR_FROM_STORAGE
>>
>> VIR_LOG_INIT("storage.storage_backend_gluster");
>>
>> +
>> +typedef struct _virStorageBackendGlusterState virStorageBackendGlusterState;
>> +typedef virStorageBackendGlusterState *virStorageBackendGlusterStatePtr;
>> +
>> +typedef struct _virStorageFileBackendGlusterPriv
virStorageFileBackendGlusterPriv;
>> +typedef virStorageFileBackendGlusterPriv *virStorageFileBackendGlusterPrivPtr;
>> +
>> +typedef struct _virStorageBackendGlusterStatePreopened
virStorageBackendGlusterStatePreopened;
>> +typedef virStorageBackendGlusterStatePreopened
*virStorageBackendGlusterStatePtrPreopened;
>> +
>> +typedef struct _virStorageBackendGlusterconnCache
virStorageBackendGlusterconnCache;
>> +typedef virStorageBackendGlusterconnCache
*virStorageBackendGlusterconnCachePtr;
>> +
>> +
>> struct _virStorageBackendGlusterState {
>> glfs_t *vol;
>>
>> @@ -47,8 +62,207 @@ struct _virStorageBackendGlusterState {
>> char *dir; /* dir from URI, or "/"; always starts and ends in
'/' */
>> };
>>
>> -typedef struct _virStorageBackendGlusterState virStorageBackendGlusterState;
>> -typedef virStorageBackendGlusterState *virStorageBackendGlusterStatePtr;
>> +struct _virStorageFileBackendGlusterPriv {
>> + glfs_t *vol;
>> + char *canonpath;
>> +};
>> +
>> +struct _virStorageBackendGlusterStatePreopened {
>> + virObjectLockable parent;
>> + virStorageFileBackendGlusterPrivPtr priv;
>> + size_t nservers;
>> + virStorageNetHostDefPtr hosts;
>> + char *volname;
>> +};
>> +
>> +struct _virStorageBackendGlusterconnCache {
>> + virMutex lock;
>> + size_t nConn;
>> + virStorageBackendGlusterStatePtrPreopened *conn;
>> +};
>> +
>> +
>> +static virStorageBackendGlusterconnCachePtr connCache;
>
> connCache still does not follow the naming scheme that was requested by
> Dan.
>
>> +static virClassPtr virStorageBackendGlusterStatePreopenedClass;
>> +static virOnceControl glusterConnOnce = VIR_ONCE_CONTROL_INITIALIZER;
>
> This neither.
>
>> +static bool virStorageBackendGlusterPreopenedGlobalError;
>
> I don't see a reason to have this as a global flag.
But how do I bounce the errors from 'virStorageBackendGlusterConnInit' ?
The problem is that you did not follow how libvirt is using virOnce
(VIR_ONCE_GLOBAL_INIT) and tried to do it yourself. Libvirt has multiple
examples that return error codes from one-time initializers.
>
>> +
>> +
>> +static void virStorageBackendGlusterStatePreopenedDispose(void *obj);
>> +
>> +
>> +static int virStorageBackendGlusterStatePreopenedOnceInit(void)
>> +{
>> + if (!(virStorageBackendGlusterStatePreopenedClass =
>> + virClassNew(virClassForObjectLockable(),
>> +
"virStorageBackendGlusterStatePreopenedClass",
>> +
sizeof(virStorageBackendGlusterStatePreopened),
>> +
virStorageBackendGlusterStatePreopenedDispose)))
>> + return -1;
>> +
>> + return 0;
>> +}
>> +
>> +static void virStorageBackendGlusterStatePreopenedDispose(void *object)
>> +{
>> + virStorageBackendGlusterStatePtrPreopened entry = object;
>> +
>> + glfs_fini(entry->priv->vol);
>> +
>> + VIR_FREE(entry->priv->canonpath);
>> + VIR_FREE(entry->priv);
>> + VIR_FREE(entry->volname);
>> +
>> + virStorageNetHostDefFree(entry->nservers, entry->hosts);
>> +}
>> +
>> +static void
>> +virStorageBackendGlusterConnInit(void)
>> +{
>> + if (VIR_ALLOC(connCache) < 0) {
>> + virStorageBackendGlusterPreopenedGlobalError = false;
>> + return;
>> + }
>> +
>> + if (virMutexInit(&connCache->lock) < 0) {
>> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>> + _("Unable to initialize mutex"));
>> + virStorageBackendGlusterPreopenedGlobalError = false;
>> + }
>> +}
>> +
>> +VIR_ONCE_GLOBAL_INIT(virStorageBackendGlusterStatePreopened);
>> +
>> +static int
>> +virStorageBackendGlusterPreopenedSet(virStorageSourcePtr src,
>
> At this point it's not really preopened. I find this naming is weird.
>
> virStorageBackendGlusterCacheAdd ?
Only reason to keep *preopened* in the naming was to maintain the
similarity with qemu & samba.
That is not desirable. The naming should be more consistent with libvirt
itself.
Peter