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

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

[Max Reitz]

v1: https://lists.nongnu.org/archive/html/qemu-block/2020-06/msg00992.html


Hi,

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.

As far as I could tell, there were two main critique points about v1:
(1) If we force metadata preallocation on creation, we should also do it
    when the image is grown.
(2) We could go even further and make qemu ignore all L1/L2 tables for
    images with raw external data files.  Ideally, we wouldn’t even
    write them at all.

(1) is addressed in this v2.

As for (2)...  It’s complicated.  I think we want the fix from this
series now and if we want (2), we can have a go at it later.  Many
things are to be considered there.

For example: data-file-raw is an autoclear flag.  Technically, it is
possible for some qcow2 implementation to support data-file, but not
data-file-raw.  If we ignore metadata for images with data-file-raw, we
would break them, because “ignoring” would mean we don’t even create it,
ever, so the external data file would appear empty to such
implementations.
Now, in practice, there is no such implementation.  data-file-raw has
been introduced alongside data-file.
However, also in practice, qemu always did and still does rely on the
metadata in the qcow2 image.  So we have to ensure the metadata is
there, or all versions of qemu that support data-file will break.

The easiest way to ensure the metadata is there is to preallocate it on
creation/growth.  If at same later point we decide we want to ignore it
on runtime, this preallocation would actually allow us to do that.  So
the preallocation is the necessary first step (the second step would
probably be a second auto-clear flag that states that all metadata has
been preallocated and can thus be ignored at runtime).

((Even today, we could ignore the L2 tables when reading, but the
problems are that (1) images can then appear differently to qemu
versions that do ignore them and versions that don’t, and (2) when
writing to a cluster, we still need to ensure that its L2 entry is there
(i.e., allocated and pointing to the correct offset).  I don’t think it
makes sense to ignore the tables when reading but not when writing.))


There have also been proposals of instead just not writing any metadata.
This would naturally require an incompatible new flag, because such
images would not be usable by current qemu versions.  Such a flag would
make this series unnecessary, but do we really want to break
incompatibility with all qemu versions going back to 4.0 just so we
don’t have to waste space on L2 tables?  Users are free to just use 2M
clusters for data-file-raw images so the wasted space is minimized (to
1/2M of the image size, e.g. 512M per 1T).

And in any case: I think patch 1 is simple enough that we can just take
it now and it wouldn’t be too bad to write it off as a loss if we ever
add an incompatible no-l2 flag.

Point is, we have no actual patches to implement a no-l2 flag, but there
is something that needs to be fixed about raw external data files, and
this series fixes it.


v2:
- Patch 1: Force metadata preallocation when the image is resized
- Patch 2:
  - Use blockdev-create to create the qcow2 image instead of creating
    the qcow2 image first and then (technically illegally) writing to
    the external data file
  - Test growing a qcow2 image with an external data file, where the
    data file is grown first and the new area is filled with data


git-backport-diff against v1:

