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

[Max Reitz]

On 10.07.19 21:00, John Snow wrote:
> 
> 
> 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.

Now that you mention it...  I didn’t realize that.  Yes, you’re right.

> 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.)

No, I’m with you on that one.

Max

>>> +                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
>> [...]
>>

signature.asc
Description: OpenPGP digital signature