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