[Qemu-devel] [PATCH 0/2] qemu-io: fix aio_read/write problems

[Qemu-devel] [PATCH 0/2] qemu-io: fix aio_read/write problems

[MORITA Kazutaka]

Hi,

This patchset fixes the following qemu-io problems:

 - Qemu-io exits suddenly when we do aio_read/write to drivers which
   use posix-aio-compat.

 - We cannot get the results of aio_read/write if we don't do other
   operations.  This problem occurs when the block driver uses a fd
   handler to get I/O completion.

Thanks,

Kazutaka


MORITA Kazutaka (2):
  qemu-io: retry fgets() when errno is EINTR
  qemu-io: check registered fds in command_loop()

 cmd.c |   56 ++++++++++++++++++++++++++++++++++++++++++--------------
 1 files changed, 42 insertions(+), 14 deletions(-)

[Qemu-devel] [PATCH 1/2] qemu-io: retry fgets() when errno is EINTR

[MORITA Kazutaka]

posix-aio-compat sends a signal in aio operations, so we should
consider that fgets() could be interrupted here.

Signed-off-by: MORITA Kazutaka <morita.kazutaka@lab.ntt.co.jp>
---
 cmd.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/cmd.c b/cmd.c
index 2336334..460df92 100644
--- a/cmd.c
+++ b/cmd.c
@@ -272,7 +272,10 @@ fetchline(void)
 		return NULL;
 	printf("%s", get_prompt());
 	fflush(stdout);
+again:
 	if (!fgets(line, MAXREADLINESZ, stdin)) {
+		if (errno == EINTR)
+			goto again;
 		free(line);
 		return NULL;
 	}
-- 
1.5.6.5

[Qemu-devel] Re: [PATCH 1/2] qemu-io: retry fgets() when errno is EINTR

[Kevin Wolf]

Am 15.06.2010 19:53, schrieb MORITA Kazutaka:
> posix-aio-compat sends a signal in aio operations, so we should
> consider that fgets() could be interrupted here.
> 
> Signed-off-by: MORITA Kazutaka <morita.kazutaka@lab.ntt.co.jp>
> ---
>  cmd.c |    3 +++
>  1 files changed, 3 insertions(+), 0 deletions(-)
> 
> diff --git a/cmd.c b/cmd.c
> index 2336334..460df92 100644
> --- a/cmd.c
> +++ b/cmd.c
> @@ -272,7 +272,10 @@ fetchline(void)
>  		return NULL;
>  	printf("%s", get_prompt());
>  	fflush(stdout);
> +again:
>  	if (!fgets(line, MAXREADLINESZ, stdin)) {
> +		if (errno == EINTR)
> +			goto again;
>  		free(line);
>  		return NULL;
>  	}

This looks like a loop replaced by goto (and braces are missing). What
about this instead?

do {
    ret = fgets(...)
} while (ret == NULL && errno == EINTR)

if (ret == NULL) {
   fail
}

Kevin

Re: [Qemu-devel] Re: [PATCH 1/2] qemu-io: retry fgets() when errno is EINTR

[MORITA Kazutaka]

At Wed, 16 Jun 2010 13:04:47 +0200,
Kevin Wolf wrote:
> 
> Am 15.06.2010 19:53, schrieb MORITA Kazutaka:
> > posix-aio-compat sends a signal in aio operations, so we should
> > consider that fgets() could be interrupted here.
> > 
> > Signed-off-by: MORITA Kazutaka <morita.kazutaka@lab.ntt.co.jp>
> > ---
> >  cmd.c |    3 +++
> >  1 files changed, 3 insertions(+), 0 deletions(-)
> > 
> > diff --git a/cmd.c b/cmd.c
> > index 2336334..460df92 100644
> > --- a/cmd.c
> > +++ b/cmd.c
> > @@ -272,7 +272,10 @@ fetchline(void)
> >  		return NULL;
> >  	printf("%s", get_prompt());
> >  	fflush(stdout);
> > +again:
> >  	if (!fgets(line, MAXREADLINESZ, stdin)) {
> > +		if (errno == EINTR)
> > +			goto again;
> >  		free(line);
> >  		return NULL;
> >  	}
> 
> This looks like a loop replaced by goto (and braces are missing). What
> about this instead?
> 
> do {
>     ret = fgets(...)
> } while (ret == NULL && errno == EINTR)
> 
> if (ret == NULL) {
>    fail
> }
> 

