Re: [Qemu-devel] [PATCH 1/3] Add blkmirror block driver
On Thu, Feb 23, 2012 at 4:14 PM, Stefan Hajnoczi stefa...@gmail.com wrote: On Wed, Feb 22, 2012 at 5:13 PM, Federico Simoncelli fsimo...@redhat.com wrote: From: Marcelo Tosatti mtosa...@redhat.com Mirrored writes are used by live block copy. I think the right approach is to create a single blkmirror driver that also includes blkverify functionality. The code is basically the same except blkverify also compares reads - just use a flag to enable/disable that behavior. Feel free to rename the blkverify driver to blkmirror if you wish. Federico: Any response to this? I still think blkmirror and blkverify do essentially the same thing and should be unified. Stefan
Re: [Qemu-devel] [PATCH 1/3] Add blkmirror block driver
On 02/27/2012 10:23 AM, Stefan Hajnoczi wrote: I think the right approach is to create a single blkmirror driver that also includes blkverify functionality. The code is basically the same except blkverify also compares reads - just use a flag to enable/disable that behavior. Feel free to rename the blkverify driver to blkmirror if you wish. Federico: Any response to this? I still think blkmirror and blkverify do essentially the same thing and should be unified. Once non-incremental mode is added, I suspect blkmirror will diverge from blkverify significantly. In particular, we would need to track where have writes been done in the destination. We also would need to hooks for block/stream.c, or perhaps a completely separate implementation of streaming. Also, blkverify doesn't support cancellation. I know we do quite poorly in this area, but I'd rather not make it worse... Paolo
Re: [Qemu-devel] [PATCH 1/3] Add blkmirror block driver
On Mon, Feb 27, 2012 at 11:37 AM, Paolo Bonzini pbonz...@redhat.com wrote: On 02/27/2012 10:23 AM, Stefan Hajnoczi wrote: I think the right approach is to create a single blkmirror driver that also includes blkverify functionality. The code is basically the same except blkverify also compares reads - just use a flag to enable/disable that behavior. Feel free to rename the blkverify driver to blkmirror if you wish. Federico: Any response to this? I still think blkmirror and blkverify do essentially the same thing and should be unified. Once non-incremental mode is added, I suspect blkmirror will diverge from blkverify significantly. In particular, we would need to track where have writes been done in the destination. We also would need to hooks for block/stream.c, or perhaps a completely separate implementation of streaming. I'm not familiar with non-incremental mode, could you please explain it or link to it? Also, blkverify doesn't support cancellation. I know we do quite poorly in this area, but I'd rather not make it worse... That's why I suggested unifying them - take the best of both approaches. Stefan
Re: [Qemu-devel] [PATCH 1/3] Add blkmirror block driver
On 02/27/2012 12:42 PM, Stefan Hajnoczi wrote: Once non-incremental mode is added, I suspect blkmirror will diverge from blkverify significantly. In particular, we would need to track where have writes been done in the destination. We also would need to hooks for block/stream.c, or perhaps a completely separate implementation of streaming. I'm not familiar with non-incremental mode, could you please explain it or link to it? Non-incremental mode is pre-copy migration. It would stream in the background from the source to the destination. In this case: - you need to differentiate streaming writes from other writes. When streaming, you do not want to issue spurious writes to the source; - you can skip streaming anything that has been written to the destination already. This means that you need: 1) a bitmap similar to is_allocated; 2) to widen writes to a cluster; 3) serialization similar to copy-on-read. I'm not yet sure how much of this will be generalized in the block layer and block/stream.c, and how much will be specific to blkmirror, but in general none of this applies to blkverify. Paolo
Re: [Qemu-devel] [PATCH 1/3] Add blkmirror block driver
On Mon, Feb 27, 2012 at 11:48 AM, Paolo Bonzini pbonz...@redhat.com wrote: On 02/27/2012 12:42 PM, Stefan Hajnoczi wrote: Once non-incremental mode is added, I suspect blkmirror will diverge from blkverify significantly. In particular, we would need to track where have writes been done in the destination. We also would need to hooks for block/stream.c, or perhaps a completely separate implementation of streaming. I'm not familiar with non-incremental mode, could you please explain it or link to it? Non-incremental mode is pre-copy migration. It would stream in the background from the source to the destination. In this case: - you need to differentiate streaming writes from other writes. When streaming, you do not want to issue spurious writes to the source; - you can skip streaming anything that has been written to the destination already. This means that you need: 1) a bitmap similar to is_allocated; 2) to widen writes to a cluster; 3) serialization similar to copy-on-read. I'm not yet sure how much of this will be generalized in the block layer and block/stream.c, and how much will be specific to blkmirror, but in general none of this applies to blkverify. Agreed but I'm not sure it has to do with blkmirror either. blkmirror and blkverify perform the same function except that blkverify mirrors reads too and checks that they match. Who knows when and if pre-copy will be (re)implemented, it's not a good argument to say let's duplicate block mirroring because we're not sure about the design of a feature feature yet which might want to hook in here. Stefan
Re: [Qemu-devel] [PATCH 1/3] Add blkmirror block driver
On 02/27/2012 02:09 PM, Stefan Hajnoczi wrote: Non-incremental mode is pre-copy migration. It would stream in the background from the source to the destination. In this case: - you need to differentiate streaming writes from other writes. When streaming, you do not want to issue spurious writes to the source; - you can skip streaming anything that has been written to the destination already. This means that you need: 1) a bitmap similar to is_allocated; 2) to widen writes to a cluster; 3) serialization similar to copy-on-read. I'm not yet sure how much of this will be generalized in the block layer and block/stream.c, and how much will be specific to blkmirror, but in general none of this applies to blkverify. Agreed but I'm not sure it has to do with blkmirror either. blkmirror and blkverify perform the same function except that blkverify mirrors reads too and checks that they match. Who knows when and if pre-copy will be (re)implemented, it's not a good argument to say let's duplicate block mirroring because we're not sure about the design of a feature feature yet which might want to hook in here. It's not a duplicate, it just happens that two very simple drivers share 1 operation out of 4 (open, read, write, flush). There are other differences, for example: 1) blkverify hardcodes raw for one format and auto-detects the other. blkmirror needs to have a specified format for both, and I don't think starting another bikeshedding discussion on blkverify backwards compatibility is a good idea; 2) blkverify doesn't flush the raw file; 3) blkverify in the future could add callbacks to create snapshots or load/save imgstate, and forward them to the test file; this doesn't make sense for blkmirror. Paolo
Re: [Qemu-devel] [PATCH 1/3] Add blkmirror block driver
On Mon, Feb 27, 2012 at 1:47 PM, Paolo Bonzini pbonz...@redhat.com wrote: On 02/27/2012 02:09 PM, Stefan Hajnoczi wrote: Non-incremental mode is pre-copy migration. It would stream in the background from the source to the destination. In this case: - you need to differentiate streaming writes from other writes. When streaming, you do not want to issue spurious writes to the source; - you can skip streaming anything that has been written to the destination already. This means that you need: 1) a bitmap similar to is_allocated; 2) to widen writes to a cluster; 3) serialization similar to copy-on-read. I'm not yet sure how much of this will be generalized in the block layer and block/stream.c, and how much will be specific to blkmirror, but in general none of this applies to blkverify. Agreed but I'm not sure it has to do with blkmirror either. blkmirror and blkverify perform the same function except that blkverify mirrors reads too and checks that they match. Who knows when and if pre-copy will be (re)implemented, it's not a good argument to say let's duplicate block mirroring because we're not sure about the design of a feature feature yet which might want to hook in here. It's not a duplicate, it just happens that two very simple drivers share 1 operation out of 4 (open, read, write, flush). There are other differences, for example: 1) blkverify hardcodes raw for one format and auto-detects the other. blkmirror needs to have a specified format for both, and I don't think starting another bikeshedding discussion on blkverify backwards compatibility is a good idea; Backwards compatibility isn't needed here, it's a debugging tool and some distros even disable it. Allowing another format for the reference image is a feature that would be nice to have. It allows direct qcow2 to vmdk comparison, for example, if we ever hit a bug that benefits from this type of comparison. 2) blkverify doesn't flush the raw file; It's fine to flush the reference file. This is an accidental difference :). 3) blkverify in the future could add callbacks to create snapshots or load/save imgstate, and forward them to the test file; this doesn't make sense for blkmirror. I guess what I'm saying is, if we need to copy-paste in order to fork them in the future that's fine, but why maintain duplicates in the mean-time? Please make the codebase nice today. We can always extend it in the future, we're not forced to keep them unified forever if it turns out we want to split them. But as it stands they are essentially the same thing. Stefan
Re: [Qemu-devel] [PATCH 1/3] Add blkmirror block driver
On Mon, Feb 27, 2012 at 2:49 PM, Stefan Hajnoczi stefa...@gmail.com wrote: On Mon, Feb 27, 2012 at 1:47 PM, Paolo Bonzini pbonz...@redhat.com wrote: On 02/27/2012 02:09 PM, Stefan Hajnoczi wrote: 3) blkverify in the future could add callbacks to create snapshots or load/save imgstate, and forward them to the test file; this doesn't make sense for blkmirror. I guess what I'm saying is, if we need to copy-paste in order to fork them in the future that's fine, but why maintain duplicates in the mean-time? Please make the codebase nice today. We can always extend it in the future, we're not forced to keep them unified forever if it turns out we want to split them. But as it stands they are essentially the same thing. BTW, I feel that I may just be missing context on what the plans are around mirroring and block migration. Is there a plan or discussions that haven't hit qemu-devel (maybe they were done in ovirt or internally)? Stefan
Re: [Qemu-devel] [PATCH 1/3] Add blkmirror block driver
On 02/27/2012 03:59 PM, Stefan Hajnoczi wrote: I guess what I'm saying is, if we need to copy-paste in order to fork them in the future that's fine, but why maintain duplicates in the mean-time? Please make the codebase nice today. We can always extend it in the future, we're not forced to keep them unified forever if it turns out we want to split them. But as it stands they are essentially the same thing. BTW, I feel that I may just be missing context on what the plans are around mirroring and block migration. Is there a plan or discussions that haven't hit qemu-devel (maybe they were done in ovirt or internally)? Not that I know of. Me, I'm not on any oVirt list. :) Paolo
Re: [Qemu-devel] [PATCH 1/3] Add blkmirror block driver
On Wed, Feb 22, 2012 at 5:13 PM, Federico Simoncelli fsimo...@redhat.com wrote: From: Marcelo Tosatti mtosa...@redhat.com Mirrored writes are used by live block copy. I think the right approach is to create a single blkmirror driver that also includes blkverify functionality. The code is basically the same except blkverify also compares reads - just use a flag to enable/disable that behavior. Feel free to rename the blkverify driver to blkmirror if you wish. By the way, this code does not build against qemu.git/master. It uses interfaces which have been dropped. Stefan
Re: [Qemu-devel] [PATCH 1/3] Add blkmirror block driver
On Thu, Feb 23, 2012 at 4:14 PM, Stefan Hajnoczi stefa...@gmail.com wrote: By the way, this code does not build against qemu.git/master. It uses interfaces which have been dropped. Ah, I see you changed that in Patch 2/3. Please don't do that, it's hard to review and breaks git-bisect. Stefan
Re: [Qemu-devel] [PATCH 1/3] Add blkmirror block driver
- Original Message - From: Stefan Hajnoczi stefa...@gmail.com To: Federico Simoncelli fsimo...@redhat.com Cc: qemu-devel@nongnu.org, kw...@redhat.com, mtosa...@redhat.com Sent: Thursday, February 23, 2012 5:14:09 PM Subject: Re: [Qemu-devel] [PATCH 1/3] Add blkmirror block driver On Wed, Feb 22, 2012 at 5:13 PM, Federico Simoncelli fsimo...@redhat.com wrote: From: Marcelo Tosatti mtosa...@redhat.com Mirrored writes are used by live block copy. I think the right approach is to create a single blkmirror driver that also includes blkverify functionality. The code is basically the same except blkverify also compares reads - just use a flag to enable/disable that behavior. Feel free to rename the blkverify driver to blkmirror if you wish. By the way, this code does not build against qemu.git/master. It uses interfaces which have been dropped. Yes you also need: [PATCH 2/3] Update the blkmirror block driver Which was sent separately to facilitate the review for people who already reviewed this. -- Federico
Re: [Qemu-devel] [PATCH 1/3] Add blkmirror block driver
- Original Message - From: Stefan Hajnoczi stefa...@gmail.com To: Federico Simoncelli fsimo...@redhat.com Cc: qemu-devel@nongnu.org, kw...@redhat.com, mtosa...@redhat.com Sent: Thursday, February 23, 2012 5:18:41 PM Subject: Re: [Qemu-devel] [PATCH 1/3] Add blkmirror block driver On Thu, Feb 23, 2012 at 4:14 PM, Stefan Hajnoczi stefa...@gmail.com wrote: By the way, this code does not build against qemu.git/master. It uses interfaces which have been dropped. Ah, I see you changed that in Patch 2/3. Please don't do that, it's hard to review and breaks git-bisect. Squashing is easy (anyone could do it at the commit phase), reviewing is hard (if you hide your changes in someone else's code). -- Federico
Re: [Qemu-devel] [PATCH 1/3] Add blkmirror block driver
On Thu, Feb 23, 2012 at 4:20 PM, Federico Simoncelli fsimo...@redhat.com wrote: - Original Message - From: Stefan Hajnoczi stefa...@gmail.com To: Federico Simoncelli fsimo...@redhat.com Cc: qemu-devel@nongnu.org, kw...@redhat.com, mtosa...@redhat.com Sent: Thursday, February 23, 2012 5:18:41 PM Subject: Re: [Qemu-devel] [PATCH 1/3] Add blkmirror block driver On Thu, Feb 23, 2012 at 4:14 PM, Stefan Hajnoczi stefa...@gmail.com wrote: By the way, this code does not build against qemu.git/master. It uses interfaces which have been dropped. Ah, I see you changed that in Patch 2/3. Please don't do that, it's hard to review and breaks git-bisect. Squashing is easy (anyone could do it at the commit phase), reviewing is hard (if you hide your changes in someone else's code). I don't care if you or Marcelo wrote it, I want to see a coherent piece of code. The way to do it is to merge the g_malloc() and BlockDriver interface changes into this path. Then you have not snuck any significant changes into Marcelo's code. Keep the BDRV_O_NO_BACKING separate. Stefan
Re: [Qemu-devel] [PATCH 1/3] Add blkmirror block driver
- Original Message - From: Stefan Hajnoczi stefa...@gmail.com To: Federico Simoncelli fsimo...@redhat.com Cc: qemu-devel@nongnu.org, kw...@redhat.com, mtosa...@redhat.com Sent: Thursday, February 23, 2012 5:28:23 PM Subject: Re: [Qemu-devel] [PATCH 1/3] Add blkmirror block driver On Thu, Feb 23, 2012 at 4:20 PM, Federico Simoncelli fsimo...@redhat.com wrote: - Original Message - From: Stefan Hajnoczi stefa...@gmail.com To: Federico Simoncelli fsimo...@redhat.com Cc: qemu-devel@nongnu.org, kw...@redhat.com, mtosa...@redhat.com Sent: Thursday, February 23, 2012 5:18:41 PM Subject: Re: [Qemu-devel] [PATCH 1/3] Add blkmirror block driver On Thu, Feb 23, 2012 at 4:14 PM, Stefan Hajnoczi stefa...@gmail.com wrote: By the way, this code does not build against qemu.git/master. It uses interfaces which have been dropped. Ah, I see you changed that in Patch 2/3. Please don't do that, it's hard to review and breaks git-bisect. Squashing is easy (anyone could do it at the commit phase), reviewing is hard (if you hide your changes in someone else's code). I don't care if you or Marcelo wrote it, I want to see a coherent piece of code. The way to do it is to merge the g_malloc() and BlockDriver interface changes into this path. Then you have not snuck any significant changes into Marcelo's code. Well for example I'd have snuck the qemu_vmalloc/qemu_vfree mistake. Keep the BDRV_O_NO_BACKING separate. Ok, thanks. -- Federico
