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

[John Snow]

On 7/17/19 5:50 AM, Max Reitz wrote:
> 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 <address@hidden>
>>>> ---
>>>>  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 <address@hidden>
> 

Whoops. I definitely fixed that typo *after* I ran my tests and didn't
re-run them.

>>> (The rest looks good to me.)
>>>
>>> Max
>>>

Thank you for your reviews!

signature.asc
Description: OpenPGP digital signature