Re: [Qemu-devel] [Qemu-block] [PATCH v2] qcow2: Release dirty entries with cache-clean-interval

[John Snow]

On 08/09/2018 09:44 AM, Alberto Garcia wrote:
> The cache-clean-interval option is used to periodically release unused
> entries from the L2 and refcount caches. Dirty cache entries are left
> untouched, even if they are otherwise valid candidates for removal.
> 
> This patch allows releasing those entries by flushing them to disk
> first. This is a blocking operation, so we need to do it in coroutine
> context.
> 
> Signed-off-by: Alberto Garcia <address@hidden>

This hit over a month old today, do we still want this for 3.1?
--js

> ---
>  block/qcow2-cache.c | 26 ++++++++++++++++++++------
>  block/qcow2.c       | 15 ++++++++++++---
>  block/qcow2.h       |  2 +-
>  3 files changed, 33 insertions(+), 10 deletions(-)
> 
> diff --git a/block/qcow2-cache.c b/block/qcow2-cache.c
> index d9dafa31e5..33207eca9b 100644
> --- a/block/qcow2-cache.c
> +++ b/block/qcow2-cache.c
> @@ -46,6 +46,8 @@ struct Qcow2Cache {
>      uint64_t                cache_clean_lru_counter;
>  };
>  
> +static int qcow2_cache_entry_flush(BlockDriverState *bs, Qcow2Cache *c, int 
> i);
> +
>  static inline void *qcow2_cache_get_table_addr(Qcow2Cache *c, int table)
>  {
>      return (uint8_t *) c->table_array + (size_t) table * c->table_size;
> @@ -86,26 +88,36 @@ static void qcow2_cache_table_release(Qcow2Cache *c, int 
> i, int num_tables)
>  #endif
>  }
>  
> -static inline bool can_clean_entry(Qcow2Cache *c, int i)
> +static inline bool can_clean_entry(BlockDriverState *bs, 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;
> +    if (t->ref || !t->offset || t->lru_counter > c->cache_clean_lru_counter) 
> {
> +        return false;
> +    }
> +
> +    if (qcow2_cache_entry_flush(bs, c, i) < 0) {
> +        return false;
> +    }
> +
> +    return true;
>  }
>  
> -void qcow2_cache_clean_unused(Qcow2Cache *c)
> +void qcow2_cache_clean_unused(BlockDriverState *bs, Qcow2Cache *c)
>  {
>      int i = 0;
> +
> +    bdrv_inc_in_flight(bs);
> +
>      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)) {
> +        while (i < c->size && !can_clean_entry(bs, c, i)) {
>              i++;
>          }
>  
>          /* And count how many we can clean in a row */
> -        while (i < c->size && can_clean_entry(c, i)) {
> +        while (i < c->size && can_clean_entry(bs, c, i)) {
>              c->entries[i].offset = 0;
>              c->entries[i].lru_counter = 0;
>              i++;
> @@ -118,6 +130,8 @@ void qcow2_cache_clean_unused(Qcow2Cache *c)
>      }
>  
>      c->cache_clean_lru_counter = c->lru_counter;
> +
> +    bdrv_dec_in_flight(bs);
>  }
>  
>  Qcow2Cache *qcow2_cache_create(BlockDriverState *bs, int num_tables,
> diff --git a/block/qcow2.c b/block/qcow2.c
> index ec9e6238a0..2fcb41b12f 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -728,16 +728,25 @@ static const char 
> *overlap_bool_option_names[QCOW2_OL_MAX_BITNR] = {
>      [QCOW2_OL_BITMAP_DIRECTORY_BITNR] = QCOW2_OPT_OVERLAP_BITMAP_DIRECTORY,
>  };
>  
> -static void cache_clean_timer_cb(void *opaque)
> +static void coroutine_fn cache_co_clean_unused(void *opaque)
>  {
>      BlockDriverState *bs = opaque;
>      BDRVQcow2State *s = bs->opaque;
> -    qcow2_cache_clean_unused(s->l2_table_cache);
> -    qcow2_cache_clean_unused(s->refcount_block_cache);
> +
> +    qemu_co_mutex_lock(&s->lock);
> +    qcow2_cache_clean_unused(bs, s->l2_table_cache);
> +    qcow2_cache_clean_unused(bs, s->refcount_block_cache);
> +    qemu_co_mutex_unlock(&s->lock);
> +
>      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_cb(void *opaque)
> +{
> +    qemu_coroutine_enter(qemu_coroutine_create(cache_co_clean_unused, 
> opaque));
> +}
> +
>  static void cache_clean_timer_init(BlockDriverState *bs, AioContext *context)
>  {
>      BDRVQcow2State *s = bs->opaque;
> diff --git a/block/qcow2.h b/block/qcow2.h
> index 81b844e936..e8b390ba4b 100644
> --- a/block/qcow2.h
> +++ b/block/qcow2.h
> @@ -659,7 +659,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(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,
>