Re: [PATCH 2/2] iotests: add test for backup-top failure on permission activation

[Vladimir Sementsov-Ogievskiy]

21.01.2020 15:39, Max Reitz wrote:
> On 21.01.20 11:40, Vladimir Sementsov-Ogievskiy wrote:
>> 21.01.2020 12:41, Max Reitz wrote:
>>> On 21.01.20 10:23, Vladimir Sementsov-Ogievskiy wrote:
>>>> 21.01.2020 12:14, Max Reitz wrote:
>>>>> On 20.01.20 18:20, Vladimir Sementsov-Ogievskiy wrote:
>>>>>> 20.01.2020 20:04, Max Reitz wrote:
>>>>>>> On 16.01.20 16:54, Vladimir Sementsov-Ogievskiy wrote:
>>>>>>>> This test checks that bug is really fixed by previous commit.
>>>>>>>>
>>>>>>>> Cc: address@hidden # v4.2.0
>>>>>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
>>>>>>>> ---
>>>>>>>>      tests/qemu-iotests/283     | 75 
>>>>>>>> ++++++++++++++++++++++++++++++++++++++
>>>>>>>>      tests/qemu-iotests/283.out |  8 ++++
>>>>>>>>      tests/qemu-iotests/group   |  1 +
>>>>>>>>      3 files changed, 84 insertions(+)
>>>>>>>>      create mode 100644 tests/qemu-iotests/283
>>>>>>>>      create mode 100644 tests/qemu-iotests/283.out
>>>>>>>
>>>>>>> The test looks good to me, I just have a comment nit and a note on the
>>>>>>> fact that this should probably be queued only after Thomas’s “Enable
>>>>>>> more iotests during "make check-block"” series.
>>>>>>>
>>>>>>>> diff --git a/tests/qemu-iotests/283 b/tests/qemu-iotests/283
>>>>>>>> new file mode 100644
>>>>>>>> index 0000000000..f0f216d109
>>>>>>>> --- /dev/null
>>>>>>>> +++ b/tests/qemu-iotests/283
>>>>>>>> @@ -0,0 +1,75 @@
>>>>>>>> +#!/usr/bin/env python
>>>>>>>> +#
>>>>>>>> +# Test for backup-top filter permission activation failure
>>>>>>>> +#
>>>>>>>> +# Copyright (c) 2019 Virtuozzo International GmbH.
>>>>>>>> +#
>>>>>>>> +# This program is free software; you can redistribute it and/or modify
>>>>>>>> +# it under the terms of the GNU General Public License as published by
>>>>>>>> +# the Free Software Foundation; either version 2 of the License, or
>>>>>>>> +# (at your option) any later version.
>>>>>>>> +#
>>>>>>>> +# This program is distributed in the hope that it will be useful,
>>>>>>>> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
>>>>>>>> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>>>>>>>> +# GNU General Public License for more details.
>>>>>>>> +#
>>>>>>>> +# You should have received a copy of the GNU General Public License
>>>>>>>> +# along with this program.  If not, see 
>>>>>>>> <http://www.gnu.org/licenses/>.
>>>>>>>> +#
>>>>>>>> +
>>>>>>>> +import iotests
>>>>>>>> +
>>>>>>>> +# The test is unrelated to formats, restrict it to qcow2 to avoid 
>>>>>>>> extra runs
>>>>>>>> +iotests.verify_image_format(supported_fmts=['qcow2'])
>>>>>>>> +
>>>>>>>> +size = 1024 * 1024
>>>>>>>> +
>>>>>>>> +"""
>>>>>>>> +On activation, backup-top is going to unshare write permission on its
>>>>>>>> +source child. It will be impossible for the following configuration:
>>>>>>>
>>>>>>> “The following configuration will become impossible”?
>>>>>>
>>>>>> Hmm, no, the configuration is possible. But "it", i.e. "unshare write 
>>>>>> permission",
>>>>>> is impossible with such configuration..
>>>>>
>>>>> But backup_top always unshares the write permission on the source.
>>>>
>>>> Yes, and I just try to say, that this action will fail. And the test 
>>>> checks that it
>>>> fails (and it crashes with current master instead of fail).
>>>
>>> OK.  So what I was trying to say is that the comment currently only
>>> states that this will fail.  I’d prefer it to also reassure me that it’s
>>> correct that this fails (because all writes on the backup source must go
>>> through backup_top), and that this is exactly what we want to test here.
>>>
>>> On first reading, I was wondering why exactly this comment would tell me
>>> all these things, because I didn’t know what the test wants to test in
>>> the first place.
>>>
>>> Max
>>
>> Hmm, something like:
>>
>> Backup wants to copy a point-in-time state of the source node. So, it 
>> catches all writes
>> to the source node by appending backup-top filter above it. So we handle all 
>> changes which
>> comes from source node parents. To prevent appearing of new writing parents 
>> during the
>> progress, backup-top unshares write permission on its source child. This has 
>> additional
>> implication: as this "unsharing" is propagated by default by backing/file 
>> children,
>> backup-top conflicts with any side parents of source sub-tree with write 
>> permission.
>> And this is in good relation with the general idea: with such parents we 
>> can't guarantee
>> point-in-time backup.
> 
> Works for me (thanks :-)), but a shorter “When performing a backup, all
> writes on the source subtree must go through the backup-top filter so it
> can copy all data to the target before it is changed.  Therefore,
> backup-top cannot allow other nodes to change data on its source child.”
> would work for me just as well.

Hmm, I don't like this "Therefore". For me the last statement
"cannot allow" doesn't looks like a consequence of the first
"all writes must go through", it more like rephrasing (still
not completely equal)... So, I'll keep my wording)

> 
>> So, trying to backup the configuration with writing side parents of
>> source sub-tree nodes should fail. Let's test it.
> Max
> 


-- 
Best regards,
Vladimir