I agree.

However, it seems that my second patch have already solved the
problem.  We register this readline routines as an aio handler now, so
fgets() does not block and cannot return with EINTR.

This patch looks no longer needed, sorry.

Thanks,

Kazutaka

Re: [Qemu-devel] Re: [PATCH 1/2] qemu-io: retry fgets() when errno is EINTR

[Kevin Wolf]

Am 16.06.2010 18:52, schrieb MORITA Kazutaka:
> At Wed, 16 Jun 2010 13:04:47 +0200,
> Kevin Wolf wrote:
>>
>> Am 15.06.2010 19:53, schrieb MORITA Kazutaka:
>>> posix-aio-compat sends a signal in aio operations, so we should
>>> consider that fgets() could be interrupted here.
>>>
>>> Signed-off-by: MORITA Kazutaka <morita.kazutaka@lab.ntt.co.jp>
>>> ---
>>>  cmd.c |    3 +++
>>>  1 files changed, 3 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/cmd.c b/cmd.c
>>> index 2336334..460df92 100644
>>> --- a/cmd.c
>>> +++ b/cmd.c
>>> @@ -272,7 +272,10 @@ fetchline(void)
>>>  		return NULL;
>>>  	printf("%s", get_prompt());
>>>  	fflush(stdout);
>>> +again:
>>>  	if (!fgets(line, MAXREADLINESZ, stdin)) {
>>> +		if (errno == EINTR)
>>> +			goto again;
>>>  		free(line);
>>>  		return NULL;
>>>  	}
>>
>> This looks like a loop replaced by goto (and braces are missing). What
>> about this instead?
>>
>> do {
>>     ret = fgets(...)
>> } while (ret == NULL && errno == EINTR)
>>
>> if (ret == NULL) {
>>    fail
>> }
>>
> 
> I agree.
> 
> However, it seems that my second patch have already solved the
> problem.  We register this readline routines as an aio handler now, so
> fgets() does not block and cannot return with EINTR.
> 
> This patch looks no longer needed, sorry.

Good point. Thanks for having a look.

Kevin

Re: [Qemu-devel] Re: [PATCH 1/2] qemu-io: retry fgets() when errno is EINTRg

[Jamie Lokier]

Kevin Wolf wrote:
> Am 16.06.2010 18:52, schrieb MORITA Kazutaka:
> > At Wed, 16 Jun 2010 13:04:47 +0200,
> > Kevin Wolf wrote:
> >>
> >> Am 15.06.2010 19:53, schrieb MORITA Kazutaka:
> >>> posix-aio-compat sends a signal in aio operations, so we should
> >>> consider that fgets() could be interrupted here.
> >>>
> >>> Signed-off-by: MORITA Kazutaka <morita.kazutaka@lab.ntt.co.jp>
> >>> ---
> >>>  cmd.c |    3 +++
> >>>  1 files changed, 3 insertions(+), 0 deletions(-)
> >>>
> >>> diff --git a/cmd.c b/cmd.c
> >>> index 2336334..460df92 100644
> >>> --- a/cmd.c
> >>> +++ b/cmd.c
> >>> @@ -272,7 +272,10 @@ fetchline(void)
> >>>  		return NULL;
> >>>  	printf("%s", get_prompt());
> >>>  	fflush(stdout);
> >>> +again:
> >>>  	if (!fgets(line, MAXREADLINESZ, stdin)) {
> >>> +		if (errno == EINTR)
> >>> +			goto again;
> >>>  		free(line);
> >>>  		return NULL;
> >>>  	}
> >>
> >> This looks like a loop replaced by goto (and braces are missing). What
> >> about this instead?
> >>
> >> do {
> >>     ret = fgets(...)
> >> } while (ret == NULL && errno == EINTR)
> >>
> >> if (ret == NULL) {
> >>    fail
> >> }
> >>
> > 
> > I agree.
> > 
> > However, it seems that my second patch have already solved the
> > problem.  We register this readline routines as an aio handler now, so
> > fgets() does not block and cannot return with EINTR.
> > 
> > This patch looks no longer needed, sorry.
> 
> Good point. Thanks for having a look.

Anyway, are you sure stdio functions can be interrupted with EINTR?
Linus reminds us that some stdio functions have to retry internally
anyway:

http://comments.gmane.org/gmane.comp.version-control.git/18285

-- Jamie

