Libguestfs - May 2019 - Re: [nbdkit PATCH 1/2] plugins: Add .thread_model callback

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 adjusted to use the v2 API,
although they could support this one even without implementing v2.

Eric Blake (2):
  plugins: Add .thread_model callback
  sh: Implement .thread_model callback

 docs/nbdkit-plugin.pod          | 33 ++++++++++++++++++---
 plugins/sh/nbdkit-sh-plugin.pod | 15 ++++++++++
 include/nbdkit-plugin.h         |  2 ++
 plugins/sh/sh.c                 | 51 +++++++++++++++++++++++++++++++--
 server/plugins.c                | 12 +++++++-
 tests/test-eflags.sh            | 49 ++++++++++++++++++++-----------
 6 files changed, 139 insertions(+), 23 deletions(-)

-- 
2.20.1

Plugins cannot get rid of #define THREAD_MODEL (doing so would break
API/ABI compatibility); however, we CAN add an optional callback for
allowing a runtime reduction of the thread model. This can be
especially useful for language bindings, as in the next patch, where
the C glue code for the language has to cater to the maximum
parallelism possible for that language, but where an individual plugin
may still want to be stricter.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 docs/nbdkit-plugin.pod  | 33 +++++++++++++++++++++++++++++----
 include/nbdkit-plugin.h |  2 ++
 server/plugins.c        | 12 +++++++++++-
 3 files changed, 42 insertions(+), 5 deletions(-)

diff --git a/docs/nbdkit-plugin.pod b/docs/nbdkit-plugin.pod
index 7f83234..9a20663 100644
--- a/docs/nbdkit-plugin.pod
+++ b/docs/nbdkit-plugin.pod
@@ -133,6 +133,12 @@ has been passed to the plugin.
 Both are called after loading the plugin but before any connections
 are accepted.

+=item C<.thread_model>
+
+C<.thread_model> is called once after all configuration information
+has been passed to the plugin, and before any connections are
+accepted.
+
 =item C<.open>

 A new client has connected.
@@ -466,6 +472,19 @@ what already appears in C<.description>.
 If the plugin doesn't take any config parameters you should probably
 omit this.

+=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 details.  Attempts to request a looser (more parallel) model
+are silently ignored.
+
+If there is an error, C<.thread_model> should call C<nbdkit_error>
+with an error message and return C<-1>.
+
 =head2 C<.open>

  void *open (int readonly);
@@ -898,9 +917,14 @@ error message, and C<nbdkit_set_error> to record an
appropriate error

 =head1 THREADS

-Each nbdkit plugin must declare its thread safety model by defining
-the C<THREAD_MODEL> macro.  (This macro is used by
-C<NBDKIT_REGISTER_PLUGIN>).
+Each nbdkit plugin must declare its maximum thread safety model by
+defining the C<THREAD_MODEL> macro.  (This macro is used by
+C<NBDKIT_REGISTER_PLUGIN>).  Additionally, a plugin may implement the
+C<.thread_model> callback, called right after C<.config_complete>
to
+make a runtime decision on which thread model to use.  The nbdkit
+server chooses the most restrictive model between the plugin's
+C<THREAD_MODEL>, the C<.thread_model> if present, and any
restrictions
+requested by filters.

 The possible settings for C<THREAD_MODEL> are defined below.

@@ -935,7 +959,8 @@ parallel.  However only one request will happen per handle
at a time
 =item C<#define THREAD_MODEL NBDKIT_THREAD_MODEL_PARALLEL>

 Multiple handles can be open and multiple data requests can happen in
-parallel (even on the same handle).
+parallel (even on the same handle).  The server may reorder replies,
+answering a later request before an earlier one.

 All the libraries you use must be thread-safe and reentrant.  You may
 also need to provide mutexes for fields in your connection handle.
diff --git a/include/nbdkit-plugin.h b/include/nbdkit-plugin.h
index e9b1808..632df86 100644
--- a/include/nbdkit-plugin.h
+++ b/include/nbdkit-plugin.h
@@ -130,6 +130,8 @@ struct nbdkit_plugin {
                   struct nbdkit_extents *extents);
   int (*can_cache) (void *handle);
   int (*cache) (void *handle, uint32_t count, uint64_t offset, uint32_t flags);
