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

[Max Reitz <mreitz@redhat.com>]

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

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

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

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

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

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

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

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

Max


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