From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:34559) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aqHsy-0006iY-W8 for qemu-devel@nongnu.org; Wed, 13 Apr 2016 06:19:02 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aqHsx-0002XE-PI for qemu-devel@nongnu.org; Wed, 13 Apr 2016 06:19:00 -0400 References: <1460538604-12132-1-git-send-email-famz@redhat.com> From: "Denis V. Lunev" Message-ID: <570E1CFE.3070907@openvz.org> Date: Wed, 13 Apr 2016 13:18:38 +0300 MIME-Version: 1.0 In-Reply-To: <1460538604-12132-1-git-send-email-famz@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 , qemu-devel@nongnu.org Cc: Kevin Wolf , Max Reitz , Jeff Cody , Markus Armbruster , Eric Blake , John Snow , qemu-block@nongnu.org, berrange@redhat.com, pbonzini@redhat.com 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 Den