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

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

On 4/1/19 10:09 AM, Martin Kletzander wrote:
>>> +++ 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?
The earlier commit had:

-* More NBD protocol features. In the upstream pipeline: proposals for
-  block status and online resize.
+* More NBD protocol features.  The only currently missing feature is
+  online resize.

We implemented block status (which can indirectly be used to avoid the
need for sparse reads, for the same effect), but we did not actually
implement sparse reads (where we send FAR less network traffic for
blocks that are known to read as zeroes). So my change in wording was
intentional.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

On Mon, Apr 01, 2019 at 10:14:44AM -0500, Eric Blake
wrote:
>On 4/1/19 10:09 AM, Martin Kletzander wrote:
>
>>>> +++ 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?
>
>The earlier commit had:
>
>-* More NBD protocol features. In the upstream pipeline: proposals for
>-  block status and online resize.
>+* More NBD protocol features.  The only currently missing feature is
>+  online resize.
>
>We implemented block status (which can indirectly be used to avoid the
>need for sparse reads, for the same effect), but we did not actually
>implement sparse reads (where we send FAR less network traffic for
>blocks that are known to read as zeroes). So my change in wording was
>intentional.
>
Oh, thanks for the explanation, I did mistake sparse streams with block status.
>--
>Eric Blake, Principal Software Engineer
>Red Hat, Inc.           +1-919-301-3226
>Virtualization:  qemu.org | libvirt.org
>

On Mon, Apr 01, 2019 at 05:09:38PM +0200, Martin Kletzander
wrote:
> >>-* 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?
The missing protocol features are support for structured reads in the
pread method, and online resize.  I'll adjust the TODO file to be
clearer, but the thrust of it is correct and it's not an accidental
revert.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-df lists disk usage of guests without needing to install any
software inside the virtual machine.  Supports Linux and Windows.
http://people.redhat.com/~rjones/virt-df/