Re: [Qemu-devel] [PATCH 1/4] qemu-img: Fix preallocation with -S 0 for convert

[Kevin Wolf]

Am 20.02.2016 um 18:39 hat Max Reitz geschrieben:
> When passing -S 0 to qemu-img convert, the target image is supposed to
> be fully allocated. Right now, this is not the case if the source image
> contains areas which bdrv_get_block_status() reports as being zero.
> 
> This patch introduces a new ImgConvertBlockStatus named BLK_ZERO_DATA
> which is set by convert_iteration_sectors() if an area is detected to be
> zero and min_sparse is 0. Simply using BLK_DATA is not optimal, because
> knowing an area to be zero allows us to memset() the buffer to be
> written directly rather than having to use blk_read().
> 
> Other than that, BLK_ZERO_DATA is handled exactly like BLK_DATA.
> 
> This patch changes the reference output for iotest 122; contrary to what
> it assumed, -S 0 really should allocate everything in the output, not
> just areas that are filled with zeros (as opposed to being zeroed).
> 
> Signed-off-by: Max Reitz <address@hidden>

I don't like how you touch the conversion code all over the place.
Specifically, convert_iteration_sectors() and convert_read() (and
consequently s->status) are supposed to be only about the source file.
-S 0 doesn't make a difference for what the source file looks like, so
we shouldn't need to change anything there.

The following change should do the same thing (it passes your test case
anyway) and is more contained to the actual writeout of image data.

Kevin


diff --git a/qemu-img.c b/qemu-img.c
index 2edb139..3b234cf 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1540,14 +1540,21 @@ static int convert_read(ImgConvertState *s, int64_t 
sector_num, int nb_sectors,
 }
 
 static int convert_write(ImgConvertState *s, int64_t sector_num, int 
nb_sectors,
-                         const uint8_t *buf)
+                         uint8_t *buf)
 {
     int ret;
 
     while (nb_sectors > 0) {
         int n = nb_sectors;
+        int status = s->status;
 
-        switch (s->status) {
+        if (!s->min_sparse && status == BLK_ZERO) {
+            n = MIN(n, s->buf_sectors);
+            memset(buf, 0, n * BDRV_SECTOR_SIZE);
+            status = BLK_DATA;
+        }
+
+        switch (status) {
         case BLK_BACKING_FILE:
             /* If we have a backing file, leave clusters unallocated that are
              * unallocated in the source image, so that the backing file is
@@ -1602,7 +1609,9 @@ static int convert_write(ImgConvertState *s, int64_t 
sector_num, int nb_sectors,
 
         sector_num += n;
         nb_sectors -= n;
-        buf += n * BDRV_SECTOR_SIZE;
+        if (s->status == BLK_DATA) {
+            buf += n * BDRV_SECTOR_SIZE;
+        }
     }
 
     return 0;