[v7,4/9] qcow2: Avoid duplication in setting the refcount cache size

Message ID 20180810062647.23211-5-lbloch@janustech.com
State New
Headers show
Series
  • Take the image size into account when allocating the L2 cache
Related show

Commit Message

Leonid Bloch Aug. 10, 2018, 6:26 a.m.
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 <lbloch@janustech.com>
---
 block/qcow2.c | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

Comments

Alberto Garcia Aug. 10, 2018, 1:14 p.m. | #1
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 <lbloch@janustech.com>

> -    } 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.
Leonid Bloch Aug. 11, 2018, 6:40 p.m. | #2
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
>> qcow2_update_options_prepare().
>>
>> Signed-off-by: Leonid Bloch <lbloch@janustech.com>
> 
>> -    } 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.
> 
> 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

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

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

Leonid.

Patch

diff --git a/block/qcow2.c b/block/qcow2.c
index 7a2d7a1d48..053242f94e 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -829,16 +829,13 @@  static void read_cache_sizes(BlockDriverState *bs, QemuOpts *opts,
                 *l2_cache_size = combined_cache_size - *refcount_cache_size;
             }
         }
-    } 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;
-        }
+    } 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 is not specified, it will be set to minimum
+     * in qcow2_update_options_prepare(). No need to set it here. */
 
     if (*l2_cache_entry_size < (1 << MIN_CLUSTER_BITS) ||
         *l2_cache_entry_size > s->cluster_size ||