[Qemu-devel] [PATCH 0/3] migration: Call blk_resume_after_migration() for postcopy

[Qemu-devel] [PATCH 0/3] migration: Call blk_resume_after_migration() for postcopy

[Kevin Wolf]

This fixes one of the bugs that made us disable the op blocker assertions for
the 2.9 release.

Kevin Wolf (3):
  migration: Call blk_resume_after_migration() for postcopy
  qemu-iotests: Filter HMP readline escape characters
  qemu-iotests: Test postcopy migration

 migration/savevm.c               |   8 +++
 tests/qemu-iotests/028.out       |   2 +-
 tests/qemu-iotests/130.out       |   4 +-
 tests/qemu-iotests/181           | 117 +++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/181.out       |  38 +++++++++++++
 tests/qemu-iotests/common.filter |   7 +++
 tests/qemu-iotests/common.qemu   |   4 +-
 tests/qemu-iotests/group         |   1 +
 8 files changed, 176 insertions(+), 5 deletions(-)
 create mode 100755 tests/qemu-iotests/181
 create mode 100644 tests/qemu-iotests/181.out

-- 
1.8.3.1

[Qemu-devel] [PATCH 1/3] migration: Call blk_resume_after_migration() for postcopy

[Kevin Wolf]

Commit d35ff5e6 ('block: Ignore guest dev permissions during incoming
migration') added blk_resume_after_migration() to the precopy migration
path, but neglected to add it to the duplicated code that is used for
postcopy migration. This means that the guest device doesn't request the
necessary permissions, which ultimately led to failing assertions.

Add the missing blk_resume_after_migration() to the postcopy path.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 migration/savevm.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/migration/savevm.c b/migration/savevm.c
index 3b19a4a..43fa9bf 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -1623,6 +1623,14 @@ static void loadvm_postcopy_handle_run_bh(void *opaque)
         error_report_err(local_err);
     }
 
+    /* If we get an error here, just don't restart the VM yet. */
+    blk_resume_after_migration(&local_err);
+    if (local_err) {
+        error_free(local_err);
+        local_err = NULL;
+        autostart = false;
+    }
+
     trace_loadvm_postcopy_handle_run_cpu_sync();
     cpu_synchronize_all_post_init();
 
-- 
1.8.3.1

[Qemu-devel] [PATCH 2/3] qemu-iotests: Filter HMP readline escape characters

[Kevin Wolf]

The only thing the escape characters achieve is making the reference
output unreadable and lines that are potentially so long that git
doesn't want to put them into an email any more. Let's filter them out.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 tests/qemu-iotests/028.out       | 2 +-
 tests/qemu-iotests/130.out       | 4 ++--
 tests/qemu-iotests/common.filter | 7 +++++++
 tests/qemu-iotests/common.qemu   | 4 ++--
 4 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/tests/qemu-iotests/028.out b/tests/qemu-iotests/028.out
index acd2870..7d54aeb 100644
--- a/tests/qemu-iotests/028.out
+++ b/tests/qemu-iotests/028.out
@@ -469,7 +469,7 @@ No errors were found on the image.
 block-backup
 
 Formatting 'TEST_DIR/t.IMGFMT.copy', fmt=IMGFMT size=4294968832 backing_file=TEST_DIR/t.IMGFMT.base backing_fmt=IMGFMT
