Clean unused entries in the qcow2 L2/refcount cache

This adds a new 'cache-clean-interval' option that cleans all qcow2 cache entries that haven't been used in a certain interval, given in seconds.

This allows setting a large L2 cache size so it can handle scenarios with lots of I/O and at the same time use little memory during periods of inactivity.

This feature currently relies on MADV_DONTNEED to free that memory, so it is not useful in systems that don't follow that behavior.

Signed-off-by Alberto Garcia <[email protected]>

Cc Max Reitz <[email protected]>

---
 block/qcow2-cache.c  | 35 ++++++++++++++++++++++++++++
 block/qcow2.c        | 66 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 block/qcow2.h        |  4 ++++
 qapi/block-core.json | 13 +++++++++--
 4 files changed, 116 insertions(+), 2 deletions(-)

block/qcow2.c +2 -0

+static void cache_clean_timer_init(BlockDriverState *bs, AioContext *context)
+{

Quoted 1 line

+static void cache_clean_timer_init(BlockDriverState *bs, AioContext *context)

This function sets up a timer for non-zero interval, but does nothing if interval is zero. [1]

block/qcow2.c static int qcow2_open(BlockDriverState *bs, QDict *options, int flags, · line 897

Quoted 5 lines

block/qcow2.c +5 -0

+    if (cache_clean_interval > UINT_MAX) {
+        error_setg(errp, "Cache clean interval too big");
+        ret = -EINVAL;
+        goto fail;
+    }

If you type the qapi code as 'uint32' rather than 'int', you could skip the error checking here because the parser would have already clamped things. But I can live with this as-is.

block/qcow2.c static int qcow2_open(BlockDriverState *bs, QDict *options, int flags, · line 899

Quoted 2 lines

block/qcow2.c +2 -0

+    s->cache_clean_interval = cache_clean_interval;
+    cache_clean_timer_init(bs, bdrv_get_aio_context(bs));

[1] But here, you are unconditionally calling init, whether the new value is 0 or nonzero.

Can a block reopen ever cause an existing BDS to change its interval, in which case I could create a BDS originally with a timer, then reopen it without a timer, and init() would have to remove the existing timer? If I'm reading this patch correctly, right now the interval is a write-once deal (no way to change it after the fact), so your code is okay; but should a separate patch be added to allow adjusting the interval, via a reopen operation?

Quoted 22 lines · Alberto Garcia

This adds a new 'cache-clean-interval' option that cleans all qcow2 cache entries that haven't been used in a certain interval, given in seconds.

This allows setting a large L2 cache size so it can handle scenarios with lots of I/O and at the same time use little memory during periods of inactivity.

This feature currently relies on MADV_DONTNEED to free that memory, so it is not useful in systems that don't follow that behavior.

Signed-off-by Alberto Garcia <[email protected]>

Cc Max Reitz <[email protected]>

---
  block/qcow2-cache.c  | 35 ++++++++++++++++++++++++++++
  block/qcow2.c        | 66 ++++++++++++++++++++++++++++++++++++++++++++++++++++
  block/qcow2.h        |  4 ++++
  qapi/block-core.json | 13 +++++++++--
  4 files changed, 116 insertions(+), 2 deletions(-)

block/qcow2.c +2 -0

+static void cache_clean_timer_init(BlockDriverState *bs, AioContext *context)
+{

block/qcow2.c static const char *overlap_bool_option_names[QCOW2_OL_MAX_BITNR] = { · line 512

Quoted 10 lines

block/qcow2.c +10 -0

+    BDRVQcowState *s = bs->opaque;
+    if (s->cache_clean_interval > 0) {
+        s->cache_clean_timer = aio_timer_new(context, QEMU_CLOCK_VIRTUAL,
+                                             SCALE_MS, cache_clean_timer_cb,
+                                             bs);
+        timer_mod(s->cache_clean_timer, qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) +
+                  (int64_t) s->cache_clean_interval * 1000);
+    }
+}
+

This function sets up a timer for non-zero interval, but does nothing if interval is zero. [1]

Quoted 4 lines

block/qcow2.c +0 -0

@@ -839,6 +888,16 @@ static int qcow2_open(BlockDriverState *bs, QDict *options, int flags,
          goto fail;
      }
 

block/qcow2.c static int qcow2_open(BlockDriverState *bs, QDict *options, int flags, · line 897

Quoted 7 lines

block/qcow2.c +7 -0

+    cache_clean_interval =
+        qemu_opt_get_number(opts, QCOW2_OPT_CACHE_CLEAN_INTERVAL, 0);
+    if (cache_clean_interval > UINT_MAX) {
+        error_setg(errp, "Cache clean interval too big");
+        ret = -EINVAL;
+        goto fail;
+    }

If you type the qapi code as 'uint32' rather than 'int', you could skip the error checking here because the parser would have already clamped things. But I can live with this as-is.

block/qcow2.c static int qcow2_open(BlockDriverState *bs, QDict *options, int flags, · line 899

Quoted 2 lines

block/qcow2.c +2 -0

+    s->cache_clean_interval = cache_clean_interval;
+    cache_clean_timer_init(bs, bdrv_get_aio_context(bs));

[1] But here, you are unconditionally calling init, whether the new value is 0 or nonzero. Can a block reopen ever cause an existing BDS to change its interval, in which case I could create a BDS originally with a timer, then reopen it without a timer, and init() would have to remove the existing timer? If I'm reading this patch correctly, right now the interval is a write-once deal (no way to change it after the fact), so your code is okay; but should a separate patch be added to allow adjusting the interval, via a reopen operation?

Reviewed-by Eric Blake <[email protected]>

This adds a new 'cache-clean-interval' option that cleans all qcow2 cache entries that haven't been used in a certain interval, given in seconds.

This allows setting a large L2 cache size so it can handle scenarios with lots of I/O and at the same time use little memory during periods of inactivity.

This feature currently relies on MADV_DONTNEED to free that memory, so it is not useful in systems that don't follow that behavior.

Signed-off-by Alberto Garcia <[email protected]>

Cc Max Reitz <[email protected]>

---
  block/qcow2-cache.c  | 35 ++++++++++++++++++++++++++++
  block/qcow2.c        | 66 ++++++++++++++++++++++++++++++++++++++++++++++++++++
  block/qcow2.h        |  4 ++++
  qapi/block-core.json | 13 +++++++++--
  4 files changed, 116 insertions(+), 2 deletions(-)

block/qcow2-cache.c +35 -0

index ed14a92..a215f5b 100644
@@ -43,6 +43,7 @@ struct Qcow2Cache {
      bool                    depends_on_flush;
      void                   *table_array;
      uint64_t                lru_counter;
+    uint64_t                cache_clean_lru_counter;
  };
 
  static inline void *qcow2_cache_get_table_addr(BlockDriverState *bs,

@@ -78,6 +79,40 @@ static void qcow2_cache_table_release(BlockDriverState *bs, Qcow2Cache *c,
  #endif
  }
 
+static inline bool can_clean_entry(Qcow2Cache *c, int i)
+{
+    Qcow2CachedTable *t = &c->entries[i];
+    return t->ref == 0 && !t->dirty && t->offset != 0 &&
+        t->lru_counter <= c->cache_clean_lru_counter;
+}
+
+void qcow2_cache_clean_unused(BlockDriverState *bs, Qcow2Cache *c)
+{
+    int i = 0;
+    while (i < c->size) {
+        int to_clean = 0;
+
+        /* Skip the entries that we don't need to clean */
+        while (i < c->size && !can_clean_entry(c, i)) {
+            i++;
+        }
+
+        /* And count how many we can clean in a row */
+        while (i < c->size && can_clean_entry(c, i)) {
+            c->entries[i].offset = 0;
+            c->entries[i].lru_counter = 0;
+            i++;
+            to_clean++;
+        }
+
+        if (to_clean > 0) {
+            qcow2_cache_table_release(bs, c, i - to_clean, to_clean);
+        }
+    }
+
+    c->cache_clean_lru_counter = c->lru_counter;
+}
+
  Qcow2Cache *qcow2_cache_create(BlockDriverState *bs, int num_tables)
  {
      BDRVQcowState *s = bs->opaque;

block/qcow2.c +66 -0

index f7b4cc6..abafcdf 100644
@@ -468,6 +468,11 @@ static QemuOptsList qcow2_runtime_opts = {
              .type = QEMU_OPT_SIZE,
              .help = "Maximum refcount block cache size",
          },
+        {
+            .name = QCOW2_OPT_CACHE_CLEAN_INTERVAL,
+            .type = QEMU_OPT_NUMBER,
+            .help = "Clean unused cache entries after this time (in seconds)",
+        },
          { /* end of list */ }
      },
  };

@@ -483,6 +488,49 @@ static const char *overlap_bool_option_names[QCOW2_OL_MAX_BITNR] = {
      [QCOW2_OL_INACTIVE_L2_BITNR]    = QCOW2_OPT_OVERLAP_INACTIVE_L2,
  };
 
+static void cache_clean_timer_cb(void *opaque)
+{
+    BlockDriverState *bs = opaque;
+    BDRVQcowState *s = bs->opaque;
+    qcow2_cache_clean_unused(bs, s->l2_table_cache);
+    qcow2_cache_clean_unused(bs, s->refcount_block_cache);
+    timer_mod(s->cache_clean_timer, qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) +
+              (int64_t) s->cache_clean_interval * 1000);
+}
+
+static void cache_clean_timer_init(BlockDriverState *bs, AioContext *context)
+{
+    BDRVQcowState *s = bs->opaque;
+    if (s->cache_clean_interval > 0) {
+        s->cache_clean_timer = aio_timer_new(context, QEMU_CLOCK_VIRTUAL,
+                                             SCALE_MS, cache_clean_timer_cb,
+                                             bs);
+        timer_mod(s->cache_clean_timer, qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) +
+                  (int64_t) s->cache_clean_interval * 1000);
+    }
+}
+
+static void cache_clean_timer_del(BlockDriverState *bs)
+{
+    BDRVQcowState *s = bs->opaque;
+    if (s->cache_clean_timer) {
+        timer_del(s->cache_clean_timer);
+        timer_free(s->cache_clean_timer);
+        s->cache_clean_timer = NULL;
+    }
+}
+
+static void qcow2_detach_aio_context(BlockDriverState *bs)
+{
+    cache_clean_timer_del(bs);
+}
+
+static void qcow2_attach_aio_context(BlockDriverState *bs,
+                                     AioContext *new_context)
+{
+    cache_clean_timer_init(bs, new_context);
+}
+
  static void read_cache_sizes(QemuOpts *opts, uint64_t *l2_cache_size,
                               uint64_t *refcount_cache_size, Error **errp)
  {

@@ -552,6 +600,7 @@ static int qcow2_open(BlockDriverState *bs, QDict *options, int flags,
      const char *opt_overlap_check, *opt_overlap_check_template;
      int overlap_check_template = 0;
      uint64_t l2_cache_size, refcount_cache_size;
+    uint64_t cache_clean_interval;
 
      ret = bdrv_pread(bs->file, 0, &header, sizeof(header));
      if (ret < 0) {

@@ -839,6 +888,16 @@ static int qcow2_open(BlockDriverState *bs, QDict *options, int flags,
          goto fail;
      }
 
+    cache_clean_interval =
+        qemu_opt_get_number(opts, QCOW2_OPT_CACHE_CLEAN_INTERVAL, 0);
+    if (cache_clean_interval > UINT_MAX) {
+        error_setg(errp, "Cache clean interval too big");
+        ret = -EINVAL;
+        goto fail;
+    }
+    s->cache_clean_interval = cache_clean_interval;
+    cache_clean_timer_init(bs, bdrv_get_aio_context(bs));
+
      s->cluster_cache = g_malloc(s->cluster_size);
      /* one more sector for decompressed data alignment */
      s->cluster_data = qemu_try_blockalign(bs->file, QCOW_MAX_CRYPT_CLUSTERS

@@ -1004,6 +1063,7 @@ static int qcow2_open(BlockDriverState *bs, QDict *options, int flags,
      qemu_vfree(s->l1_table);
      /* else pre-write overlap checks in cache_destroy may crash */
      s->l1_table = NULL;
+    cache_clean_timer_del(bs);
      if (s->l2_table_cache) {
          qcow2_cache_destroy(bs, s->l2_table_cache);
      }

@@ -1458,6 +1518,7 @@ static void qcow2_close(BlockDriverState *bs)
          }
      }
 
+    cache_clean_timer_del(bs);
      qcow2_cache_destroy(bs, s->l2_table_cache);
      qcow2_cache_destroy(bs, s->refcount_block_cache);
 

@@ -2552,6 +2613,8 @@ static ImageInfoSpecific *qcow2_get_specific_info(BlockDriverState *bs)
          };
      }
 
+    spec_info->qcow2->cache_clean_interval = s->cache_clean_interval;
+
      return spec_info;
  }
 

@@ -2970,6 +3033,9 @@ BlockDriver bdrv_qcow2 = {
      .create_opts         = &qcow2_create_opts,
      .bdrv_check          = qcow2_check,
      .bdrv_amend_options  = qcow2_amend_options,
+
+    .bdrv_detach_aio_context  = qcow2_detach_aio_context,
+    .bdrv_attach_aio_context  = qcow2_attach_aio_context,
  };
 
  static void bdrv_qcow2_init(void)

block/qcow2.h +4 -0

index 0076512..2c45eb2 100644
@@ -93,6 +93,7 @@
  #define QCOW2_OPT_CACHE_SIZE "cache-size"
  #define QCOW2_OPT_L2_CACHE_SIZE "l2-cache-size"
  #define QCOW2_OPT_REFCOUNT_CACHE_SIZE "refcount-cache-size"
+#define QCOW2_OPT_CACHE_CLEAN_INTERVAL "cache-clean-interval"
 
  typedef struct QCowHeader {
      uint32_t magic;

@@ -236,6 +237,8 @@ typedef struct BDRVQcowState {
 
      Qcow2Cache* l2_table_cache;
      Qcow2Cache* refcount_block_cache;
+    QEMUTimer *cache_clean_timer;
+    unsigned cache_clean_interval;
 
      uint8_t *cluster_cache;
      uint8_t *cluster_data;

@@ -581,6 +584,7 @@ int qcow2_cache_set_dependency(BlockDriverState *bs, Qcow2Cache *c,
      Qcow2Cache *dependency);
  void qcow2_cache_depends_on_flush(Qcow2Cache *c);
 
+void qcow2_cache_clean_unused(BlockDriverState *bs, Qcow2Cache *c);
  int qcow2_cache_empty(BlockDriverState *bs, Qcow2Cache *c);
 
  int qcow2_cache_get(BlockDriverState *bs, Qcow2Cache *c, uint64_t offset,

qapi/block-core.json +4 -0

index 863ffea..3a9b590 100644
@@ -41,6 +41,10 @@
  # @corrupt: #optional true if the image has been marked corrupt; only valid for
  #           compat >= 1.1 (since 2.2)
  #
+# @cache-clean-interval: interval in seconds after which unused L2 and
+#                        refcount cache entries are removed. If 0 then
+#                        this feature is not enabled (since 2.4)
+#
  # @refcount-bits: width of a refcount entry in bits (since 2.3)
  #
  # Since: 1.7

qapi/block-core.json +5 -1

  ##

@@ -1538,6 +1543,9 @@
  # @refcount-cache-size:   #optional the maximum size of the refcount block cache
  #                         in bytes (since 2.2)
  #
+# @cache-clean-interval:  #optional clean unused entries in the L2 and refcount
+#                         caches. The interval is in seconds (since 2.4)
+#
  # Since: 1.7
  ##
  { 'struct': 'BlockdevOptionsQcow2',

@@ -1549,7 +1557,8 @@
              '*overlap-check': 'Qcow2OverlapChecks',
              '*cache-size': 'int',
              '*l2-cache-size': 'int',
-            '*refcount-cache-size': 'int' } }
+            '*refcount-cache-size': 'int',
+            '*cache-clean-interval': 'int' } }
 
 
  ##

qapi/block-core.json line 59

Quoted 7 lines

        'compat': 'str',
        '*lazy-refcounts': 'bool',
        '*corrupt': 'bool',

qapi/block-core.json +2 -1

-      'refcount-bits': 'int'
+      'refcount-bits': 'int',
+      'cache-clean-interval': 'int'
    } }

I'm not too happy about making this part of ImageInfoSpecificQCow2. Two reasons for this: First, it's eventually part of ImageInfo, which is defined as "Information about a QEMU image file", but this option cannot be set in the image file itself but is only a run-time option.

Quoted 7 lines · Max Reitz

Quoted 7 lines

         'compat': 'str',
         '*lazy-refcounts': 'bool',
         '*corrupt': 'bool',

qapi/block-core.json +2 -1

-      'refcount-bits': 'int'
+      'refcount-bits': 'int',
+      'cache-clean-interval': 'int'
     } }

