5:39 p.m.
This filter caches the last result of the extents() call and offers a nice
speed-up for clients that only support req_on=1 in combination with plugins like
vddk, which has no overhead for returning information for multiple extents in
one call, but that call is very time-consuming.
Quick test showed that on a fast connection and a sparsely allocated 16G disk
with a OS installed `qemu-img map` runs 16s instead of 33s (out of which it
takes 5s to the first extents request). For 100G disk with no data on it, that
is one hole extent spanning the whole disk (where there is no space for
improvement) this does not add any noticeable overhead.
Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com>
---
v2:
- Errors are set in _add and _fill functions
- Return values are checked for only -1
- Added a test
- Indentation fixed
- Fixed access before lock
configure.ac | 2 +
filters/cacheextents/Makefile.am | 64 ++++++
filters/cacheextents/cacheextents.c | 196 ++++++++++++++++++
.../nbdkit-cacheextents-filter.pod | 47 +++++
tests/Makefile.am | 5 +-
tests/test-cacheextents.sh | 56 +++++
6 files changed, 369 insertions(+), 1 deletion(-)
create mode 100644 filters/cacheextents/Makefile.am
create mode 100644 filters/cacheextents/cacheextents.c
create mode 100644 filters/cacheextents/nbdkit-cacheextents-filter.pod
create mode 100755 tests/test-cacheextents.sh
diff --git a/configure.ac b/configure.ac
index 58031f3c6d86..8db5a4cac3c5 100644
--- a/configure.ac
+++ b/configure.ac
@@ -825,6 +825,7 @@ plugins="$(echo $(printf %s\\n $lang_plugins $non_lang_plugins |
sort -u))"
filters="\
blocksize \
cache \
+ cacheextents \
cow \
delay \
error \
@@ -900,6 +901,7 @@ AC_CONFIG_FILES([Makefile
filters/Makefile
filters/blocksize/Makefile
filters/cache/Makefile
+ filters/cacheextents/Makefile
filters/cow/Makefile
filters/delay/Makefile
filters/error/Makefile
diff --git a/filters/cacheextents/Makefile.am b/filters/cacheextents/Makefile.am
new file mode 100644
index 000000000000..9e5b59c09c59
--- /dev/null
+++ b/filters/cacheextents/Makefile.am
@@ -0,0 +1,64 @@
+# 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-cacheextents-filter.pod
+
+filter_LTLIBRARIES = nbdkit-cacheextents-filter.la
+
+nbdkit_cacheextents_filter_la_SOURCES = \
+ cacheextents.c \
+ $(top_srcdir)/include/nbdkit-filter.h
+
+nbdkit_cacheextents_filter_la_CPPFLAGS = \
+ -I$(top_srcdir)/include \
+ -I$(top_srcdir)/common/include \
+ -I$(top_srcdir)/common/utils
+nbdkit_cacheextents_filter_la_CFLAGS = \
+ $(WARNINGS_CFLAGS)
+nbdkit_cacheextents_filter_la_LDFLAGS = \
+ -module -avoid-version -shared \
+ -Wl,--version-script=$(top_srcdir)/filters/filters.syms
+nbdkit_cacheextents_filter_la_LIBADD = \
+ $(top_builddir)/common/utils/libutils.la
+
+if HAVE_POD
+
+man_MANS = nbdkit-cacheextents-filter.1
+CLEANFILES += $(man_MANS)
+
+nbdkit-cacheextents-filter.1: nbdkit-cacheextents-filter.pod
+ $(PODWRAPPER) --section=1 --man $@ \
+ --html $(top_builddir)/html/$@.html \
+ $<
+
+endif HAVE_POD
diff --git a/filters/cacheextents/cacheextents.c b/filters/cacheextents/cacheextents.c
new file mode 100644
index 000000000000..01c0328aa424
--- /dev/null
+++ b/filters/cacheextents/cacheextents.c
@@ -0,0 +1,196 @@
+/* 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 <errno.h>
+#include <inttypes.h>
+
+#include <pthread.h>
+
+#include <nbdkit-filter.h>
+
+#include "cleanup.h"
+
+#define THREAD_MODEL NBDKIT_THREAD_MODEL_PARALLEL
+
+/* This lock protects the global state. */
+static pthread_mutex_t lock = PTHREAD_MUTEX_INITIALIZER;
+
+/* The real size of the underlying plugin. */
+static uint64_t size;
+
+/* Cached extents from the last extents () call and its start and end for the
+ sake of simplicity. */
+struct nbdkit_extents *cache_extents;
+static uint64_t cache_start;
+static uint64_t cache_end;
+
+static void
+cacheextents_unload (void)
+{
+ nbdkit_extents_free (cache_extents);
+}
+
+static int
+cacheextents_add (struct nbdkit_extents *extents, int *err)
+{
+ size_t i = 0;
+
+ for (i = 0; i < nbdkit_extents_count (cache_extents); i++) {
+ struct nbdkit_extent ex = nbdkit_get_extent (cache_extents, i);
+ if (nbdkit_add_extent (extents, ex.offset, ex.length, ex.type) == -1) {
+ *err = errno;
+ return -1;
+ }
+ }
+
+ return 0;
+}
+
+static int
+cacheextents_fill (struct nbdkit_extents *extents, int *err)
+{
+ size_t i = 0;
+ size_t count = nbdkit_extents_count (extents);
+ struct nbdkit_extent first = nbdkit_get_extent (extents, 0);
+ struct nbdkit_extent last = nbdkit_get_extent (extents, count - 1);
+
+ nbdkit_extents_free (cache_extents);
+ cache_start = first.offset;
+ cache_end = last.offset + last.length;
+ cache_extents = nbdkit_extents_new (cache_start, cache_end);
+
+ for (i = 0; i < count; i++) {
+ struct nbdkit_extent ex = nbdkit_get_extent (extents, i);
+ nbdkit_debug ("cacheextents: updating cache with"
+ ": offset=%" PRIu64
+ ": length=%" PRIu64
+ "; type=%x",
+ ex.offset, ex.length, ex.type);
+ if (nbdkit_add_extent (cache_extents, ex.offset, ex.length, ex.type) == -1) {
+ *err = errno;
+ return -1;
+ }
+ }
+
+ return 0;
+}
+
+static int
+cacheextents_extents (struct nbdkit_next_ops *next_ops, void *nxdata,
+ void *handle, uint32_t count, uint64_t offset, uint32_t flags,
+ struct nbdkit_extents *extents,
+ int *err)
+{
+ ACQUIRE_LOCK_FOR_CURRENT_SCOPE (&lock);
+
+ nbdkit_debug ("cacheextents:"
+ " cache_start=%" PRIu64
+ " cache_end=%" PRIu64
+ " cache_extents=%p",
+ cache_start, cache_end, cache_extents);
+
+ if (cache_extents &&
+ offset >= cache_start && offset < cache_end) {
+ nbdkit_debug ("cacheextents: returning from cache");
+ return cacheextents_add (extents, err);
+ }
+
+ nbdkit_debug ("cacheextents: cache miss");
+ int r = next_ops->extents (nxdata, count, offset, flags, extents, err);
+ if (r == -1)
+ return r;
+
+ return cacheextents_fill (extents, err);
+}
+
+/* Any changes to the data need to clean the cache.
+ *
+ * Similarly to readahead filter this could be more intelligent, but there would
+ * be very little benefit.
+ */
+
+static void
+kill_cacheextents (void)
+{
+ ACQUIRE_LOCK_FOR_CURRENT_SCOPE (&lock);
+ nbdkit_extents_free (cache_extents);
+ cache_extents = NULL;
+}
+
+static int
+cacheextents_pwrite (struct nbdkit_next_ops *next_ops, void *nxdata,
+ void *handle,
+ const void *buf, uint32_t count, uint64_t offset,
+ uint32_t flags, int *err)
+{
+ kill_cacheextents ();
+ return next_ops->pwrite (nxdata, buf, count, offset, flags, err);
+}
+
+static int
+cacheextents_trim (struct nbdkit_next_ops *next_ops, void *nxdata,
+ void *handle,
+ uint32_t count, uint64_t offset, uint32_t flags,
+ int *err)
+{
+ kill_cacheextents ();
+ return next_ops->trim (nxdata, count, offset, flags, err);
+}
+
+static int
+cacheextents_zero (struct nbdkit_next_ops *next_ops, void *nxdata,
+ void *handle,
+ uint32_t count, uint64_t offset, uint32_t flags,
+ int *err)
+{
+ kill_cacheextents ();
+ return next_ops->zero (nxdata, count, offset, flags, err);
+}
+
+static struct nbdkit_filter filter = {
+ .name = "cacheextents",
+ .longname = "nbdkit cacheextents filter",
+ .version = PACKAGE_VERSION,
+ .unload = cacheextents_unload,
+ .pwrite = cacheextents_pwrite,
+ .trim = cacheextents_trim,
+ .zero = cacheextents_zero,
+ .extents = cacheextents_extents,
+};
+
+NBDKIT_REGISTER_FILTER (filter)
diff --git a/filters/cacheextents/nbdkit-cacheextents-filter.pod
b/filters/cacheextents/nbdkit-cacheextents-filter.pod
new file mode 100644
index 000000000000..88f3173b9375
--- /dev/null
+++ b/filters/cacheextents/nbdkit-cacheextents-filter.pod
@@ -0,0 +1,47 @@
+=head1 NAME
+
+nbdkit-cacheextents-filter - cache extents
+
+=head1 SYNOPSIS
+
+ nbdkit --filter=cacheextents plugin
+
+=head1 DESCRIPTION
+
+C<nbdkit-cacheextents-filter> is a filter that caches the result of last
+extents() call.
+
+A common use for this filter is to accelerate returning extents data for
+clients which ask for extents with the REQ_ONE flag set (like
+S<C<qemu-img convert>>) and is especially useful in combination with
+plugins that report multiple extents in one call, but with high latency
+for each of those calls (like L<nbdkit-vddk-plugin(1)>). For example:
+
+ nbdkit -U - --filter=cacheextents --run 'qemu-img map $nbd' vddk ...
+
+For files with big extents (when it is unlikely for one extents() call
+to return multiple different extents) this does not slow down the
+access.
+
+=head1 PARAMETERS
+
+There are no parameters specific to nbdkit-cacheextents-filter. Any
+parameters are passed through to and processed by the underlying
+plugin in the normal way.
+
+=head1 SEE ALSO
+
+L<nbdkit(1)>,
+L<nbdkit-cache-filter(1)>,
+L<nbdkit-readahead-filter(1)>,
+L<nbdkit-vddk-plugin(1)>,
+L<nbdkit-filter(3)>,
+L<qemu-img(1)>.
+
+=head1 AUTHORS
+
+Martin Kletzander
+
+=head1 COPYRIGHT
+
+Copyright (C) 2019 Red Hat Inc.
diff --git a/tests/Makefile.am b/tests/Makefile.am
index 4148793c7297..a2bbaeac5d45 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -47,6 +47,7 @@ EXTRA_DIST = \
test-cache.sh \
test-cache-max-size.sh \
test-cache-on-read.sh \
+ test-cacheextents.sh \
test-captive.sh \
test-cow.sh \
test-cxx.sh \
@@ -365,7 +366,9 @@ test_oldstyle_LDADD = libtest.la $(LIBGUESTFS_LIBS)
endif HAVE_LIBGUESTFS
# Test export flags.
-TESTS += test-eflags.sh
+TESTS += \
+ test-cacheextents.sh \
+ test-eflags.sh
# common disk image shared with several tests
if HAVE_GUESTFISH
diff --git a/tests/test-cacheextents.sh b/tests/test-cacheextents.sh
new file mode 100755
index 000000000000..fd0bd1c1bc48
--- /dev/null
+++ b/tests/test-cacheextents.sh
@@ -0,0 +1,56 @@
+#!/usr/bin/env bash
+# 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.
+
+source ./functions.sh
+set -x
+set -e
+
+requires diff --version
+requires grep --version
+requires qemu-img --version
+
+nbdkit \
+ -U - \
+ --filter=cacheextents \
+ --filter=log \
+ --run 'qemu-img map --output=json $nbd' \
+ data data="0 @0x100000 1" size=2M \
+ logfile=test-cacheextents-access.log >test-cacheextents-actual.log
+
+
+nbdkit \
+ -U - \
+ --run 'qemu-img map --output=json $nbd' \
+ data data="0 @0x100000 1" size=2M >test-cacheextents-expected.log
+
+diff -q test-cacheextents-actual.log test-cacheextents-expected.log
+test $(grep -c "connection=1 Extents" test-cacheextents-access.log) -eq 1
--
2.21.0
5:58 p.m.
On 5/15/19 5:39 PM, Martin Kletzander wrote:
This filter caches the last result of the extents() call and offers a
nice
speed-up for clients that only support req_on=1 in combination with plugins like
vddk, which has no overhead for returning information for multiple extents in
one call, but that call is very time-consuming.
Quick test showed that on a fast connection and a sparsely allocated 16G disk
with a OS installed `qemu-img map` runs 16s instead of 33s (out of which it
takes 5s to the first extents request). For 100G disk with no data on it, that
is one hole extent spanning the whole disk (where there is no space for
improvement) this does not add any noticeable overhead.
Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com>
---
v2:
- Errors are set in _add and _fill functions
- Return values are checked for only -1
- Added a test
- Indentation fixed
- Fixed access before lock
+
+/* This lock protects the global state. */
+static pthread_mutex_t lock = PTHREAD_MUTEX_INITIALIZER;
+
+/* The real size of the underlying plugin. */
+static uint64_t size;
+
We recently just fixed the truncate filter to NOT cache this globally
(as doing so is too risky); instead, it needs to be cached
per-connection. See commit b3a43ccd.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
11:47 p.m.
On Wed, May 15, 2019 at 05:58:27PM -0500, Eric Blake wrote:
On 5/15/19 5:39 PM, Martin Kletzander wrote:
> This filter caches the last result of the extents() call and offers a nice
> speed-up for clients that only support req_on=1 in combination with plugins like
> vddk, which has no overhead for returning information for multiple extents in
> one call, but that call is very time-consuming.
>
> Quick test showed that on a fast connection and a sparsely allocated 16G disk
> with a OS installed `qemu-img map` runs 16s instead of 33s (out of which it
> takes 5s to the first extents request). For 100G disk with no data on it, that
> is one hole extent spanning the whole disk (where there is no space for
> improvement) this does not add any noticeable overhead.
>
> Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com>
> ---
> v2:
> - Errors are set in _add and _fill functions
> - Return values are checked for only -1
> - Added a test
> - Indentation fixed
> - Fixed access before lock
>
> +
> +/* This lock protects the global state. */
> +static pthread_mutex_t lock = PTHREAD_MUTEX_INITIALIZER;
> +
> +/* The real size of the underlying plugin. */
> +static uint64_t size;
> +
We recently just fixed the truncate filter to NOT cache this globally
(as doing so is too risky); instead, it needs to be cached
per-connection. See commit b3a43ccd.
Oh, this should not be here, it is not used at all, just a leftover from
previous version where I cached it, but then figured I'm better off not using
it.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
2:42 a.m.
Apply the attached patch on top of yours which contains miscellaneous
fixes. If you use ‘./configure --enable-gcc-warnings’ then it will
enable GCC warnings.
The test doesn't really test anything. If extents are being cached
then I have two ideas about how you could see that: Either (1) you
could look at debug messages, the second extents call shouldn't be
processed at the plugin layer. Or (2) write a small nbdkit-sh-plugin
script which should see the first extent request and not the second
one (and even test that a subsequent write should kill the cache so
another extent request is seen by the plugin).
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/
5:51 a.m.
On Thu, May 16, 2019 at 08:42:43AM +0100, Richard W.M. Jones wrote:
Apply the attached patch on top of yours which contains miscellaneous
fixes. If you use ‘./configure --enable-gcc-warnings’ then it will
enable GCC warnings.
Thanks. Again, too much used to libvirt codebase where those are being enabled
by default for builds from git.
The test doesn't really test anything. If extents are being
cached
then I have two ideas about how you could see that: Either (1) you
could look at debug messages, the second extents call shouldn't be
processed at the plugin layer. Or (2) write a small nbdkit-sh-plugin
script which should see the first extent request and not the second
one (and even test that a subsequent write should kill the cache so
another extent request is seen by the plugin).
Good point with the write. The test checks that there is only one request to
the plugin for extents before the log filter is applied after the cacheextents
filter. If I remove the cacheextents one, the log will contain multiple
requests for extents for which the grep will return 4 and the test will fail.
Anyway for the write the sh plugin will help, so I'll convert it to that.
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/
diff --git a/filters/cacheextents/cacheextents.c
b/filters/cacheextents/cacheextents.c
index 01c0328..6cd22a7 100644
--- a/filters/cacheextents/cacheextents.c
+++ b/filters/cacheextents/cacheextents.c
@@ -50,9 +50,6 @@
/* This lock protects the global state. */
static pthread_mutex_t lock = PTHREAD_MUTEX_INITIALIZER;
-/* The real size of the underlying plugin. */
-static uint64_t size;
-
/* Cached extents from the last extents () call and its start and end for the
sake of simplicity. */
struct nbdkit_extents *cache_extents;
@@ -131,9 +128,8 @@ cacheextents_extents (struct nbdkit_next_ops *next_ops, void *nxdata,
}
nbdkit_debug ("cacheextents: cache miss");
- int r = next_ops->extents (nxdata, count, offset, flags, extents, err);
- if (r == -1)
- return r;
+ if (next_ops->extents (nxdata, count, offset, flags, extents, err) == -1)
+ return -1;
return cacheextents_fill (extents, err);
}
diff --git a/tests/Makefile.am b/tests/Makefile.am
index a2bbaea..aaf7450 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -366,9 +366,7 @@ test_oldstyle_LDADD = libtest.la $(LIBGUESTFS_LIBS)
endif HAVE_LIBGUESTFS
# Test export flags.
-TESTS += \
- test-cacheextents.sh \
- test-eflags.sh
+TESTS += test-eflags.sh
# common disk image shared with several tests
if HAVE_GUESTFISH
@@ -868,6 +866,9 @@ TESTS += \
endif HAVE_GUESTFISH
TESTS += test-cache-max-size.sh
+# cacheextents filter test.
+TESTS += test-cacheextents.sh
+
# cow filter test.
if HAVE_GUESTFISH
TESTS += test-cow.sh
diff --git a/tests/test-cacheextents.sh b/tests/test-cacheextents.sh
index fd0bd1c..b9145ed 100755
--- a/tests/test-cacheextents.sh
+++ b/tests/test-cacheextents.sh
@@ -38,6 +38,10 @@ requires diff --version
requires grep --version
requires qemu-img --version
+files="test-cacheextents-actual.log test-cacheextents-expected.log"
+rm -f $files
+cleanup_fn rm -f $files
+
nbdkit \
-U - \
--filter=cacheextents \
7:50 a.m.
On 5/16/19 5:51 AM, Martin Kletzander wrote:
On Thu, May 16, 2019 at 08:42:43AM +0100, Richard W.M. Jones wrote:
>
> Apply the attached patch on top of yours which contains miscellaneous
> fixes. If you use ‘./configure --enable-gcc-warnings’ then it will
> enable GCC warnings.
>
Thanks. Again, too much used to libvirt codebase where those are being
enabled
by default for builds from git.
Probably worth copying that trick over then, as it is useful :)
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
8:05 a.m.
On 5/15/19 5:39 PM, Martin Kletzander wrote:
This filter caches the last result of the extents() call and offers a
nice
speed-up for clients that only support req_on=1 in combination with plugins like
s/on/one/
vddk, which has no overhead for returning information for multiple
extents in
one call, but that call is very time-consuming.
Quick test showed that on a fast connection and a sparsely allocated 16G disk
with a OS installed `qemu-img map` runs 16s instead of 33s (out of which it
takes 5s to the first extents request). For 100G disk with no data on it, that
is one hole extent spanning the whole disk (where there is no space for
improvement) this does not add any noticeable overhead.
Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com>
---
+++ b/filters/cacheextents/cacheextents.c
+
+static int
+cacheextents_extents (struct nbdkit_next_ops *next_ops, void *nxdata,
+ void *handle, uint32_t count, uint64_t offset, uint32_t flags,
+ struct nbdkit_extents *extents,
+ int *err)
+{
+ ACQUIRE_LOCK_FOR_CURRENT_SCOPE (&lock);
+
+ nbdkit_debug ("cacheextents:"
+ " cache_start=%" PRIu64
+ " cache_end=%" PRIu64
+ " cache_extents=%p",
+ cache_start, cache_end, cache_extents);
+
+ if (cache_extents &&
+ offset >= cache_start && offset < cache_end) {
+ nbdkit_debug ("cacheextents: returning from cache");
+ return cacheextents_add (extents, err);
+ }
+
+ nbdkit_debug ("cacheextents: cache miss");
+ int r = next_ops->extents (nxdata, count, offset, flags, extents, err);
This is a bit pessimistic. Observe:
request A (offset=0, count=1G) populates extents (0-.5G data, .5G-1.5G
hole)
request B (offset=.5G, count=1G) serviced from the cache
compared to:
request A (offset=0, count=1G) populates extents (0-.5G data, .5G-1.0G
hole)
request B (offset=.5G, count=1G) treated as cache miss
It should be possible to note that request B overlaps with the cache,
and to therefore do a three-step action: fill 'extents' with the tail of
the cached extents (filling .5-1.0G), call next_ops->extents for the
remainder (1.0G-wherever), extend the cache to fill out the additional
information learned.
+ if (r == -1)
+ return r;
+
+ return cacheextents_fill (extents, err);
+}
+
+++ b/filters/cacheextents/nbdkit-cacheextents-filter.pod
@@ -0,0 +1,47 @@
+=head1 NAME
+
+nbdkit-cacheextents-filter - cache extents
+=head1 SEE ALSO
+
+L<nbdkit(1)>,
+L<nbdkit-cache-filter(1)>,
+L<nbdkit-readahead-filter(1)>,
+L<nbdkit-vddk-plugin(1)>,
+L<nbdkit-filter(3)>,
+L<qemu-img(1)>.
+
I'd also patch filters/nbdkit-cache-filter.pod to cross-link to the
cacheextents filter. And nbdkit-filter.pod probably needs an update to
point to all filters (hmm, I missed that in my recent proposed patch to
add a nocache filter; that filter could also do with a link to
cacheextents, as the two are orthogonal and can both be used at once)
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
8:23 a.m.
On Thu, May 16, 2019 at 08:05:18AM -0500, Eric Blake wrote:
On 5/15/19 5:39 PM, Martin Kletzander wrote:
> This filter caches the last result of the extents() call and offers a nice
> speed-up for clients that only support req_on=1 in combination with plugins like
s/on/one/
> vddk, which has no overhead for returning information for multiple extents in
> one call, but that call is very time-consuming.
>
> Quick test showed that on a fast connection and a sparsely allocated 16G disk
> with a OS installed `qemu-img map` runs 16s instead of 33s (out of which it
> takes 5s to the first extents request). For 100G disk with no data on it, that
> is one hole extent spanning the whole disk (where there is no space for
> improvement) this does not add any noticeable overhead.
>
> Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com>
> ---
> +++ b/filters/cacheextents/cacheextents.c
> +
> +static int
> +cacheextents_extents (struct nbdkit_next_ops *next_ops, void *nxdata,
> + void *handle, uint32_t count, uint64_t offset, uint32_t
flags,
> + struct nbdkit_extents *extents,
> + int *err)
> +{
> + ACQUIRE_LOCK_FOR_CURRENT_SCOPE (&lock);
> +
> + nbdkit_debug ("cacheextents:"
> + " cache_start=%" PRIu64
> + " cache_end=%" PRIu64
> + " cache_extents=%p",
> + cache_start, cache_end, cache_extents);
> +
> + if (cache_extents &&
> + offset >= cache_start && offset < cache_end) {
> + nbdkit_debug ("cacheextents: returning from cache");
> + return cacheextents_add (extents, err);
> + }
> +
> + nbdkit_debug ("cacheextents: cache miss");
> + int r = next_ops->extents (nxdata, count, offset, flags, extents, err);
This is a bit pessimistic. Observe:
request A (offset=0, count=1G) populates extents (0-.5G data, .5G-1.5G
hole)
request B (offset=.5G, count=1G) serviced from the cache
compared to:
request A (offset=0, count=1G) populates extents (0-.5G data, .5G-1.0G
hole)
request B (offset=.5G, count=1G) treated as cache miss
It should be possible to note that request B overlaps with the cache,
and to therefore do a three-step action: fill 'extents' with the tail of
the cached extents (filling .5-1.0G), call next_ops->extents for the
remainder (1.0G-wherever), extend the cache to fill out the additional
information learned.
I'm not quite sure what you mean. Are you suggesting keeping the previous
information and just updating it with new ones? If yes, then my response would
be yes, sure, I can do that, but...
1) I would have to change the way the cached data are stored
2) Make sure that when updating the data nothing is rewritten
3) When returning data from cache check for continuity
and when we're at that, we can also:
3) Make the responses to write-actions more granular instead of just throwing
away the cache.
Of course, none of the above is impossible or blocking, I just don't think it
would provide much of an added value, since most of the time the requests would
be sequential and if there is a quick response (or the client is not using
req_one), this would not speed things up. So this is quite a niche filter for
now.
> + if (r == -1)
> + return r;
> +
> + return cacheextents_fill (extents, err);
> +}
> +
> +++ b/filters/cacheextents/nbdkit-cacheextents-filter.pod
> @@ -0,0 +1,47 @@
> +=head1 NAME
> +
> +nbdkit-cacheextents-filter - cache extents
> +=head1 SEE ALSO
> +
> +L<nbdkit(1)>,
> +L<nbdkit-cache-filter(1)>,
> +L<nbdkit-readahead-filter(1)>,
> +L<nbdkit-vddk-plugin(1)>,
> +L<nbdkit-filter(3)>,
> +L<qemu-img(1)>.
> +
I'd also patch filters/nbdkit-cache-filter.pod to cross-link to the
cacheextents filter. And nbdkit-filter.pod probably needs an update to
point to all filters (hmm, I missed that in my recent proposed patch to
add a nocache filter; that filter could also do with a link to
cacheextents, as the two are orthogonal and can both be used at once)
Good point.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
9:08 a.m.
On 5/20/19 8:23 AM, Martin Kletzander wrote:
>> + if (cache_extents &&
>> + offset >= cache_start && offset < cache_end) {
>> + nbdkit_debug ("cacheextents: returning from cache");
>> + return cacheextents_add (extents, err);
>> + }
>> +
>> + nbdkit_debug ("cacheextents: cache miss");
>> + int r = next_ops->extents (nxdata, count, offset, flags, extents,
>> err);
>
> This is a bit pessimistic. Observe:
> request A (offset=0, count=1G) populates extents (0-.5G data, .5G-1.5G
> hole)
> request B (offset=.5G, count=1G) serviced from the cache
> compared to:
> request A (offset=0, count=1G) populates extents (0-.5G data, .5G-1.0G
> hole)
> request B (offset=.5G, count=1G) treated as cache miss
>
> It should be possible to note that request B overlaps with the cache,
> and to therefore do a three-step action: fill 'extents' with the tail of
> the cached extents (filling .5-1.0G), call next_ops->extents for the
> remainder (1.0G-wherever), extend the cache to fill out the additional
> information learned.
>
I'm not quite sure what you mean. Are you suggesting keeping the previous
information and just updating it with new ones? If yes, then my
response would
be yes, sure, I can do that, but...
Okay, re-reading the code, you aren't checking whether offset+count lies
within the cache, but are instead truncating your answer back to the
client to the amount that was previously cached, and relying on the
client's next request to likely start at the first byte that was not
cached. So at most, you end up splitting what could have been a
contiguous answer that crossed the cache_end boundary from one extent
reply into two. Still, I wonder if clearing the REQ_ONE flag before
calling into next_ops->extents(), or possibly even calling into
next_ops->extents() with count widened out to a larger value, to learn
as much information as possible on one pass, can result in fewer calls
into next_ops.
1) I would have to change the way the cached data are stored
No, you'd still store the cache in the same way. It is more a matter of
noting that what you have stored has a start and end boundary, and your
incoming request partially overlaps what you already have stored, so use
the cached values for the first half as well as limiting the call to the
actual plugin to the second half. Something like:
bool append = false;
if (cache_extents &&
offset >= cache_start && offset < cache_end) {
/* Partially or completely cached */
if (cacheextents_add (extents, err) == -1)
return -1;
if (offset + count <= cache_end)
return 0;
count -= cache_end - offset;
offset = cache_end;
append = true;
}
/* Miss or partially cached */
if (next_ops->extents (nxdata, count, offset, flags, extents, err) == -1)
return -1;
return cacheextents_fill (extents, append, err);
and maybe even consider passing in a larger count to next_ops->extents,
or even intentionally clearing the REQ_ONE flag before calling into
next_ops, if our goal is to get as much information as feasible from
next_ops for saving in our cache.
2) Make sure that when updating the data nothing is rewritten
3) When returning data from cache check for continuity
and when we're at that, we can also:
3) Make the responses to write-actions more granular instead of just
throwing
away the cache.
If the point of the cache is to speed up one-time read passes, we aren't
writing. I think invalidating the entire cache on writes is just fine,
where the focus is to make reads faster when there are no competing writes.
Of course, none of the above is impossible or blocking, I just don't
think it
would provide much of an added value, since most of the time the
requests would
be sequential and if there is a quick response (or the client is not using
req_one), this would not speed things up. So this is quite a niche
filter for
now.
So maybe it's not worth the effort of trying anything fancier; so much
as a question to at least let us think about it. I'm fine if we go with
your initial approach as the version we check in first.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
5:17 a.m.
On Mon, May 20, 2019 at 09:08:02AM -0500, Eric Blake wrote:
On 5/20/19 8:23 AM, Martin Kletzander wrote:
>>> + if (cache_extents &&
>>> + offset >= cache_start && offset < cache_end) {
>>> + nbdkit_debug ("cacheextents: returning from cache");
>>> + return cacheextents_add (extents, err);
>>> + }
>>> +
>>> + nbdkit_debug ("cacheextents: cache miss");
>>> + int r = next_ops->extents (nxdata, count, offset, flags, extents,
>>> err);
>>
>> This is a bit pessimistic. Observe:
>> request A (offset=0, count=1G) populates extents (0-.5G data, .5G-1.5G
>> hole)
>> request B (offset=.5G, count=1G) serviced from the cache
>> compared to:
>> request A (offset=0, count=1G) populates extents (0-.5G data, .5G-1.0G
>> hole)
>> request B (offset=.5G, count=1G) treated as cache miss
>>
>> It should be possible to note that request B overlaps with the cache,
>> and to therefore do a three-step action: fill 'extents' with the tail of
>> the cached extents (filling .5-1.0G), call next_ops->extents for the
>> remainder (1.0G-wherever), extend the cache to fill out the additional
>> information learned.
>>
>
> I'm not quite sure what you mean. Are you suggesting keeping the previous
> information and just updating it with new ones? If yes, then my
> response would
> be yes, sure, I can do that, but...
Okay, re-reading the code, you aren't checking whether offset+count lies
within the cache, but are instead truncating your answer back to the
client to the amount that was previously cached, and relying on the
client's next request to likely start at the first byte that was not
cached. So at most, you end up splitting what could have been a
contiguous answer that crossed the cache_end boundary from one extent
reply into two. Still, I wonder if clearing the REQ_ONE flag before
calling into next_ops->extents(), or possibly even calling into
next_ops->extents() with count widened out to a larger value, to learn
as much information as possible on one pass, can result in fewer calls
into next_ops.
>
> 1) I would have to change the way the cached data are stored
No, you'd still store the cache in the same way. It is more a matter of
noting that what you have stored has a start and end boundary, and your
incoming request partially overlaps what you already have stored, so use
the cached values for the first half as well as limiting the call to the
actual plugin to the second half. Something like:
bool append = false;
if (cache_extents &&
offset >= cache_start && offset < cache_end) {
/* Partially or completely cached */
if (cacheextents_add (extents, err) == -1)
return -1;
if (offset + count <= cache_end)
return 0;
count -= cache_end - offset;
offset = cache_end;
append = true;
}
/* Miss or partially cached */
if (next_ops->extents (nxdata, count, offset, flags, extents, err) == -1)
return -1;
return cacheextents_fill (extents, append, err);
and maybe even consider passing in a larger count to next_ops->extents,
or even intentionally clearing the REQ_ONE flag before calling into
next_ops, if our goal is to get as much information as feasible from
next_ops for saving in our cache.
Oh, now I understand. This makes sense if the client is not using req_one. IF
it is, then it will interpret the first extent, request the next one and even
though we have the whole extent in cache (probably), we do another request to
the plugin because offset+count > cache_end, even though it will not et
interpreted by the client.
Passing larger count and clearing the req_one makes sense as the plugins
themselves are supposed to also choose the most efficient way of returning the
information (e.g. (not)honouring req_one, not having to fill @count bytes).
I'll adjust that.
> 2) Make sure that when updating the data nothing is rewritten
> 3) When returning data from cache check for continuity
>
> and when we're at that, we can also:
>
> 3) Make the responses to write-actions more granular instead of just
> throwing
> away the cache.
If the point of the cache is to speed up one-time read passes, we aren't
writing. I think invalidating the entire cache on writes is just fine,
where the focus is to make reads faster when there are no competing writes.
>
> Of course, none of the above is impossible or blocking, I just don't
> think it
> would provide much of an added value, since most of the time the
> requests would
> be sequential and if there is a quick response (or the client is not using
> req_one), this would not speed things up. So this is quite a niche
> filter for
> now.
So maybe it's not worth the effort of trying anything fancier; so much
as a question to at least let us think about it. I'm fine if we go with
your initial approach as the version we check in first.
Thanks. I'm glad we reached a mutual understanding. There will always be room
for improvement, the question is if there is enough use cases for this filter.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
1993
days inactive
1999
days old
9 comments
3 participants
participants (3)
-
Eric Blake -
Martin Kletzander -
Richard W.M. Jones