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

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

[Kevin Wolf]

See patch 2 for the description of the bug fixed.

v2:
- Switched order of bs->total_sectors update and zero write [Vladimir]
- Fixed coding style [Vladimir]
- Changed the commit message to contain what was in the cover letter
- Test all preallocation modes
- Test allocation status with qemu-io 'map' [Vladimir]

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                    |  38 +++++-
 tests/qemu-iotests/274        | 141 +++++++++++++++++++++
 tests/qemu-iotests/274.out    | 227 ++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/group      |   1 +
 tests/qemu-iotests/iotests.py |  11 +-
 5 files changed, 413 insertions(+), 5 deletions(-)
 create mode 100755 tests/qemu-iotests/274
 create mode 100644 tests/qemu-iotests/274.out

-- 
2.20.1

[PATCH v2 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>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Alberto Garcia <berto@igalia.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 v2 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.

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 patch, 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.

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

diff --git a/block/io.c b/block/io.c
index 003f4ea38c..6a5144f8d2 100644
--- a/block/io.c
+++ b/block/io.c
@@ -3385,12 +3385,44 @@ int coroutine_fn bdrv_co_truncate(BdrvChild *child, int64_t offset, bool exact,
     ret = refresh_total_sectors(bs, offset >> BDRV_SECTOR_BITS);
     if (ret < 0) {
         error_setg_errno(errp, -ret, "Could not refresh total sector count");
+        goto fail_refresh_total_sectors;
     } else {
         offset = bs->total_sectors * BDRV_SECTOR_SIZE;
     }
+
+    /*
+     * 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.
+     *
+     * Note that if the image has a backing file, but was opened without the
+     * backing file, taking care of keeping things consistent with that backing
+     * file is the user's responsibility.
+     */
+    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;
+            }
+        }
+    }
+
     /* It's possible that truncation succeeded but refresh_total_sectors
      * failed, but the latter doesn't affect how we should finish the request.
      * Pass 0 as the last parameter so that dirty bitmaps etc. are handled. */
+fail_refresh_total_sectors:
     bdrv_co_write_req_finish(child, offset - new_bytes, new_bytes, &req, 0);
 
 out:
-- 
2.20.1

[PATCH v2 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>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.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 v2 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>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.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 v2 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>
Reviewed-by: Eric Blake <eblake@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()
-- 
2.20.1

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

[Kevin Wolf]

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 tests/qemu-iotests/274        | 141 +++++++++++++++++++++
 tests/qemu-iotests/274.out    | 227 ++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/group      |   1 +
 tests/qemu-iotests/iotests.py |   2 +-
 4 files changed, 370 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..0f2ef87327
--- /dev/null
+++ b/tests/qemu-iotests/274
@@ -0,0 +1,141 @@
+#!/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 ==')
+
+    for prealloc in ['off', 'metadata', 'falloc', 'full']:
+
+        iotests.log('=== preallocation=%s ===' % prealloc)
+        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 with preallocation=off and
+        # allocated with preallocation enabled
+        iotests.qemu_img_log('resize', '-f', iotests.imgfmt,
+                             '--preallocation', prealloc, top, '8G')
+        iotests.qemu_io_log('-c', 'read -P 0 3G 64k', top)
+        iotests.qemu_io_log('-c', 'read -P 0 5G 64k', top)
+
+        # Metadata preallocation doesn't have a defined result on the file
+        # system level with respect to holes, so skip it here
+        iotests.qemu_io_log('-c', 'map', top)
+        if prealloc != 'metadata':
+            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..4f9bea332a
--- /dev/null
+++ b/tests/qemu-iotests/274.out
@@ -0,0 +1,227 @@
+== 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 ==
+=== preallocation=off ===
+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)
+
+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)
+
+[{ "start": 0, "length": 1073741824, "depth": 1, "zero": true, "data": false},
+{ "start": 1073741824, "length": 7516192768, "depth": 0, "zero": true, "data": false}]
+
+=== preallocation=metadata ===
+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)
+
+1 GiB (0x40000000) bytes not allocated at offset 0 bytes (0x0)
+7 GiB (0x1c0000000) bytes     allocated at offset 1 GiB (0x40000000)
+
+=== preallocation=falloc ===
+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)
+
+1 GiB (0x40000000) bytes not allocated at offset 0 bytes (0x0)
+7 GiB (0x1c0000000) bytes     allocated at offset 1 GiB (0x40000000)
+
+[{ "start": 0, "length": 1073741824, "depth": 1, "zero": true, "data": false},
+{ "start": 1073741824, "length": 7516192768, "depth": 0, "zero": false, "data": true, "offset": 589824}]
+
+=== preallocation=full ===
+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)
+
+1 GiB (0x40000000) bytes not allocated at offset 0 bytes (0x0)
+7 GiB (0x1c0000000) bytes     allocated at offset 1 GiB (0x40000000)
+
+[{ "start": 0, "length": 1073741824, "depth": 1, "zero": true, "data": false},
+{ "start": 1073741824, "length": 7516192768, "depth": 0, "zero": false, "data": true, "offset": 589824}]
+
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index c56bb69031..848a578782 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
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 v2 2/6] block: truncate: Don't make backing file data visible

