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 >>>> Reviewed-by: Vladimir Sementsov-Ogievskiy >>> >>> 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