[Qemu-devel] [PATCH 0/3] commit: Fix completion with extra reference

[Qemu-devel] [PATCH 0/3] commit: Fix completion with extra reference

[Kevin Wolf]

Kevin Wolf (3):
  commit: Fix completion with extra reference
  qemu-iotests: Allow starting new qemu after cleanup
  qemu-iotests: Test exiting qemu with running job

 block/commit.c                 |   7 ++
 tests/qemu-iotests/185         | 189 +++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/185.out     |  59 +++++++++++++
 tests/qemu-iotests/common.qemu |   3 +
 tests/qemu-iotests/group       |   1 +
 5 files changed, 259 insertions(+)
 create mode 100755 tests/qemu-iotests/185
 create mode 100644 tests/qemu-iotests/185.out

-- 
1.8.3.1

[Qemu-devel] [PATCH 1/3] commit: Fix completion with extra reference

[Kevin Wolf]

commit_complete() can't assume that after its block_job_completed() the
job is actually immediately freed; someone else may still be holding
references. In this case, the op blockers on the intermediate nodes make
the graph reconfiguration in the completion code fail.

Call block_job_remove_all_bdrv() manually so that we know for sure that
any blockers on intermediate nodes are given up.

Cc: qemu-stable@nongnu.org
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/commit.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/block/commit.c b/block/commit.c
index af6fa68..8c09c3d 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -119,6 +119,13 @@ static void commit_complete(BlockJob *job, void *opaque)
     }
     g_free(s->backing_file_str);
     blk_unref(s->top);
+
+    /* If there is more than one reference to the job (e.g. if called from
+     * block_job_finish_sync()), block_job_completed() won't free it and
+     * therefore the blockers on the intermediate nodes remain. This would
+     * cause bdrv_set_backing_hd() to fail. */
+    block_job_remove_all_bdrv(job);
+
     block_job_completed(&s->common, ret);
     g_free(data);
 
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 1/3] commit: Fix completion with extra reference

[Eric Blake]

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

On 06/09/2017 06:50 AM, Kevin Wolf wrote:
> commit_complete() can't assume that after its block_job_completed() the
> job is actually immediately freed; someone else may still be holding
> references. In this case, the op blockers on the intermediate nodes make
> the graph reconfiguration in the completion code fail.
> 
> Call block_job_remove_all_bdrv() manually so that we know for sure that
> any blockers on intermediate nodes are given up.
> 
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/commit.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 

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

-- 
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] [Qemu-block] [PATCH 1/3] commit: Fix completion with extra reference

[Max Reitz]

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

On 2017-06-09 13:50, Kevin Wolf wrote:
> commit_complete() can't assume that after its block_job_completed() the
> job is actually immediately freed; someone else may still be holding
> references. In this case, the op blockers on the intermediate nodes make
> the graph reconfiguration in the completion code fail.
> 
> Call block_job_remove_all_bdrv() manually so that we know for sure that
> any blockers on intermediate nodes are given up.
> 
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/commit.c | 7 +++++++
>  1 file changed, 7 insertions(+)

Reviewed-by: Max Reitz <mreitz@redhat.com>


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

[Qemu-devel] [PATCH 2/3] qemu-iotests: Allow starting new qemu after cleanup

[Kevin Wolf]

After _cleanup_qemu(), test cases should be able to start the next qemu
process and call _cleanup_qemu() for that one as well. For this to work
cleanly, we need to improve the cleanup so that the second invocation
doesn't try to kill the qemu instances from the first invocation a
second time (which would result in error messages).

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 tests/qemu-iotests/common.qemu | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/tests/qemu-iotests/common.qemu b/tests/qemu-iotests/common.qemu
index 7a78a00..76ef298 100644
--- a/tests/qemu-iotests/common.qemu
+++ b/tests/qemu-iotests/common.qemu
@@ -222,5 +222,8 @@ function _cleanup_qemu()
         rm -f "${QEMU_FIFO_IN}_${i}" "${QEMU_FIFO_OUT}_${i}"
         eval "exec ${QEMU_IN[$i]}<&-"   # close file descriptors
         eval "exec ${QEMU_OUT[$i]}<&-"
+
+        unset QEMU_IN[$i]
+        unset QEMU_OUT[$i]
     done
 }
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 2/3] qemu-iotests: Allow starting new qemu after cleanup

