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

[Dong Xu Wang]

On Tue, Dec 11, 2012 at 1:11 AM, Kevin Wolf <address@hidden> wrote:
> 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 <address@hidden>
>> ---
>>  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 <address@hidden>
>> + *
>> + * 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 <address@hidden>
>> + *
>> + * 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.
>
Okay.

>> +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?
>

I will use original code here. I have no reason to swap them..

>> 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 <address@hidden>
>> + *
>> + * 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 <address@hidden>
>> + *
>> + * 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.
>
Both qcow2.h:BDRVQcowState and add-cow.c: BDRVAddCowState have a
member typed  BlockCache,
such as:
typedef struct BDRVAddCowState {
    BlockDriverState    *image_hd;
    CoMutex             lock;
    int                 cluster_size;
    BlockCache          *bitmap_cache;
    uint64_t            bitmap_size;
    AddCowHeader        header;
    char                backing_fmt[16];
    char                image_fmt[16];
} BDRVAddCowState;

So I have to move the definitions to the header file, so that both
add-cow.c and qcow2.h can use  BlockCache.
Is it Okay?
>> +
>> +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
>

Thank you Kevin.