[PATCH for-4.2? 0/6] block: Fix resize (extending) of short overlays

[PATCH for-4.2? 0/6] block: Fix resize (extending) of short overlays

[Kevin Wolf]

Consider the following scenario where the overlay is shorter than its
backing file:

    base.qcow2:     AAAAAAAA
    overlay.qcow2:  BBBB

When resizing (extending) overlay.qcow2, the new blocks should not stay
unallocated and make the additional As from base.qcow2 visible like
before this series, but zeros should be read.

A similar case happens with the various variants of a commit job when an
intermediate file is short (- for unallocated):

    base.qcow2:     A-A-AAAA
    mid.qcow2:      BB-B
    top.qcow2:      C--C--C-

After commit top.qcow2 to mid.qcow2, the following happens:

    mid.qcow2:      CB-C00C0 (correct result)
    mid.qcow2:      CB-C--C- (before this fix)

Without the fix, blocks that previously read as zeros on top.qcow2
suddenly turn into A.

Kevin Wolf (6):
  block: bdrv_co_do_pwrite_zeroes: 64 bit 'bytes' parameter
  block: truncate: Don't make backing file data visible
  iotests: Add qemu_io_log()
  iotests: Fix timeout in run_job()
  iotests: Support job-complete in run_job()
  iotests: Test committing to short backing file

 block/io.c                    |  31 ++++++-
 tests/qemu-iotests/274        | 131 +++++++++++++++++++++++++++++
 tests/qemu-iotests/274.out    | 150 ++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/group      |   1 +
 tests/qemu-iotests/iotests.py |  11 ++-
 5 files changed, 319 insertions(+), 5 deletions(-)
 create mode 100755 tests/qemu-iotests/274
 create mode 100644 tests/qemu-iotests/274.out

-- 
2.20.1

[PATCH 1/6] block: bdrv_co_do_pwrite_zeroes: 64 bit 'bytes' parameter

[Kevin Wolf]

bdrv_co_do_pwrite_zeroes() can already cope with maximum request sizes
by calling the driver in a loop until everything is done. Make the small
remaining change that is necessary to let it accept a 64 bit byte count.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/io.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/block/io.c b/block/io.c
index f75777f5ea..003f4ea38c 100644
--- a/block/io.c
+++ b/block/io.c
@@ -42,7 +42,7 @@
 
 static void bdrv_parent_cb_resize(BlockDriverState *bs);
 static int coroutine_fn bdrv_co_do_pwrite_zeroes(BlockDriverState *bs,
-    int64_t offset, int bytes, BdrvRequestFlags flags);
+    int64_t offset, int64_t bytes, BdrvRequestFlags flags);
 
 static void bdrv_parent_drained_begin(BlockDriverState *bs, BdrvChild *ignore,
                                       bool ignore_bds_parents)
