On 07/23/2016 05:34 AM, Joao Martins wrote:
> On 07/22/2016 10:50 PM, Jim Fehlig wrote:
>> On 07/20/2016 04:48 PM, Joao Martins wrote:
>>> Introduce initial support for domainBlockStats API call that
>>> allow us to query block device statistics. openstack nova
>>> uses this API call to query block statistics, alongside
>>> virDomainMemoryStats and virDomainInterfaceStats.  Note that
>>> this patch only introduces it for VBD for starters. QDisk
>>> would come in a separate patch series.
>>>
>>> A new statistics data structure is introduced to fit common
>>> statistics among others specific to the underlying block
>>> backends. For the VBD statistics on linux these are exported
>>> via sysfs on the path:
>>>
>>> "/sys/bus/xen-backend/devices/vbd-<domid>-<devid>/statistics"
>>>
>>> To calculate the block devno libxlDiskPathToID is introduced.
>> Too bad libxl or libxlutil doesn't provide such a function.
> Yeah :( Unfortunately the function is private to libxl.
>
> But we could get devno indirectly through
> libxl_device_disk_getinfo provided by libxl_diskinfo->devid field.
> But at the same time that function does much more than that i.e.
> 5 other xenstore reads being the reason why I didn't pursued this way
> initially. On the other hand while it's incurs more overhead for just
> calculating an ID, it's better towards unlikely adjustments on this
> area. I say unlikely because this hasn't changed for the past 6 years
> (Xen 4.0?), see xen commit e9703a9 ("libxl: Properly parse vbd name").
> What do you think?

I think copying the pertinent code to libxlDiskPathToID is the best approach in
this case. xend, whose functionality was replaced by the libvirt libxl driver,
had a similar function

http://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=tools/python/xen/util/blkif.py;h=4bbae308af51dee946a5cd498e93807e05ab9ef3;hb=refs/heads/stable-4.4#l13

>
>>> Each backend implements its own function to extract statistics
>>> and let there be handled the different platforms.
>>>
>>> VBD stats are exposed in reqs and number of sectors from
>>> blkback, and it's up to us to convert it to sector sizes.
>>> The sector size is gathered through xenstore in the device
>>> backend entry "physical-sector-size".
>>>
>>> BlockStatsFlags variant is also implemented which has the
>>> added benefit of getting the number of flush requests.
>>>
>>> Signed-off-by: Joao Martins <joao.m.mart...@oracle.com>
>>> ---
>>> Changes since v3:
>>>  - No need for error in libxlDiskSectorSize()
>>>  - Fix an identation issue.
>>>  - Rework cleanup paths on libxlDiskSectorSize()
>>>  - Do not unlock vm if libxlDomainObjEndJob() returns false and
>>>  adjust to the newly introduced virDomainObjEndAPI()
>>>  - Bump version to 2.1.0
>>>
>>> Changes since v1:
>>>  - Fix identation issues
>>>  - Set ret to LIBXL_VBD_SECTOR_SIZE
>>>  - Reuse VIR_STRDUP error instead of doing virReportError
>>>  when we fail to set stats->backend
>>>  - Change virAsprintf(...) error checking
>>>  - Change error to VIR_ERR_OPERATION_FAILED when xenstore path
>>>  does not exist and when failing to read stat.
>>>  - Resolve issues with 'make syntax-check' with cppi installed.
>>>  - Remove libxlDiskPathMatches in favor of using virutil
>>>  virDiskNameParse to fetch disk and partition index.
>>>  - Rename libxlDiskPathParse to libxlDiskPathToID and rework
>>>  function to just convert disk and partition index to devno.
>>>  - Bump version to 1.2.22
>>> ---
>>>  src/libxl/libxl_driver.c | 364 
>>> +++++++++++++++++++++++++++++++++++++++++++++++
>>>  1 file changed, 364 insertions(+)
>>>
>>> diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
>>> index f153f69..189bfb5 100644
>>> --- a/src/libxl/libxl_driver.c
>>> +++ b/src/libxl/libxl_driver.c
>>> @@ -30,6 +30,7 @@
>>>  #include <math.h>
>>>  #include <libxl.h>
>>>  #include <libxl_utils.h>
>>> +#include <xenstore.h>
>>>  #include <fcntl.h>
>>>  #include <regex.h>
>>>  
>>> @@ -72,6 +73,7 @@ VIR_LOG_INIT("libxl.libxl_driver");
>>>  #define LIBXL_DOM_REQ_HALT     4
>>>  
>>>  #define LIBXL_NB_TOTAL_CPU_STAT_PARAM 1
>>> +#define LIBXL_NB_TOTAL_BLK_STAT_PARAM 6
>>>  
>>>  #define HYPERVISOR_CAPABILITIES "/proc/xen/capabilities"
>>>  #define HYPERVISOR_XENSTORED "/dev/xen/xenstored"
>>> @@ -100,6 +102,25 @@ struct _libxlOSEventHookInfo {
>>>      int id;
>>>  };
>>>  
>>> +/* Object used to store disk statistics across multiple xen backends */
>>> +typedef struct _libxlBlockStats libxlBlockStats;
>>> +typedef libxlBlockStats *libxlBlockStatsPtr;
>>> +struct _libxlBlockStats {
>>> +    long long rd_req;
>>> +    long long rd_bytes;
>>> +    long long wr_req;
>>> +    long long wr_bytes;
>>> +    long long f_req;
>>> +
>>> +    char *backend;
>>> +    union {
>>> +        struct {
>>> +            long long ds_req;
>>> +            long long oo_req;
>>> +        } vbd;
>>> +    } u;
>>> +};
>>> +
>>>  /* Function declarations */
>>>  static int
>>>  libxlDomainManagedSaveLoad(virDomainObjPtr vm,
>>> @@ -5031,6 +5052,347 @@ libxlDomainGetJobStats(virDomainPtr dom,
>>>  }
>>>  
>>>  static int
>>> +libxlDiskPathToID(const char *virtpath)
>>> +{
>>> +    static char const* drive_prefix[] = {"xvd", "hd", "sd"};
>>> +    int disk, partition, chrused;
>>> +    int fmt, id;
>>> +    char *r;
>>> +    size_t i;
>>> +
>>> +    fmt = id = -1;
>>> +
>>> +    /* Find any disk prefixes we know about */
>>> +    for (i = 0; i < ARRAY_CARDINALITY(drive_prefix); i++) {
>>> +        if (STRPREFIX(virtpath, drive_prefix[i]) &&
>>> +            !virDiskNameParse(virtpath, &disk, &partition)) {
>>> +            fmt = i;
>>> +            break;
>>> +        }
>>> +    }
>>> +
>>> +    /* Handle it same way as xvd */
>>> +    if (fmt < 0 &&
>>> +        (sscanf(virtpath, "d%ip%i%n", &disk, &partition, &chrused) >= 2
>>> +         && chrused == strlen(virtpath)))
>>> +        fmt = 0;
>>> +
>>> +    /* Test indexes ranges and calculate the device id */
>>> +    switch (fmt) {
>>> +    case 0: /* xvd */
>>> +        if (disk <= 15 && partition <= 15)
>>> +            id = (202 << 8) | (disk << 4) | partition;
>>> +        else if ((disk <= ((1<<20)-1)) || partition <= 255)
>>> +            id = (1 << 28) | (disk << 8) | partition;
>>> +        break;
>>> +    case 1: /* hd */
>>> +        if (disk <= 3 && partition <= 63)
>>> +            id = ((disk < 2 ? 3 : 22) << 8) | ((disk & 1) << 6) | 
>>> partition;
>>> +        break;
>>> +    case 2: /* sd */
>>> +        if (disk <= 15 && (partition <= 15))
>>> +            id = (8 << 8) | (disk << 4) | partition;
>>> +        break;
>>> +    default:
>>> +        errno = virStrToLong_i(virtpath, &r, 0, &id);
>>> +        if (errno || *r || id > INT_MAX)
>>> +            id = -1;
>>> +        break;
>> The purpose of the default case is not clear to me. Can you explain that a 
>> bit? 
>> IIUC, virtpath would contain values such as xvda, sdc, or /path/to/image.
> virtpath would only contain device names (no image paths). This falltrough 
> case is to
> have device number directly from unsigned long instead of parsing device 
> name. See
> (xen) libxl commit 8ca0ed2cd ("libxl: fail to parse disk vpath if a disk+part 
> number
> needed but unavailable") as docs/misc/vbd-interface.txt isn't exactly clear 
> on this.
> Regardless probably doesn't make a lot of sense for libvirt since we can only 
> have
> device names. I will probably just set "id = -1" or depending on your 
> preference rely
> on libxl_device_disk_getinfo(..) returned devid.

Yeah, I don't think it makes much sense for libvirt. virtpath is the 'disk'
parameter in virDomainBlockStats*, which is documented as

"The @disk parameter is either the device target shorthand (the <target
dev='...'/> sub-element, such as "vda"), or (since 0.9.8) an unambiguous source
name of the block device (the <source file='...'/> sub-element, such as
"/path/to/image"). Valid names can be found by calling virDomainGetXMLDesc() and
inspecting elements within //domain/devices/disk. Some drivers might also accept
the empty string for the @disk parameter, and then yield summary stats for the
entire domain."

Setting 'id = -1' is the right thing to do IMO.

>
>>> +    }
>>> +    return id;
>>> +}
>>> +
>>> +#define LIBXL_VBD_SECTOR_SIZE 512
>>> +
>>> +static int
>>> +libxlDiskSectorSize(int domid, int devno)
>>> +{
>>> +    char *path, *val;
>>> +    struct xs_handle *handle;
>>> +    int ret = LIBXL_VBD_SECTOR_SIZE;
>>> +    unsigned int len;
>>> +
>>> +    handle = xs_daemon_open_readonly();
>>> +    if (!handle) {
>>> +        VIR_WARN("cannot read sector size");
>>> +        return ret;
>>> +    }
>>> +
>>> +    path = val = NULL;
>>> +    if (virAsprintf(&path, "/local/domain/%d/device/vbd/%d/backend",
>>> +                    domid, devno) < 0)
>>> +        goto cleanup;
>>> +
>>> +    if ((val = xs_read(handle, XBT_NULL, path, &len)) == NULL)
>>> +        goto cleanup;
>>> +
>>> +    VIR_FREE(path);
>>> +    if (virAsprintf(&path, "%s/physical-sector-size", val) < 0)
>>> +        goto cleanup;
>>> +
>>> +    VIR_FREE(val);
>>> +    if ((val = xs_read(handle, XBT_NULL, path, &len)) == NULL)
>>> +        goto cleanup;
>>> +
>>> +    if (sscanf(val, "%d", &ret) != 1)
>>> +        ret = LIBXL_VBD_SECTOR_SIZE;
>>> +
>>> + cleanup:
>>> +    VIR_FREE(val);
>>> +    VIR_FREE(path);
>>> +    xs_daemon_close(handle);
>>> +    return ret;
>>> +}
>>> +
>>> +static int
>>> +libxlDomainBlockStatsVBD(virDomainObjPtr vm,
>>> +                         const char *dev,
>>> +                         libxlBlockStatsPtr stats)
>> The stats parameter is unused when __linux__ is not defined. Otherwise, the
>> patch looks fine.
> Ah yes, and size will get declared but not used too.
> Would you prefer resorting to this syntax instead:
>
> #ifdef __linux__
> static int
> libxlDomainBlockStatsVBD(virDomainObjPtr vm,
>                          const char *dev,
>                          libxlBlockStatsPtr stats)
> {
>    ...
> }
> #else
> static int
> libxlDomainBlockStatsVBD(virDomainObjPtr vm,
>                          const char *dev ATTRIBUTE_UNUSED,
>                          libxlBlockStatsPtr stats ATTRIBUTE_UNUSED)
> {
>     virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
>                    "%s", _("platform unsupported"));
>     return -1;
> }
> #endif
>
> Or in alternative moving __linux__ conditional above devno declaration and 
> always set
> ATTRIBUTE_UNUSED on dev and stats params?

grepping through the sources shows both patterns are used. I find the first one
more readable and future-proof. And having ATTRIBUTE_UNUSED on a used param
feels odd to me :-).

Regards,
Jim

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Reply via email to