Re: [Qemu-block] [PATCH v6 04/42] block: Add child access functions
Am 10.09.2019 um 14:59 hat Max Reitz geschrieben: > On 10.09.19 14:48, Kevin Wolf wrote: > > Am 10.09.2019 um 13:36 hat Max Reitz geschrieben: > >> On 10.09.19 12:47, Kevin Wolf wrote: > >>> Am 10.09.2019 um 11:14 hat Max Reitz geschrieben: > 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. > >> > >> It feels difficult to do in this series because this is a whole new can > >> of worms. > >> > >> In patch 35, I actually replace the mirror use case by > >> is_filtered_child(). So it looks to me as if that should not be done, > >> because I should instead fix bdrv_recurse_is_first_non_filter() (and > >> rename it), because quorum does allow replacing its children by mirror, > >> even if it does not act as a filter for them. > >> > >> OTOH, there are other users of bdrv_is_first_non_filter(). Those are > >> qmp_block_resize() and external_snapshot_prepare(), who throw an error > >> if that returns false. > >> > >> I think that’s just wrong. First of all, I don’t even know why we have > >> that restriction anymore (I can imagine why it used to make sense before > >> the permission system). qmp_block_resize() should always work as long > >> as it can get BLK_PERM_RESIZE; and I don’t know why the parents of some > >> node would care if you take a snapshot of their child. > > > > Hm, doesn't it make sense in a way for qmp_block_resize() at least? It > > means that you can't resize just a filter, but you need to resize the > > image that actually provides the data for the filter. > > Filters generally implement .bdrv_truncate() by passing it through, so > it should be fine. Good point. Then checking bdrv_is_first_non_filter() probably just forbids the only command that would actually work correctly (resizing the top-level filter). Kevin signature.asc Description: PGP signature
Re: [Qemu-block] [PATCH v6 04/42] block: Add child access functions
On 10.09.19 14:48, Kevin Wolf wrote: > Am 10.09.2019 um 13:36 hat Max Reitz geschrieben: >> On 10.09.19 12:47, Kevin Wolf wrote: >>> Am 10.09.2019 um 11:14 hat Max Reitz geschrieben: 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. >> >> It feels difficult to do in this series because this is a whole new can >> of worms. >> >> In patch 35, I actually replace the mirror use case by >> is_filtered_child(). So it looks to me as if that should not be done, >> because I should instead fix bdrv_recurse_is_first_non_filter() (and >> rename it), because quorum does allow replacing its children by mirror, >> even if it does not act as a filter for them. >> >> OTOH, there are other users of bdrv_is_first_non_filter(). Those are >> qmp_block_resize() and external_snapshot_prepare(), who throw an error >> if that returns false. >> >> I think that’s just wrong. First of all, I don’t even know why we have >> that restriction anymore (I can imagine why it used to make sense before >> the permission system). qmp_block_resize() should always work as long >> as it can get BLK_PERM_RESIZE; and I don’t know why the parents of some >> node would care if you take a snapshot of their child. > > Hm, doesn't it make sense in a way for qmp_block_resize() at least? It > means that you can't resize just a filter, but you need to resize the > image that actually provides the data for the filter. Filters generally implement .bdrv_truncate() by passing it through, so it should be fine. > Of course, there is no reason for it to be the _first_ non-filter as > long as BLK_PERM_RESIZE is shared, but just some non-filter node. > > Two more random observations: > > * quorum uses bdrv_filter_default_perms(), which allows BLK_PERM_RESIZE. > I think this is wrong and quorum should make sure that all children are > always the same size because otherwise it can't tell what its own size > is. (Or vote on size...? :-/) Probably not a problem in practice as > long as we check bdrv_is_first_non_filter(). (“Quorum is broken” seems to be a recurring observation.) I agree, it shouldn’t share that permission. > * child_file and child_backing don't implement .resize. So if you resize > a non-top-level image, parents (in particular filters) don't get their > size adjusted. This is probably a bug, too, but one that isn't > prevented by bdrv_is_first_non_filter() and should be visible today. Hm. :-/ The good news is that I can try to fix this independently of this series. [...] >> We have come to two results, as far as I can see: >> >> First, naming COW backing nodes “COW filtered children” clashes with our >> existing use of ”filter”. There is no point in forcing the ”filter” >> label on everything. We can just keep calling (R/W) filters filters and >> COW backing children COW children. The names are succinct enough. >> >> In some cases, we don’t care whether something is a COW or filtered >> child, in such a case a caller can be bothered to use the slightly >> longer bdrv_cow_or_filtered_child(). > > Aye. > >> Second, most of the time we want a filter node to have a clear and >> unique path to go down. This is the important property of filters: That >> you can skip them and go to the node that actually has the data. >> >> Quorum breaks this by having multiple children, and nobody knows which >> of them has the data we will see on the next read operation. >> >> All “filters” who could have multiple children would have this problem. >> Hence a filter must always have a single unique data child. I think. > > I agree, and this is the condition that I mentioned somewhere above, but > failed to actually find guaranteed somewhere. We should probably make > this explicit. > > Of course, quorum and similar things intend all their children to > provide the same data, but the whole point of the driver is that this is > not always guaranteed, so they aren't actually filters. OK, great, I’ll get cracking then. Max signature.asc Description: OpenPGP digital signature
Re: [Qemu-block] [PATCH v6 04/42] block: Add child access functions
Am 10.09.2019 um 13:36 hat Max Reitz geschrieben: > On 10.09.19 12:47, Kevin Wolf wrote: > > Am 10.09.2019 um 11:14 hat Max Reitz geschrieben: > >> 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. > > It feels difficult to do in this series because this is a whole new can > of worms. > > In patch 35, I actually replace the mirror use case by > is_filtered_child(). So it looks to me as if that should not be done, > because I should instead fix bdrv_recurse_is_first_non_filter() (and > rename it), because quorum does allow replacing its children by mirror, > even if it does not act as a filter for them. > > OTOH, there are other users of bdrv_is_first_non_filter(). Those are > qmp_block_resize() and external_snapshot_prepare(), who throw an error > if that returns false. > > I think that’s just wrong. First of all, I don’t even know why we have > that restriction anymore (I can imagine why it used to make sense before > the permission system). qmp_block_resize() should always work as long > as it can get BLK_PERM_RESIZE; and I don’t know why the parents of some > node would care if you take a snapshot of their child. Hm, doesn't it make sense in a way for qmp_block_resize() at least? It means that you can't resize just a filter, but you need to resize the image that actually provides the data for the filter. Of course, there is no reason for it to be the _first_ non-filter as long as BLK_PERM_RESIZE is shared, but just some non-filter node. Two more random observations: * quorum uses bdrv_filter_default_perms(), which allows BLK_PERM_RESIZE. I think this is wrong and quorum should make sure that all children are always the same size because otherwise it can't tell what its own size is. (Or vote on size...? :-/) Probably not a problem in practice as long as we check bdrv_is_first_non_filter(). * child_file and child_backing don't implement .resize. So if you resize a non-top-level image, parents (in particular filters) don't get their size adjusted. This is probably a bug, too, but one that isn't prevented by bdrv_is_first_non_filter() and should be visible today. > >>> 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. > > No. Some qcow2 nodes match the criteria. But not all, which makes the > qcow2 driver not a filter driver. > > > 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? > > “Where in the code”? Do you want to add a comment to every BlockDriver > structure on why it does or doesn’t set .is_filter? Never mind, I just didn't understand that .is_filter is the thing that defines a R/W filter. In fact, I didn't really understand what .is_filter was supposed to mean at all because I was so confused. For some reason I was sure it had to mean any kind of filter, but that assumption just didn't match up with its use at all. > > 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. > > Why don’t you say so from the start then? Yes, I wrote "nodes", thought "child nodes" and should have said "children" because edges are not nodes. My bad, sorry. > (Sorry, but honestly about 30 % of this discussion to me feels like > you’re playing games with me. Please don’t take this the wrong way, I > mean it very neutrally. It’s just that I feel like I’m explaining > things to you that you very much know, but you just want me to say them. > And that feels unproductive and sometimes indeed frustrating.) No, certainly not. If my mails seemed confusing or pointless, it just shows how thoroughly confused I was. > One thing is that this wouldn’t make the quorum case any easier because > it actually doesn’t know for which children it acts as a filter and for > which it doesn’t. > > > But by now I think mos
Re: [Qemu-block] [PATCH v6 04/42] block: Add child access functions
On 10.09.19 12:47, Kevin Wolf wrote: > 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 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... >>> >>> 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. It feels difficult to do in this series because this is a whole new can of worms. In patch 35, I actually replace the mirror use case by is_filtered_child(). So it looks to me as if that should not be done, because I should instead fix bdrv_recurse_is_first_non_filter() (and rename it), because quorum does allow replacing its children by mirror, even if it does not act as a filter for them. OTOH, there are other users of bdrv_is_first_non_filter(). Those are qmp_block_resize() and external_snapshot_prepare(), who throw an error if that returns false. I think that’s just wrong. First of all, I don’t even know why we have that restriction anymore (I can imagine why it used to make sense before the permission system). qmp_block_resize() should always work as long as it can get BLK_PERM_RESIZE; and I don’t know why the parents of some node would care if you take a snapshot of their 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). > > 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 descr
Re: [Qemu-block] [PATCH v6 04/42] block: Add child access functions
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 > >> 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... > > > > 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,
Re: [Qemu-block] [PATCH v6 04/42] block: Add child access functions
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 >> 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... > > 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. :
Re: [Qemu-block] [PATCH v6 04/42] block: Add child access functions
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 > 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... 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. > >>> 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 * accesses and whether data is copied onto this BDS through COR. 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. 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.) > > 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. 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. I already think this is a bit inconsistent, because why should a driver that declares itself a non-filter be considered to filter children? 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 th
Re: [Qemu-block] [PATCH v6 04/42] block: Add child access functions
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 meanin
Re: [Qemu-block] [PATCH v6 04/42] block: Add child access functions
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? > > 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. Specficially, according to your definition, qcow2 filters both the backing file (COW filter) and the external data file (R/W filter). > 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". > 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. > 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. 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. Kevin signature.asc Description: PGP signature
Re: [Qemu-block] [PATCH v6 04/42] block: Add child access functions
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 signature.asc Description: OpenPGP digital signature
Re: [Qemu-block] [PATCH v6 04/42] block: Add child access functions
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), 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? 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. > +/* > + * 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). Kevin
[Qemu-block] [PATCH v6 04/42] block: Add child access functions
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
---
include/block/block_int.h | 57 --
block.c | 99 +++
2 files changed, 153 insertions(+), 3 deletions(-)
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 60d9261f8e..6c60abc4c3 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -90,9 +90,11 @@ struct BlockDriver {
int instance_size;
/* set to true if the BlockDriver is a block filter. Block filters pass
- * certain callbacks that refer to data (see block.c) to their bs->file if
- * the driver doesn't implement them. Drivers that do not wish to forward
- * must implement them and return -ENOTSUP.
+ * certain callbacks that refer to data (see block.c) to their bs->file
+ * or bs->backing (whichever one exists) if the driver doesn't implement
+ * them. Drivers that do not wish to forward must implement them and return
+ * -ENOTSUP.
+ * Note that filters are not allowed to modify data.
*/
bool is_filter;
/* for snapshots block filter like Quorum can implement the
@@ -562,6 +564,13 @@ struct BlockDriver {
* If this pointer is NULL, the array is considered empty.
* "filename" and "driver" are always considered strong. */
const char *const *strong_runtime_opts;
+
+/**
+ * Return the data storage child, if there is exactly one. If
+ * this function is not implemented, the block layer will assume
+ * bs->file to be this child.
+ */
+BdrvChild *(*bdrv_storage_child)(BlockDriverState *bs);
};
typedef struct BlockLimits {
@@ -1276,4 +1285,46 @@ int coroutine_fn bdrv_co_copy_range_to(BdrvChild *src,
uint64_t src_offset,
int refresh_total_sectors(BlockDriverState *bs, int64_t hint);
+BdrvChild *bdrv_filtered_cow_child(BlockDriverState *bs);
+BdrvChild *bdrv_filtered_rw_child(BlockDriverState *bs);
+BdrvChild *bdrv_filtered_child(BlockDriverState *bs);
+BdrvChild *bdrv_metadata_child(BlockDriverState *bs);
+BdrvChild *bdrv_storage_child(BlockDriverState *bs);
+BdrvChild *bdrv_primary_child(BlockDriverState *bs);
+
+static inline BlockDriverState *child_bs(BdrvChild *child)
+{
+return child ? child->bs : NULL;
+}
+
+static inline BlockDriverState *bdrv_filtered_cow_bs(BlockDriverState *bs)
+{
+return child_bs(bdrv_filtered_cow_child(bs));
+}
+
+static inline BlockDriverState *bdrv_filtered_rw_bs(BlockDriverState *bs)
+{
+return child_bs(bdrv_filtered_rw_child(bs));
+}
+
+static inline BlockDriverState *bdrv_filtered_bs(BlockDriverState *bs)
+{
+return child_bs(bdrv_filtered_child(bs));
+}
+
+static inline BlockDriverState *bdrv_metadata_bs(BlockDriverState *bs)
+{
+return child_bs(bdrv_metadata_child(bs));
+}
+
+static inline BlockDriverState *bdrv_storage_bs(BlockDriverState *bs)
+{
+return child_bs(bdrv_storage_child(bs));
+}
+
+static inline BlockDriverState *bdrv_primary_bs(BlockDriverState *bs)
+{
+return child_bs(bdrv_primary_child(bs));
+}
+
#endif /* BLOCK_INT_H */
diff --git a/block.c b/block.c
index aae3417dd5..f6c5f8c3eb 100644
--- a/block.c
+++ b/block.c
@@ -6556,3 +6556,102 @@ bool bdrv_can_store_new_dirty_bitmap(BlockDriverState
*bs, const char *name,
return drv->bdrv_can_store_new_dirty_bitmap(bs, name, granularity, errp);
}
+
+/*
+ * 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.
+ */
+BdrvChild *bdrv_filtered_cow_child(BlockDriverState *bs)
+{
+if (!bs || !bs->drv) {
+return NULL;
+}
+
+if (bs->drv->is_filter) {
+return NULL;
+}
+
+return bs->backing;
+}
+
+/*
+ * If @bs acts as a pass-through filter for one of its children,
+ * return that child. "Pass-through" means that write operations to
+ * @bs are forwarded to that child instead of triggering COW.
+ */
+BdrvChild *bdrv_filtered_rw_child(BlockDriverState *bs)
+{
+if (!bs || !bs->drv) {
+return NULL;
+}
+
+if (!bs->drv->is_filter) {
+return NULL;
+}
+
+/* Only one of @backing or @file may be used */
+assert(!(bs->backing && bs->file));
+
+return bs->backing ?: bs->file;
+}
+
+/*
+ * Return any filtered child, independently of how it reacts to write