[Eric Blake]

On 11/20/19 12:44 PM, 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.
> 
> 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 patch, 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.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>   block/io.c | 32 ++++++++++++++++++++++++++++++++
>   1 file changed, 32 insertions(+)
> 

> +    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;
> +            }
> +        }
> +    }

Note that if writing zeroes is not fast, and it turns out that we copy a 
lot of data rather than unallocated sections from the image being 
committed, that this can actually slow things down (doing a bulk 
pre-zero doubles up data I/O unless it is fast, which is why we added 
BDRV_REQ_NO_FALLBACK to avoid slow pre-zeroing).  However, the 
complication of zeroing only the unallocated clusters rather than a bulk 
pre-zeroing for something that is an unlikely corner case (how often do 
you create an overlay shorter than the backing file?) is not worth the 
extra code maintenance (unlike in the 'qemu-img convert' case where it 
was worth the optimization). So I'm fine with how you fixed it here.

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 v2 6/6] iotests: Test committing to short backing file

[Eric Blake]

On 11/20/19 12:45 PM, Kevin Wolf wrote:
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>   tests/qemu-iotests/274        | 141 +++++++++++++++++++++
>   tests/qemu-iotests/274.out    | 227 ++++++++++++++++++++++++++++++++++
>   tests/qemu-iotests/group      |   1 +
>   tests/qemu-iotests/iotests.py |   2 +-
>   4 files changed, 370 insertions(+), 1 deletion(-)
>   create mode 100755 tests/qemu-iotests/274
>   create mode 100644 tests/qemu-iotests/274.out
> 


> +    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)
> +

Would it also be worth testing a commit of mid -> base, and showing that 
top still sees the same contents afterwards?

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 v2 2/6] block: truncate: Don't make backing file data visible

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 1589 bytes --]

On 20.11.19 19:44, 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.
> 
> 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 patch, 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.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/io.c | 32 ++++++++++++++++++++++++++++++++
>  1 file changed, 32 insertions(+)

Zeroing the intersection may take some time.  So is it right for QMP’s
block_resize to do this, seeing it is a synchronous operation?

As far as I can tell, jobs actually have the same problem.  I don’t
think mirror or commit have a pause point before truncating, so they
still block the monitor there, don’t they?

Max


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

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

[Vladimir Sementsov-Ogievskiy]