[Eric Blake]

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

On 06/09/2017 06:50 AM, Kevin Wolf wrote:
> After _cleanup_qemu(), test cases should be able to start the next qemu
> process and call _cleanup_qemu() for that one as well. For this to work
> cleanly, we need to improve the cleanup so that the second invocation
> doesn't try to kill the qemu instances from the first invocation a
> second time (which would result in error messages).

Yeah, idempotency is a nice thing.

> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  tests/qemu-iotests/common.qemu | 3 +++
>  1 file changed, 3 insertions(+)
> 

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

> diff --git a/tests/qemu-iotests/common.qemu b/tests/qemu-iotests/common.qemu
> index 7a78a00..76ef298 100644
> --- a/tests/qemu-iotests/common.qemu
> +++ b/tests/qemu-iotests/common.qemu
> @@ -222,5 +222,8 @@ function _cleanup_qemu()
>          rm -f "${QEMU_FIFO_IN}_${i}" "${QEMU_FIFO_OUT}_${i}"
>          eval "exec ${QEMU_IN[$i]}<&-"   # close file descriptors
>          eval "exec ${QEMU_OUT[$i]}<&-"
> +
> +        unset QEMU_IN[$i]
> +        unset QEMU_OUT[$i]
>      done
>  }
> 

-- 
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] [Qemu-block] [PATCH 2/3] qemu-iotests: Allow starting new qemu after cleanup

[Max Reitz]

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

On 2017-06-09 13:50, Kevin Wolf wrote:
> After _cleanup_qemu(), test cases should be able to start the next qemu
> process and call _cleanup_qemu() for that one as well. For this to work
> cleanly, we need to improve the cleanup so that the second invocation
> doesn't try to kill the qemu instances from the first invocation a
> second time (which would result in error messages).
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  tests/qemu-iotests/common.qemu | 3 +++
>  1 file changed, 3 insertions(+)

Reviewed-by: Max Reitz <mreitz@redhat.com>


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

[Qemu-devel] [PATCH 3/3] qemu-iotests: Test exiting qemu with running job

[Kevin Wolf]

When qemu is exited, all running jobs should be cancelled successfully.
This adds a test for this for all types of block jobs that currently
exist in qemu.

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

