Libguestfs - Jul 2019 - Re: [libnbd PATCH] api: Allow completion callbacks to auto-retire

When using the nbd_aio_FOO_callback commands, there is nothing further
to be learned about the command by calling nbd_aio_command_completed()
compared to what the callback already had access to.  There are still
scenarios where manually retiring the command after the fact is useful
(whether the return was 0 to keep the status unchanged, or -1 to alter
the retirement status to *error), but by allowing a return value of 1,
we can reduce the number of API calls required.  We can also
auto-retire the lone NBD_CMD_DISC request, reducing the amount of
special casing in nbd_aio_[peek_]command_completed.

Note that although this is not an API change, it does change ABI, but
that's okay as we have not declared a stable interface yet.

Update a couple of the tests and examples to give this coverage:
examples/glib-main-loop no longer piles up unretired commands, and
tests/server-death shows that even a command stranded by server death
can still be auto-retired.
---

This probably will conflict with Rich's work to improve callbacks to
make it easier to free data on the last use of a callback.

 examples/glib-main-loop.c |  6 +++--
 generator/generator       | 49 ++++++++++++++++++++++-----------------
 generator/states-reply.c  | 38 ++++++++++++++++++++----------
 generator/states.c        | 39 +++++++++++++++++++------------
 lib/aio.c                 | 15 ++++--------
 tests/server-death.c      | 30 +++++++++++++++++++++++-
 6 files changed, 116 insertions(+), 61 deletions(-)

diff --git a/examples/glib-main-loop.c b/examples/glib-main-loop.c
index c633c1d..2230077 100644
--- a/examples/glib-main-loop.c
+++ b/examples/glib-main-loop.c
@@ -188,6 +188,8 @@ finalize (GSource *sp)

   DEBUG (source, "finalize");

+  assert (nbd_aio_in_flight (source->nbd) == 0);
+  assert (nbd_aio_peek_command_completed (source->nbd) == -1);
   nbd_close (source->nbd);
 }

@@ -418,7 +420,7 @@ finished_read (void *vp, int64_t rcookie, int *error)
   /* Create a writer idle handler. */
   g_idle_add (write_data, NULL);

-  return 0;
+  return 1;
 }

 /* This idle callback schedules a write. */
@@ -507,5 +509,5 @@ finished_write (void *vp, int64_t wcookie, int *error)
     g_main_loop_quit (loop);
   }

-  return 0;
+  return 1;
 }
diff --git a/generator/generator b/generator/generator
index fdacd71..e0a2805 100755
--- a/generator/generator
+++ b/generator/generator
@@ -1738,9 +1738,9 @@ unique positive 64 bit cookie for this command, or
C<-1> on
 error.  If this command returns a cookie, then C<callback>
 will be called when the server is done replying,
 although you must still use C<nbd_aio_command_completed> after
-the callback to retire the command.  Note that you must ensure
-C<buf> is valid until the command has completed.  Other
-parameters behave as documented in C<nbd_pread>.
+the callback to retire the command unless the callback returns C<1>.
+Note that you must ensure C<buf> is valid until the command has
+completed.  Other parameters behave as documented in C<nbd_pread>.

 The C<callback> function is called with the same C<user_data>
passed to
 this function, C<cookie> set to the return value of this function,
@@ -1796,8 +1796,8 @@ unique positive 64 bit cookie for this command, or
C<-1> on
 error.  If this command returns a cookie, then C<callback>
 will be called when the server is done replying,
 although you must still use C<nbd_aio_command_completed> after
-the callback to retire the command.  Other parameters behave as
-documented in C<nbd_pread_structured>.
+the callback to retire the command unless the callback returns C<1>.
+Other parameters behave as documented in C<nbd_pread_structured>.

 The C<callback> function is called with the same C<user_data>
passed to
 this function, C<cookie> set to the return value of this function,
@@ -1906,8 +1906,8 @@ unique positive 64 bit cookie for this command, or
C<-1> on
 error.  If this command returns a cookie, then C<callback>
 will be called when the server is done replying,
 although you must still use C<nbd_aio_command_completed> after
-the callback to retire the command.  Other parameters behave as
-documented in C<nbd_flush>.
+the callback to retire the command unless the callback returns C<1>.
+Other parameters behave as documented in C<nbd_flush>.

 The C<callback> function is called with the same C<user_data>
passed to
 this function, C<cookie> set to the return value of this function,
@@ -1949,8 +1949,8 @@ unique positive 64 bit cookie for this command, or
C<-1> on
 error.  If this command returns a cookie, then C<callback>
 will be called when the server is done replying,
 although you must still use C<nbd_aio_command_completed> after
-the callback to retire the command.  Other parameters behave as
-documented in C<nbd_trim>.
+the callback to retire the command unless the callback returns C<1>.
+Other parameters behave as documented in C<nbd_trim>.

 The C<callback> function is called with the same C<user_data>
passed to
 this function, C<cookie> set to the return value of this function,
@@ -1992,8 +1992,8 @@ returns the unique positive 64 bit cookie for this
command, or
 C<-1> on error.  If this command returns a cookie, then C<callback>
 will be called when the server is done replying,
 although you must still use C<nbd_aio_command_completed> after
-the callback to retire the command.  Other parameters behave as
-documented in C<nbd_cache>.
+the callback to retire the command unless the callback returns C<1>.
+Other parameters behave as documented in C<nbd_cache>.

 The C<callback> function is called with the same C<user_data>
passed to
 this function, C<cookie> set to the return value of this function,
@@ -2035,8 +2035,8 @@ unique positive 64 bit cookie for this command, or
C<-1> on
 error.  If this command returns a cookie, then C<callback>
 will be called when the server is done replying,
 although you must still use C<nbd_aio_command_completed> after
-the callback to retire the command.  Other parameters behave as
-documented in C<nbd_zero>.
+the callback to retire the command unless the callback returns C<1>.
+Other parameters behave as documented in C<nbd_zero>.

 The C<callback> function is called with the same C<user_data>
passed to
 this function, C<cookie> set to the return value of this function,
@@ -2090,8 +2090,8 @@ unique positive 64 bit cookie for this command, or
C<-1> on
 error.  If this command returns a cookie, then C<callback>
 will be called when the server is done replying,
 although you must still use C<nbd_aio_command_completed> after
-the callback to retire the command.  Other parameters behave as
-documented in C<nbd_block_status>.
+the callback to retire the command unless the callback returns C<1>.
+Other parameters behave as documented in C<nbd_block_status>.

 The C<callback> function is called with the same C<user_data>
passed to
 this function, C<cookie> set to the return value of this function,
@@ -2267,7 +2267,10 @@ Return true if the command completed.  If this function
returns
 true then the command was successful and it has been retired.
 Return false if the command is still in flight.  This can also
 fail with an error in case the command failed (in this case
-the command is also retired).
+the command is also retired).  A command is retired either via
+this command, or by using a completion callback which returns
+C<1> (completion callbacks are registered via
+C<nbd_aio_pread_callback> and similar).

 The C<cookie> parameter is the positive unique 64 bit cookie
 for the command, as returned by a call such as C<nbd_aio_pread>.";
@@ -2279,10 +2282,11 @@ for the command, as returned by a call such as
C<nbd_aio_pread>.";
     shortdesc = "check if any command has completed";
     longdesc = "\
 Return the unique positive 64 bit cookie of the first non-retired but
