From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:57151) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aBZsh-00006r-4a for qemu-devel@nongnu.org; Tue, 22 Dec 2015 22:14:28 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aBZsg-0002FR-2f for qemu-devel@nongnu.org; Tue, 22 Dec 2015 22:14:27 -0500 Date: Wed, 23 Dec 2015 11:14:12 +0800 From: Fam Zheng Message-ID: <20151223031412.GC14423@ad.usersys.redhat.com> References: <1450802786-20893-1-git-send-email-kwolf@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1450802786-20893-1-git-send-email-kwolf@redhat.com> Subject: Re: [Qemu-devel] [PATCH 00/10] qcow2: Implement image locking List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf Cc: qemu-devel@nongnu.org, qemu-block@nongnu.org, mreitz@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 > >