Re: [RFC v2] nbd/proto: introduce extended request and 64bit commands

[Wouter Verhelst <wouter@grep.be>]

Hi Vladimir,

Sorry for the delay; I got married late last month (yay!), so obviously
I was a little preoccupied ;-)

On Fri, Feb 28, 2020 at 01:22:43PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> ping
> 
> 06.02.2020 18:15, Vladimir Sementsov-Ogievskiy wrote:
[...]
> > +The extended request message, sent by the client, looks as follows:
> > +
> > +C: 32 bits, 0x23876289, magic (`NBD_EXTENDED_REQUEST_MAGIC`)
> > +C: 16 bits, flags
> > +C: 16 bits, type
> > +C: 64 bits, handle
> > +C: 32 bits, length of payload (unsigned)
> > +C: *length* bytes of payload data (if *length* is nonzero)
[...]
> >       A client MUST NOT send a trim request unless `NBD_FLAG_SEND_TRIM`
> >       was set in the transmission flags field.
> > +    `NBD_CMD_TRIM` supports extended requests, with the following
> > +    16-bytes payload:
> > +
> > +    64 bits: offset
> > +    64 bits: length
> > +
> >   * `NBD_CMD_CACHE` (5)
> >       A cache request.  The client is informing the server that it plans
> > @@ -2095,6 +2127,12 @@ The following request types exist:
> >       including one or more sectors beyond the size of the device. It SHOULD
> >       return `NBD_EPERM` if it receives a write zeroes request on a read-only export.
> > +    `NBD_CMD_WRITE_ZEROES` supports extended requests, with the following
> > +    16-bytes payload:
> > +
> > +    64 bits: offset
> > +    64 bits: length
> > +
> >   * `NBD_CMD_BLOCK_STATUS` (7)
> >       A block status query request. Length and offset define the range
> > @@ -2154,6 +2192,12 @@ The following request types exist:
> >       `NBD_EINVAL` if it receives a `NBD_CMD_BLOCK_STATUS` request including
> >       one or more sectors beyond the size of the device.
> > +    `NBD_CMD_BLOCK_STATUS` supports extended requests, with the following
> > +    16-bytes payload:
> > +
> > +    64 bits: offset
> > +    64 bits: length

I can't actually think of any command that would not require offset and
length fields; and given my quote, it would appear neither can you.

Given that, wouldn't it make more sense to have the offset and length
fields be part of the extended request message, and to keep the payload
empty for messages that don't actually send any data along? That would
make the handling for such messages easier to do, too. In other words,
make the extended request message have a magic, flags, type, handle,
payload length, offset, and "affected length" field, and leave payload
for actual data.

If somehow we do end up with a message that does not need the offset or
length fields, we can then always mark those as reserved and make the
server ignore them.

-- 
<Lo-lan-do> Home is where you have to wash the dishes.
  -- #debian-devel, Freenode, 2004-09-22