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

[Richard W.M. Jones]

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