Libguestfs - Mar 2023 - [PATCH v2 2/6] spec: Tweak description of maximum block size

Hi Eric,

Busy days, busy times. Sorry about the insane delays here.

On Mon, Nov 14, 2022 at 04:46:51PM -0600, Eric Blake
wrote:
> Commit 9f30fedb improved the spec to allow non-payload requests that
> exceed any advertised maximum block size.  Take this one step further
> by permitting the server to use NBD_EOVERFLOW as a hint to the client
> when a request is oversize (while permitting NBD_EINVAL for
> back-compat), and by rewording the text to explicitly call out that
> what is being advertised is the maximum payload length, not maximum
> block size.  This becomes more important when we add 64-bit
> extensions, where it becomes possible to extend `NBD_CMD_BLOCK_STATUS`
> to have both an effect length (how much of the image does the client
> want status on - may be larger than 32 bits) and an optional payload
> length (a way to filter the response to a subset of negotiated
> metadata contexts).  In the shorter term, it means that a server may
> (but not must) accept a read request larger than the maximum block
> size if it can use structured replies to keep each chunk of the
> response under the maximum payload limits.
> ---
>  doc/proto.md | 127 +++++++++++++++++++++++++++++----------------------
>  1 file changed, 73 insertions(+), 54 deletions(-)
> 
> diff --git a/doc/proto.md b/doc/proto.md
> index 8f08583..53c334a 100644
> --- a/doc/proto.md
> +++ b/doc/proto.md
> @@ -745,8 +745,8 @@ text unless the client insists on TLS.
> 
>  During transmission phase, several operations are constrained by the
>  export size sent by the final `NBD_OPT_EXPORT_NAME` or `NBD_OPT_GO`,
> -as well as by three block size constraints defined here (minimum,
> -preferred, and maximum).
> +as well as by three block size constraints defined here (minimum
> +block, preferred block, and maximum payload).
> 
>  If a client can honour server block size constraints (as set out below
>  and under `NBD_INFO_BLOCK_SIZE`), it SHOULD announce this during the
> @@ -772,15 +772,15 @@ learn the server's constraints without committing
to them.
> 
>  If block size constraints have not been advertised or agreed on
>  externally, then a server SHOULD support a default minimum block size
> -of 1, a preferred block size of 2^12 (4,096), and a maximum block size
> -that is effectively unlimited (0xffffffff, or the export size if that
> -is smaller), while a client desiring maximum interoperability SHOULD
> -constrain its requests to a minimum block size of 2^9 (512), and limit
> -`NBD_CMD_READ` and `NBD_CMD_WRITE` commands to a maximum block size of
> -2^25 (33,554,432).  A server that wants to enforce block sizes other
> -than the defaults specified here MAY refuse to go into transmission
> -phase with a client that uses `NBD_OPT_EXPORT_NAME` (via a hard
> -disconnect) or which uses `NBD_OPT_GO` without requesting
> +of 1, a preferred block size of 2^12 (4,096), and a maximum block
> +payload size that is at least 2^25 (33,554,432) (even if the export
> +size is smaller); while a client desiring maximum interoperability
> +SHOULD constrain its requests to a minimum block size of 2^9 (512),
> +and limit `NBD_CMD_READ` and `NBD_CMD_WRITE` commands to a maximum
> +block size of 2^25 (33,554,432).  A server that wants to enforce block
> +sizes other than the defaults specified here MAY refuse to go into
> +transmission phase with a client that uses `NBD_OPT_EXPORT_NAME` (via
> +a hard disconnect) or which uses `NBD_OPT_GO` without requesting
This does more than what the commit message says: it also limits payload
size from 0xffffffff to 2^25. We already have a "A server that desires
maximum interoperability" clause that mentions the 2^25, so I'm not
entirely sure why we need to restrict that for the default cause.

Remember, apart from specifying how something should be implemented, the
spec also documents current and historic behavior. I am probably
convinced that it makes more sense to steer people towards limiting to
2^25, but it should be done in such a way that servers which accept a
0xffffffff block size are not suddenly noncompliant. I don't think this
does that.

[no further comments on this one]
-- 
     w at uter.{be,co.za}
wouter@{grep.be,fosdem.org,debian.org}

I will have a Tin-Actinium-Potassium mixture, thanks.

