Eric Blake
2019-Mar-29 03:44 UTC
[Libguestfs] [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
Richard W.M. Jones
2019-Mar-29 12:43 UTC
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@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
Richard W.M. Jones
2019-Mar-29 13:49 UTC
Re: [Libguestfs] [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.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
Martin Kletzander
2019-Apr-01 15:09 UTC
Re: [Libguestfs] [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
Seemingly Similar Threads
- Re: [nbdkit PATCH] protocol: Trivially implement NBD_CMD_FLAG_DF
- Re: [nbdkit PATCH] protocol: Trivially implement NBD_CMD_FLAG_DF
- [nbdkit PATCH 1/3] server: Add internal support for NBDKIT_FLAG_FAST_ZERO
- [PATCH libnbd] lib: Copy nbd-protocol.h from nbdkit 1.15.3.
- [PATCH nbdkit 3/9] server: Implement Block Status requests to read allocation status.