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

[QingFeng Hao]

在 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) {
                    /* 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!

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

--
Regards
QingFeng Hao