+
+  int (*thread_model) (void);
 };

 extern void nbdkit_set_error (int err);
diff --git a/server/plugins.c b/server/plugins.c
index f293e6a..85736bb 100644
--- a/server/plugins.c
+++ b/server/plugins.c
@@ -87,8 +87,18 @@ static int
 plugin_thread_model (struct backend *b)
 {
   struct backend_plugin *p = container_of (b, struct backend_plugin, backend);
+  int thread_model = p->plugin._thread_model;
+  int r;

-  return p->plugin._thread_model;
+  if (p->plugin.thread_model) {
+    r = p->plugin.thread_model ();
+    if (r == -1)
+      exit (EXIT_FAILURE);
+    if (r < thread_model)
+      thread_model = r;
+  }
+
+  return thread_model;
 }

 static const char *
-- 
2.20.1

Wire up .thread_model to the sh plugin, with a change to test-eflags
to demonstrate the effect of a script that requests the tighter
SERIALIZE_CONNECTIONS.  While at it, fix two swapped tests messed up
in afbcd070.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 plugins/sh/nbdkit-sh-plugin.pod | 15 ++++++++++
 plugins/sh/sh.c                 | 51 +++++++++++++++++++++++++++++++--
 tests/test-eflags.sh            | 49 ++++++++++++++++++++-----------
 3 files changed, 97 insertions(+), 18 deletions(-)

diff --git a/plugins/sh/nbdkit-sh-plugin.pod b/plugins/sh/nbdkit-sh-plugin.pod
index a5285ed..7ba8830 100644
--- a/plugins/sh/nbdkit-sh-plugin.pod
+++ b/plugins/sh/nbdkit-sh-plugin.pod
@@ -180,6 +180,21 @@ ignored.

  /path/to/script config_complete

+=item C<thread_model>
+
+ /path/to/script thread_model
+
+On success this should print the desired thread model of the script,
+one of C<"serialize_connections">,
C<"serialize_all_requests">,
+C<"serialize_requests">, or C<"parallel">.
+
+This method is I<not> required; if omitted, then the plugin will be
+executed under the default sh thread model (currently
+C<"serialize_all_requests">, which implies this callback only
makes a
+difference with an output of C<"serialize_connections">).  If
an error
+occurs, the script should output an error message and exit with status
+C<1>; unrecognized output is ignored.
+
 =item C<open>

  /path/to/script open <readonly>
diff --git a/plugins/sh/sh.c b/plugins/sh/sh.c
index 862be21..aeb0191 100644
--- a/plugins/sh/sh.c
+++ b/plugins/sh/sh.c
@@ -257,6 +257,54 @@ sh_config_complete (void)
   }
 }

