Re: [Qemu-devel] [PATCH v5 2/4] qcow2: add qcow2_cache_discard

[Kevin Wolf <kwolf@redhat.com>]

Am 12.07.2017 um 13:46 hat Pavel Butsykin geschrieben:
> Whenever l2/refcount table clusters are discarded from the file we can
> automatically drop unnecessary content of the cache tables. This reduces
> the chance of eviction useful cache data and eliminates inconsistent data
> in the cache with the data in the file.
> 
> Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com>
> Reviewed-by: Max Reitz <mreitz@redhat.com>
> ---
>  block/qcow2-cache.c    | 26 ++++++++++++++++++++++++++
>  block/qcow2-refcount.c | 14 ++++++++++++++
>  block/qcow2.h          |  3 +++
>  3 files changed, 43 insertions(+)
> 
> diff --git a/block/qcow2-cache.c b/block/qcow2-cache.c
> index 1d25147392..75746a7f43 100644
> --- a/block/qcow2-cache.c
> +++ b/block/qcow2-cache.c
> @@ -411,3 +411,29 @@ void qcow2_cache_entry_mark_dirty(BlockDriverState *bs, Qcow2Cache *c,
>      assert(c->entries[i].offset != 0);
>      c->entries[i].dirty = true;
>  }
> +
> +void *qcow2_cache_is_table_offset(BlockDriverState *bs, Qcow2Cache *c,
> +                                  uint64_t offset)
> +{
> +    int i;
> +
> +    for (i = 0; i < c->size; i++) {
> +        if (c->entries[i].offset == offset) {
> +            return qcow2_cache_get_table_addr(bs, c, i);
> +        }
> +    }
> +    return NULL;
> +}
> +
> +void qcow2_cache_discard(BlockDriverState *bs, Qcow2Cache *c, void *table)
> +{
> +    int i = qcow2_cache_get_table_idx(bs, c, table);
> +
> +    assert(c->entries[i].ref == 0);
> +
> +    c->entries[i].offset = 0;
> +    c->entries[i].lru_counter = 0;
> +    c->entries[i].dirty = false;
> +
> +    qcow2_cache_table_release(bs, c, i, 1);
> +}
> diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
> index c9b0dcb4f3..8050db4544 100644
> --- a/block/qcow2-refcount.c
> +++ b/block/qcow2-refcount.c
> @@ -862,6 +862,20 @@ static int QEMU_WARN_UNUSED_RESULT update_refcount(BlockDriverState *bs,
>          s->set_refcount(refcount_block, block_index, refcount);
>  
>          if (refcount == 0 && s->discard_passthrough[type]) {
> +            void *table;
> +
> +            table = qcow2_cache_is_table_offset(bs, s->refcount_block_cache,
> +                                                offset);
> +            if (table != NULL) {
> +                qcow2_cache_put(bs, s->refcount_block_cache, &refcount_block);
> +                qcow2_cache_discard(bs, s->refcount_block_cache, table);
> +            }
> +
> +            table = qcow2_cache_is_table_offset(bs, s->l2_table_cache, offset);
> +            if (table != NULL) {
> +                qcow2_cache_discard(bs, s->l2_table_cache, table);
> +            }
> +
>              update_refcount_discard(bs, cluster_offset, s->cluster_size);
>          }
>      }

This is not wrong, but wouldn't actually even refcount == 0 be enough as
a condition to evict the cluster from the cache? I don't think it really
matters whether we actually send a discard request or not for the image
file.

Kevin