| Message ID | 1348247243-12446-3-git-send-email-lcapitulino@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 09/21/2012 11:07 AM, Luiz Capitulino wrote: > fd_write_vmcore() will indefinitely spin for a non-blocking > file-descriptor that would block. However, if the fd is non-blocking, > how does it make sense to spin? > > Change this behavior to return an error instead. > > Note that this can only happen with an fd provided by a management > application. The fd opened internally by dump-guest-memory is blocking. > > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com> > --- > dump.c | 13 +++---------- > 1 file changed, 3 insertions(+), 10 deletions(-) > > diff --git a/dump.c b/dump.c > index 2bf8d8d..5eea015 100644 > --- a/dump.c > +++ b/dump.c > @@ -100,18 +100,11 @@ static void dump_error(DumpState *s, const char *reason) > static int fd_write_vmcore(void *buf, size_t size, void *opaque) > { > DumpState *s = opaque; > - int fd = s->fd; > size_t writen_size; While you are here, s/writen/written/ in the local variable.
At 09/22/2012 01:07 AM, Luiz Capitulino Wrote: > fd_write_vmcore() will indefinitely spin for a non-blocking > file-descriptor that would block. However, if the fd is non-blocking, > how does it make sense to spin? > > Change this behavior to return an error instead. > > Note that this can only happen with an fd provided by a management > application. The fd opened internally by dump-guest-memory is blocking. > > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com> > --- > dump.c | 13 +++---------- > 1 file changed, 3 insertions(+), 10 deletions(-) > > diff --git a/dump.c b/dump.c > index 2bf8d8d..5eea015 100644 > --- a/dump.c > +++ b/dump.c > @@ -100,18 +100,11 @@ static void dump_error(DumpState *s, const char *reason) > static int fd_write_vmcore(void *buf, size_t size, void *opaque) > { > DumpState *s = opaque; > - int fd = s->fd; > size_t writen_size; > > - /* The fd may be passed from user, and it can be non-blocked */ > - while (size) { > - writen_size = qemu_write_full(fd, buf, size); > - if (writen_size != size && errno != EAGAIN) { Hmm, if the fd is a blocking fd, errno can't be EAGAIN. So the function doesn't spin. What problems do you meet? Thanks Wen Congyang > - return -1; > - } > - > - buf += writen_size; > - size -= writen_size; > + writen_size = qemu_write_full(s->fd, buf, size); > + if (writen_size != size) { > + return -1; > } > > return 0;
On Mon, 24 Sep 2012 14:27:17 +0800 Wen Congyang <wency@cn.fujitsu.com> wrote: > At 09/22/2012 01:07 AM, Luiz Capitulino Wrote: > > fd_write_vmcore() will indefinitely spin for a non-blocking > > file-descriptor that would block. However, if the fd is non-blocking, > > how does it make sense to spin? > > > > Change this behavior to return an error instead. > > > > Note that this can only happen with an fd provided by a management > > application. The fd opened internally by dump-guest-memory is blocking. > > > > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com> > > --- > > dump.c | 13 +++---------- > > 1 file changed, 3 insertions(+), 10 deletions(-) > > > > diff --git a/dump.c b/dump.c > > index 2bf8d8d..5eea015 100644 > > --- a/dump.c > > +++ b/dump.c > > @@ -100,18 +100,11 @@ static void dump_error(DumpState *s, const char *reason) > > static int fd_write_vmcore(void *buf, size_t size, void *opaque) > > { > > DumpState *s = opaque; > > - int fd = s->fd; > > size_t writen_size; > > > > - /* The fd may be passed from user, and it can be non-blocked */ > > - while (size) { > > - writen_size = qemu_write_full(fd, buf, size); > > - if (writen_size != size && errno != EAGAIN) { > > Hmm, if the fd is a blocking fd, errno can't be EAGAIN. So the > function doesn't spin. What problems do you meet? The problem is with non-blocking fds, where spinning isn't correct, for two reasons: 1. If the fd is non-blocking, that means you don't want to block and spinning for a long time will have the same effects 2. Spinning consumes host resources
At 09/24/2012 09:34 PM, Luiz Capitulino Wrote: > On Mon, 24 Sep 2012 14:27:17 +0800 > Wen Congyang <wency@cn.fujitsu.com> wrote: > >> At 09/22/2012 01:07 AM, Luiz Capitulino Wrote: >>> fd_write_vmcore() will indefinitely spin for a non-blocking >>> file-descriptor that would block. However, if the fd is non-blocking, >>> how does it make sense to spin? >>> >>> Change this behavior to return an error instead. >>> >>> Note that this can only happen with an fd provided by a management >>> application. The fd opened internally by dump-guest-memory is blocking. >>> >>> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com> >>> --- >>> dump.c | 13 +++---------- >>> 1 file changed, 3 insertions(+), 10 deletions(-) >>> >>> diff --git a/dump.c b/dump.c >>> index 2bf8d8d..5eea015 100644 >>> --- a/dump.c >>> +++ b/dump.c >>> @@ -100,18 +100,11 @@ static void dump_error(DumpState *s, const char *reason) >>> static int fd_write_vmcore(void *buf, size_t size, void *opaque) >>> { >>> DumpState *s = opaque; >>> - int fd = s->fd; >>> size_t writen_size; >>> >>> - /* The fd may be passed from user, and it can be non-blocked */ >>> - while (size) { >>> - writen_size = qemu_write_full(fd, buf, size); >>> - if (writen_size != size && errno != EAGAIN) { >> >> Hmm, if the fd is a blocking fd, errno can't be EAGAIN. So the >> function doesn't spin. What problems do you meet? > > The problem is with non-blocking fds, where spinning isn't correct, for > two reasons: But, If the fd is non-blocking, errno can't be EAGAIN. So it doesn't spin. Thanks Wen Congyang > > 1. If the fd is non-blocking, that means you don't want to block > and spinning for a long time will have the same effects > > 2. Spinning consumes host resources >
Wen Congyang <wency@cn.fujitsu.com> writes: > At 09/24/2012 09:34 PM, Luiz Capitulino Wrote: >> On Mon, 24 Sep 2012 14:27:17 +0800 >> Wen Congyang <wency@cn.fujitsu.com> wrote: >> >>> At 09/22/2012 01:07 AM, Luiz Capitulino Wrote: >>>> fd_write_vmcore() will indefinitely spin for a non-blocking >>>> file-descriptor that would block. However, if the fd is non-blocking, >>>> how does it make sense to spin? >>>> >>>> Change this behavior to return an error instead. >>>> >>>> Note that this can only happen with an fd provided by a management >>>> application. The fd opened internally by dump-guest-memory is blocking. >>>> >>>> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com> >>>> --- >>>> dump.c | 13 +++---------- >>>> 1 file changed, 3 insertions(+), 10 deletions(-) >>>> >>>> diff --git a/dump.c b/dump.c >>>> index 2bf8d8d..5eea015 100644 >>>> --- a/dump.c >>>> +++ b/dump.c >>>> @@ -100,18 +100,11 @@ static void dump_error(DumpState *s, const char *reason) >>>> static int fd_write_vmcore(void *buf, size_t size, void *opaque) >>>> { >>>> DumpState *s = opaque; >>>> - int fd = s->fd; >>>> size_t writen_size; >>>> >>>> - /* The fd may be passed from user, and it can be non-blocked */ >>>> - while (size) { >>>> - writen_size = qemu_write_full(fd, buf, size); >>>> - if (writen_size != size && errno != EAGAIN) { >>> >>> Hmm, if the fd is a blocking fd, errno can't be EAGAIN. So the >>> function doesn't spin. What problems do you meet? >> >> The problem is with non-blocking fds, where spinning isn't correct, for >> two reasons: > > But, If the fd is non-blocking, errno can't be EAGAIN. So it doesn't spin. I'm afraid you're confused. http://pubs.opengroup.org/onlinepubs/009695399/functions/write.html [EAGAIN] The O_NONBLOCK flag is set for the file descriptor and the thread would be delayed in the write() operation.
At 09/25/2012 05:01 PM, Markus Armbruster Wrote: > Wen Congyang <wency@cn.fujitsu.com> writes: > >> At 09/24/2012 09:34 PM, Luiz Capitulino Wrote: >>> On Mon, 24 Sep 2012 14:27:17 +0800 >>> Wen Congyang <wency@cn.fujitsu.com> wrote: >>> >>>> At 09/22/2012 01:07 AM, Luiz Capitulino Wrote: >>>>> fd_write_vmcore() will indefinitely spin for a non-blocking >>>>> file-descriptor that would block. However, if the fd is non-blocking, >>>>> how does it make sense to spin? >>>>> >>>>> Change this behavior to return an error instead. >>>>> >>>>> Note that this can only happen with an fd provided by a management >>>>> application. The fd opened internally by dump-guest-memory is blocking. >>>>> >>>>> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com> >>>>> --- >>>>> dump.c | 13 +++---------- >>>>> 1 file changed, 3 insertions(+), 10 deletions(-) >>>>> >>>>> diff --git a/dump.c b/dump.c >>>>> index 2bf8d8d..5eea015 100644 >>>>> --- a/dump.c >>>>> +++ b/dump.c >>>>> @@ -100,18 +100,11 @@ static void dump_error(DumpState *s, const char *reason) >>>>> static int fd_write_vmcore(void *buf, size_t size, void *opaque) >>>>> { >>>>> DumpState *s = opaque; >>>>> - int fd = s->fd; >>>>> size_t writen_size; >>>>> >>>>> - /* The fd may be passed from user, and it can be non-blocked */ >>>>> - while (size) { >>>>> - writen_size = qemu_write_full(fd, buf, size); >>>>> - if (writen_size != size && errno != EAGAIN) { >>>> >>>> Hmm, if the fd is a blocking fd, errno can't be EAGAIN. So the >>>> function doesn't spin. What problems do you meet? >>> >>> The problem is with non-blocking fds, where spinning isn't correct, for >>> two reasons: >> >> But, If the fd is non-blocking, errno can't be EAGAIN. So it doesn't spin. > > I'm afraid you're confused. > > http://pubs.opengroup.org/onlinepubs/009695399/functions/write.html > > [EAGAIN] The O_NONBLOCK flag is set for the file descriptor and the > thread would be delayed in the write() operation. > Ahh, you are right. Thanks Wen Congyang
At 09/24/2012 09:34 PM, Luiz Capitulino Wrote: > On Mon, 24 Sep 2012 14:27:17 +0800 > Wen Congyang <wency@cn.fujitsu.com> wrote: > >> At 09/22/2012 01:07 AM, Luiz Capitulino Wrote: >>> fd_write_vmcore() will indefinitely spin for a non-blocking >>> file-descriptor that would block. However, if the fd is non-blocking, >>> how does it make sense to spin? >>> >>> Change this behavior to return an error instead. >>> >>> Note that this can only happen with an fd provided by a management >>> application. The fd opened internally by dump-guest-memory is blocking. >>> >>> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com> >>> --- >>> dump.c | 13 +++---------- >>> 1 file changed, 3 insertions(+), 10 deletions(-) >>> >>> diff --git a/dump.c b/dump.c >>> index 2bf8d8d..5eea015 100644 >>> --- a/dump.c >>> +++ b/dump.c >>> @@ -100,18 +100,11 @@ static void dump_error(DumpState *s, const char *reason) >>> static int fd_write_vmcore(void *buf, size_t size, void *opaque) >>> { >>> DumpState *s = opaque; >>> - int fd = s->fd; >>> size_t writen_size; >>> >>> - /* The fd may be passed from user, and it can be non-blocked */ >>> - while (size) { >>> - writen_size = qemu_write_full(fd, buf, size); >>> - if (writen_size != size && errno != EAGAIN) { >> >> Hmm, if the fd is a blocking fd, errno can't be EAGAIN. So the >> function doesn't spin. What problems do you meet? > > The problem is with non-blocking fds, where spinning isn't correct, for > two reasons: > > 1. If the fd is non-blocking, that means you don't want to block > and spinning for a long time will have the same effects > > 2. Spinning consumes host resources > If so, I agree with it, and the patch looks fine to me Thanks Wen Congyang
diff --git a/dump.c b/dump.c index 2bf8d8d..5eea015 100644 --- a/dump.c +++ b/dump.c @@ -100,18 +100,11 @@ static void dump_error(DumpState *s, const char *reason) static int fd_write_vmcore(void *buf, size_t size, void *opaque) { DumpState *s = opaque; - int fd = s->fd; size_t writen_size; - /* The fd may be passed from user, and it can be non-blocked */ - while (size) { - writen_size = qemu_write_full(fd, buf, size); - if (writen_size != size && errno != EAGAIN) { - return -1; - } - - buf += writen_size; - size -= writen_size; + writen_size = qemu_write_full(s->fd, buf, size); + if (writen_size != size) { + return -1; } return 0;
fd_write_vmcore() will indefinitely spin for a non-blocking file-descriptor that would block. However, if the fd is non-blocking, how does it make sense to spin? Change this behavior to return an error instead. Note that this can only happen with an fd provided by a management application. The fd opened internally by dump-guest-memory is blocking. Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com> --- dump.c | 13 +++---------- 1 file changed, 3 insertions(+), 10 deletions(-)