21.11.2019 0:27, Eric Blake wrote:
> On 11/20/19 12:45 PM, Kevin Wolf wrote:
>> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>> ---
>>   tests/qemu-iotests/274        | 141 +++++++++++++++++++++
>>   tests/qemu-iotests/274.out    | 227 ++++++++++++++++++++++++++++++++++
>>   tests/qemu-iotests/group      |   1 +
>>   tests/qemu-iotests/iotests.py |   2 +-
>>   4 files changed, 370 insertions(+), 1 deletion(-)
>>   create mode 100755 tests/qemu-iotests/274
>>   create mode 100644 tests/qemu-iotests/274.out
>>
> 
> 
>> +    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)
>> +
> 
> Would it also be worth testing a commit of mid -> base, and showing that top still sees the same contents afterwards?
> 

This is just still broken, see may "[PATCH 0/4] fix & merge block_status_above and is_allocated_above" series.


-- 
Best regards,
Vladimir

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

[Vladimir Sementsov-Ogievskiy]

21.11.2019 11:59, Max Reitz wrote:
> On 20.11.19 19:44, 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.
>>
>> 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 patch, 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.
>>
>> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>> ---
>>   block/io.c | 32 ++++++++++++++++++++++++++++++++
>>   1 file changed, 32 insertions(+)
> 
> Zeroing the intersection may take some time.  So is it right for QMP’s
> block_resize to do this, seeing it is a synchronous operation?
> 
> As far as I can tell, jobs actually have the same problem.  I don’t
> think mirror or commit have a pause point before truncating, so they
> still block the monitor there, don’t they?
> 
> Max
> 

For mirror there is alternative way: we may somehow cheat with block-status
to consider space after EOF as allocated on upper level, then mirror will
ZERO it by itself..

-- 
Best regards,
Vladimir

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

[Vladimir Sementsov-Ogievskiy]

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


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

-- 
Best regards,
Vladimir

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

[Vladimir Sementsov-Ogievskiy]

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

Hmm, allocating 7G will break tests on small disks, for example,
on my 2G tmpfs.

So, we probably should
detect "+qemu-img: Failed to resize underlying file: Could not write zeros for preallocation: No space left on device"
errors and skip test. (better skip testcases, but it's not possible for text-comparing tests :(


-- 
Best regards,
Vladimir

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

[Vladimir Sementsov-Ogievskiy]

21.11.2019 13:28, Vladimir Sementsov-Ogievskiy wrote:
> 20.11.2019 21:45, Kevin Wolf wrote:
>> Signed-off-by: Kevin Wolf<kwolf@redhat.com>
> 
> Hmm, allocating 7G will break tests on small disks, for example,
> on my 2G tmpfs.
> 
> So, we probably should
> detect "+qemu-img: Failed to resize underlying file: Could not write zeros for preallocation: No space left on device"
> errors and skip test. (better skip testcases, but it's not possible for text-comparing tests :(
> 
> 

Or could you just use smaller disks for resize? What is the purpose of such a big size?

-- 
Best regards,
Vladimir

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

[Kevin Wolf]

Am 20.11.2019 um 22:15 hat Eric Blake geschrieben:
> On 11/20/19 12:44 PM, 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.
> > 
> > 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 patch, 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.
> > 
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> >   block/io.c | 32 ++++++++++++++++++++++++++++++++
> >   1 file changed, 32 insertions(+)
> > 
> 
> > +    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;
> > +            }
> > +        }
> > +    }
> 
> Note that if writing zeroes is not fast, and it turns out that we copy a lot
> of data rather than unallocated sections from the image being committed,
> that this can actually slow things down (doing a bulk pre-zero doubles up
> data I/O unless it is fast, which is why we added BDRV_REQ_NO_FALLBACK to
> avoid slow pre-zeroing).  However, the complication of zeroing only the
> unallocated clusters rather than a bulk pre-zeroing

I'm not sure how there could already be any allocated clusters in the
area that we just added? (Apart from preallocation, of course, which is
why this is restricted to PREALLOC_MODE_OFF.)

