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

[Daniel P. Berrange]

On Wed, Nov 30, 2016 at 05:47:46PM +0530, Prasanna Kalever wrote:
> On Wed, Nov 30, 2016 at 4:59 PM, Daniel P. Berrange <berrange at redhat.com> wrote:
> > On Wed, Nov 30, 2016 at 04:06:37PM +0530, prasanna.kalever at 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 ?

It is required - this is global state that needs to exist forever.
If you try to destroy the mutex you'll create thread safety problems.


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/ :|