From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:37697) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Yqpn3-0006p0-3T for qemu-devel@nongnu.org; Fri, 08 May 2015 17:26:38 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Yqpn1-000508-Ra for qemu-devel@nongnu.org; Fri, 08 May 2015 17:26:37 -0400 Message-ID: <554D2A03.3080201@weilnetz.de> Date: Fri, 08 May 2015 23:26:27 +0200 From: Stefan Weil MIME-Version: 1.0 References: <1430971496-32659-1-git-send-email-phoeagon@gmail.com> <1431011818-15822-1-git-send-email-phoeagon@gmail.com> <554CB6C6.3060809@redhat.com> <20150508135512.GJ4318@noname.redhat.com> In-Reply-To: <20150508135512.GJ4318@noname.redhat.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v4] block/vdi: Use bdrv_flush after metadata updates List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf , Max Reitz Cc: Zhe Qiu , qemu-devel@nongnu.org, qemu-block@nongnu.org Am 08.05.2015 um 15:55 schrieb Kevin Wolf: > Am 08.05.2015 um 15:14 hat Max Reitz geschrieben: >> On 07.05.2015 17:16, Zhe Qiu wrote: >>> In reference to b0ad5a45...078a458e, metadata writes to >>> qcow2/cow/qcow/vpc/vmdk are all synced prior to succeeding writes. >>> >>> Only when write is successful that bdrv_flush is called. >>> >>> Signed-off-by: Zhe Qiu >>> --- >>> block/vdi.c | 3 +++ >>> 1 file changed, 3 insertions(+) >> I missed Kevin's arguments before, but I think that adding this is >> more correct than not having it; and when thinking about speed, this >> is vdi, a format supported for compatibility. > If you use it only as a convert target, you probably care more about > speed than about leaks in case of a host crash. > >> So if we wanted to optimize it, we'd probably have to cache multiple >> allocations, do them at once and then flush afterwards (like the >> metadata cache we have in qcow2?) > That would defeat the purpose of this patch which aims at having > metadata and data written out almost at the same time. On the other > hand, fully avoiding the problem instead of just making the window > smaller would require a journal, which VDI just doesn't have. > > I'm not convinced of this patch, but I'll defer to Stefan Weil as the > VDI maintainer. > > Kevin Thanks for asking. I share your concerns regarding reduced performance caused by bdrv_flush. Conversions to VDI will take longer (how much?), and also installation of an OS on a new VDI disk image will be slower because that are the typical scenarios where the disk usage grows. @phoeagon: Did the benchmark which you used allocate additional disk storage? If not or if it only allocated once and then spent some time on already allocated blocks, that benchmark was not valid for this case. On the other hand I don't see a need for the flushing because the kind of failures (power failure) and their consequences seem to be acceptable for typical VDI usage, namely either image conversion or tests with existing images. That's why I'd prefer not to use bdrv_flush here. Could we make bdrv_flush optional (either generally or for cases like this one) so both people who prefer speed and people who would want bdrv_flush to decrease the likelihood of inconsistencies can be satisfied? Stefan