You're right that if this truncate was called in the context of a commit
block job rather than a resize, the zeros might be overwritten later.
I'm not sure if leaving clusters unallocated while the job is running
(and might be cancelled) is right, though. On the other hand, while the
job is running, the target image on its own is invalid anyway.

> for something that is an unlikely corner case (how often do you create
> an overlay shorter than the backing file?) is not worth the extra code
> maintenance (unlike in the 'qemu-img convert' case where it was worth
> the optimization). So I'm fine with how you fixed it here.

Also note that raw doesn't support backing files and qcow2 generally
supports zero writes. If you use an external data file and backing file
at the same time and your file system doesn't support zero writes, you
could run into the problem. Or if you use a more unusual image format
(including qcow2 v2).

So it's really two corner cases combined.

Kevin

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

[Kevin Wolf]

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

Am 21.11.2019 um 09:59 hat Max Reitz geschrieben:
> On 20.11.19 19:44, 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.
> > 
> > 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 patch, 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.
> > 
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> >  block/io.c | 32 ++++++++++++++++++++++++++++++++
> >  1 file changed, 32 insertions(+)
> 
> Zeroing the intersection may take some time.  So is it right for QMP’s
> block_resize to do this, seeing it is a synchronous operation?

What else would be right? Returning an error?

Common cases (raw and qcow2 v3 without external data files) are quick
anyway.

> As far as I can tell, jobs actually have the same problem.  I don’t
> think mirror or commit have a pause point before truncating, so they
> still block the monitor there, don’t they?

Do you really need a pause point? They call bdrv_co_truncate() from
inside the job coroutine, so it will yield. I would expect that this
is enough.

But in fact, all jobs have a pause point before even calling .run(), so
even if that made a difference, it should still be fine.

Kevin

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

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

[Kevin Wolf]

Am 21.11.2019 um 11:30 hat Vladimir Sementsov-Ogievskiy geschrieben:
> 21.11.2019 13:28, Vladimir Sementsov-Ogievskiy wrote:
> > 20.11.2019 21:45, Kevin Wolf wrote:
> >> Signed-off-by: Kevin Wolf<kwolf@redhat.com>
> > 
> > Hmm, allocating 7G will break tests on small disks, for example,
> > on my 2G tmpfs.
> > 
> > So, we probably should
> > detect "+qemu-img: Failed to resize underlying file: Could not write zeros for preallocation: No space left on device"
> > errors and skip test. (better skip testcases, but it's not possible for text-comparing tests :(
> 
> Or could you just use smaller disks for resize? What is the purpose of such a big size?

I want to exceed a 32 bit byte count for the write_zeroes operation so
that it would break if patch 1 were missing. I guess I could reduce it
to a little over 4 GB, but not less.

Hm, though that is only for preallocation=off, which shouldn't actually
allocate the space anyway. I could use smaller sizes for falloc and
full, even though I'm not sure if we're still testing these modes with
sizes larger than INT_MAX anywhere.

Kevin

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

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 2589 bytes --]

On 21.11.19 12:34, Kevin Wolf wrote:
> Am 21.11.2019 um 09:59 hat Max Reitz geschrieben:
>> On 20.11.19 19:44, 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.
>>>
>>> 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 patch, 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.
>>>
>>> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>>> ---
>>>  block/io.c | 32 ++++++++++++++++++++++++++++++++
>>>  1 file changed, 32 insertions(+)
>>
>> Zeroing the intersection may take some time.  So is it right for QMP’s
>> block_resize to do this, seeing it is a synchronous operation?
> 
> What else would be right? Returning an error?

Going through a deprecation cycle.

> Common cases (raw and qcow2 v3 without external data files) are quick
> anyway.

Well, but quick enough for a fully blocking operation?

>> As far as I can tell, jobs actually have the same problem.  I don’t
>> think mirror or commit have a pause point before truncating, so they
>> still block the monitor there, don’t they?
> 
> Do you really need a pause point? They call bdrv_co_truncate() from
> inside the job coroutine, so it will yield. I would expect that this
> is enough.

