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

[Vladimir Sementsov-Ogievskiy]

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..

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.

> > +
> > +    tran_finalize(tran, ret);
> > +
> > +    if (old_bs) {
> > +        bdrv_drained_end(old_bs);
> > +        bdrv_unref(old_bs);
> > +    }
> > +    bdrv_drained_end(new_bs);
> > +
> > +    return ret;
> > +}
> > +
> >   static void bdrv_delete(BlockDriverState *bs)
> >   {
> >       assert(bdrv_op_blocker_is_empty(bs));


--
Best regards,
Vladimir