diff --git a/tests/qemu-iotests/185 b/tests/qemu-iotests/185
new file mode 100755
index 0000000..645ec9a
--- /dev/null
+++ b/tests/qemu-iotests/185
@@ -0,0 +1,189 @@
+#!/bin/bash
+#
+# Test exiting qemu while jobs are still running
+#
+# 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 "${TEST_IMG}.mid"
+    rm -f "${TEST_IMG}.copy"
+    _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 qcow2 raw
+_supported_proto file
+_supported_os Linux
+
+size=64M
+TEST_IMG="${TEST_IMG}.base" _make_test_img $size
+
+echo
+echo === Starting VM ===
+echo
+
+qemu_comm_method="qmp"
+
+_launch_qemu \
+    -drive file="${TEST_IMG}.base",cache=$CACHEMODE,driver=$IMGFMT,id=disk
+h=$QEMU_HANDLE
+_send_qemu_cmd $h "{ 'execute': 'qmp_capabilities' }" 'return'
+
+echo
+echo === Creating backing chain ===
+echo
+
+_send_qemu_cmd $h \
+    "{ 'execute': 'blockdev-snapshot-sync',
+       'arguments': { 'device': 'disk',
+                      'snapshot-file': '$TEST_IMG.mid',
+                      'format': '$IMGFMT',
+                      'mode': 'absolute-paths' } }" \
+    "return"
+
+_send_qemu_cmd $h \
+    "{ 'execute': 'human-monitor-command',
+       'arguments': { 'command-line':
+                      'qemu-io disk \"write 0 4M\"' } }" \
+    "return"
+
+_send_qemu_cmd $h \
+    "{ 'execute': 'blockdev-snapshot-sync',
+       'arguments': { 'device': 'disk',
+                      'snapshot-file': '$TEST_IMG',
+                      'format': '$IMGFMT',
+                      'mode': 'absolute-paths' } }" \
+    "return"
+
+echo
+echo === Start commit job and exit qemu ===
+echo
+
+_send_qemu_cmd $h \
+    "{ 'execute': 'block-commit',
+       'arguments': { 'device': 'disk',
+                      'base':'$TEST_IMG.base',
+                      'top': '$TEST_IMG.mid',
+                      'speed': 65536 } }" \
+    "return"
+
+_send_qemu_cmd $h "{ 'execute': 'quit' }" "return"
+wait=1 _cleanup_qemu
+
+echo
+echo === Start active commit job and exit qemu ===
+echo
+
+_launch_qemu \
+    -drive file="${TEST_IMG}",cache=$CACHEMODE,driver=$IMGFMT,id=disk
+h=$QEMU_HANDLE
+_send_qemu_cmd $h "{ 'execute': 'qmp_capabilities' }" 'return'
+
+_send_qemu_cmd $h \
+    "{ 'execute': 'block-commit',
+       'arguments': { 'device': 'disk',
+                      'base':'$TEST_IMG.base',
+                      'speed': 65536 } }" \
+    "return"
+
+_send_qemu_cmd $h "{ 'execute': 'quit' }" "return"
+wait=1 _cleanup_qemu
+
+echo
+echo === Start mirror job and exit qemu ===
+echo
+
+_launch_qemu \
+    -drive file="${TEST_IMG}",cache=$CACHEMODE,driver=$IMGFMT,id=disk
+h=$QEMU_HANDLE
+_send_qemu_cmd $h "{ 'execute': 'qmp_capabilities' }" 'return'
+
+_send_qemu_cmd $h \
+    "{ 'execute': 'drive-mirror',
+       'arguments': { 'device': 'disk',
+                      'target': '$TEST_IMG.copy',
+                      'format': '$IMGFMT',
+                      'sync': 'full',
+                      'speed': 65536 } }" \
+    "return"
+
+_send_qemu_cmd $h "{ 'execute': 'quit' }" "return"
+wait=1 _cleanup_qemu
+
+echo
+echo === Start backup job and exit qemu ===
+echo
+
+_launch_qemu \
+    -drive file="${TEST_IMG}",cache=$CACHEMODE,driver=$IMGFMT,id=disk
+h=$QEMU_HANDLE
+_send_qemu_cmd $h "{ 'execute': 'qmp_capabilities' }" 'return'
+
+_send_qemu_cmd $h \
+    "{ 'execute': 'drive-backup',
+       'arguments': { 'device': 'disk',
+                      'target': '$TEST_IMG.copy',
+                      'format': '$IMGFMT',
+                      'sync': 'full',
+                      'speed': 65536 } }" \
+    "return"
+
+_send_qemu_cmd $h "{ 'execute': 'quit' }" "return"
+wait=1 _cleanup_qemu
+
+echo
+echo === Start streaming job and exit qemu ===
+echo
+
+_launch_qemu \
+    -drive file="${TEST_IMG}",cache=$CACHEMODE,driver=$IMGFMT,id=disk
+h=$QEMU_HANDLE
+_send_qemu_cmd $h "{ 'execute': 'qmp_capabilities' }" 'return'
+
+_send_qemu_cmd $h \
+    "{ 'execute': 'block-stream',
+       'arguments': { 'device': 'disk',
+                      'speed': 65536 } }" \
+    "return"
+
+_send_qemu_cmd $h "{ 'execute': 'quit' }" "return"
+wait=1 _cleanup_qemu
+
+_check_test_img
+
+# success, all done
+echo "*** done"
+rm -f $seq.full
+status=0
diff --git a/tests/qemu-iotests/185.out b/tests/qemu-iotests/185.out
new file mode 100644
index 0000000..45bc7cb
--- /dev/null
+++ b/tests/qemu-iotests/185.out
@@ -0,0 +1,59 @@
+QA output created by 185
+Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=67108864
+
+=== Starting VM ===
+
+{"return": {}}
+
+=== Creating backing chain ===
+
+Formatting 'TEST_DIR/t.qcow2.mid', fmt=qcow2 size=67108864 backing_file=TEST_DIR/t.qcow2.base backing_fmt=qcow2 encryption=off cluster_size=65536 lazy_refcounts=off refcount_bits=16
+{"return": {}}
+wrote 4194304/4194304 bytes at offset 0
+4 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+{"return": ""}
+Formatting 'TEST_DIR/t.qcow2', fmt=qcow2 size=67108864 backing_file=TEST_DIR/t.qcow2.mid backing_fmt=qcow2 encryption=off cluster_size=65536 lazy_refcounts=off refcount_bits=16
+{"return": {}}
+
+=== Start commit job and exit qemu ===
+
+{"return": {}}
+{"return": {}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "BLOCK_JOB_CANCELLED", "data": {"device": "disk", "len": 67108864, "offset": 524288, "speed": 65536, "type": "commit"}}
+
+=== Start active commit job and exit qemu ===
+
+{"return": {}}
+{"return": {}}
+{"return": {}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "BLOCK_JOB_CANCELLED", "data": {"device": "disk", "len": 4194304, "offset": 4194304, "speed": 65536, "type": "commit"}}
+
+=== Start mirror job and exit qemu ===
+
+{"return": {}}
+Formatting 'TEST_DIR/t.qcow2.copy', fmt=qcow2 size=67108864 encryption=off cluster_size=65536 lazy_refcounts=off refcount_bits=16
+{"return": {}}
+{"return": {}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "BLOCK_JOB_CANCELLED", "data": {"device": "disk", "len": 4194304, "offset": 4194304, "speed": 65536, "type": "mirror"}}
+
+=== Start backup job and exit qemu ===
+
+{"return": {}}
+Formatting 'TEST_DIR/t.qcow2.copy', fmt=qcow2 size=67108864 encryption=off cluster_size=65536 lazy_refcounts=off refcount_bits=16
+{"return": {}}
+{"return": {}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "BLOCK_JOB_CANCELLED", "data": {"device": "disk", "len": 67108864, "offset": 65536, "speed": 65536, "type": "backup"}}
+
+=== Start streaming job and exit qemu ===
+
+{"return": {}}
+{"return": {}}
+{"return": {}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "BLOCK_JOB_CANCELLED", "data": {"device": "disk", "len": 67108864, "offset": 524288, "speed": 65536, "type": "stream"}}
+No errors were found on the image.
+*** done
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index a6acaff..318ae74 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -175,3 +175,4 @@
 181 rw auto migration
 182 rw auto quick
 183 rw auto migration
