[Qemu-devel] [PATCH] Revert "posix-aio-compat: avoid signal race when spawning a thread"

[Qemu-devel] [PATCH] Revert "posix-aio-compat: avoid signal race when spawning a thread"

[Michael S. Tsirkin]

This reverts commit ee3993069ff55fa6f1c64daf1e09963e340db8e4.

With commit ee3993069ff55fa6f1c64daf1e09963e340db8e4,
winxp installation on a raw format file fails
during disk format, with a message "your
disk may be damaged".

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Cc: malc <av1474@comtv.ru>
---

malc, care to describe what the problem you
are solving here is?

All, could everyone please start being nice and post patches
and give time for review before applying?
Thanks!

 posix-aio-compat.c |   14 +++++---------
 1 files changed, 5 insertions(+), 9 deletions(-)

diff --git a/posix-aio-compat.c b/posix-aio-compat.c
index 400d898..68cbec8 100644
--- a/posix-aio-compat.c
+++ b/posix-aio-compat.c
@@ -301,9 +301,14 @@ static size_t handle_aiocb_rw(struct qemu_paiocb *aiocb)
 static void *aio_thread(void *unused)
 {
     pid_t pid;
+    sigset_t set;
 
     pid = getpid();
 
+    /* block all signals */
+    if (sigfillset(&set)) die("sigfillset");
+    if (sigprocmask(SIG_BLOCK, &set, NULL)) die("sigprocmask");
+
     while (1) {
         struct qemu_paiocb *aiocb;
         size_t ret = 0;
@@ -364,18 +369,9 @@ static void *aio_thread(void *unused)
 
 static void spawn_thread(void)
 {
-    sigset_t set, oldset;
-
     cur_threads++;
     idle_threads++;
-
-    /* block all signals */
-    if (sigfillset(&set)) die("sigfillset");
-    if (sigprocmask(SIG_SETMASK, &set, &oldset)) die("sigprocmask");
-
     thread_create(&thread_id, &attr, aio_thread, NULL);
-
-    if (sigprocmask(SIG_SETMASK, &oldset, NULL)) die("sigprocmask restore");
 }
 
 static void qemu_paio_submit(struct qemu_paiocb *aiocb)
-- 
1.6.5.rc2

[Qemu-devel] Re: [PATCH] Revert "posix-aio-compat: avoid signal race when spawning a thread"

[malc]

On Wed, 7 Oct 2009, Michael S. Tsirkin wrote:

> This reverts commit ee3993069ff55fa6f1c64daf1e09963e340db8e4.
> 
> With commit ee3993069ff55fa6f1c64daf1e09963e340db8e4,
> winxp installation on a raw format file fails
> during disk format, with a message "your
> disk may be damaged".
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> Cc: malc <av1474@comtv.ru>
> ---
> 
> malc, care to describe what the problem you
> are solving here is?

New thread is spawned, SIGALRM arrives, kernel picks up this new thread
since it's mask allows it - bad thigs happen.

> 
> All, could everyone please start being nice and post patches
> and give time for review before applying?
> Thanks!

There's nothing to review in this particular case, the thing was plain
broken, besides same thing was already discussed between me and
Andrzej, see for instance:
http://www.mail-archive.com/qemu-devel@nongnu.org/msg09542.html

Needless to say the revert is broken also.

-- 
mailto:av1474@comtv.ru

[Qemu-devel] Re: [PATCH] Revert "posix-aio-compat: avoid signal race when spawning a thread"

[Anthony Liguori]

> There's nothing to review in this particular case, the thing was plain
> broken, besides same thing was already discussed between me and
> Andrzej, see for instance:
> http://www.mail-archive.com/qemu-devel@nongnu.org/msg09542.html
>
> Needless to say the revert is broken also.
>   

Yeah, I understand the race the original commit fixes but I don't 
understand how the original commit broke WinXP.

Do you have further insight Michael?  Blindly reverting something that 
appears right because of a regression is likely just papering over a 
bigger problem.

Regards,

Anthony Liguori

[Qemu-devel] Re: [PATCH] Revert "posix-aio-compat: avoid signal race when spawning a thread"

[Michael S. Tsirkin]

On Wed, Oct 07, 2009 at 11:28:21AM -0500, Anthony Liguori wrote:
>
>> There's nothing to review in this particular case, the thing was plain
>> broken, besides same thing was already discussed between me and
>> Andrzej, see for instance:
>> http://www.mail-archive.com/qemu-devel@nongnu.org/msg09542.html
>>
>> Needless to say the revert is broken also.
>>   
>
> Yeah, I understand the race the original commit fixes but I don't  
> understand how the original commit broke WinXP.
>
> Do you have further insight Michael?

Nope, I just bisected and then reverted and tested that this helps.

>  Blindly reverting something that  
> appears right because of a regression is likely just papering over a  
> bigger problem.
>
> Regards,
>
> Anthony Liguori
>

[Qemu-devel] Re: [PATCH] Revert "posix-aio-compat: avoid signal race when spawning a thread"

[Michael S. Tsirkin]

On Wed, Oct 07, 2009 at 11:28:21AM -0500, Anthony Liguori wrote:
>
>> There's nothing to review in this particular case, the thing was plain
>> broken, besides same thing was already discussed between me and
>> Andrzej, see for instance:
>> http://www.mail-archive.com/qemu-devel@nongnu.org/msg09542.html
>>
>> Needless to say the revert is broken also.
>>   
>
> Yeah, I understand the race the original commit fixes but I don't  
> understand how the original commit broke WinXP.
>
> Do you have further insight Michael?

My guess is that SIGALARM gets lost, this somehow leads to
disk request to timeout. Possible?

> Blindly reverting something that  appears right because of a
> regression is likely just papering over a  bigger problem.
>
> Regards,
>
> Anthony Liguori
>

[Qemu-devel] Re: [PATCH] Revert "posix-aio-compat: avoid signal race when spawning a thread"

[malc]

On Wed, 7 Oct 2009, Michael S. Tsirkin wrote:

> On Wed, Oct 07, 2009 at 11:28:21AM -0500, Anthony Liguori wrote:
> >
> >> There's nothing to review in this particular case, the thing was plain
> >> broken, besides same thing was already discussed between me and
> >> Andrzej, see for instance:
> >> http://www.mail-archive.com/qemu-devel@nongnu.org/msg09542.html
> >>
> >> Needless to say the revert is broken also.
> >>   
> >
> > Yeah, I understand the race the original commit fixes but I don't  
> > understand how the original commit broke WinXP.
> >
> > Do you have further insight Michael?
> 
> My guess is that SIGALARM gets lost, this somehow leads to
> disk request to timeout. Possible?

Being delayed - possible, being lost - violation of the spec.

> 
> > Blindly reverting something that  appears right because of a
> > regression is likely just papering over a  bigger problem.
> >
> > Regards,
> >
> > Anthony Liguori
> >
> 

-- 
mailto:av1474@comtv.ru

[Qemu-devel] Re: [PATCH] Revert "posix-aio-compat: avoid signal race when spawning a thread"

[Michael S. Tsirkin]

On Thu, Oct 08, 2009 at 12:54:00AM +0400, malc wrote:
> On Wed, 7 Oct 2009, Michael S. Tsirkin wrote:
> 
> > On Wed, Oct 07, 2009 at 11:28:21AM -0500, Anthony Liguori wrote:
> > >
> > >> There's nothing to review in this particular case, the thing was plain
> > >> broken, besides same thing was already discussed between me and
> > >> Andrzej, see for instance:
> > >> http://www.mail-archive.com/qemu-devel@nongnu.org/msg09542.html
> > >>
> > >> Needless to say the revert is broken also.
> > >>   
> > >
> > > Yeah, I understand the race the original commit fixes but I don't  
> > > understand how the original commit broke WinXP.
> > >
> > > Do you have further insight Michael?
> > 
> > My guess is that SIGALARM gets lost, this somehow leads to
> > disk request to timeout. Possible?
> 
> Being delayed - possible, being lost - violation of the spec.

I see this:

	The  use of sigprocmask() is unspecified in a
	multithreaded process; see pthread_sigmask(3).

Does it matter?

> > 
> > > Blindly reverting something that  appears right because of a
> > > regression is likely just papering over a  bigger problem.
> > >
> > > Regards,
> > >
> > > Anthony Liguori
> > >
> > 
> 
> -- 
> mailto:av1474@comtv.ru

[Qemu-devel] Re: [PATCH] Revert "posix-aio-compat: avoid signal race when spawning a thread"

[malc]

On Wed, 7 Oct 2009, Michael S. Tsirkin wrote:

> On Thu, Oct 08, 2009 at 12:54:00AM +0400, malc wrote:
> > On Wed, 7 Oct 2009, Michael S. Tsirkin wrote:
> > 
> > > On Wed, Oct 07, 2009 at 11:28:21AM -0500, Anthony Liguori wrote:
> > > >
> > > >> There's nothing to review in this particular case, the thing was plain
> > > >> broken, besides same thing was already discussed between me and
> > > >> Andrzej, see for instance:
> > > >> http://www.mail-archive.com/qemu-devel@nongnu.org/msg09542.html
> > > >>
> > > >> Needless to say the revert is broken also.
> > > >>   
> > > >
> > > > Yeah, I understand the race the original commit fixes but I don't  
> > > > understand how the original commit broke WinXP.
> > > >
> > > > Do you have further insight Michael?
> > > 
> > > My guess is that SIGALARM gets lost, this somehow leads to
> > > disk request to timeout. Possible?
> > 
> > Being delayed - possible, being lost - violation of the spec.
> 
> I see this:
> 
> 	The  use of sigprocmask() is unspecified in a
> 	multithreaded process; see pthread_sigmask(3).
> 
> Does it matter?

One of the patches i've asked you to try today replaced sigprocmask with
pthread_sigmask, you've said it did nothing. In any case, strictly
speaking, the code is wrong, so yes it does matter in theory.

[..snip..]

-- 
mailto:av1474@comtv.ru

Re: [Qemu-devel] Re: [PATCH] Revert "posix-aio-compat: avoid signal race when spawning a thread"

[Jamie Lokier]

malc wrote:
> > 	The  use of sigprocmask() is unspecified in a
> > 	multithreaded process; see pthread_sigmask(3).
> > 
> > Does it matter?
> 
> One of the patches i've asked you to try today replaced sigprocmask with
> pthread_sigmask, you've said it did nothing. In any case, strictly
> speaking, the code is wrong, so yes it does matter in theory.

It won't matter on a Linux host (they are the same), but
pthread_sigmask should be used because it's Right(tm) and it could
make a difference on some other host.

-- Jamie

Re: [Qemu-devel] Re: [PATCH] Revert "posix-aio-compat: avoid signal race when spawning a thread"

[malc]

On Thu, 8 Oct 2009, Jamie Lokier wrote:

> malc wrote:
> > > 	The  use of sigprocmask() is unspecified in a
> > > 	multithreaded process; see pthread_sigmask(3).
> > > 
> > > Does it matter?
> > 
> > One of the patches i've asked you to try today replaced sigprocmask with
> > pthread_sigmask, you've said it did nothing. In any case, strictly
> > speaking, the code is wrong, so yes it does matter in theory.
> 
> It won't matter on a Linux host (they are the same), but
> pthread_sigmask should be used because it's Right(tm) and it could
> make a difference on some other host.

What made you think i'm of a different opinion?

-- 
mailto:av1474@comtv.ru

Re: [Qemu-devel] Re: [PATCH] Revert "posix-aio-compat: avoid signal race when spawning a thread"

[Jamie Lokier]

malc wrote:
> On Thu, 8 Oct 2009, Jamie Lokier wrote:
> 
> > malc wrote:
> > > > 	The  use of sigprocmask() is unspecified in a
> > > > 	multithreaded process; see pthread_sigmask(3).
> > > > 
> > > > Does it matter?
> > > 
> > > One of the patches i've asked you to try today replaced sigprocmask with
> > > pthread_sigmask, you've said it did nothing. In any case, strictly
> > > speaking, the code is wrong, so yes it does matter in theory.
> > 
> > It won't matter on a Linux host (they are the same), but
> > pthread_sigmask should be used because it's Right(tm) and it could
> > make a difference on some other host.
> 
> What made you think i'm of a different opinion?

The fact you asked Michael to test a patch which replaced sigprocmask
with pthread_sigmask.

-- Jamie

Re: [Qemu-devel] Re: [PATCH] Revert "posix-aio-compat: avoid signal race when spawning a thread"

[malc]

On Thu, 8 Oct 2009, Jamie Lokier wrote:

> malc wrote:
> > On Thu, 8 Oct 2009, Jamie Lokier wrote:
> > 
> > > malc wrote:
> > > > > 	The  use of sigprocmask() is unspecified in a
> > > > > 	multithreaded process; see pthread_sigmask(3).
> > > > > 
> > > > > Does it matter?
> > > > 
> > > > One of the patches i've asked you to try today replaced sigprocmask with
> > > > pthread_sigmask, you've said it did nothing. In any case, strictly
> > > > speaking, the code is wrong, so yes it does matter in theory.
> > > 
> > > It won't matter on a Linux host (they are the same), but
> > > pthread_sigmask should be used because it's Right(tm) and it could
> > > make a difference on some other host.
> > 
> > What made you think i'm of a different opinion?
> 
> The fact you asked Michael to test a patch which replaced sigprocmask
> with pthread_sigmask.

That made you think i'm somehow in favour of sigprocmask? I'm confused.

-- 
mailto:av1474@comtv.ru

Re: [Qemu-devel] Re: [PATCH] Revert "posix-aio-compat: avoid signal race when spawning a thread"

[Michael S. Tsirkin]

On Thu, Oct 08, 2009 at 04:15:27PM +0100, Jamie Lokier wrote:
> malc wrote:
> > On Thu, 8 Oct 2009, Jamie Lokier wrote:
> > 
> > > malc wrote:
> > > > > 	The  use of sigprocmask() is unspecified in a
> > > > > 	multithreaded process; see pthread_sigmask(3).
> > > > > 
> > > > > Does it matter?
> > > > 
> > > > One of the patches i've asked you to try today replaced sigprocmask with
> > > > pthread_sigmask, you've said it did nothing. In any case, strictly
> > > > speaking, the code is wrong, so yes it does matter in theory.

BTW, tried looking at the code.
I saw some (unrelated) issues:

static void aio_signal_handler(int signum)
{
    if (posix_aio_state) {
        char byte = 0;

        write(posix_aio_state->wfd, &byte, sizeof(byte));
    }

    qemu_service_io();
}

And qemu_service_io does a *ton* of things.
Questions:
-  Do we need the call to qemu_service_io? Seems to
   behave the same with and without it.
-  Are all of the data structures touched by qemu_service_io
   protected by blocking signals before access?

Also:
	- let's use signalfd on linux, if available?
	- for SIGALARM, maybe timerfd?

Thanks,

-- 
MST

Re: [Qemu-devel] Re: [PATCH] Revert "posix-aio-compat: avoid signal race when spawning a thread"

[Anthony Liguori]

Michael S. Tsirkin wrote:
> BTW, tried looking at the code.
> I saw some (unrelated) issues:
>
> static void aio_signal_handler(int signum)
> {
>     if (posix_aio_state) {
>         char byte = 0;
>
>         write(posix_aio_state->wfd, &byte, sizeof(byte));
>     }
>
>     qemu_service_io();
> }
>
> And qemu_service_io does a *ton* of things.
>   

It just does a cpu_exit().

> Questions:
> -  Do we need the call to qemu_service_io? Seems to
>    behave the same with and without it.
>   

Where you run into trouble is when you have a guest that has no periodic 
timer  and needs a mechanism to break out of the tcg loop whenever an IO 
operation completes.  See


Author: aliguori <aliguori@c046a42c-6fe2-441c-8c8c-71466251a162>
Date:   Wed Oct 8 19:50:24 2008 +0000

    Fix IO performance regression in sparc

> -  Are all of the data structures touched by qemu_service_io
>    protected by blocking signals before access?
>   

cpu_exit is pretty careful to not do anything that could be racey.

> Also:
> 	- let's use signalfd on linux, if available?
>   

signalfd doesn't help.  In fact, we originally used signalfd and it 
caused a regression.

> 	- for SIGALARM, maybe timerfd?
>   

Or just get rid of SIGALARM...

Regards,

Anthony Liguori

Re: [Qemu-devel] Re: [PATCH] Revert "posix-aio-compat: avoid signal race when spawning a thread"

[Anthony Liguori]

Michael S. Tsirkin wrote:
> BTW, tried looking at the code.
> I saw some (unrelated) issues:
>
> static void aio_signal_handler(int signum)
> {
>     if (posix_aio_state) {
>         char byte = 0;
>
>         write(posix_aio_state->wfd, &byte, sizeof(byte));
>     }
>
>     qemu_service_io();
> }
>
> And qemu_service_io does a *ton* of things.
>   

It just does a cpu_exit().

> Questions:
> -  Do we need the call to qemu_service_io? Seems to
>    behave the same with and without it.
>   

Where you run into trouble is when you have a guest that has no periodic 
timer  and needs a mechanism to break out of the tcg loop whenever an IO 
operation completes.  See


Author: aliguori <aliguori@c046a42c-6fe2-441c-8c8c-71466251a162>
Date:   Wed Oct 8 19:50:24 2008 +0000

    Fix IO performance regression in sparc

> -  Are all of the data structures touched by qemu_service_io
>    protected by blocking signals before access?
>   

cpu_exit is pretty careful to not do anything that could be racey.

> Also:
> 	- let's use signalfd on linux, if available?
>   

signalfd doesn't help.  In fact, we originally used signalfd and it 
caused a regression.

> 	- for SIGALARM, maybe timerfd?
>   

Or just get rid of SIGALARM...

Regards,

Anthony Liguori

Re: [Qemu-devel] Re: [PATCH] Revert "posix-aio-compat: avoid signal race when spawning a thread"

[Jamie Lokier]

malc wrote:
> On Thu, 8 Oct 2009, Jamie Lokier wrote:
> 
> > malc wrote:
> > > On Thu, 8 Oct 2009, Jamie Lokier wrote:
> > > 
> > > > malc wrote:
> > > > > > 	The  use of sigprocmask() is unspecified in a
> > > > > > 	multithreaded process; see pthread_sigmask(3).
> > > > > > 
> > > > > > Does it matter?
> > > > > 
> > > > > One of the patches i've asked you to try today replaced sigprocmask with
> > > > > pthread_sigmask, you've said it did nothing. In any case, strictly
> > > > > speaking, the code is wrong, so yes it does matter in theory.
> > > > 
> > > > It won't matter on a Linux host (they are the same), but
> > > > pthread_sigmask should be used because it's Right(tm) and it could
> > > > make a difference on some other host.
> > > 
> > > What made you think i'm of a different opinion?
> > 
> > The fact you asked Michael to test a patch which replaced sigprocmask
> > with pthread_sigmask.
> 
> That made you think i'm somehow in favour of sigprocmask? I'm confused.

It didn't make me think you're in favour of sigprocmask.

It made me think you thought there was a difference between them that
would affect the bug.

Anyways, I think we're done with this subthread :-)

-- Jamie

Re: [Qemu-devel] Re: [PATCH] Revert "posix-aio-compat: avoid signal race when spawning a thread"

[Jamie Lokier]

Michael S. Tsirkin wrote:
> And qemu_service_io does a *ton* of things.
> Questions:
> -  Do we need the call to qemu_service_io? Seems to
>    behave the same with and without it.
> -  Are all of the data structures touched by qemu_service_io
>    protected by blocking signals before access?

Plus:

-  Are all the things done by qemu_service_io async-signal-safe?
   E.g. any use of pthread primitives is not async-signal-safe.
   I don't know if thread-local storage is.

-- Jamie

Re: [Qemu-devel] Re: [PATCH] Revert "posix-aio-compat: avoid signal race when spawning a thread"

[malc]

On Fri, 9 Oct 2009, Jamie Lokier wrote:

> malc wrote:
> > On Thu, 8 Oct 2009, Jamie Lokier wrote:
> > 
> > > malc wrote:
> > > > On Thu, 8 Oct 2009, Jamie Lokier wrote:
> > > > 
> > > > > malc wrote:
> > > > > > > 	The  use of sigprocmask() is unspecified in a
> > > > > > > 	multithreaded process; see pthread_sigmask(3).
> > > > > > > 
> > > > > > > Does it matter?
> > > > > > 
> > > > > > One of the patches i've asked you to try today replaced sigprocmask with
> > > > > > pthread_sigmask, you've said it did nothing. In any case, strictly
> > > > > > speaking, the code is wrong, so yes it does matter in theory.
> > > > > 
> > > > > It won't matter on a Linux host (they are the same), but
> > > > > pthread_sigmask should be used because it's Right(tm) and it could
> > > > > make a difference on some other host.
> > > > 
> > > > What made you think i'm of a different opinion?
> > > 
> > > The fact you asked Michael to test a patch which replaced sigprocmask
> > > with pthread_sigmask.
> > 
> > That made you think i'm somehow in favour of sigprocmask? I'm confused.
> 
> It didn't make me think you're in favour of sigprocmask.
> 
> It made me think you thought there was a difference between them that
> would affect the bug.
> 
> Anyways, I think we're done with this subthread :-)

Goooood :)

-- 
mailto:av1474@comtv.ru