[libvirt] [PATCH v3 0/3] gluster: cache glfs connection object per volume
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 | 46 +++--- src/qemu/qemu_command.c | 65 ++++---- src/qemu/qemu_domain.c | 2 +- src/qemu/qemu_domain.h | 5 + src/qemu/qemu_driver.c | 40 +++-- src/qemu/qemu_parse_command.c | 44 +++--- src/qemu/qemu_process.c | 45 ++++++ src/qemu/qemu_process.h | 4 + src/storage/storage_backend_fs.c | 2 + src/storage/storage_backend_gluster.c | 270 ++++++++++++++++++++++++++++++---- src/storage/storage_driver.c | 23 +-- src/util/virstoragefile.c | 85 ++++++----- src/util/virstoragefile.h | 20 ++- tests/virstoragetest.c | 2 +- 14 files changed, 477 insertions(+), 176 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 | 46 ++++++++++--------- src/qemu/qemu_command.c | 65 +++++++++++++-------------- src/qemu/qemu_parse_command.c | 44 +++++++++--------- src/storage/storage_backend_gluster.c | 21 ++++----- src/storage/storage_driver.c | 7 ++- src/util/virstoragefile.c | 85 ++++++++++++++++++----------------- src/util/virstoragefile.h | 20 +++++++-- tests/virstoragetest.c | 2 +- 8 files changed, 154 insertions(+), 136 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index b001efc..bdb60a7 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -5849,6 +5849,7 @@ virDomainStorageHostParse(xmlNodePtr node, int ret = -1; xmlNodePtr child; char *transport = NULL; + char *socket = NULL; virStorageNetHostDef host; memset(&host, 0, sizeof(host)); @@ -5858,12 +5859,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); @@ -5871,17 +5873,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"), @@ -5889,16 +5891,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) @@ -13946,8 +13949,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; @@ -20204,16 +20207,17 @@ virDomainDiskSourceFormatInternal(virBufferPtr buf, for (n = 0; n < src->nhosts; n++) { virBufferAddLit(buf, "<host"); virBufferEscapeString(buf, " name='%s'", - src->hosts[n].name); + src->hosts[n].u.inet.addr); virBufferEscapeString(buf, " port='%s'", - src->hosts[n].port); + src->hosts[n].u.inet.port); - if (src->hosts[n].transport) + if (src->hosts[n].type) virBufferAsprintf(buf, " transport='%s'", - virStorageNetHostTransportTypeToString(src->hosts[n].transport)); + virStorageNetHostTransportTypeToString(src->hosts[n].type)); - virBufferEscapeString(buf, " socket='%s'", - src->hosts[n].socket); + if (src->hosts[n].type == VIR_STORAGE_NET_HOST_TRANS_UNIX) + virBufferEscapeString(buf, " socket='%s'", + src->hosts[n].u.uds.path); virBufferAddLit(buf, "/>\n"); } @@ -20984,8 +20988,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/qemu/qemu_command.c b/src/qemu/qemu_command.c index 6c457dd..5058560 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,19 +915,19 @@ qemuBuildNetworkDriveURI(virStorageSourcePtr src, if (VIR_ALLOC(uri) < 0) goto cleanup; - if (src->hosts->transport == VIR_STORAGE_NET_HOST_TRANS_TCP) { + if (src->hosts->type == 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) + virStorageNetHostTransportTypeToString(src->hosts->type)) < 0) goto cleanup; } if ((uri->port = qemuNetworkDriveGetPort(src->protocol, - src->hosts->port)) < 0) + src->hosts->u.inet.port)) < 0) goto cleanup; if (src->path) { @@ -944,14 +943,14 @@ qemuBuildNetworkDriveURI(virStorageSourcePtr src, } } - if (src->hosts->socket && - virAsprintf(&uri->query, "socket=%s", src->hosts->socket) < 0) + if ((src->hosts->type == VIR_STORAGE_NET_HOST_TRANS_UNIX) && + (virAsprintf(&uri->query, "socket=%s", src->hosts->u.uds.path) < 0)) goto cleanup; if (qemuBuildGeneralSecinfoURI(uri, secinfo) < 0) goto cleanup; - if (VIR_STRDUP(uri->server, src->hosts->name) < 0) + if (VIR_STRDUP(uri->server, src->hosts->u.inet.addr) < 0) goto cleanup; ret = virURIFormat(uri); @@ -979,38 +978,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 +1048,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 +1083,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..41a8172 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,19 @@ 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) + if (def->src->hosts->type != VIR_STORAGE_NET_HOST_TRANS_UNIX) { + 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; + def->src->hosts->u.inet.addr = NULL; + def->src->hosts->u.inet.port = 0; 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, @@ -221,8 +221,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 +233,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 +729,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 +2005,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 +2547,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 df65807..52099a2 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..900a801 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -1603,9 +1603,12 @@ virStorageNetHostDefClear(virStorageNetHostDefPtr def) if (!def) return; - VIR_FREE(def->name); - VIR_FREE(def->port); - VIR_FREE(def->socket); + if (def->type == VIR_STORAGE_NET_HOST_TRANS_UNIX) { + VIR_FREE(def->u.uds.path); + } else { + VIR_FREE(def->u.inet.addr); + VIR_FREE(def->u.inet.port); + } } @@ -1643,6 +1646,9 @@ virStorageNetHostDefCopy(size_t nhosts, virStorageNetHostDefPtr ret = NULL; size_t i; + if (!hosts) + return NULL; + if (VIR_ALLOC_N(ret, nhosts) < 0) goto error; @@ -1650,16 +1656,17 @@ 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; + if (src->type == VIR_STORAGE_NET_HOST_TRANS_UNIX) { + if (VIR_STRDUP(dst->u.uds.path, src->u.uds.path) < 0) + goto error; + } else { + 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; + } } return ret; @@ -1669,7 +1676,6 @@ virStorageNetHostDefCopy(size_t nhosts, return NULL; } - void virStorageAuthDefFree(virStorageAuthDefPtr authdef) { @@ -2292,7 +2298,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 +2307,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 +2345,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 +2384,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 +2529,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 +2548,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 +2559,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 +2569,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 +2729,7 @@ virStorageSourceParseBackingJSONGlusterHost(virStorageNetHostDefPtr host, return -1; } - host->transport = transport; + host->type = transport; switch ((virStorageNetHostTransport) transport) { case VIR_STORAGE_NET_HOST_TRANS_TCP: @@ -2735,8 +2740,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 +2754,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 +2871,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 +2937,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/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
On Mon, Dec 05, 2016 at 18:55:17 +0530, Prasanna Kumar Kalever wrote: qemuargv2xmltest, qemuxml2argvtest, qemuxml2xmltest, virstoragetest fail after this patch. I did not bother checking further or see the case. Peter
On Mon, Dec 5, 2016 at 7:25 PM, Peter Krempa <pkrempa@redhat.com> wrote:
On Mon, Dec 05, 2016 at 18:55:17 +0530, Prasanna Kumar Kalever wrote:
qemuargv2xmltest, qemuxml2argvtest, qemuxml2xmltest, virstoragetest fail after this patch. I did not bother checking further or see the case.
Yeah, I got them all. Thanks for bringing them. -- Prasanna
Peter
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 optimize/unify calls to virStorageFileInit and virStorageFileDeinit. Signed-off-by: Prasanna Kumar Kalever <prasanna.kalever@redhat.com> --- src/qemu/qemu_domain.c | 2 +- src/qemu/qemu_domain.h | 5 +++++ src/qemu/qemu_driver.c | 40 +++++++++++++++++++++++++++++---------- src/qemu/qemu_process.c | 45 ++++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_process.h | 4 ++++ src/storage/storage_driver.c | 11 ++++++++--- 6 files changed, 93 insertions(+), 14 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 47332a8..35914c5 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, diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 7650ff3..a9e38bd 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -591,6 +591,11 @@ bool qemuDomainDiskSourceDiffers(virDomainDiskDefPtr disk, bool qemuDomainDiskChangeSupported(virDomainDiskDefPtr disk, virDomainDiskDefPtr orig_disk); +void qemuDomainGetImageIds(virQEMUDriverConfigPtr cfg, + virDomainObjPtr vm, + virStorageSourcePtr src, + uid_t *uid, gid_t *gid); + int qemuDomainStorageFileInit(virQEMUDriverPtr driver, virDomainObjPtr vm, virStorageSourcePtr src); diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 3517aa2..ea76933 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -328,6 +328,7 @@ qemuSecurityChownCallback(virStorageSourcePtr src, { struct stat sb; int save_errno = 0; + bool initFlag = false; int ret = -1; if (!virStorageFileSupportsSecurityDriver(src)) @@ -351,8 +352,11 @@ qemuSecurityChownCallback(virStorageSourcePtr src, } /* storage file init reports errors, return -2 on failure */ - if (virStorageFileInit(src) < 0) - return -2; + if (!src->drv) { + if (virStorageFileInit(src) < 0) + return -2; + initFlag = true; + } if (virStorageFileChown(src, uid, gid) < 0) { save_errno = errno; @@ -362,7 +366,8 @@ qemuSecurityChownCallback(virStorageSourcePtr src, ret = 0; cleanup: - virStorageFileDeinit(src); + if (initFlag) + virStorageFileDeinit(src); errno = save_errno; return ret; @@ -13865,9 +13870,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 +13893,6 @@ qemuDomainSnapshotPrepareDiskExternal(virConnectPtr conn, ret = 0; cleanup: - virStorageFileDeinit(snapdisk->src); return ret; } @@ -14126,6 +14127,7 @@ qemuDomainSnapshotCreateSingleDiskActive(virQEMUDriverPtr driver, const char *formatStr = NULL; int ret = -1, rc; bool need_unlink = false; + bool initFlag = false; if (snap->snapshot != VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", @@ -14138,12 +14140,18 @@ qemuDomainSnapshotCreateSingleDiskActive(virQEMUDriverPtr driver, if (!(newDiskSrc = virStorageSourceCopy(snap->src, false))) goto cleanup; + else + if (snap->src->drv) + newDiskSrc->drv = snap->src->drv; if (virStorageSourceInitChainElement(newDiskSrc, disk->src, false) < 0) goto cleanup; - if (qemuDomainStorageFileInit(driver, vm, newDiskSrc) < 0) - goto cleanup; + if (!newDiskSrc->drv) { + if (qemuDomainStorageFileInit(driver, vm, newDiskSrc) < 0) + goto cleanup; + initFlag = true; + } if (qemuGetDriveSourceString(newDiskSrc, NULL, &source) < 0) goto cleanup; @@ -14211,7 +14219,8 @@ qemuDomainSnapshotCreateSingleDiskActive(virQEMUDriverPtr driver, cleanup: if (need_unlink && virStorageFileUnlink(newDiskSrc)) VIR_WARN("unable to unlink just-created %s", source); - virStorageFileDeinit(newDiskSrc); + if (initFlag) + virStorageFileDeinit(newDiskSrc); virStorageSourceFree(newDiskSrc); virStorageSourceFree(persistDiskSrc); VIR_FREE(device); @@ -14566,6 +14575,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 +14584,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 | @@ -14690,6 +14701,10 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain, qemuDomainObjSetAsyncJobMask(vm, QEMU_JOB_NONE); + for (i = 0; i < def->ndisks; i++) + if (virStorageVolumeRegister(cfg, vm, def->disks[i].src) < 0) + goto cleanup; + if (redefine) { if (virDomainSnapshotRedefinePrep(domain, vm, &def, &snap, &update_current, flags) < 0) @@ -14800,6 +14815,11 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain, snapshot = virGetDomainSnapshot(domain, snap->def->name); endjob: + refDef = (!snap) ? def : snap->def; + + for (i = 0; i < refDef->ndisks; i++) + virStorageVolumeUnRegister(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 ecd7ded..e949fc8 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -4588,6 +4588,35 @@ qemuProcessStartValidate(virQEMUDriverPtr driver, } +int +virStorageVolumeRegister(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 +virStorageVolumeUnRegister(virStorageSourcePtr src) +{ + if (virStorageSourceIsEmpty(src)) + return; + + virStorageFileDeinit(src); +} + + /** * qemuProcessInit: * @@ -4612,6 +4641,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); @@ -4664,6 +4694,10 @@ qemuProcessInit(virQEMUDriverPtr driver, if (qemuDomainSetPrivatePaths(driver, vm) < 0) goto cleanup; + for (i = 0; i < vm->def->ndisks; i++) + if (virStorageVolumeRegister(cfg, vm, vm->def->disks[i]->src) < 0) + goto cleanup; + ret = 0; cleanup: @@ -5671,6 +5705,7 @@ qemuProcessFinishStartup(virConnectPtr conn, { virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); int ret = -1; + size_t i; if (startCPUs) { VIR_DEBUG("Starting domain CPUs"); @@ -5695,6 +5730,9 @@ qemuProcessFinishStartup(virConnectPtr conn, VIR_HOOK_SUBOP_BEGIN) < 0) goto cleanup; + for (i = 0; i < vm->def->ndisks; i++) + virStorageVolumeUnRegister(vm->def->disks[i]->src); + ret = 0; cleanup: @@ -6044,6 +6082,10 @@ void qemuProcessStop(virQEMUDriverPtr driver, VIR_FREE(xml); } + for (i = 0; i < vm->def->ndisks; i++) + if (virStorageVolumeRegister(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, @@ -6207,6 +6249,9 @@ void qemuProcessStop(virQEMUDriverPtr driver, VIR_FREE(xml); } + for (i = 0; i < vm->def->ndisks; i++) + virStorageVolumeUnRegister(vm->def->disks[i]->src); + virDomainObjRemoveTransientDef(vm); endjob: diff --git a/src/qemu/qemu_process.h b/src/qemu/qemu_process.h index 21f3b0c..14f6ea0 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 virStorageVolumeRegister(virQEMUDriverConfigPtr cfg, + virDomainObjPtr vm, virStorageSourcePtr src); +void virStorageVolumeUnRegister(virStorageSourcePtr src); + int qemuProcessInit(virQEMUDriverPtr driver, virDomainObjPtr vm, qemuDomainAsyncJob asyncJob, diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 52099a2..bf7c938 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -3195,6 +3195,7 @@ virStorageFileGetMetadataRecurse(virStorageSourcePtr src, ssize_t headerLen; virStorageSourcePtr backingStore = NULL; int backingFormat; + bool initFlag = false; VIR_DEBUG("path=%s format=%d uid=%u gid=%u probe=%d", src->path, src->format, @@ -3204,8 +3205,11 @@ virStorageFileGetMetadataRecurse(virStorageSourcePtr src, if (!virStorageFileSupportsBackingChainTraversal(src)) return 0; - if (virStorageFileInitAs(src, uid, gid) < 0) - return -1; + if (!src->drv) { + if (virStorageFileInitAs(src, uid, gid) < 0) + return -1; + initFlag = true; + } if (virStorageFileAccess(src, F_OK) < 0) { if (src == parent) { @@ -3281,7 +3285,8 @@ virStorageFileGetMetadataRecurse(virStorageSourcePtr src, cleanup: VIR_FREE(buf); - virStorageFileDeinit(src); + if (initFlag) + virStorageFileDeinit(src); virStorageSourceFree(backingStore); return ret; } -- 2.7.4
On Mon, Dec 05, 2016 at 18:55:18 +0530, Prasanna Kumar Kalever wrote:
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 optimize/unify calls to virStorageFileInit and virStorageFileDeinit.
Signed-off-by: Prasanna Kumar Kalever <prasanna.kalever@redhat.com> --- src/qemu/qemu_domain.c | 2 +- src/qemu/qemu_domain.h | 5 +++++ src/qemu/qemu_driver.c | 40 +++++++++++++++++++++++++++++---------- src/qemu/qemu_process.c | 45 ++++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_process.h | 4 ++++ src/storage/storage_driver.c | 11 ++++++++--- 6 files changed, 93 insertions(+), 14 deletions(-)
After this patch qemuxml2argvtest gets stuck. This is because you call the code accessing the volumes in the test code which attempts to call gluster. This is not acceptable in tests. There should be some existing mechanisms which skips certain parts of the process in tests. See how VIR_QEMU_PROCESS_START_PRETEND is used. Peter
On Mon, Dec 5, 2016 at 7:34 PM, Peter Krempa <pkrempa@redhat.com> wrote:
On Mon, Dec 05, 2016 at 18:55:18 +0530, Prasanna Kumar Kalever wrote:
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 optimize/unify calls to virStorageFileInit and virStorageFileDeinit.
Signed-off-by: Prasanna Kumar Kalever <prasanna.kalever@redhat.com> --- src/qemu/qemu_domain.c | 2 +- src/qemu/qemu_domain.h | 5 +++++ src/qemu/qemu_driver.c | 40 +++++++++++++++++++++++++++++---------- src/qemu/qemu_process.c | 45 ++++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_process.h | 4 ++++ src/storage/storage_driver.c | 11 ++++++++--- 6 files changed, 93 insertions(+), 14 deletions(-)
After this patch qemuxml2argvtest gets stuck. This is because you call the code accessing the volumes in the test code which attempts to call gluster. This is not acceptable in tests. There should be some existing mechanisms which skips certain parts of the process in tests.
See how VIR_QEMU_PROCESS_START_PRETEND is used.
That's an intelligent clue. It really helped a lot. Caught it, It was in qemuProcessInit. Thanks, -- Prasanna
Peter
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_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); } 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" #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; +static virClassPtr virStorageBackendGlusterStatePreopenedClass; +static virOnceControl glusterConnOnce = VIR_ONCE_CONTROL_INITIALIZER; +static bool virStorageBackendGlusterPreopenedGlobalError; + + +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, + 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); + return 0; + } + } + virMutexUnlock(&connCache->lock); + + 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; + + 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) { + 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; + 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)) { + virObjectUnref(connCache->conn[i]); + if (connCache->conn[i]->volname) { + if (priv && priv->canonpath) + VIR_FREE(priv->canonpath); + goto unlock; + } + VIR_DELETE_ELEMENT(connCache->conn, i, connCache->nConn); + 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); + 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; } + if (virStorageBackendGlusterPreopenedSet(src, priv) < 0) + goto error; + 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); } -- 2.7.4
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@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.
+ + +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 ?
+ 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.
On Mon, Dec 5, 2016 at 8:08 PM, Peter Krempa <pkrempa@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@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' ?
+ + +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.
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@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@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
participants (3)
-
Peter Krempa -
Prasanna Kalever -
Prasanna Kumar Kalever