Re: [Qemu-devel] Re: [PATCH 1/2] qemu-io: retry fgets() when errno is EINTRg

[MORITA Kazutaka]

At Thu, 17 Jun 2010 18:18:18 +0100,
Jamie Lokier wrote:
> 
> Kevin Wolf wrote:
> > Am 16.06.2010 18:52, schrieb MORITA Kazutaka:
> > > At Wed, 16 Jun 2010 13:04:47 +0200,
> > > Kevin Wolf wrote:
> > >>
> > >> Am 15.06.2010 19:53, schrieb MORITA Kazutaka:
> > >>> posix-aio-compat sends a signal in aio operations, so we should
> > >>> consider that fgets() could be interrupted here.
> > >>>
> > >>> Signed-off-by: MORITA Kazutaka <morita.kazutaka@lab.ntt.co.jp>
> > >>> ---
> > >>>  cmd.c |    3 +++
> > >>>  1 files changed, 3 insertions(+), 0 deletions(-)
> > >>>
> > >>> diff --git a/cmd.c b/cmd.c
> > >>> index 2336334..460df92 100644
> > >>> --- a/cmd.c
> > >>> +++ b/cmd.c
> > >>> @@ -272,7 +272,10 @@ fetchline(void)
> > >>>  		return NULL;
> > >>>  	printf("%s", get_prompt());
> > >>>  	fflush(stdout);
> > >>> +again:
> > >>>  	if (!fgets(line, MAXREADLINESZ, stdin)) {
> > >>> +		if (errno == EINTR)
> > >>> +			goto again;
> > >>>  		free(line);
> > >>>  		return NULL;
> > >>>  	}
> > >>
> > >> This looks like a loop replaced by goto (and braces are missing). What
> > >> about this instead?
> > >>
> > >> do {
> > >>     ret = fgets(...)
> > >> } while (ret == NULL && errno == EINTR)
> > >>
> > >> if (ret == NULL) {
> > >>    fail
> > >> }
> > >>
> > > 
> > > I agree.
> > > 
> > > However, it seems that my second patch have already solved the
> > > problem.  We register this readline routines as an aio handler now, so
> > > fgets() does not block and cannot return with EINTR.
> > > 
> > > This patch looks no longer needed, sorry.
> > 
> > Good point. Thanks for having a look.
> 
> Anyway, are you sure stdio functions can be interrupted with EINTR?
> Linus reminds us that some stdio functions have to retry internally
> anyway:
> 
> http://comments.gmane.org/gmane.comp.version-control.git/18285
> 

I think It is another problem whether fgets() retries internally when
a read system call is interrupted.  We should handle EINTR if the
system call can set EINTR.  I think a read() doesn't return with EINTR
if it doesn't block on Linux environment, but it may be not true on
other operating systems.

I send the fixed patch.  I'm not sure this patch is really needed, but
doesn't hurt anyway.

=
posix-aio-compat sends a signal in aio operations, so we should
consider that fgets() could be interrupted here.

Signed-off-by: MORITA Kazutaka <morita.kazutaka@lab.ntt.co.jp>
---
 cmd.c |   14 +++++++++-----
 1 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/cmd.c b/cmd.c
