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@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' ?
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
Attachment:
signature.asc
Description: PGP signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
