[PATCH 0/2] qcow2: Force preallocation with data-file-raw

[PATCH 0/2] qcow2: Force preallocation with data-file-raw

[Max Reitz]

Hi,

As discussed here:

https://lists.nongnu.org/archive/html/qemu-block/2020-02/msg00644.html
https://lists.nongnu.org/archive/html/qemu-block/2020-04/msg00329.html
https://lists.nongnu.org/archive/html/qemu-block/2020-06/msg00240.html

I think that qcow2 images with data-file-raw should always have
preallocated 1:1 L1/L2 tables, so that the image always looks the same
whether you respect or ignore the qcow2 metadata.  The easiest way to
achieve that is to enforce at least metadata preallocation whenever
data-file-raw is given.


Max Reitz (2):
  qcow2: Force preallocation with data-file-raw
  iotests/244: Test preallocation for data-file-raw

 block/qcow2.c              | 22 +++++++++++++
 tests/qemu-iotests/244     | 65 ++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/244.out | 32 ++++++++++++++++---
 3 files changed, 114 insertions(+), 5 deletions(-)

-- 
2.26.2

[PATCH 1/2] qcow2: Force preallocation with data-file-raw

[Max Reitz]

Setting the qcow2 data-file-raw bit means that you can ignore the
qcow2 metadata when reading from the external data file.  It does not
mean that you have to ignore it, though.  Therefore, the data read must
be the same regardless of whether you interpret the metadata or whether
you ignore it, and thus the L1/L2 tables must all be present and give a
1:1 mapping.

This patch changes 244's output: First, the qcow2 file is larger right
after creation, because of metadata preallocation.  Second, the qemu-img
map output changes: Everything that was not explicitly discarded or
zeroed is now a data area.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/qcow2.c              | 22 ++++++++++++++++++++++
 tests/qemu-iotests/244.out |  9 ++++-----
 2 files changed, 26 insertions(+), 5 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index 0cd2e6757e..2a588d8091 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -3460,6 +3460,28 @@ qcow2_co_create(BlockdevCreateOptions *create_options, Error **errp)
         ret = -EINVAL;
         goto out;
     }
+    if (qcow2_opts->data_file_raw &&
+        qcow2_opts->preallocation == PREALLOC_MODE_OFF)
+    {
+        /*
+         * data-file-raw means that "the external data file can be
+         * read as a consistent standalone raw image without looking
+         * at the qcow2 metadata."  It does not say that the metadata
+         * must be ignored, though (and the qcow2 driver in fact does
+         * not ignore it), so the L1/L2 tables must be present and
+         * give a 1:1 mapping, so you get the same result regardless
+         * of whether you look at the metadata or whether you ignore
+         * it.
+         */
+        qcow2_opts->preallocation = PREALLOC_MODE_METADATA;
+
+        /*
+         * Cannot use preallocation with backing files, but giving a
+         * backing file when specifying data_file_raw is an error
+         * anyway.
+         */
+        assert(!qcow2_opts->has_backing_file);
+    }
 
     if (qcow2_opts->data_file) {
         if (version < 3) {
diff --git a/tests/qemu-iotests/244.out b/tests/qemu-iotests/244.out
index dbab7359a9..24f02363dd 100644
--- a/tests/qemu-iotests/244.out
+++ b/tests/qemu-iotests/244.out
@@ -83,7 +83,7 @@ qcow2 file size after I/O: 327680
 === Standalone image with external data file (valid raw) ===
 
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864 data_file=TEST_DIR/t.IMGFMT.data data_file_raw=on
-qcow2 file size before I/O: 196616
+qcow2 file size before I/O: 327680
 
 wrote 4194304/4194304 bytes at offset 1048576
 4 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
@@ -93,11 +93,10 @@ wrote 3145728/3145728 bytes at offset 3145728
 3 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 No errors were found on the image.
 
-[{ "start": 0, "length": 1048576, "depth": 0, "zero": true, "data": false},
-{ "start": 1048576, "length": 1048576, "depth": 0, "zero": false, "data": true, "offset": 1048576},
+[{ "start": 0, "length": 2097152, "depth": 0, "zero": false, "data": true, "offset": 0},
 { "start": 2097152, "length": 2097152, "depth": 0, "zero": true, "data": false},
-{ "start": 4194304, "length": 1048576, "depth": 0, "zero": true, "data": false, "offset": 4194304},
-{ "start": 5242880, "length": 61865984, "depth": 0, "zero": true, "data": false}]
+{ "start": 4194304, "length": 2097152, "depth": 0, "zero": true, "data": false, "offset": 4194304},
+{ "start": 6291456, "length": 60817408, "depth": 0, "zero": false, "data": true, "offset": 6291456}]
 
 read 1048576/1048576 bytes at offset 0
 1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-- 
2.26.2

[PATCH 2/2] iotests/244: Test preallocation for data-file-raw

[Max Reitz]

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 tests/qemu-iotests/244     | 65 ++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/244.out | 23 ++++++++++++++
 2 files changed, 88 insertions(+)

diff --git a/tests/qemu-iotests/244 b/tests/qemu-iotests/244
index efe3c0428b..c2fdeab0c7 100755
--- a/tests/qemu-iotests/244
+++ b/tests/qemu-iotests/244
@@ -217,6 +217,71 @@ $QEMU_IMG amend -f $IMGFMT -o "data_file=blkdebug::$TEST_IMG.data" "$TEST_IMG"
 $QEMU_IMG convert -f $IMGFMT -O $IMGFMT -n -C "$TEST_IMG.src" "$TEST_IMG"
 $QEMU_IMG compare -f $IMGFMT -F $IMGFMT "$TEST_IMG.src" "$TEST_IMG"
 
+echo
+echo '=== Preallocation with data-file-raw ==='
+
+echo
+echo '--- Using a non-zeroed data file ---'
+
+# Using data-file-raw must enforce at least metadata preallocation so
+# that it does not matter whether one reads the raw file or the qcow2
+# file
+
+# The real test we would like to do here is to use an existing block
+# device with some random data on it as the external data file.
+# When creating the qcow2 file, it would not be overwritten and its
+# data would stay as it is.  However, using an existing block device
+# is a bit cumbersome in a test, so we are going to cheat by using a
+# normal regular file.
+
+# Unfortunately, this will O_CREAT | O_TRUNC that regular file, so
+# there is no point in creating it beforehand and filling it with
+# random data:
+_make_test_img -o "data_file=$TEST_IMG.data,data_file_raw=on" 1M
+
+# So now comes the cheating: We write directly into the data file.
+# That is actually unsupported, but it works for this test.
+# (As written above, the actual case would be to use a block device
+# as the external data file.  Such a device would not be emptied when
+# the qcow2 file is created, so its data would persist that step.)
+$QEMU_IO -f raw -c 'write -P 42 0 1M' "$TEST_IMG.data" | _filter_qemu_io
+
+echo
+echo 'Comparing pattern:'
+
+# Reading from either the qcow2 file or the data file should return
+# the same result:
+$QEMU_IO -f raw -c 'read -P 42 0 1M' "$TEST_IMG.data" | _filter_qemu_io
+$QEMU_IO -f $IMGFMT -c 'read -P 42 0 1M' "$TEST_IMG" | _filter_qemu_io
+
+# For good measure
+$QEMU_IMG compare -f raw "$TEST_IMG.data" "$TEST_IMG"
+
+echo
+echo '--- Giving a backing file at runtime ---'
+
+# qcow2 files with data-file-raw cannot have backing files given by
+# their image header, but qemu will allow you to set a backing node at
+# runtime -- it should not have any effect, though (because reading
+# from the qcow2 node should return the same data as reading from the
+# raw node).
+
+_make_test_img -o "data_file=$TEST_IMG.data,data_file_raw=on" 1M
+TEST_IMG="$TEST_IMG.base" _make_test_img 1M
+
+# Write something that is not zero into the base image
+$QEMU_IO -c 'write -P 42 0 1M' "$TEST_IMG.base" | _filter_qemu_io
+
+echo
+echo 'Comparing qcow2 image and raw data file:'
+
+# $TEST_IMG and $TEST_IMG.data must show the same data at all times;
+# that is, the qcow2 node must not fall through to the backing image
+# at any point
+$QEMU_IMG compare --image-opts \
+    "driver=raw,file.filename=$TEST_IMG.data"  \
+    "file.filename=$TEST_IMG,backing.file.filename=$TEST_IMG.base"
+
 # success, all done
 echo "*** done"
 rm -f $seq.full
diff --git a/tests/qemu-iotests/244.out b/tests/qemu-iotests/244.out
index 24f02363dd..34d6b0e626 100644
--- a/tests/qemu-iotests/244.out
+++ b/tests/qemu-iotests/244.out
@@ -130,4 +130,27 @@ Offset          Length          Mapped to       File
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864 data_file=TEST_DIR/t.IMGFMT.data
 Images are identical.
 Images are identical.
+
+=== Preallocation with data-file-raw ===
+
+--- Using a non-zeroed data file ---
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1048576 data_file=TEST_DIR/t.IMGFMT.data data_file_raw=on
+wrote 1048576/1048576 bytes at offset 0
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+Comparing pattern:
+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 0
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+Images are identical.
+
+--- Giving a backing file at runtime ---
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1048576 data_file=TEST_DIR/t.IMGFMT.data data_file_raw=on
+Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=1048576
+wrote 1048576/1048576 bytes at offset 0
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+Comparing qcow2 image and raw data file:
+Images are identical.
 *** done
-- 
2.26.2

Re: [PATCH 0/2] qcow2: Force preallocation with data-file-raw

[no-reply]

Patchew URL: https://patchew.org/QEMU/20200619104012.235977-1-mreitz@redhat.com/



Hi,

This series failed the asan build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.

=== TEST SCRIPT BEGIN ===
#!/bin/bash
export ARCH=x86_64
make docker-image-fedora V=1 NETWORK=1
time make docker-test-debug@fedora TARGET_LIST=x86_64-softmmu J=14 NETWORK=1
=== TEST SCRIPT END ===

  CC      qga/commands.o
  CC      qga/guest-agent-command-state.o
  CC      qga/main.o
/usr/bin/ld: /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o): warning: common of `__interception::real_vfork' overridden by definition from /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
  CC      qga/commands-posix.o
  CC      qga/channel-posix.o
  CC      qga/qapi-generated/qga-qapi-types.o
---
  GEN     docs/interop/qemu-ga-ref.html
  GEN     docs/interop/qemu-ga-ref.txt
  GEN     docs/interop/qemu-ga-ref.7
/usr/bin/ld: /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o): warning: common of `__interception::real_vfork' overridden by definition from /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
  AS      pc-bios/optionrom/linuxboot.o
  CC      pc-bios/optionrom/linuxboot_dma.o
  AS      pc-bios/optionrom/kvmvapic.o
---
  BUILD   pc-bios/optionrom/pvh.raw
  LINK    qemu-keymap
  SIGN    pc-bios/optionrom/pvh.bin
/usr/bin/ld: /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o): warning: common of `__interception::real_vfork' overridden by definition from /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
/usr/bin/ld: /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o): warning: common of `__interception::real_vfork' overridden by definition from /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
  LINK    ivshmem-client
  LINK    ivshmem-server
/usr/bin/ld: /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o): warning: common of `__interception::real_vfork' overridden by definition from /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
  LINK    qemu-nbd
/usr/bin/ld: /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o): warning: common of `__interception::real_vfork' overridden by definition from /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
  LINK    qemu-storage-daemon
/usr/bin/ld: /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o): warning: common of `__interception::real_vfork' overridden by definition from /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
  LINK    qemu-img
/usr/bin/ld: /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o): warning: common of `__interception::real_vfork' overridden by definition from /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
  LINK    qemu-io
  LINK    qemu-edid
/usr/bin/ld: /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o): warning: common of `__interception::real_vfork' overridden by definition from /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
/usr/bin/ld: /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o): warning: common of `__interception::real_vfork' overridden by definition from /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
  LINK    fsdev/virtfs-proxy-helper
  LINK    scsi/qemu-pr-helper
/usr/bin/ld: /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o): warning: common of `__interception::real_vfork' overridden by definition from /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
  LINK    qemu-bridge-helper
  LINK    virtiofsd
/usr/bin/ld: /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o): warning: common of `__interception::real_vfork' overridden by definition from /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
  LINK    vhost-user-input
