[v2,1/3] block: Allow replacement of a BDS by its overlay

change_parent_backing_link() asserts that the BDS to be replaced is not
used as a backing file. However, we may want to replace a BDS by its
overlay in which case that very link should not be redirected.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

Am 06.06.2016 um 16:42 hat Max Reitz geschrieben:
> change_parent_backing_link() asserts that the BDS to be replaced is not
> used as a backing file. However, we may want to replace a BDS by its
> overlay in which case that very link should not be redirected.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>

So the scenario is like this?

              +---- to
              v
    base <- from <- top

And we want to change it into this:

    base <- from <- to <- top

Okay, makes sense.

(Hm, put ASCII art in the commit message? I'd be all for it.)

>  block.c | 15 ++++++++++++++-
>  1 file changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/block.c b/block.c
> index f54bc25..16463aa 100644
> --- a/block.c
> +++ b/block.c
> @@ -2224,9 +2224,22 @@ void bdrv_close_all(void)
>  static void change_parent_backing_link(BlockDriverState *from,
>                                         BlockDriverState *to)
>  {
> -    BdrvChild *c, *next;
> +    BdrvChild *c, *next, *to_c;
>  
>      QLIST_FOREACH_SAFE(c, &from->parents, next_parent, next) {
> +        if (c->role == &child_backing) {
> +            /* Allow @from to be in a backing chain, but only if it is @to's
> +             * backing chain. Do not replace @from by @to there. */

The comment suggests bdrv_chain_contains(), but you only accept it as a
direct child of top and you accept non-backing-file children.

Intuitively I would say that anywhere in the backing chain would make
sense, but we can always allow that later when we actually need it.
Accepting all types of children sounds right when I think about
inserting a quorum node as 'to'.

So I guess the code is fine and the comment should be changed to
correctly reflect what the code does.

> +            QLIST_FOREACH(to_c, &to->children, next) {
> +                if (to_c == c) {
> +                    break;
> +                }
> +            }
> +            if (to_c) {
> +                continue;
> +            }
> +        }
> +
>          assert(c->role != &child_backing);
>          bdrv_ref(to);
>          bdrv_replace_child(c, to);

The other thing is that I'm unsure whether this function makes any sense
at all. "Replace in all parents" is kind of arbitrary. In the long term,
we may want to allow the user to specify the exact graph modifications
on (block-)job-complete.

Kevin

On 08.06.2016 10:58, Kevin Wolf wrote:
> Am 06.06.2016 um 16:42 hat Max Reitz geschrieben:
>> change_parent_backing_link() asserts that the BDS to be replaced is not
>> used as a backing file. However, we may want to replace a BDS by its
>> overlay in which case that very link should not be redirected.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
> 
> So the scenario is like this?
> 
>               +---- to
>               v
>     base <- from <- top
> 
> And we want to change it into this:
> 
>     base <- from <- to <- top
> 
> Okay, makes sense.

Doesn't the assert(c->role != &child_backing) prevent this scenario?
(which is the reason for the first sentence in my commit message.)

So the scenario is rather:

           +----- target
           v
base <- source <- BB

to:

base <- source <- target <- BB

> (Hm, put ASCII art in the commit message? I'd be all for it.)

Well, depending on whether a v3 will come or not, I can put it there, of
course.

> 
>>  block.c | 15 ++++++++++++++-
>>  1 file changed, 14 insertions(+), 1 deletion(-)
>>
>> diff --git a/block.c b/block.c
>> index f54bc25..16463aa 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -2224,9 +2224,22 @@ void bdrv_close_all(void)
>>  static void change_parent_backing_link(BlockDriverState *from,
>>                                         BlockDriverState *to)
>>  {
>> -    BdrvChild *c, *next;
>> +    BdrvChild *c, *next, *to_c;
>>  
>>      QLIST_FOREACH_SAFE(c, &from->parents, next_parent, next) {
>> +        if (c->role == &child_backing) {
>> +            /* Allow @from to be in a backing chain, but only if it is @to's
>> +             * backing chain. Do not replace @from by @to there. */
> 
> The comment suggests bdrv_chain_contains(), but you only accept it as a
> direct child of top and you accept non-backing-file children.
> 
> Intuitively I would say that anywhere in the backing chain would make
> sense, but we can always allow that later when we actually need it.
> Accepting all types of children sounds right when I think about
> inserting a quorum node as 'to'.
> 
> So I guess the code is fine and the comment should be changed to
> correctly reflect what the code does.

OK, will do.

>> +            QLIST_FOREACH(to_c, &to->children, next) {
>> +                if (to_c == c) {
>> +                    break;
>> +                }
>> +            }
>> +            if (to_c) {
>> +                continue;
>> +            }
>> +        }
>> +
>>          assert(c->role != &child_backing);
>>          bdrv_ref(to);
>>          bdrv_replace_child(c, to);
> 
> The other thing is that I'm unsure whether this function makes any sense
> at all. "Replace in all parents" is kind of arbitrary. In the long term,
> we may want to allow the user to specify the exact graph modifications
> on (block-)job-complete.

Well, as a replacement of bdrv_swap(), this function definitely does
make sense. Whether we can do even better in the long run... Is a
question for the long run, I think.

Max