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 > >
