iotests: Test abnormally large size in compressed cluster descriptor
PATCH v4 1/1 iotests: Test abnormally large size in compressed cluster descriptor
L2 entries for compressed clusters have a field that indicates the number of sectors used to store the data in the image.
That's however not the size of the compressed data itself, just the number of sectors where that data is located. The actual data size is usually not a multiple of the sector size, and therefore cannot be represented with this field.
The way it works is that QEMU reads all the specified sectors and starts decompressing the data until there's enough to recover the original uncompressed cluster. If there are any bytes left that haven't been decompressed they are simply ignored.
One consequence of this is that even if the size field is larger than it needs to be QEMU can handle it just fine: it will read more data from disk but it will ignore the extra bytes.
This test creates an image with two compressed clusters that use 5 sectors (2.5 KB) each, increases the size field to the maximum (8192 sectors, or 4 MB) and verifies that the data can be read without problems.
This test is important because while the decompressed data takes exactly one cluster, the maximum value allowed in the compressed size field is twice the cluster size. So although QEMU won't produce images with such large values we need to make sure that it can handle them.
Another effect of increasing the size field is that it can make it include data from the following host cluster(s). In this case 'qemu-img check' will detect that the refcounts are not correct, and we'll need to rebuild them.
Additionally, this patch also tests that decreasing the size corrupts the image since the original data can no longer be recovered. In this case QEMU returns an error when trying to read the compressed data, but 'qemu-img check' doesn't see anything wrong if the refcounts are consistent.
One possible task for the future is to make 'qemu-img check' verify the sizes of the compressed clusters, by trying to decompress the data and checking that the size stored in the L2 entry is correct.
---
v4: Resend for 2.12
v3: Add TODO comment, as suggested by Eric.
Corrupt the length of the second compressed cluster as well so the uncompressed data would span three host clusters.
v2: We now have two scenarios where we make QEMU read data from the
next host cluster and from beyond the end of the image. This version also runs qemu-img check on the corrupted image.
If the size field is too small, reading fails but qemu-img check succeeds.
If the size field is too large, reading succeeds but qemu-img check fails (this can be repaired, though).
--- tests/qemu-iotests/122 | 45 +++++++++++++++++++++++++++++++++++++++++++++ tests/qemu-iotests/122.out | 31 +++++++++++++++++++++++++++++++ 2 files changed, 76 insertions(+)
tests/qemu-iotests/122 +45 -0
@@ -130,6 +130,51 @@ $QEMU_IO -c "read -P 0 1024k 1022k" "$TEST_IMG" 2>&1 | _filter_qemu_io | _fil echo +echo "=== Corrupted size field in compressed cluster descriptor ===" +echo +# Create an empty image, fill half of it with data and compress it. +# The L2 entries of the two compressed clusters are located at +# 0x800000 and 0x800008, their original values are 0x4008000000a00000 +# and 0x4008000000a00802 (5 sectors for compressed data each). +TEST_IMG="$TEST_IMG".1 _make_test_img 8M +$QEMU_IO -c "write -P 0x11 0 4M" "$TEST_IMG".1 2>&1 | _filter_qemu_io | _filter_testdir +$QEMU_IMG convert -c -O qcow2 -o cluster_size=2M "$TEST_IMG".1 "$TEST_IMG" + +# Reduce size of compressed data to 4 sectors: this corrupts the image. +poke_file "$TEST_IMG" $((0x800000)) "\x40\x06" +$QEMU_IO -c "read -P 0x11 0 4M" "$TEST_IMG" 2>&1 | _filter_qemu_io | _filter_testdir + +# 'qemu-img check' however doesn't see anything wrong because it +# doesn't try to decompress the data and the refcounts are consistent. +# TODO: update qemu-img so this can be detected +_check_test_img + +# Increase size of compressed data to the maximum (8192 sectors). +# This makes QEMU read more data (8192 sectors instead of 5, host +# addresses [0xa00000, 0xdfffff]), but the decompression algorithm +# stops once we have enough to restore the uncompressed cluster, so +# the rest of the data is ignored. +poke_file "$TEST_IMG" $((0x800000)) "\x7f\xfe" +# Do it also for the second compressed cluster (L2 entry at 0x800008). +# In this case the compressed data would span 3 host clusters +# (host addresses: [0xa00802, 0xe00801]) +poke_file "$TEST_IMG" $((0x800008)) "\x7f\xfe" + +# Here the image is too small so we're asking QEMU to read beyond the +# end of the image. +$QEMU_IO -c "read -P 0x11 0 4M" "$TEST_IMG" 2>&1 | _filter_qemu_io | _filter_testdir +# But if we grow the image we won't be reading beyond its end anymore. +$QEMU_IO -c "write -P 0x22 4M 4M" "$TEST_IMG" 2>&1 | _filter_qemu_io | _filter_testdir +$QEMU_IO -c "read -P 0x11 0 4M" "$TEST_IMG" 2>&1 | _filter_qemu_io | _filter_testdir + +# The refcount data is however wrong because due to the increased size +# of the compressed data it now reaches the following host clusters. +# This can be repaired by qemu-img check. +_check_test_img -r all +$QEMU_IO -c "read -P 0x11 0 4M" "$TEST_IMG" 2>&1 | _filter_qemu_io | _filter_testdir +$QEMU_IO -c "read -P 0x22 4M 4M" "$TEST_IMG" 2>&1 | _filter_qemu_io | _filter_testdir + +echo echo "=== Full allocation with -S 0 ===" echo
tests/qemu-iotests/122.out +31 -0
@@ -99,6 +99,37 @@ read 1024/1024 bytes at offset 1047552 read 1046528/1046528 bytes at offset 1048576 1022 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +=== Corrupted size field in compressed cluster descriptor === + +Formatting 'TEST_DIR/t.IMGFMT.1', fmt=IMGFMT size=8388608 +wrote 4194304/4194304 bytes at offset 0 +4 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +read failed: Input/output error +No errors were found on the image. +read 4194304/4194304 bytes at offset 0 +4 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +wrote 4194304/4194304 bytes at offset 4194304 +4 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +read 4194304/4194304 bytes at offset 0 +4 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +ERROR cluster 6 refcount=1 reference=3 +ERROR cluster 7 refcount=1 reference=2 +Repairing cluster 6 refcount=1 reference=3 +Repairing cluster 7 refcount=1 reference=2 +Repairing OFLAG_COPIED data cluster: l2_entry=8000000000c00000 refcount=3 +Repairing OFLAG_COPIED data cluster: l2_entry=8000000000e00000 refcount=2 +The following inconsistencies were found and repaired: + + 0 leaked clusters + 4 corruptions + +Double checking the fixed image now... +No errors were found on the image. +read 4194304/4194304 bytes at offset 0 +4 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +read 4194304/4194304 bytes at offset 4194304 +4 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) + === Full allocation with -S 0 === Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
Signature · 1 line
2.11.0
tests/qemu-iotests/122
Quoted 70 lines · Alberto Garcia
L2 entries for compressed clusters have a field that indicates the number of sectors used to store the data in the image.
That's however not the size of the compressed data itself, just the number of sectors where that data is located. The actual data size is usually not a multiple of the sector size, and therefore cannot be represented with this field.
The way it works is that QEMU reads all the specified sectors and starts decompressing the data until there's enough to recover the original uncompressed cluster. If there are any bytes left that haven't been decompressed they are simply ignored.
One consequence of this is that even if the size field is larger than it needs to be QEMU can handle it just fine: it will read more data from disk but it will ignore the extra bytes.
This test creates an image with two compressed clusters that use 5 sectors (2.5 KB) each, increases the size field to the maximum (8192 sectors, or 4 MB) and verifies that the data can be read without problems.
This test is important because while the decompressed data takes exactly one cluster, the maximum value allowed in the compressed size field is twice the cluster size. So although QEMU won't produce images with such large values we need to make sure that it can handle them.
Another effect of increasing the size field is that it can make it include data from the following host cluster(s). In this case 'qemu-img check' will detect that the refcounts are not correct, and we'll need to rebuild them.
Additionally, this patch also tests that decreasing the size corrupts the image since the original data can no longer be recovered. In this case QEMU returns an error when trying to read the compressed data, but 'qemu-img check' doesn't see anything wrong if the refcounts are consistent.
One possible task for the future is to make 'qemu-img check' verify the sizes of the compressed clusters, by trying to decompress the data and checking that the size stored in the L2 entry is correct.
Signed-off-by Alberto Garcia <[email protected]>Reviewed-by Eric Blake <[email protected]>---
v4: Resend for 2.12v3: Add TODO comment, as suggested by Eric.
Corrupt the length of the second compressed cluster as well so the uncompressed data would span three host clusters.
v2: We now have two scenarios where we make QEMU read data from the
next host cluster and from beyond the end of the image. This version also runs qemu-img check on the corrupted image.If the size field is too small, reading fails but qemu-img check succeeds.
If the size field is too large, reading succeeds but qemu-img check fails (this can be repaired, though).
--- tests/qemu-iotests/122 | 45 +++++++++++++++++++++++++++++++++++++++++++++ tests/qemu-iotests/122.out | 31 +++++++++++++++++++++++++++++++ 2 files changed, 76 insertions(+)tests/qemu-iotests/122 +0
-0
Not sure if 122 is the right file for this...
Or, let me rephrase, it does look to me like it is not the right file. But on the other hand, I don't see a more suitable file.
tests/qemu-iotests/122 $QEMU_IO -c "read -P 0 1024k 1022k" "$TEST_IMG" 2>&1 | _filter_qemu_io | _fil · line 141
Quoted 13 lines
tests/qemu-iotests/122 +9
-0@@ -130,6 +130,51 @@ $QEMU_IO -c "read -P 0 1024k 1022k" "$TEST_IMG" 2>&1 | _filter_qemu_io | _fil echo +echo "=== Corrupted size field in compressed cluster descriptor ===" +echo +# Create an empty image, fill half of it with data and compress it. +# The L2 entries of the two compressed clusters are located at +# 0x800000 and 0x800008, their original values are 0x4008000000a00000 +# and 0x4008000000a00802 (5 sectors for compressed data each). +TEST_IMG="$TEST_IMG".1 _make_test_img 8M +$QEMU_IO -c "write -P 0x11 0 4M" "$TEST_IMG".1 2>&1 | _filter_qemu_io | _filter_testdir +$QEMU_IMG convert -c -O qcow2 -o cluster_size=2M "$TEST_IMG".1 "$TEST_IMG"
Why not just use "write -c" and drop the convert? (You'd have to use two writes, though, one for each cluster.)
Quoted 17 lines
tests/qemu-iotests/122 +17
-0+ +# Reduce size of compressed data to 4 sectors: this corrupts the image. +poke_file "$TEST_IMG" $((0x800000)) "\x40\x06" +$QEMU_IO -c "read -P 0x11 0 4M" "$TEST_IMG" 2>&1 | _filter_qemu_io | _filter_testdir + +# 'qemu-img check' however doesn't see anything wrong because it +# doesn't try to decompress the data and the refcounts are consistent. +# TODO: update qemu-img so this can be detected +_check_test_img + +# Increase size of compressed data to the maximum (8192 sectors). +# This makes QEMU read more data (8192 sectors instead of 5, host +# addresses [0xa00000, 0xdfffff]), but the decompression algorithm +# stops once we have enough to restore the uncompressed cluster, so +# the rest of the data is ignored. +poke_file "$TEST_IMG" $((0x800000)) "\x7f\xfe" +# Do it also for the second compressed cluster (L2 entry at 0x800008).
tests/qemu-iotests/122 $QEMU_IO -c "read -P 0 1024k 1022k" "$TEST_IMG" 2>&1 | _filter_qemu_io | _fil · line 168
Quoted 10 lines
tests/qemu-iotests/122 +10
-0+# In this case the compressed data would span 3 host clusters +# (host addresses: [0xa00802, 0xe00801]) +poke_file "$TEST_IMG" $((0x800008)) "\x7f\xfe" + +# Here the image is too small so we're asking QEMU to read beyond the +# end of the image. +$QEMU_IO -c "read -P 0x11 0 4M" "$TEST_IMG" 2>&1 | _filter_qemu_io | _filter_testdir +# But if we grow the image we won't be reading beyond its end anymore. +$QEMU_IO -c "write -P 0x22 4M 4M" "$TEST_IMG" 2>&1 | _filter_qemu_io | _filter_testdir +$QEMU_IO -c "read -P 0x11 0 4M" "$TEST_IMG" 2>&1 | _filter_qemu_io | _filter_testdir
Both reads result in exactly the same output, though, so it doesn't seem like qemu cares.
(This is the reason I'm not merging this patch now, because that looks a bit fishy.)
tests/qemu-iotests/122 $QEMU_IO -c "read -P 0 1024k 1022k" "$TEST_IMG" 2>&1 | _filter_qemu_io | _fil · line 172
Quoted 4 lines
tests/qemu-iotests/122 +4
-0+ +# The refcount data is however wrong because due to the increased size +# of the compressed data it now reaches the following host clusters. +# This can be repaired by qemu-img check.
The OFLAG_COPIED repair looks a bit interesting, but, er, well.
Max
(Since a compressed cluster does not correspond 1:1 to a host cluster, you cannot say that a compressed cluster has a refcount -- only host clusters are refcounted. So what is it supposed to mean that a compressed cluster has a refcount of 1?
I'd argue from a point of usefulness. In theory, you could modify compressed clusters in-place, and then you'd need the information whether you are allowed to. But that doesn't really depend on whether the host clusters touched by the compressed cluster have a refcount of 1, instead if depends on how many times the compressed cluster itself is referenced. Unfortunately we have no refcounting structure for that.
So all in all OFLAG_COPIED is just useless for compressed clusters.)
Quoted 51 lines
tests/qemu-iotests/122 +5
-0+_check_test_img -r all +$QEMU_IO -c "read -P 0x11 0 4M" "$TEST_IMG" 2>&1 | _filter_qemu_io | _filter_testdir +$QEMU_IO -c "read -P 0x22 4M 4M" "$TEST_IMG" 2>&1 | _filter_qemu_io | _filter_testdir + +echo echo "=== Full allocation with -S 0 ===" echotests/qemu-iotests/122.out +31
-0@@ -99,6 +99,37 @@ read 1024/1024 bytes at offset 1047552 read 1046528/1046528 bytes at offset 1048576 1022 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +=== Corrupted size field in compressed cluster descriptor === + +Formatting 'TEST_DIR/t.IMGFMT.1', fmt=IMGFMT size=8388608 +wrote 4194304/4194304 bytes at offset 0 +4 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +read failed: Input/output error +No errors were found on the image. +read 4194304/4194304 bytes at offset 0 +4 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +wrote 4194304/4194304 bytes at offset 4194304 +4 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +read 4194304/4194304 bytes at offset 0 +4 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +ERROR cluster 6 refcount=1 reference=3 +ERROR cluster 7 refcount=1 reference=2 +Repairing cluster 6 refcount=1 reference=3 +Repairing cluster 7 refcount=1 reference=2 +Repairing OFLAG_COPIED data cluster: l2_entry=8000000000c00000 refcount=3 +Repairing OFLAG_COPIED data cluster: l2_entry=8000000000e00000 refcount=2 +The following inconsistencies were found and repaired: + + 0 leaked clusters + 4 corruptions + +Double checking the fixed image now... +No errors were found on the image. +read 4194304/4194304 bytes at offset 0 +4 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +read 4194304/4194304 bytes at offset 4194304 +4 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) + === Full allocation with -S 0 === Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
Quoted 10 lines · Max Reitz
Quoted 8 lines · Alberto Garcia
L2 entries for compressed clusters have a field that indicates the number of sectors used to store the data in the image.
That's however not the size of the compressed data itself, just the number of sectors where that data is located. The actual data size is usually not a multiple of the sector size, and therefore cannot be represented with this field.
Quoted 6 lines
Quoted 1 line
+++ b/tests/qemu-iotests/122
Not sure if 122 is the right file for this...
Or, let me rephrase, it does look to me like it is not the right file. But on the other hand, I don't see a more suitable file.
Short of cloning 122 as the starting point and creating a new test file.
Quoted 4 lines
Quoted 4 lines
(quoted diff) +0
-0@@ -130,6 +130,51 @@ $QEMU_IO -c "read -P 0 1024k 1022k" "$TEST_IMG" 2>&1 | _filter_qemu_io | _fil
Quoted 13 lines
tests/qemu-iotests/122 $QEMU_IO -c "read -P 0 1024k 1022k" "$TEST_IMG" 2>&1 | _filter_qemu_io | _fil · line 141
Quoted 10 lines
tests/qemu-iotests/122 +9
-0echo +echo "=== Corrupted size field in compressed cluster descriptor ===" +echo +# Create an empty image, fill half of it with data and compress it. +# The L2 entries of the two compressed clusters are located at +# 0x800000 and 0x800008, their original values are 0x4008000000a00000 +# and 0x4008000000a00802 (5 sectors for compressed data each). +TEST_IMG="$TEST_IMG".1 _make_test_img 8M +$QEMU_IO -c "write -P 0x11 0 4M" "$TEST_IMG".1 2>&1 | _filter_qemu_io | _filter_testdir +$QEMU_IMG convert -c -O qcow2 -o cluster_size=2M "$TEST_IMG".1 "$TEST_IMG"Why not just use "write -c" and drop the convert? (You'd have to use two writes, though, one for each cluster.)
'write -c' is newer code; it may work, but it may also cause offsets to live elsewhere for knock-on effects later in the test. (It used to be compression was only possible during convert)
Quoted 11 lines
tests/qemu-iotests/122 $QEMU_IO -c "read -P 0 1024k 1022k" "$TEST_IMG" 2>&1 | _filter_qemu_io | _fil · line 168
Quoted 7 lines
tests/qemu-iotests/122 +7
-0+ +# Here the image is too small so we're asking QEMU to read beyond the +# end of the image. +$QEMU_IO -c "read -P 0x11 0 4M" "$TEST_IMG" 2>&1 | _filter_qemu_io | _filter_testdir +# But if we grow the image we won't be reading beyond its end anymore. +$QEMU_IO -c "write -P 0x22 4M 4M" "$TEST_IMG" 2>&1 | _filter_qemu_io | _filter_testdir +$QEMU_IO -c "read -P 0x11 0 4M" "$TEST_IMG" 2>&1 | _filter_qemu_io | _filter_testdirBoth reads result in exactly the same output, though, so it doesn't seem like qemu cares.
Quoted 2 lines
(This is the reason I'm not merging this patch now, because that looks a bit fishy.)
Hmm, you have an interesting point - should the read fail (you asked me to read more clusters than I have available) or succeed (since you asked me to read beyond EOF, I filled the tail of the buffer with all zeroes, then tried decompressing, and since decompression worked, it obviously didn't need the bytes that I filled in as zero). It's a little nicer to fail (the image is fishy for requesting more clusters than are present, even if what IS present is sufficient to reconstruct the data).
Quoted 8 lines
tests/qemu-iotests/122 $QEMU_IO -c "read -P 0 1024k 1022k" "$TEST_IMG" 2>&1 | _filter_qemu_io | _fil · line 172
Quoted 5 lines
tests/qemu-iotests/122 +4
-0+ +# The refcount data is however wrong because due to the increased size +# of the compressed data it now reaches the following host clusters. +# This can be repaired by qemu-img check.The
OFLAG_COPIEDrepair looks a bit interesting, but, er, well.
Quoted 6 lines
Max
(Since a compressed cluster does not correspond 1:1 to a host cluster, you cannot say that a compressed cluster has a refcount -- only host clusters are refcounted. So what is it supposed to mean that a compressed cluster has a refcount of 1?
A compressed cluster may affect the refcount of multiple clusters: the cluster that contains the initial offset, and the cluster that contains any of the nb_sectors that did not fit in the first cluster. So, if I have a 4k-cluster image (where each character is a sector), and where the compressed clusters are nicely sector-aligned:
|1-------|2-------|3-------| |AAAAAABB|BBBBCCCC|CC------|
Here, the L2 entry for A, B, and C each list nb_sectors of 5, as it takes 6 sectors to list the entire image, but nb_sectors does not include the sector that includes the original offset. The refcount for cluster 1 is 2 (the full contents of compressed A and the first half of compressed B); for cluster 2 is 2 (the second half of compressed B and the first half of compressed C); and for cluster 3 is 1 (the second half of compressed C).
But what this patch is dealing with is when nb_sectors is larger than required. With 4k sectors, qemu will never populate nb_clusters more than 8 (if the output is not nicely aligned, and 4096 bytes compresses down to only 4095, we can end up with 1 byte in the first sector, then 7 complete sectors, then 510 bytes in a final sector, for 8 sectors beyond the initial offset). But the qcow2 image is still valid even if the L2 entry claims nb_sectors of 15; if that happens, then a compressed cluster can now affect the refcount of 3 clusters rather than the usual 1 or 2.
Quoted 9 lines
I'd argue from a point of usefulness. In theory, you could modify compressed clusters in-place, and then you'd need the information whether you are allowed to. But that doesn't really depend on whether the host clusters touched by the compressed cluster have a refcount of 1, instead if depends on how many times the compressed cluster itself is referenced. Unfortunately we have no refcounting structure for that.
So all in all
OFLAG_COPIEDis just useless for compressed clusters.)
In general, because we intentionally allow multiple compressed clusters to (try to) share a single host cluster, the refcount for compressed clusters will usually be larger than 1. And OFLAG_COPIED is only useful when the refcount is 1, right?
Signature · 3 lines
Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Quoted 1 line · Eric Blake
On 03/28/2018 12:34 PM, Max Reitz wrote:
[...]
Quoted 44 lines
Quoted 8 lines
The
OFLAG_COPIEDrepair looks a bit interesting, but, er, well.Max
(Since a compressed cluster does not correspond 1:1 to a host cluster, you cannot say that a compressed cluster has a refcount -- only host clusters are refcounted. So what is it supposed to mean that a compressed cluster has a refcount of 1?
A compressed cluster may affect the refcount of multiple clusters: the cluster that contains the initial offset, and the cluster that contains any of the
nb_sectorsthat did not fit in the first cluster. So, if I have a 4k-cluster image (where each character is a sector), and where the compressed clusters are nicely sector-aligned:|1-------|2-------|3-------| |AAAAAABB|BBBBCCCC|CC------|Here, the L2 entry for A, B, and C each list
nb_sectorsof 5, as it takes 6 sectors to list the entire image, butnb_sectorsdoes not include the sector that includes the original offset. The refcount for cluster 1 is 2 (the full contents of compressed A and the first half of compressed B); for cluster 2 is 2 (the second half of compressed B and the first half of compressed C); and for cluster 3 is 1 (the second half of compressed C).But what this patch is dealing with is when
nb_sectorsis larger than required. With 4k sectors, qemu will never populatenb_clustersmore than 8 (if the output is not nicely aligned, and 4096 bytes compresses down to only 4095, we can end up with 1 byte in the first sector, then 7 complete sectors, then 510 bytes in a final sector, for 8 sectors beyond the initial offset). But the qcow2 image is still valid even if the L2 entry claimsnb_sectorsof 15; if that happens, then a compressed cluster can now affect the refcount of 3 clusters rather than the usual 1 or 2.Quoted 8 lines
I'd argue from a point of usefulness. In theory, you could modify compressed clusters in-place, and then you'd need the information whether you are allowed to. But that doesn't really depend on whether the host clusters touched by the compressed cluster have a refcount of 1, instead if depends on how many times the compressed cluster itself is referenced. Unfortunately we have no refcounting structure for that.
Quoted 6 lines
Quoted 1 line
So all in all
OFLAG_COPIEDis just useless for compressed clusters.)In general, because we intentionally allow multiple compressed clusters to (try to) share a single host cluster, the refcount for compressed clusters will usually be larger than 1. And
OFLAG_COPIEDis only useful when the refcount is 1, right?
OFLAG_COPIED is only useful when it reflects the number of references to the data cluster, because only then can we update it in-place without having to worry about other users. For normal clusters, one data cluster is one host cluster, so we can get the number of references from the refcount structures (which contain the reference counts for host clusters, but *not* for data clusters).
But for compressed clusters, the data cluster may be part of a host cluster, or it may even be multiple host clusters. So the information we get from the refcount structures is totally useless here, it doesn't tell us how many references there are to that compressed data cluster.
There is only one exception: If all of the host clusters the compressed cluster touches have a refcount of 1, we can be certain that there are no other users, so then we know there is only one reference to the compressed cluster. But as soon as they have a higher refcount... We don't know whether that's because of multiple references to this compressed cluster or because of multiple compressed cluster sharing a single host cluster.
In practice it doesn't really matter of course because modifying compressed clusters in-place seems like a whole different can of worms.
Max
Quoted 9 lines · Max Reitz
tests/qemu-iotests/122
Quoted 4 lines
tests/qemu-iotests/122 +0
-0Not sure if 122 is the right file for this...
Or, let me rephrase, it does look to me like it is not the right file. But on the other hand, I don't see a more suitable file.
Yeah I was tempted to create a new one, but in the end I decided to put it there. We can still create a new one if you feel strongly about it.
Quoted 5 lines
Quoted 5 lines
echo
tests/qemu-iotests/122 +4
-0+echo "=== Corrupted size field in compressed cluster descriptor ===" +echo +# Create an empty image, fill half of it with data and compress it. +# The L2 entries of the two compressed clusters are located at
Quoted 8 lines
tests/qemu-iotests/122 $QEMU_IO -c "read -P 0 1024k 1022k" "$TEST_IMG" 2>&1 | _filter_qemu_io | _fil · line 141
Quoted 5 lines
tests/qemu-iotests/122 +5
-0+# 0x800000 and 0x800008, their original values are 0x4008000000a00000 +# and 0x4008000000a00802 (5 sectors for compressed data each). +TEST_IMG="$TEST_IMG".1 _make_test_img 8M +$QEMU_IO -c "write -P 0x11 0 4M" "$TEST_IMG".1 2>&1 | _filter_qemu_io | _filter_testdir +$QEMU_IMG convert -c -O qcow2 -o cluster_size=2M "$TEST_IMG".1 "$TEST_IMG"Why not just use "write -c" and drop the convert? (You'd have to use two writes, though, one for each cluster.)
Yeah, it's also possible, you have to do it in the same qemu-io command though, else it will allocate two host clusters. But yes, I can change that.
Quoted 30 lines
Quoted 16 lines
tests/qemu-iotests/122 +16
-0+# Reduce size of compressed data to 4 sectors: this corrupts the image. +poke_file "$TEST_IMG" $((0x800000)) "\x40\x06" +$QEMU_IO -c "read -P 0x11 0 4M" "$TEST_IMG" 2>&1 | _filter_qemu_io | _filter_testdir + +# 'qemu-img check' however doesn't see anything wrong because it +# doesn't try to decompress the data and the refcounts are consistent. +# TODO: update qemu-img so this can be detected +_check_test_img + +# Increase size of compressed data to the maximum (8192 sectors). +# This makes QEMU read more data (8192 sectors instead of 5, host +# addresses [0xa00000, 0xdfffff]), but the decompression algorithm +# stops once we have enough to restore the uncompressed cluster, so +# the rest of the data is ignored. +poke_file "$TEST_IMG" $((0x800000)) "\x7f\xfe" +# Do it also for the second compressed cluster (L2 entry at 0x800008).tests/qemu-iotests/122 $QEMU_IO -c "read -P 0 1024k 1022k" "$TEST_IMG" 2>&1 | _filter_qemu_io | _fil · line 168
Quoted 10 lines
tests/qemu-iotests/122 +10
-0+# In this case the compressed data would span 3 host clusters +# (host addresses: [0xa00802, 0xe00801]) +poke_file "$TEST_IMG" $((0x800008)) "\x7f\xfe" + +# Here the image is too small so we're asking QEMU to read beyond the +# end of the image. +$QEMU_IO -c "read -P 0x11 0 4M" "$TEST_IMG" 2>&1 | _filter_qemu_io | _filter_testdir +# But if we grow the image we won't be reading beyond its end anymore. +$QEMU_IO -c "write -P 0x22 4M 4M" "$TEST_IMG" 2>&1 | _filter_qemu_io | _filter_testdir +$QEMU_IO -c "read -P 0x11 0 4M" "$TEST_IMG" 2>&1 | _filter_qemu_io | _filter_testdirBoth reads result in exactly the same output, though, so it doesn't seem like qemu cares.
Quoted 2 lines
(This is the reason I'm not merging this patch now, because that looks a bit fishy.)
In this example the size stored on the compressed cluster descriptor is corrupted and larger than the size of the compressed data on disk.
In the first read the image is too small so this makes QEMU read not only past the end of the compressed data, but also past the end of the qcow2 image itself.
In the second read the qcow2 image is larger so QEMU reads actual data from the subsequent clusters.
It doesn't make much difference. In the first case the buffer is padded with zeroes and in the second with data from other clusters, but QEMU only uses the data needed to restore the original uncompressed cluster and the rest is ignored. But I thought I could still test both cases, that's why there's two reads.
Quoted 6 lines
tests/qemu-iotests/122 $QEMU_IO -c "read -P 0 1024k 1022k" "$TEST_IMG" 2>&1 | _filter_qemu_io | _fil · line 172
Quoted 3 lines
tests/qemu-iotests/122 +3
-0+# The refcount data is however wrong because due to the increased size +# of the compressed data it now reaches the following host clusters. +# This can be repaired by qemu-img check.The
OFLAG_COPIEDrepair looks a bit interesting, but, er, well.
Quoted 6 lines
Max
(Since a compressed cluster does not correspond 1:1 to a host cluster, you cannot say that a compressed cluster has a refcount -- only host clusters are refcounted. So what is it supposed to mean that a compressed cluster has a refcount of 1?
Compressed clusters never have OFLAG_COPIED and we treat it as an error if they do (see check_refcounts_l2()), but their host clusters still have a reference count.
A host cluster with 4 compressed clusters has a reference count of 4.
A compressed cluster that uses two host clusters (even if it's only partially) increases the reference count of both of them.
In this test case the size field of the compressed cluster is too large, so it includes uncompressed clusters that are contiguous to it. The uncompressed clusters have refcount == 1 and OFLAG_COPIED, but QEMU detects that the refcount should be larger and that OFLAG_COPIED should not be there, that's why it's repaired.
Admittedly the way it's repaired is not the best one: the uncompressed clusters have now extra references, but other than that I don't see any harmful effect.
The proper fix for this would be to detect that the compressed cluster size is incorrect and correct its value. There's a TODO for that (in the first _check_test_img call where it's more severe), but now that I think of it I should probably mention that in the second one as well.
Berto
Quoted 72 lines · Alberto Garcia
Quoted 9 lines · Max Reitz
tests/qemu-iotests/122
Quoted 4 lines
tests/qemu-iotests/122 +0
-0Not sure if 122 is the right file for this...
Or, let me rephrase, it does look to me like it is not the right file. But on the other hand, I don't see a more suitable file.
Yeah I was tempted to create a new one, but in the end I decided to put it there. We can still create a new one if you feel strongly about it.
Quoted 5 lines
Quoted 5 lines
echo
tests/qemu-iotests/122 +4
-0+echo "=== Corrupted size field in compressed cluster descriptor ===" +echo +# Create an empty image, fill half of it with data and compress it. +# The L2 entries of the two compressed clusters are located atQuoted 8 lines
tests/qemu-iotests/122 $QEMU_IO -c "read -P 0 1024k 1022k" "$TEST_IMG" 2>&1 | _filter_qemu_io | _fil · line 141
Quoted 5 lines
tests/qemu-iotests/122 +5
-0+# 0x800000 and 0x800008, their original values are 0x4008000000a00000 +# and 0x4008000000a00802 (5 sectors for compressed data each). +TEST_IMG="$TEST_IMG".1 _make_test_img 8M +$QEMU_IO -c "write -P 0x11 0 4M" "$TEST_IMG".1 2>&1 | _filter_qemu_io | _filter_testdir +$QEMU_IMG convert -c -O qcow2 -o cluster_size=2M "$TEST_IMG".1 "$TEST_IMG"Why not just use "write -c" and drop the convert? (You'd have to use two writes, though, one for each cluster.)
Yeah, it's also possible, you have to do it in the same qemu-io command though, else it will allocate two host clusters. But yes, I can change that.
Quoted 30 lines
Quoted 16 lines
tests/qemu-iotests/122 +16
-0+# Reduce size of compressed data to 4 sectors: this corrupts the image. +poke_file "$TEST_IMG" $((0x800000)) "\x40\x06" +$QEMU_IO -c "read -P 0x11 0 4M" "$TEST_IMG" 2>&1 | _filter_qemu_io | _filter_testdir + +# 'qemu-img check' however doesn't see anything wrong because it +# doesn't try to decompress the data and the refcounts are consistent. +# TODO: update qemu-img so this can be detected +_check_test_img + +# Increase size of compressed data to the maximum (8192 sectors). +# This makes QEMU read more data (8192 sectors instead of 5, host +# addresses [0xa00000, 0xdfffff]), but the decompression algorithm +# stops once we have enough to restore the uncompressed cluster, so +# the rest of the data is ignored. +poke_file "$TEST_IMG" $((0x800000)) "\x7f\xfe" +# Do it also for the second compressed cluster (L2 entry at 0x800008).tests/qemu-iotests/122 $QEMU_IO -c "read -P 0 1024k 1022k" "$TEST_IMG" 2>&1 | _filter_qemu_io | _fil · line 168
Quoted 10 lines
tests/qemu-iotests/122 +10
-0+# In this case the compressed data would span 3 host clusters +# (host addresses: [0xa00802, 0xe00801]) +poke_file "$TEST_IMG" $((0x800008)) "\x7f\xfe" + +# Here the image is too small so we're asking QEMU to read beyond the +# end of the image. +$QEMU_IO -c "read -P 0x11 0 4M" "$TEST_IMG" 2>&1 | _filter_qemu_io | _filter_testdir +# But if we grow the image we won't be reading beyond its end anymore. +$QEMU_IO -c "write -P 0x22 4M 4M" "$TEST_IMG" 2>&1 | _filter_qemu_io | _filter_testdir +$QEMU_IO -c "read -P 0x11 0 4M" "$TEST_IMG" 2>&1 | _filter_qemu_io | _filter_testdirBoth reads result in exactly the same output, though, so it doesn't seem like qemu cares.
Quoted 2 lines
(This is the reason I'm not merging this patch now, because that looks a bit fishy.)
In this example the size stored on the compressed cluster descriptor is corrupted and larger than the size of the compressed data on disk.
In the first read the image is too small so this makes QEMU read not only past the end of the compressed data, but also past the end of the qcow2 image itself.
Quoted 8 lines
In the second read the qcow2 image is larger so QEMU reads actual data from the subsequent clusters.
It doesn't make much difference. In the first case the buffer is padded with zeroes and in the second with data from other clusters, but QEMU only uses the data needed to restore the original uncompressed cluster and the rest is ignored. But I thought I could still test both cases, that's why there's two reads.
Ah, OK. I would have preferred a comment then so that people know it's just expected to work.
Quoted 8 lines
Quoted 8 lines
tests/qemu-iotests/122 $QEMU_IO -c "read -P 0 1024k 1022k" "$TEST_IMG" 2>&1 | _filter_qemu_io | _fil · line 172
Quoted 3 lines
tests/qemu-iotests/122 +3
-0+# The refcount data is however wrong because due to the increased size +# of the compressed data it now reaches the following host clusters. +# This can be repaired by qemu-img check.The
OFLAG_COPIEDrepair looks a bit interesting, but, er, well.Max
Quoted 7 lines
Quoted 4 lines
(Since a compressed cluster does not correspond 1:1 to a host cluster, you cannot say that a compressed cluster has a refcount -- only host clusters are refcounted. So what is it supposed to mean that a compressed cluster has a refcount of 1?
Compressed clusters never have
OFLAG_COPIEDand we treat it as an error if they do (seecheck_refcounts_l2()),
Interesting. Wonder why the qcow2 specification doesn't mention that...
Quoted 5 lines
but their host clusters still
have a reference count.A host cluster with 4 compressed clusters has a reference count of 4.
Quoted 8 lines
A compressed cluster that uses two host clusters (even if it's only partially) increases the reference count of both of them.
In this test case the size field of the compressed cluster is too large, so it includes uncompressed clusters that are contiguous to it. The uncompressed clusters have refcount == 1 and
OFLAG_COPIED, but QEMU detects that the refcount should be larger and thatOFLAG_COPIEDshould not be there, that's why it's repaired.
Aaah, OK, I see. Thanks for explaining.
Quoted 8 lines
Admittedly the way it's repaired is not the best one: the uncompressed clusters have now extra references, but other than that I don't see any harmful effect.
The proper fix for this would be to detect that the compressed cluster size is incorrect and correct its value. There's a TODO for that (in the first
_check_test_imgcall where it's more severe), but now that I think of it I should probably mention that in the second one as well.
Well, the current repair method will at least prevent data loss, so I'm OK with it.
Max