On Fri 10 Aug 2018 08:26:42 AM CEST, Leonid Bloch wrote:
> The refcount cache size does not need to be set to its minimum value in
> read_cache_sizes(), as it is set to at least its minimum value in
> qcow2_update_options_prepare().
>
> Signed-off-by: Leonid Bloch <[email protected]>
> - } else {
> - if (!l2_cache_size_set) {
> - *l2_cache_size = MAX(DEFAULT_L2_CACHE_SIZE,
> - (uint64_t)DEFAULT_L2_CACHE_CLUSTERS
> - * s->cluster_size);
> - }
> - if (!refcount_cache_size_set) {
> - *refcount_cache_size = min_refcount_cache;
> - }
Since you're getting rid of the rest of the code later anyway, I think
it's cleaner to only remove these last three lines here and leave the
rest untouched. It makes the patch shorter and easier to read.
> + /* If refcount-cache-size is not specified, it will be set to minimum
> + * in qcow2_update_options_prepare(). No need to set it here. */
This is not quite correct, because apart from the "not specified" case,
refcount_cache_size is also overridden in other circumstances: (a) the
value is specified but is too low, or (b) the value is set indirectly
via "cache-size" but the end result is too low[*].
Also, the same thing happens to l2-cache-size: if you set it manually
but it's too low then it will be overridden.
I'd personally remove the comment altogether, I think the explanation in
the commit message is enough.
Berto
[*] Now that I think of it: if you set e.g. cache-size = 16M and
l2-cache-size = 16MB then you'll end up with refcount-cache-size =
16M - 16M = 0, which will be overridden and set to the minimum.
But then refcount-cache-size + l2-cache-size > cache-size
So perhaps this should produce an error, and it may make sense to
take the opportunity to move all the logic that ensures that
MIN_SIZE <= size <= INT_MAX to read_cache_sizes(). But that's a
possible task for the future and I wouldn't worry about it in this
series.
