After calling qcow2_inactivate(), all qcow2 caches must be flushed, but this
may not happen, because the last call qcow2_store_persistent_dirty_bitmaps()
can lead to marking l2/refcont cache as dirty.

Let's move qcow2_store_persistent_dirty_bitmaps() before the caсhe flushing
to fix it.

Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com>
---
 block/qcow2.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index a3679c69e8..dcf49084c5 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -2037,6 +2037,14 @@ static int qcow2_inactivate(BlockDriverState *bs)
     int ret, result = 0;
     Error *local_err = NULL;
 
+    qcow2_store_persistent_dirty_bitmaps(bs, &local_err);
+    if (local_err != NULL) {
+        result = -EINVAL;
+        error_report_err(local_err);
+        error_report("Persistent bitmaps are lost for node '%s'",
+                     bdrv_get_device_or_node_name(bs));
+    }
+
     ret = qcow2_cache_flush(bs, s->l2_table_cache);
     if (ret) {
         result = ret;
@@ -2051,14 +2059,6 @@ static int qcow2_inactivate(BlockDriverState *bs)
                      strerror(-ret));
     }
 
-    qcow2_store_persistent_dirty_bitmaps(bs, &local_err);
-    if (local_err != NULL) {
-        result = -EINVAL;
-        error_report_err(local_err);
-        error_report("Persistent bitmaps are lost for node '%s'",
-                     bdrv_get_device_or_node_name(bs));
-    }
-
     if (result == 0) {
         qcow2_mark_clean(bs);
     }
-- 
2.14.1

On 09/04/2017 05:18 AM, Pavel Butsykin wrote:
> After calling qcow2_inactivate(), all qcow2 caches must be flushed, but this
> may not happen, because the last call qcow2_store_persistent_dirty_bitmaps()
> can lead to marking l2/refcont cache as dirty.
> 
> Let's move qcow2_store_persistent_dirty_bitmaps() before the caсhe flushing
> to fix it.
> 
> Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com>
> ---
>  block/qcow2.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 

Should this cc: qemu-stable?

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

On 05.09.2017 22:30, Eric Blake wrote:
> On 09/04/2017 05:18 AM, Pavel Butsykin wrote:
>> After calling qcow2_inactivate(), all qcow2 caches must be flushed, but this
>> may not happen, because the last call qcow2_store_persistent_dirty_bitmaps()
>> can lead to marking l2/refcont cache as dirty.
>>
>> Let's move qcow2_store_persistent_dirty_bitmaps() before the caсhe flushing
>> to fix it.
>>
>> Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com>
>> ---
>>   block/qcow2.c | 16 ++++++++--------
>>   1 file changed, 8 insertions(+), 8 deletions(-)
>>
> 
> Should this cc: qemu-stable?

The latest stable branch (2.8?) doesn't contain the persistent dirty bitmap.

> Reviewed-by: Eric Blake <eblake@redhat.com>
> 

Am 06.09.2017 um 10:19 hat Pavel Butsykin geschrieben:
> On 05.09.2017 22:30, Eric Blake wrote:
> > On 09/04/2017 05:18 AM, Pavel Butsykin wrote:
> > > After calling qcow2_inactivate(), all qcow2 caches must be flushed, but this
> > > may not happen, because the last call qcow2_store_persistent_dirty_bitmaps()
> > > can lead to marking l2/refcont cache as dirty.
> > > 
> > > Let's move qcow2_store_persistent_dirty_bitmaps() before the caсhe flushing
> > > to fix it.
> > > 
> > > Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com>
> > > ---
> > >   block/qcow2.c | 16 ++++++++--------
> > >   1 file changed, 8 insertions(+), 8 deletions(-)
> > > 
> > 
> > Should this cc: qemu-stable?
> 
> The latest stable branch (2.8?) doesn't contain the persistent dirty bitmap.

Cc: qemu-stable would now be for qemu 2.10.1, which I think does need
the fix. I'm adding the tag.

Thanks, applied to the block branch.

Kevin