only mentioning things that I managed to think of in addition to Eric's
comments (which is of course not to say I thought of *everything* that
Eric pointed out):
On 10/14/21 22:58, Richard W.M. Jones wrote:
Similar to nbdkit-retry-filter, but this only retries single failing
requests rather than reopening the whole plugin. This is intended to
be used with the curl filter to solve:
https://bugzilla.redhat.com/show_bug.cgi?id=2013000
---
docs/nbdkit-captive.pod | 3 +-
.../nbdkit-retry-request-filter.pod | 67 +++++
filters/retry/nbdkit-retry-filter.pod | 7 +-
plugins/curl/nbdkit-curl-plugin.pod | 1 +
configure.ac | 2 +
filters/retry-request/Makefile.am | 68 ++++++
filters/retry-request/retry-request.c | 229 ++++++++++++++++++
7 files changed, 375 insertions(+), 2 deletions(-)
diff --git a/docs/nbdkit-captive.pod b/docs/nbdkit-captive.pod
index d340b8ae0..eafe36d8b 100644
--- a/docs/nbdkit-captive.pod
+++ b/docs/nbdkit-captive.pod
@@ -118,7 +118,8 @@ help performance:
--run 'qemu-img convert $nbd disk.img'
If the source suffers from temporary network failures
-L<nbdkit-retry-filter(1)> may help.
+L<nbdkit-retry-filter(1)> or L<nbdkit-retry-request-filter(1)> may
+help.
To overwrite a file inside an uncompressed tar file (the file being
overwritten must be the same size), use L<nbdkit-tar-filter(1)> like
diff --git a/filters/retry-request/nbdkit-retry-request-filter.pod
b/filters/retry-request/nbdkit-retry-request-filter.pod
new file mode 100644
index 000000000..1216cf83b
--- /dev/null
+++ b/filters/retry-request/nbdkit-retry-request-filter.pod
@@ -0,0 +1,67 @@
+=head1 NAME
+
+nbdkit-retry-request-filter - retry single requests on error
+
+=head1 SYNOPSIS
+
+ nbdkit --filter=retry-request PLUGIN [retry-request-retries=N]
+
+=head1 DESCRIPTION
+
+C<nbdkit-retry-request-filter> is a filter for nbdkit that
+transparently retries single requests if they fail. This is useful
+for plugins that are not completely reliable or have random behaviour.
+For example L<nbdkit-curl-plugin(1)> might behave this way if pointed
+at a load balancer which sometimes redirects to web server that are
+not responsive.
+
+An alternative filter with different trade-offs is
+L<nbdkit-retry-filter(1)>. That filter is more heavyweight because it
+always reopens the whole plugin connection on failure.
+
+=head1 PARAMETERS
+
+=over 4
+
+=item B<retry-request-retries=>N
+
+The number of times any single request will be retried before we give
+up and fail the operation. The default is 2.
+
+=item B<retry-request-delay=>N
+
+The number of seconds to wait before retrying. The default is 2
+seconds.
+
+=back
+
+=head1 FILES
+
+=over 4
+
+=item F<$filterdir/nbdkit-retry-request-filter.so>
+
+The filter.
+
+Use C<nbdkit --dump-config> to find the location of C<$filterdir>.
+
+=back
+
+=head1 VERSION
+
+C<nbdkit-retry-filter> first appeared in nbdkit 1.30.
+
+=head1 SEE ALSO
+
+L<nbdkit(1)>,
+L<nbdkit-filter(3)>,
+L<nbdkit-readahead-filter(1)>,
+L<nbdkit-retry-filter(1)>.
+
+=head1 AUTHORS
+
+Richard W.M. Jones
+
+=head1 COPYRIGHT
+
+Copyright (C) 2019-2021 Red Hat Inc.
diff --git a/filters/retry/nbdkit-retry-filter.pod
b/filters/retry/nbdkit-retry-filter.pod
index fee4b7e64..babd82bbd 100644
--- a/filters/retry/nbdkit-retry-filter.pod
+++ b/filters/retry/nbdkit-retry-filter.pod
@@ -16,6 +16,10 @@ make long-running copy operations reliable in the presence of
temporary network failures, without requiring any changes to the
plugin or the NBD client.
+An alternative and more fine-grained filter is
+L<nbdkit-retry-request-filter(1)> which will retry only single
+requests that fail.
+
Several optional parameters are available to control:
=over 4
@@ -113,7 +117,8 @@ C<nbdkit-retry-filter> first appeared in nbdkit 1.16.
L<nbdkit(1)>,
L<nbdkit-filter(3)>,
-L<nbdkit-readahead-filter(1)>.
+L<nbdkit-readahead-filter(1)>,
+L<nbdkit-retry-request-filter(1)>.
=head1 AUTHORS
diff --git a/plugins/curl/nbdkit-curl-plugin.pod b/plugins/curl/nbdkit-curl-plugin.pod
index c7acf6225..07f71b389 100644
--- a/plugins/curl/nbdkit-curl-plugin.pod
+++ b/plugins/curl/nbdkit-curl-plugin.pod
@@ -503,6 +503,7 @@ L<nbdkit-extentlist-filter(1)>,
L<nbdkit-file-plugin(1)>,
L<nbdkit-readahead-filter(1)>,
L<nbdkit-retry-filter(1)>,
+L<nbdkit-retry-request-filter(1)>,
L<nbdkit-ssh-plugin(1)>,
L<nbdkit-torrent-plugin(1)>,
L<nbdkit-plugin(3)>,
diff --git a/configure.ac b/configure.ac
index fd15e2960..3fd8a397e 100644
--- a/configure.ac
+++ b/configure.ac
@@ -139,6 +139,7 @@ filters="\
rate \
readahead \
retry \
+ retry-request \
stats \
swab \
tar \
@@ -1342,6 +1343,7 @@ AC_CONFIG_FILES([Makefile
filters/rate/Makefile
filters/readahead/Makefile
filters/retry/Makefile
+ filters/retry-request/Makefile
filters/stats/Makefile
filters/swab/Makefile
filters/tar/Makefile
diff --git a/filters/retry-request/Makefile.am b/filters/retry-request/Makefile.am
new file mode 100644
index 000000000..95fdafdcd
--- /dev/null
+++ b/filters/retry-request/Makefile.am
@@ -0,0 +1,68 @@
+# nbdkit
+# Copyright (C) 2019-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.
+
+include $(top_srcdir)/common-rules.mk
+
+EXTRA_DIST = nbdkit-retry-request-filter.pod
+
+filter_LTLIBRARIES = nbdkit-retry-request-filter.la
+
+nbdkit_retry_request_filter_la_SOURCES = \
+ retry-request.c \
+ $(top_srcdir)/include/nbdkit-filter.h \
+ $(NULL)
+
+nbdkit_retry_request_filter_la_CPPFLAGS = \
+ -I$(top_srcdir)/include \
+ -I$(top_srcdir)/common/include \
+ -I$(top_srcdir)/common/utils \
+ $(NULL)
+nbdkit_retry_request_filter_la_CFLAGS = $(WARNINGS_CFLAGS)
+nbdkit_retry_request_filter_la_LDFLAGS = \
+ -module -avoid-version -shared $(NO_UNDEFINED_ON_WINDOWS) \
+ -Wl,--version-script=$(top_srcdir)/filters/filters.syms \
+ $(NULL)
+nbdkit_retry_request_filter_la_LIBADD = \
+ $(top_builddir)/common/utils/libutils.la \
+ $(IMPORT_LIBRARY_ON_WINDOWS) \
+ $(NULL)
+
+if HAVE_POD
+
+man_MANS = nbdkit-retry-request-filter.1
+CLEANFILES += $(man_MANS)
+
+nbdkit-retry-request-filter.1: nbdkit-retry-request-filter.pod
+ $(PODWRAPPER) --section=1 --man $@ \
+ --html $(top_builddir)/html/$@.html \
+ $<
+
+endif HAVE_POD
diff --git a/filters/retry-request/retry-request.c
b/filters/retry-request/retry-request.c
new file mode 100644
index 000000000..d0a8b67bc
--- /dev/null
+++ b/filters/retry-request/retry-request.c
@@ -0,0 +1,229 @@
+/* nbdkit
+ * Copyright (C) 2019-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.
+ */
+
+#include <config.h>
+
+#include <stdio.h>
+#include <stdlib.h>
+#include <stdint.h>
+#include <stdbool.h>
+#include <inttypes.h>
+#include <string.h>
+#include <sys/time.h>
+
+#include <nbdkit-filter.h>
+
+#include "cleanup.h"
+#include "windows-compat.h"
+
+static unsigned retries = 2; /* 0 = filter is disabled */
+static unsigned delay = 2;
+
+static int
+retry_request_thread_model (void)
+{
+ return NBDKIT_THREAD_MODEL_PARALLEL;
+}
+
+static int
+retry_request_config (nbdkit_next_config *next, nbdkit_backend *nxdata,
+ const char *key, const char *value)
+{
+ if (strcmp (key, "retry-request-retries") == 0) {
+ if (nbdkit_parse_unsigned ("retry-request-retries", value, &retries)
== -1)
+ return -1;
+ return 0;
+ }
+ else if (strcmp (key, "retry-request-delay") == 0) {
+ if (nbdkit_parse_unsigned ("retry-request-delay", value, &delay) ==
-1)
+ return -1;
+ if (delay == 0) {
+ nbdkit_error ("retry-request-delay cannot be 0");
+ return -1;
+ }
+ return 0;
+ }
+
+ return next (nxdata, key, value);
+}
+
+#define retry_request_config_help \
+ "retry-request-retries=<N> Number of retries (default: 2).\n" \
+ "retry-request-delay=<N> Seconds to wait before retry (default:
2).\n"
+
+/* This macro encapsulates the logic of retrying. */
+#define RETRY(code) \
+ do { \
+ unsigned i; \
(1) Just for my own education: doesn't the nbdkit coding style dictate
that we should separate this declaration from the following code with an
empty line? For me, this is pretty hard to read.
(And yes, I strongly dislike C99-style intermixing of declarations and
code :)))
+ r = -1;
\
+ for (i = 0; r == -1 && i <= retries; ++i) { \
+ if (i > 0) { \
+ nbdkit_debug ("retry %d: waiting %d seconds before retrying", \
+ i, delay); \
(2) My inner pedant raised his hand and said: "i" and "delay" are
both
unsigned, so they should be printed with "%u", not "%d". Both of
these
variables can extend up to user-specified "unsigned" values (i.e., up to
UINT_MAX), so they can -- in theory at least -- exceed INT_MAX.
(3) A similarly impractical comment (but still): if "retries" (coming
from the user) is UINT_MAX, then (i <= UINT_MAX) is constant 1. Simplest
workaround is likely to put some arbitrary limits on the user specified
inputs in retry_request_config().
+ if (nbdkit_nanosleep (delay, 0) == -1) {
\
+ if (*err == 0) *err = errno; \
(4) I find this -- i.e., not putting the dependent assignment on a new
line -- borderline unreadable; is this frequent in nbdkit?
+ return -1;
\
+ } \
+ } \
+ code; \
[*]
+ }
\
+ } while (0)
+
+/* XXX This doesn't attempt to retry the initial open call. Should it?
+ * Argument against: You probably want the initial open to fail
+ * quickly. Argument for: curl's open call makes a request (to fetch
+ * the size) so retrying could be useful.
+ */
+
+static int
+retry_request_pread (nbdkit_next *next,
+ void *handle, void *buf, uint32_t count, uint64_t offset,
+ uint32_t flags, int *err)
+{
+ int r;
+
+ RETRY (r = next->pread (next, buf, count, offset, flags, err));
+ return r;
+}
+
+static int
+retry_request_pwrite (nbdkit_next *next,
+ void *handle,
+ const void *buf, uint32_t count, uint64_t offset,
+ uint32_t flags, int *err)
+{
+ int r;
+
+ RETRY (r = next->pwrite (next, buf, count, offset, flags, err));
+ return r;
+}
+
+static int
+retry_request_trim (nbdkit_next *next,
+ void *handle,
+ uint32_t count, uint64_t offset, uint32_t flags,
+ int *err)
+{
+ int r;
+
+ RETRY (r = next->trim (next, count, offset, flags, err));
+ return r;
+}
+
+static int
+retry_request_flush (nbdkit_next *next,
+ void *handle, uint32_t flags,
+ int *err)
+{
+ int r;
+
+ RETRY (r = next->flush (next, flags, err));
+ return r;
+}
+
+static int
+retry_request_zero (nbdkit_next *next,
+ void *handle,
+ uint32_t count, uint64_t offset, uint32_t flags,
+ int *err)
+{
+ int r;
+
+ RETRY (r = next->zero (next, count, offset, flags, err));
+ return r;
+}
+
+static int
+retry_request_extents (nbdkit_next *next,
+ void *handle,
+ uint32_t count, uint64_t offset, uint32_t flags,
+ struct nbdkit_extents *extents, int *err)
+{
+ CLEANUP_EXTENTS_FREE struct nbdkit_extents *extents2 = NULL;
+ int r;
+
+ RETRY (
+ /* Each retry must begin with extents reset to the right beginning. */
+ nbdkit_extents_free (extents2);
+ extents2 = nbdkit_extents_new (offset, next->get_size (next));
+ if (extents2 == NULL) {
+ *err = errno;
+ return -1; /* Not worth a retry after ENOMEM. */
+ }
+ r = next->extents (next, count, offset, flags, extents2, err);
+ );
(5) So this exemplifies why [*] rubs me the wrong way. This is just too
large & confusing as an argument to the RETRY() macro. In particular,
the replacement text of this macro invocation will contain *two*
semicolons (unlike the other invocations of the macro), because the
macro says "code;" and here we finish off with "r = next->extents
(...);".
I see this as a problem with the macro, really, and not with this call
site -- in my opinion, RETRY should not be a macro, but a function, and
we should pass it function pointers. Yes, I realize that's many more
boilerplate functions; I'd still find that better. (Likely easier to
step-through in gdb as well!)
For turning RETRY into a function, we might have to define a bunch of
"context" structures too (for the individual callbacks to take), so...
even more boilerplate. :)
+
+ if (r == 0) {
+ size_t i;
+
+ /* Transfer the successful extents back to the caller. */
+ for (i = 0; i < nbdkit_extents_count (extents2); ++i) {
+ struct nbdkit_extent e = nbdkit_get_extent (extents2, i);
+
+ if (nbdkit_add_extent (extents, e.offset, e.length, e.type) == -1) {
+ *err = errno;
+ return -1;
+ }
+ }
+ }
+
+ return r;
+}
+
+static int
+retry_request_cache (nbdkit_next *next,
+ void *handle,
+ uint32_t count, uint64_t offset, uint32_t flags,
+ int *err)
+{
+ int r;
+
+ RETRY (r = next->cache (next, count, offset, flags, err));
+ return r;
+}
+
+static struct nbdkit_filter filter = {
Constify this? ... Ah, no, NBDKIT_REGISTER_FILTER requires this to be
modifiable. OK.
Thanks!
Laszlo
+ .name = "retry-request",
+ .longname = "nbdkit retry request filter",
+ .thread_model = retry_request_thread_model,
+ .config = retry_request_config,
+ .config_help = retry_request_config_help,
+ .pread = retry_request_pread,
+ .pwrite = retry_request_pwrite,
+ .trim = retry_request_trim,
+ .flush = retry_request_flush,
+ .zero = retry_request_zero,
+ .extents = retry_request_extents,
+ .cache = retry_request_cache,
+};
+
+NBDKIT_REGISTER_FILTER(filter)