Re: [Qemu-devel] [PATCH v2 1/3] block: Allow replacement of a BDS by its overlay

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3102 bytes --]

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]