Am 09.09.2019 um 16:04 hat Max Reitz geschrieben:
> On 09.09.19 11:36, Kevin Wolf wrote:
> > Am 09.09.2019 um 09:56 hat Max Reitz geschrieben:
> >> On 04.09.19 18:16, Kevin Wolf wrote:
> >>> Am 09.08.2019 um 18:13 hat Max Reitz geschrieben:
> >>>> There are BDS children that the general block layer code can access,
> >>>> namely bs->file and bs->backing.  Since the introduction of filters and
> >>>> external data files, their meaning is not quite clear.  bs->backing can
> >>>> be a COW source, or it can be an R/W-filtered child; bs->file can be an
> >>>> R/W-filtered child, it can be data and metadata storage, or it can be
> >>>> just metadata storage.
> >>>>
> >>>> This overloading really is not helpful.  This patch adds function that
> >>>> retrieve the correct child for each exact purpose.  Later patches in
> >>>> this series will make use of them.  Doing so will allow us to handle
> >>>> filter nodes and external data files in a meaningful way.
> >>>>
> >>>> Signed-off-by: Max Reitz <mreitz@redhat.com>
> >>>> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> >>>
> >>> Each time I look at this patch, I'm confused by the function names.
> >>> Maybe I should just ask what the idea there was, or more specifically:
> >>> What does the "filtered" in "filtered child" really mean?
> >>>
> >>> Apparently any child of a filter node is "filtered" (which makes sense),
> >>
> >> It isn’t, filters can have non-filter children.  For example, backup-top
> >> could have the source as a filtered child and the target as a non-filter
> >> child.
> > 
> > Hm, okay, makes sense. I had a definition in mind that says that filter
> > nodes only have a single child node. Is it that a filter may have only a
> > single _filtered_ child node?
> 
> Well, there’s Quorum...

Ah, nice, quorum sets is_filter = true even though it neither fulfulls
the conditions for it before this series, nor the changed conditions
after this series.

So either quorum lies and isn't actually a filter driver, or our
definition in the documentation of is_filter is wrong.

> >>> but also bs->backing of a qcow2 image, while bs->file of qcow2 isn't.
> >>> raw doesn't have any "filtered" child. What's the system behind this?
> >>
> >> “filtered” means: If the parent node returns data from this child, it
> >> won’t modify it, neither its content nor its position.  COW and R/W
> >> filters differ in how they handle writes; R/W filters pass them through
> >> to the filtered child, COW filters copy them off to some other child
> >> node (and then the filtered child’s data will no longer be visible at
> >> that location).
> > 
> > But there is no reason why a node couldn't fulfill this condition for
> > more than one child node. bdrv_filtered_child() isn't well-defined then.
> > Technically, the description "Return any filtered child" is correct
> > because "any" can be interpreted as "an arbitrary", but obviously that
> > makes the function useless.
> 
> Which is why it currently returns NULL for Quorum.

Which is about the only possible choice that breaks the contract...

 * Return any filtered child, independently of how it reacts to write
 * accesses and whether data is copied onto this BDS through COR.

Maybe the documentation of bdrv_filtered_child() needs to be rephrased?

Going back to qcow2, it's really not much different as it has multiple
(two) filtered children, too. So if quorum returns NULL to mean "no
unambiguous result", why does it return bs->backing instead of NULL for
a qcow2 node?

(Yes, I know, because it's useful. But I'm trying to get some basic
consistency into these interfaces.)

> > Specficially, according to your definition, qcow2 filters both the
> > backing file (COW filter) and the external data file (R/W filter).
> 
> Not wrong.  But the same question as for raw arises: Is there any use to
> declaring qcow2 an R/W filter driver just because it fits the definition?

Wait, where is there even a place where this could be declared?

The once thing I see that a driver even can declare is drv->is_filter,
which is about the whole driver and not about nodes. It is false for
qcow2.

Then you made some criteria above that tell us whether a specific child
of a node is a filtered child or not. As it happens, qcow2 (which is not
a filter driver) can have two children that match the criteria for being
filtered children.

I already think this is a bit inconsistent, because why should a driver
that declares itself a non-filter be considered to filter children?
Okay, you say a broader definition of a filtered child is useful because
you can then include all BdrvChild links in a backing/filter chain. Fair
enough, it's not intuitive, but use a broader definition then.

But the point where you say that even though two of the children
are filtered children under your broader definition, for the purpose of
the API only one of them should be considered because the other one
isn't that useful, that's really one inconsistency too much for me. You
can't use a broad definition and then arbitrarily restrict the
definition again so that it matches the special case you're currently
interested in.

Either use a narrow definition, or use a broad one. But use only one and
use it consistently.

