[Qemu-devel] [PATCH] exec-migration: handle EINTR in popen_get_buffer()

[Qemu-devel] [PATCH] exec-migration: handle EINTR in popen_get_buffer()

[Uri Lublin]

[-- Attachment #1: Type: text/plain, Size: 119 bytes --]

Hello,


The attached patch fix occasional failures of incoming exec migrations.

Patch based on qemu-kvm.git.

Uri.



[-- Attachment #2: 0001-exec-migration-handle-EINTR-in-popen_get_buffer.patch --]
[-- Type: text/x-patch, Size: 1018 bytes --]

>From 0003cebfd2afdf03e2f2cff599691952950a05ce Mon Sep 17 00:00:00 2001
From: Uri Lublin <uril@redhat.com>
Date: Tue, 2 Jun 2009 17:09:57 +0300
Subject: [PATCH] exec-migration: handle EINTR in popen_get_buffer()

Upon interrupt, fread returns with no data, and
the (incoming exec) migration fails.

Fix by retrying on such a case.

Signed-off-by: Uri Lublin <uril@redhat.com>
---
 savevm.c |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/savevm.c b/savevm.c
index 68ffd03..f53de43 100644
--- a/savevm.c
+++ b/savevm.c
@@ -215,7 +215,12 @@ static int popen_put_buffer(void *opaque, const uint8_t *buf, int64_t pos, int s
 static int popen_get_buffer(void *opaque, uint8_t *buf, int64_t pos, int size)
 {
     QEMUFilePopen *s = opaque;
-    return fread(buf, 1, size, s->popen_file);
+    int bytes;
+
+    do {
+        bytes = fread(buf, 1, size, s->popen_file);
+    } while ((bytes == 0) && (errno == EINTR));
+    return bytes;
 }

 static int popen_close(void *opaque)
-- 
1.6.2.2

[Qemu-devel] [PATCH] exec-migration: handle EINTR in popen_get_buffer()

[Uri Lublin]

Sometimes, upon interrupt, fread returns with no data, and
the (incoming exec) migration fails.

Fix by retrying on such a case.

Signed-off-by: Uri Lublin <uril@redhat.com>
---
 savevm.c |    9 ++++++++-
 1 files changed, 8 insertions(+), 1 deletions(-)

diff --git a/savevm.c b/savevm.c
index 248aea3..df2486d 100644
--- a/savevm.c
+++ b/savevm.c
@@ -215,7 +215,14 @@ static int popen_put_buffer(void *opaque, const uint8_t *buf, int64_t pos, int s
 static int popen_get_buffer(void *opaque, uint8_t *buf, int64_t pos, int size)
 {
     QEMUFilePopen *s = opaque;
-    return fread(buf, 1, size, s->popen_file);
+    FILE *fp = s->popen_file;
+    int bytes;
+
+    do {
+        clearerr(fp);
+        bytes = fread(buf, 1, size, fp);
+    } while ((bytes == 0) && ferror(fp) && (errno == EINTR));
+    return bytes;
 }

 static int popen_close(void *opaque)
-- 
1.6.2.2

Re: [Qemu-devel] [PATCH] exec-migration: handle EINTR in popen_get_buffer()

[Blue Swirl]

On 6/8/09, Uri Lublin <uril@redhat.com> wrote:
> Sometimes, upon interrupt, fread returns with no data, and
>  the (incoming exec) migration fails.
>
>  Fix by retrying on such a case.

Maybe a better solution would be to introduce qemu_{f,}{read,write},
which handle EINTR and partial reads/writes.

Re: [Qemu-devel] [PATCH] exec-migration: handle EINTR in popen_get_buffer()

[Michael S. Tsirkin]

On Mon, Jun 08, 2009 at 07:27:21PM +0300, Uri Lublin wrote:
> Sometimes, upon interrupt, fread returns with no data, and
> the (incoming exec) migration fails.
> 
> Fix by retrying on such a case.
> 
> Signed-off-by: Uri Lublin <uril@redhat.com>
> ---
>  savevm.c |    9 ++++++++-
>  1 files changed, 8 insertions(+), 1 deletions(-)
> 
> diff --git a/savevm.c b/savevm.c
> index 248aea3..df2486d 100644
> --- a/savevm.c
> +++ b/savevm.c
> @@ -215,7 +215,14 @@ static int popen_put_buffer(void *opaque, const uint8_t *buf, int64_t pos, int s
>  static int popen_get_buffer(void *opaque, uint8_t *buf, int64_t pos, int size)
>  {
>      QEMUFilePopen *s = opaque;
> -    return fread(buf, 1, size, s->popen_file);
> +    FILE *fp = s->popen_file;
> +    int bytes;
> +
> +    do {
> +        clearerr(fp);

Would it make sense to only clearerr on EINTR - if we intend to retry?

> +        bytes = fread(buf, 1, size, fp);
> +    } while ((bytes == 0) && ferror(fp) && (errno == EINTR));


This does nothing about partial reads (bytes != 0)
I think it's intentional because the user actually retries
partial reads. Right?
 
> +    return bytes;
>  }
> 
>  static int popen_close(void *opaque)

Looking at qemu_fill_buffer, it seems that it is enough to set
bytes to -EAGAIN. User will then retry. Correct?

-- 
MST

Re: [Qemu-devel] [PATCH] exec-migration: handle EINTR in popen_get_buffer()

[Uri Lublin]

On 06/08/2009 07:55 PM, Blue Swirl wrote:
> On 6/8/09, Uri Lublin<uril@redhat.com>  wrote:
>> Sometimes, upon interrupt, fread returns with no data, and
>>   the (incoming exec) migration fails.
>>
>>   Fix by retrying on such a case.
>
> Maybe a better solution would be to introduce qemu_{f,}{read,write},
> which handle EINTR and partial reads/writes.
>
>

The migration code does not care about partial reads/writes (at this level). 
Data is read /written to/from a buffer. When needed/available we retry.

I can introduce qemu_{f,}{read,write} which only takes care of EINTR (by 
retrying) but the caller would have to handle partial reads/writes (and EAGAIN). 
Would that be helpful ?

Thanks,
     Uri.

Re: [Qemu-devel] [PATCH] exec-migration: handle EINTR in popen_get_buffer()

[Michael S. Tsirkin]

On Mon, Jun 08, 2009 at 08:54:52PM +0300, Michael S. Tsirkin wrote:
> On Mon, Jun 08, 2009 at 07:27:21PM +0300, Uri Lublin wrote:
> > Sometimes, upon interrupt, fread returns with no data, and
> > the (incoming exec) migration fails.
> > 
> > Fix by retrying on such a case.
> > 
> > Signed-off-by: Uri Lublin <uril@redhat.com>
> > ---
> >  savevm.c |    9 ++++++++-
> >  1 files changed, 8 insertions(+), 1 deletions(-)
> > 
> > diff --git a/savevm.c b/savevm.c
> > index 248aea3..df2486d 100644
> > --- a/savevm.c
> > +++ b/savevm.c
> > @@ -215,7 +215,14 @@ static int popen_put_buffer(void *opaque, const uint8_t *buf, int64_t pos, int s
> >  static int popen_get_buffer(void *opaque, uint8_t *buf, int64_t pos, int size)
> >  {
> >      QEMUFilePopen *s = opaque;
> > -    return fread(buf, 1, size, s->popen_file);
> > +    FILE *fp = s->popen_file;
> > +    int bytes;
> > +
> > +    do {
> > +        clearerr(fp);
> 
> Would it make sense to only clearerr on EINTR - if we intend to retry?
> 
> > +        bytes = fread(buf, 1, size, fp);
> > +    } while ((bytes == 0) && ferror(fp) && (errno == EINTR));
> 
> 
> This does nothing about partial reads (bytes != 0)
> I think it's intentional because the user actually retries
> partial reads. Right?
> 
> > +    return bytes;
> >  }
> > 
> >  static int popen_close(void *opaque)
> 
> Looking at qemu_fill_buffer, it seems that it is enough to set
> bytes to -EAGAIN. User will then retry. Correct?

Looking at the code some more, at least qemu_get_byte really seems to
expect a reliable read underneath, so it won't work.  Generally the fact
that qemu_get_byte returns 0 on error seems broken to me maybe we could
fix that and then have a single loop retrying reads. But that's for
another patch.

Acked-by: Michael S. Tsirkin <mst@redhat.com>


-- 
MST

Re: [Qemu-devel] [PATCH] exec-migration: handle EINTR in popen_get_buffer()

[Uri Lublin]

On 06/08/2009 08:54 PM, Michael S. Tsirkin wrote:
> On Mon, Jun 08, 2009 at 07:27:21PM +0300, Uri Lublin wrote:
>> Sometimes, upon interrupt, fread returns with no data, and
>> the (incoming exec) migration fails.
>>
>> Fix by retrying on such a case.
>>
>> Signed-off-by: Uri Lublin<uril@redhat.com>
>> ---
>>   savevm.c |    9 ++++++++-
>>   1 files changed, 8 insertions(+), 1 deletions(-)
>>
>> diff --git a/savevm.c b/savevm.c
>> index 248aea3..df2486d 100644
>> --- a/savevm.c
>> +++ b/savevm.c
>> @@ -215,7 +215,14 @@ static int popen_put_buffer(void *opaque, const uint8_t *buf, int64_t pos, int s
>>   static int popen_get_buffer(void *opaque, uint8_t *buf, int64_t pos, int size)
>>   {
>>       QEMUFilePopen *s = opaque;
>> -    return fread(buf, 1, size, s->popen_file);
>> +    FILE *fp = s->popen_file;
>> +    int bytes;
>> +
>> +    do {
>> +        clearerr(fp);
>
> Would it make sense to only clearerr on EINTR - if we intend to retry?

I am not sure we should be worried about calling clearerr.
I guess we can clearerr only upon ferror()!=0.


>
>> +        bytes = fread(buf, 1, size, fp);
>> +    } while ((bytes == 0)&&  ferror(fp)&&  (errno == EINTR));
>
>
> This does nothing about partial reads (bytes != 0)
> I think it's intentional because the user actually retries
> partial reads. Right?

It is intentional. The caller takes care of partial reads (lazily calling 
fill_buffer when needed).

>
>> +    return bytes;
>>   }
>>
>>   static int popen_close(void *opaque)
>
> Looking at qemu_fill_buffer, it seems that it is enough to set
> bytes to -EAGAIN. User will then retry. Correct?
>

Currently qemu_fill_buffer does not retry (nor its callers).


Thanks for the review,
     Uri.

Re: [Qemu-devel] [PATCH] exec-migration: handle EINTR in popen_get_buffer()

[Blue Swirl]

On 6/9/09, Uri Lublin <uril@redhat.com> wrote:
> On 06/08/2009 07:55 PM, Blue Swirl wrote:
>
> > On 6/8/09, Uri Lublin<uril@redhat.com>  wrote:
> >
> > > Sometimes, upon interrupt, fread returns with no data, and
> > >  the (incoming exec) migration fails.
> > >
> > >  Fix by retrying on such a case.
> > >
> >
> > Maybe a better solution would be to introduce qemu_{f,}{read,write},
> > which handle EINTR and partial reads/writes.
> >
> >
> >
>
>  The migration code does not care about partial reads/writes (at this
> level). Data is read /written to/from a buffer. When needed/available we
> retry.
>
>  I can introduce qemu_{f,}{read,write} which only takes care of EINTR (by
> retrying) but the caller would have to handle partial reads/writes (and
> EAGAIN). Would that be helpful ?

Would we really need two functions, one with partial read/write and
EAGAIN handling and one with only EINTR? There's fread_targphys which
handles partial reads/writes, but not EINTR/EAGAIN, otherwise in many
{f]{read,write} use cases there is no handling for any of these.