9:23 a.m.
We can't enable the knob by default since many nbdkit plugins don't
care about export name, but it is certainly more efficient to limit
consistency to just clients visiting the same export. Thankfully,
NBD_CMD_FLUSH does not take size and offset, and therefore is safe
(although wasting CPU cycles) to use even when a different export has
a different size.
---
Respinning just this patch to fix the flaw I noted earlier about not
properly tracking which connections are dirty when filtering by gruop
name.
.../multi-conn/nbdkit-multi-conn-filter.pod | 29 ++++-
tests/Makefile.am | 2 +
filters/multi-conn/multi-conn.c | 118 +++++++++++++++---
tests/test-multi-conn-name.sh | 88 +++++++++++++
tests/test-multi-conn-plugin.sh | 63 ++++++----
5 files changed, 256 insertions(+), 44 deletions(-)
create mode 100755 tests/test-multi-conn-name.sh
diff --git a/filters/multi-conn/nbdkit-multi-conn-filter.pod
b/filters/multi-conn/nbdkit-multi-conn-filter.pod
index 007c4e3a..efeba570 100644
--- a/filters/multi-conn/nbdkit-multi-conn-filter.pod
+++ b/filters/multi-conn/nbdkit-multi-conn-filter.pod
@@ -4,8 +4,8 @@ nbdkit-multi-conn-filter - nbdkit multi-conn filter
=head1 SYNOPSIS
- nbdkit --filter=multi-conn plugin \
- [multi-conn-mode=MODE] [multi-conn-track-dirty=LEVEL] [plugin-args...]
+ nbdkit --filter=multi-conn plugin [multi-conn-mode=MODE] \
+ [multi-conn-track-dirty=LEVEL] [multi-conn-exportname=BOOL] [plugin-args...]
=head1 DESCRIPTION
@@ -26,7 +26,8 @@ it also has additional modes useful for evaluating performance and
correctness of client and plugin multi-conn behaviors. This filter
assumes that multiple connections to a plugin will eventually share
data, other than any caching effects; it is not suitable for use with
-a plugin that produces completely independent data per connection.
+a plugin that produces completely independent data per connection from
+the same export name.
Additional control over the behavior of client flush commands is
possible by combining this filter with L<nbdkit-fua-filter(1)>. Note
@@ -125,6 +126,27 @@ B<multi-conn-mode=emulate>, a client which disregards
NBD_FLAG_MULTI_CONN by flushing on each connection itself results in a
quadratic number of flush operations on the plugin.
+=item B<multi-conn-exportname=false>
+
+The exportname switch defaults to false for safety, and causes the
+filter to flush across all active connections regardless of the export
+name in use by that connection when doing emulation. However, when a
+plugin supports distinct data according to export name, this behavior
+will penalize the performance of clients visiting an unrelated export
+by spending time on replicated flush operations not actually relevant
+to that export.
+
+=item B<multi-conn-exportname=true>
+
+Setting the exportname switch to true causes the filter to only
+synchronize flushes to connections visiting the same export name.
+This avoids penalizing clients visiting an unrelated export name (such
+as L<nbdkit-file-plugin(1)> in B<dir=> mode), but is unsafe when used
+with a plugin that serves shared content across all connections
+regardless of the export name requested by the client, if that plugin
+is not already multi-conn consistent (such as
+L<nbdkit-vddk-plugin(1)>).
+
=back
=head1 EXAMPLES
@@ -161,6 +183,7 @@ C<nbdkit-multi-conn-filter> first appeared in nbdkit 1.26.
L<nbdkit(1)>,
L<nbdkit-file-plugin(1)>,
+L<nbdkit-vddk-plugin(1)>,
L<nbdkit-filter(3)>,
L<nbdkit-cache-filter(1)>,
L<nbdkit-fua-filter(1)>,
diff --git a/tests/Makefile.am b/tests/Makefile.am
index 4b3ee65c..6df0b490 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -1541,10 +1541,12 @@ EXTRA_DIST += \
# multi-conn filter test.
TESTS += \
test-multi-conn.sh \
+ test-multi-conn-name.sh \
$(NULL)
EXTRA_DIST += \
test-multi-conn-plugin.sh \
test-multi-conn.sh \
+ test-multi-conn-name.sh \
$(NULL)
# nofilter test.
diff --git a/filters/multi-conn/multi-conn.c b/filters/multi-conn/multi-conn.c
index d5459410..2335e13f 100644
--- a/filters/multi-conn/multi-conn.c
+++ b/filters/multi-conn/multi-conn.c
@@ -60,6 +60,8 @@ static enum TrackDirtyMode {
OFF,
} track;
+static bool byname = false;
+
enum dirty {
WRITE = 1, /* A write may have populated a cache */
READ = 2, /* A read may have populated a cache */
@@ -74,12 +76,21 @@ struct handle {
void *nxdata;
enum MultiConnMode mode; /* Runtime resolution of mode==AUTO */
enum dirty dirty; /* What aspects of this connection are dirty */
+ char *name; /* Used when byname==true to assign group */
+ struct group *group; /* All connections grouped with this one */
};
DEFINE_VECTOR_TYPE(conns_vector, struct handle *);
-static conns_vector conns = empty_vector;
-static bool dirty; /* True if any connection is dirty */
+struct group {
+ conns_vector conns;
+ char *name;
+ bool dirty; /* True if any connection in group is dirty */
+};
+DEFINE_VECTOR_TYPE(group_vector, struct group *);
+static group_vector groups = empty_vector;
-/* Accept 'multi-conn-mode=mode' and 'multi-conn-track-dirty=level' */
+/* Accept 'multi-conn-mode=mode', 'multi-conn-track-dirty=level', and
+ * 'multi-conn-exportname=bool'.
+ */
static int
multi_conn_config (nbdkit_next_config *next, void *nxdata,
const char *key, const char *value)
@@ -115,13 +126,24 @@ multi_conn_config (nbdkit_next_config *next, void *nxdata,
}
return 0;
}
+ else if (strcmp (key, "multi-conn-exportname") == 0 ||
+ strcmp (key, "multi-conn-export-name") == 0) {
+ int r;
+
+ r = nbdkit_parse_bool (value);
+ if (r == -1)
+ return -1;
+ byname = r;
+ return 0;
+ }
return next (nxdata, key, value);
}
#define multi_conn_config_help \
"multi-conn-mode=<MODE> 'auto' (default),
'emulate', 'plugin',\n" \
" 'disable', or 'unsafe'.\n"
\
- "multi-conn-track-dirty=<LEVEL> 'conn' (default), 'fast',
or 'off'.\n"
+ "multi-conn-track-dirty=<LEVEL> 'conn' (default), 'fast',
or 'off'.\n" \
+ "multi-conn-exportname=<BOOL> true to limit emulation by export
name.\n"
static int
multi_conn_get_ready (nbdkit_next_get_ready *next, void *nxdata,
@@ -148,6 +170,13 @@ multi_conn_open (nbdkit_next_open *next, void *nxdata,
nbdkit_error ("calloc: %m");
return NULL;
}
+ if (byname) {
+ h->name = strdup (exportname);
+ if (h->name == NULL) {
+ nbdkit_error ("strdup: %m");
+ return NULL;
+ }
+ }
return h;
}
@@ -156,6 +185,8 @@ multi_conn_prepare (struct nbdkit_next_ops *next_ops, void *nxdata,
void *handle, int readonly)
{
struct handle *h = handle;
+ struct group *g;
+ bool new_group = false;
int r;
h->next_ops = next_ops;
@@ -177,7 +208,39 @@ multi_conn_prepare (struct nbdkit_next_ops *next_ops, void *nxdata,
}
ACQUIRE_LOCK_FOR_CURRENT_SCOPE (&lock);
- return conns_vector_append (&conns, h);
+ if (byname) {
+ g = NULL;
+ for (size_t i = 0; i < groups.size; i++)
+ if (strcmp (groups.ptr[i]->name, h->name) == 0) {
+ g = groups.ptr[i];
+ break;
+ }
+ }
+ else
+ g = groups.size ? groups.ptr[0] : NULL;
+
+ if (!g) {
+ g = calloc (1, sizeof *g);
+ if (g == NULL) {
+ nbdkit_error ("calloc: %m");
+ return -1;
+ }
+ if (group_vector_append (&groups, g) == -1)
+ return -1;
+ g->name = h->name;
+ h->name = NULL;
+ new_group = true;
+ }
+ if (conns_vector_append (&g->conns, h) == -1) {
+ if (new_group) {
+ group_vector_remove (&groups, groups.size - 1);
+ free (g->name);
+ free (g);
+ }
+ return -1;
+ }
+ h->group = g;
+ return 0;
}
static int
@@ -189,21 +252,36 @@ multi_conn_finalize (struct nbdkit_next_ops *next_ops, void
*nxdata,
ACQUIRE_LOCK_FOR_CURRENT_SCOPE (&lock);
assert (h->next_ops == next_ops);
assert (h->nxdata == nxdata);
+ assert (h->group);
/* XXX should we add a config param to flush if the client forgot? */
- for (size_t i = 0; i < conns.size; i++) {
- if (conns.ptr[i] == h) {
- conns_vector_remove (&conns, i);
+ for (size_t i = 0; i < h->group->conns.size; i++) {
+ if (h->group->conns.ptr[i] == h) {
+ conns_vector_remove (&h->group->conns, i);
break;
}
}
+ if (h->group->conns.size == 0) {
+ for (size_t i = 0; i < groups.size; i++)
+ if (groups.ptr[i] == h->group) {
+ group_vector_remove (&groups, i);
+ free (h->group->name);
+ free (h->group);
+ break;
+ }
+ }
+ h->group = NULL;
return 0;
}
static void
multi_conn_close (void *handle)
{
- free (handle);
+ struct handle *h = handle;
+
+ assert (h->group == NULL);
+ free (h->name);
+ free (h);
}
static int
@@ -259,7 +337,7 @@ mark_dirty (struct handle *h, bool is_write)
/* fallthrough */
case FAST:
if (is_write)
- dirty = true;
+ h->group->dirty = true;
break;
case OFF:
break;
@@ -376,22 +454,24 @@ multi_conn_flush (struct nbdkit_next_ops *next_ops, void *nxdata,
struct handle *h = handle, *h2;
size_t i;
+ assert (h->group);
if (h->mode == EMULATE) {
ACQUIRE_LOCK_FOR_CURRENT_SCOPE (&lock);
- for (i = 0; i < conns.size; i++) {
- h2 = conns.ptr[i];
- if (track == OFF || (dirty && (track == FAST || h2->dirty & READ))
||
+ for (i = 0; i < h->group->conns.size; i++) {
+ h2 = h->group->conns.ptr[i];
+ if (track == OFF || (h->group->dirty &&
+ (track == FAST || h2->dirty & READ)) ||
h2->dirty & WRITE) {
if (h2->next_ops->flush (h2->nxdata, flags, err) == -1)
return -1;
h2->dirty = 0;
}
}
- dirty = 0;
+ h->group->dirty = 0;
}
else {
/* !EMULATE: Check if the image is clean, allowing us to skip a flush. */
- if (track != OFF && !dirty)
+ if (track != OFF && !h->group->dirty)
return 0;
/* Perform the flush, then update dirty tracking. */
if (next_ops->flush (nxdata, flags, err) == -1)
@@ -400,15 +480,15 @@ multi_conn_flush (struct nbdkit_next_ops *next_ops, void *nxdata,
case CONN:
if (next_ops->can_multi_conn (nxdata) == 1) {
ACQUIRE_LOCK_FOR_CURRENT_SCOPE (&lock);
- for (i = 0; i < conns.size; i++)
- conns.ptr[i]->dirty = 0;
- dirty = 0;
+ for (i = 0; i < h->group->conns.size; i++)
+ h->group->conns.ptr[i]->dirty = 0;
+ h->group->dirty = 0;
}
else
h->dirty = 0;
break;
case FAST:
- dirty = false;
+ h->group->dirty = false;
break;
case OFF:
break;
diff --git a/tests/test-multi-conn-name.sh b/tests/test-multi-conn-name.sh
new file mode 100755
index 00000000..698d386e
--- /dev/null
+++ b/tests/test-multi-conn-name.sh
@@ -0,0 +1,88 @@
+#!/usr/bin/env bash
+# nbdkit
+# Copyright (C) 2018-2021 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.
+
+# Demonstrate effect of multi-conn-exportname config flag
+
+source ./functions.sh
+set -e
+set -x
+
+requires_plugin sh
+requires_nbdsh_uri
+requires nbdsh -c 'print(h.set_opt_mode)'
+requires dd iflag=count_bytes </dev/null
+
+files="test-multi-conn-name.out"
+rm -f $files
+cleanup_fn rm -f $files
+
+fail=0
+export script='
+import os
+
+uri = os.environ["uri"]
+h = {}
+for name in ["a", "b"]:
+ for conn in [1, 2]:
+ key = "%s%d" % (name, conn)
+ h[key] = nbd.NBD()
+ h[key].set_opt_mode(True)
+ h[key].connect_uri(uri)
+ h[key].set_export_name(name)
+ h[key].opt_go()
+h["a1"].pread(1, 0)
+h["a2"].pwrite(b"A", 0)
+h["b1"].pread(1, 0)
+h["b2"].pwrite(b"B", 0, nbd.CMD_FLAG_FUA)
+print(h["a1"].pread(1, 0))
+print(h["b1"].pread(1, 0))
+'
+
+# Without the knob we flush all exports
+nbdkit -vf -U - sh test-multi-conn-plugin.sh --filter=multi-conn \
+ --run 'export uri; nbdsh -c "$script"' > test-multi-conn-name.out
|| fail=1
+diff -u <(cat <<\EOF
+b'A'
+b'B'
+EOF
+ ) test-multi-conn-name.out || fail=1
+# But with the knob, our flush is specific to the correct export
+nbdkit -vf -U - sh test-multi-conn-plugin.sh --filter=multi-conn \
+ multi-conn-exportname=true \
+ --run 'export uri; nbdsh -c "$script"' > test-multi-conn-name.out
|| fail=1
+diff -u <(cat <<\EOF
+b'I'
+b'B'
+EOF
+ ) test-multi-conn-name.out || fail=1
+
+exit $fail
diff --git a/tests/test-multi-conn-plugin.sh b/tests/test-multi-conn-plugin.sh
index 7262a348..5d627a07 100755
--- a/tests/test-multi-conn-plugin.sh
+++ b/tests/test-multi-conn-plugin.sh
@@ -39,30 +39,36 @@
# it ever sends overlapping writes without coordinating flushes and still
# expects any particular write to occur last).
+get_export() {
+ case $1 in
+ */*) export="$tmpdir/$(dirname $1)" conn=$(basename $1) ;;
+ *) export="$tmpdir" conn=$1 ;;
+ esac
+}
fill_cache() {
- if test ! -f "$tmpdir/$1"; then
- cp "$tmpdir/0" "$tmpdir/$1"
+ if test ! -f "$export/$conn"; then
+ cp "$export/0" "$export/$conn"
fi
}
do_fua() {
- case ,$4, in
+ case ,$3, in
*,fua,*)
if test -f "$tmpdir/strictfua"; then
- dd of="$tmpdir/0" if="$tmpdir/$1" skip=$3 seek=$3
count=$2 \
+ dd of="$export/0" if="$export/$conn" skip=$2 seek=$2
count=$1 \
conv=notrunc iflag=count_bytes,skip_bytes oflag=seek_bytes
else
- do_flush $1
+ do_flush
fi ;;
esac
}
do_flush() {
- if test -f "$tmpdir/$1-replay"; then
+ if test -f "$export/$conn-replay"; then
while read cnt off; do
- dd of="$tmpdir/0" if="$tmpdir/$1" skip=$off seek=$off
count=$cnt \
+ dd of="$export/0" if="$export/$conn" skip=$off seek=$off
count=$cnt \
conv=notrunc iflag=count_bytes,skip_bytes oflag=seek_bytes
- done < "$tmpdir/$1-replay"
+ done < "$export/$conn-replay"
fi
- rm -f "$tmpdir/$1" "$tmpdir/$1-replay"
+ rm -f "$export/$conn" "$export/$conn-replay"
}
case "$1" in
config)
@@ -91,30 +97,43 @@ case "$1" in
;;
open)
read i < "$tmpdir/counter"
- echo $((i+1)) | tee "$tmpdir/counter"
+ i=$((i+1))
+ echo $i > "$tmpdir/counter"
+ if test -z "$3"; then
+ echo $i
+ else
+ mkdir -p "$tmpdir/$3" || exit 1
+ cp "$tmpdir/0" "$tmpdir/$3/0"
+ echo "$3/$i"
+ fi
;;
pread)
- fill_cache $2
- dd if="$tmpdir/$2" skip=$4 count=$3 iflag=count_bytes,skip_bytes
+ get_export $2
+ fill_cache
+ dd if="$export/$conn" skip=$4 count=$3 iflag=count_bytes,skip_bytes
;;
cache)
- fill_cache $2
+ get_export $2
+ fill_cache
;;
pwrite)
- fill_cache $2
- dd of="$tmpdir/$2" seek=$4 conv=notrunc oflag=seek_bytes
- echo $3 $4 >> "$tmpdir/$2-replay"
- do_fua $2 $3 $4 $5
+ get_export $2
+ fill_cache
+ dd of="$export/$conn" seek=$4 conv=notrunc oflag=seek_bytes
+ echo $3 $4 >> "$export/$conn-replay"
+ do_fua $3 $4 $5
;;
zero | trim)
- fill_cache $2
- dd of="$tmpdir/$2" if="/dev/zero" count=$3 seek=$4
conv=notrunc\
+ get_export $2
+ fill_cache
+ dd of="$export/$conn" if="/dev/zero" count=$3 seek=$4
conv=notrunc\
oflag=seek_bytes iflag=count_bytes
- echo $3 $4 >> "$tmpdir/$2-replay"
- do_fua $2 $3 $4 $5
+ echo $3 $4 >> "$export/$conn-replay"
+ do_fua $3 $4 $5
;;
flush)
- do_flush $2
+ get_export $2
+ do_flush
;;
*)
exit 2
--
2.30.1