qemu-block
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-block] [Qemu-devel] [PATCH v1 1/1] iotests: fix test case 185


From: QingFeng Hao
Subject: Re: [Qemu-block] [Qemu-devel] [PATCH v1 1/1] iotests: fix test case 185
Date: Wed, 21 Mar 2018 11:17:33 +0800
User-agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.6.0



在 2018/3/21 11:12, QingFeng Hao 写道:


在 2018/3/20 22:31, Stefan Hajnoczi 写道:
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.
Thanks for all of your good comments! I think the better way is to filter out this case but I am sure if this is a proper way to do it that adds a yielded field in struct BlockJob to record if it's yielded by block_job_do_yield. However, this way can only solve the offset problem but not the status. Maybe we need also to change 185.out? I am bit confused. thanks
What I did is setting job->yielded in block_job_do_yield and adding the check for it in block_job_pause and block_job_resume that if yielded is
true, just return.

We need to distinguish these two cases to avoid resuming the job in the
latter case.  I think this would be the proper way to avoid unnecessary
blockjob activity across drain.

I favor updating the test output though, because the code change adds
complexity and the practical benefit is not obvious to me.

QingFeng, if you decide to modify blockjobs, please CC Jeffrey Cody
<address@hidden> and I'd also be happy to review the patch

Stefan



--
Regards
QingFeng Hao




reply via email to

[Prev in Thread] Current Thread [Next in Thread]