Re: [Qemu-devel] [RFC PATCH 2/4] block: add live block commit functionality

[Paolo Bonzini]

Il 01/08/2012 08:32, Kevin Wolf ha scritto:
>>>> >>> +enum {
>>>> >>> +    /*
>>>> >>> +     * Size of data buffer for populating the image file.  This 
>>>> >>> should be large
>>>> >>> +     * enough to process multiple clusters in a single call, so that 
>>>> >>> populating
>>>> >>> +     * contiguous regions of the image is efficient.
>>>> >>> +     */
>>>> >>> +    COMMIT_BUFFER_SIZE = 512 * 1024, /* in bytes */
>>>> >>> +};
>>> >>
>>> >> Paolo's latest round of patches got to the point of making this
>>> >> configurable for drive-mirror; is that something you should be copying 
>>> >> here?
>> > 
>> > Yes
> Though its use is very limited for live commit. For the mirror it's
> important because a larger number can mean that more data is
> unnecessarily written, and the target can become larger than the source.

Note that the latest version of mirroring has _two_ knobs:

1) granularity is what decides how much data could be written
unnecessarily, because of the dirty bitmap.

2) buffer size is what decides how much I/O is in flight at one time.

The default values are resp. the cluster size (or 64K for raw) and 10M.
 The two together give mirroring some self-tuning capability.  For
example in the first part of the mirroring you will likely proceed in
10M chunks with no concurrency; once you're synchronized, you'll
probably send several chunks, perhaps all 64K if the guest does random
writes.

Live commit as it is done now doesn't need any of this complication; it
is just a background operation that does not need to compete with the
guest.  So using a larger buffer is indeed always better, and 512K is a
nice intermediate value between mirroring's 64K and 10M extremes.

> For live commit, I think using a larger buffer is always better.
> 
> Hm, part of the difference is that I assume that commit uses
> bdrv_is_allocated() to check whether some data must really be copied.
> But then, there's no reason why mirroring couldn't do that as well. Paolo?

We copy a cluster at a time, and that's also the resolution of
bdrv_is_allocated so we wouldn't gain anything.  Nice idea though, I had
to mull about it to find the flaw. :)

Paolo