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

[Alex Bligh <alex@...872...>]

Wouter,

Phew! I think we are in agreement. So unless I hear otherwise soonish, I'm
going to apply v6 with Eric's changes.

Alex

On 28 Apr 2016, at 17:35, Wouter Verhelst <w@...112...> wrote:

> 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
> 

-- 
Alex Bligh