Re: [Qemu-devel] bitmaps branch rebase

[Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>]

08.08.2019 0:45, John Snow wrote:
> FYI: I rebased jsnow/bitmaps on top of kwolf/block-next, itself based on
> top of v4.1.0-rc4.
> 
> I'll post this along with the eventual pull request, but here's the
> diffstat against the published patches:
> 
> 011/33:[0003] [FC] 'block/backup: upgrade copy_bitmap to BdrvDirtyBitmap'
> 016/33:[----] [-C] 'iotests: Add virtio-scsi device helper'
> 017/33:[0002] [FC] 'iotests: add test 257 for bitmap-mode backups'
> 030/33:[0001] [FC] 'block/backup: teach TOP to never copy unallocated
> regions'
> 032/33:[0018] [FC] 'iotests/257: test traditional sync modes'
> 
> 11: A new hbitmap call was added upstream, changed to
> bdrv_dirty_bitmap_next_zero.
> 16: Context-only (self.has_quit is new context in 040)
> 17: Removed 'auto' to follow upstream trends in iotest fashion
> 30: Remove ret = -ECANCELED as agreed on-list;
>      Context changes for dirty_end patches
> 32: Fix capitalization in test, as mentioned on list.
> 
> I think the changes are actually fairly minimal and translate fairly
> directly; let's review the rebase on-list in response to the PULL mails
> when I send them.
> 


There is a bug in "block/backup: teach TOP to never copy unallocated regions":


 > @@ -256,6 +287,15 @@ static int coroutine_fn backup_do_cow(BackupBlockJob *job,
 >              continue; /* already copied */
 >          }
 >
 > +        if (job->initializing_bitmap) {
 > +            ret = backup_bitmap_reset_unallocated(job, start, &skip_bytes);
 > +            if (ret == 0) {
 > +                trace_backup_do_cow_skip_range(job, start, skip_bytes);
 > +                start += skip_bytes;
 > +                continue;
 > +            }

assume ret == 1, so we see skip_bytes of allocated bytes

 > +        }
 > +
 >          dirty_end = bdrv_dirty_bitmap_next_zero(job->copy_bitmap, start,
 >                                                  (end - start));
 >          if (dirty_end < 0) {
 >

but then, we may copy more than skip_bytes, i.e. touch following possibly unallocated area.

===

Also, if want to fix it anyway, I think it's better to make additional while loop before this one
and reset all unallocated from start to end, otherwise we may call block_status for every cluster
on each loop iteration, even if the first call returns skip_bytes >= (end - start).

-- 
Best regards,
Vladimir