9:48 a.m.
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