[Libguestfs] [PATCH nbdkit 1/2] server: Add .after_fork callback, mainly for plugins to create threads.

[Richard W.M. Jones]

On Mon, Jun 22, 2020 at 02:00:50PM -0500, Eric Blake wrote:
> >+++ b/docs/nbdkit-filter.pod
> 
> >+=head2 C<.after_fork>
> >+
> >+ int (*after_fork) (nbdkit_next_after_fork *next, void *nxdata);
> >+
> >+This intercepts the plugin C<.after_fork> method and can be used by
> >+the filter to start background threads (although these have limited
> >+usefulness since they will not be able to access the plugin).
> >+
> >+If there is an error, C<.after_fork> should call C<nbdkit_error> with
> >+an error message and return C<-1>.
> 
> Should the backend code guarantee that .after_fork is called for all
> layers in the backend chain?  The only flexibility I see us gaining
> by letting the filter choose when .after_fork is called is
> fine-grained control over what threads the filter can start before
> or after the plugin's .after_fork, but is that flexibility really
> needed, or are we equally safe if the backend just always calls
> .after_fork and the filter doesn't have to worry about calling
> next(nxdata)?
> 
> Of course, given that filters are in-tree, we can change this
> signature as needed later on, even if we make it void for now we can
> hook it into passing .after_fork later if we find a use for that
> finer-grained call semantics.

In theory a filter that wanted background threads would have exactly
the same problem as a plugin and would need to add an .after_fork
callback to create them.  I can't think of a situation where a filter
would be correct to not call the plugin's .after_fork, but I suppose
it's plausible it might want to do something before or after the
plugin.

The biggest practical problem at the moment however is ...

At this point I would link to the TODO file on github, but it seems
like github is down.  If you look at the TODO file you'll see I
mentioned a general problem with background threads in filters.

Anyway as you say they're all in tree so we can worry about this
later.

> >+=item C<.after_fork>
> >+
> >+In normal operation, C<.after_fork> is called after the server has
> >+forked into the background and changed UID and directory.  If a plugin
> >+needs to create background threads (or uses an external library that
> >+creates threads) it should do so here, because background threads are
> >+killed by fork.
> 
> s/killed/invalidated/ (they are still alive in the parent process,
> but after the fork we are no longer in the parent process to
> interact with them)

OK, I'll change this.  It's also of course a point that if a plugin
creates background threads before the fork and they grab locks then
that could lead to problems.  So I need to add a note to .get_ready
saying that you shouldn't create background threads there.

> >+
> >+Because the server may have forked into the background, error messages
> >+and failures from C<.after_fork> cannot be seen by the user unless
> >+they look through syslog.  An error in C<.after_fork> can appear to
> >+the user as if nbdkit “just died”.  So in almost all cases it is
> >+better to use C<.get_ready> instead of this callback, or to do as much
> >+preparation work as possible in C<.get_ready> and only start
> >+background threads here.
> >+
> >+The server doesn't always fork (eg. if the I<-f> flag is used), but
> >+even so this callback will be called.  If you want to find out if the
> >+server forked between C<.get_ready> and C<.after_fork> use
> >+L<getpid(2)>.
> 
> Thinking about back-compat:
> We promise that plugins compiled against older nbdkit will continue
> to run against newer nbdkit, and that is preserved here.
> What we do not promise is that plugins compiled against newer nbdkit
> will work wehn loaded by older nbdkit.  This is one of those cases:
> anything you stick in .after_fork will not be called by older nbdkit
> which didn't know about the callback.
> 
> At present, we silently ignore the tail of 'struct plugin' from any
> plugin compiled against newer nbdkit when loaded by older nbdkit.
> Should we tweak that to instead output a warning that the plugin
> might rely on features present only in newer nbdkit and therefore
> might not work in the current nbdkit version?  Of course, that
> doesn't help the fact that existing older nbdkit is lacking this
> safety check.

It should probably only do that if the tail of the struct is non-zero,
because who cares if it contains NULL pointers.  But that's another
patch.

> Do we need to instead add some sort of API for plugins to be able to
> learn which features are provided by the nbdkit loading the plugin,
> so that a plugin that requires the use of 1.22 nbdkit features (such
> as .after_fork) can gracefully fail during .config_complete (or
> similar) when loaded by a 1.20 nbdkit, rather than mysteriously
> failing to work when .after_fork is not called?  [In a distro
> setting, the solution is version dependencies: you could make it so
> that installing the nbdkit-vddk-plugin package forces an upgrade of
> the nbdkit package to 1.22 - but this is more a question for
> situations not covered by distro packaging setups]

TBH this is a non-supported case so if it fails you get to keep all
the pieces.

> >+=head2 C<.after_fork>
> >+
> >+ int after_ready (void);
> 
> Typo in the function name.

Oops.

> >+++ b/server/filters.c
> >@@ -193,6 +193,28 @@ filter_get_ready (struct backend *b)
> >      b->next->get_ready (b->next);
> >  }
> >+static int
> >+next_after_fork (struct backend *b)
> >+{
> >+  b->after_fork (b);
> >+  return 0;
> >+}
> >+
> >+static void
> >+filter_after_fork (struct backend *b)
> >+{
> >+  struct backend_filter *f = container_of (b, struct backend_filter, backend);
> >+
> >+  debug ("%s: after_fork", b->name);
> >+
> >+  if (f->filter.after_fork) {
> >+    if (f->filter.after_fork (next_after_fork, b->next) == -1)
> >+      exit (EXIT_FAILURE);
> >+  }
> >+  else
> >+    b->next->after_fork (b->next);
> >+}
> 
> This code changes if we decide that filters could live with 'void'
> instead of next_after_fork, because we mandate the chain traversal
> through the backend rather than leaving it up to the filters.
> 
> Still a few tweaks needed, but the idea looks sane.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-df lists disk usage of guests without needing to install any
software inside the virtual machine.  Supports Linux and Windows.
http://people.redhat.com/~rjones/virt-df/