Re: [Nbd] [PATCH/RFCv2] Remove NBD_OPT_BLOCK_SIZE

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

On Tue, Apr 26, 2016 at 11:00:15AM +0100, Alex Bligh wrote:
> Wouter,
> 
> Some comments in line.
> 
> On 26 Apr 2016, at 09:30, Wouter Verhelst <w@...112...> wrote:
> > I understand (and share) the desire to keep things simple, but I think
> > this is a mistake.
> 
> Well, that was the point of the RFC :-)

Good :-)

> > We create extensions not only for people who implement the NBD protocol
> > today, but also for future implementors. As such, I want to make as few
> > as possible cases of "if you support feature A, you MUST also support
> > feature B", because it allows a possible misreading of the standard and,
> > as a result, implementation bugs.
> 
> Right. I agree with that, and that's why I proposed NBD_OPT_BLOCK_SIZE
> in the first place.
> 
> However, in this instance the block sizing info comes through (and
> *only* comes through) NBD_OPT_INFO. So in order to support block sizes
> you need to support NBD_OPT_INFO because there is no other way to
> obtain the block sizes anyway (*). The converse is really what we
> are arguing about. Can you legitimately support NBD_OPT_INFO (as a
> client) then ignore block sizes?

Right, that's the argument I think matters most. I think that yes, you
can. We provide more information about exports than *just* the block
sizes they support (the export size, whether the export requires TLS,
possibly whether future extensions are supported as well), and there is
no reason to require that this one arbitrary bit of information is
assumed to be abided by just because you asked for general information.

> I'm arguing that we can legitimately make block size support part of the info
> spec, and say if you ask for info, you must be prepared to deal with it. The
> alternate view is that info and blocksizes are different things, in which
> case the server needs some way of knowing whether the client will respect
> block sizes if given.

Right. I think they are different things. They look similar today,
because we mostly add INFO because of the block sizes -- but we also add
it for STARTTLS, and a client which doesn't care about block sizes but
which does care about STARTTLS should not have to suddenly worry about
block-aligning requests, etc.

> (*) = I am ignoring negotiating block sizes out of band, because if
> you negotiate block sizes out of band or indeed anything else
> out of band, it can impose whatever SHOULD/MUST/MAY you like.

Sure.

[...]
> An alternative here would be to take a leaf out of DHCP's (dubious)
> book. When asking for NBD_OPT_INFO you'd supply a list of extensions
> you understanderstand (i.e. a list of the NBD_REP_INFO codes), and
> you only get sent back the ones you ask for. The fact you've explicitly
> asked for it CAN be taken to mean you've understood it.

Yes; that makes a lot more more sense. I had in fact been thinking about
something along those lines, just hadn't suggested it yet.

> I guess up to this point in the email, we are talking about how best
> for a client to signal to the server that it can respect the
> server's block size constraints. I proposed but then decided I
> didn't much like NBD_OPT_BLOCK_SIZE. There's another idea above.

I haven't really understood why you dislike it, but perhaps that's just
me.

> Yet another idea would be a single 'I understand block sizes'
> bit in the flags the client sends right at the start of the negotiation
> (perhaps mirrored by a server bit - I have something useful to
> say about block sizes); I think I'd prefer that.

I don't. There's no need to burn flags for this.

> But we have more contentiousness in the following.

Not really.

> > In addition, while I understand the reasoning behind announcing block
> > sizes, I do not want to make "abiding by announced block sizes" a
> > prerequisite of "using the more modern way of choosing an export". The
> > protocol currently has no requirement for a server to use a particular
> > block size, and I think this is a feature. Sure, most clients *do*
> > currently request data in block-sized units, but that doesn't mean we
> > need to make that a requirement. A simple user-space client which wants
> > to inspect (and possibly modify) some parts of an export might not care
> > about block sizes; we should make writing such clients not harder than
> > needs be.
> 
> Well, actually every server I know of ALREADY has block size constraints.
> nbd-server.c is weakest, in that it won't support operations >= 1>>31
> bytes. Qemu will fail stuff >32Mb. gonbdserver.c fails on stuff >128Mb
> (and I won't be removing that - the DoS opportunity is too great).
> Many servers perform substantially better if reads/writes are aligned.

Sure, but that's not my point.

A server is already allowed to reject 'too large' requests, even if
block sizes aren't negotiated. That's fine, and we should keep that.

However, servers *aren't* currently allowed to reject requests that are
too small, or that are not block-aligned. I think that's the only case
in which NBD_REP_ERR_BLOCK_SIZE_REQD should be sent; it signals that the
server can't support a certain type of request, and that the client
should be prepared for that.

Block-aligning requests on the client side may be a bit much work
though, and some clients may not have the ability to abide by that
request. Therefore, if yoyu're going to require block-aligned requests,
you're effectively passing work to clients.

> If you write a server which has arcane block size constraints it
> will fail if some dumb clients talk to it. I don't think that's an
> issue. It's better they fail early than try to connect and fail on
> the 37th write.
> 
> > In that same light, I think we should discourage (but allow) the use of
> > NBD_REP_ERR_BLOCK_SIZE_REQD (although we might want to make that error's
> > name a bit shorter -- boy that's a lot of typing) in the same way that
> > we currently allow but discourage ENOMEM.
> 
> Don't agree.
> 
> If don't support block sizes under 4096 bytes (say) it's pretty
> non-negotiable, or you risk something that fails obscurely later.

Sure. My point isn't that you shouldn't announce that; it's that you
should add code that makes it possible for you to talk to older clients,
even if that requires a performance cost.

> Much better to fail at negotiation stage if you really can't
> support (or can't risk pretending to support) the client.
> 
> If what you are saying is 'it is unreasonable to use the nbd
> protocol unless you can always somehow support byte-aligned
> writes' I don't really agree.

I'm not saying that.

A small server could certainly use NBD_REP_ERR_BLOCK_SIZE_REQD and
refuse to talk to clients who don't (announce they) know how to
block-align requests. There's nothing wrong with that.

However, a server written for full interoperability and maximum
usefulness should not do so. It may issue a warning that it will be
slower, but it should be able to operate at a basic level.

For that reason, I think we should add some language to discourage the
use of that option, with the understanding that "discourage" does not
mean "forbid".

> Just as you are arguing it should be possible to write a simple
> client, I'd argue it should be possible to write a simple server.

And I agree with that :-)

Perhaps we should add a section to the spec outlining requirements for
"minimum" implementations that are still interoperable with other
implementations...?

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