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

[Max Reitz <mreitz@redhat.com>]

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 <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.

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 <eblake@redhat.com>
>