On Thu, Jun 14, 2012 at 7:13 PM, Paolo Bonzini <pbonz...@redhat.com> 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. :)
