Re: [Qemu-devel] [PATCH 8/8] iotests/257: test traditional sync modes

[John Snow]

On 7/10/19 1:14 PM, Max Reitz wrote:
> On 10.07.19 03:05, John Snow wrote:
>> Signed-off-by: John Snow <address@hidden>
>> ---
>>  tests/qemu-iotests/257     |   31 +
>>  tests/qemu-iotests/257.out | 3089 ++++++++++++++++++++++++++++++++++++
>>  2 files changed, 3120 insertions(+)
> 
> Oof.
> 

Yeah, it's... a lot of test output. We probably shouldn't count the
reference test output against any kind of SLOC metrics.

>> diff --git a/tests/qemu-iotests/257 b/tests/qemu-iotests/257
>> index de8707cb19..8de1c4da19 100755
>> --- a/tests/qemu-iotests/257
>> +++ b/tests/qemu-iotests/257
> 
> [...]
> 
>> @@ -410,6 +416,11 @@ def test_bitmap_sync(bsync_mode, msync_mode='bitmap', 
>> failure=None):
>>          if bsync_mode == 'always' and failure == 'intermediate':
>>              # We manage to copy one sector (one bit) before the error.
>>              ebitmap.clear_bit(ebitmap.first_bit)
>> +            if msync_mode in ('full', 'top'):
>> +                # These modes return all bits set except what was 
>> copied/skipped
> 
> Hm.  How useful is bitmap support for 'top' then, anyway?  That means
> that if you want to resume a top backup, you always have to resume it
> like it was a full backup.  Which sounds kind of useless.
> 
> Max
> 

Good point!

I think this can be fixed by doing an initialization pass of the
copy_bitmap when sync=top to set only the allocated regions in the bitmap.

This means that the write notifier won't copy out regions that are
written to that weren't already in the top layer. I believe this is
actually a bugfix; the data we'd copy out in such cases is actually in
the backing layer and shouldn't be copied with sync=top.

So this would have two effects:
(1) sync=top gets a little more judicious about what it copies out on
sync=top, and
(2) the bitmap return value is more meaningful again.

This doesn't touch sync=none at all, which needs more invasive fixes if
we wanted it to have useful bitmap return values (it needs to
differentiate the idea between must-copy and can-copy, and I still don't
know if this is worthwhile to do, so until I hear otherwise, I'm not gonna.)

>> +                fail_bit = ebitmap.first_bit
>> +                ebitmap.clear()
>> +                ebitmap.dirty_bits(range(fail_bit, SIZE // GRANULARITY))
>>          ebitmap.compare(get_bitmap(bitmaps, drive0.device, 'bitmap0'))
>>  
>>          # 2 - Writes and Reference Backup
> [...]
>