Quoted 4 lines

I'm not too happy about making this part of ImageInfoSpecificQCow2. Two reasons for this: First, it's eventually part of ImageInfo, which is defined as "Information about a QEMU image file", but this option cannot be set in the image file itself but is only a run-time option.

That's a valid point. Now that I think of it, do we actually have a way to retrieve the sizes of the L2 and refcount caches? I think cache-size, l2-cache-size, and refcount-cache-size are already write-only values.

Quoted 12 lines · Alberto Garcia

Quoted 11 lines · Max Reitz

qapi/block-core.json line 59

Quoted 7 lines

         'compat': 'str',
         '*lazy-refcounts': 'bool',
         '*corrupt': 'bool',

qapi/block-core.json +2 -1

-      'refcount-bits': 'int'
+      'refcount-bits': 'int',
+      'cache-clean-interval': 'int'
     } }

I'm not too happy about making this part of ImageInfoSpecificQCow2. Two reasons for this: First, it's eventually part of ImageInfo, which is defined as "Information about a QEMU image file", but this option cannot be set in the image file itself but is only a run-time option.

Quoted 3 lines

That's a valid point. Now that I think of it, do we actually have a way to retrieve the sizes of the L2 and refcount caches? I think cache-size, l2-cache-size, and refcount-cache-size are already write-only values.