Key:
[----] : patches are identical
[####] : number of functional differences between upstream/downstream patch
[down] : patch is downstream-only
The flags [FC] indicate (F)unctional and (C)ontextual differences, respectively

001/2:[0012] [FC] 'qcow2: Force preallocation with data-file-raw'
002/2:[0110] [FC] 'iotests/244: Test preallocation for data-file-raw'


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

 block/qcow2.c              |  34 ++++++++++++
 tests/qemu-iotests/244     | 104 +++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/244.out |  68 ++++++++++++++++++++++--
 3 files changed, 201 insertions(+), 5 deletions(-)

-- 
2.29.2

[PATCH v2 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              | 34 ++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/244.out |  9 ++++-----
 2 files changed, 38 insertions(+), 5 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index 0db1227ac9..9920c756eb 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -3502,6 +3502,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) {
@@ -4237,6 +4259,18 @@ static int coroutine_fn qcow2_co_truncate(BlockDriverState *bs, int64_t offset,
             error_setg_errno(errp, -ret, "Failed to grow the L1 table");
             goto fail;
         }
+
+        if (data_file_is_raw(bs) && prealloc == PREALLOC_MODE_OFF) {
+            /*
+             * When creating a qcow2 image with data-file-raw, we enforce
+             * at least prealloc=metadata, so that the L1/L2 tables are
+             * fully allocated and reading from the data file will return
+             * the same data as reading from the qcow2 image.  When the
+             * image is grown, we must consequently preallocate the
+             * metadata structures to cover the added area.
+             */
+            prealloc = PREALLOC_MODE_METADATA;
+        }
     }
 
     switch (prealloc) {
diff --git a/tests/qemu-iotests/244.out b/tests/qemu-iotests/244.out
index 7269b4295a..1a3ae31dde 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.29.2

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

[Max Reitz]

Three test cases:
(1) Adding a qcow2 (metadata) file to an existing data file, see whether
    we can read the existing data through the qcow2 image.
(2) Append data to the data file, grow the qcow2 image accordingly, see
    whether we can read the new data through the qcow2 image.
(3) At runtime, add a backing image to a freshly created qcow2 image
    with an external data file (with data-file-raw).  Reading data from
    the qcow2 image must return the same result as reading data from the
    data file, so everything in the backing image must be ignored.
    (This did not use to be the case, because without the L2 tables
    preallocated, all clusters would appear as unallocated, and so the
    qcow2 driver would fall through to the backing file.)

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

diff --git a/tests/qemu-iotests/244 b/tests/qemu-iotests/244
index a46b441627..3e61fa25bb 100755
--- a/tests/qemu-iotests/244
+++ b/tests/qemu-iotests/244
@@ -38,6 +38,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
 # get standard environment, filters and checks
 . ./common.rc
 . ./common.filter
+. ./common.qemu
 
 _supported_fmt qcow2
 _supported_proto file
@@ -267,6 +268,109 @@ case $result in
         ;;
 esac
 
+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
+
+# Pre-create the data file, write some data.  Real-world use cases for
+# this are adding a qcow2 metadata file to a block device (i.e., using
+# the device as the data file) or adding qcow2 features to pre-existing
+# raw images (e.g. because the user now wants persistent dirty bitmaps).
+truncate -s 1M "$TEST_IMG.data"
+$QEMU_IO -f raw -c 'write -P 42 0 1M' "$TEST_IMG.data" | _filter_qemu_io
+
+# We cannot use qemu-img to create the qcow2 image, because it would
+# clear the data file.  Use the blockdev-create job instead, which will
+# only format the qcow2 image file.
+touch "$TEST_IMG"
+_launch_qemu \
+    -blockdev file,node-name=data,filename="$TEST_IMG.data" \
+    -blockdev file,node-name=meta,filename="$TEST_IMG"
+
+_send_qemu_cmd $QEMU_HANDLE '{ "execute": "qmp_capabilities" }' 'return'
+
+_send_qemu_cmd $QEMU_HANDLE \
+    '{ "execute": "blockdev-create",
+       "arguments": {
+           "job-id": "create",
+           "options": {
+               "driver": "qcow2",
+               "size": '"$((1 * 1024 * 1024))"',
+               "file": "meta",
+               "data-file": "data",
+               "data-file-raw": true
+           } } }' \
+    '"status": "concluded"'
+
+_send_qemu_cmd $QEMU_HANDLE \
+    '{ "execute": "job-dismiss", "arguments": { "id": "create" } }' \
+    'return'
+
+_cleanup_qemu
+
+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 '--- Truncation (growing) ---'
+
+# Append some new data to the raw file, then resize the qcow2 image
+# accordingly and see whether the new data is visible.  Technically
+# that is not allowed, but it is reasonable behavior, so test it.
+truncate -s 2M "$TEST_IMG.data"
+$QEMU_IO -f raw -c 'write -P 84 1M 1M' "$TEST_IMG.data" | _filter_qemu_io
+
+$QEMU_IMG resize "$TEST_IMG" 2M
+
+echo
+echo 'Comparing pattern:'
+
+$QEMU_IO -f raw -c 'read -P 42 0 1M' -c 'read -P 84 1M 1M' "$TEST_IMG.data" \
+    | _filter_qemu_io
+$QEMU_IO -f $IMGFMT -c 'read -P 42 0 1M' -c 'read -P 84 1M 1M' "$TEST_IMG" \
+    | _filter_qemu_io
+
+$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 1a3ae31dde..99f56ac18c 100644
--- a/tests/qemu-iotests/244.out
+++ b/tests/qemu-iotests/244.out
@@ -137,4 +137,63 @@ wrote 512/512 bytes at offset 0
 512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 
 Success: qemu-io failed, so the data file was flushed
+
+=== Preallocation with data-file-raw ===
+
+--- Using a non-zeroed data file ---
+wrote 1048576/1048576 bytes at offset 0
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+{ "execute": "qmp_capabilities" }
+{"return": {}}
+{ "execute": "blockdev-create",
+       "arguments": {
+           "job-id": "create",
+           "options": {
+               "driver": "IMGFMT",
+               "size": 1048576,
+               "file": "meta",
+               "data-file": "data",
+               "data-file-raw": true
+           } } }
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "created", "id": "create"}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "running", "id": "create"}}
+{"return": {}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "waiting", "id": "create"}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "pending", "id": "create"}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "concluded", "id": "create"}}
+{ "execute": "job-dismiss", "arguments": { "id": "create" } }
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "null", "id": "create"}}
+{"return": {}}
+
+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.
+
+--- Truncation (growing) ---
+wrote 1048576/1048576 bytes at offset 1048576
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+Image resized.
+
+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 1048576
+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)
+read 1048576/1048576 bytes at offset 1048576
+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.29.2

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

