On Tue, Dec 06, 2016 at 22:48:39 +0530, Prasanna Kalever wrote: > On Mon, Dec 5, 2016 at 8:08 PM, Peter Krempa <[email protected]> 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 <[email protected]> > >> --- > >> 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
signature.asc
Description: PGP signature
-- libvir-list mailing list [email protected] https://www.redhat.com/mailman/listinfo/libvir-list
