Re: [Libguestfs] [nbdkit PATCH] protocol: Trivially implement NBD_CMD_FLAG_DF

Richard W.M. Jones
2019-03-29T12:43:33Z

On Thu, Mar 28, 2019 at 10:44:42PM -0500, Eric Blake
wrote:
> The DF flag is only available to clients that negotiated structured
> replies, and even then, the spec does not require that it be
> implemented. However, since our current implementation can't fragment
> NBD_CMD_READ replies, we trivially implement the flag (by ignoring
> it); we don't even have to pass it on to the plugins.
>
> Enhance some documentation about sparse reads while at it (when we
> finally do allow plugins to implement sparse reads, we'll have to pass
> on the DF flag, as well as perform reassembly back into one reply
> ourselves if the plugin ignored the flag).
>
> tests/test-eflags.sh can't really test this, as it requires the
> negotiation of structured replies (which in turn requires newstyle,
> not oldstyle...)
>
> Signed-off-by: Eric Blake <eblake@redhat.com>

Looks good to me, so ACK.

Thanks, Rich.

>  docs/nbdkit-protocol.pod    | 13 +++++++++++++
>  common/protocol/protocol.h  |  2 ++
>  server/protocol-handshake.c |  3 +++
>  server/protocol.c           | 16 +++++++++++++++-
>  TODO                        | 10 +++++++---
>  5 files changed, 40 insertions(+), 4 deletions(-)
>
> diff --git a/docs/nbdkit-protocol.pod b/docs/nbdkit-protocol.pod
> index a9a3390..f706cfd 100644
> --- a/docs/nbdkit-protocol.pod
> +++ b/docs/nbdkit-protocol.pod
> @@ -139,6 +139,19 @@ Supported in nbdkit E<ge> 1.11.10.
>  Only C<base:allocation> (ie. querying which parts of an image are
>  sparse) is supported.
>
> +Sparse reads (using C<NBD_REPLY_TYPE_OFFSET_HOLE> are not directly
> +supported, but a client can use block status to infer which portions
> +of the export do not need to be read.
> +
> +=item C<NBD_FLAG_DF>
> +
> +Supported in nbdkit E<ge> 1.11.11.
> +
> +This protocol extension allows a client to force an all-or-none read
> +when structured replies are in effect. However, the flag is a no-op
> +until we extend the plugin API to allow a fragmented read in the first
> +place.
> +
>  =item Resize Extension
>
>  I<Not supported>.
> diff --git a/common/protocol/protocol.h b/common/protocol/protocol.h
> index a7de2f0..4334be4 100644
> --- a/common/protocol/protocol.h
> +++ b/common/protocol/protocol.h
> @@ -94,6 +94,7 @@ extern const char *name_of_nbd_flag (int);
>  #define NBD_FLAG_ROTATIONAL        (1 << 4)
>  #define NBD_FLAG_SEND_TRIM         (1 << 5)
>  #define NBD_FLAG_SEND_WRITE_ZEROES (1 << 6)
> +#define NBD_FLAG_SEND_DF           (1 << 7)
>  #define NBD_FLAG_CAN_MULTI_CONN    (1 << 8)
>
>  /* NBD options (new style handshake only). */
> @@ -217,6 +218,7 @@ extern const char *name_of_nbd_cmd (int);
>  extern const char *name_of_nbd_cmd_flag (int);
>  #define NBD_CMD_FLAG_FUA      (1<<0)
>  #define NBD_CMD_FLAG_NO_HOLE  (1<<1)
> +#define NBD_CMD_FLAG_DF       (1<<2)
>  #define NBD_CMD_FLAG_REQ_ONE  (1<<3)
>
>  /* Error codes (previously errno).
> diff --git a/server/protocol-handshake.c b/server/protocol-handshake.c
> index 9653210..f0e5a2c 100644
> --- a/server/protocol-handshake.c
> +++ b/server/protocol-handshake.c
> @@ -121,6 +121,9 @@ protocol_compute_eflags (struct connection *conn,
uint16_t *flags)
>    if (fl)
>      conn->can_extents = true;
>
> +  if (conn->structured_replies)
> +    eflags |= NBD_FLAG_SEND_DF;
> +
>    *flags = eflags;
>    return 0;
>  }
> diff --git a/server/protocol.c b/server/protocol.c
> index 383938f..d94cd19 100644
> --- a/server/protocol.c
> +++ b/server/protocol.c
> @@ -110,7 +110,7 @@ validate_request (struct connection *conn,
>
>    /* Validate flags */
>    if (flags & ~(NBD_CMD_FLAG_FUA | NBD_CMD_FLAG_NO_HOLE |
> -                NBD_CMD_FLAG_REQ_ONE)) {
> +                NBD_CMD_FLAG_DF | NBD_CMD_FLAG_REQ_ONE)) {
>      nbdkit_error ("invalid request: unknown flag (0x%x)",
flags);
>      *error = EINVAL;
>      return false;
> @@ -121,6 +121,20 @@ validate_request (struct connection *conn,
>      *error = EINVAL;
>      return false;
>    }
> +  if (flags & NBD_CMD_FLAG_DF) {
> +    if (cmd != NBD_CMD_READ) {
> +      nbdkit_error ("invalid request: DF flag needs READ
request");
> +      *error = EINVAL;
> +      return false;
> +    }
> +    if (!conn->structured_replies) {
> +      nbdkit_error ("invalid request: "
> +                    "%s: structured replies was not negotiated",
> +                    name_of_nbd_cmd (cmd));
> +      *error = EINVAL;
> +      return false;
> +    }
> +  }
>    if ((flags & NBD_CMD_FLAG_REQ_ONE) &&
>        cmd != NBD_CMD_BLOCK_STATUS) {
>      nbdkit_error ("invalid request: REQ_ONE flag needs BLOCK_STATUS
request");
> diff --git a/TODO b/TODO
> index 81d692b..2b944e9 100644
> --- a/TODO
> +++ b/TODO
> @@ -24,8 +24,8 @@ General ideas for improvements
>    to inform nbdkit when the response is ready:
>    https://www.redhat.com/archives/libguestfs/2018-January/msg00149.html
>
> -* More NBD protocol features.  The only currently missing feature is
> -  online resize.
> +* More NBD protocol features.  The only currently missing features are
> +  sparse reads and online resize.
>
>  * Add a callback to let plugins request minimum alignment for the
>    buffer to pread/pwrite; useful for a plugin utilizing O_DIRECT or
> @@ -216,7 +216,11 @@ using ‘#define NBDKIT_API_VERSION <version>’.
>
>  * pread could be changed to allow it to support Structured Replies
>    (SRs).  This could mean allowing it to return partial data, holes,
> -  zeroes, etc.
> +  zeroes, etc.  For a client that negotiates SR coupled with a plugin
> +  that supports .extents, the v2 protocol would allow us to at least
> +  synthesize NBD_REPLY_TYPE_OFFSET_HOLE for less network traffic, even
> +  though the plugin will still have to fully populate the .pread
> +  buffer; the v3 protocol should make sparse reads more direct.
>
>  * Parameters should be systematized so that they aren't just (key,
>    value) strings.  nbdkit should know the possible keys for the plugin
> --
> 2.20.1
>
> _______________________________________________
> Libguestfs mailing list
> Libguestfs@redhat.com
> https://www.redhat.com/mailman/listinfo/libguestfs

--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-p2v converts physical machines to virtual machines.  Boot with a
live CD or over the network (PXE) and turn machines into KVM guests.