No, in fact we don't. Well, except for if you manage to retrieve the JSON filename for the qcow2 BDS, it should be part of that. :-P

Quoted 26 lines · Alberto Garcia

This adds a new 'cache-clean-interval' option that cleans all qcow2 cache entries that haven't been used in a certain interval, given in seconds.

This allows setting a large L2 cache size so it can handle scenarios with lots of I/O and at the same time use little memory during periods of inactivity.

This feature currently relies on MADV_DONTNEED to free that memory, so it is not useful in systems that don't follow that behavior.

+++ b/qapi/block-core.json

(quoted diff) +0 -0

@@ -41,6 +41,10 @@
  # @corrupt: #optional true if the image has been marked corrupt;

only valid for
# compat >= 1.1 (since 2.2)
#

qapi/block-core.json +4 -0

+# @cache-clean-interval: interval in seconds after which unused L2 and
+#                        refcount cache entries are removed. If 0 then
+#                        this feature is not enabled (since 2.4)
+#
  # @refcount-bits: width of a refcount entry in bits (since 2.3)
  #
  # Since: 1.7

qapi/block-core.json line 59

Quoted 7 lines

qapi/block-core.json +2 -1

@@ -50,7 +54,8 @@
        'compat': 'str',
        '*lazy-refcounts': 'bool',
        '*corrupt': 'bool',