OK then.

> But in fact, all jobs have a pause point before even calling .run(), so
> even if that made a difference, it should still be fine.

Good.

But I believe this is still a problem for block_resize.  I don’t see why
this needs to be fixed in 4.2-rc3.  What’s the problem with going
through a proper deprecation cycle other than the fact that we can’t
start it in 4.2 because we don’t have a resize job yet?

Max


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

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

[Vladimir Sementsov-Ogievskiy]

21.11.2019 14:39, Kevin Wolf wrote:
> Am 21.11.2019 um 11:30 hat Vladimir Sementsov-Ogievskiy geschrieben:
>> 21.11.2019 13:28, Vladimir Sementsov-Ogievskiy wrote:
>>> 20.11.2019 21:45, Kevin Wolf wrote:
>>>> Signed-off-by: Kevin Wolf<kwolf@redhat.com>
>>>
>>> Hmm, allocating 7G will break tests on small disks, for example,
>>> on my 2G tmpfs.
>>>
>>> So, we probably should
>>> detect "+qemu-img: Failed to resize underlying file: Could not write zeros for preallocation: No space left on device"
>>> errors and skip test. (better skip testcases, but it's not possible for text-comparing tests :(
>>
>> Or could you just use smaller disks for resize? What is the purpose of such a big size?
> 
> I want to exceed a 32 bit byte count for the write_zeroes operation so
> that it would break if patch 1 were missing. I guess I could reduce it
> to a little over 4 GB, but not less.
> 
> Hm, though that is only for preallocation=off, which shouldn't actually
> allocate the space anyway. I could use smaller sizes for falloc and
> full, even though I'm not sure if we're still testing these modes with
> sizes larger than INT_MAX anywhere.
> 

Yes, preallocation=off works for me with big disk, so this should work.


-- 
Best regards,
Vladimir

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

[Alberto Garcia]

On Wed 20 Nov 2019 07:44:59 PM CET, 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: Eric Blake <eblake@redhat.com>
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

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

Berto

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

[Alberto Garcia]

On Wed 20 Nov 2019 07:45:00 PM CET, 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: Eric Blake <eblake@redhat.com>
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

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

Berto

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

[Alberto Garcia]

On Wed 20 Nov 2019 07:44:58 PM CET, 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: Eric Blake <eblake@redhat.com>
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

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

Berto

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

[Stefan Hajnoczi]

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

On Wed, Nov 20, 2019 at 07:44:55PM +0100, Kevin Wolf wrote:
> See patch 2 for the description of the bug fixed.
> 
> v2:
> - Switched order of bs->total_sectors update and zero write [Vladimir]
> - Fixed coding style [Vladimir]
> - Changed the commit message to contain what was in the cover letter
> - Test all preallocation modes
> - Test allocation status with qemu-io 'map' [Vladimir]
> 
> 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                    |  38 +++++-
>  tests/qemu-iotests/274        | 141 +++++++++++++++++++++
>  tests/qemu-iotests/274.out    | 227 ++++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/group      |   1 +
>  tests/qemu-iotests/iotests.py |  11 +-
>  5 files changed, 413 insertions(+), 5 deletions(-)
>  create mode 100755 tests/qemu-iotests/274
>  create mode 100644 tests/qemu-iotests/274.out
> 
> -- 
> 2.20.1
> 
> 

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

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

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

[Kevin Wolf]

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

Am 21.11.2019 um 13:21 hat Max Reitz geschrieben:
> On 21.11.19 12:34, Kevin Wolf wrote:
> > Am 21.11.2019 um 09:59 hat Max Reitz geschrieben:
> >> On 20.11.19 19:44, 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.
> >>>
> >>> 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 patch, 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.
> >>>
> >>> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> >>> ---
> >>>  block/io.c | 32 ++++++++++++++++++++++++++++++++
> >>>  1 file changed, 32 insertions(+)
> >>
> >> Zeroing the intersection may take some time.  So is it right for QMP’s
> >> block_resize to do this, seeing it is a synchronous operation?
> > 
> > What else would be right? Returning an error?
> 
> Going through a deprecation cycle.

And keeping the buggy behaviour for two more releases?

> > Common cases (raw and qcow2 v3 without external data files) are quick
> > anyway.
> 
> Well, but quick enough for a fully blocking operation?

For raw definitely yes, because raw doesn't have backing files, so the
code will never run.

For qcow2, block_resize can already block for a relatively long time
while metadata tables are resized, clusters are discarded etc. I just
don't really see the difference in quality between that and allocating
some zero clusters in a corner case like having a short overlay.

Would you feel more comfortable if we set BDRV_REQ_NO_FALLBACK and
return an error if we can't zero out the area? We would have to
advertise that flag in bs->supported_zero_flags for qcow2 then (but
probably we should do that anyway?)

> >> As far as I can tell, jobs actually have the same problem.  I don’t
> >> think mirror or commit have a pause point before truncating, so they
> >> still block the monitor there, don’t they?
> > 
> > Do you really need a pause point? They call bdrv_co_truncate() from
> > inside the job coroutine, so it will yield. I would expect that this
> > is enough.
> 
> OK then.
> 
> > But in fact, all jobs have a pause point before even calling .run(), so
> > even if that made a difference, it should still be fine.
> 
> Good.
> 
> But I believe this is still a problem for block_resize.  I don’t see why
> this needs to be fixed in 4.2-rc3.  What’s the problem with going
> through a proper deprecation cycle other than the fact that we can’t
> start it in 4.2 because we don’t have a resize job yet?

That the behaviour is wrong.

For commit it's an image corruptor. For resize, I'll admit that it's
just wrong behaviour that is probably harmless in most cases, but it's
still wrong behaviour. It would be a corruptor in the same way as commit
if it allowed resizing intermediate nodes, but I _think_ the old-style
op blockers still forbid this. We'd have to double-check this if we
leave things broken for block_resize.

Anyway, so are you suggesting adding another parameter to
bdrv_co_truncate() that enables wrong, but quicker semantics, and that
would only be used by block_resize?

Kevin

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

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

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 6244 bytes --]

