[libvirt] [PATCH v4 0/4] gluster: cache glfs connection object per volume
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 (4): util: change the virStorageNetHostDef type storage: optimize calls to virStorageFileInit and friends virStorageFileDeinit: don't free metadata used for storage driver access gluster: cache glfs connection object per volume src/conf/domain_conf.c | 52 +++--- src/qemu/qemu_command.c | 68 ++++---- 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 | 47 ++++++ src/qemu/qemu_process.h | 4 + src/storage/storage_backend_fs.c | 2 + src/storage/storage_backend_gluster.c | 299 ++++++++++++++++++++++++++++++---- src/storage/storage_driver.c | 23 +-- src/util/virstoragefile.c | 85 +++++----- src/util/virstoragefile.h | 20 ++- tests/virstoragetest.c | 2 +- 14 files changed, 512 insertions(+), 181 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 | 52 +++++++++++---------- src/qemu/qemu_command.c | 68 ++++++++++++++-------------- 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, 160 insertions(+), 139 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 0e879e1..20d4d01 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,20 @@ 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].type != VIR_STORAGE_NET_HOST_TRANS_UNIX) { + virBufferEscapeString(buf, " name='%s'", + src->hosts[n].u.inet.addr); + virBufferEscapeString(buf, " port='%s'", + 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"); } @@ -21103,8 +21109,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..fd62597 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,15 +943,16 @@ 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) - goto cleanup; + if (src->hosts->type != VIR_STORAGE_NET_HOST_TRANS_UNIX) + if (VIR_STRDUP(uri->server, src->hosts->u.inet.addr) < 0) + goto cleanup; ret = virURIFormat(uri); @@ -979,38 +979,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 +1049,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 +1084,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 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..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 Tue, Dec 06, 2016 at 22:51:58 +0530, Prasanna Kumar Kalever wrote:
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; };
I'm not really persuaded that this is much better than the current state. Data-wise you save one char *. Code wise you add the complexity of accessing the enum everywhere.
This patch performs the required changes in transforming _virStorageNetHostDef to fit union discrimination.
On a machine with xen installed this fails to compile: xenconfig/xen_xl.c: In function 'xenFormatXLDiskSrcNet': xenconfig/xen_xl.c:954:41: error: 'virStorageNetHostDef {aka struct _virStorageNetHostDef}' has no member named 'name' if (strchr(src->hosts[i].name, ':')) ^ xenconfig/xen_xl.c:956:50: error: 'virStorageNetHostDef {aka struct _virStorageNetHostDef}' has no member named 'name' src->hosts[i].name); ^ xenconfig/xen_xl.c:958:64: error: 'virStorageNetHostDef {aka struct _virStorageNetHostDef}' has no member named 'name' virBufferAsprintf(&buf, "%s", src->hosts[i].name); ^ xenconfig/xen_xl.c:960:34: error: 'virStorageNetHostDef {aka struct _virStorageNetHostDef}' has no member named 'port' if (src->hosts[i].port) ^ xenconfig/xen_xl.c:961:69: error: 'virStorageNetHostDef {aka struct _virStorageNetHostDef}' has no member named 'port' virBufferAsprintf(&buf, "\\\\:%s", src->hosts[i].port); I suspect there are more than just these in other hypervisor drivers. As said. I'm not a fan of this change. If you want to continue doing this, please make sure that you install all VM drivers so that you change all the appropriate places. Also please make sure to use a switch statement in appropriate places as described below.
Signed-off-by: Prasanna Kumar Kalever <prasanna.kalever@redhat.com> --- src/conf/domain_conf.c | 52 +++++++++++---------- src/qemu/qemu_command.c | 68 ++++++++++++++-------------- 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, 160 insertions(+), 139 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 0e879e1..20d4d01 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)) {
Too many parentheses, the inner terms have the same operator priority as with the parentheses.
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("unknown protocol transport type '%s'"), transport);
[...]
@@ -20322,17 +20325,20 @@ 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].type != VIR_STORAGE_NET_HOST_TRANS_UNIX) { + virBufferEscapeString(buf, " name='%s'", + src->hosts[n].u.inet.addr); + virBufferEscapeString(buf, " port='%s'", + 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");
You need to convert this hunk to use a switch, so the code is extensible.
}
[...]
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 6c457dd..fd62597 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;
Wrong indentation.
if (virJSONValueObjectAdd(server, - "s:host", host->name, + "s:host", host->u.inet.addr, "s:port", portstr, NULL) < 0) goto cleanup;
[...]
@@ -944,15 +943,16 @@ 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) - goto cleanup; + if (src->hosts->type != VIR_STORAGE_NET_HOST_TRANS_UNIX) + if (VIR_STRDUP(uri->server, src->hosts->u.inet.addr) < 0) + goto cleanup;
This also should be converted to a switch.
ret = virURIFormat(uri);
[...]
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) {
This is not extensible. Use switch and typecast if def->type is not of the correct type.
+ 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; +
This change is not relevant to the rest of the patch.
if (VIR_ALLOC_N(ret, nhosts) < 0) goto error;
@@ -1669,7 +1676,6 @@ virStorageNetHostDefCopy(size_t nhosts, return NULL; }
-
Spurious whitespace change.
void virStorageAuthDefFree(virStorageAuthDefPtr authdef) {
On Wed, Dec 7, 2016 at 7:31 PM, Peter Krempa <pkrempa@redhat.com> wrote:
On Tue, Dec 06, 2016 at 22:51:58 +0530, Prasanna Kumar Kalever wrote:
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; };
I'm not really persuaded that this is much better than the current state. Data-wise you save one char *. Code wise you add the complexity of accessing the enum everywhere.
This patch performs the required changes in transforming _virStorageNetHostDef to fit union discrimination.
On a machine with xen installed this fails to compile:
xenconfig/xen_xl.c: In function 'xenFormatXLDiskSrcNet': xenconfig/xen_xl.c:954:41: error: 'virStorageNetHostDef {aka struct _virStorageNetHostDef}' has no member named 'name' if (strchr(src->hosts[i].name, ':')) ^ xenconfig/xen_xl.c:956:50: error: 'virStorageNetHostDef {aka struct _virStorageNetHostDef}' has no member named 'name' src->hosts[i].name); ^ xenconfig/xen_xl.c:958:64: error: 'virStorageNetHostDef {aka struct _virStorageNetHostDef}' has no member named 'name' virBufferAsprintf(&buf, "%s", src->hosts[i].name); ^ xenconfig/xen_xl.c:960:34: error: 'virStorageNetHostDef {aka struct _virStorageNetHostDef}' has no member named 'port' if (src->hosts[i].port) ^ xenconfig/xen_xl.c:961:69: error: 'virStorageNetHostDef {aka struct _virStorageNetHostDef}' has no member named 'port' virBufferAsprintf(&buf, "\\\\:%s", src->hosts[i].port);
I suspect there are more than just these in other hypervisor drivers.
HUhhh... How to make sure that I have covered all the drivers ? Any long list of all drivers in libvirt or config to compile all the modules ? Thanks, -- Prasanna
As said. I'm not a fan of this change. If you want to continue doing this, please make sure that you install all VM drivers so that you change all the appropriate places.
Also please make sure to use a switch statement in appropriate places as described below.
Signed-off-by: Prasanna Kumar Kalever <prasanna.kalever@redhat.com> --- src/conf/domain_conf.c | 52 +++++++++++---------- src/qemu/qemu_command.c | 68 ++++++++++++++-------------- 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, 160 insertions(+), 139 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 0e879e1..20d4d01 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)) {
Too many parentheses, the inner terms have the same operator priority as with the parentheses.
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("unknown protocol transport type '%s'"), transport);
[...]
@@ -20322,17 +20325,20 @@ 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].type != VIR_STORAGE_NET_HOST_TRANS_UNIX) { + virBufferEscapeString(buf, " name='%s'", + src->hosts[n].u.inet.addr); + virBufferEscapeString(buf, " port='%s'", + 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");
You need to convert this hunk to use a switch, so the code is extensible.
}
[...]
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 6c457dd..fd62597 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;
Wrong indentation.
if (virJSONValueObjectAdd(server, - "s:host", host->name, + "s:host", host->u.inet.addr, "s:port", portstr, NULL) < 0) goto cleanup;
[...]
@@ -944,15 +943,16 @@ 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) - goto cleanup; + if (src->hosts->type != VIR_STORAGE_NET_HOST_TRANS_UNIX) + if (VIR_STRDUP(uri->server, src->hosts->u.inet.addr) < 0) + goto cleanup;
This also should be converted to a switch.
ret = virURIFormat(uri);
[...]
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) {
This is not extensible. Use switch and typecast if def->type is not of the correct type.
+ 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; +
This change is not relevant to the rest of the patch.
if (VIR_ALLOC_N(ret, nhosts) < 0) goto error;
@@ -1669,7 +1676,6 @@ virStorageNetHostDefCopy(size_t nhosts, return NULL; }
-
Spurious whitespace change.
void virStorageAuthDefFree(virStorageAuthDefPtr authdef) {
On Mon, Dec 12, 2016 at 19:54:47 +0530, Prasanna Kalever wrote:
On Wed, Dec 7, 2016 at 7:31 PM, Peter Krempa <pkrempa@redhat.com> wrote:
On Tue, Dec 06, 2016 at 22:51:58 +0530, Prasanna Kumar Kalever wrote:
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; };
I'm not really persuaded that this is much better than the current state. Data-wise you save one char *. Code wise you add the complexity of accessing the enum everywhere.
This patch performs the required changes in transforming _virStorageNetHostDef to fit union discrimination.
On a machine with xen installed this fails to compile:
xenconfig/xen_xl.c: In function 'xenFormatXLDiskSrcNet': xenconfig/xen_xl.c:954:41: error: 'virStorageNetHostDef {aka struct _virStorageNetHostDef}' has no member named 'name' if (strchr(src->hosts[i].name, ':')) ^ xenconfig/xen_xl.c:956:50: error: 'virStorageNetHostDef {aka struct _virStorageNetHostDef}' has no member named 'name' src->hosts[i].name); ^ xenconfig/xen_xl.c:958:64: error: 'virStorageNetHostDef {aka struct _virStorageNetHostDef}' has no member named 'name' virBufferAsprintf(&buf, "%s", src->hosts[i].name); ^ xenconfig/xen_xl.c:960:34: error: 'virStorageNetHostDef {aka struct _virStorageNetHostDef}' has no member named 'port' if (src->hosts[i].port) ^ xenconfig/xen_xl.c:961:69: error: 'virStorageNetHostDef {aka struct _virStorageNetHostDef}' has no member named 'port' virBufferAsprintf(&buf, "\\\\:%s", src->hosts[i].port);
I suspect there are more than just these in other hypervisor drivers.
HUhhh... How to make sure that I have covered all the drivers ? Any long list of all drivers in libvirt or config to compile all the modules ?
Output of 'configure' lists which ones are enabled and which are disabled. I'd suggest that you drop this patch though as the complexity of accessing the union is not worth the gain of removing one pointer.
On Mon, Dec 12, 2016 at 8:00 PM, Peter Krempa <pkrempa@redhat.com> wrote:
On Mon, Dec 12, 2016 at 19:54:47 +0530, Prasanna Kalever wrote:
On Wed, Dec 7, 2016 at 7:31 PM, Peter Krempa <pkrempa@redhat.com> wrote:
On Tue, Dec 06, 2016 at 22:51:58 +0530, Prasanna Kumar Kalever wrote:
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; };
I'm not really persuaded that this is much better than the current state. Data-wise you save one char *. Code wise you add the complexity of accessing the enum everywhere.
This patch performs the required changes in transforming _virStorageNetHostDef to fit union discrimination.
On a machine with xen installed this fails to compile:
xenconfig/xen_xl.c: In function 'xenFormatXLDiskSrcNet': xenconfig/xen_xl.c:954:41: error: 'virStorageNetHostDef {aka struct _virStorageNetHostDef}' has no member named 'name' if (strchr(src->hosts[i].name, ':')) ^ xenconfig/xen_xl.c:956:50: error: 'virStorageNetHostDef {aka struct _virStorageNetHostDef}' has no member named 'name' src->hosts[i].name); ^ xenconfig/xen_xl.c:958:64: error: 'virStorageNetHostDef {aka struct _virStorageNetHostDef}' has no member named 'name' virBufferAsprintf(&buf, "%s", src->hosts[i].name); ^ xenconfig/xen_xl.c:960:34: error: 'virStorageNetHostDef {aka struct _virStorageNetHostDef}' has no member named 'port' if (src->hosts[i].port) ^ xenconfig/xen_xl.c:961:69: error: 'virStorageNetHostDef {aka struct _virStorageNetHostDef}' has no member named 'port' virBufferAsprintf(&buf, "\\\\:%s", src->hosts[i].port);
I suspect there are more than just these in other hypervisor drivers.
HUhhh... How to make sure that I have covered all the drivers ? Any long list of all drivers in libvirt or config to compile all the modules ?
Output of 'configure' lists which ones are enabled and which are disabled.
I'd suggest that you drop this patch though as the complexity of accessing the union is not worth the gain of removing one pointer.
IMO, this is not just about saving a pointer. It is preferred to have the transport elements on the type. Please excuse me, if it doesn't make sense to mention about other projects approaches here but, recently after qemu add supports to qapi union discrimination it now follows similar union. In terms of all the gluster api users does use the union discrimination (there are another bunch of projects) If we cannot migrate to this now, Its very unfortunate we get this chance again. I shall post a patch addressing all the review comments in your previous email, lets give a final try ? Apologies for stressing this out. Thanks, -- Prasanna
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 | 47 ++++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_process.h | 4 ++++ src/storage/storage_driver.c | 11 ++++++++--- 6 files changed, 95 insertions(+), 14 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 4aae14d..43579ed 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..d0e05f8 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 (qemuStorageVolumeRegister(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++) + 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..c58e622 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,10 @@ void qemuProcessStop(virQEMUDriverPtr driver, VIR_FREE(xml); } + for (i = 0; i < vm->def->ndisks; i++) + 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 +6254,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_driver.c b/src/storage/storage_driver.c index 7c7fddd..24e2f35 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 Tue, Dec 06, 2016 at 22:51:59 +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.
A bit clearer explanation would help. This is mostly relevat for VM startup where the multiple calls are done. VM shutdown currently calls just one operation, but is not optimized with this patch.
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 | 47 ++++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_process.h | 4 ++++ src/storage/storage_driver.c | 11 ++++++++--- 6 files changed, 95 insertions(+), 14 deletions(-)
Read below for a suggestion of a farbetter approach than having the 'initFlag' variable in all places that call the code. [...]
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 3517aa2..d0e05f8 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;
I'd call this 'deinitsrc'.
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) {
You should export and reuse virStorageFileIsInitialized rather than reimplementing the detection.
+ 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;
The above code is confusing and does not conform to the coding style. Drop the 'else' statement completely, since the first part jumps to cleanup anyways. Also snap->src->drv won't ever be initialized. the 'snap' object is freshly created from the XML provided by the user so it never was part of the definition and thus couldn't ever be initialized.
if (virStorageSourceInitChainElement(newDiskSrc, disk->src, false) < 0) goto cleanup;
- if (qemuDomainStorageFileInit(driver, vm, newDiskSrc) < 0) - goto cleanup; + if (!newDiskSrc->drv) {
This condition is always true due to the fact above.
+ 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 (qemuStorageVolumeRegister(cfg, vm, def->disks[i].src) < 0) + goto cleanup;
Initializing the domain disks is useless at this point. The new snapshot volumes will be accessed, not the existing disks. This code also is not necessary for internal snapshots and thus should not be executed in such cases.
+ 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++) + 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..c58e622 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); +}
This wrapper is rather useless. You can call virStorageFileDeinit on volume files that were not initialized.
+ + /** * 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;
This does not conform to the coding style. Multi line bodies need to be included in a block.
+ } + ret = 0;
cleanup: @@ -6047,6 +6087,10 @@ void qemuProcessStop(virQEMUDriverPtr driver, VIR_FREE(xml); }
+ for (i = 0; i < vm->def->ndisks; i++) + 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,
If it's requested not to relabel the file as seed just below the code you added, you don't need to initialize the volume at all.
@@ -6210,6 +6254,9 @@ void qemuProcessStop(virQEMUDriverPtr driver, VIR_FREE(xml); }
+ for (i = 0; i < vm->def->ndisks; i++) + qemuStorageVolumeUnRegister(vm->def->disks[i]->src);
In the above case, this will need to be skipped as well.
+ virDomainObjRemoveTransientDef(vm);
endjob:
[...]
diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 7c7fddd..24e2f35 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; }
Alternatively and preferably you could turn src->drv into a object. This would add the option to do reference counting and would get rid of the need to remember whether a given piece of code needs to deinitialize a storage volume. Most of the changes above would not be necessary then. Peter
On Wed, Dec 7, 2016 at 9:38 PM, Peter Krempa <pkrempa@redhat.com> wrote:
On Tue, Dec 06, 2016 at 22:51:59 +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.
A bit clearer explanation would help. This is mostly relevat for VM startup where the multiple calls are done. VM shutdown currently calls just one operation, but is not optimized with this patch.
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 | 47 ++++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_process.h | 4 ++++ src/storage/storage_driver.c | 11 ++++++++--- 6 files changed, 95 insertions(+), 14 deletions(-)
Read below for a suggestion of a farbetter approach than having the 'initFlag' variable in all places that call the code.
[...]
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 3517aa2..d0e05f8 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;
I'd call this 'deinitsrc'.
I like it.
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) {
You should export and reuse virStorageFileIsInitialized rather than reimplementing the detection.
Okay!
+ 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;
The above code is confusing and does not conform to the coding style.
Drop the 'else' statement completely, since the first part jumps to cleanup anyways.
correct, just over looked at it :)
Also snap->src->drv won't ever be initialized. the 'snap' object is freshly created from the XML provided by the user so it never was part of the definition and thus couldn't ever be initialized.
As part of this patch the initialization of the external snapshot file happens as part of qemuStorageVolumeRegister in qemuDomainSnapshotCreateXML and servers the initialized data to 1. qemuDomainSnapshotPrepareDiskExternal called by qemuDomainSnapshotPrepare 2. qemuDomainSnapshotCreateSingleDiskActive 3. qemuSecurityChownCallback and 4. virStorageFileGetMetadataRecurse saving the calls to virStorageFileInitAs at each points literally 4 calls May be I can change the hunk to use virStorageFileIsInitialized.
if (virStorageSourceInitChainElement(newDiskSrc, disk->src, false) < 0) goto cleanup;
- if (qemuDomainStorageFileInit(driver, vm, newDiskSrc) < 0) - goto cleanup; + if (!newDiskSrc->drv) {
This condition is always true due to the fact above.
same here.
+ 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 (qemuStorageVolumeRegister(cfg, vm, def->disks[i].src) < 0) + goto cleanup;
Initializing the domain disks is useless at this point. The new snapshot volumes will be accessed, not the existing disks.
Let me know if I read it wrong. In this function variable 'snap->def' will be assigned to 'def' in virDomainSnapshotAssignDef so we should initialize def->disks[ ] right ?
This code also is not necessary for internal snapshots and thus should not be executed in such cases.
Yeah revised one should look like for (i = 0; i < def->ndisks; i++) { if(def->disks[i].snapshot == VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL) { if (qemuStorageVolumeRegister(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++) + 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..c58e622 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); +}
This wrapper is rather useless. You can call virStorageFileDeinit on volume files that were not initialized.
Right! knowing the fact, thought good to have a Unregister for counter function Register ?
+ + /** * 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;
This does not conform to the coding style. Multi line bodies need to be included in a block.
+ } + ret = 0;
cleanup: @@ -6047,6 +6087,10 @@ void qemuProcessStop(virQEMUDriverPtr driver, VIR_FREE(xml); }
+ for (i = 0; i < vm->def->ndisks; i++) + 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,
If it's requested not to relabel the file as seed just below the code you added, you don't need to initialize the volume at all.
@@ -6210,6 +6254,9 @@ void qemuProcessStop(virQEMUDriverPtr driver, VIR_FREE(xml); }
+ for (i = 0; i < vm->def->ndisks; i++) + qemuStorageVolumeUnRegister(vm->def->disks[i]->src);
In the above case, this will need to be skipped as well.
+ virDomainObjRemoveTransientDef(vm);
endjob:
[...]
diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 7c7fddd..24e2f35 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; }
Alternatively and preferably you could turn src->drv into a object. This would add the option to do reference counting and would get rid of the need to remember whether a given piece of code needs to deinitialize a storage volume. Most of the changes above would not be necessary then.
I doubt If I got you right here. An object as part of ? Thanks, -- Prasanna
Peter
On Mon, Dec 12, 2016 at 20:01:35 +0530, Prasanna Kalever wrote:
On Wed, Dec 7, 2016 at 9:38 PM, Peter Krempa <pkrempa@redhat.com> wrote:
On Tue, Dec 06, 2016 at 22:51:59 +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.
A bit clearer explanation would help. This is mostly relevat for VM startup where the multiple calls are done. VM shutdown currently calls just one operation, but is not optimized with this patch.
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 | 47 ++++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_process.h | 4 ++++ src/storage/storage_driver.c | 11 ++++++++--- 6 files changed, 95 insertions(+), 14 deletions(-)
Read below for a suggestion of a farbetter approach than having the 'initFlag' variable in all places that call the code.
[...]
Also snap->src->drv won't ever be initialized. the 'snap' object is freshly created from the XML provided by the user so it never was part of the definition and thus couldn't ever be initialized.
As part of this patch the initialization of the external snapshot file happens as part of qemuStorageVolumeRegister in qemuDomainSnapshotCreateXML and servers the initialized data to 1. qemuDomainSnapshotPrepareDiskExternal called by qemuDomainSnapshotPrepare 2. qemuDomainSnapshotCreateSingleDiskActive 3. qemuSecurityChownCallback and 4. virStorageFileGetMetadataRecurse
saving the calls to virStorageFileInitAs at each points literally 4 calls
May be I can change the hunk to use virStorageFileIsInitialized.
Yes I thought you were not initializing it originally. That's fine then. ...
if (virStorageSourceInitChainElement(newDiskSrc, disk->src, false) < 0) goto cleanup;
- if (qemuDomainStorageFileInit(driver, vm, newDiskSrc) < 0) - goto cleanup; + if (!newDiskSrc->drv) {
This condition is always true due to the fact above.
same here.
... That's true. But if you make sure that the caller always initializes it, the condition will become always false.
+ if (qemuDomainStorageFileInit(driver, vm, newDiskSrc) < 0) + goto cleanup; + initFlag = true; + }
if (qemuGetDriveSourceString(newDiskSrc, NULL, &source) < 0) goto cleanup;
@@ -14690,6 +14701,10 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain,
qemuDomainObjSetAsyncJobMask(vm, QEMU_JOB_NONE);
+ for (i = 0; i < def->ndisks; i++) + if (qemuStorageVolumeRegister(cfg, vm, def->disks[i].src) < 0) + goto cleanup;
Initializing the domain disks is useless at this point. The new snapshot volumes will be accessed, not the existing disks.
Let me know if I read it wrong. In this function variable 'snap->def' will be assigned to 'def' in virDomainSnapshotAssignDef so we should initialize def->disks[ ] right ?
Yes you are right. I was probably looking ad a different function, since libvirt almost everywhere uses 'def' for the domain definition.
This code also is not necessary for internal snapshots and thus should not be executed in such cases.
Yeah revised one should look like
for (i = 0; i < def->ndisks; i++) { if(def->disks[i].snapshot == VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL) { if (qemuStorageVolumeRegister(cfg, vm, def->disks[i].src) < 0) goto cleanup; }
Yes that's right. Also you should execute this code after the call to virDomainSnapshotAlignDisks and qemuDomainSnapshotPrepare. They are checking various stuff and determining the default location for the disks, so the initialization would fail in some cases otherwise.
}
[...]
+ +void +qemuStorageVolumeUnRegister(virStorageSourcePtr src) +{ + if (virStorageSourceIsEmpty(src)) + return; + + virStorageFileDeinit(src); +}
This wrapper is rather useless. You can call virStorageFileDeinit on volume files that were not initialized.
Right! knowing the fact, thought good to have a Unregister for counter function Register ?
Okay. It may make sense in the future to have all the exit points marked using a special function. [...]
diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 7c7fddd..24e2f35 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c
[...]
@@ -3281,7 +3285,8 @@ virStorageFileGetMetadataRecurse(virStorageSourcePtr src,
cleanup: VIR_FREE(buf); - virStorageFileDeinit(src); + if (initFlag) + virStorageFileDeinit(src); virStorageSourceFree(backingStore); return ret; }
Alternatively and preferably you could turn src->drv into a object. This would add the option to do reference counting and would get rid of the need to remember whether a given piece of code needs to deinitialize a storage volume. Most of the changes above would not be necessary then.
I doubt If I got you right here. An object as part of ?
I meant to make it a virObject, so that you can refcount it. The need for a flag to track whether it was initialized is rather fragile.
On Mon, Dec 12, 2016 at 9:53 PM, Peter Krempa <pkrempa@redhat.com> wrote:
On Mon, Dec 12, 2016 at 20:01:35 +0530, Prasanna Kalever wrote:
On Wed, Dec 7, 2016 at 9:38 PM, Peter Krempa <pkrempa@redhat.com> wrote:
On Tue, Dec 06, 2016 at 22:51:59 +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.
A bit clearer explanation would help. This is mostly relevat for VM startup where the multiple calls are done. VM shutdown currently calls just one operation, but is not optimized with this patch.
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 | 47 ++++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_process.h | 4 ++++ src/storage/storage_driver.c | 11 ++++++++--- 6 files changed, 95 insertions(+), 14 deletions(-)
Read below for a suggestion of a farbetter approach than having the 'initFlag' variable in all places that call the code.
[...]
Also snap->src->drv won't ever be initialized. the 'snap' object is freshly created from the XML provided by the user so it never was part of the definition and thus couldn't ever be initialized.
As part of this patch the initialization of the external snapshot file happens as part of qemuStorageVolumeRegister in qemuDomainSnapshotCreateXML and servers the initialized data to 1. qemuDomainSnapshotPrepareDiskExternal called by qemuDomainSnapshotPrepare 2. qemuDomainSnapshotCreateSingleDiskActive 3. qemuSecurityChownCallback and 4. virStorageFileGetMetadataRecurse
saving the calls to virStorageFileInitAs at each points literally 4 calls
May be I can change the hunk to use virStorageFileIsInitialized.
Yes I thought you were not initializing it originally. That's fine then.
...
if (virStorageSourceInitChainElement(newDiskSrc, disk->src, false) < 0) goto cleanup;
- if (qemuDomainStorageFileInit(driver, vm, newDiskSrc) < 0) - goto cleanup; + if (!newDiskSrc->drv) {
This condition is always true due to the fact above.
same here.
...
That's true. But if you make sure that the caller always initializes it, the condition will become always false.
Oh yeah!
+ if (qemuDomainStorageFileInit(driver, vm, newDiskSrc) < 0) + goto cleanup; + initFlag = true; + }
if (qemuGetDriveSourceString(newDiskSrc, NULL, &source) < 0) goto cleanup;
@@ -14690,6 +14701,10 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain,
qemuDomainObjSetAsyncJobMask(vm, QEMU_JOB_NONE);
+ for (i = 0; i < def->ndisks; i++) + if (qemuStorageVolumeRegister(cfg, vm, def->disks[i].src) < 0) + goto cleanup;
Initializing the domain disks is useless at this point. The new snapshot volumes will be accessed, not the existing disks.
Let me know if I read it wrong. In this function variable 'snap->def' will be assigned to 'def' in virDomainSnapshotAssignDef so we should initialize def->disks[ ] right ?
Yes you are right. I was probably looking ad a different function, since libvirt almost everywhere uses 'def' for the domain definition.
This code also is not necessary for internal snapshots and thus should not be executed in such cases.
Yeah revised one should look like
for (i = 0; i < def->ndisks; i++) { if(def->disks[i].snapshot == VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL) { if (qemuStorageVolumeRegister(cfg, vm, def->disks[i].src) < 0) goto cleanup; }
Yes that's right.
Also you should execute this code after the call to virDomainSnapshotAlignDisks and qemuDomainSnapshotPrepare. They are checking various stuff and determining the default location for the disks, so the initialization would fail in some cases otherwise.
In that case the *VolumeRegister* needs to happen as part of 'qemuDomainSnapshotPrepare' to avoid 'virStorageFileInit' call in 'qemuDomainSnapshotPrepareDiskExternal'. and then the UnRegister happens as part of 'qemuDomainSnapshotCreateXML' Hope that should be fine ?
}
[...]
+ +void +qemuStorageVolumeUnRegister(virStorageSourcePtr src) +{ + if (virStorageSourceIsEmpty(src)) + return; + + virStorageFileDeinit(src); +}
This wrapper is rather useless. You can call virStorageFileDeinit on volume files that were not initialized.
Right! knowing the fact, thought good to have a Unregister for counter function Register ?
Okay. It may make sense in the future to have all the exit points marked using a special function.
Yeah, sure.
[...]
diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 7c7fddd..24e2f35 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c
[...]
@@ -3281,7 +3285,8 @@ virStorageFileGetMetadataRecurse(virStorageSourcePtr src,
cleanup: VIR_FREE(buf); - virStorageFileDeinit(src); + if (initFlag) + virStorageFileDeinit(src); virStorageSourceFree(backingStore); return ret; }
Alternatively and preferably you could turn src->drv into a object. This would add the option to do reference counting and would get rid of the need to remember whether a given piece of code needs to deinitialize a storage volume. Most of the changes above would not be necessary then.
I doubt If I got you right here. An object as part of ?
I meant to make it a virObject, so that you can refcount it. The need for a flag to track whether it was initialized is rather fragile.
Make sense. Thanks, -- Prasanna
Let the metadata for storage driver access to remote and local volumes be cleaned by its respective driver *Deinit methods. This will be used in the next patch, which will implement a connection cache for/in gluster protocol driver. Signed-off-by: Prasanna Kumar Kalever <prasanna.kalever@redhat.com> --- src/storage/storage_backend_fs.c | 2 ++ src/storage/storage_backend_gluster.c | 3 ++- src/storage/storage_driver.c | 5 +---- 3 files changed, 5 insertions(+), 5 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..e0841ca 100644 --- a/src/storage/storage_backend_gluster.c +++ b/src/storage/storage_backend_gluster.c @@ -562,7 +562,8 @@ virStorageFileBackendGlusterDeinit(virStorageSourcePtr src) VIR_FREE(priv->canonpath); VIR_FREE(priv); - src->drv->priv = NULL; + + VIR_FREE(src->drv); } static int diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 24e2f35..2a4e160 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 Tue, Dec 06, 2016 at 22:52:00 +0530, Prasanna Kumar Kalever wrote:
Let the metadata for storage driver access to remote and local volumes be cleaned by its respective driver *Deinit methods.
This will be used in the next patch, which will implement a connection cache for/in gluster protocol driver.
Signed-off-by: Prasanna Kumar Kalever <prasanna.kalever@redhat.com> --- src/storage/storage_backend_fs.c | 2 ++ src/storage/storage_backend_gluster.c | 3 ++- src/storage/storage_driver.c | 5 +---- 3 files changed, 5 insertions(+), 5 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);
Hmm, so this is a global (for the storage driver) data structure for the initialized file. It contains the function pointers and a pointer to private data. Even after this patch it's still initialized in the global function. Also the src->drv object describes the state of a given virStorageSource. I thought you wanted to get rid of this construct and replace it by something else, but looking at the following patches it's not the case. As of such, this change does not make sense. virStorageFileInit(As) allocates this structure, so virStorageFileDeinit should free it. NACK
On Wed, Dec 7, 2016 at 7:39 PM, Peter Krempa <pkrempa@redhat.com> wrote:
On Tue, Dec 06, 2016 at 22:52:00 +0530, Prasanna Kumar Kalever wrote:
Let the metadata for storage driver access to remote and local volumes be cleaned by its respective driver *Deinit methods.
This will be used in the next patch, which will implement a connection cache for/in gluster protocol driver.
Signed-off-by: Prasanna Kumar Kalever <prasanna.kalever@redhat.com> --- src/storage/storage_backend_fs.c | 2 ++ src/storage/storage_backend_gluster.c | 3 ++- src/storage/storage_driver.c | 5 +---- 3 files changed, 5 insertions(+), 5 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);
Hmm, so this is a global (for the storage driver) data structure for the initialized file. It contains the function pointers and a pointer to private data.
Even after this patch it's still initialized in the global function.
Also the src->drv object describes the state of a given virStorageSource.
I thought you wanted to get rid of this construct and replace it by something else, but looking at the following patches it's not the case.
Something like ? Apologies, I don't understand your intention here, with some clues and if it is needed I can make it better.
As of such, this change does not make sense. virStorageFileInit(As) allocates this structure, so virStorageFileDeinit should free it.
Okay, will discard this patch Thanks, -- Prasanna
NACK
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 | 279 +++++++++++++++++++++++++++++++--- 1 file changed, 254 insertions(+), 25 deletions(-) diff --git a/src/storage/storage_backend_gluster.c b/src/storage/storage_backend_gluster.c index e0841ca..9f633ac 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,205 @@ 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; + virStorageFileBackendGlusterPrivPtr priv; + 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 bool virStorageBackendGlusterCacheInitGlobalError; +static bool virStorageBackendGlusterCacheConnCleaned; + + +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->priv->vol); + + VIR_FREE(conn->priv->canonpath); + VIR_FREE(conn->priv); + VIR_FREE(conn->volname); + + virStorageNetHostDefFree(conn->nhosts, conn->hosts); + + /* all set to delete the connection from cache */ + virStorageBackendGlusterCacheConnCleaned = true; +} + +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, + virStorageFileBackendGlusterPrivPtr priv) +{ + 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 (virStorageBackendGlusterCacheConnInitialize() < 0) + return -1; + + if (!(newConn = virObjectLockableNew(virStorageBackendGlusterCacheConnClass))) + return -1; + + 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->priv = priv; + + if (VIR_APPEND_ELEMENT(virStorageBackendGlusterGlobalCacheObj->conn, + virStorageBackendGlusterGlobalCacheObj->nConn, + newConn) < 0) + goto error; + + return 0; + + error: + virStorageNetHostDefFree(newConn->nhosts, newConn->hosts); + VIR_FREE(newConn->volname); + return -1; +} + +static glfs_t * +virStorageBackendGlusterCacheQuery(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)) { + virObjectRef(conn); + return conn->priv->vol; + } + } + + return NULL; +} + +static void +virStorageBackendGlusterCacheRefresh(virStorageSourcePtr src) +{ + virStorageFileBackendGlusterPrivPtr priv = src->drv->priv; + 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)) { + virObjectUnref(conn); + if (!virStorageBackendGlusterCacheConnCleaned) { + if (priv && priv->canonpath) + VIR_FREE(priv->canonpath); + goto unlock; + } + virStorageBackendGlusterCacheConnCleaned = false; + VIR_DELETE_ELEMENT(virStorageBackendGlusterGlobalCacheObj->conn, i, + virStorageBackendGlusterGlobalCacheObj->nConn); + VIR_FREE(src->drv); + } + } + + unlock: + virMutexUnlock(&virStorageBackendGlusterGlobalCacheObj->lock); +} static void virStorageBackendGlusterClose(virStorageBackendGlusterStatePtr state) @@ -538,32 +749,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); - - VIR_FREE(src->drv); + virStorageBackendGlusterCacheRefresh(src); } static int @@ -610,6 +804,20 @@ virStorageFileBackendGlusterInitServer(virStorageFileBackendGlusterPrivPtr priv, return 0; } +static void +virStorageBackendGlusterCacheInit(void) +{ + if (VIR_ALLOC(virStorageBackendGlusterGlobalCacheObj) < 0) { + virStorageBackendGlusterCacheInitGlobalError = false; + return; + } + + if (virMutexInit(&virStorageBackendGlusterGlobalCacheObj->lock) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Unable to initialize mutex")); + virStorageBackendGlusterCacheInitGlobalError = false; + } +} static int virStorageFileBackendGlusterInit(virStorageSourcePtr src) @@ -632,11 +840,32 @@ 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 (virStorageBackendGlusterCacheInitGlobalError) + return -1; + + virMutexLock(&virStorageBackendGlusterGlobalCacheObj->lock); + + priv->vol = virStorageBackendGlusterCacheQuery(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) < 0) + goto unlock; + + virMutexUnlock(&virStorageBackendGlusterGlobalCacheObj->lock); + for (i = 0; i < src->nhosts; i++) { if (virStorageFileBackendGlusterInitServer(priv, src->hosts + i) < 0) goto error; @@ -653,11 +882,11 @@ virStorageFileBackendGlusterInit(virStorageSourcePtr src) return 0; - error: - if (priv->vol) - glfs_fini(priv->vol); - VIR_FREE(priv); + unlock: + virMutexUnlock(&virStorageBackendGlusterGlobalCacheObj->lock); + error: + virStorageBackendGlusterCacheRefresh(src); return -1; } -- 2.7.4
On Tue, Dec 06, 2016 at 22:52:01 +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 part was extracted to a separate patch already, thus this doesn't have to deal with snapshots in any way than with other gluster connections.
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 | 279 +++++++++++++++++++++++++++++++--- 1 file changed, 254 insertions(+), 25 deletions(-)
diff --git a/src/storage/storage_backend_gluster.c b/src/storage/storage_backend_gluster.c index e0841ca..9f633ac 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,205 @@ 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;
You should also keep a reference to the object in the cache, so that you can return it and don't have to look it up again.
+ char *canonpath; +}; + +struct _virStorageBackendGlusterCacheConn { + virObjectLockable parent; + virStorageFileBackendGlusterPrivPtr priv;
You don't really need the complete object here. you only need the glfs_t part here. The 'cannonpath' variable is different for possible storage source objects.
+ 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 bool virStorageBackendGlusterCacheInitGlobalError; +static bool virStorageBackendGlusterCacheConnCleaned;
As said. Don't use global flags. You should be able to avoid both of them. Okay. I've seen below. They not only are avoidable, but your usage is outright dangerous.
+ + +static void virStorageBackendGlusterCacheConnDispose(void *obj); + + +static int virStorageBackendGlusterCacheConnOnceInit(void) +{ + if (!(virStorageBackendGlusterCacheConnClass = + virClassNew(virClassForObjectLockable(), + "virStorageBackendGlusterCacheConnClass", + sizeof(virStorageBackendGlusterCacheConn), + virStorageBackendGlusterCacheConnDispose)))
I've noticed that you actually create a lockable object here, but don't use the locks. This is currently okay, since it's write only. The lock might come handy though ...
+ return -1; + + return 0; +} + +static void virStorageBackendGlusterCacheConnDispose(void *object) +{ + virStorageBackendGlusterCacheConnPtr conn = object; + + glfs_fini(conn->priv->vol); + + VIR_FREE(conn->priv->canonpath); + VIR_FREE(conn->priv); + VIR_FREE(conn->volname); + + virStorageNetHostDefFree(conn->nhosts, conn->hosts); + + /* all set to delete the connection from cache */ + virStorageBackendGlusterCacheConnCleaned = true; +} + +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, + virStorageFileBackendGlusterPrivPtr priv) +{ + virStorageBackendGlusterCacheConnPtr newConn = NULL; + virStorageBackendGlusterCacheConnPtr conn = NULL; + size_t i; + + for (i = 0; i < virStorageBackendGlusterGlobalCacheObj->nConn; i++) {
Umm ... you iterate the cache ...
+ conn = virStorageBackendGlusterGlobalCacheObj->conn[i]; + if (STRNEQ(conn->volname, src->volume)) + continue; + if (virStorageBackendGlusterCompareHosts(conn->nhosts, conn->hosts, + src->nhosts, src->hosts)) { + return 0; + } + } + + if (virStorageBackendGlusterCacheConnInitialize() < 0) + return -1;
... before attempting to initialize the global cache?!? That doesn't make sense. Also this call is not needed here, since the only entrypoint is virStorageFileBackendGlusterInit which initializes it.
+ + if (!(newConn = virObjectLockableNew(virStorageBackendGlusterCacheConnClass))) + return -1; + + 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->priv = priv; + + if (VIR_APPEND_ELEMENT(virStorageBackendGlusterGlobalCacheObj->conn, + virStorageBackendGlusterGlobalCacheObj->nConn, + newConn) < 0) + goto error; + + return 0; + + error: + virStorageNetHostDefFree(newConn->nhosts, newConn->hosts); + VIR_FREE(newConn->volname);
You leak newConn in some cases.
+ return -1; +} + +static glfs_t * +virStorageBackendGlusterCacheQuery(virStorageSourcePtr src)
We usually use "Lookup".
+{ + 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)) { + virObjectRef(conn); + return conn->priv->vol; + } + } + + return NULL; +} + +static void +virStorageBackendGlusterCacheRefresh(virStorageSourcePtr src)
This name is weird. Did you mean "Release"?
+{ + virStorageFileBackendGlusterPrivPtr priv = src->drv->priv; + 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)) { + virObjectUnref(conn); + if (!virStorageBackendGlusterCacheConnCleaned) {
Umm, nope, that's not how it works. This is totaly thread unsafe. Other thread can modify your global flag. You probably did not check how virObjectUnref actually works. The return value notifes you if the object was disposed (you removed the last referernce) or not.
+ if (priv && priv->canonpath) + VIR_FREE(priv->canonpath); + goto unlock; + } + virStorageBackendGlusterCacheConnCleaned = false; + VIR_DELETE_ELEMENT(virStorageBackendGlusterGlobalCacheObj->conn, i, + virStorageBackendGlusterGlobalCacheObj->nConn); + VIR_FREE(src->drv);
This leaks src->drv in cases where the entry was used by two objects simultaneously. This needs to be done in the calling function. (As also pointed out in one of the other patches)
+ } + } + + unlock: + virMutexUnlock(&virStorageBackendGlusterGlobalCacheObj->lock); +}
static void virStorageBackendGlusterClose(virStorageBackendGlusterStatePtr state) @@ -538,32 +749,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); - - VIR_FREE(src->drv); + virStorageBackendGlusterCacheRefresh(src); }
static int @@ -610,6 +804,20 @@ virStorageFileBackendGlusterInitServer(virStorageFileBackendGlusterPrivPtr priv, return 0; }
+static void +virStorageBackendGlusterCacheInit(void) +{ + if (VIR_ALLOC(virStorageBackendGlusterGlobalCacheObj) < 0) { + virStorageBackendGlusterCacheInitGlobalError = false; + return; + } + + if (virMutexInit(&virStorageBackendGlusterGlobalCacheObj->lock) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Unable to initialize mutex")); + virStorageBackendGlusterCacheInitGlobalError = false; + } +}
static int virStorageFileBackendGlusterInit(virStorageSourcePtr src) @@ -632,11 +840,32 @@ 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 (virStorageBackendGlusterCacheInitGlobalError) + return -1; + + virMutexLock(&virStorageBackendGlusterGlobalCacheObj->lock); + + priv->vol = virStorageBackendGlusterCacheQuery(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) < 0) + goto unlock; + + virMutexUnlock(&virStorageBackendGlusterGlobalCacheObj->lock); +
So yet another thread-unsafe part. You correctly add the not-yet-opened connection to the cache, but there is no mechanism to guard suff after this. If a second thread obtains the cache entry wile the first one is not yet finished opening the connection it will get an invalid one. Any thread obtaining an entry from the cache needs to check whether the conection was initialized already as the first thing to do. It can continue only if it's already open and otherwise needs to wait until the original thread finishes. The first thread that added the object into the cache needs to open the connection and notify all other threads potentialy waiting on the condition that it was opened. A virCond should help.
for (i = 0; i < src->nhosts; i++) { if (virStorageFileBackendGlusterInitServer(priv, src->hosts + i) < 0) goto error;
On Wed, Dec 7, 2016 at 10:38 PM, Peter Krempa <pkrempa@redhat.com> wrote:
On Tue, Dec 06, 2016 at 22:52:01 +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 part was extracted to a separate patch already, thus this doesn't have to deal with snapshots in any way than with other gluster connections.
Yeah!
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 | 279 +++++++++++++++++++++++++++++++--- 1 file changed, 254 insertions(+), 25 deletions(-)
diff --git a/src/storage/storage_backend_gluster.c b/src/storage/storage_backend_gluster.c index e0841ca..9f633ac 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,205 @@ 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;
You should also keep a reference to the object in the cache, so that you can return it and don't have to look it up again.
+ char *canonpath; +}; + +struct _virStorageBackendGlusterCacheConn { + virObjectLockable parent; + virStorageFileBackendGlusterPrivPtr priv;
You don't really need the complete object here. you only need the glfs_t part here. The 'cannonpath' variable is different for possible storage source objects.
got it.
+ 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 bool virStorageBackendGlusterCacheInitGlobalError; +static bool virStorageBackendGlusterCacheConnCleaned;
As said. Don't use global flags. You should be able to avoid both of them.
Okay. I've seen below. They not only are avoidable, but your usage is outright dangerous.
Okay.
+ + +static void virStorageBackendGlusterCacheConnDispose(void *obj); + + +static int virStorageBackendGlusterCacheConnOnceInit(void) +{ + if (!(virStorageBackendGlusterCacheConnClass = + virClassNew(virClassForObjectLockable(), + "virStorageBackendGlusterCacheConnClass", + sizeof(virStorageBackendGlusterCacheConn), + virStorageBackendGlusterCacheConnDispose)))
I've noticed that you actually create a lockable object here, but don't use the locks. This is currently okay, since it's write only. The lock might come handy though ...
+ return -1; + + return 0; +} + +static void virStorageBackendGlusterCacheConnDispose(void *object) +{ + virStorageBackendGlusterCacheConnPtr conn = object; + + glfs_fini(conn->priv->vol); + + VIR_FREE(conn->priv->canonpath); + VIR_FREE(conn->priv); + VIR_FREE(conn->volname); + + virStorageNetHostDefFree(conn->nhosts, conn->hosts); + + /* all set to delete the connection from cache */ + virStorageBackendGlusterCacheConnCleaned = true; +} + +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, + virStorageFileBackendGlusterPrivPtr priv) +{ + virStorageBackendGlusterCacheConnPtr newConn = NULL; + virStorageBackendGlusterCacheConnPtr conn = NULL; + size_t i; + + for (i = 0; i < virStorageBackendGlusterGlobalCacheObj->nConn; i++) {
Umm ... you iterate the cache ...
This is the problem with changing the code in the last minute. I have edited something after I did git format-patch. Will address them.
+ conn = virStorageBackendGlusterGlobalCacheObj->conn[i]; + if (STRNEQ(conn->volname, src->volume)) + continue; + if (virStorageBackendGlusterCompareHosts(conn->nhosts, conn->hosts, + src->nhosts, src->hosts)) { + return 0; + } + } + + if (virStorageBackendGlusterCacheConnInitialize() < 0) + return -1;
... before attempting to initialize the global cache?!? That doesn't make sense. Also this call is not needed here, since the only entrypoint is virStorageFileBackendGlusterInit which initializes it.
+ + if (!(newConn = virObjectLockableNew(virStorageBackendGlusterCacheConnClass))) + return -1; + + 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->priv = priv; + + if (VIR_APPEND_ELEMENT(virStorageBackendGlusterGlobalCacheObj->conn, + virStorageBackendGlusterGlobalCacheObj->nConn, + newConn) < 0) + goto error; + + return 0; + + error: + virStorageNetHostDefFree(newConn->nhosts, newConn->hosts); + VIR_FREE(newConn->volname);
You leak newConn in some cases.
Unref!
+ return -1; +} + +static glfs_t * +virStorageBackendGlusterCacheQuery(virStorageSourcePtr src)
We usually use "Lookup".
good one. My naming skills are pathetic. Mercy me ...
+{ + 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)) { + virObjectRef(conn); + return conn->priv->vol; + } + } + + return NULL; +} + +static void +virStorageBackendGlusterCacheRefresh(virStorageSourcePtr src)
This name is weird. Did you mean "Release"?
As you say.
+{ + virStorageFileBackendGlusterPrivPtr priv = src->drv->priv; + 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)) { + virObjectUnref(conn); + if (!virStorageBackendGlusterCacheConnCleaned) {
Umm, nope, that's not how it works. This is totaly thread unsafe. Other thread can modify your global flag.
You probably did not check how virObjectUnref actually works. The return value notifes you if the object was disposed (you removed the last referernce) or not.
Exactly. Okay I realized virObjectUnref has ret value, which no one used it earlier or I missed them all. Me bad hacker used the same way. I will start using the ret value. The best part is, I did not notice your comment above and bugged the code, after many tries finally found virObjectUnref has ret, when I stared reading the email for reply I found that you already gave the clue above. Poor me!
+ if (priv && priv->canonpath) + VIR_FREE(priv->canonpath); + goto unlock; + } + virStorageBackendGlusterCacheConnCleaned = false; + VIR_DELETE_ELEMENT(virStorageBackendGlusterGlobalCacheObj->conn, i, + virStorageBackendGlusterGlobalCacheObj->nConn); + VIR_FREE(src->drv);
This leaks src->drv in cases where the entry was used by two objects simultaneously. This needs to be done in the calling function. (As also pointed out in one of the other patches)
right, Will make this part of virStorageFileDeinit.
+ } + } + + unlock: + virMutexUnlock(&virStorageBackendGlusterGlobalCacheObj->lock); +}
static void virStorageBackendGlusterClose(virStorageBackendGlusterStatePtr state) @@ -538,32 +749,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); - - VIR_FREE(src->drv); + virStorageBackendGlusterCacheRefresh(src); }
static int @@ -610,6 +804,20 @@ virStorageFileBackendGlusterInitServer(virStorageFileBackendGlusterPrivPtr priv, return 0; }
+static void +virStorageBackendGlusterCacheInit(void) +{ + if (VIR_ALLOC(virStorageBackendGlusterGlobalCacheObj) < 0) { + virStorageBackendGlusterCacheInitGlobalError = false; + return; + } + + if (virMutexInit(&virStorageBackendGlusterGlobalCacheObj->lock) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Unable to initialize mutex")); + virStorageBackendGlusterCacheInitGlobalError = false; + } +}
static int virStorageFileBackendGlusterInit(virStorageSourcePtr src) @@ -632,11 +840,32 @@ 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 (virStorageBackendGlusterCacheInitGlobalError) + return -1; + + virMutexLock(&virStorageBackendGlusterGlobalCacheObj->lock); + + priv->vol = virStorageBackendGlusterCacheQuery(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) < 0) + goto unlock; + + virMutexUnlock(&virStorageBackendGlusterGlobalCacheObj->lock); +
So yet another thread-unsafe part. You correctly add the not-yet-opened connection to the cache, but there is no mechanism to guard suff after this.
If a second thread obtains the cache entry wile the first one is not yet finished opening the connection it will get an invalid one.
Any thread obtaining an entry from the cache needs to check whether the conection was initialized already as the first thing to do. It can continue only if it's already open and otherwise needs to wait until the original thread finishes.
The first thread that added the object into the cache needs to open the connection and notify all other threads potentialy waiting on the condition that it was opened. A virCond should help.
Thanks for the clue, will need to figure out how to use virCond.
for (i = 0; i < src->nhosts; i++) { if (virStorageFileBackendGlusterInitServer(priv, src->hosts + i) < 0) goto error;
Apologies for the delay. Got a switch to internal priority item in gluster. Will try to work on the reviews tomorrow. Thanks Peter! -- Prasanna
On Mon, Dec 12, 2016 at 8:02 PM, Prasanna Kalever <pkalever@redhat.com> wrote:
On Wed, Dec 7, 2016 at 10:38 PM, Peter Krempa <pkrempa@redhat.com> wrote:
On Tue, Dec 06, 2016 at 22:52:01 +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 part was extracted to a separate patch already, thus this doesn't have to deal with snapshots in any way than with other gluster connections.
Yeah!
Sorry, this still holds good. on taking a snapshot virStorageFileGetMetadataRecurse will be called on each overlay. Which seeks/hits the cache help while the virStorageFileInitAs respective calls (In case if snaps reside on the same volume). Thanks, -- Prasanna
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 | 279 +++++++++++++++++++++++++++++++--- 1 file changed, 254 insertions(+), 25 deletions(-)
diff --git a/src/storage/storage_backend_gluster.c b/src/storage/storage_backend_gluster.c index e0841ca..9f633ac 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,205 @@ 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;
You should also keep a reference to the object in the cache, so that you can return it and don't have to look it up again.
+ char *canonpath; +}; + +struct _virStorageBackendGlusterCacheConn { + virObjectLockable parent; + virStorageFileBackendGlusterPrivPtr priv;
You don't really need the complete object here. you only need the glfs_t part here. The 'cannonpath' variable is different for possible storage source objects.
got it.
+ 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 bool virStorageBackendGlusterCacheInitGlobalError; +static bool virStorageBackendGlusterCacheConnCleaned;
As said. Don't use global flags. You should be able to avoid both of them.
Okay. I've seen below. They not only are avoidable, but your usage is outright dangerous.
Okay.
+ + +static void virStorageBackendGlusterCacheConnDispose(void *obj); + + +static int virStorageBackendGlusterCacheConnOnceInit(void) +{ + if (!(virStorageBackendGlusterCacheConnClass = + virClassNew(virClassForObjectLockable(), + "virStorageBackendGlusterCacheConnClass", + sizeof(virStorageBackendGlusterCacheConn), + virStorageBackendGlusterCacheConnDispose)))
I've noticed that you actually create a lockable object here, but don't use the locks. This is currently okay, since it's write only. The lock might come handy though ...
+ return -1; + + return 0; +} + +static void virStorageBackendGlusterCacheConnDispose(void *object) +{ + virStorageBackendGlusterCacheConnPtr conn = object; + + glfs_fini(conn->priv->vol); + + VIR_FREE(conn->priv->canonpath); + VIR_FREE(conn->priv); + VIR_FREE(conn->volname); + + virStorageNetHostDefFree(conn->nhosts, conn->hosts); + + /* all set to delete the connection from cache */ + virStorageBackendGlusterCacheConnCleaned = true; +} + +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, + virStorageFileBackendGlusterPrivPtr priv) +{ + virStorageBackendGlusterCacheConnPtr newConn = NULL; + virStorageBackendGlusterCacheConnPtr conn = NULL; + size_t i; + + for (i = 0; i < virStorageBackendGlusterGlobalCacheObj->nConn; i++) {
Umm ... you iterate the cache ...
This is the problem with changing the code in the last minute. I have edited something after I did git format-patch. Will address them.
+ conn = virStorageBackendGlusterGlobalCacheObj->conn[i]; + if (STRNEQ(conn->volname, src->volume)) + continue; + if (virStorageBackendGlusterCompareHosts(conn->nhosts, conn->hosts, + src->nhosts, src->hosts)) { + return 0; + } + } + + if (virStorageBackendGlusterCacheConnInitialize() < 0) + return -1;
... before attempting to initialize the global cache?!? That doesn't make sense. Also this call is not needed here, since the only entrypoint is virStorageFileBackendGlusterInit which initializes it.
+ + if (!(newConn = virObjectLockableNew(virStorageBackendGlusterCacheConnClass))) + return -1; + + 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->priv = priv; + + if (VIR_APPEND_ELEMENT(virStorageBackendGlusterGlobalCacheObj->conn, + virStorageBackendGlusterGlobalCacheObj->nConn, + newConn) < 0) + goto error; + + return 0; + + error: + virStorageNetHostDefFree(newConn->nhosts, newConn->hosts); + VIR_FREE(newConn->volname);
You leak newConn in some cases.
Unref!
+ return -1; +} + +static glfs_t * +virStorageBackendGlusterCacheQuery(virStorageSourcePtr src)
We usually use "Lookup".
good one. My naming skills are pathetic. Mercy me ...
+{ + 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)) { + virObjectRef(conn); + return conn->priv->vol; + } + } + + return NULL; +} + +static void +virStorageBackendGlusterCacheRefresh(virStorageSourcePtr src)
This name is weird. Did you mean "Release"?
As you say.
+{ + virStorageFileBackendGlusterPrivPtr priv = src->drv->priv; + 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)) { + virObjectUnref(conn); + if (!virStorageBackendGlusterCacheConnCleaned) {
Umm, nope, that's not how it works. This is totaly thread unsafe. Other thread can modify your global flag.
You probably did not check how virObjectUnref actually works. The return value notifes you if the object was disposed (you removed the last referernce) or not.
Exactly.
Okay I realized virObjectUnref has ret value, which no one used it earlier or I missed them all. Me bad hacker used the same way. I will start using the ret value.
The best part is, I did not notice your comment above and bugged the code, after many tries finally found virObjectUnref has ret, when I stared reading the email for reply I found that you already gave the clue above. Poor me!
+ if (priv && priv->canonpath) + VIR_FREE(priv->canonpath); + goto unlock; + } + virStorageBackendGlusterCacheConnCleaned = false; + VIR_DELETE_ELEMENT(virStorageBackendGlusterGlobalCacheObj->conn, i, + virStorageBackendGlusterGlobalCacheObj->nConn); + VIR_FREE(src->drv);
This leaks src->drv in cases where the entry was used by two objects simultaneously. This needs to be done in the calling function. (As also pointed out in one of the other patches)
right, Will make this part of virStorageFileDeinit.
+ } + } + + unlock: + virMutexUnlock(&virStorageBackendGlusterGlobalCacheObj->lock); +}
static void virStorageBackendGlusterClose(virStorageBackendGlusterStatePtr state) @@ -538,32 +749,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); - - VIR_FREE(src->drv); + virStorageBackendGlusterCacheRefresh(src); }
static int @@ -610,6 +804,20 @@ virStorageFileBackendGlusterInitServer(virStorageFileBackendGlusterPrivPtr priv, return 0; }
+static void +virStorageBackendGlusterCacheInit(void) +{ + if (VIR_ALLOC(virStorageBackendGlusterGlobalCacheObj) < 0) { + virStorageBackendGlusterCacheInitGlobalError = false; + return; + } + + if (virMutexInit(&virStorageBackendGlusterGlobalCacheObj->lock) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Unable to initialize mutex")); + virStorageBackendGlusterCacheInitGlobalError = false; + } +}
static int virStorageFileBackendGlusterInit(virStorageSourcePtr src) @@ -632,11 +840,32 @@ 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 (virStorageBackendGlusterCacheInitGlobalError) + return -1; + + virMutexLock(&virStorageBackendGlusterGlobalCacheObj->lock); + + priv->vol = virStorageBackendGlusterCacheQuery(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) < 0) + goto unlock; + + virMutexUnlock(&virStorageBackendGlusterGlobalCacheObj->lock); +
So yet another thread-unsafe part. You correctly add the not-yet-opened connection to the cache, but there is no mechanism to guard suff after this.
If a second thread obtains the cache entry wile the first one is not yet finished opening the connection it will get an invalid one.
Any thread obtaining an entry from the cache needs to check whether the conection was initialized already as the first thing to do. It can continue only if it's already open and otherwise needs to wait until the original thread finishes.
The first thread that added the object into the cache needs to open the connection and notify all other threads potentialy waiting on the condition that it was opened. A virCond should help.
Thanks for the clue, will need to figure out how to use virCond.
for (i = 0; i < src->nhosts; i++) { if (virStorageFileBackendGlusterInitServer(priv, src->hosts + i) < 0) goto error;
Apologies for the delay. Got a switch to internal priority item in gluster.
Will try to work on the reviews tomorrow.
Thanks Peter!
-- Prasanna
participants (3)
-
Peter Krempa -
Prasanna Kalever -
Prasanna Kumar Kalever