[libvirt] [PATCH v5 0/3] gluster: cache glfs connection object per volume
v5: Address review comments from Peter on v4 Dropping Patch 3 from v3 Patch 1: fix transport type in all drivers, possible switch case converts Patch 2: use virObject with src->drv to avoid 'initFlag' for remembering 'virStorageFileInit' status Patch 3: * change virStorageBackendGlusterCacheQuery to *Lookup * change virStorageBackendGlusterCacheRefresh to *Release * Avoid the global Flags * Thread safe measure by using virCond* Thanks Peter! v4: Address review comments from Peter on v3 Patch 1: changes to fix test failures (qemuargv2xmltest, qemuxml2argvtest, qemuxml2xmltest and virstoragetest) Patch 2: fix hung in qemuxml2argvtest Patch 3 v3: Split this Patch to 2 parts as below Patch 3 v4: don't free metadata used for storage driver access Patch 4: * update variable naming as per standards * virStorageBackendGlusterPreopened* -> virStorageBackendGlusterCache* Thanks Peter! v3: Address comments by Daniel and Peter on v2 * Split the patch to 3 parts Patch 1: change the virStorageNetHostDef type Patch 2: optimize calls to virStorageFileInit and friends Patch 3: add the caching for glfs * Thanks to Daniel, this version make all the methods as thread safe * Thanks to Peter for pointing use of virObjectLockable, this had simplified rest of the parts a lot. v2: Address review comments from Peter on v1 * Rebased on latest master * Changes to commit msg * Introduce storage API's for Register and Unregister of volume * During qemu process Start/Stop and snapshot create * Check Transport and Port type * Atomic element add/del to list and ref counting Pending: Treating IP and FQDN belong to same host v1: Initial patch Prasanna Kumar Kalever (3): util: change the virStorageNetHostDef type storage: optimize calls to virStorageFileInit and friends gluster: cache glfs connection object per volume src/conf/domain_conf.c | 60 +++-- src/libxl/libxl_conf.c | 10 +- src/qemu/qemu_command.c | 97 ++++--- src/qemu/qemu_domain.c | 25 +- src/qemu/qemu_domain.h | 8 +- src/qemu/qemu_driver.c | 26 +- src/qemu/qemu_parse_command.c | 50 ++-- src/qemu/qemu_process.c | 49 ++++ src/qemu/qemu_process.h | 4 + src/storage/storage_backend.h | 1 + src/storage/storage_backend_fs.c | 5 + src/storage/storage_backend_gluster.c | 329 ++++++++++++++++++++--- src/storage/storage_driver.c | 40 ++- src/storage/storage_driver.h | 2 + src/util/virstoragefile.c | 93 ++++--- src/util/virstoragefile.h | 21 +- src/xenconfig/xen_xl.c | 10 +- tests/domainsnapshotxml2xmlin/disk_snapshot.xml | 2 +- tests/domainsnapshotxml2xmlout/disk_snapshot.xml | 2 +- tests/virstoragetest.c | 2 +- 20 files changed, 614 insertions(+), 222 deletions(-) -- 2.7.4
Currently, the Host object looks like struct _virStorageNetHostDef { char *name; char *port; int transport; /* virStorageNetHostTransport */ char *socket; /* path to unix socket */ } We don't actually need a 'name' and 'port' if the transport type is unix domain sockets, and if the transport is inet tcp/rdma we don't actually care for socket path. With a simple union discrimination we can make this much better struct _virStorageNetHostUnixSockAddr { char *path; }; struct _virStorageNetHostInetSockAddr { char *addr; char *port; }; struct _virStorageNetHostDef { virStorageNetHostTransport type; union { /* union tag is @type */ virStorageNetHostUnixSockAddr uds; virStorageNetHostInetSockAddr inet; } u; }; This patch performs the required changes in transforming _virStorageNetHostDef to fit union discrimination. Signed-off-by: Prasanna Kumar Kalever <prasanna.kalever@redhat.com> --- src/conf/domain_conf.c | 60 +++++++++------ src/libxl/libxl_conf.c | 10 +-- src/qemu/qemu_command.c | 97 ++++++++++++++---------- src/qemu/qemu_parse_command.c | 50 ++++++------ src/storage/storage_backend_gluster.c | 21 ++--- src/storage/storage_driver.c | 7 +- src/util/virstoragefile.c | 93 +++++++++++++---------- src/util/virstoragefile.h | 20 ++++- src/xenconfig/xen_xl.c | 10 +-- tests/domainsnapshotxml2xmlin/disk_snapshot.xml | 2 +- tests/domainsnapshotxml2xmlout/disk_snapshot.xml | 2 +- tests/virstoragetest.c | 2 +- 12 files changed, 214 insertions(+), 160 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 0e879e1..d3684ee 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -5967,6 +5967,7 @@ virDomainStorageHostParse(xmlNodePtr node, int ret = -1; xmlNodePtr child; char *transport = NULL; + char *socket = NULL; virStorageNetHostDef host; memset(&host, 0, sizeof(host)); @@ -5976,12 +5977,13 @@ virDomainStorageHostParse(xmlNodePtr node, if (child->type == XML_ELEMENT_NODE && xmlStrEqual(child->name, BAD_CAST "host")) { - host.transport = VIR_STORAGE_NET_HOST_TRANS_TCP; + host.type = VIR_STORAGE_NET_HOST_TRANS_TCP; /* transport can be tcp (default), unix or rdma. */ if ((transport = virXMLPropString(child, "transport"))) { - host.transport = virStorageNetHostTransportTypeFromString(transport); - if (host.transport < 0) { + host.type = virStorageNetHostTransportTypeFromString(transport); + if (host.type < 0 || + host.type >= VIR_STORAGE_NET_HOST_TRANS_LAST) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("unknown protocol transport type '%s'"), transport); @@ -5989,17 +5991,17 @@ virDomainStorageHostParse(xmlNodePtr node, } } - host.socket = virXMLPropString(child, "socket"); + socket = virXMLPropString(child, "socket"); - if (host.transport == VIR_STORAGE_NET_HOST_TRANS_UNIX && - host.socket == NULL) { + if (host.type == VIR_STORAGE_NET_HOST_TRANS_UNIX && + socket == NULL) { virReportError(VIR_ERR_XML_ERROR, "%s", _("missing socket for unix transport")); goto cleanup; } - if (host.transport != VIR_STORAGE_NET_HOST_TRANS_UNIX && - host.socket != NULL) { + if (host.type != VIR_STORAGE_NET_HOST_TRANS_UNIX && + socket != NULL) { virReportError(VIR_ERR_XML_ERROR, _("transport '%s' does not support " "socket attribute"), @@ -6007,16 +6009,17 @@ virDomainStorageHostParse(xmlNodePtr node, goto cleanup; } + host.u.uds.path = socket; VIR_FREE(transport); - if (host.transport != VIR_STORAGE_NET_HOST_TRANS_UNIX) { - if (!(host.name = virXMLPropString(child, "name"))) { + if (host.type != VIR_STORAGE_NET_HOST_TRANS_UNIX) { + if (!(host.u.inet.addr = virXMLPropString(child, "name"))) { virReportError(VIR_ERR_XML_ERROR, "%s", _("missing name for host")); goto cleanup; } - host.port = virXMLPropString(child, "port"); + host.u.inet.port = virXMLPropString(child, "port"); } if (VIR_APPEND_ELEMENT(*hosts, *nhosts, host) < 0) @@ -14065,8 +14068,8 @@ virDomainHostdevMatchSubsysSCSIiSCSI(virDomainHostdevDefPtr first, virDomainHostdevSubsysSCSIiSCSIPtr second_iscsisrc = &second->source.subsys.u.scsi.u.iscsi; - if (STREQ(first_iscsisrc->hosts[0].name, second_iscsisrc->hosts[0].name) && - STREQ(first_iscsisrc->hosts[0].port, second_iscsisrc->hosts[0].port) && + if (STREQ(first_iscsisrc->hosts[0].u.inet.addr, second_iscsisrc->hosts[0].u.inet.addr) && + STREQ(first_iscsisrc->hosts[0].u.inet.port, second_iscsisrc->hosts[0].u.inet.port) && STREQ(first_iscsisrc->path, second_iscsisrc->path)) return 1; return 0; @@ -20322,17 +20325,26 @@ virDomainDiskSourceFormatInternal(virBufferPtr buf, for (n = 0; n < src->nhosts; n++) { virBufferAddLit(buf, "<host"); - virBufferEscapeString(buf, " name='%s'", - src->hosts[n].name); - virBufferEscapeString(buf, " port='%s'", - src->hosts[n].port); - if (src->hosts[n].transport) + if (src->hosts[n].type) virBufferAsprintf(buf, " transport='%s'", - virStorageNetHostTransportTypeToString(src->hosts[n].transport)); - - virBufferEscapeString(buf, " socket='%s'", - src->hosts[n].socket); + virStorageNetHostTransportTypeToString(src->hosts[n].type)); + + switch (src->hosts[n].type) { + case VIR_STORAGE_NET_HOST_TRANS_TCP: + case VIR_STORAGE_NET_HOST_TRANS_RDMA: + virBufferEscapeString(buf, " name='%s'", + src->hosts[n].u.inet.addr); + virBufferEscapeString(buf, " port='%s'", + src->hosts[n].u.inet.port); + break; + case VIR_STORAGE_NET_HOST_TRANS_UNIX: + virBufferEscapeString(buf, " socket='%s'", + src->hosts[n].u.uds.path); + break; + case VIR_STORAGE_NET_HOST_TRANS_LAST: + break; + } virBufferAddLit(buf, "/>\n"); } @@ -21103,8 +21115,8 @@ virDomainHostdevDefFormatSubsys(virBufferPtr buf, case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI: if (scsisrc->protocol == VIR_DOMAIN_HOSTDEV_SCSI_PROTOCOL_TYPE_ISCSI) { virBufferAddLit(buf, "<host"); - virBufferEscapeString(buf, " name='%s'", iscsisrc->hosts[0].name); - virBufferEscapeString(buf, " port='%s'", iscsisrc->hosts[0].port); + virBufferEscapeString(buf, " name='%s'", iscsisrc->hosts[0].u.inet.addr); + virBufferEscapeString(buf, " port='%s'", iscsisrc->hosts[0].u.inet.port); virBufferAddLit(buf, "/>\n"); } else { virBufferAsprintf(buf, "<adapter name='%s'/>\n", diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c index dcf8e7e..3d80ea8 100644 --- a/src/libxl/libxl_conf.c +++ b/src/libxl/libxl_conf.c @@ -637,14 +637,14 @@ libxlMakeNetworkDiskSrcStr(virStorageSourcePtr src, virBufferAddLit(&buf, "\\;"); /* assume host containing : is ipv6 */ - if (strchr(src->hosts[i].name, ':')) + if (strchr(src->hosts[i].u.inet.addr, ':')) virBufferEscape(&buf, '\\', ":", "[%s]", - src->hosts[i].name); + src->hosts[i].u.inet.addr); else - virBufferAsprintf(&buf, "%s", src->hosts[i].name); + virBufferAsprintf(&buf, "%s", src->hosts[i].u.inet.addr); - if (src->hosts[i].port) - virBufferAsprintf(&buf, "\\:%s", src->hosts[i].port); + if (src->hosts[i].u.inet.port) + virBufferAsprintf(&buf, "\\:%s", src->hosts[i].u.inet.port); } } diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 6c457dd..39c1fdc 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -822,19 +822,18 @@ qemuBuildGlusterDriveJSONHosts(virStorageSourcePtr src) for (i = 0; i < src->nhosts; i++) { host = src->hosts + i; - transport = virStorageNetHostTransportTypeToString(host->transport); - portstr = host->port; + transport = virStorageNetHostTransportTypeToString(host->type); if (virJSONValueObjectCreate(&server, "s:type", transport, NULL) < 0) goto cleanup; - if (!portstr) - portstr = QEMU_DEFAULT_GLUSTER_PORT; - - switch ((virStorageNetHostTransport) host->transport) { + switch ((virStorageNetHostTransport) host->type) { case VIR_STORAGE_NET_HOST_TRANS_TCP: + portstr = host->u.inet.port; + if (!portstr) + portstr = QEMU_DEFAULT_GLUSTER_PORT; if (virJSONValueObjectAdd(server, - "s:host", host->name, + "s:host", host->u.inet.addr, "s:port", portstr, NULL) < 0) goto cleanup; @@ -842,7 +841,7 @@ qemuBuildGlusterDriveJSONHosts(virStorageSourcePtr src) case VIR_STORAGE_NET_HOST_TRANS_UNIX: if (virJSONValueObjectAdd(server, - "s:socket", host->socket, + "s:socket", host->u.uds.path, NULL) < 0) goto cleanup; break; @@ -916,20 +915,41 @@ qemuBuildNetworkDriveURI(virStorageSourcePtr src, if (VIR_ALLOC(uri) < 0) goto cleanup; - if (src->hosts->transport == VIR_STORAGE_NET_HOST_TRANS_TCP) { + switch (src->hosts->type) { + case VIR_STORAGE_NET_HOST_TRANS_TCP: if (VIR_STRDUP(uri->scheme, virStorageNetProtocolTypeToString(src->protocol)) < 0) goto cleanup; - } else { - if (virAsprintf(&uri->scheme, "%s+%s", - virStorageNetProtocolTypeToString(src->protocol), - virStorageNetHostTransportTypeToString(src->hosts->transport)) < 0) + + case VIR_STORAGE_NET_HOST_TRANS_RDMA: + if (VIR_STRDUP(uri->server, src->hosts->u.inet.addr) < 0) + goto cleanup; + + if ((uri->port = qemuNetworkDriveGetPort(src->protocol, + src->hosts->u.inet.port)) < 0) goto cleanup; - } - if ((uri->port = qemuNetworkDriveGetPort(src->protocol, - src->hosts->port)) < 0) + case VIR_STORAGE_NET_HOST_TRANS_UNIX: + if (src->hosts->type != VIR_STORAGE_NET_HOST_TRANS_TCP) { + if (virAsprintf(&uri->scheme, "%s+%s", + virStorageNetProtocolTypeToString(src->protocol), + virStorageNetHostTransportTypeToString(src->hosts->type)) < 0) + goto cleanup; + } + + if (src->hosts->type == VIR_STORAGE_NET_HOST_TRANS_UNIX && + virAsprintf(&uri->query, "socket=%s", src->hosts->u.uds.path) < 0) + goto cleanup; + break; + + case VIR_STORAGE_NET_HOST_TRANS_LAST: + default: + virReportError(VIR_ERR_INTERNAL_ERROR, + _("protocol '%s' doesn't support transport %s"), + virStorageNetProtocolTypeToString(src->protocol), + virStorageNetHostTransportTypeToString(src->hosts->type)); goto cleanup; + } if (src->path) { if (src->volume) { @@ -944,16 +964,9 @@ qemuBuildNetworkDriveURI(virStorageSourcePtr src, } } - if (src->hosts->socket && - virAsprintf(&uri->query, "socket=%s", src->hosts->socket) < 0) - goto cleanup; - if (qemuBuildGeneralSecinfoURI(uri, secinfo) < 0) goto cleanup; - if (VIR_STRDUP(uri->server, src->hosts->name) < 0) - goto cleanup; - ret = virURIFormat(uri); cleanup: @@ -979,38 +992,38 @@ qemuBuildNetworkDriveStr(virStorageSourcePtr src, goto cleanup; } - if (!((src->hosts->name && strchr(src->hosts->name, ':')) || - (src->hosts->transport == VIR_STORAGE_NET_HOST_TRANS_TCP && - !src->hosts->name) || - (src->hosts->transport == VIR_STORAGE_NET_HOST_TRANS_UNIX && - src->hosts->socket && - src->hosts->socket[0] != '/'))) { + if (!((src->hosts->u.inet.addr && strchr(src->hosts->u.inet.addr, ':')) || + (src->hosts->type == VIR_STORAGE_NET_HOST_TRANS_TCP && + !src->hosts->u.inet.addr) || + (src->hosts->type == VIR_STORAGE_NET_HOST_TRANS_UNIX && + src->hosts->u.uds.path && + src->hosts->u.uds.path[0] != '/'))) { virBufferAddLit(&buf, "nbd:"); - switch (src->hosts->transport) { + switch (src->hosts->type) { case VIR_STORAGE_NET_HOST_TRANS_TCP: - virBufferStrcat(&buf, src->hosts->name, NULL); + virBufferStrcat(&buf, src->hosts->u.inet.addr, NULL); virBufferAsprintf(&buf, ":%s", - src->hosts->port ? src->hosts->port : + src->hosts->u.inet.port ? src->hosts->u.inet.port : QEMU_DEFAULT_NBD_PORT); break; case VIR_STORAGE_NET_HOST_TRANS_UNIX: - if (!src->hosts->socket) { + if (!src->hosts->u.uds.path) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("socket attribute required for " "unix transport")); goto cleanup; } - virBufferAsprintf(&buf, "unix:%s", src->hosts->socket); + virBufferAsprintf(&buf, "unix:%s", src->hosts->u.uds.path); break; default: virReportError(VIR_ERR_INTERNAL_ERROR, _("nbd does not support transport '%s'"), - virStorageNetHostTransportTypeToString(src->hosts->transport)); + virStorageNetHostTransportTypeToString(src->hosts->type)); goto cleanup; } @@ -1049,8 +1062,8 @@ qemuBuildNetworkDriveStr(virStorageSourcePtr src, goto cleanup; } else if (src->nhosts == 1) { if (virAsprintf(&ret, "sheepdog:%s:%s:%s", - src->hosts->name, - src->hosts->port ? src->hosts->port : "7000", + src->hosts->u.inet.addr, + src->hosts->u.inet.port ? src->hosts->u.inet.port : "7000", src->path) < 0) goto cleanup; } else { @@ -1084,14 +1097,14 @@ qemuBuildNetworkDriveStr(virStorageSourcePtr src, virBufferAddLit(&buf, "\\;"); /* assume host containing : is ipv6 */ - if (strchr(src->hosts[i].name, ':')) + if (strchr(src->hosts[i].u.inet.addr, ':')) virBufferEscape(&buf, '\\', ":", "[%s]", - src->hosts[i].name); + src->hosts[i].u.inet.addr); else - virBufferAsprintf(&buf, "%s", src->hosts[i].name); + virBufferAsprintf(&buf, "%s", src->hosts[i].u.inet.addr); - if (src->hosts[i].port) - virBufferAsprintf(&buf, "\\:%s", src->hosts[i].port); + if (src->hosts[i].u.inet.port) + virBufferAsprintf(&buf, "\\:%s", src->hosts[i].u.inet.port); } } diff --git a/src/qemu/qemu_parse_command.c b/src/qemu/qemu_parse_command.c index 405e655..d6f7010 100644 --- a/src/qemu/qemu_parse_command.c +++ b/src/qemu/qemu_parse_command.c @@ -77,10 +77,10 @@ qemuParseDriveURIString(virDomainDiskDefPtr def, virURIPtr uri, } if (!transp) { - def->src->hosts->transport = VIR_STORAGE_NET_HOST_TRANS_TCP; + def->src->hosts->type = VIR_STORAGE_NET_HOST_TRANS_TCP; } else { - def->src->hosts->transport = virStorageNetHostTransportTypeFromString(transp); - if (def->src->hosts->transport < 0) { + def->src->hosts->type = virStorageNetHostTransportTypeFromString(transp); + if (def->src->hosts->type < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Invalid %s transport type '%s'"), scheme, transp); goto error; @@ -88,19 +88,20 @@ qemuParseDriveURIString(virDomainDiskDefPtr def, virURIPtr uri, } def->src->nhosts = 0; /* set to 1 once everything succeeds */ - if (def->src->hosts->transport != VIR_STORAGE_NET_HOST_TRANS_UNIX) { - if (VIR_STRDUP(def->src->hosts->name, uri->server) < 0) + switch (def->src->hosts->type) { + case VIR_STORAGE_NET_HOST_TRANS_TCP: + case VIR_STORAGE_NET_HOST_TRANS_RDMA: + if (VIR_STRDUP(def->src->hosts->u.inet.addr, uri->server) < 0) goto error; - if (virAsprintf(&def->src->hosts->port, "%d", uri->port) < 0) + if (virAsprintf(&def->src->hosts->u.inet.port, "%d", uri->port) < 0) goto error; - } else { - def->src->hosts->name = NULL; - def->src->hosts->port = 0; + break; + case VIR_STORAGE_NET_HOST_TRANS_UNIX: if (uri->query) { if (STRPREFIX(uri->query, "socket=")) { sock = strchr(uri->query, '=') + 1; - if (VIR_STRDUP(def->src->hosts->socket, sock) < 0) + if (VIR_STRDUP(def->src->hosts->u.uds.path, sock) < 0) goto error; } else { virReportError(VIR_ERR_INTERNAL_ERROR, @@ -108,6 +109,9 @@ qemuParseDriveURIString(virDomainDiskDefPtr def, virURIPtr uri, goto error; } } + break; + case VIR_STORAGE_NET_HOST_TRANS_LAST: + break; } if (uri->path) { volimg = uri->path + 1; /* skip the prefix slash */ @@ -221,8 +225,8 @@ qemuParseNBDString(virDomainDiskDefPtr disk) if (src) *src++ = '\0'; - h->transport = VIR_STORAGE_NET_HOST_TRANS_UNIX; - if (VIR_STRDUP(h->socket, host + strlen("unix:")) < 0) + h->type = VIR_STORAGE_NET_HOST_TRANS_UNIX; + if (VIR_STRDUP(h->u.uds.path, host + strlen("unix:")) < 0) goto error; } else { port = strchr(host, ':'); @@ -233,14 +237,14 @@ qemuParseNBDString(virDomainDiskDefPtr disk) } *port++ = '\0'; - if (VIR_STRDUP(h->name, host) < 0) + if (VIR_STRDUP(h->u.inet.addr, host) < 0) goto error; src = strchr(port, ':'); if (src) *src++ = '\0'; - if (VIR_STRDUP(h->port, port) < 0) + if (VIR_STRDUP(h->u.inet.port, port) < 0) goto error; } @@ -729,11 +733,10 @@ qemuParseCommandLineDisk(virDomainXMLOptionPtr xmlopt, if (VIR_ALLOC(def->src->hosts) < 0) goto error; def->src->nhosts = 1; - def->src->hosts->name = def->src->path; - if (VIR_STRDUP(def->src->hosts->port, port) < 0) + def->src->hosts->u.inet.addr = def->src->path; + if (VIR_STRDUP(def->src->hosts->u.inet.port, port) < 0) goto error; - def->src->hosts->transport = VIR_STORAGE_NET_HOST_TRANS_TCP; - def->src->hosts->socket = NULL; + def->src->hosts->type = VIR_STORAGE_NET_HOST_TRANS_TCP; if (VIR_STRDUP(def->src->path, vdi) < 0) goto error; } @@ -2006,8 +2009,8 @@ qemuParseCommandLine(virCapsPtr caps, if (VIR_ALLOC(disk->src->hosts) < 0) goto error; disk->src->nhosts = 1; - disk->src->hosts->name = disk->src->path; - if (VIR_STRDUP(disk->src->hosts->port, port) < 0) + disk->src->hosts->u.inet.addr = disk->src->path; + if (VIR_STRDUP(disk->src->hosts->u.inet.port, port) < 0) goto error; if (VIR_STRDUP(disk->src->path, vdi) < 0) goto error; @@ -2548,14 +2551,13 @@ qemuParseCommandLine(virCapsPtr caps, goto error; } } - first_rbd_disk->src->hosts[first_rbd_disk->src->nhosts].port = port; - if (VIR_STRDUP(first_rbd_disk->src->hosts[first_rbd_disk->src->nhosts].name, + first_rbd_disk->src->hosts[first_rbd_disk->src->nhosts].u.inet.port = port; + if (VIR_STRDUP(first_rbd_disk->src->hosts[first_rbd_disk->src->nhosts].u.inet.addr, token) < 0) { VIR_FREE(hosts); goto error; } - first_rbd_disk->src->hosts[first_rbd_disk->src->nhosts].transport = VIR_STORAGE_NET_HOST_TRANS_TCP; - first_rbd_disk->src->hosts[first_rbd_disk->src->nhosts].socket = NULL; + first_rbd_disk->src->hosts[first_rbd_disk->src->nhosts].type = VIR_STORAGE_NET_HOST_TRANS_TCP; first_rbd_disk->src->nhosts++; token = strtok_r(NULL, ",", &saveptr); diff --git a/src/storage/storage_backend_gluster.c b/src/storage/storage_backend_gluster.c index 8e86704..0d5b7f6 100644 --- a/src/storage/storage_backend_gluster.c +++ b/src/storage/storage_backend_gluster.c @@ -553,7 +553,8 @@ virStorageFileBackendGlusterDeinit(virStorageSourcePtr src) virStorageFileBackendGlusterPrivPtr priv = src->drv->priv; VIR_DEBUG("deinitializing gluster storage file %p (gluster://%s:%s/%s%s)", - src, src->hosts->name, src->hosts->port ? src->hosts->port : "0", + src, src->hosts->u.inet.addr, + src->hosts->u.inet.port ? src->hosts->u.inet.port : "0", src->volume, src->path); if (priv->vol) @@ -568,27 +569,27 @@ static int virStorageFileBackendGlusterInitServer(virStorageFileBackendGlusterPrivPtr priv, virStorageNetHostDefPtr host) { - const char *transport = virStorageNetHostTransportTypeToString(host->transport); + const char *transport = virStorageNetHostTransportTypeToString(host->type); const char *hoststr = NULL; int port = 0; - switch ((virStorageNetHostTransport) host->transport) { + switch ((virStorageNetHostTransport) host->type) { case VIR_STORAGE_NET_HOST_TRANS_RDMA: case VIR_STORAGE_NET_HOST_TRANS_TCP: - hoststr = host->name; + hoststr = host->u.inet.addr; - if (host->port && - virStrToLong_i(host->port, NULL, 10, &port) < 0) { + if (host->u.inet.port && + virStrToLong_i(host->u.inet.port, NULL, 10, &port) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("failed to parse port number '%s'"), - host->port); + host->u.inet.port); return -1; } break; case VIR_STORAGE_NET_HOST_TRANS_UNIX: - hoststr = host->socket; + hoststr = host->u.uds.path; break; case VIR_STORAGE_NET_HOST_TRANS_LAST: @@ -797,8 +798,8 @@ virStorageFileBackendGlusterGetUniqueIdentifier(virStorageSourcePtr src) return NULL; ignore_value(virAsprintf(&priv->canonpath, "gluster://%s:%s/%s/%s", - src->hosts->name, - src->hosts->port, + src->hosts->u.inet.addr, + src->hosts->u.inet.port, src->volume, filePath)); diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index a79acc6..7c7fddd 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -3355,10 +3355,10 @@ virStorageAddISCSIPoolSourceHost(virDomainDiskDefPtr def, if (VIR_ALLOC_N(def->src->hosts, def->src->nhosts) < 0) goto cleanup; - if (VIR_STRDUP(def->src->hosts[0].name, pooldef->source.hosts[0].name) < 0) + if (VIR_STRDUP(def->src->hosts[0].u.inet.addr, pooldef->source.hosts[0].name) < 0) goto cleanup; - if (virAsprintf(&def->src->hosts[0].port, "%d", + if (virAsprintf(&def->src->hosts[0].u.inet.port, "%d", pooldef->source.hosts[0].port ? pooldef->source.hosts[0].port : 3260) < 0) @@ -3384,8 +3384,7 @@ virStorageAddISCSIPoolSourceHost(virDomainDiskDefPtr def, /* Storage pool have not supported these 2 attributes yet, * use the defaults. */ - def->src->hosts[0].transport = VIR_STORAGE_NET_HOST_TRANS_TCP; - def->src->hosts[0].socket = NULL; + def->src->hosts[0].type = VIR_STORAGE_NET_HOST_TRANS_TCP; def->src->protocol = VIR_STORAGE_NET_PROTOCOL_ISCSI; diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 1011bd0..8ec8081 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -1603,9 +1603,18 @@ virStorageNetHostDefClear(virStorageNetHostDefPtr def) if (!def) return; - VIR_FREE(def->name); - VIR_FREE(def->port); - VIR_FREE(def->socket); + switch (def->type) { + case VIR_STORAGE_NET_HOST_TRANS_UNIX: + VIR_FREE(def->u.uds.path); + break; + case VIR_STORAGE_NET_HOST_TRANS_TCP: + case VIR_STORAGE_NET_HOST_TRANS_RDMA: + VIR_FREE(def->u.inet.addr); + VIR_FREE(def->u.inet.port); + break; + case VIR_STORAGE_NET_HOST_TRANS_LAST: + break; + } } @@ -1650,16 +1659,23 @@ virStorageNetHostDefCopy(size_t nhosts, virStorageNetHostDefPtr src = &hosts[i]; virStorageNetHostDefPtr dst = &ret[i]; - dst->transport = src->transport; - - if (VIR_STRDUP(dst->name, src->name) < 0) - goto error; + dst->type = src->type; - if (VIR_STRDUP(dst->port, src->port) < 0) - goto error; - - if (VIR_STRDUP(dst->socket, src->socket) < 0) - goto error; + switch (src->type) { + case VIR_STORAGE_NET_HOST_TRANS_UNIX: + if (VIR_STRDUP(dst->u.uds.path, src->u.uds.path) < 0) + goto error; + break; + case VIR_STORAGE_NET_HOST_TRANS_TCP: + case VIR_STORAGE_NET_HOST_TRANS_RDMA: + if (VIR_STRDUP(dst->u.inet.addr, src->u.inet.addr)< 0) + goto error; + if (VIR_STRDUP(dst->u.inet.port, src->u.inet.port)< 0) + goto error; + break; + case VIR_STORAGE_NET_HOST_TRANS_LAST: + break; + } } return ret; @@ -2292,7 +2308,7 @@ virStorageSourceParseBackingURI(virStorageSourcePtr src, } if (scheme[1] && - (src->hosts->transport = virStorageNetHostTransportTypeFromString(scheme[1])) < 0) { + (src->hosts->type = virStorageNetHostTransportTypeFromString(scheme[1])) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("invalid protocol transport type '%s'"), scheme[1]); @@ -2301,7 +2317,7 @@ virStorageSourceParseBackingURI(virStorageSourcePtr src, /* handle socket stored as a query */ if (uri->query) { - if (VIR_STRDUP(src->hosts->socket, STRSKIP(uri->query, "socket=")) < 0) + if (VIR_STRDUP(src->hosts->u.uds.path, STRSKIP(uri->query, "socket=")) < 0) goto cleanup; } @@ -2339,11 +2355,11 @@ virStorageSourceParseBackingURI(virStorageSourcePtr src, } if (uri->port > 0) { - if (virAsprintf(&src->hosts->port, "%d", uri->port) < 0) + if (virAsprintf(&src->hosts->u.inet.port, "%d", uri->port) < 0) goto cleanup; } - if (VIR_STRDUP(src->hosts->name, uri->server) < 0) + if (VIR_STRDUP(src->hosts->u.inet.addr, uri->server) < 0) goto cleanup; ret = 0; @@ -2378,26 +2394,25 @@ virStorageSourceRBDAddHost(virStorageSourcePtr src, if (port) { *port = '\0'; port += skip; - if (VIR_STRDUP(src->hosts[src->nhosts - 1].port, port) < 0) + if (VIR_STRDUP(src->hosts[src->nhosts - 1].u.inet.port, port) < 0) goto error; } parts = virStringSplit(hostport, "\\:", 0); if (!parts) goto error; - src->hosts[src->nhosts-1].name = virStringListJoin((const char **)parts, ":"); + src->hosts[src->nhosts-1].u.inet.addr = virStringListJoin((const char **)parts, ":"); virStringListFree(parts); - if (!src->hosts[src->nhosts-1].name) + if (!src->hosts[src->nhosts-1].u.inet.addr) goto error; - src->hosts[src->nhosts-1].transport = VIR_STORAGE_NET_HOST_TRANS_TCP; - src->hosts[src->nhosts-1].socket = NULL; + src->hosts[src->nhosts-1].type = VIR_STORAGE_NET_HOST_TRANS_TCP; return 0; error: - VIR_FREE(src->hosts[src->nhosts-1].port); - VIR_FREE(src->hosts[src->nhosts-1].name); + VIR_FREE(src->hosts[src->nhosts-1].u.inet.port); + VIR_FREE(src->hosts[src->nhosts-1].u.inet.addr); return -1; } @@ -2524,7 +2539,7 @@ virStorageSourceParseNBDColonString(const char *nbdstr, goto cleanup; src->nhosts = 1; - src->hosts->transport = VIR_STORAGE_NET_HOST_TRANS_TCP; + src->hosts->type = VIR_STORAGE_NET_HOST_TRANS_TCP; /* format: [] denotes optional sections, uppercase are variable strings * nbd:unix:/PATH/TO/SOCKET[:exportname=EXPORTNAME] @@ -2543,7 +2558,7 @@ virStorageSourceParseNBDColonString(const char *nbdstr, goto cleanup; } - if (VIR_STRDUP(src->hosts->socket, backing[2]) < 0) + if (VIR_STRDUP(src->hosts->u.uds.path, backing[2]) < 0) goto cleanup; } else { @@ -2554,7 +2569,7 @@ virStorageSourceParseNBDColonString(const char *nbdstr, goto cleanup; } - if (VIR_STRDUP(src->hosts->name, backing[1]) < 0) + if (VIR_STRDUP(src->hosts->u.inet.addr, backing[1]) < 0) goto cleanup; if (!backing[2]) { @@ -2564,7 +2579,7 @@ virStorageSourceParseNBDColonString(const char *nbdstr, goto cleanup; } - if (VIR_STRDUP(src->hosts->port, backing[2]) < 0) + if (VIR_STRDUP(src->hosts->u.inet.port, backing[2]) < 0) goto cleanup; } @@ -2724,7 +2739,7 @@ virStorageSourceParseBackingJSONGlusterHost(virStorageNetHostDefPtr host, return -1; } - host->transport = transport; + host->type = transport; switch ((virStorageNetHostTransport) transport) { case VIR_STORAGE_NET_HOST_TRANS_TCP: @@ -2735,8 +2750,8 @@ virStorageSourceParseBackingJSONGlusterHost(virStorageNetHostDefPtr host, return -1; } - if (VIR_STRDUP(host->name, hostname) < 0 || - VIR_STRDUP(host->port, port) < 0) + if (VIR_STRDUP(host->u.inet.addr, hostname) < 0 || + VIR_STRDUP(host->u.inet.port, port) < 0) return -1; break; @@ -2749,7 +2764,7 @@ virStorageSourceParseBackingJSONGlusterHost(virStorageNetHostDefPtr host, } - if (VIR_STRDUP(host->socket, socket) < 0) + if (VIR_STRDUP(host->u.uds.path, socket) < 0) return -1; break; @@ -2866,15 +2881,15 @@ virStorageSourceParseBackingJSONNbd(virStorageSourcePtr src, src->nhosts = 1; if (path) { - src->hosts[0].transport = VIR_STORAGE_NET_HOST_TRANS_UNIX; - if (VIR_STRDUP(src->hosts[0].socket, path) < 0) + src->hosts[0].type = VIR_STORAGE_NET_HOST_TRANS_UNIX; + if (VIR_STRDUP(src->hosts[0].u.uds.path, path) < 0) return -1; } else { - src->hosts[0].transport = VIR_STORAGE_NET_HOST_TRANS_TCP; - if (VIR_STRDUP(src->hosts[0].name, host) < 0) + src->hosts[0].type = VIR_STORAGE_NET_HOST_TRANS_TCP; + if (VIR_STRDUP(src->hosts[0].u.inet.addr, host) < 0) return -1; - if (VIR_STRDUP(src->hosts[0].port, port) < 0) + if (VIR_STRDUP(src->hosts[0].u.inet.port, port) < 0) return -1; } @@ -2932,11 +2947,11 @@ virStorageSourceParseBackingJSONSSH(virStorageSourcePtr src, return -1; src->nhosts = 1; - src->hosts[0].transport = VIR_STORAGE_NET_HOST_TRANS_TCP; - if (VIR_STRDUP(src->hosts[0].name, host) < 0) + src->hosts[0].type = VIR_STORAGE_NET_HOST_TRANS_TCP; + if (VIR_STRDUP(src->hosts[0].u.inet.addr, host) < 0) return -1; - if (VIR_STRDUP(src->hosts[0].port, port) < 0) + if (VIR_STRDUP(src->hosts[0].u.inet.port, port) < 0) return -1; return 0; diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index 3d09468..99b4a31 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -149,13 +149,25 @@ typedef enum { VIR_ENUM_DECL(virStorageNetHostTransport) +typedef struct _virStorageNetHostUnixSockAddr virStorageNetHostUnixSockAddr; +struct _virStorageNetHostUnixSockAddr { + char *path; +}; + +typedef struct _virStorageNetHostInetSockAddr virStorageNetHostInetSockAddr; +struct _virStorageNetHostInetSockAddr { + char *addr; + char *port; +}; + typedef struct _virStorageNetHostDef virStorageNetHostDef; typedef virStorageNetHostDef *virStorageNetHostDefPtr; struct _virStorageNetHostDef { - char *name; - char *port; - int transport; /* virStorageNetHostTransport */ - char *socket; /* path to unix socket */ + virStorageNetHostTransport type; + union { /* union tag is @type */ + virStorageNetHostUnixSockAddr uds; + virStorageNetHostInetSockAddr inet; + } u; }; /* Information for a storage volume from a virStoragePool */ diff --git a/src/xenconfig/xen_xl.c b/src/xenconfig/xen_xl.c index bcdd355..0fadf2b 100644 --- a/src/xenconfig/xen_xl.c +++ b/src/xenconfig/xen_xl.c @@ -951,14 +951,14 @@ xenFormatXLDiskSrcNet(virStorageSourcePtr src) virBufferAddLit(&buf, "\\\\;"); /* assume host containing : is ipv6 */ - if (strchr(src->hosts[i].name, ':')) + if (strchr(src->hosts[i].u.inet.addr, ':')) virBufferEscape(&buf, '\\', ":", "[%s]", - src->hosts[i].name); + src->hosts[i].u.inet.addr); else - virBufferAsprintf(&buf, "%s", src->hosts[i].name); + virBufferAsprintf(&buf, "%s", src->hosts[i].u.inet.addr); - if (src->hosts[i].port) - virBufferAsprintf(&buf, "\\\\:%s", src->hosts[i].port); + if (src->hosts[i].u.inet.port) + virBufferAsprintf(&buf, "\\\\:%s", src->hosts[i].u.inet.port); } } diff --git a/tests/domainsnapshotxml2xmlin/disk_snapshot.xml b/tests/domainsnapshotxml2xmlin/disk_snapshot.xml index aa1522a..5149ac2 100644 --- a/tests/domainsnapshotxml2xmlin/disk_snapshot.xml +++ b/tests/domainsnapshotxml2xmlin/disk_snapshot.xml @@ -26,7 +26,7 @@ <disk name='hdh' snapshot='external' type='network'> <source protocol='rbd' name='name'> <host name='host' port='1234'/> - <host name='host2' port='1234' transport='rdma'/> + <host transport='rdma' name='host2' port='1234'/> <host name='host3' port='1234'/> </source> </disk> diff --git a/tests/domainsnapshotxml2xmlout/disk_snapshot.xml b/tests/domainsnapshotxml2xmlout/disk_snapshot.xml index c2e77d7..3d4eb52 100644 --- a/tests/domainsnapshotxml2xmlout/disk_snapshot.xml +++ b/tests/domainsnapshotxml2xmlout/disk_snapshot.xml @@ -25,7 +25,7 @@ <disk name='hdh' snapshot='external' type='network'> <source protocol='rbd' name='name'> <host name='host' port='1234'/> - <host name='host2' port='1234' transport='rdma'/> + <host transport='rdma' name='host2' port='1234'/> <host name='host3' port='1234'/> </source> </disk> diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c index f766df1..5c685dd 100644 --- a/tests/virstoragetest.c +++ b/tests/virstoragetest.c @@ -387,7 +387,7 @@ testStorageChain(const void *args) elt->type, elt->format, virStorageNetProtocolTypeToString(elt->protocol), - NULLSTR(elt->nhosts ? elt->hosts[0].name : NULL), + NULLSTR(elt->nhosts ? elt->hosts[0].u.inet.addr : NULL), NULLSTR(elt->auth ? elt->auth->username : NULL)) < 0) { VIR_FREE(expect); VIR_FREE(actual); -- 2.7.4
Currently, each among virStorageFileGetMetadataRecurse, qemuSecurityChownCallback, qemuDomainSnapshotPrepareDiskExternal and qemuDomainSnapshotCreateSingleDiskActive makes calls to virStorageFileInit and friends for simple operations like stat, read headers, chown and etc. This patch 1. optimize/unify calls to virStorageFileInit and virStorageFileDeinit. 2. addes virObject to virStorageDriverData to keep reference. Signed-off-by: Prasanna Kumar Kalever <prasanna.kalever@redhat.com> --- src/qemu/qemu_domain.c | 25 +----------------- src/qemu/qemu_domain.h | 8 +++--- src/qemu/qemu_driver.c | 26 ++++++++++++------- src/qemu/qemu_process.c | 49 +++++++++++++++++++++++++++++++++++ src/qemu/qemu_process.h | 4 +++ src/storage/storage_backend.h | 1 + src/storage/storage_backend_fs.c | 5 ++++ src/storage/storage_backend_gluster.c | 6 ++++- src/storage/storage_driver.c | 33 +++++++++++++++++++---- src/storage/storage_driver.h | 2 ++ src/util/virstoragefile.h | 1 + 11 files changed, 117 insertions(+), 43 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 4aae14d..fcdf717 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -4837,7 +4837,7 @@ qemuDomainCleanupRun(virQEMUDriverPtr driver, priv->ncleanupCallbacks_max = 0; } -static void +void qemuDomainGetImageIds(virQEMUDriverConfigPtr cfg, virDomainObjPtr vm, virStorageSourcePtr src, @@ -4869,29 +4869,6 @@ qemuDomainGetImageIds(virQEMUDriverConfigPtr cfg, } -int -qemuDomainStorageFileInit(virQEMUDriverPtr driver, - virDomainObjPtr vm, - virStorageSourcePtr src) -{ - virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); - uid_t uid; - gid_t gid; - int ret = -1; - - qemuDomainGetImageIds(cfg, vm, src, &uid, &gid); - - if (virStorageFileInitAs(src, uid, gid) < 0) - goto cleanup; - - ret = 0; - - cleanup: - virObjectUnref(cfg); - return ret; -} - - char * qemuDomainStorageAlias(const char *device, int depth) { diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 7650ff3..8beab7a 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -591,9 +591,11 @@ bool qemuDomainDiskSourceDiffers(virDomainDiskDefPtr disk, bool qemuDomainDiskChangeSupported(virDomainDiskDefPtr disk, virDomainDiskDefPtr orig_disk); -int qemuDomainStorageFileInit(virQEMUDriverPtr driver, - virDomainObjPtr vm, - virStorageSourcePtr src); +void qemuDomainGetImageIds(virQEMUDriverConfigPtr cfg, + virDomainObjPtr vm, + virStorageSourcePtr src, + uid_t *uid, gid_t *gid); + char *qemuDomainStorageAlias(const char *device, int depth); void qemuDomainDiskChainElementRevoke(virQEMUDriverPtr driver, diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 3517aa2..6761915 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -13865,9 +13865,6 @@ qemuDomainSnapshotPrepareDiskExternal(virConnectPtr conn, return -1; } - if (virStorageFileInit(snapdisk->src) < 0) - return -1; - if (virStorageFileStat(snapdisk->src, &st) < 0) { if (errno != ENOENT) { virReportSystemError(errno, @@ -13891,7 +13888,6 @@ qemuDomainSnapshotPrepareDiskExternal(virConnectPtr conn, ret = 0; cleanup: - virStorageFileDeinit(snapdisk->src); return ret; } @@ -13955,7 +13951,8 @@ qemuDomainSnapshotPrepareDiskInternal(virConnectPtr conn, static int -qemuDomainSnapshotPrepare(virConnectPtr conn, +qemuDomainSnapshotPrepare(virQEMUDriverConfigPtr cfg, + virConnectPtr conn, virDomainObjPtr vm, virDomainSnapshotDefPtr def, unsigned int *flags) @@ -14026,6 +14023,9 @@ qemuDomainSnapshotPrepare(virConnectPtr conn, goto cleanup; } + if (qemuStorageVolumeRegister(cfg, vm, def->disks[i].src) < 0) + goto cleanup; + if (qemuDomainSnapshotPrepareDiskExternal(conn, dom_disk, disk, active, reuse) < 0) goto cleanup; @@ -14139,10 +14139,9 @@ qemuDomainSnapshotCreateSingleDiskActive(virQEMUDriverPtr driver, if (!(newDiskSrc = virStorageSourceCopy(snap->src, false))) goto cleanup; - if (virStorageSourceInitChainElement(newDiskSrc, disk->src, false) < 0) - goto cleanup; + newDiskSrc->drv = snap->src->drv; - if (qemuDomainStorageFileInit(driver, vm, newDiskSrc) < 0) + if (virStorageSourceInitChainElement(newDiskSrc, disk->src, false) < 0) goto cleanup; if (qemuGetDriveSourceString(newDiskSrc, NULL, &source) < 0) @@ -14211,7 +14210,6 @@ qemuDomainSnapshotCreateSingleDiskActive(virQEMUDriverPtr driver, cleanup: if (need_unlink && virStorageFileUnlink(newDiskSrc)) VIR_WARN("unable to unlink just-created %s", source); - virStorageFileDeinit(newDiskSrc); virStorageSourceFree(newDiskSrc); virStorageSourceFree(persistDiskSrc); VIR_FREE(device); @@ -14566,6 +14564,7 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain, virDomainSnapshotObjPtr snap = NULL; virDomainSnapshotPtr snapshot = NULL; virDomainSnapshotDefPtr def = NULL; + virDomainSnapshotDefPtr refDef = NULL; bool update_current = true; bool redefine = flags & VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE; unsigned int parse_flags = VIR_DOMAIN_SNAPSHOT_PARSE_DISKS; @@ -14574,6 +14573,7 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain, bool align_match = true; virQEMUDriverConfigPtr cfg = NULL; virCapsPtr caps = NULL; + size_t i; virCheckFlags(VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE | VIR_DOMAIN_SNAPSHOT_CREATE_CURRENT | @@ -14732,7 +14732,7 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain, } if (virDomainSnapshotAlignDisks(def, align_location, align_match) < 0 || - qemuDomainSnapshotPrepare(conn, vm, def, &flags) < 0) + qemuDomainSnapshotPrepare(cfg, conn, vm, def, &flags) < 0) goto endjob; } @@ -14800,6 +14800,12 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain, snapshot = virGetDomainSnapshot(domain, snap->def->name); endjob: + refDef = (!snap) ? def : snap->def; + for (i = 0; i < refDef->ndisks; i++) { + if (refDef->disks[i].snapshot == VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL) + qemuStorageVolumeUnRegister(refDef->disks[i].src); + } + if (snapshot && !(flags & VIR_DOMAIN_SNAPSHOT_CREATE_NO_METADATA)) { if (qemuDomainSnapshotWriteMetadata(vm, snap, driver->caps, cfg->snapshotDir) < 0) { diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 7f19c69..7bec469 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -4590,6 +4590,35 @@ qemuProcessStartValidate(virQEMUDriverPtr driver, } +int +qemuStorageVolumeRegister(virQEMUDriverConfigPtr cfg, + virDomainObjPtr vm, virStorageSourcePtr src) +{ + uid_t uid; + gid_t gid; + + if (virStorageSourceIsEmpty(src)) + return 0; + + qemuDomainGetImageIds(cfg, vm, src, &uid, &gid); + + if (virStorageFileInitAs(src, uid, gid) < 0) + return -1; + + return 0; +} + + +void +qemuStorageVolumeUnRegister(virStorageSourcePtr src) +{ + if (virStorageSourceIsEmpty(src)) + return; + + virStorageFileDeinit(src); +} + + /** * qemuProcessInit: * @@ -4614,6 +4643,7 @@ qemuProcessInit(virQEMUDriverPtr driver, qemuDomainObjPrivatePtr priv = vm->privateData; int stopFlags; int ret = -1; + size_t i; VIR_DEBUG("vm=%p name=%s id=%d migration=%d", vm, vm->def->name, vm->def->id, migration); @@ -4666,6 +4696,12 @@ qemuProcessInit(virQEMUDriverPtr driver, if (qemuDomainSetPrivatePaths(driver, vm) < 0) goto cleanup; + if (!(flags & VIR_QEMU_PROCESS_START_PRETEND)) { + for (i = 0; i < vm->def->ndisks; i++) + if (qemuStorageVolumeRegister(cfg, vm, vm->def->disks[i]->src) < 0) + goto cleanup; + } + ret = 0; cleanup: @@ -5674,6 +5710,7 @@ qemuProcessFinishStartup(virConnectPtr conn, { virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); int ret = -1; + size_t i; if (startCPUs) { VIR_DEBUG("Starting domain CPUs"); @@ -5698,6 +5735,9 @@ qemuProcessFinishStartup(virConnectPtr conn, VIR_HOOK_SUBOP_BEGIN) < 0) goto cleanup; + for (i = 0; i < vm->def->ndisks; i++) + qemuStorageVolumeUnRegister(vm->def->disks[i]->src); + ret = 0; cleanup: @@ -6047,6 +6087,12 @@ void qemuProcessStop(virQEMUDriverPtr driver, VIR_FREE(xml); } + for (i = 0; i < vm->def->ndisks; i++) { + vm->def->disks[i]->src->drv = NULL; /* FIXME: Brings in garbage */ + if (qemuStorageVolumeRegister(cfg, vm, vm->def->disks[i]->src) < 0) + goto cleanup; + } + /* Reset Security Labels unless caller don't want us to */ if (!(flags & VIR_QEMU_PROCESS_STOP_NO_RELABEL)) virSecurityManagerRestoreAllLabel(driver->securityManager, @@ -6210,6 +6256,9 @@ void qemuProcessStop(virQEMUDriverPtr driver, VIR_FREE(xml); } + for (i = 0; i < vm->def->ndisks; i++) + qemuStorageVolumeUnRegister(vm->def->disks[i]->src); + virDomainObjRemoveTransientDef(vm); endjob: diff --git a/src/qemu/qemu_process.h b/src/qemu/qemu_process.h index 21f3b0c..e53a154 100644 --- a/src/qemu/qemu_process.h +++ b/src/qemu/qemu_process.h @@ -90,6 +90,10 @@ virCommandPtr qemuProcessCreatePretendCmd(virConnectPtr conn, bool standalone, unsigned int flags); +int qemuStorageVolumeRegister(virQEMUDriverConfigPtr cfg, + virDomainObjPtr vm, virStorageSourcePtr src); +void qemuStorageVolumeUnRegister(virStorageSourcePtr src); + int qemuProcessInit(virQEMUDriverPtr driver, virDomainObjPtr vm, qemuDomainAsyncJob asyncJob, diff --git a/src/storage/storage_backend.h b/src/storage/storage_backend.h index 28e1a65..94fa118 100644 --- a/src/storage/storage_backend.h +++ b/src/storage/storage_backend.h @@ -249,6 +249,7 @@ typedef struct _virStorageFileBackend virStorageFileBackend; typedef virStorageFileBackend *virStorageFileBackendPtr; struct _virStorageDriverData { + virObject parent; virStorageFileBackendPtr backend; void *priv; diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index de0e8d5..1fd134c 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -1486,6 +1486,11 @@ virStorageFileBackendFileDeinit(virStorageSourcePtr src) virStorageFileBackendFsPrivPtr priv = src->drv->priv; + if (virObjectUnref(src->drv)) + return; + + src->drv = NULL; + VIR_FREE(priv->canonpath); VIR_FREE(priv); } diff --git a/src/storage/storage_backend_gluster.c b/src/storage/storage_backend_gluster.c index 0d5b7f6..779a3bd 100644 --- a/src/storage/storage_backend_gluster.c +++ b/src/storage/storage_backend_gluster.c @@ -557,12 +557,16 @@ virStorageFileBackendGlusterDeinit(virStorageSourcePtr src) src->hosts->u.inet.port ? src->hosts->u.inet.port : "0", src->volume, src->path); + if (virObjectUnref(src->drv)) + return; + + src->drv = NULL; + if (priv->vol) glfs_fini(priv->vol); VIR_FREE(priv->canonpath); VIR_FREE(priv); - src->drv->priv = NULL; } static int diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 7c7fddd..39f38d3 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -2831,7 +2831,7 @@ int storageRegister(void) /* ----------- file handlers cooperating with storage driver --------------- */ -static bool +bool virStorageFileIsInitialized(virStorageSourcePtr src) { return src && src->drv; @@ -2894,6 +2894,22 @@ virStorageFileSupportsSecurityDriver(virStorageSourcePtr src) } +static virClassPtr virStorageDriverDataClass; + +static int virStorageDriverDataOnceInit(void) +{ + if (!(virStorageDriverDataClass = virClassNew(virClassForObject(), + "virStorageDriverData", + sizeof(virStorageDriverData), + NULL))) + return -1; + + return 0; +} + +VIR_ONCE_GLOBAL_INIT(virStorageDriverData) + + void virStorageFileDeinit(virStorageSourcePtr src) { @@ -2903,8 +2919,6 @@ virStorageFileDeinit(virStorageSourcePtr src) if (src->drv->backend && src->drv->backend->backendDeinit) src->drv->backend->backendDeinit(src); - - VIR_FREE(src->drv); } @@ -2926,7 +2940,16 @@ virStorageFileInitAs(virStorageSourcePtr src, uid_t uid, gid_t gid) { int actualType = virStorageSourceGetActualType(src); - if (VIR_ALLOC(src->drv) < 0) + + if (virStorageFileIsInitialized(src)) { + virObjectRef(src->drv); + return 0; + } + + if (virStorageDriverDataInitialize() < 0) + return -1; + + if (!(src->drv = virObjectNew(virStorageDriverDataClass))) return -1; if (uid == (uid_t) -1) @@ -2950,7 +2973,7 @@ virStorageFileInitAs(virStorageSourcePtr src, return 0; error: - VIR_FREE(src->drv); + virObjectUnref(src->drv); return -1; } diff --git a/src/storage/storage_driver.h b/src/storage/storage_driver.h index 3f2549d..3fadb53 100644 --- a/src/storage/storage_driver.h +++ b/src/storage/storage_driver.h @@ -30,6 +30,8 @@ # include "storage_conf.h" # include "virstoragefile.h" +bool virStorageFileIsInitialized(virStorageSourcePtr src); + int virStorageFileInit(virStorageSourcePtr src); int virStorageFileInitAs(virStorageSourcePtr src, uid_t uid, gid_t gid); diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index 99b4a31..03308d4 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -29,6 +29,7 @@ # include "virstorageencryption.h" # include "virutil.h" # include "virsecret.h" +# include "virobject.h" /* Minimum header size required to probe all known formats with * virStorageFileProbeFormat, or obtain metadata from a known format. -- 2.7.4
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@redhat.com> --- src/storage/storage_backend_gluster.c | 302 +++++++++++++++++++++++++++++++--- 1 file changed, 283 insertions(+), 19 deletions(-) diff --git a/src/storage/storage_backend_gluster.c b/src/storage/storage_backend_gluster.c index 779a3bd..71426c8 100644 --- a/src/storage/storage_backend_gluster.c +++ b/src/storage/storage_backend_gluster.c @@ -36,6 +36,20 @@ VIR_LOG_INIT("storage.storage_backend_gluster"); + +typedef struct _virStorageBackendGlusterState virStorageBackendGlusterState; +typedef virStorageBackendGlusterState *virStorageBackendGlusterStatePtr; + +typedef struct _virStorageFileBackendGlusterPriv virStorageFileBackendGlusterPriv; +typedef virStorageFileBackendGlusterPriv *virStorageFileBackendGlusterPrivPtr; + +typedef struct _virStorageBackendGlusterCache virStorageBackendGlusterCache; +typedef virStorageBackendGlusterCache *virStorageBackendGlusterCachePtr; + +typedef struct _virStorageBackendGlusterCacheConn virStorageBackendGlusterCacheConn; +typedef virStorageBackendGlusterCacheConn *virStorageBackendGlusterCacheConnPtr; + + struct _virStorageBackendGlusterState { glfs_t *vol; @@ -47,8 +61,206 @@ 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 _virStorageBackendGlusterCacheConn { + virObjectLockable parent; + virCond cond; + bool quit; + + glfs_t *fs; + size_t nhosts; + virStorageNetHostDefPtr hosts; + char *volname; +}; + +struct _virStorageBackendGlusterCache { + virMutex lock; + size_t nConn; + virStorageBackendGlusterCacheConnPtr *conn; +}; + + +static virStorageBackendGlusterCachePtr virStorageBackendGlusterGlobalCacheObj; +static virClassPtr virStorageBackendGlusterCacheConnClass; +static virOnceControl virStorageBackendGlusterCacheOnce = VIR_ONCE_CONTROL_INITIALIZER; + + +static void virStorageBackendGlusterCacheConnDispose(void *obj); + + +static int virStorageBackendGlusterCacheConnOnceInit(void) +{ + if (!(virStorageBackendGlusterCacheConnClass = + virClassNew(virClassForObjectLockable(), + "virStorageBackendGlusterCacheConnClass", + sizeof(virStorageBackendGlusterCacheConn), + virStorageBackendGlusterCacheConnDispose))) + return -1; + + return 0; +} + +static void virStorageBackendGlusterCacheConnDispose(void *object) +{ + virStorageBackendGlusterCacheConnPtr conn = object; + + glfs_fini(conn->fs); + + VIR_FREE(conn->volname); + + virStorageNetHostDefFree(conn->nhosts, conn->hosts); + virCondDestroy(&conn->cond); +} + +VIR_ONCE_GLOBAL_INIT(virStorageBackendGlusterCacheConn); + +static bool +virStorageBackendGlusterCompareHosts(size_t nSrcHosts, + virStorageNetHostDefPtr srcHosts, + size_t nDstHosts, + virStorageNetHostDefPtr dstHosts) +{ + bool match = false; + size_t i; + size_t j; + + if (nSrcHosts != nDstHosts) + return false; + for (i = 0; i < nSrcHosts; i++) { + for (j = 0; j < nDstHosts; j++) { + if (srcHosts[i].type != dstHosts[j].type) + continue; + switch (srcHosts[i].type) { + case VIR_STORAGE_NET_HOST_TRANS_UNIX: + if (STREQ(srcHosts[i].u.uds.path, dstHosts[j].u.uds.path)) + match = true; + break; + case VIR_STORAGE_NET_HOST_TRANS_TCP: + case VIR_STORAGE_NET_HOST_TRANS_RDMA: + if (STREQ(srcHosts[i].u.inet.addr, dstHosts[j].u.inet.addr) + && + STREQ(srcHosts[i].u.inet.port, dstHosts[j].u.inet.port)) + match = true; + break; + case VIR_STORAGE_NET_HOST_TRANS_LAST: + break; + } + if (match) + break; /* out of inner for loop */ + } + if (!match) + return false; + match = false; /* reset */ + } + return true; +} + +static int +virStorageBackendGlusterCacheAdd(virStorageSourcePtr src, glfs_t *fs) +{ + virStorageBackendGlusterCacheConnPtr newConn = NULL; + virStorageBackendGlusterCacheConnPtr conn = NULL; + size_t i; + + for (i = 0; i < virStorageBackendGlusterGlobalCacheObj->nConn; i++) { + conn = virStorageBackendGlusterGlobalCacheObj->conn[i]; + if (STRNEQ(conn->volname, src->volume)) + continue; + if (virStorageBackendGlusterCompareHosts(conn->nhosts, conn->hosts, + src->nhosts, src->hosts)) { + return 0; + } + } + + if (!(newConn = virObjectLockableNew(virStorageBackendGlusterCacheConnClass))) + return -1; + + if (virCondInit(&newConn->cond) < 0) { + virReportSystemError(errno, "%s", + _("failed to initialize domain condition")); + goto error; + } + + if (VIR_STRDUP(newConn->volname, src->volume) < 0) + goto error; + + newConn->nhosts = src->nhosts; + if (!(newConn->hosts = virStorageNetHostDefCopy(src->nhosts, src->hosts))) + goto error; + newConn->fs = fs; + + if (VIR_APPEND_ELEMENT(virStorageBackendGlusterGlobalCacheObj->conn, + virStorageBackendGlusterGlobalCacheObj->nConn, + newConn) < 0) + goto error; + + return 0; + + error: + virStorageNetHostDefFree(newConn->nhosts, newConn->hosts); + VIR_FREE(newConn->volname); + virObjectUnref(newConn); + return -1; +} + +static glfs_t * +virStorageBackendGlusterCacheLookup(virStorageSourcePtr src) +{ + virStorageBackendGlusterCacheConnPtr conn; + size_t i; + + if (virStorageBackendGlusterGlobalCacheObj == NULL) + return NULL; + + for (i = 0; i < virStorageBackendGlusterGlobalCacheObj->nConn; i++) { + conn = virStorageBackendGlusterGlobalCacheObj->conn[i]; + if (STRNEQ(conn->volname, src->volume)) + continue; + if (virStorageBackendGlusterCompareHosts(conn->nhosts, conn->hosts, + src->nhosts, src->hosts)) { + while (!conn->quit) { + if (virCondWait(&conn->cond, &conn->parent.lock) < 0) { + virReportSystemError(errno, "%s", + _("failed to wait for domain condition")); + return NULL; + } + } + virObjectRef(conn); + return conn->fs; + } + } + + return NULL; +} + +static void +virStorageBackendGlusterCacheRelease(virStorageSourcePtr src) +{ + virStorageBackendGlusterCacheConnPtr conn = NULL; + size_t i; + + virMutexLock(&virStorageBackendGlusterGlobalCacheObj->lock); + for (i = 0; i < virStorageBackendGlusterGlobalCacheObj->nConn; i++) { + conn = virStorageBackendGlusterGlobalCacheObj->conn[i]; + if (STRNEQ(conn->volname, src->volume)) + continue; + if (virStorageBackendGlusterCompareHosts(conn->nhosts, conn->hosts, + src->nhosts, src->hosts)) { + if (virObjectUnref(conn)) + goto unlock; + + VIR_DELETE_ELEMENT(virStorageBackendGlusterGlobalCacheObj->conn, i, + virStorageBackendGlusterGlobalCacheObj->nConn); + } + } + + unlock: + virMutexUnlock(&virStorageBackendGlusterGlobalCacheObj->lock); +} static void virStorageBackendGlusterClose(virStorageBackendGlusterStatePtr state) @@ -69,6 +281,26 @@ virStorageBackendGlusterClose(virStorageBackendGlusterStatePtr state) VIR_FREE(state); } +static void +virStorageBackendGlusterBroadcast(virStorageSourcePtr src) +{ + virStorageBackendGlusterCacheConnPtr conn; + size_t i; + + virMutexLock(&virStorageBackendGlusterGlobalCacheObj->lock); + for (i = 0; i < virStorageBackendGlusterGlobalCacheObj->nConn; i++) { + conn = virStorageBackendGlusterGlobalCacheObj->conn[i]; + if (STRNEQ(conn->volname, src->volume)) + continue; + if (virStorageBackendGlusterCompareHosts(conn->nhosts, conn->hosts, + src->nhosts, src->hosts)) { + conn->quit = true; + virCondBroadcast(&conn->cond); + } + } + virMutexUnlock(&virStorageBackendGlusterGlobalCacheObj->lock); +} + static virStorageBackendGlusterStatePtr virStorageBackendGlusterOpen(virStoragePoolObjPtr pool) { @@ -538,15 +770,6 @@ virStorageBackend virStorageBackendGluster = { }; -typedef struct _virStorageFileBackendGlusterPriv virStorageFileBackendGlusterPriv; -typedef virStorageFileBackendGlusterPriv *virStorageFileBackendGlusterPrivPtr; - -struct _virStorageFileBackendGlusterPriv { - glfs_t *vol; - char *canonpath; -}; - - static void virStorageFileBackendGlusterDeinit(virStorageSourcePtr src) { @@ -562,10 +785,9 @@ virStorageFileBackendGlusterDeinit(virStorageSourcePtr src) src->drv = NULL; - if (priv->vol) - glfs_fini(priv->vol); - VIR_FREE(priv->canonpath); + virStorageBackendGlusterCacheRelease(src); + VIR_FREE(priv->canonpath); VIR_FREE(priv); } @@ -613,6 +835,19 @@ virStorageFileBackendGlusterInitServer(virStorageFileBackendGlusterPrivPtr priv, return 0; } +static void +virStorageBackendGlusterCacheInit(void) +{ + if (VIR_ALLOC(virStorageBackendGlusterGlobalCacheObj) < 0) + return; + + if (virMutexInit(&virStorageBackendGlusterGlobalCacheObj->lock) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Unable to initialize mutex")); + return; + } + errno = VIR_ERR_OK; +} static int virStorageFileBackendGlusterInit(virStorageSourcePtr src) @@ -635,11 +870,38 @@ virStorageFileBackendGlusterInit(virStorageSourcePtr src) src, priv, src->volume, src->path, (unsigned int)src->drv->uid, (unsigned int)src->drv->gid); + if (virOnce(&virStorageBackendGlusterCacheOnce, + virStorageBackendGlusterCacheInit) < 0) + return -1; + + if (errno) + return -errno; + + if (!virStorageBackendGlusterGlobalCacheObj) + return -1; + + if (virStorageBackendGlusterCacheConnInitialize() < 0) + return -1; + + virMutexLock(&virStorageBackendGlusterGlobalCacheObj->lock); + + priv->vol = virStorageBackendGlusterCacheLookup(src); + if (priv->vol) { + src->drv->priv = priv; + virMutexUnlock(&virStorageBackendGlusterGlobalCacheObj->lock); + return 0; + } + if (!(priv->vol = glfs_new(src->volume))) { virReportOOMError(); - goto error; + goto unlock; } + if (virStorageBackendGlusterCacheAdd(src, priv->vol) < 0) + goto unlock; + + virMutexUnlock(&virStorageBackendGlusterGlobalCacheObj->lock); + for (i = 0; i < src->nhosts; i++) { if (virStorageFileBackendGlusterInitServer(priv, src->hosts + i) < 0) goto error; @@ -652,15 +914,17 @@ virStorageFileBackendGlusterInit(virStorageSourcePtr src) goto error; } + virStorageBackendGlusterBroadcast(src); + src->drv->priv = priv; return 0; - error: - if (priv->vol) - glfs_fini(priv->vol); - VIR_FREE(priv); + unlock: + virMutexUnlock(&virStorageBackendGlusterGlobalCacheObj->lock); + error: + virStorageBackendGlusterCacheRelease(src); return -1; } -- 2.7.4
On Thu, Dec 15, 2016 at 13:37:05 +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@redhat.com> --- src/storage/storage_backend_gluster.c | 302 +++++++++++++++++++++++++++++++--- 1 file changed, 283 insertions(+), 19 deletions(-)
diff --git a/src/storage/storage_backend_gluster.c b/src/storage/storage_backend_gluster.c index 779a3bd..71426c8 100644 --- a/src/storage/storage_backend_gluster.c +++ b/src/storage/storage_backend_gluster.c @@ -36,6 +36,20 @@
VIR_LOG_INIT("storage.storage_backend_gluster");
+ +typedef struct _virStorageBackendGlusterState virStorageBackendGlusterState; +typedef virStorageBackendGlusterState *virStorageBackendGlusterStatePtr; + +typedef struct _virStorageFileBackendGlusterPriv virStorageFileBackendGlusterPriv; +typedef virStorageFileBackendGlusterPriv *virStorageFileBackendGlusterPrivPtr; + +typedef struct _virStorageBackendGlusterCache virStorageBackendGlusterCache; +typedef virStorageBackendGlusterCache *virStorageBackendGlusterCachePtr; + +typedef struct _virStorageBackendGlusterCacheConn virStorageBackendGlusterCacheConn; +typedef virStorageBackendGlusterCacheConn *virStorageBackendGlusterCacheConnPtr; + + struct _virStorageBackendGlusterState { glfs_t *vol;
@@ -47,8 +61,206 @@ 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 _virStorageBackendGlusterCacheConn { + virObjectLockable parent; + virCond cond; + bool quit;
The name 'quit' is wrong. The flag is used to mark that the connection is valid.
+ + glfs_t *fs; + size_t nhosts; + virStorageNetHostDefPtr hosts; + char *volname; +}; + +struct _virStorageBackendGlusterCache { + virMutex lock; + size_t nConn; + virStorageBackendGlusterCacheConnPtr *conn; +}; + +static bool +virStorageBackendGlusterCompareHosts(size_t nSrcHosts, + virStorageNetHostDefPtr srcHosts, + size_t nDstHosts, + virStorageNetHostDefPtr dstHosts) +{ + bool match = false; + size_t i; + size_t j; + + if (nSrcHosts != nDstHosts) + return false; + for (i = 0; i < nSrcHosts; i++) { + for (j = 0; j < nDstHosts; j++) { + if (srcHosts[i].type != dstHosts[j].type) + continue; + switch (srcHosts[i].type) { + case VIR_STORAGE_NET_HOST_TRANS_UNIX: + if (STREQ(srcHosts[i].u.uds.path, dstHosts[j].u.uds.path)) + match = true; + break; + case VIR_STORAGE_NET_HOST_TRANS_TCP: + case VIR_STORAGE_NET_HOST_TRANS_RDMA: + if (STREQ(srcHosts[i].u.inet.addr, dstHosts[j].u.inet.addr) + && + STREQ(srcHosts[i].u.inet.port, dstHosts[j].u.inet.port)) + match = true; + break; + case VIR_STORAGE_NET_HOST_TRANS_LAST: + break; + } + if (match) + break; /* out of inner for loop */ + } + if (!match) + return false; + match = false; /* reset */ + } + return true; +} + +static int +virStorageBackendGlusterCacheAdd(virStorageSourcePtr src, glfs_t *fs)
Since need access to the cache entry after calling this function and you repeatedly try to look it up, this function should return the cache entry so that the code does not have to look it up afterwards.
+{ + virStorageBackendGlusterCacheConnPtr newConn = NULL; + virStorageBackendGlusterCacheConnPtr conn = NULL; + size_t i; + + for (i = 0; i < virStorageBackendGlusterGlobalCacheObj->nConn; i++) { + conn = virStorageBackendGlusterGlobalCacheObj->conn[i]; + if (STRNEQ(conn->volname, src->volume)) + continue; + if (virStorageBackendGlusterCompareHosts(conn->nhosts, conn->hosts, + src->nhosts, src->hosts)) { + return 0;
This looks wrong. The function would return success both if the entry was found and when it was created. Since this function does not initialize the connection, you need a way to be able to tell the two states apart.
+ } + } + + if (!(newConn = virObjectLockableNew(virStorageBackendGlusterCacheConnClass))) + return -1; + + if (virCondInit(&newConn->cond) < 0) { + virReportSystemError(errno, "%s", + _("failed to initialize domain condition"));
You did not modify the error message after copying this.
+ goto error; + } + + if (VIR_STRDUP(newConn->volname, src->volume) < 0) + goto error; + + newConn->nhosts = src->nhosts; + if (!(newConn->hosts = virStorageNetHostDefCopy(src->nhosts, src->hosts))) + goto error; + newConn->fs = fs; + + if (VIR_APPEND_ELEMENT(virStorageBackendGlusterGlobalCacheObj->conn, + virStorageBackendGlusterGlobalCacheObj->nConn, + newConn) < 0) + goto error; + + return 0; + + error: + virStorageNetHostDefFree(newConn->nhosts, newConn->hosts); + VIR_FREE(newConn->volname); + virObjectUnref(newConn); + return -1; +} + +static glfs_t * +virStorageBackendGlusterCacheLookup(virStorageSourcePtr src) +{ + virStorageBackendGlusterCacheConnPtr conn; + size_t i; + + if (virStorageBackendGlusterGlobalCacheObj == NULL) + return NULL; + + for (i = 0; i < virStorageBackendGlusterGlobalCacheObj->nConn; i++) { + conn = virStorageBackendGlusterGlobalCacheObj->conn[i]; + if (STRNEQ(conn->volname, src->volume)) + continue; + if (virStorageBackendGlusterCompareHosts(conn->nhosts, conn->hosts, + src->nhosts, src->hosts)) { + while (!conn->quit) { + if (virCondWait(&conn->cond, &conn->parent.lock) < 0) {
The usage of the condition does not solve anything here. This function is called while still holding the lock on the connection cache, so as this condition waits until the connection is up the cache lock would be held as well, thus this does not fix the problem I've pointed out. You need to add a reference on the connection cache entry, add a reference, drop the lock on the connection cache and wait until it's ready. Also the condition requires that the mutex is locked, which you did not prior to trying to wait on it. This also means that you don't properly protect access to the cache entries. You have a lockable object but don't use it at all. Additionally, after virCondWait finishes the mutex is locked, but you don't ever unlock it.
+ virReportSystemError(errno, "%s", + _("failed to wait for domain condition"));
Again, wrongly copied message.
+ return NULL; + } + } + virObjectRef(conn); + return conn->fs;
Here this should return the whole entry ...
+ } + } + + return NULL; +} + +static void +virStorageBackendGlusterCacheRelease(virStorageSourcePtr src) +{ + virStorageBackendGlusterCacheConnPtr conn = NULL; + size_t i; + + virMutexLock(&virStorageBackendGlusterGlobalCacheObj->lock);
Please note that you can only lock the cache lock before locking the entry lock to avoid deadlocks. This currently is not a problem, because you don't lock the entries themselves, but you'll need to do that.
+ for (i = 0; i < virStorageBackendGlusterGlobalCacheObj->nConn; i++) { + conn = virStorageBackendGlusterGlobalCacheObj->conn[i]; + if (STRNEQ(conn->volname, src->volume)) + continue; + if (virStorageBackendGlusterCompareHosts(conn->nhosts, conn->hosts, + src->nhosts, src->hosts)) { + if (virObjectUnref(conn)) + goto unlock;
... so that you can store it in the private data and then it'll allow to look up by pointer comparison.
+ + VIR_DELETE_ELEMENT(virStorageBackendGlusterGlobalCacheObj->conn, i, + virStorageBackendGlusterGlobalCacheObj->nConn); + } + } + + unlock: + virMutexUnlock(&virStorageBackendGlusterGlobalCacheObj->lock); +}
static void virStorageBackendGlusterClose(virStorageBackendGlusterStatePtr state) @@ -69,6 +281,26 @@ virStorageBackendGlusterClose(virStorageBackendGlusterStatePtr state) VIR_FREE(state); }
+static void +virStorageBackendGlusterBroadcast(virStorageSourcePtr src) +{ + virStorageBackendGlusterCacheConnPtr conn; + size_t i; + + virMutexLock(&virStorageBackendGlusterGlobalCacheObj->lock); + for (i = 0; i < virStorageBackendGlusterGlobalCacheObj->nConn; i++) {
This function should not have to look up the connection once you pass in the cache entry itself, since you've added it a while prior calling this function. Also due to the fact that you hold the cache lock while waiting for the connection, this would deadlock.
+ conn = virStorageBackendGlusterGlobalCacheObj->conn[i]; + if (STRNEQ(conn->volname, src->volume)) + continue; + if (virStorageBackendGlusterCompareHosts(conn->nhosts, conn->hosts, + src->nhosts, src->hosts)) { + conn->quit = true; + virCondBroadcast(&conn->cond); + } + } + virMutexUnlock(&virStorageBackendGlusterGlobalCacheObj->lock); +} + static virStorageBackendGlusterStatePtr virStorageBackendGlusterOpen(virStoragePoolObjPtr pool) { @@ -538,15 +770,6 @@ virStorageBackend virStorageBackendGluster = { };
-typedef struct _virStorageFileBackendGlusterPriv virStorageFileBackendGlusterPriv; -typedef virStorageFileBackendGlusterPriv *virStorageFileBackendGlusterPrivPtr; - -struct _virStorageFileBackendGlusterPriv { - glfs_t *vol; - char *canonpath; -}; - - static void virStorageFileBackendGlusterDeinit(virStorageSourcePtr src) { @@ -562,10 +785,9 @@ virStorageFileBackendGlusterDeinit(virStorageSourcePtr src)
src->drv = NULL;
- if (priv->vol) - glfs_fini(priv->vol); - VIR_FREE(priv->canonpath); + virStorageBackendGlusterCacheRelease(src);
+ VIR_FREE(priv->canonpath); VIR_FREE(priv); }
@@ -613,6 +835,19 @@ virStorageFileBackendGlusterInitServer(virStorageFileBackendGlusterPrivPtr priv, return 0; }
+static void +virStorageBackendGlusterCacheInit(void) +{ + if (VIR_ALLOC(virStorageBackendGlusterGlobalCacheObj) < 0) + return; + + if (virMutexInit(&virStorageBackendGlusterGlobalCacheObj->lock) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Unable to initialize mutex")); + return; + } + errno = VIR_ERR_OK;
Please use VIR_ONCE_GLOBAL_INIT instead of abusing 'errno'.
+}
static int virStorageFileBackendGlusterInit(virStorageSourcePtr src) @@ -635,11 +870,38 @@ virStorageFileBackendGlusterInit(virStorageSourcePtr src) src, priv, src->volume, src->path, (unsigned int)src->drv->uid, (unsigned int)src->drv->gid);
+ if (virOnce(&virStorageBackendGlusterCacheOnce, + virStorageBackendGlusterCacheInit) < 0) + return -1;
And replace this with the appropriate call to the initializer, so that we don't have multiple different instances of the same approach.,
+ + if (errno) + return -errno; + + if (!virStorageBackendGlusterGlobalCacheObj) + return -1; + + if (virStorageBackendGlusterCacheConnInitialize() < 0) + return -1; + + virMutexLock(&virStorageBackendGlusterGlobalCacheObj->lock);
I'd strongly suggest you turn the cache into a virObjectLockable, so that you can use virObjectLock here instead of hand-rolling this.
+ + priv->vol = virStorageBackendGlusterCacheLookup(src); + if (priv->vol) { + src->drv->priv = priv; + virMutexUnlock(&virStorageBackendGlusterGlobalCacheObj->lock); + return 0; + } + if (!(priv->vol = glfs_new(src->volume))) { virReportOOMError(); - goto error; + goto unlock; }
+ if (virStorageBackendGlusterCacheAdd(src, priv->vol) < 0) + goto unlock; + + virMutexUnlock(&virStorageBackendGlusterGlobalCacheObj->lock); + for (i = 0; i < src->nhosts; i++) { if (virStorageFileBackendGlusterInitServer(priv, src->hosts + i) < 0) goto error; @@ -652,15 +914,17 @@ virStorageFileBackendGlusterInit(virStorageSourcePtr src) goto error; }
+ virStorageBackendGlusterBroadcast(src); + src->drv->priv = priv;
return 0;
- error: - if (priv->vol) - glfs_fini(priv->vol); - VIR_FREE(priv); + unlock: + virMutexUnlock(&virStorageBackendGlusterGlobalCacheObj->lock);
+ error: + virStorageBackendGlusterCacheRelease(src); return -1; }
Hi Virt Team, Please review this patch series. Thanks, -- Prasanna On Thu, Dec 15, 2016 at 1:37 PM, Prasanna Kumar Kalever <prasanna.kalever@redhat.com> wrote:
v5: Address review comments from Peter on v4 Dropping Patch 3 from v3 Patch 1: fix transport type in all drivers, possible switch case converts Patch 2: use virObject with src->drv to avoid 'initFlag' for remembering 'virStorageFileInit' status Patch 3: * change virStorageBackendGlusterCacheQuery to *Lookup * change virStorageBackendGlusterCacheRefresh to *Release * Avoid the global Flags * Thread safe measure by using virCond* Thanks Peter!
v4: Address review comments from Peter on v3 Patch 1: changes to fix test failures (qemuargv2xmltest, qemuxml2argvtest, qemuxml2xmltest and virstoragetest) Patch 2: fix hung in qemuxml2argvtest Patch 3 v3: Split this Patch to 2 parts as below Patch 3 v4: don't free metadata used for storage driver access Patch 4: * update variable naming as per standards * virStorageBackendGlusterPreopened* -> virStorageBackendGlusterCache* Thanks Peter!
v3: Address comments by Daniel and Peter on v2 * Split the patch to 3 parts Patch 1: change the virStorageNetHostDef type Patch 2: optimize calls to virStorageFileInit and friends Patch 3: add the caching for glfs * Thanks to Daniel, this version make all the methods as thread safe * Thanks to Peter for pointing use of virObjectLockable, this had simplified rest of the parts a lot.
v2: Address review comments from Peter on v1 * Rebased on latest master * Changes to commit msg * Introduce storage API's for Register and Unregister of volume * During qemu process Start/Stop and snapshot create * Check Transport and Port type * Atomic element add/del to list and ref counting Pending: Treating IP and FQDN belong to same host
v1: Initial patch
Prasanna Kumar Kalever (3): util: change the virStorageNetHostDef type storage: optimize calls to virStorageFileInit and friends gluster: cache glfs connection object per volume
src/conf/domain_conf.c | 60 +++-- src/libxl/libxl_conf.c | 10 +- src/qemu/qemu_command.c | 97 ++++--- src/qemu/qemu_domain.c | 25 +- src/qemu/qemu_domain.h | 8 +- src/qemu/qemu_driver.c | 26 +- src/qemu/qemu_parse_command.c | 50 ++-- src/qemu/qemu_process.c | 49 ++++ src/qemu/qemu_process.h | 4 + src/storage/storage_backend.h | 1 + src/storage/storage_backend_fs.c | 5 + src/storage/storage_backend_gluster.c | 329 ++++++++++++++++++++--- src/storage/storage_driver.c | 40 ++- src/storage/storage_driver.h | 2 + src/util/virstoragefile.c | 93 ++++--- src/util/virstoragefile.h | 21 +- src/xenconfig/xen_xl.c | 10 +- tests/domainsnapshotxml2xmlin/disk_snapshot.xml | 2 +- tests/domainsnapshotxml2xmlout/disk_snapshot.xml | 2 +- tests/virstoragetest.c | 2 +- 20 files changed, 614 insertions(+), 222 deletions(-)
-- 2.7.4
participants (3)
-
Peter Krempa -
Prasanna Kalever -
Prasanna Kumar Kalever