diff mbox series

[for-5.0,02/31] block: Add BdrvChildRole

Message ID 20191127131624.1062403-3-mreitz@redhat.com (mailing list archive)
State New, archived
Headers show
Series block: Introduce real BdrvChildRole | expand

Commit Message

Max Reitz Nov. 27, 2019, 1:15 p.m. UTC
This enum will supplement BdrvChildClass when it comes to what role (or
combination of roles) a child takes for its parent.

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(+)

Comments

Vladimir Sementsov-Ogievskiy Nov. 28, 2019, 9:31 a.m. UTC | #1
27.11.2019 16:15, Max Reitz wrote:
> This enum will supplement BdrvChildClass when it comes to what role (or
> combination of roles) a child takes for its parent.
> 
> 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 38963ef203..36817d5689 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -279,6 +279,44 @@ enum {
>       DEFAULT_PERM_UNCHANGED      = BLK_PERM_ALL & ~DEFAULT_PERM_PASSTHROUGH,
>   };
>   
> +typedef enum BdrvChildRole {

Don't you want to call it just BdrvChildFlags ?
Benefits:

1. Do not intersect with old BdrvChildRole.
2. I think, BDRV_CHILD_STAY_AT_NODE is not a role, but just a property or flag..

> +    /*
> +     * If present, bdrv_replace_node() will not change the node this
> +     * BdrvChild points to.
> +     */
> +    BDRV_CHILD_STAY_AT_NODE = (1 << 0),
> +
> +    /* Child stores data */
> +    BDRV_CHILD_DATA         = (1 << 1),
> +
> +    /* Child stores metadata */
> +    BDRV_CHILD_METADATA     = (1 << 2),
> +
> +    /* Filtered child */
> +    BDRV_CHILD_FILTERED     = (1 << 3),
> +
> +    /* Child to COW from (backing child) */
> +    BDRV_CHILD_COW          = (1 << 4),
> +
> +    /* Child is expected to be a protocol node */
> +    BDRV_CHILD_PROTOCOL     = (1 << 5),
> +
> +    /* Child is expected to be a format node */
> +    BDRV_CHILD_FORMAT       = (1 << 6),
> +
> +    /*
> +     * The primary child.  For most drivers, this is the child whose
> +     * filename applies best to the parent node.
> +     */
> +    BDRV_CHILD_PRIMARY      = (1 << 7),
> +
> +    /* Useful combination of flags */
> +    BDRV_CHILD_IMAGE        = BDRV_CHILD_DATA
> +                              | BDRV_CHILD_METADATA
> +                              | BDRV_CHILD_PROTOCOL
> +                              | BDRV_CHILD_PRIMARY,
> +} BdrvChildRole;
> +
>   char *bdrv_perm_names(uint64_t perm);
>   uint64_t bdrv_qapi_perm_to_blk_perm(BlockPermission qapi_perm);
>   
>
Max Reitz Nov. 28, 2019, 10:52 a.m. UTC | #2
On 28.11.19 10:31, Vladimir Sementsov-Ogievskiy wrote:
> 27.11.2019 16:15, Max Reitz wrote:
>> This enum will supplement BdrvChildClass when it comes to what role (or
>> combination of roles) a child takes for its parent.
>>
>> 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 38963ef203..36817d5689 100644
>> --- a/include/block/block.h
>> +++ b/include/block/block.h
>> @@ -279,6 +279,44 @@ enum {
>>       DEFAULT_PERM_UNCHANGED      = BLK_PERM_ALL & ~DEFAULT_PERM_PASSTHROUGH,
>>   };
>>   
>> +typedef enum BdrvChildRole {
> 
> Don't you want to call it just BdrvChildFlags ?
> Benefits:
> 
> 1. Do not intersect with old BdrvChildRole.

Well, that doesn’t change the fact that the old BdrvChildRole just
doesn’t describe a role.

> 2. I think, BDRV_CHILD_STAY_AT_NODE is not a role, but just a property or flag..

I can be convinced to let STAY_AT_NODE stay a property of
BdrvChildClass. :-)

What I don’t like about “BdrvChildFlags” is that “flags” doesn’t express
anything.  The permissions are flags, too.

