Re: [Qemu-block] [PATCH v5 24/27] iotests: Disable image locking in 085

[Max Reitz]

On 03.06.2016 09:18, Fam Zheng wrote:
> On Wed, 05/25 15:52, Max Reitz wrote:
>> On 17.05.2016 09:35, Fam Zheng wrote:
>>> The cases is about live snapshot features. Disable image locking because
>>> otherwise a few tests are going to fail because we reuse the same images
>>> at blockdev-add.
>>>
>>> Signed-off-by: Fam Zheng <address@hidden>
>>> ---
>>>  tests/qemu-iotests/085 | 3 ++-
>>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/tests/qemu-iotests/085 b/tests/qemu-iotests/085
>>> index aa77eca..48f6684 100755
>>> --- a/tests/qemu-iotests/085
>>> +++ b/tests/qemu-iotests/085
>>> @@ -102,6 +102,7 @@ function add_snapshot_image()
>>>      cmd="{ 'execute': 'blockdev-add', 'arguments':
>>>             { 'options':
>>>               { 'driver': 'qcow2', 'node-name': 'snap_${1}', ${extra_params}
>>> +               'lock-mode': 'off',
>>>                 'file':
>>>                 { 'driver': 'file', 'filename': '${snapshot_file}',
>>>                   'node-name': 'file_${1}' } } } }"
>>> @@ -130,7 +131,7 @@ echo === Running QEMU ===
>>>  echo
>>>  
>>>  qemu_comm_method="qmp"
>>> -_launch_qemu -drive file="${TEST_IMG}.1",if=virtio -drive 
>>> file="${TEST_IMG}.2",if=virtio
>>> +_launch_qemu -drive file="${TEST_IMG}.1",if=virtio,lock-mode=off -drive 
>>> file="${TEST_IMG}.2",if=virtio,lock-mode=off
>>>  h=$QEMU_HANDLE
>>>  
>>>  echo
>>>
>>
>> So as far as I understand it, add_snapshot_image() is supposed to add
>> images from the backing chain to the running VM. The top image is never
>> used by add_snapshot_image(), thus the lock-mode=off in the QEMU command
>> line seems superfluous.
> 
> But down the backing chain is 10-snapshot-v0.qcow2, created in
> create_single_snapshot (or create_group_snapshot?). Without lock-mode=off in
> the command line, the shared lock cannot work.

My reasoning was wrong, but still it works for me if the lock-mode is
omitted everywhere but the blockdev-add operation specifies
"'read-only': true".

OK, so let's try again.

<what_the_read_only_does_and_where_to_put_it>
<explanation_what_happens>

The files specified on the command line will later be snapshotted; the
snapshot operation will reopen them read-only, therefore, they will have
a shared lock then.

Only after those snapshots have occurred will we blockdev-add new files.
If we do not force them to be opened read-only, the "Invalid command -
snapshot node has a backing image" test will fail. This is because of
the following (I believe):

This test is the only one that adds a new snapshot image with its
backing chain (of which some images have already been opened by qemu).
Of course, the whole backing chain is opened read-only, therefore this
is only an issue if some image of that chain has been opened read/write
by qemu before.

Now, blockdev-snapshot-sync will keep the flags of the old BDS for the
new snapshot BDS. Therefore, if we made the original drive read-only,
all snapshots created with that command would remain read-only and no
locking issue would occur.

However, we are sometimes using blockdev-snapshot, and that command of
course will not change the flags the existing overlay node has been
created with. If we are therefore not making these nodes read-only, the
images opened for virtio0 and virtio1 will not be read-only at the time
of the "Invalid command - snapshot node has a backing image" test.

</explanation_what_happens>

If we specify "'read-only': true" in the blockdev-add command, they will
be read-only, on the other hand. Then, the locking will not interfere.

For the sake of clarity, I would then however propose specifying
read-only on the command line, too, so it is clear that we are trying to
keep the drives read-only the entire time.

</what_the_read_only_does_and_where_to_put_it>


But this is a bit complicated, and rather complicated to understand,
too. The issue we really have is that that one test tries to open a
snapshot with its backing chain while parts of the backing chain are
still opened by qemu. Therefore, alternatively, we can just switch off
locking in that single case. This can be accomplished by replacing

extra_params=""

by

extra_params="'lock-mode': 'off', "

in add_snapshot_image (second line in that function's body). That works
for me.

Also, I guess I made us waste too much time on this already... If this
doesn't work for you, I'm fine with the original patch as it was.

Max

signature.asc
Description: OpenPGP digital signature