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

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

[QingFeng Hao]

Hi,
This patch is to remove the redundant call to bdrv_drain_all in vm_shutdown.
Thanks!

Test case 185 failed as below:
185 2s ... - output mismatch (see 185.out.bad)
--- /home/mc/gitcheck/work/qemu-master/tree/qemu/tests/qemu-iotests/185.out     2018-03-09 01:00:40.451603189 +0100
+++ /home/mc/gitcheck/work/qemu-master/tree/qemu/tests/qemu-iotests/185.out.bad 2018-03-19 09:40:22.781603189 +0100
@@ -20,7 +20,7 @@
 {"return": {}}
 {"return": {}}
 {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false}}
-{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "BLOCK_JOB_CANCELLED", "data": {"device": "disk", "len": 67108864, "offset": 524288, "speed": 65536, "type": "commit"}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "BLOCK_JOB_CANCELLED", "data": {"device": "disk", "len": 67108864, "offset": 1048576, "speed": 65536, "type": "commit"}}

 === Start active commit job and exit qemu ===

@@ -28,7 +28,8 @@
 {"return": {}}
 {"return": {}}
 {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false}}
-{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "BLOCK_JOB_CANCELLED", "data": {"device": "disk", "len": 4194304, "offset": 4194304, "speed": 65536, "type": "commit"}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "BLOCK_JOB_READY", "data": {"device": "disk", "len": 4194304, "offset": 4194304, "speed": 65536, "type": "commit"}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "BLOCK_JOB_COMPLETED", "data": {"device": "disk", "len": 4194304, "offset": 4194304, "speed": 65536, "type": "commit"}}

 === Start mirror job and exit qemu ===

@@ -37,7 +38,8 @@
 {"return": {}}
 {"return": {}}
 {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false}}
-{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "BLOCK_JOB_CANCELLED", "data": {"device": "disk", "len": 4194304, "offset": 4194304, "speed": 65536, "type": "mirror"}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "BLOCK_JOB_READY", "data": {"device": "disk", "len": 4194304, "offset": 4194304, "speed": 65536, "type": "mirror"}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "BLOCK_JOB_COMPLETED", "data": {"device": "disk", "len": 4194304, "offset": 4194304, "speed": 65536, "type": "mirror"}}

 === Start backup job and exit qemu ===

@@ -46,7 +48,7 @@
 {"return": {}}
 {"return": {}}
 {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false}}
-{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "BLOCK_JOB_CANCELLED", "data": {"device": "disk", "len": 67108864, "offset": 65536, "speed": 65536, "type": "backup"}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "BLOCK_JOB_CANCELLED", "data": {"device": "disk", "len": 67108864, "offset": 131072, "speed": 65536, "type": "backup"}}

 === Start streaming job and exit qemu ===

@@ -54,6 +56,6 @@
 {"return": {}}
 {"return": {}}
 {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false}}
-{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "BLOCK_JOB_CANCELLED", "data": {"device": "disk", "len": 67108864, "offset": 524288, "speed": 65536, "type": "stream"}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "BLOCK_JOB_CANCELLED", "data": {"device": "disk", "len": 67108864, "offset": 1048576, "speed": 65536, "type": "stream"}}
 No errors were found on the image.
 *** done
Failures: 185
Failed 1 of 1 tests

QingFeng Hao (1):
  iotests: fix test case 185

 cpus.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

-- 
2.13.5

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

[QingFeng Hao]

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 <haoqf@linux.vnet.ibm.com>
---
 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();
+    }
     replay_disable_events();
     ret = bdrv_flush_all();
 
-- 
2.13.5

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 <haoqf@linux.vnet.ibm.com> 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 <haoqf@linux.vnet.ibm.com>
> ---
>  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

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

[Christian Borntraeger]

On 03/19/2018 05:10 PM, Stefan Hajnoczi wrote:
> On Mon, Mar 19, 2018 at 9:35 AM, QingFeng Hao <haoqf@linux.vnet.ibm.com> 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 <haoqf@linux.vnet.ibm.com>
>> ---
>>  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?

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

[Kevin Wolf]

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 <haoqf@linux.vnet.ibm.com> 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 <haoqf@linux.vnet.ibm.com>
> >> ---
> >>  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.

Kevin

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

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 5017 bytes --]

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 <haoqf@linux.vnet.ibm.com> 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 <haoqf@linux.vnet.ibm.com>
> > >> ---
> > >>  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
<jcody@redhat.com> and I'd also be happy to review the patch.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

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

[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 <haoqf@linux.vnet.ibm.com> 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 <haoqf@linux.vnet.ibm.com>
>>>>> ---
>>>>>   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
> 
> 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
> <jcody@redhat.com> and I'd also be happy to review the patch
> 
> Stefan
> 

-- 
Regards
QingFeng Hao

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

[QingFeng Hao]

在 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 
>>>>> <haoqf@linux.vnet.ibm.com> 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 <haoqf@linux.vnet.ibm.com>
>>>>>> ---
>>>>>>   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
>> <jcody@redhat.com> and I'd also be happy to review the patch
>>
>> Stefan
>>
> 

-- 
Regards
QingFeng Hao