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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, Nov 30, 2016 at 04:06:37PM +0530, prasanna.kalever@xxxxxxxxxx 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/ :|

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]
  Powered by Linux