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

Martin Kletzander
2019-04-01T15:09:38Z

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