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> >> (The rest looks good to me.) >> >> Max >> >>> ebitmap.compare(get_bitmap(bitmaps, drive0.device, 'bitmap0')) >>> >>> # 2 - Writes and Reference Backup >> >
signature.asc
Description: OpenPGP digital signature