qemu-block
[Top][All Lists]
Advanced

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

Re: [PATCH v2 12/36] block: refactor bdrv_child* permission functions


From: Vladimir Sementsov-Ogievskiy
Subject: Re: [PATCH v2 12/36] block: refactor bdrv_child* permission functions
Date: Wed, 20 Jan 2021 12:56:19 +0300
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.6.1

20.01.2021 12:09, Kevin Wolf wrote:
Am 19.01.2021 um 19:30 hat Vladimir Sementsov-Ogievskiy geschrieben:
19.01.2021 21:09, Kevin Wolf wrote:
Am 27.11.2020 um 15:44 hat Vladimir Sementsov-Ogievskiy geschrieben:
Split out non-recursive parts, and refactor as block graph transaction
action.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
   block.c | 79 ++++++++++++++++++++++++++++++++++++++++++---------------
   1 file changed, 59 insertions(+), 20 deletions(-)

diff --git a/block.c b/block.c
index a756f3e8ad..7267b4a3e9 100644
--- a/block.c
+++ b/block.c
@@ -48,6 +48,7 @@
   #include "qemu/timer.h"
   #include "qemu/cutils.h"
   #include "qemu/id.h"
+#include "qemu/transactions.h"
   #include "block/coroutines.h"
   #ifdef CONFIG_BSD
@@ -2033,6 +2034,61 @@ static void bdrv_child_perm(BlockDriverState *bs, 
BlockDriverState *child_bs,
       }
   }
+static void bdrv_child_set_perm_commit(void *opaque)
+{
+    BdrvChild *c = opaque;
+
+    c->has_backup_perm = false;
+}
+
+static void bdrv_child_set_perm_abort(void *opaque)
+{
+    BdrvChild *c = opaque;
+    /*
+     * We may have child->has_backup_perm unset at this point, as in case of
+     * _check_ stage of permission update failure we may _check_ not the whole
+     * subtree.  Still, _abort_ is called on the whole subtree anyway.
+     */
+    if (c->has_backup_perm) {
+        c->perm = c->backup_perm;
+        c->shared_perm = c->backup_shared_perm;
+        c->has_backup_perm = false;
+    }
+}
+
+static TransactionActionDrv bdrv_child_set_pem_drv = {
+    .abort = bdrv_child_set_perm_abort,
+    .commit = bdrv_child_set_perm_commit,
+};
+
+/*
+ * With tran=NULL needs to be followed by direct call to either
+ * bdrv_child_set_perm_commit() or bdrv_child_set_perm_abort().
+ *
+ * With non-NUll tran needs to be followed by tran_abort() or tran_commit()

s/NUll/NULL/

+ * instead.
+ */
+static void bdrv_child_set_perm_safe(BdrvChild *c, uint64_t perm,
+                                     uint64_t shared, GSList **tran)
+{
+    if (!c->has_backup_perm) {
+        c->has_backup_perm = true;
+        c->backup_perm = c->perm;
+        c->backup_shared_perm = c->shared_perm;
+    }

This is the obvious refactoring at this point, and it's fine as such.

However, when you start to actually use tran (and in new places), it
means that I have to check that we can never end up here recursively
with a different tran.

I don't follow.. Which different tran do you mean?

Any really. At this point in the series, nothing passes a non-NULL tran
yet. When you add the first user, it is only a local transaction for a
single node. If something else called bdrv_child_set_perm_safe() on the
same node before the transaction has completed, the result would be
broken.

But this problem is preexisting: if someone call bdrv_child_set_perm twice on the 
same node during one update operation, c->backup* will be rewritten.


So reviewing/understanding this change (and actually developing it in
the first place) means going through all the code that ends up inside
the transaction and making sure that we never try to change permissions
for the same node a second time in any context.

I think we do it, when find same node several times during update. And that is fixed in 
"[PATCH v2 15/36] block: use topological sort for permission update", when we 
move to topological sorted list.



It would probably be much cleaner if the next patch moved the backup
state into the opaque struct for BdrvAction.

But old code which calls the function with tran=NULL can't use it..
Hmm, we can probably support both ways: c->backup_perm for callers
with tran=NULL and opaque struct for new style callers.

Hm, you're right about that... Maybe that's too much complication then.

What happens in the next patch is still understandable enough with the
way you currently have it. Let's see what it looks like with the rest.



--
Best regards,
Vladimir



reply via email to

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