Max
Vladimir Sementsov-Ogievskiy Nov. 28, 2019, 11:24 a.m. UTC | #3
28.11.2019 13:52, Max Reitz wrote:
> On 28.11.19 10:31, Vladimir Sementsov-Ogievskiy wrote:
>> 27.11.2019 16:15, Max Reitz wrote:
>>> This enum will supplement BdrvChildClass when it comes to what role (or
>>> combination of roles) a child takes for its parent.
>>>
>>> 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 38963ef203..36817d5689 100644
>>> --- a/include/block/block.h
>>> +++ b/include/block/block.h
>>> @@ -279,6 +279,44 @@ enum {
>>>        DEFAULT_PERM_UNCHANGED      = BLK_PERM_ALL & ~DEFAULT_PERM_PASSTHROUGH,
>>>    };
>>>    
>>> +typedef enum BdrvChildRole {
>>
>> Don't you want to call it just BdrvChildFlags ?
>> Benefits:
>>
>> 1. Do not intersect with old BdrvChildRole.
> 
> Well, that doesn’t change the fact that the old BdrvChildRole just
> doesn’t describe a role.
> 
>> 2. I think, BDRV_CHILD_STAY_AT_NODE is not a role, but just a property or flag..
> 
> I can be convinced to let STAY_AT_NODE stay a property of
> BdrvChildClass. :-)

or BdrvChild if we want it to be property of object, not class.

> 
> What I don’t like about “BdrvChildFlags” is that “flags” doesn’t express
> anything.  The permissions are flags, too.
> 
> Max
>
Max Reitz Nov. 28, 2019, 2 p.m. UTC | #4
On 28.11.19 12:24, Vladimir Sementsov-Ogievskiy wrote:
> 28.11.2019 13:52, Max Reitz wrote:
>> On 28.11.19 10:31, Vladimir Sementsov-Ogievskiy wrote:
>>> 27.11.2019 16:15, Max Reitz wrote:
>>>> This enum will supplement BdrvChildClass when it comes to what role (or
>>>> combination of roles) a child takes for its parent.
>>>>
>>>> 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 38963ef203..36817d5689 100644
>>>> --- a/include/block/block.h
>>>> +++ b/include/block/block.h
>>>> @@ -279,6 +279,44 @@ enum {
>>>>        DEFAULT_PERM_UNCHANGED      = BLK_PERM_ALL & ~DEFAULT_PERM_PASSTHROUGH,
>>>>    };
>>>>    
>>>> +typedef enum BdrvChildRole {
>>>
>>> Don't you want to call it just BdrvChildFlags ?
>>> Benefits:
>>>
>>> 1. Do not intersect with old BdrvChildRole.
>>
>> Well, that doesn’t change the fact that the old BdrvChildRole just
>> doesn’t describe a role.
>>
>>> 2. I think, BDRV_CHILD_STAY_AT_NODE is not a role, but just a property or flag..
>>
>> I can be convinced to let STAY_AT_NODE stay a property of
>> BdrvChildClass. :-)
> 
> or BdrvChild if we want it to be property of object, not class.

Sure, but that would then no longer concern this series, I think.  (That
is, either I make STAY_AT_NODE a BdrvChildRole in this series, or I just
don’t.)

