[Qemu-devel] [PATCH] proto: add 'shift' extension.

[Qemu-devel] [PATCH] proto: add 'shift' extension.

[Vladimir Sementsov-Ogievskiy]

This extension allows big requests for TRIM and WRITE_ZEROES through
special 'shift' parameter, which means that offset and length should be
shifted left by several bits.

This is needed for efficient clearing large regions of the disk (up to
the whole disk).

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---

Here mentioned WRITE_ZEROES command which is only an experemental
extension for now.

To dicuss:
NBD_OPT_SHIFT Data. It can be reduced to 8 bits actually... Are some
   reserved bits needed here?

 doc/proto.md | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/doc/proto.md b/doc/proto.md
index 2de3a6a..6fd1b16 100644
--- a/doc/proto.md
+++ b/doc/proto.md
@@ -682,6 +682,8 @@ The field has the following format:
   experimental `WRITE_ZEROES` [extension](https://github.com/yoe/nbd/blob/extension-write-zeroes/doc/proto.md).
 - bit 7, `NBD_FLAG_SEND_DF`: defined by the experimental `STRUCTURED_REPLY`
   [extension](https://github.com/yoe/nbd/blob/extension-structured-reply/doc/proto.md).
+- bit 8, `NBD_FLAG_SEND_SHIFT` : exposes support for `NBD_CMD_FLAG_SHIFT` and
+  `NBD_OPT_SHIFT`
 
 Clients SHOULD ignore unknown flags.
 
@@ -765,6 +767,15 @@ of the newstyle negotiation.
 
     Defined by the experimental `INFO` [extension](https://github.com/yoe/nbd/blob/extension-info/doc/proto.md).
 
+- `NBD_OPT_SHIFT` (10)
+
+    Defines shift used to calculate request offset and length if
+    `NBD_CMD_FLAG_SHIFT` is set.
+
+    Data:
+
+    - 32 bits, shift (unsigned); Must not be larger than 32.
+
 #### Option reply types
 
 These values are used in the "reply type" field, sent by the server
@@ -872,7 +883,13 @@ valid may depend on negotiation during the handshake phase.
   [extension](https://github.com/yoe/nbd/blob/extension-write-zeroes/doc/proto.md).
 - bit 2, `NBD_CMD_FLAG_DF`; defined by the experimental `STRUCTURED_REPLY`
   [extension](https://github.com/yoe/nbd/blob/extension-structured-reply/doc/proto.md).
-
+- bit 3, `NBD_CMD_FLAG_SHIFT`; This flag is valid for `NBD_CMD_TRIM` and
+  `NBD_CMD_WRITE_ZEROES`. If this flag is set the server shifts request
+  *length* and *offset* left by N bits, where N is defined by `NBD_OPT_SHIFT`
+  option or is assumed to be 16 bits by default if `NBD_OPT_SHIFT` option is
+  not specified. If after shift `(offset + length)` exceeds disk size, length
+  should be reduced to `(<disk size> - offset)`. However, `(offset + length)`
+  must not exceed disk size by more than `(1 << N) - 1`.
 
 #### Request types
 
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH] proto: add 'shift' extension.

[Paolo Bonzini]

On 26/09/2016 14:46, Vladimir Sementsov-Ogievskiy wrote:
> This extension allows big requests for TRIM and WRITE_ZEROES through
> special 'shift' parameter, which means that offset and length should be
> shifted left by several bits.
> 
> This is needed for efficient clearing large regions of the disk (up to
> the whole disk).
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
> 
> Here mentioned WRITE_ZEROES command which is only an experemental
> extension for now.
> 
> To dicuss:
> NBD_OPT_SHIFT Data. It can be reduced to 8 bits actually... Are some
>    reserved bits needed here?
> 
>  doc/proto.md | 19 ++++++++++++++++++-
>  1 file changed, 18 insertions(+), 1 deletion(-)
> 
> diff --git a/doc/proto.md b/doc/proto.md
> index 2de3a6a..6fd1b16 100644
> --- a/doc/proto.md
> +++ b/doc/proto.md
> @@ -682,6 +682,8 @@ The field has the following format:
>    experimental `WRITE_ZEROES` [extension](https://github.com/yoe/nbd/blob/extension-write-zeroes/doc/proto.md).
>  - bit 7, `NBD_FLAG_SEND_DF`: defined by the experimental `STRUCTURED_REPLY`
>    [extension](https://github.com/yoe/nbd/blob/extension-structured-reply/doc/proto.md).
> +- bit 8, `NBD_FLAG_SEND_SHIFT` : exposes support for `NBD_CMD_FLAG_SHIFT` and
> +  `NBD_OPT_SHIFT`
>  
>  Clients SHOULD ignore unknown flags.
>  
> @@ -765,6 +767,15 @@ of the newstyle negotiation.
>  
>      Defined by the experimental `INFO` [extension](https://github.com/yoe/nbd/blob/extension-info/doc/proto.md).
>  
> +- `NBD_OPT_SHIFT` (10)
> +
> +    Defines shift used to calculate request offset and length if
> +    `NBD_CMD_FLAG_SHIFT` is set.
> +
> +    Data:
> +
> +    - 32 bits, shift (unsigned); Must not be larger than 32.
> +
>  #### Option reply types
>  
>  These values are used in the "reply type" field, sent by the server
> @@ -872,7 +883,13 @@ valid may depend on negotiation during the handshake phase.
>    [extension](https://github.com/yoe/nbd/blob/extension-write-zeroes/doc/proto.md).
>  - bit 2, `NBD_CMD_FLAG_DF`; defined by the experimental `STRUCTURED_REPLY`
>    [extension](https://github.com/yoe/nbd/blob/extension-structured-reply/doc/proto.md).
> -
> +- bit 3, `NBD_CMD_FLAG_SHIFT`; This flag is valid for `NBD_CMD_TRIM` and
> +  `NBD_CMD_WRITE_ZEROES`. If this flag is set the server shifts request
> +  *length* and *offset* left by N bits, where N is defined by `NBD_OPT_SHIFT`
> +  option or is assumed to be 16 bits by default if `NBD_OPT_SHIFT` option is
> +  not specified. If after shift `(offset + length)` exceeds disk size, length
> +  should be reduced to `(<disk size> - offset)`. However, `(offset + length)`
> +  must not exceed disk size by more than `(1 << N) - 1`.
>  
>  #### Request types
>  
> 

This is very ad hoc.  Can we instead have a block size common to all
commands?  Block devices in practice have one, in fact that's why
they're called block devices...

Paolo

Re: [Qemu-devel] [PATCH] proto: add 'shift' extension.

[Vladimir Sementsov-Ogievskiy]

On 26.09.2016 15:51, Paolo Bonzini wrote:
>
> On 26/09/2016 14:46, Vladimir Sementsov-Ogievskiy wrote:
>> This extension allows big requests for TRIM and WRITE_ZEROES through
>> special 'shift' parameter, which means that offset and length should be
>> shifted left by several bits.
>>
>> This is needed for efficient clearing large regions of the disk (up to
>> the whole disk).
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>
>> Here mentioned WRITE_ZEROES command which is only an experemental
>> extension for now.
>>
>> To dicuss:
>> NBD_OPT_SHIFT Data. It can be reduced to 8 bits actually... Are some
>>     reserved bits needed here?
>>
>>   doc/proto.md | 19 ++++++++++++++++++-
>>   1 file changed, 18 insertions(+), 1 deletion(-)
>>
>> diff --git a/doc/proto.md b/doc/proto.md
>> index 2de3a6a..6fd1b16 100644
>> --- a/doc/proto.md
>> +++ b/doc/proto.md
>> @@ -682,6 +682,8 @@ The field has the following format:
>>     experimental `WRITE_ZEROES` [extension](https://github.com/yoe/nbd/blob/extension-write-zeroes/doc/proto.md).
>>   - bit 7, `NBD_FLAG_SEND_DF`: defined by the experimental `STRUCTURED_REPLY`
>>     [extension](https://github.com/yoe/nbd/blob/extension-structured-reply/doc/proto.md).
>> +- bit 8, `NBD_FLAG_SEND_SHIFT` : exposes support for `NBD_CMD_FLAG_SHIFT` and
>> +  `NBD_OPT_SHIFT`
>>   
>>   Clients SHOULD ignore unknown flags.
>>   
>> @@ -765,6 +767,15 @@ of the newstyle negotiation.
>>   
>>       Defined by the experimental `INFO` [extension](https://github.com/yoe/nbd/blob/extension-info/doc/proto.md).
>>   
>> +- `NBD_OPT_SHIFT` (10)
>> +
>> +    Defines shift used to calculate request offset and length if
>> +    `NBD_CMD_FLAG_SHIFT` is set.
>> +
>> +    Data:
>> +
>> +    - 32 bits, shift (unsigned); Must not be larger than 32.
>> +
>>   #### Option reply types
>>   
>>   These values are used in the "reply type" field, sent by the server
>> @@ -872,7 +883,13 @@ valid may depend on negotiation during the handshake phase.
>>     [extension](https://github.com/yoe/nbd/blob/extension-write-zeroes/doc/proto.md).
>>   - bit 2, `NBD_CMD_FLAG_DF`; defined by the experimental `STRUCTURED_REPLY`
>>     [extension](https://github.com/yoe/nbd/blob/extension-structured-reply/doc/proto.md).
>> -
>> +- bit 3, `NBD_CMD_FLAG_SHIFT`; This flag is valid for `NBD_CMD_TRIM` and
>> +  `NBD_CMD_WRITE_ZEROES`. If this flag is set the server shifts request
>> +  *length* and *offset* left by N bits, where N is defined by `NBD_OPT_SHIFT`
>> +  option or is assumed to be 16 bits by default if `NBD_OPT_SHIFT` option is
>> +  not specified. If after shift `(offset + length)` exceeds disk size, length
>> +  should be reduced to `(<disk size> - offset)`. However, `(offset + length)`
>> +  must not exceed disk size by more than `(1 << N) - 1`.
>>   
>>   #### Request types
>>   
>>
> This is very ad hoc.  Can we instead have a block size common to all
> commands?  Block devices in practice have one, in fact that's why
> they're called block devices...
>
> Paolo

Block size can be too small to clear the whole disk in one request (i.e. 
(2**31 * block_size) is too small..)
.

-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH] proto: add 'shift' extension.

[Paolo Bonzini]

On 26/09/2016 15:53, Vladimir Sementsov-Ogievskiy wrote:
> On 26.09.2016 15:51, Paolo Bonzini wrote:
>> This is very ad hoc.  Can we instead have a block size common to all
>> commands?  Block devices in practice have one, in fact that's why
>> they're called block devices...
> 
> Block size can be too small to clear the whole disk in one request (i.e.
> (2**31 * block_size) is too small..)

Considering that NBD supports multiple outstanding requests, is it a big
deal to require one request per terabyte of storage?

Paolo

Re: [Qemu-devel] [PATCH] proto: add 'shift' extension.

[Alex Bligh]

> On 26 Sep 2016, at 14:54, Paolo Bonzini <pbonzini@redhat.com> wrote:
> 
> 
> Considering that NBD supports multiple outstanding requests, is it a big
> deal to require one request per terabyte of storage?

+1

Also I don't think we particularly want to make clearing the entire
disk easy to do by mistake!

This whole 'clear the whole disk in one command' seems like a
dire case of premature optimisation.

-- 
Alex Bligh

Re: [Qemu-devel] [PATCH] proto: add 'shift' extension.

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2178 bytes --]

On 09/26/2016 07:51 AM, Paolo Bonzini wrote:

>> +- bit 3, `NBD_CMD_FLAG_SHIFT`; This flag is valid for `NBD_CMD_TRIM` and
>> +  `NBD_CMD_WRITE_ZEROES`. If this flag is set the server shifts request
>> +  *length* and *offset* left by N bits, where N is defined by `NBD_OPT_SHIFT`
>> +  option or is assumed to be 16 bits by default if `NBD_OPT_SHIFT` option is
>> +  not specified. If after shift `(offset + length)` exceeds disk size, length
>> +  should be reduced to `(<disk size> - offset)`. However, `(offset + length)`
>> +  must not exceed disk size by more than `(1 << N) - 1`.
>>  
>>  #### Request types
>>  
>>
> 
> This is very ad hoc.  Can we instead have a block size common to all
> commands?  Block devices in practice have one, in fact that's why
> they're called block devices...

The INFO extensions are supposed to be a way for the server to
communicate block sizes to the guest (note, it is one-way communication;
the guest does NOT get to pick an arbitrary size, but relies on the
server reporting it) - but I don't know if that sizing information is
useful enough for the task at hand.  As it was, the INFO extension had a
proposal idea a while back about advertising a different size for TRIM
and WRITE_ZEROES than what is preferred for WRITE and READ, so having a
single size that works for shifted commands that operate on blocks
instead of bytes may not even be feasible if there is no single block
size to settle on.

But having a universal command flag that says "this command requests
offset and length in units of blocks instead of bytes", where blocks is
an up-front sizing settled on during handshake via INFO extension, and
NOT something that the client can change at-will during a live session,
may be useful.  Or it may be the source of arithmetic overflow exploits
in poor implementations, or of denial-of-service with used with a READ
or WRITE to send more than 2G of data in a single command.  In other
words, I don't yet see a compelling use case for being able to request
shifted sizes.

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH] proto: add 'shift' extension.

[Alex Bligh]

> On 26 Sep 2016, at 13:46, Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:
> 
> #### Option reply types
> 
> These values are used in the "reply type" field, sent by the server
> @@ -872,7 +883,13 @@ valid may depend on negotiation during the handshake phase.
>   [extension](https://github.com/yoe/nbd/blob/extension-write-zeroes/doc/proto.md).
> - bit 2, `NBD_CMD_FLAG_DF`; defined by the experimental `STRUCTURED_REPLY`
>   [extension](https://github.com/yoe/nbd/blob/extension-structured-reply/doc/proto.md).
> -
> +- bit 3, `NBD_CMD_FLAG_SHIFT`; This flag is valid for `NBD_CMD_TRIM` and
> +  `NBD_CMD_WRITE_ZEROES`. If this flag is set the server shifts request
> +  *length* and *offset* left by N bits, where N is defined by `NBD_OPT_SHIFT`
> +  option or is assumed to be 16 bits by default if `NBD_OPT_SHIFT` option is
> +  not specified. If after shift `(offset + length)` exceeds disk size, length
> +  should be reduced to `(<disk size> - offset)`. However, `(offset + length)`
> +  must not exceed disk size by more than `(1 << N) - 1`.

Is there a good reason the shift size can't be either the minimum block size
or otherwise negotiated using NBD_OPT_INFO?

-- 
Alex Bligh

Re: [Qemu-devel] [PATCH] proto: add 'shift' extension.

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1251 bytes --]

On 09/26/2016 07:46 AM, Vladimir Sementsov-Ogievskiy wrote:
> This extension allows big requests for TRIM and WRITE_ZEROES through
> special 'shift' parameter, which means that offset and length should be
> shifted left by several bits.
> 
> This is needed for efficient clearing large regions of the disk (up to
> the whole disk).
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
> 
> Here mentioned WRITE_ZEROES command which is only an experemental
> extension for now.
> 
> To dicuss:

> +- `NBD_OPT_SHIFT` (10)
> +
> +    Defines shift used to calculate request offset and length if
> +    `NBD_CMD_FLAG_SHIFT` is set.
> +
> +    Data:
> +
> +    - 32 bits, shift (unsigned); Must not be larger than 32.

Uggh. You're making the protocol stateful (the server has to remember
what the client has previously requested via NBD_CMD_FLAG_SHIFT, rather
than having ALL information it needs immediately available in the
current NBD_CMD_WRITE_ZEROES).

I'd much rather support a single flag that says to zero the entire disk
than to introduce stateful variable-amount shifting.

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [Nbd] [PATCH] proto: add 'shift' extension.

[Wouter Verhelst]

On Mon, Sep 26, 2016 at 03:21:46PM -0500, Eric Blake wrote:
> I'd much rather support a single flag that says to zero the entire disk
> than to introduce stateful variable-amount shifting.

That's almost exactly the opposite of what I said :)

Now, I don't feel very strong either way, but what matters to me is:

- NBD is a simple, easy to understand protocol; that is a feature, and
  so it should remain that way.
- Every time we add another option, flag, or command, we make the
  protocol slightly more complex, which is counter to that goal.
- Adding a command with a single use case (i.e., a "wipe the whole
  device" command) seems like it would not see much use, except perhaps
  in the use case that Virtuozzo is thinking about. In other words, it
  makes things slightly more complex for little benefit.

I thought a negotiated shift size could be creatively used for other
things beyond just "wipe the whole disk" commands, and that it might be
elegant in that way. On the other hand, I recognize that adding state in
that manner also complicates the protocol in that an observer which sees
only part of the traffic may not understand what's going on anymore.

So let's just say that an NBD_CMD_FLAG_SHIFT would:
- Left-shift the size by 16 bits; no more, no less
  - 2^32-1 is too large a granularity for this to be useful beyond "wipe
    whole disk" commands; 2^16-1 (65535) seems like a more useful
    granularity.
  - This allows for a maximum number of 2^48-1 bytes (one byte shy of
    256 tebibytes) to be affected by a single command, which seems
    sufficient for the given purpose.
  - If someone really wants to wipe 2^64-1 bytes (i.e., 16 exbibytes),
    they are probably using the wrong tools.
- Be only valid for commands that don't send or expect data to be sent
  out over the wire.
  - currently TRIM and WRITE_ZEROES, but not READ or WRITE.

Thoughts?

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

Re: [Qemu-devel] [Nbd] [PATCH] proto: add 'shift' extension.

[Alex Bligh]

> On 27 Sep 2016, at 00:41, Wouter Verhelst <w@uter.be> wrote:
> 
> Thoughts?

Honestly? This whole thing seems like complication for little gain. One command per 2GB wiped doesn't seem like a bad thing.

-- 
Alex Bligh

Re: [Qemu-devel] [PATCH] proto: add 'shift' extension.

[Denis V. Lunev]

On 09/26/2016 03:46 PM, Vladimir Sementsov-Ogievskiy wrote:
> This extension allows big requests for TRIM and WRITE_ZEROES through
> special 'shift' parameter, which means that offset and length should be
> shifted left by several bits.
>
> This is needed for efficient clearing large regions of the disk (up to
> the whole disk).
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>
> Here mentioned WRITE_ZEROES command which is only an experemental
> extension for now.
>
> To dicuss:
> NBD_OPT_SHIFT Data. It can be reduced to 8 bits actually... Are some
>    reserved bits needed here?
>
>  doc/proto.md | 19 ++++++++++++++++++-
>  1 file changed, 18 insertions(+), 1 deletion(-)
>
> diff --git a/doc/proto.md b/doc/proto.md
> index 2de3a6a..6fd1b16 100644
> --- a/doc/proto.md
> +++ b/doc/proto.md
> @@ -682,6 +682,8 @@ The field has the following format:
>    experimental `WRITE_ZEROES` [extension](https://github.com/yoe/nbd/blob/extension-write-zeroes/doc/proto.md).
>  - bit 7, `NBD_FLAG_SEND_DF`: defined by the experimental `STRUCTURED_REPLY`
>    [extension](https://github.com/yoe/nbd/blob/extension-structured-reply/doc/proto.md).
> +- bit 8, `NBD_FLAG_SEND_SHIFT` : exposes support for `NBD_CMD_FLAG_SHIFT` and
> +  `NBD_OPT_SHIFT`
>  
>  Clients SHOULD ignore unknown flags.
>  
> @@ -765,6 +767,15 @@ of the newstyle negotiation.
>  
>      Defined by the experimental `INFO` [extension](https://github.com/yoe/nbd/blob/extension-info/doc/proto.md).
>  
> +- `NBD_OPT_SHIFT` (10)
> +
> +    Defines shift used to calculate request offset and length if
> +    `NBD_CMD_FLAG_SHIFT` is set.
> +
> +    Data:
> +
> +    - 32 bits, shift (unsigned); Must not be larger than 32.
> +
>  #### Option reply types
>  
>  These values are used in the "reply type" field, sent by the server
> @@ -872,7 +883,13 @@ valid may depend on negotiation during the handshake phase.
>    [extension](https://github.com/yoe/nbd/blob/extension-write-zeroes/doc/proto.md).
>  - bit 2, `NBD_CMD_FLAG_DF`; defined by the experimental `STRUCTURED_REPLY`
>    [extension](https://github.com/yoe/nbd/blob/extension-structured-reply/doc/proto.md).
> -
> +- bit 3, `NBD_CMD_FLAG_SHIFT`; This flag is valid for `NBD_CMD_TRIM` and
> +  `NBD_CMD_WRITE_ZEROES`. If this flag is set the server shifts request
> +  *length* and *offset* left by N bits, where N is defined by `NBD_OPT_SHIFT`
> +  option or is assumed to be 16 bits by default if `NBD_OPT_SHIFT` option is
> +  not specified. If after shift `(offset + length)` exceeds disk size, length
> +  should be reduced to `(<disk size> - offset)`. However, `(offset + length)`
> +  must not exceed disk size by more than `(1 << N) - 1`.
>  
>  #### Request types
>  
We could go in a different direction and export flag
'has_zero_init' which will report that the storage is
initialized with all zeroes at the moment. In this
case mirroring code will not fall into this
branch.

Den

Re: [Qemu-devel] [PATCH] proto: add 'shift' extension.

[Paolo Bonzini]

> We could go in a different direction and export flag
> 'has_zero_init' which will report that the storage is
> initialized with all zeroes at the moment. In this
> case mirroring code will not fall into this
> branch.

Why don't you add the zero_init flag to QEMU's NBD driver instead?

Thanks,

Paolo

Re: [Qemu-devel] [PATCH] proto: add 'shift' extension.

[Denis V. Lunev]

On 09/27/2016 01:15 PM, Paolo Bonzini wrote:
>> We could go in a different direction and export flag
>> 'has_zero_init' which will report that the storage is
>> initialized with all zeroes at the moment. In this
>> case mirroring code will not fall into this
>> branch.
> Why don't you add the zero_init flag to QEMU's NBD driver instead?
>
> Thanks,
>
> Paolo
for all cases without knowing real backend on the server side?
I think this would be wrong.

Den

Re: [Qemu-devel] [PATCH] proto: add 'shift' extension.

[Paolo Bonzini]

----- Original Message -----
> From: "Denis V. Lunev" <den@virtuozzo.com>
> To: "Paolo Bonzini" <pbonzini@redhat.com>
> Cc: "Vladimir Sementsov-Ogievskiy" <vsementsov@virtuozzo.com>, qemu-devel@nongnu.org, qemu-block@nongnu.org,
> nbd-general@lists.sourceforge.net, alex@alex.org.uk, eblake@redhat.com, kwolf@redhat.com, stefanha@redhat.com,
> w@uter.be
> Sent: Tuesday, September 27, 2016 12:25:54 PM
> Subject: Re: [PATCH] proto: add 'shift' extension.
> 
> On 09/27/2016 01:15 PM, Paolo Bonzini wrote:
> >> We could go in a different direction and export flag
> >> 'has_zero_init' which will report that the storage is
> >> initialized with all zeroes at the moment. In this
> >> case mirroring code will not fall into this
> >> branch.
> > Why don't you add the zero_init flag to QEMU's NBD driver instead?
>
> for all cases without knowing real backend on the server side?
> I think this would be wrong.

Add it to the command line, and leave it to libvirt or the user to
pass "-drive file.driver=nbd,...,file.zero-init=on".

Paolo

Re: [Qemu-devel] [PATCH] proto: add 'shift' extension.

[Denis V. Lunev]

On 09/27/2016 03:07 PM, Paolo Bonzini wrote:
>
> ----- Original Message -----
>> From: "Denis V. Lunev" <den@virtuozzo.com>
>> To: "Paolo Bonzini" <pbonzini@redhat.com>
>> Cc: "Vladimir Sementsov-Ogievskiy" <vsementsov@virtuozzo.com>, qemu-devel@nongnu.org, qemu-block@nongnu.org,
>> nbd-general@lists.sourceforge.net, alex@alex.org.uk, eblake@redhat.com, kwolf@redhat.com, stefanha@redhat.com,
>> w@uter.be
>> Sent: Tuesday, September 27, 2016 12:25:54 PM
>> Subject: Re: [PATCH] proto: add 'shift' extension.
>>
>> On 09/27/2016 01:15 PM, Paolo Bonzini wrote:
>>>> We could go in a different direction and export flag
>>>> 'has_zero_init' which will report that the storage is
>>>> initialized with all zeroes at the moment. In this
>>>> case mirroring code will not fall into this
>>>> branch.
>>> Why don't you add the zero_init flag to QEMU's NBD driver instead?
>> for all cases without knowing real backend on the server side?
>> I think this would be wrong.
> Add it to the command line, and leave it to libvirt or the user to
> pass "-drive file.driver=nbd,...,file.zero-init=on".
>
> Paolo
I have started with something very similar for 'drive-mirror' command.
We have added additional flag for this to improve migration speed
and this was rejected.

Den

Re: [Qemu-devel] [PATCH] proto: add 'shift' extension.

[Paolo Bonzini]

On 27/09/2016 15:28, Denis V. Lunev wrote:
> On 09/27/2016 03:07 PM, Paolo Bonzini wrote:
>>
>> ----- Original Message -----
>>> From: "Denis V. Lunev" <den@virtuozzo.com>
>>> To: "Paolo Bonzini" <pbonzini@redhat.com>
>>> Cc: "Vladimir Sementsov-Ogievskiy" <vsementsov@virtuozzo.com>, qemu-devel@nongnu.org, qemu-block@nongnu.org,
>>> nbd-general@lists.sourceforge.net, alex@alex.org.uk, eblake@redhat.com, kwolf@redhat.com, stefanha@redhat.com,
>>> w@uter.be
>>> Sent: Tuesday, September 27, 2016 12:25:54 PM
>>> Subject: Re: [PATCH] proto: add 'shift' extension.
>>>
>>> On 09/27/2016 01:15 PM, Paolo Bonzini wrote:
>>>>> We could go in a different direction and export flag
>>>>> 'has_zero_init' which will report that the storage is
>>>>> initialized with all zeroes at the moment. In this
>>>>> case mirroring code will not fall into this
>>>>> branch.
>>>> Why don't you add the zero_init flag to QEMU's NBD driver instead?
>>> for all cases without knowing real backend on the server side?
>>> I think this would be wrong.
>> Add it to the command line, and leave it to libvirt or the user to
>> pass "-drive file.driver=nbd,...,file.zero-init=on".
>
> I have started with something very similar for 'drive-mirror' command.
> We have added additional flag for this to improve migration speed
> and this was rejected.

You can add it through the filename path too, through a URI option
"nbd://...?zero-init=on".

Paolo

Re: [Qemu-devel] [PATCH] proto: add 'shift' extension.

[Denis V. Lunev]

On 09/27/2016 08:04 PM, Paolo Bonzini wrote:
>
> On 27/09/2016 15:28, Denis V. Lunev wrote:
>> On 09/27/2016 03:07 PM, Paolo Bonzini wrote:
>>> ----- Original Message -----
>>>> From: "Denis V. Lunev" <den@virtuozzo.com>
>>>> To: "Paolo Bonzini" <pbonzini@redhat.com>
>>>> Cc: "Vladimir Sementsov-Ogievskiy" <vsementsov@virtuozzo.com>, qemu-devel@nongnu.org, qemu-block@nongnu.org,
>>>> nbd-general@lists.sourceforge.net, alex@alex.org.uk, eblake@redhat.com, kwolf@redhat.com, stefanha@redhat.com,
>>>> w@uter.be
>>>> Sent: Tuesday, September 27, 2016 12:25:54 PM
>>>> Subject: Re: [PATCH] proto: add 'shift' extension.
>>>>
>>>> On 09/27/2016 01:15 PM, Paolo Bonzini wrote:
>>>>>> We could go in a different direction and export flag
>>>>>> 'has_zero_init' which will report that the storage is
>>>>>> initialized with all zeroes at the moment. In this
>>>>>> case mirroring code will not fall into this
>>>>>> branch.
>>>>> Why don't you add the zero_init flag to QEMU's NBD driver instead?
>>>> for all cases without knowing real backend on the server side?
>>>> I think this would be wrong.
>>> Add it to the command line, and leave it to libvirt or the user to
>>> pass "-drive file.driver=nbd,...,file.zero-init=on".
>> I have started with something very similar for 'drive-mirror' command.
>> We have added additional flag for this to improve migration speed
>> and this was rejected.
> You can add it through the filename path too, through a URI option
> "nbd://...?zero-init=on".
>
> Paolo
ha, cool idea! Thanks!

Den

Re: [Qemu-devel] [PATCH] proto: add 'shift' extension.

[Kevin Wolf]

Am 27.09.2016 um 20:59 hat Denis V. Lunev geschrieben:
> On 09/27/2016 08:04 PM, Paolo Bonzini wrote:
> >
> > On 27/09/2016 15:28, Denis V. Lunev wrote:
> >> On 09/27/2016 03:07 PM, Paolo Bonzini wrote:
> >>> ----- Original Message -----
> >>>> From: "Denis V. Lunev" <den@virtuozzo.com>
> >>>> To: "Paolo Bonzini" <pbonzini@redhat.com>
> >>>> Cc: "Vladimir Sementsov-Ogievskiy" <vsementsov@virtuozzo.com>, qemu-devel@nongnu.org, qemu-block@nongnu.org,
> >>>> nbd-general@lists.sourceforge.net, alex@alex.org.uk, eblake@redhat.com, kwolf@redhat.com, stefanha@redhat.com,
> >>>> w@uter.be
> >>>> Sent: Tuesday, September 27, 2016 12:25:54 PM
> >>>> Subject: Re: [PATCH] proto: add 'shift' extension.
> >>>>
> >>>> On 09/27/2016 01:15 PM, Paolo Bonzini wrote:
> >>>>>> We could go in a different direction and export flag
> >>>>>> 'has_zero_init' which will report that the storage is
> >>>>>> initialized with all zeroes at the moment. In this
> >>>>>> case mirroring code will not fall into this
> >>>>>> branch.
> >>>>> Why don't you add the zero_init flag to QEMU's NBD driver instead?
> >>>> for all cases without knowing real backend on the server side?
> >>>> I think this would be wrong.
> >>> Add it to the command line, and leave it to libvirt or the user to
> >>> pass "-drive file.driver=nbd,...,file.zero-init=on".
> >> I have started with something very similar for 'drive-mirror' command.
> >> We have added additional flag for this to improve migration speed
> >> and this was rejected.
> > You can add it through the filename path too, through a URI option
> > "nbd://...?zero-init=on".
> >
> > Paolo
> ha, cool idea! Thanks!

What's the advantage of writing "?zero-init=on" instead of
",zero-init=on"? Doesn't it only add more string parsing code for no
benefit?

Kevin

Re: [Qemu-devel] [PATCH] proto: add 'shift' extension.

[Denis V. Lunev]

On 09/28/2016 11:34 AM, Kevin Wolf wrote:
> Am 27.09.2016 um 20:59 hat Denis V. Lunev geschrieben:
>> On 09/27/2016 08:04 PM, Paolo Bonzini wrote:
>>> On 27/09/2016 15:28, Denis V. Lunev wrote:
>>>> On 09/27/2016 03:07 PM, Paolo Bonzini wrote:
>>>>> ----- Original Message -----
>>>>>> From: "Denis V. Lunev" <den@virtuozzo.com>
>>>>>> To: "Paolo Bonzini" <pbonzini@redhat.com>
>>>>>> Cc: "Vladimir Sementsov-Ogievskiy" <vsementsov@virtuozzo.com>, qemu-devel@nongnu.org, qemu-block@nongnu.org,
>>>>>> nbd-general@lists.sourceforge.net, alex@alex.org.uk, eblake@redhat.com, kwolf@redhat.com, stefanha@redhat.com,
>>>>>> w@uter.be
>>>>>> Sent: Tuesday, September 27, 2016 12:25:54 PM
>>>>>> Subject: Re: [PATCH] proto: add 'shift' extension.
>>>>>>
>>>>>> On 09/27/2016 01:15 PM, Paolo Bonzini wrote:
>>>>>>>> We could go in a different direction and export flag
>>>>>>>> 'has_zero_init' which will report that the storage is
>>>>>>>> initialized with all zeroes at the moment. In this
>>>>>>>> case mirroring code will not fall into this
>>>>>>>> branch.
>>>>>>> Why don't you add the zero_init flag to QEMU's NBD driver instead?
>>>>>> for all cases without knowing real backend on the server side?
>>>>>> I think this would be wrong.
>>>>> Add it to the command line, and leave it to libvirt or the user to
>>>>> pass "-drive file.driver=nbd,...,file.zero-init=on".
>>>> I have started with something very similar for 'drive-mirror' command.
>>>> We have added additional flag for this to improve migration speed
>>>> and this was rejected.
>>> You can add it through the filename path too, through a URI option
>>> "nbd://...?zero-init=on".
>>>
>>> Paolo
>> ha, cool idea! Thanks!
> What's the advantage of writing "?zero-init=on" instead of
> ",zero-init=on"? Doesn't it only add more string parsing code for no
> benefit?
>
> Kevin
Here I appreciate the idea to pass command line options in the
target file name. Will it be performed via comma or '?' - there
is no difference for us. We will check and use what is already
implemented.

The most important for us is an approach.

Den

Re: [Qemu-devel] [PATCH] proto: add 'shift' extension.

[Kevin Wolf]

Am 28.09.2016 um 10:37 hat Denis V. Lunev geschrieben:
> On 09/28/2016 11:34 AM, Kevin Wolf wrote:
> > Am 27.09.2016 um 20:59 hat Denis V. Lunev geschrieben:
> >> On 09/27/2016 08:04 PM, Paolo Bonzini wrote:
> >>> On 27/09/2016 15:28, Denis V. Lunev wrote:
> >>>> On 09/27/2016 03:07 PM, Paolo Bonzini wrote:
> >>>>> ----- Original Message -----
> >>>>>> From: "Denis V. Lunev" <den@virtuozzo.com>
> >>>>>> To: "Paolo Bonzini" <pbonzini@redhat.com>
> >>>>>> Cc: "Vladimir Sementsov-Ogievskiy" <vsementsov@virtuozzo.com>, qemu-devel@nongnu.org, qemu-block@nongnu.org,
> >>>>>> nbd-general@lists.sourceforge.net, alex@alex.org.uk, eblake@redhat.com, kwolf@redhat.com, stefanha@redhat.com,
> >>>>>> w@uter.be
> >>>>>> Sent: Tuesday, September 27, 2016 12:25:54 PM
> >>>>>> Subject: Re: [PATCH] proto: add 'shift' extension.
> >>>>>>
> >>>>>> On 09/27/2016 01:15 PM, Paolo Bonzini wrote:
> >>>>>>>> We could go in a different direction and export flag
> >>>>>>>> 'has_zero_init' which will report that the storage is
> >>>>>>>> initialized with all zeroes at the moment. In this
> >>>>>>>> case mirroring code will not fall into this
> >>>>>>>> branch.
> >>>>>>> Why don't you add the zero_init flag to QEMU's NBD driver instead?
> >>>>>> for all cases without knowing real backend on the server side?
> >>>>>> I think this would be wrong.
> >>>>> Add it to the command line, and leave it to libvirt or the user to
> >>>>> pass "-drive file.driver=nbd,...,file.zero-init=on".
> >>>> I have started with something very similar for 'drive-mirror' command.
> >>>> We have added additional flag for this to improve migration speed
> >>>> and this was rejected.
> >>> You can add it through the filename path too, through a URI option
> >>> "nbd://...?zero-init=on".
> >>>
> >>> Paolo
> >> ha, cool idea! Thanks!
> > What's the advantage of writing "?zero-init=on" instead of
> > ",zero-init=on"? Doesn't it only add more string parsing code for no
> > benefit?
> >
> > Kevin
> Here I appreciate the idea to pass command line options in the
> target file name. Will it be performed via comma or '?' - there
> is no difference for us. We will check and use what is already
> implemented.
> 
> The most important for us is an approach.

For me, too. With commas it's not part of the file name that must be
parsed out of the string, but a separate option, which is the much
cleaner approach.

The good thing is that the conversion of NBD to individual options has
progressed far enough that you wouldn't even be able to implement the
URL extension without implementing the separate option, too. :-)
(Because all the URL parser does is splitting the URL into individual
options before passing them to nbd_open().)

Kevin

Re: [Qemu-devel] [PATCH] proto: add 'shift' extension.

[Vladimir Sementsov-Ogievskiy]

On 28.09.2016 11:56, Kevin Wolf wrote:
> Am 28.09.2016 um 10:37 hat Denis V. Lunev geschrieben:
>> On 09/28/2016 11:34 AM, Kevin Wolf wrote:
>>> Am 27.09.2016 um 20:59 hat Denis V. Lunev geschrieben:
>>>> On 09/27/2016 08:04 PM, Paolo Bonzini wrote:
>>>>> On 27/09/2016 15:28, Denis V. Lunev wrote:
>>>>>> On 09/27/2016 03:07 PM, Paolo Bonzini wrote:
>>>>>>> ----- Original Message -----
>>>>>>>> From: "Denis V. Lunev" <den@virtuozzo.com>
>>>>>>>> To: "Paolo Bonzini" <pbonzini@redhat.com>
>>>>>>>> Cc: "Vladimir Sementsov-Ogievskiy" <vsementsov@virtuozzo.com>, qemu-devel@nongnu.org, qemu-block@nongnu.org,
>>>>>>>> nbd-general@lists.sourceforge.net, alex@alex.org.uk, eblake@redhat.com, kwolf@redhat.com, stefanha@redhat.com,
>>>>>>>> w@uter.be
>>>>>>>> Sent: Tuesday, September 27, 2016 12:25:54 PM
>>>>>>>> Subject: Re: [PATCH] proto: add 'shift' extension.
>>>>>>>>
>>>>>>>> On 09/27/2016 01:15 PM, Paolo Bonzini wrote:
>>>>>>>>>> We could go in a different direction and export flag
>>>>>>>>>> 'has_zero_init' which will report that the storage is
>>>>>>>>>> initialized with all zeroes at the moment. In this
>>>>>>>>>> case mirroring code will not fall into this
>>>>>>>>>> branch.
>>>>>>>>> Why don't you add the zero_init flag to QEMU's NBD driver instead?
>>>>>>>> for all cases without knowing real backend on the server side?
>>>>>>>> I think this would be wrong.
>>>>>>> Add it to the command line, and leave it to libvirt or the user to
>>>>>>> pass "-drive file.driver=nbd,...,file.zero-init=on".
>>>>>> I have started with something very similar for 'drive-mirror' command.
>>>>>> We have added additional flag for this to improve migration speed
>>>>>> and this was rejected.
>>>>> You can add it through the filename path too, through a URI option
>>>>> "nbd://...?zero-init=on".
>>>>>
>>>>> Paolo
>>>> ha, cool idea! Thanks!
>>> What's the advantage of writing "?zero-init=on" instead of
>>> ",zero-init=on"? Doesn't it only add more string parsing code for no
>>> benefit?
>>>
>>> Kevin
>> Here I appreciate the idea to pass command line options in the
>> target file name. Will it be performed via comma or '?' - there
>> is no difference for us. We will check and use what is already
>> implemented.
>>
>> The most important for us is an approach.
> For me, too. With commas it's not part of the file name that must be
> parsed out of the string, but a separate option, which is the much
> cleaner approach.
>
> The good thing is that the conversion of NBD to individual options has
> progressed far enough that you wouldn't even be able to implement the
> URL extension without implementing the separate option, too. :-)
> (Because all the URL parser does is splitting the URL into individual
> options before passing them to nbd_open().)
>
> Kevin

Just note: we can use json instead of url, like this: virsh 
qemu-monitor-command backup-vm 
'{"execute":"drive-backup","arguments":{"device": "disk", "target": 
"json://{\"driver\":\"nbd\",\"host\":\"127.0.0.1\",\"port\":\"10809\",\"zero-init\":\"on\"}", 
"mode": "existing", "sync": "full"}}'

-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH] proto: add 'shift' extension.

[Kevin Wolf]

Am 28.09.2016 um 11:00 hat Vladimir Sementsov-Ogievskiy geschrieben:
> On 28.09.2016 11:56, Kevin Wolf wrote:
> >Am 28.09.2016 um 10:37 hat Denis V. Lunev geschrieben:
> >>On 09/28/2016 11:34 AM, Kevin Wolf wrote:
> >>>Am 27.09.2016 um 20:59 hat Denis V. Lunev geschrieben:
> >>>>On 09/27/2016 08:04 PM, Paolo Bonzini wrote:
> >>>>>On 27/09/2016 15:28, Denis V. Lunev wrote:
> >>>>>>On 09/27/2016 03:07 PM, Paolo Bonzini wrote:
> >>>>>>>----- Original Message -----
> >>>>>>>>From: "Denis V. Lunev" <den@virtuozzo.com>
> >>>>>>>>To: "Paolo Bonzini" <pbonzini@redhat.com>
> >>>>>>>>Cc: "Vladimir Sementsov-Ogievskiy" <vsementsov@virtuozzo.com>, qemu-devel@nongnu.org, qemu-block@nongnu.org,
> >>>>>>>>nbd-general@lists.sourceforge.net, alex@alex.org.uk, eblake@redhat.com, kwolf@redhat.com, stefanha@redhat.com,
> >>>>>>>>w@uter.be
> >>>>>>>>Sent: Tuesday, September 27, 2016 12:25:54 PM
> >>>>>>>>Subject: Re: [PATCH] proto: add 'shift' extension.
> >>>>>>>>
> >>>>>>>>On 09/27/2016 01:15 PM, Paolo Bonzini wrote:
> >>>>>>>>>>We could go in a different direction and export flag
> >>>>>>>>>>'has_zero_init' which will report that the storage is
> >>>>>>>>>>initialized with all zeroes at the moment. In this
> >>>>>>>>>>case mirroring code will not fall into this
> >>>>>>>>>>branch.
> >>>>>>>>>Why don't you add the zero_init flag to QEMU's NBD driver instead?
> >>>>>>>>for all cases without knowing real backend on the server side?
> >>>>>>>>I think this would be wrong.
> >>>>>>>Add it to the command line, and leave it to libvirt or the user to
> >>>>>>>pass "-drive file.driver=nbd,...,file.zero-init=on".
> >>>>>>I have started with something very similar for 'drive-mirror' command.
> >>>>>>We have added additional flag for this to improve migration speed
> >>>>>>and this was rejected.
> >>>>>You can add it through the filename path too, through a URI option
> >>>>>"nbd://...?zero-init=on".
> >>>>>
> >>>>>Paolo
> >>>>ha, cool idea! Thanks!
> >>>What's the advantage of writing "?zero-init=on" instead of
> >>>",zero-init=on"? Doesn't it only add more string parsing code for no
> >>>benefit?
> >>>
> >>>Kevin
> >>Here I appreciate the idea to pass command line options in the
> >>target file name. Will it be performed via comma or '?' - there
> >>is no difference for us. We will check and use what is already
> >>implemented.
> >>
> >>The most important for us is an approach.
> >For me, too. With commas it's not part of the file name that must be
> >parsed out of the string, but a separate option, which is the much
> >cleaner approach.
> >
> >The good thing is that the conversion of NBD to individual options has
> >progressed far enough that you wouldn't even be able to implement the
> >URL extension without implementing the separate option, too. :-)
> >(Because all the URL parser does is splitting the URL into individual
> >options before passing them to nbd_open().)
> >
> >Kevin
> 
> Just note: we can use json instead of url, like this: virsh
> qemu-monitor-command backup-vm
> '{"execute":"drive-backup","arguments":{"device": "disk", "target": "json://{\"driver\":\"nbd\",\"host\":\"127.0.0.1\",\"port\":\"10809\",\"zero-init\":\"on\"}",
> "mode": "existing", "sync": "full"}}'

Ah, sorry, I missed the part that you need a file name because that's
the only thing the QMP command accepts. Yes, then the json: pseudo
protocol is a good workaround for the moment.

I hope we can declare blockdev-add stable soon, and then you can use
blockdev-backup instead of drive-backup.

Kevin

Re: [Qemu-devel] [PATCH] proto: add 'shift' extension.

[Denis V. Lunev]

On 09/27/2016 03:07 PM, Paolo Bonzini wrote:
>
> ----- Original Message -----
>> From: "Denis V. Lunev" <den@virtuozzo.com>
>> To: "Paolo Bonzini" <pbonzini@redhat.com>
>> Cc: "Vladimir Sementsov-Ogievskiy" <vsementsov@virtuozzo.com>, qemu-devel@nongnu.org, qemu-block@nongnu.org,
>> nbd-general@lists.sourceforge.net, alex@alex.org.uk, eblake@redhat.com, kwolf@redhat.com, stefanha@redhat.com,
>> w@uter.be
>> Sent: Tuesday, September 27, 2016 12:25:54 PM
>> Subject: Re: [PATCH] proto: add 'shift' extension.
>>
>> On 09/27/2016 01:15 PM, Paolo Bonzini wrote:
>>>> We could go in a different direction and export flag
>>>> 'has_zero_init' which will report that the storage is
>>>> initialized with all zeroes at the moment. In this
>>>> case mirroring code will not fall into this
>>>> branch.
>>> Why don't you add the zero_init flag to QEMU's NBD driver instead?
>> for all cases without knowing real backend on the server side?
>> I think this would be wrong.
> Add it to the command line, and leave it to libvirt or the user to
> pass "-drive file.driver=nbd,...,file.zero-init=on".
>
> Paolo
this specifically means that all QMP commands like 'drive-backup' and
'drive-mirror' will have to be modified to pass this attribute. If this is
OK, we can proceed with that.

Den