[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: |
Alberto Garcia |
Subject: |
Re: [Qemu-devel] [PATCH 2/3] qcow2: add option to clean unused cache entries after some time |
Date: |
Wed, 27 May 2015 16:34:57 +0200 |
User-agent: |
Notmuch/0.13.2 (http://notmuchmail.org) Emacs/23.2.1 (i486-pc-linux-gnu) |
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
- [Qemu-devel] [PATCH v3 0/3] Clean unused entries in the qcow2 L2/refcount cache, Alberto Garcia, 2015/05/27
- [Qemu-devel] [PATCH 3/3] qcow2: reorder fields in Qcow2CachedTable to reduce padding, Alberto Garcia, 2015/05/27
- [Qemu-devel] [PATCH 1/3] qcow2: mark the memory as no longer needed after qcow2_cache_empty(), Alberto Garcia, 2015/05/27
- [Qemu-devel] [PATCH 2/3] qcow2: add option to clean unused cache entries after some time, Alberto Garcia, 2015/05/27
- Re: [Qemu-devel] [PATCH 2/3] qcow2: add option to clean unused cache entries after some time, Eric Blake, 2015/05/28
- Re: [Qemu-devel] [PATCH 2/3] qcow2: add option to clean unused cache entries after some time, Max Reitz, 2015/05/28
- Re: [Qemu-devel] [PATCH 2/3] qcow2: add option to clean unused cache entries after some time, Alberto Garcia, 2015/05/28
- Re: [Qemu-devel] [PATCH 2/3] qcow2: add option to clean unused cache entries after some time, Max Reitz, 2015/05/28
- Re: [Qemu-devel] [PATCH 2/3] qcow2: add option to clean unused cache entries after some time, Alberto Garcia, 2015/05/28