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

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1.1: Type: text/plain, Size: 3885 bytes --]

On 16.07.19 18:58, John Snow wrote:
> 
> 
> On 7/16/19 8:04 AM, Max Reitz wrote:
>> On 16.07.19 02:01, John Snow wrote:
>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>> ---
>>>  tests/qemu-iotests/257     |   41 +-
>>>  tests/qemu-iotests/257.out | 3089 ++++++++++++++++++++++++++++++++++++
>>>  2 files changed, 3128 insertions(+), 2 deletions(-)
>>
>> This needs a %s/specify Bitmap sync mode/specify bitmap sync mode/.
>>
>>> diff --git a/tests/qemu-iotests/257 b/tests/qemu-iotests/257
>>> index 53ab31c92e..c2a72c577a 100755
>>> --- a/tests/qemu-iotests/257
>>> +++ b/tests/qemu-iotests/257
>>
>> [...]
>>
>>> @@ -393,7 +399,7 @@ def test_bitmap_sync(bsync_mode, msync_mode='bitmap', failure=None):
>>>              # group 1 gets cleared first, then group two gets written.
>>>              if ((bsync_mode == 'on-success' and not failure) or
>>>                  (bsync_mode == 'always')):
>>> -                ebitmap.clear_group(1)
>>> +                ebitmap.clear()
>>
>> Hmmm...  Why?
>>
> 
> From an order of operations standpoint, if we are here, we are expecting
> the bitmap to be synchronized. We can clear any existing data it holds,
> and then:
> 
>>>              ebitmap.dirty_group(2)
>>>  
> 
> Add new writes that occurred during the job; which only happen here in
> this callback.
> 
> (The old code cleared specifically only group 1, the new code is just
> more general. I wound up changing it for a version that didn't make it
> to the list, but this is still correct.)
> 
>>>          vm.run_job(job, auto_dismiss=True, auto_finalize=False,
>>> @@ -404,8 +410,19 @@ def test_bitmap_sync(bsync_mode, msync_mode='bitmap', failure=None):
>>>          log('')
>>>  
>>>          if bsync_mode == 'always' and failure == 'intermediate':
>>> +            # TOP treats anything allocated as dirty, expect to see:
>>> +            if msync_mode == 'top':
>>> +                ebitmap.dirty_group(0)
>>> +
> 
> Sorry, this code is definitely in the "cute" category...
> 
> If the failure was intermediate, we never call the pre-finalize callback
> above. So we know that the allocated regions of the file are only from
> groups 0 and 1.
> 
> So, HERE, we can mark the emulated bitmap's group 0 as dirty, to mimic
> what the copy_bitmap is going to have started the operation with.
> 
>>>              # We manage to copy one sector (one bit) before the error.
>>>              ebitmap.clear_bit(ebitmap.first_bit)
> 
> And then right here, we clear the first bit which we did copy out
> successfully. The emulated bitmap is now correct for sync=top.
> 
>>> +
>>> +            # Full returns all bits set except what was copied/skipped
>>> +            if msync_mode == 'full':
>>> +                fail_bit = ebitmap.first_bit
>>> +                ebitmap.clear()
>>> +                ebitmap.dirty_bits(range(fail_bit, SIZE // GRANULARITY))
>>> +
> 
> The full mode, though, is special. We cleared the first allocated bit
> just like for sync=top, but we take note of the second bit which is the
> one that caused the injected failure.
> 
> For both 'top' and 'full' modes here we're really using the ebitmap as
> an allocation record to inform what the output bitmap is going to look like.
> 
>>
>> So sync=top didn‘t copy anything?  Is that because it now errors out
>> before getting to copy something?
>>
> 
> The ebitmap.clear_bit(ebitmap.first_bit) triggers for top, too. The test
> output should hopefully make sense here.

I...   I have no idea how I missed the ebitmap.clear_bit().

So with the test output fixed:

Reviewed-by: Max Reitz <mreitz@redhat.com>

>> (The rest looks good to me.)
>>
>> Max
>>
>>>          ebitmap.compare(get_bitmap(bitmaps, drive0.device, 'bitmap0'))
>>>  
>>>          # 2 - Writes and Reference Backup
>>
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]