On 1/20/21 10:26 AM, Max Reitz wrote: > Hi, > > I’ve run into trouble with Vladimir’s async backup series on MacOS, > namely that iotest 256 fails with qemu exiting because of a SIGUSR2. > > Turns out this is because MacOS (-xcode) uses coroutine-sigaltstack, > when I use this on Linux, I get the same error. > > (You can find the series applied on my block branch e.g. here: > > https://github.com/XanClic/qemu.git block > ) > > Some debugging later I found that the problem seems to be two threads > simultaneously creating a coroutine. It makes sense that this case > would appear with Vladimir’s series and iotest 256, because 256 runs two > backup jobs in two different threads in a transaction, i.e. they’re > launched simultaneously. The async backup series makes backup use many > concurrent coroutines and so by default launches 64+x coroutines when > the backup is started. Thus, the case of two coroutines created > concurrently in two threads is very likely to occur. > > I think the problem is in coroutine-sigaltstack’s qemu_coroutine_new(). > It sets up a SIGUSR2 handler, then changes the signal handling stack, > then raises SIGUSR2, then reverts the signal handling stack and the > SIGUSR2 handler. As far as I’m aware, setting up signal handlers and > changing the signal handling stack are both process-global operations, > and so if two threads do so concurrently, they will interfere with each > other.
Yes, that is absolutely correct - messing with the signal handlers is process-wide. I guess we've been lucky that we haven't been trying to create coroutines in separate threads in the past. > What usually happens is that one thread sets up everything, > while the other is already in the process of reverting its changes: So > the second thread reverts the SIGUSR2 handler to the default, and then > the first thread raises SIGUSR2, thus making qemu exit. > > (Could be worse though. Both threads could set up the sigaltstack, then > both raise SIGUSR2, and then we get one coroutine_trampoline() > invocation in each thread, but both would use the same stack. But I > don’t think I’ve ever seen that happen, presumably because the race time > window is much shorter.) > > Now, this all seems obvious to me, but I’m wondering... If > coroutine-sigaltstack really couldn’t create coroutines concurrently, > why wouldn’t we have noticed before? I mean, this new backup case is > kind of a stress test, yes, but surely we would have seen the problem > already, right? That’s why I’m not sure whether my analysis is correct. I'm not sure if there is anything else going wrong, but you have definitely uncovered a latent problem, and I agree that a mutex is the right way to fix it. > > Anyway, I’ve attached a patch that wraps the whole SIGUSR2 handling > section in a mutex, and that makes 256 pass reliably with Vladimir’s > async backup series. Besides being unsure whether the problem is really > in coroutine-sigaltstack, I also don’t know whether getting out the big > guns and wrapping everything in the mutex is the best solution. So, > it’s an RFC, I guess. > > Max >>From 08d4bb6a98fa731025683f20afe1381291d26031 Mon Sep 17 00:00:00 2001 > From: Max Reitz <mre...@redhat.com> > Date: Wed, 20 Jan 2021 16:59:40 +0100 > Subject: [RFC] coroutine-sigaltstack: Add SIGUSR2 mutex > > Modifying signal handlers or the signal handling stack is a > process-global operation. When two threads run coroutine-sigaltstack's > qemu_coroutine_new() concurrently, thay may interfere with each other, they > e.g.: > > - One of the threads may revert the SIGUSR2 handler back to the default > between the other thread setting up coroutine_trampoline() as the > handler and raising SIGUSR2. That SIGUSR2 will then lead to the > process exiting. > > - Both threads may set up their coroutine stack with sigaltstack() > simultaneously, so that only one of them sticks. Both then raise > SIGUSR2, which goes to each of the threads separately, but both signal > handler invocations will then use the same stack, which cannot work. > > We have to ensure that only one thread at a time can modify the > process-global SIGUSR2 handler and the signal handling stack. To do so, > wrap the whole section where that is done in a mutex. > > Signed-off-by: Max Reitz <mre...@redhat.com> > --- > util/coroutine-sigaltstack.c | 10 ++++++++++ > 1 file changed, 10 insertions(+) Reviewed-by: Eric Blake <ebl...@redhat.com> -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