-completed command, C<0> if no command is awaiting retirement, or
C<-1>
-on error. Any cookie returned by this function must still be passed to
-C<nbd_aio_command_completed> to actually retire the command and learn
-whether the command was successful.";
+completed command, C<0> if there are in-flight commands but none of
+them are awaiting retirement, or C<-1> on error including when there
+are no in-flight commands. Any cookie returned by this function must
+still be passed to C<nbd_aio_command_completed> to actually retire
+the command and learn whether the command was successful.";
   };

   "aio_in_flight", {
@@ -3613,7 +3617,10 @@ for how to get further details of the error.

  void nbd_close (struct nbd_handle *nbd);

-Closes the handle frees any associated resources.
+Closes the handle and frees any associated resources.  The final
+status of any command that has not been retired (whether by
+C<nbd_aio_command_completed> or by a low-level completion callback
+returning C<1>) is lost.

 =head1 GETTING THE LATEST ERROR MESSAGE IN THE THREAD

diff --git a/generator/states-reply.c b/generator/states-reply.c
index 1a0c149..6ea43d5 100644
--- a/generator/states-reply.c
+++ b/generator/states-reply.c
@@ -149,6 +149,7 @@ save_reply_state (struct nbd_handle *h)
  REPLY.FINISH_COMMAND:
   struct command *prev_cmd, *cmd;
   uint64_t cookie;
+  bool retire;

   /* NB: This works for both simple and structured replies because the
    * handle (our cookie) is stored at the same offset.
@@ -164,6 +165,23 @@ save_reply_state (struct nbd_handle *h)
   assert (cmd != NULL);
   assert (h->reply_cmd == cmd);
   h->reply_cmd = NULL;
+  retire = cmd->type == NBD_CMD_DISC;
+
+  /* Notify the user */
+  if (cmd->cb.callback) {
+    int error = cmd->error;
+
+    assert (cmd->type != NBD_CMD_DISC);
+    switch (cmd->cb.callback (cmd->cb.user_data, cookie, &error)) {
+    case -1:
+      if (error)
+        cmd->error = error;
+      break;
+    case 1:
+      retire = true;
+      break;
+    }
+  }

   /* Move it to the end of the cmds_done list. */
   if (prev_cmd != NULL)
@@ -171,23 +189,19 @@ save_reply_state (struct nbd_handle *h)
   else
     h->cmds_in_flight = cmd->next;
   cmd->next = NULL;
-  if (h->cmds_done_tail != NULL)
-    h->cmds_done_tail = h->cmds_done_tail->next = cmd;
+  if (retire)
+    free (cmd);
   else {
-    assert (h->cmds_done == NULL);
-    h->cmds_done = h->cmds_done_tail = cmd;
+    if (h->cmds_done_tail != NULL)
+      h->cmds_done_tail = h->cmds_done_tail->next = cmd;
+    else {
+      assert (h->cmds_done == NULL);
+      h->cmds_done = h->cmds_done_tail = cmd;
+    }
   }
   h->in_flight--;
   assert (h->in_flight >= 0);

-  /* Notify the user */
-  if (cmd->cb.callback) {
-    int error = cmd->error;
-
-    if (cmd->cb.callback (cmd->cb.user_data, cookie, &error) == -1
&& error)
-      cmd->error = error;
-  }
-
   SET_NEXT_STATE (%.READY);
   return 0;

diff --git a/generator/states.c b/generator/states.c
index 69aa431..2d7e197 100644
--- a/generator/states.c
+++ b/generator/states.c
@@ -115,31 +115,40 @@ send_from_wbuf (struct nbd_handle *h)
 void abort_commands (struct nbd_handle *h,
                      struct command **list)
 {
-  struct command *prev_cmd, *cmd;
+  struct command *next, *cmd;

-  for (cmd = *list, prev_cmd = NULL;
-       cmd != NULL;
-       prev_cmd = cmd, cmd = cmd->next) {
+  for (cmd = *list, *list = NULL; cmd != NULL; cmd = next) {
+    bool retire = cmd->type == NBD_CMD_DISC;
+
+    next = cmd->next;
     if (cmd->cb.callback) {
       int error = cmd->error ? cmd->error : ENOTCONN;

       assert (cmd->type != NBD_CMD_DISC);
-      if (cmd->cb.callback (cmd->cb.user_data, cmd->cookie,
-                            &error) == -1 && error)
-        cmd->error = error;
+      switch (cmd->cb.callback (cmd->cb.user_data, cmd->cookie,
&error)) {
+      case -1:
+        if (error)
+          cmd->error = error;
+        break;
+      case 1:
+        retire = true;
+        break;
+      }
     }
     if (cmd->error == 0)
       cmd->error = ENOTCONN;
-  }
-  if (prev_cmd) {
-    if (h->cmds_done_tail)
-      h->cmds_done_tail->next = *list;
+    if (retire)
+      free (cmd);
     else {
-      assert (h->cmds_done == NULL);
-      h->cmds_done = *list;
+      cmd->next = NULL;
+      if (h->cmds_done_tail)
+        h->cmds_done_tail->next = cmd;
+      else {
+        assert (h->cmds_done == NULL);
+        h->cmds_done = cmd;
+      }
+      h->cmds_done_tail = cmd;
     }
-    h->cmds_done_tail = prev_cmd;
-    *list = NULL;
   }
 }

diff --git a/lib/aio.c b/lib/aio.c
index 8d7cb8d..5a9841e 100644
--- a/lib/aio.c
+++ b/lib/aio.c
@@ -69,11 +69,12 @@ nbd_unlocked_aio_command_completed (struct nbd_handle *h,
     if (cmd->cookie == cookie)
       break;
   }
-  if (!cmd || cmd->type == NBD_CMD_DISC)
+  if (!cmd)
     return 0;

   type = cmd->type;
   error = cmd->error;
+  assert (cmd->type != NBD_CMD_DISC);
   /* The spec states that a 0-length read request is unspecified; but
    * it is easy enough to treat it as successful as an extension.
    */
@@ -105,16 +106,10 @@ nbd_unlocked_aio_command_completed (struct nbd_handle *h,
 int64_t
 nbd_unlocked_aio_peek_command_completed (struct nbd_handle *h)
 {
-  /* Special case NBD_CMD_DISC, as it does not have a user-visible cookie */
-  if (h->cmds_done && h->cmds_done->type == NBD_CMD_DISC) {
-    struct command *cmd = h->cmds_done;
-
-    h->cmds_done = cmd->next;
-    free (cmd);
-  }
-
-  if (h->cmds_done != NULL)
+  if (h->cmds_done != NULL) {
+    assert (h->cmds_done->type != NBD_CMD_DISC);
     return h->cmds_done->cookie;
+  }

   if (h->cmds_in_flight != NULL || h->cmds_to_issue != NULL) {
     set_error (0, "no in-flight command has completed yet");
diff --git a/tests/server-death.c b/tests/server-death.c
index 4093c20..08e5358 100644
--- a/tests/server-death.c
+++ b/tests/server-death.c
@@ -30,6 +30,21 @@

 #include <libnbd.h>

+static bool trim_retired;
+static const char *progname;
+
+static int
+callback (void *ignored, int64_t cookie, int *error)
+{
+  if (*error != ENOTCONN) {
+    fprintf (stderr, "%s: unexpected error in trim callback: %s\n",
+             progname, strerror (*error));
+    return 0;
+  }
+  trim_retired = 1;
+  return 1;
+}
+
 int
 main (int argc, char *argv[])
 {
@@ -47,6 +62,8 @@ main (int argc, char *argv[])
                         "--exit-with-parent",
"--filter=delay", "memory",
                         "size=1m", "delay-reads=5", NULL };

+  progname = argv[0];
+
   /* We're going to kill the child, but don't want to wait for a zombie
*/
   if (signal (SIGCHLD, SIG_IGN) == SIG_ERR) {
     fprintf (stderr, "%s: signal: %s\n", argv[0], strerror (errno));
@@ -80,11 +97,17 @@ main (int argc, char *argv[])
     goto fail;
   }

-  /* Issue a read that should not complete yet. */
+  /* Issue a read and trim that should not complete yet. Set up the
+   * trim to auto-retire via callback.
+   */
   if ((cookie = nbd_aio_pread (nbd, buf, sizeof buf, 0, 0)) == -1) {
     fprintf (stderr, "%s: test failed: nbd_aio_pread\n", argv[0]);
     goto fail;
   }
+  if (nbd_aio_trim_callback (nbd, sizeof buf, 0, callback, NULL, 0) == -1) {
+    fprintf (stderr, "%s: test failed: nbd_aio_trim_callback\n",
argv[0]);
+    goto fail;
+  }
   if (nbd_aio_peek_command_completed (nbd) != 0) {
     fprintf (stderr, "%s: test failed:
nbd_aio_peek_command_completed\n",
              argv[0]);
@@ -153,6 +176,11 @@ main (int argc, char *argv[])
   }

   /* With all commands retired, no further command should be pending */
+  if (!trim_retired) {
+    fprintf (stderr, "%s: test failed: nbd_aio_trim_callback not
retired\n",
+             argv[0]);
+    goto fail;
+  }
   if (nbd_aio_peek_command_completed (nbd) != -1) {
     fprintf (stderr, "%s: test failed:
nbd_aio_peek_command_completed\n",
              argv[0]);
-- 
2.20.1

On 7/23/19 3:30 PM, Eric Blake wrote:
> When using the nbd_aio_FOO_callback commands, there is nothing further
> to be learned about the command by calling nbd_aio_command_completed()
> compared to what the callback already had access to.  There are still
> scenarios where manually retiring the command after the fact is useful
> (whether the return was 0 to keep the status unchanged, or -1 to alter
> the retirement status to *error), but by allowing a return value of 1,
> we can reduce the number of API calls required.  We can also
> auto-retire the lone NBD_CMD_DISC request, reducing the amount of
> special casing in nbd_aio_[peek_]command_completed.
> 
> Note that although this is not an API change, it does change ABI, but
> that's okay as we have not declared a stable interface yet.
> 
> Update a couple of the tests and examples to give this coverage:
> examples/glib-main-loop no longer piles up unretired commands, and
> tests/server-death shows that even a command stranded by server death
> can still be auto-retired.
> ---
> 
> This probably will conflict with Rich's work to improve callbacks to
> make it easier to free data on the last use of a callback.
Here's the counterpart patch for nbdkit to utilize this; however, my
timing runs of libnbd/examples/threaded-reads-and-writes did not show
any noticeable change, so this particular optimization is in the noise
compared to any real bottlenecks remaining.  Still, the reduced lines of
code makes me like the idea:

diff --git i/plugins/nbd/nbd.c w/plugins/nbd/nbd.c
index 83a30583..ccd773ba 100644
--- i/plugins/nbd/nbd.c
+++ w/plugins/nbd/nbd.c
@@ -57,7 +57,6 @@

 /* The per-transaction details */
 struct transaction {
-  int64_t cookie;
   sem_t sem;
   uint32_t early_err;
   uint32_t err;
@@ -360,14 +359,12 @@ nbdplug_notify (void *opaque, int64_t cookie, int
*error)

   nbdkit_debug ("cookie %" PRId64 " completed state machine,
status %d",
                 cookie, *error);
-  assert (trans->cookie == 0 || trans->cookie == cookie);
-  trans->cookie = cookie;
   trans->err = *error;
   if (sem_post (&trans->sem)) {
     nbdkit_error ("failed to post semaphore: %m");
     abort ();
   }
-  return 0;
+  return 1;
 }

 /* Register a cookie and kick the I/O thread. */
@@ -386,8 +383,6 @@ nbdplug_register (struct handle *h, struct
transaction *trans, int64_t cookie)

   if (write (h->fds[1], &c, 1) != 1 && errno != EAGAIN)
     nbdkit_debug ("failed to kick reader thread: %m");
-  assert (trans->cookie == 0 || trans->cookie == cookie);
-  trans->cookie = cookie;
 }

 /* Perform the reply half of a transaction. */
@@ -405,13 +400,8 @@ nbdplug_reply (struct handle *h, struct transaction
*trans)
       nbdkit_debug ("failed to wait on semaphore: %m");
       err = EIO;
     }
-    else {
-      assert (trans->cookie > 0);
-      err = nbd_aio_command_completed (h->nbd, trans->cookie);
-      assert (err != 0);
-      assert (err == 1 ? trans->err == 0 : trans->err == nbd_get_errno
());
+    else
       err = trans->err;
-    }
   }
   if (sem_destroy (&trans->sem))
     abort ();

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

On Tue, Jul 23, 2019 at 03:30:16PM -0500, Eric Blake
wrote:
> diff --git a/generator/generator b/generator/generator
> index fdacd71..e0a2805 100755
> --- a/generator/generator
> +++ b/generator/generator
> @@ -1738,9 +1738,9 @@ unique positive 64 bit cookie for this command, or
C<-1> on
>  error.  If this command returns a cookie, then C<callback>
>  will be called when the server is done replying,
>  although you must still use C<nbd_aio_command_completed> after
> -the callback to retire the command.  Note that you must ensure
> -C<buf> is valid until the command has completed.  Other
> -parameters behave as documented in C<nbd_pread>.
> +the callback to retire the command unless the callback returns C<1>.
> +Note that you must ensure C<buf> is valid until the command has
> +completed.  Other parameters behave as documented in C<nbd_pread>.
> 
>  The C<callback> function is called with the same C<user_data>
passed to
>  this function, C<cookie> set to the return value of this function,
> @@ -1796,8 +1796,8 @@ unique positive 64 bit cookie for this command, or
C<-1> on
>  error.  If this command returns a cookie, then C<callback>
>  will be called when the server is done replying,
>  although you must still use C<nbd_aio_command_completed> after
> -the callback to retire the command.  Other parameters behave as
> -documented in C<nbd_pread_structured>.
> +the callback to retire the command unless the callback returns C<1>.
> +Other parameters behave as documented in C<nbd_pread_structured>.
> 
>  The C<callback> function is called with the same C<user_data>
passed to
>  this function, C<cookie> set to the return value of this function,
There's quite a lot of duplicated documentation here.  There are two
approaches to deduplicating it:

(1) Add a section in the main manual (docs/libnbd.pod):

  =head1 CALLBACKS

  ...

  =head2 Completion callbacks

  common text here

and link to it from the longdesc using "L<libnbd(3)/Completion
callbacks>"

(2) Put the common text in an OCaml string and interpolate it into the
longdesc in the generator:

  let completion_text = "\
  common text here
  ";

  ...

  "function", ...
  longdesc = "\
  normal description
  " ^ completion_text;


The rest of the patch is fine.

Rich.

-- 
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

The new server-death test fails when run under valgrind.  I guess it's
a race because valgrind will make the test run slightly more slowly.
The log is attached.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-p2v converts physical machines to virtual machines.  Boot with a
live CD or over the network (PXE) and turn machines into KVM guests.
http://libguestfs.org/virt-v2v

On 7/25/19 6:48 AM, Richard W.M. Jones wrote:
> 
> The new server-death test fails when run under valgrind.  I guess it's
> a race because valgrind will make the test run slightly more slowly.
> The log is attached.
D'oh - the test uses the nbdkit delay filter, but while it delays reads
to be longer than what is needed even for a slow test to kill nbdkit
first, I forgot to delay trims.  I'll push the obvious patch shortly.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org