Re: [PATCH 01/21] block: introduce bdrv_replace_child_bs()

[Max Reitz <mreitz@redhat.com>]

On 17.05.21 16:30, Vladimir Sementsov-Ogievskiy wrote:
> 17.05.2021 15:09, Max Reitz wrote:
>> On 17.05.21 08:44, Vladimir Sementsov-Ogievskiy wrote:
>>> Add function to transactionally replace bs inside BdrvChild.
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>> ---
>>>   include/block/block.h |  2 ++
>>>   block.c               | 36 ++++++++++++++++++++++++++++++++++++
>>>   2 files changed, 38 insertions(+)
>>
>> As you may guess, I know little about the rewritten replacing 
>> functions, so this is kind of difficult to review for me.  However, 
>> nothing looks out of place, and the function looks sufficiently 
>> similar to bdrv_replace_node_common() to make me happy.
>>
>>> diff --git a/include/block/block.h b/include/block/block.h
>>> index 82185965ff..f9d5fcb108 100644
>>> --- a/include/block/block.h
>>> +++ b/include/block/block.h
>>> @@ -361,6 +361,8 @@ int bdrv_append(BlockDriverState *bs_new, 
>>> BlockDriverState *bs_top,
>>>                   Error **errp);
>>>   int bdrv_replace_node(BlockDriverState *from, BlockDriverState *to,
>>>                         Error **errp);
>>> +int bdrv_replace_child_bs(BdrvChild *child, BlockDriverState *new_bs,
>>> +                          Error **errp);
>>>   BlockDriverState *bdrv_insert_node(BlockDriverState *bs, QDict 
>>> *node_options,
>>>                                      int flags, Error **errp);
>>>   int bdrv_drop_filter(BlockDriverState *bs, Error **errp);
>>> diff --git a/block.c b/block.c
>>> index 9ad725d205..755fa53d85 100644
>>> --- a/block.c
>>> +++ b/block.c
>>> @@ -4961,6 +4961,42 @@ out:
>>>       return ret;
>>>   }
>>> +int bdrv_replace_child_bs(BdrvChild *child, BlockDriverState *new_bs,
>>> +                          Error **errp)
>>> +{
>>> +    int ret;
>>> +    Transaction *tran = tran_new();
>>> +    g_autoptr(GHashTable) found = NULL;
>>> +    g_autoptr(GSList) refresh_list = NULL;
>>> +    BlockDriverState *old_bs = child->bs;
>>> +
>>> +    if (old_bs) {
>>
>> Hm.  Can child->bs be ever NULL?
> 
> Hmm. Most probably not :)
> 
> In some intermediate states we don't have bs in child, but it shouldn't 
> be the place where bdrv_replace_child_bs is called.
> 
>>
>>> +        bdrv_ref(old_bs);
>>> +        bdrv_drained_begin(old_bs);
>>> +    }
>>> +    bdrv_drained_begin(new_bs);
>>
>> (I was wondering why we couldn’t handle the new_bs == NULL case here 
>> to replace bdrv_remove_filter_or_cow_child(), but then I realized it’s 
>> probably because that’s kind of difficult, precisely because child->bs 
>> at least should generally be non-NULL.  Which is why 
>> bdrv_remove_filter_or_cow_child() needs to add its own transaction 
>> entry to handle the BdrvChild object and the pointer to it.
>>
>> Hence me wondering whether we could assume child->bs not to be NULL.)
> 
> bdrv_remove_filter_or_cow_child() is "lower leve" function: it doesn't 
> do drained section nor permission update. And new 
> bdrv_replace_child_bs() is public function, which cares about these things.
> 
>>
>>> +
>>> +    bdrv_replace_child(child, new_bs, tran);
>>> +
>>> +    found = g_hash_table_new(NULL, NULL);
>>> +    if (old_bs) {
>>> +        refresh_list = bdrv_topological_dfs(refresh_list, found, 
>>> old_bs);
>>> +    }
>>> +    refresh_list = bdrv_topological_dfs(refresh_list, found, new_bs);
>>> +
>>> +    ret = bdrv_list_refresh_perms(refresh_list, NULL, tran, errp);
>>
>> Speaking of bdrv_remove_filter_or_cow_child(): That function doesn’t 
>> refresh permissions.  I think it’s correct to do it here, so the 
>> following question doesn’t really concern this patch, but: Why don’t 
>> we do it there?
>>
>> I guess it’s because we expect the node to go away anyway, so we don’t 
>> need to refresh the permissions.  And that assumption should hold true 
>> right now, given its callers.  But is that a safe assumption in 
>> general?  Would there be a problem if we refreshed permissions there?  
>> Or is not refreshing permissions just part of the function’s interface?
>>
> 
> Caller of bdrv_remove_filter_or_cow_child() should care about 
> permissions:  bdrv_replace_node_common() do this, and 
> bdrv_set_backing_noperm() has "_noperm" in the name..

OK.  Makes me wonder why bdrv_remove_filter_or_cow_child() then doesn’t 
have _noperm in its name, or why its comment doesn’t explain this 
interface contract, but, well. :)

> The main impact of previous big rework of permission is new scheme of 
> working with permission update:
> 
>   - first do all graph modifications, not thinking about permissions
>   - refresh permissions for the whole updated subgraph
>   - if refresh failed, rollback all the modifications (main sense if 
> transactions here and there is possibility to do this rollback)
> 
> So a lot of internal functions with @tran argument don't update 
> permissions. But of course, we should care to update permissions after 
> any graph modification.

Ah, OK.  Makes sense, thanks.

Max