Re: [Qemu-devel] [patch 1/5][v2] Extract code from get_cluster_offset()

[Kevin Wolf <kwolf@suse.de>]

Laurent Vivier schrieb:
> Extract code from get_cluster_offset() into new functions:
> 
> - seek_l2_table()
> 
> Search an l2 offset in the l2_cache table.
> 
> - l2_load()
> 
> Read the l2 entry from disk
> 
> - l2_allocate()
> 
> Allocate a new l2 entry.
> 
> Signed-off-by: Laurent Vivier <Laurent.Vivier@bull.net>

Acked-by: Kevin Wolf <kwolf@suse.de>


However, unrelated to your patch, I noticed that you touch code which
could use more comments in some places. (Oh btw, why do you explain the
new functions in the patch header but not in the code itself? But okay,
these are more or less obvious.)

> +static uint64_t *seek_l2_table(BDRVQcowState *s, uint64_t l2_offset)
> +{
> +    int i,j;
> +
> +    for(i = 0; i < L2_CACHE_SIZE; i++) {
> +        if (l2_offset == s->l2_cache_offsets[i]) {
> +            /* increment the hit count */
> +            if (++s->l2_cache_counts[i] == 0xffffffff) {
> +                for(j = 0; j < L2_CACHE_SIZE; j++) {
> +                    s->l2_cache_counts[j] >>= 1;
> +                }
> +            }

This if block is one of them. While it's quite obvious that some counter
is incremented, the action to avoid an overflow isn't. Why don't we do
something like if (count < 0xffffffff) count++?

And l2_cache_counts is a bad name anyway, IMHO. I first thought of
something like a reference counter (and then it would be definitely
worth a comment; now it is debatable). l2_cache_hits could be a better name.

> +        if (!(l2_offset & QCOW_OFLAG_COPIED) && allocate) {
> +            free_clusters(bs, l2_offset, s->l2_size * sizeof(uint64_t));
> +            ret = l2_allocate(bs, l1_index, &l2_table, &l2_offset);
> +            if (ret == 0)
> +                return 0;

The assumption is that allocate != 0 means there will be a write
operation and the cluster has to be copied, right? You certainly can
figure that out from the code, but it would be nice to have a comment
saying exactly this and that free_clusters is used to decrease the
refcount of the copied cluster.

Agreed, having written this, both are not really that bad. But it were
the places where I needed a bit more time to understand what the code is
doing and if it's right, so I thought I'd mention them.

Kevin