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

[Kevin Wolf <kwolf@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 4625 bytes --]

Am 05.05.2020 um 15:20 hat Max Reitz geschrieben:
> On 05.05.20 14:54, Kevin Wolf wrote:
> > When you're the author, the meaning of everything is clear to you. :-)
> > 
> > In case of doubt, I would be more explicit so that the comment gives a
> > clear guideline for which role to use in which scenario.
> 
> OK, so you mean just noting everywhere explicitly how many children can
> get a specific flag, and not just in some cases?  That is, make a note
> for DATA and METADATA that they can be given to an arbitrary number of
> children, and COW only to at most one.

Sounds good to me.

> >>> 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.)
> > 
> > Well, yes, this is exceptional. I would ignore that property for
> > assigning roles because when it comes to play, roles don't matter any
> > more because the whole process is gone. So...
> > 
> >> 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.
> > 
> > ...I don't think this case is relevant. If blkverify returns something,
> > both children have the same data.
> 
> Another argument is that right now, bs->file points to x-raw, and
> .is_filter is set.  So x-raw is already treated as the filtered child.

I admit defeat. :-)

> >> 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”.
> > 
> > Maybe the missing part for me is what FILTERED is even used for. I
> > assume it's for skipping over filters in certain functions in the
> > generic block layer?
> 
> Yes.
> 
> > In this case, maybe the right answer is that...
> > 
> >>> 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.
> > 
> > ...blkverify isn't one either because performing an operation on only
> > one child usually won't be correct.
> 
> Good point.  It would work if filters are just skipped for functions
> that read/query stuff, which I think is the case.  I don’t think we ever
> skip filters when it comes to modifying data.
> 
> In any case, I wouldn’t lose too much sleep over blkverify whatever we
> do.  It’s a driver used purely for debugging purposes.

Yeah, I'm not really worried about blkverify per se. It just seems like
an interesting corner case to discuss to make sure that the design of
the role system is right and doesn't miss anything important.

> > Maybe the more relevant question would be if a FILTERED child must be
> > the only child to avoid the problems we're discussing for blkverify. But
> > I think I already answered that question for myself with "no", so
> > probably not much use asking it.
> 
> blkverify is just a bit weird, and I personally don’t mind just treating
> it as something “special”, considering it’s just a debugging aid.
> 
> Regardless of blkverify, I don’t think FILTERED children must be the
> only children, though, because I can well imagine filter drivers having
> metadata children on the side, e.g. config data or bitmaps (not just
> dirty bitmaps, but also e.g. what to cache for a hypothetical cache driver).

The example of a caching driver that uses a child node for the cached
data (probably on a fast, but small disk) was what made me answer the
question with "no".

But as you write, having a pure metadata child could make sense, too.

Kevin

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]