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

[Kevin Wolf]

Am 10.08.2012 17:39, schrieb Dong Xu Wang:
> add-cow and qcow2 file format will share the same cache code, so rename
> block-cache.c to block-cache.c. And related structure and qcow2 code also
> are changed.
> 
> Signed-off-by: Dong Xu Wang <address@hidden>
> ---
>  block.h                |    3 +
>  block/Makefile.objs    |    3 +-
>  block/qcow2-cache.c    |  323 
> ------------------------------------------------
>  block/qcow2-cluster.c  |   66 ++++++----
>  block/qcow2-refcount.c |   66 ++++++-----
>  block/qcow2.c          |   36 +++---
>  block/qcow2.h          |   24 +---
>  trace-events           |   13 +-
>  8 files changed, 109 insertions(+), 425 deletions(-)
>  delete mode 100644 block/qcow2-cache.c
> 
> diff --git a/block.h b/block.h
> index e5dfcd7..c325661 100644
> --- a/block.h
> +++ b/block.h
> @@ -401,6 +401,9 @@ typedef enum {
>      BLKDBG_CLUSTER_ALLOC_BYTES,
>      BLKDBG_CLUSTER_FREE,
>  
> +    BLKDBG_ADD_COW_UPDATE,
> +    BLKDBG_ADD_COW_LOAD,
> +

I don't think you should add new events, the existing ones should be
generic enough that you can reuse them. It's somewhat hard to see
without block-cache.c, though.

Can you make sure to have one patch with pure code motion, and a
separate one with the changes needed to make it work with add-cow? It
will help reviewers a lot.

>      BLKDBG_EVENT_MAX,
>  } BlkDebugEvent;
>  
> diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
> index e179211..335dc7a 100644
> --- a/block/qcow2-cluster.c
> +++ b/block/qcow2-cluster.c
> @@ -28,6 +28,7 @@
>  #include "block_int.h"
>  #include "block/qcow2.h"
>  #include "trace.h"
> +#include "block-cache.h"
>  
>  int qcow2_grow_l1_table(BlockDriverState *bs, int min_size, bool exact_size)
>  {
> @@ -69,7 +70,8 @@ int qcow2_grow_l1_table(BlockDriverState *bs, int min_size, 
> bool exact_size)
>          return new_l1_table_offset;
>      }
>  
> -    ret = qcow2_cache_flush(bs, s->refcount_block_cache);
> +    ret = block_cache_flush(bs, s->refcount_block_cache,
> +        BLOCK_TABLE_REF, s->cluster_size);

I think its better to pass s->cluster_size to the cache initialisation
instead of in each call of the cache function.

For the blkdebug events I guess it's possible as well to move this to
the initialisation, but I'd have to see the block-cache.c code to say
something specific about this.

> @@ -659,18 +669,16 @@ int qcow2_alloc_cluster_link_l2(BlockDriverState *bs, 
> QCowL2Meta *m)
>       * handled.
>       */
>      if (cow) {
> -        qcow2_cache_depends_on_flush(s->l2_table_cache);
> +        block_cache_depends_on_flush(s->l2_table_cache);
>      }
>  
> -    if (qcow2_need_accurate_refcounts(s)) {
> -        qcow2_cache_set_dependency(bs, s->l2_table_cache,
> -                                   s->refcount_block_cache);
> -    }
> +    block_cache_set_dependency(bs, s->l2_table_cache, BLOCK_TABLE_L2,
> +        s->refcount_block_cache, s->cluster_size);

What happened with lazy refcounting? Is this a mismerge or did you
intentionally remove the condition? (There's a second place where you do
the same)

Kevin