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

On Wed, Nov 30, 2016 at 4:59 PM, Daniel P. Berrange <berrange(a)redhat.com&gt; wrote: > 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 Thanks for correcting me here. > > >> +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,} yeah, pretty straight! > >> +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. Perfectly make sense. > >> + 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. If we try to keep this in virOnce intializer (executed only once during process life time), then we end up not having a virMutexDestroy(). Is that okay ?