Re: [Qemu-devel] [PATCH] block: Give always priority to unused entries in the qcow2 L2 cache

[Max Reitz <mreitz@redhat.com>]

On 2015-02-05 at 07:55, Alberto Garcia wrote:
> The current algorithm to replace entries from the L2 cache gives
> priority to newer hits by dividing the hit count of all existing
> entries by two everytime there is a cache miss.
>
> However, if there are several cache misses the hit count of the
> existing entries can easily go down to 0. This will result in those
> entries being replaced even when there are others that have never been
> used.
>
> This problem is more noticeable with larger disk images and cache
> sizes, since the chances of having several misses before the cache is
> full are higher.
>
> If we make sure that the hit count can never go down to 0 again,
> unused entries will always have priority.
>
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> ---
>   block/qcow2-cache.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/block/qcow2-cache.c b/block/qcow2-cache.c
> index 904f6b1..b115549 100644
> --- a/block/qcow2-cache.c
> +++ b/block/qcow2-cache.c
> @@ -253,7 +253,9 @@ static int qcow2_cache_find_entry_to_replace(Qcow2Cache *c)
>   
>           /* Give newer hits priority */
>           /* TODO Check how to optimize the replacement strategy */
> -        c->entries[i].cache_hits /= 2;
> +        if (c->entries[i].cache_hits > 1) {
> +            c->entries[i].cache_hits /= 2;
> +        }
>       }
>   
>       if (min_index == -1) {

Hm, I can't see where the code is actually giving priority to unused 
entries. qcow2_cache_find_entry_to_replace() is the only place which 
selects the entry to be used, and it does not check entries[i].offset 
which it should in order to determine whether there are unused entries.

Furthermore, I think we should prioritize clean entries over dirty ones; 
but I guess that's what the TODO comment is hinting at...

Max