diff mbox series

[v9,8/9] qcow2: Set the default cache-clean-interval to 10 minutes

Message ID 20180918152923.24824-9-lbloch@janustech.com (mailing list archive)
State New, archived
Headers show
Series Take the image size into account when allocating the L2 cache | expand

Commit Message

Leonid Bloch Sept. 18, 2018, 3:29 p.m. UTC 
The default cache-clean-interval is set to 10 minutes, in order to lower
the overhead of the qcow2 caches (before the default was 0, i.e.
disabled).

Signed-off-by: Leonid Bloch <lbloch@janustech.com>
Reviewed-by: Alberto Garcia <berto@igalia.com>
---
 block/qcow2.c        | 2 +-
 block/qcow2.h        | 1 +
 docs/qcow2-cache.txt | 4 ++--
 qapi/block-core.json | 3 ++-
 qemu-options.hx      | 2 +-
 5 files changed, 7 insertions(+), 5 deletions(-)

Comments

Alberto Garcia Sept. 21, 2018, 12:35 p.m. UTC | #1 
On Tue 18 Sep 2018 05:29:22 PM CEST, Leonid Bloch wrote:
>      /* New interval for cache cleanup timer */
>      r->cache_clean_interval =
>          qemu_opt_get_number(opts, QCOW2_OPT_CACHE_CLEAN_INTERVAL,
> -                            s->cache_clean_interval);
> +                            DEFAULT_CACHE_CLEAN_INTERVAL);

I just realized we're ignoring the old value (s->cache_clean_interval)
here. What's the consequence of this? (this was a change made by Kevin
Wolf in 5b0959a7d432062dcd740f8065004285b15695fa).

>  #ifndef CONFIG_LINUX
>      if (r->cache_clean_interval != 0) {
>          error_setg(errp, QCOW2_OPT_CACHE_CLEAN_INTERVAL
>                     " not supported on this host");

Another thing that I noticed (see below)...

> diff --git a/block/qcow2.h b/block/qcow2.h
> index 0f0e3534bf..8c863897b0 100644
> --- a/block/qcow2.h
> +++ b/block/qcow2.h
> @@ -82,6 +82,7 @@
>  
>  #define DEFAULT_CLUSTER_SIZE S_64KiB
>  
> +#define DEFAULT_CACHE_CLEAN_INTERVAL 600  /* seconds */

Shouldn't this be set to 0 in non-Linux platforms? Otherwise it will try
to set it to 600 by default in all platforms and will complain with the
"not supported on this host" error message that I quoted above.

Berto
Kevin Wolf Sept. 21, 2018, 12:56 p.m. UTC | #2 
Am 21.09.2018 um 14:35 hat Alberto Garcia geschrieben:
> On Tue 18 Sep 2018 05:29:22 PM CEST, Leonid Bloch wrote:
> >      /* New interval for cache cleanup timer */
> >      r->cache_clean_interval =
> >          qemu_opt_get_number(opts, QCOW2_OPT_CACHE_CLEAN_INTERVAL,
> > -                            s->cache_clean_interval);
> > +                            DEFAULT_CACHE_CLEAN_INTERVAL);
> 
> I just realized we're ignoring the old value (s->cache_clean_interval)
> here. What's the consequence of this? (this was a change made by Kevin
> Wolf in 5b0959a7d432062dcd740f8065004285b15695fa).

I think the consequence is that bdrv_reopen() will not keep the current
value if the option isn't given, but reset it to the default.

Kevin
Alberto Garcia Sept. 21, 2018, 1:11 p.m. UTC | #3 
On Fri 21 Sep 2018 02:56:06 PM CEST, Kevin Wolf wrote:
> Am 21.09.2018 um 14:35 hat Alberto Garcia geschrieben:
>> On Tue 18 Sep 2018 05:29:22 PM CEST, Leonid Bloch wrote:
>> >      /* New interval for cache cleanup timer */
>> >      r->cache_clean_interval =
>> >          qemu_opt_get_number(opts, QCOW2_OPT_CACHE_CLEAN_INTERVAL,
>> > -                            s->cache_clean_interval);
>> > +                            DEFAULT_CACHE_CLEAN_INTERVAL);
>> 
>> I just realized we're ignoring the old value (s->cache_clean_interval)
>> here. What's the consequence of this? (this was a change made by Kevin
>> Wolf in 5b0959a7d432062dcd740f8065004285b15695fa).
>
> I think the consequence is that bdrv_reopen() will not keep the current
> value if the option isn't given, but reset it to the default.

But does it happen in practice?

bdrv_reopen() always keeps the current set of options unless they're
overridden, and in my new "blockdev-reopen" patches the idea is that if
you don't specify the value then it should be reset to the default.

