[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-block] [Qemu-devel] [PATCH v2 31/32] qcow2: Allow configuring

From: Alberto Garcia
Subject: Re: [Qemu-block] [Qemu-devel] [PATCH v2 31/32] qcow2: Allow configuring the L2 slice size
Date: Mon, 22 Jan 2018 16:56:30 +0100
User-agent: Notmuch/0.18.2 (http://notmuchmail.org) Emacs/24.4.1 (i586-pc-linux-gnu)

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

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


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.


reply via email to

[Prev in Thread] Current Thread [Next in Thread]