From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:49427) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aimlm-0006S3-Fw for qemu-devel@nongnu.org; Wed, 23 Mar 2016 13:40:35 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aimlj-0002xD-78 for qemu-devel@nongnu.org; Wed, 23 Mar 2016 13:40:34 -0400 Received: from barbershop.grep.be ([89.106.240.122]:48623) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aimli-0002wJ-Tt for qemu-devel@nongnu.org; Wed, 23 Mar 2016 13:40:31 -0400 Date: Wed, 23 Mar 2016 18:40:15 +0100 From: Wouter Verhelst Message-ID: <20160323174015.GB2467@grep.be> References: <1458742562-30624-1-git-send-email-den@openvz.org> <1458742562-30624-2-git-send-email-den@openvz.org> <56F2B2C2.9000503@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <56F2B2C2.9000503@redhat.com> Subject: Re: [Qemu-devel] [Nbd] [PATCH 1/2] NBD proto: add WRITE_ZEROES extension List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake Cc: nbd-general@lists.sourceforge.net, Kevin Wolf , qemu-devel@nongnu.org, Stefan Hajnoczi , Paolo Bonzini , "Denis V. Lunev" On Wed, Mar 23, 2016 at 09:14:10AM -0600, Eric Blake wrote: > On 03/23/2016 08:16 AM, Denis V. Lunev wrote: [...] > [1] Oh, you ARE adding this to the "Experimental extensions" section of > the document, so your wording IS correct. I guess the idea is that we > write up the documentation in the experimental section, tweak qemu to > implement it both as NBD client and as NBD server (since qemu has code > that can serve in both positions), see how well it worked, and THEN do a > followup patch to proto.md to move the text into the non-experimental > section, along with any tweaks learned during the qemu patches. That's the way I accept specification changes now, yes: define an experimental spec first, wait for a first implementation (whether that's qemu or the reference implementation is not as relevant), and only then move it to the normative part of the spec. > > +one new command with two command flags. > > + > > +* `NBD_CMD_WRITE_ZEROES` (6) > > + > > + A write request with no payload. Length and offset define the location > > + and amount of data to be zeroed. > > + > > + The server MUST zero out the data on disk, and then send the reply > > + message. The server MAY send the reply message before the data has > > + reached permanent storage. > > + > > + If the `NBD_FLAG_SEND_FUA` flag ("Force Unit Access") was set in the > > + export flags field, the client MAY set the flag `NBD_CMD_FLAG_FUA` (bit 0) > > + in the command flags field. If this flag was set, the server MUST NOT send > > + the reply until it has ensured that the newly-zeroed data has reached > > + permanent storage. > > Do we want to add: > > The server SHOULD return EINVAL if the client set NBD_CMD_FLAG_FUA but > the export flags did not include NBD_FLAG_SEND_FUA. Well, if the export flags didn't include NBD_FLAG_SEND_FUA, that means the server may not implement that flag, and hence the server may not know what to do with it. It's probably a good idea to send a particular error (EINVAL sounds good, and is what the current reference nbd-server implementation sends[1]) for commands or flags the server implementation doesn't know about, but the spec doesn't currently say that explicitly. [1] actually, the current server sends EINVAL for commands the server doesn't know about, but silently ignores flags the server doesn't know about. > > + If the flag `NBD_CMD_FLAG_MAY_TRIM` (bit 1) was set by the client in the > > + command flags field, the server MAY use trimming to zero out the area, > > + but it MUST ensure that the data reads back as zero. > > Bug in the existing spec: The constant NBD_CMD_FLAG_FUA is mentioned, > but never defined with a fixed value. Your text above defines it to > 'bit 0' in the command flags field - is that correct? (checks code) Yes, it is. > If so, should we add a section to the document that lists the bit > values of all supported command flags? Currently that's only the FUA flag, but yes, I'll do so. > Meanwhile, your proposed text hardcodes NBD_CMD_FLAG_MAY_TRIM to 'bit > 1'; that might also be worth adding into the same new section of the > document documenting all supported command flags. > > Do we want to require that the server has negotiated the > NBD_FLAG_SEND_TRIM export flag prior to allowing the > NBD_CMD_FLAG_MAY_TRIM flag in this command? > > Possibly-related bug in the existing spec: Should the text of the > NBD_CMD_TRIM (4) command mention the desired server behavior if the > client sends NBD_CMD_TRIM even though NBD_FLAG_SEND_TRIM was not > negotiated in the export flags? Not sure that's a good idea. The export flag is there to say "I support it". If the server doesn't support a feature, the client shouldn't use that feature. If the client *does* use it, it ends up in undefined behaviour-land (because the server may be from whatever source). > Similarly, should the text of the NBD_CMD_FLUSH () command mention the > desired server behavior if the client sends NBD_CMD_FLUSH even though > NBD_FLAG_SEND_FLUSH was not negotiated in the export flags? At least > NBD_CMD_FLUSH recommended that the client must not send the command if > the feature was not negotiated. I suppose it would make sense to make the section on NBD_CMD_TRIM have a similar notice, yes. > > + If an error occurs, the server SHOULD set the appropriate error code > > + in the error field. The server MAY then close the connection. > > + > > +The server SHOULD return `ENOSPC` if it receives a write zeroes request > > +including one or more sectors beyond the size of the device. It SHOULD > > +return `EPERM` if it receives a write zeroes request on a read-only export. > > Should we add a paragraph stating that the client MUST NOT send > NBD_CMD_WRITE_ZEROES if NBD_FLAG_SEND_WRITE_ZEROES was not negotiated in > the export options? Similarly, should we suggest that the server reply > with EINVAL if it knows about the command, but the client issues the > command in spite of not negotiating it? Should we enhance the > documentation in the "Error values" heading to mention that EINVAL > should be used in general for any client command not expected by the server? Yes, probably. As above, the current nbd-server implementation does so, and I'll make that explicit. -- < 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