* [PATCH 1/2] block: Skip COR for inactive nodes
2019-10-01 17:48 [PATCH 0/2] block: Skip COR for inactive nodes Max Reitz
@ 2019-10-01 17:48 ` Max Reitz
2019-10-01 17:48 ` [PATCH 2/2] iotests/262: Switch source/dest VM launch order Max Reitz
` (2 subsequent siblings)
3 siblings, 0 replies; 5+ messages in thread
From: Max Reitz @ 2019-10-01 17:48 UTC (permalink / raw)
To: qemu-block; +Cc: Kevin Wolf, qemu-devel, Stefan Hajnoczi, Max Reitz
We must not write data to inactive nodes, and a COR is certainly
something we can simply not do without upsetting anyone. So skip COR
operations on inactive nodes.
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
block/io.c | 41 +++++++++++++++++++++++++++--------------
1 file changed, 27 insertions(+), 14 deletions(-)
diff --git a/block/io.c b/block/io.c
index f8c3596131..4f9ee97c2b 100644
--- a/block/io.c
+++ b/block/io.c
@@ -1246,11 +1246,18 @@ static int coroutine_fn bdrv_co_do_copy_on_readv(BdrvChild *child,
int max_transfer = MIN_NON_ZERO(bs->bl.max_transfer,
BDRV_REQUEST_MAX_BYTES);
unsigned int progress = 0;
+ bool skip_write;
if (!drv) {
return -ENOMEDIUM;
}
+ /*
+ * Do not write anything when the BDS is inactive. That is not
+ * allowed, and it would not help.
+ */
+ skip_write = (bs->open_flags & BDRV_O_INACTIVE);
+
/* FIXME We cannot require callers to have write permissions when all they
* are doing is a read request. If we did things right, write permissions
* would be obtained anyway, but internally by the copy-on-read code. As
@@ -1274,23 +1281,29 @@ static int coroutine_fn bdrv_co_do_copy_on_readv(BdrvChild *child,
while (cluster_bytes) {
int64_t pnum;
- ret = bdrv_is_allocated(bs, cluster_offset,
- MIN(cluster_bytes, max_transfer), &pnum);
- if (ret < 0) {
- /* Safe to treat errors in querying allocation as if
- * unallocated; we'll probably fail again soon on the
- * read, but at least that will set a decent errno.
- */
+ if (skip_write) {
+ ret = 1; /* "already allocated", so nothing will be copied */
pnum = MIN(cluster_bytes, max_transfer);
- }
+ } else {
+ ret = bdrv_is_allocated(bs, cluster_offset,
+ MIN(cluster_bytes, max_transfer), &pnum);
+ if (ret < 0) {
+ /*
+ * Safe to treat errors in querying allocation as if
+ * unallocated; we'll probably fail again soon on the
+ * read, but at least that will set a decent errno.
+ */
+ pnum = MIN(cluster_bytes, max_transfer);
+ }
- /* Stop at EOF if the image ends in the middle of the cluster */
- if (ret == 0 && pnum == 0) {
- assert(progress >= bytes);
- break;
- }
+ /* Stop at EOF if the image ends in the middle of the cluster */
+ if (ret == 0 && pnum == 0) {
+ assert(progress >= bytes);
+ break;
+ }
- assert(skip_bytes < pnum);
+ assert(skip_bytes < pnum);
+ }
if (ret <= 0) {
QEMUIOVector local_qiov;
--
2.21.0
^ permalink raw reply related [flat|nested] 5+ messages in thread* [PATCH 2/2] iotests/262: Switch source/dest VM launch order
2019-10-01 17:48 [PATCH 0/2] block: Skip COR for inactive nodes Max Reitz
2019-10-01 17:48 ` [PATCH 1/2] " Max Reitz
@ 2019-10-01 17:48 ` Max Reitz
2019-10-01 19:51 ` [PATCH 0/2] block: Skip COR for inactive nodes Eric Blake
2019-10-04 9:13 ` Stefan Hajnoczi
3 siblings, 0 replies; 5+ messages in thread
From: Max Reitz @ 2019-10-01 17:48 UTC (permalink / raw)
To: qemu-block; +Cc: Kevin Wolf, qemu-devel, Stefan Hajnoczi, Max Reitz
Launching the destination VM before the source VM gives us a regression
test for HEAD^:
The guest device causes a read from the disk image through
guess_disk_lchs(). This will not work if the first sector (containing
the partition table) is yet unallocated, we use COR, and the node is
inactive.
By launching the source VM before the destination, however, the COR
filter on the source will allocate that area in the image shared between
both VMs, thus the problem will not become apparent.
Switching the launch order causes the sector to still be unallocated
when guess_disk_lchs() runs on the inactive node in the destination VM,
and thus we get our test case.
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
tests/qemu-iotests/262 | 12 ++++++------
tests/qemu-iotests/262.out | 6 +++---
2 files changed, 9 insertions(+), 9 deletions(-)
diff --git a/tests/qemu-iotests/262 b/tests/qemu-iotests/262
index 398f63587e..0963daa806 100755
--- a/tests/qemu-iotests/262
+++ b/tests/qemu-iotests/262
@@ -54,12 +54,6 @@ with iotests.FilePath('img') as img_path, \
os.mkfifo(fifo)
- iotests.log('Launching source VM...')
- add_opts(vm_a)
- vm_a.launch()
-
- vm_a.enable_migration_events('A')
-
iotests.log('Launching destination VM...')
add_opts(vm_b)
vm_b.add_incoming("exec: cat '%s'" % (fifo))
@@ -67,6 +61,12 @@ with iotests.FilePath('img') as img_path, \
vm_b.enable_migration_events('B')
+ iotests.log('Launching source VM...')
+ add_opts(vm_a)
+ vm_a.launch()
+
+ vm_a.enable_migration_events('A')
+
iotests.log('Starting migration to B...')
iotests.log(vm_a.qmp('migrate', uri='exec:cat >%s' % (fifo)))
with iotests.Timeout(3, 'Migration does not complete'):
diff --git a/tests/qemu-iotests/262.out b/tests/qemu-iotests/262.out
index 5a58e5e9f8..8e04c496c4 100644
--- a/tests/qemu-iotests/262.out
+++ b/tests/qemu-iotests/262.out
@@ -1,9 +1,9 @@
-Launching source VM...
-Enabling migration QMP events on A...
-{"return": {}}
Launching destination VM...
Enabling migration QMP events on B...
{"return": {}}
+Launching source VM...
+Enabling migration QMP events on A...
+{"return": {}}
Starting migration to B...
{"return": {}}
{"data": {"status": "setup"}, "event": "MIGRATION", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
--
2.21.0
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH 0/2] block: Skip COR for inactive nodes
2019-10-01 17:48 [PATCH 0/2] block: Skip COR for inactive nodes Max Reitz
2019-10-01 17:48 ` [PATCH 1/2] " Max Reitz
2019-10-01 17:48 ` [PATCH 2/2] iotests/262: Switch source/dest VM launch order Max Reitz
@ 2019-10-01 19:51 ` Eric Blake
2019-10-04 9:13 ` Stefan Hajnoczi
3 siblings, 0 replies; 5+ messages in thread
From: Eric Blake @ 2019-10-01 19:51 UTC (permalink / raw)
To: Max Reitz, qemu-block; +Cc: Kevin Wolf, qemu-devel, Stefan Hajnoczi
On 10/1/19 12:48 PM, Max Reitz wrote:
> Hi,
>
> While working on $IMGOPTS support for Python iotests, I noticed a minor
> bug. Let’s fix it, as you do with bugs.
>
>
> Max Reitz (2):
> block: Skip COR for inactive nodes
> iotests/262: Switch source/dest VM launch order
Reviewed-by: Eric Blake <eblake@redhat.com>
>
> block/io.c | 41 +++++++++++++++++++++++++-------------
> tests/qemu-iotests/262 | 12 +++++------
> tests/qemu-iotests/262.out | 6 +++---
> 3 files changed, 36 insertions(+), 23 deletions(-)
>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 0/2] block: Skip COR for inactive nodes
2019-10-01 17:48 [PATCH 0/2] block: Skip COR for inactive nodes Max Reitz
` (2 preceding siblings ...)
2019-10-01 19:51 ` [PATCH 0/2] block: Skip COR for inactive nodes Eric Blake
@ 2019-10-04 9:13 ` Stefan Hajnoczi
3 siblings, 0 replies; 5+ messages in thread
From: Stefan Hajnoczi @ 2019-10-04 9:13 UTC (permalink / raw)
To: Max Reitz; +Cc: Kevin Wolf, qemu-devel, qemu-block
[-- Attachment #1: Type: text/plain, Size: 656 bytes --]
On Tue, Oct 01, 2019 at 07:48:25PM +0200, Max Reitz wrote:
> Hi,
>
> While working on $IMGOPTS support for Python iotests, I noticed a minor
> bug. Let’s fix it, as you do with bugs.
>
>
> Max Reitz (2):
> block: Skip COR for inactive nodes
> iotests/262: Switch source/dest VM launch order
>
> block/io.c | 41 +++++++++++++++++++++++++-------------
> tests/qemu-iotests/262 | 12 +++++------
> tests/qemu-iotests/262.out | 6 +++---
> 3 files changed, 36 insertions(+), 23 deletions(-)
>
> --
> 2.21.0
>
Thanks, applied to my block tree:
https://github.com/stefanha/qemu/commits/block
Stefan
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread