[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 2/2] iotests: add test for backup-top failure on permission a
From: |
Vladimir Sementsov-Ogievskiy |
Subject: |
Re: [PATCH 2/2] iotests: add test for backup-top failure on permission activation |
Date: |
Tue, 21 Jan 2020 13:55:59 +0000 |
21.01.2020 16:51, Max Reitz wrote:
> On 21.01.20 14:48, 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: 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.
>>>
>>>> So, trying to backup the configuration with writing side parents of
>>>> source sub-tree nodes should fail. Let's test it.
>>
>> But than, we need somehow link part about appending backup-top and so-on...
>>
>> 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.
>> backup-top filter is appended above source node, to achieve this thing, so
>> all parents of source node are handled.
>> A configuration with side parents of source sub-tree with write permission
>> is unsupported (we'd have append several backup-top filter like nodes to
>> handle such parents).
>> The test create an example of such configuration and checks that backup
>> fails.
>
> Sounds good!
>
> (Except maybe s/that backup fails/that a backup is then not allowed/?
> “backup fails” might also mean that the job just produces garbage.)
OK for me. May be "backup is then not allowed (blockdev-backup command should
fail)".
Should I resend? I think it's better drop "auto" mark and not create extra
dependency on other series.
--
Best regards,
Vladimir
- Re: [PATCH 2/2] iotests: add test for backup-top failure on permission activation, (continued)
- Re: [PATCH 2/2] iotests: add test for backup-top failure on permission activation, Vladimir Sementsov-Ogievskiy, 2020/01/20
- Re: [PATCH 2/2] iotests: add test for backup-top failure on permission activation, Max Reitz, 2020/01/21
- Re: [PATCH 2/2] iotests: add test for backup-top failure on permission activation, Vladimir Sementsov-Ogievskiy, 2020/01/21
- Re: [PATCH 2/2] iotests: add test for backup-top failure on permission activation, Max Reitz, 2020/01/21
- Re: [PATCH 2/2] iotests: add test for backup-top failure on permission activation, Vladimir Sementsov-Ogievskiy, 2020/01/21
- Re: [PATCH 2/2] iotests: add test for backup-top failure on permission activation, Max Reitz, 2020/01/21
- Re: [PATCH 2/2] iotests: add test for backup-top failure on permission activation, Vladimir Sementsov-Ogievskiy, 2020/01/21
- Re: [PATCH 2/2] iotests: add test for backup-top failure on permission activation, Max Reitz, 2020/01/21
- Re: [PATCH 2/2] iotests: add test for backup-top failure on permission activation, Vladimir Sementsov-Ogievskiy, 2020/01/21
- Re: [PATCH 2/2] iotests: add test for backup-top failure on permission activation, Max Reitz, 2020/01/21
- Re: [PATCH 2/2] iotests: add test for backup-top failure on permission activation,
Vladimir Sementsov-Ogievskiy <=
- Re: [PATCH 2/2] iotests: add test for backup-top failure on permission activation, Max Reitz, 2020/01/21
[PATCH 1/2] block/backup-top: fix failure path, Vladimir Sementsov-Ogievskiy, 2020/01/16