-      'refcount-bits': 'int'
+      'refcount-bits': 'int',
+      'cache-clean-interval': 'int'
    } }

I'm not too happy about making this part of ImageInfoSpecificQCow2. Two reasons for this: First, it's eventually part of ImageInfo, which is defined as "Information about a QEMU image file", but this option cannot be set in the image file itself but is only a run-time option.

Second, my original goal for ImageInfoSpecific was to provide more information through qemu-img info, and this value will look pretty strange there.

I don't know how to resolve this quandary, though. Since qemu cannot change this interval by itself, I think not providing an interface for reading it is fine. On the other hand, if Eric finds such an interface absolutely mandatory, I can't think of a better place to return it than here.

The only solution which would satisfy both requirements would be another structure which contains "online" flags, and thus is not evaluated by qemu-img info, but only by QMP commands. But that's ugly.

Quoted 42 lines · Max Reitz

Quoted 25 lines · Alberto Garcia

This adds a new 'cache-clean-interval' option that cleans all qcow2 cache entries that haven't been used in a certain interval, given in seconds.

This allows setting a large L2 cache size so it can handle scenarios with lots of I/O and at the same time use little memory during periods of inactivity.

This feature currently relies on MADV_DONTNEED to free that memory, so it is not useful in systems that don't follow that behavior.

