Re: [Qemu-devel] [PATCH v2 31/32] qcow2: Allow configuring the L2 slice size

[Alberto Garcia]

On Tue 16 Jan 2018 05:57:22 PM CET, Anton Nefedov wrote:
>> -    r->l2_slice_size = s->cluster_size / sizeof(uint64_t);
>> -    r->l2_table_cache = qcow2_cache_create(bs, l2_cache_size);
>> -    r->refcount_block_cache = qcow2_cache_create(bs, refcount_cache_size);
>> +    l2_cache_size *= s->cluster_size / l2_cache_entry_size;
>
> A bit confusing; l2_cache_size first means the size in bytes, then the
> size in clusters and now the size in entries.

You're right, I'll convert it to entries right after the
read_cache_sizes() call, there's no use for the size in clusters
anymore.

> Maybe in the comparison with MIN_L2_CACHE_SIZE, we should multiply
> MIN_L2_CACHE_SIZE to cluster_size instead.
> And perhaps MIN_L2_CACHE_CLUSTERS is a better name. Or should it even
> be MIN_L2_CACHE_ENTRIES instead, taking into account its motivation to
> make it possible to handle COW.

I think I'll keep the MIN_L2_CACHE_SIZE name but clarify in the
documentation that it means cache entries.

One consequence of this is that the L2 cache can be made smaller now,
and there's nothing wrong with that (it actually allows us to use less
memory in images with large clusters).

>> +    r->l2_slice_size = l2_cache_entry_size / sizeof(uint64_t);
>> +    r->l2_table_cache = qcow2_cache_create(bs, l2_cache_size,
>> +                                           l2_cache_entry_size);
>
> my gcc thinks l2_cache_entry_size may be used uninitialized here.
> can't quite see how that may happen though..

My gcc doesn't say anything but I've seen the warning:

   https://lists.gnu.org/archive/html/qemu-block/2017-12/msg00521.html

l2_cache_entry_size can remain uninitialized if read_cache_sizes()
fails, but then the calling function would return immediately. I guess
gcc doesn't know that.

I guess I'll initialize it to 0 to fix the warning.

Berto