thr3ads.net - Libguestfs - [Libguestfs] [nbdkit PATCH 3/3] filters: Use only .thread_model, not THREAD

If this information is useful, please help other people find it:
Share via:

Eric Blake

2019-May-17 20:20 UTC

[Libguestfs] [nbdkit PATCH 0/3] Add noparallel filter

Being able to programmatically force nbdkit to be less parallel can be
useful during testing. I was less sure about patch 3, but if you like
it, I'm inclined to instead squash it into patch 1. This patch is
written to apply after my NBD_CMD_CACHE work (since I touched the
nocache filter); but can be rearranged if we think this series should
go in first while that one undergoes any adjustments from review.

Eric Blake (3):
  server: Allow filters to reduce thread model dynamically
  noparallel: Implement new filter
  filters: Use only .thread_model, not THREAD_MODEL

 docs/nbdkit-filter.pod                        | 45 ++++++----
 filters/fua/nbdkit-fua-filter.pod             |  1 +
 filters/nocache/nbdkit-nocache-filter.pod     |  1 +
 filters/noextents/nbdkit-noextents-filter.pod |  1 +
 .../noparallel/nbdkit-noparallel-filter.pod   | 66 ++++++++++++++
 filters/nozero/nbdkit-nozero-filter.pod       |  1 +
 configure.ac                                  |  2 +
 include/nbdkit-filter.h                       |  7 +-
 filters/cow/cow.c                             |  2 -
 filters/delay/delay.c                         |  2 -
 filters/error/error.c                         |  2 -
 filters/fua/fua.c                             |  2 -
 filters/log/log.c                             |  2 -
 filters/nocache/nocache.c                     |  2 -
 filters/noextents/noextents.c                 |  2 -
 filters/noparallel/noparallel.c               | 85 +++++++++++++++++++
 filters/nozero/nozero.c                       |  2 -
 filters/offset/offset.c                       |  2 -
 filters/partition/partition.c                 |  2 -
 filters/rate/rate.c                           |  2 -
 filters/readahead/readahead.c                 |  2 -
 filters/stats/stats.c                         |  2 -
 filters/truncate/truncate.c                   |  2 -
 filters/xz/xz.c                               |  6 +-
 server/filters.c                              |  8 +-
 server/main.c                                 | 10 ++-
 filters/noparallel/Makefile.am                | 61 +++++++++++++
 tests/test-eflags.sh                          | 16 ++++
 tests/test-parallel-file.sh                   | 13 ++-
 29 files changed, 296 insertions(+), 55 deletions(-)
 create mode 100644 filters/noparallel/nbdkit-noparallel-filter.pod
 create mode 100644 filters/noparallel/noparallel.c
 create mode 100644 filters/noparallel/Makefile.am

-- 
2.20.1

Eric Blake

2019-May-17 20:20 UTC

head link

[Libguestfs] [nbdkit PATCH 1/3] server: Allow filters to reduce thread model dynamically

Delay the initialization of the thread model until after
backend->config_complete; this is safe, since we don't make any lock
calls until backend->connect. Then add a function callback that
filters can use to further reduce things based on runtime
configuration.

Signed-off-by: Eric Blake <eblake@redhat.com>

---

Is it worth having filters both #define THREAD_MODEL and optionally
declare .thread_model? We have no API stability requirements; we could
instead declare that NBDKIT_REGISTER_FILTER() does NOT require a macro
THREAD_MODEL, and that .thread_model is optional (defaulting to
parallel, but MUST be provided by filters that want to reduce things).
---
 docs/nbdkit-filter.pod  | 25 ++++++++++++++++++++++---
 include/nbdkit-filter.h |  1 +
 server/filters.c        |  8 ++++++++
 server/main.c           | 10 ++++++----
 4 files changed, 37 insertions(+), 7 deletions(-)

diff --git a/docs/nbdkit-filter.pod b/docs/nbdkit-filter.pod
index 857f241..4033789 100644
--- a/docs/nbdkit-filter.pod
+++ b/docs/nbdkit-filter.pod
@@ -109,12 +109,16 @@ All filters should start by including this header file.

 =head1 C<#define THREAD_MODEL>

-All filters must define a thread model.  See
-L<nbdkit-plugin(3)/THREADS> for a discussion of thread models.
+All filters must define a default thread model by defining
+C<THREAD_MODEL>.  See L<nbdkit-plugin(3)/THREADS> for a discussion
of
+thread models.

 The final thread model is the smallest (ie. most serialized) out of
 all the filters and the plugin.  Filters cannot alter the thread model
