[Qemu-devel] [PATCH 0/2] qemu-img: Special post-backing convert handle

[Qemu-devel] [PATCH 0/2] qemu-img: Special post-backing convert handle

[Max Reitz]

This is the issue:

$ qemu-img create -f qcow2 base.qcow2 512M
Formatting 'base.qcow2', fmt=qcow2 size=536870912 cluster_size=65536 lazy_refcounts=off refcount_bits=16
$ qemu-img create -f qcow2 -b base.qcow2 source.qcow2 1G
Formatting 'source.qcow2', fmt=qcow2 size=1073741824 backing_file=base.qcow2 cluster_size=65536 lazy_refcounts=off refcount_bits=16
$ qemu-img convert -O qcow2 -B base.qcow2 -o compat=0.10 \
    source.qcow2 target.qcow2
$ qemu-img info target.qcow2
image: target.qcow2
file format: qcow2
virtual size: 1.0G (1073741824 bytes)
disk size: 512M    <-------------------- this here
cluster_size: 65536
backing file: base.qcow2
Format specific information:
    compat: 0.10
    refcount bits: 16
$ qemu-img map target.qcow2
Offset          Length          Mapped to       File
0x20000000      0x20000000      0x50000         target.qcow2

So qemu-img convert sees that source.qcow2 contains only zeroes past the
end of base.qcow2 -- but that is not the backing file, so it will
explicitly write zeroes to target.qcow2.  But that file is compat=0.10,
so it does not support efficient zero writes and will actually fill that
area with real zeroes.  Hence the mapping, hence the disk size.

However, we don't need to write zeroes to an image when it is
initialized to zeroes -- which the current qemu-img code says doesn't
happen for target images with backing files.  But it does happen when
the target backing file is shorter than the target image, then the area
past the end of the backing file may indeed read as zeroes and we don't
need to write anything there.

So the first patch in this series makes qemu-img convert detect that
case and handle zeroes past the end of the backing file as "fall back to
the backing file" (because that means zeroes, at least if unallocated
areas read as zeroes (which I presume they have to for formats
supporting backing files, but better be safe than sorry)), and the
second adds an iotest.


Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1527898


Max Reitz (2):
  qemu-img: Special post-backing convert handling
  iotests: Test post-backing convert target behavior

 qemu-img.c                 | 26 +++++++++++++++++++++++++-
 tests/qemu-iotests/122     | 42 ++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/122.out | 18 ++++++++++++++++++
 3 files changed, 85 insertions(+), 1 deletion(-)

-- 
2.14.3

[Qemu-devel] [PATCH 1/2] qemu-img: Special post-backing convert handling

[Max Reitz]

Currently, qemu-img convert writes zeroes when it reads zeroes.
Sometimes it does not because the target is initialized to zeroes
anyway, so we do not need to overwrite (and thus potentially allocate)
it.  This is never the case for targets with backing files, though.  But
even them may have an area that is initialized to zeroes, and that is
the area past the end of the backing file (if that is shorter than the
overlay).

So if the target format's unallocated blocks are zero and there is a gap
between the target's backing file's end and the target's end, we do not
have to explicitly write zeroes there.

Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1527898
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 qemu-img.c | 26 +++++++++++++++++++++++++-
 1 file changed, 25 insertions(+), 1 deletion(-)

diff --git a/qemu-img.c b/qemu-img.c
index 855fa52514..5365afbd13 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1542,7 +1542,9 @@ typedef struct ImgConvertState {
     BlockBackend *target;
     bool has_zero_init;
     bool compressed;
+    bool unallocated_blocks_are_zero;
     bool target_has_backing;
+    int64_t target_backing_sectors; /* negative if unknown */
     bool wr_in_order;
     int min_sparse;
     size_t cluster_sectors;
@@ -1571,12 +1573,23 @@ static int convert_iteration_sectors(ImgConvertState *s, int64_t sector_num)
 {
     int64_t src_cur_offset;
     int ret, n, src_cur;
+    bool post_backing_zero = false;
 
     convert_select_part(s, sector_num, &src_cur, &src_cur_offset);
 
     assert(s->total_sectors > sector_num);
     n = MIN(s->total_sectors - sector_num, BDRV_REQUEST_MAX_SECTORS);
 
+    if (s->target_backing_sectors >= 0) {
+        if (sector_num >= s->target_backing_sectors) {
+            post_backing_zero = s->unallocated_blocks_are_zero;
+        } else if (sector_num + n > s->target_backing_sectors) {
+            /* Split requests around target_backing_sectors (because
+             * starting from there, zeros are handled differently) */
+            n = s->target_backing_sectors - sector_num;
+        }
+    }
+
     if (s->sector_next_status <= sector_num) {
         int64_t count = n * BDRV_SECTOR_SIZE;
 
@@ -1598,7 +1611,7 @@ static int convert_iteration_sectors(ImgConvertState *s, int64_t sector_num)
         n = DIV_ROUND_UP(count, BDRV_SECTOR_SIZE);
 
         if (ret & BDRV_BLOCK_ZERO) {
-            s->status = BLK_ZERO;
+            s->status = post_backing_zero ? BLK_BACKING_FILE : BLK_ZERO;
         } else if (ret & BDRV_BLOCK_DATA) {
             s->status = BLK_DATA;
         } else {
@@ -2319,6 +2332,16 @@ static int img_convert(int argc, char **argv)
         }
     }
 
+    if (s.target_has_backing) {
+        /* Errors are treated as "backing length unknown" (which means
+         * s.target_backing_sectors has to be negative, which it will
+         * be automatically).  The backing file length is used only
+         * for optimizations, so such a case is not fatal. */
+        s.target_backing_sectors = bdrv_nb_sectors(out_bs->backing->bs);
+    } else {
+        s.target_backing_sectors = -1;
+    }
+
     ret = bdrv_get_info(out_bs, &bdi);
     if (ret < 0) {
         if (s.compressed) {
@@ -2328,6 +2351,7 @@ static int img_convert(int argc, char **argv)
     } else {
         s.compressed = s.compressed || bdi.needs_compressed_writes;
         s.cluster_sectors = bdi.cluster_size / BDRV_SECTOR_SIZE;
+        s.unallocated_blocks_are_zero = bdi.unallocated_blocks_are_zero;
     }
 
     ret = convert_do_copy(&s);
-- 
2.14.3

[Qemu-devel] [PATCH 2/2] iotests: Test post-backing convert target behavior

[Max Reitz]

This adds a test case to 122 for what happens when you convert to a
target with a backing file that is shorter than the target, and the
image format does not support efficient zero writes (as is the case with
qcow2 v2).

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 tests/qemu-iotests/122     | 42 ++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/122.out | 18 ++++++++++++++++++
 2 files changed, 60 insertions(+)

diff --git a/tests/qemu-iotests/122 b/tests/qemu-iotests/122
index 45b359c2ba..d8c8ad722d 100755
--- a/tests/qemu-iotests/122
+++ b/tests/qemu-iotests/122
@@ -76,6 +76,48 @@ $QEMU_IMG convert -O $IMGFMT -c -B "$TEST_IMG".base "$TEST_IMG" "$TEST_IMG".orig
 $QEMU_IO -c "read -P 0 0 3M" "$TEST_IMG".orig 2>&1 | _filter_qemu_io | _filter_testdir
 
 
+echo
+echo "=== Converting to an overlay larger than its backing file ==="
+echo
+
+TEST_IMG="$TEST_IMG".base _make_test_img 256M
+# Needs to be at least how much an L2 table covers
+# (64 kB/entry * 64 kB / 8 B/entry = 512 MB)
+# That way, qcow2 will yield at least two status request responses.
+# With just a single response, it would always say "Allocated in the
+# backing file", so the optimization qemu-img convert tries to do is
+# done automatically.  Once it has to be queried twice, however (and
+# one of the queries is completely after the end of the backing file),
+# the block layer will automatically add a ZERO flag that qemu-img
+# convert used to follow up with a zero write to the target.
+# We do not want such a zero write, however, because we are past the
+# end of the backing file on the target as well, so we do not need to
+# write anything there.
+_make_test_img -b "$TEST_IMG".base 768M
+
+# Use compat=0.10 as the output so there is no zero cluster support
+$QEMU_IMG convert -O $IMGFMT -B "$TEST_IMG".base -o compat=0.10 \
+    "$TEST_IMG" "$TEST_IMG".orig
+# See that nothing has been allocated past 64M
+$QEMU_IMG map "$TEST_IMG".orig | _filter_qemu_img_map
+
+echo
+
+# Just before the end of the backing file
+$QEMU_IO -c 'write -P 0x11 255M 1M' "$TEST_IMG".base 2>&1 | _filter_qemu_io
+# Somewhere in the second L2 table
+$QEMU_IO -c 'write -P 0x22 600M 1M' "$TEST_IMG" 2>&1 | _filter_qemu_io
+
+$QEMU_IMG convert -O $IMGFMT -B "$TEST_IMG".base -o compat=0.10 \
+    "$TEST_IMG" "$TEST_IMG".orig
+
+$QEMU_IMG map "$TEST_IMG".orig | _filter_qemu_img_map
+$QEMU_IO -c 'read -P 0x11 255M 1M' \
+         -c 'read -P 0x22 600M 1M' \
+         "$TEST_IMG".orig \
+    | _filter_qemu_io
+
+
 echo
 echo "=== Concatenate multiple source images ==="
 echo
diff --git a/tests/qemu-iotests/122.out b/tests/qemu-iotests/122.out
index 47d8656db8..6c7ee1da6c 100644
--- a/tests/qemu-iotests/122.out
+++ b/tests/qemu-iotests/122.out
@@ -28,6 +28,24 @@ read 3145728/3145728 bytes at offset 0
 read 3145728/3145728 bytes at offset 0
 3 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 
+=== Converting to an overlay larger than its backing file ===
+
+Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=268435456
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=805306368 backing_file=TEST_DIR/t.IMGFMT.base
+Offset          Length          File
+
+wrote 1048576/1048576 bytes at offset 267386880
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+wrote 1048576/1048576 bytes at offset 629145600
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+Offset          Length          File
+0xff00000       0x100000        TEST_DIR/t.IMGFMT.base
+0x25800000      0x100000        TEST_DIR/t.IMGFMT.orig
+read 1048576/1048576 bytes at offset 267386880
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 1048576/1048576 bytes at offset 629145600
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
 === Concatenate multiple source images ===
 
 Formatting 'TEST_DIR/t.IMGFMT.1', fmt=IMGFMT size=4194304
-- 
2.14.3

Re: [Qemu-devel] [PATCH 1/2] qemu-img: Special post-backing convert handling

[Eric Blake]

On 05/01/2018 11:57 AM, Max Reitz wrote:
> Currently, qemu-img convert writes zeroes when it reads zeroes.
> Sometimes it does not because the target is initialized to zeroes
> anyway, so we do not need to overwrite (and thus potentially allocate)
> it.  This is never the case for targets with backing files, though.  But
> even them may have an area that is initialized to zeroes, and that is

s/them/they/

> the area past the end of the backing file (if that is shorter than the
> overlay).
> 
> So if the target format's unallocated blocks are zero and there is a gap
> between the target's backing file's end and the target's end, we do not
> have to explicitly write zeroes there.
> 
> Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1527898
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>   qemu-img.c | 26 +++++++++++++++++++++++++-
>   1 file changed, 25 insertions(+), 1 deletion(-)
> 

Some long variable names, but it aids legibility.  Nice fix.

Reviewed-by: Eric Blake <eblake@redhat.com>

(And here's hoping that Red Hat someday flips the switch so that new 
images avoid compat=0.10 in their downstream builds, as we've had quite 
a few corner cases where we already have optimized speed for compat=1.1 ...)

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH 2/2] iotests: Test post-backing convert target behavior

[Eric Blake]

On 05/01/2018 11:57 AM, Max Reitz wrote:
> This adds a test case to 122 for what happens when you convert to a
> target with a backing file that is shorter than the target, and the
> image format does not support efficient zero writes (as is the case with
> qcow2 v2).
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>   tests/qemu-iotests/122     | 42 ++++++++++++++++++++++++++++++++++++++++++
>   tests/qemu-iotests/122.out | 18 ++++++++++++++++++
>   2 files changed, 60 insertions(+)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH 0/2] qemu-img: Special post-backing convert handle

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 2613 bytes --]

