From patchwork Tue Dec 13 12:17:49 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Wouter Verhelst X-Patchwork-Id: 9472225 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id 2BD6B6021C for ; Tue, 13 Dec 2016 12:20:47 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 14DCF28574 for ; Tue, 13 Dec 2016 12:20:47 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 08D4D28589; Tue, 13 Dec 2016 12:20:47 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.9 required=2.0 tests=BAYES_00,RCVD_IN_DNSWL_HI autolearn=ham version=3.3.1 Received: from lists.gnu.org (lists.gnu.org [208.118.235.17]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (No client certificate requested) by mail.wl.linuxfoundation.org (Postfix) with ESMTPS id 897C028574 for ; Tue, 13 Dec 2016 12:20:45 +0000 (UTC) Received: from localhost ([::1]:37745 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1cGm4a-0002eX-En for patchwork-qemu-devel@patchwork.kernel.org; Tue, 13 Dec 2016 07:20:44 -0500 Received: from eggs.gnu.org ([2001:4830:134:3::10]:50744) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1cGm28-00018r-0M for qemu-devel@nongnu.org; Tue, 13 Dec 2016 07:18:15 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1cGm23-0000iS-TB for qemu-devel@nongnu.org; Tue, 13 Dec 2016 07:18:12 -0500 Received: from [2a01:4f8:140:52e5::2] (port=36868 helo=latin.grep.be) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.71) (envelope-from ) id 1cGm23-0000iH-Is for qemu-devel@nongnu.org; Tue, 13 Dec 2016 07:18:07 -0500 Received: from [193.191.251.1] (helo=gangtai.grep.be) by latin.grep.be with esmtpsa (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.84_2) (envelope-from ) id 1cGm1q-0001SJ-Tb; Tue, 13 Dec 2016 13:17:55 +0100 Received: from wouter by gangtai.grep.be with local (Exim 4.88) (envelope-from ) id 1cGm1l-0007qa-LV; Tue, 13 Dec 2016 13:17:49 +0100 Date: Tue, 13 Dec 2016 13:17:49 +0100 From: Wouter Verhelst To: Stefan Hajnoczi Message-ID: <20161213121749.l572tsrnmnhnckvq@grep.be> References: <20161212204313.anhjire2gkwd6gzi@grep.be> <20161213103700.GD3103@stefanha-x1.localdomain> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20161213103700.GD3103@stefanha-x1.localdomain> X-Speed: Gates' Law: Every 18 months, the speed of software halves. Organization: none User-Agent: NeoMutt/20161126 (1.7.1) X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] X-Received-From: 2a01:4f8:140:52e5::2 Subject: Re: [Qemu-devel] [Nbd] [PATCH v5] doc: Add NBD_CMD_BLOCK_STATUS extension X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: "nbd-general@lists.sourceforge.net" , Kevin Wolf , Vladimir Sementsov-Ogievskiy , John Snow , "qemu-devel@nongnu.org" , Pavel Borzenkov , "Denis V. Lunev" , Markus Pargmann , Paolo Bonzini Errors-To: qemu-devel-bounces+patchwork-qemu-devel=patchwork.kernel.org@nongnu.org Sender: "Qemu-devel" X-Virus-Scanned: ClamAV using ClamSMTP On Tue, Dec 13, 2016 at 10:37:00AM +0000, Stefan Hajnoczi wrote: > On Mon, Dec 12, 2016 at 09:43:13PM +0100, Wouter Verhelst wrote: > > +- `NBD_OPT_LIST_META_CONTEXT` (10) > > + > > + Return a list of `NBD_REP_META_CONTEXT` replies, one per context, > > + followed by an `NBD_REP_ACK`. If a server replies to such a request > > + with no error message, clients MAY send NBD_CMD_BLOCK_STATUS > > + commands during the transmission phase. > > + > > + If the query string is syntactically invalid, the server SHOULD send > > + `NBD_REP_ERR_INVALID`. If the query string is syntactically valid > > + but finds no metadata contexts, the server MUST send a single > > + reply of type `NBD_REP_ACK`. > > + > > + This option MUST NOT be requested unless structured replies have > > + been negotiated first. If a client attempts to do so, a server > > + SHOULD send `NBD_REP_ERR_INVALID`. > > + > > + Data: > > + - 32 bits, length of export name > > + - String, name of export for which we wish to list or select metadata > > + contexts. > > + - 32 bits, length of query > > + - String, query to select a subset of the available metadata > > + contexts. If this is not specified (i.e., the "length of query" > > + field is 0 and no query is sent), then the server MUST send all > > + the metadata contexts it knows about. If specified, this query > > + string MUST start with a name that uniquely identifies a server > > + implementation; e.g., the reference implementation that > > + accompanies this document would support query strings starting > > + with 'nbd-server:' > > What about querying "base:" if the NBD spec adds more standard metadata > contexts in the future? "base:" does not "uniquely identify a server > implementation" but it should still be possible to query it so that > additional contexts can be added to this spec in the future. If that happens, we can update the spec to define how you can select more than one context. > Is the query matching algorithm defined anywhere? I guess it is > strncmp(query, context, strlen(query)) but I'm not sure from this text. It was my intent to allow namespaces to define their own matching algorithm. A simple strncmp could certainly work, but I could imagine that some more complicated scheme could work too (e.g., a syntax to ask "all snapshots created between date X and date Y") > Another common syntax would use an asterisk wildcard ('*') so that it's > possible to differentiate between full matches and (wildcard) partial > matches. Yes. > > + The server MUST reply with a list of `NBD_REP_META_CONTEXT` replies, > > + followed by `NBD_REP_ACK`. The metadata context ID in these replies > > + is reserved and SHOULD be set to zero; clients SHOULD disregard it. > > + > > +- `NBD_OPT_SET_META_CONTEXT` (11) > > + > > + Change the set of active metadata contexts. Issuing this command > > + replaces all previously-set metadata contexts; clients must ensure > > + that all metadata contexts they're interested in are selected with > > + the final query that they sent. > > + > > + Data: > > + - 32 bits, length of query > > + - String, query to select metadata contexts. The syntax of this > > + query is implementation-defined, except that it MUST start with a > > + namespace. This namespace may be one of the following: > > + - `base:`, for metadata contexts defined by this document; > > + - `nbd-server:`, for metadata contexts defined by the > > + implementation that accompanies this document (none > > + currently); > > + - `x-*:`, where `*` can be replaced by any random string not > > + containing colons, for local experiments. This SHOULD NOT be > > + used by metadata contexts that are expected to e widely used. > > s/ e / be / Thanks, fixed that. > > + - third-party implementations can register additional > > + namespaces by simple request to the mailinglist. > > Does this mean it is possible to activate multiple contexts but only if > their namespace is identical? That seems like an arbitrary limitation. > > In other words, the spec suggests you can activate nbd-server:a and > nbd-server:b but you cannot activate base:a and nbd-server:a. As written, it does. It was my intent (hence the addition of a field "length of query") to add the ability to send multiple queries in one SET_META_CONTEXT, and then this wouldn't be a problem. I now see that I forgot to add the wording to that effect. I'm adding the following to remedy this oversight: > I'd prefer a programming model where the contexts don't need to be set > for the lifetime of the connection. Instead the client passes a bitmap > (64-bits?) of contexts along with each BLOCK_STATUS command. That way > the client can select contexts on a per-command basis. It's unlikely > that more than 64 contexts need to be available at once but I admit it's > an arbitrary limitation... > > I guess you've considered this model and decided it's better to > negotiate upfront, it's wasteful to enable multiple contexts and then > discard the response data on the client side because only a subset is > needed for this command invocation. I do agree that it might be nice to be able to enable or disable particular metadata contexts for the lifetime of one BLOCK_STATUS command. However, the problem then becomes one of "where do we do that". The request header has a fixed size, and there is no space for such data in the request header. This could be worked around in ways that do not break compatibility with older implementations (not in the least because we're defining a new command that needs to be negotiated first, and so we could say that the server needs to understand a new request format), but I haven't found a way to do so that doesn't strike me as "very ugly". In addition, most use cases that we've been presented with seem to require only one or (at most) a handful of metadata contexts. In that case, the ability to select which metadata contexts are to be used in transmission doesn't strike me as a very useful possibility, which would be used often. Given that, and given the problems described in the previous paragraph, I'm not convinced it's worth complicating things much over. However, I could be convinced otherwise by strong arguments and by a suggested spec for how to pass the required information in a clean way. > > + The server MUST reply with a number of `NBD_REP_META_CONTEXT` > > + replies, one for each selected metadata context, each with a unique > > + metadata context ID. It is not an error if a > > + `NBD_OPT_SET_META_CONTEXT` option does not select any metadata > > + context, provided the client then does not attempt to issue > > + `NBD_CMD_BLOCK_STATUS` commands. > > + > > #### Option reply types > > > > These values are used in the "reply type" field, sent by the server > > @@ -882,7 +989,7 @@ during option haggling in the fixed newstyle negotiation. > > information is available, or when sending data related to the option > > (in the case of `NBD_OPT_LIST`) has finished. No data. > > > > -* `NBD_REP_SERVER` (2) > > +- `NBD_REP_SERVER` (2) > > > > A description of an export. Data: > > > > @@ -897,10 +1004,18 @@ during option haggling in the fixed newstyle negotiation. > > particular client request, this field is defined to be a string > > suitable for direct display to a human being. > > > > -* `NBD_REP_INFO` (3) > > +- `NBD_REP_INFO` (3) > > > > Defined by the experimental `INFO` [extension](https://github.com/NetworkBlockDevice/nbd/blob/extension-info/doc/proto.md). > > > > +- `NBD_REP_META_CONTEXT` (4) > > + > > + A description of a metadata context. Data: > > + > > + - 32 bits, NBD metadata context ID. > > + - String, name of the metadata context. This is not required to be > > + a human-readable string, but it MUST be valid UTF-8 data. > > + > > There are a number of error reply types, all of which are denoted by > > having bit 31 set. All error replies MAY have some data set, in which > > case that data is an error message string suitable for display to the user. > > @@ -938,15 +1053,62 @@ case that data is an error message string suitable for display to the user. > > > > Defined by the experimental `INFO` [extension](https://github.com/NetworkBlockDevice/nbd/blob/extension-info/doc/proto.md). > > > > -* `NBD_REP_ERR_SHUTDOWN` (2^32 + 7) > > +* `NBD_REP_ERR_SHUTDOWN` (2^31 + 7) > > > > The server is unwilling to continue negotiation as it is in the > > process of being shut down. > > > > -* `NBD_REP_ERR_BLOCK_SIZE_REQD` (2^32 + 8) > > +* `NBD_REP_ERR_BLOCK_SIZE_REQD` (2^31 + 8) > > Are these separate fixes that slipped into the patch? Not directly > relevant to BLOCK_STATUS. Yeah, like I said, they were. I should probably update master (and structured reply) to fix this, but it's not been as important ;-) diff --git a/doc/proto.md b/doc/proto.md index 5737abe..e03f434 100644 --- a/doc/proto.md +++ b/doc/proto.md @@ -971,6 +971,9 @@ of the newstyle negotiation. - third-party implementations can register additional namespaces by simple request to the mailinglist. + These two fields MAY be repeated as much as is necessary to select all + metadata contexts the client is interested in. + The server MUST reply with a number of `NBD_REP_META_CONTEXT` replies, one for each selected metadata context, each with a unique metadata context ID. It is not an error if a