[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-block] [Qemu-devel] [PATCH RFC 1/1] vmstate: draft fix for fai

From: QingFeng Hao
Subject: Re: [Qemu-block] [Qemu-devel] [PATCH RFC 1/1] vmstate: draft fix for failed iotests case 68 and 91
Date: Thu, 9 Mar 2017 10:55:04 +0800
User-agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:45.0) Gecko/20100101 Thunderbird/45.8.0

在 2017/3/8 19:33, 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)
   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
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.
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) {

yes, this works per my test!

                     /* 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 
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

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?
Sorry, I don't know how to write that test case. Can you please write one?


QingFeng Hao

reply via email to

[Prev in Thread] Current Thread [Next in Thread]