Re: [libvirt] [PATCH v2 1/1] gluster: cache glfs connection object per volume

On Wed, Nov 30, 2016 at 04:06:37PM +0530, prasanna.kalever(a)redhat.com wrote: > diff --git a/src/storage/storage_backend_gluster.c b/src/storage/storage_backend_gluster.c > index 8e86704..18b7fc3 100644 > --- a/src/storage/storage_backend_gluster.c > +++ b/src/storage/storage_backend_gluster.c > @@ -31,11 +31,34 @@ > #include "virstoragefile.h" > #include "virstring.h" > #include "viruri.h" > +#include "viratomic.h" > > #define VIR_FROM_THIS VIR_FROM_STORAGE > > VIR_LOG_INIT("storage.storage_backend_gluster"); > > +static bool virGlusterGlobalError; > +static virOnceControl glusterConnOnce = VIR_ONCE_CONTROL_INITIALIZER; > + > +typedef struct _virStorageBackendGlusterState virStorageBackendGlusterState; > +typedef virStorageBackendGlusterState *virStorageBackendGlusterStatePtr; > + > +typedef struct _virStorageFileBackendGlusterPriv virStorageFileBackendGlusterPriv; > +typedef virStorageFileBackendGlusterPriv *virStorageFileBackendGlusterPrivPtr; > + > +typedef struct _unixSocketAddress unixSocketAddress; > + > +typedef struct _inetSocketAddress inetSocketAddress; > + > +typedef struct _glusterServer glusterServer; > +typedef struct _glusterServer *glusterServerPtr; *all* structs should be fully namespaced. ie have a VirStorageBackendGluster name prefix

> +struct _virStorageBackendGlusterStatePreopened { > + virStorageFileBackendGlusterPrivPtr priv; > + unsigned int nservers; size_t > + glusterServerPtr *hosts; > + char *volname; > + volatile int ref; > +}; > + > +struct _virStorageBackendGlusterconnCache { > + virMutex lock; > + size_t nConn; > + virStorageBackendGlusterStatePtrPreopened *conn; > +}; > + > +virStorageBackendGlusterconnCachePtr connCache = {0,}; 'static' and all static vars are initialized to zero so kill {0,}

> +static void > +virGlusterConnAlloc(void) > +{ > + if ((VIR_ALLOC(connCache) < 0)) > + virGlusterGlobalError = false; > +} > + > +static int > +virStorageBackendGlusterSetPreopened(virStorageSourcePtr src, > + virStorageFileBackendGlusterPrivPtr priv) > +{ > + unsigned int idx; > + virStorageBackendGlusterStatePtrPreopened entry = NULL; > + > + if (connCache == NULL && (virOnce(&glusterConnOnce, > + virGlusterConnAlloc) < 0)) > + return -1; Checking if connCache is NULL before calling virOnce is pointless. The whole point of virOnce is that you can blindly call it multiple times and it'll only run once.

> + if (virGlusterGlobalError) > + return -1; > + > + if (!connCache->nConn) { > + if (virMutexInit(&connCache->lock) < 0) { > + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", > + _("Unable to initialize mutex")); > + return -1; > + } > + } This is not thread safe - mutex initialization should be in the virOnce intializer.

> + > + for (idx = 0; idx < connCache->nConn; idx++) { > + if (STREQ(connCache->conn[idx]->volname, src->volume)) > + return 0; > + } Not thread safe since connCache is not locked at this time > + > + if (VIR_ALLOC(entry) < 0) > + 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; > + > + for (idx = 0; idx < src->nhosts; idx++) { > + if (VIR_ALLOC(entry->hosts[idx]) < 0) > + goto error; > + if (src->hosts[idx].transport == VIR_STORAGE_NET_HOST_TRANS_UNIX) { > + if (VIR_STRDUP(entry->hosts[idx]->u.uds.path, > + src->hosts[idx].socket) < 0) > + goto error; > + entry->hosts[idx]->type = VIR_STORAGE_NET_HOST_TRANS_UNIX; > + } else if (src->hosts[idx].transport == > + VIR_STORAGE_NET_HOST_TRANS_TCP) { > + if (VIR_STRDUP(entry->hosts[idx]->u.tcp.host, > + src->hosts[idx].name) < 0) > + goto error; > + if (VIR_STRDUP(entry->hosts[idx]->u.tcp.port, > + src->hosts[idx].port) < 0) > + goto error; > + entry->hosts[idx]->type = VIR_STORAGE_NET_HOST_TRANS_TCP; > + } else { > + entry->hosts[idx]->type = VIR_STORAGE_NET_HOST_TRANS_LAST; > + } > + } > + entry->priv = priv; > + > + virMutexLock(&connCache->lock); > + virAtomicIntSet(&entry->ref, 1); > + if (VIR_INSERT_ELEMENT(connCache->conn, -1, connCache->nConn, entry) < 0) { > + virMutexUnlock(&connCache->lock); > + goto error; > + } > + virMutexUnlock(&connCache->lock); > + > + return 0; > + > + error: > + for (idx = 0; idx < entry->nservers; idx++) { > + if (entry->hosts[idx]->type == VIR_STORAGE_NET_HOST_TRANS_UNIX) { > + VIR_FREE(entry->hosts[idx]->u.uds.path); > + } else { > + VIR_FREE(entry->hosts[idx]->u.tcp.host); > + VIR_FREE(entry->hosts[idx]->u.tcp.port); > + } > + VIR_FREE(entry->hosts[idx]); > + } > + > + VIR_FREE(entry->hosts); > + VIR_FREE(entry->volname); > + VIR_FREE(entry); > + > + return -1; > +} > + > +static glfs_t * > +virStorageBackendGlusterFindPreopened(virStorageSourcePtr src) > +{ > + unsigned int cIdx, sIdx, nIdx; /* connectionIdx, savedIdx, newIdx */ > + bool flag = false; > + > + if (connCache == NULL) > + return NULL; Not thread safe, since something could be in middlle of calling the virOnce to initialize this. > + > + virStorageBackendGlusterStatePtrPreopened entry; > + > + for (cIdx = 0; cIdx < connCache->nConn; cIdx++) { > + entry = connCache->conn[cIdx]; You've not locked connCache, and same in later methods > + if (STREQ(entry->volname, src->volume)) { > + if (entry->nservers == src->nhosts) { > + for (sIdx = 0; sIdx < entry->nservers; sIdx++) { > + for (nIdx = 0; nIdx < src->nhosts; nIdx++) { > + if (entry->hosts[sIdx]->type == > + src->hosts[nIdx].transport) { > + if (entry->hosts[sIdx]->type > + == VIR_STORAGE_NET_HOST_TRANS_UNIX) { > + if (STREQ(entry->hosts[sIdx]->u.uds.path, > + src->hosts[nIdx].socket)) { > + flag = true; > + break; > + } > + } else { > + if (STREQ(entry->hosts[sIdx]->u.tcp.host, > + src->hosts[nIdx].name) && > + STREQ(entry->hosts[sIdx]->u.tcp.port, > + src->hosts[nIdx].port)) { > + flag = true; > + break; > + } > + } > + } > + } > + if (!flag) > + return NULL; > + flag = false; > + } > + virAtomicIntInc(&entry->ref); > + return entry->priv->vol; > + } else { > + return NULL; > + } > + } > + } > + return NULL; > +} Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://entangle-photo.org -o- http://search.cpan.org/~danberr/ :|