[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, &param);
> +    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