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

["Dr. David Alan Gilbert" <dgilbert@redhat.com>]

* Halil Pasic (pasic@linux.vnet.ibm.com) wrote:
> 
> 
> 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 "$@" )
> >>  QEMU X.Y.Z monitor - type 'help' for more information
> >>  (qemu) savevm 0
> >> -(qemu) quit
> >> +qemu-system-s390x: Device 'virtio0' does not have the requested snapshot '0'
> >>  QEMU X.Y.Z monitor - type 'help' for more information
> >>  (qemu) quit
> >>  *** done
> >>
> >> 091 1s ... [failed, exit status 1] - output mismatch (see 091.out.bad)
> >>     --- tests/qemu-iotests/091.out	2016-08-30 12:35:04.207683276 +0200
> >>     +++ 091.out.bad	2017-03-06 13:08:03.717135426 +0100
> >>     @@ -11,18 +11,23 @@
> >>      
> >>      vm1: qemu-io disk write complete
> >>      vm1: live migration started
> >>     -vm1: live migration completed
> >>     -
> >>     -=== VM 2: Post-migration, write to disk, verify running ===
> >>     -
> >>     -vm2: qemu-io disk write complete
> >>     -vm2: qemu process running successfully
> >>     -vm2: flush io, and quit
> >>     -Check image pattern
> >>     -read 4194304/4194304 bytes at offset 0
> >>     -4 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> >>     -Running 'qemu-img check -r all $TEST_IMG'
> >>     -No errors were found on the image.
> >>     -80/16384 = 0.49% allocated, 0.00% fragmented, 0.00% compressed clusters
> >>     -Image end offset: 5570560
> >>     -*** done
> >>     +./common.qemu: line 110: write error: Broken pipe
> >>     +./common.qemu: line 110: write error: Broken pipe
> >>     +./common.qemu: line 110: write error: Broken pipe
> >>     +./common.qemu: line 110: write error: Broken pipe
> >>     +./common.qemu: line 110: write error: Broken pipe
> >>     +./common.qemu: line 110: write error: Broken pipe
> >>     +./common.qemu: line 110: write error: Broken pipe
> >>     +./common.qemu: line 110: write error: Broken pipe
> >>     +./common.qemu: line 110: write error: Broken pipe
> >>     +./common.qemu: line 110: write error: Broken pipe
> >>     +./common.qemu: line 110: write error: Broken pipe
> >>     +./common.qemu: line 110: write error: Broken pipe
> >>     +./common.qemu: line 110: write error: Broken pipe
> >>     +./common.qemu: line 110: write error: Broken pipe
> >>     +./common.qemu: line 110: write error: Broken pipe
> >>     +./common.qemu: line 110: write error: Broken pipe
> >>     +./common.qemu: line 110: write error: Broken pipe
> >>     +./common.qemu: line 110: write error: Broken pipe
> >>     +./common.qemu: line 110: write error: Broken pipe
> >>     +Timeout waiting for completed on handle 0
> >>
> >> Signed-off-by: QingFeng Hao <haoqf@linux.vnet.ibm.com>
> >> ---
> >>  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.
> 
> I think the migration maintainers (Juan and Dave) should make a
> call regarding if the described usage  of VMS_BUFFER is a legal
> or an illegal one.

So is it this code in target/s390x/machine.c ?

        VMSTATE_VBUFFER_UINT32(irqstate, S390CPU, 4, NULL,
                               irqstate_saved_size),

it should be legal for that to be zero length.
It also makes sense that if that's zero length it should be OK for
the pointer to be NULL.

I think it's best if you do change that assert.

Dave


> Regards,
> Halil
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK