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