+185 rw auto
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 3/3] qemu-iotests: Test exiting qemu with running job

[Eric Blake]

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

On 06/09/2017 06:50 AM, Kevin Wolf wrote:
> When qemu is exited, all running jobs should be cancelled successfully.
> This adds a test for this for all types of block jobs that currently
> exist in qemu.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  tests/qemu-iotests/185     | 189 +++++++++++++++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/185.out |  59 ++++++++++++++
>  tests/qemu-iotests/group   |   1 +
>  3 files changed, 249 insertions(+)
>  create mode 100755 tests/qemu-iotests/185
>  create mode 100644 tests/qemu-iotests/185.out
> 

> +
> +_send_qemu_cmd $h \
> +    "{ 'execute': 'human-monitor-command',
> +       'arguments': { 'command-line':
> +                      'qemu-io disk \"write 0 4M\"' } }" \
> +    "return"

My first reaction? "Why are we still dropping back to HMP?"  My second -
"Oh yeah - qemu-io is a debugging interface, and we really don't
need/want it in QMP".  This part is fine.

> +_send_qemu_cmd $h \
> +    "{ 'execute': 'drive-backup',
> +       'arguments': { 'device': 'disk',
> +                      'target': '$TEST_IMG.copy',
> +                      'format': '$IMGFMT',
> +                      'sync': 'full',
> +                      'speed': 65536 } }" \

Fun with slow speeds :)

64k is slow enough compared to your 4M chunk that you should be fairly
immune to a heavy load allowing the job to converge.  However,

> +{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false}}
> +{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "BLOCK_JOB_CANCELLED", "data": {"device": "disk", "len": 67108864, "offset": 524288, "speed": 65536, "type": "commit"}}

I'm worried that if you don't sanitize at least offset, you will still
be prone to some race conditions changing the output.  You may want to
add in some additional filtering on the output to be safer.

Other than that, the patch looks good to 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 3/3] qemu-iotests: Test exiting qemu with running job

[Kevin Wolf]

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