Max
Kevin Wolf Nov. 28, 2019, 2:12 p.m. UTC | #5
Am 27.11.2019 um 14:15 hat Max Reitz geschrieben:
> This enum will supplement BdrvChildClass when it comes to what role (or
> combination of roles) a child takes for its parent.
> 
> 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 38963ef203..36817d5689 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -279,6 +279,44 @@ enum {
>      DEFAULT_PERM_UNCHANGED      = BLK_PERM_ALL & ~DEFAULT_PERM_PASSTHROUGH,
>  };
>  
> +typedef enum BdrvChildRole {
> +    /*
> +     * If present, bdrv_replace_node() will not change the node this
> +     * BdrvChild points to.
> +     */
> +    BDRV_CHILD_STAY_AT_NODE = (1 << 0),
> +
> +    /* Child stores data */
> +    BDRV_CHILD_DATA         = (1 << 1),
> +
> +    /* Child stores metadata */
> +    BDRV_CHILD_METADATA     = (1 << 2),
> +
> +    /* Filtered child */
> +    BDRV_CHILD_FILTERED     = (1 << 3),
> +
> +    /* Child to COW from (backing child) */
> +    BDRV_CHILD_COW          = (1 << 4),
> +
> +    /* Child is expected to be a protocol node */
> +    BDRV_CHILD_PROTOCOL     = (1 << 5),
> +
> +    /* Child is expected to be a format node */
> +    BDRV_CHILD_FORMAT       = (1 << 6),

In theory, a node shouldn't care what other nodes it has as its
children. For a parent, protocols and formats look exactly the same.

Of course, we do have BDRV_O_PROTOCOL, but if I'm not mistaken this is
basically only about probing or not probing an image format when a
legacy filename is given rather than BlockdevOptions.

Therefore, unless you have a real reason for this to be here, I'd prefer
if we could keep such legacy flags outside of the core infrastructure if
at all possible.

> +    /*
> +     * The primary child.  For most drivers, this is the child whose
> +     * filename applies best to the parent node.
> +     */
> +    BDRV_CHILD_PRIMARY      = (1 << 7),

If primary is a flag of each BdrvChild, then you could end up having
multiple children that claim that they're the primary child. On the
other hand, if we have a bs->primary_child instead to make sure that we
only have one primary child, we'd have to keep this consistent when
children change.

So maybe just document that this flag must be given to only one
BdrvChild link for each parent.

> +    /* Useful combination of flags */
> +    BDRV_CHILD_IMAGE        = BDRV_CHILD_DATA
> +                              | BDRV_CHILD_METADATA
> +                              | BDRV_CHILD_PROTOCOL
> +                              | BDRV_CHILD_PRIMARY,
> +} BdrvChildRole;
> +
>  char *bdrv_perm_names(uint64_t perm);
>  uint64_t bdrv_qapi_perm_to_blk_perm(BlockPermission qapi_perm);

