[libvirt] [PATCH] qemu: Adjust qemuDomainGetBlockInfo data for sparse backed files

[Michal Privoznik]

On 13.12.2016 21:58, John Ferlan wrote:
> According to commit id '0282ca45a' the 'physical' value should
> essentially be the last offset of the image or the host physical
> size in bytes of the image container. However, commit id '15fa84ac'
> refactored the GetBlockInfo to use the same returned data as the
> GetStatsBlock API for an active domain. For the 'entry->physical'
> that would end up being the "actual-size" as set through the
> qemuMonitorJSONBlockStatsUpdateCapacityOne (commit '7b11f5e5').
> Digging deeper into QEMU code one finds that actual_size is
> filled in using the same algorithm as GetBlockInfo has used for
> setting the 'allocation' field when the domain is inactive.
> 
> The difference in values is seen primarily in sparse raw files
> and other container type files (such as qcow2), which will return
> a smaller value via the stat API for 'st_blocks'. Additionally
> for container files, the 'capacity' field (populated via the
> QEMU "virtual-size" value) may be slightly different (smaller)
> in order to accomodate the overhead for the container. For
> sparse files, the state 'st_size' field is returned.
> 
> This patch thus alters the allocation and physical values for
> sparse backed storage files to be more appropriate to the API
> contract. The result for GetBlockInfo is the following:
> 
>  capacity: logical size in bytes of the image (how much storage
>            the guest will see)
>  allocation: host storage in bytes occupied by the image (such
>              as highest allocated extent if there are no holes,
>              similar to 'du')
>  physical: host physical size in bytes of the image container
>            (last offset, similar to 'ls')
> 
> NB: The GetStatsBlock API allows a different contract for the
> values:
> 
>  "block.<num>.allocation" - offset of the highest written sector
>                             as unsigned long long.
>  "block.<num>.capacity" - logical size in bytes of the block device
>                           backing image as unsigned long long.
>  "block.<num>.physical" - physical size in bytes of the container
>                           of the backing image as unsigned long long.
> 
> Signed-off-by: John Ferlan <jferlan at redhat.com>
> ---
> 
>  Initially based on a question posed on libvirt-users:
> 
> https://www.redhat.com/archives/libvirt-users/2016-November/msg00050.html
> 
>  related to determining whether the provided physical value for an
>  inactive guest would be valid I found that virDomainGetBlockInfo was
>  returning different results for capacity and physical when the domain
>  was running. As I dug deeper I found Eric Blake's commit regarding how
>  libvirt uses/describes the allocation, capacity, and physical and felt
>  that the changes made to virDomainGetBlockInfo especially for sparse files
>  and "container" files (such as qcow2) would essentially provide incorrect
>  and perhaps unexpected results.  
> 
>  If it's felt that the live/active domain should return/display the same
>  as the virConnectGetAllDomainStats, then I can drop the patch. I guess I
>  find it odd that the physical size could be 4K matching the allocation
>  value for a sparse file. Likewise, for a fully preallocated file was
>  showing an allocation value of 0 since wr_highest_offset hadn't yet
>  been updated.
> 
>  I've been testing with a guest with different types of "file" and "block"
>  storage backings where the 'block' storage is essentially iSCSI devices
>  added to the guest. Logically the changes result in what I'd expect for
>  results based upon reading the API docs.
> 
>  src/qemu/qemu_driver.c | 22 +++++++++++++++++++---
>  1 file changed, 19 insertions(+), 3 deletions(-)
> 
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 88778d4..c537891 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -11808,13 +11808,29 @@ qemuDomainGetBlockInfo(virDomainPtr dom,
>              info->allocation = entry->wr_highest_offset;
>      }
>  
> -    if (entry->physical) {
> -        info->physical = entry->physical;
> -    } else {
> +    /* Unlike GetStatsBlock, this API has defined the expected return values
> +     * for allocation and physical slightly differently.
> +     *
> +     * Having a zero for either or if they're the same is an indication that
> +     * there's a sparse file backing this device. In this case, we'll force
> +     * the setting of physical based on the on disk file size.
> +     *
> +     * Additionally, if qemu hasn't written to the file yet, then set the
> +     * allocation to whatever qemu returned for physical (e.g. the "actual-
> +     * size" from the json query) as that will match the expected allocation
> +     * value for this API. */
> +    if (entry->physical == 0 || info->allocation == 0 ||
> +        info->allocation == entry->physical) {
> +        info->allocation = entry->physical;
> +        if (info->allocation == 0)
> +            info->allocation = entry->physical;
> +
>          if (qemuDomainStorageUpdatePhysical(driver, cfg, vm, disk->src) < 0)
>              goto endjob;
>  
>          info->physical = disk->src->physical;
> +    } else {
> +        info->physical = entry->physical;
>      }
>  
>      info->capacity = entry->capacity;
> 

ACK

Michal