Re: [PATCH 6/7] block: Let replace_child_noperm free children

[Kevin Wolf <kwolf@redhat.com>]

Am 04.11.2021 um 11:38 hat Hanna Reitz geschrieben:
> In most of the block layer, especially when traversing down from other
> BlockDriverStates, we assume that BdrvChild.bs can never be NULL.  When
> it becomes NULL, it is expected that the corresponding BdrvChild pointer
> also becomes NULL and the BdrvChild object is freed.
> 
> Therefore, once bdrv_replace_child_noperm() sets the BdrvChild.bs
> pointer to NULL, it should also immediately set the corresponding
> BdrvChild pointer (like bs->file or bs->backing) to NULL.
> 
> In that context, it also makes sense for this function to free the
> child.  Sometimes we cannot do so, though, because it is called in a
> transactional context where the caller might still want to reinstate the
> child in the abort branch (and free it only on commit), so this behavior
> has to remain optional.
> 
> In bdrv_replace_child_tran()'s abort handler, we now rely on the fact
> that the BdrvChild passed to bdrv_replace_child_tran() must have had a
> non-NULL .bs pointer initially.  Make a note of that and assert it.
> 
> Signed-off-by: Hanna Reitz <hreitz@redhat.com>
> ---
>  block.c | 102 +++++++++++++++++++++++++++++++++++++++++++++++---------
>  1 file changed, 86 insertions(+), 16 deletions(-)
> 
> diff --git a/block.c b/block.c
> index ff45447686..0a85ede8dc 100644
> --- a/block.c
> +++ b/block.c
> @@ -87,8 +87,10 @@ static BlockDriverState *bdrv_open_inherit(const char *filename,
>  static bool bdrv_recurse_has_child(BlockDriverState *bs,
>                                     BlockDriverState *child);
>  
> +static void bdrv_child_free(BdrvChild *child);
>  static void bdrv_replace_child_noperm(BdrvChild **child,
> -                                      BlockDriverState *new_bs);
> +                                      BlockDriverState *new_bs,
> +                                      bool free_empty_child);
>  static void bdrv_remove_file_or_backing_child(BlockDriverState *bs,
>                                                BdrvChild *child,
>                                                Transaction *tran);
> @@ -2256,12 +2258,16 @@ typedef struct BdrvReplaceChildState {
>      BdrvChild *child;
>      BdrvChild **childp;
>      BlockDriverState *old_bs;
> +    bool free_empty_child;
>  } BdrvReplaceChildState;
>  
>  static void bdrv_replace_child_commit(void *opaque)
>  {
>      BdrvReplaceChildState *s = opaque;
>  
> +    if (s->free_empty_child && !s->child->bs) {
> +        bdrv_child_free(s->child);
> +    }
>      bdrv_unref(s->old_bs);
>  }
>  
> @@ -2270,8 +2276,23 @@ static void bdrv_replace_child_abort(void *opaque)
>      BdrvReplaceChildState *s = opaque;
>      BlockDriverState *new_bs = s->child->bs;
>  
> -    /* old_bs reference is transparently moved from @s to *s->childp */
> -    bdrv_replace_child_noperm(s->childp, s->old_bs);
> +    /*
> +     * old_bs reference is transparently moved from @s to s->child;
> +     * pass &s->child here instead of s->childp, because *s->childp
> +     * will have been cleared by bdrv_replace_child_tran()'s
> +     * bdrv_replace_child_noperm() call if new_bs is NULL, and we must
> +     * not pass a NULL *s->childp here.
> +     */
> +    bdrv_replace_child_noperm(&s->child, s->old_bs, true);

Passing free_empty_child=true with a non-NULL new_bs looks a bit
confusing because the child isn't supposed to become empty anyway.

> +    /*
> +     * The child was pre-existing, so s->old_bs must be non-NULL, and
> +     * s->child thus must not have been freed
> +     */
> +    assert(s->child != NULL);
> +    if (!new_bs) {
> +        /* As described above, *s->childp was cleared, so restore it */
> +        *s->childp = s->child;
> +    }

If it wasn't cleared, doesn't it still contain s->child, so this could
just be an unconditional rollback?

>      bdrv_unref(new_bs);
>  }

Kevin