Re: [Libguestfs] [libnbd PATCH] api: Add LIBNBD_SHUTDOWN_IMMEDIATE flag

As mentioned in commits 176fc4ea and 609c25f0, our original plan in adding a flags argument to nbd_shutdown was to let us specify different behaviors at the libnbd level, rather than NBD protocol flags (for that, the user has nbd_aio_disconnect). But when we later parameterized OFlags to accept various bitmasks (commit f891340b), we failed to mark nbd_shutdown as using a different bitmask than NBD_CMD_FLAG. I've finally found a use for such a flag. By itself, nbd_aio_disconnect merely queues itself at the back of all pending commands. But if the server is processing responses slowly, it can be desirable to elevate a disconnect request to the front of the queue, intentionally abandoning all subsequent commands that have not already started on their way to the server. Fortunately, we have been rejecting all flag values, so changing the type of the OFlags argument now has no impact to either the C API or the ABI. Other language bindings that are more strongly-typed will see a different enum, but we don't promise compatibility there, and even then, languages that permit omitting the flags argument in favor of a default don't see any difference for client that were omitting the argument in favor of the default.

Note that the new LIBNBD_SHUTDOWN_IMMEDIATE flag is not necessarily instant: if the server is still receiving the previous command, we have to wait for that before the server can learn of our intent, and the command itself still blocks until we enter closed or dead states. But it can certainly reduce the time spent in nbd_shutdown by not having to wait for all unsent commands in the queue to also be processed by the server. --- lib/internal.h | 2 + generator/API.ml | 28 ++++++- generator/states.c | 12 +-- lib/disconnect.c | 16 +++- tests/Makefile.am | 7 ++ tests/shutdown-flags.c | 166 +++++++++++++++++++++++++++++++++++++++++ .gitignore | 1 + 7 files changed, 221 insertions(+), 11 deletions(-) create mode 100644 tests/shutdown-flags.c diff --git a/lib/internal.h b/lib/internal.h index b2637bd..96699b5 100644 --- a/lib/internal.h +++ b/lib/internal.h @@ -435,6 +435,8 @@ extern int64_t nbd_internal_command_common (struct nbd_handle *h, struct socket *nbd_internal_socket_create (int fd); /* states.c */ +extern void nbd_internal_abort_commands (struct nbd_handle *h, + struct command **list); extern int nbd_internal_run (struct nbd_handle *h, enum external_event ev); extern const char *nbd_internal_state_short_string (enum state state); extern enum state_group nbd_internal_state_group (enum state state); diff --git a/generator/API.ml b/generator/API.ml index 1d920cf..6cdab34 100644 --- a/generator/API.ml +++ b/generator/API.ml @@ -181,7 +181,14 @@ let allow_transport_flags = { "VSOCK", 1 lsl 2; ] } -let all_flags = [ cmd_flags; handshake_flags; allow_transport_flags ] +let shutdown_flags = { + flag_prefix = "SHUTDOWN"; + flags = [ + "IMMEDIATE", 1 lsl 1; + ] +} +let all_flags = [ cmd_flags; handshake_flags; allow_transport_flags; + shutdown_flags] let default_call = { args = []; optargs = []; ret = RErr; shortdesc = ""; longdesc = ""; example = None; @@ -1595,7 +1602,7 @@ L<nbd_can_fua(3)>)."; "shutdown", { default_call with - args = []; optargs = [ OFlags ("flags", cmd_flags) ]; ret = RErr; + args = []; optargs = [ OFlags ("flags", shutdown_flags) ]; ret = RErr; permitted_states = [ Connected ]; shortdesc = "disconnect from the NBD server"; longdesc = "\ @@ -1608,8 +1615,21 @@ This function works whether or not the handle is ready for transmission of commands. If more fine-grained control is needed, see L<nbd_aio_disconnect(3)>. -The C<flags> parameter must be C<0> for now (it exists for future NBD -protocol extensions)."; +The C<flags> argument is a bitmask, including zero or more of the +following shutdown flags: + +=over 4 + +=item C<LIBNBD_SHUTDOWN_IMMEDIATE> = 1 + +If there are any pending requests which have not yet been sent to +the server (see L<nbd_aio_in_flight(3)>), abandon them without +sending them to the server, rather than the usual practice of +issuing those commands before informing the server of the intent +to disconnect. + +=back +"; see_also = [Link "close"; Link "aio_disconnect"]; example = Some "examples/reads-and-writes.c"; }; diff --git a/generator/states.c b/generator/states.c index 9a12e82..0ecba7e 100644 --- a/generator/states.c +++ b/generator/states.c @@ -122,8 +122,8 @@ abort_option (struct nbd_handle *h) } /* Forcefully fail any remaining in-flight commands in list */ -static void -abort_commands (struct nbd_handle *h, struct command **list) +void +nbd_internal_abort_commands (struct nbd_handle *h, struct command **list) { struct command *next, *cmd; @@ -179,8 +179,8 @@ STATE_MACHINE { /* The caller should have used set_error() before reaching here */ assert (nbd_get_error ()); abort_option (h); - abort_commands (h, &h->cmds_to_issue); - abort_commands (h, &h->cmds_in_flight); + nbd_internal_abort_commands (h, &h->cmds_to_issue); + nbd_internal_abort_commands (h, &h->cmds_in_flight); h->in_flight = 0; if (h->sock) { h->sock->ops->close (h->sock); @@ -190,8 +190,8 @@ STATE_MACHINE { CLOSED: abort_option (h); - abort_commands (h, &h->cmds_to_issue); - abort_commands (h, &h->cmds_in_flight); + nbd_internal_abort_commands (h, &h->cmds_to_issue); + nbd_internal_abort_commands (h, &h->cmds_in_flight); h->in_flight = 0; if (h->sock) { h->sock->ops->close (h->sock); diff --git a/lib/disconnect.c b/lib/disconnect.c index dcb95d8..b8356b7 100644 --- a/lib/disconnect.c +++ b/lib/disconnect.c @@ -23,17 +23,31 @@ #include <stdbool.h> #include <errno.h> #include <inttypes.h> +#include <assert.h> #include "internal.h" int nbd_unlocked_shutdown (struct nbd_handle *h, uint32_t flags) { - if (flags != 0) { + if ((flags & ~LIBNBD_SHUTDOWN_IMMEDIATE) != 0) { set_error (EINVAL, "invalid flag: %" PRIu32, flags); return -1; } + /* If IMMEDIATE, abort any commands that have not yet had any bytes + * sent to the server, so that NBD_CMD_DISC will be first in line. + */ + if (flags & LIBNBD_SHUTDOWN_IMMEDIATE) { + struct command **cmd = &h->cmds_to_issue; + if (!nbd_internal_is_state_ready (get_next_state (h))) { + assert (*cmd); + h->cmds_to_issue_tail = *cmd; + cmd = &((*cmd)->next); + } + nbd_internal_abort_commands (h, cmd); + } + if (!h->disconnect_request && (nbd_internal_is_state_ready (get_next_state (h)) || nbd_internal_is_state_processing (get_next_state (h)))) { diff --git a/tests/Makefile.am b/tests/Makefile.am index be7c83c..007c69e 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -142,6 +142,7 @@ if HAVE_NBDKIT check_PROGRAMS += \ errors \ server-death \ + shutdown-flags \ get-size \ read-only-flag \ read-write-flag \ @@ -180,6 +181,7 @@ check_PROGRAMS += \ TESTS += \ errors \ server-death \ + shutdown-flags \ get-size \ read-only-flag \ read-write-flag \ @@ -225,6 +227,11 @@ server_death_CPPFLAGS = -I$(top_srcdir)/include server_death_CFLAGS = $(WARNINGS_CFLAGS) server_death_LDADD = $(top_builddir)/lib/libnbd.la +shutdown_flags_SOURCES = shutdown-flags.c +shutdown_flags_CPPFLAGS = -I$(top_srcdir)/include +shutdown_flags_CFLAGS = $(WARNINGS_CFLAGS) +shutdown_flags_LDADD = $(top_builddir)/lib/libnbd.la + get_size_SOURCES = get-size.c get_size_CPPFLAGS = -I$(top_srcdir)/include get_size_CFLAGS = $(WARNINGS_CFLAGS) diff --git a/tests/shutdown-flags.c b/tests/shutdown-flags.c new file mode 100644 index 0000000..6043ee2 --- /dev/null +++ b/tests/shutdown-flags.c @@ -0,0 +1,166 @@ +/* NBD client library in userspace + * Copyright (C) 2013-2020 Red Hat Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, write to the Free Software + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA + */ + +/* Deliberately shutdown while stranding commands, to check their status. + */ + +#include <config.h> + +#include <stdio.h> +#include <stdlib.h> +#include <string.h> +#include <errno.h> +#include <signal.h> + +#include <libnbd.h> + +static bool write_retired; +static const char *progname; + +static int +callback (void *ignored, int *error) +{ + if (*error != ENOTCONN) { + fprintf (stderr, "%s: unexpected error in pwrite callback: %s\n", + progname, strerror (*error)); + return 0; + } + write_retired = 1; + return 1; +} + +static char buf[2 * 1024 * 1024]; + +int +main (int argc, char *argv[]) +{ + struct nbd_handle *nbd; + int err; + const char *msg; + int64_t cookie; + const char *cmd[] = { "nbdkit", "-s", "--exit-with-parent", + "memory", "size=2m", NULL }; + + progname = argv[0]; + + nbd = nbd_create (); + if (nbd == NULL) { + fprintf (stderr, "%s: %s\n", argv[0], nbd_get_error ()); + exit (EXIT_FAILURE); + } + + /* Connect to a server. */ + if (nbd_connect_command (nbd, (char **) cmd) == -1) { + fprintf (stderr, "%s: %s\n", argv[0], nbd_get_error ()); + exit (EXIT_FAILURE); + } + + /* Pause the server from reading, so that our first request will + * exceed the buffer and force the second request to be stuck client + * side (without stopping the server, we would be racing on whether + * we hit a block on writes based on whether the server can read + * faster than we can fill the pipe). + */ + if (nbd_kill_subprocess (nbd, SIGSTOP) == -1) { + fprintf (stderr, "%s: test failed: nbd_kill_subprocess: %s\n", argv[0], + nbd_get_error ()); + exit (EXIT_FAILURE); + } + + /* Issue back-to-back write requests, both large enough to block. Set up + * the second to auto-retire via callback. + */ + if ((cookie = nbd_aio_pwrite (nbd, buf, sizeof buf, 0, + NBD_NULL_COMPLETION, 0)) == -1) { + fprintf (stderr, "%s: test failed: first nbd_aio_pwrite: %s\n", argv[0], + nbd_get_error ()); + exit (EXIT_FAILURE); + } + if (nbd_aio_pwrite (nbd, buf, sizeof buf, 0, + (nbd_completion_callback) { .callback = callback }, + 0) == -1) { + fprintf (stderr, "%s: test failed: second nbd_aio_pwrite: %s\n", argv[0], + nbd_get_error ()); + exit (EXIT_FAILURE); + } + + /* Resume the server; but now our state machine remains blocked + * until we notify or otherwise poll it. + */ + if (nbd_kill_subprocess (nbd, SIGCONT) == -1) { + fprintf (stderr, "%s: test failed: nbd_kill_subprocess: %s\n", argv[0], + nbd_get_error ()); + exit (EXIT_FAILURE); + } + if (nbd_aio_peek_command_completed (nbd) != 0) { + fprintf (stderr, "%s: test failed: nbd_aio_peek_command_completed\n", + argv[0]); + exit (EXIT_FAILURE); + } + if (nbd_aio_command_completed (nbd, cookie) != 0) { + fprintf (stderr, "%s: test failed: nbd_aio_command_completed\n", argv[0]); + exit (EXIT_FAILURE); + } + + /* Send an immediate shutdown. This will abort the second write, as + * well as kick the state machine to finish the first. + */ + if (nbd_shutdown (nbd, LIBNBD_SHUTDOWN_IMMEDIATE) == -1) { + fprintf (stderr, "%s: test failed: nbd_shutdown\n", argv[0]); + exit (EXIT_FAILURE); + } + + /* All in-flight commands should now be completed. But whether the + * first write succeeded or failed depends on the server, so we + * merely retire it without checking status. + */ + if (nbd_aio_in_flight (nbd) != 0) { + fprintf (stderr, "%s: test failed: nbd_aio_in_flight\n", argv[0]); + exit (EXIT_FAILURE); + } + if (nbd_aio_peek_command_completed (nbd) != cookie) { + fprintf (stderr, "%s: test failed: nbd_aio_peek_command_completed\n", + argv[0]); + exit (EXIT_FAILURE); + } + nbd_aio_command_completed (nbd, cookie); + + /* With all commands retired, no further command should be pending */ + if (!write_retired) { + fprintf (stderr, "%s: test failed: second nbd_aio_pwrite not retired\n", + argv[0]); + exit (EXIT_FAILURE); + } + if (nbd_aio_peek_command_completed (nbd) != -1) { + fprintf (stderr, "%s: test failed: nbd_aio_peek_command_completed\n", + argv[0]); + exit (EXIT_FAILURE); + } + msg = nbd_get_error (); + err = nbd_get_errno (); + printf ("error: \"%s\"\n", msg); + printf ("errno: %d (%s)\n", err, strerror (err)); + if (err != EINVAL) { + fprintf (stderr, "%s: test failed: unexpected errno %d (%s)\n", argv[0], + err, strerror (err)); + exit (EXIT_FAILURE); + } + + nbd_close (nbd); + exit (EXIT_SUCCESS); +} diff --git a/.gitignore b/.gitignore index 6812fe8..aa9e0bb 100644 --- a/.gitignore +++ b/.gitignore @@ -179,6 +179,7 @@ Makefile.in /tests/read-only-flag /tests/read-write-flag /tests/server-death +/tests/shutdown-flags /tests/synch-parallel /tests/synch-parallel-tls /tests/version -- 2.28.0 _______________________________________________ Libguestfs mailing list Libguestfs(a)redhat.com https://www.redhat.com/mailman/listinfo/libguestfs