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

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1.1: Type: text/plain, Size: 9972 bytes --]

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 <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.

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.

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

>>>>> 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. :-?

>  * 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”

> 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.

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.”.

> 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.

> 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, 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*).

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).


*
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.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]