Re: [Qemu-devel] [PATCH 2/3] qcow2: add option to clean unused cache entries after some time

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2971 bytes --]

On 05/27/2015 03:46 AM, Alberto Garcia wrote:
> This adds a new 'cache-clean-interval' option that cleans all qcow2
> cache entries that haven't been used in a certain interval, given in
> seconds.
> 
> This allows setting a large L2 cache size so it can handle scenarios
> with lots of I/O and at the same time use little memory during periods
> of inactivity.
> 
> This feature currently relies on MADV_DONTNEED to free that memory, so
> it is not useful in systems that don't follow that behavior.
> 
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> Cc: Max Reitz <mreitz@redhat.com>
> ---
>  block/qcow2-cache.c  | 35 ++++++++++++++++++++++++++++
>  block/qcow2.c        | 66 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  block/qcow2.h        |  4 ++++
>  qapi/block-core.json | 13 +++++++++--
>  4 files changed, 116 insertions(+), 2 deletions(-)
> 

> +static void cache_clean_timer_init(BlockDriverState *bs, AioContext *context)
> +{
> +    BDRVQcowState *s = bs->opaque;
> +    if (s->cache_clean_interval > 0) {
> +        s->cache_clean_timer = aio_timer_new(context, QEMU_CLOCK_VIRTUAL,
> +                                             SCALE_MS, cache_clean_timer_cb,
> +                                             bs);
> +        timer_mod(s->cache_clean_timer, qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) +
> +                  (int64_t) s->cache_clean_interval * 1000);
> +    }
> +}
> +

This function sets up a timer for non-zero interval, but does nothing if
interval is zero. [1]

> @@ -839,6 +888,16 @@ static int qcow2_open(BlockDriverState *bs, QDict *options, int flags,
>          goto fail;
>      }
>  
> +    cache_clean_interval =
> +        qemu_opt_get_number(opts, QCOW2_OPT_CACHE_CLEAN_INTERVAL, 0);
> +    if (cache_clean_interval > UINT_MAX) {
> +        error_setg(errp, "Cache clean interval too big");
> +        ret = -EINVAL;
> +        goto fail;
> +    }

If you type the qapi code as 'uint32' rather than 'int', you could skip
the error checking here because the parser would have already clamped
things.  But I can live with this as-is.

> +    s->cache_clean_interval = cache_clean_interval;
> +    cache_clean_timer_init(bs, bdrv_get_aio_context(bs));

[1] But here, you are unconditionally calling init, whether the new
value is 0 or nonzero.  Can a block reopen ever cause an existing BDS to
change its interval, in which case I could create a BDS originally with
a timer, then reopen it without a timer, and init() would have to remove
the existing timer?  If I'm reading this patch correctly, right now the
interval is a write-once deal (no way to change it after the fact), so
your code is okay; but should a separate patch be added to allow
adjusting the interval, via a reopen operation?

Reviewed-by: Eric Blake <eblake@redhat.com>

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]