+++ b/qapi/block-core.json

(quoted diff) +0 -0

@@ -41,6 +41,10 @@
   # @corrupt: #optional true if the image has been marked corrupt;

only valid for
# compat >= 1.1 (since 2.2)
#

qapi/block-core.json +4 -0

+# @cache-clean-interval: interval in seconds after which unused L2 and
+#                        refcount cache entries are removed. If 0 then
+#                        this feature is not enabled (since 2.4)
+#
   # @refcount-bits: width of a refcount entry in bits (since 2.3)
   #
   # Since: 1.7

qapi/block-core.json line 59

Quoted 7 lines

qapi/block-core.json +2 -1

@@ -50,7 +54,8 @@
         'compat': 'str',
         '*lazy-refcounts': 'bool',
         '*corrupt': 'bool',
-      'refcount-bits': 'int'
+      'refcount-bits': 'int',
+      'cache-clean-interval': 'int'
     } }

I'm not too happy about making this part of ImageInfoSpecificQCow2. Two reasons for this: First, it's eventually part of ImageInfo, which is defined as "Information about a QEMU image file", but this option cannot be set in the image file itself but is only a run-time option.

Second, my original goal for ImageInfoSpecific was to provide more information through qemu-img info, and this value will look pretty strange there.

Quoted 5 lines