-to make it larger (more parallel).
+to make it larger (more parallel).  However, they can dynamically
+request a smaller (more serialized) model from decisions made during
+C<.config> and C<.config_complete>, by implementing a
<.thread_model>
+function callback.

 If possible filters should be be written to handle fully parallel
 requests (C<NBDKIT_THREAD_MODEL_PARALLEL>, even multiple requests
@@ -267,6 +271,21 @@ what already appears in C<.description>.
 If the filter doesn't take any config parameters you should probably
 omit this.

+=head2 C<.thread_model>
+
+ int (*thread_model) (void);
+
+This optional function is called after C<.config_complete> to provide
+the filter an opportunity to reduce the resulting thread model below
+the value used at compilation via C<THREAD_MODEL>. The resulting
+thread model for all connections is determined by selecting the most
+restrictive model from the C<THREAD_MODEL> of the plugin and all
+filters, and from the results of any C<.thread_model> across all
+filters.
+
+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) (nbdkit_next_open *next, void *nxdata,
diff --git a/include/nbdkit-filter.h b/include/nbdkit-filter.h
index 5893dd8..70d32ce 100644
--- a/include/nbdkit-filter.h
+++ b/include/nbdkit-filter.h
@@ -115,6 +115,7 @@ struct nbdkit_filter {
                  const char *key, const char *value);
   int (*config_complete) (nbdkit_next_config_complete *next, void *nxdata);
   const char *config_help;
+  int (*thread_model) (void);

   void * (*open) (nbdkit_next_open *next, void *nxdata,
                   int readonly);
diff --git a/server/filters.c b/server/filters.c
index 3bd91fe..87a9c0e 100644
--- a/server/filters.c
+++ b/server/filters.c
@@ -99,6 +99,14 @@ filter_thread_model (struct backend *b)
   if (filter_thread_model < thread_model) /* more serialized */
     thread_model = filter_thread_model;

+  if (f->filter.thread_model) {
+    filter_thread_model = f->filter.thread_model ();
+    if (filter_thread_model == -1)
+      exit (EXIT_FAILURE);
+    if (filter_thread_model < thread_model)
+      thread_model = filter_thread_model;
+  }
+
   return thread_model;
 }

diff --git a/server/main.c b/server/main.c
index 8ad3f7a..d46888a 100644
--- a/server/main.c
+++ b/server/main.c
@@ -1,5 +1,5 @@
 /* nbdkit
- * Copyright (C) 2013-2018 Red Hat Inc.
+ * Copyright (C) 2013-2019 Red Hat Inc.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions are
@@ -568,9 +568,6 @@ main (int argc, char *argv[])
     debug_flags = next;
   }

-  /* Select a thread model. */
-  lock_init_thread_model ();
-
   if (help) {
     struct backend *b;

@@ -643,6 +640,11 @@ main (int argc, char *argv[])

   backend->config_complete (backend);

+  /* Select a thread model. Plugin models are fixed at compile time,
+   * but filters are allowed to reduce the level during config.
+   */
+  lock_init_thread_model ();
+
   start_serving ();

   backend->free (backend);
-- 
2.20.1

Eric Blake

2019-May-17 20:20 UTC

head link

[Libguestfs] [nbdkit PATCH 2/3] noparallel: Implement new filter

Similar to the existing fua, nocache, nozero and noextents filters,
add a filter to make it easy to override the plugin's choice of
parallel processing.  Several possible uses:

- facilitate timing tests of whether a plugin's parallel support
and/or client's use of multi-conn increases throughput

- enable a quick workaround of any plugin that was mistakenly compiled
with a claim of parallel support but with insufficient locking

- serve a parallel plugin to a client that batches up requests but is
not prepared to handle out-of-order replies (however, this does not
necessarily prevent deadlock if the client batches a large write
immediately behind a large read before reading the server's response
to the read)

Update test-eflags (showing the effect on the multi-conn flag) and
test-parallel-file (showing the effect on preventing out-of-order
replies).

In implementing this, I wonder if we should have a thread model
inbetween serialize-requests and parallel. Right now, if I use
--filter=delay rdelay=2 wdelay=3, coupled with a client that starts a
write followed by a read, the behaviors are:

Parallel (filter not used), latency 3, out-of-order replies:
0   1   2   3   4   5
+---+---+---+---+---+
=>write request
 =>read request
         <=read reply
	    <=write reply

Serialize-requests (--filter=noparallel), latency 5, in-order replies:
0   1   2   3   4   5
+---+---+---+---+---+
=>write request
	    <=write reply
 ============>read request
                     <=read reply

But merely guaranteeing in-order completion falls in between:

New mode, latency 3, in-order replies:
0   1   2   3   4   5
+---+---+---+---+---+
=>write request
	    <=write reply
 =>read request
         <=====read reply

Since nbdkit is already farming out requests to different threads, it
could keep a list of which thread was fired off in which order, then
use pthread_cond_broadcast and friends to block a thread from sending
the reply to the client until all other threads with earlier
sequencing have completed first. But adding such a mode is tricky -
our existing modes are API in nbdkit-common.h, so we can't renumber
NBDKIT_THREAD_MODEL_PARALLEL, yet adding a new mode 5 that falls in
between 3 and 4 changes all our existing code that has so far relied
on descending values being stricter. At the same time, such a mode HAS
to be done in nbdkit proper - by the time a filter's callbacks are
reached within separate threads, scheduling races means the filter has
no reliable way of knowing which of the two threads represents the
earlier client request.

Anyways, if nbdkit adds such a threading model, noparallel would be
the obvious filter to allow easy use of that model with any
pre-existing plugin :)

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 filters/fua/nbdkit-fua-filter.pod             |  1 +
 filters/nocache/nbdkit-nocache-filter.pod     |  1 +
 filters/noextents/nbdkit-noextents-filter.pod |  1 +
 .../noparallel/nbdkit-noparallel-filter.pod   | 66 ++++++++++++++
 filters/nozero/nbdkit-nozero-filter.pod       |  1 +
 configure.ac                                  |  2 +
 filters/noparallel/noparallel.c               | 87 +++++++++++++++++++
 filters/noparallel/Makefile.am                | 61 +++++++++++++
 tests/test-eflags.sh                          | 16 ++++
 tests/test-parallel-file.sh                   | 13 ++-
 10 files changed, 248 insertions(+), 1 deletion(-)
 create mode 100644 filters/noparallel/nbdkit-noparallel-filter.pod
 create mode 100644 filters/noparallel/noparallel.c
 create mode 100644 filters/noparallel/Makefile.am

