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

From: John Snow <jsnow@redhat.com>
To: Max Reitz <mreitz@redhat.com>,
	qemu-block@nongnu.org, qemu-devel@nongnu.org
Cc: Kevin Wolf <kwolf@redhat.com>,
	vsementsov@virtuozzo.com, Markus Armbruster <armbru@redhat.com>
Subject: Re: [Qemu-devel] [PATCH v2 11/11] iotests/257: test traditional sync modes
Date: Tue, 16 Jul 2019 12:58:47 -0400	[thread overview]
Message-ID: <91af10c4-e4ff-df49-d1e9-31ea210fc637@redhat.com> (raw)
In-Reply-To: <9cf9c85d-c41e-123c-07cb-81c453363aea@redhat.com>

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

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.

> (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: 833 bytes --]