From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:56039) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YYatG-0001oa-3t for qemu-devel@nongnu.org; Thu, 19 Mar 2015 09:53:39 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YYatC-0003YJ-Sx for qemu-devel@nongnu.org; Thu, 19 Mar 2015 09:53:38 -0400 Message-ID: <550AD4D6.3090507@redhat.com> Date: Thu, 19 Mar 2015 09:53:26 -0400 From: Max Reitz MIME-Version: 1.0 References: <1426768413-31079-1-git-send-email-kwolf@redhat.com> <1426768413-31079-3-git-send-email-kwolf@redhat.com> In-Reply-To: <1426768413-31079-3-git-send-email-kwolf@redhat.com> Content-Type: text/plain; charset=iso-8859-15; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v2 2/3] qemu-img convert: Rewrite copying logic List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf , qemu-block@nongnu.org Cc: pl@kamp.de, qemu-devel@nongnu.org, stefanha@redhat.com On 2015-03-19 at 08:33, Kevin Wolf wrote: > The implementation of qemu-img convert is (a) messy, (b) buggy, and > (c) less efficient than possible. The changes required to beat some > sense into it are massive enough that incremental changes would only > make my and the reviewers' life harder. So throw it away and reimplement > it from scratch. > > Let me give some examples what I mean by messy, buggy and inefficient: > > (a) The copying logic of qemu-img convert has two separate branches for > compressed and normal target images, which roughly do the same - > except for a little code that handles actual differences between > compressed and uncompressed images, and much more code that > implements just a different set of optimisations and bugs. This is > unnecessary code duplication, and makes the code for compressed > output (unsurprisingly) suffer from bitrot. > > The code for uncompressed ouput is run twice to count the the total > length for the progress bar. In the first run it just takes a > shortcut and runs only half the loop, and when it's done, it toggles > a boolean, jumps out of the loop with a backwards goto and starts > over. Works, but pretty is something different. > > (b) Converting while keeping a backing file (-B option) is broken in > several ways. This includes not writing to the image file if the > input has zero clusters or data filled with zeros (ignoring that the > backing file will be visible instead). > > It also doesn't correctly limit every iteration of the copy loop to > sectors of the same status so that too many sectors may be copied to > in the target image. For -B this gives an unexpected result, for > other images it just does more work than necessary. > > Conversion with a compressed target completely ignores any target > backing file. > > (c) qemu-img convert skips reading and writing an area if it knows from > metadata that copying isn't needed (except for the bug mentioned > above that ignores a status change in some cases). It does, however, > read from the source even if it knows that it will read zeros, and > then search for non-zero bytes in the read buffer, if it's possible > that a write might be needed. > > This reimplementation of the copying core reorganises the code to remove > the duplication and have a much more obvious code flow, by essentially > splitting the copy iteration loop into three parts: > > 1. Find the number of contiguous sectors of the same status at the > current offset (This can also be called in a separate loop before the > copying loop in order to determine the total sectors for the progress > bar.) > > 2. Read sectors. If the status implies that there is no data there to > read (zero or unallocated cluster), don't do anything. > > 3. Write sectors depending on the status. If it's data, write it. If > we want the backing file to be visible (with -B), don't write it. If > it's zeroed, skip it if you can, otherwise use bdrv_write_zeroes() to > optimise the write at least where possible. > > Signed-off-by: Kevin Wolf > --- > qemu-img.c | 516 +++++++++++++++++++++++++++++++++++++------------------------ > 1 file changed, 310 insertions(+), 206 deletions(-) Reviewed-by: Max Reitz