bdrv_append is not very good for inserting filters: it does extra
permission update as part of bdrv_set_backing_hd(). During this update
filter may conflict with other parents of top_bs.

Instead, let's first do all graph modifications and after it update
permissions.

Note: bdrv_append() is still only works for backing-child based
filters. It's something to improve later.

It simplifies the fact that bdrv_append() used to append new nodes,
without backing child. Let's add an assertion.

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

diff --git a/block.c b/block.c
index 02da1a90bc..7094922509 100644
--- a/block.c
+++ b/block.c
@@ -4998,22 +4998,28 @@ int bdrv_replace_node(BlockDriverState *from, BlockDriverState *to,
 int bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top,
                 Error **errp)
 {
-    Error *local_err = NULL;
+    int ret;
+    GSList *tran = NULL;
 
-    bdrv_set_backing_hd(bs_new, bs_top, &local_err);
-    if (local_err) {
-        error_propagate(errp, local_err);
-        return -EPERM;
+    assert(!bs_new->backing);
+
+    ret = bdrv_attach_child_noperm(bs_new, bs_top, "backing",
+                                   &child_of_bds, bdrv_backing_role(bs_new),
+                                   &bs_new->backing, &tran, errp);
+    if (ret < 0) {
+        goto out;
     }
 
-    bdrv_replace_node(bs_top, bs_new, &local_err);
-    if (local_err) {
-        error_propagate(errp, local_err);
-        bdrv_set_backing_hd(bs_new, NULL, &error_abort);
-        return -EPERM;
+    ret = bdrv_replace_node_noperm(bs_top, bs_new, true, &tran, errp);
+    if (ret < 0) {
+        goto out;
     }
 
-    return 0;
+    ret = bdrv_refresh_perms(bs_new, errp);
+out:
+    tran_finalize(tran, ret);
+
+    return ret;
 }
 
 static void bdrv_delete(BlockDriverState *bs)
-- 
2.21.3

Am 27.11.2020 um 15:45 hat Vladimir Sementsov-Ogievskiy geschrieben:
> bdrv_append is not very good for inserting filters: it does extra
> permission update as part of bdrv_set_backing_hd(). During this update
> filter may conflict with other parents of top_bs.
> 
> Instead, let's first do all graph modifications and after it update
> permissions.

This sounds like it fixes a bug. If so, should we have a test like for
the other cases fixed by this series?

> Note: bdrv_append() is still only works for backing-child based
> filters. It's something to improve later.
> 
> It simplifies the fact that bdrv_append() used to append new nodes,
> without backing child. Let's add an assertion.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  block.c | 28 +++++++++++++++++-----------
>  1 file changed, 17 insertions(+), 11 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 02da1a90bc..7094922509 100644
> --- a/block.c
> +++ b/block.c
> @@ -4998,22 +4998,28 @@ int bdrv_replace_node(BlockDriverState *from, BlockDriverState *to,
>  int bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top,
>                  Error **errp)
>  {
> -    Error *local_err = NULL;
> +    int ret;
> +    GSList *tran = NULL;
>  
> -    bdrv_set_backing_hd(bs_new, bs_top, &local_err);
> -    if (local_err) {
> -        error_propagate(errp, local_err);
> -        return -EPERM;
> +    assert(!bs_new->backing);
> +
> +    ret = bdrv_attach_child_noperm(bs_new, bs_top, "backing",
> +                                   &child_of_bds, bdrv_backing_role(bs_new),
> +                                   &bs_new->backing, &tran, errp);
> +    if (ret < 0) {
> +        goto out;
>      }

I don't think changing bs->backing without bdrv_set_backing_hd() is
correct at the moment. We lose a few things:

1. The bdrv_is_backing_chain_frozen() check
2. Updating backing_hd->inherits_from if necessary
3. bdrv_refresh_limits()

If I'm not missing anything, all of these are needed in the context of
bdrv_append().

> -    bdrv_replace_node(bs_top, bs_new, &local_err);
> -    if (local_err) {
> -        error_propagate(errp, local_err);
> -        bdrv_set_backing_hd(bs_new, NULL, &error_abort);
> -        return -EPERM;
> +    ret = bdrv_replace_node_noperm(bs_top, bs_new, true, &tran, errp);
> +    if (ret < 0) {
> +        goto out;
>      }
>  
> -    return 0;
> +    ret = bdrv_refresh_perms(bs_new, errp);
> +out:
> +    tran_finalize(tran, ret);
> +
> +    return ret;
>  }

Kevin

04.02.2021 00:33, Kevin Wolf wrote:
> Am 27.11.2020 um 15:45 hat Vladimir Sementsov-Ogievskiy geschrieben:
>> bdrv_append is not very good for inserting filters: it does extra
>> permission update as part of bdrv_set_backing_hd(). During this update
>> filter may conflict with other parents of top_bs.
>>
>> Instead, let's first do all graph modifications and after it update
>> permissions.
> 
> This sounds like it fixes a bug. If so, should we have a test like for
> the other cases fixed by this series?

Hm. I considered it mostly like a lack not a bug. We just have to workaround this lack by "inactive" mode of filters. But adding a test is good idea anyway. Will do.

> 
>> Note: bdrv_append() is still only works for backing-child based
>> filters. It's something to improve later.
>>
>> It simplifies the fact that bdrv_append() used to append new nodes,
>> without backing child. Let's add an assertion.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   block.c | 28 +++++++++++++++++-----------
>>   1 file changed, 17 insertions(+), 11 deletions(-)
>>
>> diff --git a/block.c b/block.c
>> index 02da1a90bc..7094922509 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -4998,22 +4998,28 @@ int bdrv_replace_node(BlockDriverState *from, BlockDriverState *to,
>>   int bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top,
>>                   Error **errp)
>>   {
>> -    Error *local_err = NULL;
>> +    int ret;
>> +    GSList *tran = NULL;
>>   
>> -    bdrv_set_backing_hd(bs_new, bs_top, &local_err);
>> -    if (local_err) {
>> -        error_propagate(errp, local_err);
>> -        return -EPERM;
>> +    assert(!bs_new->backing);
>> +
>> +    ret = bdrv_attach_child_noperm(bs_new, bs_top, "backing",
>> +                                   &child_of_bds, bdrv_backing_role(bs_new),
>> +                                   &bs_new->backing, &tran, errp);
>> +    if (ret < 0) {
>> +        goto out;
>>       }
> 
> I don't think changing bs->backing without bdrv_set_backing_hd() is
> correct at the moment. We lose a few things:
> 
> 1. The bdrv_is_backing_chain_frozen() check
> 2. Updating backing_hd->inherits_from if necessary
> 3. bdrv_refresh_limits()
> 
> If I'm not missing anything, all of these are needed in the context of
> bdrv_append().

I decided that bdrv_append() is only for appending new nodes, so frozen and inherts_from checks are not needed. And I've added assert(!bs_new->backing)...

Checking this now:

- appending filters is obvious
- bdrv_append_temp_snapshot() creates new qcow2 node based on tmp file, don't see any backing initialization (and it would be rather strange)
- external_snapshot_prepare() do check if (bdrv_cow_child(state->new_bs)) {  error-out }

So everything is OK. I should describe it in commit message and add a comment to bdrv_append.

> 
>> -    bdrv_replace_node(bs_top, bs_new, &local_err);
>> -    if (local_err) {
>> -        error_propagate(errp, local_err);
>> -        bdrv_set_backing_hd(bs_new, NULL, &error_abort);
>> -        return -EPERM;
>> +    ret = bdrv_replace_node_noperm(bs_top, bs_new, true, &tran, errp);
>> +    if (ret < 0) {
>> +        goto out;
>>       }
>>   
>> -    return 0;
>> +    ret = bdrv_refresh_perms(bs_new, errp);
>> +out:
>> +    tran_finalize(tran, ret);
>> +
>> +    return ret;
>>   }
> 
> Kevin
> 


-- 
Best regards,
Vladimir

Am 04.02.2021 um 09:30 hat Vladimir Sementsov-Ogievskiy geschrieben:
> 04.02.2021 00:33, Kevin Wolf wrote:
> > Am 27.11.2020 um 15:45 hat Vladimir Sementsov-Ogievskiy geschrieben:
> > >   int bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top,
> > >                   Error **errp)
> > >   {
> > > -    Error *local_err = NULL;
> > > +    int ret;
> > > +    GSList *tran = NULL;
> > > -    bdrv_set_backing_hd(bs_new, bs_top, &local_err);
> > > -    if (local_err) {
> > > -        error_propagate(errp, local_err);
> > > -        return -EPERM;
> > > +    assert(!bs_new->backing);
> > > +
> > > +    ret = bdrv_attach_child_noperm(bs_new, bs_top, "backing",
> > > +                                   &child_of_bds, bdrv_backing_role(bs_new),
> > > +                                   &bs_new->backing, &tran, errp);
> > > +    if (ret < 0) {
> > > +        goto out;
> > >       }
> > 
> > I don't think changing bs->backing without bdrv_set_backing_hd() is
> > correct at the moment. We lose a few things:
> > 
> > 1. The bdrv_is_backing_chain_frozen() check
> > 2. Updating backing_hd->inherits_from if necessary
> > 3. bdrv_refresh_limits()
> > 
> > If I'm not missing anything, all of these are needed in the context of
> > bdrv_append().
> 
> I decided that bdrv_append() is only for appending new nodes, so
> frozen and inherts_from checks are not needed. And I've added
> assert(!bs_new->backing)...
> 
> Checking this now:
> 
> - appending filters is obvious
> - bdrv_append_temp_snapshot() creates new qcow2 node based on tmp
>   file, don't see any backing initialization (and it would be rather
>   strange)

Yes, the internal uses are obviously unproblematic for the frozen check.

> - external_snapshot_prepare() do check if
>   (bdrv_cow_child(state->new_bs)) {  error-out }

Ok, the only thing bdrv_set_backing_hd() can and must check is whether
the link to the old backing file was frozen, and we know that we don't
have an old backing file. Makes sense.

Same thing for inherits_from, we only do this if the the new backing
file (i.e. the old active layer for bdrv_append) was already in the
backing chain of the new node.

> So everything is OK. I should describe it in commit message and add a
> comment to bdrv_append.

What about bdrv_refresh_limits()? The node gains a new backing file, so
I think the limits could change.

Ideally, bdrv_child_cb_attach/detach() would take care of this, but at
the moment they don't.

Kevin

04.02.2021 12:05, Kevin Wolf wrote:
> Am 04.02.2021 um 09:30 hat Vladimir Sementsov-Ogievskiy geschrieben:
>> 04.02.2021 00:33, Kevin Wolf wrote:
>>> Am 27.11.2020 um 15:45 hat Vladimir Sementsov-Ogievskiy geschrieben:
>>>>    int bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top,
>>>>                    Error **errp)
>>>>    {
>>>> -    Error *local_err = NULL;
>>>> +    int ret;
>>>> +    GSList *tran = NULL;
>>>> -    bdrv_set_backing_hd(bs_new, bs_top, &local_err);
>>>> -    if (local_err) {
>>>> -        error_propagate(errp, local_err);
>>>> -        return -EPERM;
>>>> +    assert(!bs_new->backing);
>>>> +
>>>> +    ret = bdrv_attach_child_noperm(bs_new, bs_top, "backing",
>>>> +                                   &child_of_bds, bdrv_backing_role(bs_new),
>>>> +                                   &bs_new->backing, &tran, errp);
>>>> +    if (ret < 0) {
>>>> +        goto out;
>>>>        }
>>>
>>> I don't think changing bs->backing without bdrv_set_backing_hd() is
>>> correct at the moment. We lose a few things:
>>>
>>> 1. The bdrv_is_backing_chain_frozen() check
>>> 2. Updating backing_hd->inherits_from if necessary
>>> 3. bdrv_refresh_limits()
>>>
>>> If I'm not missing anything, all of these are needed in the context of
>>> bdrv_append().
>>
>> I decided that bdrv_append() is only for appending new nodes, so
>> frozen and inherts_from checks are not needed. And I've added
>> assert(!bs_new->backing)...
>>
>> Checking this now:
>>
>> - appending filters is obvious
>> - bdrv_append_temp_snapshot() creates new qcow2 node based on tmp
>>    file, don't see any backing initialization (and it would be rather
>>    strange)
> 
> Yes, the internal uses are obviously unproblematic for the frozen check.
> 
>> - external_snapshot_prepare() do check if
>>    (bdrv_cow_child(state->new_bs)) {  error-out }
> 
> Ok, the only thing bdrv_set_backing_hd() can and must check is whether
> the link to the old backing file was frozen, and we know that we don't
> have an old backing file. Makes sense.
> 
> Same thing for inherits_from, we only do this if the the new backing
> file (i.e. the old active layer for bdrv_append) was already in the
> backing chain of the new node.
> 
>> So everything is OK. I should describe it in commit message and add a
>> comment to bdrv_append.
> 
> What about bdrv_refresh_limits()? The node gains a new backing file, so
> I think the limits could change.
> 
> Ideally, bdrv_child_cb_attach/detach() would take care of this, but at
> the moment they don't.
> 

when answering I thought that it is called at the end of a function. But I both forget to write it in the answer and was wrong :) As it's actually bdrv_refresh_perms().  I'll add call of bdrv_refresh_limits()


-- 
Best regards,
Vladimir