Re: [Qemu-devel] [PATCH V17 5/6] add-cow file format core code.

[Kevin Wolf <kwolf@redhat.com>]

Am 06.12.2012 07:51, schrieb Dong Xu Wang:
> add-cow file format core code. It use block-cache.c as cache code.
> It lacks of snapshot_blkdev support.
> 
> v16->v17:
> 1) Use stringify.
> 
> v15->v16:
> 1) Judge if opts is null in add_cow_create function.
> 
> Signed-off-by: Dong Xu Wang <wdongxu@linux.vnet.ibm.com>
> ---
>  block/Makefile.objs |    1 +
>  block/add-cow.c     |  690 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  block/add-cow.h     |   83 ++++++
>  block/block-cache.c |    4 +
>  block/block-cache.h |    1 +
>  block_int.h         |    2 +
>  6 files changed, 781 insertions(+), 0 deletions(-)
>  create mode 100644 block/add-cow.c
>  create mode 100644 block/add-cow.h
> 
> diff --git a/block/Makefile.objs b/block/Makefile.objs
> index d23c250..f0ff067 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
> +block-obj-y += add-cow.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
> diff --git a/block/add-cow.c b/block/add-cow.c
> new file mode 100644
> index 0000000..8cd7305
> --- /dev/null
> +++ b/block/add-cow.c
> @@ -0,0 +1,690 @@
> +/*
> + * QEMU ADD-COW Disk Format
> + *
> + * Copyright IBM, Corp. 2012
> + *
> + * Authors:
> + *  Dong Xu Wang <wdongxu@linux.vnet.ibm.com>
> + *
> + * This work is licensed under the terms of the GNU LGPL, version 2 or later.
> + * See the COPYING.LIB file in the top-level directory.
> + *
> + */
> +
> +#include "qemu-common.h"
> +#include "block_int.h"
> +#include "module.h"
> +#include "add-cow.h"

You have only this source file, so there's no need to have a header.

> +    s->image_hd = bdrv_new("");
> +    if (path_has_protocol(tmp_name)) {
> +        pstrcpy(image_filename, sizeof(image_filename), tmp_name);
> +    } else {
> +        path_combine(image_filename, sizeof(image_filename),
> +                     bs->filename, tmp_name);
> +    }
> +    bdrv_get_full_backing_filename(bs, backing_filename,
> +                                   sizeof(backing_filename));
> +    if (!strcmp(backing_filename, image_filename)) {
> +        fprintf(stderr, "Error: backing file and image file are same: %s\n",
> +                backing_filename);

error_report().

Also, s->image_hd is leaked.

> +        ret = EINVAL;
> +        goto fail;
> +    }
> +    ret = bdrv_open(s->image_hd, image_filename, flags,
> +                    bdrv_find_format(s->header.image_fmt));
> +    if (ret < 0) {
> +        bdrv_delete(s->image_hd);
> +        goto fail;
> +    }
> +
> +    bs->total_sectors = bdrv_getlength(s->image_hd) / BDRV_SECTOR_SIZE;
> +    s->cluster_size = 1 << s->header.cluster_bits;
> +    sector_per_byte = (s->cluster_size / BDRV_SECTOR_SIZE) * 8;
> +    s->bitmap_size =
> +        (bs->total_sectors + sector_per_byte - 1) / sector_per_byte;
> +    s->bitmap_cache =  block_cache_create(bs, ADD_COW_CACHE_SIZE,
> +                                          ADD_COW_CACHE_ENTRY_SIZE,
> +                                          BLOCK_TABLE_BITMAP);
> +    qemu_co_mutex_init(&s->lock);
> +    return 0;
> +fail:
> +    if (s->bitmap_cache) {

This is never true.

> +        block_cache_destroy(bs, s->bitmap_cache);
> +    }
> +    return ret;
> +}

header.header_size isn't checked to be 4096 as required in the spec.

