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()
+ * 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;
+    }
+    /*
+     * Note: it's OK if c->has_backup_perm was already set, as we can find the
+     * same c twice during check_perm procedure
+     */
+
+    c->perm = perm;
+    c->shared_perm = shared;
+
+    if (tran) {
+        tran_prepend(tran, &bdrv_child_set_pem_drv, c);
+    }
+}
+
 /*
  * Check whether permissions on this node can be changed in a way that
  * @cumulative_perms and @cumulative_shared_perms are the new cumulative
@@ -2298,37 +2354,20 @@ static int bdrv_child_check_perm(BdrvChild *c, BlockReopenQueue *q,
         return ret;
     }
 
-    if (!c->has_backup_perm) {
-        c->has_backup_perm = true;
-        c->backup_perm = c->perm;
-        c->backup_shared_perm = c->shared_perm;
-    }
-    /*
-     * Note: it's OK if c->has_backup_perm was already set, as we can find the
-     * same child twice during check_perm procedure
-     */
-
-    c->perm = perm;
-    c->shared_perm = shared;
+    bdrv_child_set_perm_safe(c, perm, shared, NULL);
 
     return 0;
 }
 
 static void bdrv_child_set_perm(BdrvChild *c)
 {
-    c->has_backup_perm = false;
-
+    bdrv_child_set_perm_commit(c);
     bdrv_set_perm(c->bs);
 }
 
 static void bdrv_child_abort_perm_update(BdrvChild *c)
 {
-    if (c->has_backup_perm) {
-        c->perm = c->backup_perm;
-        c->shared_perm = c->backup_shared_perm;
-        c->has_backup_perm = false;
-    }
-
+    bdrv_child_set_perm_abort(c);
     bdrv_abort_perm_update(c->bs);
 }
 
-- 
2.21.3

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.

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

Kevin

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?

> 
> 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.


-- 
Best regards,
Vladimir

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.

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.

> > 
> > 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.

Kevin

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

Am 20.01.2021 um 10:56 hat Vladimir Sementsov-Ogievskiy geschrieben:
> 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.

Ah, fair. Knowing that the state is broken before this patch makes
things easier in a way...

Kevin