Berto
Leonid Bloch Sept. 21, 2018, 1:46 p.m. UTC | #4 
On 9/21/18 3:35 PM, Alberto Garcia wrote:
> On Tue 18 Sep 2018 05:29:22 PM CEST, Leonid Bloch wrote:
>>       /* New interval for cache cleanup timer */
>>       r->cache_clean_interval =
>>           qemu_opt_get_number(opts, QCOW2_OPT_CACHE_CLEAN_INTERVAL,
>> -                            s->cache_clean_interval);
>> +                            DEFAULT_CACHE_CLEAN_INTERVAL);
> 
> I just realized we're ignoring the old value (s->cache_clean_interval)
> here. What's the consequence of this? (this was a change made by Kevin
> Wolf in 5b0959a7d432062dcd740f8065004285b15695fa).
> 
>>   #ifndef CONFIG_LINUX
>>       if (r->cache_clean_interval != 0) {
>>           error_setg(errp, QCOW2_OPT_CACHE_CLEAN_INTERVAL
>>                      " not supported on this host");
> 
> Another thing that I noticed (see below)...
> 
>> diff --git a/block/qcow2.h b/block/qcow2.h
>> index 0f0e3534bf..8c863897b0 100644
>> --- a/block/qcow2.h
>> +++ b/block/qcow2.h
>> @@ -82,6 +82,7 @@
>>   
>>   #define DEFAULT_CLUSTER_SIZE S_64KiB
>>   
>> +#define DEFAULT_CACHE_CLEAN_INTERVAL 600  /* seconds */
> 
> Shouldn't this be set to 0 in non-Linux platforms? Otherwise it will try
> to set it to 600 by default in all platforms and will complain with the
> "not supported on this host" error message that I quoted above.

You're right! Will fix, thanks.

Leonid.

> 
> Berto
>
diff mbox series

Patch

diff --git a/block/qcow2.c b/block/qcow2.c
index 1445cd5360..f885afa0ed 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -944,7 +944,7 @@  static int qcow2_update_options_prepare(BlockDriverState *bs,
     /* New interval for cache cleanup timer */
     r->cache_clean_interval =
         qemu_opt_get_number(opts, QCOW2_OPT_CACHE_CLEAN_INTERVAL,
-                            s->cache_clean_interval);
+                            DEFAULT_CACHE_CLEAN_INTERVAL);
 #ifndef CONFIG_LINUX
     if (r->cache_clean_interval != 0) {
         error_setg(errp, QCOW2_OPT_CACHE_CLEAN_INTERVAL
diff --git a/block/qcow2.h b/block/qcow2.h
index 0f0e3534bf..8c863897b0 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -82,6 +82,7 @@ 
 
 #define DEFAULT_CLUSTER_SIZE S_64KiB
 
+#define DEFAULT_CACHE_CLEAN_INTERVAL 600  /* seconds */
 
 #define QCOW2_OPT_LAZY_REFCOUNTS "lazy-refcounts"
 #define QCOW2_OPT_DISCARD_REQUEST "pass-discard-request"
diff --git a/docs/qcow2-cache.txt b/docs/qcow2-cache.txt
index 5965d3d094..15ae797931 100644
--- a/docs/qcow2-cache.txt
+++ b/docs/qcow2-cache.txt
@@ -209,8 +209,8 @@  This example removes all unused cache entries every 15 minutes:
 
    -drive file=hd.qcow2,cache-clean-interval=900
 
-If unset, the default value for this parameter is 0 and it disables
-this feature.
+If unset, the default value for this parameter is 600. Setting it to 0
+disables this feature.
 
 Note that this functionality currently relies on the MADV_DONTNEED
 argument for madvise() to actually free the memory. This is a
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 4c7a37afdc..08c27b9af7 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -2827,7 +2827,8 @@ 
 #
 # @cache-clean-interval:  clean unused entries in the L2 and refcount
 #                         caches. The interval is in seconds. The default value
-#                         is 0 and it disables this feature (since 2.5)
+#                         is 600, and 0 disables this feature. (since 2.5)
+#
 # @encrypt:               Image decryption options. Mandatory for
 #                         encrypted images, except when doing a metadata-only
 #                         probe of the image. (since 2.10)
diff --git a/qemu-options.hx b/qemu-options.hx
index d5f4bcadd4..2975fdf9f8 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -757,7 +757,7 @@  it which is not used for the L2 cache)
 
 @item cache-clean-interval
 Clean unused entries in the L2 and refcount caches. The interval is in seconds.
-The default value is 0 and it disables this feature.
+The default value is 600. Setting it to 0 disables this feature.
 
 @item pass-discard-request
 Whether discard requests to the qcow2 device should be forwarded to the data