index aee2a38..733bacd 100644
--- a/cmd.c
+++ b/cmd.c
@@ -293,14 +293,18 @@ fetchline(void)
 char *
 fetchline(void)
 {
-	char	*p, *line = malloc(MAXREADLINESZ);
+	char	*p, *line = malloc(MAXREADLINESZ), *ret;
 
 	if (!line)
 		return NULL;
-	if (!fgets(line, MAXREADLINESZ, stdin)) {
-		free(line);
-		return NULL;
-	}
+    do {
+        ret = fgets(line, MAXREADLINESZ, stdin);
+    } while (ret == NULL && errno == EINTR);
+
+    if (ret == NULL) {
+        free(line);
+        return NULL;
+    }
 	p = line + strlen(line);
 	if (p != line && p[-1] == '\n')
 		p[-1] = '\0';
-- 
1.5.6.5

[Qemu-devel] [PATCH 2/2] qemu-io: check registered fds in command_loop()

[MORITA Kazutaka]

Some block drivers use an aio handler and do I/O completion routines
in it.  However, the handler is not invoked if we only do
aio_read/write, because registered fds are not checked at all.

This patch registers a command processing function as a fd handler to
STDIO, and calls qemu_aio_wait() in command_loop().  Any other
handlers can be invoked when user input is idle.

Signed-off-by: MORITA Kazutaka <morita.kazutaka@lab.ntt.co.jp>
---
 cmd.c |   53 +++++++++++++++++++++++++++++++++++++++--------------
 1 files changed, 39 insertions(+), 14 deletions(-)

diff --git a/cmd.c b/cmd.c
index 460df92..2b66e24 100644
--- a/cmd.c
+++ b/cmd.c
@@ -24,6 +24,7 @@
 #include <getopt.h>
 
 #include "cmd.h"
+#include "qemu-aio.h"
 
 #define _(x)	x	/* not gettext support yet */
 
@@ -149,6 +150,37 @@ add_args_command(
 	args_func = af;
 }
 
+static char *get_prompt(void);
+
+static void do_command(void *opaque)
+{
+	int		c;
+	int *done = opaque;
+	char		*input;
+	char		**v;
+	const cmdinfo_t	*ct;
+
+	if ((input = fetchline()) == NULL) {
+		*done = 1;
+		return;
+	}
+	v = breakline(input, &c);
+	if (c) {
+		ct = find_command(v[0]);
+		if (ct)
+			*done = command(ct, c, v);
+		else
+			fprintf(stderr, _("command \"%s\" not found\n"),
+				v[0]);
+	}
+	doneline(input, v);
+
+	if (*done == 0) {
+		printf("%s", get_prompt());
+		fflush(stdout);
+	}
+}
+
 void
 command_loop(void)
 {
@@ -186,20 +218,15 @@ command_loop(void)
 		free(cmdline);
 		return;
 	}
+
+	printf("%s", get_prompt());
+	fflush(stdout);
+
+	qemu_aio_set_fd_handler(STDIN_FILENO, do_command, NULL, NULL, NULL, &done);
 	while (!done) {
-		if ((input = fetchline()) == NULL)
-			break;
-		v = breakline(input, &c);
-		if (c) {
-			ct = find_command(v[0]);
-			if (ct)
-				done = command(ct, c, v);
-			else
-				fprintf(stderr, _("command \"%s\" not found\n"),
-					v[0]);
-		}
-		doneline(input, v);
+		qemu_aio_wait();
 	}
+	qemu_aio_set_fd_handler(STDIN_FILENO, NULL, NULL, NULL, NULL, NULL);
 }
 
 /* from libxcmd/input.c */
@@ -270,8 +297,6 @@ fetchline(void)
 
 	if (!line)
 		return NULL;