-(qemu) i^[[K^[[Din^[[K^[[D^[[Dinf^[[K^[[D^[[D^[[Dinfo^[[K^[[D^[[D^[[D^[[Dinfo ^[[K^[[D^[[D^[[D^[[D^[[Dinfo b^[[K^[[D^[[D^[[D^[[D^[[D^[[Dinfo bl^[[K^[[D^[[D^[[D^[[D^[[D^[[D^[[Dinfo blo^[[K^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[Dinfo bloc^[[K^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[Dinfo block^[[K^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[Dinfo block-^[[K^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[Dinfo block-j^[[K^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[Dinfo block-jo^[[K^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[Dinfo block-job^[[K^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[Dinfo block-jobs^[[K
+(qemu) info block-jobs
 No active jobs
 === IO: pattern 195
 read 512/512 bytes at offset 3221194240
diff --git a/tests/qemu-iotests/130.out b/tests/qemu-iotests/130.out
index ae95b50..93020c3 100644
--- a/tests/qemu-iotests/130.out
+++ b/tests/qemu-iotests/130.out
@@ -9,14 +9,14 @@ virtual size: 64M (67108864 bytes)
 === HMP commit ===
 
 QEMU X.Y.Z monitor - type 'help' for more information
-(qemu) c^[[K^[[Dco^[[K^[[D^[[Dcom^[[K^[[D^[[D^[[Dcomm^[[K^[[D^[[D^[[D^[[Dcommi^[[K^[[D^[[D^[[D^[[D^[[Dcommit^[[K^[[D^[[D^[[D^[[D^[[D^[[Dcommit ^[[K^[[D^[[D^[[D^[[D^[[D^[[D^[[Dcommit t^[[K^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[Dcommit te^[[K^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[Dcommit tes^[[K^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[Dcommit test^[[K^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[Dcommit testd^[[K^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[Dcommit testdi^[[K^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[Dcommit testdis^[[K^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[Dcommit testdisk^[[K
+(qemu) commit testdisk
 (qemu) 
 image: TEST_DIR/t.IMGFMT
 file format: IMGFMT
 virtual size: 64M (67108864 bytes)
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864 backing_file=TEST_DIR/t.IMGFMT.orig backing_fmt=raw
 QEMU X.Y.Z monitor - type 'help' for more information
-(qemu) c^[[K^[[Dco^[[K^[[D^[[Dcom^[[K^[[D^[[D^[[Dcomm^[[K^[[D^[[D^[[D^[[Dcommi^[[K^[[D^[[D^[[D^[[D^[[Dcommit^[[K^[[D^[[D^[[D^[[D^[[D^[[Dcommit ^[[K^[[D^[[D^[[D^[[D^[[D^[[D^[[Dcommit t^[[K^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[Dcommit te^[[K^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[Dcommit tes^[[K^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[Dcommit test^[[K^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[Dcommit testd^[[K^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[Dcommit testdi^[[K^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[Dcommit testdis^[[K^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[D^[[Dcommit testdisk^[[K
+(qemu) commit testdisk
 (qemu) 
 image: TEST_DIR/t.IMGFMT
 file format: IMGFMT
diff --git a/tests/qemu-iotests/common.filter b/tests/qemu-iotests/common.filter
index 1040013..78b7bfa 100644
--- a/tests/qemu-iotests/common.filter
+++ b/tests/qemu-iotests/common.filter
@@ -86,6 +86,13 @@ _filter_qmp()
         -e '    QMP_VERSION'
 }
 
+# readline makes HMP command strings so long that git complains
+_filter_hmp()
+{
+    sed -e 's/(qemu).*\o33\[D/(qemu) /g' \
+        -e 's/\o33\[K//g'
+}
+
 # replace block job offset
 _filter_block_job_offset()
 {
diff --git a/tests/qemu-iotests/common.qemu b/tests/qemu-iotests/common.qemu
index 4278789..7a78a00 100644
--- a/tests/qemu-iotests/common.qemu
+++ b/tests/qemu-iotests/common.qemu
@@ -59,7 +59,7 @@ function _timed_wait_for()
     do
         if [ -z "${silent}" ]; then
             echo "${resp}" | _filter_testdir | _filter_qemu \
-                           | _filter_qemu_io | _filter_qmp
+                           | _filter_qemu_io | _filter_qmp | _filter_hmp
         fi
         grep -q "${*}" < <(echo ${resp})
         if [ $? -eq 0 ]; then
@@ -217,7 +217,7 @@ function _cleanup_qemu()
 
         if [ -n "${wait}" ]; then
             cat <&${QEMU_OUT[$i]} | _filter_testdir | _filter_qemu \
-                                  | _filter_qemu_io | _filter_qmp
+                                  | _filter_qemu_io | _filter_qmp | _filter_hmp
         fi
         rm -f "${QEMU_FIFO_IN}_${i}" "${QEMU_FIFO_OUT}_${i}"
         eval "exec ${QEMU_IN[$i]}<&-"   # close file descriptors
-- 
1.8.3.1

[Qemu-devel] [PATCH 3/3] qemu-iotests: Test postcopy migration

[Kevin Wolf]

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 tests/qemu-iotests/181     | 117 +++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/181.out |  38 +++++++++++++++
 tests/qemu-iotests/group   |   1 +
 3 files changed, 156 insertions(+)
 create mode 100755 tests/qemu-iotests/181
 create mode 100644 tests/qemu-iotests/181.out

diff --git a/tests/qemu-iotests/181 b/tests/qemu-iotests/181
new file mode 100755
index 0000000..563aee7
--- /dev/null
+++ b/tests/qemu-iotests/181
@@ -0,0 +1,117 @@
+#!/bin/bash
+#
+# Test postcopy live migration with shared storage
+#
+# Copyright (C) 2017 Red Hat, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+
+# creator
+owner=kwolf@redhat.com
+
+seq=`basename $0`
+echo "QA output created by $seq"
+
+here=`pwd`
+status=1	# failure is the default!
+
+MIG_SOCKET="${TEST_DIR}/migrate"
+
+_cleanup()
+{
+    rm -f "${MIG_SOCKET}"
+	_cleanup_test_img
+    _cleanup_qemu
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+. ./common.qemu
+
+_supported_fmt generic
+_supported_proto generic
+_supported_os Linux
+
+size=64M
+_make_test_img $size
+
+echo
+echo === Starting VMs ===
+echo
+
+qemu_comm_method="monitor"
+
+_launch_qemu -drive file="${TEST_IMG}",cache=${CACHEMODE},id=disk
+src=$QEMU_HANDLE
+
+_launch_qemu -drive file="${TEST_IMG}",cache=${CACHEMODE},id=disk \
+             -incoming "unix:${MIG_SOCKET}"
+dest=$QEMU_HANDLE
+
+echo
+echo === Write something on the source ===
+echo
+
+silent=
+_send_qemu_cmd $src 'qemu-io disk "write -P 0x55 0 64k"' "(qemu)"
+_send_qemu_cmd $src "" "ops/sec"
+_send_qemu_cmd $src 'qemu-io disk "read -P 0x55 0 64k"' "(qemu)"
+_send_qemu_cmd $src "" "ops/sec"
+
+echo
+echo === Do postcopy migration to destination ===
+echo
+
+# Slow down migration so much that it definitely won't finish before we can
+# switch to postcopy
+silent=yes
+_send_qemu_cmd $src 'migrate_set_speed 4k' "(qemu)"
+_send_qemu_cmd $src 'migrate_set_capability postcopy-ram on' "(qemu)"
+_send_qemu_cmd $src "migrate -d unix:${MIG_SOCKET}" "(qemu)"
+_send_qemu_cmd $src 'migrate_start_postcopy' "(qemu)"
+
+QEMU_COMM_TIMEOUT=1 qemu_cmd_repeat=10 silent=yes \
+    _send_qemu_cmd $src "info migrate" "completed\|failed"
+silent=yes _send_qemu_cmd $src "" "(qemu)"
+
+echo
+echo === Do some I/O on the destination ===
+echo
+
+# It is important that we use the BlockBackend of the guest device here instead
+# of the node name, which would create a new BlockBackend and not test whether
+# the guest has the necessary permissions to access the image now
+silent=
+_send_qemu_cmd $dest 'qemu-io disk "read -P 0x55 0 64k"' "(qemu)"
+_send_qemu_cmd $dest "" "ops/sec"
+_send_qemu_cmd $dest 'qemu-io disk "write -P 0x66 1M 64k"' "(qemu)"
+_send_qemu_cmd $dest "" "ops/sec"
+
+echo
+echo === Shut down and check image ===
+echo
+
+_send_qemu_cmd $src 'quit' ""
+_send_qemu_cmd $dest 'quit' ""
+wait=1 _cleanup_qemu
+
+_check_test_img
+
+# success, all done
+echo "*** done"
+rm -f $seq.full
+status=0
diff --git a/tests/qemu-iotests/181.out b/tests/qemu-iotests/181.out
new file mode 100644
index 0000000..6534ba2
--- /dev/null
+++ b/tests/qemu-iotests/181.out
@@ -0,0 +1,38 @@
+QA output created by 181
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
+
+=== Starting VMs ===
+
+
+=== Write something on the source ===
+
+QEMU X.Y.Z monitor - type 'help' for more information
+(qemu) qemu-io disk "write -P 0x55 0 64k"
+wrote 65536/65536 bytes at offset 0
+64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+(qemu) 
+(qemu) qemu-io disk "read -P 0x55 0 64k"
+read 65536/65536 bytes at offset 0
+64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+=== Do postcopy migration to destination ===
+
+
+=== Do some I/O on the destination ===
+
+QEMU X.Y.Z monitor - type 'help' for more information
+(qemu) qemu-io disk "read -P 0x55 0 64k"
+read 65536/65536 bytes at offset 0
+64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+(qemu) 
+(qemu) qemu-io disk "write -P 0x66 1M 64k"
+wrote 65536/65536 bytes at offset 1048576
+64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+=== Shut down and check image ===
+
+(qemu) quit
+(qemu) 
+(qemu) quit
+No errors were found on the image.
+*** done
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index 43142dd..893962d 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -169,3 +169,4 @@
 174 auto
 175 auto quick
 176 rw auto backing
+181 rw auto migration
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 2/3] qemu-iotests: Filter HMP readline escape characters

[Eric Blake]

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

On 04/13/2017 12:23 PM, Kevin Wolf wrote:
> The only thing the escape characters achieve is making the reference
> output unreadable and lines that are potentially so long that git
> doesn't want to put them into an email any more. Let's filter them out.

Hear! Hear!

> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  tests/qemu-iotests/028.out       | 2 +-
>  tests/qemu-iotests/130.out       | 4 ++--
>  tests/qemu-iotests/common.filter | 7 +++++++
>  tests/qemu-iotests/common.qemu   | 4 ++--
>  4 files changed, 12 insertions(+), 5 deletions(-)

Yay!  But I think you're incomplete:

What about 051, 068, 142, and 145?

(found by:
 git grep -l $'\e' tests/qemu-iotests/*.out
using bash as the shell)


> +++ b/tests/qemu-iotests/common.filter
> @@ -86,6 +86,13 @@ _filter_qmp()
>          -e '    QMP_VERSION'
>  }
>  
> +# readline makes HMP command strings so long that git complains
> +_filter_hmp()
> +{
> +    sed -e 's/(qemu).*\o33\[D/(qemu) /g' \

\oNN is a GNU sed-ism, as far as I can tell.  Is it portable to our
BSD/MacOS builds?

> +        -e 's/\o33\[K//g'
> +}


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


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

Re: [Qemu-devel] [PATCH 1/3] migration: Call blk_resume_after_migration() for postcopy

[Eric Blake]

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

On 04/13/2017 12:23 PM, Kevin Wolf wrote:
> Commit d35ff5e6 ('block: Ignore guest dev permissions during incoming
> migration') added blk_resume_after_migration() to the precopy migration
> path, but neglected to add it to the duplicated code that is used for
> postcopy migration. This means that the guest device doesn't request the
> necessary permissions, which ultimately led to failing assertions.
> 
> Add the missing blk_resume_after_migration() to the postcopy path.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  migration/savevm.c | 8 ++++++++
>  1 file changed, 8 insertions(+)

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

Are we targetting this for 2.9-rc5, or is it 2.10 material?

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


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

Re: [Qemu-devel] [PATCH 2/3] qemu-iotests: Filter HMP readline escape characters

[Eric Blake]

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

On 04/13/2017 12:38 PM, Eric Blake wrote:
> On 04/13/2017 12:23 PM, Kevin Wolf wrote:
>> The only thing the escape characters achieve is making the reference
>> output unreadable and lines that are potentially so long that git
>> doesn't want to put them into an email any more. Let's filter them out.
> 
> Hear! Hear!
> 

>> +++ b/tests/qemu-iotests/common.filter
>> @@ -86,6 +86,13 @@ _filter_qmp()
>>          -e '    QMP_VERSION'
>>  }
>>  
>> +# readline makes HMP command strings so long that git complains
>> +_filter_hmp()
>> +{
>> +    sed -e 's/(qemu).*\o33\[D/(qemu) /g' \
> 
> \oNN is a GNU sed-ism, as far as I can tell.  Is it portable to our
> BSD/MacOS builds?

And if it's not, qemu-iotests are running under bash, so writing this as:

sed -e $'s/(qemu).*\e\\[D/(qemu) /g'

> 
>> +        -e 's/\o33\[K//g'

-e $'s/\e\\[K//g'

is a reliable alternative.

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


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

Re: [Qemu-devel] [PATCH 1/3] migration: Call blk_resume_after_migration() for postcopy

[Kevin Wolf]

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

Am 13.04.2017 um 19:39 hat Eric Blake geschrieben:
> On 04/13/2017 12:23 PM, Kevin Wolf wrote:
> > Commit d35ff5e6 ('block: Ignore guest dev permissions during incoming
> > migration') added blk_resume_after_migration() to the precopy migration
> > path, but neglected to add it to the duplicated code that is used for
> > postcopy migration. This means that the guest device doesn't request the
> > necessary permissions, which ultimately led to failing assertions.
> > 
> > Add the missing blk_resume_after_migration() to the postcopy path.
> > 
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> >  migration/savevm.c | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>
> 
> Are we targetting this for 2.9-rc5, or is it 2.10 material?

At this point, I think it's clearly 2.10.

Kevin

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [Qemu-devel] [PATCH 1/3] migration: Call blk_resume_after_migration() for postcopy

[Eric Blake]

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

On 04/13/2017 12:54 PM, Kevin Wolf wrote:
> Am 13.04.2017 um 19:39 hat Eric Blake geschrieben:
>> On 04/13/2017 12:23 PM, Kevin Wolf wrote:
>>> Commit d35ff5e6 ('block: Ignore guest dev permissions during incoming
>>> migration') added blk_resume_after_migration() to the precopy migration
>>> path, but neglected to add it to the duplicated code that is used for
>>> postcopy migration. This means that the guest device doesn't request the
>>> necessary permissions, which ultimately led to failing assertions.
>>>
>>> Add the missing blk_resume_after_migration() to the postcopy path.
>>>
>>> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>>> ---
>>>  migration/savevm.c | 8 ++++++++
>>>  1 file changed, 8 insertions(+)
>>
>> Reviewed-by: Eric Blake <eblake@redhat.com>
>>
>> Are we targetting this for 2.9-rc5, or is it 2.10 material?
> 
> At this point, I think it's clearly 2.10.

Okay.  Restating, to make sure I got your reasoning: the removed
assertions of commit e3e0003 imply that 2.9 is not regressing in
behavior, and at this point the worst the code can do without this patch
applied is behave like it's done previously; therefore this patch is not
fixing an observable 2.9 behavior and therefore not worth holding up the
release.

But for 2.10, it's absolutely essential, as we have another patch
pending to revert e3e0003 at which point we have a behavior break
without this patch.

Works for me.

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


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

Re: [Qemu-devel] [PATCH 1/3] migration: Call blk_resume_after_migration() for postcopy

[Kevin Wolf]

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

Am 13.04.2017 um 20:03 hat Eric Blake geschrieben:
> On 04/13/2017 12:54 PM, Kevin Wolf wrote:
> > Am 13.04.2017 um 19:39 hat Eric Blake geschrieben:
> >> On 04/13/2017 12:23 PM, Kevin Wolf wrote:
> >>> Commit d35ff5e6 ('block: Ignore guest dev permissions during incoming
> >>> migration') added blk_resume_after_migration() to the precopy migration
> >>> path, but neglected to add it to the duplicated code that is used for
> >>> postcopy migration. This means that the guest device doesn't request the
> >>> necessary permissions, which ultimately led to failing assertions.
> >>>
> >>> Add the missing blk_resume_after_migration() to the postcopy path.
> >>>
> >>> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> >>> ---
> >>>  migration/savevm.c | 8 ++++++++
> >>>  1 file changed, 8 insertions(+)
> >>
> >> Reviewed-by: Eric Blake <eblake@redhat.com>
> >>
> >> Are we targetting this for 2.9-rc5, or is it 2.10 material?
> > 
> > At this point, I think it's clearly 2.10.
> 
> Okay.  Restating, to make sure I got your reasoning: the removed
> assertions of commit e3e0003 imply that 2.9 is not regressing in
> behavior, and at this point the worst the code can do without this patch
> applied is behave like it's done previously; therefore this patch is not
> fixing an observable 2.9 behavior and therefore not worth holding up the
> release.

Right, basically the new op blockers become ineffective for guest
devices after postcopy migration.

> But for 2.10, it's absolutely essential, as we have another patch
> pending to revert e3e0003 at which point we have a behavior break
> without this patch.

Correct.

Kevin

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [Qemu-devel] [PATCH 3/3] qemu-iotests: Test postcopy migration

[Eric Blake]

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

On 04/13/2017 12:23 PM, Kevin Wolf wrote:
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  tests/qemu-iotests/181     | 117 +++++++++++++++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/181.out |  38 +++++++++++++++
>  tests/qemu-iotests/group   |   1 +
>  3 files changed, 156 insertions(+)
>  create mode 100755 tests/qemu-iotests/181
>  create mode 100644 tests/qemu-iotests/181.out
> 

I don't see a 180.out mentioned on-list anywhere (but the choice of test
number is not too serious).

./check -raw 181

 === Write something on the source ===

+WARNING: Image format was not specified for 'TEST_DIR/t.raw' and
probing guessed raw.
+Automatically detecting the format is dangerous for raw images, write
operations on block 0 will be restricted.
+Specify the 'raw' format explicitly to remove the restrictions.


You'll want to fix that.


> +# Slow down migration so much that it definitely won't finish before we can
> +# switch to postcopy
> +silent=yes
> +_send_qemu_cmd $src 'migrate_set_speed 4k' "(qemu)"
> +_send_qemu_cmd $src 'migrate_set_capability postcopy-ram on' "(qemu)"
> +_send_qemu_cmd $src "migrate -d unix:${MIG_SOCKET}" "(qemu)"
> +_send_qemu_cmd $src 'migrate_start_postcopy' "(qemu)"
> +
> +QEMU_COMM_TIMEOUT=1 qemu_cmd_repeat=10 silent=yes \
> +    _send_qemu_cmd $src "info migrate" "completed\|failed"
> +silent=yes _send_qemu_cmd $src "" "(qemu)"

Do we want to tell the source to go back to a higher migrate_set_speed
once we know we are in postcopy mode?  But the test passed for me under
-qcow2 in less than 2 seconds, on an unloaded machine, so hopefully even
on a heavily loaded machine we aren't giving up due to timeouts.


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


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

Re: [Qemu-devel] [PATCH 3/3] qemu-iotests: Test postcopy migration

[Dr. David Alan Gilbert]

* Kevin Wolf (kwolf@redhat.com) wrote:
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  tests/qemu-iotests/181     | 117 +++++++++++++++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/181.out |  38 +++++++++++++++
>  tests/qemu-iotests/group   |   1 +
>  3 files changed, 156 insertions(+)
>  create mode 100755 tests/qemu-iotests/181
>  create mode 100644 tests/qemu-iotests/181.out
> 
> diff --git a/tests/qemu-iotests/181 b/tests/qemu-iotests/181
> new file mode 100755
> index 0000000..563aee7
> --- /dev/null
> +++ b/tests/qemu-iotests/181
> @@ -0,0 +1,117 @@
> +#!/bin/bash
> +#
> +# Test postcopy live migration with shared storage
> +#
> +# Copyright (C) 2017 Red Hat, Inc.
> +#
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 2 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +#
> +
> +# creator
> +owner=kwolf@redhat.com
> +
> +seq=`basename $0`
> +echo "QA output created by $seq"
> +
> +here=`pwd`
> +status=1	# failure is the default!
> +
> +MIG_SOCKET="${TEST_DIR}/migrate"
> +
> +_cleanup()
> +{
> +    rm -f "${MIG_SOCKET}"
> +	_cleanup_test_img
> +    _cleanup_qemu
> +}
> +trap "_cleanup; exit \$status" 0 1 2 3 15
> +
> +# get standard environment, filters and checks
> +. ./common.rc
> +. ./common.filter
> +. ./common.qemu
> +
> +_supported_fmt generic
> +_supported_proto generic
> +_supported_os Linux
> +
> +size=64M
> +_make_test_img $size
> +
> +echo
> +echo === Starting VMs ===
> +echo
> +
> +qemu_comm_method="monitor"
> +
> +_launch_qemu -drive file="${TEST_IMG}",cache=${CACHEMODE},id=disk
> +src=$QEMU_HANDLE
> +
> +_launch_qemu -drive file="${TEST_IMG}",cache=${CACHEMODE},id=disk \
> +             -incoming "unix:${MIG_SOCKET}"
> +dest=$QEMU_HANDLE
> +
> +echo
> +echo === Write something on the source ===
> +echo
> +
> +silent=
> +_send_qemu_cmd $src 'qemu-io disk "write -P 0x55 0 64k"' "(qemu)"
> +_send_qemu_cmd $src "" "ops/sec"
> +_send_qemu_cmd $src 'qemu-io disk "read -P 0x55 0 64k"' "(qemu)"
> +_send_qemu_cmd $src "" "ops/sec"
> +
> +echo
> +echo === Do postcopy migration to destination ===
> +echo
> +
> +# Slow down migration so much that it definitely won't finish before we can
> +# switch to postcopy
> +silent=yes
> +_send_qemu_cmd $src 'migrate_set_speed 4k' "(qemu)"
> +_send_qemu_cmd $src 'migrate_set_capability postcopy-ram on' "(qemu)"

You should probably also set this on the destination as well;  we
do it in pretty much all the places where we use postcopy, but I think
it'll work without it.

However,

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> +_send_qemu_cmd $src "migrate -d unix:${MIG_SOCKET}" "(qemu)"
> +_send_qemu_cmd $src 'migrate_start_postcopy' "(qemu)"
> +
> +QEMU_COMM_TIMEOUT=1 qemu_cmd_repeat=10 silent=yes \
> +    _send_qemu_cmd $src "info migrate" "completed\|failed"
> +silent=yes _send_qemu_cmd $src "" "(qemu)"
> +
> +echo
> +echo === Do some I/O on the destination ===
> +echo
> +
> +# It is important that we use the BlockBackend of the guest device here instead
> +# of the node name, which would create a new BlockBackend and not test whether
> +# the guest has the necessary permissions to access the image now
> +silent=
> +_send_qemu_cmd $dest 'qemu-io disk "read -P 0x55 0 64k"' "(qemu)"
> +_send_qemu_cmd $dest "" "ops/sec"
> +_send_qemu_cmd $dest 'qemu-io disk "write -P 0x66 1M 64k"' "(qemu)"
> +_send_qemu_cmd $dest "" "ops/sec"
> +
> +echo
> +echo === Shut down and check image ===
> +echo
> +
> +_send_qemu_cmd $src 'quit' ""
> +_send_qemu_cmd $dest 'quit' ""
> +wait=1 _cleanup_qemu
> +
> +_check_test_img
> +
> +# success, all done
> +echo "*** done"
> +rm -f $seq.full
> +status=0
> diff --git a/tests/qemu-iotests/181.out b/tests/qemu-iotests/181.out
> new file mode 100644
> index 0000000..6534ba2
> --- /dev/null
> +++ b/tests/qemu-iotests/181.out
> @@ -0,0 +1,38 @@
> +QA output created by 181
> +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
> +
> +=== Starting VMs ===
> +
> +
> +=== Write something on the source ===
> +
> +QEMU X.Y.Z monitor - type 'help' for more information
> +(qemu) qemu-io disk "write -P 0x55 0 64k"
> +wrote 65536/65536 bytes at offset 0
> +64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> +(qemu) 
> +(qemu) qemu-io disk "read -P 0x55 0 64k"
> +read 65536/65536 bytes at offset 0
> +64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> +
> +=== Do postcopy migration to destination ===
> +
> +
> +=== Do some I/O on the destination ===
> +
> +QEMU X.Y.Z monitor - type 'help' for more information
> +(qemu) qemu-io disk "read -P 0x55 0 64k"
> +read 65536/65536 bytes at offset 0
> +64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> +(qemu) 
> +(qemu) qemu-io disk "write -P 0x66 1M 64k"
> +wrote 65536/65536 bytes at offset 1048576
> +64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> +
> +=== Shut down and check image ===
> +
> +(qemu) quit
> +(qemu) 
> +(qemu) quit
> +No errors were found on the image.
> +*** done
> diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
> index 43142dd..893962d 100644
> --- a/tests/qemu-iotests/group
> +++ b/tests/qemu-iotests/group
> @@ -169,3 +169,4 @@
>  174 auto
>  175 auto quick
>  176 rw auto backing
> +181 rw auto migration
> -- 
> 1.8.3.1
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH 3/3] qemu-iotests: Test postcopy migration

[Kevin Wolf]

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

Am 13.04.2017 um 20:44 hat Eric Blake geschrieben:
> On 04/13/2017 12:23 PM, Kevin Wolf wrote:
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> >  tests/qemu-iotests/181     | 117 +++++++++++++++++++++++++++++++++++++++++++++
> >  tests/qemu-iotests/181.out |  38 +++++++++++++++
> >  tests/qemu-iotests/group   |   1 +
> >  3 files changed, 156 insertions(+)
> >  create mode 100755 tests/qemu-iotests/181
> >  create mode 100644 tests/qemu-iotests/181.out
> > 
> 
> I don't see a 180.out mentioned on-list anywhere (but the choice of test
> number is not too serious).

It's used by this one: [PATCH 00/18] nbd: BLOCK_STATUS

> ./check -raw 181
> 
>  === Write something on the source ===
> 
> +WARNING: Image format was not specified for 'TEST_DIR/t.raw' and
> probing guessed raw.
> +Automatically detecting the format is dangerous for raw images, write
> operations on block 0 will be restricted.
> +Specify the 'raw' format explicitly to remove the restrictions.
> 
> 
> You'll want to fix that.

Thanks for catching this, will fix.

> > +# Slow down migration so much that it definitely won't finish before we can
> > +# switch to postcopy
> > +silent=yes
> > +_send_qemu_cmd $src 'migrate_set_speed 4k' "(qemu)"
> > +_send_qemu_cmd $src 'migrate_set_capability postcopy-ram on' "(qemu)"
> > +_send_qemu_cmd $src "migrate -d unix:${MIG_SOCKET}" "(qemu)"
> > +_send_qemu_cmd $src 'migrate_start_postcopy' "(qemu)"
> > +
> > +QEMU_COMM_TIMEOUT=1 qemu_cmd_repeat=10 silent=yes \
> > +    _send_qemu_cmd $src "info migrate" "completed\|failed"
> > +silent=yes _send_qemu_cmd $src "" "(qemu)"
> 
> Do we want to tell the source to go back to a higher migrate_set_speed
> once we know we are in postcopy mode?  But the test passed for me under
> -qcow2 in less than 2 seconds, on an unloaded machine, so hopefully even
> on a heavily loaded machine we aren't giving up due to timeouts.

After switching to postcopy mode, the limits aren't applied any more
anyway (because the running VM is actively waiting for the pages and we
don't want to stall it artificially).

Kevin

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [Qemu-devel] [PATCH 2/3] qemu-iotests: Filter HMP readline escape characters

[Kevin Wolf]

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

Am 13.04.2017 um 19:38 hat Eric Blake geschrieben:
> On 04/13/2017 12:23 PM, Kevin Wolf wrote:
> > The only thing the escape characters achieve is making the reference
> > output unreadable and lines that are potentially so long that git
> > doesn't want to put them into an email any more. Let's filter them out.
> 
> Hear! Hear!
> 
> > 
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> >  tests/qemu-iotests/028.out       | 2 +-
> >  tests/qemu-iotests/130.out       | 4 ++--
> >  tests/qemu-iotests/common.filter | 7 +++++++
> >  tests/qemu-iotests/common.qemu   | 4 ++--
> >  4 files changed, 12 insertions(+), 5 deletions(-)
> 
> Yay!  But I think you're incomplete:
> 
> What about 051, 068, 142, and 145?

I only converted the cases that use common.qemu, so they failed after
my change. The other test cases you found are running qemu manually and
didn't apply the filter yet, so they wouldn't fail.

For v2, I'll add the new filter to them and change the reference output
accordingly.

> (found by:
>  git grep -l $'\e' tests/qemu-iotests/*.out
> using bash as the shell)
> 
> 
> > +++ b/tests/qemu-iotests/common.filter
> > @@ -86,6 +86,13 @@ _filter_qmp()
> >          -e '    QMP_VERSION'
> >  }
> >  
> > +# readline makes HMP command strings so long that git complains
> > +_filter_hmp()
> > +{
> > +    sed -e 's/(qemu).*\o33\[D/(qemu) /g' \
> 
> \oNN is a GNU sed-ism, as far as I can tell.  Is it portable to our
> BSD/MacOS builds?

I'll use your bash version in v2.

Kevin

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]