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
signature.asc
Description: OpenPGP digital signature
_______________________________________________ Libguestfs mailing list Libguestfs@redhat.com https://www.redhat.com/mailman/listinfo/libguestfs
