Re: [PATCH v3 3/3] gluster: cache glfs connection object per volume

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

 



On Mon, Dec 5, 2016 at 8:08 PM, Peter Krempa <pkrempa@xxxxxxxxxx> wrote:
> On Mon, Dec 05, 2016 at 18:55:19 +0530, Prasanna Kumar Kalever wrote:
>> Currently, in case if we have 4 extra attached disks, then for each disk
>> we need to call 'glfs_init' (over network) and friends which could be costly.
>>
>> Additionally snapshot(external) scenario will further complex the situation.
>>
>> This patch maintain a cache of glfs objects per volume, hence the
>> all disks/snapshots belonging to the volume whose glfs object is cached
>> can avoid calls to 'glfs_init' and friends by simply taking a ref on
>> existing object.
>>
>> The glfs object is shared only if volume name and all the volfile servers match
>> (includes hostname, transport and port number).
>>
>> Thanks to 'Peter Krempa' for all the inputs.
>>
>> Signed-off-by: Prasanna Kumar Kalever <prasanna.kalever@xxxxxxxxxx>
>> ---
>>  src/storage/storage_backend_fs.c      |   2 +
>>  src/storage/storage_backend_gluster.c | 249 +++++++++++++++++++++++++++++++---
>>  src/storage/storage_driver.c          |   5 +-
>>  3 files changed, 230 insertions(+), 26 deletions(-)
>>
>> diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c
>> index de0e8d5..0e03e06 100644
>> --- a/src/storage/storage_backend_fs.c
>> +++ b/src/storage/storage_backend_fs.c
>> @@ -1488,6 +1488,8 @@ virStorageFileBackendFileDeinit(virStorageSourcePtr src)
>>
>>      VIR_FREE(priv->canonpath);
>>      VIR_FREE(priv);
>> +
>> +    VIR_FREE(src->drv);
>
> See at the end.
>
>>  }
>>
>>
>> diff --git a/src/storage/storage_backend_gluster.c b/src/storage/storage_backend_gluster.c
>> index 0d5b7f6..1f85dfa 100644
>> --- a/src/storage/storage_backend_gluster.c
>> +++ b/src/storage/storage_backend_gluster.c
>> @@ -31,11 +31,26 @@
>>  #include "virstoragefile.h"
>>  #include "virstring.h"
>>  #include "viruri.h"
>> +#include "viratomic.h"
>
> It doesn't look like you use it in this iteration.
>
>>
>>  #define VIR_FROM_THIS VIR_FROM_STORAGE
>>
>>  VIR_LOG_INIT("storage.storage_backend_gluster");
>>
>> +
>> +typedef struct _virStorageBackendGlusterState virStorageBackendGlusterState;
>> +typedef virStorageBackendGlusterState *virStorageBackendGlusterStatePtr;
>> +
>> +typedef struct _virStorageFileBackendGlusterPriv virStorageFileBackendGlusterPriv;
>> +typedef virStorageFileBackendGlusterPriv *virStorageFileBackendGlusterPrivPtr;
>> +
>> +typedef struct _virStorageBackendGlusterStatePreopened virStorageBackendGlusterStatePreopened;
>> +typedef virStorageBackendGlusterStatePreopened *virStorageBackendGlusterStatePtrPreopened;
>> +
>> +typedef struct _virStorageBackendGlusterconnCache virStorageBackendGlusterconnCache;
>> +typedef virStorageBackendGlusterconnCache *virStorageBackendGlusterconnCachePtr;
>> +
>> +
>>  struct _virStorageBackendGlusterState {
>>      glfs_t *vol;
>>
>> @@ -47,8 +62,207 @@ struct _virStorageBackendGlusterState {
>>      char *dir; /* dir from URI, or "/"; always starts and ends in '/' */
>>  };
>>
>> -typedef struct _virStorageBackendGlusterState virStorageBackendGlusterState;
>> -typedef virStorageBackendGlusterState *virStorageBackendGlusterStatePtr;
>> +struct _virStorageFileBackendGlusterPriv {
>> +    glfs_t *vol;
>> +    char *canonpath;
>> +};
>> +
>> +struct _virStorageBackendGlusterStatePreopened {
>> +    virObjectLockable parent;
>> +    virStorageFileBackendGlusterPrivPtr priv;
>> +    size_t nservers;
>> +    virStorageNetHostDefPtr hosts;
>> +    char *volname;
>> +};
>> +
>> +struct _virStorageBackendGlusterconnCache {
>> +    virMutex lock;
>> +    size_t nConn;
>> +    virStorageBackendGlusterStatePtrPreopened *conn;
>> +};
>> +
>> +
>> +static virStorageBackendGlusterconnCachePtr connCache;
>
> connCache still does not follow the naming scheme that was requested by
> Dan.
>
>> +static virClassPtr virStorageBackendGlusterStatePreopenedClass;
>> +static virOnceControl glusterConnOnce = VIR_ONCE_CONTROL_INITIALIZER;
>
> This neither.
>
>> +static bool virStorageBackendGlusterPreopenedGlobalError;
>
> I don't see a reason to have this as a global flag.

But how do I bounce the errors from 'virStorageBackendGlusterConnInit' ?

>
>> +
>> +
>> +static void virStorageBackendGlusterStatePreopenedDispose(void *obj);
>> +
>> +
>> +static int virStorageBackendGlusterStatePreopenedOnceInit(void)
>> +{
>> +    if (!(virStorageBackendGlusterStatePreopenedClass =
>> +                                virClassNew(virClassForObjectLockable(),
>> +                                "virStorageBackendGlusterStatePreopenedClass",
>> +                                sizeof(virStorageBackendGlusterStatePreopened),
>> +                                virStorageBackendGlusterStatePreopenedDispose)))
>> +        return -1;
>> +
>> +    return 0;
>> +}
>> +
>> +static void virStorageBackendGlusterStatePreopenedDispose(void *object)
>> +{
>> +    virStorageBackendGlusterStatePtrPreopened entry = object;
>> +
>> +    glfs_fini(entry->priv->vol);
>> +
>> +    VIR_FREE(entry->priv->canonpath);
>> +    VIR_FREE(entry->priv);
>> +    VIR_FREE(entry->volname);
>> +
>> +    virStorageNetHostDefFree(entry->nservers, entry->hosts);
>> +}
>> +
>> +static void
>> +virStorageBackendGlusterConnInit(void)
>> +{
>> +    if (VIR_ALLOC(connCache) < 0) {
>> +        virStorageBackendGlusterPreopenedGlobalError = false;
>> +        return;
>> +    }
>> +
>> +    if (virMutexInit(&connCache->lock) < 0) {
>> +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>> +                       _("Unable to initialize mutex"));
>> +        virStorageBackendGlusterPreopenedGlobalError = false;
>> +    }
>> +}
>> +
>> +VIR_ONCE_GLOBAL_INIT(virStorageBackendGlusterStatePreopened);
>> +
>> +static int
>> +virStorageBackendGlusterPreopenedSet(virStorageSourcePtr src,
>
> At this point it's not really preopened. I find this naming is weird.
>
> virStorageBackendGlusterCacheAdd ?

Only reason to keep *preopened* in the naming was to maintain the
similarity with qemu & samba.
But I don't think it is bad to compromise, in the next spin will use
virStorageBackendGlusterCache*

I agree with all the rest of the comments.

Will drop the changes soon.

Thanks a lot Peter!
--
Prasanna

>
>> +                                     virStorageFileBackendGlusterPrivPtr priv)
>> +{
>> +    size_t i;
>> +    virStorageBackendGlusterStatePtrPreopened entry = NULL;
>> +
>> +    if (virOnce(&glusterConnOnce, virStorageBackendGlusterConnInit) < 0)
>> +        return -1;
>> +
>> +    if (virStorageBackendGlusterPreopenedGlobalError)
>> +        return -1;
>> +
>> +    virMutexLock(&connCache->lock);
>> +    for (i = 0; i < connCache->nConn; i++) {
>> +        if (STREQ(connCache->conn[i]->volname, src->volume)) {
>> +            virMutexUnlock(&connCache->lock);
>
> This checking is weak, you need to do the same kind of checking as when
> looking it up.
>
>> +            return 0;
>> +        }
>> +    }
>> +    virMutexUnlock(&connCache->lock);
>
> This is NOT thread safe. Other thread may add the same object while you
> are in this part of the code. On the other hand, you should not attempt
> to open the gluster connection itself while holding the lock, since it
> may get stuck.
>
>> +
>> +    if (virStorageBackendGlusterStatePreopenedInitialize() < 0)
>> +        return -1;
>> +
>> +    if (!(entry = virObjectLockableNew(virStorageBackendGlusterStatePreopenedClass)))
>> +        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;
>> +    if (!(entry->hosts = virStorageNetHostDefCopy(src->nhosts, src->hosts)))
>> +        goto error;
>> +    entry->priv = priv;
>> +
>> +    virMutexLock(&connCache->lock);
>> +    if (VIR_APPEND_ELEMENT(connCache->conn, connCache->nConn, entry) < 0) {
>> +        virMutexUnlock(&connCache->lock);
>> +        goto error;
>> +    }
>> +    virMutexUnlock(&connCache->lock);
>> +
>> +    return 0;
>> +
>> + error:
>> +    virStorageNetHostDefFree(entry->nservers, entry->hosts);
>> +    VIR_FREE(entry->volname);
>> +    return -1;
>> +}
>> +
>> +static glfs_t *
>> +virStorageBackendGlusterPreopenedFind(virStorageSourcePtr src)
>> +{
>> +    glfs_t *ret = NULL;
>> +    bool match = false;
>> +    size_t i;
>> +    size_t j;
>> +    size_t k;
>> +
>> +    if (connCache == NULL)
>> +        return NULL;
>> +
>> +    virStorageBackendGlusterStatePtrPreopened entry;
>
> Variables should be declared only at the beginning of a block.
>
>> +
>> +    virMutexLock(&connCache->lock);
>> +    for (i = 0; i < connCache->nConn; i++) {
>> +        entry = connCache->conn[i];
>> +        if (!(STREQ(entry->volname, src->volume) &&
>> +            (entry->nservers == src->nhosts)))
>> +            continue;
>> +        for (j = 0; j < entry->nservers; j++) {
>> +            for (k = 0; k < src->nhosts; k++) {
>> +                if (entry->hosts[j].type != src->hosts[k].type)
>> +                    continue;
>> +                if (entry->hosts[j].type == VIR_STORAGE_NET_HOST_TRANS_UNIX) {
>
> Use a switch statement, so that if any further transport is added this
> does not need to be refactored.
>
>> +                    if (STREQ(entry->hosts[j].u.uds.path,
>> +                              src->hosts[k].u.uds.path)) {
>> +                        match = true;
>> +                        break;
>> +                    }
>> +                } else {
>> +                    if (STREQ(entry->hosts[j].u.inet.addr,
>> +                              src->hosts[k].u.inet.addr) &&
>> +                        STREQ(entry->hosts[j].u.inet.port,
>> +                              src->hosts[k].u.inet.port)) {
>> +                        match = true;
>> +                        break;
>> +                    }
>> +                }
>> +            }
>> +            if (!match)
>> +                goto unlock;
>
> This jumps to failure, if you have two gluster volumes with the same
> name but with a different set of servers, which may be two distinct
> volumes. If the first volume is not the one you are looking for you
> never find the second one.
>
>> +            match = false; /* reset */
>> +        }
>> +        virObjectRef(entry);
>> +        ret = entry->priv->vol;
>> +    }
>> +
>> + unlock:
>> +    virMutexUnlock(&connCache->lock);
>> +    return ret;
>> +}
>> +
>> +static void
>> +virStorageBackendGlusterPreopenedClose(virStorageSourcePtr src)
>> +{
>> +    virStorageFileBackendGlusterPrivPtr priv = src->drv->priv;
>> +    size_t i;
>> +
>> +    virMutexLock(&connCache->lock);
>> +    for (i = 0; i < connCache->nConn; i++) {
>> +        if (STREQ(connCache->conn[i]->volname, src->volume)) {
>
> This matching is not sufficient. You need to do the same level of
> matching as when accessing the cache.
>
>> +            virObjectUnref(connCache->conn[i]);
>
> This may free the object if this was the last reference.
>
>> +            if (connCache->conn[i]->volname) {
>
> So this pointer will be invalid in that case.
>
>> +                if (priv && priv->canonpath)
>> +                    VIR_FREE(priv->canonpath);
>> +                goto unlock;
>> +            }
>> +            VIR_DELETE_ELEMENT(connCache->conn, i, connCache->nConn);
>
> This should not be done unconditionally, only if the last reference was
> dropped. Otherwise cache removal won't work.
>
>> +            VIR_FREE(src->drv);
>> +        }
>> +    }
>> +
>> + unlock:
>> +    virMutexUnlock(&connCache->lock);
>> +}
>>
>>  static void
>>  virStorageBackendGlusterClose(virStorageBackendGlusterStatePtr state)
>> @@ -538,31 +752,15 @@ virStorageBackend virStorageBackendGluster = {
>>  };
>>
>>
>> -typedef struct _virStorageFileBackendGlusterPriv virStorageFileBackendGlusterPriv;
>> -typedef virStorageFileBackendGlusterPriv *virStorageFileBackendGlusterPrivPtr;
>> -
>> -struct _virStorageFileBackendGlusterPriv {
>> -    glfs_t *vol;
>> -    char *canonpath;
>> -};
>> -
>> -
>>  static void
>>  virStorageFileBackendGlusterDeinit(virStorageSourcePtr src)
>>  {
>> -    virStorageFileBackendGlusterPrivPtr priv = src->drv->priv;
>> -
>>      VIR_DEBUG("deinitializing gluster storage file %p (gluster://%s:%s/%s%s)",
>>                src, src->hosts->u.inet.addr,
>>                src->hosts->u.inet.port ? src->hosts->u.inet.port : "0",
>>                src->volume, src->path);
>>
>> -    if (priv->vol)
>> -        glfs_fini(priv->vol);
>> -    VIR_FREE(priv->canonpath);
>> -
>> -    VIR_FREE(priv);
>> -    src->drv->priv = NULL;
>> +    virStorageBackendGlusterPreopenedClose(src);
>>  }
>>
>>  static int
>> @@ -626,6 +824,12 @@ virStorageFileBackendGlusterInit(virStorageSourcePtr src)
>>      if (VIR_ALLOC(priv) < 0)
>>          return -1;
>>
>> +    priv->vol = virStorageBackendGlusterPreopenedFind(src);
>
> So, this is not thread stafe either. If this does not find the cache
> entry ...
>
>> +    if (priv->vol) {
>> +        src->drv->priv = priv;
>> +        return 0;
>> +    }
>> +
>>      VIR_DEBUG("initializing gluster storage file %p "
>>                "(priv='%p' volume='%s' path='%s') as [%u:%u]",
>>                src, priv, src->volume, src->path,
>> @@ -636,6 +840,9 @@ virStorageFileBackendGlusterInit(virStorageSourcePtr src)
>>          goto error;
>>      }
>
> It may be created by a second thread, which had the same result in the
> cache checking ...
>
>>
>> +    if (virStorageBackendGlusterPreopenedSet(src, priv) < 0)
>> +        goto error;
>
> And then both try to add the same stuff.
>
>> +
>>      for (i = 0; i < src->nhosts; i++) {
>>          if (virStorageFileBackendGlusterInitServer(priv, src->hosts + i) < 0)
>>              goto error;
>> @@ -653,9 +860,7 @@ virStorageFileBackendGlusterInit(virStorageSourcePtr src)
>>      return 0;
>>
>>   error:
>> -    if (priv->vol)
>> -        glfs_fini(priv->vol);
>> -    VIR_FREE(priv);
>> +    virStorageBackendGlusterPreopenedClose(src);
>>
>>      return -1;
>>  }
>> diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c
>> index bf7c938..be69adf 100644
>> --- a/src/storage/storage_driver.c
>> +++ b/src/storage/storage_driver.c
>> @@ -2900,11 +2900,8 @@ virStorageFileDeinit(virStorageSourcePtr src)
>>      if (!virStorageFileIsInitialized(src))
>>          return;
>>
>> -    if (src->drv->backend &&
>> -        src->drv->backend->backendDeinit)
>> +    if (src->drv->backend && src->drv->backend->backendDeinit)
>>          src->drv->backend->backendDeinit(src);
>> -
>> -    VIR_FREE(src->drv);
>
> This change should be separated. Along with the relevant stuff into a
> separate patch.

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