From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:55171) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1b8qI6-0000r7-Q5 for qemu-devel@nongnu.org; Fri, 03 Jun 2016 10:41:40 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1b8qI5-0006XB-Ct for qemu-devel@nongnu.org; Fri, 03 Jun 2016 10:41:38 -0400 References: <1463470536-8981-1-git-send-email-famz@redhat.com> <1463470536-8981-25-git-send-email-famz@redhat.com> <1e6c8138-8b83-f3f9-dad9-d9bd2ad1e13b@redhat.com> <20160603071854.GM29298@ad.usersys.redhat.com> From: Max Reitz Message-ID: <46961ea9-7269-720f-ff04-aab7265d329e@redhat.com> Date: Fri, 3 Jun 2016 16:41:25 +0200 MIME-Version: 1.0 In-Reply-To: <20160603071854.GM29298@ad.usersys.redhat.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="Sv5SxQWu3cEAm9QKmE4HBJvbxLwxtawOK" Subject: Re: [Qemu-devel] [PATCH v5 24/27] iotests: Disable image locking in 085 List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Fam Zheng Cc: qemu-devel@nongnu.org, Kevin Wolf , Jeff Cody , Markus Armbruster , Eric Blake , John Snow , qemu-block@nongnu.org, berrange@redhat.com, pbonzini@redhat.com, den@openvz.org, stefanha@redhat.com This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --Sv5SxQWu3cEAm9QKmE4HBJvbxLwxtawOK From: Max Reitz To: Fam Zheng Cc: qemu-devel@nongnu.org, Kevin Wolf , Jeff Cody , Markus Armbruster , Eric Blake , John Snow , qemu-block@nongnu.org, berrange@redhat.com, pbonzini@redhat.com, den@openvz.org, stefanha@redhat.com Message-ID: <46961ea9-7269-720f-ff04-aab7265d329e@redhat.com> Subject: Re: [PATCH v5 24/27] iotests: Disable image locking in 085 References: <1463470536-8981-1-git-send-email-famz@redhat.com> <1463470536-8981-25-git-send-email-famz@redhat.com> <1e6c8138-8b83-f3f9-dad9-d9bd2ad1e13b@redhat.com> <20160603071854.GM29298@ad.usersys.redhat.com> In-Reply-To: <20160603071854.GM29298@ad.usersys.redhat.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: quoted-printable 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 beca= use >>> otherwise a few tests are going to fail because we reuse the same ima= ges >>> at blockdev-add. >>> >>> Signed-off-by: Fam Zheng >>> --- >>> 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=3D"{ '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 =3D=3D=3D Running QEMU =3D=3D=3D >>> echo >>> =20 >>> qemu_comm_method=3D"qmp" >>> -_launch_qemu -drive file=3D"${TEST_IMG}.1",if=3Dvirtio -drive file=3D= "${TEST_IMG}.2",if=3Dvirtio >>> +_launch_qemu -drive file=3D"${TEST_IMG}.1",if=3Dvirtio,lock-mode=3Do= ff -drive file=3D"${TEST_IMG}.2",if=3Dvirtio,lock-mode=3Doff >>> h=3D$QEMU_HANDLE >>> =20 >>> 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 neve= r >> used by add_snapshot_image(), thus the lock-mode=3Doff in the QEMU com= mand >> line seems superfluous. >=20 > But down the backing chain is 10-snapshot-v0.qcow2, created in > create_single_snapshot (or create_group_snapshot?). Without lock-mode=3D= 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. 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. 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. 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=3D"" by extra_params=3D"'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 --Sv5SxQWu3cEAm9QKmE4HBJvbxLwxtawOK Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQEcBAEBCAAGBQJXUZcWAAoJEDuxQgLoOKytF8YIAIZOovA/nMCvivVtGVwD3Rj6 dxf+BRbqWaKnP9rcj3Sy7ZWCIB0urM0Kwi1nCCJRocmc7MqVSzTKZ1lZaYF0jAeJ EQOSKcUYO/HvwncOmAlUt898VrFynQGpufLBLDSUZyds3N2JTgeWJ7Ju5KOMnpOv ixmL9khTyTbUA+9b/K8Dej5UJSHY/WjDl28Tvh958zMFhnnPeM3xe3RgCEhvuRWg zknAgaLHi0Grx6lxmuaJ59xftOSRsWQ3Igy3RjhCBxyEuYrPOTE+Z4Enu49NySMC IZsL//6FRgZM0Op7uJb7qdzrkoDsT3Iz8FCS9atageNGUJr5HpdH+ilWvDdHx0g= =ZAbN -----END PGP SIGNATURE----- --Sv5SxQWu3cEAm9QKmE4HBJvbxLwxtawOK--