Re: [Qemu-devel] [PATCH for-2.7 00/15] block: Lock images when opening

["Denis V. Lunev" <den@openvz.org>]

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