[Eric Blake]

On 3/26/21 9:55 AM, Max Reitz wrote:
> 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              | 34 ++++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/244.out |  9 ++++-----
>  2 files changed, 38 insertions(+), 5 deletions(-)
> 

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

I think this counts as a bug fix worthy of inclusion in 6.0.

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

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

[Eric Blake]

On 3/26/21 9:55 AM, Max Reitz wrote:
> Three test cases:
> (1) Adding a qcow2 (metadata) file to an existing data file, see whether
>     we can read the existing data through the qcow2 image.
> (2) Append data to the data file, grow the qcow2 image accordingly, see
>     whether we can read the new data through the qcow2 image.
> (3) At runtime, add a backing image to a freshly created qcow2 image
>     with an external data file (with data-file-raw).  Reading data from
>     the qcow2 image must return the same result as reading data from the
>     data file, so everything in the backing image must be ignored.
>     (This did not use to be the case, because without the L2 tables
>     preallocated, all clusters would appear as unallocated, and so the
>     qcow2 driver would fall through to the backing file.)
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  tests/qemu-iotests/244     | 104 +++++++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/244.out |  59 +++++++++++++++++++++
>  2 files changed, 163 insertions(+)
> 

> +
> +# We cannot use qemu-img to create the qcow2 image, because it would
> +# clear the data file.  Use the blockdev-create job instead, which will
> +# only format the qcow2 image file.

Well, perhaps we could use qemu-img to create a qcow2 pointing to a
temporary file, then rewrite it to point to the real data file, but that
feels hackish, and your approach worked.  And besides, while we have
qemu-img rebase -u to rewrite the backing file, I don't know if we have
a similar qemu-img command for rewriting the data file.

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

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

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

[Max Reitz]

On 26.03.21 16:17, Eric Blake wrote:
> On 3/26/21 9:55 AM, Max Reitz wrote:
>> Three test cases:
>> (1) Adding a qcow2 (metadata) file to an existing data file, see whether
>>      we can read the existing data through the qcow2 image.
>> (2) Append data to the data file, grow the qcow2 image accordingly, see
>>      whether we can read the new data through the qcow2 image.
>> (3) At runtime, add a backing image to a freshly created qcow2 image
>>      with an external data file (with data-file-raw).  Reading data from
>>      the qcow2 image must return the same result as reading data from the
>>      data file, so everything in the backing image must be ignored.
>>      (This did not use to be the case, because without the L2 tables
>>      preallocated, all clusters would appear as unallocated, and so the
>>      qcow2 driver would fall through to the backing file.)
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>   tests/qemu-iotests/244     | 104 +++++++++++++++++++++++++++++++++++++
>>   tests/qemu-iotests/244.out |  59 +++++++++++++++++++++
>>   2 files changed, 163 insertions(+)
>>
> 
>> +
>> +# We cannot use qemu-img to create the qcow2 image, because it would
>> +# clear the data file.  Use the blockdev-create job instead, which will
>> +# only format the qcow2 image file.
> 
> Well, perhaps we could use qemu-img to create a qcow2 pointing to a
> temporary file, then rewrite it to point to the real data file, but that
> feels hackish, and your approach worked.  And besides, while we have
> qemu-img rebase -u to rewrite the backing file, I don't know if we have
> a similar qemu-img command for rewriting the data file.

Yes, we do, but I decided this would be the cleanest way.

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

Thanks!  I’ll decide on Monday whether to include this in 6.0.  I think 
it would be small enough for rc1, but it’s also no regression, so... 
Well, I’ll see then.

Max

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

[Max Reitz]

On 26.03.21 15:55, Max Reitz wrote:
> v1: https://lists.nongnu.org/archive/html/qemu-block/2020-06/msg00992.html
> 
> 
> Hi,
> 
> 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.

Thanks for the review, applied to my block branch:

https://git.xanclic.moe/XanClic/qemu/commits/branch/block

Max