Re: [Qemu-devel] [PATCH 1/3] Add blkmirror block driver

[Stefan Hajnoczi]

On Mon, Feb 27, 2012 at 1:47 PM, Paolo Bonzini <address@hidden> 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