Re: [Qemu-devel] [PATCH v4] block: add event when disk usage exceeds threshold

[Eric Blake]

On 12/04/2014 01:59 AM, Francesco Romani wrote:
> Managing applications, like oVirt (http://www.ovirt.org), make extensive
> use of thin-provisioned disk images.
> To let the guest run smoothly and be not unnecessarily paused, oVirt sets
> a disk usage threshold (so called 'high water mark') based on the occupation
> of the device,  and automatically extends the image once the threshold
> is reached or exceeded.
> 
> In order to detect the crossing of the threshold, oVirt has no choice but
> aggressively polling the QEMU monitor using the query-blockstats command.
> This lead to unnecessary system load, and is made even worse under scale:
> deployments with hundreds of VMs are no longer rare.
> 
> To fix this, this patch adds:
> * A new monitor command to set a write threshold for a given block device.
> * A new event to report if a block device usage exceeds the threshold.

Please also mention the names of those two things in the commit message,
to make it easier to find them when doing 'git log' archaeology.

> 
> This will allow the managing application to use smarter and more
> efficient monitoring, greatly reducing the need of polling.
> 
> A followup patch is planned to allow to add the write threshold at
> device creation.
> 
> Signed-off-by: Francesco Romani <address@hidden>
> ---

> --- /dev/null
> +++ b/block/write-threshold.c
> @@ -0,0 +1,125 @@
> +/*
> + * QEMU System Emulator block write threshold notification
> + *
> + * Copyright Red Hat, Inc. 2014

I've been so slow on the review that you may want to add 2015.


> +bool bdrv_write_threshold_is_set(const BlockDriverState *bs)
> +{
> +    return !!(bs->write_threshold_offset > 0);

The !! is spurious; use of > already guarantees a bool result.

> +++ b/qapi/block-core.json
> @@ -239,6 +239,9 @@
>  #
>  # @iops_size: #optional an I/O size in bytes (Since 1.7)
>  #
> +# @write_threshold: configured write threshold for the device.
> +#                   0 if disabled. (Since 2.3)
> +#
>  # Since: 0.14.0
>  #
>  ##
> @@ -253,7 +256,7 @@
>              '*bps_max': 'int', '*bps_rd_max': 'int',
>              '*bps_wr_max': 'int', '*iops_max': 'int',
>              '*iops_rd_max': 'int', '*iops_wr_max': 'int',
> -            '*iops_size': 'int' } }
> +            '*iops_size': 'int', 'write_threshold': 'uint64' } }

'int' works as well as 'uint64'; since this is an output parameter, we
aren't gaining any stricter input parsing by using a more-specific type.

My findings are minor, so I'm okay if you post a v5 that addresses them
and includes:
Reviewed-by: Eric Blake <address@hidden>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature