Re: [Nbd] [PATCH/RFCv4] Remove NBD_OPT_BLOCK_SIZE; add specific requests to NBD_OPT_INFO

[Wouter Verhelst <w@...112...>]

On Thu, Apr 28, 2016 at 09:25:10AM +0100, Alex Bligh wrote:
> Wouter,
> 
> On 28 Apr 2016, at 09:06, Wouter Verhelst <w@...112...> wrote:
> 
> > On Tue, Apr 26, 2016 at 08:36:10PM +0100, Alex Bligh wrote:
> >> Allow a client to requests specific bits of information using
> >> NBD_OPT_INFO and thereby signal to the server that it supports them.
> >> This makes it easier for the server to know whether the information
> >> it is providing will be used / respected.
> >> 
> >> Now a server can determine whether a client supports block sizes by
> >> the fact it uses NBD_OPT_INFO or NBD_OPT_GO with an NBD_INFO_BLOCK_SIZE,
> >> request as these are part of the same extension, so there is no
> >> need for NBD_OPT_BLOCK_SIZE.
> > 
> > I think we're close, except for this:
> > 
> > - I'd like to change things so that NBD_OPT_INFO doesn't commit a client
> >  to following block sizes. The rationale for this is bifold:
> >  - First and foremost, it will simplify the server, since it doesn't
> >    need to retain as much state (it can simply look at the NBD_OPT_GO
> >    command, and that one alone, to decide on whether it can do the
> >    optimized O_DIRECT path, rather than having to do a lot of global
> >    variables and ifs and buts etc).
> 
> >  - Second (and somewhat less likely), it allows a client to see if a
> >    server allows for a block size that it too can support without
> >    having to disconnect and reconnect to get more information. Let's
> >    say a client exists which can support the default block size and
> >    which can group and split requests if needs be, but which cannot
> >    guarantee a minimum block size other than 512. Such a client might
> >    want to check what the maximum block size is, but if the server
> >    replies to that with "Oh jolly, I can go to my optimized path now!
> >    Please use a minimum and preferred block size of 4K", it's pretty
> >    much screwed. As said, this is somewhat less likely to happen, but
> >    if we can support it, and in combination with the above, I think we
> >    should do this.
> 
> So, to be clear, the client signals its intention to support block
> sizes through the options passed to NBD_OPT_GO, rather than NBD_OPT_INFO
> or NBD_OPT_GO?

Right.

> I had thought of that, and would be prepared to go with it. In fact
> I rather like the fact that INFO no longer changes state (after all
> it's called ... INFO).

Exactly :-)

> Minor challenges I see:
> 
> * We'd need to suggest that the client repeat any info blocks it
>   might rely on. Arguably this introduces some pointless repetition
>   into the protocol, but there we go.

Well, INFO is just reading information upon which the client might then
make a decision whether or not to use a paritcular option. I suppose we
could claim that the server may send an NBD_REP_INFO_UNCHANGED or
something if everything the client got earlier hasn't changed, but then
you do add state and I don't want to do that. It's also just a minor
repetition anyway, so who cares -- not like it's going to increase the
negotiation time significantly.

> * We currently have text saying the server shouldn't error an
>   NBD_OPT_GO if it previously didn't error an NBD_OPT_INFO for
>   that export with no intervening other options; we'd need to
>   change that to say "and provided the requested information blocks
>   are the same".

yeah, clearly.

> > - I think we should not allow a client to do OPT_INFO followed by
> >  OPT_EXPORT_NAME. The latter is broken, and a client which can do
> >  OPT_INFO can very much do OPT_GO too. This should be a MUST for the
> >  client, and a SHOULD for the server to do a hard disconnect then. That
> >  also makes it easier to implement the above (and is possibly related)
> 
> That's fine by me, but is unrelated to blocksizes.

Not what I meant, but what I meant is stupid anyway, so nvm.

> We'd need to ensure that it didn't preclude a client falling
> back to NBD_OPT_EXPORT_NAME if the server didn't actually
> understand the NBD_OPT_INFO.

Yes, obviously :)
 
> How about going further and adding that clients SHOULD use
> NBD_OPT_GO and not NBD_OPT_EXPORT_NAME (even if they didn't
> use NBD_OPT_INFO)? Effectively as and when this becomes
> part of the main standard, we'd then be deprecating (but
> still maintaining support for) NBD_OPT_EXPORT_NAME. Again
> using it as a fallback would be fine.

That works.
 
-- 
< ron> I mean, the main *practical* problem with C++, is there's like a dozen
       people in the world who think they really understand all of its rules,
       and pretty much all of them are just lying to themselves too.
 -- #debian-devel, OFTC, 2016-02-12