Re: [PATCH v4 15/18] iotests/image-fleecing: add test case with bitmap

[Vladimir Sementsov-Ogievskiy]

24.02.2022 15:58, Hanna Reitz wrote:
> On 16.02.22 20:46, Vladimir Sementsov-Ogievskiy wrote:
> > Note that reads zero areas (not dirty in the bitmap) fails, that's
> > correct.
> >
> > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> > ---
> >   tests/qemu-iotests/tests/image-fleecing     | 32 ++++++--
> >   tests/qemu-iotests/tests/image-fleecing.out | 84 +++++++++++++++++++++
> >   2 files changed, 108 insertions(+), 8 deletions(-)
>
> Looks good, just one general usage question:
>
> > diff --git a/tests/qemu-iotests/tests/image-fleecing 
> > b/tests/qemu-iotests/tests/image-fleecing
> > index 909fc0a7ad..33995612be 100755
> > --- a/tests/qemu-iotests/tests/image-fleecing
> > +++ b/tests/qemu-iotests/tests/image-fleecing
> > @@ -23,7 +23,7 @@
> >   # Creator/Owner: John Snow <jsnow@redhat.com>
> >   import iotests
> > -from iotests import log, qemu_img, qemu_io, qemu_io_silent
> > +from iotests import log, qemu_img, qemu_io, qemu_io_silent, 
> > qemu_io_pipe_and_status
> >   iotests.script_initialize(
> >       supported_fmts=['qcow2', 'qcow', 'qed', 'vmdk', 'vhdx', 'raw'],
> > @@ -50,11 +50,15 @@ remainder = [('0xd5', '0x108000',  '32k'), # Right-end of 
> > partial-left [1]
> >                ('0xcd', '0x3ff0000', '64k')] # patterns[3]
> >   def do_test(use_cbw, use_snapshot_access_filter, base_img_path,
> > -            fleece_img_path, nbd_sock_path, vm):
> > +            fleece_img_path, nbd_sock_path, vm,
> > +            bitmap=False):
> >       log('--- Setting up images ---')
> >       log('')
> >       assert qemu_img('create', '-f', iotests.imgfmt, base_img_path, '64M') == 0
> > +    if bitmap:
> > +        assert qemu_img('bitmap', '--add', base_img_path, 'bitmap0') == 0
> > +
> >       if use_snapshot_access_filter:
> >           assert use_cbw
> >           assert qemu_img('create', '-f', 'raw', fleece_img_path, '64M') == 0
> > @@ -106,12 +110,17 @@ def do_test(use_cbw, use_snapshot_access_filter, 
> > base_img_path,
> >       # Establish CBW from source to fleecing node
> >       if use_cbw:
> > -        log(vm.qmp('blockdev-add', {
> > +        fl_cbw = {
> >               'driver': 'copy-before-write',
> >               'node-name': 'fl-cbw',
> >               'file': src_node,
> >               'target': tmp_node
> > -        }))
> > +        }
> > +
> > +        if bitmap:
> > +            fl_cbw['bitmap'] = {'node': src_node, 'name': 'bitmap0'}
> > +
> > +        log(vm.qmp('blockdev-add', fl_cbw))
> >           log(vm.qmp('qom-set', path=qom_path, property='drive', 
> > value='fl-cbw'))
>
> This makes me wonder how exactly the @bitmap parameter is to be used. In this 
> case here, we use an active bitmap that tracks all writes, so it looks like a 
> case of trying to copy the changes since some previous checkpoint (as a 
> point-in-time state).  But if there are any writes between the blockdev-add and 
> the qom-set, then they will not be included in the CBW bitmap.  Is that fine?  
> Or is it perhaps even intentional?
>
> (Is the idea that one would use a transaction to disable the current bitmap 
> (say “A”), and add a new one (say “B”) at the same time, then use bitmap A for 
> the CBW filter, delete it after the backup, and then use B for the subsequent 
> backup?)

Hmm, good question. If we do this way, we break a point-in-time of backup.. 
We'll make a copy of disk in state of the moment of qom-set, but use an 
outdated copy of bitmap..

Good solution would do blockdev-add and qom-set in one transaction. But it's 
more possible to make transaction support for my proposed blockdev-replace, 
which should substitute qom-set in this scenario..

And supporting blockdev-add in transaction is not simple too.

With usual backup we simply do blockdev-backup and all needed bitmap 
manipulations in one transaction. With filter, actual backup start is qom-set 
(or blockdev-replace), not blockdev-add.. But we can't pass bitmap parameter to 
qom-set or blockdev-replace.

We probably could support blockdev-reopen in transaction, and change the bitmap 
in reopen.. But that seems wrong to me: we should not use reopen in scenario 
where we've just created this temporary node with all arguments we want.

Keeping in mind my recent series that introduces a kind of transaction for 
bdrv_close, may be the best and more native way is really support blockdev-add 
and blockdev-del in transaction.


The only alternative way I see is to not copy the user-given bitmap, but use 
exactly what user gives. This way, we do the following:

1. User give active bitmap A to cbw_open, so bitmap continue to track dirtiness.
2. User start a new dirty bitmap B
3. On filter insertion, we have a good bitmap with all needed dirty bits
4. After filter insertion, user stops tracking in bitmap A (disable it)

This way, we'll not lose any data. The drawback, is that we may copy some extra 
data, that was not actually dirty at point [3] (because we disable bitmap A 
after it, not in transaction). As well, bitmap B which will be used for next 
incremental backup will probably contain some extra dirty bits. That's not bad, 
but that's not an ideal architecture.

--
Best regards,
Vladimir