[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: |
Alberto Garcia |
Subject: |
Re: [Qemu-block] [PATCH v7 4/9] qcow2: Avoid duplication in setting the refcount cache size |
Date: |
Fri, 10 Aug 2018 15:14:40 +0200 |
User-agent: |
Notmuch/0.18.2 (http://notmuchmail.org) Emacs/24.4.1 (i586-pc-linux-gnu) |
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 <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.
> + /* 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.
- [Qemu-block] [PATCH v7 0/9] Take the image size into account when allocating the L2 cache, Leonid Bloch, 2018/08/10
- [Qemu-block] [PATCH v7 1/9] qcow2: Options' documentation fixes, Leonid Bloch, 2018/08/10
- [Qemu-block] [PATCH v7 2/9] qcow2: Cosmetic changes, Leonid Bloch, 2018/08/10
- [Qemu-block] [PATCH v7 3/9] qcow2: Make sizes more humanly readable, Leonid Bloch, 2018/08/10
- [Qemu-block] [PATCH v7 4/9] qcow2: Avoid duplication in setting the refcount cache size, Leonid Bloch, 2018/08/10
- Re: [Qemu-block] [PATCH v7 4/9] qcow2: Avoid duplication in setting the refcount cache size,
Alberto Garcia <=
- [Qemu-block] [PATCH v7 5/9] qcow2: Assign the L2 cache relatively to the image size, Leonid Bloch, 2018/08/10
[Qemu-block] [PATCH v7 6/9] qcow2: Increase the default upper limit on the L2 cache size, Leonid Bloch, 2018/08/10