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

[Dong Xu Wang]

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

>> +    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.
>
Okay.

>> +        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.
>
Okay.

>> +        block_cache_destroy(bs, s->bitmap_cache);
>> +    }
>> +    return ret;
>> +}
>
> header.header_size isn't checked to be 4096 as required in the spec.
>
Okay, will check in newer version.
>> +
>> +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.
>
Okay. will add cluster_secs field.

>> +    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.
>
Okay, will optimize in next version.

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

Sorry,  I have read enum BlkDebugEvent to find out which type would be
suitable for this case. In the previous
comments, you said existing DebugEvent types would be enough, do you
think which types should be picked up
for add-cow BLKDBG_EVENT?

Thank you Kevin.
>
> Kevin
>