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

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

在 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