On Thu, Aug 01, 2019 at 05:09:58AM -0500, Eric Blake wrote:
On 8/1/19 4:12 AM, Richard W.M. Jones wrote:
On Wed, Jul 31, 2019 at 05:01:52PM -0500, Eric Blake wrote:
On 7/31/19 4:31 PM, Eric Blake wrote:
The rate filter is potentially opening fds in one thread while another thread is processing a fork() in the plugin. Although the file is not open for long, we MUST atomically use CLOEXEC to avoid fd leaks. This one is a bit harder to observe using only the sh plugin, because the window is small; you'll have better success at catching the leak by using gdb or recompiling code to insert strategic sleeps.
In fact, I have to tweak this commit message: you CAN'T observe this one with the sh plugin unless you recompile it to use #define THREAD_MODEL NBDKIT_THREAD_MODEL_SERIALIZE_REQUESTS, as well as introducing the timing hacks mentioned above (that's because with our current SERIALIZE_ALL_REQUESTS, there is never more than one thread in filter/plugin code at a time).
The current nbdkit-sh-plugin is only SERIALIZE_ALL_REQUESTS in order to make writing the shell scripts a bit more sane. I believe it could be fully PARALLEL.
Other than the fact that it uses pipe() instead of pipe2(), I'm not seeing any other strong reasons why it can't be parallel. I'll change patch 9 along those lines.
(As an aside: Ideally in future we'll allow the thread model to be specified by the plugin dynamically. It's one of the things I thought I had listed in the TODO file - it wasn't there so I've added it now.)
That's because we already have that: See commit afbcd070 and nearby. So I'll just revert your TODO change :)
Indeed we do. Should add that to all the language plugins at some point ... Rich.
But it does raise an interesting point - if we hit platforms that are unable to support atomic CLOEXEC, one possibility is a patch that forces SERIALIZE_ALL_REQUESTS as the maximum parallelism allowed on that platform (while remaining at our goal of PARALLEL on more competent systems) - once we do that, the lacking systems will be serialized to the point that there is no race window where one thread can fork() while another is obtaining an fd.
Yup. But probably better to encourage those platforms to support atomic CLOEXEC everywhere.
Yes, it would be nice for Haiku to realize how much they are losing out on by not providing it.
-- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
-- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-builder quickly builds VMs from scratch http://libguestfs.org/virt-builder.1.html