Re: [Qemu-devel] [PATCH 3/6] add-cow file format

[Paolo Bonzini <pbonzini@redhat.com>]

I just took a quick look at the flush code.

Il 13/06/2012 16:36, Dong Xu Wang ha scritto:
> 
> +bool add_cow_cache_set_writethrough(BlockDriverState *bs, AddCowCache *c,
> +    bool enable)
> +{
> +    bool old = c->writethrough;
> +
> +    if (!old && enable) {
> +        add_cow_cache_flush(bs, c);
> +    }
> +
> +    c->writethrough = enable;
> +    return old;
> +}

Writethrough mode should not be needed anymore in 1.2 if the new
implementation of writethrough is added.

And anyway...

> +    if (changed) {
> +        ret = add_cow_cache_flush(bs, s->bitmap_cache);
> +    }

... here you're treating the cache essentially as writethrough.  Is this
flush necessary?

> +    qemu_co_mutex_unlock(&s->lock);
> +    qemu_iovec_destroy(&hd_qiov);
> +    return ret;
> +}
> +
> +static int bdrv_add_cow_truncate(BlockDriverState *bs, int64_t size)
> +{
> +    BDRVAddCowState *s = bs->opaque;
> +    int sector_per_byte = SECTORS_PER_CLUSTER * 8;
> +    int ret;
> +    int64_t bitmap_size =
> +        (size / BDRV_SECTOR_SIZE + sector_per_byte - 1) / sector_per_byte;
> +
> +    ret = bdrv_truncate(bs->file,
> +        ADD_COW_BITMAP_POS + bitmap_size);
> +    if (ret < 0) {
> +        return ret;
> +    }
> +    bdrv_truncate(s->image_hd, size);
> +    return 0;
> +}
> +
> +static coroutine_fn int add_cow_co_flush(BlockDriverState *bs)
> +{
> +    BDRVAddCowState *s = bs->opaque;
> +    int ret;
> +
> +    qemu_co_mutex_lock(&s->lock);
> +    ret = add_cow_cache_flush(bs, s->bitmap_cache);
> +    qemu_co_mutex_unlock(&s->lock);
> +    if (ret < 0) {
> +        return ret;
> +    }
> +
> +    return bdrv_co_flush(bs->file);

Flushing bs->file here is not needed anymore...

> +}
> +
> +static QEMUOptionParameter add_cow_create_options[] = {
> +    {
> +        .name = BLOCK_OPT_SIZE,
> +        .type = OPT_SIZE,
> +        .help = "Virtual disk size"
> +    },
> +    {
> +        .name = BLOCK_OPT_BACKING_FILE,
> +        .type = OPT_STRING,
> +        .help = "File name of a base image"
> +    },
> +    {
> +        .name = BLOCK_OPT_IMAGE_FILE,
> +        .type = OPT_STRING,
> +        .help = "File name of a image file"
> +    },
> +    {
> +        .name = BLOCK_OPT_BACKING_FMT,
> +        .type = OPT_STRING,
> +        .help = "Image format of the base image"
> +    },
> +    { NULL }
> +};
> +
> +static BlockDriver bdrv_add_cow = {
> +    .format_name                = "add-cow",
> +    .instance_size              = sizeof(BDRVAddCowState),
> +    .bdrv_probe                 = add_cow_probe,
> +    .bdrv_open                  = add_cow_open,
> +    .bdrv_close                 = add_cow_close,
> +    .bdrv_create                = add_cow_create,
> +    .bdrv_co_readv              = add_cow_co_readv,
> +    .bdrv_co_writev             = add_cow_co_writev,
> +    .bdrv_truncate              = bdrv_add_cow_truncate,
> +    .bdrv_co_is_allocated       = add_cow_is_allocated,
> +
> +    .create_options             = add_cow_create_options,
> +    .bdrv_co_flush_to_disk      = add_cow_co_flush,

... and this should be bdrv_co_flush_to_os.

Paolo