diff --git a/filters/fua/nbdkit-fua-filter.pod
b/filters/fua/nbdkit-fua-filter.pod
index b76917b..a5595da 100644
--- a/filters/fua/nbdkit-fua-filter.pod
+++ b/filters/fua/nbdkit-fua-filter.pod
@@ -69,6 +69,7 @@ L<nbdkit-blocksize-filter(1)>,
 L<nbdkit-log-filter(1)>,
 L<nbdkit-nocache-filter(1)>,
 L<nbdkit-noextents-filter(1)>,
+L<nbdkit-noparallel-filter(1)>,
 L<nbdkit-nozero-filter(1)>.

 =head1 AUTHORS
diff --git a/filters/nocache/nbdkit-nocache-filter.pod
b/filters/nocache/nbdkit-nocache-filter.pod
index 0f43433..3802a7e 100644
--- a/filters/nocache/nbdkit-nocache-filter.pod
+++ b/filters/nocache/nbdkit-nocache-filter.pod
@@ -58,6 +58,7 @@ L<nbdkit-filter(3)>,
 L<nbdkit-cache-filter(1)>,
 L<nbdkit-fua-filter(1)>,
 L<nbdkit-noextents-filter(1)>,
+L<nbdkit-noparallel-filter(1)>,
 L<nbdkit-nozero-filter(1)>.

 =head1 AUTHORS
diff --git a/filters/noextents/nbdkit-noextents-filter.pod
b/filters/noextents/nbdkit-noextents-filter.pod
index 24519a0..4722392 100644
--- a/filters/noextents/nbdkit-noextents-filter.pod
+++ b/filters/noextents/nbdkit-noextents-filter.pod
@@ -29,6 +29,7 @@ L<nbdkit(1)>,
 L<nbdkit-filter(3)>,
 L<nbdkit-fua-filter(1)>,
 L<nbdkit-nocache-filter(1)>,
+L<nbdkit-noparallel-filter(1)>,
 L<nbdkit-nozero-filter(1)>,
 L<nbdkit-file-plugin(1)>.

diff --git a/filters/noparallel/nbdkit-noparallel-filter.pod
b/filters/noparallel/nbdkit-noparallel-filter.pod
new file mode 100644
index 0000000..284f6eb
--- /dev/null
+++ b/filters/noparallel/nbdkit-noparallel-filter.pod
@@ -0,0 +1,66 @@
+=head1 NAME
+
+nbdkit-noparallel-filter - nbdkit noparallel filter
+
+=head1 SYNOPSIS
+
+ nbdkit --filter=noparallel plugin [serialize=MODE] [plugin-args...]
+
+=head1 DESCRIPTION
+
+C<nbdkit-noparallel-filter> is a filter that intentionally disables
+parallelism in handling requests from clients. It is mainly useful for
+evaluating timing differences between various levels of
+parallelism. It can also be used as a way to work around any bugs in a
+plugin's claimed level of parallel support, without recompiling the
+plugin, or to ease efforts when connecting with a client that can
+batch up several requests but is not prepared to handle out-of-order
+replies.
+
+=head1 PARAMETERS
+
+=over 4
+
+=item B<serialize=requests|all-requests|connections>
+
+Optional, controls how much serialization the filter will
+enforce. Mode B<requests> (default) prevents a single client from
+having more than one in-flight request, but does not prevent parallel
+requests from a second connection (if the plugin supports that). Mode
+B<all-requests> is stricter, enforcing that at most one request
+(regardless of connection) will be active, but does not prevent
+parallel connections (if the plugin supports that). Mode
+B<connections> is strictest, where there can be at most one client at
+a time, and the server no longer advertise C<NBD_FLAG_MULTI_CONN> to
+clients.
+
+=back
+
+=head1 EXAMPLES
+
+Serve the file F<disk.img>, but disallow out-of-order transaction
+completion to a given client:
+
+ nbdkit --filter=noparallel file disk.img
+
+Serve the file F<disk.img>, but allowing only one client at a time:
+
+ nbdkit --filter=noparallel file serialize=connections disk.img
+
+=head1 SEE ALSO
+
+L<nbdkit(1)>,
+L<nbdkit-file-plugin(1)>,
+L<nbdkit-filter(3)>,
+L<nbdkit-fua-filter(1)>,
+L<nbdkit-nocache-filter(1)>,
+L<nbdkit-noextents-filter(1)>,
+L<nbdkit-nozero-filter(1)>.
+
+=head1 AUTHORS
+
+Eric Blake
+
+=head1 COPYRIGHT
+
+Copyright (C) 2018-2019 Red Hat Inc.
diff --git a/filters/nozero/nbdkit-nozero-filter.pod
b/filters/nozero/nbdkit-nozero-filter.pod
index d94dd8d..144b823 100644
--- a/filters/nozero/nbdkit-nozero-filter.pod
+++ b/filters/nozero/nbdkit-nozero-filter.pod
@@ -54,6 +54,7 @@ L<nbdkit-file-plugin(1)>,
 L<nbdkit-filter(3)>,
 L<nbdkit-fua-filter(1)>,
 L<nbdkit-nocache-filter(1)>,
