On 25.09.19 15:39, Vladimir Sementsov-Ogievskiy wrote:
> 20.09.2019 18:27, Max Reitz wrote:
>> Signed-off-by: Max Reitz <mre...@redhat.com>
>> ---
>> block/quorum.c | 62 ++++++++++++++++++++++++++++++++++++++++++++++++++
>> 1 file changed, 62 insertions(+)
>>
>> diff --git a/block/quorum.c b/block/quorum.c
>> index 207054a64e..81b57dbae2 100644
>> --- a/block/quorum.c
>> +++ b/block/quorum.c
>> @@ -825,6 +825,67 @@ static bool
>> quorum_recurse_is_first_non_filter(BlockDriverState *bs,
>> return false;
>> }
>>
>> +static bool quorum_recurse_can_replace(BlockDriverState *bs,
>> + BlockDriverState *to_replace)
>> +{
>> + BDRVQuorumState *s = bs->opaque;
>> + int i;
>> +
>> + for (i = 0; i < s->num_children; i++) {
>> + /*
>> + * We have no idea whether our children show the same data as
>> + * this node (@bs). It is actually highly likely that
>> + * @to_replace does not, because replacing a broken child is
>> + * one of the main use cases here.
>> + *
>> + * We do know that the new BDS will match @bs, so replacing
>> + * any of our children by it will be safe. It cannot change
>> + * the data this quorum node presents to its parents.
>> + *
>> + * However, replacing @to_replace by @bs in any of our
>
> I'm a bit misleaded: by @bs, or by node equal to @bs (accordingly to
> bdrv_recurse_can_replace spec)?
>
>> + * children's chains may change visible data somewhere in
>> + * there. We therefore cannot recurse down those chains with
>> + * bdrv_recurse_can_replace().
>> + * (More formally, bdrv_recurse_can_replace() requires that
>> + * @to_replace will be replaced by something matching the @bs
>> + * passed to it. We cannot guarantee that.)
>
> Aha, and you answered already :) OK.
>
>> + *
>> + * Thus, we can only check whether any of our immediate
>> + * children matches @to_replace.
>> + *
>> + * (In the future, we might add a function to recurse down a
>> + * chain that checks that nothing there cares about a change
>> + * in data from the respective child in question. For
>> + * example, most filters do not care when their child's data
>> + * suddenly changes, as long as their parents do not care.)
>> + */
>> + if (s->children[i].child->bs == to_replace) {
>
> Hmm, still, what is wrong if we just put
> bdrv_recurse_can_replace(s->children[i].child->bs, to_replace) into this if
> condition?
> (or may be more correct to call it after taking permissions)It’s wrong because: bs=quorum -> child=filter -> to_replace <- other_parent Quorum now takes WRITE on the child and unshares CONSISTENT_READ. That doesn’t concern the @other_parent at all, however, it only concerns parents immediately on its child (the filter node). bdrv_recurse_can_replace() will happily acknowledge you replacing @to_replace by something equal to @filter, because that shouldn’t concern @other_parent. The problem of course is that we don’t want to replace @to_replace by something equal to @filter, but by something equal to @quorum. And that may be different from @to_replace, so @other_parent should have a say in it. What we’d need to do is have a function that recurses down and checks whether there are any parents that care; which is exactly what I’ve described in the last paragraph of the long comment above. For now, I think it’s fine to just ignore such cases and forbid them. Max
signature.asc
Description: OpenPGP digital signature
