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

[Dong Xu Wang]

On Thu, Jun 14, 2012 at 7:13 PM, Paolo Bonzini <address@hidden> wrote:
> 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
>
Okay, thanks for your review, Paolo. :)