[Top][All Lists]

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

Re: [Qemu-devel] [PATCH 2/3] qcow2: add option to clean unused cache ent

From: Max Reitz
Subject: Re: [Qemu-devel] [PATCH 2/3] qcow2: add option to clean unused cache entries after some time
Date: Thu, 28 May 2015 16:47:55 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.7.0

On 27.05.2015 14:27, Eric Blake wrote:
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

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 <address@hidden>
Cc: Max Reitz <address@hidden>
  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.

Well, for blockdev-add, yes, but I don't think that applies when the option has been passed on the command line.


+    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 <address@hidden>

reply via email to

[Prev in Thread] Current Thread [Next in Thread]