Re: [PATCH v2 5/5] iotests: add commit top->base cases to 274
On 5/19/20 4:25 PM, Vladimir Sementsov-Ogievskiy wrote:
$ ./qemu-img map --output=json top.qcow2
[{ "start": 0, "length": 1048576, "depth": 2, "zero": false, "data":
true, "offset": 327680},
{ "start": 1048576, "length": 1048576, "depth": 0, "zero": true,
"data": false}]
I think what we really want is:
[{ "start": 0, "length": 1048576, "depth": 2, "zero": false, "data":
true, "offset": 327680},
{ "start": 1048576, "length": 1048576, "depth": 1, "zero": true,
"data": false}]
because then we would be _accurately_ reporting that the zeroes that
we read from 1m-2m come _because_ we read from mid (beyond EOF), which
is different from our current answer that the zeroes come from top
(they don't, because top deferred to mid).
Right. This is exactly the logic which I bring to block_status_above and
is_allocated_above by this series
If we fix up qemu-img map output to correctly report zeroes beyond EOF
from the correct layer, will that also fix up the bug we are seeing in
qemu-img commit?
No it will not fix it, because img_map has own implementation of
block_status_above - get_block_status function in qemu-img.c, which goes
through backing chain by itself, and is used only in img_map (not in
img_convert). But you are right that it should be fixed too.
You are in a maze of twisty passages, all alike ;)
[Hope neither of us is eaten by a grue by the time we get this series in]
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
Re: [PATCH v2 5/5] iotests: add commit top->base cases to 274
20.05.2020 00:13, Eric Blake wrote:
On 5/19/20 2:55 PM, Vladimir Sementsov-Ogievskiy wrote:
These cases are fixed by previous patches around block_status and
is_allocated.
Signed-off-by: Vladimir Sementsov-Ogievskiy
---
tests/qemu-iotests/274 | 20
tests/qemu-iotests/274.out | 65 ++
2 files changed, 85 insertions(+)
Okay, so this test fails when applied in isolation without the rest of your
series.
diff --git a/tests/qemu-iotests/274 b/tests/qemu-iotests/274
index 5d1bf34dff..e910455f13 100755
--- a/tests/qemu-iotests/274
+++ b/tests/qemu-iotests/274
@@ -115,6 +115,26 @@ with iotests.FilePath('base') as base, \
iotests.qemu_io_log('-c', 'read -P 1 0 %d' % size_short, mid)
iotests.qemu_io_log('-c', 'read -P 0 %d %d' % (size_short, size_diff),
mid)
+ iotests.log('=== Testing qemu-img commit (top -> base) ===')
+
+ create_chain()
+ iotests.qemu_img_log('commit', '-b', base, top)
+ iotests.img_info_log(base)
+ iotests.qemu_io_log('-c', 'read -P 1 0 %d' % size_short, base)
+ iotests.qemu_io_log('-c', 'read -P 0 %d %d' % (size_short, size_diff),
base)
So if I understand it, we are going from:
base
mid
top
guest
and we want to go to:
base
except that we are not properly writing the zeroes into base, because we
grabbed the wrong status, ending up with:
base
The status of top from 1M onwards is unallocated, and if we were to commit to
just mid, Kevin's truncate fixes solve that (we now zero out the tail of mid as
part of resizing it to be large enough). But you are instead skipping mid, and
committing all the way to base. So we need _something_ that can tell qemu-img
commit that even though the region 1m-2m is unallocated in top, we must behave
as though the status of mid reports it as allocated (because when reading
beyond EOF in mid, we DO read zero). Since the data is allocated not in top,
but acts as though it was allocated in mid, which is above base, then the
commit operation has to do something to preserve that allocation.
Okay, you've convinced me we have a bug. > However, I'm still not sold that
patches 1 and 4 are quite the right fix. Going back to the original setup,
unpatched qemu.git head reports:
$ ./qemu-img map --output=json top.qcow2
[{ "start": 0, "length": 1048576, "depth": 2, "zero": false, "data": true,
"offset": 327680},
{ "start": 1048576, "length": 1048576, "depth": 0, "zero": true, "data": false}]
I think what we really want is:
[{ "start": 0, "length": 1048576, "depth": 2, "zero": false, "data": true,
"offset": 327680},
{ "start": 1048576, "length": 1048576, "depth": 1, "zero": true, "data": false}]
because then we would be _accurately_ reporting that the zeroes that we read from 1m-2m come _because_ we read from mid (beyond EOF), which is different from our current answer that the zeroes come from top (they don't, because top deferred to mid).
Right. This is exactly the logic which I bring to block_status_above and
is_allocated_above by this series
If we fix up qemu-img map output to correctly report zeroes beyond EOF from the
correct layer, will that also fix up the bug we are seeing in qemu-img commit?
No it will not fix it, because img_map has own implementation of
block_status_above - get_block_status function in qemu-img.c, which goes
through backing chain by itself, and is used only in img_map (not in
img_convert). But you are right that it should be fixed too.
--
Best regards,
Vladimir
Re: [PATCH v2 5/5] iotests: add commit top->base cases to 274
On 5/19/20 2:55 PM, Vladimir Sementsov-Ogievskiy wrote:
These cases are fixed by previous patches around block_status and
is_allocated.
Signed-off-by: Vladimir Sementsov-Ogievskiy
---
tests/qemu-iotests/274 | 20
tests/qemu-iotests/274.out | 65 ++
2 files changed, 85 insertions(+)
Okay, so this test fails when applied in isolation without the rest of
your series.
diff --git a/tests/qemu-iotests/274 b/tests/qemu-iotests/274
index 5d1bf34dff..e910455f13 100755
--- a/tests/qemu-iotests/274
+++ b/tests/qemu-iotests/274
@@ -115,6 +115,26 @@ with iotests.FilePath('base') as base, \
iotests.qemu_io_log('-c', 'read -P 1 0 %d' % size_short, mid)
iotests.qemu_io_log('-c', 'read -P 0 %d %d' % (size_short, size_diff),
mid)
+iotests.log('=== Testing qemu-img commit (top -> base) ===')
+
+create_chain()
+iotests.qemu_img_log('commit', '-b', base, top)
+iotests.img_info_log(base)
+iotests.qemu_io_log('-c', 'read -P 1 0 %d' % size_short, base)
+iotests.qemu_io_log('-c', 'read -P 0 %d %d' % (size_short, size_diff),
base)
So if I understand it, we are going from:
base
mid
top
guest
and we want to go to:
base
except that we are not properly writing the zeroes into base, because we
grabbed the wrong status, ending up with:
base
The status of top from 1M onwards is unallocated, and if we were to
commit to just mid, Kevin's truncate fixes solve that (we now zero out
the tail of mid as part of resizing it to be large enough). But you are
instead skipping mid, and committing all the way to base. So we need
_something_ that can tell qemu-img commit that even though the region
1m-2m is unallocated in top, we must behave as though the status of mid
reports it as allocated (because when reading beyond EOF in mid, we DO
read zero). Since the data is allocated not in top, but acts as though
it was allocated in mid, which is above base, then the commit operation
has to do something to preserve that allocation.
Okay, you've convinced me we have a bug. However, I'm still not sold
that patches 1 and 4 are quite the right fix. Going back to the
original setup, unpatched qemu.git head reports:
$ ./qemu-img map --output=json top.qcow2
[{ "start": 0, "length": 1048576, "depth": 2, "zero": false, "data":
true, "offset": 327680},
{ "start": 1048576, "length": 1048576, "depth": 0, "zero": true, "data":
false}]
I think what we really want is:
[{ "start": 0, "length": 1048576, "depth": 2, "zero": false, "data":
true, "offset": 327680},
{ "start": 1048576, "length": 1048576, "depth": 1, "zero": true, "data":
false}]
because then we would be _accurately_ reporting that the zeroes that we
read from 1m-2m come _because_ we read from mid (beyond EOF), which is
different from our current answer that the zeroes come from top (they
don't, because top deferred to mid). If we fix up qemu-img map output
to correctly report zeroes beyond EOF from the correct layer, will that
also fix up the bug we are seeing in qemu-img commit?
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
[PATCH v2 5/5] iotests: add commit top->base cases to 274
These cases are fixed by previous patches around block_status and
is_allocated.
Signed-off-by: Vladimir Sementsov-Ogievskiy
---
tests/qemu-iotests/274 | 20
tests/qemu-iotests/274.out | 65 ++
2 files changed, 85 insertions(+)
diff --git a/tests/qemu-iotests/274 b/tests/qemu-iotests/274
index 5d1bf34dff..e910455f13 100755
--- a/tests/qemu-iotests/274
+++ b/tests/qemu-iotests/274
@@ -115,6 +115,26 @@ with iotests.FilePath('base') as base, \
iotests.qemu_io_log('-c', 'read -P 1 0 %d' % size_short, mid)
iotests.qemu_io_log('-c', 'read -P 0 %d %d' % (size_short, size_diff), mid)
+iotests.log('=== Testing qemu-img commit (top -> base) ===')
+
+create_chain()
+iotests.qemu_img_log('commit', '-b', base, top)
+iotests.img_info_log(base)
+iotests.qemu_io_log('-c', 'read -P 1 0 %d' % size_short, base)
+iotests.qemu_io_log('-c', 'read -P 0 %d %d' % (size_short, size_diff),
base)
+
+iotests.log('=== Testing QMP active commit (top -> base) ===')
+
+create_chain()
+with create_vm() as vm:
+vm.launch()
+vm.qmp_log('block-commit', device='top', base_node='base',
+ job_id='job0', auto_dismiss=False)
+vm.run_job('job0', wait=5)
+
+iotests.img_info_log(mid)
+iotests.qemu_io_log('-c', 'read -P 1 0 %d' % size_short, base)
+iotests.qemu_io_log('-c', 'read -P 0 %d %d' % (size_short, size_diff),
base)
iotests.log('== Resize tests ==')
diff --git a/tests/qemu-iotests/274.out b/tests/qemu-iotests/274.out
index d24ff681af..9806dea8b6 100644
--- a/tests/qemu-iotests/274.out
+++ b/tests/qemu-iotests/274.out
@@ -129,6 +129,71 @@ read 1048576/1048576 bytes at offset 0
read 1048576/1048576 bytes at offset 1048576
1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+=== Testing qemu-img commit (top -> base) ===
+Formatting 'TEST_DIR/PID-base', fmt=qcow2 size=2097152 cluster_size=65536
lazy_refcounts=off refcount_bits=16 compression_type=zlib
+
+Formatting 'TEST_DIR/PID-mid', fmt=qcow2 size=1048576
backing_file=TEST_DIR/PID-base cluster_size=65536 lazy_refcounts=off
refcount_bits=16 compression_type=zlib
+
+Formatting 'TEST_DIR/PID-top', fmt=qcow2 size=2097152
backing_file=TEST_DIR/PID-mid cluster_size=65536 lazy_refcounts=off
refcount_bits=16 compression_type=zlib
+
+wrote 2097152/2097152 bytes at offset 0
+2 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+Image committed.
+
+image: TEST_IMG
+file format: IMGFMT
+virtual size: 2 MiB (2097152 bytes)
+cluster_size: 65536
+Format specific information:
+compat: 1.1
+compression type: zlib
+lazy refcounts: false
+refcount bits: 16
+corrupt: false
+
+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)
+
+=== Testing QMP active commit (top -> base) ===
+Formatting 'TEST_DIR/PID-base', fmt=qcow2 size=2097152 cluster_size=65536
lazy_refcounts=off refcount_bits=16 compression_type=zlib
+
+Formatting 'TEST_DIR/PID-mid', fmt=qcow2 size=1048576
backing_file=TEST_DIR/PID-base cluster_size=65536 lazy_refcounts=off
refcount_bits=16 compression_type=zlib
+
+Formatting 'TEST_DIR/PID-top', fmt=qcow2 size=2097152
backing_file=TEST_DIR/PID-mid cluster_size=65536 lazy_refcounts=off
refcount_bits=16 compression_type=zlib
+
+wrote 2097152/2097152 bytes at offset 0
+2 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+{"execute": "block-commit", "arguments": {"auto-dismiss": false, "base-node":
"base", "device": "top", "job-id": "job0"}}
+{"return": {}}
+{"execute": "job-complete", "arguments": {"id": "job0"}}
+{"return": {}}
+{"data": {"device": "job0", "len": 1048576, "offset": 1048576, "speed": 0,
"type": "commit"}, "event": "BLOCK_JOB_READY", "timestamp": {"microseconds":
"USECS", "seconds": "SECS"}}
+{"data": {"device": "job0", "len": 1048576, "offset": 1048576, "speed": 0,
"type": "commit"}, "event": "BLOCK_JOB_COMPLETED", "timestamp":
{"microseconds": "USECS", "seconds": "SECS"}}
+{"execute": "job-dismiss", "arguments": {"id": "job0"}}
+{"return": {}}
+image: TEST_IMG
+file format: IMGFMT
+virtual size: 1 MiB (1048576 bytes)
+cluster_size: 65536
+backing file: TEST_DIR/PID-base
+Format specific information:
+compat: 1.1
+compression type: zlib
+lazy refcounts: false
+refcount bits: 16
+corrupt: false
+
+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)
+
== Resize tests ==
=== preallocation=off ===
Formatting 'TEST_DIR/PID-base', fmt=qcow2 size=6442450944 cluster_size=65536
lazy_refcounts=off refcount_bits=16 compression_type=zlib
--
2.21.0
