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@xxxxxxxxxx>
> ---
> 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.
> +
> +
> +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 ?
> + virStorageFileBackendGlusterPrivPtr priv)
> +{
> + size_t i;
> + virStorageBackendGlusterStatePtrPreopened entry = NULL;
> +
> + if (virOnce(&glusterConnOnce, virStorageBackendGlusterConnInit) < 0)
> + return -1;
> +
> + if (virStorageBackendGlusterPreopenedGlobalError)
> + return -1;
> +
> + virMutexLock(&connCache->lock);
> + for (i = 0; i < connCache->nConn; i++) {
> + if (STREQ(connCache->conn[i]->volname, src->volume)) {
> + virMutexUnlock(&connCache->lock);
This checking is weak, you need to do the same kind of checking as when
looking it up.
> + return 0;
> + }
> + }
> + virMutexUnlock(&connCache->lock);
This is NOT thread safe. Other thread may add the same object while you
are in this part of the code. On the other hand, you should not attempt
to open the gluster connection itself while holding the lock, since it
may get stuck.
> +
> + if (virStorageBackendGlusterStatePreopenedInitialize() < 0)
> + return -1;
> +
> + if (!(entry = virObjectLockableNew(virStorageBackendGlusterStatePreopenedClass)))
> + return -1;
> +
> + if (VIR_STRDUP(entry->volname, src->volume) < 0)
> + goto error;
> +
> + if (VIR_ALLOC_N(entry->hosts, entry->nservers) < 0)
> + goto error;
> +
> + entry->nservers = src->nhosts;
> + if (!(entry->hosts = virStorageNetHostDefCopy(src->nhosts, src->hosts)))
> + goto error;
> + entry->priv = priv;
> +
> + virMutexLock(&connCache->lock);
> + if (VIR_APPEND_ELEMENT(connCache->conn, connCache->nConn, entry) < 0) {
> + virMutexUnlock(&connCache->lock);
> + goto error;
> + }
> + virMutexUnlock(&connCache->lock);
> +
> + return 0;
> +
> + error:
> + virStorageNetHostDefFree(entry->nservers, entry->hosts);
> + VIR_FREE(entry->volname);
> + return -1;
> +}
> +
> +static glfs_t *
> +virStorageBackendGlusterPreopenedFind(virStorageSourcePtr src)
> +{
> + glfs_t *ret = NULL;
> + bool match = false;
> + size_t i;
> + size_t j;
> + size_t k;
> +
> + if (connCache == NULL)
> + return NULL;
> +
> + virStorageBackendGlusterStatePtrPreopened entry;
Variables should be declared only at the beginning of a block.
> +
> + virMutexLock(&connCache->lock);
> + for (i = 0; i < connCache->nConn; i++) {
> + entry = connCache->conn[i];
> + if (!(STREQ(entry->volname, src->volume) &&
> + (entry->nservers == src->nhosts)))
> + continue;
> + for (j = 0; j < entry->nservers; j++) {
> + for (k = 0; k < src->nhosts; k++) {
> + if (entry->hosts[j].type != src->hosts[k].type)
> + continue;
> + if (entry->hosts[j].type == VIR_STORAGE_NET_HOST_TRANS_UNIX) {
Use a switch statement, so that if any further transport is added this
does not need to be refactored.
> + if (STREQ(entry->hosts[j].u.uds.path,
> + src->hosts[k].u.uds.path)) {
> + match = true;
> + break;
> + }
> + } else {
> + if (STREQ(entry->hosts[j].u.inet.addr,
> + src->hosts[k].u.inet.addr) &&
> + STREQ(entry->hosts[j].u.inet.port,
> + src->hosts[k].u.inet.port)) {
> + match = true;
> + break;
> + }
> + }
> + }
> + if (!match)
> + goto unlock;
This jumps to failure, if you have two gluster volumes with the same
name but with a different set of servers, which may be two distinct
volumes. If the first volume is not the one you are looking for you
never find the second one.
> + match = false; /* reset */
> + }
> + virObjectRef(entry);
> + ret = entry->priv->vol;
> + }
> +
> + unlock:
> + virMutexUnlock(&connCache->lock);
> + return ret;
> +}
> +
> +static void
> +virStorageBackendGlusterPreopenedClose(virStorageSourcePtr src)
> +{
> + virStorageFileBackendGlusterPrivPtr priv = src->drv->priv;
> + size_t i;
> +
> + virMutexLock(&connCache->lock);
> + for (i = 0; i < connCache->nConn; i++) {
> + if (STREQ(connCache->conn[i]->volname, src->volume)) {
This matching is not sufficient. You need to do the same level of
matching as when accessing the cache.
> + virObjectUnref(connCache->conn[i]);
This may free the object if this was the last reference.
> + if (connCache->conn[i]->volname) {
So this pointer will be invalid in that case.
> + if (priv && priv->canonpath)
> + VIR_FREE(priv->canonpath);
> + goto unlock;
> + }
> + VIR_DELETE_ELEMENT(connCache->conn, i, connCache->nConn);
This should not be done unconditionally, only if the last reference was
dropped. Otherwise cache removal won't work.
> + VIR_FREE(src->drv);
> + }
> + }
> +
> + unlock:
> + virMutexUnlock(&connCache->lock);
> +}
>
> static void
> virStorageBackendGlusterClose(virStorageBackendGlusterStatePtr state)
> @@ -538,31 +752,15 @@ virStorageBackend virStorageBackendGluster = {
> };
>
>
> -typedef struct _virStorageFileBackendGlusterPriv virStorageFileBackendGlusterPriv;
> -typedef virStorageFileBackendGlusterPriv *virStorageFileBackendGlusterPrivPtr;
> -
> -struct _virStorageFileBackendGlusterPriv {
> - glfs_t *vol;
> - char *canonpath;
> -};
> -
> -
> static void
> virStorageFileBackendGlusterDeinit(virStorageSourcePtr src)
> {
> - virStorageFileBackendGlusterPrivPtr priv = src->drv->priv;
> -
> VIR_DEBUG("deinitializing gluster storage file %p (gluster://%s:%s/%s%s)",
> src, src->hosts->u.inet.addr,
> src->hosts->u.inet.port ? src->hosts->u.inet.port : "0",
> src->volume, src->path);
>
> - if (priv->vol)
> - glfs_fini(priv->vol);
> - VIR_FREE(priv->canonpath);
> -
> - VIR_FREE(priv);
> - src->drv->priv = NULL;
> + virStorageBackendGlusterPreopenedClose(src);
> }
>
> static int
> @@ -626,6 +824,12 @@ virStorageFileBackendGlusterInit(virStorageSourcePtr src)
> if (VIR_ALLOC(priv) < 0)
> return -1;
>
> + priv->vol = virStorageBackendGlusterPreopenedFind(src);
So, this is not thread stafe either. If this does not find the cache
entry ...
> + if (priv->vol) {
> + src->drv->priv = priv;
> + return 0;
> + }
> +
> VIR_DEBUG("initializing gluster storage file %p "
> "(priv='%p' volume='%s' path='%s') as [%u:%u]",
> src, priv, src->volume, src->path,
> @@ -636,6 +840,9 @@ virStorageFileBackendGlusterInit(virStorageSourcePtr src)
> goto error;
> }
It may be created by a second thread, which had the same result in the
cache checking ...
>
> + if (virStorageBackendGlusterPreopenedSet(src, priv) < 0)
> + goto error;
And then both try to add the same stuff.
> +
> for (i = 0; i < src->nhosts; i++) {
> if (virStorageFileBackendGlusterInitServer(priv, src->hosts + i) < 0)
> goto error;
> @@ -653,9 +860,7 @@ virStorageFileBackendGlusterInit(virStorageSourcePtr src)
> return 0;
>
> error:
> - if (priv->vol)
> - glfs_fini(priv->vol);
> - VIR_FREE(priv);
> + virStorageBackendGlusterPreopenedClose(src);
>
> return -1;
> }
> diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c
> index bf7c938..be69adf 100644
> --- a/src/storage/storage_driver.c
> +++ b/src/storage/storage_driver.c
> @@ -2900,11 +2900,8 @@ virStorageFileDeinit(virStorageSourcePtr src)
> if (!virStorageFileIsInitialized(src))
> return;
>
> - if (src->drv->backend &&
> - src->drv->backend->backendDeinit)
> + if (src->drv->backend && src->drv->backend->backendDeinit)
> src->drv->backend->backendDeinit(src);
> -
> - VIR_FREE(src->drv);
This change should be separated. Along with the relevant stuff into a
separate patch.
Attachment:
signature.asc
Description: PGP signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