+#define THREAD_MODEL NBDKIT_THREAD_MODEL_SERIALIZE_ALL_REQUESTS
+
+static int
+sh_thread_model (void)
+{
+  const char *args[] = { script, "thread_model", NULL };
+  CLEANUP_FREE char *s = NULL;
+  size_t slen;
+  int r;
+
+  switch (call_read (&s, &slen, args)) {
+  case OK:
+    if (slen > 0 && s[slen-1] == '\n')
+      s[slen-1] = '\0';
+    if (strcasecmp (s, "parallel") == 0)
+      r = NBDKIT_THREAD_MODEL_PARALLEL;
+    else if (strcasecmp (s, "serialize_requests") == 0 ||
+             strcasecmp (s, "serialize-requests") == 0)
+      r = NBDKIT_THREAD_MODEL_SERIALIZE_REQUESTS;
+    else if (strcasecmp (s, "serialize_all_requests") == 0 ||
+             strcasecmp (s, "serialize-all-requests") == 0)
+      r = NBDKIT_THREAD_MODEL_SERIALIZE_ALL_REQUESTS;
+    else if (strcasecmp (s, "serialize_connections") == 0 ||
+             strcasecmp (s, "serialize-connections") == 0)
+      r = NBDKIT_THREAD_MODEL_SERIALIZE_CONNECTIONS;
+    else {
+      nbdkit_debug ("%s: ignoring unrecognized thread model: %s",
+                    script, s);
+      r = THREAD_MODEL;
+    }
+    return r;
+
+  case MISSING:
+    return THREAD_MODEL;
+
+  case ERROR:
+    return -1;
+
+  case RET_FALSE:
+    nbdkit_error ("%s: %s method returned unexpected code (3/false)",
+                  script, "thread_model");
+    errno = EIO;
+    return -1;
+
+  default: abort ();
+  }
+}
+
 static void *
 sh_open (int readonly)
 {
@@ -865,8 +913,6 @@ sh_cache (void *handle, uint32_t count, uint64_t offset,
uint32_t flags)
   "script=<FILENAME>     (required) The shell script to run.\n"
\
   "[other arguments may be used by the plugin that you load]"

-#define THREAD_MODEL NBDKIT_THREAD_MODEL_SERIALIZE_ALL_REQUESTS
-
 static struct nbdkit_plugin plugin = {
   .name              = "sh",
   .version           = PACKAGE_VERSION,
@@ -878,6 +924,7 @@ static struct nbdkit_plugin plugin = {
   .config            = sh_config,
   .config_complete   = sh_config_complete,
   .config_help       = sh_config_help,
+  .thread_model      = sh_thread_model,

   .open              = sh_open,
   .close             = sh_close,
diff --git a/tests/test-eflags.sh b/tests/test-eflags.sh
index 8158f75..80816ff 100755
--- a/tests/test-eflags.sh
+++ b/tests/test-eflags.sh
@@ -277,22 +277,6 @@ EOF
 # -r
 # can_multi_conn=true

-late_args="serialize=connections" do_nbdkit -r --filter=noparallel
<<'EOF'
-case "$1" in
-     get_size) echo 1M ;;
-     can_multi_conn) exit 0 ;;
-     *) exit 2 ;;
-esac
-EOF
-
-[ $eflags -eq $(( HAS_FLAGS|READ_ONLY|SEND_DF )) ] ||
-    fail "expected HAS_FLAGS|READ_ONLY|SEND_DF"
-
-#----------------------------------------------------------------------
-# -r
-# --filter=noparallel serialize=connections
-# can_multi_conn=true
-
 do_nbdkit -r <<'EOF'
 case "$1" in
      get_size) echo 1M ;;
@@ -304,6 +288,39 @@ EOF
 [ $eflags -eq $(( HAS_FLAGS|READ_ONLY|SEND_DF|CAN_MULTI_CONN )) ] ||
     fail "expected HAS_FLAGS|READ_ONLY|SEND_DF|CAN_MULTI_CONN"

+#----------------------------------------------------------------------
+# -r
+# --filter=noparallel serialize=connections
+# can_multi_conn=true
+
+late_args="serialize=connections" do_nbdkit -r --filter=noparallel
<<'EOF'
+case "$1" in
+     get_size) echo 1M ;;
+     can_multi_conn) exit 0 ;;
+     *) exit 2 ;;
+esac
+EOF
+
+[ $eflags -eq $(( HAS_FLAGS|READ_ONLY|SEND_DF )) ] ||
+    fail "expected HAS_FLAGS|READ_ONLY|SEND_DF"
+
+#----------------------------------------------------------------------
+# -r
+# thread_model=serialize_connections
+# can_multi_conn=true
+
+do_nbdkit -r <<'EOF'
+case "$1" in
+     get_size) echo 1M ;;
+     can_multi_conn) exit 0 ;;
+     thread_model) echo "serialize_connections" ;;
+     *) exit 2 ;;
+esac
+EOF
+
+[ $eflags -eq $(( HAS_FLAGS|READ_ONLY|SEND_DF )) ] ||
+    fail "expected HAS_FLAGS|READ_ONLY|SEND_DF"
+
 #----------------------------------------------------------------------
 # -r
 # can_cache=native
-- 
2.20.1

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 details.  Attempts to request a looser (more parallel) model
> +are silently ignored.
> +
> +If there is an error, C<.thread_model> should call
C<nbdkit_error>
> +with an error message and return C<-1>.
Two comments:

(1) Do we have an opportunity to change the way the thread model is
specified to allow future expansion.  This might involve returning
something other than the simple int, or could be that we specify
different constants to be returned by this call.

(Actually while making that point I think I've talked myself out of
it.  But probably we should do this in the V3 API, so a note can be
added to that effect at the end of the TODO file.)

(2) Shouldn't it be an error if the thread model returns a more
parallel thread model than the constant?

Anyway patch series generally looks fine to me.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW