Re: [PATCH v4] libxl: implement virDomainBlockStats

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




On 07/25/2016 09:35 PM, Jim Fehlig wrote:
> 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.
OK.

> 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
>
Yep.

[snip]

>>>>  static int
>>>> +libxlDiskPathToID(const char *virtpath)
>>>> +{
>>>> +    static char const* drive_prefix[] = {"xvd", "hd", "sd"};
>>>> +    int disk, partition, chrused;
>>>> +    int fmt, id;
>>>> +    char *r;[snip
>>>> +    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
> 
/nods

> "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."
> 
My apologies, I was misinformed that the path could actually be a file and wasn't
handling that case in this patch. I've fixed that in v5 - and since I fetch anyways
the DiskDef by path I can just use disk->dst (aka disk/target/@dev). Note that I
don't validate disk->dst existence which AFAICT disk definition will always have this
dst field not NULL.

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

> 
>>
>>>> +    }
>>>> +    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 :-).
> 
Hehe - I also lean towards option 1, much more readable.

Just sent v5 here:

https://www.redhat.com/archives/libvir-list/2016-July/msg01029.html

Regards,
Joao

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



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]