On Tue, Feb 21, 2023 at 05:21:37PM +0200, Wouter Verhelst
wrote:
> Hi Eric,
> 
> Busy days, busy times. Sorry about the insane delays here.
No problem; I've been tackling other things in the meantime too, so
this extension has taken far longer than I planned for more reasons
than just slow review time.
> 
> On Mon, Nov 14, 2022 at 04:46:51PM -0600, Eric Blake wrote:
> > Commit 9f30fedb improved the spec to allow non-payload requests that
> > exceed any advertised maximum block size.  Take this one step further
> > by permitting the server to use NBD_EOVERFLOW as a hint to the client
> > when a request is oversize (while permitting NBD_EINVAL for
> > back-compat), and by rewording the text to explicitly call out that
> > what is being advertised is the maximum payload length, not maximum
> > block size.  This becomes more important when we add 64-bit
> > extensions, where it becomes possible to extend `NBD_CMD_BLOCK_STATUS`
> > to have both an effect length (how much of the image does the client
> > want status on - may be larger than 32 bits) and an optional payload
> > length (a way to filter the response to a subset of negotiated
> > metadata contexts).  In the shorter term, it means that a server may
> > (but not must) accept a read request larger than the maximum block
> > size if it can use structured replies to keep each chunk of the
> > response under the maximum payload limits.
> > ---
> >  doc/proto.md | 127
+++++++++++++++++++++++++++++----------------------
> >  1 file changed, 73 insertions(+), 54 deletions(-)
> > 
> > diff --git a/doc/proto.md b/doc/proto.md
> > index 8f08583..53c334a 100644
> > --- a/doc/proto.md
> > +++ b/doc/proto.md
> > @@ -745,8 +745,8 @@ text unless the client insists on TLS.
> > 
> >  During transmission phase, several operations are constrained by the
> >  export size sent by the final `NBD_OPT_EXPORT_NAME` or `NBD_OPT_GO`,
> > -as well as by three block size constraints defined here (minimum,
> > -preferred, and maximum).
> > +as well as by three block size constraints defined here (minimum
> > +block, preferred block, and maximum payload).
> > 
> >  If a client can honour server block size constraints (as set out
below
> >  and under `NBD_INFO_BLOCK_SIZE`), it SHOULD announce this during the
> > @@ -772,15 +772,15 @@ learn the server's constraints without
committing to them.
> > 
> >  If block size constraints have not been advertised or agreed on
> >  externally, then a server SHOULD support a default minimum block size
> > -of 1, a preferred block size of 2^12 (4,096), and a maximum block
size
> > -that is effectively unlimited (0xffffffff, or the export size if that
> > -is smaller), while a client desiring maximum interoperability SHOULD
> > -constrain its requests to a minimum block size of 2^9 (512), and
limit
> > -`NBD_CMD_READ` and `NBD_CMD_WRITE` commands to a maximum block size
of
> > -2^25 (33,554,432).  A server that wants to enforce block sizes other
> > -than the defaults specified here MAY refuse to go into transmission
> > -phase with a client that uses `NBD_OPT_EXPORT_NAME` (via a hard
> > -disconnect) or which uses `NBD_OPT_GO` without requesting
> > +of 1, a preferred block size of 2^12 (4,096), and a maximum block
> > +payload size that is at least 2^25 (33,554,432) (even if the export
> > +size is smaller); while a client desiring maximum interoperability
> > +SHOULD constrain its requests to a minimum block size of 2^9 (512),
> > +and limit `NBD_CMD_READ` and `NBD_CMD_WRITE` commands to a maximum
> > +block size of 2^25 (33,554,432).  A server that wants to enforce
block
> > +sizes other than the defaults specified here MAY refuse to go into
> > +transmission phase with a client that uses `NBD_OPT_EXPORT_NAME` (via
> > +a hard disconnect) or which uses `NBD_OPT_GO` without requesting
> 
> This does more than what the commit message says: it also limits payload
> size from 0xffffffff to 2^25. We already have a "A server that desires
> maximum interoperability" clause that mentions the 2^25, so I'm
not
> entirely sure why we need to restrict that for the default cause.
> 
> Remember, apart from specifying how something should be implemented, the
> spec also documents current and historic behavior. I am probably
> convinced that it makes more sense to steer people towards limiting to
> 2^25, but it should be done in such a way that servers which accept a
> 0xffffffff block size are not suddenly noncompliant. I don't think this
> does that.
I'll have to play more with the wording here.  A client shouldn't send
larger write requests to a server without knowing the server will
accept it (because traditional servers disconnect instead - the
alternative is to read the client's entire payload, and the larger the
payload is, the longer that takes - the client is starving the server
from serving other clients by making it chew through data).  But
sending larger read requests MAY be tolerated by a server that sends a
graceful error message ("you requested more than I'm willing to send,
but my error response is short so the connection can stay open"), even
if several historical implementations have also hung up at that point
(at least qemu implements a malloc() call for both read and writes;
reads populate the buffer from the client, writes populate the buffer
to send back to the client - the connection is aborted when the
malloc() is not attempted because it exceeds 2^25 bytes).

But I also see your point about not declaring a server non-compliant
merely because it allows for a larger payload, nor a client that sends
larger payloads to a known-happy server that accepts those payloads.
nbdkit historically chose 64M as its limits instead of qemu's 32M.

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