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 :)
> 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