Re: [PATCH v4 4/4] iotests: Test node/bitmap aliases during migration

[Vladimir Sementsov-Ogievskiy]

20.08.2020 16:17, Max Reitz wrote:
> On 20.08.20 03:58, Eric Blake wrote:
> > On 8/18/20 8:32 AM, Max Reitz wrote:
> > > Signed-off-by: Max Reitz <mreitz@redhat.com>
> > > ---
> > >    tests/qemu-iotests/300     | 595 +++++++++++++++++++++++++++++++++++++
> > >    tests/qemu-iotests/300.out |   5 +
> >
> > Rather sparse output (I hate debugging those sorts of outputs when the
> > test is failing).
>
> Hm.  I don’t know, the stack trace usually gives a good idea and
> ./check -d gives QMP context.
>
> The advantage of a sparse output is that we don’t need to adjust the
> reference output every time some optional field is added somewhere.
>
> > >    tests/qemu-iotests/group   |   1 +
> > >    3 files changed, 601 insertions(+)
> > >    create mode 100755 tests/qemu-iotests/300
> > >    create mode 100644 tests/qemu-iotests/300.out
> >
> > > +        # Dirty some random megabytes
> > > +        for _ in range(9):
> > > +            mb_ofs = random.randrange(1024)
> > > +            self.vm_a.hmp_qemu_io(self.src_node_name, f'write
> > > {mb_ofs}M 1M')
> >
> > It turns out that the discard operation likewise dirties the bitmap, but
> > slightly faster (see edb90bbd).  We could optimize it on top, but I'm
> > not going to require a micro-optimizing to get it in.  The test takes
> > about 12 seconds to run for me, but you didn't mark it as such in
> > 'group', so that's good; but it turns up a problem:
> >
> > 300      fail       [20:55:54] [20:56:06]                    output
> > mismatch (see 300.out.bad)
> > --- /home/eblake/qemu-tmp2/tests/qemu-iotests/300.out    2020-08-19
> > 20:53:11.087791988 -0500
> > +++ /home/eblake/qemu-tmp2/tests/qemu-iotests/300.out.bad    2020-08-19
> > 20:56:06.092428756 -0500
> > @@ -1,5 +1,41 @@
> > -.....................................
> > +WARNING:qemu.machine:qemu received signal 11; command:
> > "/home/eblake/qemu-tmp2/tests/qemu-iotests/../../x86_64-softmmu/qemu-system-x86_64
> > -display none -vga none -chardev
> > socket,id=mon,path=/tmp/tmp.qT831UThme/qemu-b-798452-monitor.sock -mon
> > chardev=mon,mode=control -qtest
> > unix:path=/tmp/tmp.qT831UThme/qemu-b-798452-qtest.sock -accel qtest
> > -nodefaults -display none -accel qtest -blockdev
> > node-name=node0,driver=null-co -incoming unix:/tmp/tmp.qT831UThme/mig_sock"
> > +.............FE.......................
> > +======================================================================
> > +ERROR: test_migratee_bitmap_is_not_mapped_on_dst
> > (__main__.TestBlockBitmapMappingErrors)
> > +----------------------------------------------------------------------
> > +Traceback (most recent call last):
> > +  File
> > "/home/eblake/qemu-tmp2/tests/qemu-iotests/../../python/qemu/machine.py", line
> > 435, in _do_shutdown
> > +    self._soft_shutdown(timeout, has_quit)
> > +  File
> > "/home/eblake/qemu-tmp2/tests/qemu-iotests/../../python/qemu/machine.py", line
> > 415, in _soft_shutdown
> > +    self._qmp.cmd('quit')
> > +  File
> > "/home/eblake/qemu-tmp2/tests/qemu-iotests/../../python/qemu/qmp.py",
> > line 266, in cmd
> > +    return self.cmd_obj(qmp_cmd)
> > +  File
> > "/home/eblake/qemu-tmp2/tests/qemu-iotests/../../python/qemu/qmp.py",
> > line 246, in cmd_obj
> > +    self.__sock.sendall(json.dumps(qmp_cmd).encode('utf-8'))
> > +BrokenPipeError: [Errno 32] Broken pipe
> > +
> > +The above exception was the direct cause of the following exception:
> > +
> > +Traceback (most recent call last):
> > +  File "300", line 76, in tearDown
> > +    self.vm_b.shutdown()
> > +  File
> > "/home/eblake/qemu-tmp2/tests/qemu-iotests/../../python/qemu/machine.py", line
> > 465, in shutdown
> > +    self._do_shutdown(timeout, has_quit)
> > +  File
> > "/home/eblake/qemu-tmp2/tests/qemu-iotests/../../python/qemu/machine.py", line
> > 438, in _do_shutdown
> > +    raise AbnormalShutdown("Could not perform graceful shutdown") \
> > +qemu.machine.AbnormalShutdown: Could not perform graceful shutdown
> > +
> > +======================================================================
> > +FAIL: test_migratee_bitmap_is_not_mapped_on_dst
> > (__main__.TestBlockBitmapMappingErrors)
> > +----------------------------------------------------------------------
> > +Traceback (most recent call last):
> > +  File "300", line 384, in test_migratee_bitmap_is_not_mapped_on_dst
> > +    self.migrate(False)
> > +  File "300", line 99, in migrate
> > +    self.assertEqual(self.vm_a.wait_migration('postmigrate'),
> > +AssertionError: False != True
> > +
> >   ----------------------------------------------------------------------
> >   Ran 37 tests
> >
> > -OK
> > +FAILED (failures=1, errors=1)
> >
> > I'm not sure why I'm seeing that, but it looks like you've got a bad
> > deref somewhere in the alias code.
>
> Ah, crap.
>
> This should fix it:
>
> diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c
> index 89cb16b12c..d407dfefea 100644
> --- a/migration/block-dirty-bitmap.c
> +++ b/migration/block-dirty-bitmap.c
> @@ -1091,7 +1091,9 @@ static int dirty_bitmap_load_header(QEMUFile *f,
> DBMLoadState *s,
>               } else {
>                   bitmap_name = s->bitmap_alias;
>               }
> +        }
>
> +        if (!s->cancelled) {
>               g_strlcpy(s->bitmap_name, bitmap_name, sizeof(s->bitmap_name));
>               s->bitmap = bdrv_find_dirty_bitmap(s->bs, s->bitmap_name);

That's correct thing to do

> I had this originally, and then I decided to drop that hunk just before
> sending v4 because I couldn’t see the point.  But we need it, because if
> the bitmap alias is unknown, the migration is cancelled, so we need to
> re-check s->cancalled after the alias lookup block.
>
> Would you be OK with squashing that into patch 1?

Ok for me.

--
Best regards,
Vladimir