Laszlo Ersek
2021-Oct-15 10:17 UTC
[Libguestfs] [PATCH nbdkit] New filter: request-request
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) >
Richard W.M. Jones
2021-Oct-15 10:52 UTC
[Libguestfs] [PATCH nbdkit] New filter: request-request
On Fri, Oct 15, 2021 at 12:17:05PM +0200, Laszlo Ersek wrote:> On 10/14/21 22:58, Richard W.M. Jones wrote: > > +/* 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.It turns out that emacs c-mode cannot format the macro correctly when I do that :-(> (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.Yes, that's true - will fix. Now I'm wondering why GCC didn't give me a warning - I need to look at that.> (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?It should be on the next line. Perils of writing a filter at 10pm at night ...> > + 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. :)nbdkit definitely abuses macros. I guess you've not seen https://gitlab.com/nbdkit/nbdkit/-/blob/master/common/utils/vector.h yet. Or for even more excitement, https://github.com/libguestfs/libguestfs-common/blob/master/utils/libxml2-writer-macros.h Lots of little callback functions would annoy me TBH. How about Eric's idea of having the START_RETRY / END_RETRY macros instead?> > +static struct nbdkit_filter filter = { > > Constify this? ... Ah, no, NBDKIT_REGISTER_FILTER requires this to be > modifiable. OK.Yeah we can't change that - it's API ... Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-builder quickly builds VMs from scratch http://libguestfs.org/virt-builder.1.html