* [Qemu-devel] [PATCH] proto: add 'shift' extension. @ 2016-09-26 12:46 Vladimir Sementsov-Ogievskiy 2016-09-26 12:51 ` Paolo Bonzini ` (3 more replies) 0 siblings, 4 replies; 23+ messages in thread From: Vladimir Sementsov-Ogievskiy @ 2016-09-26 12:46 UTC (permalink / raw) To: qemu-devel, qemu-block, nbd-general Cc: alex, eblake, kwolf, pbonzini, stefanha, w, den, vsementsov 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 ^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCH] proto: add 'shift' extension. 2016-09-26 12:46 [Qemu-devel] [PATCH] proto: add 'shift' extension Vladimir Sementsov-Ogievskiy @ 2016-09-26 12:51 ` Paolo Bonzini 2016-09-26 13:53 ` Vladimir Sementsov-Ogievskiy 2016-09-26 20:35 ` Eric Blake 2016-09-26 14:05 ` Alex Bligh ` (2 subsequent siblings) 3 siblings, 2 replies; 23+ messages in thread From: Paolo Bonzini @ 2016-09-26 12:51 UTC (permalink / raw) To: Vladimir Sementsov-Ogievskiy, qemu-devel, qemu-block, nbd-general Cc: alex, eblake, kwolf, stefanha, w, den 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 ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCH] proto: add 'shift' extension. 2016-09-26 12:51 ` Paolo Bonzini @ 2016-09-26 13:53 ` Vladimir Sementsov-Ogievskiy 2016-09-26 13:54 ` Paolo Bonzini 2016-09-26 20:35 ` Eric Blake 1 sibling, 1 reply; 23+ messages in thread From: Vladimir Sementsov-Ogievskiy @ 2016-09-26 13:53 UTC (permalink / raw) To: Paolo Bonzini, qemu-devel, qemu-block, nbd-general Cc: alex, eblake, kwolf, stefanha, w, den 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 ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCH] proto: add 'shift' extension. 2016-09-26 13:53 ` Vladimir Sementsov-Ogievskiy @ 2016-09-26 13:54 ` Paolo Bonzini 2016-09-26 14:06 ` Alex Bligh 0 siblings, 1 reply; 23+ messages in thread From: Paolo Bonzini @ 2016-09-26 13:54 UTC (permalink / raw) To: Vladimir Sementsov-Ogievskiy, qemu-devel, qemu-block, nbd-general Cc: alex, eblake, kwolf, stefanha, w, den 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 ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCH] proto: add 'shift' extension. 2016-09-26 13:54 ` Paolo Bonzini @ 2016-09-26 14:06 ` Alex Bligh 0 siblings, 0 replies; 23+ messages in thread From: Alex Bligh @ 2016-09-26 14:06 UTC (permalink / raw) To: Paolo Bonzini Cc: Alex Bligh, Vladimir Sementsov-Ogievskiy, qemu-devel, qemu-block, nbd-general, Eric Blake, Kevin Wolf, Stefan stefanha@redhat. com, Wouter Verhelst, den > 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 ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCH] proto: add 'shift' extension. 2016-09-26 12:51 ` Paolo Bonzini 2016-09-26 13:53 ` Vladimir Sementsov-Ogievskiy @ 2016-09-26 20:35 ` Eric Blake 1 sibling, 0 replies; 23+ messages in thread From: Eric Blake @ 2016-09-26 20:35 UTC (permalink / raw) To: Paolo Bonzini, Vladimir Sementsov-Ogievskiy, qemu-devel, qemu-block, nbd-general Cc: alex, kwolf, stefanha, w, den [-- 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 --] ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCH] proto: add 'shift' extension. 2016-09-26 12:46 [Qemu-devel] [PATCH] proto: add 'shift' extension Vladimir Sementsov-Ogievskiy 2016-09-26 12:51 ` Paolo Bonzini @ 2016-09-26 14:05 ` Alex Bligh 2016-09-26 20:21 ` Eric Blake 2016-09-27 9:43 ` [Qemu-devel] " Denis V. Lunev 3 siblings, 0 replies; 23+ messages in thread From: Alex Bligh @ 2016-09-26 14:05 UTC (permalink / raw) To: Vladimir Sementsov-Ogievskiy Cc: Alex Bligh, qemu-devel, qemu-block, nbd-general, Eric Blake, Kevin Wolf, Paolo Bonzini, Stefan stefanha@redhat. com, Wouter Verhelst, den > 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 ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCH] proto: add 'shift' extension. 2016-09-26 12:46 [Qemu-devel] [PATCH] proto: add 'shift' extension Vladimir Sementsov-Ogievskiy 2016-09-26 12:51 ` Paolo Bonzini 2016-09-26 14:05 ` Alex Bligh @ 2016-09-26 20:21 ` Eric Blake 2016-09-26 23:41 ` [Qemu-devel] [Nbd] " Wouter Verhelst 2016-09-27 9:43 ` [Qemu-devel] " Denis V. Lunev 3 siblings, 1 reply; 23+ messages in thread From: Eric Blake @ 2016-09-26 20:21 UTC (permalink / raw) To: Vladimir Sementsov-Ogievskiy, qemu-devel, qemu-block, nbd-general Cc: alex, kwolf, pbonzini, stefanha, w, den [-- 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 --] ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [Nbd] [PATCH] proto: add 'shift' extension. 2016-09-26 20:21 ` Eric Blake @ 2016-09-26 23:41 ` Wouter Verhelst 2016-09-27 7:36 ` Alex Bligh 0 siblings, 1 reply; 23+ messages in thread From: Wouter Verhelst @ 2016-09-26 23:41 UTC (permalink / raw) To: Eric Blake Cc: Vladimir Sementsov-Ogievskiy, qemu-devel, qemu-block, nbd-general, kwolf, den, stefanha, pbonzini 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 ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [Nbd] [PATCH] proto: add 'shift' extension. 2016-09-26 23:41 ` [Qemu-devel] [Nbd] " Wouter Verhelst @ 2016-09-27 7:36 ` Alex Bligh 0 siblings, 0 replies; 23+ messages in thread From: Alex Bligh @ 2016-09-27 7:36 UTC (permalink / raw) To: Wouter Verhelst Cc: Alex Bligh, Eric Blake, nbd-general, Kevin Wolf, Vladimir Sementsov-Ogievskiy, den, qemu block, qemu-devel, Stefan stefanha@redhat. com, Paolo Bonzini > 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 ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCH] proto: add 'shift' extension. 2016-09-26 12:46 [Qemu-devel] [PATCH] proto: add 'shift' extension Vladimir Sementsov-Ogievskiy ` (2 preceding siblings ...) 2016-09-26 20:21 ` Eric Blake @ 2016-09-27 9:43 ` Denis V. Lunev 2016-09-27 10:15 ` Paolo Bonzini 3 siblings, 1 reply; 23+ messages in thread From: Denis V. Lunev @ 2016-09-27 9:43 UTC (permalink / raw) To: Vladimir Sementsov-Ogievskiy, qemu-devel, qemu-block, nbd-general Cc: alex, eblake, kwolf, pbonzini, stefanha, w 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 ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCH] proto: add 'shift' extension. 2016-09-27 9:43 ` [Qemu-devel] " Denis V. Lunev @ 2016-09-27 10:15 ` Paolo Bonzini 2016-09-27 10:25 ` Denis V. Lunev 0 siblings, 1 reply; 23+ messages in thread From: Paolo Bonzini @ 2016-09-27 10:15 UTC (permalink / raw) To: Denis V. Lunev Cc: Vladimir Sementsov-Ogievskiy, qemu-devel, qemu-block, nbd-general, alex, eblake, kwolf, stefanha, w > 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 ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCH] proto: add 'shift' extension. 2016-09-27 10:15 ` Paolo Bonzini @ 2016-09-27 10:25 ` Denis V. Lunev 2016-09-27 12:07 ` Paolo Bonzini 0 siblings, 1 reply; 23+ messages in thread From: Denis V. Lunev @ 2016-09-27 10:25 UTC (permalink / raw) To: Paolo Bonzini Cc: Vladimir Sementsov-Ogievskiy, qemu-devel, qemu-block, nbd-general, alex, eblake, kwolf, stefanha, w 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 ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCH] proto: add 'shift' extension. 2016-09-27 10:25 ` Denis V. Lunev @ 2016-09-27 12:07 ` Paolo Bonzini 2016-09-27 13:28 ` Denis V. Lunev 2016-09-27 13:46 ` Denis V. Lunev 0 siblings, 2 replies; 23+ messages in thread From: Paolo Bonzini @ 2016-09-27 12:07 UTC (permalink / raw) To: Denis V. Lunev Cc: Vladimir Sementsov-Ogievskiy, qemu-devel, qemu-block, nbd-general, alex, eblake, kwolf, stefanha, w ----- 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 ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCH] proto: add 'shift' extension. 2016-09-27 12:07 ` Paolo Bonzini @ 2016-09-27 13:28 ` Denis V. Lunev 2016-09-27 17:04 ` Paolo Bonzini 2016-09-27 13:46 ` Denis V. Lunev 1 sibling, 1 reply; 23+ messages in thread From: Denis V. Lunev @ 2016-09-27 13:28 UTC (permalink / raw) To: Paolo Bonzini Cc: Vladimir Sementsov-Ogievskiy, qemu-devel, qemu-block, nbd-general, alex, eblake, kwolf, stefanha, w 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 ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCH] proto: add 'shift' extension. 2016-09-27 13:28 ` Denis V. Lunev @ 2016-09-27 17:04 ` Paolo Bonzini 2016-09-27 18:59 ` Denis V. Lunev 0 siblings, 1 reply; 23+ messages in thread From: Paolo Bonzini @ 2016-09-27 17:04 UTC (permalink / raw) To: Denis V. Lunev Cc: Vladimir Sementsov-Ogievskiy, qemu-devel, qemu-block, nbd-general, alex, eblake, kwolf, stefanha, w 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 ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCH] proto: add 'shift' extension. 2016-09-27 17:04 ` Paolo Bonzini @ 2016-09-27 18:59 ` Denis V. Lunev 2016-09-28 8:34 ` Kevin Wolf 0 siblings, 1 reply; 23+ messages in thread From: Denis V. Lunev @ 2016-09-27 18:59 UTC (permalink / raw) To: Paolo Bonzini Cc: Vladimir Sementsov-Ogievskiy, qemu-devel, qemu-block, nbd-general, alex, eblake, kwolf, stefanha, w 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 ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCH] proto: add 'shift' extension. 2016-09-27 18:59 ` Denis V. Lunev @ 2016-09-28 8:34 ` Kevin Wolf 2016-09-28 8:37 ` Denis V. Lunev 0 siblings, 1 reply; 23+ messages in thread From: Kevin Wolf @ 2016-09-28 8:34 UTC (permalink / raw) To: Denis V. Lunev Cc: Paolo Bonzini, Vladimir Sementsov-Ogievskiy, qemu-devel, qemu-block, nbd-general, alex, eblake, stefanha, w 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 ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCH] proto: add 'shift' extension. 2016-09-28 8:34 ` Kevin Wolf @ 2016-09-28 8:37 ` Denis V. Lunev 2016-09-28 8:56 ` Kevin Wolf 0 siblings, 1 reply; 23+ messages in thread From: Denis V. Lunev @ 2016-09-28 8:37 UTC (permalink / raw) To: Kevin Wolf Cc: Paolo Bonzini, Vladimir Sementsov-Ogievskiy, qemu-devel, qemu-block, nbd-general, alex, eblake, stefanha, w 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 ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCH] proto: add 'shift' extension. 2016-09-28 8:37 ` Denis V. Lunev @ 2016-09-28 8:56 ` Kevin Wolf 2016-09-28 9:00 ` Vladimir Sementsov-Ogievskiy 0 siblings, 1 reply; 23+ messages in thread From: Kevin Wolf @ 2016-09-28 8:56 UTC (permalink / raw) To: Denis V. Lunev Cc: Paolo Bonzini, Vladimir Sementsov-Ogievskiy, qemu-devel, qemu-block, nbd-general, alex, eblake, stefanha, w, mreitz 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 ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCH] proto: add 'shift' extension. 2016-09-28 8:56 ` Kevin Wolf @ 2016-09-28 9:00 ` Vladimir Sementsov-Ogievskiy 2016-09-28 9:32 ` Kevin Wolf 0 siblings, 1 reply; 23+ messages in thread From: Vladimir Sementsov-Ogievskiy @ 2016-09-28 9:00 UTC (permalink / raw) To: Kevin Wolf, Denis V. Lunev Cc: Paolo Bonzini, qemu-devel, qemu-block, nbd-general, alex, eblake, stefanha, w, mreitz 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 ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCH] proto: add 'shift' extension. 2016-09-28 9:00 ` Vladimir Sementsov-Ogievskiy @ 2016-09-28 9:32 ` Kevin Wolf 0 siblings, 0 replies; 23+ messages in thread From: Kevin Wolf @ 2016-09-28 9:32 UTC (permalink / raw) To: Vladimir Sementsov-Ogievskiy Cc: Denis V. Lunev, Paolo Bonzini, qemu-devel, qemu-block, nbd-general, alex, eblake, stefanha, w, mreitz 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 ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Qemu-devel] [PATCH] proto: add 'shift' extension. 2016-09-27 12:07 ` Paolo Bonzini 2016-09-27 13:28 ` Denis V. Lunev @ 2016-09-27 13:46 ` Denis V. Lunev 1 sibling, 0 replies; 23+ messages in thread From: Denis V. Lunev @ 2016-09-27 13:46 UTC (permalink / raw) To: Paolo Bonzini Cc: Vladimir Sementsov-Ogievskiy, qemu-devel, qemu-block, nbd-general, alex, eblake, kwolf, stefanha, w 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 ^ permalink raw reply [flat|nested] 23+ messages in thread
end of thread, other threads:[~2016-09-28 9:32 UTC | newest] Thread overview: 23+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-09-26 12:46 [Qemu-devel] [PATCH] proto: add 'shift' extension Vladimir Sementsov-Ogievskiy 2016-09-26 12:51 ` Paolo Bonzini 2016-09-26 13:53 ` Vladimir Sementsov-Ogievskiy 2016-09-26 13:54 ` Paolo Bonzini 2016-09-26 14:06 ` Alex Bligh 2016-09-26 20:35 ` Eric Blake 2016-09-26 14:05 ` Alex Bligh 2016-09-26 20:21 ` Eric Blake 2016-09-26 23:41 ` [Qemu-devel] [Nbd] " Wouter Verhelst 2016-09-27 7:36 ` Alex Bligh 2016-09-27 9:43 ` [Qemu-devel] " Denis V. Lunev 2016-09-27 10:15 ` Paolo Bonzini 2016-09-27 10:25 ` Denis V. Lunev 2016-09-27 12:07 ` Paolo Bonzini 2016-09-27 13:28 ` Denis V. Lunev 2016-09-27 17:04 ` Paolo Bonzini 2016-09-27 18:59 ` Denis V. Lunev 2016-09-28 8:34 ` Kevin Wolf 2016-09-28 8:37 ` Denis V. Lunev 2016-09-28 8:56 ` Kevin Wolf 2016-09-28 9:00 ` Vladimir Sementsov-Ogievskiy 2016-09-28 9:32 ` Kevin Wolf 2016-09-27 13:46 ` Denis V. Lunev
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.