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 <be...@igalia.com> > Cc: Max Reitz <mre...@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 <ebl...@redhat.com> -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature