From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:46100) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Ti6tF-0002Ig-3H for qemu-devel@nongnu.org; Mon, 10 Dec 2012 12:11:47 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Ti6t9-0006KG-Qo for qemu-devel@nongnu.org; Mon, 10 Dec 2012 12:11:37 -0500 Received: from mx1.redhat.com ([209.132.183.28]:39922) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Ti6t9-0006KB-Iw for qemu-devel@nongnu.org; Mon, 10 Dec 2012 12:11:31 -0500 Message-ID: <50C617B6.2050200@redhat.com> Date: Mon, 10 Dec 2012 18:11:18 +0100 From: Kevin Wolf MIME-Version: 1.0 References: <1354776711-12449-1-git-send-email-wdongxu@linux.vnet.ibm.com> <1354776711-12449-5-git-send-email-wdongxu@linux.vnet.ibm.com> In-Reply-To: <1354776711-12449-5-git-send-email-wdongxu@linux.vnet.ibm.com> Content-Type: text/plain; charset=ISO-8859-15 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH V17 4/6] rename qcow2-cache.c to block-cache.c List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Dong Xu Wang Cc: qemu-devel@nongnu.org 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 > --- > 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 > + * > + * 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 > + * > + * 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 > + * > + * 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 > + * > + * 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