[Top][All Lists]

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

Re: [Qemu-block] [PATCH v7 4/9] qcow2: Avoid duplication in setting the

From: Leonid Bloch
Subject: Re: [Qemu-block] [PATCH v7 4/9] qcow2: Avoid duplication in setting the refcount cache size
Date: Sat, 11 Aug 2018 21:40:02 +0300
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.0

On 8/10/18 4:14 PM, Alberto Garcia wrote:
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

Signed-off-by: Leonid Bloch <address@hidden>

-    } 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.

But a correct formatting is important, I think. In every patch individually.

+    /* 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[*].

Yes, I'll fix the comment. But I think that it's important that it remains, because someone who looks at the code does not necessarily looks at the commit message, and it may be puzzling that a minimal size is not enforced there.

How about the following:
"l2_cache_size and refcount_cache_size are ensured to have at least their minimum values in qcow2_update_options_prepare()"

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.


[*] 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

Yes, I have noticed this behavior, and I think that it is OK: the errors should be a response to a wrong input of the user, who does not necessarily understands the internals. So if a user inputs l2-cache-size which is larger than cache-size, that's obviously a mistake. But if they are equal, it's totally OK if QEMU will use some 256 or 128 KB for the minimal caches transparently. This is really negligible relatively to the total QEMU overhead.

Now the error message is: "l2-cache-size may not exceed cache-size".
Is it reasonable to make it: "l2-cache-size may not exceed the size of cache-size minus the minimal refcount cache size" (or similar)? :)

     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

Yeah, but it would be a good idea to move *all* the size-determining logic in one function.


reply via email to

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