On Thu, Oct 14, 2021 at 09:58:40PM +0100, 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:

You'll want to fix the subject line: retry-request, not request-request.

https://bugzilla.redhat.com/show_bug.cgi?id=2013000 ---

+++ 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]

Should this mention retry-request-delay?

+ +=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

either 'servers ... are' or 'server ... is'

+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.

Do we want this to support both '1' (seconds implied) and '1ms' (milliseconds), similar to the delay filter, for finer granularity? Do we want any sort of exponential backoff knob?

+++ b/filters/retry-request/retry-request.c + +#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; \ + r = -1; \

Assumes 'r' is already declared and in scope, but matches use below.

+ for (i = 0; r == -1 && i <= retries; ++i) { \ + if (i > 0) { \ + nbdkit_debug ("retry %d: waiting %d seconds before retrying", \ + i, delay); \ + if (nbdkit_nanosleep (delay, 0) == -1) { \ + if (*err == 0) *err = errno; \ + return -1; \ + } \ + } \ + code; \

Assumes that 'code' expands to something that sets 'r'; worth documenting?

+ } \ + } 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. + */

Both good arguments. Maybe worth a separate knob?

+ +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; +} + ...

Fairly straightforward.

+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); + );

Wow - quite the large amount of code crammed in that macro call. All of the ',' in that code occur inside another set of (), so you do end up with only one argument to the macro proper. Would it read any cleaner if we had two macros, RETRY_START and RETRY_END, to write things like: RETRY_START /* Each retry ... */ nbdkit_extents_free (extents2); ... r = next->extents (...); RETRY_END

+ + 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; +}

Retrying extents is complicated, but your code looks correct. I know this is just an RFC, but the idea looks like it may have merit. Would we want to merge this functionality into the 'retry' filter (a single filter, with a knob on whether to retry single requests vs. full connection), or would that be too heavyweight for one filter? -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org

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)