On Wed, Feb 26, 2025 at 09:50:15AM +0100, Thomas Huth wrote:
> When compiling QEMU with --enable-trace-backends=simple , the
> iotest 233 is currently hanging. This happens because qemu-nbd
> calls trace_init_backends() first - which causes simpletrace to
> install its writer thread and the atexit() handler - before
> calling fork(). But the simpletrace writer thread is then only
> available in the parent process, not in the child process anymore.
> Thus when the child process exits, its atexit handler waits forever
> on the trace_empty_cond condition to be set by the non-existing
> writer thread, so the process never finishes.
> 
> Fix it by installing a pthread_atfork() handler, too, which
> makes sure that the trace_writeout_enabled variable gets set
> to false again in the child process, so we can use it in the
> atexit() handler to check whether we still need to wait on the
> writer thread or not.
> 
> Signed-off-by: Thomas Huth <th...@redhat.com>
> ---
>  trace/simple.c | 17 ++++++++++++++++-
>  1 file changed, 16 insertions(+), 1 deletion(-)
> 
> diff --git a/trace/simple.c b/trace/simple.c
> index c0aba00cb7f..269bbda69f1 100644
> --- a/trace/simple.c
> +++ b/trace/simple.c
> @@ -380,8 +380,22 @@ void st_print_trace_file_status(void)
>  
>  void st_flush_trace_buffer(void)
>  {
> -    flush_trace_file(true);
> +    flush_trace_file(trace_writeout_enabled);
> +}
> +
> +#ifndef _WIN32
> +static void trace_thread_atfork(void)
> +{
> +    /*
> +     * If we fork, the writer thread does not exist in the child, so
> +     * make sure to allow st_flush_trace_buffer() to clean up correctly.
> +     */
> +    g_mutex_lock(&trace_lock);

And are we sure trace_lock was previously initialized in memory
visible to the thread that is calling the after-fork handler here?

POSIX admits that pthread_atfork() is generally too hard to use
successfully (there are just too many corner cases), and instead
recommends that the only portable thing a multi-threaded app can do
after forking is limit itself to async-signal-safe functions until it
execs a new program.  The other common reason for forking is for
daemonization; there, the advice is to be completely single-threaded
until after the fork() has set up the right environment for the
daemon, at which point the child can then finally go multi-threaded.

And with either of those solutions (Using fork() to spawn a child
process? do nothing unsafe after fork except for what it takes to
reach exec. Using fork() to daemonize? do nothing unsafe before fork)
eliminates the need for using pthread_atfork() in the first place.

At any rate, per your plea elsewhere in the thread, I'll take a look
at deferring the logging init until after qemu-nbd has daemonized.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization:  qemu.org | libguestfs.org


Reply via email to