/usr/bin/ld: /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o): warning: common of `__interception::real_vfork' overridden by definition from /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
/usr/bin/ld: /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o): warning: common of `__interception::real_vfork' overridden by definition from /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
/usr/bin/ld: /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o): warning: common of `__interception::real_vfork' overridden by definition from /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
/usr/bin/ld: /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o): warning: common of `__interception::real_vfork' overridden by definition from /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
  GEN     x86_64-softmmu/hmp-commands-info.h
  GEN     x86_64-softmmu/config-devices.h
  GEN     x86_64-softmmu/hmp-commands.h
---
  CC      x86_64-softmmu/gdbstub-xml.o
  CC      x86_64-softmmu/trace/generated-helpers.o
  LINK    x86_64-softmmu/qemu-system-x86_64
/usr/bin/ld: /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o): warning: common of `__interception::real_vfork' overridden by definition from /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
common.rc: line 50: test: check: binary operator expected
(printf '#define QEMU_PKGVERSION ""\n'; printf '#define QEMU_FULL_VERSION "5.0.50"\n'; ) > qemu-version.h.tmp
make -C /tmp/qemu-test/src/slirp BUILD_DIR="/tmp/qemu-test/build/slirp" PKG_CONFIG="pkg-config" CC="clang" AR="ar"      LD="ld" RANLIB="ranlib" CFLAGS="-I/usr/include/pixman-1   -Werror -fsanitize=undefined -fsanitize=address  -pthread -I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include  -fPIE -DPIE -m64 -mcx16 -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -Wstrict-prototypes -Wredundant-decls -Wall -Wundef -Wwrite-strings -Wmissing-prototypes -fno-strict-aliasing -fno-common -fwrapv -std=gnu99  -Wold-style-definition -Wtype-limits -Wformat-security -Wformat-y2k -Winit-self -Wignored-qualifiers -Wempty-body -Wnested-externs -Wendif-labels -Wexpansion-to-defined -Wno-initializer-overrides -Wno-missing-include-dirs -Wno-shift-negative-value -Wno-string-plus-int -Wno-typedef-redefinition -Wno-tautological-type-limit-compare -fstack-protector-strong   -I/usr/include/p11-kit-1   -DSTRUCT_IOVEC_DEFINED  -I/usr/include/libpng16  -I/usr/include/spice-1 -I/usr/include/spice-server -I/usr/include/cacard -I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include -I/usr/include/nss3 -I/usr/include/nspr4 -pthread -I/usr/include/libmount -I/usr/include/blkid -I/usr/include/pixman-1   -I/tmp/qemu-test/src/tests -I/tmp/qemu-test/src/tests/qtest -g " LDFLAGS="-Wl,--warn-common -fsanitize=undefined -fsanitize=address -Wl,-z,relro -Wl,-z,now -pie -m64  -fstack-protector-strong"
---
clang -iquote /tmp/qemu-test/build/tests/qtest/libqos -iquote tests/qtest/libqos -iquote /tmp/qemu-test/src/tcg/i386 -isystem /tmp/qemu-test/src/linux-headers -isystem /tmp/qemu-test/build/linux-headers -iquote . -iquote /tmp/qemu-test/src -iquote /tmp/qemu-test/src/accel/tcg -iquote /tmp/qemu-test/src/include -iquote /tmp/qemu-test/src/disas/libvixl -I/usr/include/pixman-1   -Werror -fsanitize=undefined -fsanitize=address  -pthread -I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include  -fPIE -DPIE -m64 -mcx16 -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -Wstrict-prototypes -Wredundant-decls -Wall -Wundef -Wwrite-strings -Wmissing-prototypes -fno-strict-aliasing -fno-common -fwrapv -std=gnu99  -Wold-style-definition -Wtype-limits -Wformat-security -Wformat-y2k -Winit-self -Wignored-qualifiers -Wempty-body -Wnested-externs -Wendif-labels -Wexpansion-to-defined -Wno-initializer-overrides -Wno-missing-include-dirs -Wno-shift-negative-value -Wno-string-plus-int -Wno-typedef-redefinition -Wno-tautological-type-limit-compare -fstack-protector-strong   -I/usr/include/p11-kit-1   -DSTRUCT_IOVEC_DEFINED  -I/usr/include/libpng16  -I/usr/include/spice-1 -I/usr/include/spice-server -I/usr/include/cacard -I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include -I/usr/include/nss3 -I/usr/include/nspr4 -pthread -I/usr/include/libmount -I/usr/include/blkid -I/usr/include/pixman-1   -I/tmp/qemu-test/src/tests -I/tmp/qemu-test/src/tests/qtest -MMD -MP -MT tests/qtest/libqos/malloc-spapr.o -MF tests/qtest/libqos/malloc-spapr.d -g   -c -o tests/qtest/libqos/malloc-spapr.o /tmp/qemu-test/src/tests/qtest/libqos/malloc-spapr.c
clang -iquote /tmp/qemu-test/build/tests/qtest/libqos -iquote tests/qtest/libqos -iquote /tmp/qemu-test/src/tcg/i386 -isystem /tmp/qemu-test/src/linux-headers -isystem /tmp/qemu-test/build/linux-headers -iquote . -iquote /tmp/qemu-test/src -iquote /tmp/qemu-test/src/accel/tcg -iquote /tmp/qemu-test/src/include -iquote /tmp/qemu-test/src/disas/libvixl -I/usr/include/pixman-1   -Werror -fsanitize=undefined -fsanitize=address  -pthread -I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include  -fPIE -DPIE -m64 -mcx16 -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -Wstrict-prototypes -Wredundant-decls -Wall -Wundef -Wwrite-strings -Wmissing-prototypes -fno-strict-aliasing -fno-common -fwrapv -std=gnu99  -Wold-style-definition -Wtype-limits -Wformat-security -Wformat-y2k -Winit-self -Wignored-qualifiers -Wempty-body -Wnested-externs -Wendif-labels -Wexpansion-to-defined -Wno-initializer-overrides -Wno-missing-include-dirs -Wno-shift-negative-value -Wno-string-plus-int -Wno-typedef-redefinition -Wno-tautological-type-limit-compare -fstack-protector-strong   -I/usr/include/p11-kit-1   -DSTRUCT_IOVEC_DEFINED  -I/usr/include/libpng16  -I/usr/include/spice-1 -I/usr/include/spice-server -I/usr/include/cacard -I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include -I/usr/include/nss3 -I/usr/include/nspr4 -pthread -I/usr/include/libmount -I/usr/include/blkid -I/usr/include/pixman-1   -I/tmp/qemu-test/src/tests -I/tmp/qemu-test/src/tests/qtest -MMD -MP -MT tests/qtest/libqos/libqos-spapr.o -MF tests/qtest/libqos/libqos-spapr.d -g   -c -o tests/qtest/libqos/libqos-spapr.o /tmp/qemu-test/src/tests/qtest/libqos/libqos-spapr.c
clang -iquote /tmp/qemu-test/build/tests/qtest/libqos -iquote tests/qtest/libqos -iquote /tmp/qemu-test/src/tcg/i386 -isystem /tmp/qemu-test/src/linux-headers -isystem /tmp/qemu-test/build/linux-headers -iquote . -iquote /tmp/qemu-test/src -iquote /tmp/qemu-test/src/accel/tcg -iquote /tmp/qemu-test/src/include -iquote /tmp/qemu-test/src/disas/libvixl -I/usr/include/pixman-1   -Werror -fsanitize=undefined -fsanitize=address  -pthread -I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include  -fPIE -DPIE -m64 -mcx16 -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -Wstrict-prototypes -Wredundant-decls -Wall -Wundef -Wwrite-strings -Wmissing-prototypes -fno-strict-aliasing -fno-common -fwrapv -std=gnu99  -Wold-style-definition -Wtype-limits -Wformat-security -Wformat-y2k -Winit-self -Wignored-qualifiers -Wempty-body -Wnested-externs -Wendif-labels -Wexpansion-to-defined -Wno-initializer-overrides -Wno-missing-include-dirs -Wno-shift-negative-value -Wno-string-plus-int -Wno-typedef-redefinition -Wno-tautological-type-limit-compare -fstack-protector-strong   -I/usr/include/p11-kit-1   -DSTRUCT_IOVEC_DEFINED  -I/usr/include/libpng16  -I/usr/include/spice-1 -I/usr/include/spice-server -I/usr/include/cacard -I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include -I/usr/include/nss3 -I/usr/include/nspr4 -pthread -I/usr/include/libmount -I/usr/include/blkid -I/usr/include/pixman-1   -I/tmp/qemu-test/src/tests -I/tmp/qemu-test/src/tests/qtest -MMD -MP -MT tests/qtest/libqos/rtas.o -MF tests/qtest/libqos/rtas.d -g   -c -o tests/qtest/libqos/rtas.o /tmp/qemu-test/src/tests/qtest/libqos/rtas.c
/tmp/qemu-test/src/tests/qht-bench.c:287:29: error: implicit conversion from 'unsigned long' to 'double' changes value from 18446744073709551615 to 18446744073709551616 [-Werror,-Wimplicit-int-float-conversion]
        *threshold = rate * UINT64_MAX;
                          ~ ^~~~~~~~~~
/usr/include/stdint.h:130:23: note: expanded from macro 'UINT64_MAX'
---
18446744073709551615UL
^~~~~~~~~~~~~~~~~~~~~~
1 error generated.
make: *** [/tmp/qemu-test/src/rules.mak:69: tests/qht-bench.o] Error 1
make: *** Waiting for unfinished jobs....
clang -iquote /tmp/qemu-test/build/. -iquote . -iquote /tmp/qemu-test/src/tcg/i386 -isystem /tmp/qemu-test/src/linux-headers -isystem /tmp/qemu-test/build/linux-headers -iquote . -iquote /tmp/qemu-test/src -iquote /tmp/qemu-test/src/accel/tcg -iquote /tmp/qemu-test/src/include -iquote /tmp/qemu-test/src/disas/libvixl -I/tmp/qemu-test/src/tests/fp -I/tmp/qemu-test/src/tests/fp/berkeley-softfloat-3/source/include -I/tmp/qemu-test/src/tests/fp/berkeley-softfloat-3/source/8086-SSE -I/tmp/qemu-test/src/tests/fp/berkeley-testfloat-3/source -I/usr/include/pixman-1 -Werror -fsanitize=undefined -fsanitize=address -pthread -I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include -fPIE -DPIE -m64 -mcx16 -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -Wstrict-prototypes -Wredundant-decls -Wall -Wundef -Wwrite-strings -Wmissing-prototypes -fno-strict-aliasing -fno-common -fwrapv -std=gnu99 -Wold-style-definition -Wtype-limits -Wformat-security -Wformat-y2k -Winit-self -Wignored-qualifiers -Wempty-body -Wnested-externs -Wendif-labels -Wexpansion-to-defined -Wno-initializer-overrides -Wno-missing-include-dirs -Wno-shift-negative-value -Wno-string-plus-int -Wno-typedef-redefinition -Wno-tautological-type-limit-compare -fstack-protector-strong -I/usr/include/p11-kit-1 -DSTRUCT_IOVEC_DEFINED -I/usr/include/libpng16 -I/usr/include/spice-1 -I/usr/include/spice-server -I/usr/include/cacard -I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include -I/usr/include/nss3 -I/usr/include/nspr4 -pthread -I/usr/include/libmount -I/usr/include/blkid -I/usr/include/pixman-1 -DHW_POISON_H -DTARGET_ARM  -DSOFTFLOAT_ROUND_ODD -DINLINE_LEVEL=5 -DSOFTFLOAT_FAST_DIV32TO16 -DSOFTFLOAT_FAST_DIV64TO32 -DSOFTFLOAT_FAST_INT64  -DFLOAT16 -DFLOAT64 -DEXTFLOAT80 -DFLOAT128 -DFLOAT_ROUND_ODD -DLONG_DOUBLE_IS_EXTFLOAT80  -Wno-strict-prototypes -Wno-unknown-pragmas -Wno-uninitialized -Wno-missing-prototypes -Wno-return-type -Wno-unused-function -Wno-error -MMD -MP -MT fail.o -MF ./fail.d -g   -c -o fail.o /tmp/qemu-test/src/tests/fp/berkeley-testfloat-3/source/fail.c
clang -iquote /tmp/qemu-test/build/. -iquote . -iquote /tmp/qemu-test/src/tcg/i386 -isystem /tmp/qemu-test/src/linux-headers -isystem /tmp/qemu-test/build/linux-headers -iquote . -iquote /tmp/qemu-test/src -iquote /tmp/qemu-test/src/accel/tcg -iquote /tmp/qemu-test/src/include -iquote /tmp/qemu-test/src/disas/libvixl -I/tmp/qemu-test/src/tests/fp -I/tmp/qemu-test/src/tests/fp/berkeley-softfloat-3/source/include -I/tmp/qemu-test/src/tests/fp/berkeley-softfloat-3/source/8086-SSE -I/tmp/qemu-test/src/tests/fp/berkeley-testfloat-3/source -I/usr/include/pixman-1 -Werror -fsanitize=undefined -fsanitize=address -pthread -I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include -fPIE -DPIE -m64 -mcx16 -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -Wstrict-prototypes -Wredundant-decls -Wall -Wundef -Wwrite-strings -Wmissing-prototypes -fno-strict-aliasing -fno-common -fwrapv -std=gnu99 -Wold-style-definition -Wtype-limits -Wformat-security -Wformat-y2k -Winit-self -Wignored-qualifiers -Wempty-body -Wnested-externs -Wendif-labels -Wexpansion-to-defined -Wno-initializer-overrides -Wno-missing-include-dirs -Wno-shift-negative-value -Wno-string-plus-int -Wno-typedef-redefinition -Wno-tautological-type-limit-compare -fstack-protector-strong -I/usr/include/p11-kit-1 -DSTRUCT_IOVEC_DEFINED -I/usr/include/libpng16 -I/usr/include/spice-1 -I/usr/include/spice-server -I/usr/include/cacard -I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include -I/usr/include/nss3 -I/usr/include/nspr4 -pthread -I/usr/include/libmount -I/usr/include/blkid -I/usr/include/pixman-1 -DHW_POISON_H -DTARGET_ARM  -DSOFTFLOAT_ROUND_ODD -DINLINE_LEVEL=5 -DSOFTFLOAT_FAST_DIV32TO16 -DSOFTFLOAT_FAST_DIV64TO32 -DSOFTFLOAT_FAST_INT64  -DFLOAT16 -DFLOAT64 -DEXTFLOAT80 -DFLOAT128 -DFLOAT_ROUND_ODD -DLONG_DOUBLE_IS_EXTFLOAT80  -Wno-strict-prototypes -Wno-unknown-pragmas -Wno-uninitialized -Wno-missing-prototypes -Wno-return-type -Wno-unused-function -Wno-error -MMD -MP -MT functions_common.o -MF ./functions_common.d -g   -c -o functions_common.o /tmp/qemu-test/src/tests/fp/berkeley-testfloat-3/source/functions_common.c
---
rm -f libtestfloat.a && ar rcs libtestfloat.a uint128_inline.o uint128.o fail.o functions_common.o functionInfos.o standardFunctionInfos.o random.o genCases_common.o genCases_ui32.o genCases_ui64.o genCases_i32.o genCases_i64.o genCases_f16.o genCases_f32.o genCases_f64.o genCases_extF80.o genCases_f128.o genCases_writeTestsTotal.o verCases_inline.o verCases_common.o verCases_writeFunctionName.o readHex.o writeHex.o writeCase_a_ui32.o writeCase_a_ui64.o writeCase_a_f16.o writeCase_ab_f16.o writeCase_abc_f16.o writeCase_a_f32.o writeCase_ab_f32.o writeCase_abc_f32.o writeCase_a_f64.o writeCase_ab_f64.o writeCase_abc_f64.o writeCase_a_extF80M.o writeCase_ab_extF80M.o writeCase_a_f128M.o writeCase_ab_f128M.o writeCase_abc_f128M.o writeCase_z_bool.o writeCase_z_ui32.o writeCase_z_ui64.o writeCase_z_f16.o writeCase_z_f32.o writeCase_z_f64.o writeCase_z_extF80M.o writeCase_z_f128M.o testLoops_common.o test_a_ui32_z_f16.o test_a_ui32_z_f32.o test_a_ui32_z_f64.o test_a_ui32_z_extF80.o test_a_ui32_z_f128.o test_a_ui64_z_f16.o test_a_ui64_z_f32.o test_a_ui64_z_f64.o test_a_ui64_z_extF80.o test_a_ui64_z_f128.o test_a_i32_z_f16.o test_a_i32_z_f32.o test_a_i32_z_f64.o test_a_i32_z_extF80.o test_a_i32_z_f128.o test_a_i64_z_f16.o test_a_i64_z_f32.o test_a_i64_z_f64.o test_a_i64_z_extF80.o test_a_i64_z_f128.o test_a_f16_z_ui32_rx.o test_a_f16_z_ui64_rx.o test_a_f16_z_i32_rx.o test_a_f16_z_i64_rx.o test_a_f16_z_ui32_x.o test_a_f16_z_ui64_x.o test_a_f16_z_i32_x.o test_a_f16_z_i64_x.o test_a_f16_z_f32.o test_a_f16_z_f64.o test_a_f16_z_extF80.o test_a_f16_z_f128.o test_az_f16.o test_az_f16_rx.o test_abz_f16.o test_abcz_f16.o test_ab_f16_z_bool.o test_a_f32_z_ui32_rx.o test_a_f32_z_ui64_rx.o test_a_f32_z_i32_rx.o test_a_f32_z_i64_rx.o test_a_f32_z_ui32_x.o test_a_f32_z_ui64_x.o test_a_f32_z_i32_x.o test_a_f32_z_i64_x.o test_a_f32_z_f16.o test_a_f32_z_f64.o test_a_f32_z_extF80.o test_a_f32_z_f128.o test_az_f32.o test_az_f32_rx.o test_abz_f32.o test_abcz_f32.o test_ab_f32_z_bool.o test_a_f64_z_ui32_rx.o test_a_f64_z_ui64_rx.o test_a_f64_z_i32_rx.o test_a_f64_z_i64_rx.o test_a_f64_z_ui32_x.o test_a_f64_z_ui64_x.o test_a_f64_z_i32_x.o test_a_f64_z_i64_x.o test_a_f64_z_f16.o test_a_f64_z_f32.o test_a_f64_z_extF80.o test_a_f64_z_f128.o test_az_f64.o test_az_f64_rx.o test_abz_f64.o test_abcz_f64.o test_ab_f64_z_bool.o test_a_extF80_z_ui32_rx.o test_a_extF80_z_ui64_rx.o test_a_extF80_z_i32_rx.o test_a_extF80_z_i64_rx.o test_a_extF80_z_ui32_x.o test_a_extF80_z_ui64_x.o test_a_extF80_z_i32_x.o test_a_extF80_z_i64_x.o test_a_extF80_z_f16.o test_a_extF80_z_f32.o test_a_extF80_z_f64.o test_a_extF80_z_f128.o test_az_extF80.o test_az_extF80_rx.o test_abz_extF80.o test_ab_extF80_z_bool.o test_a_f128_z_ui32_rx.o test_a_f128_z_ui64_rx.o test_a_f128_z_i32_rx.o test_a_f128_z_i64_rx.o test_a_f128_z_ui32_x.o test_a_f128_z_ui64_x.o test_a_f128_z_i32_x.o test_a_f128_z_i64_x.o test_a_f128_z_f16.o test_a_f128_z_f32.o test_a_f128_z_f64.o test_a_f128_z_extF80.o test_az_f128.o test_az_f128_rx.o test_abz_f128.o test_abcz_f128.o test_ab_f128_z_bool.o
rm -f libsoftfloat.a && ar rcs libsoftfloat.a s_eq128.o s_le128.o s_lt128.o s_shortShiftLeft128.o s_shortShiftRight128.o s_shortShiftRightJam64.o s_shortShiftRightJam64Extra.o s_shortShiftRightJam128.o s_shortShiftRightJam128Extra.o s_shiftRightJam32.o s_shiftRightJam64.o s_shiftRightJam64Extra.o s_shiftRightJam128.o s_shiftRightJam128Extra.o s_shiftRightJam256M.o s_countLeadingZeros8.o s_countLeadingZeros16.o s_countLeadingZeros32.o s_countLeadingZeros64.o s_add128.o s_add256M.o s_sub128.o s_sub256M.o s_mul64ByShifted32To128.o s_mul64To128.o s_mul128By32.o s_mul128To256M.o s_approxRecip_1Ks.o s_approxRecip32_1.o s_approxRecipSqrt_1Ks.o s_approxRecipSqrt32_1.o s_roundToUI32.o s_roundToUI64.o s_roundToI32.o s_roundToI64.o s_normSubnormalF16Sig.o s_roundPackToF16.o s_normRoundPackToF16.o s_addMagsF16.o s_subMagsF16.o s_mulAddF16.o s_normSubnormalF32Sig.o s_roundPackToF32.o s_normRoundPackToF32.o s_addMagsF32.o s_subMagsF32.o s_mulAddF32.o s_normSubnormalF64Sig.o s_roundPackToF64.o s_normRoundPackToF64.o s_addMagsF64.o s_subMagsF64.o s_mulAddF64.o s_normSubnormalExtF80Sig.o s_roundPackToExtF80.o s_normRoundPackToExtF80.o s_addMagsExtF80.o s_subMagsExtF80.o s_normSubnormalF128Sig.o s_roundPackToF128.o s_normRoundPackToF128.o s_addMagsF128.o s_subMagsF128.o s_mulAddF128.o softfloat_state.o ui32_to_f16.o ui32_to_f32.o ui32_to_f64.o ui32_to_extF80.o ui32_to_extF80M.o ui32_to_f128.o ui32_to_f128M.o ui64_to_f16.o ui64_to_f32.o ui64_to_f64.o ui64_to_extF80.o ui64_to_extF80M.o ui64_to_f128.o ui64_to_f128M.o i32_to_f16.o i32_to_f32.o i32_to_f64.o i32_to_extF80.o i32_to_extF80M.o i32_to_f128.o i32_to_f128M.o i64_to_f16.o i64_to_f32.o i64_to_f64.o i64_to_extF80.o i64_to_extF80M.o i64_to_f128.o i64_to_f128M.o f16_to_ui32.o f16_to_ui64.o f16_to_i32.o f16_to_i64.o f16_to_ui32_r_minMag.o f16_to_ui64_r_minMag.o f16_to_i32_r_minMag.o f16_to_i64_r_minMag.o f16_to_f32.o f16_to_f64.o f16_to_extF80.o f16_to_extF80M.o f16_to_f128.o f16_to_f128M.o f16_roundToInt.o f16_add.o f16_sub.o f16_mul.o f16_mulAdd.o f16_div.o f16_rem.o f16_sqrt.o f16_eq.o f16_le.o f16_lt.o f16_eq_signaling.o f16_le_quiet.o f16_lt_quiet.o f16_isSignalingNaN.o f32_to_ui32.o f32_to_ui64.o f32_to_i32.o f32_to_i64.o f32_to_ui32_r_minMag.o f32_to_ui64_r_minMag.o f32_to_i32_r_minMag.o f32_to_i64_r_minMag.o f32_to_f16.o f32_to_f64.o f32_to_extF80.o f32_to_extF80M.o f32_to_f128.o f32_to_f128M.o f32_roundToInt.o f32_add.o f32_sub.o f32_mul.o f32_mulAdd.o f32_div.o f32_rem.o f32_sqrt.o f32_eq.o f32_le.o f32_lt.o f32_eq_signaling.o f32_le_quiet.o f32_lt_quiet.o f32_isSignalingNaN.o f64_to_ui32.o f64_to_ui64.o f64_to_i32.o f64_to_i64.o f64_to_ui32_r_minMag.o f64_to_ui64_r_minMag.o f64_to_i32_r_minMag.o f64_to_i64_r_minMag.o f64_to_f16.o f64_to_f32.o f64_to_extF80.o f64_to_extF80M.o f64_to_f128.o f64_to_f128M.o f64_roundToInt.o f64_add.o f64_sub.o f64_mul.o f64_mulAdd.o f64_div.o f64_rem.o f64_sqrt.o f64_eq.o f64_le.o f64_lt.o f64_eq_signaling.o f64_le_quiet.o f64_lt_quiet.o f64_isSignalingNaN.o extF80_to_ui32.o extF80_to_ui64.o extF80_to_i32.o extF80_to_i64.o extF80_to_ui32_r_minMag.o extF80_to_ui64_r_minMag.o extF80_to_i32_r_minMag.o extF80_to_i64_r_minMag.o extF80_to_f16.o extF80_to_f32.o extF80_to_f64.o extF80_to_f128.o extF80_roundToInt.o extF80_add.o extF80_sub.o extF80_mul.o extF80_div.o extF80_rem.o extF80_sqrt.o extF80_eq.o extF80_le.o extF80_lt.o extF80_eq_signaling.o extF80_le_quiet.o extF80_lt_quiet.o extF80_isSignalingNaN.o extF80M_to_ui32.o extF80M_to_ui64.o extF80M_to_i32.o extF80M_to_i64.o extF80M_to_ui32_r_minMag.o extF80M_to_ui64_r_minMag.o extF80M_to_i32_r_minMag.o extF80M_to_i64_r_minMag.o extF80M_to_f16.o extF80M_to_f32.o extF80M_to_f64.o extF80M_to_f128M.o extF80M_roundToInt.o extF80M_add.o extF80M_sub.o extF80M_mul.o extF80M_div.o extF80M_rem.o extF80M_sqrt.o extF80M_eq.o extF80M_le.o extF80M_lt.o extF80M_eq_signaling.o extF80M_le_quiet.o extF80M_lt_quiet.o f128_to_ui32.o f128_to_ui64.o f128_to_i32.o f128_to_i64.o f128_to_ui32_r_minMag.o f128_to_ui64_r_minMag.o f128_to_i32_r_minMag.o f128_to_i64_r_minMag.o f128_to_f16.o f128_to_f32.o f128_to_extF80.o f128_to_f64.o f128_roundToInt.o f128_add.o f128_sub.o f128_mul.o f128_mulAdd.o f128_div.o f128_rem.o f128_sqrt.o f128_eq.o f128_le.o f128_lt.o f128_eq_signaling.o f128_le_quiet.o f128_lt_quiet.o f128_isSignalingNaN.o f128M_to_ui32.o f128M_to_ui64.o f128M_to_i32.o f128M_to_i64.o f128M_to_ui32_r_minMag.o f128M_to_ui64_r_minMag.o f128M_to_i32_r_minMag.o f128M_to_i64_r_minMag.o f128M_to_f16.o f128M_to_f32.o f128M_to_extF80M.o f128M_to_f64.o f128M_roundToInt.o f128M_add.o f128M_sub.o f128M_mul.o f128M_mulAdd.o f128M_div.o f128M_rem.o f128M_sqrt.o f128M_eq.o f128M_le.o f128M_lt.o f128M_eq_signaling.o f128M_le_quiet.o f128M_lt_quiet.o softfloat_raiseFlags.o s_f16UIToCommonNaN.o s_commonNaNToF16UI.o s_propagateNaNF16UI.o s_f32UIToCommonNaN.o s_commonNaNToF32UI.o s_propagateNaNF32UI.o s_f64UIToCommonNaN.o s_commonNaNToF64UI.o s_propagateNaNF64UI.o extF80M_isSignalingNaN.o s_extF80UIToCommonNaN.o s_commonNaNToExtF80UI.o s_propagateNaNExtF80UI.o f128M_isSignalingNaN.o s_f128UIToCommonNaN.o s_commonNaNToF128UI.o s_propagateNaNF128UI.o
clang++ -g  -Wl,--warn-common -fsanitize=undefined -fsanitize=address -Wl,-z,relro -Wl,-z,now -pie -m64  -fstack-protector-strong -o fp-test fp-test.o slowfloat.o softfloat.o  libtestfloat.a libsoftfloat.a /tmp/qemu-test/build/libqemuutil.a   -lm -lz  -lgthread-2.0 -pthread -lglib-2.0  -lnettle  -lgnutls  -lzstd   -lrt
/usr/bin/ld: /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors_vfork.S.o): warning: common of `__interception::real_vfork' overridden by definition from /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.asan-x86_64.a(asan_interceptors.cpp.o)
make[1]: Leaving directory '/tmp/qemu-test/build/tests/fp'
Traceback (most recent call last):
  File "./tests/docker/docker.py", line 669, in <module>
