Re: [Qemu-devel] [PATCH 00/10] qcow2: Implement image locking

[Fam Zheng <famz@redhat.com>]

On Tue, 12/22 17:46, Kevin Wolf wrote:
> Enough innocent images have died because users called 'qemu-img snapshot' while
> the VM was still running. Educating the users doesn't seem to be a working
> strategy, so this series adds locking to qcow2 that refuses to access the image
> read-write from two processes.
> 
> Eric, this will require a libvirt update to deal with qemu crashes which leave
> locked images behind. The simplest thinkable way would be to unconditionally
> override the lock in libvirt whenever the option is present. In that case,
> libvirt VMs would be protected against concurrent non-libvirt accesses, but not
> the other way round. If you want more than that, libvirt would have to check
> somehow if it was its own VM that used the image and left the lock behind. I
> imagine that can't be too hard either.

The motivation is great, but I'm not sure I like the side-effect that an
unclean shutdown will require a "forced" open, because it makes using qcow2 in
development cumbersome, and like you said, management/user also needs to handle
this explicitly. This is a bit of a personal preference, but it's strong enough
that I want to speak up.

As an alternative, can we introduce .bdrv_flock() in protocol drivers, with
similar semantics to flock(2) or lockf(3)? That way all formats can benefit,
and a program crash will automatically drop the lock.

Fam

> 
> Also note that this kind of depends on Max's bdrv_close_all() series, but only
> in order to pass test case 142. This is not a bug in this series, but a
> preexisting one (bs->file can be closed before bs), and it becomes apparent
> when qemu fails to unlock an image due to this bug. Max's series fixes this.
> 
> Kevin Wolf (10):
>   qcow2: Write feature table only for v3 images
>   qcow2: Write full header on image creation
>   block: Assert no write requests under BDRV_O_INCOMING
>   block: Fix error path in bdrv_invalidate_cache()
>   block: Inactivate BDS when migration completes
>   qemu-img: Prepare for locked images
>   qcow2: Implement .bdrv_inactivate
>   qcow2: Fix BDRV_O_INCOMING handling in qcow2_invalidate_cache()
>   qcow2: Make image inaccessible after failed qcow2_invalidate_cache()
>   qcow2: Add image locking
> 
>  block.c                    |  36 +++++++++
>  block/io.c                 |   2 +
>  block/qcow2.c              | 190 +++++++++++++++++++++++++++++++++++----------
>  block/qcow2.h              |   7 +-
>  docs/specs/qcow2.txt       |   7 +-
>  include/block/block.h      |   2 +
>  include/block/block_int.h  |   1 +
>  include/qapi/error.h       |   1 +
>  migration/migration.c      |   7 ++
>  qapi/common.json           |   3 +-
>  qemu-img-cmds.hx           |  10 ++-
>  qemu-img.c                 |  96 +++++++++++++++++++----
>  qemu-img.texi              |  20 ++++-
>  qmp.c                      |  12 +++
>  tests/qemu-iotests/026     |   2 +-
>  tests/qemu-iotests/026.out |  60 ++++++++++++--
>  tests/qemu-iotests/031.out |  23 +++---
>  tests/qemu-iotests/036     |   2 +
>  tests/qemu-iotests/036.out |   7 +-
>  tests/qemu-iotests/039     |   4 +-
>  tests/qemu-iotests/061     |   2 +
>  tests/qemu-iotests/061.out |  43 +++++-----
>  tests/qemu-iotests/071     |   7 ++
>  tests/qemu-iotests/071.out |   4 +
>  tests/qemu-iotests/089     |   2 +-
>  tests/qemu-iotests/089.out |   2 -
>  tests/qemu-iotests/091     |   2 +-
>  tests/qemu-iotests/098     |   2 +-
>  28 files changed, 445 insertions(+), 111 deletions(-)
> 
> -- 
> 1.8.3.1
> 
>