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

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(a)redhat.com&gt; 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(a)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(a)redhat.com https://www.redhat.com/mailman/listinfo/libguestfs