Re: [Qemu-devel] [PATCH v2 2/3] qcow2: handle reopening bitmaps on bdrv_invalidate_cache

[Vladimir Sementsov-Ogievskiy]

22.12.2017 16:39, Kevin Wolf wrote:
> Am 12.12.2017 um 17:04 hat Vladimir Sementsov-Ogievskiy geschrieben:
> > Consider migration with shared storage. Persistent bitmaps are stored
> > on bdrv_inactivate. Then, on destination
> > process_incoming_migration_bh() calls bdrv_invalidate_cache_all() which
> > leads to qcow2_load_autoloading_dirty_bitmaps() which fails if bitmaps
> > are already loaded on destination start. In this case we should call
> > qcow2_reopen_bitmaps_rw instead.
> >
> > Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
> > Reviewed-by: John Snow <address@hidden>
> qcow2_invalidate_cache() calls qcow2_close() first, so why are there
> still any bitmaps loaded? Isn't this a bug? Do we leak bitmaps when a
> qcow2 image is closed?
>
> Kevin

Interesting point.

Now persistent dirty bitmaps are released at the end of 
bdrv_inactivate_recurse,
which is not called here. It was a separate patch

commit 615b5dcf2decbc5f0abb512d13d7e5db2385fa23
Author: Vladimir Sementsov-Ogievskiy <address@hidden>
Date:   Wed Jun 28 15:05:30 2017 +0300

    block: release persistent bitmaps on inactivate


May be it is more correct to release them immediately after storing, in
qcow2_inactivete. But it will not fix the issue, because qcow2_close will
call qcow2_inactivate only if (!(s->flags & BDRV_O_INACTIVE)), but it is
not the case.

or we can do like this, it fixes the new test:
  static void qcow2_close(BlockDriverState *bs)
{
      BDRVQcow2State *s = bs->opaque;
qemu_vfree(s->l1_table);
      /* else pre-write overlap checks in cache_destroy may crash */
      s->l1_table = NULL;

      if (!(s->flags & BDRV_O_INACTIVE)) {
qcow2_inactivate(bs);
}
+     bdrv_release_persistent_dirty_bitmaps(bs);


What do you think?

--
Best regards,
Vladimir