Am 09.06.2017 um 14:14 hat Eric Blake geschrieben:
> On 06/09/2017 06:50 AM, Kevin Wolf wrote:
> > When qemu is exited, all running jobs should be cancelled successfully.
> > This adds a test for this for all types of block jobs that currently
> > exist in qemu.
> > 
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> >  tests/qemu-iotests/185     | 189 +++++++++++++++++++++++++++++++++++++++++++++
> >  tests/qemu-iotests/185.out |  59 ++++++++++++++
> >  tests/qemu-iotests/group   |   1 +
> >  3 files changed, 249 insertions(+)
> >  create mode 100755 tests/qemu-iotests/185
> >  create mode 100644 tests/qemu-iotests/185.out
> > 
> 
> > +
> > +_send_qemu_cmd $h \
> > +    "{ 'execute': 'human-monitor-command',
> > +       'arguments': { 'command-line':
> > +                      'qemu-io disk \"write 0 4M\"' } }" \
> > +    "return"
> 
> My first reaction? "Why are we still dropping back to HMP?"  My second -
> "Oh yeah - qemu-io is a debugging interface, and we really don't
> need/want it in QMP".  This part is fine.
> 
> > +_send_qemu_cmd $h \
> > +    "{ 'execute': 'drive-backup',
> > +       'arguments': { 'device': 'disk',
> > +                      'target': '$TEST_IMG.copy',
> > +                      'format': '$IMGFMT',
> > +                      'sync': 'full',
> > +                      'speed': 65536 } }" \
> 
> Fun with slow speeds :)
> 
> 64k is slow enough compared to your 4M chunk that you should be fairly
> immune to a heavy load allowing the job to converge.  However,
> 
> > +{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false}}
> > +{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "BLOCK_JOB_CANCELLED", "data": {"device": "disk", "len": 67108864, "offset": 524288, "speed": 65536, "type": "commit"}}
> 
> I'm worried that if you don't sanitize at least offset, you will still
> be prone to some race conditions changing the output.  You may want to
> add in some additional filtering on the output to be safer.

I considered that at first, but then I realised that these offsets are
indeed predictable and we want to know if they change (it would likely
mean that the throttling is broken).

If you look at the individual cases, we have:

* offset=512k for (intermediate) commit and streaming. This is exactly
  the buffer size for a single request and will be followed by a delay
  of eight seconds before the next chunk is copied, so we will never get
  a different value here.

* offset=4M for active commit and mirror, because the mirror job has a
  larger buffer size by default, so one request completes it all. This
  number is already the maximum, so nothing is going to change here
  either.

* offset=64k for backup, which works cluster by cluster. We know that
  the cluster size is exactly 64k, and while we have only one second
  of delay here, that's still plenty of time for the 'quit' command to
  arrive.

Note that only the 'quit' command must be received in time on the QMP
socket, everything after that is synchronously, so even on a heavily
loaded host I don't see this fail. Well, maybe if it's swapping to
death, but then you have other problems.

So I think the offses actually make sense as part of the test.

Kevin

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

Re: [Qemu-devel] [PATCH 3/3] qemu-iotests: Test exiting qemu with running job

[Eric Blake]

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

On 06/09/2017 07:58 AM, Kevin Wolf wrote:
> Am 09.06.2017 um 14:14 hat Eric Blake geschrieben:
>> On 06/09/2017 06:50 AM, Kevin Wolf wrote:
>>> When qemu is exited, all running jobs should be cancelled successfully.
>>> This adds a test for this for all types of block jobs that currently
>>> exist in qemu.
>>>
>>> Signed-off-by: Kevin Wolf <kwolf@redhat.com>

>>> +{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false}}
>>> +{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "BLOCK_JOB_CANCELLED", "data": {"device": "disk", "len": 67108864, "offset": 524288, "speed": 65536, "type": "commit"}}
>>
>> I'm worried that if you don't sanitize at least offset, you will still
>> be prone to some race conditions changing the output.  You may want to
>> add in some additional filtering on the output to be safer.
> 
> I considered that at first, but then I realised that these offsets are
> indeed predictable and we want to know if they change (it would likely
> mean that the throttling is broken).
> 
> If you look at the individual cases, we have:
> 
> * offset=512k for (intermediate) commit and streaming. This is exactly
>   the buffer size for a single request and will be followed by a delay
>   of eight seconds before the next chunk is copied, so we will never get
>   a different value here.
> 
> * offset=4M for active commit and mirror, because the mirror job has a
>   larger buffer size by default, so one request completes it all. This
>   number is already the maximum, so nothing is going to change here
>   either.
> 
> * offset=64k for backup, which works cluster by cluster. We know that
>   the cluster size is exactly 64k, and while we have only one second
>   of delay here, that's still plenty of time for the 'quit' command to
>   arrive.