> +
> +static void add_cow_close(BlockDriverState *bs)
> +{
> +    BDRVAddCowState *s = bs->opaque;
> +    if (s->bitmap_cache) {
> +        block_cache_destroy(bs, s->bitmap_cache);
> +    }
> +    bdrv_delete(s->image_hd);
> +}
> +
> +static bool is_allocated(BlockDriverState *bs, int64_t sector_num)
> +{
> +    BDRVAddCowState *s  = bs->opaque;
> +    BlockCache *c = s->bitmap_cache;
> +    int64_t cluster_num = sector_num / (s->cluster_size / BDRV_SECTOR_SIZE);

s->cluster_size / BDRV_SECTOR_SIZE is so common that it could be worth
having another field s->cluster_secs like in qcow2.

> +    uint8_t *table      = NULL;
> +    bool val = false;
> +    int ret;
> +
> +    uint64_t offset = s->header.header_size
> +        + (offset_in_bitmap(sector_num, s->cluster_size / BDRV_SECTOR_SIZE)
> +            & (~(c->cluster_size - 1)));

It becomes very obvious here that your choice of the image format layout
means that all bitmap blocks are not aligned to cluster boundaries, but
offset by header_size.

This isn't necessarily a big problem, but I'm not sure if it's a good
idea either.

> +    ret = block_cache_get(bs, s->bitmap_cache, offset, (void **)&table);
> +    if (ret < 0) {
> +        return ret;
> +    }
> +
> +    val = table[cluster_num / 8 % ADD_COW_CACHE_ENTRY_SIZE]
> +        & (1 << (cluster_num % 8));

Here you calculate the array index using ADD_COW_CACHE_ENTRY_SIZE, above
you select the offset of the table based on the cluster size instead.
One of both is wrong.

You should give add-cow some testing with non-standard cluster sizes.

> +    ret = block_cache_put(bs, s->bitmap_cache, (void **)&table);
> +    if (ret < 0) {
> +        return ret;
> +    }
> +    return val;
> +}

> +static coroutine_fn int add_cow_co_writev(BlockDriverState *bs,
> +                                          int64_t sector_num,
> +                                          int remaining_sectors,
> +                                          QEMUIOVector *qiov)
> +{
> +    BDRVAddCowState *s = bs->opaque;
> +    BlockCache *c = s->bitmap_cache;
> +    int ret = 0, i;
> +    QEMUIOVector hd_qiov;
> +    uint8_t *table;
> +    uint64_t offset;
> +    int mask = (s->cluster_size / BDRV_SECTOR_SIZE) - 1;
> +    int cluster_mask = c->cluster_size - 1;
> +    int64_t sector_per_cluster = s->cluster_size / BDRV_SECTOR_SIZE;
> +
> +    qemu_co_mutex_lock(&s->lock);
> +    qemu_iovec_init(&hd_qiov, qiov->niov);
> +    ret = bdrv_co_writev(s->image_hd, sector_num,
> +                         remaining_sectors, qiov);

So you hold s->lock during all writes, not only during cluster
allocation. I'm almost sure that this will hurt performance a lot.

> index bf5c57c..1a30462 100644
> --- a/block/block-cache.c
> +++ b/block/block-cache.c
> @@ -112,6 +112,8 @@ static int block_cache_entry_flush(BlockDriverState *bs, BlockCache *c, int i)
>          BLKDBG_EVENT(bs->file, BLKDBG_REFBLOCK_UPDATE_PART);
>      } else if (c->table_type == BLOCK_TABLE_L2) {
>          BLKDBG_EVENT(bs->file, BLKDBG_L2_UPDATE);
> +    } else if (c->table_type == BLOCK_TABLE_BITMAP) {
> +        BLKDBG_EVENT(bs->file, BLKDBG_COW_WRITE);
>      }
>  
>      ret = bdrv_pwrite(bs->file, c->entries[i].offset,
> @@ -245,6 +247,8 @@ static int block_cache_do_get(BlockDriverState *bs, BlockCache *c,
>      if (read_from_disk) {
>          if (c->table_type == BLOCK_TABLE_L2) {
>              BLKDBG_EVENT(bs->file, BLKDBG_L2_LOAD);
> +        } else if (c->table_type == BLOCK_TABLE_BITMAP) {
> +            BLKDBG_EVENT(bs->file, BLKDBG_COW_READ);
>          }

I must admit that I don't like this table_type stuff at all, even more
so if every new format adds new types to it. Not sure what to suggest here.

But anyway, even if we leave it, aren't BLKDBG_COW_READ/WRITE something
completely different?

Kevin