Kevin
Max Reitz Nov. 28, 2019, 4:36 p.m. UTC | #6
On 28.11.19 15:12, Kevin Wolf wrote:
> Am 27.11.2019 um 14:15 hat Max Reitz geschrieben:
>> This enum will supplement BdrvChildClass when it comes to what role (or
>> combination of roles) a child takes for its parent.
>>
>> 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 38963ef203..36817d5689 100644
>> --- a/include/block/block.h
>> +++ b/include/block/block.h
>> @@ -279,6 +279,44 @@ enum {
>>      DEFAULT_PERM_UNCHANGED      = BLK_PERM_ALL & ~DEFAULT_PERM_PASSTHROUGH,
>>  };
>>  
>> +typedef enum BdrvChildRole {
>> +    /*
>> +     * If present, bdrv_replace_node() will not change the node this
>> +     * BdrvChild points to.
>> +     */
>> +    BDRV_CHILD_STAY_AT_NODE = (1 << 0),
>> +
>> +    /* Child stores data */
>> +    BDRV_CHILD_DATA         = (1 << 1),
>> +
>> +    /* Child stores metadata */
>> +    BDRV_CHILD_METADATA     = (1 << 2),
>> +
>> +    /* Filtered child */
>> +    BDRV_CHILD_FILTERED     = (1 << 3),
>> +
>> +    /* Child to COW from (backing child) */
>> +    BDRV_CHILD_COW          = (1 << 4),
>> +
>> +    /* Child is expected to be a protocol node */
>> +    BDRV_CHILD_PROTOCOL     = (1 << 5),
>> +
>> +    /* Child is expected to be a format node */
>> +    BDRV_CHILD_FORMAT       = (1 << 6),
> 
> In theory, a node shouldn't care what other nodes it has as its
> children. For a parent, protocols and formats look exactly the same.
> 
> Of course, we do have BDRV_O_PROTOCOL, but if I'm not mistaken this is
> basically only about probing or not probing an image format when a
> legacy filename is given rather than BlockdevOptions.
> 
> Therefore, unless you have a real reason for this to be here, I'd prefer
> if we could keep such legacy flags outside of the core infrastructure if
> at all possible.

Hm.  The reason I have it here is because currently this is handled by
BdrvChildClass.inherit_options.  For filtered and backing children, that
will leave PROTOCOL as it is; for the file child of format nodes it will
set PROTOCOL; and for some children (blkverify and quorum) it will clear
PROTOCOL.

So without these flags here, we can’t unify child_file, child_format,
and child_backing in a single class, just because they bequeath the
PROTOCOL flag differently.  At least not directly.

(I’d like to note that this doesn’t make anything worse.  Right now,
drivers need to make a conscious choice on this flag, too, namely by
choosing the right BdrvChildClass.)

Hmm.  Can we do better?  Instead of the driver hinting at what they
expect from the child, can we somehow infer that automatically in
block.c (i.e., in inherit_options without it being given PROTOCOL or
FORMAT)?  FILTERED || COW always means keeping it as-is.  METADATA
generally means setting PROTOCOL, I suppose.

The two problems that come to my mind are blkverify and quorum.
blkverify is special: It must enforce format-probing on the test image,
and it must disable format-probing on the verification (the raw) image.

Quorum wants format probing on everything, but all its children are
simply DATA children.  Other DATA children are e.g. external data files
of qcow2, and we certainly want to force-disable format probing there.

I suppose for the quorum problem we could introduce a
BlockDriver.is_format flag that would force O_PROTOCOL for all non-COW
children, but we would unset O_PROTOCOL for DATA children of
non-is_format drivers.

I suppose the same could work for blkverify’s test image.  For its raw
image, we’d probably just have to enforce the raw driver (or rely on the
fact that blkverify is technically a protocol driver in a way (it
implements .bdrv_file_open...), so it will always have O_PROTOCOL set on
itself; thus, its filtered child (the raw child) will automatically have
it, too, as long as we don’t touch it).

Do you have a better idea?

>> +    /*
>> +     * The primary child.  For most drivers, this is the child whose
>> +     * filename applies best to the parent node.
>> +     */
>> +    BDRV_CHILD_PRIMARY      = (1 << 7),
> 
> If primary is a flag of each BdrvChild, then you could end up having
> multiple children that claim that they're the primary child. On the
> other hand, if we have a bs->primary_child instead to make sure that we
> only have one primary child, we'd have to keep this consistent when
> children change.
> 
> So maybe just document that this flag must be given to only one
> BdrvChild link for each parent.

Sure.

Max
diff mbox series

Patch

diff --git a/include/block/block.h b/include/block/block.h
index 38963ef203..36817d5689 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -279,6 +279,44 @@  enum {
     DEFAULT_PERM_UNCHANGED      = BLK_PERM_ALL & ~DEFAULT_PERM_PASSTHROUGH,
 };
 
+typedef enum BdrvChildRole {
+    /*
+     * If present, bdrv_replace_node() will not change the node this
+     * BdrvChild points to.
+     */
+    BDRV_CHILD_STAY_AT_NODE = (1 << 0),
+
+    /* Child stores data */
+    BDRV_CHILD_DATA         = (1 << 1),
+
+    /* Child stores metadata */
+    BDRV_CHILD_METADATA     = (1 << 2),
+
+    /* Filtered child */
+    BDRV_CHILD_FILTERED     = (1 << 3),
+
+    /* Child to COW from (backing child) */
+    BDRV_CHILD_COW          = (1 << 4),
+
+    /* Child is expected to be a protocol node */
+    BDRV_CHILD_PROTOCOL     = (1 << 5),
+
+    /* Child is expected to be a format node */
+    BDRV_CHILD_FORMAT       = (1 << 6),
+
+    /*
+     * The primary child.  For most drivers, this is the child whose
+     * filename applies best to the parent node.
+     */
+    BDRV_CHILD_PRIMARY      = (1 << 7),
+
+    /* Useful combination of flags */
+    BDRV_CHILD_IMAGE        = BDRV_CHILD_DATA
+                              | BDRV_CHILD_METADATA
+                              | BDRV_CHILD_PROTOCOL
+                              | BDRV_CHILD_PRIMARY,
+} BdrvChildRole;
+
 char *bdrv_perm_names(uint64_t perm);
 uint64_t bdrv_qapi_perm_to_blk_perm(BlockPermission qapi_perm);