+L<nbdkit-noparallel-filter(1)>,
 L<nbdkit-noextents-filter(1)>.

 =head1 AUTHORS
diff --git a/configure.ac b/configure.ac
index 2163930..270e981 100644
--- a/configure.ac
+++ b/configure.ac
@@ -833,6 +833,7 @@ filters="\
         log \
         nocache \
         noextents \
+        noparallel \
         nozero \
         offset \
         partition \
@@ -909,6 +910,7 @@ AC_CONFIG_FILES([Makefile
                  filters/log/Makefile
                  filters/nocache/Makefile
                  filters/noextents/Makefile
+                 filters/noparallel/Makefile
                  filters/nozero/Makefile
                  filters/offset/Makefile
                  filters/partition/Makefile
diff --git a/filters/noparallel/noparallel.c b/filters/noparallel/noparallel.c
new file mode 100644
index 0000000..7034b01
--- /dev/null
+++ b/filters/noparallel/noparallel.c
@@ -0,0 +1,87 @@
+/* nbdkit
+ * Copyright (C) 2019 Red Hat Inc.
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions are
+ * met:
+ *
+ * * Redistributions of source code must retain the above copyright
+ * notice, this list of conditions and the following disclaimer.
+ *
+ * * Redistributions in binary form must reproduce the above copyright
+ * notice, this list of conditions and the following disclaimer in the
+ * documentation and/or other materials provided with the distribution.
+ *
+ * * Neither the name of Red Hat nor the names of its contributors may be
+ * used to endorse or promote products derived from this software without
+ * specific prior written permission.
+ *
+ * THIS SOFTWARE IS PROVIDED BY RED HAT AND CONTRIBUTORS ''AS
IS'' AND
+ * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO,
+ * THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A
+ * PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL RED HAT OR
+ * CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+ * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+ * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF
+ * USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND
+ * ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY,
+ * OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT
+ * OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
+ * SUCH DAMAGE.
+ */
+
+#include <config.h>
+
+#include <stdio.h>
+#include <stdlib.h>
+#include <stdint.h>
+#include <string.h>
+#include <stdbool.h>
+#include <assert.h>
+
+#include <nbdkit-filter.h>
+
+#define THREAD_MODEL NBDKIT_THREAD_MODEL_PARALLEL
+
+static int thread_model = NBDKIT_THREAD_MODEL_SERIALIZE_REQUESTS;
+
+static int
+noparallel_config (nbdkit_next_config *next, void *nxdata,
+                   const char *key, const char *value)
+{
+  if (strcmp (key, "serialize") == 0 ||
+      strcmp (key, "serialise") == 0) {
+    if (strcmp (value, "connections") == 0)
+      thread_model = NBDKIT_THREAD_MODEL_SERIALIZE_CONNECTIONS;
+    else if (strcmp (value, "all_requests") == 0 ||
+             strcmp (value, "all-requests") == 0)
+      thread_model = NBDKIT_THREAD_MODEL_SERIALIZE_ALL_REQUESTS;
+    else if (strcmp (value, "requests") != 0) {
+      nbdkit_error ("unknown noparallel serialize mode '%s'",
value);
+      return -1;
+    }
+    return 0;
+  }
+  return next (nxdata, key, value);
+}
+
+#define noparallel_config_help \
+  "serialize=<MODE>      'requests' (default),
'all-requests', or 'connections'.\n" \
+
+/* Apply runtime reduction to thread model. */
+static int
+noparallel_thread_model (void)
+{
+  return thread_model;
+}
+
+static struct nbdkit_filter filter = {
+  .name              = "noparallel",
+  .longname          = "nbdkit noparallel filter",
+  .version           = PACKAGE_VERSION,
+  .config            = noparallel_config,
+  .config_help       = noparallel_config_help,
+  .thread_model      = noparallel_thread_model,
+};
+
+NBDKIT_REGISTER_FILTER(filter)
diff --git a/filters/noparallel/Makefile.am b/filters/noparallel/Makefile.am
new file mode 100644
index 0000000..9c34ee2
--- /dev/null
+++ b/filters/noparallel/Makefile.am
@@ -0,0 +1,61 @@
+# nbdkit
+# Copyright (C) 2019 Red Hat Inc.
+#
+# Redistribution and use in source and binary forms, with or without
+# modification, are permitted provided that the following conditions are
+# met:
+#
+# * Redistributions of source code must retain the above copyright
+# notice, this list of conditions and the following disclaimer.
+#
+# * Redistributions in binary form must reproduce the above copyright
+# notice, this list of conditions and the following disclaimer in the
+# documentation and/or other materials provided with the distribution.
+#
+# * Neither the name of Red Hat nor the names of its contributors may be
+# used to endorse or promote products derived from this software without
+# specific prior written permission.
+#
+# THIS SOFTWARE IS PROVIDED BY RED HAT AND CONTRIBUTORS ''AS
IS'' AND
+# ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO,
+# THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A
+# PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL RED HAT OR
+# CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+# SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+# LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF
+# USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND
+# ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY,
+# OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT
+# OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
+# SUCH DAMAGE.
+
+include $(top_srcdir)/common-rules.mk
+
+EXTRA_DIST = nbdkit-noparallel-filter.pod
+
+filter_LTLIBRARIES = nbdkit-noparallel-filter.la
+
+nbdkit_noparallel_filter_la_SOURCES = \
+	noparallel.c \
+	$(top_srcdir)/include/nbdkit-filter.h
+
+nbdkit_noparallel_filter_la_CPPFLAGS = \
+	-I$(top_srcdir)/include \
+	-I$(top_srcdir)/common/include
+nbdkit_noparallel_filter_la_CFLAGS = \
+	$(WARNINGS_CFLAGS)
+nbdkit_noparallel_filter_la_LDFLAGS = \
+	-module -avoid-version -shared \
+	-Wl,--version-script=$(top_srcdir)/filters/filters.syms
+
+if HAVE_POD
+
+man_MANS = nbdkit-noparallel-filter.1
+CLEANFILES += $(man_MANS)
+
+nbdkit-noparallel-filter.1: nbdkit-noparallel-filter.pod
+	$(PODWRAPPER) --section=1 --man $@ \
+	    --html $(top_builddir)/html/$@.html \
+	    $<
+
+endif HAVE_POD
diff --git a/tests/test-eflags.sh b/tests/test-eflags.sh
index 14a0099..8158f75 100755
--- a/tests/test-eflags.sh
+++ b/tests/test-eflags.sh
@@ -277,6 +277,22 @@ 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 ;;
diff --git a/tests/test-parallel-file.sh b/tests/test-parallel-file.sh
index 3012384..8335dc9 100755
--- a/tests/test-parallel-file.sh
+++ b/tests/test-parallel-file.sh
@@ -1,6 +1,6 @@
 #!/usr/bin/env bash
 # nbdkit
