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

[QingFeng Hao <haoqf@linux.vnet.ibm.com>]

在 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