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

[Prasanna Kalever]

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 ?

If that is acceptable we can move mutex initialization into virOnce
intializer or else we end up issue while re initialization of mutex
var.

Rest of the comments on locks looks fine to me.

Thanks Daniel.

--
Prasanna


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