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

[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.

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

signature.asc
Description: PGP signature