4:22 p.m.
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.
+++ 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