From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:46770) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Yxz6O-0003A6-KT for qemu-devel@nongnu.org; Thu, 28 May 2015 10:48:09 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Yxz6N-0002dD-FM for qemu-devel@nongnu.org; Thu, 28 May 2015 10:48:08 -0400 Message-ID: <55672A9B.6010500@redhat.com> Date: Thu, 28 May 2015 16:47:55 +0200 From: Max Reitz MIME-Version: 1.0 References: <8007efe81120cd72f7c4145b8bbc3f4bc558e62d.1432719752.git.berto@igalia.com> <5565B837.3090300@redhat.com> In-Reply-To: <5565B837.3090300@redhat.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 2/3] qcow2: add option to clean unused cache entries after some time List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake , Alberto Garcia , qemu-devel@nongnu.org Cc: Kevin Wolf , qemu-block@nongnu.org, Stefan Hajnoczi 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 >> 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 >> Cc: Max Reitz >> --- >> 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. Max >> + 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 >