On 21.11.19 15:33, Kevin Wolf wrote:
> Am 21.11.2019 um 13:21 hat Max Reitz geschrieben:
>> On 21.11.19 12:34, Kevin Wolf wrote:
>>> Am 21.11.2019 um 09:59 hat Max Reitz geschrieben:
>>>> On 20.11.19 19:44, 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.
>>>>>
>>>>> 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 patch, 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.
>>>>>
>>>>> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>>>>> ---
>>>>>  block/io.c | 32 ++++++++++++++++++++++++++++++++
>>>>>  1 file changed, 32 insertions(+)
>>>>
>>>> Zeroing the intersection may take some time.  So is it right for QMP’s
>>>> block_resize to do this, seeing it is a synchronous operation?
>>>
>>> What else would be right? Returning an error?
>>
>> Going through a deprecation cycle.
> 
> And keeping the buggy behaviour for two more releases?

This sounds like you don’t care about adding another bug when it means
fixing this bug.  I do care.  And so all I was saying is that it seemed
problematic to me to fix the problem in this way, because clearly this
would make block_resize block the monitor in some cases and clearly that
would be a problem, if not a bug.

(And that’s precisely what can be said about the current block_resize
behavior of revealing previous backing file data: It is a problem, but I
wouldn’t call it a full-on bug.  It just seems to me that nobody has
ever really thought about it.)

Also, I don’t see this as a really pressing issue for block_resize at
least, because this isn’t a recent regression or anything.  It was just
the behavior we had, I believe everyone knew it, we just never thought
about whether it really is the best kind of behavior.  So, yes, I’m in
absolutely no hurry to fix this for block_resize.  (Commit is a
different story, but then again commit is a job already, so it doesn’t
suffer from the blocking issue.)


