[libvirt] [PATCH] parallels: add block device statistics to driver
Dmitry Guryanov
dguryanov at parallels.com
Wed Jun 3 12:16:09 UTC 2015
On 05/22/2015 10:42 AM, Nikolay Shirokovskiy wrote:
> Statistics provided through PCS SDK. As we have only async interface in SDK we
> need to be subscribed to statistics in order to get it. Trivial solution on
> every stat request to subscribe, wait event and then unsubscribe will lead to
> significant delays in case of a number of successive requests, as the event
> will be delivered on next PCS server notify cycle. On the other hand we don't
> want to keep unnesessary subscribtion. So we take an hibrid solution to
> subcsribe on first request and then keep a subscription while requests are
> active. We populate cache of statistics on subscribtion events and use this
> cache to serve libvirts requests.
>
> Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy at parallels.com>
> ---
> src/parallels/parallels_driver.c | 106 +++++++++++++++++++++
> src/parallels/parallels_sdk.c | 193 ++++++++++++++++++++++++++++++++------
> src/parallels/parallels_sdk.h | 2 +
> src/parallels/parallels_utils.h | 15 +++
> 4 files changed, 285 insertions(+), 31 deletions(-)
>
> diff --git a/src/parallels/parallels_driver.c b/src/parallels/parallels_driver.c
> index 4b87213..ce59e00 100644
> --- a/src/parallels/parallels_driver.c
> +++ b/src/parallels/parallels_driver.c
> @@ -51,6 +51,7 @@
> #include "nodeinfo.h"
> #include "virstring.h"
> #include "cpu/cpu.h"
> +#include "virtypedparam.h"
>
> #include "parallels_driver.h"
> #include "parallels_utils.h"
> @@ -1179,6 +1180,109 @@ parallelsDomainGetMaxMemory(virDomainPtr domain)
> return ret;
> }
>
> +static int
> +parallelsDomainBlockStats(virDomainPtr domain, const char *path,
> + virDomainBlockStatsPtr stats)
> +{
> + virDomainObjPtr dom = NULL;
> + int ret = -1;
> + size_t i;
> + int idx;
> +
> + if (!(dom = parallelsDomObjFromDomain(domain)))
> + return -1;
> +
> + if (*path) {
> + if ((idx = virDomainDiskIndexByName(dom->def, path, false)) < 0) {
> + virReportError(VIR_ERR_INVALID_ARG, _("invalid path: %s"), path);
> + goto cleanup;
> + }
> + if (prlsdkGetBlockStats(dom, dom->def->disks[idx], stats) < 0)
> + goto cleanup;
> + } else {
> + virDomainBlockStatsStruct s;
> +
> +#define PARALLELS_ZERO_STATS(VAR, TYPE, NAME) \
> + stats->VAR = 0;
> +
> + PARALLELS_BLOCK_STATS_FOREACH(PARALLELS_ZERO_STATS)
> +
> +#undef PARALLELS_ZERO_STATS
> +
Why don't you use memset here?
> + for (i = 0; i < dom->def->ndisks; i++) {
> + if (prlsdkGetBlockStats(dom, dom->def->disks[i], &s) < 0)
> + goto cleanup;
> +
> +#define PARALLELS_SUM_STATS(VAR, TYPE, NAME) \
> + if (s.VAR != -1) \
> + stats->VAR += s.VAR;
> +
> + PARALLELS_BLOCK_STATS_FOREACH(PARALLELS_SUM_STATS)
> +
> +#undef PARALLELS_SUM_STATS
> + }
> + }
> + stats->errs = -1;
> + ret = 0;
> +
> + cleanup:
> + if (dom)
> + virObjectUnlock(dom);
> +
> + return ret;
> +}
> +
> +static int
> +parallelsDomainBlockStatsFlags(virDomainPtr domain,
> + const char *path,
> + virTypedParameterPtr params,
> + int *nparams,
> + unsigned int flags)
> +{
> + virDomainBlockStatsStruct stats;
> + int ret = -1;
> + size_t i;
> +
> + virCheckFlags(VIR_TYPED_PARAM_STRING_OKAY, -1);
> + /* We don't return strings, and thus trivially support this flag. */
> + flags &= ~VIR_TYPED_PARAM_STRING_OKAY;
> +
> + if (parallelsDomainBlockStats(domain, path, &stats) < 0)
> + goto cleanup;
> +
> + if (*nparams == 0) {
> +#define PARALLELS_COUNT_STATS(VAR, TYPE, NAME) \
> + if ((stats.VAR) != -1) \
> + ++*nparams;
> +
> + PARALLELS_BLOCK_STATS_FOREACH(PARALLELS_COUNT_STATS)
> +
> +#undef PARALLELS_COUNT_STATS
> + ret = 0;
> + goto cleanup;
> + }
> +
> + i = 0;
> +#define PARALLELS_BLOCK_STATS_ASSIGN_PARAM(VAR, TYPE, NAME) \
> + if (i < *nparams && (stats.VAR) != -1) { \
> + if (virTypedParameterAssign(params + i, TYPE, \
> + VIR_TYPED_PARAM_LLONG, (stats.VAR)) < 0) \
> + goto cleanup; \
> + i++; \
> + }
> +
> + PARALLELS_BLOCK_STATS_FOREACH(PARALLELS_BLOCK_STATS_ASSIGN_PARAM)
> +
> +#undef PARALLELS_BLOCK_STATS_ASSIGN_PARAM
> +
> + *nparams = i;
> + ret = 0;
> +
> + cleanup:
> + return ret;
> +}
> +
> +
> static virHypervisorDriver parallelsDriver = {
> .name = "Parallels",
> .connectOpen = parallelsConnectOpen, /* 0.10.0 */
> @@ -1228,6 +1332,8 @@ static virHypervisorDriver parallelsDriver = {
> .domainManagedSave = parallelsDomainManagedSave, /* 1.2.14 */
> .domainManagedSaveRemove = parallelsDomainManagedSaveRemove, /* 1.2.14 */
> .domainGetMaxMemory = parallelsDomainGetMaxMemory, /* 1.2.15 */
> + .domainBlockStats = parallelsDomainBlockStats, /* 1.2.16 */
> + .domainBlockStatsFlags = parallelsDomainBlockStatsFlags, /* 1.2.16 */
Could you, please, update there versions to 1.2.17?
> };
>
> static virConnectDriver parallelsConnectDriver = {
> diff --git a/src/parallels/parallels_sdk.c b/src/parallels/parallels_sdk.c
> index 88ad59b..eb8d965 100644
> --- a/src/parallels/parallels_sdk.c
> +++ b/src/parallels/parallels_sdk.c
> @@ -21,6 +21,7 @@
> */
....
> + goto cleanup;
> +
> switch (prlEventType) {
> case PET_DSP_EVT_VM_STATE_CHANGED:
> + prlsdkHandleVmStateEvent(privconn, prlEvent, uuid);
> + break;
> case PET_DSP_EVT_VM_CONFIG_CHANGED:
> + prlsdkHandleVmConfigEvent(privconn, uuid);
> + break;
> case PET_DSP_EVT_VM_CREATED:
> case PET_DSP_EVT_VM_ADDED:
> + prlsdkHandleVmAddedEvent(privconn, uuid);
> + break;
> case PET_DSP_EVT_VM_DELETED:
> case PET_DSP_EVT_VM_UNREGISTERED:
> - pret = prlsdkHandleVmEvent(privconn, prlEvent);
> + prlsdkHandleVmRemovedEvent(privconn, uuid);
> + break;
> + case PET_DSP_EVT_VM_PERFSTATS:
> + prlsdkHandlePerfEvent(privconn, prlEvent, uuid);
> + // above function takes own of event
> + prlEvent = PRL_INVALID_HANDLE;
> break;
> default:
> VIR_DEBUG("Skipping event of type %d", prlEventType);
> @@ -3455,3 +3481,108 @@ prlsdkDomainManagedSaveRemove(virDomainObjPtr dom)
>
> return 0;
> }
> +
Could you describe the idea with stats cache in more detail? Why do you
keer up to 3 prlsdk stats, but use only one? And where do you free these
handles?
> +static int
> +prlsdkExtractStatsParam(PRL_HANDLE sdkstats, const char *name, long long *val)
> +{
> + PRL_HANDLE param = PRL_INVALID_HANDLE;
> + PRL_RESULT pret;
> + PRL_INT64 pval = 0;
> + int ret = -1;
> +
> + pret = PrlEvent_GetParamByName(sdkstats, name, ¶m);
> + if (pret == PRL_ERR_NO_DATA) {
> + *val = -1;
> + ret = 0;
> + goto cleanup;
> + } else if (PRL_FAILED(pret)) {
> + logPrlError(pret);
> + goto cleanup;
> + }
> + pret = PrlEvtPrm_ToInt64(param, &pval);
> + prlsdkCheckRetGoto(pret, cleanup);
> +
> + *val = pval;
> + ret = 0;
> +
> + cleanup:
> + PrlHandle_Free(param);
> + return ret;
> +}
> +
> +static int
> +prlsdkGetStatsParam(virDomainObjPtr dom, const char *name, long long *val)
> +{
> + parallelsDomObjPtr privdom = dom->privateData;
> + PRL_HANDLE job = PRL_INVALID_HANDLE;
> +
> + if (privdom->cache.stats != PRL_INVALID_HANDLE) {
> + privdom->cache.count = 0;
> + return prlsdkExtractStatsParam(privdom->cache.stats, name, val);
> + }
> +
> + job = PrlVm_SubscribeToPerfStats(privdom->sdkdom, NULL);
> + if (PRL_FAILED(waitJob(job)))
> + goto error;
> +
> + while (privdom->cache.stats == PRL_INVALID_HANDLE) {
> + if (virCondWait(&privdom->cache.cond, &dom->parent.lock) < 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("Unable to wait on monitor condition"));
> + goto error;
> + }
> + }
> +
> + return prlsdkExtractStatsParam(privdom->cache.stats, name, val);
> + error:
> + return -1;
> +}
> +
> +int
> +prlsdkGetBlockStats(virDomainObjPtr dom, virDomainDiskDefPtr disk, virDomainBlockStatsPtr stats)
> +{
> + virDomainDeviceDriveAddressPtr address;
> + int idx;
> + const char *prefix;
> + int ret = -1;
> + char *name = NULL;
> +
> + address = &disk->info.addr.drive;
> + switch (disk->bus) {
> + case VIR_DOMAIN_DISK_BUS_IDE:
> + prefix = "ide";
> + idx = address->bus * 2 + address->unit;
> + break;
> + case VIR_DOMAIN_DISK_BUS_SATA:
> + prefix = "sata";
> + idx = address->unit;
> + break;
> + case VIR_DOMAIN_DISK_BUS_SCSI:
> + prefix = "scsi";
> + idx = address->unit;
> + break;
> + default:
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Unknown disk bus: %X"), disk->bus);
> + goto cleanup;
> + }
> +
I think this won't work if the domain has disks on different buses.
> +
> +#define PRLSDK_GET_STAT_PARAM(VAL, TYPE, NAME) \
> + if (virAsprintf(&name, "devices.%s%d.%s", prefix, idx, NAME) < 0) \
> + goto cleanup; \
> + if (prlsdkGetStatsParam(dom, name, &stats->VAL) < 0) \
> + goto cleanup; \
> + VIR_FREE(name);
> +
> + PARALLELS_BLOCK_STATS_FOREACH(PRLSDK_GET_STAT_PARAM)
> +
> +#undef PRLSDK_GET_STAT_PARAM
> +
> + ret = 0;
> +
> + cleanup:
> +
> + VIR_FREE(name);
> + return ret;
> +}
> diff --git a/src/parallels/parallels_sdk.h b/src/parallels/parallels_sdk.h
> index 3f17fc8..afa6745 100644
> --- a/src/parallels/parallels_sdk.h
> +++ b/src/parallels/parallels_sdk.h
> @@ -64,3 +64,5 @@ int
> prlsdkAttachVolume(virDomainObjPtr dom, virDomainDiskDefPtr disk);
> int
> prlsdkDetachVolume(virDomainObjPtr dom, virDomainDiskDefPtr disk);
> +int
> +prlsdkGetBlockStats(virDomainObjPtr dom, virDomainDiskDefPtr disk, virDomainBlockStatsPtr stats);
> diff --git a/src/parallels/parallels_utils.h b/src/parallels/parallels_utils.h
> index 2d1d405..e424405 100644
> --- a/src/parallels/parallels_utils.h
> +++ b/src/parallels/parallels_utils.h
> @@ -73,11 +73,20 @@ struct _parallelsConn {
> typedef struct _parallelsConn parallelsConn;
> typedef struct _parallelsConn *parallelsConnPtr;
>
> +struct _parallelsContersCache {
> + PRL_HANDLE stats;
> + virCond cond;
> + int count;
> +};
> +
> +typedef struct _parallelsContersCache parallelsContersCache;
> +
> struct parallelsDomObj {
> int id;
> char *uuid;
> char *home;
> PRL_HANDLE sdkdom;
> + parallelsContersCache cache;
> };
>
> typedef struct parallelsDomObj *parallelsDomObjPtr;
> @@ -106,4 +115,10 @@ virStorageVolPtr parallelsStorageVolLookupByPathLocked(virConnectPtr conn,
> int parallelsStorageVolDefRemove(virStoragePoolObjPtr privpool,
> virStorageVolDefPtr privvol);
>
> +#define PARALLELS_BLOCK_STATS_FOREACH(OP) \
> + OP(rd_req, VIR_DOMAIN_BLOCK_STATS_READ_REQ, "read_requests") \
> + OP(rd_bytes, VIR_DOMAIN_BLOCK_STATS_READ_BYTES, "read_total") \
> + OP(wr_req, VIR_DOMAIN_BLOCK_STATS_WRITE_REQ, "write_requests") \
> + OP(wr_bytes, VIR_DOMAIN_BLOCK_STATS_WRITE_BYTES, "write_total")
> +
> #endif
More information about the libvir-list
mailing list