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

[Alberto Garcia]

On Wed 27 May 2015 02:27:35 PM CEST, Eric Blake <address@hidden> wrote:

>> +static void cache_clean_timer_init(BlockDriverState *bs, AioContext 
>> *context)
>
> This function sets up a timer for non-zero interval, but does nothing
> if interval is zero. [1]

Right, zero means there's no interval. I could clarify that in the
documentation (I did it in the ImageInfoSpecificQCow2 part, though).

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

Hmmm... the UINT_MAX limit is just a way to prevent an overflow because
of an abnormal value, not because the limit itself make sense (it's ~136
years, way beyond any reasonable value for that setting).

I'm not sure if it's a good idea to expose that in the API, it gives the
idea that there's a reason why it is a 32-bit integer, but there's none.

I would actually be ok with having a smaller limit (I don't know, 1
year?), but there's also no easy way to choose one I guess, so I decided
to let the user choose as long as it doesn't break anything.

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

Instead of having the same check in all places where we call
cache_clean_timer_init() (there's two at the moment) I think it's enough
with checking the value in the function where it is actually used (which
is anyway a good idea).

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

I have no objections against changing the interval, I just didn't
consider that case.

If we want to support it it should be as simple as

 timer_del();   s->interval = new_interval;   timer_init();

Berto