> >> The main reason behind the common “filtered” name is for the generic
> >> functions that work on both COW and true filter (R/W filters) chains.
> >> We need such functionality sometimes.  I personally felt like the
> >> concept of true (R/W) filters and COW children was similar enough to
> >> share a common name base.
> > 
> > We generally call this concept a "backing chain".
> 
> I suppose that’s an exclusive “we”?  Because I use ”backing chain” to
> refer to COW chains exclusively.
> 
> Such a chain may or may not include filters, but they are not really
> load-bearing nodes of the chain.  As such, I generally want to skip them
> when looking at a backing chain (hence e.g. bdrv_backing_chain_next()).
> 
> From what I can tell nobody has ever formalized any terms regarding COW
> backing chains or R/W filter chains.  This series is an attempt.

Well, as you can see, this attempt feels confusing to me.

I agree with your naming of bdrv_backing_chain_next(), it's clear enough
what path it will follow down the graph. I just disagree that "filter
chain" is a good term for something that prefers backing file links when
it has a choice.

> >> qcow2 has a COW child.  As such, it acts as a COW filter in the sense of
> >> the function names.
> >>
> >> raw has neither a COW child nor acts as an R/W filter.  As such, it has
> >> no filtered child.  My opinion on this hasn’t changed.
> >>
> >> (To reiterate, in practice I see no way anyone would ever use raw as an
> >> R/W filter.
> >> Either you use it without offset/size, in which case you simply use it
> >> in lieu of a format node, so you precisely don’t want it to act as a
> >> filter when it comes to allocation information and so on (even though it
> >> can be classified a filter here).
> >> Or you use it as kind of a filter with offset/size, but then it no
> >> longer is a filter.
> > 
> > Agreed with offset, but with only size, it matches your definition of a
> > filter.
> 
> So?
> 
> Should we treat it as a filter when @offset is 0 but otherwise not?
> That totally wouldn’t be confusing to users.

No, I'm just applying your definitions to see if the contradictions
between them and your explanations are of any importance. *shrug*

> >> Filters are defined by “Every filter must fulfill these conditions: ...”
> >> – not by “Everything that fulfills these conditions is a filter”.
> >> Marking a driver as a filter has consequences, and I don’t see why we
> >> would want those consequences for raw.)
> >>
> >>> It looks like bdrv_filtered_child() is the right function to iterate
> >>> along a backing file chain, but I just still fail to connect that and
> >>> the name of the function in a meaningful way.
> >>
> >> It‘s the right function to iterate along a filter chain.  This includes
> >> COW backing children and R/W filtered children.
> > 
> > qcow2 doesn't fulfill the conditions for begin a filter driver. Two of
> > its possible children fulfill the conditions for being a filtered child.
> > You can pick either approach, talking about a "filter chain" just
> > doesn't make sense there. Either the chain is broken by a non-filter
> > driver like qcow2, or it must become a filter tree.
> 
> I have no idea what your point is.  There is no point in making it a
> filter tree at this point, just as we never had a backing tree.
> 
> And the good example is Quorum.  qcow2 is a bad example because there is
> no benefit in marking it an R/W filter for its external data file,
> exactly like is the case for raw.

My point is not about changing the logic in the code, but about using
names that actually describe accurately what the logic does.

And as I said above, neither is "not useful" a convincing argument for
ignoring filtered children (as I think we're trying to build something
rather generic, not something that works only for what we consider
useful today) nor do I see how qcow2 could be marked or not marked an
R/W filter (as mentioned above).

> > What we're really interested in is iterating the backing chain even
> > across filter nodes, so your implementation achieves the right result.
> > It just feels completely arbitrary, counterintuitive and confusing to
> > call this a (or actually "the") "filter chain" and to pretend that the
> > name tells anyone what it really is.
> 
> So exactly the same as “bs->backing” or “backing chain” for me.
> 
> You disagreeing with me on these terms to me shows that there is a need
> to formalize.  This is precisely what I want to do in this series.
> 
> The fact that we don’t use the term “filter chain” so far is the reason
> why I introduce it.  Because it comes as a clean slate.  “backing chain”
> already means something to me, and it means something different.

Well, if "backing chain" is too narrow, "filter chain" is both too
unspecific and inconsistent with the various definitions of "filter" and
"filtered" we're using, and we can't think of anything more concise, we
might have to use names that just mention both.

bdrv_cow_child() // don't call COW a filter, because .is_filter = false
bdrv_filter_child() // your R/W filter, only for .is_filter = true nodes
bdrv_filter_or_cow_child()

Or something like that. This would bring some more consistency into the
way we use the words filter/filtered at least.

Kevin