* [Qemu-devel] [PATCH v3 1/3] qemu-img: Allow rebase with no input base
2019-05-09 17:52 [Qemu-devel] [PATCH v3 0/3] qemu-img: Allow rebase with no input base Max Reitz
@ 2019-05-09 17:52 ` Max Reitz
2019-05-09 17:52 ` [Qemu-devel] [PATCH v3 2/3] qemu-img: Use zero writes after source backing EOF Max Reitz
` (2 subsequent siblings)
3 siblings, 0 replies; 5+ messages in thread
From: Max Reitz @ 2019-05-09 17:52 UTC (permalink / raw)
To: qemu-block
Cc: Kevin Wolf, Vladimir Sementsov-Ogievskiy, qemu-devel, Max Reitz
Currently, without -u, you cannot add a backing file to an image when it
currently has none:
$ qemu-img rebase -b base.qcow2 foo.qcow2
qemu-img: Could not open old backing file '': The 'file' block driver
requires a file name
It is really simple to allow this, though (effectively by setting
old_backing_size to 0), so this patch does just that.
Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
qemu-img.c | 61 ++++++++++++++++++++++++++++++------------------------
1 file changed, 34 insertions(+), 27 deletions(-)
diff --git a/qemu-img.c b/qemu-img.c
index 71c92f142a..cfa44b4153 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -3312,26 +3312,30 @@ static int img_rebase(int argc, char **argv)
char backing_name[PATH_MAX];
QDict *options = NULL;
- if (bs->backing_format[0] != '\0') {
- options = qdict_new();
- qdict_put_str(options, "driver", bs->backing_format);
- }
-
- if (force_share) {
- if (!options) {
+ if (bs->backing) {
+ if (bs->backing_format[0] != '\0') {
options = qdict_new();
+ qdict_put_str(options, "driver", bs->backing_format);
}
- qdict_put_bool(options, BDRV_OPT_FORCE_SHARE, true);
- }
- bdrv_get_backing_filename(bs, backing_name, sizeof(backing_name));
- blk_old_backing = blk_new_open(backing_name, NULL,
- options, src_flags, &local_err);
- if (!blk_old_backing) {
- error_reportf_err(local_err,
- "Could not open old backing file '%s': ",
- backing_name);
- ret = -1;
- goto out;
+
+ if (force_share) {
+ if (!options) {
+ options = qdict_new();
+ }
+ qdict_put_bool(options, BDRV_OPT_FORCE_SHARE, true);
+ }
+ bdrv_get_backing_filename(bs, backing_name, sizeof(backing_name));
+ blk_old_backing = blk_new_open(backing_name, NULL,
+ options, src_flags, &local_err);
+ if (!blk_old_backing) {
+ error_reportf_err(local_err,
+ "Could not open old backing file '%s': ",
+ backing_name);
+ ret = -1;
+ goto out;
+ }
+ } else {
+ blk_old_backing = NULL;
}
if (out_baseimg[0]) {
@@ -3384,7 +3388,7 @@ static int img_rebase(int argc, char **argv)
*/
if (!unsafe) {
int64_t size;
- int64_t old_backing_size;
+ int64_t old_backing_size = 0;
int64_t new_backing_size = 0;
uint64_t offset;
int64_t n;
@@ -3400,15 +3404,18 @@ static int img_rebase(int argc, char **argv)
ret = -1;
goto out;
}
- old_backing_size = blk_getlength(blk_old_backing);
- if (old_backing_size < 0) {
- char backing_name[PATH_MAX];
+ if (blk_old_backing) {
+ old_backing_size = blk_getlength(blk_old_backing);
+ if (old_backing_size < 0) {
+ char backing_name[PATH_MAX];
- bdrv_get_backing_filename(bs, backing_name, sizeof(backing_name));
- error_report("Could not get size of '%s': %s",
- backing_name, strerror(-old_backing_size));
- ret = -1;
- goto out;
+ bdrv_get_backing_filename(bs, backing_name,
+ sizeof(backing_name));
+ error_report("Could not get size of '%s': %s",
+ backing_name, strerror(-old_backing_size));
+ ret = -1;
+ goto out;
+ }
}
if (blk_new_backing) {
new_backing_size = blk_getlength(blk_new_backing);
--
2.21.0
^ permalink raw reply related [flat|nested] 5+ messages in thread* [Qemu-devel] [PATCH v3 2/3] qemu-img: Use zero writes after source backing EOF
2019-05-09 17:52 [Qemu-devel] [PATCH v3 0/3] qemu-img: Allow rebase with no input base Max Reitz
2019-05-09 17:52 ` [Qemu-devel] [PATCH v3 1/3] " Max Reitz
@ 2019-05-09 17:52 ` Max Reitz
2019-05-09 17:52 ` [Qemu-devel] [PATCH v3 3/3] iotests: Add test for rebase without input base Max Reitz
2019-05-10 9:40 ` [Qemu-devel] [PATCH v3 0/3] qemu-img: Allow rebase with no " Kevin Wolf
3 siblings, 0 replies; 5+ messages in thread
From: Max Reitz @ 2019-05-09 17:52 UTC (permalink / raw)
To: qemu-block
Cc: Kevin Wolf, Vladimir Sementsov-Ogievskiy, qemu-devel, Max Reitz
Past the end of the source backing file, we memset() buf_old to zero, so
it is clearly easy to use blk_pwrite_zeroes() instead of blk_pwrite()
then.
Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
qemu-img.c | 11 +++++++++--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/qemu-img.c b/qemu-img.c
index cfa44b4153..28fba1e7a7 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -3432,6 +3432,8 @@ static int img_rebase(int argc, char **argv)
}
for (offset = 0; offset < size; offset += n) {
+ bool buf_old_is_zero = false;
+
/* How many bytes can we handle with the next read? */
n = MIN(IO_BUF_SIZE, size - offset);
@@ -3452,6 +3454,7 @@ static int img_rebase(int argc, char **argv)
*/
if (offset >= old_backing_size) {
memset(buf_old, 0, n);
+ buf_old_is_zero = true;
} else {
if (offset + n > old_backing_size) {
n = old_backing_size - offset;
@@ -3487,8 +3490,12 @@ static int img_rebase(int argc, char **argv)
if (compare_buffers(buf_old + written, buf_new + written,
n - written, &pnum))
{
- ret = blk_pwrite(blk, offset + written,
- buf_old + written, pnum, 0);
+ if (buf_old_is_zero) {
+ ret = blk_pwrite_zeroes(blk, offset + written, pnum, 0);
+ } else {
+ ret = blk_pwrite(blk, offset + written,
+ buf_old + written, pnum, 0);
+ }
if (ret < 0) {
error_report("Error while writing to COW image: %s",
strerror(-ret));
--
2.21.0
^ permalink raw reply related [flat|nested] 5+ messages in thread* [Qemu-devel] [PATCH v3 3/3] iotests: Add test for rebase without input base
2019-05-09 17:52 [Qemu-devel] [PATCH v3 0/3] qemu-img: Allow rebase with no input base Max Reitz
2019-05-09 17:52 ` [Qemu-devel] [PATCH v3 1/3] " Max Reitz
2019-05-09 17:52 ` [Qemu-devel] [PATCH v3 2/3] qemu-img: Use zero writes after source backing EOF Max Reitz
@ 2019-05-09 17:52 ` Max Reitz
2019-05-10 9:40 ` [Qemu-devel] [PATCH v3 0/3] qemu-img: Allow rebase with no " Kevin Wolf
3 siblings, 0 replies; 5+ messages in thread
From: Max Reitz @ 2019-05-09 17:52 UTC (permalink / raw)
To: qemu-block
Cc: Kevin Wolf, Vladimir Sementsov-Ogievskiy, qemu-devel, Max Reitz
This patch adds a test for rebasing an image that currently does not
have a backing file.
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
tests/qemu-iotests/252 | 124 +++++++++++++++++++++++++++++++++++++
tests/qemu-iotests/252.out | 39 ++++++++++++
tests/qemu-iotests/group | 1 +
3 files changed, 164 insertions(+)
create mode 100755 tests/qemu-iotests/252
create mode 100644 tests/qemu-iotests/252.out
diff --git a/tests/qemu-iotests/252 b/tests/qemu-iotests/252
new file mode 100755
index 0000000000..f6c8f71444
--- /dev/null
+++ b/tests/qemu-iotests/252
@@ -0,0 +1,124 @@
+#!/usr/bin/env bash
+#
+# Tests for rebasing COW images that require zero cluster support
+#
+# 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=mreitz@redhat.com
+
+seq=$(basename $0)
+echo "QA output created by $seq"
+
+status=1 # failure is the default!
+
+_cleanup()
+{
+ _cleanup_test_img
+ rm -f "$TEST_IMG.base_new"
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+. ./common.pattern
+
+# Currently only qcow2 and qed support rebasing, and only qcow2 v3 has
+# zero cluster support
+_supported_fmt qcow2
+_unsupported_imgopts 'compat=0.10'
+_supported_proto file
+_supported_os Linux
+
+CLUSTER_SIZE=65536
+
+echo
+echo "=== Test rebase without input base ==="
+echo
+
+# Cluster allocations to be tested:
+#
+# Backing (new) 11 -- 11 -- 11 --
+# COW image 22 22 11 11 -- --
+#
+# Expected result:
+#
+# COW image 22 22 11 11 00 --
+#
+# (Cluster 2 might be "--" after the rebase, too, but rebase just
+# compares the new backing file to the old one and disregards the
+# overlay. Therefore, it will never discard overlay clusters.)
+
+_make_test_img $((6 * CLUSTER_SIZE))
+TEST_IMG="$TEST_IMG.base_new" _make_test_img $((6 * CLUSTER_SIZE))
+
+echo
+
+$QEMU_IO "$TEST_IMG" \
+ -c "write -P 0x22 $((0 * CLUSTER_SIZE)) $((2 * CLUSTER_SIZE))" \
+ -c "write -P 0x11 $((2 * CLUSTER_SIZE)) $((2 * CLUSTER_SIZE))" \
+ | _filter_qemu_io
+
+$QEMU_IO "$TEST_IMG.base_new" \
+ -c "write -P 0x11 $((0 * CLUSTER_SIZE)) $CLUSTER_SIZE" \
+ -c "write -P 0x11 $((2 * CLUSTER_SIZE)) $CLUSTER_SIZE" \
+ -c "write -P 0x11 $((4 * CLUSTER_SIZE)) $CLUSTER_SIZE" \
+ | _filter_qemu_io
+
+echo
+
+# This should be a no-op
+$QEMU_IMG rebase -b "" "$TEST_IMG"
+
+# Verify the data is correct
+$QEMU_IO "$TEST_IMG" \
+ -c "read -P 0x22 $((0 * CLUSTER_SIZE)) $((2 * CLUSTER_SIZE))" \
+ -c "read -P 0x11 $((2 * CLUSTER_SIZE)) $((2 * CLUSTER_SIZE))" \
+ -c "read -P 0x00 $((4 * CLUSTER_SIZE)) $((2 * CLUSTER_SIZE))" \
+ | _filter_qemu_io
+
+echo
+
+# Verify the allocation status (first four cluster should be allocated
+# in TEST_IMG, clusters 4 and 5 should be unallocated (marked as zero
+# clusters here because there is no backing file))
+$QEMU_IMG map --output=json "$TEST_IMG" | _filter_qemu_img_map
+
+echo
+
+$QEMU_IMG rebase -b "$TEST_IMG.base_new" "$TEST_IMG"
+
+# Verify the data is correct
+$QEMU_IO "$TEST_IMG" \
+ -c "read -P 0x22 $((0 * CLUSTER_SIZE)) $((2 * CLUSTER_SIZE))" \
+ -c "read -P 0x11 $((2 * CLUSTER_SIZE)) $((2 * CLUSTER_SIZE))" \
+ -c "read -P 0x00 $((4 * CLUSTER_SIZE)) $((2 * CLUSTER_SIZE))" \
+ | _filter_qemu_io
+
+echo
+
+# Verify the allocation status (first four cluster should be allocated
+# in TEST_IMG, cluster 4 should be zero, and cluster 5 should be
+# unallocated (signified by '"depth": 1'))
+$QEMU_IMG map --output=json "$TEST_IMG" | _filter_qemu_img_map
+
+
+# success, all done
+echo "*** done"
+rm -f $seq.full
+status=0
diff --git a/tests/qemu-iotests/252.out b/tests/qemu-iotests/252.out
new file mode 100644
index 0000000000..12dce889f8
--- /dev/null
+++ b/tests/qemu-iotests/252.out
@@ -0,0 +1,39 @@
+QA output created by 252
+
+=== Test rebase without input base ===
+
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=393216
+Formatting 'TEST_DIR/t.IMGFMT.base_new', fmt=IMGFMT size=393216
+
+wrote 131072/131072 bytes at offset 0
+128 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+wrote 131072/131072 bytes at offset 131072
+128 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+wrote 65536/65536 bytes at offset 0
+64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+wrote 65536/65536 bytes at offset 131072
+64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+wrote 65536/65536 bytes at offset 262144
+64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+read 131072/131072 bytes at offset 0
+128 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 131072/131072 bytes at offset 131072
+128 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 131072/131072 bytes at offset 262144
+128 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+[{ "start": 0, "length": 262144, "depth": 0, "zero": false, "data": true, "offset": OFFSET},
+{ "start": 262144, "length": 131072, "depth": 0, "zero": true, "data": false}]
+
+read 131072/131072 bytes at offset 0
+128 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 131072/131072 bytes at offset 131072
+128 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 131072/131072 bytes at offset 262144
+128 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+[{ "start": 0, "length": 262144, "depth": 0, "zero": false, "data": true, "offset": OFFSET},
+{ "start": 262144, "length": 65536, "depth": 0, "zero": true, "data": false},
+{ "start": 327680, "length": 65536, "depth": 1, "zero": true, "data": false}]
+*** done
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index 7ac9a5ea4a..00e474ab0a 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -249,3 +249,4 @@
247 rw auto quick
248 rw auto quick
249 rw auto quick
+252 rw auto backing quick
--
2.21.0
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [Qemu-devel] [PATCH v3 0/3] qemu-img: Allow rebase with no input base
2019-05-09 17:52 [Qemu-devel] [PATCH v3 0/3] qemu-img: Allow rebase with no input base Max Reitz
` (2 preceding siblings ...)
2019-05-09 17:52 ` [Qemu-devel] [PATCH v3 3/3] iotests: Add test for rebase without input base Max Reitz
@ 2019-05-10 9:40 ` Kevin Wolf
3 siblings, 0 replies; 5+ messages in thread
From: Kevin Wolf @ 2019-05-10 9:40 UTC (permalink / raw)
To: Max Reitz; +Cc: Vladimir Sementsov-Ogievskiy, qemu-devel, qemu-block
Am 09.05.2019 um 19:52 hat Max Reitz geschrieben:
> This series allows using qemu-img rebase (without -u) on images that do
> not have a backing file. Right now, this fails with the rather cryptic
> error message:
>
> $ qemu-img rebase -b base.qcow2 foo.qcow2
> qemu-img: Could not open old backing file '': The 'file' block driver requires a file name
>
> Yeah, well, OK.
>
> With how rebase currently works, this would lead to the overlay being
> filled with zeroes, however. This is where patch 2 comes in and instead
> makes rebase use blk_pwrite_zeroes() whenever it handles an area past
> the input’s backing file’s EOF.
>
> (Note that additionally we could try to punch holes in the overlay
> whenever it matches the new backing file, but that’s something I’ll put
> off for later. (We don’t even have a reliable method for punching holes
> into an overlay yet, although I would like to have such because it could
> make active commit more efficient.))
>
>
> And patch 3 adds the usual test.
>
>
> v3:
> - Patch 3: Move test to own file so it doesn’t run for qed or qcow2 v2
> (because it can’t, it requires zero clusters)
Thanks, updated in the block branch.
Kevin
^ permalink raw reply [flat|nested] 5+ messages in thread