Libguestfs - Mar 2019 - Re: [nbdkit PATCH] protocol: Trivially implement NBD_CMD_FLAG_DF

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>
---
 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

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.
http://libguestfs.org/virt-v2v

On Fri, Mar 29, 2019 at 12:43:33PM +0000, Richard W.M. Jones
wrote:
> 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.
I have pushed this because I want to do another release containing the
bug fixes from the past few days.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-top is 'top' for virtual machines.  Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://people.redhat.com/~rjones/virt-top

On Fri, Mar 29, 2019 at 12:43:33PM +0000, Richard W.M. Jones
wrote:
>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.
>>
This resets the change from commit 26455d452574, probably unintentionally?
>>  * 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.
>http://libguestfs.org/virt-v2v
>
>_______________________________________________
>Libguestfs mailing list
>Libguestfs@redhat.com
>https://www.redhat.com/mailman/listinfo/libguestfs