These belong in comments in the test proper, because it is not obvious
otherwise.  But with comments added (so someone debugging a theoretical
test failure down the road knows what they are up against),

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

-- 
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 exiting qemu with running job

[Kevin Wolf]

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

Am 09.06.2017 um 16:16 hat Eric Blake geschrieben:
> On 06/09/2017 07:58 AM, Kevin Wolf wrote:
> > Am 09.06.2017 um 14:14 hat Eric Blake geschrieben:
> >> On 06/09/2017 06:50 AM, Kevin Wolf wrote:
> >>> When qemu is exited, all running jobs should be cancelled successfully.
> >>> This adds a test for this for all types of block jobs that currently
> >>> exist in qemu.
> >>>
> >>> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> 
> >>> +{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false}}
> >>> +{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "BLOCK_JOB_CANCELLED", "data": {"device": "disk", "len": 67108864, "offset": 524288, "speed": 65536, "type": "commit"}}
> >>
> >> I'm worried that if you don't sanitize at least offset, you will still
> >> be prone to some race conditions changing the output.  You may want to
> >> add in some additional filtering on the output to be safer.
> > 
> > I considered that at first, but then I realised that these offsets are
> > indeed predictable and we want to know if they change (it would likely
> > mean that the throttling is broken).
> > 
> > If you look at the individual cases, we have:
> > 
> > * offset=512k for (intermediate) commit and streaming. This is exactly
> >   the buffer size for a single request and will be followed by a delay
> >   of eight seconds before the next chunk is copied, so we will never get
> >   a different value here.
> > 
> > * offset=4M for active commit and mirror, because the mirror job has a
> >   larger buffer size by default, so one request completes it all. This
> >   number is already the maximum, so nothing is going to change here
> >   either.
> > 
> > * offset=64k for backup, which works cluster by cluster. We know that
> >   the cluster size is exactly 64k, and while we have only one second
> >   of delay here, that's still plenty of time for the 'quit' command to
> >   arrive.
> 
> These belong in comments in the test proper, because it is not obvious
> otherwise.  But with comments added (so someone debugging a theoretical
> test failure down the road knows what they are up against),

# Note that the reference output intentionally includes the 'offset' field in
# BLOCK_JOB_CANCELLED events for all of the following block jobs. They are
# predictable and any change in the offsets would hint at a bug in the job
# throttling code.
#
# In order to achieve these predictable offsets, all of the following tests
# use speed=65536. Each job will perform exactly one iteration before it has
# to sleep at least for a second, which is plenty of time for the 'quit' QMP
# command to be received (after receiving the command, the rest runs
# synchronously, so jobs can arbitrarily continue or complete).
#
# The buffer size for commit and streaming is 512k (waiting for 8 seconds after
# the first request), for active commit and mirror it's large enough to cover
# the full 4M, and for backup it's the qcow2 cluster size, which we know is
# 64k. As all of these are at least as large as the speed, we are sure that the
# offset doesn't advance after the first iteration before qemu exits.

Does this look okay?

Kevin

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

Re: [Qemu-devel] [PATCH 3/3] qemu-iotests: Test exiting qemu with running job

[Eric Blake]

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

On 06/09/2017 10:55 AM, Kevin Wolf wrote:

>>
>> These belong in comments in the test proper, because it is not obvious
>> otherwise.  But with comments added (so someone debugging a theoretical
>> test failure down the road knows what they are up against),
> 
> # Note that the reference output intentionally includes the 'offset' field in
> # BLOCK_JOB_CANCELLED events for all of the following block jobs. They are
> # predictable and any change in the offsets would hint at a bug in the job
> # throttling code.
> #
> # In order to achieve these predictable offsets, all of the following tests
> # use speed=65536. Each job will perform exactly one iteration before it has
> # to sleep at least for a second, which is plenty of time for the 'quit' QMP
> # command to be received (after receiving the command, the rest runs
> # synchronously, so jobs can arbitrarily continue or complete).
> #
> # The buffer size for commit and streaming is 512k (waiting for 8 seconds after
> # the first request), for active commit and mirror it's large enough to cover
> # the full 4M, and for backup it's the qcow2 cluster size, which we know is
> # 64k. As all of these are at least as large as the speed, we are sure that the
> # offset doesn't advance after the first iteration before qemu exits.
> 
> Does this look okay?

