[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-block] [PATCH v6 04/42] block: Add child access functions

From: Kevin Wolf
Subject: Re: [Qemu-block] [PATCH v6 04/42] block: Add child access functions
Date: Tue, 10 Sep 2019 12:47:48 +0200
User-agent: Mutt/1.12.1 (2019-06-15)

Am 10.09.2019 um 11:14 hat Max Reitz geschrieben:
> On 09.09.19 18:13, Kevin Wolf wrote:
> > 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 <address@hidden>
> >>>>>> Reviewed-by: Vladimir Sementsov-Ogievskiy <address@hidden>
> >>>>>
> >>>>> 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.
> You could say it lies because in FIFO mode it clearly isn’t a filter for
> all of its children.
> There is a reason for lying, though, which is
> bdrv_recurse_is_first_non_filter(), which is necessary to use the whole
> to_replace mirror stuff.

Hm, actually, now that you mention bdrv_recurse_is_first_non_filter(),
quorum was the first driver to declare itself a filter, so strictly
speaking, if there is an inconsistency, it's the other uses that are
abusing the field...

> (You mirror from a quorum with a failed child and then replace the
> failed child.  mirror needs to ensure that there are only R/W filters
> between the child and the mirror source so that replacing it will not
> suddenly change any visible data.  Which is actually a lie for quorum,
> because the child is clearly broken and thus precisely doesn’t show the
> same data...)
> Maybe we should stop declaring Quorum a filter and then rename the
> bdrv_recurse_is_first_non_filter() to, I don’t know,
> bdrv_recurse_can_be_replaced_by_mirror()?

Why not.

> >>>>> 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
> I don’t know if you’re serious about this proposition, because I don’t
> know whether that could be useful in any way. :-?

Huh? This is just quoting the contract from your code?

> >  * accesses and whether data is copied onto this BDS through COR.
> I meant the contract as “Return the single filtered child there is, or NULL”

Then that should probably be spelt out in the contract. Probably even
explicitly "NULL if there is either no filtered child or multiple
filtered children".

> > 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.
> Well, it doesn’t.  It isn’t an R/W filter.

What do I have to look at to see whether something is an R/W filter or
not? qcow2 matches your criteria for an R/W filter. You say that it's
not useful, so it's not an R/W filter anyway. But where in the code
could I get this information?

This just doesn't make sense to me. If a driver matches the criteria for
an R/W filter, then it should be one. If qcow2 should not be considered
a R/W filter, then the criteria must be changed so that it isn't.

> Maybe what we actually need to rephrase is the definition of .is_filter.
>  (Namely something along the lines of “Fulfills these guarantees (same
> data, etc. pp.), *and* should be skipped for allocation information
> queries etc.”.

Hm - does this imply that .is_filter == this is a R/W filter? Because
this was never spelt out, neither in code comments nor in commit

If we called R/W filters just "filters" (which makes it obvious how it
relates to .is_filter) and COW nodes something that doesn't include the
word "filter", things might become a lot clearer.

> > 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.)
> Not precisely because it’s useful, but because qcow2 does not have
> .is_filter set.  :-)
> (And it doesn’t have it set because that wouldn’t be useful.)
> >>> 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.
> That’s correct.  But that’s not a fundamental problem, of course, we
> could make it a per-BDS attribute if that made sense.

I was thinking per-child, actually, because you declare one BdrvChild
filtered and another not filtered.

But by now I think most of the confusion is really just a result of COW
being considered a filter in some respects (mainly just the names of the
child access functions), but not in others (like .is_filter).

> > 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.
> But just arguing that I’m incapable of giving a good definition won’t
> bring us along.
> > I already think this is a bit inconsistent, because why should a driver
> > that declares itself a non-filter be considered to filter children?
> .is_filter is for R/W filters.  COW filters have .supports_backing for that.

Okay, so you confirm what I concluded above.

> > 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.
> I think the problem appears because you restrict the process to a single
> step where there’s actually two.
> Drivers can be either
> (1) R/W filters (e.g. throttle)
> (2) COW filters (e.g. qcow2)
> (3) None of the above (e.g. vhdx, curl)
> This choice is made on the driver level, not on the node level (for good
> reason, see below*).

What prevents a driver from being
(4) COW filter and R/W filter (e.g. qcow2 if it were useful)?

I mean, conceptually, not in the implementation.

> And then we derive the node’s filtered children from what the driver is.
>  If it’s an R/W filter, bdrv_filtered_child() will return the R/W
> filtered child.  If it’s a COW filter, bdrv_filtered_child() will return
> the potentially existing COW backing child (or NULL, if there is no
> backing child).

I guess it boils down to me just not being able to get behind the
concept that COW is some sort of filter (especially when other things
like an external data file aren't).

> *
> What is clear to me is that it isn’t useful to treat nodes of a specific
> driver sometimes as R/W filter nodes and sometimes not.  R/W filter
> nodes are handled differently from other nodes, and it would be
> confusing if a certain driver sometimes behaves this and sometimes that
> way.  (For example, if you put a raw node on top of a qcow2 node,
> sometimes it would stop the backing chain, sometimes it wouldn’t.  That
> makes no sense to me.)
> OTOH, for COW filters, we do exactly that.  Sometimes they have a
> backing file, sometimes they don’t.  That’s completely fine because
> their overall behavior doesn’t change.
> That makes me agree that there is indeed too much of a difference
> between R/W filters and COW filters to lump them together under the
> “filter” label.
> [...]
> > My point is not about changing the logic in the code, but about using
> > names that actually describe accurately what the logic does.
> Again, naming things is hard.
> [...]
> >> 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.
> I’ll see how that looks overall, but why not.  Sounds good to me.

Good. Or, well, good enough at least. ;-)

bdrv_filter_or_cow_child() is not a pretty name, but as long as we can't
think of anything that accurately covers both in a single word, it will
do the job...


Attachment: signature.asc
Description: PGP signature

reply via email to

[Prev in Thread] Current Thread [Next in Thread]