[Top][All Lists]

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

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

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

在 2018/3/23 18:04, Stefan Hajnoczi 写道:
On Fri, Mar 23, 2018 at 3:43 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.

The fix is to not resume the jobs that are already yielded and also change
185.out accordingly.

Suggested-by: Stefan Hajnoczi <address@hidden>
Signed-off-by: QingFeng Hao <address@hidden>
  blockjob.c                 | 10 +++++++++-
  include/block/blockjob.h   |  5 +++++
  tests/qemu-iotests/185.out | 11 +++++++++--

If drain no longer forces the block job to iterate, shouldn't the test
output remain the same?  (The means the test is fixed by the QEMU

  3 files changed, 23 insertions(+), 3 deletions(-)

diff --git a/blockjob.c b/blockjob.c
index ef3ed69ff1..fa9838ac97 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -206,11 +206,16 @@ void block_job_txn_add_job(BlockJobTxn *txn, BlockJob 

  static void block_job_pause(BlockJob *job)
-    job->pause_count++;
+    if (!job->yielded) {
+        job->pause_count++;
+    }

The pause cannot be ignored.  This change introduces a bug.

Pause is not a synchronous operation that stops the job immediately.
Pause just remembers that the job needs to be paused.   When the job
runs again (e.g. timer callback, fd handler) it eventually reaches
block_job_pause_point() where it really pauses.

The bug in this patch is:

1. The job has a timer pending.
2. block_job_pause() is called during drain.
3. The timer fires during drain but now the job doesn't know it needs
to pause, so it continues running!

Instead what needs to happen is that block_job_pause() remains
unmodified but block_job_resume if extended:

static void block_job_resume(BlockJob *job)
     assert(job->pause_count > 0);
     if (job->pause_count) {
+    if (job_yielded_before_pause_and_is_still_yielded) {
Thanks a lot for your great comments! But I can't figure out how to check this.
+    }

This handles the case I mentioned above, where the yield ends before
pause ends (therefore resume must enter the job!).

To make this a little clearer, there are two cases to consider:

Case 1:
1. Job yields
2. Pause
3. Job is entered from timer/fd callback
How do I know that if job is entered from ...? thanks
4. Resume (enter job? yes)

Case 2:
1. Job yields
2. Pause
3. Resume (enter job? no)
4. Job is entered from timer/fd callback


QingFeng Hao

reply via email to

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