similar to: [nbdkit PATCH v2 00/17] fd leak safety

On Fri, Aug 02, 2019 at 02:26:11PM -0500, Eric Blake wrote: > Allow a plugin field to declare whether a parallel plugin can tolerate > windows where fds are not CLOEXEC, or must take precautions to avoid > leaking fds if the plugin may fork. For safety reasons, the flag > defaults to off, but many in-tree plugins can set it to on (most > commonly because they don't fork after

On platforms that lack atomic CLOEXEC, it's simpler to just always force serialization (no client thread will be executing when nbdkit itself is creating a new file descriptor) than to audit which plugins actually care about not getting any leaked fds. We have one final function that we need to use for atomic CLOEXEC; the next patch will actually put accept4() to use. Maybe this penalization

There's enough here to need a review; some of it probably needs backporting to stable-1.12. This probably breaks tests on Haiku or other platforms that have not been as on-the-ball about atomic CLOEXEC; feel free to report issues that arise, and I'll help come up with workarounds (even if we end up leaving a rare fd leak on less-capable systems). Meanwhile, I'm still working on my

On Fri, Aug 02, 2019 at 02:26:16PM -0500, Eric Blake wrote: > When we first created the sh plugin, we copied the thread model used > in most other language bindings of SERIALIZE_ALL_REQUESTS, because it > was easier to reason about and there was no way for a script to > override our choice. However, we've since added support for scripts > to request a tighter model when needed,

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

On 8/1/19 4:15 AM, Richard W.M. Jones wrote: > On Thu, Aug 01, 2019 at 10:06:01AM +0100, Richard W.M. Jones wrote: >> As far as I can see Haiku (hrev52698) has O_CLOEXEC but NOT >> SOCK_CLOEXEC. As this version is a little old I'll do an update and >> see if newer versions support it. > > I'm on hrev53313+1 which also doesn't appear to have SOCK_CLOEXEC

On Wed, Aug 05, 2020 at 02:39:44PM +0300, Nir Soffer wrote: > Can we use something like the file plugin? thread pool of workers, > each keeping open vddk handle, and serving requests in parallel from > the same nbd socket? Yes, but this isn't implemented in the plugins, it's implemented in the server. The server always uses a thread pool, but plugins can opt for more or less

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

On Wed, Aug 05, 2020 at 03:40:43PM +0300, Nir Soffer wrote: > On Wed, Aug 5, 2020 at 2:58 PM Richard W.M. Jones <rjones@redhat.com> wrote: > > > > On Wed, Aug 05, 2020 at 02:39:44PM +0300, Nir Soffer wrote: > > > Can we use something like the file plugin? thread pool of workers, > > > each keeping open vddk handle, and serving requests in parallel from >

[NB: Adding PUBLIC mailing list because this is upstream discussion] On Mon, Aug 03, 2020 at 06:27:04PM +0100, Richard W.M. Jones wrote: > On Mon, Aug 03, 2020 at 06:03:23PM +0300, Nir Soffer wrote: > > On Mon, Aug 3, 2020 at 5:47 PM Richard W.M. Jones <rjones@redhat.com> wrote: > > All this make sense, but when we upload 10 disks we have 10 connections > > but still we

Rich pointed out that making thread_model dynamic even for plugins makes some sense, so here is the code for doing it for 'sh'. I'm less confident on how to do it for OCaml and Rust (not to mention that those allow the plugin to already compile in their model, rather than the language binding glue forcing a model). The other languages (lua, perl, python, ruby) still need to be

When we first created the sh plugin, we copied the thread model used in most other language bindings of SERIALIZE_ALL_REQUESTS, because it was easier to reason about and there was no way for a script to override our choice. However, we've since added support for scripts to request a tighter model when needed, and we've also just fixed remaining bugs about fds inadvertently leaked into the

We noticed while writing various libnbd tests that when the delay filter is in use, there are scenarios where we had to resort to SIGKILL to get rid of nbdkit, because it was non-responsive to SIGINT. I'm still trying to figure out the best way to add testsuite coverage of this, but already proved to myself that it works from the command line, under two scenarios that both used to cause long

When I added .can_FOO caching in 1.16, I missed the case that the sh plugin itself was calling .can_flush twice in some situations (in order to default .can_fua). Then right after, I regressed it to call .can_zero twice (in order to default .can_fast_zero). I also missed that .thread_model could use better caching, because at the time, I did not add testsuite coverage. Fix that now. Eric Blake

With this, I am finally able to get the nbd plugin to do out-of-order responses to the client. Once this series goes in, we should be ready for Rich to cut a release. Eric Blake (4): nbd: Split reading into separate thread nbd: Protect writes with mutex nbd: Enable parallel handling tests: Test parallel nbd behavior plugins/nbd/nbd.c | 217

I'm still working on the interleaving (and Rich reminded me on IRC that we still don't have THREAD_MODEL_PARALLEL working anywhere yet, anyways). Since nbdkit doesn't really have a parallel plugin yet, my testing on that front will have to use qemu-nbd as the original server, as well as qemu-io as the driver (qemu-io's aio_read and aio_write commands can be used to trigger

On Wed, Jul 31, 2019 at 04:31:32PM -0500, Eric Blake wrote: > This reverts commit 25206df20275aeff346d9b86adf5e9be99cc9e43. > > An upcoming patch wants to ensure no leaked fds from the server to a > child process. POSIX has required O_CLOEXEC since 2008, and although > current POSIX doesn't yet specify full atomic interfaces everywhere > such as SOCK_CLOEXEC, it does have

The patches following do indeed allow you to compile nbdkit.exe, but it does not actually work yet. I'm posting this experimental series more as a work in progress and to get feedback. Note this does not require Windows itself to build or test. You can cross-compile it using mingw64-* packages on Fedora or Debian, and test it [spoiler alert: it fails] using Wine. Rich.

On Mon, May 20, 2019 at 07:30:31AM -0500, Eric Blake wrote: > +=head2 C<.thread_model> > + > + int thread_model (void) > + > +This optional callback is called after all the configuration has been > +passed to the plugin. It can be used to force a stricter thread model > +based on configuration, compared to C<THREAD_MODEL>. See L</THREADS> > +below for

Allow a plugin field to declare whether a parallel plugin can tolerate windows where fds are not CLOEXEC, or must take precautions to avoid leaking fds if the plugin may fork. For safety reasons, the flag defaults to off, but many in-tree plugins can set it to on (most commonly because they don't fork after .config_complete; for libvirt because it is documented to clean up fds on fork so it