From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:34326) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aqZg9-00031u-VT for qemu-devel@nongnu.org; Thu, 14 Apr 2016 01:18:59 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aqZg8-0003Hs-OY for qemu-devel@nongnu.org; Thu, 14 Apr 2016 01:18:57 -0400 References: <1460538604-12132-1-git-send-email-famz@redhat.com> <570E1CFE.3070907@openvz.org> <20160414023627.GD31467@ad.usersys.redhat.com> From: "Denis V. Lunev" Message-ID: <570F24C8.9020501@openvz.org> Date: Thu, 14 Apr 2016 08:04:08 +0300 MIME-Version: 1.0 In-Reply-To: <20160414023627.GD31467@ad.usersys.redhat.com> Content-Type: text/plain; charset="windows-1252"; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH for-2.7 00/15] block: Lock images when opening List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Fam Zheng Cc: qemu-devel@nongnu.org, Kevin Wolf , Max Reitz , Jeff Cody , Markus Armbruster , Eric Blake , John Snow , qemu-block@nongnu.org, berrange@redhat.com, pbonzini@redhat.com On 04/14/2016 05:36 AM, Fam Zheng wrote: > On Wed, 04/13 13:18, Denis V. Lunev wrote: >> On 04/13/2016 12:09 PM, Fam Zheng wrote: >>> Too many troubles have been caused by two processes writing to the same image >>> unexpectedly. This series introduces automatical image locking into QEMU to >>> avoid such tragedy. With this, the user won't be able to open the image from >>> two processes (e.g. using qemu-img when the image is attached to the guest). >>> >>> Underneath is the fcntl syscall that locks the local file, similar to what is >>> already used in libvirt virtlockd. Also because of that, we cannot directly >>> apply fcntl lock on the image file itself, instead we open and lock >>> "/var/tmp/.qemu-$sha1.lock", where $sha1 is derived from the image's full path >>> as in realpath(3). This mechanism should be equally useful for the single host >>> case, and it doesn't conflict with virtlockd when managed by libvirt. >>> >>> The alternative file locking API on Linux, flock(2), cannot protect host NFS >>> mount points, so it's not used. >>> >>> Gluster locking is also implemented wrapping glfs_posix_lock in patch 6. It's >>> only lightly tested. >>> >>> All other drivers that don't implement .bdrv_lockf are always permissive and >>> does no checking. >>> >>> In the future, the intention is that image format drivers that introduce >>> locking mechanisms could also fit into this API. >>> >>> The first 6 patches define the internal and external interfaces, and implement >>> the image locking. >>> >>> Patch 7 adds an option in qemu-io to allow disabling the lock, for testing >>> purpose. >>> >>> Patches 8 - 14 fixes the potential failures of test cases where multiple >>> processes may open the image concurrently. >>> >>> Finally the default behavior is switched to on in patch 15. >>> >>> Fam Zheng (15): >>> block: Add BDRV_O_NO_LOCK >>> qapi: Add lock-image in blockdev-add options >>> blockdev: Add and parse "lock-image" option for block devices >>> block: Introduce image file locking interface >>> raw-posix: Implement .bdrv_lockf >>> gluster: Implement .bdrv_lockf >>> qemu-io: Add "-L" option for BDRV_O_NO_LOCK >>> qemu-iotests: 140: Disable image lock for qemu-io access >>> qemu-iotests: 046: Move version detection out from verify_io >>> qemu-iotests: Fix lock-image for shared disk in test case 091 >>> qemu-iotests: Disable image lock when checking test image >>> qemu-iotests: 051: Disable image lock in the command line >>> ahci-test: Specify "lock-image=off" in CLI >>> ide-test: Specify "lock-image=off" in command lines >>> block: Turn on image locking by default >>> >>> block.c | 25 +++++++++++ >>> block/gluster.c | 34 +++++++++++++++ >>> block/raw-posix.c | 97 +++++++++++++++++++++++++++++++++++++++++++ >>> blockdev.c | 8 ++++ >>> include/block/block.h | 9 ++++ >>> include/block/block_int.h | 5 +++ >>> qapi/block-core.json | 6 ++- >>> qemu-io.c | 22 +++++++++- >>> tests/ahci-test.c | 16 +++++-- >>> tests/ide-test.c | 5 ++- >>> tests/qemu-iotests/030 | 2 +- >>> tests/qemu-iotests/046 | 22 +++++----- >>> tests/qemu-iotests/051 | 2 +- >>> tests/qemu-iotests/051.out | 10 ++--- >>> tests/qemu-iotests/051.pc.out | 10 ++--- >>> tests/qemu-iotests/091 | 4 +- >>> tests/qemu-iotests/140 | 2 +- >>> 17 files changed, 245 insertions(+), 34 deletions(-) >>> >> First of all, I like the approach as we have discussed :) Then, >> in general, I support Daniel with the point that >> the locking should be done on the image file >> directly. >> >> Also, it looks like this will break migration with the shared storage. >> For me it seems that we will have lock the image from both ends > There should already be state handling logic between source and destinition > ends, I'll take a look and see if we can add lock/unlock calls there to keep it > working. > > Fam unfortunately no. If the lock will be on the image file, we will have get it on the target node on QEMU start and re-acquire it in bdrv_invalidate_cache. From my POW you should not get the lock if BDRV_O_INACTIVE is set. Den