I don't know how to resolve this quandary, though. Since qemu cannot change this interval by itself, I think not providing an interface for reading it is fine. On the other hand, if Eric finds such an interface absolutely mandatory, I can't think of a better place to return it than here.

Can we mark the parameter optional, and only provide it when it is non-zero? That way, qemu-img (which cannot set an interval) will not report it, and the only time it will appear is if it was set as part of opening the block device under qemu.

Quoted 4 lines

The only solution which would satisfy both requirements would be another structure which contains "online" flags, and thus is not evaluated by qemu-img info, but only by QMP commands. But that's ugly.

Yeah, I'm not sure such duplication helps. I'd still like it reported somewhere, though, as it is nice to query that a requested setting is actually working.

Quoted 13 lines

Quoted 13 lines

qapi/block-core.json line 59

Quoted 7 lines

         'compat': 'str',
         '*lazy-refcounts': 'bool',
         '*corrupt': 'bool',

qapi/block-core.json +2 -1

-      'refcount-bits': 'int'
+      'refcount-bits': 'int',
+      'cache-clean-interval': 'int'
     } }

I'm not too happy about making this part of ImageInfoSpecificQCow2. Two reasons for this: First, it's eventually part of ImageInfo, which is defined as "Information about a QEMU image file", but this option cannot be set in the image file itself but is only a run-time option.

Quoted 4 lines

Can we mark the parameter optional, and only provide it when it is non-zero? That way, qemu-img (which cannot set an interval) will not report it, and the only time it will appear is if it was set as part of opening the block device under qemu.

That sounds good.

Quoted 17 lines · Max Reitz

Quoted 17 lines

Quoted 7 lines

qapi/block-core.json line 59

Quoted 7 lines

          'compat': 'str',
          '*lazy-refcounts': 'bool',
          '*corrupt': 'bool',

qapi/block-core.json +2 -1

-      'refcount-bits': 'int'
+      'refcount-bits': 'int',
+      'cache-clean-interval': 'int'
      } }

Quoted 6 lines

I'm not too happy about making this part of ImageInfoSpecificQCow2. Two reasons for this: First, it's eventually part of ImageInfo, which is defined as "Information about a QEMU image file", but this option cannot be set in the image file itself but is only a run-time option.

Can we mark the parameter optional, and only provide it when it is non-zero? That way, qemu-img (which cannot set an interval) will not report it, and the only time it will appear is if it was set as part of opening the block device under qemu.

Quoted 1 line

That sounds good.

But what do we do with the other parameters (refcount-cache-size, l2-cache-size)? We cannot have the same solution there because they don't belong to the image file either, and they're never going go be zero.

Quoted 6 lines

Quoted 6 lines

Quoted 4 lines

Can we mark the parameter optional, and only provide it when it is non-zero? That way, qemu-img (which cannot set an interval) will not report it, and the only time it will appear is if it was set as part of opening the block device under qemu.

That sounds good.

Quoted 4 lines

But what do we do with the other parameters (refcount-cache-size, l2-cache-size)? We cannot have the same solution there because they don't belong to the image file either, and they're never going go be zero.

Of course, the simplest solution is to worry about cache-clean-interval for now and worry about the cache sizes later… But that basically means "We'll never worry about them unless someone complains", so…

Quoted 9 lines

Quoted 5 lines

Quoted 4 lines

Can we mark the parameter optional, and only provide it when it is non-zero? That way, qemu-img (which cannot set an interval) will not report it, and the only time it will appear is if it was set as part of opening the block device under qemu.

That sounds good.

