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

[Peter Krempa]

On Tue, Dec 06, 2016 at 22:48:39 +0530, Prasanna Kalever wrote:
> On Mon, Dec 5, 2016 at 8:08 PM, Peter Krempa <pkrempa at redhat.com> 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 at redhat.com>
> >> ---
> >>  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' ?

The problem is that you did not follow how libvirt is using virOnce
(VIR_ONCE_GLOBAL_INIT) and tried to do it yourself. Libvirt has multiple
examples that return error codes from one-time initializers.

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

That is not desirable. The naming should be more consistent with libvirt
itself.

Peter
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20161207/b70b4f72/attachment-0001.sig>