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 <js...@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 <mre...@redhat.com>
> 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