-# Copyright (C) 2017-2018 Red Hat Inc.
+# Copyright (C) 2017-2019 Red Hat Inc.
 #
 # Redistribution and use in source and binary forms, with or without
 # modification, are permitted provided that the following conditions are
@@ -72,4 +72,15 @@ wrote 512/512 bytes at offset 512"; then
   exit 1
 fi

+# With --filter=noparallel, the write should complete first because it was
issued first
+nbdkit -v -U - --filter=noparallel --filter=delay file test-parallel-file.data
\
+  wdelay=2 rdelay=1 --run 'qemu-io -f raw -c "aio_write -P 2 512
512" \
+                           -c "aio_read -P 1 0 512" -c aio_flush
$nbd' |
+    tee test-parallel-file.out
+if test "$(grep '512/512' test-parallel-file.out)" != \
+"wrote 512/512 bytes at offset 512
+read 512/512 bytes at offset 0"; then
+  exit 1
+fi
+
 exit 0
-- 
2.20.1

Eric Blake

2019-May-17 20:20 UTC

head link

[Libguestfs] [nbdkit PATCH 3/3] filters: Use only .thread_model, not THREAD_MODEL

No need for filters to specify their model in two different ways; the
callback alone is sufficient. Since all filters are in-tree, we can
deal with the API/ABI change made here.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 docs/nbdkit-filter.pod          | 48 +++++++++++++++------------------
 include/nbdkit-filter.h         |  6 ++---
 filters/cow/cow.c               |  2 --
 filters/delay/delay.c           |  2 --
 filters/error/error.c           |  2 --
 filters/fua/fua.c               |  2 --
 filters/log/log.c               |  2 --
 filters/nocache/nocache.c       |  2 --
 filters/noextents/noextents.c   |  2 --
 filters/noparallel/noparallel.c |  2 --
 filters/nozero/nozero.c         |  2 --
 filters/offset/offset.c         |  2 --
 filters/partition/partition.c   |  2 --
 filters/rate/rate.c             |  2 --
 filters/readahead/readahead.c   |  2 --
 filters/stats/stats.c           |  2 --
 filters/truncate/truncate.c     |  2 --
 filters/xz/xz.c                 |  6 ++++-
 server/filters.c                | 10 +++----
 19 files changed, 32 insertions(+), 68 deletions(-)