@@ -1730,7 +1730,7 @@ int coroutine_fn bdrv_co_preadv_part(BdrvChild *child,
 }
 
 static int coroutine_fn bdrv_co_do_pwrite_zeroes(BlockDriverState *bs,
-    int64_t offset, int bytes, BdrvRequestFlags flags)
+    int64_t offset, int64_t bytes, BdrvRequestFlags flags)
 {
     BlockDriver *drv = bs->drv;
     QEMUIOVector qiov;
@@ -1760,7 +1760,7 @@ static int coroutine_fn bdrv_co_do_pwrite_zeroes(BlockDriverState *bs,
     assert(max_write_zeroes >= bs->bl.request_alignment);
 
     while (bytes > 0 && !ret) {
-        int num = bytes;
+        int num = MIN(bytes, BDRV_REQUEST_MAX_BYTES);
 
         /* Align request.  Block drivers can expect the "bulk" of the request
          * to be aligned, and that unaligned requests do not cross cluster
-- 
2.20.1

[PATCH 2/6] block: truncate: Don't make backing file data visible

[Kevin Wolf]

When extending the size of an image that has a backing file larger than
its old size, make sure that the backing file data doesn't become
visible in the guest, but the added area is properly zeroed out.

The old behaviour made a difference in 'block_resize' (where showing the
backing file data from an old snapshot rather than zeros is
questionable) as well as in commit block jobs (both from active and
intermediate nodes) and HMP 'commit', where committing to a short
backing file would incorrectly omit writing zeroes for unallocated
blocks on the top layer after the EOF of the short backing file.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/io.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/block/io.c b/block/io.c
index 003f4ea38c..8683f7a4bd 100644
--- a/block/io.c
+++ b/block/io.c
@@ -3382,6 +3382,31 @@ int coroutine_fn bdrv_co_truncate(BdrvChild *child, int64_t offset, bool exact,
         goto out;
     }
 
+    /*
+     * If the image has a backing file that is large enough that it would
+     * provide data for the new area, we cannot leave it unallocated because
+     * then the backing file content would become visible. Instead, zero-fill
+     * the area where backing file and new area overlap.
+     */
+    if (new_bytes && bs->backing && prealloc == PREALLOC_MODE_OFF) {
+        int64_t backing_len;
+
+        backing_len = bdrv_getlength(backing_bs(bs));
+        if (backing_len < 0) {
+            ret = backing_len;
+            goto out;
+        }
+
+        if (backing_len > old_size) {
+            ret = bdrv_co_do_pwrite_zeroes(bs, old_size,
+                                           MIN(new_bytes, backing_len - old_size),
+                                           BDRV_REQ_ZERO_WRITE | BDRV_REQ_MAY_UNMAP);
+            if (ret < 0) {
+                goto out;
+            }
+        }
+    }
+
     ret = refresh_total_sectors(bs, offset >> BDRV_SECTOR_BITS);
     if (ret < 0) {
         error_setg_errno(errp, -ret, "Could not refresh total sector count");
-- 
2.20.1

[PATCH 3/6] iotests: Add qemu_io_log()

[Kevin Wolf]

Add a function that runs qemu-io and logs the output with the
appropriate filters applied.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 tests/qemu-iotests/iotests.py | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 6a248472b9..330681ad02 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -157,6 +157,11 @@ def qemu_io(*args):
         sys.stderr.write('qemu-io received signal %i: %s\n' % (-exitcode, ' '.join(args)))
     return subp.communicate()[0]
 
+def qemu_io_log(*args):
+    result = qemu_io(*args)
+    log(result, filters=[filter_testfiles, filter_qemu_io])
+    return result
+
 def qemu_io_silent(*args):
     '''Run qemu-io and return the exit code, suppressing stdout'''
     args = qemu_io_args + list(args)
-- 
2.20.1

[PATCH 4/6] iotests: Fix timeout in run_job()

[Kevin Wolf]

run_job() accepts a wait parameter for a timeout, but it doesn't
actually use it. The only thing that is missing is passing it to
events_wait(), so do that now.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 tests/qemu-iotests/iotests.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 330681ad02..b409198e47 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -604,7 +604,7 @@ class VM(qtest.QEMUQtestMachine):
         ]
         error = None
         while True:
-            ev = filter_qmp_event(self.events_wait(events))
+            ev = filter_qmp_event(self.events_wait(events, timeout=wait))
             if ev['event'] != 'JOB_STATUS_CHANGE':
                 if use_log:
                     log(ev)
-- 
2.20.1

[PATCH 5/6] iotests: Support job-complete in run_job()

[Kevin Wolf]

Automatically complete jobs that have a 'ready' state and need an
explicit job-complete. Without this, run_job() would hang for such
jobs.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 tests/qemu-iotests/iotests.py | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index b409198e47..c4063ef6bb 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -617,6 +617,8 @@ class VM(qtest.QEMUQtestMachine):
                         error = j['error']
                         if use_log:
                             log('Job failed: %s' % (j['error']))
+            elif status == 'ready':
+                self.qmp_log('job-complete', id=job)
             elif status == 'pending' and not auto_finalize:
                 if pre_finalize:
                     pre_finalize()
-- 
2.20.1

[PATCH 6/6] iotests: Test committing to short backing file

[Kevin Wolf]

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 tests/qemu-iotests/274        | 131 +++++++++++++++++++++++++++++
 tests/qemu-iotests/274.out    | 150 ++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/group      |   1 +
 tests/qemu-iotests/iotests.py |   2 +-
 4 files changed, 283 insertions(+), 1 deletion(-)
 create mode 100755 tests/qemu-iotests/274
 create mode 100644 tests/qemu-iotests/274.out

diff --git a/tests/qemu-iotests/274 b/tests/qemu-iotests/274
new file mode 100755
index 0000000000..f3b71e2859
--- /dev/null
+++ b/tests/qemu-iotests/274
@@ -0,0 +1,131 @@
+#!/usr/bin/env python
+#
+# Copyright (C) 2019 Red Hat, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+# Creator/Owner: Kevin Wolf <kwolf@redhat.com>
+#
+# Some tests for short backing files and short overlays
+
+import iotests
+import os
+
+iotests.verify_image_format(supported_fmts=['qcow2'])
+iotests.verify_platform(['linux'])
+
+size_short = 1 * 1024 * 1024
+size_long = 2 * 1024 * 1024
+size_diff = size_long - size_short
+
+def create_chain():
+    iotests.qemu_img_log('create', '-f', iotests.imgfmt, base,
+                         str(size_long))
+    iotests.qemu_img_log('create', '-f', iotests.imgfmt, '-b', base, mid,
+                         str(size_short))
+    iotests.qemu_img_log('create', '-f', iotests.imgfmt, '-b', mid, top,
+                         str(size_long))
+
+    iotests.qemu_io_log('-c', 'write -P 1 0 %d' % size_long, base)
+
+def create_vm():
+    vm = iotests.VM()
+    vm.add_blockdev('file,filename=%s,node-name=base-file' % (base))
+    vm.add_blockdev('%s,file=base-file,node-name=base' % (iotests.imgfmt))
+    vm.add_blockdev('file,filename=%s,node-name=mid-file' % (mid))
+    vm.add_blockdev('%s,file=mid-file,node-name=mid,backing=base' % (iotests.imgfmt))
+    vm.add_drive(top, 'backing=mid,node-name=top')
+    return vm
+
+with iotests.FilePath('base') as base, \
+     iotests.FilePath('mid') as mid, \
+     iotests.FilePath('top') as top:
+
+    iotests.log('== Commit tests ==')
+
+    create_chain()
+
+    iotests.log('=== Check visible data ===')
+
+    iotests.qemu_io_log('-c', 'read -P 1 0 %d' % size_short, top)
+    iotests.qemu_io_log('-c', 'read -P 0 %d %d' % (size_short, size_diff), top)
+
+    iotests.log('=== Checking allocation status ===')
+
+    iotests.qemu_io_log('-c', 'alloc 0 %d' % size_short,
+                        '-c', 'alloc %d %d' % (size_short, size_diff),
+                        base)
+
+    iotests.qemu_io_log('-c', 'alloc 0 %d' % size_short,
+                        '-c', 'alloc %d %d' % (size_short, size_diff),
+                        mid)
+
+    iotests.qemu_io_log('-c', 'alloc 0 %d' % size_short,
+                        '-c', 'alloc %d %d' % (size_short, size_diff),
+                        top)
+
+    iotests.log('=== Checking map ===')
+
+    iotests.qemu_img_log('map', '--output=json', base)
+    iotests.qemu_img_log('map', '--output=human', base)
+    iotests.qemu_img_log('map', '--output=json', mid)
+    iotests.qemu_img_log('map', '--output=human', mid)
+    iotests.qemu_img_log('map', '--output=json', top)
+    iotests.qemu_img_log('map', '--output=human', top)
+
+    iotests.log('=== Testing qemu-img commit (top -> mid) ===')
+
+    iotests.qemu_img_log('commit', top)
+    iotests.img_info_log(mid)
+    iotests.qemu_io_log('-c', 'read -P 1 0 %d' % size_short, mid)
+    iotests.qemu_io_log('-c', 'read -P 0 %d %d' % (size_short, size_diff), mid)
+
+    iotests.log('=== Testing HMP commit (top -> mid) ===')
+
+    create_chain()
+    with create_vm() as vm:
+        vm.launch()
+        vm.qmp_log('human-monitor-command', command_line='commit drive0')
+
+    iotests.img_info_log(mid)
+    iotests.qemu_io_log('-c', 'read -P 1 0 %d' % size_short, mid)
+    iotests.qemu_io_log('-c', 'read -P 0 %d %d' % (size_short, size_diff), mid)
+
+    iotests.log('=== Testing QMP active commit (top -> mid) ===')
+
+    create_chain()
+    with create_vm() as vm:
+        vm.launch()
+        vm.qmp_log('block-commit', device='top', base_node='mid',
+                   job_id='job0', auto_dismiss=False)
+        vm.run_job('job0', wait=5)
+
+    iotests.img_info_log(mid)
+    iotests.qemu_io_log('-c', 'read -P 1 0 %d' % size_short, mid)
+    iotests.qemu_io_log('-c', 'read -P 0 %d %d' % (size_short, size_diff), mid)
+
+
+    iotests.log('== Resize tests ==')
+
+    iotests.qemu_img_log('create', '-f', iotests.imgfmt, base, '6G')
+    iotests.qemu_img_log('create', '-f', iotests.imgfmt, '-b', base, top, '1G')
+    iotests.qemu_io_log('-c', 'write -P 1 3G 64k', base)
+    iotests.qemu_io_log('-c', 'write -P 2 5G 64k', base)
+
+    # After this, 0 to 6 GB should be allocated/zeroed
+    # 6 GB to 8 BG should be unallocated
+    iotests.qemu_img_log('resize', '-f', iotests.imgfmt, top, '8G')
+    iotests.qemu_io_log('-c', 'read -P 0 3G 64k', top)
+    iotests.qemu_io_log('-c', 'read -P 0 5G 64k', top)
+    iotests.qemu_img_log('map', '--output=json', top)
diff --git a/tests/qemu-iotests/274.out b/tests/qemu-iotests/274.out
new file mode 100644
index 0000000000..def0ac7d27
--- /dev/null
+++ b/tests/qemu-iotests/274.out
@@ -0,0 +1,150 @@
+== Commit tests ==
+Formatting 'TEST_DIR/PID-base', fmt=qcow2 size=2097152 cluster_size=65536 lazy_refcounts=off refcount_bits=16
+
+Formatting 'TEST_DIR/PID-mid', fmt=qcow2 size=1048576 backing_file=TEST_DIR/PID-base cluster_size=65536 lazy_refcounts=off refcount_bits=16
+
+Formatting 'TEST_DIR/PID-top', fmt=qcow2 size=2097152 backing_file=TEST_DIR/PID-mid cluster_size=65536 lazy_refcounts=off refcount_bits=16
+
+wrote 2097152/2097152 bytes at offset 0
+2 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+=== Check visible data ===
+read 1048576/1048576 bytes at offset 0
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+read 1048576/1048576 bytes at offset 1048576
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+=== Checking allocation status ===
+1048576/1048576 bytes allocated at offset 0 bytes
+1048576/1048576 bytes allocated at offset 1 MiB
+
+0/1048576 bytes allocated at offset 0 bytes
+0/0 bytes allocated at offset 1 MiB
+
+0/1048576 bytes allocated at offset 0 bytes
+0/1048576 bytes allocated at offset 1 MiB
+
+=== Checking map ===
+[{ "start": 0, "length": 2097152, "depth": 0, "zero": false, "data": true, "offset": 327680}]
+
+Offset          Length          Mapped to       File
+0               0x200000        0x50000         TEST_DIR/PID-base
+
+[{ "start": 0, "length": 1048576, "depth": 1, "zero": false, "data": true, "offset": 327680}]
+
+Offset          Length          Mapped to       File
+0               0x100000        0x50000         TEST_DIR/PID-base
+
+[{ "start": 0, "length": 1048576, "depth": 2, "zero": false, "data": true, "offset": 327680},
+{ "start": 1048576, "length": 1048576, "depth": 0, "zero": true, "data": false}]
+
+Offset          Length          Mapped to       File
+0               0x100000        0x50000         TEST_DIR/PID-base
+
+=== Testing qemu-img commit (top -> mid) ===
+Image committed.
+
+image: TEST_IMG
+file format: IMGFMT
+virtual size: 2 MiB (2097152 bytes)
+cluster_size: 65536
+backing file: TEST_DIR/PID-base
+Format specific information:
+    compat: 1.1
+    lazy refcounts: false
+    refcount bits: 16
+    corrupt: false
+
+read 1048576/1048576 bytes at offset 0
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+read 1048576/1048576 bytes at offset 1048576
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+=== Testing HMP commit (top -> mid) ===
+Formatting 'TEST_DIR/PID-base', fmt=qcow2 size=2097152 cluster_size=65536 lazy_refcounts=off refcount_bits=16
+
+Formatting 'TEST_DIR/PID-mid', fmt=qcow2 size=1048576 backing_file=TEST_DIR/PID-base cluster_size=65536 lazy_refcounts=off refcount_bits=16
+
+Formatting 'TEST_DIR/PID-top', fmt=qcow2 size=2097152 backing_file=TEST_DIR/PID-mid cluster_size=65536 lazy_refcounts=off refcount_bits=16
+
+wrote 2097152/2097152 bytes at offset 0
+2 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+{"execute": "human-monitor-command", "arguments": {"command-line": "commit drive0"}}
+{"return": ""}
+image: TEST_IMG
+file format: IMGFMT
+virtual size: 2 MiB (2097152 bytes)
+cluster_size: 65536
+backing file: TEST_DIR/PID-base
+Format specific information:
+    compat: 1.1
+    lazy refcounts: false
+    refcount bits: 16
+    corrupt: false
+
+read 1048576/1048576 bytes at offset 0
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+read 1048576/1048576 bytes at offset 1048576
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+=== Testing QMP active commit (top -> mid) ===
+Formatting 'TEST_DIR/PID-base', fmt=qcow2 size=2097152 cluster_size=65536 lazy_refcounts=off refcount_bits=16
+
+Formatting 'TEST_DIR/PID-mid', fmt=qcow2 size=1048576 backing_file=TEST_DIR/PID-base cluster_size=65536 lazy_refcounts=off refcount_bits=16
+
+Formatting 'TEST_DIR/PID-top', fmt=qcow2 size=2097152 backing_file=TEST_DIR/PID-mid cluster_size=65536 lazy_refcounts=off refcount_bits=16
+
+wrote 2097152/2097152 bytes at offset 0
+2 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+{"execute": "block-commit", "arguments": {"auto-dismiss": false, "base-node": "mid", "device": "top", "job-id": "job0"}}
+{"return": {}}
+{"execute": "job-complete", "arguments": {"id": "job0"}}
+{"return": {}}
+{"data": {"device": "job0", "len": 0, "offset": 0, "speed": 0, "type": "commit"}, "event": "BLOCK_JOB_READY", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
+{"data": {"device": "job0", "len": 0, "offset": 0, "speed": 0, "type": "commit"}, "event": "BLOCK_JOB_COMPLETED", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
+{"execute": "job-dismiss", "arguments": {"id": "job0"}}
+{"return": {}}
+image: TEST_IMG
+file format: IMGFMT
+virtual size: 2 MiB (2097152 bytes)
+cluster_size: 65536
+backing file: TEST_DIR/PID-base
+Format specific information:
+    compat: 1.1
+    lazy refcounts: false
+    refcount bits: 16
+    corrupt: false
+
+read 1048576/1048576 bytes at offset 0
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+read 1048576/1048576 bytes at offset 1048576
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+== Resize tests ==
+Formatting 'TEST_DIR/PID-base', fmt=qcow2 size=6442450944 cluster_size=65536 lazy_refcounts=off refcount_bits=16
+
+Formatting 'TEST_DIR/PID-top', fmt=qcow2 size=1073741824 backing_file=TEST_DIR/PID-base cluster_size=65536 lazy_refcounts=off refcount_bits=16
+
+wrote 65536/65536 bytes at offset 3221225472
+64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+wrote 65536/65536 bytes at offset 5368709120
+64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+Image resized.
+
+read 65536/65536 bytes at offset 3221225472
+64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+read 65536/65536 bytes at offset 5368709120
+64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+[{ "start": 0, "length": 1073741824, "depth": 1, "zero": true, "data": false},
+{ "start": 1073741824, "length": 7516192768, "depth": 0, "zero": true, "data": false}]
+
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index c56bb69031..329c9e4d37 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -285,3 +285,4 @@
 270 rw backing quick
 272 rw
 273 backing quick
+274 rw backing quick
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index c4063ef6bb..5fd6d820b1 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -144,7 +144,7 @@ def img_info_log(filename, filter_path=None, imgopts=False, extra_args=[]):
     output = qemu_img_pipe(*args)
     if not filter_path:
         filter_path = filename
-    log(filter_img_info(output, filter_path))
+    log(filter_img_info(output, filter_path), filters=[filter_testfiles])
 
 def qemu_io(*args):
     '''Run qemu-io and return the stdout data'''
-- 
2.20.1

Re: [PATCH 1/6] block: bdrv_co_do_pwrite_zeroes: 64 bit 'bytes' parameter

[Eric Blake]

On 11/20/19 8:03 AM, Kevin Wolf wrote:
> bdrv_co_do_pwrite_zeroes() can already cope with maximum request sizes
> by calling the driver in a loop until everything is done. Make the small
> remaining change that is necessary to let it accept a 64 bit byte count.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>   block/io.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)

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

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

Re: [PATCH 2/6] block: truncate: Don't make backing file data visible

[Eric Blake]

On 11/20/19 8:03 AM, Kevin Wolf wrote:
> When extending the size of an image that has a backing file larger than
> its old size, make sure that the backing file data doesn't become
> visible in the guest, but the added area is properly zeroed out.
> 
> The old behaviour made a difference in 'block_resize' (where showing the
> backing file data from an old snapshot rather than zeros is
> questionable) as well as in commit block jobs (both from active and
> intermediate nodes) and HMP 'commit', where committing to a short
> backing file would incorrectly omit writing zeroes for unallocated
> blocks on the top layer after the EOF of the short backing file.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>   block/io.c | 25 +++++++++++++++++++++++++
>   1 file changed, 25 insertions(+)

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

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

Re: [PATCH 3/6] iotests: Add qemu_io_log()

[Eric Blake]

On 11/20/19 8:03 AM, Kevin Wolf wrote:
> Add a function that runs qemu-io and logs the output with the
> appropriate filters applied.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>   tests/qemu-iotests/iotests.py | 5 +++++
>   1 file changed, 5 insertions(+)

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

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

Re: [PATCH 1/6] block: bdrv_co_do_pwrite_zeroes: 64 bit 'bytes' parameter

[Vladimir Sementsov-Ogievskiy]

20.11.2019 17:03, Kevin Wolf wrote:
> bdrv_co_do_pwrite_zeroes() can already cope with maximum request sizes
> by calling the driver in a loop until everything is done. Make the small
> remaining change that is necessary to let it accept a 64 bit byte count.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>



-- 
Best regards,
Vladimir

Re: [PATCH 4/6] iotests: Fix timeout in run_job()

[Eric Blake]

On 11/20/19 8:03 AM, Kevin Wolf wrote:
> run_job() accepts a wait parameter for a timeout, but it doesn't
> actually use it. The only thing that is missing is passing it to
> events_wait(), so do that now.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>   tests/qemu-iotests/iotests.py | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)

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

> 
> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
> index 330681ad02..b409198e47 100644
> --- a/tests/qemu-iotests/iotests.py
> +++ b/tests/qemu-iotests/iotests.py
> @@ -604,7 +604,7 @@ class VM(qtest.QEMUQtestMachine):
>           ]
>           error = None
>           while True:
> -            ev = filter_qmp_event(self.events_wait(events))
> +            ev = filter_qmp_event(self.events_wait(events, timeout=wait))
>               if ev['event'] != 'JOB_STATUS_CHANGE':
>                   if use_log:
>                       log(ev)
> 

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

Re: [PATCH 5/6] iotests: Support job-complete in run_job()

[Eric Blake]

On 11/20/19 8:03 AM, Kevin Wolf wrote:
> Automatically complete jobs that have a 'ready' state and need an
> explicit job-complete. Without this, run_job() would hang for such
> jobs.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>   tests/qemu-iotests/iotests.py | 2 ++
>   1 file changed, 2 insertions(+)

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

> 
> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
> index b409198e47..c4063ef6bb 100644
> --- a/tests/qemu-iotests/iotests.py
> +++ b/tests/qemu-iotests/iotests.py
> @@ -617,6 +617,8 @@ class VM(qtest.QEMUQtestMachine):
>                           error = j['error']
>                           if use_log:
>                               log('Job failed: %s' % (j['error']))
> +            elif status == 'ready':
> +                self.qmp_log('job-complete', id=job)
>               elif status == 'pending' and not auto_finalize:
>                   if pre_finalize:
>                       pre_finalize()
> 

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

Re: [PATCH 2/6] block: truncate: Don't make backing file data visible

[Vladimir Sementsov-Ogievskiy]

20.11.2019 17:03, Kevin Wolf wrote:
> When extending the size of an image that has a backing file larger than
> its old size, make sure that the backing file data doesn't become
> visible in the guest, but the added area is properly zeroed out.
> 
> The old behaviour made a difference in 'block_resize' (where showing the
> backing file data from an old snapshot rather than zeros is
> questionable) as well as in commit block jobs (both from active and
> intermediate nodes) and HMP 'commit', where committing to a short
> backing file would incorrectly omit writing zeroes for unallocated
> blocks on the top layer after the EOF of the short backing file.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>   block/io.c | 25 +++++++++++++++++++++++++
>   1 file changed, 25 insertions(+)
> 
> diff --git a/block/io.c b/block/io.c
> index 003f4ea38c..8683f7a4bd 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -3382,6 +3382,31 @@ int coroutine_fn bdrv_co_truncate(BdrvChild *child, int64_t offset, bool exact,
>           goto out;
>       }
>   
> +    /*
> +     * If the image has a backing file that is large enough that it would
> +     * provide data for the new area, we cannot leave it unallocated because
> +     * then the backing file content would become visible. Instead, zero-fill
> +     * the area where backing file and new area overlap.
> +     */

Should we mention that, still, we don't care if user for some reason will change
backing file in future?

> +    if (new_bytes && bs->backing && prealloc == PREALLOC_MODE_OFF) {
> +        int64_t backing_len;
> +
> +        backing_len = bdrv_getlength(backing_bs(bs));
> +        if (backing_len < 0) {
> +            ret = backing_len;
> +            goto out;
> +        }
> +
> +        if (backing_len > old_size) {
> +            ret = bdrv_co_do_pwrite_zeroes(bs, old_size,
> +                                           MIN(new_bytes, backing_len - old_size),
> +                                           BDRV_REQ_ZERO_WRITE | BDRV_REQ_MAY_UNMAP);

two over-80 lines

> +            if (ret < 0) {
> +                goto out;
> +            }
> +        }
> +    }

should we improve "off" mode specification in qapi?

> +
>       ret = refresh_total_sectors(bs, offset >> BDRV_SECTOR_BITS);
>       if (ret < 0) {
>           error_setg_errno(errp, -ret, "Could not refresh total sector count");
> 

Hmm. is it correct to call write_zeroes before refresh_total_sectors?
Note that qcow2_co_pwrite_zeroes rely on bs->total_sectors...

-- 
Best regards,
Vladimir

Re: [PATCH 3/6] iotests: Add qemu_io_log()

[Vladimir Sementsov-Ogievskiy]

20.11.2019 17:03, Kevin Wolf wrote:
> Add a function that runs qemu-io and logs the output with the
> appropriate filters applied.
> 
> Signed-off-by: Kevin Wolf<kwolf@redhat.com>

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

-- 
Best regards,
Vladimir

Re: [PATCH 4/6] iotests: Fix timeout in run_job()

[Vladimir Sementsov-Ogievskiy]

20.11.2019 17:03, Kevin Wolf wrote:
> run_job() accepts a wait parameter for a timeout, but it doesn't
> actually use it. The only thing that is missing is passing it to
> events_wait(), so do that now.
> 
> Signed-off-by: Kevin Wolf<kwolf@redhat.com>

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

-- 
Best regards,
Vladimir

Re: [PATCH 6/6] iotests: Test committing to short backing file

[Eric Blake]

On 11/20/19 8:03 AM, Kevin Wolf wrote:
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>   tests/qemu-iotests/274        | 131 +++++++++++++++++++++++++++++
>   tests/qemu-iotests/274.out    | 150 ++++++++++++++++++++++++++++++++++
>   tests/qemu-iotests/group      |   1 +
>   tests/qemu-iotests/iotests.py |   2 +-
>   4 files changed, 283 insertions(+), 1 deletion(-)
>   create mode 100755 tests/qemu-iotests/274
>   create mode 100644 tests/qemu-iotests/274.out
> 

> +def create_chain():
> +    iotests.qemu_img_log('create', '-f', iotests.imgfmt, base,
> +                         str(size_long))
> +    iotests.qemu_img_log('create', '-f', iotests.imgfmt, '-b', base, mid,
> +                         str(size_short))
> +    iotests.qemu_img_log('create', '-f', iotests.imgfmt, '-b', mid, top,
> +                         str(size_long))
> +
> +    iotests.qemu_io_log('-c', 'write -P 1 0 %d' % size_long, base)

Matches the setup in the cover letter, and definitely exposes the issues 
at hand.

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

As leaving this bug open represents a data leak/corruption to guests, I 
think this series counts as worthy of -rc3.

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

Re: [PATCH 5/6] iotests: Support job-complete in run_job()

[Vladimir Sementsov-Ogievskiy]

20.11.2019 17:03, Kevin Wolf wrote:
> Automatically complete jobs that have a 'ready' state and need an
> explicit job-complete. Without this, run_job() would hang for such
> jobs.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

> ---
>   tests/qemu-iotests/iotests.py | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
> index b409198e47..c4063ef6bb 100644
> --- a/tests/qemu-iotests/iotests.py
> +++ b/tests/qemu-iotests/iotests.py
> @@ -617,6 +617,8 @@ class VM(qtest.QEMUQtestMachine):
>                           error = j['error']
>                           if use_log:
>                               log('Job failed: %s' % (j['error']))
> +            elif status == 'ready':
> +                self.qmp_log('job-complete', id=job)
>               elif status == 'pending' and not auto_finalize:
>                   if pre_finalize:
>                       pre_finalize()
> 


-- 
Best regards,
Vladimir

Re: [PATCH 2/6] block: truncate: Don't make backing file data visible

[Kevin Wolf]

Am 20.11.2019 um 15:47 hat Vladimir Sementsov-Ogievskiy geschrieben:
> 20.11.2019 17:03, Kevin Wolf wrote:
> > When extending the size of an image that has a backing file larger than
> > its old size, make sure that the backing file data doesn't become
> > visible in the guest, but the added area is properly zeroed out.
> > 
> > The old behaviour made a difference in 'block_resize' (where showing the
> > backing file data from an old snapshot rather than zeros is
> > questionable) as well as in commit block jobs (both from active and
> > intermediate nodes) and HMP 'commit', where committing to a short
> > backing file would incorrectly omit writing zeroes for unallocated
> > blocks on the top layer after the EOF of the short backing file.
> > 
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> >   block/io.c | 25 +++++++++++++++++++++++++
> >   1 file changed, 25 insertions(+)
> > 
> > diff --git a/block/io.c b/block/io.c
> > index 003f4ea38c..8683f7a4bd 100644
> > --- a/block/io.c
> > +++ b/block/io.c
> > @@ -3382,6 +3382,31 @@ int coroutine_fn bdrv_co_truncate(BdrvChild *child, int64_t offset, bool exact,
> >           goto out;
> >       }
> >   
> > +    /*
> > +     * If the image has a backing file that is large enough that it would
> > +     * provide data for the new area, we cannot leave it unallocated because
> > +     * then the backing file content would become visible. Instead, zero-fill
> > +     * the area where backing file and new area overlap.
> > +     */
> 
> Should we mention that, still, we don't care if user for some reason will change
> backing file in future?

This should be obvious, but I can add something.

> > +    if (new_bytes && bs->backing && prealloc == PREALLOC_MODE_OFF) {
> > +        int64_t backing_len;
> > +
> > +        backing_len = bdrv_getlength(backing_bs(bs));
> > +        if (backing_len < 0) {
> > +            ret = backing_len;
> > +            goto out;
> > +        }
> > +
> > +        if (backing_len > old_size) {
> > +            ret = bdrv_co_do_pwrite_zeroes(bs, old_size,
> > +                                           MIN(new_bytes, backing_len - old_size),
> > +                                           BDRV_REQ_ZERO_WRITE | BDRV_REQ_MAY_UNMAP);
> 
> two over-80 lines

Will fix.

> > +            if (ret < 0) {
> > +                goto out;
> > +            }
> > +        }
> > +    }
> 
> should we improve "off" mode specification in qapi?

I don't think we're changing the semantics of "off". We're merely fixing
a bug that happens not to exist with preallocation.

> > +
> >       ret = refresh_total_sectors(bs, offset >> BDRV_SECTOR_BITS);
> >       if (ret < 0) {
> >           error_setg_errno(errp, -ret, "Could not refresh total sector count");
> > 
> 
> Hmm. is it correct to call write_zeroes before refresh_total_sectors?
> Note that qcow2_co_pwrite_zeroes rely on bs->total_sectors...

Hm... I placed the code where I did because I didn't want to make the
new area valid before it isn't zeroed. But you're probably right that
we shouldn't run requests with inconsistent bs->total_sectors, so I'll
switch the order.

Kevin

Re: [PATCH 6/6] iotests: Test committing to short backing file

[Vladimir Sementsov-Ogievskiy]

20.11.2019 17:03, Kevin Wolf wrote:
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>   tests/qemu-iotests/274        | 131 +++++++++++++++++++++++++++++
>   tests/qemu-iotests/274.out    | 150 ++++++++++++++++++++++++++++++++++
>   tests/qemu-iotests/group      |   1 +
>   tests/qemu-iotests/iotests.py |   2 +-
>   4 files changed, 283 insertions(+), 1 deletion(-)
>   create mode 100755 tests/qemu-iotests/274
>   create mode 100644 tests/qemu-iotests/274.out
> 
> diff --git a/tests/qemu-iotests/274 b/tests/qemu-iotests/274
> new file mode 100755
> index 0000000000..f3b71e2859
> --- /dev/null
> +++ b/tests/qemu-iotests/274
> @@ -0,0 +1,131 @@
> +#!/usr/bin/env python
> +#
> +# Copyright (C) 2019 Red Hat, Inc.
> +#
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 2 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +#
> +# Creator/Owner: Kevin Wolf <kwolf@redhat.com>
> +#
> +# Some tests for short backing files and short overlays
> +
> +import iotests
> +import os
> +
> +iotests.verify_image_format(supported_fmts=['qcow2'])
> +iotests.verify_platform(['linux'])
> +
> +size_short = 1 * 1024 * 1024
> +size_long = 2 * 1024 * 1024
> +size_diff = size_long - size_short
> +
> +def create_chain():
> +    iotests.qemu_img_log('create', '-f', iotests.imgfmt, base,
> +                         str(size_long))
> +    iotests.qemu_img_log('create', '-f', iotests.imgfmt, '-b', base, mid,
> +                         str(size_short))
> +    iotests.qemu_img_log('create', '-f', iotests.imgfmt, '-b', mid, top,
> +                         str(size_long))
> +
> +    iotests.qemu_io_log('-c', 'write -P 1 0 %d' % size_long, base)
> +
> +def create_vm():
> +    vm = iotests.VM()
> +    vm.add_blockdev('file,filename=%s,node-name=base-file' % (base))
> +    vm.add_blockdev('%s,file=base-file,node-name=base' % (iotests.imgfmt))
> +    vm.add_blockdev('file,filename=%s,node-name=mid-file' % (mid))
> +    vm.add_blockdev('%s,file=mid-file,node-name=mid,backing=base' % (iotests.imgfmt))
> +    vm.add_drive(top, 'backing=mid,node-name=top')
> +    return vm
> +
> +with iotests.FilePath('base') as base, \
> +     iotests.FilePath('mid') as mid, \
> +     iotests.FilePath('top') as top:
> +
> +    iotests.log('== Commit tests ==')
> +
> +    create_chain()
> +
> +    iotests.log('=== Check visible data ===')
> +
> +    iotests.qemu_io_log('-c', 'read -P 1 0 %d' % size_short, top)
> +    iotests.qemu_io_log('-c', 'read -P 0 %d %d' % (size_short, size_diff), top)
> +
> +    iotests.log('=== Checking allocation status ===')
> +
> +    iotests.qemu_io_log('-c', 'alloc 0 %d' % size_short,
> +                        '-c', 'alloc %d %d' % (size_short, size_diff),
> +                        base)
> +
> +    iotests.qemu_io_log('-c', 'alloc 0 %d' % size_short,
> +                        '-c', 'alloc %d %d' % (size_short, size_diff),
> +                        mid)
> +
> +    iotests.qemu_io_log('-c', 'alloc 0 %d' % size_short,
> +                        '-c', 'alloc %d %d' % (size_short, size_diff),
> +                        top)
> +
> +    iotests.log('=== Checking map ===')
> +
> +    iotests.qemu_img_log('map', '--output=json', base)
> +    iotests.qemu_img_log('map', '--output=human', base)
> +    iotests.qemu_img_log('map', '--output=json', mid)
> +    iotests.qemu_img_log('map', '--output=human', mid)
> +    iotests.qemu_img_log('map', '--output=json', top)
> +    iotests.qemu_img_log('map', '--output=human', top)
> +
> +    iotests.log('=== Testing qemu-img commit (top -> mid) ===')
> +
> +    iotests.qemu_img_log('commit', top)
> +    iotests.img_info_log(mid)
> +    iotests.qemu_io_log('-c', 'read -P 1 0 %d' % size_short, mid)
> +    iotests.qemu_io_log('-c', 'read -P 0 %d %d' % (size_short, size_diff), mid)
> +
> +    iotests.log('=== Testing HMP commit (top -> mid) ===')
> +
> +    create_chain()
> +    with create_vm() as vm:
> +        vm.launch()
> +        vm.qmp_log('human-monitor-command', command_line='commit drive0')
> +
> +    iotests.img_info_log(mid)
> +    iotests.qemu_io_log('-c', 'read -P 1 0 %d' % size_short, mid)
> +    iotests.qemu_io_log('-c', 'read -P 0 %d %d' % (size_short, size_diff), mid)
> +
> +    iotests.log('=== Testing QMP active commit (top -> mid) ===')
> +
> +    create_chain()
> +    with create_vm() as vm:
> +        vm.launch()
> +        vm.qmp_log('block-commit', device='top', base_node='mid',
> +                   job_id='job0', auto_dismiss=False)
> +        vm.run_job('job0', wait=5)
> +
> +    iotests.img_info_log(mid)
> +    iotests.qemu_io_log('-c', 'read -P 1 0 %d' % size_short, mid)
> +    iotests.qemu_io_log('-c', 'read -P 0 %d %d' % (size_short, size_diff), mid)
> +
> +
> +    iotests.log('== Resize tests ==')
> +
> +    iotests.qemu_img_log('create', '-f', iotests.imgfmt, base, '6G')
> +    iotests.qemu_img_log('create', '-f', iotests.imgfmt, '-b', base, top, '1G')
> +    iotests.qemu_io_log('-c', 'write -P 1 3G 64k', base)
> +    iotests.qemu_io_log('-c', 'write -P 2 5G 64k', base)
> +
> +    # After this, 0 to 6 GB should be allocated/zeroed
> +    # 6 GB to 8 BG should be unallocated

Hmm, the problem is that the following qemu-img map can't show it, as it merges
1G..6G and 6G..8G into one chunk..

> +    iotests.qemu_img_log('resize', '-f', iotests.imgfmt, top, '8G')
> +    iotests.qemu_io_log('-c', 'read -P 0 3G 64k', top)
> +    iotests.qemu_io_log('-c', 'read -P 0 5G 64k', top)
> +    iotests.qemu_img_log('map', '--output=json', top)
> diff --git a/tests/qemu-iotests/274.out b/tests/qemu-iotests/274.out
> new file mode 100644
> index 0000000000..def0ac7d27
> --- /dev/null
> +++ b/tests/qemu-iotests/274.out
> @@ -0,0 +1,150 @@
> +== Commit tests ==
> +Formatting 'TEST_DIR/PID-base', fmt=qcow2 size=2097152 cluster_size=65536 lazy_refcounts=off refcount_bits=16
> +
> +Formatting 'TEST_DIR/PID-mid', fmt=qcow2 size=1048576 backing_file=TEST_DIR/PID-base cluster_size=65536 lazy_refcounts=off refcount_bits=16
> +
> +Formatting 'TEST_DIR/PID-top', fmt=qcow2 size=2097152 backing_file=TEST_DIR/PID-mid cluster_size=65536 lazy_refcounts=off refcount_bits=16
> +
> +wrote 2097152/2097152 bytes at offset 0
> +2 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> +
> +=== Check visible data ===
> +read 1048576/1048576 bytes at offset 0
> +1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> +
> +read 1048576/1048576 bytes at offset 1048576
> +1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> +
> +=== Checking allocation status ===
> +1048576/1048576 bytes allocated at offset 0 bytes
> +1048576/1048576 bytes allocated at offset 1 MiB
> +
> +0/1048576 bytes allocated at offset 0 bytes
> +0/0 bytes allocated at offset 1 MiB
> +
> +0/1048576 bytes allocated at offset 0 bytes
> +0/1048576 bytes allocated at offset 1 MiB
> +
> +=== Checking map ===
> +[{ "start": 0, "length": 2097152, "depth": 0, "zero": false, "data": true, "offset": 327680}]
> +
> +Offset          Length          Mapped to       File
> +0               0x200000        0x50000         TEST_DIR/PID-base
> +
> +[{ "start": 0, "length": 1048576, "depth": 1, "zero": false, "data": true, "offset": 327680}]
> +
> +Offset          Length          Mapped to       File
> +0               0x100000        0x50000         TEST_DIR/PID-base
> +
> +[{ "start": 0, "length": 1048576, "depth": 2, "zero": false, "data": true, "offset": 327680},
> +{ "start": 1048576, "length": 1048576, "depth": 0, "zero": true, "data": false}]

I think depth of second chunk should be 1, not 0.. But this is for another fixing series.

> +
> +Offset          Length          Mapped to       File
> +0               0x100000        0x50000         TEST_DIR/PID-base
> +
> +=== Testing qemu-img commit (top -> mid) ===
> +Image committed.
> +
> +image: TEST_IMG
> +file format: IMGFMT
> +virtual size: 2 MiB (2097152 bytes)
> +cluster_size: 65536
> +backing file: TEST_DIR/PID-base
> +Format specific information:
> +    compat: 1.1
> +    lazy refcounts: false
> +    refcount bits: 16
> +    corrupt: false
> +
> +read 1048576/1048576 bytes at offset 0
> +1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> +
> +read 1048576/1048576 bytes at offset 1048576
> +1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> +
> +=== Testing HMP commit (top -> mid) ===
> +Formatting 'TEST_DIR/PID-base', fmt=qcow2 size=2097152 cluster_size=65536 lazy_refcounts=off refcount_bits=16
> +
> +Formatting 'TEST_DIR/PID-mid', fmt=qcow2 size=1048576 backing_file=TEST_DIR/PID-base cluster_size=65536 lazy_refcounts=off refcount_bits=16
> +
> +Formatting 'TEST_DIR/PID-top', fmt=qcow2 size=2097152 backing_file=TEST_DIR/PID-mid cluster_size=65536 lazy_refcounts=off refcount_bits=16
> +
> +wrote 2097152/2097152 bytes at offset 0
> +2 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> +
> +{"execute": "human-monitor-command", "arguments": {"command-line": "commit drive0"}}
> +{"return": ""}
> +image: TEST_IMG
> +file format: IMGFMT
> +virtual size: 2 MiB (2097152 bytes)
> +cluster_size: 65536
> +backing file: TEST_DIR/PID-base
> +Format specific information:
> +    compat: 1.1
> +    lazy refcounts: false
> +    refcount bits: 16
> +    corrupt: false
> +
> +read 1048576/1048576 bytes at offset 0
> +1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> +
> +read 1048576/1048576 bytes at offset 1048576
> +1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> +
> +=== Testing QMP active commit (top -> mid) ===
> +Formatting 'TEST_DIR/PID-base', fmt=qcow2 size=2097152 cluster_size=65536 lazy_refcounts=off refcount_bits=16
> +
> +Formatting 'TEST_DIR/PID-mid', fmt=qcow2 size=1048576 backing_file=TEST_DIR/PID-base cluster_size=65536 lazy_refcounts=off refcount_bits=16
> +
> +Formatting 'TEST_DIR/PID-top', fmt=qcow2 size=2097152 backing_file=TEST_DIR/PID-mid cluster_size=65536 lazy_refcounts=off refcount_bits=16
> +
> +wrote 2097152/2097152 bytes at offset 0
> +2 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> +
> +{"execute": "block-commit", "arguments": {"auto-dismiss": false, "base-node": "mid", "device": "top", "job-id": "job0"}}
> +{"return": {}}
> +{"execute": "job-complete", "arguments": {"id": "job0"}}
> +{"return": {}}
> +{"data": {"device": "job0", "len": 0, "offset": 0, "speed": 0, "type": "commit"}, "event": "BLOCK_JOB_READY", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
> +{"data": {"device": "job0", "len": 0, "offset": 0, "speed": 0, "type": "commit"}, "event": "BLOCK_JOB_COMPLETED", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
> +{"execute": "job-dismiss", "arguments": {"id": "job0"}}
> +{"return": {}}
> +image: TEST_IMG
> +file format: IMGFMT
> +virtual size: 2 MiB (2097152 bytes)
> +cluster_size: 65536
> +backing file: TEST_DIR/PID-base
> +Format specific information:
> +    compat: 1.1
> +    lazy refcounts: false
> +    refcount bits: 16
> +    corrupt: false
> +
> +read 1048576/1048576 bytes at offset 0
> +1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> +
> +read 1048576/1048576 bytes at offset 1048576
> +1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> +
> +== Resize tests ==
> +Formatting 'TEST_DIR/PID-base', fmt=qcow2 size=6442450944 cluster_size=65536 lazy_refcounts=off refcount_bits=16
> +
> +Formatting 'TEST_DIR/PID-top', fmt=qcow2 size=1073741824 backing_file=TEST_DIR/PID-base cluster_size=65536 lazy_refcounts=off refcount_bits=16
> +
> +wrote 65536/65536 bytes at offset 3221225472
> +64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> +
> +wrote 65536/65536 bytes at offset 5368709120
> +64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> +
> +Image resized.
> +
> +read 65536/65536 bytes at offset 3221225472
> +64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> +
> +read 65536/65536 bytes at offset 5368709120
> +64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> +
> +[{ "start": 0, "length": 1073741824, "depth": 1, "zero": true, "data": false},
> +{ "start": 1073741824, "length": 7516192768, "depth": 0, "zero": true, "data": false}]
> +
> diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
> index c56bb69031..329c9e4d37 100644
> --- a/tests/qemu-iotests/group
> +++ b/tests/qemu-iotests/group
> @@ -285,3 +285,4 @@
>   270 rw backing quick
>   272 rw
>   273 backing quick
> +274 rw backing quick
> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
> index c4063ef6bb..5fd6d820b1 100644
> --- a/tests/qemu-iotests/iotests.py
> +++ b/tests/qemu-iotests/iotests.py
> @@ -144,7 +144,7 @@ def img_info_log(filename, filter_path=None, imgopts=False, extra_args=[]):
>       output = qemu_img_pipe(*args)
>       if not filter_path:
>           filter_path = filename
> -    log(filter_img_info(output, filter_path))
> +    log(filter_img_info(output, filter_path), filters=[filter_testfiles])
>   
>   def qemu_io(*args):
>       '''Run qemu-io and return the stdout data'''
> 

anyway,

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

-- 
Best regards,
Vladimir

Re: [PATCH for-4.2? 0/6] block: Fix resize (extending) of short overlays

[Vladimir Sementsov-Ogievskiy]

20.11.2019 17:03, Kevin Wolf wrote:
> Consider the following scenario where the overlay is shorter than its
> backing file:
> 
>      base.qcow2:     AAAAAAAA
>      overlay.qcow2:  BBBB
> 
> When resizing (extending) overlay.qcow2, the new blocks should not stay
> unallocated and make the additional As from base.qcow2 visible like
> before this series, but zeros should be read.
> 
> A similar case happens with the various variants of a commit job when an
> intermediate file is short (- for unallocated):
> 
>      base.qcow2:     A-A-AAAA
>      mid.qcow2:      BB-B
>      top.qcow2:      C--C--C-
> 
> After commit top.qcow2 to mid.qcow2, the following happens:
> 
>      mid.qcow2:      CB-C00C0 (correct result)
>      mid.qcow2:      CB-C--C- (before this fix)
> 
> Without the fix, blocks that previously read as zeros on top.qcow2
> suddenly turn into A.

Note, that committing to base should still be broken after these series,
and is fixed in my "[PATCH 0/4] fix & merge block_status_above and is_allocated_above".

I think, I'll post a 5/4 patch, to add this case to your new test.

> 
> Kevin Wolf (6):
>    block: bdrv_co_do_pwrite_zeroes: 64 bit 'bytes' parameter
>    block: truncate: Don't make backing file data visible
>    iotests: Add qemu_io_log()
>    iotests: Fix timeout in run_job()
>    iotests: Support job-complete in run_job()
>    iotests: Test committing to short backing file
> 
>   block/io.c                    |  31 ++++++-
>   tests/qemu-iotests/274        | 131 +++++++++++++++++++++++++++++
>   tests/qemu-iotests/274.out    | 150 ++++++++++++++++++++++++++++++++++
>   tests/qemu-iotests/group      |   1 +
>   tests/qemu-iotests/iotests.py |  11 ++-
>   5 files changed, 319 insertions(+), 5 deletions(-)
>   create mode 100755 tests/qemu-iotests/274
>   create mode 100644 tests/qemu-iotests/274.out
> 


-- 
Best regards,
Vladimir

Re: [PATCH 6/6] iotests: Test committing to short backing file

[Vladimir Sementsov-Ogievskiy]

20.11.2019 17:03, Kevin Wolf wrote:
> Signed-off-by: Kevin Wolf<kwolf@redhat.com>

Tested-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

-- 
Best regards,
Vladimir

Re: [PATCH 6/6] iotests: Test committing to short backing file

[Kevin Wolf]

Am 20.11.2019 um 16:41 hat Vladimir Sementsov-Ogievskiy geschrieben:
> 20.11.2019 17:03, Kevin Wolf wrote:
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> >   tests/qemu-iotests/274        | 131 +++++++++++++++++++++++++++++
> >   tests/qemu-iotests/274.out    | 150 ++++++++++++++++++++++++++++++++++
> >   tests/qemu-iotests/group      |   1 +
> >   tests/qemu-iotests/iotests.py |   2 +-
> >   4 files changed, 283 insertions(+), 1 deletion(-)
> >   create mode 100755 tests/qemu-iotests/274
> >   create mode 100644 tests/qemu-iotests/274.out
> > 
> > diff --git a/tests/qemu-iotests/274 b/tests/qemu-iotests/274
> > new file mode 100755
> > index 0000000000..f3b71e2859
> > --- /dev/null
> > +++ b/tests/qemu-iotests/274
> > @@ -0,0 +1,131 @@
> > +#!/usr/bin/env python
> > +#
> > +# Copyright (C) 2019 Red Hat, Inc.
> > +#
> > +# This program is free software; you can redistribute it and/or modify
> > +# it under the terms of the GNU General Public License as published by
> > +# the Free Software Foundation; either version 2 of the License, or
> > +# (at your option) any later version.
> > +#
> > +# This program is distributed in the hope that it will be useful,
> > +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> > +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > +# GNU General Public License for more details.
> > +#
> > +# You should have received a copy of the GNU General Public License
> > +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> > +#
> > +# Creator/Owner: Kevin Wolf <kwolf@redhat.com>
> > +#
> > +# Some tests for short backing files and short overlays
> > +
> > +import iotests
> > +import os
> > +
> > +iotests.verify_image_format(supported_fmts=['qcow2'])
> > +iotests.verify_platform(['linux'])
> > +
> > +size_short = 1 * 1024 * 1024
> > +size_long = 2 * 1024 * 1024
> > +size_diff = size_long - size_short
> > +
> > +def create_chain():
> > +    iotests.qemu_img_log('create', '-f', iotests.imgfmt, base,
> > +                         str(size_long))
> > +    iotests.qemu_img_log('create', '-f', iotests.imgfmt, '-b', base, mid,
> > +                         str(size_short))
> > +    iotests.qemu_img_log('create', '-f', iotests.imgfmt, '-b', mid, top,
> > +                         str(size_long))
> > +
> > +    iotests.qemu_io_log('-c', 'write -P 1 0 %d' % size_long, base)
> > +
> > +def create_vm():
> > +    vm = iotests.VM()
> > +    vm.add_blockdev('file,filename=%s,node-name=base-file' % (base))
> > +    vm.add_blockdev('%s,file=base-file,node-name=base' % (iotests.imgfmt))
> > +    vm.add_blockdev('file,filename=%s,node-name=mid-file' % (mid))
> > +    vm.add_blockdev('%s,file=mid-file,node-name=mid,backing=base' % (iotests.imgfmt))
> > +    vm.add_drive(top, 'backing=mid,node-name=top')
> > +    return vm
> > +
> > +with iotests.FilePath('base') as base, \
> > +     iotests.FilePath('mid') as mid, \
> > +     iotests.FilePath('top') as top:
> > +
> > +    iotests.log('== Commit tests ==')
> > +
> > +    create_chain()
> > +
> > +    iotests.log('=== Check visible data ===')
> > +
> > +    iotests.qemu_io_log('-c', 'read -P 1 0 %d' % size_short, top)
> > +    iotests.qemu_io_log('-c', 'read -P 0 %d %d' % (size_short, size_diff), top)
> > +
> > +    iotests.log('=== Checking allocation status ===')
> > +
> > +    iotests.qemu_io_log('-c', 'alloc 0 %d' % size_short,
> > +                        '-c', 'alloc %d %d' % (size_short, size_diff),
> > +                        base)
> > +
> > +    iotests.qemu_io_log('-c', 'alloc 0 %d' % size_short,
> > +                        '-c', 'alloc %d %d' % (size_short, size_diff),
> > +                        mid)
> > +
> > +    iotests.qemu_io_log('-c', 'alloc 0 %d' % size_short,
> > +                        '-c', 'alloc %d %d' % (size_short, size_diff),
> > +                        top)
> > +
> > +    iotests.log('=== Checking map ===')
> > +
> > +    iotests.qemu_img_log('map', '--output=json', base)
> > +    iotests.qemu_img_log('map', '--output=human', base)
> > +    iotests.qemu_img_log('map', '--output=json', mid)
> > +    iotests.qemu_img_log('map', '--output=human', mid)
> > +    iotests.qemu_img_log('map', '--output=json', top)
> > +    iotests.qemu_img_log('map', '--output=human', top)
> > +
> > +    iotests.log('=== Testing qemu-img commit (top -> mid) ===')
> > +
> > +    iotests.qemu_img_log('commit', top)
> > +    iotests.img_info_log(mid)
> > +    iotests.qemu_io_log('-c', 'read -P 1 0 %d' % size_short, mid)
> > +    iotests.qemu_io_log('-c', 'read -P 0 %d %d' % (size_short, size_diff), mid)
> > +
> > +    iotests.log('=== Testing HMP commit (top -> mid) ===')
> > +
> > +    create_chain()
> > +    with create_vm() as vm:
> > +        vm.launch()
> > +        vm.qmp_log('human-monitor-command', command_line='commit drive0')
> > +
> > +    iotests.img_info_log(mid)
> > +    iotests.qemu_io_log('-c', 'read -P 1 0 %d' % size_short, mid)
> > +    iotests.qemu_io_log('-c', 'read -P 0 %d %d' % (size_short, size_diff), mid)
> > +
> > +    iotests.log('=== Testing QMP active commit (top -> mid) ===')
> > +
> > +    create_chain()
> > +    with create_vm() as vm:
> > +        vm.launch()
> > +        vm.qmp_log('block-commit', device='top', base_node='mid',
> > +                   job_id='job0', auto_dismiss=False)
> > +        vm.run_job('job0', wait=5)
> > +
> > +    iotests.img_info_log(mid)
> > +    iotests.qemu_io_log('-c', 'read -P 1 0 %d' % size_short, mid)
> > +    iotests.qemu_io_log('-c', 'read -P 0 %d %d' % (size_short, size_diff), mid)
> > +
> > +
> > +    iotests.log('== Resize tests ==')
> > +
> > +    iotests.qemu_img_log('create', '-f', iotests.imgfmt, base, '6G')
> > +    iotests.qemu_img_log('create', '-f', iotests.imgfmt, '-b', base, top, '1G')
> > +    iotests.qemu_io_log('-c', 'write -P 1 3G 64k', base)
> > +    iotests.qemu_io_log('-c', 'write -P 2 5G 64k', base)
> > +
> > +    # After this, 0 to 6 GB should be allocated/zeroed
> > +    # 6 GB to 8 BG should be unallocated
> 
> Hmm, the problem is that the following qemu-img map can't show it, as it merges
> 1G..6G and 6G..8G into one chunk..

Hm, true, because it's more about the content of the blocks than about
the allocation status. I'll add a qemu-io 'map' call, which display the
actual allocation status:

1 GiB (0x40000000) bytes not allocated at offset 0 bytes (0x0)
5 GiB (0x140000000) bytes     allocated at offset 1 GiB (0x40000000)
2 GiB (0x80000000) bytes not allocated at offset 6 GiB (0x180000000)

> > +    iotests.qemu_img_log('resize', '-f', iotests.imgfmt, top, '8G')
> > +    iotests.qemu_io_log('-c', 'read -P 0 3G 64k', top)
> > +    iotests.qemu_io_log('-c', 'read -P 0 5G 64k', top)
> > +    iotests.qemu_img_log('map', '--output=json', top)
> > diff --git a/tests/qemu-iotests/274.out b/tests/qemu-iotests/274.out
> > new file mode 100644
> > index 0000000000..def0ac7d27
> > --- /dev/null
> > +++ b/tests/qemu-iotests/274.out
> > @@ -0,0 +1,150 @@
> > +== Commit tests ==
> > +Formatting 'TEST_DIR/PID-base', fmt=qcow2 size=2097152 cluster_size=65536 lazy_refcounts=off refcount_bits=16
> > +
> > +Formatting 'TEST_DIR/PID-mid', fmt=qcow2 size=1048576 backing_file=TEST_DIR/PID-base cluster_size=65536 lazy_refcounts=off refcount_bits=16
> > +
> > +Formatting 'TEST_DIR/PID-top', fmt=qcow2 size=2097152 backing_file=TEST_DIR/PID-mid cluster_size=65536 lazy_refcounts=off refcount_bits=16
> > +
> > +wrote 2097152/2097152 bytes at offset 0
> > +2 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> > +
> > +=== Check visible data ===
> > +read 1048576/1048576 bytes at offset 0
> > +1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> > +
> > +read 1048576/1048576 bytes at offset 1048576
> > +1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> > +
> > +=== Checking allocation status ===
> > +1048576/1048576 bytes allocated at offset 0 bytes
> > +1048576/1048576 bytes allocated at offset 1 MiB
> > +
> > +0/1048576 bytes allocated at offset 0 bytes
> > +0/0 bytes allocated at offset 1 MiB
> > +
> > +0/1048576 bytes allocated at offset 0 bytes
> > +0/1048576 bytes allocated at offset 1 MiB
> > +
> > +=== Checking map ===
> > +[{ "start": 0, "length": 2097152, "depth": 0, "zero": false, "data": true, "offset": 327680}]
> > +
> > +Offset          Length          Mapped to       File
> > +0               0x200000        0x50000         TEST_DIR/PID-base
> > +
> > +[{ "start": 0, "length": 1048576, "depth": 1, "zero": false, "data": true, "offset": 327680}]
> > +
> > +Offset          Length          Mapped to       File
> > +0               0x100000        0x50000         TEST_DIR/PID-base
> > +
> > +[{ "start": 0, "length": 1048576, "depth": 2, "zero": false, "data": true, "offset": 327680},
> > +{ "start": 1048576, "length": 1048576, "depth": 0, "zero": true, "data": false}]
> 
> I think depth of second chunk should be 1, not 0.. But this is for
> another fixing series.

The part from 1 GB to 6 GB should be 0 without any question, this is
where we wrote zeros into the overlay.

The part from 7 GB to 8 GB is a bit more open to interpretation because
this is unallocated in the overlay and reads zeros because the backing
file is shorter. I think 0 makes sense, but it's debatable.

Kevin

Re: [PATCH 1/6] block: bdrv_co_do_pwrite_zeroes: 64 bit 'bytes' parameter

[Alberto Garcia]

On Wed 20 Nov 2019 03:03:14 PM CET, Kevin Wolf wrote:
> bdrv_co_do_pwrite_zeroes() can already cope with maximum request sizes
> by calling the driver in a loop until everything is done. Make the small
> remaining change that is necessary to let it accept a 64 bit byte count.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>

Reviewed-by: Alberto Garcia <berto@igalia.com>

Berto

Re: [PATCH 6/6] iotests: Test committing to short backing file

[Vladimir Sementsov-Ogievskiy]

20.11.2019 19:11, Kevin Wolf wrote:
> Am 20.11.2019 um 16:41 hat Vladimir Sementsov-Ogievskiy geschrieben:
>> 20.11.2019 17:03, Kevin Wolf wrote:
>>> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>>> ---
>>>    tests/qemu-iotests/274        | 131 +++++++++++++++++++++++++++++
>>>    tests/qemu-iotests/274.out    | 150 ++++++++++++++++++++++++++++++++++
>>>    tests/qemu-iotests/group      |   1 +
>>>    tests/qemu-iotests/iotests.py |   2 +-
>>>    4 files changed, 283 insertions(+), 1 deletion(-)
>>>    create mode 100755 tests/qemu-iotests/274
>>>    create mode 100644 tests/qemu-iotests/274.out
>>>
>>> diff --git a/tests/qemu-iotests/274 b/tests/qemu-iotests/274
>>> new file mode 100755
>>> index 0000000000..f3b71e2859
>>> --- /dev/null
>>> +++ b/tests/qemu-iotests/274
>>> @@ -0,0 +1,131 @@
>>> +#!/usr/bin/env python
>>> +#
>>> +# Copyright (C) 2019 Red Hat, Inc.
>>> +#
>>> +# This program is free software; you can redistribute it and/or modify
>>> +# it under the terms of the GNU General Public License as published by
>>> +# the Free Software Foundation; either version 2 of the License, or
>>> +# (at your option) any later version.
>>> +#
>>> +# This program is distributed in the hope that it will be useful,
>>> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
>>> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>>> +# GNU General Public License for more details.
>>> +#
>>> +# You should have received a copy of the GNU General Public License
>>> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
>>> +#
>>> +# Creator/Owner: Kevin Wolf <kwolf@redhat.com>
>>> +#
>>> +# Some tests for short backing files and short overlays
>>> +
>>> +import iotests
>>> +import os
>>> +
>>> +iotests.verify_image_format(supported_fmts=['qcow2'])
>>> +iotests.verify_platform(['linux'])
>>> +
>>> +size_short = 1 * 1024 * 1024
>>> +size_long = 2 * 1024 * 1024
>>> +size_diff = size_long - size_short
>>> +
>>> +def create_chain():
>>> +    iotests.qemu_img_log('create', '-f', iotests.imgfmt, base,
>>> +                         str(size_long))
>>> +    iotests.qemu_img_log('create', '-f', iotests.imgfmt, '-b', base, mid,
>>> +                         str(size_short))
>>> +    iotests.qemu_img_log('create', '-f', iotests.imgfmt, '-b', mid, top,
>>> +                         str(size_long))
>>> +
>>> +    iotests.qemu_io_log('-c', 'write -P 1 0 %d' % size_long, base)
>>> +
>>> +def create_vm():
>>> +    vm = iotests.VM()
>>> +    vm.add_blockdev('file,filename=%s,node-name=base-file' % (base))
>>> +    vm.add_blockdev('%s,file=base-file,node-name=base' % (iotests.imgfmt))
>>> +    vm.add_blockdev('file,filename=%s,node-name=mid-file' % (mid))
>>> +    vm.add_blockdev('%s,file=mid-file,node-name=mid,backing=base' % (iotests.imgfmt))
>>> +    vm.add_drive(top, 'backing=mid,node-name=top')
>>> +    return vm
>>> +
>>> +with iotests.FilePath('base') as base, \
>>> +     iotests.FilePath('mid') as mid, \
>>> +     iotests.FilePath('top') as top:
>>> +
>>> +    iotests.log('== Commit tests ==')
>>> +
>>> +    create_chain()
>>> +
>>> +    iotests.log('=== Check visible data ===')
>>> +
>>> +    iotests.qemu_io_log('-c', 'read -P 1 0 %d' % size_short, top)
>>> +    iotests.qemu_io_log('-c', 'read -P 0 %d %d' % (size_short, size_diff), top)
>>> +
>>> +    iotests.log('=== Checking allocation status ===')
>>> +
>>> +    iotests.qemu_io_log('-c', 'alloc 0 %d' % size_short,
>>> +                        '-c', 'alloc %d %d' % (size_short, size_diff),
>>> +                        base)
>>> +
>>> +    iotests.qemu_io_log('-c', 'alloc 0 %d' % size_short,
>>> +                        '-c', 'alloc %d %d' % (size_short, size_diff),
>>> +                        mid)
>>> +
>>> +    iotests.qemu_io_log('-c', 'alloc 0 %d' % size_short,
>>> +                        '-c', 'alloc %d %d' % (size_short, size_diff),
>>> +                        top)
>>> +
>>> +    iotests.log('=== Checking map ===')
>>> +
>>> +    iotests.qemu_img_log('map', '--output=json', base)
>>> +    iotests.qemu_img_log('map', '--output=human', base)
>>> +    iotests.qemu_img_log('map', '--output=json', mid)
>>> +    iotests.qemu_img_log('map', '--output=human', mid)
>>> +    iotests.qemu_img_log('map', '--output=json', top)
>>> +    iotests.qemu_img_log('map', '--output=human', top)
>>> +
>>> +    iotests.log('=== Testing qemu-img commit (top -> mid) ===')
>>> +
>>> +    iotests.qemu_img_log('commit', top)
>>> +    iotests.img_info_log(mid)
>>> +    iotests.qemu_io_log('-c', 'read -P 1 0 %d' % size_short, mid)
>>> +    iotests.qemu_io_log('-c', 'read -P 0 %d %d' % (size_short, size_diff), mid)
>>> +
>>> +    iotests.log('=== Testing HMP commit (top -> mid) ===')
>>> +
>>> +    create_chain()
>>> +    with create_vm() as vm:
>>> +        vm.launch()
>>> +        vm.qmp_log('human-monitor-command', command_line='commit drive0')
>>> +
>>> +    iotests.img_info_log(mid)
>>> +    iotests.qemu_io_log('-c', 'read -P 1 0 %d' % size_short, mid)
>>> +    iotests.qemu_io_log('-c', 'read -P 0 %d %d' % (size_short, size_diff), mid)
>>> +
>>> +    iotests.log('=== Testing QMP active commit (top -> mid) ===')
>>> +
>>> +    create_chain()
>>> +    with create_vm() as vm:
>>> +        vm.launch()
>>> +        vm.qmp_log('block-commit', device='top', base_node='mid',
>>> +                   job_id='job0', auto_dismiss=False)
>>> +        vm.run_job('job0', wait=5)
>>> +
>>> +    iotests.img_info_log(mid)
>>> +    iotests.qemu_io_log('-c', 'read -P 1 0 %d' % size_short, mid)
>>> +    iotests.qemu_io_log('-c', 'read -P 0 %d %d' % (size_short, size_diff), mid)
>>> +
>>> +
>>> +    iotests.log('== Resize tests ==')
>>> +
>>> +    iotests.qemu_img_log('create', '-f', iotests.imgfmt, base, '6G')
>>> +    iotests.qemu_img_log('create', '-f', iotests.imgfmt, '-b', base, top, '1G')
>>> +    iotests.qemu_io_log('-c', 'write -P 1 3G 64k', base)
>>> +    iotests.qemu_io_log('-c', 'write -P 2 5G 64k', base)
>>> +
>>> +    # After this, 0 to 6 GB should be allocated/zeroed
>>> +    # 6 GB to 8 BG should be unallocated
>>
>> Hmm, the problem is that the following qemu-img map can't show it, as it merges
>> 1G..6G and 6G..8G into one chunk..
> 
> Hm, true, because it's more about the content of the blocks than about
> the allocation status. I'll add a qemu-io 'map' call, which display the
> actual allocation status:
> 
> 1 GiB (0x40000000) bytes not allocated at offset 0 bytes (0x0)
> 5 GiB (0x140000000) bytes     allocated at offset 1 GiB (0x40000000)
> 2 GiB (0x80000000) bytes not allocated at offset 6 GiB (0x180000000)
> 
>>> +    iotests.qemu_img_log('resize', '-f', iotests.imgfmt, top, '8G')
>>> +    iotests.qemu_io_log('-c', 'read -P 0 3G 64k', top)
>>> +    iotests.qemu_io_log('-c', 'read -P 0 5G 64k', top)
>>> +    iotests.qemu_img_log('map', '--output=json', top)
>>> diff --git a/tests/qemu-iotests/274.out b/tests/qemu-iotests/274.out
>>> new file mode 100644
>>> index 0000000000..def0ac7d27
>>> --- /dev/null
>>> +++ b/tests/qemu-iotests/274.out
>>> @@ -0,0 +1,150 @@
>>> +== Commit tests ==
>>> +Formatting 'TEST_DIR/PID-base', fmt=qcow2 size=2097152 cluster_size=65536 lazy_refcounts=off refcount_bits=16
>>> +
>>> +Formatting 'TEST_DIR/PID-mid', fmt=qcow2 size=1048576 backing_file=TEST_DIR/PID-base cluster_size=65536 lazy_refcounts=off refcount_bits=16
>>> +
>>> +Formatting 'TEST_DIR/PID-top', fmt=qcow2 size=2097152 backing_file=TEST_DIR/PID-mid cluster_size=65536 lazy_refcounts=off refcount_bits=16
>>> +
>>> +wrote 2097152/2097152 bytes at offset 0
>>> +2 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
>>> +
>>> +=== Check visible data ===
>>> +read 1048576/1048576 bytes at offset 0
>>> +1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
>>> +
>>> +read 1048576/1048576 bytes at offset 1048576
>>> +1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
>>> +
>>> +=== Checking allocation status ===
>>> +1048576/1048576 bytes allocated at offset 0 bytes
>>> +1048576/1048576 bytes allocated at offset 1 MiB
>>> +
>>> +0/1048576 bytes allocated at offset 0 bytes
>>> +0/0 bytes allocated at offset 1 MiB
>>> +
>>> +0/1048576 bytes allocated at offset 0 bytes
>>> +0/1048576 bytes allocated at offset 1 MiB
>>> +
>>> +=== Checking map ===
>>> +[{ "start": 0, "length": 2097152, "depth": 0, "zero": false, "data": true, "offset": 327680}]
>>> +
>>> +Offset          Length          Mapped to       File
>>> +0               0x200000        0x50000         TEST_DIR/PID-base
>>> +
>>> +[{ "start": 0, "length": 1048576, "depth": 1, "zero": false, "data": true, "offset": 327680}]
>>> +
>>> +Offset          Length          Mapped to       File
>>> +0               0x100000        0x50000         TEST_DIR/PID-base
>>> +
>>> +[{ "start": 0, "length": 1048576, "depth": 2, "zero": false, "data": true, "offset": 327680},
>>> +{ "start": 1048576, "length": 1048576, "depth": 0, "zero": true, "data": false}]
>>
>> I think depth of second chunk should be 1, not 0.. But this is for
>> another fixing series.
> 
> The part from 1 GB to 6 GB should be 0 without any question, this is
> where we wrote zeros into the overlay.
> 
> The part from 7 GB to 8 GB is a bit more open to interpretation because
> this is unallocated in the overlay and reads zeros because the backing
> file is shorter. I think 0 makes sense, but it's debatable.
> 
> Kevin
> 

Here I'm not about last test-case but about "depth" in second element of map, which covers [1M,2M) region.
Zeroes comes from second layer (after EOF), not from first. On first this are is unallocated in all possible meanings.
Still, I doubt that "depth" in qemu-img map output is something well-documented, so it's not significant.

-- 
Best regards,
Vladimir

Re: [PATCH 2/6] block: truncate: Don't make backing file data visible

[Vladimir Sementsov-Ogievskiy]

20.11.2019 17:03, Kevin Wolf wrote:
> When extending the size of an image that has a backing file larger than
> its old size, make sure that the backing file data doesn't become
> visible in the guest, but the added area is properly zeroed out.
> 
> The old behaviour made a difference in 'block_resize' (where showing the
> backing file data from an old snapshot rather than zeros is
> questionable) as well as in commit block jobs (both from active and
> intermediate nodes) and HMP 'commit', where committing to a short
> backing file would incorrectly omit writing zeroes for unallocated
> blocks on the top layer after the EOF of the short backing file.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>   block/io.c | 25 +++++++++++++++++++++++++
>   1 file changed, 25 insertions(+)
> 
> diff --git a/block/io.c b/block/io.c
> index 003f4ea38c..8683f7a4bd 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -3382,6 +3382,31 @@ int coroutine_fn bdrv_co_truncate(BdrvChild *child, int64_t offset, bool exact,
>           goto out;
>       }
>   
> +    /*
> +     * If the image has a backing file that is large enough that it would
> +     * provide data for the new area, we cannot leave it unallocated because
> +     * then the backing file content would become visible. Instead, zero-fill
> +     * the area where backing file and new area overlap.
> +     */
> +    if (new_bytes && bs->backing && prealloc == PREALLOC_MODE_OFF) {
> +        int64_t backing_len;
> +
> +        backing_len = bdrv_getlength(backing_bs(bs));
> +        if (backing_len < 0) {
> +            ret = backing_len;
> +            goto out;
> +        }
> +
> +        if (backing_len > old_size) {
> +            ret = bdrv_co_do_pwrite_zeroes(bs, old_size,
> +                                           MIN(new_bytes, backing_len - old_size),
> +                                           BDRV_REQ_ZERO_WRITE | BDRV_REQ_MAY_UNMAP);
> +            if (ret < 0) {
> +                goto out;
> +            }
> +        }
> +    }
> +
>       ret = refresh_total_sectors(bs, offset >> BDRV_SECTOR_BITS);
>       if (ret < 0) {
>           error_setg_errno(errp, -ret, "Could not refresh total sector count");
> 


Hmmmm. I'm think that for commit, we also should zero truncated area if !bdrv_has_zero_init_truncate(bs).
But we should not do it here, as it should not be done if we just resizing disk..

What formats are that bad?

-- 
Best regards,
Vladimir

Re: [PATCH 2/6] block: truncate: Don't make backing file data visible

[Kevin Wolf]

Am 20.11.2019 um 19:01 hat Vladimir Sementsov-Ogievskiy geschrieben:
> 20.11.2019 17:03, Kevin Wolf wrote:
> > When extending the size of an image that has a backing file larger than
> > its old size, make sure that the backing file data doesn't become
> > visible in the guest, but the added area is properly zeroed out.
> > 
> > The old behaviour made a difference in 'block_resize' (where showing the
> > backing file data from an old snapshot rather than zeros is
> > questionable) as well as in commit block jobs (both from active and
> > intermediate nodes) and HMP 'commit', where committing to a short
> > backing file would incorrectly omit writing zeroes for unallocated
> > blocks on the top layer after the EOF of the short backing file.
> > 
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> >   block/io.c | 25 +++++++++++++++++++++++++
> >   1 file changed, 25 insertions(+)
> > 
> > diff --git a/block/io.c b/block/io.c
> > index 003f4ea38c..8683f7a4bd 100644
> > --- a/block/io.c
> > +++ b/block/io.c
> > @@ -3382,6 +3382,31 @@ int coroutine_fn bdrv_co_truncate(BdrvChild *child, int64_t offset, bool exact,
> >           goto out;
> >       }
> >   
> > +    /*
> > +     * If the image has a backing file that is large enough that it would
> > +     * provide data for the new area, we cannot leave it unallocated because
> > +     * then the backing file content would become visible. Instead, zero-fill
> > +     * the area where backing file and new area overlap.
> > +     */
> > +    if (new_bytes && bs->backing && prealloc == PREALLOC_MODE_OFF) {
> > +        int64_t backing_len;
> > +
> > +        backing_len = bdrv_getlength(backing_bs(bs));
> > +        if (backing_len < 0) {
> > +            ret = backing_len;
> > +            goto out;
> > +        }
> > +
> > +        if (backing_len > old_size) {
> > +            ret = bdrv_co_do_pwrite_zeroes(bs, old_size,
> > +                                           MIN(new_bytes, backing_len - old_size),
> > +                                           BDRV_REQ_ZERO_WRITE | BDRV_REQ_MAY_UNMAP);
> > +            if (ret < 0) {
> > +                goto out;
> > +            }
> > +        }
> > +    }
> > +
> >       ret = refresh_total_sectors(bs, offset >> BDRV_SECTOR_BITS);
> >       if (ret < 0) {
> >           error_setg_errno(errp, -ret, "Could not refresh total sector count");
> 
> Hmmmm. I'm think that for commit, we also should zero truncated area
> if !bdrv_has_zero_init_truncate(bs). But we should not do it here, as
> it should not be done if we just resizing disk..

Hm, yes, we need to do this for METADATA and FALLOC preallocation at
least. I think we already guarantee zeros for FULL, do we?

Resize needs zero init in the opposite case: When you are resizing a
short backing file, the longer overlay still needs to read the same
zeros it read before after EOF of the backing file. This one sounds
actually even nastier to fix than what this series does. :-/

Anyway, maybe instead of the no_fallback parameter I introduced in v3,
what we really want is a need_zero_init parameter that only commit jobs
set for now? Or actually add a new preallocation mode like you suggested
that would add a zero write and pass OFF to the driver implementations.
Then we wouldn't have to add a new parameter everywhere.

We'd still unconditionally write zeros where it's necessary to allocate
blocks to cover the backing file (and to provide correct data to the
overlay if we ever figure out how to check this condition). I think I've
come to the conclusion that blocking on block_resize is better than
failing.

> What formats are that bad?

You mean that they don't have zero init? The usual suspect for bad image
formats is raw, but fortunately that doesn't support backing files. So
maybe it's not a problem we would see in practice.

Kevin