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
