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

Some tests requires O_DIRECT, or want it by default. Introduce smarter

O_DIRECT handling:

- Check O_DIRECT in common.rc, if it is requested by selected

cache-mode.

- Support second fall-through argument in _default_cache_mode

Inspired-by: Max's 23e1d054112cec1e

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

Message-Id: <20200430124713.3067-2-vsementsov@virtuozzo.com>

Signed-off-by: Kevin Wolf <kwolf@redhat.com>

---

tests/qemu-iotests/091 | 2 +-

tests/qemu-iotests/common.rc | 37 ++++++++++++++++++++++++++++++++++--

2 files changed, 36 insertions(+), 3 deletions(-)

diff --git a/tests/qemu-iotests/091 b/tests/qemu-iotests/091

index XXXXXXX..XXXXXXX 100755

--- a/tests/qemu-iotests/091

+++ b/tests/qemu-iotests/091

@@ -XXX,XX +XXX,XX @@ trap "_cleanup; exit \$status" 0 1 2 3 15

_supported_fmt qcow2

_supported_proto file

_supported_os Linux

-_default_cache_mode none

_supported_cache_modes writethrough none writeback

+_default_cache_mode none writeback

size=1G

diff --git a/tests/qemu-iotests/common.rc b/tests/qemu-iotests/common.rc

index XXXXXXX..XXXXXXX 100644

--- a/tests/qemu-iotests/common.rc

+++ b/tests/qemu-iotests/common.rc

@@ -XXX,XX +XXX,XX @@ _supported_cache_modes()

_notrun "not suitable for cache mode: $CACHEMODE"

}

+# Check whether the filesystem supports O_DIRECT

+_check_o_direct()

+{

+ $QEMU_IMG create -f raw "$TEST_IMG".test_o_direct 1M > /dev/null

+ out=$($QEMU_IO -f raw -t none -c quit "$TEST_IMG".test_o_direct 2>&1)

+ rm -f "$TEST_IMG".test_o_direct

+

+ [[ "$out" != *"O_DIRECT"* ]]

+}

+

+_require_o_direct()

+{

+ if ! _check_o_direct; then

+ _notrun "file system on $TEST_DIR does not support O_DIRECT"

+ fi

+}

+

+_check_cache_mode()

+{

+ if [ $CACHEMODE == "none" ] || [ $CACHEMODE == "directsync" ]; then

+ _require_o_direct

+ fi

+}

+

+_check_cache_mode

+

+# $1 - cache mode to use by default

+# $2 - (optional) cache mode to use by default if O_DIRECT is not supported

_default_cache_mode()

{

if $CACHEMODE_IS_DEFAULT; then

- CACHEMODE="$1"

- QEMU_IO="$QEMU_IO --cache $1"

+ if [ -z "$2" ] || _check_o_direct; then

+ CACHEMODE="$1"

+ else

+ CACHEMODE="$2"

+ fi

+ QEMU_IO="$QEMU_IO --cache $CACHEMODE"

+ _check_cache_mode

return

fi

}

--

2.25.3

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

Test fails if bochs not whitelisted, so, skip it in this case.

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

Message-Id: <20200430124713.3067-3-vsementsov@virtuozzo.com>

Signed-off-by: Kevin Wolf <kwolf@redhat.com>

---

tests/qemu-iotests/082 | 1 +

1 file changed, 1 insertion(+)

diff --git a/tests/qemu-iotests/082 b/tests/qemu-iotests/082

index XXXXXXX..XXXXXXX 100755

--- a/tests/qemu-iotests/082

+++ b/tests/qemu-iotests/082

@@ -XXX,XX +XXX,XX @@ trap "_cleanup; exit \$status" 0 1 2 3 15

_supported_fmt qcow2

_supported_proto file nfs

+_require_drivers bochs

run_qemu_img()

