Re: [PATCH v3 03/33] block: Add BdrvChildRole and BdrvChildRoleBits

[Kevin Wolf <kwolf@redhat.com>]

Am 18.02.2020 um 13:42 hat Max Reitz geschrieben:
> This mask will supplement BdrvChildClass when it comes to what role (or
> combination of roles) a child takes for its parent.  It consists of
> BdrvChildRoleBits values (which is an enum).
> 
> Because empty enums are not allowed, let us just start with it filled.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  include/block/block.h | 38 ++++++++++++++++++++++++++++++++++++++
>  1 file changed, 38 insertions(+)
> 
> diff --git a/include/block/block.h b/include/block/block.h
> index fd89eb6c75..8c23948d08 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -268,6 +268,44 @@ enum {
>      DEFAULT_PERM_UNCHANGED      = BLK_PERM_ALL & ~DEFAULT_PERM_PASSTHROUGH,
>  };
>  
> +enum BdrvChildRoleBits {
> +    /* Child stores data */
> +    BDRV_CHILD_DATA         = (1 << 0),
> +
> +    /* Child stores metadata */
> +    BDRV_CHILD_METADATA     = (1 << 1),
> +
> +    /*
> +     * A child to which the parent forwards all reads and writes.  It

Is "_all_ reads and writes" really required? Imagine a caching block
driver, should it not be considered a filter because it may just
complete the requests from its cache rather than asking the child?

> +     * must present exactly the same visible data as the parent.
> +     * Any node may have at most one filtered child at a time.
> +     */
> +    BDRV_CHILD_FILTERED     = (1 << 2),
> +
> +    /*
> +     * Child from which to read all data that isn’t allocated in the
> +     * parent (i.e., the backing child); such data is copied to the
> +     * parent through COW (and optionally COR).
> +     */
> +    BDRV_CHILD_COW          = (1 << 3),
> +
> +    /*
> +     * The primary child.  For most drivers, this is the child whose
> +     * filename applies best to the parent node.
> +     * Each parent must give this flag to no more than one child at a
> +     * time.
> +     */
> +    BDRV_CHILD_PRIMARY      = (1 << 4),

And I assume some drivers like quorum don't set it on any child.

> +    /* Useful combination of flags */
> +    BDRV_CHILD_IMAGE        = BDRV_CHILD_DATA
> +                              | BDRV_CHILD_METADATA
> +                              | BDRV_CHILD_PRIMARY,
> +};
> +
> +/* Mask of BdrvChildRoleBits values */
> +typedef unsigned int BdrvChildRole;
> +
>  char *bdrv_perm_names(uint64_t perm);
>  uint64_t bdrv_qapi_perm_to_blk_perm(BlockPermission qapi_perm);

The list intuitively makes sense to me. Let me try to think of some
interesting cases to see whether the documentation is complete or
whether it could be improved.


qcow2 is what everyone has in mind, so it should be obvious:

* Without a data file:
  * file: BDRV_CHILD_IMAGE
  * backing: BDRV_CHILD_COW

* With a data file:
  * file: BDRV_CHILD_PRIMARY | BDRV_CHILD_METADATA
  * data-file: BDRV_CHILD_DATA
  * backing: BDRV_CHILD_COW


We can use VMDK to make things a bit more interesting:

* file: BDRV_CHILD_PRIMARY | BDRV_CHILD_METADATA
* extents.*: BDRV_CHILD_METADATA | BDRV_CHILD_DATA
* backing: BDRV_CHILD_COW

In other words, we can have multiple data and metadata children. Is this
correct or should extents not be marked as metadata? (Checked the final
code: yes we do have multiple of them in vmdk.) Should this be mentioned
in the documentation?

Do we then also want to allow multiple BDRV_CHILD_COW children? We don't
currently have a driver that needs it, but maybe it would be consistent
with DATA and METADATA then. However, it would contradict the
documentation that it's the "Child from which to read all data".


blkverify:

* x-image: BDRV_CHILD_PRIMARY | BDRV_CHILD_DATA | BDRV_CHILD_FILTERED
* x-raw: BDRV_CHILD_DATA | BDRV_CHILD_FILTERED

Hm, according to the documentation, this doesn't work, FILTERED can be
set only for one node. But the condition ("the parent forwards all reads
and writes") applies to both children. I think the documentation should
mention what needs to be done in such cases. For blkverify, both
children are not equal in intention, so I guess the "real" filtered
child is x-image. But for quorum, you can't make any such distinction. I
assume the recommendation should be not to set FILTERED for any child
then.

Looking at the final code... Hm, your choice looks quite different: You
don't have DATA for x-raw, but you make it the PRIMARY and FILTERED
child. I think PRIMARY/FILTERED is just a bug (e.g. getlength and flush
being forwarded only to x-image show that it's primary). I do wonder
whether I have a different interpretation of DATA than you, though.

Also, the comparison makes me wonder whether FILTERED always implies
PRIMARY? Would there ever be a scenario where a child is FILTERED, but
not PRIMARY?


So I'm not completely sure if I understand the roles correctly, but I
guess my understanding is good enough to continue with the rest of the
series.

Kevin