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

[Kevin Wolf <kwolf@redhat.com>]

Am 06.08.2018 um 16:13 hat Alberto Garcia geschrieben:
> 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.
> 
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> ---
>  block/qcow2-cache.c | 21 +++++++++++++++------
>  block/qcow2.c       |  4 ++--
>  block/qcow2.h       |  2 +-
>  3 files changed, 18 insertions(+), 9 deletions(-)
> 
> diff --git a/block/qcow2-cache.c b/block/qcow2-cache.c
> index d9dafa31e5..0812164e46 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,33 @@ 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;
> +    }

We're not in coroutine context here, so qcow2_cache_entry_flush() will
be blocking. I don't think that's acceptable in a timer callback.

On the other hand, if we made it non-blocking by moving it into a
coroutine that could yield, we would have to consider races with other
parts of the code and at least take s->lock and implement
.bdrv_co_drain_begin/end callbacks.

Kevin