-	printf("%s", get_prompt());
-	fflush(stdout);
 again:
 	if (!fgets(line, MAXREADLINESZ, stdin)) {
 		if (errno == EINTR)
-- 
1.5.6.5

[Qemu-devel] Re: [PATCH 2/2] qemu-io: check registered fds in command_loop()

[Kevin Wolf]

Am 15.06.2010 19:53, schrieb MORITA Kazutaka:
> Some block drivers use an aio handler and do I/O completion routines
> in it.  However, the handler is not invoked if we only do
> aio_read/write, because registered fds are not checked at all.
> 
> This patch registers a command processing function as a fd handler to
> STDIO, and calls qemu_aio_wait() in command_loop().  Any other
> handlers can be invoked when user input is idle.
> 
> Signed-off-by: MORITA Kazutaka <morita.kazutaka@lab.ntt.co.jp>

This patch is much nicer than I would have expected it to be!

Thanks, applied to the block branch.

Kevin

[Qemu-devel] Re: [PATCH 2/2] qemu-io: check registered fds in command_loop()

[Kevin Wolf]

Am 15.06.2010 19:53, schrieb MORITA Kazutaka:
> Some block drivers use an aio handler and do I/O completion routines
> in it.  However, the handler is not invoked if we only do
> aio_read/write, because registered fds are not checked at all.
> 
> This patch registers a command processing function as a fd handler to
> STDIO, and calls qemu_aio_wait() in command_loop().  Any other
> handlers can be invoked when user input is idle.
> 
> Signed-off-by: MORITA Kazutaka <morita.kazutaka@lab.ntt.co.jp>

This breaks qemu-iotests. The reason is that synchronous requests get
out of order. Previously, do_aio_readv/writev waited until the request
was completed, and only afterwards the next command was read from stdio.
Now the next command can start during the qemu_aio_wait() that
do_aio_readv/writev uses internally to wait.

So we either need to deregister the fd handler while a command is
running, or (more cleanly) have an async_context_push/pop for any
command except aio_*.

Kevin

[Qemu-devel] [PATCH v2] qemu-io: check registered fds in command_loop()

[MORITA Kazutaka]

Some block drivers use an aio handler and do I/O completion routines
in it.  However, the handler is not invoked if we only do
aio_read/write, because registered fds are not checked at all.

This patch registers an aio handler of STDIO to checks whether we can
read a command without blocking, and calls qemu_aio_wait() in
command_loop().  Any other handlers can be invoked when user input is
idle.

Signed-off-by: MORITA Kazutaka <morita.kazutaka@lab.ntt.co.jp>
---

It seems that the QEMU aio implementation doesn't allow to call
qemu_aio_wait() in the aio handler, so the previous patch is broken.

This patch only checks that STDIO is ready to read a line in the aio
handler, and invokes a command in command_loop().

I think this also fixes the problem which occurs in qemu-iotests.

 cmd.c |   33 ++++++++++++++++++++++++++++++---
 1 files changed, 30 insertions(+), 3 deletions(-)

diff --git a/cmd.c b/cmd.c
index 2336334..db2c9c4 100644
--- a/cmd.c
+++ b/cmd.c
@@ -24,6 +24,7 @@
 #include <getopt.h>
 
 #include "cmd.h"
+#include "qemu-aio.h"
 
 #define _(x)	x	/* not gettext support yet */
 
@@ -149,10 +150,20 @@ add_args_command(
 	args_func = af;
 }
 
+static void prep_fetchline(void *opaque)
+{
+    int *fetchable = opaque;
+
+    qemu_aio_set_fd_handler(STDIN_FILENO, NULL, NULL, NULL, NULL, NULL);
+    *fetchable= 1;
+}
+
+static char *get_prompt(void);
+
 void
 command_loop(void)
 {
-	int		c, i, j = 0, done = 0;
+	int		c, i, j = 0, done = 0, fetchable = 0, prompted = 0;
 	char		*input;
 	char		**v;
 	const cmdinfo_t	*ct;
@@ -186,7 +197,21 @@ command_loop(void)
 		free(cmdline);
 		return;
 	}
+
 	while (!done) {
+        if (!prompted) {
+            printf("%s", get_prompt());
+            fflush(stdout);
+            qemu_aio_set_fd_handler(STDIN_FILENO, prep_fetchline, NULL, NULL,
+                                    NULL, &fetchable);
+            prompted = 1;
+        }
+
+        qemu_aio_wait();
+
+        if (!fetchable) {
+            continue;
+        }
 		if ((input = fetchline()) == NULL)
 			break;
 		v = breakline(input, &c);
@@ -199,7 +224,11 @@ command_loop(void)
 					v[0]);
 		}
 		doneline(input, v);
+
+        prompted = 0;
+        fetchable = 0;
 	}
+    qemu_aio_set_fd_handler(STDIN_FILENO, NULL, NULL, NULL, NULL, NULL);
 }
 
 /* from libxcmd/input.c */
@@ -270,8 +299,6 @@ fetchline(void)
 
 	if (!line)
 		return NULL;
-	printf("%s", get_prompt());
-	fflush(stdout);
 	if (!fgets(line, MAXREADLINESZ, stdin)) {
 		free(line);
 		return NULL;
-- 
1.5.6.5

[Qemu-devel] Re: [PATCH v2] qemu-io: check registered fds in command_loop()

[Kevin Wolf]

Am 20.06.2010 21:03, schrieb MORITA Kazutaka:
> Some block drivers use an aio handler and do I/O completion routines
> in it.  However, the handler is not invoked if we only do
> aio_read/write, because registered fds are not checked at all.
> 
> This patch registers an aio handler of STDIO to checks whether we can
> read a command without blocking, and calls qemu_aio_wait() in
> command_loop().  Any other handlers can be invoked when user input is
> idle.
> 
> Signed-off-by: MORITA Kazutaka <morita.kazutaka@lab.ntt.co.jp>
> ---
> 
> It seems that the QEMU aio implementation doesn't allow to call
> qemu_aio_wait() in the aio handler, so the previous patch is broken.
> 
> This patch only checks that STDIO is ready to read a line in the aio
> handler, and invokes a command in command_loop().
> 
> I think this also fixes the problem which occurs in qemu-iotests.

It does, indeed. Thanks, applied to the block branch.

Kevin