From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:37800) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Sf7z9-0004Jm-O1 for qemu-devel@nongnu.org; Thu, 14 Jun 2012 07:13:13 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Sf7z7-0008Ug-Pr for qemu-devel@nongnu.org; Thu, 14 Jun 2012 07:13:07 -0400 Received: from mail-we0-f173.google.com ([74.125.82.173]:57325) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Sf7z7-0008UA-Gi for qemu-devel@nongnu.org; Thu, 14 Jun 2012 07:13:05 -0400 Received: by werf3 with SMTP id f3so1331354wer.4 for ; Thu, 14 Jun 2012 04:13:03 -0700 (PDT) Sender: Paolo Bonzini Message-ID: <4FD9C73D.4050701@redhat.com> Date: Thu, 14 Jun 2012 13:13:01 +0200 From: Paolo Bonzini MIME-Version: 1.0 References: <1339598189-17933-1-git-send-email-wdongxu@linux.vnet.ibm.com> <1339598189-17933-3-git-send-email-wdongxu@linux.vnet.ibm.com> In-Reply-To: <1339598189-17933-3-git-send-email-wdongxu@linux.vnet.ibm.com> Content-Type: text/plain; charset=ISO-8859-15 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 3/6] add-cow file format List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Dong Xu Wang Cc: kwolf@redhat.com, qemu-devel@nongnu.org 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