Re: [Qemu-devel] [PATCH v5 14/42] block: Use CAFs when working with backing chains

[Vladimir Sementsov-Ogievskiy]

14.06.2019 19:02, Max Reitz wrote:
> On 14.06.19 16:31, Vladimir Sementsov-Ogievskiy wrote:
>> 14.06.2019 16:50, Max Reitz wrote:
>>> On 14.06.19 15:26, Vladimir Sementsov-Ogievskiy wrote:
>>>> 13.06.2019 1:09, Max Reitz wrote:
>>>>> Use child access functions when iterating through backing chains so
>>>>> filters do not break the chain.
>>>>>
>>>>> Signed-off-by: Max Reitz <address@hidden>
>>>>> ---
>>>>>     block.c | 40 ++++++++++++++++++++++++++++------------
>>>>>     1 file changed, 28 insertions(+), 12 deletions(-)
>>>>>
>>>>> diff --git a/block.c b/block.c
>>>>> index 11f37983d9..505b3e9a01 100644
>>>>> --- a/block.c
>>>>> +++ b/block.c
>>>
>>> [...]
>>>
>>>>> @@ -4273,11 +4274,18 @@ int bdrv_change_backing_file(BlockDriverState *bs,
>>>>>     BlockDriverState *bdrv_find_overlay(BlockDriverState *active,
>>>>>                                         BlockDriverState *bs)
>>>>>     {
>>>>> -    while (active && bs != backing_bs(active)) {
>>>>> -        active = backing_bs(active);
>>>>> +    bs = bdrv_skip_rw_filters(bs);
>>>>> +    active = bdrv_skip_rw_filters(active);
>>>>> +
>>>>> +    while (active) {
>>>>> +        BlockDriverState *next = bdrv_backing_chain_next(active);
>>>>> +        if (bs == next) {
>>>>> +            return active;
>>>>> +        }
>>>>> +        active = next;
>>>>>         }
>>>>>     
>>>>> -    return active;
>>>>> +    return NULL;
>>>>>     }
>>>>
>>>> Semantics changed for this function.
>>>> It is used in two places
>>>> 1. from bdrv_find_base wtih @bs=NULL, it should be unchanged, as I hope we 
>>>> will never have
>>>>       filter node as a bottom of some valid chain
>>>>
>>>> 2. from qmp_block_commit, only to check op-blocker... hmmm. I really don't 
>>>> understand,
>>>> why do we check BLOCK_OP_TYPE_COMMIT_TARGET on top_bs overlay.. top_bs 
>>>> overlay is out of the job,
>>>> what is this check for?
>>>
>>> There is a loop before this check which checks that the same blocker is
>>> not set on any nodes between top and base (both inclusive).  I guess
>>> non-active commit checks the node above @top, too, because its backing
>>> file will change.
>>
>> So in this case frozen chain works better.
> 
> Perhaps.  The op blockers are in this weird state anyway.  I don’t think
> we even need them any more, because the permissions were intended to
> replace them (they were originally called “fine-grained op blockers”, I
> seem to remember).
> 
> I dare not touch them.
> 
>>>>>     /* Given a BDS, searches for the base layer. */
>>>
>>> [...]
>>>
>>>>> @@ -5149,7 +5165,7 @@ BlockDriverState 
>>>>> *bdrv_find_backing_image(BlockDriverState *bs,
>>>>>                 char *backing_file_full_ret;
>>>>>     
>>>>>                 if (strcmp(backing_file, curr_bs->backing_file) == 0) {
>>>>
>>>> hmm, interesting, what bs->backing_file now means? It's strange enough to 
>>>> store such field on
>>>> bds, when we have backing link anyway..
>>>
>>> Patch 37 has you covered. :-)
>>>
>>
>> Hmm, if it has removed this field, but it doesn't)
> 
> Because it’s needed.  (Just not in the current form, but that’s what 37
> is for.)
> 
>> So, we finished with some object, called "overlay", but it is not an overlay 
>> of bs, it's overlay of
>> first non-implicit filtered node in bs backing chain, it may be found by 
>> bdrv_find_overlay() helper (which is
>> almost unused and my be safely dropped), and filename of this "overlay" is 
>> stored in bs->backing_file string
>> variable, keeping in mind that bs->backing is pointer to backing child of bs 
>> which is completely another thing?
> 
> I don’t quite see what you mean.  There is no “overlay” in this function.

Hmm, sorry, I kept in mind overlay from the next patch..

> 
>> Oh, no, everything related to filename-based backing chain logic is not for 
>> me o_O. If something doesn't work
>> with filename-based logic users should use node-names..
> 
> In theory yes, but that isn’t an option for qemu-img commit, for example.

And if something doesn't work with qemu-img, users should use qemu process in 
stopped state. And I'd prefer to
deprecate most of qemu-img :) Actually we in Virtuozzo already go this way for 
some things. QMP interface is
a lot more useful than qemu-img, and it's always simpler to maintain and 
develop one thing than two.

> 
>> And I'd prefer to deprecate filename based interfaces at all.
> 
> Me too.
> 
> https://lists.nongnu.org/archive/html/qemu-devel/2014-09/msg04878.html
> 
> :-/
> 

Really sad..


-- 
Best regards,
Vladimir