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

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

On Fri, May 27, 2022 at 05:25:43PM -0500, Eric Blake
wrote:
> 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
Reviewed-by: Richard W.M. Jones <rjones at redhat.com>

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