---
    raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', '--label', 'com.qemu.instance.uuid=25dd4a3d2d1748a1946b005588d35cd5', '-u', '1003', '--security-opt', 'seccomp=unconfined', '--rm', '-e', 'TARGET_LIST=x86_64-softmmu', '-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', '-e', 'SHOW_ENV=', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', '/home/patchew2/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', '/var/tmp/patchew-tester-tmp-o7o8446p/src/docker-src.2020-06-19-07.50.55.27161:/var/tmp/qemu:z,ro', 'qemu:fedora', '/var/tmp/qemu/run', 'test-debug']' returned non-zero exit status 2.
filter=--filter=label=com.qemu.instance.uuid=25dd4a3d2d1748a1946b005588d35cd5
make[1]: *** [docker-run] Error 1
make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-o7o8446p/src'
make: *** [docker-run-test-debug@fedora] Error 2

real    4m43.095s
user    0m7.721s


The full log is available at
http://patchew.org/logs/20200619104012.235977-1-mreitz@redhat.com/testing.asan/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [PATCH 1/2] qcow2: Force preallocation with data-file-raw

[Alberto Garcia]

On Fri 19 Jun 2020 12:40:11 PM CEST, Max Reitz wrote:
> +    if (qcow2_opts->data_file_raw &&
> +        qcow2_opts->preallocation == PREALLOC_MODE_OFF)
> +    {
> +        /*
> +         * data-file-raw means that "the external data file can be
> +         * read as a consistent standalone raw image without looking
> +         * at the qcow2 metadata."  It does not say that the metadata
> +         * must be ignored, though (and the qcow2 driver in fact does
> +         * not ignore it), so the L1/L2 tables must be present and
> +         * give a 1:1 mapping, so you get the same result regardless
> +         * of whether you look at the metadata or whether you ignore
> +         * it.
> +         */
> +        qcow2_opts->preallocation = PREALLOC_MODE_METADATA;

I'm not convinced by this, but your comment made me think of another
possible alternative: in qcow2_get_cluster_offset(), if the cluster is
unallocated and we are using a raw data file then we return _ZERO_PLAIN:

--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -654,6 +654,10 @@ out:
     assert(bytes_available - offset_in_cluster <= UINT_MAX);
     *bytes = bytes_available - offset_in_cluster;
 
+    if (type == QCOW2_CLUSTER_UNALLOCATED && data_file_is_raw(bs)) {
+        type = QCOW2_CLUSTER_ZERO_PLAIN;
+    }
+
     return type;

You could even add a '&& bs->backing' to the condition and emit a
warning to make it more explicit.

Berto

Re: [PATCH 0/2] qcow2: Force preallocation with data-file-raw

[Nir Soffer]

On Fri, Jun 19, 2020 at 1:40 PM Max Reitz <mreitz@redhat.com> wrote:
>
> Hi,
>
> As discussed here:
>
> https://lists.nongnu.org/archive/html/qemu-block/2020-02/msg00644.html
> https://lists.nongnu.org/archive/html/qemu-block/2020-04/msg00329.html
> https://lists.nongnu.org/archive/html/qemu-block/2020-06/msg00240.html
>
> I think that qcow2 images with data-file-raw should always have
> preallocated 1:1 L1/L2 tables, so that the image always looks the same
> whether you respect or ignore the qcow2 metadata.

I don't know the internals of qcow2 data_file, but are we really using
qcow2 metadata
when accessing the data file? This may have unwanted performance consequences.

If I understand correctly, qcow2 metadata is needed only for keeping
bitmaps (or maybe
future extensions) for raw data file, and reading from the qcow2 image
should be read
directly from the raw file without any extra work.

Writing to the data file should also bypass the qcow2 metadata, since the bitmap
is updated in memory.

>  The easiest way to
> achieve that is to enforce at least metadata preallocation whenever
> data-file-raw is given.

But preallocation is not free, even on file systems, it can be even
slow (NFS < 4.2).
With block storage this means you need to allocate the entire image size on
storage for writing the metadata.

While oVirt does not use qcow2 with data_file, having preallocated qcow2
will make this very hard to use, for example for 500 GiB disk we will have to
allocate 500 GiB disk for the raw data file and 500 GiB disk for the qcow2
metadata disk which will be 99% unused.

I don't think that kubevirt is planning to use this either, but if
they decide to use
this it may be a problem for them as well when using block storage.

It looks like we abuse preallocation for getting the side effect that
the backing file
will be rejected, instead of adding the validation rejecting backing
file in this case.

Nir


Nir

> Max Reitz (2):
>   qcow2: Force preallocation with data-file-raw
>   iotests/244: Test preallocation for data-file-raw
>
>  block/qcow2.c              | 22 +++++++++++++
>  tests/qemu-iotests/244     | 65 ++++++++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/244.out | 32 ++++++++++++++++---
>  3 files changed, 114 insertions(+), 5 deletions(-)
>
> --
> 2.26.2
>
>

Re: [PATCH 1/2] qcow2: Force preallocation with data-file-raw

[Max Reitz]

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

On 19.06.20 18:47, Alberto Garcia wrote:
> On Fri 19 Jun 2020 12:40:11 PM CEST, Max Reitz wrote:
>> +    if (qcow2_opts->data_file_raw &&
>> +        qcow2_opts->preallocation == PREALLOC_MODE_OFF)
>> +    {
>> +        /*
>> +         * data-file-raw means that "the external data file can be
>> +         * read as a consistent standalone raw image without looking
>> +         * at the qcow2 metadata."  It does not say that the metadata
>> +         * must be ignored, though (and the qcow2 driver in fact does
>> +         * not ignore it), so the L1/L2 tables must be present and
>> +         * give a 1:1 mapping, so you get the same result regardless
>> +         * of whether you look at the metadata or whether you ignore
>> +         * it.
>> +         */
>> +        qcow2_opts->preallocation = PREALLOC_MODE_METADATA;
> 
> I'm not convinced by this,

Why not?

This is how I read the spec.  Furthermore, I see two problems that we
have right now that are fixed by this patch (namely (1) using a device
file as the external data file, which may have non-zero data at
creation; and (2) assigning a backing file at runtime must not show the
data).

> but your comment made me think of another
> possible alternative: in qcow2_get_cluster_offset(), if the cluster is
> unallocated and we are using a raw data file then we return _ZERO_PLAIN:
> 
> --- a/block/qcow2-cluster.c
> +++ b/block/qcow2-cluster.c
> @@ -654,6 +654,10 @@ out:
>      assert(bytes_available - offset_in_cluster <= UINT_MAX);
>      *bytes = bytes_available - offset_in_cluster;
>  
> +    if (type == QCOW2_CLUSTER_UNALLOCATED && data_file_is_raw(bs)) {
> +        type = QCOW2_CLUSTER_ZERO_PLAIN;
> +    }
> +
>      return type;
> 
> You could even add a '&& bs->backing' to the condition and emit a
> warning to make it more explicit.

No, this is wrong.  This still wouldn’t fix the problem of having a
device file as the external data file, when it already has non-zero data
during creation.  (Reading the qcow2 file would return zeroes, but
reading the device would not.)

So it would need to be QCOW2_CLUSTER_NORMAL.  Which is kind of the
point, when you think about it – with data-file-raw, all clusters must
always effectively be QCOW2_CLUSTER_NORMAL and be mapped 1:1.

Well, and that’s in turn the point of this patch.

I interpret the spec in that the metadata can be ignored, but it does
not need to be ignored.  So the L1/L2 tables must be 1:1 mapping of
QCOW2_CLUSTER_NORMAL entries.

We could also choose to interpret it as “With data-file-raw, the L1/L2
tables must be ignored”.  In that case, our qcow2 driver would need to
be modified to indeed fully ignore the L1/L2 tables with data-file-raw.
 (I certainly don’t interpret the spec this way, but I suppose we could
call it a bug fix and amend it.)

Max


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

Re: [PATCH 0/2] qcow2: Force preallocation with data-file-raw

[Max Reitz]

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

On 22.06.20 00:25, Nir Soffer wrote:
> On Fri, Jun 19, 2020 at 1:40 PM Max Reitz <mreitz@redhat.com> wrote:
>>
>> Hi,
>>
>> As discussed here:
>>
>> https://lists.nongnu.org/archive/html/qemu-block/2020-02/msg00644.html
>> https://lists.nongnu.org/archive/html/qemu-block/2020-04/msg00329.html
>> https://lists.nongnu.org/archive/html/qemu-block/2020-06/msg00240.html
>>
>> I think that qcow2 images with data-file-raw should always have
>> preallocated 1:1 L1/L2 tables, so that the image always looks the same
>> whether you respect or ignore the qcow2 metadata.
> 
> I don't know the internals of qcow2 data_file, but are we really using
> qcow2 metadata when accessing the data file?

Yes.

> This may have unwanted performance consequences.

I don’t think so, because in practice normal lookups of L1/L2 mappings
generally don’t cost that much performance.

> If I understand correctly, qcow2 metadata is needed only for keeping
> bitmaps (or maybe
> future extensions) for raw data file, and reading from the qcow2 image
> should be read
> directly from the raw file without any extra work.
> 
> Writing to the data file should also bypass the qcow2 metadata, since the bitmap
> is updated in memory.

Well, with this series, writing would no longer update the metadata at
least, because it would always be preallocated already.

>>  The easiest way to
>> achieve that is to enforce at least metadata preallocation whenever
>> data-file-raw is given.
> 
> But preallocation is not free, even on file systems, it can be even
> slow (NFS < 4.2).

Metadata preallocation with an external data file should be the same
speed on every file system.  We only need to create the metadata
structures, which, with the default cluster size (64k) take up a bit
more than 1/8192 of the full image size.

Sure, it’s not free.  But if we decide we should indeed fully ignore the
L1/L2 tables for data-file-raw images, the qcow2 spec must be amended.
As I can read it, it currently doesn’t say so.

(By the way, this is not a trivial change.  Right now, data-file-raw is
an autoclear flag: If a version of qemu that doesn’t support it accesses
the image, it will automatically clear the flag, but the image stays
valid.  If we decide to completely ignore the L1/L2 tables (i.e. not
even create them), then this can no longer be an autoclear flag.  We’d
need a new incompatible flag.  (Because without L1/L2 tables, the image
becomes useless to older qemu versions.))

> With block storage this means you need to allocate the entire image size on
> storage for writing the metadata.
> 
> While oVirt does not use qcow2 with data_file, having preallocated qcow2
> will make this very hard to use, for example for 500 GiB disk we will have to
> allocate 500 GiB disk for the raw data file and 500 GiB disk for the qcow2
> metadata disk which will be 99% unused.

I don’t understand this.  When you use an external data file, the qcow2
file will only contain the metadata:

$ qemu-img create -f qcow2 \
    -o data_file=foo.data,data_file_raw=on,preallocation=metadata \
    foo.qcow2 8G
Formatting 'foo.qcow2', fmt=qcow2 size=8589934592 data_file=foo.data
data_file_raw=on cluster_size=65536 preallocation=metadata
lazy_refcounts=off refcount_bits=16
$ ls -l foo.qcow2
... 1310720 ... foo.qcow2
$ ls -l foo.data
... 8589934592 ... foo.data

> I don't think that kubevirt is planning to use this either, but if
> they decide to use
> this it may be a problem for them as well when using block storage.
> 
> It looks like we abuse preallocation for getting the side effect that
> the backing file
> will be rejected, instead of adding the validation rejecting backing
> file in this case.

That isn’t the case.

I want to use preallocation because I interpret the spec such that it
requires metadata preallocation.  It says when accessing a qcow2 file
with data-file-raw, you can ignore the L1/L2 tables.  To me, that means
that the L1/L2 tables must give a 1:1 mapping so that you get the same
result whether you interpret them or not.

Max


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

Re: [PATCH 1/2] qcow2: Force preallocation with data-file-raw

[Max Reitz]

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

On 22.06.20 11:35, Max Reitz wrote:
> On 19.06.20 18:47, Alberto Garcia wrote:
>> On Fri 19 Jun 2020 12:40:11 PM CEST, Max Reitz wrote:
>>> +    if (qcow2_opts->data_file_raw &&
>>> +        qcow2_opts->preallocation == PREALLOC_MODE_OFF)
>>> +    {
>>> +        /*
>>> +         * data-file-raw means that "the external data file can be
>>> +         * read as a consistent standalone raw image without looking
>>> +         * at the qcow2 metadata."  It does not say that the metadata
>>> +         * must be ignored, though (and the qcow2 driver in fact does
>>> +         * not ignore it), so the L1/L2 tables must be present and
>>> +         * give a 1:1 mapping, so you get the same result regardless
>>> +         * of whether you look at the metadata or whether you ignore
>>> +         * it.
>>> +         */
>>> +        qcow2_opts->preallocation = PREALLOC_MODE_METADATA;
>>
>> I'm not convinced by this,
> 
> Why not?
> 
> This is how I read the spec.  Furthermore, I see two problems that we
> have right now that are fixed by this patch (namely (1) using a device
> file as the external data file, which may have non-zero data at
> creation; and (2) assigning a backing file at runtime must not show the
> data).
> 
>> but your comment made me think of another
>> possible alternative: in qcow2_get_cluster_offset(), if the cluster is
>> unallocated and we are using a raw data file then we return _ZERO_PLAIN:
>>
>> --- a/block/qcow2-cluster.c
>> +++ b/block/qcow2-cluster.c
>> @@ -654,6 +654,10 @@ out:
>>      assert(bytes_available - offset_in_cluster <= UINT_MAX);
>>      *bytes = bytes_available - offset_in_cluster;
>>  
>> +    if (type == QCOW2_CLUSTER_UNALLOCATED && data_file_is_raw(bs)) {
>> +        type = QCOW2_CLUSTER_ZERO_PLAIN;
>> +    }
>> +
>>      return type;
>>
>> You could even add a '&& bs->backing' to the condition and emit a
>> warning to make it more explicit.
> 
> No, this is wrong.  This still wouldn’t fix the problem of having a
> device file as the external data file, when it already has non-zero data
> during creation.  (Reading the qcow2 file would return zeroes, but
> reading the device would not.)
> 
> So it would need to be QCOW2_CLUSTER_NORMAL.  Which is kind of the
> point, when you think about it – with data-file-raw, all clusters must
> always effectively be QCOW2_CLUSTER_NORMAL and be mapped 1:1.
> 
> Well, and that’s in turn the point of this patch.
> 
> I interpret the spec in that the metadata can be ignored, but it does
> not need to be ignored.  So the L1/L2 tables must be 1:1 mapping of
> QCOW2_CLUSTER_NORMAL entries.
> 
> We could also choose to interpret it as “With data-file-raw, the L1/L2
> tables must be ignored”.  In that case, our qcow2 driver would need to
> be modified to indeed fully ignore the L1/L2 tables with data-file-raw.
>  (I certainly don’t interpret the spec this way, but I suppose we could
> call it a bug fix and amend it.)

I just realized that this is not possible.  data-file-raw is an
autoclear flag, so the image must appear the same to qemu versions that
do not support it.

If we want to fully ignore the L1/L2 tables or interpret them some
non-default way (like you’re proposing), we would have to add a new
incompatible flag.

Max


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

Re: [PATCH 1/2] qcow2: Force preallocation with data-file-raw

[Alberto Garcia]

On Mon 22 Jun 2020 11:35:59 AM CEST, Max Reitz wrote:
>>> +    if (qcow2_opts->data_file_raw &&
>>> +        qcow2_opts->preallocation == PREALLOC_MODE_OFF)
>>> +    {
>>> +        /*
>>> +         * data-file-raw means that "the external data file can be
>>> +         * read as a consistent standalone raw image without looking
>>> +         * at the qcow2 metadata."  It does not say that the metadata
>>> +         * must be ignored, though (and the qcow2 driver in fact does
>>> +         * not ignore it), so the L1/L2 tables must be present and
>>> +         * give a 1:1 mapping, so you get the same result regardless
>>> +         * of whether you look at the metadata or whether you ignore
>>> +         * it.
>>> +         */
>>> +        qcow2_opts->preallocation = PREALLOC_MODE_METADATA;
>> 
>> I'm not convinced by this,
>
> Why not?
>
> This is how I read the spec.  Furthermore, I see two problems that we
> have right now that are fixed by this patch (namely (1) using a device
> file as the external data file, which may have non-zero data at
> creation; and (2) assigning a backing file at runtime must not show
> the data).

What happens if you first create the image (which would be preallocated
with this patch), then you resize it and finally you assign the backing
file? Would the resized part be preallocated?

>> but your comment made me think of another possible alternative: in
>> qcow2_get_cluster_offset(), if the cluster is unallocated and we are
>> using a raw data file then we return _ZERO_PLAIN:
>> 
>> --- a/block/qcow2-cluster.c
>> +++ b/block/qcow2-cluster.c
>> @@ -654,6 +654,10 @@ out:
>>      assert(bytes_available - offset_in_cluster <= UINT_MAX);
>>      *bytes = bytes_available - offset_in_cluster;
>>  
>> +    if (type == QCOW2_CLUSTER_UNALLOCATED && data_file_is_raw(bs)) {
>> +        type = QCOW2_CLUSTER_ZERO_PLAIN;
>> +    }
>> +
>>      return type;
>> 
>> You could even add a '&& bs->backing' to the condition and emit a
>> warning to make it more explicit.
>
> No, this is wrong.  This still wouldn’t fix the problem of having a
> device file as the external data file, when it already has non-zero
> data during creation.  (Reading the qcow2 file would return zeroes,
> but reading the device would not.)

But you wouldn't fix that preallocating the metadata either, you would
need to fill the device with zeroes.

> I interpret the spec in that the metadata can be ignored, but it does
> not need to be ignored.  So the L1/L2 tables must be 1:1 mapping of
> QCOW2_CLUSTER_NORMAL entries.
>
> We could also choose to interpret it as “With data-file-raw, the L1/L2
> tables must be ignored”.  In that case, our qcow2 driver would need to
> be modified to indeed fully ignore the L1/L2 tables with
> data-file-raw.  (I certainly don’t interpret the spec this way, but I
> suppose we could call it a bug fix and amend it.)

The way I interpret it is that regardless of whether you read the data
through the qcow2 file or directly from the data file you should get the
same results, but how that should be reflected in the L1/L2 metadata is
not specified.

Berto

Re: [PATCH 1/2] qcow2: Force preallocation with data-file-raw

[Max Reitz]

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

On 22.06.20 16:46, Alberto Garcia wrote:
> On Mon 22 Jun 2020 11:35:59 AM CEST, Max Reitz wrote:
>>>> +    if (qcow2_opts->data_file_raw &&
>>>> +        qcow2_opts->preallocation == PREALLOC_MODE_OFF)
>>>> +    {
>>>> +        /*
>>>> +         * data-file-raw means that "the external data file can be
>>>> +         * read as a consistent standalone raw image without looking
>>>> +         * at the qcow2 metadata."  It does not say that the metadata
>>>> +         * must be ignored, though (and the qcow2 driver in fact does
>>>> +         * not ignore it), so the L1/L2 tables must be present and
>>>> +         * give a 1:1 mapping, so you get the same result regardless
>>>> +         * of whether you look at the metadata or whether you ignore
>>>> +         * it.
>>>> +         */
>>>> +        qcow2_opts->preallocation = PREALLOC_MODE_METADATA;
>>>
>>> I'm not convinced by this,
>>
>> Why not?
>>
>> This is how I read the spec.  Furthermore, I see two problems that we
>> have right now that are fixed by this patch (namely (1) using a device
>> file as the external data file, which may have non-zero data at
>> creation; and (2) assigning a backing file at runtime must not show
>> the data).
> 
> What happens if you first create the image (which would be preallocated
> with this patch), then you resize it and finally you assign the backing
> file? Would the resized part be preallocated?

Good point, when resizing an image with data-file-raw we also need to
preallocate the L2 tables.

>>> but your comment made me think of another possible alternative: in
>>> qcow2_get_cluster_offset(), if the cluster is unallocated and we are
>>> using a raw data file then we return _ZERO_PLAIN:
>>>
>>> --- a/block/qcow2-cluster.c
>>> +++ b/block/qcow2-cluster.c
>>> @@ -654,6 +654,10 @@ out:
>>>      assert(bytes_available - offset_in_cluster <= UINT_MAX);
>>>      *bytes = bytes_available - offset_in_cluster;
>>>  
>>> +    if (type == QCOW2_CLUSTER_UNALLOCATED && data_file_is_raw(bs)) {
>>> +        type = QCOW2_CLUSTER_ZERO_PLAIN;
>>> +    }
>>> +
>>>      return type;
>>>
>>> You could even add a '&& bs->backing' to the condition and emit a
>>> warning to make it more explicit.
>>
>> No, this is wrong.  This still wouldn’t fix the problem of having a
>> device file as the external data file, when it already has non-zero
>> data during creation.  (Reading the qcow2 file would return zeroes,
>> but reading the device would not.)
> 
> But you wouldn't fix that preallocating the metadata either, you would
> need to fill the device with zeroes.

What it fixes is that reading the qcow2 image and the raw device returns
the same data.

Initially, I also thought that we should initialize raw data files to be
zero during creation, but Eric changed my mind:

https://lists.nongnu.org/archive/html/qemu-block/2020-04/msg00223.html

>> I interpret the spec in that the metadata can be ignored, but it does
>> not need to be ignored.  So the L1/L2 tables must be 1:1 mapping of
>> QCOW2_CLUSTER_NORMAL entries.
>>
>> We could also choose to interpret it as “With data-file-raw, the L1/L2
>> tables must be ignored”.  In that case, our qcow2 driver would need to
>> be modified to indeed fully ignore the L1/L2 tables with
>> data-file-raw.  (I certainly don’t interpret the spec this way, but I
>> suppose we could call it a bug fix and amend it.)
> 
> The way I interpret it is that regardless of whether you read the data
> through the qcow2 file or directly from the data file you should get the
> same results, but how that should be reflected in the L1/L2 metadata is
> not specified.

That’s an absolute given, but the question is what does “reading through
the qcow2 file” mean.  Respecting the metadata?  Ignoring it?  Something
in between?

As I noted in my reply to myself, data-file-raw is an autoclear flag.
That means, an old version of qemu that doesn’t recognize the flag must
read the same data as a new version.  It follows that the the L2 tables
must be a 1:1 mapping.  (Or the flag can’t be an autoclear flag.)

Max


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

Re: [PATCH 1/2] qcow2: Force preallocation with data-file-raw

[Nir Soffer]

On Mon, Jun 22, 2020 at 6:07 PM Max Reitz <mreitz@redhat.com> wrote:
>
> On 22.06.20 16:46, Alberto Garcia wrote:
> > On Mon 22 Jun 2020 11:35:59 AM CEST, Max Reitz wrote:
> >>>> +    if (qcow2_opts->data_file_raw &&
> >>>> +        qcow2_opts->preallocation == PREALLOC_MODE_OFF)
> >>>> +    {
> >>>> +        /*
> >>>> +         * data-file-raw means that "the external data file can be
> >>>> +         * read as a consistent standalone raw image without looking
> >>>> +         * at the qcow2 metadata."  It does not say that the metadata
> >>>> +         * must be ignored, though (and the qcow2 driver in fact does
> >>>> +         * not ignore it), so the L1/L2 tables must be present and
> >>>> +         * give a 1:1 mapping, so you get the same result regardless
> >>>> +         * of whether you look at the metadata or whether you ignore
> >>>> +         * it.
> >>>> +         */
> >>>> +        qcow2_opts->preallocation = PREALLOC_MODE_METADATA;
> >>>
> >>> I'm not convinced by this,
> >>
> >> Why not?
> >>
> >> This is how I read the spec.  Furthermore, I see two problems that we
> >> have right now that are fixed by this patch (namely (1) using a device
> >> file as the external data file, which may have non-zero data at
> >> creation; and (2) assigning a backing file at runtime must not show
> >> the data).
> >
> > What happens if you first create the image (which would be preallocated
> > with this patch), then you resize it and finally you assign the backing
> > file? Would the resized part be preallocated?
>
> Good point, when resizing an image with data-file-raw we also need to
> preallocate the L2 tables.
>
> >>> but your comment made me think of another possible alternative: in
> >>> qcow2_get_cluster_offset(), if the cluster is unallocated and we are
> >>> using a raw data file then we return _ZERO_PLAIN:
> >>>
> >>> --- a/block/qcow2-cluster.c
> >>> +++ b/block/qcow2-cluster.c
> >>> @@ -654,6 +654,10 @@ out:
> >>>      assert(bytes_available - offset_in_cluster <= UINT_MAX);
> >>>      *bytes = bytes_available - offset_in_cluster;
> >>>
> >>> +    if (type == QCOW2_CLUSTER_UNALLOCATED && data_file_is_raw(bs)) {
> >>> +        type = QCOW2_CLUSTER_ZERO_PLAIN;
> >>> +    }
> >>> +
> >>>      return type;
> >>>
> >>> You could even add a '&& bs->backing' to the condition and emit a
> >>> warning to make it more explicit.
> >>
> >> No, this is wrong.  This still wouldn’t fix the problem of having a
> >> device file as the external data file, when it already has non-zero
> >> data during creation.  (Reading the qcow2 file would return zeroes,
> >> but reading the device would not.)
> >
> > But you wouldn't fix that preallocating the metadata either, you would
> > need to fill the device with zeroes.
>
> What it fixes is that reading the qcow2 image and the raw device returns
> the same data.
>
> Initially, I also thought that we should initialize raw data files to be
> zero during creation, but Eric changed my mind:
>
> https://lists.nongnu.org/archive/html/qemu-block/2020-04/msg00223.html
>
> >> I interpret the spec in that the metadata can be ignored, but it does
> >> not need to be ignored.  So the L1/L2 tables must be 1:1 mapping of
> >> QCOW2_CLUSTER_NORMAL entries.
> >>
> >> We could also choose to interpret it as “With data-file-raw, the L1/L2
> >> tables must be ignored”.  In that case, our qcow2 driver would need to
> >> be modified to indeed fully ignore the L1/L2 tables with
> >> data-file-raw.  (I certainly don’t interpret the spec this way, but I
> >> suppose we could call it a bug fix and amend it.)
> >
> > The way I interpret it is that regardless of whether you read the data
> > through the qcow2 file or directly from the data file you should get the
> > same results, but how that should be reflected in the L1/L2 metadata is
> > not specified.
>
> That’s an absolute given, but the question is what does “reading through
> the qcow2 file” mean.  Respecting the metadata?  Ignoring it?  Something
> in between?
>
> As I noted in my reply to myself, data-file-raw is an autoclear flag.
> That means, an old version of qemu that doesn’t recognize the flag must
> read the same data as a new version.  It follows that the the L2 tables
> must be a 1:1 mapping.  (Or the flag can’t be an autoclear flag.)

Being able to read sounds like a nice to have feature, but what about writing?

I hope that the image is not writable by older versions that do not understand
data_file. Otherwise older qemu versions can corrupt the image silently.

Re: [PATCH 1/2] qcow2: Force preallocation with data-file-raw

[Max Reitz]

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

On 22.06.20 17:15, Nir Soffer wrote:
> On Mon, Jun 22, 2020 at 6:07 PM Max Reitz <mreitz@redhat.com> wrote:
>>
>> On 22.06.20 16:46, Alberto Garcia wrote:
>>> On Mon 22 Jun 2020 11:35:59 AM CEST, Max Reitz wrote:
>>>>>> +    if (qcow2_opts->data_file_raw &&
>>>>>> +        qcow2_opts->preallocation == PREALLOC_MODE_OFF)
>>>>>> +    {
>>>>>> +        /*
>>>>>> +         * data-file-raw means that "the external data file can be
>>>>>> +         * read as a consistent standalone raw image without looking
>>>>>> +         * at the qcow2 metadata."  It does not say that the metadata
>>>>>> +         * must be ignored, though (and the qcow2 driver in fact does
>>>>>> +         * not ignore it), so the L1/L2 tables must be present and
>>>>>> +         * give a 1:1 mapping, so you get the same result regardless
>>>>>> +         * of whether you look at the metadata or whether you ignore
>>>>>> +         * it.
>>>>>> +         */
>>>>>> +        qcow2_opts->preallocation = PREALLOC_MODE_METADATA;
>>>>>
>>>>> I'm not convinced by this,
>>>>
>>>> Why not?
>>>>
>>>> This is how I read the spec.  Furthermore, I see two problems that we
>>>> have right now that are fixed by this patch (namely (1) using a device
>>>> file as the external data file, which may have non-zero data at
>>>> creation; and (2) assigning a backing file at runtime must not show
>>>> the data).
>>>
>>> What happens if you first create the image (which would be preallocated
>>> with this patch), then you resize it and finally you assign the backing
>>> file? Would the resized part be preallocated?
>>
>> Good point, when resizing an image with data-file-raw we also need to
>> preallocate the L2 tables.
>>
>>>>> but your comment made me think of another possible alternative: in
>>>>> qcow2_get_cluster_offset(), if the cluster is unallocated and we are
>>>>> using a raw data file then we return _ZERO_PLAIN:
>>>>>
>>>>> --- a/block/qcow2-cluster.c
>>>>> +++ b/block/qcow2-cluster.c
>>>>> @@ -654,6 +654,10 @@ out:
>>>>>      assert(bytes_available - offset_in_cluster <= UINT_MAX);
>>>>>      *bytes = bytes_available - offset_in_cluster;
>>>>>
>>>>> +    if (type == QCOW2_CLUSTER_UNALLOCATED && data_file_is_raw(bs)) {
>>>>> +        type = QCOW2_CLUSTER_ZERO_PLAIN;
>>>>> +    }
>>>>> +
>>>>>      return type;
>>>>>
>>>>> You could even add a '&& bs->backing' to the condition and emit a
>>>>> warning to make it more explicit.
>>>>
>>>> No, this is wrong.  This still wouldn’t fix the problem of having a
>>>> device file as the external data file, when it already has non-zero
>>>> data during creation.  (Reading the qcow2 file would return zeroes,
>>>> but reading the device would not.)
>>>
>>> But you wouldn't fix that preallocating the metadata either, you would
>>> need to fill the device with zeroes.
>>
>> What it fixes is that reading the qcow2 image and the raw device returns
>> the same data.
>>
>> Initially, I also thought that we should initialize raw data files to be
>> zero during creation, but Eric changed my mind:
>>
>> https://lists.nongnu.org/archive/html/qemu-block/2020-04/msg00223.html
>>
>>>> I interpret the spec in that the metadata can be ignored, but it does
>>>> not need to be ignored.  So the L1/L2 tables must be 1:1 mapping of
>>>> QCOW2_CLUSTER_NORMAL entries.
>>>>
>>>> We could also choose to interpret it as “With data-file-raw, the L1/L2
>>>> tables must be ignored”.  In that case, our qcow2 driver would need to
>>>> be modified to indeed fully ignore the L1/L2 tables with
>>>> data-file-raw.  (I certainly don’t interpret the spec this way, but I
>>>> suppose we could call it a bug fix and amend it.)
>>>
>>> The way I interpret it is that regardless of whether you read the data
>>> through the qcow2 file or directly from the data file you should get the
>>> same results, but how that should be reflected in the L1/L2 metadata is
>>> not specified.
>>
>> That’s an absolute given, but the question is what does “reading through
>> the qcow2 file” mean.  Respecting the metadata?  Ignoring it?  Something
>> in between?
>>
>> As I noted in my reply to myself, data-file-raw is an autoclear flag.
>> That means, an old version of qemu that doesn’t recognize the flag must
>> read the same data as a new version.  It follows that the the L2 tables
>> must be a 1:1 mapping.  (Or the flag can’t be an autoclear flag.)
> 
> Being able to read sounds like a nice to have feature, but what about writing?
> 
> I hope that the image is not writable by older versions that do not understand
> data_file. Otherwise older qemu versions can corrupt the image silently.

It’s an autoclear flag.  That means such versions of qemu will
automatically clear the flag.

(To elaborate, there are three kinds of flags for qcow2 images:
Incompatible flags, compatible flags, and autoclear flags.

When qemu encounters an image with an unknown incompatible flag, it
refuses to open the image.

When it encounters an unknown compatible flag, it just ignores that flag
(but keeps it set).

When it encounters an unknown autoclear flag, it will clear that flag
and then continue as if it hadn’t been present.

So autoclear flags are useful for features that are optional, but that
may be broken when the image is written to by versions of qemu that
don’t understand them.)

Max


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

Re: [PATCH 0/2] qcow2: Force preallocation with data-file-raw

[Nir Soffer]

On Mon, Jun 22, 2020 at 12:47 PM Max Reitz <mreitz@redhat.com> wrote:
>
> On 22.06.20 00:25, Nir Soffer wrote:
> > On Fri, Jun 19, 2020 at 1:40 PM Max Reitz <mreitz@redhat.com> wrote:
> >>
> >> Hi,
> >>
> >> As discussed here:
> >>
> >> https://lists.nongnu.org/archive/html/qemu-block/2020-02/msg00644.html
> >> https://lists.nongnu.org/archive/html/qemu-block/2020-04/msg00329.html
> >> https://lists.nongnu.org/archive/html/qemu-block/2020-06/msg00240.html
> >>
> >> I think that qcow2 images with data-file-raw should always have
> >> preallocated 1:1 L1/L2 tables, so that the image always looks the same
> >> whether you respect or ignore the qcow2 metadata.
> >
> > I don't know the internals of qcow2 data_file, but are we really using
> > qcow2 metadata when accessing the data file?
>
> Yes.
>
> > This may have unwanted performance consequences.
>
> I don’t think so, because in practice normal lookups of L1/L2 mappings
> generally don’t cost that much performance.
>
> > If I understand correctly, qcow2 metadata is needed only for keeping
> > bitmaps (or maybe
> > future extensions) for raw data file, and reading from the qcow2 image
> > should be read
> > directly from the raw file without any extra work.
> >
> > Writing to the data file should also bypass the qcow2 metadata, since the bitmap
> > is updated in memory.
>
> Well, with this series, writing would no longer update the metadata at
> least, because it would always be preallocated already.
>
> >>  The easiest way to
> >> achieve that is to enforce at least metadata preallocation whenever
> >> data-file-raw is given.
> >
> > But preallocation is not free, even on file systems, it can be even
> > slow (NFS < 4.2).
>
> Metadata preallocation with an external data file should be the same
> speed on every file system.  We only need to create the metadata
> structures, which, with the default cluster size (64k) take up a bit
> more than 1/8192 of the full image size.
>
> Sure, it’s not free.  But if we decide we should indeed fully ignore the
> L1/L2 tables for data-file-raw images, the qcow2 spec must be amended.
> As I can read it, it currently doesn’t say so.
>
> (By the way, this is not a trivial change.  Right now, data-file-raw is
> an autoclear flag: If a version of qemu that doesn’t support it accesses
> the image, it will automatically clear the flag, but the image stays
> valid.  If we decide to completely ignore the L1/L2 tables (i.e. not
> even create them), then this can no longer be an autoclear flag.  We’d
> need a new incompatible flag.  (Because without L1/L2 tables, the image
> becomes useless to older qemu versions.))
>
> > With block storage this means you need to allocate the entire image size on
> > storage for writing the metadata.
> >
> > While oVirt does not use qcow2 with data_file, having preallocated qcow2
> > will make this very hard to use, for example for 500 GiB disk we will have to
> > allocate 500 GiB disk for the raw data file and 500 GiB disk for the qcow2
> > metadata disk which will be 99% unused.
>
> I don’t understand this.  When you use an external data file, the qcow2
> file will only contain the metadata:
>
> $ qemu-img create -f qcow2 \
>     -o data_file=foo.data,data_file_raw=on,preallocation=metadata \
>     foo.qcow2 8G
> Formatting 'foo.qcow2', fmt=qcow2 size=8589934592 data_file=foo.data
> data_file_raw=on cluster_size=65536 preallocation=metadata
> lazy_refcounts=off refcount_bits=16
> $ ls -l foo.qcow2
> ... 1310720 ... foo.qcow2
> $ ls -l foo.data
> ... 8589934592 ... foo.data

When allocating metadata in regular qcow2, need the to allocate the
entire device
(+ extra space for metadata overhead):

# qemu-img create -f qcow2 -o preallocation=metadata foo.qcow2 500g
Formatting 'foo.qcow2', fmt=qcow2 size=536870912000 cluster_size=65536
preallocation=metadata lazy_refcounts=off refcount_bits=16

# qemu-img check foo.qcow2
No errors were found on the image.
8192000/8192000 = 100.00% allocated, 0.00% fragmented, 0.00% compressed clusters
Image end offset: 536953094144

But I see that with metadata file we allocate much less:

# qemu-img create -f qcow2 -o
data_file=foo.data,data_file_raw=on,preallocation=metadata foo.qcow2
500g
Formatting 'foo.qcow2', fmt=qcow2 size=536870912000 data_file=foo.data
data_file_raw=on cluster_size=65536 preallocation=metadata
lazy_refcounts=off refcount_bits=16

# qemu-img check foo.qcow2
No errors were found on the image.
8192000/8192000 = 100.00% allocated, 0.00% fragmented, 0.00% compressed clusters
Image end offset: 65798144

I tested this also with block device:

# lvcreate --size 500g --name foo.data test
  Logical volume "foo.data" created.

 lvcreate --size 128m --name foo.qcow2 test
  Logical volume "foo.qcow2" created.

# time qemu-img create -f qcow2 -o
data_file=/dev/test/foo.data,data_file_raw=on,preallocation=metadata
/dev/test/foo.qcow2 500g
Formatting '/dev/test/foo.qcow2', fmt=qcow2 size=536870912000
data_file=/dev/test/foo.data data_file_raw=on cluster_size=65536
preallocation=metadata lazy_refcounts=off refcount_bits=16

real 0m4.263s
user 0m0.149s
sys 0m0.387s

# qemu-img info /dev/test/foo.qcow2
image: /dev/test/foo.qcow2
file format: qcow2
virtual size: 500 GiB (536870912000 bytes)
disk size: 0 B
cluster_size: 65536
Format specific information:
    compat: 1.1
    lazy refcounts: false
    refcount bits: 16
    data file: /dev/test/foo.data
    data file raw: true
    corrupt: false

# qemu-img check /dev/test/foo.qcow2
No errors were found on the image.
8192000/8192000 = 100.00% allocated, 0.00% fragmented, 0.00% compressed clusters
Image end offset: 65798144


The overhead 63 MiB per 500 GiB seems reasonable and preallocating the metadata
is not that bad.

> > I don't think that kubevirt is planning to use this either, but if
> > they decide to use
> > this it may be a problem for them as well when using block storage.
> >
> > It looks like we abuse preallocation for getting the side effect that
> > the backing file
> > will be rejected, instead of adding the validation rejecting backing
> > file in this case.
>
> That isn’t the case.
>
> I want to use preallocation because I interpret the spec such that it
> requires metadata preallocation.  It says when accessing a qcow2 file
> with data-file-raw, you can ignore the L1/L2 tables.  To me, that means
> that the L1/L2 tables must give a 1:1 mapping so that you get the same
> result whether you interpret them or not.

I agree that this is reasonable, and we will be able to use this if we need.

Not having to allocate metadata at all and never using the 1:1 mapping
would be even better.

Nir

Re: [PATCH 1/2] qcow2: Force preallocation with data-file-raw

[Alberto Garcia]

On Mon 22 Jun 2020 05:06:53 PM CEST, Max Reitz wrote:
>>> No, this is wrong.  This still wouldn’t fix the problem of having a
>>> device file as the external data file, when it already has non-zero
>>> data during creation.  (Reading the qcow2 file would return zeroes,
>>> but reading the device would not.)
>> 
>> But you wouldn't fix that preallocating the metadata either, you
>> would need to fill the device with zeroes.
>
> What it fixes is that reading the qcow2 image and the raw device
> returns the same data.
>
> Initially, I also thought that we should initialize raw data files to
> be zero during creation, but Eric changed my mind:
>
> https://lists.nongnu.org/archive/html/qemu-block/2020-04/msg00223.html

Then in that case you would indeed need to preallocate all the metadata,
both on creation and on resize.

Then if you read the image with an older version of QEMU you would get
the exact same contents in both cases.

You would still have problems with images created with raw data files
but without preallocation.

Coincidentally, data_file and data_file_raw were both introduced in QEMU
4.0 so in practice there's no official QEMU release that can read an
image with an external data file but does not support data_file_raw.

But in theory it could happen of course.

Berto

Re: [PATCH 0/2] qcow2: Force preallocation with data-file-raw

[Alberto Garcia]

On Mon 22 Jun 2020 11:47:32 AM CEST, Max Reitz wrote:
>> I don't know the internals of qcow2 data_file, but are we really using
>> qcow2 metadata when accessing the data file?
>
> Yes.
>
>> This may have unwanted performance consequences.
>
> I don’t think so, because in practice normal lookups of L1/L2 mappings
> generally don’t cost that much performance.

...if the L2 cache size is large enough. Otherwise you need one extra
read operation to retrieve the L2 metadata.

Possible performance problems when you have preallocation:

   - If a block hasn't been written yet (it's all zeroes) then you still
     need to read the L2 entry and read the data block. If there is not
     L2 table then you can simply return zeroes without going to disk at
     all. This of course assumes that the contents of the unwritten data
     block are zeroes.

   - QEMU still needs to read from disk (and cache in memory) the L2
     metadata, when it already knows in advance the contents of the L2
     entry (guest_offset == host_offset).

But as you say:

> Sure, it’s not free.  But if we decide we should indeed fully ignore
> the L1/L2 tables for data-file-raw images, the qcow2 spec must be
> amended.  As I can read it, it currently doesn’t say so.
>
> (By the way, this is not a trivial change.  Right now, data-file-raw
> is an autoclear flag: If a version of qemu that doesn’t support it
> accesses the image, it will automatically clear the flag, but the
> image stays valid.  If we decide to completely ignore the L1/L2 tables
> (i.e. not even create them), then this can no longer be an autoclear
> flag.  We’d need a new incompatible flag.  (Because without L1/L2
> tables, the image becomes useless to older qemu versions.))

Berto

Re: [PATCH 1/2] qcow2: Force preallocation with data-file-raw

[Eric Blake]

On 6/22/20 10:48 AM, Max Reitz wrote:

>>> As I noted in my reply to myself, data-file-raw is an autoclear flag.
>>> That means, an old version of qemu that doesn’t recognize the flag must
>>> read the same data as a new version.  It follows that the the L2 tables
>>> must be a 1:1 mapping.  (Or the flag can’t be an autoclear flag.)

Yes, that argument is the strongest I've seen for why both creation and 
resize with a data-file-raw image should require metadata preallocation. 
  In other words, we never want to expose different guest data merely 
because we opened the file with an older version (the older version must 
either see the same data [an autoclear bit is fine], or must know that 
it cannot reliably open the image [an incompatible bit is needed]).

>>
>> Being able to read sounds like a nice to have feature, but what about writing?
>>
>> I hope that the image is not writable by older versions that do not understand
>> data_file. Otherwise older qemu versions can corrupt the image silently.
> 
> It’s an autoclear flag.  That means such versions of qemu will
> automatically clear the flag.

Well, an older version is only required to clear the flag if it modifies 
the image; if it opens the image read-only, it may leave the autoclear 
flag set.  So you are more realistically guaranteed that the autoclear 
flag is only cleared when writing to the image with a version that did 
not understand the autoclear flag.

Following that line of thought further - reopening the image under a 
qemu that once again understands the data-file-raw flag will now see 
that the file is no longer raw because the flag was cleared.  That is, 
if you are expecting data-file-raw and no longer see the flag, then you 
KNOW that the qcow2 file was modified by an older program that didn't 
necessarily preserve the 1:1 correspondence, and it is up to you what to 
do next (refuse to use the file, do a pass over the qcow2 file to flush 
contents back to the raw file, or any number of other reactions...).

> So autoclear flags are useful for features that are optional, but that
> may be broken when the image is written to by versions of qemu that
> don’t understand them.)

More importantly, autoclear flags are useful for features where older 
software can safely read the image without data loss, but where older 
software modifying the image may lose whatever property the bit was 
guaranteeing when interpreted correctly.

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

Re: [PATCH 0/2] qcow2: Force preallocation with data-file-raw

[Max Reitz]

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

On 22.06.20 19:44, Alberto Garcia wrote:
> On Mon 22 Jun 2020 11:47:32 AM CEST, Max Reitz wrote:
>>> I don't know the internals of qcow2 data_file, but are we really using
>>> qcow2 metadata when accessing the data file?
>>
>> Yes.
>>
>>> This may have unwanted performance consequences.
>>
>> I don’t think so, because in practice normal lookups of L1/L2 mappings
>> generally don’t cost that much performance.
> 
> ...if the L2 cache size is large enough. Otherwise you need one extra
> read operation to retrieve the L2 metadata.
> 
> Possible performance problems when you have preallocation:
> 
>    - If a block hasn't been written yet (it's all zeroes) then you still
>      need to read the L2 entry and read the data block. If there is not
>      L2 table then you can simply return zeroes without going to disk at
>      all. This of course assumes that the contents of the unwritten data
>      block are zeroes.
> 
>    - QEMU still needs to read from disk (and cache in memory) the L2
>      metadata, when it already knows in advance the contents of the L2
>      entry (guest_offset == host_offset).

We could well optimize this regardless of preallocation.  With
data-file-raw, qemu doesn’t have to look at the L2 metadata at all.

So the problem isn’t preallocation at all, it’s the fact that we don’t
have such an optimization.  But note that to implement such an
optimization, we really do need preallocation: Because it would mean
that we wouldn’t touch the L1/L2 tables for data-file-raw images during
runtime, which would effectively make those images empty to today’s qemu
versions.

(OTOH, preallocation would then be pretty much superfluous for all newer
versions of qemu.  To address that, we could then add an incompatible
version of data-file-raw.  But I think we should only think about that
once we get to that point.)

You make a good point that data-file-raw was introduced alongside
data-file.  But, well.  I personally can’t get myself to treating an
autoclear flag like an incompatible one...

Max


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

Re: [PATCH 1/2] qcow2: Force preallocation with data-file-raw

[Max Reitz]

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

On 22.06.20 19:36, Alberto Garcia wrote:

[...]

> You would still have problems with images created with raw data files
> but without preallocation.

Yeah, sure, but, well.  Bug in qemu. :/

Max


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

Re: [PATCH 0/2] qcow2: Force preallocation with data-file-raw

[Kevin Wolf]

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

Am 23.06.2020 um 09:28 hat Max Reitz geschrieben:
> On 22.06.20 19:44, Alberto Garcia wrote:
> > On Mon 22 Jun 2020 11:47:32 AM CEST, Max Reitz wrote:
> >>> I don't know the internals of qcow2 data_file, but are we really using
> >>> qcow2 metadata when accessing the data file?
> >>
> >> Yes.
> >>
> >>> This may have unwanted performance consequences.
> >>
> >> I don’t think so, because in practice normal lookups of L1/L2 mappings
> >> generally don’t cost that much performance.
> > 
> > ...if the L2 cache size is large enough. Otherwise you need one extra
> > read operation to retrieve the L2 metadata.
> > 
> > Possible performance problems when you have preallocation:
> > 
> >    - If a block hasn't been written yet (it's all zeroes) then you still
> >      need to read the L2 entry and read the data block. If there is not
> >      L2 table then you can simply return zeroes without going to disk at
> >      all. This of course assumes that the contents of the unwritten data
> >      block are zeroes.
> > 
> >    - QEMU still needs to read from disk (and cache in memory) the L2
> >      metadata, when it already knows in advance the contents of the L2
> >      entry (guest_offset == host_offset).
> 
> We could well optimize this regardless of preallocation.  With
> data-file-raw, qemu doesn’t have to look at the L2 metadata at all.
> 
> So the problem isn’t preallocation at all, it’s the fact that we don’t
> have such an optimization.  But note that to implement such an
> optimization, we really do need preallocation: Because it would mean
> that we wouldn’t touch the L1/L2 tables for data-file-raw images during
> runtime, which would effectively make those images empty to today’s qemu
> versions.

It depends. For reads, bypassing the L1/L2 tables is completely fine
with data-file-raw. It may miss opportunities to optimise reading
unallocated/zeroed clusters, but if the data file is actually sparse, it
shouldn't make a big difference. Maybe we should just do this.

For (potentially allocating) writes, you're right that we need to be
more careful. If we want to completely bypass L1/L2 tables,
preallocation is not enough, but we have to make sure that we never
discard any clusters.

Whatever we do for writes will be a non-trivial change. I wonder if it's
really worth doing this for optimisation when nobody uses the feature
yet anyway.

> (OTOH, preallocation would then be pretty much superfluous for all newer
> versions of qemu.  To address that, we could then add an incompatible
> version of data-file-raw.  But I think we should only think about that
> once we get to that point.)

Well, if we create an incompatible version, we can have one that doesn't
even store L1/L2 tables.

Kevin

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

Re: [PATCH 0/2] qcow2: Force preallocation with data-file-raw

[Kevin Wolf]

Am 22.06.2020 um 17:50 hat Nir Soffer geschrieben:
> On Mon, Jun 22, 2020 at 12:47 PM Max Reitz <mreitz@redhat.com> wrote:
> >
> > On 22.06.20 00:25, Nir Soffer wrote:
> > > On Fri, Jun 19, 2020 at 1:40 PM Max Reitz <mreitz@redhat.com> wrote:
> > >>
> > >> Hi,
> > >>
> > >> As discussed here:
> > >>
> > >> https://lists.nongnu.org/archive/html/qemu-block/2020-02/msg00644.html
> > >> https://lists.nongnu.org/archive/html/qemu-block/2020-04/msg00329.html
> > >> https://lists.nongnu.org/archive/html/qemu-block/2020-06/msg00240.html
> > >>
> > >> I think that qcow2 images with data-file-raw should always have
> > >> preallocated 1:1 L1/L2 tables, so that the image always looks the same
> > >> whether you respect or ignore the qcow2 metadata.
> > >
> > > I don't know the internals of qcow2 data_file, but are we really using
> > > qcow2 metadata when accessing the data file?
> >
> > Yes.
> >
> > > This may have unwanted performance consequences.
> >
> > I don’t think so, because in practice normal lookups of L1/L2 mappings
> > generally don’t cost that much performance.
> >
> > > If I understand correctly, qcow2 metadata is needed only for keeping
> > > bitmaps (or maybe
> > > future extensions) for raw data file, and reading from the qcow2 image
> > > should be read
> > > directly from the raw file without any extra work.
> > >
> > > Writing to the data file should also bypass the qcow2 metadata, since the bitmap
> > > is updated in memory.
> >
> > Well, with this series, writing would no longer update the metadata at
> > least, because it would always be preallocated already.
> >
> > >>  The easiest way to
> > >> achieve that is to enforce at least metadata preallocation whenever
> > >> data-file-raw is given.
> > >
> > > But preallocation is not free, even on file systems, it can be even
> > > slow (NFS < 4.2).
> >
> > Metadata preallocation with an external data file should be the same
> > speed on every file system.  We only need to create the metadata
> > structures, which, with the default cluster size (64k) take up a bit
> > more than 1/8192 of the full image size.
> >
> > Sure, it’s not free.  But if we decide we should indeed fully ignore the
> > L1/L2 tables for data-file-raw images, the qcow2 spec must be amended.
> > As I can read it, it currently doesn’t say so.
> >
> > (By the way, this is not a trivial change.  Right now, data-file-raw is
> > an autoclear flag: If a version of qemu that doesn’t support it accesses
> > the image, it will automatically clear the flag, but the image stays
> > valid.  If we decide to completely ignore the L1/L2 tables (i.e. not
> > even create them), then this can no longer be an autoclear flag.  We’d
> > need a new incompatible flag.  (Because without L1/L2 tables, the image
> > becomes useless to older qemu versions.))
> >
> > > With block storage this means you need to allocate the entire image size on
> > > storage for writing the metadata.
> > >
> > > While oVirt does not use qcow2 with data_file, having preallocated qcow2
> > > will make this very hard to use, for example for 500 GiB disk we will have to
> > > allocate 500 GiB disk for the raw data file and 500 GiB disk for the qcow2
> > > metadata disk which will be 99% unused.
> >
> > I don’t understand this.  When you use an external data file, the qcow2
> > file will only contain the metadata:
> >
> > $ qemu-img create -f qcow2 \
> >     -o data_file=foo.data,data_file_raw=on,preallocation=metadata \
> >     foo.qcow2 8G
> > Formatting 'foo.qcow2', fmt=qcow2 size=8589934592 data_file=foo.data
> > data_file_raw=on cluster_size=65536 preallocation=metadata
> > lazy_refcounts=off refcount_bits=16
> > $ ls -l foo.qcow2
> > ... 1310720 ... foo.qcow2
> > $ ls -l foo.data
> > ... 8589934592 ... foo.data
> 
> When allocating metadata in regular qcow2, need the to allocate the
> entire device
> (+ extra space for metadata overhead):
> 
> # qemu-img create -f qcow2 -o preallocation=metadata foo.qcow2 500g
> Formatting 'foo.qcow2', fmt=qcow2 size=536870912000 cluster_size=65536
> preallocation=metadata lazy_refcounts=off refcount_bits=16
> 
> # qemu-img check foo.qcow2
> No errors were found on the image.
> 8192000/8192000 = 100.00% allocated, 0.00% fragmented, 0.00% compressed clusters
> Image end offset: 536953094144

I think we shouldn't really call this "allocating" because we don't
actually reserve space for it yet. On a filesystem, you get a large file
size, but it's almost completely sparse. On block devices, it depends on
whether the storage has thin provisioning.

> But I see that with metadata file we allocate much less:
> 
> # qemu-img create -f qcow2 -o
> data_file=foo.data,data_file_raw=on,preallocation=metadata foo.qcow2
> 500g
> Formatting 'foo.qcow2', fmt=qcow2 size=536870912000 data_file=foo.data
> data_file_raw=on cluster_size=65536 preallocation=metadata
> lazy_refcounts=off refcount_bits=16
> 
> # qemu-img check foo.qcow2
> No errors were found on the image.
> 8192000/8192000 = 100.00% allocated, 0.00% fragmented, 0.00% compressed clusters
> Image end offset: 65798144

Actually, this is not much less, but just split in two places. You still
have the 500 GB data file. The metadata is small, but it was already
small before:

536953094144 - 536870912000 = ~78 MB.

Not exactly sure why it's more than the 64 MB you get for an external
data file, maybe some alignment thing, but not significant anyway.

Kevin

Re: [PATCH 1/2] qcow2: Force preallocation with data-file-raw

[Kevin Wolf]

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

Am 22.06.2020 um 11:48 hat Max Reitz geschrieben:
> On 22.06.20 11:35, Max Reitz wrote:
> > On 19.06.20 18:47, Alberto Garcia wrote:
> >> On Fri 19 Jun 2020 12:40:11 PM CEST, Max Reitz wrote:
> >>> +    if (qcow2_opts->data_file_raw &&
> >>> +        qcow2_opts->preallocation == PREALLOC_MODE_OFF)
> >>> +    {
> >>> +        /*
> >>> +         * data-file-raw means that "the external data file can be
> >>> +         * read as a consistent standalone raw image without looking
> >>> +         * at the qcow2 metadata."  It does not say that the metadata
> >>> +         * must be ignored, though (and the qcow2 driver in fact does
> >>> +         * not ignore it), so the L1/L2 tables must be present and
> >>> +         * give a 1:1 mapping, so you get the same result regardless
> >>> +         * of whether you look at the metadata or whether you ignore
> >>> +         * it.
> >>> +         */
> >>> +        qcow2_opts->preallocation = PREALLOC_MODE_METADATA;
> >>
> >> I'm not convinced by this,
> > 
> > Why not?
> > 
> > This is how I read the spec.  Furthermore, I see two problems that we
> > have right now that are fixed by this patch (namely (1) using a device
> > file as the external data file, which may have non-zero data at
> > creation; and (2) assigning a backing file at runtime must not show the
> > data).
> > 
> >> but your comment made me think of another
> >> possible alternative: in qcow2_get_cluster_offset(), if the cluster is
> >> unallocated and we are using a raw data file then we return _ZERO_PLAIN:
> >>
> >> --- a/block/qcow2-cluster.c
> >> +++ b/block/qcow2-cluster.c
> >> @@ -654,6 +654,10 @@ out:
> >>      assert(bytes_available - offset_in_cluster <= UINT_MAX);
> >>      *bytes = bytes_available - offset_in_cluster;
> >>  
> >> +    if (type == QCOW2_CLUSTER_UNALLOCATED && data_file_is_raw(bs)) {
> >> +        type = QCOW2_CLUSTER_ZERO_PLAIN;
> >> +    }
> >> +
> >>      return type;
> >>
> >> You could even add a '&& bs->backing' to the condition and emit a
> >> warning to make it more explicit.
> > 
> > No, this is wrong.  This still wouldn’t fix the problem of having a
> > device file as the external data file, when it already has non-zero data
> > during creation.  (Reading the qcow2 file would return zeroes, but
> > reading the device would not.)
> > 
> > So it would need to be QCOW2_CLUSTER_NORMAL.  Which is kind of the
> > point, when you think about it – with data-file-raw, all clusters must
> > always effectively be QCOW2_CLUSTER_NORMAL and be mapped 1:1.
> > 
> > Well, and that’s in turn the point of this patch.
> > 
> > I interpret the spec in that the metadata can be ignored, but it does
> > not need to be ignored.  So the L1/L2 tables must be 1:1 mapping of
> > QCOW2_CLUSTER_NORMAL entries.
> > 
> > We could also choose to interpret it as “With data-file-raw, the L1/L2
> > tables must be ignored”.  In that case, our qcow2 driver would need to
> > be modified to indeed fully ignore the L1/L2 tables with data-file-raw.
> >  (I certainly don’t interpret the spec this way, but I suppose we could
> > call it a bug fix and amend it.)
> 
> I just realized that this is not possible.  data-file-raw is an
> autoclear flag, so the image must appear the same to qemu versions that
> do not support it.
> 
> If we want to fully ignore the L1/L2 tables or interpret them some
> non-default way (like you’re proposing), we would have to add a new
> incompatible flag.

We could drop the data-file-raw autoclear flag (causing new QEMU
versions to downgrade any existing images) and call the new incompatible
flag data-file-raw (leaving the user interface unchanged).

Kevin

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