Re: [Qemu-block] [Qemu-devel] [PATCH RFC 1/1] vmstate: draft fix for failed iotests case 68 and 91

[Halil Pasic]

On 03/08/2017 08:05 AM, QingFeng Hao wrote:
> 
> 
> 在 2017/3/7 18:19, Halil Pasic 写道:
>>
>> On 03/07/2017 11:05 AM, Kevin Wolf wrote:
>>> Am 07.03.2017 um 10:54 hat Halil Pasic geschrieben:
>>>>
>>>> On 03/07/2017 10:29 AM, Kevin Wolf wrote:
>>>>> Am 07.03.2017 um 03:53 hat QingFeng Hao geschrieben:
>>>>>> I am not very clear about the logic in vmstate.c, but from its context in
>>>>>> vmstate_save_state, it seems size should not be 0, otherwise the followed
>>>>>> for loop will keep working on the same element. So I just add a simple
>>>>>> check to pass that case, not sure if it's right but it can pass iotest
>>>>>> case 68 and 91 now.
>>>>>>
>>>>>> The iotest's failed output is:
>>>>>> 068 1s ... - output mismatch (see 068.out.bad)
>>>>>> --- 
>>>>>> /home/haoqf/KVMonz/gitcheck/work/qemu-master/tree/qemu/tests/qemu-iotests/068.out
>>>>>>    2017-03-06 05:52:24.817328899 +0100
>>>>>> +++ 068.out.bad 2017-03-07 03:28:44.426714519 +0100
>>>>>> @@ -3,9 +3,13 @@
>>>>>>   === Saving and reloading a VM state to/from a qcow2 image ===
>>>>>>
>>>>>>   Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=131072
>>>>>> +qemu-system-s390x: migration/vmstate.c:336: vmstate_save_state: 
>>>>>> Assertion `first_elem || !n_elems' failed.
>>>>>> +./common.config: line 109: 52497 Aborted                 ( if [ -n 
>>>>>> "${QEMU_NEED_PID}" ]; then
>>>>>> +    echo $BASHPID > "${QEMU_TEST_DIR}/qemu-${_QEMU_HANDLE}.pid";
>>>>>> +fi; exec "$QEMU_PROG" $QEMU_OPTIONS "$@" )
>>>>>>
>>>>>>
>>>>>> Signed-off-by: QingFeng Hao <address@hidden>
>>>>>> ---
>>>>>>   migration/vmstate.c | 8 ++++++++
>>>>>>   1 file changed, 8 insertions(+)
>>>>>>
>>>>>> diff --git a/migration/vmstate.c b/migration/vmstate.c
>>>>>> index 78b3cd4..ff28dde 100644
>>>>>> --- a/migration/vmstate.c
>>>>>> +++ b/migration/vmstate.c
>>>>>> @@ -106,6 +106,10 @@ int vmstate_load_state(QEMUFile *f, const 
>>>>>> VMStateDescription *vmsd,
>>>>>>               int i, n_elems = vmstate_n_elems(opaque, field);
>>>>>>               int size = vmstate_size(opaque, field);
>>>>>>   +            if (size == 0) {
>>>>>> +                field++;
>>>>>> +                continue;
>>>>>> +            }
>>>>>>               vmstate_handle_alloc(first_elem, field, opaque);
>>>>>>               if (field->flags & VMS_POINTER) {
>>>>>>                   first_elem = *(void **)first_elem;
>>>>>> @@ -322,6 +326,10 @@ void vmstate_save_state(QEMUFile *f, const 
>>>>>> VMStateDescription *vmsd,
>>>>>>               int64_t old_offset, written_bytes;
>>>>>>               QJSON *vmdesc_loop = vmdesc;
>>>>>>   +            if (size == 0) {
>>>>>> +                field++;
>>>>>> +                continue;
>>>>>> +            }
>>>>>>               trace_vmstate_save_state_loop(vmsd->name, field->name, 
>>>>>> n_elems);
>>>>>>               if (field->flags & VMS_POINTER) {
>>>>>>                   first_elem = *(void **)first_elem;
>>>>> This is really a live migration fix, so I'm adding Juan and Dave to CC.
>>>> You are right, this is migration stuff and has very little to do with
>>>> qemu-block.
>>>>> I suspect the real question is why a field with size 0 was even stored
>>>>> in the vmstate to begin with.
>>>>>
>>>> I have looked onto the issue. It affects s390x only if we
>>>> are running without KVM. Basically, S390CPU.irqstate is unused
>>>> if we do not use KVM, and thus no buffer is allocated.
>>>>
>>>> IMHO this is a missing field and the cleaner way to handle such
>>>> missing fields is exist. However this used to work, and I recommended
>>>> QuiFeng Hao to discuss the problem upstream.
>>>>
>>>> By the way, I think, if we want to go back to the old behavior
>>>> and support VMS_VBUFFER with size 0 and nullptr, a much
>>>> cleaner way to do the fix is to change the assert to:
>>>>
>>>> assert(first_elem  || !n_elems || !size)
>>>>
>>>> Obviously the other clean way to fix is to implement exists.
>>> If you're right that this specific vmstate was valid in earlier
>>> versions, then I think it's clear that we need to make it work again.
>>> Otherwise we're breaking migration from old versions.
>> Not really. We would not break migration because nothing was written to
>> the stream for VMS_VBUFFER of size 0 except the vmdesc which is at the end,
>> 'debug only', and does not affect migration compatibility.
>>
>> IMHO it is an API question. I would have said, there is no data, therefore
>> there is no field if it's from scratch. But with prior history, I agree
>> with Dave, we should restore old behavior -- which was changed 
>> unintentionally
>> because I made a wrong assumption.
> Halil,
> Unfortunately, another assert failed after I change the code as below in
> vmstate_save_state and vmstate_load_state:
> assert(first_elem  || !n_elems || !size);
> 
> The error is:
> +qemu-system-s390x: migration/vmstate.c:341: vmstate_save_state: Assertion 
> `field->flags & VMS_ARRAY_OF_POINTER' failed.
> It's the code as below:
>  if (!curr_elem) {

Sorry, I failed to mention this yesterday. You should also do

-                 if (!curr_elem) {
+                 if (!curr_elem && size) {




>                     /* if null pointer write placeholder and do not follow    
>   */
>                   assert(field->flags & VMS_ARRAY_OF_POINTER);
> After debug, I found that the assert failure was still of irqstate and its 
> field flags is 0x102(VMS_VBUFFER | VMS_POINTER),
> while VMS_ARRAY_OF_POINTER is 0x040. The flags's value matches Dave's former 
> assumption
> on machine.c although machine.c wasn't compiled, which also confuses me.
> 
> Then I commented out that assert(field->flags & VMS_ARRAY_OF_POINTER) and the 
> case 68 & 91
> can both work!

Yeah but that would break migration compatibility for write by 
writing a nullptr-marker character into the stream.

> 
> The question is: can we do that change and what the second assert of field's 
> flags is used for?

The assert is there to guard against unintended use of this nullptr-marker
mechanism.

I have tested so this should work. By the way a vmstate test covering this
corner-case would be a good idea too (IMHO). Would you like to write one?

Regards,
Halil

> Thanks!
>> Regards,
>> Halil
>