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

[Stefan Hajnoczi]

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.

Stefan