diff --git a/docs/nbdkit-filter.pod b/docs/nbdkit-filter.pod
index 4033789..86894f1 100644
--- a/docs/nbdkit-filter.pod
+++ b/docs/nbdkit-filter.pod
@@ -6,8 +6,6 @@ nbdkit-filter - how to write nbdkit filters

  #include <nbdkit-filter.h>
  
- #define THREAD_MODEL NBDKIT_THREAD_MODEL_PARALLEL
- 
  static int
  myfilter_config (nbdkit_next_config *next, void *nxdata,
                   const char *key, const char *value)
@@ -107,24 +105,6 @@ should not expect to distribute filters separately from
nbdkit.

 All filters should start by including this header file.

-=head1 C<#define THREAD_MODEL>
-
-All filters must define a default thread model by defining
-C<THREAD_MODEL>.  See L<nbdkit-plugin(3)/THREADS> for a discussion
of
-thread models.
-
-The final thread model is the smallest (ie. most serialized) out of
-all the filters and the plugin.  Filters cannot alter the thread model
-to make it larger (more parallel).  However, they can dynamically
-request a smaller (more serialized) model from decisions made during
-C<.config> and C<.config_complete>, by implementing a
<.thread_model>
-function callback.
-
-If possible filters should be be written to handle fully parallel
-requests (C<NBDKIT_THREAD_MODEL_PARALLEL>, even multiple requests
-issued in parallel on the same connection).  This ensures that they
-don't slow down other filters or plugins.
-
 =head1 C<struct nbdkit_filter>

 All filters must define and register one C<struct nbdkit_filter>,
@@ -275,13 +255,27 @@ omit this.

  int (*thread_model) (void);

-This optional function is called after C<.config_complete> to provide
-the filter an opportunity to reduce the resulting thread model below
-the value used at compilation via C<THREAD_MODEL>. The resulting
-thread model for all connections is determined by selecting the most
-restrictive model from the C<THREAD_MODEL> of the plugin and all
-filters, and from the results of any C<.thread_model> across all
-filters.
+=head1 C<#define THREAD_MODEL>
+
+Filters may tighten (but not relax) the thread model of the plugin, by
+defining this callback.  Note that while plugins use a compile-time
+definition of C<THREAD_MODEL>, filters do not need to declare a model
+at compile time; instead, this callback is called after
+C<.config_complete> and before any connections are created.  See
+L<nbdkit-plugin(3)/THREADS> for a discussion of thread models.
+
+The final thread model used by nbdkit is the smallest (ie. most
+serialized) out of all the filters and the plugin, and applies for all
+connections.  Requests for a model larger than permitted by the plugin
+are silently ignored. It is acceptable for decisions made during
+C<.config> and C<.config_complete> to determine which model to
+request.
+
+This callback is optional; if it is not present, the filter must be
+written to handle fully parallel requests, even if it is multiple
+requests issued in parallel on the same connection, similar to a
+plugin requesting C<NBDKIT_THREAD_MODEL_PARALLEL>.  This ensures the
+filter doesn't slow down other filters or plugins.

 If there is an error, C<.thread_model> should call C<nbdkit_error>
 with an error message and return C<-1>.
