[Qemu-devel] [PATCH] Peek dont read for vmdescription

[Qemu-devel] [PATCH] Peek dont read for vmdescription

[Dr. David Alan Gilbert (git)]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

The VMDescription section maybe after the EOF mark, the current code
does a 'qemu_get_byte' and either gets the header byte identifying the
description or an error (which it ignores).  Doing the 'get' upsets
RDMA which hangs on old machine types without the VMDescription.

Using 'qemu_peek_byte' avoids that.

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 migration/savevm.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/migration/savevm.c b/migration/savevm.c
index 2004dce..4bd3709 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -1128,9 +1128,14 @@ int qemu_loadvm_state(QEMUFile *f)
      * Try to read in the VMDESC section as well, so that dumping tools that
      * intercept our migration stream have the chance to see it.
      */
-    if (qemu_get_byte(f) == QEMU_VM_VMDESCRIPTION) {
-        uint32_t size = qemu_get_be32(f);
+    if (qemu_peek_byte(f, 0) == QEMU_VM_VMDESCRIPTION) {
         uint8_t *buf = g_malloc(0x1000);
+        uint32_t size;
+
+        /* Consume the peeked byte */
+        size = qemu_get_byte(f);
+
+        size = qemu_get_be32(f);
 
         while (size > 0) {
             uint32_t read_chunk = MIN(size, 0x1000);
-- 
2.4.3

Re: [Qemu-devel] [PATCH] Peek dont read for vmdescription

[Alexander Graf]

On 22.06.15 16:49, Dr. David Alan Gilbert (git) wrote:
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> 
> The VMDescription section maybe after the EOF mark, the current code
> does a 'qemu_get_byte' and either gets the header byte identifying the
> description or an error (which it ignores).  Doing the 'get' upsets
> RDMA which hangs on old machine types without the VMDescription.
> 
> Using 'qemu_peek_byte' avoids that.
> 
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

Fun. I did actually use peek at first and then figured it's the same as
read in the qemu file implementation. Have you figured out why exactly
peek does make a difference for the RDMA case?


Alex

Re: [Qemu-devel] [PATCH] Peek dont read for vmdescription

[Dr. David Alan Gilbert]

* Alexander Graf (agraf@suse.de) wrote:
> 
> 
> On 22.06.15 16:49, Dr. David Alan Gilbert (git) wrote:
> > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> > 
> > The VMDescription section maybe after the EOF mark, the current code
> > does a 'qemu_get_byte' and either gets the header byte identifying the
> > description or an error (which it ignores).  Doing the 'get' upsets
> > RDMA which hangs on old machine types without the VMDescription.
> > 
> > Using 'qemu_peek_byte' avoids that.
> > 
> > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> 
> Fun. I did actually use peek at first and then figured it's the same as
> read in the qemu file implementation. Have you figured out why exactly
> peek does make a difference for the RDMA case?

Hmm, no, I agree that is odd but I do need to look again at it.

I started off with a simple empty VM (no guest running) and found that
it wouldn't migrate over RDMA using machine types older than 2.3
unless I sent the vmdesc, and this seems to fix that for me.

However, I've found another case; a busy migrate running stressapp
that still fails on older machine types.  My guess is that it depends
whether we've read the data already - if we're lucky and the data
is already in the qemu-file buffer it doesn't end up calling the RDMA
code again.

Dave


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

Re: [Qemu-devel] [PATCH] Peek dont read for vmdescription

[Dr. David Alan Gilbert]

* Alexander Graf (agraf@suse.de) wrote:
> 
> 
> On 22.06.15 16:49, Dr. David Alan Gilbert (git) wrote:
> > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> > 
> > The VMDescription section maybe after the EOF mark, the current code
> > does a 'qemu_get_byte' and either gets the header byte identifying the
> > description or an error (which it ignores).  Doing the 'get' upsets
> > RDMA which hangs on old machine types without the VMDescription.
> > 
> > Using 'qemu_peek_byte' avoids that.
> > 
> > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> 
> Fun. I did actually use peek at first and then figured it's the same as
> read in the qemu file implementation. Have you figured out why exactly
> peek does make a difference for the RDMA case?

Yeh, scrap this patch.

I've just posted 

'Only try and read a VMDescription if it should be there'

as a replacement.
Fundamentally, the trick of trying to send/read stuff after the EOF
just isn't safe on all transports.  We've got to read stuff if it's
expected and only if it's expected and obey the EOF marker.  If it
wasn't for keeping compatibility I'd swing this section around so it
went before the EOF, but we can't break compatibility with streams
that already have it.

Dave

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

Re: [Qemu-devel] [PATCH] Peek dont read for vmdescription

[Alexander Graf]

On 23.06.15 18:37, Dr. David Alan Gilbert wrote:
> * Alexander Graf (agraf@suse.de) wrote:
>>
>>
>> On 22.06.15 16:49, Dr. David Alan Gilbert (git) wrote:
>>> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>>>
>>> The VMDescription section maybe after the EOF mark, the current code
>>> does a 'qemu_get_byte' and either gets the header byte identifying the
>>> description or an error (which it ignores).  Doing the 'get' upsets
>>> RDMA which hangs on old machine types without the VMDescription.
>>>
>>> Using 'qemu_peek_byte' avoids that.
>>>
>>> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
>>
>> Fun. I did actually use peek at first and then figured it's the same as
>> read in the qemu file implementation. Have you figured out why exactly
>> peek does make a difference for the RDMA case?
> 
> Yeh, scrap this patch.
> 
> I've just posted 
> 
> 'Only try and read a VMDescription if it should be there'
> 
> as a replacement.
> Fundamentally, the trick of trying to send/read stuff after the EOF
> just isn't safe on all transports.  We've got to read stuff if it's
> expected and only if it's expected and obey the EOF marker.  If it
> wasn't for keeping compatibility I'd swing this section around so it
> went before the EOF, but we can't break compatibility with streams
> that already have it.

Meh, that's truly a shame. The post-things-after-EOF-hack sounded so
great...


Alex