qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

[Qemu-devel] ping Re: loading bitmaps in invalidate_cache fails


From: Vladimir Sementsov-Ogievskiy
Subject: [Qemu-devel] ping Re: loading bitmaps in invalidate_cache fails
Date: Mon, 30 Oct 2017 17:55:29 +0300
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.4.0

Hi!

Any ideas?

05.10.2017 12:41, Vladimir Sementsov-Ogievskiy wrote:
12.09.2017 12:46, Kevin Wolf wrote:
Am 11.09.2017 um 18:51 hat Vladimir Sementsov-Ogievskiy geschrieben:
Hi Kevin!

I'm confused with relations of permissions and invalidation, can you please
help?

Now dirty bitmaps are loaded in invalidate_cache. Here is a problem with
migration:

1. destination starts (inactive)

2. load bitmaps readonly

...

3. invalidate_cache: here we should make our loaded bitmaps RW, ie set
BdrvDirtyBitmap->readonly

   to false and set IN_USE bit in the image. But the latter goes into
"bdrv_aligned_pwritev: Assertion `child->perm & BLK_PERM_WRITE' failed",

   because in bdrv_invalidate_cache we call bdrv_set_perm after
drv->bdrv_invalidate_cache.


What is the true way of fixing this?
It's all still a bit of a mess. :-(

I think it makes a lot of sense that we should activate the lower layers
first, so the order in bdrv_invalidate_cache() looks wrong. It should be
something like this:

1. invalidate_cache() for the children
2. Update permissions for non-BDRV_O_INACTIVE
3. Call drv->bdrv_invalidate_cache()

I'm currently working on some fixes related to bdrv_reopen() where
things become tricky because the updated permissions shouldn't depend on
the current state, but on the state after the operation has finished.

You get something similar here, but I think just making sure that we
clear BDRV_O_INACTIVE before updating the permissions is enough here.
The only thing to be careful is that in error cases, we need to revert
both the flag and the permissions then.

Kevin

I have experimented a bit. Permission for qcow2 bs is actually set in "parent->role->activate(parent, &local_err);",

so a patch like this fixes the bug (of course, some roll back of set_perm and activate should be added to error path)

@@ -4126,21 +4129,6 @@ void bdrv_invalidate_cache(BlockDriverState *bs, Error **errp)
     }

     bs->open_flags &= ~BDRV_O_INACTIVE;
-    if (bs->drv->bdrv_invalidate_cache) {
-        bs->drv->bdrv_invalidate_cache(bs, &local_err);
-        if (local_err) {
-            bs->open_flags |= BDRV_O_INACTIVE;
-            error_propagate(errp, local_err);
-            return;
-        }
-    }
-
-    ret = refresh_total_sectors(bs, bs->total_sectors);
-    if (ret < 0) {
-        bs->open_flags |= BDRV_O_INACTIVE;
-        error_setg_errno(errp, -ret, "Could not refresh total sector count");
-        return;
-    }

     /* Update permissions, they may differ for inactive nodes */
     bdrv_get_cumulative_perm(bs, &perm, &shared_perm);
@@ -4161,6 +4149,22 @@ void bdrv_invalidate_cache(BlockDriverState *bs, Error **errp)
             }
         }
     }
+
+    if (bs->drv->bdrv_invalidate_cache) {
+        bs->drv->bdrv_invalidate_cache(bs, &local_err);
+        if (local_err) {
+            bs->open_flags |= BDRV_O_INACTIVE;
+            error_propagate(errp, local_err);
+            return;
+        }
+    }
+
+    ret = refresh_total_sectors(bs, bs->total_sectors);
+    if (ret < 0) {
+        bs->open_flags |= BDRV_O_INACTIVE;
+        error_setg_errno(errp, -ret, "Could not refresh total sector count");
+        return;
+    }
 }


however, I doubt that it is safe to call ->activate before invalidate_cache... I have no more ideas, just a patch that shows a regression :(



--
Best regards,
Vladimir




reply via email to

[Prev in Thread] Current Thread [Next in Thread]