On Tue, Mar 20, 2018 at 11:11:01AM +0100, Kevin Wolf wrote:
Am 19.03.2018 um 18:53 hat Christian Borntraeger geschrieben:
On 03/19/2018 05:10 PM, Stefan Hajnoczi wrote:
On Mon, Mar 19, 2018 at 9:35 AM, QingFeng Hao
<address@hidden> wrote:
Test case 185 failed since commit 4486e89c219 --- "vl: introduce
vm_shutdown()".
It's because of the newly introduced function vm_shutdown calls
bdrv_drain_all,
which is called later by bdrv_close_all. bdrv_drain_all resumes
the jobs
that doubles the speed and offset is doubled.
Some jobs' status are changed as well.
Thus, let's not call bdrv_drain_all in vm_shutdown.
Signed-off-by: QingFeng Hao <address@hidden>
---
cpus.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/cpus.c b/cpus.c
index 2e6701795b..ae2962508c 100644
--- a/cpus.c
+++ b/cpus.c
@@ -1006,8 +1006,9 @@ static int do_vm_stop(RunState state, bool
send_stop)
qapi_event_send_stop(&error_abort);
}
}
-
- bdrv_drain_all();
+ if (send_stop) {
+ bdrv_drain_all();
+ }
Thanks for looking into this bug!
This if statement breaks the contract of the function when send_stop
is false. Drain ensures that pending I/O completes and that device
emulation finishes before this function returns. This is important
for live migration, where there must be no more guest-related activity
after vm_stop().
This patch relies on the caller invoking bdrv_close_all() immediately
after do_vm_stop(), which is not documented and could lead to more
bugs in the future.
I suggest a different solution:
Test 185 relies on internal QEMU behavior which can change from time
to time. Buffer sizes and block job iteration counts are
implementation details that are not part of qapi-schema.json or other
documented behavior.
In fact, the existing test case was already broken in IOThread mode
since iothread_stop_all() -> bdrv_set_aio_context() also includes a
bdrv_drain() with the side-effect of an extra blockjob iteration.
After 4486e89c219 ("vl: introduce vm_shutdown()") both IOThread and
non-IOThread mode perform the drain. This has caused the test
failure.
Instead of modifying QEMU to keep the same arbitrary internal behavior
as before, please send a patch that updates tests/qemu-iotests/185.out
with the latest output.
Wouldnt it be better if the test actually masks out the values the
are not
really part of the "agreed upon" behaviour? Wouldnt block_job_offset
from
common.filter be what we want?
The test case has the following note:
# 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.
Now the question is if this particular change is okay rather than
hinting at a bug and we should update the reference output or whether we
need to change qemu again.
I think the change isn't really bad, but we are doing more useless work
now than we used to do before, and we're exiting slower because we're
spawning additional I/O that we have to wait for. So the better state
was certainly the old one.
Here is the reason for the additional I/O and how it could be
eliminated:
child_job_drained_begin() pauses and child_job_drained_end() resumes the
job. Resuming the job cancels the timer (if pending) and enters the
blockjob's coroutine. This is why draining BlockDriverState forces an
extra iteration of the blockjob.
The current behavior is intended for the case where the job has I/O
pending at child_job_drained_begin() time. When the I/O completes, the
job will see it must pause and it will yield at a pause point. It makes
sense for child_job_drained_end() to resume the job so it can start I/O
again.
In our case this behavior doesn't make sense though. The job already
yielded before child_job_drained_begin() and it doesn't need to resume
at child_job_drained_end() time. We'd prefer to wait for the timer
expiry.