[Libguestfs] [nbdkit PATCH] protocol: Trivially implement NBD_CMD_FLAG_DF
Martin Kletzander
mkletzan at redhat.com
Mon Apr 1 15:09:38 UTC 2019
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 at 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 at 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 at redhat.com
>https://www.redhat.com/mailman/listinfo/libguestfs
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libguestfs/attachments/20190401/2202e36c/attachment.sig>
More information about the Libguestfs
mailing list