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

[Wouter Verhelst]

On Tue, Mar 17, 2020 at 09:10:32PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> 17.03.2020 19:18, Wouter Verhelst wrote:
> > Hi Vladimir,
> > 
> > Sorry for the delay; I got married late last month (yay!), so obviously
> > I was a little preoccupied ;-)
> 
> Congratulations!! Be happy!
> 
> > 
> > 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.
> 
> Hmm. What about RESIZE? May be, some kind of REOPEN..

RESIZE would need a length. It would probably not need an offset, that
is true.

A REOPEN command could use both if we wanted to support a partial
reopen.

> > 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.
> > 
> 
> But why not to keep it more portable? What is a benefit?

The benefit is that you can read the whole command in one go, without
having to process the payload length and do a second read.

> I see the drawback:
> if we define it with offset/length, than, when we'll want to implement a new
> command without them, it will be incompatible extension..

No, we can just ignore the values there.

> > 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.
> 
> We may just invent a name for offset/lenght extended requests, something like
> "standard extended request", and instead of
> 
> >>> +    `***` supports extended requests, with the following
> >>> +    16-bytes payload:
> >>> +
> >>> +    64 bits: offset
> >>> +    64 bits: length
> 
> several times we'll have just
> 
> >>> +    `***` supports standarad extended requests
> 
> And we'll need additional paragraph, defining standard extended request, 
> including
> its layout (64 bits offset and 64 bits length).

That still requires the second read.

> Or what you mean by "reserved"? Just treat commands with no offset/length as 
> unknown
> commands? But this adds nothing to the spec actually.
> 
> So, do you mean, document that all extended commands have offset and length, 
> but note
> that "in future, new commands may be added without these fields" ?

No, I just mean that if we do end up defining a command that doesn't
require either of those two fields, we can just add to the spec that
"the length and offset fields are reserved, and should be set to zero
for this command".

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