Re: [Libguestfs] [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