But of course this wasn’t all that you’re saying, you give a very good
point next.

>>> Common cases (raw and qcow2 v3 without external data files) are quick
>>> anyway.
>>
>> Well, but quick enough for a fully blocking operation?> For qcow2, block_resize can already block for a relatively long time
> while metadata tables are resized, clusters are discarded etc. I just
> don't really see the difference in quality between that and allocating
> some zero clusters in a corner case like having a short overlay.

Discarding cropped clusters when shrinking is a good point.  I didn’t
think of that.  So block_resize already has the very problem I was
afraid you’d introduce, because of course discarding isn’t very
different from quick-zeroing.

> Would you feel more comfortable if we set BDRV_REQ_NO_FALLBACK and
> return an error if we can't zero out the area? We would have to
> advertise that flag in bs->supported_zero_flags for qcow2 then (but
> probably we should do that anyway?)

Hm.  I don’t feel that bad about disallowing this edge case (growing a
shrunken overlay) for potentially all non-qcow2v3 formats.  I don’t know
how bad it would be for users other than block_resize, though.

(And I suppose we want a resize job anyway then, and that would work for
those cases?)

>>>> As far as I can tell, jobs actually have the same problem.  I don’t
>>>> think mirror or commit have a pause point before truncating, so they
>>>> still block the monitor there, don’t they?
>>>
>>> Do you really need a pause point? They call bdrv_co_truncate() from
>>> inside the job coroutine, so it will yield. I would expect that this
>>> is enough.
>>
>> OK then.
>>
>>> But in fact, all jobs have a pause point before even calling .run(), so
>>> even if that made a difference, it should still be fine.
>>
>> Good.
>>
>> But I believe this is still a problem for block_resize.  I don’t see why
>> this needs to be fixed in 4.2-rc3.  What’s the problem with going
>> through a proper deprecation cycle other than the fact that we can’t
>> start it in 4.2 because we don’t have a resize job yet?
> 
> That the behaviour is wrong.

I know a couple of wrong behaviors that won’t be fixed in 4.2.

> For commit it's an image corruptor.

That’s a reason why we need it in 4.2.  It’s no reason why we need it
for block_resize.

> For resize, I'll admit that it's
> just wrong behaviour that is probably harmless in most cases, but it's
> still wrong behaviour. It would be a corruptor in the same way as commit
> if it allowed resizing intermediate nodes, but I _think_ the old-style
> op blockers still forbid this. We'd have to double-check this if we
> leave things broken for block_resize.
>> Anyway, so are you suggesting adding another parameter to
> bdrv_co_truncate() that enables wrong, but quicker semantics, and that
> would only be used by block_resize?

That would certainly be a possibility, yes.

I like your suggestion of only allowing it with NO_FALLBACK, but I
suppose we’d want the same parameter for that, too, because users other
than block_resize probably prefer a slow zeroing over an error.

So to me the question then is whether the added complexity is worth it
for an rc3.

Max


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

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

[Alberto Garcia]

On Thu 21 Nov 2019 03:33:31 PM CET, Kevin Wolf wrote:
> For commit it's an image corruptor. For resize, I'll admit that it's
> just wrong behaviour that is probably harmless in most cases, but it's
> still wrong behaviour. It would be a corruptor in the same way as
> commit if it allowed resizing intermediate nodes, but I _think_ the
> old-style op blockers still forbid this. We'd have to double-check
> this if we leave things broken for block_resize.

I tried but cannot make block_resize touch an intermediate image:

    if (!bdrv_is_first_non_filter(bs)) {
        error_setg(errp, QERR_FEATURE_DISABLED, "resize");
        goto out;
    }

Berto

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

[Alberto Garcia]

On Wed 20 Nov 2019 07:44:57 PM CET, 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.
>
> 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 patch, 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.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>

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

Berto