But what do we do with the other parameters (refcount-cache-size, l2-cache-size)? We cannot have the same solution there because they don't belong to the image file either, and they're never going go be zero.

Pssht, don't mention it, or Eric will notice. :-)

Well, one solution would be to remember whether they have been set explicitly or not. But that gets ugly really quickly. Maybe Kevin's series helps there, but I don't know.

Of course, the simplest solution is to worry about cache-clean-interval for now and worry about the cache sizes later… But that basically means "We'll never worry about them unless someone complains", so…

Quoted 17 lines

Quoted 10 lines

Quoted 6 lines

Quoted 5 lines

Can we mark the parameter optional, and only provide it when it is non-zero? That way, qemu-img (which cannot set an interval) will not report it, and the only time it will appear is if it was set as part of opening the block device under qemu.

That sounds good.

But what do we do with the other parameters (refcount-cache-size, l2-cache-size)? We cannot have the same solution there because they don't belong to the image file either, and they're never going go be zero.

Pssht, don't mention it, or Eric will notice. :-)

Well, one solution would be to remember whether they have been set explicitly or not. But that gets ugly really quickly. Maybe Kevin's series helps there, but I don't know.

Quoted 3 lines

Of course, the simplest solution is to worry about cache-clean-interval for now and worry about the cache sizes later… But that basically means "We'll never worry about them unless someone complains", so…

Hmm, now that we have three pieces of data, I'm starting to lean more towards ImageInfoSpecific being the wrong place for this after all; it would still be nice to advertise all three, but where? Is BlockdevCacheInfo more appropriate (as a sub-struct of BlockDeviceInfo)?

Quoted 44 lines · Alberto Garcia

Quoted 26 lines

This adds a new 'cache-clean-interval' option that cleans all qcow2 cache entries that haven't been used in a certain interval, given in seconds.

This allows setting a large L2 cache size so it can handle scenarios with lots of I/O and at the same time use little memory during periods of inactivity.

This feature currently relies on MADV_DONTNEED to free that memory, so it is not useful in systems that don't follow that behavior.

+++ b/qapi/block-core.json

(quoted diff) +0 -0

@@ -41,6 +41,10 @@
  # @corrupt: #optional true if the image has been marked corrupt;

only valid for
# compat >= 1.1 (since 2.2)
#

qapi/block-core.json +4 -0

+# @cache-clean-interval: interval in seconds after which unused L2 and
+#                        refcount cache entries are removed. If 0 then
+#                        this feature is not enabled (since 2.4)
+#
  # @refcount-bits: width of a refcount entry in bits (since 2.3)
  #
  # Since: 1.7

qapi/block-core.json line 59

Quoted 7 lines

qapi/block-core.json +2 -1

@@ -50,7 +54,8 @@
        'compat': 'str',
        '*lazy-refcounts': 'bool',
        '*corrupt': 'bool',
-      'refcount-bits': 'int'
+      'refcount-bits': 'int',
+      'cache-clean-interval': 'int'
    } }

I'm not too happy about making this part of ImageInfoSpecificQCow2. Two reasons for this: First, it's eventually part of ImageInfo, which is defined as "Information about a QEMU image file", but this option cannot be set in the image file itself but is only a run-time option.

Second, my original goal for ImageInfoSpecific was to provide more information through qemu-img info, and this value will look pretty strange there.

Quoted 5 lines

I don't know how to resolve this quandary, though. Since qemu cannot change this interval by itself, I think not providing an interface for reading it is fine. On the other hand, if Eric finds such an interface absolutely mandatory, I can't think of a better place to return it than here.

Can we mark the parameter optional, and only provide it when it is non-zero? That way, qemu-img (which cannot set an interval) will not report it, and the only time it will appear is if it was set as part of opening the block device under qemu.

Quoted 4 lines

The only solution which would satisfy both requirements would be another structure which contains "online" flags, and thus is not evaluated by qemu-img info, but only by QMP commands. But that's ugly.

Yeah, I'm not sure such duplication helps. I'd still like it reported somewhere, though, as it is nice to query that a requested setting is actually working.

Quoted 3 lines

Yeah, I'm not sure such duplication helps. I'd still like it reported somewhere, though, as it is nice to query that a requested setting is actually working.

This isn't duplicated information. You can have ImageInfoSpecificQCow2 show lazy_refcounts=off because that is what the image file contains, but the real value in effect could be lazy_refcounts=on.