[Libguestfs] [libnbd PATCH] api: Add LIBNBD_SHUTDOWN_IMMEDIATE flag
Richard W.M. Jones
rjones at redhat.com
Thu Sep 17 13:22:18 UTC 2020
On Fri, Sep 11, 2020 at 09:31:11AM -0500, Eric Blake wrote:
> 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.
I think for the reasons you've outlined here it's fine to
change this from cmd_flags to shutdown_flags. And the new
flag looks useful.
Patch looks good, so ACK.
Thanks,
Rich.
> 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 at redhat.com
> https://www.redhat.com/mailman/listinfo/libguestfs
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW
More information about the Libguestfs
mailing list