| Message ID | 20200204170848.614480-4-mreitz@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | block: Introduce real BdrvChildRole | expand |
On 2/4/20 11:08 AM, 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 | 27 +++++++++++++++++++++++++++ > 1 file changed, 27 insertions(+) > > diff --git a/include/block/block.h b/include/block/block.h > index 38963ef203..0f7e8caa5b 100644 > --- a/include/block/block.h > +++ b/include/block/block.h > @@ -279,6 +279,33 @@ enum { > DEFAULT_PERM_UNCHANGED = BLK_PERM_ALL & ~DEFAULT_PERM_PASSTHROUGH, > }; > > +typedef enum BdrvChildRole { > + /* Child stores data */ > + BDRV_CHILD_DATA = (1 << 0), > + > + /* Child stores metadata */ > + BDRV_CHILD_METADATA = (1 << 1), > + > + /* Filtered child */ > + BDRV_CHILD_FILTERED = (1 << 2), I'm not sure this comment does justice for what the flag represents, but am not sure of what longer comment to put in its place. > + > + /* Child to COW from (backing child) */ > + 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), > + > + /* Useful combination of flags */ > + BDRV_CHILD_IMAGE = BDRV_CHILD_DATA > + | BDRV_CHILD_METADATA > + | BDRV_CHILD_PRIMARY, > +} BdrvChildRole; > + > char *bdrv_perm_names(uint64_t perm); > uint64_t bdrv_qapi_perm_to_blk_perm(BlockPermission qapi_perm); > > Whether or not you can improve the comment, the enum makes sense. Reviewed-by: Eric Blake <eblake@redhat.com>
On 05.02.20 16:24, Eric Blake wrote: > On 2/4/20 11:08 AM, 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 | 27 +++++++++++++++++++++++++++ >> 1 file changed, 27 insertions(+) >> >> diff --git a/include/block/block.h b/include/block/block.h >> index 38963ef203..0f7e8caa5b 100644 >> --- a/include/block/block.h >> +++ b/include/block/block.h >> @@ -279,6 +279,33 @@ enum { >> DEFAULT_PERM_UNCHANGED = BLK_PERM_ALL & >> ~DEFAULT_PERM_PASSTHROUGH, >> }; >> +typedef enum BdrvChildRole { >> + /* Child stores data */ >> + BDRV_CHILD_DATA = (1 << 0), >> + >> + /* Child stores metadata */ >> + BDRV_CHILD_METADATA = (1 << 1), >> + >> + /* Filtered child */ >> + BDRV_CHILD_FILTERED = (1 << 2), > > I'm not sure this comment does justice for what the flag represents, but > am not sure of what longer comment to put in its place. You’re right. I thought I could just rely on our .is_filter documentation (at least after https://lists.gnu.org/archive/html/qemu-devel/2019-08/msg01721.html), but that doesn’t really apply here. For example, this series makes raw (without further parameters) have a CHILD_FILTERED child, without raw being a filter itself. So there should indeed be some definition here. Maybe: A child to which the parent forwards all reads and writes. Therefore, this child presents exactly the same visible data as the parent. Would that work? Max
On 06.02.20 11:47, Max Reitz wrote: > On 05.02.20 16:24, Eric Blake wrote: >> On 2/4/20 11:08 AM, 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 | 27 +++++++++++++++++++++++++++ >>> 1 file changed, 27 insertions(+) >>> >>> diff --git a/include/block/block.h b/include/block/block.h >>> index 38963ef203..0f7e8caa5b 100644 >>> --- a/include/block/block.h >>> +++ b/include/block/block.h >>> @@ -279,6 +279,33 @@ enum { >>> DEFAULT_PERM_UNCHANGED = BLK_PERM_ALL & >>> ~DEFAULT_PERM_PASSTHROUGH, >>> }; >>> +typedef enum BdrvChildRole { >>> + /* Child stores data */ >>> + BDRV_CHILD_DATA = (1 << 0), >>> + >>> + /* Child stores metadata */ >>> + BDRV_CHILD_METADATA = (1 << 1), >>> + >>> + /* Filtered child */ >>> + BDRV_CHILD_FILTERED = (1 << 2), >> >> I'm not sure this comment does justice for what the flag represents, but >> am not sure of what longer comment to put in its place. > > You’re right. I thought I could just rely on our .is_filter > documentation (at least after > https://lists.gnu.org/archive/html/qemu-devel/2019-08/msg01721.html), > but that doesn’t really apply here. > > For example, this series makes raw (without further parameters) have a > CHILD_FILTERED child, without raw being a filter itself. > > So there should indeed be some definition here. > > Maybe: > > A child to which the parent forwards all reads and writes. Therefore, > this child presents exactly the same visible data as the parent. On second thought, the “therefore” is wrong, because the first sentence applies to quorum, but the logical conclusion does not. So maybe rather: A child to which the parent forwards all reads and writes. It must present exactly the same visible data as the parent. Any node may have at most one filtered child at a time. ? Max
On 2/6/20 4:49 AM, Max Reitz wrote: >>>> + >>>> + /* Filtered child */ >>>> + BDRV_CHILD_FILTERED = (1 << 2), >>> >>> I'm not sure this comment does justice for what the flag represents, but >>> am not sure of what longer comment to put in its place. >> >> You’re right. I thought I could just rely on our .is_filter >> documentation (at least after >> https://lists.gnu.org/archive/html/qemu-devel/2019-08/msg01721.html), >> but that doesn’t really apply here. >> >> For example, this series makes raw (without further parameters) have a >> CHILD_FILTERED child, without raw being a filter itself. >> >> So there should indeed be some definition here. >> >> Maybe: >> >> A child to which the parent forwards all reads and writes. Therefore, >> this child presents exactly the same visible data as the parent. > > On second thought, the “therefore” is wrong, because the first sentence > applies to quorum, but the logical conclusion does not. > > So maybe rather: > > A child to which the parent forwards all reads and writes. It must > present exactly the same visible data as the parent. > Any node may have at most one filtered child at a time. Yes, this works for me.
On Tue 04 Feb 2020 06:08:18 PM CET, Max Reitz wrote: > + /* Child to COW from (backing child) */ > + BDRV_CHILD_COW = (1 << 3), Without the comment in brackets I'm not sure that I would have understood that this is meant for backing files. This is the "child that contains the data that is not allocated in the parent", or something like that, right? Berto
On 11.02.20 16:41, Alberto Garcia wrote: > On Tue 04 Feb 2020 06:08:18 PM CET, Max Reitz wrote: >> + /* Child to COW from (backing child) */ >> + BDRV_CHILD_COW = (1 << 3), > > Without the comment in brackets I'm not sure that I would have > understood that this is meant for backing files. I put it in brackets because bs->backing isn’t always such a child (for filters it isn’t). That’s also the reason why I prefer to stress the COW aspect. > This is the "child that contains the data that is not allocated in the > parent", or something like that, right? Hm, so I suppose the problem is that I didn’t describe in which event the COW is to occur. (I didn’t because we only have one kind of COW in the block layer, namely for backing chains.) So maybe “Child from which to read all data that isn’t allocated in the parent (backing child); such data may be copied to the parent by means of COW or COR”? (The problem I see with this description is that it is kind of a tautology, because “allocation” is in turn defined by differentiating between layers of a backing chain, i.e. this layer and that backing/COW child we’re talking about here.) Max
diff --git a/include/block/block.h b/include/block/block.h index 38963ef203..0f7e8caa5b 100644 --- a/include/block/block.h +++ b/include/block/block.h @@ -279,6 +279,33 @@ enum { DEFAULT_PERM_UNCHANGED = BLK_PERM_ALL & ~DEFAULT_PERM_PASSTHROUGH, }; +typedef enum BdrvChildRole { + /* Child stores data */ + BDRV_CHILD_DATA = (1 << 0), + + /* Child stores metadata */ + BDRV_CHILD_METADATA = (1 << 1), + + /* Filtered child */ + BDRV_CHILD_FILTERED = (1 << 2), + + /* Child to COW from (backing child) */ + 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), + + /* Useful combination of flags */ + BDRV_CHILD_IMAGE = BDRV_CHILD_DATA + | BDRV_CHILD_METADATA + | BDRV_CHILD_PRIMARY, +} BdrvChildRole; + char *bdrv_perm_names(uint64_t perm); uint64_t bdrv_qapi_perm_to_blk_perm(BlockPermission qapi_perm);
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 | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+)