{

--

2.25.3

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

Skip test-case with quorum if quorum is not whitelisted.

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

Message-Id: <20200430124713.3067-4-vsementsov@virtuozzo.com>

Signed-off-by: Kevin Wolf <kwolf@redhat.com>

---

tests/qemu-iotests/148 | 1 +

1 file changed, 1 insertion(+)

diff --git a/tests/qemu-iotests/148 b/tests/qemu-iotests/148

index XXXXXXX..XXXXXXX 100755

--- a/tests/qemu-iotests/148

+++ b/tests/qemu-iotests/148

@@ -XXX,XX +XXX,XX @@ sector = "%d"

''' % bad_sector)

file.close()

+ @iotests.skip_if_unsupported(['quorum'])

def setUp(self):

driveopts = ['driver=quorum', 'vote-threshold=2']

driveopts.append('read-pattern=%s' % self.read_pattern)

--

2.25.3

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

Drop check for no block-jobs: it's obvious that there no jobs

immediately after vm.launch().

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

Message-Id: <20200430124713.3067-5-vsementsov@virtuozzo.com>

Signed-off-by: Kevin Wolf <kwolf@redhat.com>

---

tests/qemu-iotests/041 | 8 --------

1 file changed, 8 deletions(-)

diff --git a/tests/qemu-iotests/041 b/tests/qemu-iotests/041

index XXXXXXX..XXXXXXX 100755

--- a/tests/qemu-iotests/041

+++ b/tests/qemu-iotests/041

@@ -XXX,XX +XXX,XX @@ class TestRepairQuorum(iotests.QMPTestCase):

pass

def test_complete(self):

- self.assert_no_active_block_jobs()

-

result = self.vm.qmp('drive-mirror', job_id='job0', device='quorum0',

sync='full', node_name="repair0", replaces="img1",

target=quorum_repair_img, format=iotests.imgfmt)

@@ -XXX,XX +XXX,XX @@ class TestRepairQuorum(iotests.QMPTestCase):

'target image does not match source after mirroring')

def test_cancel(self):

- self.assert_no_active_block_jobs()

-

result = self.vm.qmp('drive-mirror', job_id='job0', device='quorum0',

sync='full', node_name="repair0", replaces="img1",

target=quorum_repair_img, format=iotests.imgfmt)

@@ -XXX,XX +XXX,XX @@ class TestRepairQuorum(iotests.QMPTestCase):

self.assert_has_block_node(None, quorum_img3)

def test_cancel_after_ready(self):

- self.assert_no_active_block_jobs()

-

result = self.vm.qmp('drive-mirror', job_id='job0', device='quorum0',

sync='full', node_name="repair0", replaces="img1",

target=quorum_repair_img, format=iotests.imgfmt)

@@ -XXX,XX +XXX,XX @@ class TestRepairQuorum(iotests.QMPTestCase):

'target image does not match source after mirroring')

def test_pause(self):

- self.assert_no_active_block_jobs()

-

result = self.vm.qmp('drive-mirror', job_id='job0', device='quorum0',

sync='full', node_name="repair0", replaces="img1",

target=quorum_repair_img, format=iotests.imgfmt)

--

2.25.3

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

Instead of looping in each test, let's better refactor vmdk target case

as a subclass.

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

Message-Id: <20200430124713.3067-6-vsementsov@virtuozzo.com>

Signed-off-by: Kevin Wolf <kwolf@redhat.com>

---

tests/qemu-iotests/055 | 70 ++++++++++++++++++++------------------

tests/qemu-iotests/055.out | 4 +--

2 files changed, 39 insertions(+), 35 deletions(-)

diff --git a/tests/qemu-iotests/055 b/tests/qemu-iotests/055

index XXXXXXX..XXXXXXX 100755

--- a/tests/qemu-iotests/055

+++ b/tests/qemu-iotests/055

@@ -XXX,XX +XXX,XX @@ class TestSingleTransaction(iotests.QMPTestCase):

self.assert_no_active_block_jobs()

-class TestDriveCompression(iotests.QMPTestCase):

+class TestCompressedToQcow2(iotests.QMPTestCase):

image_len = 64 * 1024 * 1024 # MB

- fmt_supports_compression = [{'type': 'qcow2', 'args': ()},

- {'type': 'vmdk', 'args': ('-o', 'subformat=streamOptimized')}]

+ target_fmt = {'type': 'qcow2', 'args': ()}

def tearDown(self):

self.vm.shutdown()

@@ -XXX,XX +XXX,XX @@ class TestDriveCompression(iotests.QMPTestCase):

except OSError:

pass

- def do_prepare_drives(self, fmt, args, attach_target):

+ def do_prepare_drives(self, attach_target):

self.vm = iotests.VM().add_drive('blkdebug::' + test_img)

- qemu_img('create', '-f', fmt, blockdev_target_img,

- str(TestDriveCompression.image_len), *args)

+ qemu_img('create', '-f', self.target_fmt['type'], blockdev_target_img,

+ str(self.image_len), *self.target_fmt['args'])

if attach_target:

self.vm.add_drive(blockdev_target_img,

- img_format=fmt, interface="none")

+ img_format=self.target_fmt['type'],

+ interface="none")

self.vm.launch()

- def do_test_compress_complete(self, cmd, format, attach_target, **args):

- self.do_prepare_drives(format['type'], format['args'], attach_target)

+ def do_test_compress_complete(self, cmd, attach_target, **args):

+ self.do_prepare_drives(attach_target)

self.assert_no_active_block_jobs()

@@ -XXX,XX +XXX,XX @@ class TestDriveCompression(iotests.QMPTestCase):

self.vm.shutdown()

self.assertTrue(iotests.compare_images(test_img, blockdev_target_img,

- iotests.imgfmt, format['type']),

+ iotests.imgfmt,

+ self.target_fmt['type']),

'target image does not match source after backup')

def test_complete_compress_drive_backup(self):

- for format in TestDriveCompression.fmt_supports_compression:

- self.do_test_compress_complete('drive-backup', format, False,

- target=blockdev_target_img, mode='existing')

+ self.do_test_compress_complete('drive-backup', False,

+ target=blockdev_target_img,

+ mode='existing')

def test_complete_compress_blockdev_backup(self):

- for format in TestDriveCompression.fmt_supports_compression:

- self.do_test_compress_complete('blockdev-backup', format, True,

- target='drive1')

+ self.do_test_compress_complete('blockdev-backup',

+ True, target='drive1')

- def do_test_compress_cancel(self, cmd, format, attach_target, **args):

- self.do_prepare_drives(format['type'], format['args'], attach_target)

+ def do_test_compress_cancel(self, cmd, attach_target, **args):

+ self.do_prepare_drives(attach_target)

self.assert_no_active_block_jobs()

@@ -XXX,XX +XXX,XX @@ class TestDriveCompression(iotests.QMPTestCase):

self.vm.shutdown()

def test_compress_cancel_drive_backup(self):

- for format in TestDriveCompression.fmt_supports_compression:

- self.do_test_compress_cancel('drive-backup', format, False,

- target=blockdev_target_img, mode='existing')

+ self.do_test_compress_cancel('drive-backup', False,

+ target=blockdev_target_img,

+ mode='existing')

def test_compress_cancel_blockdev_backup(self):

- for format in TestDriveCompression.fmt_supports_compression:

- self.do_test_compress_cancel('blockdev-backup', format, True,

- target='drive1')

+ self.do_test_compress_cancel('blockdev-backup', True,

+ target='drive1')

- def do_test_compress_pause(self, cmd, format, attach_target, **args):

- self.do_prepare_drives(format['type'], format['args'], attach_target)

+ def do_test_compress_pause(self, cmd, attach_target, **args):

+ self.do_prepare_drives(attach_target)

self.assert_no_active_block_jobs()

@@ -XXX,XX +XXX,XX @@ class TestDriveCompression(iotests.QMPTestCase):

self.vm.shutdown()

self.assertTrue(iotests.compare_images(test_img, blockdev_target_img,

- iotests.imgfmt, format['type']),

+ iotests.imgfmt,

+ self.target_fmt['type']),

'target image does not match source after backup')

def test_compress_pause_drive_backup(self):

- for format in TestDriveCompression.fmt_supports_compression:

- self.do_test_compress_pause('drive-backup', format, False,

- target=blockdev_target_img, mode='existing')

+ self.do_test_compress_pause('drive-backup', False,

+ target=blockdev_target_img,

+ mode='existing')

def test_compress_pause_blockdev_backup(self):

- for format in TestDriveCompression.fmt_supports_compression:

- self.do_test_compress_pause('blockdev-backup', format, True,

- target='drive1')

+ self.do_test_compress_pause('blockdev-backup', True,

+ target='drive1')

+

+

+class TestCompressedToVmdk(TestCompressedToQcow2):

+ target_fmt = {'type': 'vmdk', 'args': ('-o', 'subformat=streamOptimized')}

+

if __name__ == '__main__':

iotests.main(supported_fmts=['raw', 'qcow2'],

diff --git a/tests/qemu-iotests/055.out b/tests/qemu-iotests/055.out

index XXXXXXX..XXXXXXX 100644

--- a/tests/qemu-iotests/055.out

+++ b/tests/qemu-iotests/055.out

@@ -XXX,XX +XXX,XX @@

-..............................

+....................................

----------------------------------------------------------------------

-Ran 30 tests

+Ran 36 tests

OK

--

2.25.3

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

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

Message-Id: <20200430124713.3067-7-vsementsov@virtuozzo.com>

Signed-off-by: Kevin Wolf <kwolf@redhat.com>

---

tests/qemu-iotests/055 | 4 ++++

1 file changed, 4 insertions(+)

diff --git a/tests/qemu-iotests/055 b/tests/qemu-iotests/055

index XXXXXXX..XXXXXXX 100755

--- a/tests/qemu-iotests/055

+++ b/tests/qemu-iotests/055

@@ -XXX,XX +XXX,XX @@ class TestCompressedToQcow2(iotests.QMPTestCase):

class TestCompressedToVmdk(TestCompressedToQcow2):

target_fmt = {'type': 'vmdk', 'args': ('-o', 'subformat=streamOptimized')}

+ @iotests.skip_if_unsupported(['vmdk'])

+ def setUp(self):

+ pass

+

if __name__ == '__main__':

iotests.main(supported_fmts=['raw', 'qcow2'],

--

2.25.3

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

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

Message-Id: <20200430124713.3067-8-vsementsov@virtuozzo.com>

Signed-off-by: Kevin Wolf <kwolf@redhat.com>

---

tests/qemu-iotests/109 | 1 +

1 file changed, 1 insertion(+)

diff --git a/tests/qemu-iotests/109 b/tests/qemu-iotests/109

index XXXXXXX..XXXXXXX 100755

--- a/tests/qemu-iotests/109

+++ b/tests/qemu-iotests/109

@@ -XXX,XX +XXX,XX @@ trap "_cleanup; exit \$status" 0 1 2 3 15

_supported_fmt raw

_supported_proto file

_supported_os Linux

+_require_drivers qcow qcow2 qed vdi vmdk vpc

qemu_comm_method=qmp

--

2.25.3

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

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

Message-Id: <20200430124713.3067-9-vsementsov@virtuozzo.com>

Signed-off-by: Kevin Wolf <kwolf@redhat.com>

---

tests/qemu-iotests/113 | 4 ++--

1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tests/qemu-iotests/113 b/tests/qemu-iotests/113

index XXXXXXX..XXXXXXX 100755

--- a/tests/qemu-iotests/113

+++ b/tests/qemu-iotests/113

@@ -XXX,XX +XXX,XX @@ trap "_cleanup; exit \$status" 0 1 2 3 15

. ./common.rc

. ./common.filter

-# Some of these test cases use bochs, but others do use raw, so this

-# is only half a lie.

+# Some of these test cases use bochs, but others do use raw

+_require_drivers bochs

_supported_fmt raw

_supported_proto file

_supported_os Linux

--

2.25.3

From: Alberto Garcia <berto@igalia.com>

After commit f01643fb8b47e8a70c04bbf45e0f12a9e5bc54de when an image is

extended and BDRV_REQ_ZERO_WRITE is set then the new clusters are

zeroized.

The code however does not detect correctly situations when the old and

the new end of the image are within the same cluster. The problem can

be reproduced with these steps:

qemu-img create -f qcow2 backing.qcow2 1M

qemu-img create -f qcow2 -F qcow2 -b backing.qcow2 top.qcow2

qemu-img resize --shrink top.qcow2 520k

qemu-img resize top.qcow2 567k

In the last step offset - zero_start causes an integer wraparound.

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

Message-Id: <20200504155217.10325-1-berto@igalia.com>

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

Signed-off-by: Kevin Wolf <kwolf@redhat.com>

---

block/qcow2.c | 12 ++++---

tests/qemu-iotests/292 | 73 ++++++++++++++++++++++++++++++++++++++

tests/qemu-iotests/292.out | 24 +++++++++++++

tests/qemu-iotests/group | 1 +

4 files changed, 105 insertions(+), 5 deletions(-)

create mode 100755 tests/qemu-iotests/292

create mode 100644 tests/qemu-iotests/292.out

diff --git a/block/qcow2.c b/block/qcow2.c

index XXXXXXX..XXXXXXX 100644

--- a/block/qcow2.c

+++ b/block/qcow2.c

@@ -XXX,XX +XXX,XX @@ static int coroutine_fn qcow2_co_truncate(BlockDriverState *bs, int64_t offset,

* requires a cluster-aligned start. The end may be unaligned if it is

* at the end of the image (which it is here).

*/

- ret = qcow2_cluster_zeroize(bs, zero_start, offset - zero_start, 0);

- if (ret < 0) {

- error_setg_errno(errp, -ret, "Failed to zero out new clusters");

- goto fail;

+ if (offset > zero_start) {

+ ret = qcow2_cluster_zeroize(bs, zero_start, offset - zero_start, 0);

+ if (ret < 0) {

+ error_setg_errno(errp, -ret, "Failed to zero out new clusters");

+ goto fail;

+ }

}

/* Write explicit zeros for the unaligned head */

if (zero_start > old_length) {

- uint64_t len = zero_start - old_length;

+ uint64_t len = MIN(zero_start, offset) - old_length;

uint8_t *buf = qemu_blockalign0(bs, len);

QEMUIOVector qiov;

qemu_iovec_init_buf(&qiov, buf, len);

diff --git a/tests/qemu-iotests/292 b/tests/qemu-iotests/292

new file mode 100755

index XXXXXXX..XXXXXXX

--- /dev/null

+++ b/tests/qemu-iotests/292

@@ -XXX,XX +XXX,XX @@

+#!/usr/bin/env bash

+#

+# Test resizing a qcow2 image with a backing file

+#

+# Copyright (C) 2020 Igalia, S.L.

+# Author: Alberto Garcia <berto@igalia.com>

+#

+# 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=berto@igalia.com

+

+seq=`basename $0`

+echo "QA output created by $seq"

+

+status=1 # failure is the default!

+

+_cleanup()

+{

+ _cleanup_test_img

+}

+trap "_cleanup; exit \$status" 0 1 2 3 15

+

+# get standard environment, filters and checks

+. ./common.rc

+. ./common.filter

+

+_supported_fmt qcow2

+_supported_proto file

+_supported_os Linux

+

+echo '### Create the backing image'

+BACKING_IMG="$TEST_IMG.base"

+TEST_IMG="$BACKING_IMG" _make_test_img 1M

+

+echo '### Fill the backing image with data (0x11)'

+$QEMU_IO -c 'write -P 0x11 0 1M' "$BACKING_IMG" | _filter_qemu_io

+

+echo '### Create the top image'

+_make_test_img -F "$IMGFMT" -b "$BACKING_IMG"

+

+echo '### Fill the top image with data (0x22)'

+$QEMU_IO -c 'write -P 0x22 0 1M' "$TEST_IMG" | _filter_qemu_io

+

+# Both offsets are part of the same cluster.

+echo '### Shrink the image to 520k'

+$QEMU_IMG resize --shrink "$TEST_IMG" 520k

+echo '### Grow the image to 567k'

+$QEMU_IMG resize "$TEST_IMG" 567k

+

+echo '### Check that the tail of the image reads as zeroes'

+$QEMU_IO -c 'read -P 0x22 0 520k' "$TEST_IMG" | _filter_qemu_io

+$QEMU_IO -c 'read -P 0x00 520k 47k' "$TEST_IMG" | _filter_qemu_io

+

+echo '### Show output of qemu-img map'

+$QEMU_IMG map "$TEST_IMG" | _filter_testdir

+

+# success, all done

+echo "*** done"

+rm -f $seq.full

+status=0

diff --git a/tests/qemu-iotests/292.out b/tests/qemu-iotests/292.out

new file mode 100644

index XXXXXXX..XXXXXXX

--- /dev/null

+++ b/tests/qemu-iotests/292.out

@@ -XXX,XX +XXX,XX @@

+QA output created by 292

+### Create the backing image

+Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=1048576

+### Fill the backing image with data (0x11)

+wrote 1048576/1048576 bytes at offset 0

+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)

+### Create the top image

+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1048576 backing_file=TEST_DIR/t.IMGFMT.base backing_fmt=IMGFMT

+### Fill the top image with data (0x22)

+wrote 1048576/1048576 bytes at offset 0

+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)

+### Shrink the image to 520k

+Image resized.

+### Grow the image to 567k

+Image resized.

+### Check that the tail of the image reads as zeroes

+read 532480/532480 bytes at offset 0

+520 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)

+read 48128/48128 bytes at offset 532480

+47 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)

+### Show output of qemu-img map

+Offset Length Mapped to File

+0 0x8dc00 0x50000 TEST_DIR/t.qcow2

+*** done

diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group

index XXXXXXX..XXXXXXX 100644

--- a/tests/qemu-iotests/group

+++ b/tests/qemu-iotests/group

@@ -XXX,XX +XXX,XX @@

288 quick

289 rw quick

290 rw auto quick

+292 rw auto quick

--

2.25.3

m_data is used for zero clusters even though valid == 0. It really only

means that a new cluster was allocated in the image file. Rename it to

reflect this.

While at it, change it from int to bool, too.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>

Message-Id: <20200430133007.170335-2-kwolf@redhat.com>

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

Signed-off-by: Kevin Wolf <kwolf@redhat.com>

---

block/vmdk.c | 8 ++++----

1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/block/vmdk.c b/block/vmdk.c

index XXXXXXX..XXXXXXX 100644

--- a/block/vmdk.c

+++ b/block/vmdk.c

@@ -XXX,XX +XXX,XX @@ typedef struct VmdkMetaData {

unsigned int l1_index;

unsigned int l2_index;

unsigned int l2_offset;

- int valid;

+ bool new_allocation;

uint32_t *l2_cache_entry;

} VmdkMetaData;

@@ -XXX,XX +XXX,XX @@ static int get_cluster_offset(BlockDriverState *bs,

unsigned int l2_size_bytes = extent->l2_size * extent->entry_size;

if (m_data) {

- m_data->valid = 0;

+ m_data->new_allocation = false;

}

if (extent->flat) {

*cluster_offset = extent->flat_start_offset;

@@ -XXX,XX +XXX,XX @@ static int get_cluster_offset(BlockDriverState *bs,

return ret;

}

if (m_data) {

- m_data->valid = 1;

+ m_data->new_allocation = true;

m_data->l1_index = l1_index;

m_data->l2_index = l2_index;

m_data->l2_offset = l2_offset;

@@ -XXX,XX +XXX,XX @@ static int vmdk_pwritev(BlockDriverState *bs, uint64_t offset,

if (ret) {

return ret;

}

- if (m_data.valid) {

+ if (m_data.new_allocation) {

/* update L2 tables */

if (vmdk_L2update(extent, &m_data,

cluster_offset >> BDRV_SECTOR_BITS)

--

2.25.3

m_data must contain valid data even for zero clusters when no cluster

was allocated in the image file. Without this, zero writes segfault with

images that have zeroed_grain=on.

For zero writes, we don't want to allocate a cluster in the image file

even in compressed files.

Fixes: 524089bce43fd1cd3daaca979872451efa2cf7c6

Signed-off-by: Kevin Wolf <kwolf@redhat.com>

Message-Id: <20200430133007.170335-3-kwolf@redhat.com>

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

Signed-off-by: Kevin Wolf <kwolf@redhat.com>

---

block/vmdk.c | 12 +++++++-----

1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/block/vmdk.c b/block/vmdk.c

index XXXXXXX..XXXXXXX 100644

--- a/block/vmdk.c

+++ b/block/vmdk.c

@@ -XXX,XX +XXX,XX @@ static int get_cluster_offset(BlockDriverState *bs,

extent->l2_cache_counts[min_index] = 1;

found:

l2_index = ((offset >> 9) / extent->cluster_sectors) % extent->l2_size;

+ if (m_data) {

+ m_data->l1_index = l1_index;

+ m_data->l2_index = l2_index;

+ m_data->l2_offset = l2_offset;

+ m_data->l2_cache_entry = ((uint32_t *)l2_table) + l2_index;

+ }

if (extent->sesparse) {

cluster_sector = le64_to_cpu(((uint64_t *)l2_table)[l2_index]);

@@ -XXX,XX +XXX,XX @@ static int get_cluster_offset(BlockDriverState *bs,

}

if (m_data) {

m_data->new_allocation = true;

- m_data->l1_index = l1_index;

- m_data->l2_index = l2_index;

- m_data->l2_offset = l2_offset;

- m_data->l2_cache_entry = ((uint32_t *)l2_table) + l2_index;

}

}

*cluster_offset = cluster_sector << BDRV_SECTOR_BITS;

@@ -XXX,XX +XXX,XX @@ static int vmdk_pwritev(BlockDriverState *bs, uint64_t offset,

error_report("Could not write to allocated cluster"

" for streamOptimized");

return -EIO;

- } else {

+ } else if (!zeroed) {

/* allocate */

ret = get_cluster_offset(bs, extent, &m_data, offset,

true, &cluster_offset, 0, 0);

--

2.25.3

When overwriting a zero cluster, we must not perform copy-on-write from

the backing file, but from a zeroed buffer.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>

Message-Id: <20200430133007.170335-4-kwolf@redhat.com>

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

Signed-off-by: Kevin Wolf <kwolf@redhat.com>

---

block/vmdk.c | 18 ++++++++++++------

1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/block/vmdk.c b/block/vmdk.c

index XXXXXXX..XXXXXXX 100644

--- a/block/vmdk.c

+++ b/block/vmdk.c

@@ -XXX,XX +XXX,XX @@ static void vmdk_refresh_limits(BlockDriverState *bs, Error **errp)

* get_whole_cluster

*

* Copy backing file's cluster that covers @sector_num, otherwise write zero,

- * to the cluster at @cluster_sector_num.

+ * to the cluster at @cluster_sector_num. If @zeroed is true, we're overwriting

+ * a zeroed cluster in the current layer and must not copy data from the

+ * backing file.

*

* If @skip_start_sector < @skip_end_sector, the relative range

* [@skip_start_sector, @skip_end_sector) is not copied or written, and leave

@@ -XXX,XX +XXX,XX @@ static int get_whole_cluster(BlockDriverState *bs,

uint64_t cluster_offset,

uint64_t offset,

uint64_t skip_start_bytes,

- uint64_t skip_end_bytes)

+ uint64_t skip_end_bytes,

+ bool zeroed)

{

int ret = VMDK_OK;

int64_t cluster_bytes;

uint8_t *whole_grain;

+ bool copy_from_backing;

/* For COW, align request sector_num to cluster start */

cluster_bytes = extent->cluster_sectors << BDRV_SECTOR_BITS;

offset = QEMU_ALIGN_DOWN(offset, cluster_bytes);

whole_grain = qemu_blockalign(bs, cluster_bytes);

+ copy_from_backing = bs->backing && !zeroed;

- if (!bs->backing) {

+ if (!copy_from_backing) {

memset(whole_grain, 0, skip_start_bytes);

memset(whole_grain + skip_end_bytes, 0, cluster_bytes - skip_end_bytes);

}

@@ -XXX,XX +XXX,XX @@ static int get_whole_cluster(BlockDriverState *bs,

/* Read backing data before skip range */

if (skip_start_bytes > 0) {

- if (bs->backing) {

+ if (copy_from_backing) {

/* qcow2 emits this on bs->file instead of bs->backing */

BLKDBG_EVENT(extent->file, BLKDBG_COW_READ);

ret = bdrv_pread(bs->backing, offset, whole_grain,

@@ -XXX,XX +XXX,XX @@ static int get_whole_cluster(BlockDriverState *bs,

}

/* Read backing data after skip range */

if (skip_end_bytes < cluster_bytes) {

- if (bs->backing) {

+ if (copy_from_backing) {

/* qcow2 emits this on bs->file instead of bs->backing */

BLKDBG_EVENT(extent->file, BLKDBG_COW_READ);

ret = bdrv_pread(bs->backing, offset + skip_end_bytes,

@@ -XXX,XX +XXX,XX @@ static int get_cluster_offset(BlockDriverState *bs,

* or inappropriate VM shutdown.

*/

ret = get_whole_cluster(bs, extent, cluster_sector * BDRV_SECTOR_SIZE,

- offset, skip_start_bytes, skip_end_bytes);

+ offset, skip_start_bytes, skip_end_bytes,

+ zeroed);

if (ret) {

return ret;

}

--

2.25.3

If a cluster is already zeroed, we don't have to call vmdk_L2update(),

which is rather slow because it flushes the image file.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>

Message-Id: <20200430133007.170335-5-kwolf@redhat.com>

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

Signed-off-by: Kevin Wolf <kwolf@redhat.com>

---

block/vmdk.c | 2 +-

1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/vmdk.c b/block/vmdk.c

index XXXXXXX..XXXXXXX 100644

--- a/block/vmdk.c

+++ b/block/vmdk.c

@@ -XXX,XX +XXX,XX @@ static int vmdk_pwritev(BlockDriverState *bs, uint64_t offset,

offset_in_cluster == 0 &&

n_bytes >= extent->cluster_sectors * BDRV_SECTOR_SIZE) {

n_bytes = extent->cluster_sectors * BDRV_SECTOR_SIZE;

- if (!zero_dry_run) {

+ if (!zero_dry_run && ret != VMDK_ZEROED) {

/* update L2 tables */

if (vmdk_L2update(extent, &m_data, VMDK_GTE_ZEROED)

!= VMDK_OK) {

--

2.25.3

If we have a backup L2 table, we currently flush once after writing to

the active L2 table and again after writing to the backup table. A

single flush is enough and makes things a little less slow.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>

Message-Id: <20200430133007.170335-6-kwolf@redhat.com>

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

Signed-off-by: Kevin Wolf <kwolf@redhat.com>

---

block/vmdk.c | 7 +++++--

1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/block/vmdk.c b/block/vmdk.c

index XXXXXXX..XXXXXXX 100644

--- a/block/vmdk.c

+++ b/block/vmdk.c

@@ -XXX,XX +XXX,XX @@ static int vmdk_L2update(VmdkExtent *extent, VmdkMetaData *m_data,

offset = cpu_to_le32(offset);

/* update L2 table */

BLKDBG_EVENT(extent->file, BLKDBG_L2_UPDATE);

- if (bdrv_pwrite_sync(extent->file,

+ if (bdrv_pwrite(extent->file,

((int64_t)m_data->l2_offset * 512)

+ (m_data->l2_index * sizeof(offset)),

&offset, sizeof(offset)) < 0) {

@@ -XXX,XX +XXX,XX @@ static int vmdk_L2update(VmdkExtent *extent, VmdkMetaData *m_data,

/* update backup L2 table */

if (extent->l1_backup_table_offset != 0) {

m_data->l2_offset = extent->l1_backup_table[m_data->l1_index];

- if (bdrv_pwrite_sync(extent->file,

+ if (bdrv_pwrite(extent->file,

((int64_t)m_data->l2_offset * 512)

+ (m_data->l2_index * sizeof(offset)),

&offset, sizeof(offset)) < 0) {

return VMDK_ERROR;

}

}

+ if (bdrv_flush(extent->file->bs) < 0) {

+ return VMDK_ERROR;

+ }

if (m_data->l2_cache_entry) {

*m_data->l2_cache_entry = offset;

}

--

2.25.3

In order to avoid bitrot in the zero cluster code in VMDK, enable

zeroed_grain=on by default for the tests.

059 now unsets the default options because zeroed_grain=on works only

with some subformats and the test case tests many different subformats,

including those for which it doesn't work.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>

Message-Id: <20200430133007.170335-7-kwolf@redhat.com>

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

Signed-off-by: Kevin Wolf <kwolf@redhat.com>

---

tests/qemu-iotests/059 | 6 +++---

tests/qemu-iotests/check | 3 +++

2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/tests/qemu-iotests/059 b/tests/qemu-iotests/059

index XXXXXXX..XXXXXXX 100755

--- a/tests/qemu-iotests/059

+++ b/tests/qemu-iotests/059

@@ -XXX,XX +XXX,XX @@ trap "_cleanup; exit \$status" 0 1 2 3 15

_supported_fmt vmdk

_supported_proto file

_supported_os Linux

-_unsupported_imgopts "subformat=monolithicFlat" \

- "subformat=twoGbMaxExtentFlat" \

- "subformat=twoGbMaxExtentSparse"

+

+# We test all kinds of VMDK options here, so ignore user-specified options

+IMGOPTS=""

capacity_offset=16

granularity_offset=20

diff --git a/tests/qemu-iotests/check b/tests/qemu-iotests/check

index XXXXXXX..XXXXXXX 100755

--- a/tests/qemu-iotests/check

+++ b/tests/qemu-iotests/check

@@ -XXX,XX +XXX,XX @@ fi

if [ "$IMGFMT" == "luks" ] && ! (echo "$IMGOPTS" | grep "iter-time=" > /dev/null); then

IMGOPTS=$(_optstr_add "$IMGOPTS" "iter-time=10")

fi

+if [ "$IMGFMT" == "vmdk" ] && ! (echo "$IMGOPTS" | grep "zeroed_grain=" > /dev/null); then

+ IMGOPTS=$(_optstr_add "$IMGOPTS" "zeroed_grain=on")

+fi

if [ -z "$SAMPLE_IMG_DIR" ]; then

SAMPLE_IMG_DIR="$source_iotests/sample_images"

--

2.25.3

The test case forgot to specify the null-co size for the target node.

When adding a check to backup that both sizes match, this would fail

because of the size mismatch and not the behaviour that the test really

wanted to test.

Fixes: a541fcc27c98b96da187c7d4573f3270f3ddd283

Signed-off-by: Kevin Wolf <kwolf@redhat.com>

Message-Id: <20200430142755.315494-2-kwolf@redhat.com>

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

Signed-off-by: Kevin Wolf <kwolf@redhat.com>

---

tests/qemu-iotests/283 | 6 +++++-

tests/qemu-iotests/283.out | 2 +-

2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/tests/qemu-iotests/283 b/tests/qemu-iotests/283

index XXXXXXX..XXXXXXX 100644

--- a/tests/qemu-iotests/283

+++ b/tests/qemu-iotests/283

@@ -XXX,XX +XXX,XX @@ to check that crash is fixed :)

vm = iotests.VM()

vm.launch()

-vm.qmp_log('blockdev-add', **{'node-name': 'target', 'driver': 'null-co'})

+vm.qmp_log('blockdev-add', **{

+ 'node-name': 'target',

+ 'driver': 'null-co',

+ 'size': size,

+})

vm.qmp_log('blockdev-add', **{

'node-name': 'source',

diff --git a/tests/qemu-iotests/283.out b/tests/qemu-iotests/283.out

index XXXXXXX..XXXXXXX 100644

--- a/tests/qemu-iotests/283.out

+++ b/tests/qemu-iotests/283.out

@@ -XXX,XX +XXX,XX @@

-{"execute": "blockdev-add", "arguments": {"driver": "null-co", "node-name": "target"}}

+{"execute": "blockdev-add", "arguments": {"driver": "null-co", "node-name": "target", "size": 1048576}}

{"return": {}}

{"execute": "blockdev-add", "arguments": {"driver": "blkdebug", "image": {"driver": "null-co", "node-name": "base", "size": 1048576}, "node-name": "source"}}

{"return": {}}

--

2.25.3

bdrv_get_device_name() will be an empty string with modern management

tools that don't use -drive. Use bdrv_get_device_or_node_name() instead

so that the node name is used if the BlockBackend is anonymous.

While at it, start with upper case to make the message consistent with

the rest of the function.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>

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

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

Message-Id: <20200430142755.315494-3-kwolf@redhat.com>

Signed-off-by: Kevin Wolf <kwolf@redhat.com>

---

block/backup.c | 4 ++--

1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/block/backup.c b/block/backup.c

index XXXXXXX..XXXXXXX 100644

--- a/block/backup.c

+++ b/block/backup.c

@@ -XXX,XX +XXX,XX @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs,

len = bdrv_getlength(bs);

if (len < 0) {

- error_setg_errno(errp, -len, "unable to get length for '%s'",

- bdrv_get_device_name(bs));

+ error_setg_errno(errp, -len, "Unable to get length for '%s'",

+ bdrv_get_device_or_node_name(bs));

goto error;

}

--

2.25.3

Since the introduction of a backup filter node in commit 00e30f05d, the

backup block job crashes when the target image is smaller than the

source image because it will try to write after the end of the target

node without having BLK_PERM_RESIZE. (Previously, the BlockBackend layer

would have caught this and errored out gracefully.)

We can fix this and even do better than the old behaviour: Check that

source and target have the same image size at the start of the block job

and unshare BLK_PERM_RESIZE. (This permission was already unshared

before the same commit 00e30f05d, but the BlockBackend that was used to

make the restriction was removed without a replacement.) This will

immediately error out when starting the job instead of only when writing

to a block that doesn't exist in the target.

Longer target than source would technically work because we would never

write to blocks that don't exist, but semantically these are invalid,

too, because a backup is supposed to create a copy, not just an image

that starts with a copy.

Fixes: 00e30f05de1d19586345ec373970ef4c192c6270

Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1778593

Cc: qemu-stable@nongnu.org

Signed-off-by: Kevin Wolf <kwolf@redhat.com>

Message-Id: <20200430142755.315494-4-kwolf@redhat.com>

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

Signed-off-by: Kevin Wolf <kwolf@redhat.com>

---

block/backup-top.c | 14 +++++++++-----

block/backup.c | 14 +++++++++++++-

2 files changed, 22 insertions(+), 6 deletions(-)

diff --git a/block/backup-top.c b/block/backup-top.c

index XXXXXXX..XXXXXXX 100644

--- a/block/backup-top.c

+++ b/block/backup-top.c

@@ -XXX,XX +XXX,XX @@ static void backup_top_child_perm(BlockDriverState *bs, BdrvChild *c,

*

* Share write to target (child_file), to not interfere

* with guest writes to its disk which may be in target backing chain.

+ * Can't resize during a backup block job because we check the size

+ * only upfront.

*/

- *nshared = BLK_PERM_ALL;

+ *nshared = BLK_PERM_ALL & ~BLK_PERM_RESIZE;

*nperm = BLK_PERM_WRITE;

} else {

/* Source child */

@@ -XXX,XX +XXX,XX @@ static void backup_top_child_perm(BlockDriverState *bs, BdrvChild *c,

if (perm & BLK_PERM_WRITE) {

*nperm = *nperm | BLK_PERM_CONSISTENT_READ;

}

- *nshared &= ~BLK_PERM_WRITE;

+ *nshared &= ~(BLK_PERM_WRITE | BLK_PERM_RESIZE);

}

}

@@ -XXX,XX +XXX,XX @@ BlockDriverState *bdrv_backup_top_append(BlockDriverState *source,

{

Error *local_err = NULL;

BDRVBackupTopState *state;

- BlockDriverState *top = bdrv_new_open_driver(&bdrv_backup_top_filter,

- filter_node_name,

- BDRV_O_RDWR, errp);

+ BlockDriverState *top;

bool appended = false;

+ assert(source->total_sectors == target->total_sectors);

+

+ top = bdrv_new_open_driver(&bdrv_backup_top_filter, filter_node_name,

+ BDRV_O_RDWR, errp);

if (!top) {

return NULL;

}

diff --git a/block/backup.c b/block/backup.c

index XXXXXXX..XXXXXXX 100644

--- a/block/backup.c

+++ b/block/backup.c

@@ -XXX,XX +XXX,XX @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs,

BlockCompletionFunc *cb, void *opaque,

JobTxn *txn, Error **errp)

{

- int64_t len;

+ int64_t len, target_len;

BackupBlockJob *job = NULL;

int64_t cluster_size;

BdrvRequestFlags write_flags;

@@ -XXX,XX +XXX,XX @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs,

goto error;

}

+ target_len = bdrv_getlength(target);

+ if (target_len < 0) {

+ error_setg_errno(errp, -target_len, "Unable to get length for '%s'",