Yes, that looks fine.

-- 
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] [Qemu-block] [PATCH 3/3] qemu-iotests: Test exiting qemu with running job

[Max Reitz]

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

On 2017-06-09 13:50, Kevin Wolf wrote:
> When qemu is exited, all running jobs should be cancelled successfully.
> This adds a test for this for all types of block jobs that currently
> exist in qemu.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  tests/qemu-iotests/185     | 189 +++++++++++++++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/185.out |  59 ++++++++++++++
>  tests/qemu-iotests/group   |   1 +
>  3 files changed, 249 insertions(+)
>  create mode 100755 tests/qemu-iotests/185
>  create mode 100644 tests/qemu-iotests/185.out
> 
> diff --git a/tests/qemu-iotests/185 b/tests/qemu-iotests/185
> new file mode 100755
> index 0000000..645ec9a
> --- /dev/null
> +++ b/tests/qemu-iotests/185

[...]

> +_supported_fmt qcow2 raw
> +_supported_proto file
> +_supported_os Linux

[...]

> +echo
> +echo === Creating backing chain ===
> +echo
> +
> +_send_qemu_cmd $h \
> +    "{ 'execute': 'blockdev-snapshot-sync',
> +       'arguments': { 'device': 'disk',
> +                      'snapshot-file': '$TEST_IMG.mid',
> +                      'format': '$IMGFMT',

Not having looked at this series further yet (sorry...), I just noticed
that this does not work very well with raw.

Max

> +                      'mode': 'absolute-paths' } }" \
> +    "return"
> +
> +_send_qemu_cmd $h \
> +    "{ 'execute': 'human-monitor-command',
> +       'arguments': { 'command-line':
> +                      'qemu-io disk \"write 0 4M\"' } }" \
> +    "return"
> +
> +_send_qemu_cmd $h \
> +    "{ 'execute': 'blockdev-snapshot-sync',
> +       'arguments': { 'device': 'disk',
> +                      'snapshot-file': '$TEST_IMG',
> +                      'format': '$IMGFMT',
> +                      'mode': 'absolute-paths' } }" \
> +    "return"


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

Re: [Qemu-devel] [Qemu-block] [PATCH 3/3] qemu-iotests: Test exiting qemu with running job

[Kevin Wolf]

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

Am 09.06.2017 um 16:10 hat Max Reitz geschrieben:
> On 2017-06-09 13:50, Kevin Wolf wrote:
> > When qemu is exited, all running jobs should be cancelled successfully.
> > This adds a test for this for all types of block jobs that currently
> > exist in qemu.
> > 
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> >  tests/qemu-iotests/185     | 189 +++++++++++++++++++++++++++++++++++++++++++++
> >  tests/qemu-iotests/185.out |  59 ++++++++++++++
> >  tests/qemu-iotests/group   |   1 +
> >  3 files changed, 249 insertions(+)
> >  create mode 100755 tests/qemu-iotests/185
> >  create mode 100644 tests/qemu-iotests/185.out
> > 
> > diff --git a/tests/qemu-iotests/185 b/tests/qemu-iotests/185
> > new file mode 100755
> > index 0000000..645ec9a
> > --- /dev/null
> > +++ b/tests/qemu-iotests/185
> 
> [...]
> 
> > +_supported_fmt qcow2 raw
> > +_supported_proto file
> > +_supported_os Linux
> 
> [...]
> 
> > +echo
> > +echo === Creating backing chain ===
> > +echo
> > +
> > +_send_qemu_cmd $h \
> > +    "{ 'execute': 'blockdev-snapshot-sync',
> > +       'arguments': { 'device': 'disk',
> > +                      'snapshot-file': '$TEST_IMG.mid',
> > +                      'format': '$IMGFMT',
> 
> Not having looked at this series further yet (sorry...), I just noticed
> that this does not work very well with raw.

Good point... I'll make it qcow2 only.

Kevin

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