diff --git a/include/nbdkit-filter.h b/include/nbdkit-filter.h
index 70d32ce..94f1778 100644
--- a/include/nbdkit-filter.h
+++ b/include/nbdkit-filter.h
@@ -93,13 +93,12 @@ struct nbdkit_next_ops {
 };

 struct nbdkit_filter {
-  /* Do not set these fields directly; use NBDKIT_REGISTER_FILTER.
-   * They exist so that we can diagnose filters compiled against
+  /* Do not set this field directly; use NBDKIT_REGISTER_FILTER.
+   * It exists so that we can diagnose filters compiled against
    * one version of the header with a runtime compiled against a
    * different version.
    */
   int _api_version;
-  int _thread_model;

   /* Because there is no ABI guarantee, new fields may be added
    * where logically appropriate.  */
@@ -178,7 +177,6 @@ struct nbdkit_filter {
   filter_init (void)                                                    \
   {                                                                     \
     (filter)._api_version = NBDKIT_FILTER_API_VERSION;                  \
-    (filter)._thread_model = THREAD_MODEL;                              \
     return &(filter);                                                   \
   }

diff --git a/filters/cow/cow.c b/filters/cow/cow.c
index 006007e..ae4a3af 100644
--- a/filters/cow/cow.c
+++ b/filters/cow/cow.c
@@ -51,8 +51,6 @@
 #include "minmax.h"
 #include "rounding.h"

-#define THREAD_MODEL NBDKIT_THREAD_MODEL_PARALLEL
-
 /* In order to handle parallel requests safely, this lock must be held
  * when calling any blk_* functions.
  */
diff --git a/filters/delay/delay.c b/filters/delay/delay.c
index 486a24e..11681c8 100644
--- a/filters/delay/delay.c
+++ b/filters/delay/delay.c
@@ -40,8 +40,6 @@

 #include <nbdkit-filter.h>

-#define THREAD_MODEL NBDKIT_THREAD_MODEL_PARALLEL
-
 static int delay_read_ms = 0;   /* read delay (milliseconds) */
 static int delay_write_ms = 0;  /* write delay (milliseconds) */
 static int delay_zero_ms = 0;   /* zero delay (milliseconds) */
diff --git a/filters/error/error.c b/filters/error/error.c
index aba6213..968f283 100644
--- a/filters/error/error.c
+++ b/filters/error/error.c
@@ -48,8 +48,6 @@
 #include "cleanup.h"
 #include "random.h"

-#define THREAD_MODEL NBDKIT_THREAD_MODEL_PARALLEL
-
 struct error_settings {
   int error;                   /* errno, eg. EIO */
   double rate;                 /* rate, 0.0 = never, 1.0 = always */
diff --git a/filters/fua/fua.c b/filters/fua/fua.c
index 120ff0a..9d0e561 100644
--- a/filters/fua/fua.c
+++ b/filters/fua/fua.c
@@ -41,8 +41,6 @@

 #include <nbdkit-filter.h>

-#define THREAD_MODEL NBDKIT_THREAD_MODEL_PARALLEL
-
 static enum FuaMode {
   NONE,
   EMULATE,
diff --git a/filters/log/log.c b/filters/log/log.c
index 133e352..9c0f35c 100644
--- a/filters/log/log.c
+++ b/filters/log/log.c
@@ -47,8 +47,6 @@

 #include "cleanup.h"

-#define THREAD_MODEL NBDKIT_THREAD_MODEL_PARALLEL
-
 static uint64_t connections;
 static char *logfilename;
 static FILE *logfile;
diff --git a/filters/nocache/nocache.c b/filters/nocache/nocache.c
index abb042e..a3f1198 100644
--- a/filters/nocache/nocache.c
+++ b/filters/nocache/nocache.c
@@ -43,8 +43,6 @@

 #include "minmax.h"

-#define THREAD_MODEL NBDKIT_THREAD_MODEL_PARALLEL
-
 static enum CacheMode {
   NONE,
   EMULATE,
diff --git a/filters/noextents/noextents.c b/filters/noextents/noextents.c
index 59f4c37..e39723c 100644
--- a/filters/noextents/noextents.c
+++ b/filters/noextents/noextents.c
@@ -34,8 +34,6 @@

 #include <nbdkit-filter.h>

-#define THREAD_MODEL NBDKIT_THREAD_MODEL_PARALLEL
-
 static int
 noextents_can_extents (struct nbdkit_next_ops *next_ops, void *nxdata,
                        void *handle)
diff --git a/filters/noparallel/noparallel.c b/filters/noparallel/noparallel.c
index 7034b01..057485f 100644
--- a/filters/noparallel/noparallel.c
+++ b/filters/noparallel/noparallel.c
@@ -41,8 +41,6 @@

 #include <nbdkit-filter.h>

-#define THREAD_MODEL NBDKIT_THREAD_MODEL_PARALLEL
-
 static int thread_model = NBDKIT_THREAD_MODEL_SERIALIZE_REQUESTS;

 static int
diff --git a/filters/nozero/nozero.c b/filters/nozero/nozero.c
index 6e0ffa9..16ec710 100644
--- a/filters/nozero/nozero.c
+++ b/filters/nozero/nozero.c
@@ -43,8 +43,6 @@

 #include "minmax.h"

-#define THREAD_MODEL NBDKIT_THREAD_MODEL_PARALLEL
-
 #define MAX_WRITE (64 * 1024 * 1024)

 static const char buffer[MAX_WRITE];
diff --git a/filters/offset/offset.c b/filters/offset/offset.c
index fe07d28..efe5c6d 100644
--- a/filters/offset/offset.c
+++ b/filters/offset/offset.c
@@ -41,8 +41,6 @@

 #include "cleanup.h"

-#define THREAD_MODEL NBDKIT_THREAD_MODEL_PARALLEL
-
 static int64_t offset = 0, range = -1;

 /* Called for each key=value passed on the command line. */
diff --git a/filters/partition/partition.c b/filters/partition/partition.c
index ee2cc77..56ad05e 100644
--- a/filters/partition/partition.c
+++ b/filters/partition/partition.c
@@ -45,8 +45,6 @@

 #include "partition.h"

-#define THREAD_MODEL NBDKIT_THREAD_MODEL_PARALLEL
-
 int partnum = -1;

 /* Called for each key=value passed on the command line. */
diff --git a/filters/rate/rate.c b/filters/rate/rate.c
index cf03541..30e093e 100644
--- a/filters/rate/rate.c
+++ b/filters/rate/rate.c
@@ -50,8 +50,6 @@

 #include "bucket.h"

-#define THREAD_MODEL NBDKIT_THREAD_MODEL_PARALLEL
-
 /* Per-connection and global limit, both in bits per second, with zero
  * meaning not set / not enforced.  These are only used when reading
  * the command line and initializing the buckets for the first time.
diff --git a/filters/readahead/readahead.c b/filters/readahead/readahead.c
index 95bda0e..b6c1809 100644
--- a/filters/readahead/readahead.c
+++ b/filters/readahead/readahead.c
@@ -45,8 +45,6 @@
 #include "cleanup.h"
 #include "minmax.h"

-#define THREAD_MODEL NBDKIT_THREAD_MODEL_PARALLEL
-
 /* Copied from server/plugins.c. */
 #define MAX_REQUEST_SIZE (64 * 1024 * 1024)

diff --git a/filters/stats/stats.c b/filters/stats/stats.c
index 037fc61..4f94f73 100644
--- a/filters/stats/stats.c
+++ b/filters/stats/stats.c
@@ -47,8 +47,6 @@
 #include "cleanup.h"
 #include "tvdiff.h"

-#define THREAD_MODEL NBDKIT_THREAD_MODEL_PARALLEL
-
 static char *filename;
 static bool append;
 static FILE *fp;
diff --git a/filters/truncate/truncate.c b/filters/truncate/truncate.c
index 38b1cd9..64be839 100644
--- a/filters/truncate/truncate.c
+++ b/filters/truncate/truncate.c
@@ -46,8 +46,6 @@
 #include "iszero.h"
 #include "rounding.h"

-#define THREAD_MODEL NBDKIT_THREAD_MODEL_PARALLEL
-
 /* These are the parameters. */
 static int64_t truncate_size = -1;
 static unsigned round_up = 0, round_down = 0;
diff --git a/filters/xz/xz.c b/filters/xz/xz.c
index 8ada294..51ac919 100644
--- a/filters/xz/xz.c
+++ b/filters/xz/xz.c
@@ -246,7 +246,10 @@ xz_pread (struct nbdkit_next_ops *next_ops, void *nxdata,
   return 0;
 }

-#define THREAD_MODEL NBDKIT_THREAD_MODEL_SERIALIZE_REQUESTS
+static int xz_thread_model (void)
+{
+  return NBDKIT_THREAD_MODEL_SERIALIZE_REQUESTS;
+}

 static struct nbdkit_filter filter = {
   .name              = "xz",
@@ -254,6 +257,7 @@ static struct nbdkit_filter filter = {
   .version           = PACKAGE_VERSION,
   .config            = xz_config,
   .config_help       = xz_config_help,
+  .thread_model      = xz_thread_model,
   .open              = xz_open,
   .close             = xz_close,
   .prepare           = xz_prepare,
diff --git a/server/filters.c b/server/filters.c
index 87a9c0e..3d9e1ef 100644
--- a/server/filters.c
+++ b/server/filters.c
@@ -93,20 +93,18 @@ static int
 filter_thread_model (struct backend *b)
 {
   struct backend_filter *f = container_of (b, struct backend_filter, backend);
-  int filter_thread_model = f->filter._thread_model;
+  int filter_thread_model = NBDKIT_THREAD_MODEL_PARALLEL;
   int thread_model = f->backend.next->thread_model (f->backend.next);

-  if (filter_thread_model < thread_model) /* more serialized */
-    thread_model = filter_thread_model;
-
   if (f->filter.thread_model) {
     filter_thread_model = f->filter.thread_model ();
     if (filter_thread_model == -1)
       exit (EXIT_FAILURE);
-    if (filter_thread_model < thread_model)
-      thread_model = filter_thread_model;
   }

+  if (filter_thread_model < thread_model) /* more serialized */
+    thread_model = filter_thread_model;
+
   return thread_model;
 }

-- 
2.20.1

Richard W.M. Jones

2019-May-18 09:23 UTC

head link

Re: [Libguestfs] [nbdkit PATCH 3/3] filters: Use only .thread_model, not THREAD_MODEL

Looks sensible, ACK series.

There's also a case for allowing plugins to choose the thread model
dynamically (using the same or a similar callback).  At the moment we
are unable to choose a thread model in most of the language plugins
because they generally require the language interpreter to be
initialized after the nbdkit_plugin struct has been finalized.

For example in the Perl plugin, we don't create the interpreter until
‘.load’ is called, but that happens long after nbdkit_plugin struct
has been returned to the server.  I think the only exceptions to this
rule are the natively compiled language plugins, ie. OCaml and Rust,
where we create the struct from the language.

Note also if we do this we can choose a new thread model ABI either
with gaps for future threading levels, or some other kind of ABI not
based on numbers.  There is a case for having a threading model where
everything runs from the main thread (required by VDDK).

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/

Seemingly Similar Threads

Search for more reasonably related threads

Libguestfs - May 2019 - [nbdkit PATCH 3/3] filters: Use only .thread_model, not THREAD_MODEL

[Libguestfs] [nbdkit PATCH 0/3] Add noparallel filter

[Libguestfs] [nbdkit PATCH 1/3] server: Allow filters to reduce thread model dynamically

[Libguestfs] [nbdkit PATCH 2/3] noparallel: Implement new filter

[Libguestfs] [nbdkit PATCH 3/3] filters: Use only .thread_model, not THREAD_MODEL

Re: [Libguestfs] [nbdkit PATCH 3/3] filters: Use only .thread_model, not THREAD_MODEL

Seemingly Similar Threads