[Libguestfs] [libnbd PATCH] api: Speed up nbd_pread_structured when reading holes

[Eric Blake]

Commit c7fbc71d missed an optimization: when we guarantee that a pread
buffer is pre-zeroed, we don't need to waste time memset()ing it when
the server sends a hole.  But the next commit e0953cb7 lets the user
skip pre-zeroing, at which point the memset is required to avoid data
corruption.  Thankfully, we are not leaking bogus bytes when a server
sends a hole, but we CAN speed up the case where we have pre-zeroed
the buffer to avoid a second memset().  Because the user can change
the state of pread_initialize on the fly (including while a pread is
still in flight), we must track the state per-command as it was before
we send the request to the server.
---
 lib/internal.h                      |  1 +
 generator/states-reply-structured.c |  5 +++--
 lib/rw.c                            | 18 +++++++++++-------
 3 files changed, 15 insertions(+), 9 deletions(-)

diff --git a/lib/internal.h b/lib/internal.h
index 6aaced3..885cee1 100644
--- a/lib/internal.h
+++ b/lib/internal.h
@@ -353,6 +353,7 @@ struct command {
   struct command_cb cb;
   enum state state; /* State to resume with on next POLLIN */
   bool data_seen; /* For read, true if at least one data chunk seen */
+  bool initialized; /* For read, true if getting a hole may skip memset */
   uint32_t error; /* Local errno value */
 };

diff --git a/generator/states-reply-structured.c b/generator/states-reply-structured.c
index 7001047..12c24f5 100644
--- a/generator/states-reply-structured.c
+++ b/generator/states-reply-structured.c
@@ -1,5 +1,5 @@
 /* nbd client library in userspace: state machine
- * Copyright (C) 2013-2019 Red Hat Inc.
+ * Copyright (C) 2013-2022 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
@@ -436,7 +436,8 @@ STATE_MACHINE {
      * 0-length replies are broken. Still, it's easy enough to support
      * them as an extension, and this works even when length == 0.
      */
-    memset (cmd->data + offset, 0, length);
+    if (!cmd->initialized)
+      memset (cmd->data + offset, 0, length);
     if (CALLBACK_IS_NOT_NULL (cmd->cb.fn.chunk)) {
       int error = cmd->error;

diff --git a/lib/rw.c b/lib/rw.c
index c5d041d..f32481a 100644
--- a/lib/rw.c
+++ b/lib/rw.c
@@ -244,14 +244,18 @@ nbd_internal_command_common (struct nbd_handle *h,
   if (cb)
     cmd->cb = *cb;

-  /* For NBD_CMD_READ, cmd->data was pre-zeroed in the prologue
-   * created by the generator.  Thus, if a (non-compliant) server with
-   * structured replies fails to send back sufficient data to cover
-   * the whole buffer, we still behave as if it had sent zeroes for
-   * those portions, rather than leaking any uninitialized data, and
-   * without having to complicate our state machine to track which
-   * portions of the read buffer were actually populated.
+  /* For NBD_CMD_READ, cmd->data defaults to being pre-zeroed in the
+   * prologue created by the generator.  Thus, if a (non-compliant)
+   * server with structured replies fails to send back sufficient data
+   * to cover the whole buffer, we still behave as if it had sent
+   * zeroes for those portions, rather than leaking any uninitialized
+   * data, and without having to complicate our state machine to track
+   * which portions of the read buffer were actually populated.  But
+   * if the user opts in to disabling set_pread_initialize, then we
+   * need to memset zeroes as they are read (and the user gets their
+   * own garbage back in the case of a non-compliant server).
    */
+  cmd->initialized = h->pread_initialize;

   /* Add the command to the end of the queue. Kick the state machine
    * if there is no other command being processed, otherwise, it will
-- 
2.36.1