Re: [Qemu-devel] [PATCH V17 4/6] rename qcow2-cache.c to block-cache.c

[Kevin Wolf <kwolf@redhat.com>]

Am 06.12.2012 07:51, schrieb Dong Xu Wang:
> We will re-use qcow2-cache as block layer common cache code,
> so change its name and made some changes, define a struct named
> BlockTableType, pass BlockTableType and table size parameters to
> block cache initialization function.
> 
> Signed-off-by: Dong Xu Wang <wdongxu@linux.vnet.ibm.com>
> ---
>  block/Makefile.objs    |    3 +-
>  block/block-cache.c    |  317 +++++++++++++++++++++++++++++++++++++++++++++++
>  block/block-cache.h    |   76 +++++++++++
>  block/qcow2-cache.c    |  323 ------------------------------------------------
>  block/qcow2-cluster.c  |   53 ++++----
>  block/qcow2-refcount.c |   66 ++++++-----
>  block/qcow2.c          |   21 ++--
>  block/qcow2.h          |   24 +---
>  trace-events           |   13 +-
>  9 files changed, 481 insertions(+), 415 deletions(-)
>  create mode 100644 block/block-cache.c
>  create mode 100644 block/block-cache.h
>  delete mode 100644 block/qcow2-cache.c
> 
> diff --git a/block/Makefile.objs b/block/Makefile.objs
> index 7f01510..d23c250 100644
> --- a/block/Makefile.objs
> +++ b/block/Makefile.objs
> @@ -1,5 +1,6 @@
>  block-obj-y += raw.o cow.o qcow.o vdi.o vmdk.o cloop.o dmg.o bochs.o vpc.o vvfat.o
> -block-obj-y += qcow2.o qcow2-refcount.o qcow2-cluster.o qcow2-snapshot.o qcow2-cache.o
> +block-obj-y += qcow2.o qcow2-refcount.o qcow2-cluster.o qcow2-snapshot.o
> +block-obj-y += block-cache.o
>  block-obj-y += qed.o qed-gencb.o qed-l2-cache.o qed-table.o qed-cluster.o
>  block-obj-y += qed-check.o
>  block-obj-y += parallels.o blkdebug.o blkverify.o
> diff --git a/block/block-cache.c b/block/block-cache.c
> new file mode 100644
> index 0000000..bf5c57c
> --- /dev/null
> +++ b/block/block-cache.c
> @@ -0,0 +1,317 @@
> +/*
> + * QEMU Block Layer Cache
> + *
> + * Copyright IBM, Corp. 2012
> + *
> + * Authors:
> + *  Dong Xu Wang <wdongxu@linux.vnet.ibm.com>
> + *
> + * This file is based on qcow2-cache.c, see its copyrights below:
> + *
> + * L2/refcount table cache for the QCOW2 format
> + *
> + * Copyright (c) 2010 Kevin Wolf <kwolf@redhat.com>
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a copy
> + * of this software and associated documentation files (the "Software"), to deal
> + * in the Software without restriction, including without limitation the rights
> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
> + * copies of the Software, and to permit persons to whom the Software is
> + * furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
> + * THE SOFTWARE.
> + */
> +
> +#include "block_int.h"
> +#include "qemu-common.h"
> +#include "trace.h"
> +#include "block-cache.h"
> +
> +BlockCache *block_cache_create(BlockDriverState *bs, int num_tables,
> +                               size_t cluster_size, BlockTableType type)

cluster_size should probably be called table_size. It just happens that
qcow2 tables are always one cluster, but it may be different for other
formats using this implementation in the future.

> +int block_cache_put(BlockDriverState *bs, BlockCache *c, void **table)
> +{
> +    int i;
> +
> +    for (i = 0; i < c->size; i++) {
> +        if (c->entries[i].table == *table) {
> +            goto found;
> +        }
> +    }
> +    return -ENOENT;
> +
> +found:
> +    c->entries[i].ref--;
> +    assert(c->entries[i].ref >= 0);
> +    *table = NULL;
> +    return 0;
> +}

Why did you swap the assert() and *table = NULL?

> diff --git a/block/block-cache.h b/block/block-cache.h
> new file mode 100644
> index 0000000..4efa06e
> --- /dev/null
> +++ b/block/block-cache.h
> @@ -0,0 +1,76 @@
> +/*
> + * QEMU Block Layer Cache
> + *
> + * Copyright IBM, Corp. 2012
> + *
> + * Authors:
> + *  Dong Xu Wang <wdongxu@linux.vnet.ibm.com>
> + *
> + * This file is based on qcow2-cache.c, see its copyrights below:
> + *
> + * L2/refcount table cache for the QCOW2 format
> + *
> + * Copyright (c) 2010 Kevin Wolf <kwolf@redhat.com>
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a copy
> + * of this software and associated documentation files (the "Software"), to deal
> + * in the Software without restriction, including without limitation the rights
> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
> + * copies of the Software, and to permit persons to whom the Software is
> + * furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
> + * THE SOFTWARE.
> + */
> +
> +#ifndef BLOCK_CACHE_H
> +#define BLOCK_CACHE_H
> +
> +typedef enum {
> +    BLOCK_TABLE_REF,
> +    BLOCK_TABLE_L2,
> +} BlockTableType;

I'm not excited about exposing these types, but it's okay for now. We
can always change the implementation later to be nicer.

> +
> +typedef struct BlockCachedTable {
> +    void    *table;
> +    int64_t offset;
> +    bool    dirty;
> +    int     cache_hits;
> +    int     ref;
> +} BlockCachedTable;
> +
> +struct BlockCache {
> +    BlockCachedTable    *entries;
> +    struct BlockCache   *depends;
> +    int                 size;
> +    size_t              cluster_size;
> +    BlockTableType      table_type;
> +    bool                depends_on_flush;
> +};

Why have these definitions been moved to the header file? They are
supposed to be private to the implementation.

> +
> +struct BlockCache;
> +typedef struct BlockCache BlockCache;
> +
> +BlockCache *block_cache_create(BlockDriverState *bs, int num_tables,
> +                               size_t cluster_size, BlockTableType type);
> +int block_cache_destroy(BlockDriverState *bs, BlockCache *c);
> +int block_cache_flush(BlockDriverState *bs, BlockCache *c);
> +int block_cache_set_dependency(BlockDriverState *bs,
> +                               BlockCache *c,
> +                               BlockCache *dependency);
> +void block_cache_depends_on_flush(BlockCache *c);
> +int block_cache_get(BlockDriverState *bs, BlockCache *c, uint64_t offset,
> +                    void **table);
> +int block_cache_get_empty(BlockDriverState *bs, BlockCache *c,
> +                          uint64_t offset, void **table);
> +int block_cache_put(BlockDriverState *bs, BlockCache *c, void **table);
> +void block_cache_entry_mark_dirty(BlockCache *c, void *table);

Kevin