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