Re: [Nbd] [Qemu-devel] [PATCH 1/2] NBD proto: add WRITE_ZEROES extension

[Eric Blake]

On 03/24/2016 02:26 AM, Wouter Verhelst wrote:
>> No, there is no specific reason. Looks like NBD_CMD_FLAG_ZEROES fits the
>> spec and implementations nicely. So I'll rewrite the extension and add
>> the flag instead of the whole command.
> 
> Actually, having given this some more thought...
> 
> There is at least one server-side implementation of nbd (mine) which
> silently ignores flags it doesn't know about. This isn't a problem for
> non-critical flags, but it could be a problem for a flag like this. Of
> course, a client shouldn't send a flag to a server which that server
> hasn't heard of, but mistakes do happen.

This is the first time where the presence or absence of a flag would
determine whether the length of the payload should be 0 bytes or len
bytes.  If the server doesn't recognize the flag, then it will try to
read the next length bytes off the wire as the data to write, rather
than as the next command to process.

> 
> Do we want to keep that in mind? If so, we might want to keep it as a
> separate command after all.
> 
> OTOH, it could be said that silently ignoring unknown messages is a bug.
> I should probably just fix my implementation instead.

While I agree that you should fix your implementation, I am strongly in
favor of a new command, so that we can blindly state that the
NBD_CMD_WRITE always sends a payload of length bytes, independent of
flag value.

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature
--
Transform Data into Opportunity.
Accelerate data analysis in your applications with
Intel Data Analytics Acceleration Library.
Click to learn more.
http://pubads.g.doubleclick.net/gampad/clk?id=278785351=/4140___
Nbd-general mailing list
Nbd-general@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nbd-general

Re: [Nbd] [Qemu-devel] [PATCH 1/2] NBD proto: add WRITE_ZEROES extension

[Wouter Verhelst]

On Thu, Mar 24, 2016 at 10:16:19AM +0300, Pavel Borzenkov wrote:
> So if no one objects, I'll send a patch correcting current spec
> ambiguities and than patches with new proposed commands.

Yes please. Thank you.

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

--
Transform Data into Opportunity.
Accelerate data analysis in your applications with
Intel Data Analytics Acceleration Library.
Click to learn more.
http://pubads.g.doubleclick.net/gampad/clk?id=278785351=/4140
___
Nbd-general mailing list
Nbd-general@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nbd-general

Re: [Nbd] [Qemu-devel] [PATCH 1/2] NBD proto: add WRITE_ZEROES extension

[Pavel Borzenkov]

On Wed, Mar 23, 2016 at 09:14:10AM -0600, Eric Blake wrote:
> On 03/23/2016 08:16 AM, Denis V. Lunev wrote:
> > From: Pavel Borzenkov 
> > 
> > There exist some cases when a client knows that the data it is going to
> > write is all zeroes. Such cases include mirroring or backing up a device
> > implemented by a sparse file.
> > 
> > With current NBD command set, the client has to issue NBD_CMD_WRITE
> > command with zeroed payload and transfer these zero bytes through the
> > wire. The server has to write the data onto disk, effectively denying
> > the sparseness.
> > 
> > To remedy this, the patch adds WRITE_ZEROES extension with one new
> > NBD_CMD_WRITE_ZEROES command.
> > 
> > Signed-off-by: Pavel Borzenkov 
> > Reviewed-by: Roman Kagan 
> > Signed-off-by: Denis V. Lunev 
> > CC: Wouter Verhelst 
> > CC: Paolo Bonzini 
> > CC: Kevin Wolf 
> > CC: Stefan Hajnoczi 
> > ---
> >  doc/proto.md | 44 
> >  1 file changed, 44 insertions(+)
> > 
> 
> >  
> > +* `NBD_CMD_WRITE_ZEROES` (6)
> > +
> > +Defined by the experimental `WRITE_ZEROES` extension; see below.
> 
> If this patch goes in to the NBD sources, the extension is not
> experimental any more, right?  Shouldn't the patch be written under the
> assumption that it will be the final form of the text, even though the
> feature is experimental until then? [1]
> 
> > +### `WRITE_ZEROES` extension
> > +
> > +There exist some cases when a client knows that the data it is going to 
> > write
> > +is all zeroes. Such cases include mirroring or backing up a device 
> > implemented
> > +by a sparse file. With current NBD command set, the client has to issue
> > +`NBD_CMD_WRITE` command with zeroed payload and transfer these zero bytes
> > +through the wire. The server has to write the data onto disk, effectively
> > +denying the sparseness.
> 
> s/denying/losing/ ?
> 
> > +
> > +To remedy this, a `WRITE_ZEROES` extension is envisioned. This extension 
> > adds
> 
> s/is envisioned/exists/ - again, under the argument that once it is part
> of this document, it is no longer experimental.
> 
> /me goes and reads the actual proto.md file, and light bulb turns on...
> 
> [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.

Yes, that is the plan.

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

Looks like a good idea. Needs to be added here and in NBD_CMD_WRITE
command description.

> 
> > +
> > +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?  If so, should we
> add a section to the document that lists the bit values of all supported
> command flags?

Yes, this is correct (at least this is how it's used in nbd and
QEMU).

My understanding of the NBD specification is that the 'command flags'
field contains per-command flags, thus their values for each command may
coincide. So I think it's better to describe flags supported by each
command in respective command's section, instead of in one global
"Command flags" section.

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

Re: [Nbd] [Qemu-devel] [PATCH 1/2] NBD proto: add WRITE_ZEROES extension

[Wouter Verhelst]

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 

Re: [Nbd] [Qemu-devel] [PATCH 1/2] NBD proto: add WRITE_ZEROES extension

[Eric Blake]

On 03/23/2016 08:16 AM, Denis V. Lunev wrote:
> From: Pavel Borzenkov 
> 
> There exist some cases when a client knows that the data it is going to
> write is all zeroes. Such cases include mirroring or backing up a device
> implemented by a sparse file.
> 
> With current NBD command set, the client has to issue NBD_CMD_WRITE
> command with zeroed payload and transfer these zero bytes through the
> wire. The server has to write the data onto disk, effectively denying
> the sparseness.
> 
> To remedy this, the patch adds WRITE_ZEROES extension with one new
> NBD_CMD_WRITE_ZEROES command.
> 
> Signed-off-by: Pavel Borzenkov 
> Reviewed-by: Roman Kagan 
> Signed-off-by: Denis V. Lunev 
> CC: Wouter Verhelst 
> CC: Paolo Bonzini 
> CC: Kevin Wolf 
> CC: Stefan Hajnoczi 
> ---
>  doc/proto.md | 44 
>  1 file changed, 44 insertions(+)
> 

>  
> +* `NBD_CMD_WRITE_ZEROES` (6)
> +
> +Defined by the experimental `WRITE_ZEROES` extension; see below.

If this patch goes in to the NBD sources, the extension is not
experimental any more, right?  Shouldn't the patch be written under the
assumption that it will be the final form of the text, even though the
feature is experimental until then? [1]

> +### `WRITE_ZEROES` extension
> +
> +There exist some cases when a client knows that the data it is going to write
> +is all zeroes. Such cases include mirroring or backing up a device 
> implemented
> +by a sparse file. With current NBD command set, the client has to issue
> +`NBD_CMD_WRITE` command with zeroed payload and transfer these zero bytes
> +through the wire. The server has to write the data onto disk, effectively
> +denying the sparseness.

s/denying/losing/ ?

> +
> +To remedy this, a `WRITE_ZEROES` extension is envisioned. This extension adds

s/is envisioned/exists/ - again, under the argument that once it is part
of this document, it is no longer experimental.

/me goes and reads the actual proto.md file, and light bulb turns on...

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

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

> +
> +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?  If so, should we
add a section to the document that lists the bit values of all supported
command flags?

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

> +
> +If an error occurs,