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

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1.1: Type: text/plain, Size: 5382 bytes --]

On 21.01.20 13:53, Vladimir Sementsov-Ogievskiy wrote:
> 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: qemu-stable@nongnu.org # v4.2.0
>>>>>>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>>>>>>>> ---
>>>>>>>>>      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)

I mean, you can just drop the second sentence, and then it gets even
shorter...

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]