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. > 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). 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. 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. 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. >> +/* >> + * Return the child that @bs acts as an overlay for, and from which data may be >> + * copied in COW or COR operations. Usually this is the backing file. >> + */ > > Or NULL, if no such child exists. > > It's relatively obvious here, but for some of the functions further down > it would be really good to describe in which cases NULL is expected (or > that NULL is even a possible return value). I’ll look into it. Max