2:41 p.m.
In https://bugzilla.redhat.com/show_bug.cgi?id=1841820, it was pointed
out that the retry filter not retrying .open means that an ssh
connection (such as in a vmx+ssh v2v conversion) fails when the ssh
connection itself cannot be retried. A year ago, this was an inherent
limitation of our retry implementation; but in the meantime, my work
to allow filters to open independent backends has made it feasible to
implement.
Eric Blake (2):
retry: Add in retry support during .open
retry: Test previous patch
tests/Makefile.am | 2 +
server/backend.c | 7 ++-
filters/retry/retry.c | 98 ++++++++++++++++++++++++----------------
tests/test-retry-open.sh | 85 ++++++++++++++++++++++++++++++++++
4 files changed, 150 insertions(+), 42 deletions(-)
create mode 100755 tests/test-retry-open.sh
--
2.39.1
2:41 p.m.
New subject: [nbdkit PATCH 1/2] retry: Add in retry support during .open
Now that a filter can open a backend as many times as it wants,
there's no longer a technical reason we can't retry .open. However,
adding retry logic here does mean we have to weaken an assert in the
server backend code, since prepare can now be reached more than once.
Test coverage will be added in a separate patch, so that it becomes
easy to swap patch order and see that the test fails without this
patch.
See also https://bugzilla.redhat.com/show_bug.cgi?id=1841820
---
server/backend.c | 7 +++-
filters/retry/retry.c | 98 +++++++++++++++++++++++++------------------
2 files changed, 63 insertions(+), 42 deletions(-)
diff --git a/server/backend.c b/server/backend.c
index 3bbba601..45889ea9 100644
--- a/server/backend.c
+++ b/server/backend.c
@@ -1,5 +1,5 @@
/* nbdkit
- * Copyright (C) 2013-2021 Red Hat Inc.
+ * Copyright (C) 2013-2023 Red Hat Inc.
*
* Redistribution and use in source and binary forms, with or without
* modification, are permitted provided that the following conditions are
@@ -312,7 +312,10 @@ backend_prepare (struct context *c)
struct backend *b = c->b;
assert (c->handle);
- assert ((c->state & (HANDLE_OPEN | HANDLE_CONNECTED)) == HANDLE_OPEN);
+ assert (c->state & HANDLE_OPEN);
+
+ if (c->state & HANDLE_CONNECTED)
+ return 0;
/* Call these in order starting from the filter closest to the
* plugin, similar to typical .open order. But remember that
diff --git a/filters/retry/retry.c b/filters/retry/retry.c
index fb1d0526..7abf74c4 100644
--- a/filters/retry/retry.c
+++ b/filters/retry/retry.c
@@ -1,5 +1,5 @@
/* nbdkit
- * Copyright (C) 2019-2021 Red Hat Inc.
+ * Copyright (C) 2019-2023 Red Hat Inc.
*
* Redistribution and use in source and binary forms, with or without
* modification, are permitted provided that the following conditions are
@@ -113,45 +113,6 @@ struct retry_handle {
bool open;
};
-static void *
-retry_open (nbdkit_next_open *next, nbdkit_context *nxdata,
- int readonly, const char *exportname, int is_tls)
-{
- struct retry_handle *h;
-
- if (next (nxdata, readonly, exportname) == -1)
- return NULL;
-
- h = malloc (sizeof *h);
- if (h == NULL) {
- nbdkit_error ("malloc: %m");
- return NULL;
- }
-
- h->readonly = readonly;
- h->exportname = strdup (exportname);
- h->context = nxdata;
- if (h->exportname == NULL) {
- nbdkit_error ("strdup: %m");
- free (h);
- return NULL;
- }
- h->reopens = 0;
- h->open = true;
-
- return h;
-}
-
-static void
-retry_close (void *handle)
-{
- struct retry_handle *h = handle;
-
- nbdkit_debug ("reopens needed: %u", h->reopens);
- free (h->exportname);
- free (h);
-}
-
/* This function encapsulates the common retry logic used across all
* data commands. If it returns true then the data command will retry
* the operation. ‘struct retry_data’ is stack data saved between
@@ -247,6 +208,63 @@ do_retry (struct retry_handle *h, struct retry_data *data,
return true;
}
+static void *
+retry_open (nbdkit_next_open *next, nbdkit_context *nxdata,
+ int readonly, const char *exportname, int is_tls)
+{
+ struct retry_handle *h;
+ struct retry_data data = {0};
+
+ h = malloc (sizeof *h);
+ if (h == NULL) {
+ nbdkit_error ("malloc: %m");
+ return NULL;
+ }
+
+ h->readonly = readonly;
+ h->exportname = strdup (exportname);
+ h->context = nxdata;
+ if (h->exportname == NULL) {
+ nbdkit_error ("strdup: %m");
+ free (h);
+ return NULL;
+ }
+ h->reopens = 0;
+
+ if (next (nxdata, readonly, exportname) != -1)
+ h->open = true;
+ else {
+ /* Careful - our .open must not return a handle unless do_retry()
+ * works, as the caller's next action will be calling .get_size
+ * and similar probe functions which we do not bother to wire up
+ * into retry logic because they only need to be used right after
+ * connecting.
+ */
+ nbdkit_next *next_handle = NULL;
+ int err = ESHUTDOWN;
+
+ while (! h->open && do_retry (h, &data, &next_handle,
"open", &err))
+ ;
+
+ if (! h->open) {
+ free (h->exportname);
+ free (h);
+ return NULL;
+ }
+ }
+ return h;
+}
+
+static void
+retry_close (void *handle)
+{
+ struct retry_handle *h = handle;
+
+ nbdkit_debug ("reopens needed: %u", h->reopens);
+ free (h->exportname);
+ free (h);
+}
+
static int
retry_pread (nbdkit_next *next,
void *handle, void *buf, uint32_t count, uint64_t offset,
--
2.39.1
2:58 p.m.
New subject: [nbdkit PATCH 1/2] retry: Add in retry support during .open
On Fri, Jan 27, 2023 at 02:41:22PM -0600, Eric Blake wrote:
Now that a filter can open a backend as many times as it wants,
there's no longer a technical reason we can't retry .open. However,
adding retry logic here does mean we have to weaken an assert in the
server backend code, since prepare can now be reached more than once.
Test coverage will be added in a separate patch, so that it becomes
easy to swap patch order and see that the test fails without this
patch.
See also https://bugzilla.redhat.com/show_bug.cgi?id=1841820
---
+static void *
+retry_open (nbdkit_next_open *next, nbdkit_context *nxdata,
+ int readonly, const char *exportname, int is_tls)
+{
+ struct retry_handle *h;
+ struct retry_data data = {0};
+
+ h = malloc (sizeof *h);
Uninit memory...
+ if (h == NULL) {
+ nbdkit_error ("malloc: %m");
+ return NULL;
+ }
+
+ h->readonly = readonly;
+ h->exportname = strdup (exportname);
+ h->context = nxdata;
+ if (h->exportname == NULL) {
+ nbdkit_error ("strdup: %m");
+ free (h);
+ return NULL;
+ }
+ h->reopens = 0;
+
+ if (next (nxdata, readonly, exportname) != -1)
+ h->open = true;
+ else {
and h->open has not been assigned if we get here...
+ /* Careful - our .open must not return a handle unless
do_retry()
+ * works, as the caller's next action will be calling .get_size
+ * and similar probe functions which we do not bother to wire up
+ * into retry logic because they only need to be used right after
+ * connecting.
+ */
+ nbdkit_next *next_handle = NULL;
+ int err = ESHUTDOWN;
+
+ while (! h->open && do_retry (h, &data, &next_handle,
"open", &err))
...even though I assumed it was zero-initialized here. Will fix.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
7:47 a.m.
New subject: [nbdkit PATCH 1/2] retry: Add in retry support during .open
On Fri, Jan 27, 2023 at 02:41:22PM -0600, Eric Blake wrote:
Now that a filter can open a backend as many times as it wants,
there's no longer a technical reason we can't retry .open. However,
adding retry logic here does mean we have to weaken an assert in the
server backend code, since prepare can now be reached more than once.
Test coverage will be added in a separate patch, so that it becomes
easy to swap patch order and see that the test fails without this
patch.
See also https://bugzilla.redhat.com/show_bug.cgi?id=1841820
---
server/backend.c | 7 +++-
filters/retry/retry.c | 98 +++++++++++++++++++++++++------------------
2 files changed, 63 insertions(+), 42 deletions(-)
diff --git a/server/backend.c b/server/backend.c
index 3bbba601..45889ea9 100644
--- a/server/backend.c
+++ b/server/backend.c
@@ -1,5 +1,5 @@
/* nbdkit
- * Copyright (C) 2013-2021 Red Hat Inc.
+ * Copyright (C) 2013-2023 Red Hat Inc.
*
* Redistribution and use in source and binary forms, with or without
* modification, are permitted provided that the following conditions are
@@ -312,7 +312,10 @@ backend_prepare (struct context *c)
struct backend *b = c->b;
assert (c->handle);
- assert ((c->state & (HANDLE_OPEN | HANDLE_CONNECTED)) == HANDLE_OPEN);
+ assert (c->state & HANDLE_OPEN);
+
+ if (c->state & HANDLE_CONNECTED)
+ return 0;
OK because CONNECTED means the handle has been opened & prepared already.
/* Call these in order starting from the filter closest to the
* plugin, similar to typical .open order. But remember that
diff --git a/filters/retry/retry.c b/filters/retry/retry.c
index fb1d0526..7abf74c4 100644
--- a/filters/retry/retry.c
+++ b/filters/retry/retry.c
@@ -1,5 +1,5 @@
/* nbdkit
- * Copyright (C) 2019-2021 Red Hat Inc.
+ * Copyright (C) 2019-2023 Red Hat Inc.
*
* Redistribution and use in source and binary forms, with or without
* modification, are permitted provided that the following conditions are
@@ -113,45 +113,6 @@ struct retry_handle {
bool open;
};
-static void *
-retry_open (nbdkit_next_open *next, nbdkit_context *nxdata,
- int readonly, const char *exportname, int is_tls)
-{
- struct retry_handle *h;
-
- if (next (nxdata, readonly, exportname) == -1)
- return NULL;
-
- h = malloc (sizeof *h);
- if (h == NULL) {
- nbdkit_error ("malloc: %m");
- return NULL;
- }
-
- h->readonly = readonly;
- h->exportname = strdup (exportname);
- h->context = nxdata;
- if (h->exportname == NULL) {
- nbdkit_error ("strdup: %m");
- free (h);
- return NULL;
- }
- h->reopens = 0;
- h->open = true;
-
- return h;
-}
-
-static void
-retry_close (void *handle)
-{
- struct retry_handle *h = handle;
-
- nbdkit_debug ("reopens needed: %u", h->reopens);
- free (h->exportname);
- free (h);
-}
-
/* This function encapsulates the common retry logic used across all
* data commands. If it returns true then the data command will retry
* the operation. ???struct retry_data??? is stack data saved between
@@ -247,6 +208,63 @@ do_retry (struct retry_handle *h, struct retry_data *data,
return true;
}
+static void *
+retry_open (nbdkit_next_open *next, nbdkit_context *nxdata,
+ int readonly, const char *exportname, int is_tls)
+{
+ struct retry_handle *h;
+ struct retry_data data = {0};
+
+ h = malloc (sizeof *h);
+ if (h == NULL) {
+ nbdkit_error ("malloc: %m");
+ return NULL;
+ }
+
+ h->readonly = readonly;
+ h->exportname = strdup (exportname);
+ h->context = nxdata;
+ if (h->exportname == NULL) {
+ nbdkit_error ("strdup: %m");
+ free (h);
+ return NULL;
+ }
+ h->reopens = 0;
+
+ if (next (nxdata, readonly, exportname) != -1)
+ h->open = true;
+ else {
+ /* Careful - our .open must not return a handle unless do_retry()
+ * works, as the caller's next action will be calling .get_size
+ * and similar probe functions which we do not bother to wire up
+ * into retry logic because they only need to be used right after
+ * connecting.
+ */
+ nbdkit_next *next_handle = NULL;
+ int err = ESHUTDOWN;
+
+ while (! h->open && do_retry (h, &data, &next_handle,
"open", &err))
+ ;
+
+ if (! h->open) {
+ free (h->exportname);
+ free (h);
+ return NULL;
+ }
+ }
+ return h;
+}
+
+static void
+retry_close (void *handle)
+{
+ struct retry_handle *h = handle;
+
+ nbdkit_debug ("reopens needed: %u", h->reopens);
+ free (h->exportname);
+ free (h);
+}
+
static int
retry_pread (nbdkit_next *next,
void *handle, void *buf, uint32_t count, uint64_t offset,
--
Seems OK, ACK (modulo your comment about leaking memory
in your reply).
I checked the documentation of the filter and it doesn't mention the
limitation, so it seems we don't need to adjust the documentation at
all.
I will try this out with the ssh filter once the bug moves into post.
Thanks,
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
9:23 a.m.
New subject: [nbdkit PATCH 1/2] retry: Add in retry support during .open
On Sat, Jan 28, 2023 at 01:47:32PM +0000, Richard W.M. Jones wrote:
On Fri, Jan 27, 2023 at 02:41:22PM -0600, Eric Blake wrote:
> Now that a filter can open a backend as many times as it wants,
> there's no longer a technical reason we can't retry .open. However,
> adding retry logic here does mean we have to weaken an assert in the
> server backend code, since prepare can now be reached more than once.
>
> Test coverage will be added in a separate patch, so that it becomes
> easy to swap patch order and see that the test fails without this
> patch.
>
> See also https://bugzilla.redhat.com/show_bug.cgi?id=1841820
> ---
Seems OK, ACK (modulo your comment about leaking memory
in your reply).
I checked the documentation of the filter and it doesn't mention the
limitation, so it seems we don't need to adjust the documentation at
all.
The memory bug is fixed, and this is now in as commits
42b198c9..4aabef5e
I will try this out with the ssh filter once the bug moves into post.
Ready for you to test.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
2:41 p.m.
New subject: [nbdkit PATCH 2/2] retry: Test previous patch
Separate commit to allow rearranging patches to demonstrate that open
can now be retried.
---
tests/Makefile.am | 2 +
tests/test-retry-open.sh | 85 ++++++++++++++++++++++++++++++++++++++++
2 files changed, 87 insertions(+)
create mode 100755 tests/test-retry-open.sh
diff --git a/tests/Makefile.am b/tests/Makefile.am
index e5396b5f..713ccca7 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -1807,6 +1807,7 @@ TESTS += \
test-retry-size.sh \
test-retry-reopen-fail.sh \
test-retry-zero-flags.sh \
+ test-retry-open.sh \
$(NULL)
EXTRA_DIST += \
test-retry.sh \
@@ -1815,6 +1816,7 @@ EXTRA_DIST += \
test-retry-size.sh \
test-retry-reopen-fail.sh \
test-retry-zero-flags.sh \
+ test-retry-open.sh \
$(NULL)
# retry-request filter test.
diff --git a/tests/test-retry-open.sh b/tests/test-retry-open.sh
new file mode 100755
index 00000000..468a03e1
--- /dev/null
+++ b/tests/test-retry-open.sh
@@ -0,0 +1,85 @@
+#!/usr/bin/env bash
+# nbdkit
+# Copyright (C) 2018-2023 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 -e
+set -x
+
+requires_plugin sh
+requires qemu-io --version
+
+files="retry-open-count"
+rm -f $files
+cleanup_fn rm -f $files
+
+echo 0 > retry-open-count
+start_t=$SECONDS
+
+# Create a custom plugin which will test retrying open.
+nbdkit -v -U - \
+ sh - \
+ --filter=retry retry-delay=1 \
+ --run 'qemu-io -f raw -c "r -P0 0 512" $nbd || :'
<<'EOF'
+#!/usr/bin/env bash
+case "$1" in
+ open)
+ # Count how many times the connection is (re-)opened.
+ read i < retry-open-count
+ echo $((i+1)) > retry-open-count
+ if test $i -lt 1; then
+ echo EIO >&2; exit 1
+ fi
+ ;;
+ get_size) echo 512 ;;
+ pread) dd if=/dev/zero count=$3 iflag=count_bytes ;;
+ *) exit 2 ;;
+esac
+EOF
+
+# In this test we should see 1 failure:
+# first open FAILS
+# retry and wait 1 second
+# second open PASSES
+
+# The minimum time for the test should be 1 second.
+end_t=$SECONDS
+if [ $((end_t - start_t)) -lt 1 ]; then
+ echo "$0: test ran too quickly"
+ exit 1
+fi
+
+# Check the handle was opened 2 times (first open + one reopen).
+read open_count < retry-open-count
+if [ $open_count -ne 2 ]; then
+ echo "$0: open-count ($open_count) != 2"
+ exit 1
+fi
--
2.39.1
475
days inactive
479
days old
5 comments
2 participants
participants (2)
-
Eric Blake -
Richard W.M. Jones