Re: [Libguestfs] [PATCH NOT WORKING nbdkit v2 1/2] server: Add .ready_to_serve plugin method.
On 10/11/19 8:48 AM, Richard W.M. Jones wrote: Do we need to require the filter to be involved in the recursion? With .open and .config, we do, because there are parameters to the function, where the filter may change the parameter handed to the plugin (for example, changing read-write to read-only or altering a config key). But for other functions where there is no parameter, we let nbdkit control the recursion (.prepare, .finalize, .close). There is no other parameter here to be changed, so having nbdkit run the recursion may make more sense. Two issues I think: Do we let filters see it at all? (I did think about leaving it out, but I included it because we cover every API.) Do we let filters control the recursion. I think you are probably right that it should be nbdkit doing that. Also another hypothetical issue: filters might want to start background threads. For filters that is complicated for two reasons: (1) It might not be a good idea for the same reasons as plugins, and (2) A new thread doesn't have access to the next_ops since it runs outside a connection context. When we wrote the cache filter, you lamented that there was no easy way to kick off a background thread, and instead had to write a scheme where for every client transaction, you check if the cache needs flushing, and guarantee to make forward progress, but no faster than the client is making requests. A true background thread does seem interesting, but you're right that it would need some thread-safe way to have access to a persistent copy of next_ops (while our current usage of filters has next_ops being stack-allocated and only good for the single call). Should there be any caveat about the threads needing to be joined no later than .unload, so that there is no risk of the helper threads segfaulting after the .so is unloaded? Even with the join, nbdkit-vddk-plugin still segfaulted :-( I couldn't work out if it was a problem with the design or the general existing problem that nbdkit doesn't have a well-controlled shutdown path. Possibly the latter. Thanks for the other comments. I don't think we need this just yet so let's leave it for another time. Yeah, as long as this is in the archives for legacy purposes, but not something you actually intend to apply, we can delay solving the problem of a cleaner shutdown path until another day (not that it's ever fun deferring technical debt, but when it is not impacting day-to-day usage, it's hard to spend the time on it when so much else competes for attention too). -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org ___ Libguestfs mailing list Libguestfs@redhat.com https://www.redhat.com/mailman/listinfo/libguestfs
Re: [Libguestfs] [PATCH NOT WORKING nbdkit v2 1/2] server: Add .ready_to_serve plugin method.
On Fri, Oct 11, 2019 at 07:59:30AM -0500, Eric Blake wrote: > On 10/11/19 4:42 AM, Richard W.M. Jones wrote: > >This method can be used for plugins to get control after the server > >has forked and changed user but before it accepts a connection. This > >is very late and the only real use for this is for a plugin to create > >background threads for its own use. > >--- > > >+++ b/docs/nbdkit-filter.pod > > >+=head2 C<.ready_to_serve> > >+ > >+ int (*ready_to_serve) (nbdkit_next_ready_to_serve *next, void *nxdata); > >+ > >+This intercepts the plugin C<.ready_to_serve> method. > >+ > >+If there is an error, C<.ready_to_serve> should call C > >+with an error message and return C<-1>. > > Do we need to require the filter to be involved in the recursion? > > With .open and .config, we do, because there are parameters to the > function, where the filter may change the parameter handed to the > plugin (for example, changing read-write to read-only or altering a > config key). But for other functions where there is no parameter, > we let nbdkit control the recursion (.prepare, .finalize, .close). > There is no other parameter here to be changed, so having nbdkit run > the recursion may make more sense. Two issues I think: Do we let filters see it at all? (I did think about leaving it out, but I included it because we cover every API.) Do we let filters control the recursion. I think you are probably right that it should be nbdkit doing that. Also another hypothetical issue: filters might want to start background threads. For filters that is complicated for two reasons: (1) It might not be a good idea for the same reasons as plugins, and (2) A new thread doesn't have access to the next_ops since it runs outside a connection context. > Should there be a counterpart function for cleanup? We have: > > .load/.unload - called once per nbdkit process, too early for forks > [here's where .ready_to_serve fits] > .open/.close - called once per connection, too early for next_ops > .close/.finalize - called once per connection, full access > > The new .ready_to_serve is called once per nbdkit process, but at a > point where it knows all .loads are complete and it is safe to fork. > I guess .unload can also do a pthread_join() on any thread created > during .ready_to_serve. We needed .finalize separate from .close > because the filter may still need to manipulate the plugin, but > there is no manipulation of the plugin needed during .load or > .ready_to_serve, so having a trio of related functions rather than > two nested pairs of functions still works. As you say, .unload is the counterpart. (There are other problems with .unload running too late, but they are independent of this patch.) > >+++ b/docs/nbdkit-plugin.pod > >@@ -142,6 +142,13 @@ before any connections are accepted. However, during > >C > C<.config_complete> (so a plugin which determines the results from a > > script must be prepared for a missing script). > >+=item C<.ready_to_serve> > >+ > >+This callback is called after nbdkit has forked into the background > >+and changed user, and before it accepts any client connection. If the > >+plugin needs to create its own background threads then this is a good > >+place to do that. > >+ > > Should there be any caveat about the threads needing to be joined no > later than .unload, so that there is no risk of the helper threads > segfaulting after the .so is unloaded? Even with the join, nbdkit-vddk-plugin still segfaulted :-( I couldn't work out if it was a problem with the design or the general existing problem that nbdkit doesn't have a well-controlled shutdown path. Thanks for the other comments. I don't think we need this just yet so let's leave it for another time. 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/ ___ Libguestfs mailing list Libguestfs@redhat.com https://www.redhat.com/mailman/listinfo/libguestfs
Re: [Libguestfs] [PATCH NOT WORKING nbdkit v2 1/2] server: Add .ready_to_serve plugin method.
On 10/11/19 4:42 AM, Richard W.M. Jones wrote:
This method can be used for plugins to get control after the server
has forked and changed user but before it accepts a connection. This
is very late and the only real use for this is for a plugin to create
background threads for its own use.
---
+++ b/docs/nbdkit-filter.pod
+=head2 C<.ready_to_serve>
+
+ int (*ready_to_serve) (nbdkit_next_ready_to_serve *next, void *nxdata);
+
+This intercepts the plugin C<.ready_to_serve> method.
+
+If there is an error, C<.ready_to_serve> should call C
+with an error message and return C<-1>.
Do we need to require the filter to be involved in the recursion?
With .open and .config, we do, because there are parameters to the
function, where the filter may change the parameter handed to the plugin
(for example, changing read-write to read-only or altering a config
key). But for other functions where there is no parameter, we let
nbdkit control the recursion (.prepare, .finalize, .close). There is no
other parameter here to be changed, so having nbdkit run the recursion
may make more sense.
Should there be a counterpart function for cleanup? We have:
.load/.unload - called once per nbdkit process, too early for forks
[here's where .ready_to_serve fits]
.open/.close - called once per connection, too early for next_ops
.close/.finalize - called once per connection, full access
The new .ready_to_serve is called once per nbdkit process, but at a
point where it knows all .loads are complete and it is safe to fork. I
guess .unload can also do a pthread_join() on any thread created during
.ready_to_serve. We needed .finalize separate from .close because the
filter may still need to manipulate the plugin, but there is no
manipulation of the plugin needed during .load or .ready_to_serve, so
having a trio of related functions rather than two nested pairs of
functions still works.
+++ b/docs/nbdkit-plugin.pod
@@ -142,6 +142,13 @@ before any connections are accepted. However, during
C (so a plugin which determines the results from a
script must be prepared for a missing script).
+=item C<.ready_to_serve>
+
+This callback is called after nbdkit has forked into the background
+and changed user, and before it accepts any client connection. If the
+plugin needs to create its own background threads then this is a good
+place to do that.
+
Should there be any caveat about the threads needing to be joined no
later than .unload, so that there is no risk of the helper threads
segfaulting after the .so is unloaded?
=item C<.open>
A new client has connected.
@@ -408,7 +415,8 @@ Any other bare parameters give errors.
This optional callback is called after all the configuration has been
passed to the plugin. It is a good place to do checks, for example
-that the user has passed the required parameters to the plugin.
+that the user has passed the required parameters to the plugin. It is
+also a good place to do general start-up work.
Should this emphasize that .load must not leave other threads running on
its completion, because it is invoked pre-fork()?
If there is an error, C<.config_complete> should call C
with an error message and return C<-1>.
@@ -437,6 +445,23 @@ are silently ignored.
If there is an error, C<.thread_model> should call C
with an error message and return C<-1>.
+=head2 C<.ready_to_serve>
+
+ int ready_to_serve (void);
+
+This optional callback is called after nbdkit has forked into the
+background and changed user, and before it accepts any client
+connection.
+
+If the plugin needs to create its own background threads then this is
+a good place to do that. However because nbdkit may have already
+forked into the background and so it is difficult to communicate
+errors back to the user, this is B a good place to do general
+start-up work (use C<.config_complete> instead).
+
+If there is an error, C<.ready_to_serve> should call C
+with an error message and return C<-1>.
+
=head2 C<.open>
void *open (int readonly);
diff --git a/include/nbdkit-filter.h b/include/nbdkit-filter.h
index c1930c1..7b3a64f 100644
+++ b/server/filters.c
@@ -190,6 +190,29 @@ plugin_magic_config_key (struct backend *b)
return b->next->magic_config_key (b->next);
}
+static int
+next_ready_to_serve (void *nxdata)
+{
+ struct backend *b = nxdata;
+ b->ready_to_serve (b);
+ return 0;
+}
+
+static void
+filter_ready_to_serve (struct backend *b)
+{
+ struct backend_filter *f = container_of (b, struct backend_filter, backend);
+
+ debug ("%s: ready_to_serve", b->name);
+
+ if (f->filter.ready_to_serve) {
+if (f->filter.ready_to_serve (next_ready_to_serve, b->next) == -1)
+ exit (EXIT_FAILURE);
+ }
+ else
+b->next->ready_to_serve (b->next);
Should we add a backend_ready_to_serve() helper function? Do we want
nbdkit to run the recursion itself, instead of requiring the filters to
call next_ready_to_serve?
+++ b/tests/test-la
[Libguestfs] [PATCH NOT WORKING nbdkit v2 1/2] server: Add .ready_to_serve plugin method.
This method can be used for plugins to get control after the server
has forked and changed user but before it accepts a connection. This
is very late and the only real use for this is for a plugin to create
background threads for its own use.
---
docs/nbdkit-filter.pod | 20 +++-
docs/nbdkit-plugin.pod | 27 ++-
include/nbdkit-filter.h| 2 ++
include/nbdkit-plugin.h| 2 ++
server/filters.c | 24
server/internal.h | 1 +
server/main.c | 3 +++
server/plugins.c | 16
tests/test-layers-filter.c | 11 ++-
tests/test-layers-plugin.c | 8
tests/test-layers.c| 12
11 files changed, 119 insertions(+), 7 deletions(-)
diff --git a/docs/nbdkit-filter.pod b/docs/nbdkit-filter.pod
index 07ede47..4f10494 100644
--- a/docs/nbdkit-filter.pod
+++ b/docs/nbdkit-filter.pod
@@ -126,8 +126,8 @@ which is required.
=head1 NEXT PLUGIN
-F defines three function types
-(C, C,
+F defines four function types (C,
+C, C,
C) and a structure called C.
These abstract the next plugin or filter in the chain. There is also
an opaque pointer C which must be passed along when calling
@@ -135,9 +135,10 @@ these functions.
=head2 Next config, open and close
-The filter’s C<.config>, C<.config_complete> and C<.open> methods may
-only call the next C<.config>, C<.config_complete> and C<.open> method
-in the chain (optionally for C<.config>).
+The filter’s C<.config>, C<.config_complete>, C<.ready_to_serve> and
+C<.open> methods may only call the next C<.config>,
+C<.config_complete>, C<.ready_to_serve> and C<.open> method in the
+chain (optionally for C<.config>).
The filter’s C<.close> method is called when an old connection closed,
and this has no C parameter because it cannot be
@@ -284,6 +285,15 @@ filter doesn't slow down other filters or plugins.
If there is an error, C<.thread_model> should call C
with an error message and return C<-1>.
+=head2 C<.ready_to_serve>
+
+ int (*ready_to_serve) (nbdkit_next_ready_to_serve *next, void *nxdata);
+
+This intercepts the plugin C<.ready_to_serve> method.
+
+If there is an error, C<.ready_to_serve> should call C
+with an error message and return C<-1>.
+
=head2 C<.open>
void * (*open) (nbdkit_next_open *next, void *nxdata,
diff --git a/docs/nbdkit-plugin.pod b/docs/nbdkit-plugin.pod
index e34ffd1..3ae1303 100644
--- a/docs/nbdkit-plugin.pod
+++ b/docs/nbdkit-plugin.pod
@@ -142,6 +142,13 @@ before any connections are accepted. However, during
C (so a plugin which determines the results from a
script must be prepared for a missing script).
+=item C<.ready_to_serve>
+
+This callback is called after nbdkit has forked into the background
+and changed user, and before it accepts any client connection. If the
+plugin needs to create its own background threads then this is a good
+place to do that.
+
=item C<.open>
A new client has connected.
@@ -408,7 +415,8 @@ Any other bare parameters give errors.
This optional callback is called after all the configuration has been
passed to the plugin. It is a good place to do checks, for example
-that the user has passed the required parameters to the plugin.
+that the user has passed the required parameters to the plugin. It is
+also a good place to do general start-up work.
If there is an error, C<.config_complete> should call C
with an error message and return C<-1>.
@@ -437,6 +445,23 @@ are silently ignored.
If there is an error, C<.thread_model> should call C
with an error message and return C<-1>.
+=head2 C<.ready_to_serve>
+
+ int ready_to_serve (void);
+
+This optional callback is called after nbdkit has forked into the
+background and changed user, and before it accepts any client
+connection.
+
+If the plugin needs to create its own background threads then this is
+a good place to do that. However because nbdkit may have already
+forked into the background and so it is difficult to communicate
+errors back to the user, this is B a good place to do general
+start-up work (use C<.config_complete> instead).
+
+If there is an error, C<.ready_to_serve> should call C
+with an error message and return C<-1>.
+
=head2 C<.open>
void *open (int readonly);
diff --git a/include/nbdkit-filter.h b/include/nbdkit-filter.h
index c1930c1..7b3a64f 100644
--- a/include/nbdkit-filter.h
+++ b/include/nbdkit-filter.h
@@ -64,6 +64,7 @@ extern struct nbdkit_extent nbdkit_get_extent (const struct
nbdkit_extents *,
typedef int nbdkit_next_config (void *nxdata,
const char *key, const char *value);
typedef int nbdkit_next_config_complete (void *nxdata);
+typedef int nbdkit_next_ready_to_serve (void *nxdata);
typedef int nbdkit_next_open (void *nxdata,
int readonly);
@@ -129,6 +130,7 @@ struct nbdkit_filter {
int (*config_complete) (nbdkit_next_config_complete *next, void *nxdata