On 2018-05-01 18:57, Max Reitz wrote:
> This is the issue:
> 
> $ qemu-img create -f qcow2 base.qcow2 512M
> Formatting 'base.qcow2', fmt=qcow2 size=536870912 cluster_size=65536 lazy_refcounts=off refcount_bits=16
> $ qemu-img create -f qcow2 -b base.qcow2 source.qcow2 1G
> Formatting 'source.qcow2', fmt=qcow2 size=1073741824 backing_file=base.qcow2 cluster_size=65536 lazy_refcounts=off refcount_bits=16
> $ qemu-img convert -O qcow2 -B base.qcow2 -o compat=0.10 \
>     source.qcow2 target.qcow2
> $ qemu-img info target.qcow2
> image: target.qcow2
> file format: qcow2
> virtual size: 1.0G (1073741824 bytes)
> disk size: 512M    <-------------------- this here
> cluster_size: 65536
> backing file: base.qcow2
> Format specific information:
>     compat: 0.10
>     refcount bits: 16
> $ qemu-img map target.qcow2
> Offset          Length          Mapped to       File
> 0x20000000      0x20000000      0x50000         target.qcow2
> 
> So qemu-img convert sees that source.qcow2 contains only zeroes past the
> end of base.qcow2 -- but that is not the backing file, so it will
> explicitly write zeroes to target.qcow2.  But that file is compat=0.10,
> so it does not support efficient zero writes and will actually fill that
> area with real zeroes.  Hence the mapping, hence the disk size.
> 
> However, we don't need to write zeroes to an image when it is
> initialized to zeroes -- which the current qemu-img code says doesn't
> happen for target images with backing files.  But it does happen when
> the target backing file is shorter than the target image, then the area
> past the end of the backing file may indeed read as zeroes and we don't
> need to write anything there.
> 
> So the first patch in this series makes qemu-img convert detect that
> case and handle zeroes past the end of the backing file as "fall back to
> the backing file" (because that means zeroes, at least if unallocated
> areas read as zeroes (which I presume they have to for formats
> supporting backing files, but better be safe than sorry)), and the
> second adds an iotest.
> 
> 
> Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1527898
> 
> 
> Max Reitz (2):
>   qemu-img: Special post-backing convert handling
>   iotests: Test post-backing convert target behavior
> 
>  qemu-img.c                 | 26 +++++++++++++++++++++++++-
>  tests/qemu-iotests/122     | 42 ++++++++++++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/122.out | 18 ++++++++++++++++++
>  3 files changed, 85 insertions(+), 1 deletion(-)

Applied to my block branch.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]