Re: [Qemu-block] [PATCH 08/34] block: Add list of children to BlockDriverState

[Max Reitz]

On 08.05.2015 19:21, Kevin Wolf wrote:
> This allows iterating over all children of a given BDS, not only
> including bs->file and bs->backing_hd, but also driver-specific
> ones like VMDK extents or Quorum children.
>
> Signed-off-by: Kevin Wolf <address@hidden>
> ---
>   block.c                   | 27 +++++++++++++++++++++++++++
>   include/block/block_int.h |  8 ++++++++
>   2 files changed, 35 insertions(+)
>
> diff --git a/block.c b/block.c
> index c4f0fb4..59f54ed 100644
> --- a/block.c
> +++ b/block.c
> @@ -1301,6 +1301,19 @@ out:
>       return ret;
>   }
>   
> +static void bdrv_attach_child(BlockDriverState *parent_bs,
> +                              BlockDriverState *child_bs,
> +                              const BdrvChildRole *child_role)
> +{
> +    BdrvChild *child = g_new(BdrvChild, 1);
> +    *child = (BdrvChild) {
> +        .bs     = child_bs,
> +        .role   = child_role,
> +    };
> +
> +    QLIST_INSERT_HEAD(&parent_bs->children, child, next);
> +}
> +
>   /*
>    * Opens a disk image (raw, qcow2, vmdk, ...)
>    *
> @@ -1353,6 +1366,9 @@ static int bdrv_open_inherit(BlockDriverState **pbs, 
> const char *filename,
>               return -ENODEV;
>           }
>           bdrv_ref(bs);
> +        if (child_role) {
> +            bdrv_attach_child(parent, bs, child_role);
> +        }
>           *pbs = bs;
>           return 0;
>       }
> @@ -1495,6 +1511,10 @@ static int bdrv_open_inherit(BlockDriverState **pbs, 
> const char *filename,
>           goto close_and_fail;
>       }
>   
> +    if (child_role) {
> +        bdrv_attach_child(parent, bs, child_role);
> +    }
> +
>       QDECREF(options);
>       *pbs = bs;
>       return 0;
> @@ -1789,6 +1809,12 @@ void bdrv_close(BlockDriverState *bs)
>       notifier_list_notify(&bs->close_notifiers, bs);
>   
>       if (bs->drv) {
> +        BdrvChild *child, *next;
> +
> +        QLIST_FOREACH_SAFE(child, &bs->children, next, next) {
> +            g_free(child);
> +        }
> +

Not considering the case where the child is closed before the parent 
assumes all children are reference-counted from the parent and they 
won't be closed (and maybe replaced with another BDS) on purpose. The 
first seems reasonable, the second one I'm not so sure about. It works 
for now, but I could imagine that we want to modify children of a Quorum 
instance at runtime.

But I can't imagine any case where this would break right now, so I 
guess I'm fine with it.

>           if (bs->backing_hd) {
>               BlockDriverState *backing_hd = bs->backing_hd;
>               bdrv_set_backing_hd(bs, NULL);
> @@ -1999,6 +2025,7 @@ void bdrv_append(BlockDriverState *bs_new, 
> BlockDriverState *bs_top)
>       /* The contents of 'tmp' will become bs_top, as we are
>        * swapping bs_new and bs_top contents. */
>       bdrv_set_backing_hd(bs_top, bs_new);
> +    bdrv_attach_child(bs_top, bs_new, &child_backing);
>   }
>   
>   static void bdrv_delete(BlockDriverState *bs)

Using a mirror block job, we can force bdrv_swap() on arbitrary nodes, 
right? What happens if you swap e.g. a VMDK and a quorum node? Well, 
maybe one simply cannot swap a quorum node due to blockers, but I guess 
one can swap a VMDK node with some non-VMDK node. It is actually correct 
to leave the extents behind; but the other node cannot do anything with 
them, so because they are part of the opaque VMDK structure, they will 
de-facto remain with VMDK, while being counted as children of the other 
node. But I try to keep so far away from bdrv_swap() that I don't even 
know whether this case is even possible.

Max

> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index eb01ea2..12c7fb3 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -334,6 +334,12 @@ struct BdrvChildRole {
>   extern const BdrvChildRole child_file;
>   extern const BdrvChildRole child_format;
>   
> +typedef struct BdrvChild {
> +    BlockDriverState *bs;
> +    const BdrvChildRole *role;
> +    QLIST_ENTRY(BdrvChild) next;
> +} BdrvChild;
> +
>   /*
>    * Note: the function bdrv_append() copies and swaps contents of
>    * BlockDriverStates, so if you add new fields to this struct, please
> @@ -428,6 +434,8 @@ struct BlockDriverState {
>       /* long-running background operation */
>       BlockJob *job;
>   
> +    QLIST_HEAD(, BdrvChild) children;
> +
>       QDict *options;
>       BlockdevDetectZeroesOptions detect_zeroes;
>