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

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1.1: Type: text/plain, Size: 7718 bytes --]

On 05.05.20 13:19, Kevin Wolf wrote:
> 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?

Hm.  The important thing is that parent and child always show exactly
the same data, which is the second part of the definition given here.
So maybe we should flip both sentences, e.g.:

“A child which always presents exactly the same visibile data as the
parent, e.g. by virtue of the parent forwarding all reads and writes.”

?

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

I thought “no more than one” implies that.

>> +    /* 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?

If the extents contain metadata (I thought not, but I think I was just
wrong; sparse extents must contain their respective mapping structures),
then yes, they should all be marked as metadata children.

I’m not sure whether that needs to be mentioned explicitly in the doc,
because “Child stores metadata” seems sufficient to me.

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

Yes.  I would revisit that problem when the need arises.

It seems to me like this would open a whole can of worms, just like
allowing multiple filtered children does.

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

I don’t know.  blkverify is a rare exception by design, because it can
abort when both children don’t match.  (I suppose we could theoretically
have a quorum mode where a child gets ejected once a mismatch is
detected, but that isn’t the case now.)

Furthermore, I would argue that blkverify actually expects the formatted
image to sometimes differ from the raw image, if anything, because the
format driver is to be tested.  This is the reason why I chose x-raw to
be the filtered child.

So there is no general instruction on what to do in such cases that I
followed here, I specifically chose one child based on what blkverify is
and what it’s supposed to do.  Therefore, I can’t really give a general
instruction on “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.

Quorum just isn’t a filter driver.

> 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 rather consider getlength() a special case.  Ideally, we’d forward
getlength() to both and compare the results; however, image formats
might have different size resolution than raw files, so there could be a
difference, but it’d be irrelevant.

It makes then sense to forward it to the formatted image, because
generally formats have byte resolution for the disk size, whereas for
raw files it depends on caching and the filesystem, I think.

As for flush, yes, why do we forward it only to x-image?  Why is “the
raw file not important”?

> I do wonder
> whether I have a different interpretation of DATA than you, though.

I never set DATA for FILTERED, because I consider FILTERED to be
stronger than DATA, so once FILTERED is set, it doesn’t matter whether
DATA is set or not.  I suppose that should either be mentioned in the
documentation, or we decide that we should always set DATA regardless.

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

I don’t know.  I suppose it does.  But what’s the implication?

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]