[Libguestfs] [libnbd PATCH 4/6] states: Prepare for aio notify callback

[Eric Blake]

On 6/29/19 8:28 AM, Eric Blake wrote:
> Having the client polling thread perform an O(n) loop over all known
> in-flight commands after each time the poll woke up is somewhat
> inefficient, and in a multi-threaded setup requires additional locking
> beyond libnbd to track the set of known command handles.  Better is a
> way for aio commands to call a notify callback the moment a specific
> command is ready to complete, and then a separate thread can gather
> the final completion status using just libnbd's locking, making the
> polling loop more efficient.  This also provides an opportunity to
> clean up any opaque data and/or change the final command status (for
> example, writing a strict validator for nbd_aio_pread_structured can
> change the command from success to failure if the server violated
> protocol by not returning chunks to cover the entire read).
> 
> We also want the client to be aware of any issued/in-flight commands
> that failed because they were stranded when the state machine moved to
> CLOSED or DEAD.  Previously, nbd_aio_command_completed() would never
> locate such stranded commands, but adding a common point to fire the
> notifier for such commands makes it also possible to move those
> commands to the completion queue.
> 
> This patch sets up the framework, with observable effects for stranded
> commands per the testsuite changes, but nothing yet actually sets the
> notify callback; that will come in the next patch.
> ---

> +/* Forcefully fail any remaining in-flight commands in list */
> +void abort_commands (struct nbd_handle *h,
> +                     struct command_in_flight **list)
> +{
> +  struct command_in_flight *prev_cmd, *cmd;
> +
> +  for (cmd = *list, prev_cmd = NULL;
> +       cmd != NULL;
> +       prev_cmd = cmd, cmd = cmd->next) {
> +    if (cmd->cb.notify && cmd->type != NBD_CMD_DISC) {
> +      int error = cmd->error ? cmd->error : ENOTCONN;
> +
> +      if (cmd->cb.notify (cmd->cb.opaque, cmd->handle, &error) == -1 && error)
> +        cmd->error = error;
> +    }

Note that this special-cases NBD_CMD_DISC - since we did not return a
handle to the user, nor add an nbd_aio_disconnect_notify() variant, and
since the server (if compliant) does not reply to NBD_CMD_DISC, I
decided it did not make sense to call a notify callback for that command
(instead, you know the disconnect command completed when the state
machine moves to CLOSED or DEAD).

But my special-casing was incomplete; I'm squashing this in before pushing:

diff --git a/lib/aio.c b/lib/aio.c
index b29378b..748665e 100644
--- a/lib/aio.c
+++ b/lib/aio.c
@@ -69,7 +69,7 @@ nbd_unlocked_aio_command_completed (struct nbd_handle *h,
     if (cmd->handle == handle)
       break;
   }
-  if (!cmd)
+  if (!cmd || cmd->type == NBD_CMD_DISC)
     return 0;

   type = cmd->type;
@@ -103,6 +103,14 @@ 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
handle */
+  if (h->cmds_done && h->cmds_done->type == NBD_CMD_DISC) {
+    struct command_in_flight *cmd = h->cmds_done;
+
+    h->cmds_done = cmd->next;
+    free (cmd);
+  }
+
   if (h->cmds_done != NULL)
     return h->cmds_done->handle;

diff --git a/lib/disconnect.c b/lib/disconnect.c
index 53de386..5bbc64b 100644
--- a/lib/disconnect.c
+++ b/lib/disconnect.c
@@ -60,10 +60,11 @@ nbd_unlocked_aio_disconnect (struct nbd_handle *h,
uint32_t flags)
     return -1;
   h->disconnect_request = true;

-  /* This will leave the command on the in-flight list.  Is this a
-   * problem?  Probably it isn't.  If it is, we could add a flag to
-   * the command struct to tell SEND_REQUEST not to add it to the
-   * in-flight list.
+  /* As the server does not reply to this command, it is left
+   * in-flight until the cleanup performed when moving to CLOSED or
+   * DEAD.  We don't return a handle to the user, and thus also
+   * special case things so that the user cannot request the status of
+   * this command during aio_[peek_]command_completed.
    */
   return 0;
 }
diff --git a/tests/server-death.c b/tests/server-death.c
index f8747e4..18ca5f8 100644
--- a/tests/server-death.c
+++ b/tests/server-death.c
@@ -145,6 +145,27 @@ main (int argc, char *argv[])
     goto fail;
   }

+  /* With all commands retired, no further command should be pending */
+  if (nbd_aio_in_flight (nbd) != 0) {
+    fprintf (stderr, "%s: test failed: nbd_aio_in_flight\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]);
+    goto fail;
+  }
+  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));
+    goto fail;
+  }
+
   close (fd);
   unlink (pidfile);
   nbd_close (nbd);
-- 
2.20.1



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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libguestfs/attachments/20190702/285d9e4a/attachment.sig>